KEYCLOAK-8175 Possibility of clientScope not being used if user doesn't have a role

server-spi/src/main/java/org/keycloak/models/ClientSessionContext.java

@@ -32,6 +32,9 @@ public interface ClientSessionContext {
 
     Set<ClientScopeModel> getClientScopes();
 
+    /**
+     * @return expanded roles (composite roles already applied)
+     */
     Set<RoleModel> getRoles();
 
     Set<ProtocolMapperModel> getProtocolMappers();

server-spi/src/main/java/org/keycloak/models/utils/RoleUtils.java

@@ -19,11 +19,13 @@ package org.keycloak.models.utils;
 
 import org.keycloak.models.GroupModel;
 import org.keycloak.models.RoleModel;
+import org.keycloak.models.UserModel;
 
 import java.util.ArrayDeque;
 import java.util.Deque;
 import java.util.HashSet;
 import java.util.Set;
+import java.util.stream.Collectors;
 import java.util.stream.Stream;
 import java.util.stream.StreamSupport;
 
@@ -105,12 +107,14 @@ public class RoleUtils {
     /**
      * Recursively expands composite roles into their composite.
      * @param role
+     * @param visited Track roles, which were already visited. Those will be ignored and won't be added to the stream. Besides that,
+     *                the "visited" set itself will be updated as a result of this method call and all the tracked roles will be added to it
      * @return Stream of containing all of the composite roles and their components.
      */
-    public static Stream<RoleModel> expandCompositeRolesStream(RoleModel role) {
+    private static Stream<RoleModel> expandCompositeRolesStream(RoleModel role, Set<RoleModel> visited) {
         Stream.Builder<RoleModel> sb = Stream.builder();
-        Set<RoleModel> roles = new HashSet<>();
 
+        if (!visited.contains(role)) {
             Deque<RoleModel> stack = new ArrayDeque<>();
             stack.add(role);
 
@@ -120,15 +124,50 @@ public class RoleUtils {
 
                 if (current.isComposite()) {
                     current.getComposites().stream()
-                  .filter(r -> ! roles.contains(r))
+                            .filter(r -> !visited.contains(r))
                             .forEach(r -> {
-                    roles.add(r);
+                                visited.add(r);
                                 stack.add(r);
                             });
                 }
             }
+        }
 
         return sb.build();
     }
 
+
+    /**
+     * @param roles
+     * @return new set with composite roles expanded
+     */
+    public static Set<RoleModel> expandCompositeRoles(Set<RoleModel> roles) {
+        Set<RoleModel> visited = new HashSet<>();
+
+        return roles.stream()
+                .flatMap(roleModel -> RoleUtils.expandCompositeRolesStream(roleModel, visited))
+                .collect(Collectors.toSet());
+    }
+
+
+    /**
+     * @param user
+     * @return all user role mappings including all groups of user. Composite roles will be expanded
+     */
+    public static Set<RoleModel> getDeepUserRoleMappings(UserModel user) {
+        Set<RoleModel> roleMappings = new HashSet<>(user.getRoleMappings());
+        for (GroupModel group : user.getGroups()) {
+            addGroupRoles(group, roleMappings);
+        }
+
+        return expandCompositeRoles(roleMappings);
+    }
+
+
+    private static void addGroupRoles(GroupModel group, Set<RoleModel> roleMappings) {
+        roleMappings.addAll(group.getRoleMappings());
+        if (group.getParentId() == null) return;
+        addGroupRoles(group.getParent(), roleMappings);
+    }
+
 }

services/src/main/java/org/keycloak/protocol/oidc/TokenManager.java

@@ -35,9 +35,7 @@ import org.keycloak.models.AuthenticatedClientSessionModel;
 import org.keycloak.models.ClientModel;
 import org.keycloak.models.ClientScopeModel;
 import org.keycloak.models.ClientSessionContext;
-import org.keycloak.models.GroupModel;
 import org.keycloak.models.KeycloakSession;
-import org.keycloak.models.KeycloakSessionFactory;
 import org.keycloak.models.ProtocolMapperModel;
 import org.keycloak.models.RealmModel;
 import org.keycloak.models.RoleModel;
@@ -46,13 +44,13 @@ import org.keycloak.models.UserModel;
 import org.keycloak.models.UserSessionModel;
 import org.keycloak.models.UserSessionProvider;
 import org.keycloak.models.utils.KeycloakModelUtils;
+import org.keycloak.models.utils.RoleUtils;
 import org.keycloak.protocol.ProtocolMapper;
 import org.keycloak.protocol.ProtocolMapperUtils;
 import org.keycloak.protocol.oidc.mappers.OIDCAccessTokenMapper;
 import org.keycloak.protocol.oidc.mappers.OIDCIDTokenMapper;
 import org.keycloak.protocol.oidc.mappers.UserInfoTokenMapper;
 import org.keycloak.protocol.oidc.utils.OIDCResponseType;
-import org.keycloak.protocol.oidc.utils.WebOriginsUtils;
 import org.keycloak.representations.AccessToken;
 import org.keycloak.representations.AccessTokenResponse;
 import org.keycloak.representations.IDToken;
@@ -86,20 +84,6 @@ public class TokenManager {
     private static final Logger logger = Logger.getLogger(TokenManager.class);
     private static final String JWT = "JWT";
 
-    public static void applyScope(RoleModel role, RoleModel scope, Set<RoleModel> visited, Set<RoleModel> requested) {
-        if (visited.contains(scope)) return;
-        visited.add(scope);
-        if (role.hasRole(scope)) {
-            requested.add(scope);
-            return;
-        }
-        if (!scope.isComposite()) return;
-
-        for (RoleModel contained : scope.getComposites()) {
-            applyScope(role, contained, visited, requested);
-        }
-    }
-
     public static class TokenValidation {
         public final UserModel user;
         public final UserSessionModel userSession;
@@ -468,28 +452,14 @@ public class TokenManager {
     }
 
 
-    private static void addGroupRoles(GroupModel group, Set<RoleModel> roleMappings) {
-        roleMappings.addAll(group.getRoleMappings());
-        if (group.getParentId() == null) return;
-        addGroupRoles(group.getParent(), roleMappings);
-    }
-
-
     public static Set<RoleModel> getAccess(UserModel user, ClientModel client, Set<ClientScopeModel> clientScopes) {
-        Set<RoleModel> requestedRoles = new HashSet<RoleModel>();
+        Set<RoleModel> roleMappings = RoleUtils.getDeepUserRoleMappings(user);
-
-        Set<RoleModel> mappings = user.getRoleMappings();
-        Set<RoleModel> roleMappings = new HashSet<>();
-        roleMappings.addAll(mappings);
-        for (GroupModel group : user.getGroups()) {
-            addGroupRoles(group, roleMappings);
-        }
 
         if (client.isFullScopeAllowed()) {
             if (logger.isTraceEnabled()) {
                 logger.tracef("Using full scope for client %s", client.getClientId());
             }
-            requestedRoles = roleMappings;
+            return roleMappings;
         } else {
             Set<RoleModel> scopeMappings = new HashSet<>();
 
@@ -504,15 +474,14 @@ public class TokenManager {
                 scopeMappings.addAll(clientScope.getScopeMappings());
             }
 
-            for (RoleModel role : roleMappings) {
+            // 3 - Expand scope mappings
-                for (RoleModel desiredRole : scopeMappings) {
+            scopeMappings = RoleUtils.expandCompositeRoles(scopeMappings);
-                    Set<RoleModel> visited = new HashSet<RoleModel>();
-                    applyScope(role, desiredRole, visited, requestedRoles);
-                }
-            }
-        }
 
-        return requestedRoles;
+            // Intersection of expanded user roles and expanded scopeMappings
+            roleMappings.retainAll(scopeMappings);
+
+            return roleMappings;
+        }
     }
 
 

services/src/main/java/org/keycloak/protocol/saml/mappers/RoleListMapper.java

@@ -23,6 +23,7 @@ import org.keycloak.models.ClientSessionContext;
 import org.keycloak.models.KeycloakSession;
 import org.keycloak.models.KeycloakSessionFactory;
 import org.keycloak.models.ProtocolMapperModel;
+import org.keycloak.models.RoleModel;
 import org.keycloak.models.UserSessionModel;
 import org.keycloak.models.utils.RoleUtils;
 import org.keycloak.protocol.ProtocolMapper;
@@ -32,6 +33,7 @@ import org.keycloak.provider.ProviderConfigProperty;
 
 import java.util.ArrayList;
 import java.util.HashMap;
+import java.util.HashSet;
 import java.util.LinkedList;
 import java.util.List;
 import java.util.Map;
@@ -144,7 +146,6 @@ public class RoleListMapper extends AbstractSAMLProtocolMapper implements SAMLRo
 
         List<String> allRoleNames = clientSessionCtx.getRoles().stream()
           // todo need a role mapping
-          .flatMap(RoleUtils::expandCompositeRolesStream)
           .map(roleModel -> roleNameMappers.stream()
             .map(entry -> entry.mapper.mapName(entry.model, roleModel))
             .filter(Objects::nonNull)

services/src/main/java/org/keycloak/services/managers/UserSessionManager.java

@@ -131,13 +131,7 @@ public class UserSessionManager {
         }
 
         // Check if offline_access is allowed here. Even through composite roles
-        for (RoleModel role : clientSessionCtx.getRoles()) {
+        return clientSessionCtx.getRoles().contains(offlineAccessRole);
-            if (role.hasRole(offlineAccessRole)) {
-                return true;
-            }
-        }
-
-        return false;
     }
 
     private UserSessionModel createOfflineUserSession(UserModel user, UserSessionModel userSession) {

services/src/main/java/org/keycloak/services/util/DefaultClientSessionContext.java

@@ -30,7 +30,7 @@ import org.keycloak.models.ProtocolMapperModel;
 import org.keycloak.models.RoleModel;
 import org.keycloak.models.UserModel;
 import org.keycloak.models.utils.KeycloakModelUtils;
-import org.keycloak.models.utils.RepresentationToModel;
+import org.keycloak.models.utils.RoleUtils;
 import org.keycloak.protocol.oidc.OIDCLoginProtocol;
 import org.keycloak.protocol.oidc.TokenManager;
 import org.keycloak.util.TokenUtil;
@@ -48,9 +48,14 @@ public class DefaultClientSessionContext implements ClientSessionContext {
     private final Set<String> clientScopeIds;
 
     private Set<ClientScopeModel> clientScopes;
+
+    //
     private Set<RoleModel> roles;
     private Set<ProtocolMapperModel> protocolMappers;
 
+    // All roles of user expanded. It doesn't yet take into account permitted clientScopes
+    private Set<RoleModel> userRoles;
+
     private DefaultClientSessionContext(AuthenticatedClientSessionModel clientSession, Set<String> clientScopeIds) {
         this.clientSession = clientSession;
         this.clientScopeIds = clientScopeIds;
@@ -82,9 +87,7 @@ public class DefaultClientSessionContext implements ClientSessionContext {
             clientScopeIds.add(clientScope.getId());
         }
 
-        DefaultClientSessionContext ctx = new DefaultClientSessionContext(clientSession, clientScopeIds);
+        return new DefaultClientSessionContext(clientSession, clientScopeIds);
-        ctx.clientScopes = new HashSet<>(clientScopes);
-        return ctx;
     }
 
 
@@ -122,7 +125,7 @@ public class DefaultClientSessionContext implements ClientSessionContext {
 
     @Override
     public Set<ProtocolMapperModel> getProtocolMappers() {
-        // Load roles if not yet present
+        // Load protocolMappers if not yet present
         if (protocolMappers == null) {
             protocolMappers = loadProtocolMappers();
         }
@@ -130,6 +133,15 @@ public class DefaultClientSessionContext implements ClientSessionContext {
     }
 
 
+    private Set<RoleModel> getUserRoles() {
+        // Load userRoles if not yet present
+        if (userRoles == null) {
+            userRoles = loadUserRoles();
+        }
+        return userRoles;
+    }
+
+
     @Override
     public String getScopeString() {
         StringBuilder builder = new StringBuilder();
@@ -172,13 +184,42 @@ public class DefaultClientSessionContext implements ClientSessionContext {
         for (String scopeId : clientScopeIds) {
             ClientScopeModel clientScope = KeycloakModelUtils.findClientScopeById(clientSession.getClient().getRealm(), scopeId);
             if (clientScope != null) {
+                if (isClientScopePermittedForUser(clientScope)) {
                     clientScopes.add(clientScope);
+                } else {
+                    if (logger.isTraceEnabled()) {
+                        logger.tracef("User '%s' not permitted to have client scope '%s'",
+                                clientSession.getUserSession().getUser().getUsername(), clientScope.getName());
+                    }
+                }
             }
         }
         return clientScopes;
     }
 
 
+    // Return true if clientScope can be used by the user.
+    private boolean isClientScopePermittedForUser(ClientScopeModel clientScope) {
+        if (clientScope instanceof ClientModel) {
+            return true;
+        }
+
+        Set<RoleModel> clientScopeRoles = clientScope.getScopeMappings();
+
+        // Client scope is automatically permitted if it doesn't have any role scope mappings
+        if (clientScopeRoles.isEmpty()) {
+            return true;
+        }
+
+        // Expand (resolve composite roles)
+        clientScopeRoles = RoleUtils.expandCompositeRoles(clientScopeRoles);
+
+        // Check if expanded roles of clientScope has any intersection with expanded roles of user. If not, it is not permitted
+        clientScopeRoles.retainAll(getUserRoles());
+        return !clientScopeRoles.isEmpty();
+    }
+
+
     private Set<RoleModel> loadRoles() {
         UserModel user = clientSession.getUserSession().getUser();
         ClientModel client = clientSession.getClient();
@@ -212,4 +253,10 @@ public class DefaultClientSessionContext implements ClientSessionContext {
         return protocolMappers;
     }
 
+
+    private Set<RoleModel> loadUserRoles() {
+        UserModel user = clientSession.getUserSession().getUser();
+        return RoleUtils.getDeepUserRoleMappings(user);
+    }
+
 }

services/src/main/java/org/keycloak/utils/RoleResolveUtil.java

@@ -26,7 +26,6 @@ import org.keycloak.models.KeycloakSession;
 import org.keycloak.models.RealmModel;
 import org.keycloak.models.RoleModel;
 import org.keycloak.representations.AccessToken;
-import org.keycloak.services.util.DefaultClientSessionContext;
 
 /**
  * Helper class to ensure that all the user's permitted roles (including composite roles) are loaded just once per request.
@@ -113,13 +112,13 @@ public class RoleResolveUtil {
         Set<RoleModel> requestedRoles = clientSessionCtx.getRoles();
         AccessToken token = new AccessToken();
         for (RoleModel role : requestedRoles) {
-            addComposites(token, role);
+            addToToken(token, role);
         }
         return token;
     }
 
 
-    private static void addComposites(AccessToken token, RoleModel role) {
+    private static void addToToken(AccessToken token, RoleModel role) {
         AccessToken.Access access = null;
         if (role.getContainer() instanceof RealmModel) {
             access = token.getRealmAccess();
@@ -139,12 +138,6 @@ public class RoleResolveUtil {
 
         }
         access.addRole(role.getName());
-        if (!role.isComposite()) return;
-
-        for (RoleModel composite : role.getComposites()) {
-            addComposites(token, composite);
-        }
-
     }
 
 }

testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/account/AccountFormServiceTest.java

@@ -1004,7 +1004,7 @@ public class AccountFormServiceTest extends AbstractTestRealmKeycloakTest {
         Assert.assertEquals("http://localhost:8180/auth/realms/test/account", accountEntry.getHref());
 
         AccountApplicationsPage.AppEntry testAppEntry = apps.get("test-app");
-        Assert.assertEquals(5, testAppEntry.getRolesAvailable().size());
+        Assert.assertEquals(6, testAppEntry.getRolesAvailable().size());
         Assert.assertTrue(testAppEntry.getRolesAvailable().contains("Offline access"));
         Assert.assertTrue(testAppEntry.getClientScopesGranted().contains("Full Access"));
         Assert.assertEquals("http://localhost:8180/auth/realms/master/app/auth", testAppEntry.getHref());

testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oidc/OIDCScopeTest.java

@@ -18,6 +18,7 @@
 package org.keycloak.testsuite.oidc;
 
 import java.util.Arrays;
+import java.util.Collections;
 
 import javax.ws.rs.NotFoundException;
 import javax.ws.rs.core.Response;
@@ -44,6 +45,7 @@ import org.keycloak.representations.RefreshToken;
 import org.keycloak.representations.idm.ClientRepresentation;
 import org.keycloak.representations.idm.ClientScopeRepresentation;
 import org.keycloak.representations.idm.EventRepresentation;
+import org.keycloak.representations.idm.GroupRepresentation;
 import org.keycloak.representations.idm.RealmRepresentation;
 import org.keycloak.representations.idm.RoleRepresentation;
 import org.keycloak.representations.idm.UserRepresentation;
@@ -54,6 +56,7 @@ import org.keycloak.testsuite.pages.OAuthGrantPage;
 import org.keycloak.testsuite.runonserver.RunOnServerDeployment;
 import org.keycloak.testsuite.util.ClientManager;
 import org.keycloak.testsuite.util.OAuthClient;
+import org.keycloak.testsuite.util.RoleBuilder;
 import org.keycloak.testsuite.util.UserBuilder;
 
 import static org.junit.Assert.assertEquals;
@@ -104,6 +107,51 @@ public class OIDCScopeTest extends AbstractOIDCScopeTest {
         RoleRepresentation role2 = new RoleRepresentation();
         role2.setName("role-2");
         testRealm.getRoles().getRealm().add(role2);
+
+        RoleRepresentation roleParent = RoleBuilder.create()
+                .name("role-parent")
+                .realmComposite("role-1")
+                .build();
+        testRealm.getRoles().getRealm().add(roleParent);
+
+        // Add sample group
+        GroupRepresentation group = new GroupRepresentation();
+        group.setName("group-role-1");
+        group.setRealmRoles(Collections.singletonList("role-1"));
+        testRealm.getGroups().add(group);
+
+        // Add more sample users
+        user = UserBuilder.create()
+                .username("role-1-user")
+                .enabled(true)
+                .password("password")
+                .addRoles("role-1")
+                .build();
+        testRealm.getUsers().add(user);
+
+        user = UserBuilder.create()
+                .username("role-2-user")
+                .enabled(true)
+                .password("password")
+                .addRoles("role-2")
+                .build();
+        testRealm.getUsers().add(user);
+
+        user = UserBuilder.create()
+                .username("role-parent-user")
+                .enabled(true)
+                .password("password")
+                .addRoles("role-parent")
+                .build();
+        testRealm.getUsers().add(user);
+
+        user = UserBuilder.create()
+                .username("group-role-1-user")
+                .enabled(true)
+                .password("password")
+                .addGroups("group-role-1")
+                .build();
+        testRealm.getUsers().add(user);
     }
 
     @Before
@@ -534,4 +582,78 @@ public class OIDCScopeTest extends AbstractOIDCScopeTest {
         testApp.removeOptionalClientScope(scope2Id);
     }
 
+
+    // Test that clientScope is NOT applied in case that user is not member of any role scoped to the clientScope (including composite roles)
+    @Test
+    public void testClientScopesPermissions() {
+        // Add 2 client scopes. Each with scope to 1 realm role
+        ClientScopeRepresentation clientScope1 = new ClientScopeRepresentation();
+        clientScope1.setName("scope-role-1");
+        clientScope1.setProtocol(OIDCLoginProtocol.LOGIN_PROTOCOL);
+        Response response = testRealm().clientScopes().create(clientScope1);
+        String scope1Id = ApiUtil.getCreatedId(response);
+        getCleanup().addClientScopeId(scope1Id);
+        response.close();
+
+        ClientScopeRepresentation clientScopeParent = new ClientScopeRepresentation();
+        clientScopeParent.setName("scope-role-parent");
+        clientScopeParent.setProtocol(OIDCLoginProtocol.LOGIN_PROTOCOL);
+        response = testRealm().clientScopes().create(clientScopeParent);
+        String scopeParentId = ApiUtil.getCreatedId(response);
+        getCleanup().addClientScopeId(scopeParentId);
+        response.close();
+
+        RoleRepresentation role1 = testRealm().roles().get("role-1").toRepresentation();
+        testRealm().clientScopes().get(scope1Id).getScopeMappings().realmLevel().add(Arrays.asList(role1));
+
+        RoleRepresentation roleParent = testRealm().roles().get("role-parent").toRepresentation();
+        testRealm().clientScopes().get(scopeParentId).getScopeMappings().realmLevel().add(Arrays.asList(roleParent));
+
+        // Add client scopes to our client
+        ClientResource testApp = ApiUtil.findClientByClientId(testRealm(), "test-app");
+        ClientRepresentation testAppRep = testApp.toRepresentation();
+        testApp.update(testAppRep);
+        testApp.addDefaultClientScope(scope1Id);
+        testApp.addDefaultClientScope(scopeParentId);
+
+        // role-1-user will have clientScope "scope-role-1" and also "scope-role-parent" due the composite role
+        testLoginAndClientScopesPermissions("role-1-user", "scope-role-1 scope-role-parent", "role-1");
+
+        // role-2-user won't have any of the "scope-role-1" or "scope-role-parent" applied as he is not member of "role-1" nor "role-parent"
+        testLoginAndClientScopesPermissions("role-2-user", "", "role-2");
+
+        // role-parent-user will have clientScope "scope-role-1" (due the composite role) and also "scope-role-parent"
+        testLoginAndClientScopesPermissions("role-parent-user", "scope-role-1 scope-role-parent", "role-1", "role-parent");
+
+        // group-role-1-user will have clientScope "scope-role-1" and also "scope-role-parent" due the composite role and due the fact that he is member of group
+        testLoginAndClientScopesPermissions("group-role-1-user", "scope-role-1 scope-role-parent", "role-1");
+
+
+        // Revert
+        testApp.removeOptionalClientScope(scope1Id);
+        testApp.removeOptionalClientScope(scopeParentId);
+    }
+
+
+    private void testLoginAndClientScopesPermissions(String username, String expectedRoleScopes, String... expectedRoles) {
+        String userId = ApiUtil.findUserByUsername(testRealm(), username).getId();
+
+        oauth.openLoginForm();
+        oauth.doLogin(username, "password");
+        EventRepresentation loginEvent = events.expectLogin()
+                .user(userId)
+                .assertEvent();
+
+        Tokens tokens = sendTokenRequest(loginEvent, userId,"openid email profile " + expectedRoleScopes, "test-app");
+        Assert.assertNames(tokens.accessToken.getRealmAccess().getRoles(), expectedRoles);
+
+        oauth.doLogout(tokens.refreshToken, "password");
+        events.expectLogout(tokens.idToken.getSessionState())
+                .client("test-app")
+                .user(userId)
+                .removeDetail(Details.REDIRECT_URI).assertEvent();
+    }
+
+
+
 }