Fix paging on the "Users in role" endpoint, when JPA persistence is used

- add order-by-clause to the corresponding JPA query (ordering by username ASC)
- adjust admin-client RoleResource to return a List instead of a Set, by introducing new methods #getUserMembers (instead of #getRoleUserMembers - the "Role" prefix is not needed, because it is clear from the resource name that it's about roles)
- adjust tests to use the new method and check that the expected order is returned

Closes #14772

integration/admin-client/src/main/java/org/keycloak/admin/client/resource/RoleResource.java

@@ -102,21 +102,27 @@ public interface RoleResource {
     void deleteComposites(List<RoleRepresentation> rolesToRemove);
 
     /**
-     * Get role members
+     * Get role members.
-     * <p/>
+     * <p>
-     * Returns users that have the given role
+     * Returns users that have the given role, sorted by username ascending.
+     * </p>
+     * <p>
+     * Note: This method just returns the first 100 users. In order to retrieve all users, use paging (see
+     * {@link #getUserMembers(Integer, Integer)}).
+     * </p>
      * 
      * @return a list of users with the given role
      */
     @GET
     @Path("users")
     @Produces(MediaType.APPLICATION_JSON)
-    Set<UserRepresentation> getRoleUserMembers();
+    List<UserRepresentation> getUserMembers();
 
     /**
-     * Get role members
+     * Get role members.
      * <p/>
-     * Returns users that have the given role, paginated according to the query parameters
+     * Returns users that have the given role, sorted by username ascending, paginated according to the query
+     * parameters.
      *
      * @param firstResult Pagination offset
      * @param maxResults Pagination size
@@ -125,7 +131,7 @@ public interface RoleResource {
     @GET
     @Path("users")
     @Produces(MediaType.APPLICATION_JSON)
-    Set<UserRepresentation> getRoleUserMembers(@QueryParam("first") Integer firstResult,
+    List<UserRepresentation> getUserMembers(@QueryParam("first") Integer firstResult,
             @QueryParam("max") Integer maxResults);
     
     /**
@@ -154,4 +160,37 @@ public interface RoleResource {
     @Produces(MediaType.APPLICATION_JSON)
     Set<GroupRepresentation> getRoleGroupMembers(@QueryParam("first") Integer firstResult,
                                                @QueryParam("max") Integer maxResults);
+
+    /**
+     * Get role members
+     * <p/>
+     * Returns users that have the given role
+     *
+     * @return a set of users with the given role
+     *
+     * @deprecated please use {@link #getUserMembers()}
+     */
+    @GET
+    @Path("users")
+    @Produces(MediaType.APPLICATION_JSON)
+    @Deprecated
+    Set<UserRepresentation> getRoleUserMembers();
+
+    /**
+     * Get role members
+     * <p/>
+     * Returns users that have the given role, paginated according to the query parameters
+     *
+     * @param firstResult Pagination offset
+     * @param maxResults  Pagination size
+     * @return a set of users with the given role
+     *
+     * @deprecated please use {@link #getUserMembers(Integer, Integer)}
+     */
+    @GET
+    @Path("users")
+    @Produces(MediaType.APPLICATION_JSON)
+    @Deprecated
+    Set<UserRepresentation> getRoleUserMembers(@QueryParam("first") Integer firstResult,
+                                               @QueryParam("max") Integer maxResults);
 }

model/jpa/src/main/java/org/keycloak/models/jpa/entities/UserRoleMappingEntity.java

