pkp/pkp-lib#10637 Added canLoginAs & canMergeUsers to users endpoint #10658
Conversation
| ->groupBy(function ($item) { | ||
| return $item->getData('userId'); | ||
| }); |
There was a problem hiding this comment.
I am not sure if this work as intended. userGroup has no direct association with user id and does not seems like there is a direct way to get user id from PKP\userGroup\UserGroup
There was a problem hiding this comment.
The association between UserGroup and users is managed via the user_user_groups table. The Collector class explicitly joins this table when filterByUserIds is called, establishing the link indirectly. This design eliminates the need to directly store user IDs in the UserGroup class for functionality to work as intended
Also, currently the PR to port UserGroup to Eloquent is under process which, once merged will make this relationship more explicit and clear.
There was a problem hiding this comment.
The association between UserGroup and users is managed via the user_user_groups table. The Collector class explicitly joins this table when filterByUserIds is called, establishing the link indirectly.
this is correct but in
groupBy(function ($item) {
return $item->getData('userId');
})the $item is an instance of PKP\userGroup\UserGroup and calling getData on that object will give us only those information that has direct association . I ran a test a such
Repo::userGroup()
->getCollector()
->filterByUserIds($allUserIds)
// ->getQueryBuilder()
// ->get();
->getMany()
->groupBy(function (UserGroup $item) {
dump($item);
dump($item->getData("userId"));
return $item->getData("userId");
})
->all();where the first dump dump($item) gives an object of PKP\userGroup\UserGroup and second dump dump($item->getData("userId")) gives null .
Or I am misunderstanding something ?
| } | ||
|
|
||
| // check if the current user has full administrative rights over the target user | ||
| $targetUserGroups = $userGroupAssignments->get($userId, collect()); |
There was a problem hiding this comment.
Does it work as it's intended to ? If I am not mistaken , this will always return empty collection . What I am missing ?
| foreach ($userIds as $userId) { | ||
| $permissions[$userId] = [ | ||
| 'canLoginAs' => false, | ||
| 'canMergeUser' => false, | ||
| ]; | ||
|
|
||
| // skip if the user is the current user | ||
| if ($userId === $currentUserId) { | ||
| continue; | ||
| } | ||
|
|
||
| // skip if logged in as another user | ||
| if (\PKP\security\Validation::loggedInAs()) { | ||
| continue; | ||
| } | ||
|
|
||
| // site Admin can administer all users | ||
| if ($isSiteAdmin) { | ||
| $permissions[$userId]['canLoginAs'] = true; | ||
| $permissions[$userId]['canMergeUser'] = true; | ||
| continue; | ||
| } | ||
|
|
||
| // check if the current user has full administrative rights over the target user | ||
| $targetUserGroups = $userGroupAssignments->get($userId, collect()); | ||
|
|
||
| $canAdminister = true; | ||
|
|
||
| foreach ($targetUserGroups as $userGroup) { | ||
| $roleId = $userGroup->getRoleId(); | ||
| $contextId = $userGroup->getContextId(); | ||
|
|
||
| // if the target user is a Site Admin | ||
| if ($roleId === Role::ROLE_ID_SITE_ADMIN) { | ||
| $canAdminister = false; | ||
| break; | ||
| } | ||
|
|
||
| // if the target user has roles in contexts the current user does not manage | ||
| if (!in_array($contextId, $managerContextIds)) { | ||
| $canAdminister = false; | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
Is this block consider all the cases implemented in \PKP\security\Validation::getAdministrationLevel ? It does seems so but seems missing one check to see if the requesting user has any administering role as we have at https://github.com/pkp/pkp-lib/blob/main/classes/security/Validation.php#L473-L482 . Just need some double checking .
|
|
||
| // add permissions if requested | ||
| if ($includePermissions && !empty($userIds)) { | ||
| $permissions = $this->getUserPermissions($userIds, $request); |
There was a problem hiding this comment.
Here instead of having a separate method which kind of similar to PKP\security\Validation::getAdministrationLevel , can we use the same one (with some modification if required) to maintain the logic and implementation from a single place ?
For #10637