#10290 Fixed comments and context handling

pkp · Feb 13, 2025 · a58446c · a58446c
1 parent 70ed8f8
commit a58446c
Show file tree

Hide file tree

Showing 3 changed files with 48 additions and 70 deletions.
diff --git a/classes/security/Validation.php b/classes/security/Validation.php
@@ -490,91 +490,76 @@ public static function getAdministrationLevel(
         int $administratorUserId,
         ?int $contextId = null
     ): int {
-    
+
         if ($administeredUserId === $administratorUserId) {
             return self::ADMINISTRATION_FULL;
         }
-    
+
         $siteContextId = PKPApplication::SITE_CONTEXT_ID;
-    
+
         // single query to fetch user groups assigned to either user
         $allUserGroups = UserGroup::query()
-            ->whereHas('userUserGroups', function ($q) use ($administratorUserId, $administeredUserId) {
-                $q->withActive()->withUserIds([$administratorUserId, $administeredUserId]);
-            })
-            ->with(['userUserGroups' => function ($q) use ($administratorUserId, $administeredUserId) {
-                $q->withActive()->withUserIds([$administratorUserId, $administeredUserId]);
-            }])
+            ->whereHas('userUserGroups', fn($q) =>
+                $q->withActive()->withUserIds([$administratorUserId, $administeredUserId])
+            )
+            ->with(['userUserGroups' => fn($q) =>
+                $q->withActive()->withUserIds([$administratorUserId, $administeredUserId])
+            ])
             ->get();
-
-        // parsing them in memory
-        $adminUserRoles = [
-            'siteAdmin' => false,
-            'managerContexts' => [],
-            'anyRoleContexts' => [],
-        ];
-        $administeredUserRoles = [
-            'siteAdmin' => false,
-            'allContexts' => [],
-        ];
-
+
+        $administratorMap = [];
+        $administeredMap = [];
+
         foreach ($allUserGroups as $userGroup) {
             $roleId = $userGroup->roleId;
-            $ctxId = $userGroup->contextId ?? 0;
-    
+            $userGroupContextId = $userGroup->contextId ?? PKPApplication::SITE_CONTEXT_ID;
+
             // then each user assignment row
             foreach ($userGroup->userUserGroups as $uug) {
-                $uid = $uug->userId;
-
-                // checking site admin
-                if ($roleId === Role::ROLE_ID_SITE_ADMIN && $ctxId === $siteContextId) {
-                    if ($uid === $administeredUserId) {
-                        $administeredUserRoles['siteAdmin'] = true;
-                    } elseif ($uid === $administratorUserId) {
-                        $adminUserRoles['siteAdmin'] = true;
-                    }
-                }
-
-                // manager role => store context
-                if ($roleId === Role::ROLE_ID_MANAGER && $uid === $administratorUserId) {
-                    $adminUserRoles['managerContexts'][] = $ctxId;
-                }
-
-                // for the administered user => track any role’s context
-                if ($uid === $administeredUserId) {
-                    $administeredUserRoles['allContexts'][] = $ctxId;
-                }
-
-                // for the administrator => track all roles
-                if ($uid === $administratorUserId) {
-                    $adminUserRoles['anyRoleContexts'][] = $ctxId;
+                if ($uug->userId === $administratorUserId) {
+                    $administratorMap[$userGroupContextId][] = $roleId;
+                } elseif ($uug->userId === $administeredUserId) {
+                    $administeredMap[$userGroupContextId][] = $roleId;
                 }
             }
         }
-
-        // perform the same siteAdmin / manager checks
-        if ($administeredUserRoles['siteAdmin']) {
+
+        if (
+            isset($administeredMap[$siteContextId]) &&
+            in_array(Role::ROLE_ID_SITE_ADMIN, $administeredMap[$siteContextId], true)
+        ) {
             return self::ADMINISTRATION_PROHIBITED;
         }
-        if ($adminUserRoles['siteAdmin']) {
+
+        // if administrator user is site admin => FULL
+        if (
+            isset($administratorMap[$siteContextId]) &&
+            in_array(Role::ROLE_ID_SITE_ADMIN, $administratorMap[$siteContextId], true)
+        ) {
             return self::ADMINISTRATION_FULL;
         }
 
-        $administratorManagerContexts = array_unique($adminUserRoles['managerContexts']);
+        // gather manager contexts for the administrator
+        $administratorManagerContexts = [];
+        foreach ($administratorMap as $ctx => $roles) {
+            if (in_array(Role::ROLE_ID_MANAGER, $roles, true)) {
+                $administratorManagerContexts[] = $ctx;
+            }
+        }
+
         if (empty($administratorManagerContexts)) {
             return self::ADMINISTRATION_PROHIBITED;
         }
-    
-        $administeredUserContexts = array_unique($administeredUserRoles['allContexts']);
+
+        $administeredUserContexts = array_keys($administeredMap);
         $conflictingContexts = array_diff($administeredUserContexts, $administratorManagerContexts);
-
-        if ($conflictingContexts) {
-            if ($contextId !== null && in_array($contextId, $administratorManagerContexts)) {
+        if (!empty($conflictingContexts)) {
+            if ($contextId !== null && in_array($contextId, $administratorManagerContexts, true)) {
                 return self::ADMINISTRATION_PARTIAL;
             }
             return self::ADMINISTRATION_PROHIBITED;
         }
-    
+
         return self::ADMINISTRATION_FULL;
     }
 

diff --git a/classes/submission/maps/Schema.php b/classes/submission/maps/Schema.php
@@ -551,9 +551,7 @@ protected function getPropertyReviewAssignments(Enumerable $reviewAssignments, b
             if ($reviewerId) {
                 $canLoginAs = \PKP\security\Validation::canUserLoginAs(
                     $reviewerId,
-                    $currentUser->getId(),
-                    // pass null for a site-wide check, or submission->getData('contextId') for a context check
-                    null
+                    $currentUser->getId()
                 );
             }
 
@@ -632,13 +630,11 @@ protected function getPropertyParticipants(Submission $submission): array
             return [];
         }
 
-        // collect all users assigned to this submission
-        // also can optionally pass a stageId if to get participants from a specific stage,
-        // but here getting them all.
+        // collect all users assigned to this submission. this retrieves users across all stages.
         $usersIterator = Repo::user()
             ->getCollector()
             ->filterByContextIds([$context->getId()])
-            ->assignedTo($submission->getId(), null /* or pass a stageId if desired */)
+            ->assignedTo($submission->getId())
             ->getMany();
 
         // build the array for each user

diff --git a/classes/user/maps/Schema.php b/classes/user/maps/Schema.php
@@ -320,10 +320,7 @@ protected function getPropertyCanMergeUsers(User $targetUser): bool
             return false;
         }
 
-        // ensure user administration permissions
-        $canAdminister = Validation::getAdministrationLevel($targetUser->getId(), $currentUser->getId()) === Validation::ADMINISTRATION_FULL;
-
-        // allow merging if the current user is a site admin or has full admin rights over the target user
-        return Validation::isSiteAdmin() || $canAdminister;
+        // check if the current user has full administration rights over the target user. it fully covers the site admin case.
+        return Validation::getAdministrationLevel($targetUser->getId(), $currentUser->getId()) === Validation::ADMINISTRATION_FULL;
     }
 }