diff --git a/server-spi/src/main/java/org/keycloak/models/ClientSessionContext.java b/server-spi/src/main/java/org/keycloak/models/ClientSessionContext.java index 3edfeb60d4..332079701c 100644 --- a/server-spi/src/main/java/org/keycloak/models/ClientSessionContext.java +++ b/server-spi/src/main/java/org/keycloak/models/ClientSessionContext.java @@ -32,6 +32,9 @@ public interface ClientSessionContext { Set getClientScopes(); + /** + * @return expanded roles (composite roles already applied) + */ Set getRoles(); Set getProtocolMappers(); diff --git a/server-spi/src/main/java/org/keycloak/models/utils/RoleUtils.java b/server-spi/src/main/java/org/keycloak/models/utils/RoleUtils.java index 1dd72673c8..025937e4c6 100644 --- a/server-spi/src/main/java/org/keycloak/models/utils/RoleUtils.java +++ b/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,30 +107,67 @@ 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 expandCompositeRolesStream(RoleModel role) { + private static Stream expandCompositeRolesStream(RoleModel role, Set visited) { Stream.Builder sb = Stream.builder(); - Set roles = new HashSet<>(); - Deque stack = new ArrayDeque<>(); - stack.add(role); + if (!visited.contains(role)) { + Deque stack = new ArrayDeque<>(); + stack.add(role); - while (! stack.isEmpty()) { - RoleModel current = stack.pop(); - sb.add(current); + while (!stack.isEmpty()) { + RoleModel current = stack.pop(); + sb.add(current); - if (current.isComposite()) { - current.getComposites().stream() - .filter(r -> ! roles.contains(r)) - .forEach(r -> { - roles.add(r); - stack.add(r); - }); + if (current.isComposite()) { + current.getComposites().stream() + .filter(r -> !visited.contains(r)) + .forEach(r -> { + visited.add(r); + stack.add(r); + }); + } } } return sb.build(); } + + /** + * @param roles + * @return new set with composite roles expanded + */ + public static Set expandCompositeRoles(Set roles) { + Set 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 getDeepUserRoleMappings(UserModel user) { + Set roleMappings = new HashSet<>(user.getRoleMappings()); + for (GroupModel group : user.getGroups()) { + addGroupRoles(group, roleMappings); + } + + return expandCompositeRoles(roleMappings); + } + + + private static void addGroupRoles(GroupModel group, Set roleMappings) { + roleMappings.addAll(group.getRoleMappings()); + if (group.getParentId() == null) return; + addGroupRoles(group.getParent(), roleMappings); + } + } diff --git a/services/src/main/java/org/keycloak/protocol/oidc/TokenManager.java b/services/src/main/java/org/keycloak/protocol/oidc/TokenManager.java index ba36588215..a007c718da 100755 --- a/services/src/main/java/org/keycloak/protocol/oidc/TokenManager.java +++ b/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 visited, Set 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 roleMappings) { - roleMappings.addAll(group.getRoleMappings()); - if (group.getParentId() == null) return; - addGroupRoles(group.getParent(), roleMappings); - } - - public static Set getAccess(UserModel user, ClientModel client, Set clientScopes) { - Set requestedRoles = new HashSet(); - - Set mappings = user.getRoleMappings(); - Set roleMappings = new HashSet<>(); - roleMappings.addAll(mappings); - for (GroupModel group : user.getGroups()) { - addGroupRoles(group, roleMappings); - } + Set roleMappings = RoleUtils.getDeepUserRoleMappings(user); if (client.isFullScopeAllowed()) { if (logger.isTraceEnabled()) { logger.tracef("Using full scope for client %s", client.getClientId()); } - requestedRoles = roleMappings; + return roleMappings; } else { Set scopeMappings = new HashSet<>(); @@ -504,15 +474,14 @@ public class TokenManager { scopeMappings.addAll(clientScope.getScopeMappings()); } - for (RoleModel role : roleMappings) { - for (RoleModel desiredRole : scopeMappings) { - Set visited = new HashSet(); - applyScope(role, desiredRole, visited, requestedRoles); - } - } - } + // 3 - Expand scope mappings + scopeMappings = RoleUtils.expandCompositeRoles(scopeMappings); - return requestedRoles; + // Intersection of expanded user roles and expanded scopeMappings + roleMappings.retainAll(scopeMappings); + + return roleMappings; + } } diff --git a/services/src/main/java/org/keycloak/protocol/saml/mappers/RoleListMapper.java b/services/src/main/java/org/keycloak/protocol/saml/mappers/RoleListMapper.java index dd70b53881..dd2587d29e 100755 --- a/services/src/main/java/org/keycloak/protocol/saml/mappers/RoleListMapper.java +++ b/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 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) diff --git a/services/src/main/java/org/keycloak/services/managers/UserSessionManager.java b/services/src/main/java/org/keycloak/services/managers/UserSessionManager.java index 1283f5f65e..44dcb3a9e7 100644 --- a/services/src/main/java/org/keycloak/services/managers/UserSessionManager.java +++ b/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()) { - if (role.hasRole(offlineAccessRole)) { - return true; - } - } - - return false; + return clientSessionCtx.getRoles().contains(offlineAccessRole); } private UserSessionModel createOfflineUserSession(UserModel user, UserSessionModel userSession) { diff --git a/services/src/main/java/org/keycloak/services/util/DefaultClientSessionContext.java b/services/src/main/java/org/keycloak/services/util/DefaultClientSessionContext.java index debd7000e5..2c127ed2fc 100644 --- a/services/src/main/java/org/keycloak/services/util/DefaultClientSessionContext.java +++ b/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 clientScopeIds; private Set clientScopes; + + // private Set roles; private Set protocolMappers; + // All roles of user expanded. It doesn't yet take into account permitted clientScopes + private Set userRoles; + private DefaultClientSessionContext(AuthenticatedClientSessionModel clientSession, Set 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); - ctx.clientScopes = new HashSet<>(clientScopes); - return ctx; + return new DefaultClientSessionContext(clientSession, clientScopeIds); } @@ -122,7 +125,7 @@ public class DefaultClientSessionContext implements ClientSessionContext { @Override public Set 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 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) { - clientScopes.add(clientScope); + 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 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 loadRoles() { UserModel user = clientSession.getUserSession().getUser(); ClientModel client = clientSession.getClient(); @@ -212,4 +253,10 @@ public class DefaultClientSessionContext implements ClientSessionContext { return protocolMappers; } + + private Set loadUserRoles() { + UserModel user = clientSession.getUserSession().getUser(); + return RoleUtils.getDeepUserRoleMappings(user); + } + } diff --git a/services/src/main/java/org/keycloak/utils/RoleResolveUtil.java b/services/src/main/java/org/keycloak/utils/RoleResolveUtil.java index 5f095ece28..a674bd3084 100644 --- a/services/src/main/java/org/keycloak/utils/RoleResolveUtil.java +++ b/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 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); - } - } } diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/account/AccountFormServiceTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/account/AccountFormServiceTest.java index 9267cbbb8f..31c10552be 100755 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/account/AccountFormServiceTest.java +++ b/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()); diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oidc/OIDCScopeTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oidc/OIDCScopeTest.java index 944f30406e..005f8e0466 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oidc/OIDCScopeTest.java +++ b/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(); + } + + + }