Fixed issue 18915: [security] Non-superadmin Admin user is able to ed…

…it groups not owned (#3248)
LimeSurvey · Jun 27, 2023 · d2ab1fe · d2ab1fe
1 parent 4a3db1e
commit d2ab1fe
Show file tree

Hide file tree

Showing 2 changed files with 29 additions and 13 deletions.
diff --git a/application/controllers/UserGroupController.php b/application/controllers/UserGroupController.php
@@ -236,8 +236,13 @@ public function actionEdit(int $ugid)
                 }
             } else {
                 $result = UserGroup::model()->requestEditGroup($ugid, Yii::app()->session['loginID']);
-                $aData['model'] = $result;
-                $aData['ugid'] = $result->ugid;
+                if ($result !== null) {
+                    $aData['model'] = $result;
+                    $aData['ugid'] = $result->ugid;
+                } else {
+                    Yii::app()->session['flashmessage'] = gT("You don't have permission to edit this usergroup");
+                    $this->redirect(App()->createUrl("/admin"));
+                }
             }
         } else {
             Yii::app()->session['flashmessage'] = gT("You don't have permission to edit a usergroup");

diff --git a/application/models/UserGroup.php b/application/models/UserGroup.php
@@ -159,26 +159,37 @@ public function addGroup($group_name, $group_description)
     }
 
     /**
-     * TODO should be in controller
+     * TODO should be in controller?
+     * When user is allowed to edit the group,
+     * it will be updated in the database.
+     * Returns true, when it was possible to save.
      * @param string $name
      * @param string $description
      * @param integer $ugId
      * @return bool
      */
     public function updateGroup($name, $description, $ugId)
     {
-        $group = UserGroup::model()->findByPk($ugId);
-        $group->name = $name;
-        $group->description = $description;
-        $group->save();
-        if ($group->getErrors()) {
-            return false;
-        } else {
-            return true;
+        $group = $this->requestEditGroup($ugId, App()->user->id);
+        if ($group !== null) {
+            $group->name = $name;
+            $group->description = $description;
+            $group->save();
+            if ($group->getErrors()) {
+                return false;
+            } else {
+                return true;
+            }
         }
+        return false;
     }
 
     /**
+     * Works as permission check on db level for editing user groups.
+     * The usergroup needs to exist, and if the user is not a superadmin,
+     * user also has to be the owner of that group.
+     * If successful, the usergroup is returned.
+     *
      * @param integer $ugId
      * @param integer $ownerId
      * @return static
@@ -196,8 +207,8 @@ public function requestEditGroup($ugId, $ownerId)
 
         $aParams[':ugid'] = $ugId;
         $criteria->params = $aParams;
-        $result = UserGroup::model()->find($criteria);
-        return $result;
+
+        return UserGroup::model()->find($criteria);
     }
 
     /**