From 2c6cf9ecce760392d04d5b9cd516a1fa95021c89 Mon Sep 17 00:00:00 2001 From: hassandotcms Date: Wed, 26 Aug 2026 16:46:43 +0500 Subject: [PATCH] #37233: feat(users): return direct roles per user on GET /v1/users/filter (opt-in) New `includeRoles` query param on GET /v1/users/filter. When true, each item carries a `roles` array of the user's DIRECT memberships as {id, name, roleKey} (SmallRoleView); inherited roles and the user's personal role are excluded, matching the legacy Users portlet Roles tab (UserAjax#getUserRoles). Lets the Angular Users list drop its per-row GET /v1/roles/users/{id} fan-out. Default (flag absent) response is byte-identical: the flag is only added to the paginator params when true, so neither the items nor the Link header change. Requesting roles requires the same privilege as GET /v1/roles/users/{id} (CMS Administrator, or Roles+Users portlet access), otherwise 403 -- the flag must not widen who can read role membership. Personal roles are recognized the legacy way -- ID-based DBFQN under the cms_users root role -- not via Role#isUser(), whose name-based FQN check would also drop any ordinary root role whose name starts with "User". Roles are loaded outside UserPaginator#addRequestPasswordAttr so a lookup failure surfaces as an error instead of silently emptying the item. Tests: ITs for opt-out payload parity, direct roles, inherited-child exclusion, keyless role, roleKey filter + paging, the 403 gate, and a "User Managers" root role being kept; unit tests for the paginator and resource param plumbing. UserPaginatorTest now extends UnitTestBase -- it only passed when another UnitTestBase subclass ran first in the reused fork. openapi.yaml regenerated. --- .../dotcms/rest/api/v1/user/UserResource.java | 50 +++- .../dotcms/util/pagination/UserPaginator.java | 87 +++++- .../main/webapp/WEB-INF/openapi/openapi.yaml | 22 +- .../rest/api/v1/user/UserResourceTest.java | 47 +++- .../util/pagination/UserPaginatorTest.java | 94 ++++++- .../v1/user/UserResourceIntegrationTest.java | 253 +++++++++++++++++- 6 files changed, 522 insertions(+), 31 deletions(-) diff --git a/dotCMS/src/main/java/com/dotcms/rest/api/v1/user/UserResource.java b/dotCMS/src/main/java/com/dotcms/rest/api/v1/user/UserResource.java index 1f2c4b516567..403c73f47558 100644 --- a/dotCMS/src/main/java/com/dotcms/rest/api/v1/user/UserResource.java +++ b/dotCMS/src/main/java/com/dotcms/rest/api/v1/user/UserResource.java @@ -411,13 +411,25 @@ public final Response updateCurrent(@Context final HttpServletRequest httpServle * @param permission The permission type that Users may have on the previous asset. * @param roleKeys Optional Role keys -- repeatable and/or comma-separated -- that restrict the results to * Users holding any of them, e.g. {@code roleKey=DOTCMS_BACK_END_USER}. + * @param includeRoles Opt-in flag. When {@code true}, every returned User carries a {@code roles} list with + * its directly assigned Roles -- inherited Roles and the User's personal Role are + * excluded -- as {@code {id, name, roleKey}} entries. Defaults to {@code false}, in which + * case the response is identical to the one returned before this flag existed. Reading + * role membership requires the same privilege as {@code GET /v1/roles/users/{id}}: CMS + * Administrator, or access to both the Roles and the Users portlets; otherwise 403. * * @return A {@link Response} containing the list of dotCMS users that match the filtering criteria. */ @Operation( operationId = "filterUsers", summary = "Filter users", - description = "Returns a list of dotCMS users based on specified search criteria with pagination support" + description = "Returns a list of dotCMS users based on specified search criteria with pagination support. " + + "Each item is the user's data map. When `includeRoles=true` is passed, each item additionally " + + "carries a `roles` array listing the user's directly assigned roles as `{id, name, roleKey}` " + + "objects (`roleKey` may be null for roles created without a key); roles held through the role " + + "hierarchy and the user's personal role are not included. Requesting roles requires being a CMS " + + "Administrator or having access to both the Roles and Users portlets (403 otherwise). Without the " + + "flag the payload is unchanged." ) @io.swagger.v3.oas.annotations.responses.ApiResponses(value = { @ApiResponse(responseCode = "200", @@ -431,7 +443,7 @@ public final Response updateCurrent(@Context final HttpServletRequest httpServle description = "Unauthorized - authentication required", content = @Content(mediaType = "application/json")), @ApiResponse(responseCode = "403", - description = "Forbidden - insufficient permissions", + description = "Forbidden - insufficient permissions, or includeRoles=true requested without CMS Administrator / Roles+Users portlet access", content = @Content(mediaType = "application/json")) }) @GET @@ -449,7 +461,9 @@ public Response filter(@Context final HttpServletRequest request, @Context final @Parameter(description = "Include default user in results") @QueryParam(UserPaginator.INCLUDE_DEFAULT) boolean includeDefault, @Parameter(description = "Asset inode for permission-based filtering") @QueryParam(UserPaginator.ASSET_INODE_PARAM) String assetInode, @Parameter(description = "Permission type for asset-based filtering") @QueryParam(UserPaginator.PERMISSION_PARAM) int permission, - @Parameter(description = "Role key(s) to restrict results to users holding any of them; repeatable and/or comma-separated, e.g. roleKey=DOTCMS_BACK_END_USER") @QueryParam(UserPaginator.ROLE_KEY_PARAM) final List roleKeys) { + @Parameter(description = "Role key(s) to restrict results to users holding any of them; repeatable and/or comma-separated, e.g. roleKey=DOTCMS_BACK_END_USER") @QueryParam(UserPaginator.ROLE_KEY_PARAM) final List roleKeys, + @Parameter(description = "When true, each user carries a `roles` array of its directly assigned roles as {id, name, roleKey}; inherited roles and the user's personal role are excluded. Requires CMS Administrator or Roles+Users portlet access (403 otherwise). Defaults to false (payload unchanged)") @DefaultValue("false") @QueryParam(UserPaginator.INCLUDE_ROLES_PARAM) final boolean includeRoles) + throws DotDataException { final InitDataObject initData = new WebResource.InitBuilder(webResource) .requiredBackendUser(true) .requiredFrontendUser(false) @@ -468,12 +482,40 @@ public Response filter(@Context final HttpServletRequest request, @Context final if (UtilMethods.isSet(roles)) { extraParams.put(UserPaginator.ROLES_PARAM, roles); } + final User user = initData.getUser(); + // Only set when requested: extra params are echoed into the Link header, and the default response must stay + // identical to the pre-flag one + if (includeRoles) { + if (!this.isRoleAdministrator(user)) { + // Same privilege as GET /v1/roles/users/{id}: role membership is not visible to every back-end user + final String message = USER_MSG + user.getUserId() + " does not have permissions to read user roles"; + Logger.error(this, message); + throw new ForbiddenException(message); + } + extraParams.put(UserPaginator.INCLUDE_ROLES_PARAM, true); + } final OrderDirection orderDirection = OrderDirection.valueOf(direction); - final User user = initData.getUser(); return this.paginationUtil.getPage(request, user, filter, page, perPage, orderBy, orderDirection, extraParams); } + /** + * Tells whether the given User may administer Roles and Users: a CMS Administrator, or a User with access to + * both the Roles and the Users portlets. This is the same gate applied by the other user-administration + * endpoints in this resource and by {@code GET /v1/roles/users/{userIdOrEmail}}. + * + * @param user The calling {@link User}. + * + * @return {@code true} if the User is a Role administrator. + * + * @throws DotDataException An error occurred when resolving the User's portlet access. + */ + private boolean isRoleAdministrator(final User user) throws DotDataException { + return user.isAdmin() || + (APILocator.getLayoutAPI().doesUserHaveAccessToPortlet(PortletID.ROLES.toString(), user) && + APILocator.getLayoutAPI().doesUserHaveAccessToPortlet(PortletID.USERS.toString(), user)); + } + /** * Resolves the {@code roleKey} query parameter values -- each entry may itself be a comma-separated list -- into * the {@link Role} objects expected by the {@link UserPaginator#ROLES_PARAM} parameter. diff --git a/dotCMS/src/main/java/com/dotcms/util/pagination/UserPaginator.java b/dotCMS/src/main/java/com/dotcms/util/pagination/UserPaginator.java index fb5614b702fc..c95cc5f5ff03 100644 --- a/dotCMS/src/main/java/com/dotcms/util/pagination/UserPaginator.java +++ b/dotCMS/src/main/java/com/dotcms/util/pagination/UserPaginator.java @@ -1,6 +1,7 @@ package com.dotcms.util.pagination; import com.google.common.annotations.VisibleForTesting; +import com.dotcms.rest.api.v1.system.role.SmallRoleView; import com.dotcms.rest.api.v1.user.UserResourceHelper; import com.dotcms.util.CollectionsUtils; import com.dotmarketing.business.APILocator; @@ -9,6 +10,7 @@ import com.dotmarketing.business.UserAPI; import com.dotmarketing.exception.DotDataException; import com.dotmarketing.exception.DotRuntimeException; +import com.dotmarketing.util.Logger; import com.dotmarketing.util.PaginatedArrayList; import com.dotmarketing.util.UtilMethods; import com.liferay.portal.model.User; @@ -42,6 +44,8 @@ public class UserPaginator implements PaginatorOrdered> { public static final String ROLE_KEY_PARAM = "roleKey"; public static final String REMOVE_CURRENT_USER_PARAM = "removeCurrentUser"; public static final String REQUEST_PASSWORD_PARAM = "requestPassword"; + /** Opt-in flag: when {@code true}, every item carries a {@link #ROLES_PARAM} list of its direct roles. */ + public static final String INCLUDE_ROLES_PARAM = "includeRoles"; @VisibleForTesting public UserPaginator(UserAPI userApi, RoleAPI roleAPI){ @@ -75,11 +79,15 @@ public PaginatedArrayList> getItems(final User user, final S try { final List roles = (List) extraParams.get(ROLES_PARAM); final UserAPI.FilteringParams filteringParams = new UserAPI.FilteringParams.Builder().build(extraParams); - final List> usersMap; + final boolean includeRoles = (boolean) extraParams.getOrDefault(INCLUDE_ROLES_PARAM, false); + final String usersRootRoleId = includeRoles ? this.usersRootRoleId() : null; + final List> usersMap = new ArrayList<>(); if (UtilMethods.isSet(extraParams.get(ASSET_INODE_PARAM)) && UtilMethods.isSet(extraParams.get(PERMISSION_PARAM))) { final List userList = helper.getUsersByAssetAndPermissionType(filter, offset, limit, extraParams.get(ASSET_INODE_PARAM).toString(), extraParams.get(PERMISSION_PARAM).toString()); - usersMap = userList.stream().map(this::userToMap).collect(Collectors.toList()); + for (final User userItem : userList) { + usersMap.add(this.toItem(userItem, null, includeRoles, usersRootRoleId)); + } } else { final List users = userAPI.getUsersByName(filter, roles, offset, limit, filteringParams); if ((boolean) CollectionsUtils.getMapValue(extraParams, REMOVE_CURRENT_USER_PARAM, false)) { @@ -88,8 +96,9 @@ public PaginatedArrayList> getItems(final User user, final S } final List adminRoleIds = (boolean) extraParams.getOrDefault(REQUEST_PASSWORD_PARAM, false) ? collectAdminRolesIfAny() : new ArrayList<>(); - usersMap = - users.stream().map(userItem -> this.addRequestPasswordAttr(userItem, adminRoleIds)).collect(Collectors.toList()); + for (final User userItem : users) { + usersMap.add(this.toItem(userItem, adminRoleIds, includeRoles, usersRootRoleId)); + } } final PaginatedArrayList> result = new PaginatedArrayList<>(); result.addAll(usersMap); @@ -130,14 +139,74 @@ private List collectAdminRolesIfAny() throws DotDataException { } /** - * Utility method that transforms a {@link User} object into its data map. + * Builds the paginated item for a User: its data map, the optional {@link #REQUEST_PASSWORD_PARAM} flag and, + * when requested, its direct Roles under the {@link #ROLES_PARAM} key. Role loading is deliberately kept out of + * {@link #addRequestPasswordAttr(User, List)} so that a role-lookup failure surfaces as an error instead of + * silently turning the item into an empty map. + * + * @param user The {@link User} being transformed. + * @param adminRoleIds The optional CMS Administrator Role IDs used to compute {@link #REQUEST_PASSWORD_PARAM}. + * @param includeRoles If {@code true}, the item carries the User's direct Roles. + * @param usersRootRoleId The ID of the {@code cms_users} root Role, used to leave personal Roles out; may be + * {@code null}, in which case no Role is excluded. + * + * @return The item {@link Map}. + * + * @throws DotDataException An error occurred when loading the User's Roles. + */ + private Map toItem(final User user, final List adminRoleIds, final boolean includeRoles, + final String usersRootRoleId) throws DotDataException { + final Map userMap = this.addRequestPasswordAttr(user, adminRoleIds); + if (includeRoles && !userMap.isEmpty()) { + userMap.put(ROLES_PARAM, this.directRolesOf(user, usersRootRoleId)); + } + return userMap; + } + + /** + * Returns the Roles the given User holds directly -- i.e., rows in {@code users_cms_roles} -- as minimal + * {@link SmallRoleView}s. Roles reached through the hierarchy (children of a held Role) are excluded, and so is + * the User's own personal Role, mirroring the legacy Users portlet Roles tab ({@code UserAjax#getUserRoles}). + *

