Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pkp/pkp-lib#10637 Added canLoginAs & canMergeUsers to users endpoint #10658

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
116 changes: 112 additions & 4 deletions api/v1/users/PKPUserController.php
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
use Illuminate\Support\Facades\Route;
use Illuminate\Support\LazyCollection;
use PKP\context\Context;
use PKP\core\PKPApplication;
use PKP\core\PKPBaseController;
use PKP\core\PKPRequest;
use PKP\facades\Locale;
Expand Down Expand Up @@ -116,6 +117,93 @@ public function get(Request $request): JsonResponse
return response()->json(Repo::user()->getSchemaMap()->map($user), Response::HTTP_OK);
}

protected function getUserPermissions(array $userIds, Request $request): array
{
$currentUser = $request->user();
$currentUserId = $currentUser->getId();

$permissions = [];

// check if the current user is a Site Admin
$isSiteAdmin = $currentUser->hasRole([Role::ROLE_ID_SITE_ADMIN], PKPApplication::SITE_CONTEXT_ID);

// fetch all user group assignments for current user and target users
$allUserIds = array_merge($userIds, [$currentUserId]);
$userGroupAssignments = Repo::userGroup()->getCollector()
->filterByUserIds($allUserIds)
->getMany()
->groupBy(function ($item) {
return $item->getData('userId');
});
Comment on lines +135 to +137
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?


// get context ids where the current user is a Manager
$managerContextIds = [];
if (!$isSiteAdmin) {
$currentUserGroups = $userGroupAssignments->get($currentUserId, collect());
foreach ($currentUserGroups as $userGroup) {
if ($userGroup->getRoleId() === Role::ROLE_ID_MANAGER) {
$managerContextIds[] = $userGroup->getContextId();
}
}
}

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());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it work as it's intended to ? If I am not mistaken , this will always return empty collection . What I am missing ?


$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;
}
}
Comment on lines +150 to +193
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 .


if ($canAdminister) {
$permissions[$userId]['canLoginAs'] = true;
$permissions[$userId]['canMergeUser'] = true;
}
}

return $permissions;
}




/**
* Get a collection of users
*
Expand All @@ -125,6 +213,8 @@ public function getMany(Request $request): JsonResponse
{
$context = $request->attributes->get('context'); /** @var Context $context */

$includePermissions = $request->query('includePermissions', false);

$params = $this->_processAllowedParams($request->query(null), [
'assignedToCategory',
'assignedToSection',
Expand Down Expand Up @@ -180,14 +270,32 @@ public function getMany(Request $request): JsonResponse
$users = $collector->getMany();

$map = Repo::user()->getSchemaMap();
$items = [];
$usersArray = [];
$userIds = [];

foreach ($users as $user) {
$items[] = $map->summarize($user);
$userData = $map->summarize($user);
$usersArray[] = $userData;

if ($includePermissions) {
$userIds[] = $user->getId();
}
}


// add permissions if requested
if ($includePermissions && !empty($userIds)) {
$permissions = $this->getUserPermissions($userIds, $request);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?


foreach ($usersArray as &$userData) {
$userId = $userData['id'];
$userData['canLoginAs'] = $permissions[$userId]['canLoginAs'] ?? false;
$userData['canMergeUser'] = $permissions[$userId]['canMergeUser'] ?? false;
}
}

return response()->json([
'itemsMax' => $collector->getCount(),
'items' => $items,
'items' => $usersArray,
], Response::HTTP_OK);
}

Expand Down