From a1d746fe05048b3e23998254953eafe02e777b47 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Thu, 8 Apr 2021 13:28:13 +0200 Subject: [PATCH 1/3] Only return display name as editable when the user backend allows it Signed-off-by: Joas Schilling --- apps/provisioning_api/appinfo/routes.php | 1 + .../lib/Controller/UsersController.php | 41 ++++++++++++++++--- .../tests/Controller/UsersControllerTest.php | 35 +++++++++++++--- 3 files changed, 66 insertions(+), 11 deletions(-) diff --git a/apps/provisioning_api/appinfo/routes.php b/apps/provisioning_api/appinfo/routes.php index 912dd82e85..6982a8ba28 100644 --- a/apps/provisioning_api/appinfo/routes.php +++ b/apps/provisioning_api/appinfo/routes.php @@ -53,6 +53,7 @@ return [ ['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'], diff --git a/apps/provisioning_api/lib/Controller/UsersController.php b/apps/provisioning_api/lib/Controller/UsersController.php index f65744d943..eb277af290 100644 --- a/apps/provisioning_api/lib/Controller/UsersController.php +++ b/apps/provisioning_api/lib/Controller/UsersController.php @@ -525,13 +525,38 @@ class UsersController extends AUserData { /** * @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('', OCSController::RESPOND_NOT_FOUND); + } + $permittedFields = []; + if ($userId !== $currentLoggedInUser->getUID()) { + $targetUser = $this->userManager->get($userId); + if (!$targetUser instanceof IUser) { + throw new OCSException('', OCSController::RESPOND_NOT_FOUND); + } + + $subAdminManager = $this->groupManager->getSubAdmin(); + if (!$this->groupManager->isAdmin($currentLoggedInUser->getUID()) + && !$subAdminManager->isUserAccessible($currentLoggedInUser, $targetUser)) { + throw new OCSException('', OCSController::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) { + $permittedFields[] = IAccountManager::PROPERTY_DISPLAYNAME; + } $permittedFields[] = IAccountManager::PROPERTY_EMAIL; } @@ -568,8 +593,10 @@ class UsersController extends AUserData { 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) { + $permittedFields[] = 'display'; + $permittedFields[] = IAccountManager::PROPERTY_DISPLAYNAME; + } $permittedFields[] = IAccountManager::PROPERTY_EMAIL; } @@ -608,8 +635,10 @@ class UsersController extends AUserData { 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) { + $permittedFields[] = 'display'; + $permittedFields[] = IAccountManager::PROPERTY_DISPLAYNAME; + } $permittedFields[] = IAccountManager::PROPERTY_EMAIL; $permittedFields[] = 'password'; $permittedFields[] = 'language'; diff --git a/apps/provisioning_api/tests/Controller/UsersControllerTest.php b/apps/provisioning_api/tests/Controller/UsersControllerTest.php index 2b4ca78062..c9b848f301 100644 --- a/apps/provisioning_api/tests/Controller/UsersControllerTest.php +++ b/apps/provisioning_api/tests/Controller/UsersControllerTest.php @@ -67,6 +67,8 @@ use OCP\L10N\IFactory; use OCP\Mail\IEMailTemplate; use OCP\Security\Events\GenerateSecurePasswordEvent; use OCP\Security\ISecureRandom; +use OCP\User\Backend\IGetDisplayNameBackend; +use OCP\User\Backend\ISetDisplayNameBackend; use OCP\UserInterface; use PHPUnit\Framework\MockObject\MockObject; use Test\TestCase; @@ -1443,6 +1445,10 @@ class UsersControllerTest extends TestCase { ->method('get') ->with('UserToEdit') ->willReturn($targetUser); + $targetUser + ->expects($this->once()) + ->method('getBackend') + ->willReturn($this->createMock(ISetDisplayNameBackend::class)); $targetUser ->expects($this->once()) ->method('setDisplayName') @@ -3716,20 +3722,27 @@ class UsersControllerTest extends TestCase { 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, IGetDisplayNameBackend::class, [ + IAccountManager::PROPERTY_EMAIL, + IAccountManager::PROPERTY_PHONE, + IAccountManager::PROPERTY_ADDRESS, + IAccountManager::PROPERTY_WEBSITE, + IAccountManager::PROPERTY_TWITTER, + ]], ]; } @@ -3737,9 +3750,10 @@ class UsersControllerTest extends TestCase { * @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( @@ -3747,8 +3761,19 @@ class UsersControllerTest extends TestCase { $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) { From 5975a68033e675eee8a0a3d52a6090c05301f2d2 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Mon, 26 Apr 2021 14:34:03 +0200 Subject: [PATCH 2/3] Also check implementsAction method Signed-off-by: Joas Schilling --- .../lib/Controller/UsersController.php | 10 ++- .../tests/Controller/UsersControllerTest.php | 87 ++++++++++++++++++- 2 files changed, 92 insertions(+), 5 deletions(-) diff --git a/apps/provisioning_api/lib/Controller/UsersController.php b/apps/provisioning_api/lib/Controller/UsersController.php index eb277af290..2c7b762fc7 100644 --- a/apps/provisioning_api/lib/Controller/UsersController.php +++ b/apps/provisioning_api/lib/Controller/UsersController.php @@ -50,6 +50,7 @@ use OC\Accounts\AccountManager; 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; @@ -554,7 +555,8 @@ class UsersController extends AUserData { // Editing self (display, email) if ($this->config->getSystemValue('allow_user_to_change_display_name', true) !== false) { - if ($targetUser->getBackend() instanceof ISetDisplayNameBackend) { + if ($targetUser->getBackend() instanceof ISetDisplayNameBackend + || $targetUser->getBackend()->implementsActions(Backend::SET_DISPLAYNAME)) { $permittedFields[] = IAccountManager::PROPERTY_DISPLAYNAME; } $permittedFields[] = IAccountManager::PROPERTY_EMAIL; @@ -593,7 +595,8 @@ class UsersController extends AUserData { if ($targetUser->getUID() === $currentLoggedInUser->getUID()) { // Editing self (display, email) if ($this->config->getSystemValue('allow_user_to_change_display_name', true) !== false) { - if ($targetUser->getBackend() instanceof ISetDisplayNameBackend) { + if ($targetUser->getBackend() instanceof ISetDisplayNameBackend + || $targetUser->getBackend()->implementsActions(Backend::SET_DISPLAYNAME)) { $permittedFields[] = 'display'; $permittedFields[] = IAccountManager::PROPERTY_DISPLAYNAME; } @@ -635,7 +638,8 @@ class UsersController extends AUserData { if ($this->groupManager->isAdmin($currentLoggedInUser->getUID()) || $subAdminManager->isUserAccessible($currentLoggedInUser, $targetUser)) { // They have permissions over the user - if ($targetUser->getBackend() instanceof ISetDisplayNameBackend) { + if ($targetUser->getBackend() instanceof ISetDisplayNameBackend + || $targetUser->getBackend()->implementsActions(Backend::SET_DISPLAYNAME)) { $permittedFields[] = 'display'; $permittedFields[] = IAccountManager::PROPERTY_DISPLAYNAME; } diff --git a/apps/provisioning_api/tests/Controller/UsersControllerTest.php b/apps/provisioning_api/tests/Controller/UsersControllerTest.php index c9b848f301..1420544761 100644 --- a/apps/provisioning_api/tests/Controller/UsersControllerTest.php +++ b/apps/provisioning_api/tests/Controller/UsersControllerTest.php @@ -67,7 +67,6 @@ use OCP\L10N\IFactory; use OCP\Mail\IEMailTemplate; use OCP\Security\Events\GenerateSecurePasswordEvent; use OCP\Security\ISecureRandom; -use OCP\User\Backend\IGetDisplayNameBackend; use OCP\User\Backend\ISetDisplayNameBackend; use OCP\UserInterface; use PHPUnit\Framework\MockObject\MockObject; @@ -1490,6 +1489,12 @@ class UsersControllerTest extends TestCase { ->method('getUID') ->willReturn('UID'); + $backend = $this->createMock(UserInterface::class); + $targetUser + ->expects($this->any()) + ->method('getBackend') + ->willReturn($backend); + $this->assertEquals([], $this->api->editUser('UserToEdit', 'email', 'demo@nextcloud.com')->getData()); } @@ -1523,6 +1528,12 @@ class UsersControllerTest extends TestCase { ->method('getUID') ->willReturn('UID'); + $backend = $this->createMock(UserInterface::class); + $targetUser + ->expects($this->any()) + ->method('getBackend') + ->willReturn($backend); + $this->api->editUser('UserToEdit', 'email', 'demo.org'); } @@ -1556,6 +1567,12 @@ class UsersControllerTest extends TestCase { ->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) @@ -1600,6 +1617,12 @@ class UsersControllerTest extends TestCase { ->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) @@ -1644,6 +1667,12 @@ class UsersControllerTest extends TestCase { ->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()); } @@ -1677,6 +1706,12 @@ class UsersControllerTest extends TestCase { ->method('getUID') ->willReturn('UID'); + $backend = $this->createMock(UserInterface::class); + $targetUser + ->expects($this->any()) + ->method('getBackend') + ->willReturn($backend); + $this->api->editUser('UserToEdit', 'quota', 'NewQuota'); } @@ -1709,6 +1744,12 @@ class UsersControllerTest extends TestCase { ->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()); } @@ -1744,6 +1785,12 @@ class UsersControllerTest extends TestCase { ->method('getUID') ->willReturn('UID'); + $backend = $this->createMock(UserInterface::class); + $targetUser + ->expects($this->any()) + ->method('getBackend') + ->willReturn($backend); + $this->api->editUser('UserToEdit', 'quota', 'ABC'); } @@ -1783,6 +1830,12 @@ class UsersControllerTest extends TestCase { ->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()); } @@ -1825,6 +1878,12 @@ class UsersControllerTest extends TestCase { ->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()); } @@ -1875,6 +1934,12 @@ class UsersControllerTest extends TestCase { ->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()); } @@ -1916,6 +1981,12 @@ class UsersControllerTest extends TestCase { ->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()); } @@ -1962,6 +2033,12 @@ class UsersControllerTest extends TestCase { ->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()); } @@ -2001,6 +2078,12 @@ class UsersControllerTest extends TestCase { ->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()); } @@ -3736,7 +3819,7 @@ class UsersControllerTest extends TestCase { IAccountManager::PROPERTY_WEBSITE, IAccountManager::PROPERTY_TWITTER, ]], - [true, IGetDisplayNameBackend::class, [ + [true, UserInterface::class, [ IAccountManager::PROPERTY_EMAIL, IAccountManager::PROPERTY_PHONE, IAccountManager::PROPERTY_ADDRESS, From ca8b2a6b5e64f61b48d50f7d2aef159d2793766d Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Thu, 29 Apr 2021 08:19:20 +0200 Subject: [PATCH 3/3] FIx stable21 difference Signed-off-by: Joas Schilling --- apps/provisioning_api/lib/Controller/UsersController.php | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/apps/provisioning_api/lib/Controller/UsersController.php b/apps/provisioning_api/lib/Controller/UsersController.php index 2c7b762fc7..b6b9efb078 100644 --- a/apps/provisioning_api/lib/Controller/UsersController.php +++ b/apps/provisioning_api/lib/Controller/UsersController.php @@ -71,6 +71,7 @@ use OCP\L10N\IFactory; use OCP\Security\ISecureRandom; use OCP\Security\Events\GenerateSecurePasswordEvent; use OCP\EventDispatcher\IEventDispatcher; +use OCP\User\Backend\ISetDisplayNameBackend; class UsersController extends AUserData { @@ -533,7 +534,7 @@ class UsersController extends AUserData { public function getEditableFields(?string $userId = null): DataResponse { $currentLoggedInUser = $this->userSession->getUser(); if (!$currentLoggedInUser instanceof IUser) { - throw new OCSException('', OCSController::RESPOND_NOT_FOUND); + throw new OCSException('', \OCP\API::RESPOND_NOT_FOUND); } $permittedFields = []; @@ -541,13 +542,13 @@ class UsersController extends AUserData { if ($userId !== $currentLoggedInUser->getUID()) { $targetUser = $this->userManager->get($userId); if (!$targetUser instanceof IUser) { - throw new OCSException('', OCSController::RESPOND_NOT_FOUND); + 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('', OCSController::RESPOND_NOT_FOUND); + throw new OCSException('', \OCP\API::RESPOND_NOT_FOUND); } } else { $targetUser = $currentLoggedInUser;