#37070 #37071: feat(roles): add GET /v1/roles/{roleid}/users and childCount/userCount on RoleView - #37078
Open
hassandotcms wants to merge 15 commits into
Open
#37070 #37071: feat(roles): add GET /v1/roles/{roleid}/users and childCount/userCount on RoleView#37078hassandotcms wants to merge 15 commits into
hassandotcms wants to merge 15 commits into
Conversation
Adds two int fields to RoleView so the roles tree can render the folder/leaf icon and the user-count badge without lazy expands: - childCount derives from the already-loaded Role.roleChildren (null treated as 0), zero extra queries - userCount resolves through the new RoleAPI.countUsersByRoleIds aggregate: one grouped users_cms_roles query per response, chunked IN lists, direct grants only (inherited grants excluded) - RoleHelper.toRoleViews collects all role ids (parents plus hydrated children), runs the single count query, then builds the views; loadRootRoles, loadRoleByRoleId and loadUserRoles now route through it - counts are computed per response and never stored on cached Role objects Covered by RoleResourceCountsIntegrationTest (8 tests, registered in MainSuite2b).
…e_key (#37070) UserFactoryImpl.appendRoleFilter bound role.getRoleKey() into a role_key IN (...) subquery, so a role without a roleKey bound null and matched no users. This blocked the upcoming GET /v1/roles/{roleId}/users endpoint for keyless roles. The EXISTS subquery now matches ur.role_id IN (...) binding the role id (dropping the cms_role join), shared by getUsersByName and getCountUsersByName. Ids are resolved defensively: a hand-built Role carrying only a roleKey is resolved by key so external UserAPI callers keep working. Behavior preservation is demonstrated, not assumed: the five regression tests in UserRoleFilterRegressionIntegrationTest (roleKey filtering on /v1/users/filter, multi-key union, list/count consistency, the back-end user role param loginAsData passes, and key-only Role objects) were written and green against the previous SQL, then carried unchanged across this change. The keyless-role test was red before and is green now.
…d users (#37070) New endpoint returning the paginated list of users directly granted a role, keyed by role id, using the standard user serialization with emailAddress. Closes the gap where the Users tab showed empty emails for roles without a roleKey (the FE had to combine /v1/users/filter?roleKey= and rolehierarchyanduserroles, which returns roles instead of users). - reuses UserPaginator through the non-deprecated PaginationUtil.getPageView (ResponseEntityPaginatedDataView) - direct grants only; inheritance stays a client concern via the ancestor walk on RoleView.parent - user-roles served uniformly (self-grant row) - gated behind requiredBackendUser plus the roles portlet since the response carries user PII - 404 via DoesNotExistException for missing roles - openapi.yaml regenerated (additive: new path plus required childCount/userCount on RoleView from the previous commit) Covered by RoleResourceUsersIntegrationTest (8 tests, registered in MainSuite2b).
…es (#37070) Dissolves UserRoleFilterRegressionIntegrationTest into the classes that already own these concerns: - API-level tests (keyless role, key-only Role object) move next to the existing testGetUsersByNameFilteredByRole in UserAPITest, following its javadoc and try/finally cleanup style - REST-level tests (filter by roleKey, multi-key union) move into UserResourceIntegrationTest - two tests are dropped as duplicates of the already existing testGetUsersByNameFilteredByRole coverage (roles-param count consistency and back-end role matching) All relocated tests plus the pre-existing role-filter test ran green against the role_id-based filter.
Two pre-review findings: - RoleHelper.toRoleViews now skips (with a warning) a child role id that no longer resolves instead of passing null into the RoleView constructor and throwing an NPE; the same latent NPE existed in the old inline loops - UserFactoryImpl.resolveRoleId now propagates a DotDataException from the key-to-id lookup as DotRuntimeException instead of swallowing it; a role that simply does not resolve still matches no users, but an infrastructure failure no longer masquerades as an empty result All 23 affected integration tests re-ran green.
…dpoint
Aligns GET /{roleid}/users with the existing GET /{roleid}, PUT
/{roleid} (PR #37012, where review feedback settled on lowercase) and
the membership endpoints POST /{roleid}/users/{userId} and DELETE
/{roleid}/users (PR #37077). Without this, the regenerated openapi.yaml
would end up with /v1/roles/{roleId}/users and /v1/roles/{roleid}/users
as two entries for the same real path once both branches merge, which
OpenAPI treats as ambiguous.
Template rename only: real URLs, clients and tests are unaffected.
openapi.yaml regenerated.
Contributor
|
Claude finished @hassandotcms's task in 2m 48s —— View job Claude Code Review
I reviewed the diff against New IssuesNo issues found. The implementation is clean and well-tested. Notes from verification:
Resolved
Other
Overall: no blocking issues. |
![semgrep-dotcms[bot]](https://avatars.githubusercontent.com/u/1005263?s=60&v=4){kind=small}
Contributor
|
Semgrep found 2
The method identified is susceptible to injection. The input should be validated and properly If this is a critical or high severity finding, please also link this issue in the #security channel in Slack. |
PR review found that countUsersByRoleIds counted raw users_cms_roles
rows while GET /v1/roles/{roleid}/users filters through
UserFactoryImpl.getUsersByName, which hides the system and anonymous
users, the default user, and users flagged delete_in_progress. A role
granted to any of those reported a badge count higher than the Users
tab total.
The count query now joins user_ and applies the same visibility rules
as the listing, so RoleView.userCount always equals the listing total.
Covered by a red-first integration test that grants a role to a visible
user, the system user and a delete_in_progress user, asserts the count
is 1, and asserts it equals the endpoint's totalEntries. Javadoc,
Schema description and the regenerated openapi.yaml updated to match.
9 tasks
Contributor
|
Semgrep found 8
The method identified is susceptible to injection. The input should be validated and properly If this is a critical or high severity finding, please also link this issue in the #security channel in Slack. |
Auditing the SQL surface showed the endpoint's orderBy and direction parameters were silently ignored: UserPaginator reads ordering from the FilteringParams keys (orderby, orderdirection), not from the PaginationUtil orderBy/direction arguments, and the endpoint never set those keys. Results always came back full-name ascending. The endpoint now passes both through extraParams, the same wiring /v1/users/filter uses for orderBy. The direction value stays enum-gated (OrderDirection.valueOf) and maps to the SQLUtil._ASC/_DESC constants FilteringParams expects. orderBy remains subject to the SQLUtil.sanitizeSortBy whitelist downstream. Covered by a red-first integration test asserting DESC returns the exact reverse of ASC.
PR review asked whether roles returned by loadRolesForUser carry a hydrated roleChildren list, since childCount would silently report 0 otherwise. Code-wise they do (every role goes through getRoleById, which runs populatChildrenForRoles), and this assertion pins that down: the granted role in loadUserRoles_carriesCounts now has a child and the view must report childCount 1.
Member
Author
False positive: the concatenated parts are only |
…sers-endpoint-and-counts # Conflicts: # dotCMS/src/main/java/com/dotcms/rest/api/v1/system/role/RoleHelper.java
…sers-endpoint-and-counts # Conflicts: # dotCMS/src/main/java/com/dotcms/rest/api/v1/system/role/RoleHelper.java # dotCMS/src/main/webapp/WEB-INF/openapi/openapi.yaml
hassandotcms
marked this pull request as ready for review
August 19, 2026 07:40
Executable evidence for triaging the Semgrep CUSTOM_INJECTION-2 findings as false positives: the test passes stacked-statement, tautology, UNION and quote-breaking payloads through the roleIds collection and asserts they match nothing, throw nothing, and leave the tables intact while a legitimate id in the same call still counts correctly. The IN-list is size-derived placeholders only and every id is bound as a PreparedStatement parameter.
dario-daza
reviewed
Aug 24, 2026
dario-daza
approved these changes
Aug 24, 2026
Review feedback on the PR asked for the same datagen cleanup UserAPITest uses. A literal per-test try/finally turned out to destabilize the REST tests: every role deletion clears the global role cache (user deletion cascades through the user's own role too), and WebResource.checkRolePermissions swallows any transient exception from doesUserHaveRole into 'lacks role', so deletions interleaved between REST calls intermittently produced spurious 401s (reproduced deterministically in RoleResourceCountsIntegrationTest). Created roles and users are therefore tracked per class and removed once in @afterclass: the suite is left just as clean, with no deletions between REST calls. Children are removed before parents. All three classes pass first-run with no retries.
…sers-endpoint-and-counts # Conflicts: # dotcms-integration/src/test/java/com/dotcms/rest/api/v1/user/UserResourceIntegrationTest.java
hassandotcms
enabled auto-merge
August 24, 2026 15:30
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Backend for the Angular Roles and Tools portlet (epic #36909):
GET /v1/roles/{roleid}/users([Task] Roles API: addGET /v1/roles/{roleId}/usersreturning the list of directly-granted users #37070): paginated list of users directly granted a role, standard user maps withemailAddress. Direct grants only; inheritance stays a clientconcern via the ancestor walk on
RoleView.parent. Gated by backend user + roles portlet (response carries PII). 404 on missing role.childCountanduserCountonRoleView([Task] Roles API: exposechildCountanduserCountonRoleViewfor scalable tree rendering #37071): present on the three endpoints that return this DTO (GET /v1/roles,GET /v1/roles/{roleid},GET /v1/roles/users/{userIdOrEmail}).childCountderives from the already-loadedRole.roleChildren(no extra queries).userCountresolves through the newRoleAPI.countUsersByRoleIds: one groupedusers_cms_rolesquery per response, chunked IN lists, never stored on cachedRoleobjects.Prerequisite fix
UserFactoryImpl.appendRoleFiltermatched roles byrole_key, so a role without a key matched no users. The EXISTS subquery now matchesur.role_id, with a defensive key-to-id fallback for hand-builtRoleobjects from externalUserAPIcallers. Affects/v1/users/filter?roleKey=and/v1/users/loginAsData: behavior preservation is demonstrated, not assumed. Regression tests were written and green against the previous SQL, then carried unchanged across the change.Tests
22 new integration tests (TDD, red before each implementation), all green:
RoleResourceCountsIntegrationTest(8): counts per endpoint, direct-only semantics, batch aggregateRoleResourceUsersIntegrationTest(8): emails present, keyless role (the headline regression), no inheritance, user-role self-grant, filter, pagination, 404/403UserAPITestandUserResourceIntegrationTestnext to existing coveragetestGetUsersByNameFilteredByRolere-validated against the new SQLFixes #37070, Fixes #37071