Skip to content

Commit

Permalink
Fixed issue #19107: [security] Superadmin can update any admin's info…
Browse files Browse the repository at this point in the history
…rmation (#3495)

Co-authored-by: Denis Chenu <contact@sondages.prowq>
  • Loading branch information
Shnoulle and Denis Chenu committed Oct 23, 2023
1 parent bc16cfa commit 9b54a82
Show file tree
Hide file tree
Showing 4 changed files with 43 additions and 41 deletions.
20 changes: 5 additions & 15 deletions application/controllers/UserManagementController.php
Expand Up @@ -1321,29 +1321,19 @@ public function loadModel(int $id): User
/**
* Update admin-user
*
* REFACTORED (in UserManagementController)
*
* @param array $aUser array with user details
* @return object user - updated user object
* @throws CException
*/
public function updateAdminUser(array $aUser): User
{
$oUser = User::model()->findByPk($aUser['uid']);
//If the user id of the post is spoofed somehow it would be possible to edit superadmin users
//Therefore we need to make sure no non-superadmin can modify superadmin accounts
//Since this should NEVER be the case without hacking the software, this will silently just do nothing.
if (
!Permission::model()->hasGlobalPermission('superadmin', 'read', Yii::app()->user->id)
&& Permission::model()->hasGlobalPermission('superadmin', 'read', $oUser->uid)
) {
throw new CException("This action is not allowed, and should never happen", 500);
}

$oUser = $this->loadModel($aUser['uid']);
// Abort if logged in user has no access to this user.
// Using same logic as User::getButtons().
$userManager = new UserManager(Yii::app()->user, $oUser);
if (!$userManager->canEdit()) {
if (
!$oUser->canEdit()
|| $aUser['uid'] == App()->user->id // To update self : must use personal settings
) {
throw new CHttpException(403, gT("You do not have permission to access this page."));
}

Expand Down
46 changes: 32 additions & 14 deletions application/models/User.php
Expand Up @@ -567,13 +567,8 @@ public function getManagementButtons()
'linkAttributes' => [
'data-href' => $editUrl,
],
'enabledCondition' =>
($permission_superadmin_read
&& $this->uid != 1
)
|| (!$permission_superadmin_read
&& $this->canEdit(App()->session['loginID'])
)
'enabledCondition' => $this->canEdit()
&& $this->uid != App()->user->getId() // To update self : must use personal settings
];
$dropdownItems[] = [
'title' => gT('Template permissions'),
Expand Down Expand Up @@ -997,16 +992,39 @@ public function getGroupMemberListButtons()
}

/**
* Returns true if logged in user with id $loginId can edit this user
* Return true if user with id $managerId can edit this user
* @param int|null $managerId default to current user
*
* @param int $loginId
* @return bool
*/
public function canEdit($loginId)
public function canEdit($managerId = null)
{
return
Permission::model()->hasGlobalPermission('superadmin', 'read')
|| $this->uid == $loginId
|| (Permission::model()->hasGlobalPermission('users', 'update') && $this->parent_id == $loginId);
if (is_null($managerId)) {
$managerId = Permission::model()->getUserId();
}
/* user can update himself */
if ($managerId == $this->uid) {
return true;
}
/* forcedsuperamdin (user #1) can always update all */
if (Permission::isForcedSuperAdmin($managerId)) {
return true;
}
/* forcedsuperamdin can not be update (except by another forcedsuperamdin done before) */
if (Permission::isForcedSuperAdmin($this->uid)) {
return false;
}
/* If target user is superamdin : managingUser must be allowed to create superadmin and be parent */
if (Permission::model()->hasGlobalPermission('superadmin', 'read', $this->uid)) {
return Permission::model()->hasGlobalPermission('superadmin', 'create', $managerId)
&& $this->parent_id == $managerId;
}
/* superamin can update all other user */
if (Permission::model()->hasGlobalPermission('superadmin', 'read', $managerId)) {
return true;
}
/* Finally : simple user can update only childs users */
return Permission::model()->hasGlobalPermission('users', 'update', $managerId)
&& $this->parent_id == $managerId;
}
}
12 changes: 3 additions & 9 deletions application/models/services/UserManager.php
Expand Up @@ -68,22 +68,16 @@ public function canAssignRole()
}

/**
* Returns true if the managing user can edit the target user
* Returns true if the managing user can edit the attribute of the target user
* Return true if target is same then managing (user can always update himself)
* @return bool
*/
public function canEdit()
{
if (empty($this->managingUser) || empty($this->targetUser)) {
return false;
}

return
Permission::model()->hasGlobalPermission('superadmin', 'read', $this->managingUser->id)
|| $this->targetUser->uid == $this->managingUser->id
|| (
Permission::model()->hasGlobalPermission('users', 'update', $this->managingUser->id)
&& $this->targetUser->parent_id == $this->managingUser->id
);
return $this->targetUser->canEdit($this->managingUser->id);
}

/**
Expand Down
6 changes: 3 additions & 3 deletions tests/unit/controllers/UserManagementTest.php
Expand Up @@ -118,14 +118,14 @@ public function testUpdateAdminUserTamperproofed() {
try {
$oUserManagementController->updateAdminUser($aChangeDataSet);
} catch(\CException $exception) {
if($exception->getCode() == 500) {

if($exception->statusCode == 403) {
\Yii::app()->session['loginID'] = 1;
$this->assertTrue(true);
return;
}
/* throw the exception : user was not updated, but bad exception happen */
throw $exception;
}

\Yii::app()->session['loginID'] = 1;
throw new \Exception(
"Test ".__METHOD__ ." failed: \n"
Expand Down

0 comments on commit 9b54a82

Please sign in to comment.