Fixed issue #19126: [security] Incorrect Authorization check in add U…

…ser role (#3506)
LimeSurvey · Oct 5, 2023 · e8dc909 · e8dc909
1 parent d425504
commit e8dc909
Show file tree

Hide file tree

Showing 4 changed files with 12 additions and 13 deletions.
diff --git a/application/controllers/UserManagementController.php b/application/controllers/UserManagementController.php
@@ -638,7 +638,7 @@ public function actionAddRole(): ?string
         $oUser = User::model()->findByPk($userId);
 
         $userManager = new UserManager(Yii::app()->user, $oUser);
-        if (!$userManager->canAssignRole()) {
+        if (!$userManager->canAssignRole() || $oUser->uid == App()->user->getId()) {
             return $this->renderPartial(
                 'partial/error',
                 ['errors' => [gT("You do not have permission to access this page.")], 'noButton' => true]
@@ -679,11 +679,11 @@ public function actionSaveRole(): ?string
         $oUser = User::model()->findByPk($iUserId);
 
         $userManager = new UserManager(Yii::app()->user, $oUser);
-        if (!$userManager->canAssignRole()) {
+        if (!$userManager->canAssignRole() || $oUser->uid == App()->user->getId()) {
             return Yii::app()->getController()->renderPartial('/admin/super/_renderJson', [
                 "data" => [
                     'success' => false,
-                    'errors' => [gT("You do not have permission to access this page.")],
+                    'errors' => gT("You do not have permission to access this page."),
                 ]
             ]);
         }

diff --git a/application/models/User.php b/application/models/User.php
@@ -460,6 +460,8 @@ public function getManagementButtons()
         $permission_users_read = Permission::model()->hasGlobalPermission('users', 'read');
         $permission_users_update = Permission::model()->hasGlobalPermission('users', 'update');
         $permission_users_delete = Permission::model()->hasGlobalPermission('users', 'delete');
+        $userManager = new UserManager(App()->user, $this);
+
         // User is owned or created by you
         $ownedOrCreated = $this->parent_id == App()->session['loginID'];
 
@@ -526,12 +528,7 @@ public function getManagementButtons()
             'linkAttributes'   => [
                 'data-href' => $setRoleUrl,
             ],
-            'enabledCondition' =>
-                ($permission_superadmin_read
-                    && !(Permission::isForcedSuperAdmin($this->uid)
-                        || $this->uid == App()->user->getId()
-                    )
-                )
+            'enabledCondition' => $userManager->canAssignRole() && $this->uid != App()->user->getId()
         ];
         $dropdownItems[] = [
             'title'            => gT('Edit user'),

diff --git a/application/models/services/UserManager.php b/application/models/services/UserManager.php
@@ -63,8 +63,8 @@ public function canAssignRole()
         if (empty($this->managingUser)) {
             return false;
         }
-
-        return Permission::model()->hasGlobalPermission('superadmin', 'read', $this->managingUser->id);
+        /* roles can have superadmin permission, then need superadmin/create permission */
+        return Permission::model()->hasGlobalPermission('superadmin', 'create', $this->managingUser->id);
     }
 
     /**

diff --git a/tests/unit/models/UserManagerServiceTest.php b/tests/unit/models/UserManagerServiceTest.php
@@ -327,6 +327,7 @@ public function testSuperadminCanAssignRole()
         $managingUserPermissions = array(
             'superadmin' => array(
                 'read'   => true,
+                'create' => true,
             )
         );
 
@@ -349,7 +350,7 @@ public function testSuperadminCanAssignRole()
 
         $assignPermission = $userManager->canAssignRole();
 
-        $this->assertTrue($assignPermission, 'A user with superadmin / read permissions should be able to assign roles to any user.');
+        $this->assertTrue($assignPermission, 'A user with superadmin / create permissions should be able to assign roles to any user.');
 
         //Delete managing user.
         $userManagingUser->delete();
@@ -380,6 +381,7 @@ public function testSuperadminCanNotAssignRoles()
 
         $managingUserPermissions = array(
             'superadmin' => array(
+                'read'   => true,
                 'import'   => true,
             )
         );
@@ -403,7 +405,7 @@ public function testSuperadminCanNotAssignRoles()
 
         $assignPermission = $userManager->canAssignRole();
 
-        $this->assertFalse($assignPermission, 'A user without superadmin / read permissions should not be able to assign roles to any user.');
+        $this->assertFalse($assignPermission, 'A user without superadmin / create permissions should not be able to assign roles to any user.');
 
         //Delete managing user.
         $userManagingUser->delete();