Personal Roles are recognized the way the legacy code did -- their ID-based {@code DBFQN} hangs under the + * {@code cms_users} root Role -- rather than through {@link Role#isUser()}, whose name-based {@code FQN} check + * would also swallow any ordinary root Role whose name happens to start with {@code "User"}.

+ * + * @param user The {@link User}. + * @param usersRootRoleId The ID of the {@code cms_users} root Role, or {@code null} to skip the exclusion. + * + * @return The list of direct Roles, possibly empty. + * + * @throws DotDataException An error occurred when accessing the data source. + */ + private List directRolesOf(final User user, final String usersRootRoleId) throws DotDataException { + return this.roleAPI.loadRolesForUser(user.getUserId(), false).stream() + .filter(role -> !isPersonalRole(role, usersRootRoleId)) + .map(role -> new SmallRoleView(role.getName(), role.getId(), role.getRoleKey(), false)) + .collect(Collectors.toList()); + } + + private static boolean isPersonalRole(final Role role, final String usersRootRoleId) { + return UtilMethods.isSet(usersRootRoleId) && UtilMethods.isSet(role.getDBFQN()) + && role.getDBFQN().contains(usersRootRoleId); + } + + /** + * Resolves the ID of the {@code cms_users} root Role every personal Role hangs from. It is a system Role that is + * always present; should it be missing, a warning is logged and {@code null} is returned so the roles list is + * still produced, just without the personal-Role exclusion. * - * @param user The {@link User} object. + * @return The root Role ID, or {@code null} if it cannot be resolved. * - * @return The {@link Map} containing the User data. + * @throws DotDataException An error occurred when accessing the data source. */ - private Map userToMap(final User user) { - return addRequestPasswordAttr(user, null); + private String usersRootRoleId() throws DotDataException { + final Role usersRoot = this.roleAPI.loadRoleByKey(RoleAPI.USERS_ROOT_ROLE_KEY); + if (null == usersRoot || !UtilMethods.isSet(usersRoot.getId())) { + Logger.warn(this, "Users root role '" + RoleAPI.USERS_ROOT_ROLE_KEY + + "' not found; personal roles will not be excluded from the users list"); + return null; + } + return usersRoot.getId(); } /** diff --git a/dotCMS/src/main/webapp/WEB-INF/openapi/openapi.yaml b/dotCMS/src/main/webapp/WEB-INF/openapi/openapi.yaml index d2ae160c6fbb..ba5f782211b0 100644 --- a/dotCMS/src/main/webapp/WEB-INF/openapi/openapi.yaml +++ b/dotCMS/src/main/webapp/WEB-INF/openapi/openapi.yaml @@ -18929,8 +18929,14 @@ paths: - Users /v1/users/filter: get: - description: Returns a list of dotCMS users based on specified search criteria - with pagination support + description: "Returns a list of dotCMS users based on specified search criteria\ + \ with pagination support. Each item is the user's data map. When `includeRoles=true`\ + \ is passed, each item additionally carries a `roles` array listing the user's\ + \ directly assigned roles as `{id, name, roleKey}` objects (`roleKey` may\ + \ be null for roles created without a key); roles held through the role hierarchy\ + \ and the user's personal role are not included. Requesting roles requires\ + \ being a CMS Administrator or having access to both the Roles and Users portlets\ + \ (403 otherwise). Without the flag the payload is unchanged." operationId: filterUsers parameters: - description: "Filter users by user ID, first name, last name, email address,\ @@ -18993,6 +18999,15 @@ paths: type: array items: type: string + - description: "When true, each user carries a `roles` array of its directly\ + \ assigned roles as {id, name, roleKey}; inherited roles and the user's\ + \ personal role are excluded. Requires CMS Administrator or Roles+Users\ + \ portlet access (403 otherwise). Defaults to false (payload unchanged)" + in: query + name: includeRoles + schema: + type: boolean + default: false responses: "200": content: @@ -19012,7 +19027,8 @@ paths: "403": content: application/json: {} - description: Forbidden - insufficient permissions + description: "Forbidden - insufficient permissions, or includeRoles=true\ + \ requested without CMS Administrator / Roles+Users portlet access" summary: Filter users tags: - Users diff --git a/dotCMS/src/test/java/com/dotcms/rest/api/v1/user/UserResourceTest.java b/dotCMS/src/test/java/com/dotcms/rest/api/v1/user/UserResourceTest.java index 62be02995831..cda3eb673251 100644 --- a/dotCMS/src/test/java/com/dotcms/rest/api/v1/user/UserResourceTest.java +++ b/dotCMS/src/test/java/com/dotcms/rest/api/v1/user/UserResourceTest.java @@ -564,7 +564,7 @@ private UserResource getFilterTestResource(final WebResource webResource, final /** *
    *
  • Method to test: {@link UserResource#filter(HttpServletRequest, HttpServletResponse, String, int, - * int, String, String, boolean, boolean, String, int, List)}
    • + * int, String, String, boolean, boolean, String, int, List, boolean)} *
  • Given Scenario: Call the {@code /filter} endpoint passing {@code roleKey} values, both repeated * and comma-separated.
    • *
  • Expected Result: Every key is resolved through the {@link RoleAPI} and the resulting Role list @@ -593,7 +593,7 @@ public void testFilterWithRoleKeysAddsRolesToExtraParams() throws DotDataExcepti final UserResource resource = getFilterTestResource(webResource, roleAPI, paginationUtil); resource.filter(request, response, "jane", 0, 40, null, "ASC", false, false, null, 0, - List.of("DOTCMS_BACK_END_USER,DOTCMS_FRONT_END_USER")); + List.of("DOTCMS_BACK_END_USER,DOTCMS_FRONT_END_USER"), false); final ArgumentCaptor> extraParamsCaptor = ArgumentCaptor.forClass(Map.class); verify(paginationUtil).getPage(Mockito.eq(request), Mockito.eq(user), Mockito.eq("jane"), Mockito.eq(0), @@ -605,7 +605,7 @@ public void testFilterWithRoleKeysAddsRolesToExtraParams() throws DotDataExcepti /** *
      *
    • Method to test: {@link UserResource#filter(HttpServletRequest, HttpServletResponse, String, int, - * int, String, String, boolean, boolean, String, int, List)}
      • + * int, String, String, boolean, boolean, String, int, List, boolean)} *
    • Given Scenario: Call the {@code /filter} endpoint without any {@code roleKey} value.
      • *
    • Expected Result: The paginator extra params do NOT include {@link UserPaginator#ROLES_PARAM}, * preserving the endpoint's previous behavior.
      • @@ -624,18 +624,53 @@ public void testFilterWithoutRoleKeysLeavesRolesParamOut() throws DotDataExcepti final PaginationUtil paginationUtil = mock(PaginationUtil.class); final UserResource resource = getFilterTestResource(webResource, mock(RoleAPI.class), paginationUtil); - resource.filter(request, response, "jane", 0, 40, null, "ASC", false, false, null, 0, null); + resource.filter(request, response, "jane", 0, 40, null, "ASC", false, false, null, 0, null, false); final ArgumentCaptor> extraParamsCaptor = ArgumentCaptor.forClass(Map.class); verify(paginationUtil).getPage(Mockito.eq(request), Mockito.eq(user), Mockito.eq("jane"), Mockito.eq(0), Mockito.eq(40), Mockito.isNull(), Mockito.eq(OrderDirection.ASC), extraParamsCaptor.capture()); Assert.assertFalse(extraParamsCaptor.getValue().containsKey(UserPaginator.ROLES_PARAM)); + Assert.assertFalse("includeRoles=false must not reach the paginator (Link header stays identical)", + extraParamsCaptor.getValue().containsKey(UserPaginator.INCLUDE_ROLES_PARAM)); } /** *
        *
      • Method to test: {@link UserResource#filter(HttpServletRequest, HttpServletResponse, String, int, - * int, String, String, boolean, boolean, String, int, List)}
        • + * int, String, String, boolean, boolean, String, int, List, boolean)} + *
      • Given Scenario: A CMS Administrator calls the {@code /filter} endpoint with + * {@code includeRoles=true}.
        • + *
      • Expected Result: The paginator extra params carry {@link UserPaginator#INCLUDE_ROLES_PARAM} + * set to {@code true}, so every item is enriched with its direct roles. (The 403 path for non-administrators + * is covered by {@code UserResourceIntegrationTest}.)
        • + *
      + */ + @Test + public void testFilterWithIncludeRolesPassesFlagToPaginator() throws DotDataException { + final HttpServletRequest request = mock(HttpServletRequest.class); + final HttpServletResponse response = mock(HttpServletResponse.class); + final WebResource webResource = mock(WebResource.class); + final InitDataObject initDataObject = mock(InitDataObject.class); + final User user = mock(User.class); + when(user.isAdmin()).thenReturn(true); + when(initDataObject.getUser()).thenReturn(user); + when(webResource.init(Mockito.any(InitBuilder.class))).thenReturn(initDataObject); + + final PaginationUtil paginationUtil = mock(PaginationUtil.class); + final UserResource resource = getFilterTestResource(webResource, mock(RoleAPI.class), paginationUtil); + + resource.filter(request, response, "jane", 0, 40, null, "ASC", false, false, null, 0, null, true); + + final ArgumentCaptor> extraParamsCaptor = ArgumentCaptor.forClass(Map.class); + verify(paginationUtil).getPage(Mockito.eq(request), Mockito.eq(user), Mockito.eq("jane"), Mockito.eq(0), + Mockito.eq(40), Mockito.isNull(), Mockito.eq(OrderDirection.ASC), extraParamsCaptor.capture()); + assertEquals(Boolean.TRUE, extraParamsCaptor.getValue().get(UserPaginator.INCLUDE_ROLES_PARAM)); + } + + /** + *
        + *
      • Method to test: {@link UserResource#filter(HttpServletRequest, HttpServletResponse, String, int, + * int, String, String, boolean, boolean, String, int, List, boolean)}
        • *
      • Given Scenario: Call the {@code /filter} endpoint with a {@code roleKey} that does not match any * existing Role.
        • *
      • Expected Result: A {@link BadRequestException} is thrown instead of silently returning an empty @@ -657,6 +692,6 @@ public void testFilterWithUnknownRoleKeyThrowsBadRequest() throws DotDataExcepti final UserResource resource = getFilterTestResource(webResource, roleAPI, mock(PaginationUtil.class)); resource.filter(request, response, "jane", 0, 40, null, "ASC", false, false, null, 0, - List.of("NOT_A_ROLE")); + List.of("NOT_A_ROLE"), false); } } diff --git a/dotCMS/src/test/java/com/dotcms/util/pagination/UserPaginatorTest.java b/dotCMS/src/test/java/com/dotcms/util/pagination/UserPaginatorTest.java index dcd2ea350264..a57f8e6f3fc9 100644 --- a/dotCMS/src/test/java/com/dotcms/util/pagination/UserPaginatorTest.java +++ b/dotCMS/src/test/java/com/dotcms/util/pagination/UserPaginatorTest.java @@ -1,5 +1,7 @@ package com.dotcms.util.pagination; +import com.dotcms.UnitTestBase; +import com.dotcms.rest.api.v1.system.role.SmallRoleView; import com.dotmarketing.business.Role; import com.dotmarketing.business.RoleAPI; import com.dotmarketing.business.UserAPI; @@ -12,24 +14,31 @@ import java.lang.reflect.InvocationTargetException; import java.util.ArrayList; +import java.util.HashMap; import java.util.List; import java.util.Map; import static com.dotcms.util.CollectionsUtils.list; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyInt; import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; /** * test {@link UserPaginator} + *

        Extends {@link UnitTestBase} because {@link UserPaginator}'s constructor reaches {@code APILocator} through + * {@code UserResourceHelper.getInstance()}; without the base class bootstrap the test only survives when another + * {@code UnitTestBase} subclass happened to run first in the same (reused) fork.

        */ -public class UserPaginatorTest { +public class UserPaginatorTest extends UnitTestBase { UserAPI userAPI; RoleAPI roleAPI; UserPaginator userPaginator; @@ -37,6 +46,7 @@ public class UserPaginatorTest { String loadCMSAdminRoleId = "2"; String adminRoleId = "3"; String backEndRoleId = "4"; + static final String USERS_ROOT_ID = "users-root-id"; final List roles = new ArrayList<>(1); @Before @@ -52,9 +62,13 @@ public void init() throws DotDataException { final Role roleBackend = mock(Role.class); when(roleBackend.getId()).thenReturn(backEndRoleId); + final Role usersRoot = mock(Role.class); + when(usersRoot.getId()).thenReturn(USERS_ROOT_ID); + roleAPI = mock(RoleAPI.class); when(roleAPI.loadCMSAdminRole()).thenReturn(roleAdmin); when(roleAPI.loadRoleByKey(Role.ADMINISTRATOR)).thenReturn(role); + when(roleAPI.loadRoleByKey(RoleAPI.USERS_ROOT_ROLE_KEY)).thenReturn(usersRoot); when(roleAPI.loadBackEndUserRole()).thenReturn(roleBackend); roles.add(roleBackend); @@ -180,4 +194,82 @@ public void testGetItemsException() throws DotDataException { } } + /** + *
          + *
        • Method to test: {@link UserPaginator#getItems(User, String, int, int, String, OrderDirection, Map)}
          • + *
        • Given Scenario: {@link UserPaginator#INCLUDE_ROLES_PARAM} is not set.
          • + *
        • Expected Result: Items carry no {@code roles} key and the direct-role lookup is never + * performed — the opt-out path has zero extra cost.
          • + *
        + */ + @Test + public void testGetItemsWithoutIncludeRolesAddsNothing() throws Exception { + final User userMock = mock(User.class); + when(userMock.getUserId()).thenReturn("u1"); + // stubbed map: the real User#toMap() reaches APILocator/DB, which a unit test must not do + when(userMock.toMap()).thenReturn(new HashMap<>(Map.of("userId", "u1"))); + when(userAPI.getUsersByName(anyString(), eq(null), anyInt(), anyInt(), any(UserAPI.FilteringParams.class))) + .thenReturn(list(userMock)); + + final PaginatedArrayList> items = userPaginator.getItems(mock(User.class), "filter", 5, 0, + null, null, Map.of()); + + assertFalse(items.get(0).containsKey(UserPaginator.ROLES_PARAM)); + verify(roleAPI, never()).loadRolesForUser(anyString(), any(Boolean.class)); + } + + /** + *
          + *
        • Method to test: {@link UserPaginator#getItems(User, String, int, int, String, OrderDirection, Map)}
          • + *
        • Given Scenario: {@link UserPaginator#INCLUDE_ROLES_PARAM} is {@code true}; the user directly + * holds a keyed role, a keyless role, an ordinary root role whose NAME starts with "User" (so + * {@link Role#isUser()} would wrongly flag it) and their personal role (DBFQN under the cms_users root).
          • + *
        • Expected Result: Direct roles are requested ({@code includeImplicitRoles=false}) and exposed + * as {@link SmallRoleView}s carrying id, name and (possibly null) roleKey; the "User Managers" role is kept + * and only the personal role is dropped.
          • + *
        + */ + @Test + public void testGetItemsWithIncludeRolesAddsDirectRoleViews() throws Exception { + final User userMock = mock(User.class); + when(userMock.getUserId()).thenReturn("u1"); + // stubbed map: the real User#toMap() reaches APILocator/DB, which a unit test must not do + when(userMock.toMap()).thenReturn(new HashMap<>(Map.of("userId", "u1"))); + when(userAPI.getUsersByName(anyString(), eq(null), anyInt(), anyInt(), any(UserAPI.FilteringParams.class))) + .thenReturn(list(userMock)); + + final Role keyed = mock(Role.class); + when(keyed.getId()).thenReturn("r-keyed"); + when(keyed.getName()).thenReturn("Keyed"); + when(keyed.getRoleKey()).thenReturn("keyed"); + when(keyed.getDBFQN()).thenReturn("r-keyed"); + final Role keyless = mock(Role.class); + when(keyless.getId()).thenReturn("r-keyless"); + when(keyless.getName()).thenReturn("Keyless"); + when(keyless.getDBFQN()).thenReturn("some-parent --> r-keyless"); + final Role userManagers = mock(Role.class); + when(userManagers.getId()).thenReturn("r-user-managers"); + when(userManagers.getName()).thenReturn("User Managers"); + when(userManagers.getDBFQN()).thenReturn("r-user-managers"); + when(userManagers.isUser()).thenReturn(true); // what the name-based FQN check would say + final Role personal = mock(Role.class); + when(personal.getId()).thenReturn("r-personal"); + when(personal.getDBFQN()).thenReturn(USERS_ROOT_ID + " --> r-personal"); + when(roleAPI.loadRolesForUser("u1", false)).thenReturn(list(keyed, keyless, userManagers, personal)); + + final PaginatedArrayList> items = userPaginator.getItems(mock(User.class), "filter", 5, 0, + null, null, Map.of(UserPaginator.INCLUDE_ROLES_PARAM, true)); + + @SuppressWarnings("unchecked") + final List roles = (List) items.get(0).get(UserPaginator.ROLES_PARAM); + assertEquals(3, roles.size()); + assertEquals("r-keyed", roles.get(0).getId()); + assertEquals("Keyed", roles.get(0).getName()); + assertEquals("keyed", roles.get(0).getRoleKey()); + assertEquals("r-keyless", roles.get(1).getId()); + assertNull(roles.get(1).getRoleKey()); + assertEquals("a root role merely NAMED 'User...' must be kept", "r-user-managers", roles.get(2).getId()); + verify(roleAPI, never()).loadRolesForUser("u1", true); + } + } diff --git a/dotcms-integration/src/test/java/com/dotcms/rest/api/v1/user/UserResourceIntegrationTest.java b/dotcms-integration/src/test/java/com/dotcms/rest/api/v1/user/UserResourceIntegrationTest.java index e94134197d43..a05b4f9a80ed 100644 --- a/dotcms-integration/src/test/java/com/dotcms/rest/api/v1/user/UserResourceIntegrationTest.java +++ b/dotcms-integration/src/test/java/com/dotcms/rest/api/v1/user/UserResourceIntegrationTest.java @@ -7,8 +7,17 @@ import com.dotmarketing.business.RoleAPI; import com.liferay.portal.ejb.UserTestUtil; import java.util.Collections; -import com.dotcms.rest.ErrorResponseHelper; +import com.dotcms.rest.exception.ForbiddenException; +import com.dotmarketing.business.LayoutAPI; +import com.dotmarketing.util.PortletID; +import com.dotmarketing.business.Role; +import java.util.List; +import java.util.Map; +import java.util.Set; +import java.util.stream.Collectors; import com.dotcms.rest.ResponseEntityView; +import com.dotcms.rest.api.v1.system.role.SmallRoleView; +import com.dotcms.rest.ErrorResponseHelper; import com.dotcms.rest.WebResource; import com.dotcms.rest.api.DotRestInstanceProvider; import com.dotcms.util.PaginationUtil; @@ -25,7 +34,6 @@ import com.dotmarketing.business.APILocator; import com.dotmarketing.business.CacheLocator; import com.dotmarketing.business.PermissionAPI; -import com.dotmarketing.business.Role; import com.liferay.portal.model.User; import com.liferay.portal.util.WebKeys; import javax.servlet.http.HttpServletRequest; @@ -34,9 +42,6 @@ import javax.ws.rs.core.Response.Status; import java.util.ArrayList; import java.util.Base64; -import java.util.List; -import java.util.Map; -import java.util.stream.Collectors; import static org.junit.Assert.*; import org.junit.AfterClass; @@ -98,6 +103,11 @@ public static void prepare() throws Exception { } private static HttpServletRequest mockRequest() { + return mockRequestAs("admin@dotcms.com", "admin"); + } + + /** Builds a request authenticated (Basic) as the given user. */ + private static HttpServletRequest mockRequestAs(final String email, final String password) { final MockHeaderRequest request = new MockHeaderRequest( new MockSessionRequest( new MockAttributeRequest(new MockHttpRequestIntegrationTest(host.getHostname(), "/").request()) @@ -105,7 +115,7 @@ private static HttpServletRequest mockRequest() { .request()); request.setHeader("Authorization", - "Basic " + Base64.getEncoder().encodeToString("admin@dotcms.com:admin".getBytes())); + "Basic " + Base64.getEncoder().encodeToString((email + ":" + password).getBytes())); request.getSession().setAttribute(com.dotmarketing.util.WebKeys.CURRENT_HOST,host); request.getSession().setAttribute(com.dotmarketing.util.WebKeys.CMS_SELECTED_HOST_ID,host.getIdentifier()); @@ -141,9 +151,10 @@ public void test_logoutAs_success() throws Exception { } @SuppressWarnings("unchecked") - private List filterUserIdsByRoleKeys(final String filter, final List roleKeys) { + private List filterUserIdsByRoleKeys(final String filter, final List roleKeys) + throws Exception { final Response resourceResponse = resource.filter(mockRequest(), response, filter, 0, 40, - null, "ASC", false, false, null, 0, roleKeys); + null, "ASC", false, false, null, 0, roleKeys, false); assertEquals(Status.OK.getStatusCode(), resourceResponse.getStatus()); final List> userMaps = (List>) ((ResponseEntityView) resourceResponse.getEntity()).getEntity(); @@ -477,4 +488,230 @@ public void test_create_rolesAbsent_defaultsToFrontEndUser() throws Exception { assertTrue("new user must receive the default Front-end User role", directRoles.stream().anyMatch(role -> Role.DOTCMS_FRONT_END_USER.equals(role.getRoleKey()))); } + + // ==================== GET /v1/users/filter — includeRoles (#37233) ==================== + + /** + * Invokes {@code GET /v1/users/filter} with the given query, paging, role-key filter and + * {@code includeRoles} flag; every other parameter keeps its default. + */ + private Response filter(final String query, final int page, final int perPage, + final List roleKeys, final boolean includeRoles) throws Exception { + return resource.filter(mockRequest(), response, query, page, perPage, null, "ASC", + false, false, null, 0, roleKeys, includeRoles); + } + + @SuppressWarnings("unchecked") + private static List> items(final Response resourceResponse) { + assertEquals(Status.OK.getStatusCode(), resourceResponse.getStatus()); + return (List>) ((ResponseEntityView) resourceResponse.getEntity()).getEntity(); + } + + private static Map itemFor(final List> items, final String userId) { + return items.stream() + .filter(item -> userId.equals(item.get("userId"))) + .findFirst() + .orElseThrow(() -> new AssertionError("user " + userId + " not in page")); + } + + @SuppressWarnings("unchecked") + private static List rolesOf(final Map item) { + assertTrue("item must carry a roles key", item.containsKey("roles")); + return (List) item.get("roles"); + } + + private static Set roleIds(final List roles) { + return roles.stream().map(SmallRoleView::getId).collect(Collectors.toSet()); + } + + /** + * Method to test: {@link UserResource#filter} + * Given Scenario: A user holds a role; the list is requested WITHOUT {@code includeRoles}. + * Expected Result: The item has no {@code roles} key and its key set is exactly + * {@link User#toMap()} — the existing payload is untouched (opt-in contract). + */ + @Test + public void test_filter_includeRolesOff_payloadUnchanged() throws Exception { + final User target = UserTestUtil.getUser("rolesoff" + uniq(), false, true); + final Role role = new RoleDataGen().key("rolesoff" + uniq()).nextPersisted(); + APILocator.getRoleAPI().addRoleToUser(role, target); + + final Map item = itemFor(items(filter(target.getUserId(), 1, 40, null, false)), + target.getUserId()); + + assertFalse("roles must be absent when not requested", item.containsKey("roles")); + assertEquals("item keys must be exactly User#toMap()", target.toMap().keySet(), item.keySet()); + } + + /** + * Given Scenario: A user directly holds two user-assignable roles; {@code includeRoles=true}. + * Expected Result: The item carries {@code roles} with both roles, each exposing id, name and + * roleKey; the user's personal role is not listed. + */ + @Test + public void test_filter_includeRolesOn_listsDirectRolesWithIdNameKey() throws Exception { + final RoleAPI roleAPI = APILocator.getRoleAPI(); + final User target = UserTestUtil.getUser("rolesonuser" + uniq(), false, true); + final Role roleA = new RoleDataGen().key("rolesona" + uniq()).name("Roles On A " + uniq()).nextPersisted(); + final Role roleB = new RoleDataGen().key("rolesonb" + uniq()).name("Roles On B " + uniq()).nextPersisted(); + roleAPI.addRoleToUser(roleA, target); + roleAPI.addRoleToUser(roleB, target); + final Role personal = roleAPI.getUserRole(target); + + final List roles = rolesOf(itemFor(items(filter(target.getUserId(), 1, 40, null, true)), + target.getUserId())); + + final Set ids = roleIds(roles); + assertTrue("role A must be listed", ids.contains(roleA.getId())); + assertTrue("role B must be listed", ids.contains(roleB.getId())); + assertFalse("the personal role must not be listed", ids.contains(personal.getId())); + final SmallRoleView viewA = roles.stream().filter(r -> roleA.getId().equals(r.getId())).findFirst().get(); + assertEquals(roleA.getName(), viewA.getName()); + assertEquals(roleA.getRoleKey(), viewA.getRoleKey()); + } + + /** + * Given Scenario: Role hierarchy parent -> child; the user directly holds ONLY the parent, so + * the child is held by inheritance (visible through {@code loadRolesForUser(id, true)}). + * Expected Result: {@code roles} lists the parent and NOT the child — direct memberships only. + */ + @Test + public void test_filter_includeRolesOn_excludesInheritedChildRole() throws Exception { + final RoleAPI roleAPI = APILocator.getRoleAPI(); + final User target = UserTestUtil.getUser("inheritroles" + uniq(), false, true); + final Role parent = new RoleDataGen().key("inheritparent" + uniq()).nextPersisted(); + final Role child = new RoleDataGen().key("inheritchild" + uniq()).parent(parent.getId()).nextPersisted(); + roleAPI.addRoleToUser(parent, target); + // sanity: the child really is implicit for this user + assertTrue(roleAPI.loadRolesForUser(target.getUserId(), true).stream() + .anyMatch(r -> child.getId().equals(r.getId()))); + + final Set ids = roleIds(rolesOf(itemFor(items(filter(target.getUserId(), 1, 40, null, true)), + target.getUserId()))); + + assertTrue("directly held parent must be listed", ids.contains(parent.getId())); + assertFalse("inherited child must not be listed", ids.contains(child.getId())); + } + + /** + * Given Scenario: The user directly holds a role saved WITHOUT a role key (as the Roles + * portlet allows); {@code includeRoles=true}. + * Expected Result: The role is listed with its id and name and a null roleKey. + */ + @Test + public void test_filter_includeRolesOn_keylessRoleListedWithNullKey() throws Exception { + final RoleAPI roleAPI = APILocator.getRoleAPI(); + final User target = UserTestUtil.getUser("keylessroles" + uniq(), false, true); + final Role keyless = new RoleDataGen().key(null).name("Keyless " + uniq()).nextPersisted(); + assertNull("precondition: the role must have no key", roleAPI.loadRoleById(keyless.getId()).getRoleKey()); + roleAPI.addRoleToUser(keyless, target); + + final List roles = rolesOf(itemFor(items(filter(target.getUserId(), 1, 40, null, true)), + target.getUserId())); + + final SmallRoleView view = roles.stream().filter(r -> keyless.getId().equals(r.getId())).findFirst() + .orElseThrow(() -> new AssertionError("keyless role must be listed by id")); + assertEquals(keyless.getName(), view.getName()); + assertNull(view.getRoleKey()); + } + + /** + * Given Scenario: Two users share role S (unique key); one of them also holds role E. The list + * is requested with {@code roleKey=S}, {@code per_page=1}, {@code includeRoles=true}, for + * pages 1 and 2. + * Expected Result: Each page carries exactly one item, both pages together cover both users, + * totalEntries is 2, every item lists S, and only the second user's item lists E. + */ + @Test + public void test_filter_includeRolesOn_combinedWithRoleKeyFilterAndPaging() throws Exception { + final RoleAPI roleAPI = APILocator.getRoleAPI(); + final String prefix = "pageroles" + uniq(); + final User userOne = UserTestUtil.getUser(prefix + "a", false, true); + final User userTwo = UserTestUtil.getUser(prefix + "b", false, true); + final Role shared = new RoleDataGen().key("pagerolesshared" + uniq()).nextPersisted(); + final Role extra = new RoleDataGen().key("pagerolesextra" + uniq()).nextPersisted(); + roleAPI.addRoleToUser(shared, userOne); + roleAPI.addRoleToUser(shared, userTwo); + roleAPI.addRoleToUser(extra, userTwo); + + final Response pageOne = filter(prefix, 1, 1, List.of(shared.getRoleKey()), true); + final Response pageTwo = filter(prefix, 2, 1, List.of(shared.getRoleKey()), true); + final List> first = items(pageOne); + final List> second = items(pageTwo); + + assertEquals(1, first.size()); + assertEquals(1, second.size()); + assertEquals(2, ((ResponseEntityView) pageOne.getEntity()).getPagination().getTotalEntries()); + final Set seen = Set.of((String) first.get(0).get("userId"), (String) second.get(0).get("userId")); + assertEquals(Set.of(userOne.getUserId(), userTwo.getUserId()), seen); + + for (final Map item : List.of(first.get(0), second.get(0))) { + final Set ids = roleIds(rolesOf(item)); + assertTrue("every page item must list the shared role", ids.contains(shared.getId())); + assertEquals("only userTwo holds the extra role", + userTwo.getUserId().equals(item.get("userId")), ids.contains(extra.getId())); + } + } + + /** + * Given Scenario: A plain back-end user -- not a CMS Administrator and without Users+Roles portlet + * access -- calls the list with and without {@code includeRoles}. + * Expected Result: Without the flag the call succeeds exactly as before (200); with the flag it is + * rejected with a {@link ForbiddenException} (403), matching the gate of + * {@code GET /v1/roles/users/{id}} so the flag does not widen who can read role membership. + */ + @Test + public void test_filter_includeRolesOn_plainBackendUserForbidden_defaultPathUnchanged() throws Exception { + final String suffix = uniq(); + final String password = "pw" + suffix; + final Role plain = new RoleDataGen().key("plainbackend" + suffix).nextPersisted(); + final User caller = new UserDataGen().firstName("plain").lastName("Backend") + .emailAddress("plainbackend" + suffix + "@dotcms.com").password(password) + .roles(plain, TestUserUtils.getBackendRole()).nextPersisted(); + final LayoutAPI layoutAPI = APILocator.getLayoutAPI(); + assertFalse("precondition: caller must not be an admin", caller.isAdmin()); + assertFalse("precondition: caller must lack Roles+Users portlet access", + layoutAPI.doesUserHaveAccessToPortlet(PortletID.ROLES.toString(), caller) + && layoutAPI.doesUserHaveAccessToPortlet(PortletID.USERS.toString(), caller)); + + final Response allowed = resource.filter(mockRequestAs(caller.getEmailAddress(), password), response, + caller.getUserId(), 1, 40, null, "ASC", false, false, null, 0, null, false); + assertEquals("default path must keep working for any back-end user", + Status.OK.getStatusCode(), allowed.getStatus()); + assertFalse(itemFor(items(allowed), caller.getUserId()).containsKey("roles")); + + try { + resource.filter(mockRequestAs(caller.getEmailAddress(), password), response, + caller.getUserId(), 1, 40, null, "ASC", false, false, null, 0, null, true); + fail("includeRoles=true must be forbidden for a non role-administrator"); + } catch (final ForbiddenException e) { + // expected + } + } + + /** + * Given Scenario: The user directly holds an ordinary root role whose NAME starts with "User" + * ({@code Role#isUser()} is name-based and would flag it as a personal role) plus their real + * personal role; {@code includeRoles=true}. + * Expected Result: The "User ..." role is listed and only the personal role is left out -- + * personal roles are recognized by their ID-based DBFQN under the cms_users root, as the legacy + * Users portlet did. + */ + @Test + public void test_filter_includeRolesOn_rootRoleNamedUserIsNotTreatedAsPersonal() throws Exception { + final RoleAPI roleAPI = APILocator.getRoleAPI(); + final User target = UserTestUtil.getUser("usernamedrole" + uniq(), false, true); + final Role userManagers = new RoleDataGen().key("usermanagers" + uniq()).name("User Managers " + uniq()) + .nextPersisted(); + assertTrue("precondition: the name-based check must misclassify this role", + roleAPI.loadRoleById(userManagers.getId()).isUser()); + roleAPI.addRoleToUser(userManagers, target); + final Role personal = roleAPI.getUserRole(target); + + final Set ids = roleIds(rolesOf(itemFor(items(filter(target.getUserId(), 1, 40, null, true)), + target.getUserId()))); + + assertTrue("a root role merely named 'User...' must be listed", ids.contains(userManagers.getId())); + assertFalse("the personal role must still be excluded", ids.contains(personal.getId())); + } }