Skip to content

Commit

Permalink
Fix issue 39687 - allow admin and subadmin to always change email add…
Browse files Browse the repository at this point in the history 
…resses and display names
  • Loading branch information
@phil-davis
phil-davis committed Jan 14, 2022
1 parent 4a0e077 commit 187848a
Show file tree
Hide file tree
Showing 2 changed files with 37 additions and 13 deletions.
23 changes: 10 additions & 13 deletions apps/provisioning_api/lib/Users.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -252,38 +252,35 @@ 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)
$permittedFields[] = 'display';
$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);
Expand Down
27 changes: 27 additions & 0 deletions apps/provisioning_api/tests/UsersTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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())
Expand Down Expand Up @@ -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')
Expand All @@ -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')
Expand All @@ -1008,6 +1025,7 @@ public function testEditUserRegularUserSelfEditChangeDisplaynameProhibited() {
->method('get')
->with('UserToEdit')
->will($this->returnValue($targetUser));
$this->setupSubadminMock($loggedInUser, $targetUser);
$this->config
->method('getSystemValue')
->withConsecutive(
Expand Down Expand Up @@ -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')
Expand All @@ -1058,6 +1077,7 @@ public function testEditUserRegularUserSelfEditChangeEmailProhibited() {
->method('get')
->with('UserToEdit')
->will($this->returnValue($targetUser));
$this->setupSubadminMock($loggedInUser, $targetUser);
$this->config
->method('getSystemValue')
->withConsecutive(
Expand Down Expand Up @@ -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' => '']]));
Expand All @@ -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']]));
Expand All @@ -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']]));
Expand All @@ -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']]));
Expand All @@ -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]]));
Expand Down Expand Up @@ -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']]));
Expand All @@ -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']]));
Expand Down

0 comments on commit 187848a

Please sign in to comment.