@@ -34,7 +34,7 @@ import java.io.Serializable;
  * @version $Revision: 1 $
  */
 @NamedQueries({
-        @NamedQuery(name="usersInRole", query="select u from UserRoleMappingEntity m, UserEntity u where m.roleId=:roleId and u.id=m.user"),        
+        @NamedQuery(name="usersInRole", query="select u from UserRoleMappingEntity m, UserEntity u where m.roleId=:roleId and u.id=m.user order by u.username"),
         @NamedQuery(name="userHasRole", query="select m from UserRoleMappingEntity m where m.user = :user and m.roleId = :roleId"),
         @NamedQuery(name="userRoleMappings", query="select m from UserRoleMappingEntity m where m.user = :user"),
         @NamedQuery(name="userRoleMappingIds", query="select m.roleId from UserRoleMappingEntity m where m.user = :user"),

testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/client/ClientRolesTest.java

@@ -31,14 +31,14 @@ import org.keycloak.representations.idm.UserRepresentation;
 import org.keycloak.testsuite.Assert;
 import org.keycloak.testsuite.util.AdminEventPaths;
 
+import java.util.ArrayList;
 import java.util.Arrays;
+import java.util.Collection;
 import java.util.Collections;
 import java.util.HashMap;
-import java.util.HashSet;
 import java.util.LinkedList;
 import java.util.List;
 import java.util.Map;
-import java.util.Optional;
 import java.util.Set;
 import java.util.stream.Collectors;
 import java.util.stream.IntStream;
@@ -216,36 +216,38 @@ public class ClientRolesTest extends AbstractClientTest {
         List<RoleRepresentation> roleToAdd = Collections.singletonList(rolesRsc.get(roleName).toRepresentation());
 
         //create users and assign test role
-        Set<UserRepresentation> users = new HashSet<>();
+        List<String> usernames = createUsernames(0, 10);
-        for (int i = 0; i < 10; i++) {
+        usernames.forEach(username -> {
-            String userName = "user" + i;
             UserRepresentation user = new UserRepresentation();
-            user.setUsername(userName);
+            user.setUsername(username);
             testRealmResource().users().create(user);
-            user = getFullUserRep(userName);
+            user = getFullUserRep(username);
             testRealmResource().users().get(user.getId()).roles().clientLevel(clientID).add(roleToAdd);
-            users.add(user);
+        });
-        }
 
         // check if users have test role assigned
         RoleResource roleResource = rolesRsc.get(roleName);
-        Set<UserRepresentation> usersInRole = roleResource.getRoleUserMembers();
+        List<UserRepresentation> usersInRole = roleResource.getUserMembers();
-        assertEquals(users.size(), usersInRole.size());
+        assertEquals(usernames, extractUsernames(usersInRole));
-        for (UserRepresentation user : users) {
-            Optional<UserRepresentation> result = usersInRole.stream().filter(u -> user.getUsername().equals(u.getUsername())).findAny();
-            assertTrue(result.isPresent());
-        }
 
         // pagination
-        Set<UserRepresentation> usersInRole1 = roleResource.getRoleUserMembers(0, 5);
+        List<UserRepresentation> usersInRole1 = roleResource.getUserMembers(0, 5);
-        assertEquals(5, usersInRole1.size());
+        assertEquals(createUsernames(0, 5), extractUsernames(usersInRole1));
-        Set<UserRepresentation> usersInRole2 = roleResource.getRoleUserMembers(5, 10);
+        List<UserRepresentation> usersInRole2 = roleResource.getUserMembers(5, 10);
-        assertEquals(5, usersInRole2.size());
+        assertEquals(createUsernames(5, 10), extractUsernames(usersInRole2));
-        for (UserRepresentation user : users) {
-            Optional<UserRepresentation> result1 = usersInRole1.stream().filter(u -> user.getUsername().equals(u.getUsername())).findAny();
-            Optional<UserRepresentation> result2 = usersInRole2.stream().filter(u -> user.getUsername().equals(u.getUsername())).findAny();
-            assertTrue((result1.isPresent() || result2.isPresent()) && !(result1.isPresent() && result2.isPresent()));
     }
+
+    private static List<String> createUsernames(int startIndex, int endIndex) {
+        List<String> usernames = new ArrayList<>();
+        for (int i = startIndex; i < endIndex; i++) {
+            String userName = "user" + i;
+            usernames.add(userName);
+        }
+        return usernames;
+    }
+
+    private static List<String> extractUsernames(Collection<UserRepresentation> users) {
+        return users.stream().map(UserRepresentation::getUsername).collect(Collectors.toList());
     }
 
     @Test

testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/realm/RealmRolesTest.java

@@ -39,9 +39,9 @@ import javax.ws.rs.NotFoundException;
 import javax.ws.rs.core.Response;
 
 import java.util.Arrays;
+import java.util.Collection;
 import java.util.Collections;
 import java.util.HashMap;
-import java.util.HashSet;
 import java.util.LinkedList;
 import java.util.List;
 import java.util.Map;
@@ -52,7 +52,6 @@ import javax.ws.rs.ClientErrorException;
 
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.allOf;
-import static org.hamcrest.Matchers.containsInAnyOrder;
 import static org.hamcrest.Matchers.empty;
 import static org.hamcrest.Matchers.hasItem;
 import static org.hamcrest.Matchers.is;
@@ -237,7 +236,7 @@ public class RealmRolesTest extends AbstractAdminTest {
     public void testUsersInRole() {
         RoleResource role = resource.get("role-with-users");
 
-        List<UserRepresentation> users = adminClient.realm(REALM_NAME).users().search("test-role-member", null, null, null, null, null);
+        List<UserRepresentation> users = adminClient.realm(REALM_NAME).users().search("test-role-member");
         assertEquals(1, users.size());
         UserResource user = adminClient.realm(REALM_NAME).users().get(users.get(0).getId());
         UserRepresentation userRep = user.toRepresentation();
@@ -248,12 +247,12 @@ public class RealmRolesTest extends AbstractAdminTest {
         adminClient.realm(REALM_NAME).users().get(userRep.getId()).roles().realmLevel().add(rolesToAdd);
 
         roleResource = adminClient.realm(REALM_NAME).roles().get(role.toRepresentation().getName());
-        roleResource.getRoleUserMembers();
+        assertEquals(Collections.singletonList("test-role-member"), extractUsernames(roleResource.getUserMembers()));
-        //roleResource.getRoleUserMembers().stream().forEach((member) -> log.infof("Found user {}", member.getUsername()));
-        assertEquals(1, roleResource.getRoleUserMembers().size());
-
     }
 
+    private static List<String> extractUsernames(Collection<UserRepresentation> users) {
+        return users.stream().map(UserRepresentation::getUsername).collect(Collectors.toList());
+    }
 
     /**
      * KEYCLOAK-2035  Verifies that Role with no users assigned is being properly retrieved without members in API endpoint for role membership
@@ -263,9 +262,7 @@ public class RealmRolesTest extends AbstractAdminTest {
         RoleResource role = resource.get("role-without-users");
 
         role = adminClient.realm(REALM_NAME).roles().get(role.toRepresentation().getName());
-        role.getRoleUserMembers();
+        assertThat(role.getUserMembers(), is(empty()));
-        assertEquals(0, role.getRoleUserMembers().size());
-
     }
 
 
@@ -321,13 +318,10 @@ public class RealmRolesTest extends AbstractAdminTest {
         adminClient.realm(REALM_NAME).users().get(userRep.getId()).roles().realmLevel().add(rolesToAdd);
 
         roleResource = adminClient.realm(REALM_NAME).roles().get(role.toRepresentation().getName());
-        roleResource.getRoleUserMembers();
+        assertEquals(Collections.singletonList("test-role-member"), extractUsernames(roleResource.getUserMembers()));
-        assertEquals(1, roleResource.getRoleUserMembers().size());
 
         adminClient.realm(REALM_NAME).users().delete(userRep.getId());
-        roleResource.getRoleUserMembers();
+        assertThat(roleResource.getUserMembers(), is(empty()));
-        assertEquals(0, roleResource.getRoleUserMembers().size());
-
     }
 
     @Test
@@ -355,20 +349,16 @@ public class RealmRolesTest extends AbstractAdminTest {
         }
 
         RoleResource roleResource = adminClient.realm(REALM_NAME).roles().get(role.toRepresentation().getName());
-        Set<UserRepresentation> roleUserMembers = roleResource.getRoleUserMembers(0, 1);
 
-        Set<String> expectedMembers = new HashSet<>();
+        List<UserRepresentation> roleUserMembers = roleResource.getUserMembers(0, 1);
+        assertEquals(Collections.singletonList("test-role-member"), extractUsernames(roleUserMembers));
+
+        roleUserMembers = roleResource.getUserMembers(1, 1);
         assertThat(roleUserMembers, hasSize(1));
-        expectedMembers.add(roleUserMembers.iterator().next().getUsername());
+        assertEquals(Collections.singletonList("test-role-member2"), extractUsernames(roleUserMembers));
 
-        roleUserMembers = roleResource.getRoleUserMembers(1, 1);
+        roleUserMembers = roleResource.getUserMembers(2, 1);
-        assertThat(roleUserMembers, hasSize(1));
-        expectedMembers.add(roleUserMembers.iterator().next().getUsername());
-
-        roleUserMembers = roleResource.getRoleUserMembers(2, 1);
         assertThat(roleUserMembers, is(empty()));
-
-        assertThat(expectedMembers, containsInAnyOrder("test-role-member", "test-role-member2"));
     }
 
     // issue #9587