Skip to content

[stable21] Only return display name as editable when the user backend allows it #26815

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
blizzz merged 3 commits into from
Apr 29, 2021
Merged

[stable21] Only return display name as editable when the user backend allows it #26815

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a1d746f
Only return display name as editable when the user backend allows it
nickvergessen Apr 8, 2021
5975a68
Also check implementsAction method
nickvergessen Apr 26, 2021
ca8b2a6
FIx stable21 difference
nickvergessen Apr 29, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions apps/provisioning_api/appinfo/routes.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@
['root' => '/cloud', 'name' => 'Users#getUser', 'url' => '/users/{userId}', 'verb' => 'GET'],
['root' => '/cloud', 'name' => 'Users#getCurrentUser', 'url' => '/user', 'verb' => 'GET'],
['root' => '/cloud', 'name' => 'Users#getEditableFields', 'url' => '/user/fields', 'verb' => 'GET'],
['root' => '/cloud', 'name' => 'Users#getEditableFields', 'url' => '/user/fields/{userId}', 'verb' => 'GET'],
['root' => '/cloud', 'name' => 'Users#editUser', 'url' => '/users/{userId}', 'verb' => 'PUT'],
['root' => '/cloud', 'name' => 'Users#wipeUserDevices', 'url' => '/users/{userId}/wipe', 'verb' => 'POST'],
['root' => '/cloud', 'name' => 'Users#deleteUser', 'url' => '/users/{userId}', 'verb' => 'DELETE'],
Expand Down
46 changes: 40 additions & 6 deletions apps/provisioning_api/lib/Controller/UsersController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@
use OC\Authentication\Token\RemoteWipe;
use OC\HintException;
use OC\KnownUser\KnownUserService;
use OC\User\Backend;
use OCA\Settings\Mailer\NewUserMailHelper;
use OCP\Accounts\IAccountManager;
use OCP\App\IAppManager;
Expand All @@ -70,6 +71,7 @@
use OCP\Security\ISecureRandom;
use OCP\Security\Events\GenerateSecurePasswordEvent;
use OCP\EventDispatcher\IEventDispatcher;
use OCP\User\Backend\ISetDisplayNameBackend;

