IBX-9002: Added endpoint for list users with permission info #1372

ciastektk · 2024-10-17T08:30:31Z

🎫 Issue	IBX-9002

Description:

This PR adds internal endpoint to load list of users with their permissions to by resolved context. This is required by share dialog feature to search for users by given phrase e.g.: name, surname or email. Full list of results should be displayed, grouped by users with and without permissions.

A new endpoint is not exposed in REST API to avoid public access to list of users with permissions for not logged users. To make this feature useful we cannot filter search results by permissions. Otherwise we would have to change permission settings for users to allow read User content.

Example query params:

contentId={contentId} - (int) Content item for permission check
locationId={locationId} - (int) target Location for permission check
limit={limit} - (int) number of results
offset={offset} - (int) results offset
phrase={phrase} - (string) phrase using for search user by first name, last name or email.

###  GET /permission/users-with-permission-info/{module}/{function}

GET http://localhost/admin/permission/users-with-permission-info/content/read?contentId=43&locationId=51
Accept: application/json
X-Siteaccess: admin


GET http://localhost/admin/permission/users-with-permission-info/content/edit?contentId=43&locationId=51&phrase=user&limit=5
Accept: application/json
X-Siteaccess: admin

For QA:

Documentation:

Steveb-p

Current implementation seems to - correct me if I'm wrong - not distinguish between Admin UI users and "frontend" users.

