From ab5df01f54eca7b703c8982c5703603f97ec9166 Mon Sep 17 00:00:00 2001 From: Phil Davis Date: Thu, 13 Jan 2022 21:49:45 +0545 Subject: [PATCH] Fix issue 39687 - allow admin and subadmin to always change email addresses and display names --- apps/provisioning_api/lib/Users.php | 23 +++++++++---------- apps/provisioning_api/tests/UsersTest.php | 27 +++++++++++++++++++++++ 2 files changed, 37 insertions(+), 13 deletions(-) diff --git a/apps/provisioning_api/lib/Users.php b/apps/provisioning_api/lib/Users.php index 786a13096a68..3ff8ef36eda5 100644 --- a/apps/provisioning_api/lib/Users.php +++ b/apps/provisioning_api/lib/Users.php @@ -252,6 +252,9 @@ public function editUser($parameters) { $emailChangeAllowed = $this->config->getSystemValue('allow_user_to_change_mail_address', true) !== false; $displayNameChangeAllowed = $this->config->getSystemValue('allow_user_to_change_display_name', true) !== false; + $subAdminManager = $this->groupManager->getSubAdmin(); + $isAdmin = $this->groupManager->isAdmin($currentLoggedInUser->getUID()); + $hasPermissionsOverTargetUser = ($subAdminManager->isUserAccessible($currentLoggedInUser, $targetUser) || $isAdmin); if ($targetUserId === $currentLoggedInUser->getUID()) { // Editing self (display, email) @@ -259,31 +262,25 @@ public function editUser($parameters) { $permittedFields[] = 'password'; $permittedFields[] = 'two_factor_auth_enabled'; // If admin they can edit their own quota - if ($this->groupManager->isAdmin($currentLoggedInUser->getUID())) { + if ($isAdmin) { $permittedFields[] = 'quota'; } - if ($emailChangeAllowed) { + if ($emailChangeAllowed || $hasPermissionsOverTargetUser) { $permittedFields[] = 'email'; } - if ($displayNameChangeAllowed) { + if ($displayNameChangeAllowed || $hasPermissionsOverTargetUser) { $permittedFields[] = 'displayname'; } } else { // Check if admin / subadmin - $subAdminManager = $this->groupManager->getSubAdmin(); - if ($subAdminManager->isUserAccessible($currentLoggedInUser, $targetUser) - || $this->groupManager->isAdmin($currentLoggedInUser->getUID())) { - // They have permissions over the user + if ($hasPermissionsOverTargetUser) { + // They have permissions over the user, so they can edit any field $permittedFields[] = 'display'; $permittedFields[] = 'quota'; $permittedFields[] = 'password'; $permittedFields[] = 'two_factor_auth_enabled'; - if ($emailChangeAllowed) { - $permittedFields[] = 'email'; - } - if ($displayNameChangeAllowed) { - $permittedFields[] = 'displayname'; - } + $permittedFields[] = 'email'; + $permittedFields[] = 'displayname'; } else { // No rights return new Result(null, 997); diff --git a/apps/provisioning_api/tests/UsersTest.php b/apps/provisioning_api/tests/UsersTest.php index 96e706f7daa2..d537fe1dfdde 100644 --- a/apps/provisioning_api/tests/UsersTest.php +++ b/apps/provisioning_api/tests/UsersTest.php @@ -95,6 +95,21 @@ protected function setUp(): void { ->getMock(); } + private function setupSubadminMock($loggedInUser, $targetUser, $isUserAccessible = false) { + $subAdminManager = $this->getMockBuilder(SubAdmin::class) + ->disableOriginalConstructor() + ->getMock(); + $subAdminManager + ->expects($this->once()) + ->method('isUserAccessible') + ->with($loggedInUser, $targetUser) + ->will($this->returnValue($isUserAccessible)); + $this->groupManager + ->expects($this->once()) + ->method('getSubAdmin') + ->will($this->returnValue($subAdminManager)); + } + public function testGetUsersNotLoggedIn() { $this->userSession ->expects($this->once()) @@ -961,6 +976,7 @@ public function testEditUserRegularUserSelfEditChangeDisplay() { ->method('get') ->with('UserToEdit') ->will($this->returnValue($targetUser)); + $this->setupSubadminMock($loggedInUser, $targetUser); $targetUser ->expects($this->once()) ->method('setDisplayName') @@ -986,6 +1002,7 @@ public function testEditUserRegularUserSelfEditChangeDisplayName() { ->method('get') ->with('UserToEdit') ->will($this->returnValue($targetUser)); + $this->setupSubadminMock($loggedInUser, $targetUser); $targetUser ->expects($this->once()) ->method('setDisplayName') @@ -1008,6 +1025,7 @@ public function testEditUserRegularUserSelfEditChangeDisplaynameProhibited() { ->method('get') ->with('UserToEdit') ->will($this->returnValue($targetUser)); + $this->setupSubadminMock($loggedInUser, $targetUser); $this->config ->method('getSystemValue') ->withConsecutive( @@ -1036,6 +1054,7 @@ public function testEditUserRegularUserSelfEditChangeEmailValid() { ->method('get') ->with('UserToEdit') ->will($this->returnValue($targetUser)); + $this->setupSubadminMock($loggedInUser, $targetUser); $targetUser ->expects($this->once()) ->method('setEMailAddress') @@ -1058,6 +1077,7 @@ public function testEditUserRegularUserSelfEditChangeEmailProhibited() { ->method('get') ->with('UserToEdit') ->will($this->returnValue($targetUser)); + $this->setupSubadminMock($loggedInUser, $targetUser); $this->config ->method('getSystemValue') ->withConsecutive( @@ -1090,6 +1110,7 @@ public function testEditUserRegularUserSelfEditClearEmail() { ->expects($this->once()) ->method('setEMailAddress') ->with(''); + $this->setupSubadminMock($loggedInUser, $targetUser); $expected = new Result(null, 100); $this->assertEquals($expected, $this->api->editUser(['userid' => 'UserToEdit', '_put' => ['key' => 'email', 'value' => '']])); @@ -1111,6 +1132,7 @@ public function testEditUserRegularUserSelfEditChangeEmailInvalid() { ->method('get') ->with('UserToEdit') ->will($this->returnValue($targetUser)); + $this->setupSubadminMock($loggedInUser, $targetUser); $expected = new Result(null, 102); $this->assertEquals($expected, $this->api->editUser(['userid' => 'UserToEdit', '_put' => ['key' => 'email', 'value' => 'demo.org']])); @@ -1136,6 +1158,7 @@ public function testEditUserRegularUserSelfEditChangePassword() { ->expects($this->once()) ->method('setPassword') ->with('NewPassword'); + $this->setupSubadminMock($loggedInUser, $targetUser); $expected = new Result(null, 100); $this->assertEquals($expected, $this->api->editUser(['userid' => 'UserToEdit', '_put' => ['key' => 'password', 'value' => 'NewPassword']])); @@ -1157,6 +1180,7 @@ public function testEditUserRegularUserSelfEditChangeQuota() { ->method('get') ->with('UserToEdit') ->will($this->returnValue($targetUser)); + $this->setupSubadminMock($loggedInUser, $targetUser); $expected = new Result(null, 997); $this->assertEquals($expected, $this->api->editUser(['userid' => 'UserToEdit', '_put' => ['key' => 'quota', 'value' => 'NewQuota']])); @@ -1181,6 +1205,7 @@ public function testEditTwoFactor() { $this->twoFactorAuthManager ->expects($this->once()) ->method('enableTwoFactorAuthentication'); + $this->setupSubadminMock($loggedInUser, $targetUser); $expected = new Result(null, 100); $this->assertEquals($expected, $this->api->editUser(['userid' => 'UserToEdit', '_put' => ['key' => 'two_factor_auth_enabled', 'value' => true]])); @@ -1210,6 +1235,7 @@ public function testEditUserAdminUserSelfEditChangeValidQuota() { ->method('isAdmin') ->with('UserToEdit') ->will($this->returnValue(true)); + $this->setupSubadminMock($loggedInUser, $targetUser); $expected = new Result(null, 100); $this->assertEquals($expected, $this->api->editUser(['userid' => 'UserToEdit', '_put' => ['key' => 'quota', 'value' => '3042824']])); @@ -1236,6 +1262,7 @@ public function testEditUserAdminUserSelfEditChangeInvalidQuota() { ->method('isAdmin') ->with('UserToEdit') ->will($this->returnValue(true)); + $this->setupSubadminMock($loggedInUser, $targetUser); $expected = new Result(null, 103, 'Invalid quota value ABC'); $this->assertEquals($expected, $this->api->editUser(['userid' => 'UserToEdit', '_put' => ['key' => 'quota', 'value' => 'ABC']]));