class UsersController extends AUserData {

Expand Down Expand Up @@ -525,13 +527,39 @@ public function getCurrentUser(): DataResponse {
/**
* @NoAdminRequired
* @NoSubAdminRequired
*
* @return DataResponse
* @throws OCSException
*/
public function getEditableFields(): DataResponse {
public function getEditableFields(?string $userId = null): DataResponse {
$currentLoggedInUser = $this->userSession->getUser();
if (!$currentLoggedInUser instanceof IUser) {
throw new OCSException('', \OCP\API::RESPOND_NOT_FOUND);
}

$permittedFields = [];

if ($userId !== $currentLoggedInUser->getUID()) {
$targetUser = $this->userManager->get($userId);
if (!$targetUser instanceof IUser) {
throw new OCSException('', \OCP\API::RESPOND_NOT_FOUND);
}

$subAdminManager = $this->groupManager->getSubAdmin();
if (!$this->groupManager->isAdmin($currentLoggedInUser->getUID())
&& !$subAdminManager->isUserAccessible($currentLoggedInUser, $targetUser)) {
throw new OCSException('', \OCP\API::RESPOND_NOT_FOUND);
}
} else {
$targetUser = $currentLoggedInUser;
}

// Editing self (display, email)
if ($this->config->getSystemValue('allow_user_to_change_display_name', true) !== false) {
$permittedFields[] = IAccountManager::PROPERTY_DISPLAYNAME;
if ($targetUser->getBackend() instanceof ISetDisplayNameBackend
|| $targetUser->getBackend()->implementsActions(Backend::SET_DISPLAYNAME)) {
$permittedFields[] = IAccountManager::PROPERTY_DISPLAYNAME;
}
$permittedFields[] = IAccountManager::PROPERTY_EMAIL;
}

Expand Down Expand Up @@ -568,8 +596,11 @@ public function editUser(string $userId, string $key, string $value): DataRespon
if ($targetUser->getUID() === $currentLoggedInUser->getUID()) {
// Editing self (display, email)
if ($this->config->getSystemValue('allow_user_to_change_display_name', true) !== false) {
$permittedFields[] = 'display';
$permittedFields[] = IAccountManager::PROPERTY_DISPLAYNAME;
if ($targetUser->getBackend() instanceof ISetDisplayNameBackend
|| $targetUser->getBackend()->implementsActions(Backend::SET_DISPLAYNAME)) {
$permittedFields[] = 'display';
$permittedFields[] = IAccountManager::PROPERTY_DISPLAYNAME;
}
$permittedFields[] = IAccountManager::PROPERTY_EMAIL;
}

Expand Down Expand Up @@ -608,8 +639,11 @@ public function editUser(string $userId, string $key, string $value): DataRespon
if ($this->groupManager->isAdmin($currentLoggedInUser->getUID())
|| $subAdminManager->isUserAccessible($currentLoggedInUser, $targetUser)) {
// They have permissions over the user
$permittedFields[] = 'display';
$permittedFields[] = IAccountManager::PROPERTY_DISPLAYNAME;
if ($targetUser->getBackend() instanceof ISetDisplayNameBackend
|| $targetUser->getBackend()->implementsActions(Backend::SET_DISPLAYNAME)) {
$permittedFields[] = 'display';
$permittedFields[] = IAccountManager::PROPERTY_DISPLAYNAME;
}
$permittedFields[] = IAccountManager::PROPERTY_EMAIL;
$permittedFields[] = 'password';
$permittedFields[] = 'language';
Expand Down
118 changes: 113 additions & 5 deletions apps/provisioning_api/tests/Controller/UsersControllerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@
use OCP\Mail\IEMailTemplate;
use OCP\Security\Events\GenerateSecurePasswordEvent;
use OCP\Security\ISecureRandom;
use OCP\User\Backend\ISetDisplayNameBackend;
use OCP\UserInterface;
use PHPUnit\Framework\MockObject\MockObject;
use Test\TestCase;
Expand Down Expand Up @@ -1443,6 +1444,10 @@ public function testEditUserRegularUserSelfEditChangeDisplayName() {
->method('get')
->with('UserToEdit')
->willReturn($targetUser);
$targetUser
->expects($this->once())
->method('getBackend')
->willReturn($this->createMock(ISetDisplayNameBackend::class));
$targetUser
->expects($this->once())
->method('setDisplayName')
Expand Down Expand Up @@ -1484,6 +1489,12 @@ public function testEditUserRegularUserSelfEditChangeEmailValid() {
->method('getUID')
->willReturn('UID');

$backend = $this->createMock(UserInterface::class);
$targetUser
->expects($this->any())
->method('getBackend')
->willReturn($backend);

$this->assertEquals([], $this->api->editUser('UserToEdit', 'email', '[email protected]')->getData());
}

Expand Down Expand Up @@ -1517,6 +1528,12 @@ public function testEditUserRegularUserSelfEditChangeEmailInvalid() {
->method('getUID')
->willReturn('UID');

$backend = $this->createMock(UserInterface::class);
$targetUser
->expects($this->any())
->method('getBackend')
->willReturn($backend);

$this->api->editUser('UserToEdit', 'email', 'demo.org');
}

Expand Down Expand Up @@ -1550,6 +1567,12 @@ public function testEditUserRegularUserSelfEditChangeProperty($propertyName, $ol
->with('UserToEdit')
->willReturn($loggedInUser);

$backend = $this->createMock(UserInterface::class);
$loggedInUser
->expects($this->any())
->method('getBackend')
->willReturn($backend);

$this->accountManager->expects($this->once())
->method('getUser')
->with($loggedInUser)
Expand Down Expand Up @@ -1594,6 +1617,12 @@ public function testEditUserRegularUserSelfEditChangePropertyScope($propertyName
->with('UserToEdit')
->willReturn($loggedInUser);

$backend = $this->createMock(UserInterface::class);
$loggedInUser
->expects($this->any())
->method('getBackend')
->willReturn($backend);

$this->accountManager->expects($this->once())
->method('getUser')
->with($loggedInUser)
Expand Down Expand Up @@ -1638,6 +1667,12 @@ public function testEditUserRegularUserSelfEditChangePassword() {
->method('getUID')
->willReturn('UID');

$backend = $this->createMock(UserInterface::class);
$targetUser
->expects($this->any())
->method('getBackend')
->willReturn($backend);

$this->assertEquals([], $this->api->editUser('UserToEdit', 'password', 'NewPassword')->getData());
}

Expand Down Expand Up @@ -1671,6 +1706,12 @@ public function testEditUserRegularUserSelfEditChangeQuota() {
->method('getUID')
->willReturn('UID');

$backend = $this->createMock(UserInterface::class);
$targetUser
->expects($this->any())
->method('getBackend')
->willReturn($backend);

$this->api->editUser('UserToEdit', 'quota', 'NewQuota');
}

Expand Down Expand Up @@ -1703,6 +1744,12 @@ public function testEditUserAdminUserSelfEditChangeValidQuota() {
->method('getUID')
->willReturn('UID');

$backend = $this->createMock(UserInterface::class);
$targetUser
->expects($this->any())
->method('getBackend')
->willReturn($backend);

$this->assertEquals([], $this->api->editUser('UserToEdit', 'quota', '3042824')->getData());
}

Expand Down Expand Up @@ -1738,6 +1785,12 @@ public function testEditUserAdminUserSelfEditChangeInvalidQuota() {
->method('getUID')
->willReturn('UID');

$backend = $this->createMock(UserInterface::class);
$targetUser
->expects($this->any())
->method('getBackend')
->willReturn($backend);

$this->api->editUser('UserToEdit', 'quota', 'ABC');
}

Expand Down Expand Up @@ -1777,6 +1830,12 @@ public function testEditUserAdminUserEditChangeValidQuota() {
->method('getUID')
->willReturn('UID');

$backend = $this->createMock(UserInterface::class);
$targetUser
->expects($this->any())
->method('getBackend')
->willReturn($backend);

$this->assertEquals([], $this->api->editUser('UserToEdit', 'quota', '3042824')->getData());
}

Expand Down Expand Up @@ -1819,6 +1878,12 @@ public function testEditUserSelfEditChangeLanguage() {
->method('getUID')
->willReturn('UserToEdit');

$backend = $this->createMock(UserInterface::class);
$targetUser
->expects($this->any())
->method('getBackend')
->willReturn($backend);

$this->assertEquals([], $this->api->editUser('UserToEdit', 'language', 'de')->getData());
}

Expand Down Expand Up @@ -1869,6 +1934,12 @@ public function testEditUserSelfEditChangeLanguageButForced($forced) {
->method('getUID')
->willReturn('UserToEdit');

$backend = $this->createMock(UserInterface::class);
$targetUser
->expects($this->any())
->method('getBackend')
->willReturn($backend);

$this->assertEquals([], $this->api->editUser('UserToEdit', 'language', 'de')->getData());
}

Expand Down Expand Up @@ -1910,6 +1981,12 @@ public function testEditUserAdminEditChangeLanguage() {
->method('getUID')
->willReturn('UserToEdit');

$backend = $this->createMock(UserInterface::class);
$targetUser
->expects($this->any())
->method('getBackend')
->willReturn($backend);

$this->assertEquals([], $this->api->editUser('UserToEdit', 'language', 'de')->getData());
}

Expand Down Expand Up @@ -1956,6 +2033,12 @@ public function testEditUserAdminEditChangeLanguageInvalidLanguage() {
->method('getUID')
->willReturn('UserToEdit');

$backend = $this->createMock(UserInterface::class);
$targetUser
->expects($this->any())
->method('getBackend')
->willReturn($backend);

$this->assertEquals([], $this->api->editUser('UserToEdit', 'language', 'ru')->getData());
}

Expand Down Expand Up @@ -1995,6 +2078,12 @@ public function testEditUserSubadminUserAccessible() {
->method('getUID')
->willReturn('UID');

$backend = $this->createMock(UserInterface::class);
$targetUser
->expects($this->any())
->method('getBackend')
->willReturn($backend);

$this->assertEquals([], $this->api->editUser('UserToEdit', 'quota', '3042824')->getData());
}

Expand Down Expand Up @@ -3716,39 +3805,58 @@ public function testResendWelcomeMessageFailed() {

public function dataGetEditableFields() {
return [
[false, [
[false, ISetDisplayNameBackend::class, [
IAccountManager::PROPERTY_PHONE,
IAccountManager::PROPERTY_ADDRESS,
IAccountManager::PROPERTY_WEBSITE,
IAccountManager::PROPERTY_TWITTER,
]],
[ true, [
[true, ISetDisplayNameBackend::class, [
IAccountManager::PROPERTY_DISPLAYNAME,
IAccountManager::PROPERTY_EMAIL,
IAccountManager::PROPERTY_PHONE,
IAccountManager::PROPERTY_ADDRESS,
IAccountManager::PROPERTY_WEBSITE,
IAccountManager::PROPERTY_TWITTER,
]]
]],
[true, UserInterface::class, [
IAccountManager::PROPERTY_EMAIL,
IAccountManager::PROPERTY_PHONE,
IAccountManager::PROPERTY_ADDRESS,
IAccountManager::PROPERTY_WEBSITE,
IAccountManager::PROPERTY_TWITTER,
]],
];
}

/**
* @dataProvider dataGetEditableFields
*
* @param bool $allowedToChangeDisplayName
* @param string $userBackend
* @param array $expected
*/
public function testGetEditableFields(bool $allowedToChangeDisplayName, array $expected) {
public function testGetEditableFields(bool $allowedToChangeDisplayName, string $userBackend, array $expected) {
$this->config
->method('getSystemValue')
->with(
$this->equalTo('allow_user_to_change_display_name'),
$this->anything()
)->willReturn($allowedToChangeDisplayName);

$user = $this->createMock(IUser::class);
$this->userSession->method('getUser')
->willReturn($user);

$backend = $this->createMock($userBackend);

$user->method('getUID')
->willReturn('userId');
$user->method('getBackend')
->willReturn($backend);

$expectedResp = new DataResponse($expected);
$this->assertEquals($expectedResp, $this->api->getEditableFields());
$this->assertEquals($expectedResp, $this->api->getEditableFields('userId'));
}

private function mockAccount($targetUser, $accountProperties) {
Expand Down