pkp/pkp-lib#10290 whether current user can log in as reviewer/participant #10477

Hafsa-Naeem · 2024-09-26T21:38:45Z

CLAassistant · 2024-09-26T21:38:52Z

All committers have signed the CLA.

Vitaliy-1

Thanks! Looks good. I left 2 comments.

Vitaliy-1 · 2024-11-25T16:28:48Z

classes/security/Validation.php

+
+        if ($unmanagedContextsExist) {
+            // check if partial administration is allowed in the current context
+            $isManagerInCurrentContext = $contextId !== null &&


Have you tested this? in my case $contextId is null here. Steps to debug:

Use test dataset

Create a second journal

Assign a user with a sub editor role another role in the second journal

Go to settings -> users in the first journal by a user that has only manager role in that journal.

@Vitaliy-1 what is the concern here ? I tested it and seems works fine . So I have create a new Editor and Reviewer and also assigned to a second journal as Editor and Reviewer . Then from the first journal, As JM only assigned to first journal , logged in and they are visible with following restriction

Can not disable as these 2 user has association to multi context .

can only edit the role assignment for the first journal as JM is only associated with the first journal .

I am bit confused about the part in my case $contextId is null here . Also I did not test with the fresh datasets .
@Hafsa-Naeem can you test this behaviour with importing the latest datasets ?

Thanks, @touhidurabir. What is current business logic behind login as and partial administration? Who can administer whom and what additional actions could/couldn't be done, e.g., changing the role, merging users... Is there a good description somewhere?

@Vitaliy-1 please see this #10477 (comment) .

Vitaliy-1 · 2024-11-25T16:29:30Z

classes/security/Validation.php

+            ->filterByContextIds([\PKP\core\PKPApplication::SITE_CONTEXT_ID])
+            ->filterByRoleIds([Role::ROLE_ID_SITE_ADMIN])
+            ->filterByUserIds([$administratorUserId])
+            ->getCount() > 0;


Can we use here and in other places exists?

@Hafsa-Naeem I agree with @Vitaliy-1 . As you are refactoring this method implementation , we can replace all of the getCount() with exists as following manner

Repo::userGroup() ->getCollector() ->filterByContextIds([\PKP\core\PKPApplication::SITE_CONTEXT_ID]) ->filterByRoleIds([Role::ROLE_ID_SITE_ADMIN]) ->filterByUserIds([$administeredUserId]) ->getQueryBuilder() ->exists();

exists is much more performant that getCount or count as when querying if SQL records using exists instead of using count. exists is much more efficient and breaks out of the loop when first record is found.

Vitaliy-1 · 2024-11-25T16:37:49Z

@touhidurabir, you are probably the most familiar with this new code regarding login as and the meaning behind partial administration in getAdministrationLevel. Can you also review this PR?

touhidurabir

@Hafsa-Naeem I have added few reviews , please see those . But most importantly, the proposed changes for Login As in this PR, I am not sure if that the right behaviour we want to have .

Also I think what we really need is someway to let the frontend now if the current user can do the login as for few given users in reviewers grid or stage participants grid. I think we should aim to provide those details via API to allow frontend to show the right options .

touhidurabir · 2024-11-27T06:35:35Z

classes/security/Validation.php

+            ->filterByContextIds([\PKP\core\PKPApplication::SITE_CONTEXT_ID])
+            ->filterByRoleIds([Role::ROLE_ID_SITE_ADMIN])
+            ->filterByUserIds([$administratorUserId])
+            ->getCount() > 0;


@Hafsa-Naeem I agree with @Vitaliy-1 . As you are refactoring this method implementation , we can replace all of the getCount() with exists as following manner

Repo::userGroup() ->getCollector() ->filterByContextIds([\PKP\core\PKPApplication::SITE_CONTEXT_ID]) ->filterByRoleIds([Role::ROLE_ID_SITE_ADMIN]) ->filterByUserIds([$administeredUserId]) ->getQueryBuilder() ->exists();

exists is much more performant that getCount or count as when querying if SQL records using exists instead of using count. exists is much more efficient and breaks out of the loop when first record is found.

touhidurabir · 2024-11-27T07:05:54Z

controllers/grid/settings/user/UserGridRow.php

+                    (
+                        Validation::isSiteAdmin() ||
+                        $currentUser->hasRole([Role::ROLE_ID_MANAGER], $contextId)
+                    )


@Hafsa-Naeem as you sure this is working as it's intended to ? In my case it does not allow me logged in as that user .

Also I think this is kind if dangerous as it's allow a user form a context to logged in as another user who has role in multiple context which goes against the logic of \PKP\security\Validation::getAdministrationLevel() . So I am really concern about this change .

@asmecher @Vitaliy-1 your thought on this ?

Specially @asmecher , I am pretty sure that when I worked on #7391, we both agreed that except for a Role::ROLE_ID_SITE_ADMIN , one with only role in particular context can not do the following

disable

edit user information except the role management to current journal

login as

when the target user has role associated with multiple context .

@Hafsa-Naeem after some discussion on this with @asmecher , I think this is dangerous and it will break the user administration restriction when a user as association in multiple context .

I think we should allow to pass this information via API by deciding if the user can administrate the target users by using query like what has been proposed by @asmecher at #10290 (comment) . The API can accept an optional param as to decide if response need this details and enable front end to show the proper options based on the response .

touhidurabir · 2024-11-27T07:12:57Z

controllers/grid/users/reviewer/ReviewerGridRow.php

+                (
+                    Validation::isSiteAdmin() ||
+                    $currentUser->hasRole([Role::ROLE_ID_MANAGER], $contextId)
+                ) &&


The same concern as above

touhidurabir · 2024-11-27T07:14:01Z

controllers/grid/users/stageParticipant/StageParticipantGridRow.php

+                (
+                    Validation::isSiteAdmin() ||
+                    $user->hasRole([Role::ROLE_ID_MANAGER], $contextId)
+                )


Again , the same concern as above as a JM only role in one context can logged in as another user who as role association in multiple context .

touhidurabir · 2024-11-27T08:42:20Z

pages/login/LoginHandler.php

@@ -413,7 +413,7 @@ public function signInAsUser($args, $request)
                $templateMgr->assign([
                    'pageTitle' => 'manager.people',
                    'errorMsg' => 'manager.people.noAdministrativeRights',
-                    'backLink' => $request->url(null, null, 'people', 'all'),
+                    'backLink' => $request->url(null, null, 'people', ['all']),


I think backLink url here is wrong as it's generate the url as BASE_URL/index.php/publicknowledge/en/login/people/all . It's not that important but if we want to show a link to get back to all enrolled users , it should be something like BASE_URL/index.php/publicknowledge/en/management/settings/access .

I see that we currently have $request->url(null, null, 'people', 'all') which is also wrong . However this login as can happens a few places and we need to construct the backLink of that page from where the Login As action initiated .

touhidurabir

@Hafsa-Naeem I have added 2 comments . Please those .

However, I am not sure if this enough to accommodate the requirement of what the issue describe . If I am not mistaken, currently frontend can not determine from the API response if the current logged in user has certain permissions (e.g. log in as , merge user etc) to perform actions on a user if has multiple context association . We need some way to pass that optional information with response at certain end points only when necessary for frontend to make the decisions to show those options .

touhidurabir · 2024-12-05T09:41:59Z

classes/security/Validation.php

                Repo::userGroup()
                    ->getCollector()
                    ->filterByContextIds([$contextId])
                    ->filterByUserIds([$administratorUserId])
                    ->filterByRoleIds([Role::ROLE_ID_MANAGER])
-                    ->getCount()) {
+                    ->getCount() > 0;


Here can we also use exists ?

touhidurabir · 2024-12-05T09:57:45Z

pages/login/LoginHandler.php

@@ -413,7 +413,7 @@ public function signInAsUser($args, $request)
                $templateMgr->assign([
                    'pageTitle' => 'manager.people',
                    'errorMsg' => 'manager.people.noAdministrativeRights',
-                    'backLink' => $request->url(null, null, 'people', 'all'),
+                    'backLink' => $request->url(null, 'management', 'settings', ['access']),


This is ok but ideally if we can, we would like to redirect back to url where where it's redirected here . so for example, this login in as can also happens from a submission workflow page (like dashboard/editorial?currentViewId=active&workflowSubmissionId=78) and the back link should take back to there . Here we are setting the back link always to user's management access page . Though I am not sure if this is possible with our current page routing infrastructure .

touhidurabir · 2024-12-06T11:44:38Z

controllers/grid/settings/user/UserGridRow.php

+                $currentUser = $request->getUser();
+                $targetUserId = $this->getId();
+                $context = $request->getContext();
+                $contextId = $context ? $context->getId() : null; // Use null if context is not available


instead of null, maybe we should use Application::SITE_CONTEXT_ID

touhidurabir · 2024-12-06T11:47:08Z

controllers/grid/users/reviewer/ReviewerGridRow.php

+
+            // Get the context (journal) to check for the Manager role
+            $context = $request->getContext();
+            $contextId = $context ? $context->getId() : 0;


instead of 0, we should use Application::SITE_CONTEXT_ID that point to a site as there can be no context with ID 0 .

touhidurabir · 2024-12-06T11:47:31Z

controllers/grid/users/stageParticipant/StageParticipantGridRow.php

@@ -109,10 +109,13 @@ public function initialize($request, $template = null)
            $this->addAction(new NotifyLinkAction($request, $submission, $stageId, $userId));

            $user = $request->getUser();
+            $contextId = $context ? $context->getId() : 0; // 0 in case context is not available


same as https://github.com/pkp/pkp-lib/pull/10477/files#r1873173826

Hafsa-Naeem · 2024-12-06T15:48:20Z

@touhidurabir I am also working on this issue#10637. I think it's related to your comment. Could you please have a look at this PR too?
PR: pkp/pkp-lib#10637 Added canLoginAs & canMergeUsers to users endpoint #10658

Vitaliy-1

Thanks! I left just minor comments

Vitaliy-1 · 2025-02-06T13:59:34Z

classes/security/Validation.php

-            ->exists();
+        foreach ($allUserGroups as $userGroup) {
+            $roleId = $userGroup->roleId;
+            $ctxId = $userGroup->contextId ?? 0;


Let's name it $userGroupContextId.

Also, what does 0 mean here? Should it be PKPApplication::SITE_CONTEXT_ID?

Vitaliy-1 · 2025-02-06T14:01:09Z

classes/security/Validation.php

-                      ->withActive();
+        // single query to fetch user groups assigned to either user
+        $allUserGroups = UserGroup::query()
+            ->whereHas('userUserGroups', function ($q) use ($administratorUserId, $administeredUserId) {


We can replace this anonymous function and the one below with a shorthand fn() => ...

Vitaliy-1 · 2025-02-06T14:03:35Z

classes/security/Validation.php

+        $adminUserRoles = [
+            'siteAdmin' => false,
+            'managerContexts' => [],
+            'anyRoleContexts' => [],


It isn't used anywhere

Vitaliy-1 · 2025-02-06T15:34:52Z

classes/security/Validation.php


-        if ($isAdministratorUserSiteAdmin) {
+            // then each user assignment row
+            foreach ($userGroup->userUserGroups as $uug) {


I think we can do it shorter. Let's say we define 2 arrays:

/* * [ * context ID => $roles * ] */ $administeredUserRoles = []; $administratorUserRoles = []; foreach ($allUserGroups as $userGroup) { foreach ($userGroup->userUserGroups as $uug) { if ($uug->userId === $administratorUserId) { $administratorUserRoles[inval($userGroup->contextId)][] => $userGroup->roleId } elseif ($uug->userId === $administeredUserId) { $administeredUserRoles[inval($userGroup->contextId)][] => $userGroup->roleId } } } // check if administered user is an admin // check if administrator user is an admin // check if administrator user is a manager and conflicting contexts

Vitaliy-1 · 2025-02-07T07:26:35Z

classes/submission/maps/Schema.php

+                    $reviewerId,
+                    $currentUser->getId(),
+                    // pass null for a site-wide check, or submission->getData('contextId') for a context check
+                    null


Null is passed as a default argument here

Vitaliy-1 · 2025-02-07T07:34:44Z

classes/submission/maps/Schema.php

+            return [];
+        }
+
+        // collect all users assigned to this submission


Multiline comment

/* * */

Vitaliy-1 · 2025-02-07T07:35:59Z

classes/submission/maps/Schema.php

+        $usersIterator = Repo::user()
+            ->getCollector()
+            ->filterByContextIds([$context->getId()])
+            ->assignedTo($submission->getId(), null /* or pass a stageId if desired */)


Null is passed by default as a second argument

one-line comment

//

Vitaliy-1 · 2025-02-07T07:44:03Z

classes/user/maps/Schema.php

+        $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;


Should site admin role be already covered by the Validation::getAdministrationLevel() check?

Vitaliy-1 · 2025-02-07T07:45:42Z

classes/security/Validation.php

-    {
-        // You can administer yourself
-        if ($administeredUserId == $administratorUserId) {
+    public static function getAdministrationLevel(


Can you check if indent on empty lines in your environment is the same as in the project?

…anGossip, canMergeUser to schemas for a consistent API response

Hafsa-Naeem requested a review from Vitaliy-1 September 26, 2024 21:38

Hafsa-Naeem mentioned this pull request Sep 27, 2024

Detect on the frontend whether current user can "log in as" reviewer/participant #10290

Closed

1 task

Vitaliy-1 requested changes Nov 25, 2024

View reviewed changes

Vitaliy-1 requested a review from touhidurabir November 25, 2024 16:37

touhidurabir requested changes Nov 27, 2024

View reviewed changes

touhidurabir requested changes Dec 5, 2024

View reviewed changes

touhidurabir reviewed Dec 6, 2024

View reviewed changes

Hafsa-Naeem force-pushed the i10290_login_as_reviewers_participants branch 2 times, most recently from fb2dce8 to 2f83076 Compare January 22, 2025 00:32

Hafsa-Naeem force-pushed the i10290_login_as_reviewers_participants branch 3 times, most recently from 13ba70f to 1f1962a Compare February 5, 2025 13:30

Vitaliy-1 requested changes Feb 7, 2025

View reviewed changes

Hafsa-Naeem force-pushed the i10290_login_as_reviewers_participants branch from 1f1962a to daecb7b Compare February 10, 2025 04:07

Vitaliy-1 marked this pull request as ready for review February 13, 2025 14:51

Hafsa-Naeem force-pushed the i10290_login_as_reviewers_participants branch from 1b23cb3 to a58446c Compare February 13, 2025 15:36

Hafsa-Naeem added 2 commits February 14, 2025 15:04

pkp#10290 Optimized queries using eager loading & added canLoginAs, c…

67ffa3e

…anGossip, canMergeUser to schemas for a consistent API response

pkp#10290 Fixed comments and context handling

e8cfd4c

Hafsa-Naeem force-pushed the i10290_login_as_reviewers_participants branch from a58446c to e8cfd4c Compare February 14, 2025 10:06

Vitaliy-1 merged commit 2afbf4a into pkp:main Feb 14, 2025
2 checks passed

Hafsa-Naeem mentioned this pull request Feb 20, 2025

Add canLoginAs & canMergeUsers to /users endpoint #10637

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pkp/pkp-lib#10290 whether current user can log in as reviewer/participant #10477

pkp/pkp-lib#10290 whether current user can log in as reviewer/participant #10477

Hafsa-Naeem commented Sep 26, 2024

CLAassistant commented Sep 26, 2024 •

edited

Loading

Vitaliy-1 left a comment

Vitaliy-1 Nov 25, 2024

touhidurabir Nov 27, 2024

Vitaliy-1 Nov 27, 2024

touhidurabir Nov 27, 2024

Vitaliy-1 Nov 25, 2024

touhidurabir Nov 27, 2024

Vitaliy-1 commented Nov 25, 2024

touhidurabir left a comment

touhidurabir Nov 27, 2024

touhidurabir Nov 27, 2024

touhidurabir Nov 29, 2024

touhidurabir Nov 27, 2024

touhidurabir Nov 27, 2024

touhidurabir Nov 27, 2024 •

edited

Loading

touhidurabir left a comment

touhidurabir Dec 5, 2024

touhidurabir Dec 5, 2024

touhidurabir Dec 6, 2024

touhidurabir Dec 6, 2024

touhidurabir Dec 6, 2024

Hafsa-Naeem commented Dec 6, 2024 •

edited

Loading

Vitaliy-1 left a comment

Vitaliy-1 Feb 6, 2025

Vitaliy-1 Feb 6, 2025

Vitaliy-1 Feb 6, 2025

Vitaliy-1 Feb 6, 2025

Vitaliy-1 Feb 7, 2025

Vitaliy-1 Feb 7, 2025

Vitaliy-1 Feb 7, 2025

Vitaliy-1 Feb 7, 2025

Vitaliy-1 Feb 7, 2025

pkp/pkp-lib#10290 whether current user can log in as reviewer/participant #10477

pkp/pkp-lib#10290 whether current user can log in as reviewer/participant #10477

Conversation

Hafsa-Naeem commented Sep 26, 2024

CLAassistant commented Sep 26, 2024 • edited Loading

Vitaliy-1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vitaliy-1 commented Nov 25, 2024

touhidurabir left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

touhidurabir Nov 27, 2024 • edited Loading

Choose a reason for hiding this comment

touhidurabir left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hafsa-Naeem commented Dec 6, 2024 • edited Loading

Vitaliy-1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CLAassistant commented Sep 26, 2024 •

edited

Loading

touhidurabir Nov 27, 2024 •

edited

Loading

Hafsa-Naeem commented Dec 6, 2024 •

edited

Loading