I.e. you will get results with store front customer accounts, if they are created. They will also heavily pollute this list, probably making it hardly usable (if what I'm saying is true).

On a separate note: this makes any user that is able to log into the admin to discover all usernames and emails (as this is what your endpoint basically does). Given this is public and deals with sensitive information, I don't think this can be exposed like that.

Additionally, it should be enabled only if there are features that rely on it IMO. And I would even go as far as ask developer/admin to toggle availability of this endpoint on explicitly, maybe through bundle configuration.

To sum up, it's too exposed and sensitive to be enabled by default.

src/lib/User/Mapper/UsersWithPermissionInfoToContentItemMapper.php

ciastektk · 2024-10-18T08:13:03Z

Current implementation seems to - correct me if I'm wrong - not distinguish between Admin UI users and "frontend" users.

I.e. you will get results with store front customer accounts, if they are created. They will also heavily pollute this list, probably making it hardly usable (if what I'm saying is true).

On a separate note: this makes any user that is able to log into the admin to discover all usernames and emails (as this is what your endpoint basically does). Given this is public and deals with sensitive information, I don't think this can be exposed like that.

Additionally, it should be enabled only if there are features that rely on it IMO. And I would even go as far as ask developer/admin to toggle availability of this endpoint on explicitly, maybe through bundle configuration.

To sum up, it's too exposed and sensitive to be enabled by default.

Sorry, previous version was sent for review too early. I pushed new version including new Query based on new Criteria (the previous ones did not work for elasticsearch and solr) and now guest and corporate users are skipped.

About availability of this endpoint. I have same feelings like you, and access for sensitive informations worries me. I'd to discuss this internally.

sonarcloud · 2024-10-31T12:32:11Z

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

src/lib/QueryType/UserQueryType.php

tests/integration/REST/GetUsersWithPermissionInfoTest.php

adamwojs · 2024-12-09T07:32:05Z

src/lib/Permission/Mapper/UsersWithPermissionInfoMapper.php

+
+        $results = $this->groupByPermissions($searchResult, $permissionContext, $module, $function);
+
+        $this->permissionResolver->setCurrentUserReference($currentUserReference);


This instruction should be in catch { ... } finally { ... } block

adamwojs · 2024-12-09T07:34:02Z

src/lib/Permission/Mapper/UsersWithPermissionInfoMapper.php

+        foreach ($searchResult as $result) {
+            /** @var \Ibexa\Contracts\Core\Repository\Values\Content\ContentInfo $userContentInfo */
+            $userContentInfo = $result->valueObject;
+            $user = $this->userService->loadUser($userContentInfo->getId());


We are loading here user with permission previous user result. This is a bug.

adamwojs · 2024-12-09T07:35:05Z

src/lib/QueryType/UserQueryType.php

+
+    private function cleanSearchPhrase(string $phrase): string
+    {
+        $sanitizedPhrase = preg_replace('/[^a-zA-Z0-9@._-]/', '', $phrase);


Can we move regex to const ?

…Path

Co-authored-by: Konrad Oboza <[email protected]>

sonarcloud · 2024-12-09T11:04:47Z

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

ciastektk changed the title ~~Ibx 9002 endpoint for list users with permission info~~ IBX-9002: Added endpoint for list users with permission info Oct 17, 2024

github-actions bot assigned ciastektk, mikadamczyk and ViniTou Oct 17, 2024

ciastektk marked this pull request as ready for review October 17, 2024 10:36

ciastektk requested a review from a team October 17, 2024 10:37

ciastektk added Feature New feature request Ready for review labels Oct 17, 2024

Steveb-p reviewed Oct 17, 2024

View reviewed changes

src/lib/User/Mapper/UsersWithPermissionInfoToContentItemMapper.php Outdated Show resolved Hide resolved

ciastektk force-pushed the ibx-9002-endpoint-for-list-users-with-permission-info branch 2 times, most recently from 47b4139 to 3abf68b Compare October 18, 2024 07:37

ciastektk force-pushed the ibx-9002-endpoint-for-list-users-with-permission-info branch 2 times, most recently from b2dc741 to f936d48 Compare October 31, 2024 09:19

ciastektk requested review from a team, Steveb-p and adamwojs November 6, 2024 07:30

konradoboza reviewed Nov 18, 2024

View reviewed changes

src/lib/QueryType/UserQueryType.php Outdated Show resolved Hide resolved

src/lib/QueryType/UserQueryType.php Outdated Show resolved Hide resolved

tests/integration/REST/GetUsersWithPermissionInfoTest.php Show resolved Hide resolved

ciastektk requested a review from a team November 27, 2024 07:46

ciastektk force-pushed the ibx-9002-endpoint-for-list-users-with-permission-info branch from 0ea68b4 to bba7ec8 Compare November 27, 2024 08:09

ciastektk requested a review from konradoboza November 27, 2024 11:37

konradoboza approved these changes Nov 27, 2024

View reviewed changes

ciastektk requested a review from a team November 29, 2024 08:52

adamwojs reviewed Dec 9, 2024

View reviewed changes

ciastektk added 5 commits December 9, 2024 10:14

Added UsersWithPermissionInfoToContentItemController

0eaf312

Added route

5070767

Added UserQueryType

71ed221

Added UsersWithPermissionInfoToContentItemMapper

d068a8d

[DI] Added services definitions

431c56f

ciastektk and others added 20 commits December 9, 2024 10:14

[Tests] Added integration test

97a90ad

Renamed method getUserRegistrationGroupId to getUserRegistrationGroup…

eb332ce

…Path

Fixed phpstan-type TUserData

f01113c

Reworked UserQueryType

b2e5c6c

Added PermissionCheckContextResolver

a8b831e

Added PermissionCheckContextProviderInterface

1f0af04

Added ContentItemContextProvider

8deb170

Reworked UsersWithPermissionInfoController

39c9439

Reworked UsersWithPermissionInfoMapper

e8ece01

[Tests] Reworked GetUsersWithPermissionInfoTest

fc7aa49

[DI] Added services definitions

dd78649

Added strict types declaration

5fe0a7c

Moved PermissionCheckContext to contracts

9dae622

Reoreder variables in UsersWithPermissionInforMapper::groupByPermissions

7c6cf5c

Update src/lib/QueryType/UserQueryType.php

323efd0

Co-authored-by: Konrad Oboza <[email protected]>

Dropped UserQueryType::excludeSystemUsers method

2697d27

[Tests] Fixed test

1b2978e

[Routing] Set option expose

4f3281e

Moved user search phrase pattern to const

738590b

Fixed UsersWithPermissionInfoMapper

fc53371

ciastektk force-pushed the ibx-9002-endpoint-for-list-users-with-permission-info branch from 57c7735 to fc53371 Compare December 9, 2024 09:14

ciastektk requested review from adamwojs and a team December 9, 2024 09:15

Renamed no_access key to noAccess

b48f8c9

ciastektk added Ready for QA and removed Ready for review labels Dec 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

IBX-9002: Added endpoint for list users with permission info #1372

IBX-9002: Added endpoint for list users with permission info #1372

ciastektk commented Oct 17, 2024 •

edited

Loading

Steveb-p left a comment

ciastektk commented Oct 18, 2024

sonarcloud bot commented Oct 31, 2024

adamwojs Dec 9, 2024

ciastektk Dec 9, 2024

adamwojs Dec 9, 2024

ciastektk Dec 9, 2024

adamwojs Dec 9, 2024

ciastektk Dec 9, 2024

sonarcloud bot commented Dec 9, 2024


		$results = $this->groupByPermissions($searchResult, $permissionContext, $module, $function);

		$this->permissionResolver->setCurrentUserReference($currentUserReference);

IBX-9002: Added endpoint for list users with permission info #1372

Are you sure you want to change the base?

IBX-9002: Added endpoint for list users with permission info #1372

Conversation

ciastektk commented Oct 17, 2024 • edited Loading

Description:

For QA:

Documentation:

Steveb-p left a comment

Choose a reason for hiding this comment

ciastektk commented Oct 18, 2024

sonarcloud bot commented Oct 31, 2024

Quality Gate passed

adamwojs Dec 9, 2024

Choose a reason for hiding this comment

ciastektk Dec 9, 2024

Choose a reason for hiding this comment

adamwojs Dec 9, 2024

Choose a reason for hiding this comment

ciastektk Dec 9, 2024

Choose a reason for hiding this comment

adamwojs Dec 9, 2024

Choose a reason for hiding this comment

ciastektk Dec 9, 2024

Choose a reason for hiding this comment

sonarcloud bot commented Dec 9, 2024

Quality Gate passed

ciastektk commented Oct 17, 2024 •

edited

Loading