From 8f9d02c3057205129e08872943505829286500d7 Mon Sep 17 00:00:00 2001 From: Pedro Igor Date: Mon, 26 May 2025 11:26:04 -0300 Subject: [PATCH] Avoid resolving a client scope if it was requested using the dynamic scope format (#39752) Closes #39402 Signed-off-by: Pedro Igor --- .../org/keycloak/models/ClientScopeModel.java | 5 ++ .../mappers/oidc/OrganizationScope.java | 2 +- .../keycloak/protocol/oidc/TokenManager.java | 4 ++ .../OrganizationOIDCProtocolMapperTest.java | 62 ++++++++++++++++--- 4 files changed, 63 insertions(+), 10 deletions(-) diff --git a/server-spi/src/main/java/org/keycloak/models/ClientScopeModel.java b/server-spi/src/main/java/org/keycloak/models/ClientScopeModel.java index 3cdc02eedab..6d17e448c46 100755 --- a/server-spi/src/main/java/org/keycloak/models/ClientScopeModel.java +++ b/server-spi/src/main/java/org/keycloak/models/ClientScopeModel.java @@ -28,6 +28,11 @@ import org.keycloak.provider.ProviderEvent; */ public interface ClientScopeModel extends ProtocolMapperContainerModel, ScopeContainerModel, OrderedModel { + /** + * The character separator used to specify values when the client scope is dynamic. For instance, {@code :}. + */ + String VALUE_SEPARATOR = ":"; + interface ClientScopeRemovedEvent extends ProviderEvent { ClientScopeModel getClientScope(); diff --git a/services/src/main/java/org/keycloak/organization/protocol/mappers/oidc/OrganizationScope.java b/services/src/main/java/org/keycloak/organization/protocol/mappers/oidc/OrganizationScope.java index b3e39f4dc9c..43a819383fd 100644 --- a/services/src/main/java/org/keycloak/organization/protocol/mappers/oidc/OrganizationScope.java +++ b/services/src/main/java/org/keycloak/organization/protocol/mappers/oidc/OrganizationScope.java @@ -17,6 +17,7 @@ package org.keycloak.organization.protocol.mappers.oidc; +import static org.keycloak.models.ClientScopeModel.VALUE_SEPARATOR; import static org.keycloak.organization.utils.Organizations.getProvider; import static org.keycloak.utils.StringUtil.isBlank; @@ -151,7 +152,6 @@ public enum OrganizationScope { private static final String ORGANIZATION_SCOPES_SESSION_ATTRIBUTE = "kc.org.client.scope"; private static final String UNSUPPORTED_ORGANIZATION_SCOPES_ATTRIBUTE = "kc.org.client.scope.unsupported"; - private static final char VALUE_SEPARATOR = ':'; private static final Pattern SCOPE_PATTERN = Pattern.compile("(.*)" + VALUE_SEPARATOR + "(.*)"); private static final String EMPTY_SCOPE = ""; 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 1cc782800f4..638b1cb2a06 100755 --- a/services/src/main/java/org/keycloak/protocol/oidc/TokenManager.java +++ b/services/src/main/java/org/keycloak/protocol/oidc/TokenManager.java @@ -637,6 +637,10 @@ public class TokenManager { return clientScopes; } + // skip scopes that were explicitly requested using the dynamic scope format + // we don't want dynamic and default client scopes duplicated + clientScopes = clientScopes.filter(scope -> !scopeParam.contains(scope.getName() + ClientScopeModel.VALUE_SEPARATOR)); + Map allOptionalScopes = client.getClientScopes(false); // Add optional client scopes requested by scope parameter diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/organization/mapper/OrganizationOIDCProtocolMapperTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/organization/mapper/OrganizationOIDCProtocolMapperTest.java index f281d80173a..049c68cf791 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/organization/mapper/OrganizationOIDCProtocolMapperTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/organization/mapper/OrganizationOIDCProtocolMapperTest.java @@ -22,6 +22,7 @@ import static org.hamcrest.Matchers.containsInAnyOrder; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.hasItem; +import static org.hamcrest.Matchers.hasSize; import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.not; import static org.hamcrest.Matchers.notNullValue; @@ -33,7 +34,6 @@ import static org.junit.Assert.assertTrue; import java.net.MalformedURLException; import java.net.URL; -import java.time.Duration; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -571,7 +571,6 @@ public class OrganizationOIDCProtocolMapperTest extends AbstractOrganizationTest @Test public void testAuthenticatingUsingBroker() { - driver.manage().timeouts().pageLoadTimeout(Duration.ofDays(1)); OrganizationResource organization = testRealm().organizations().get(createOrganization().getId()); IdentityProviderRepresentation idp = organization.identityProviders().get(bc.getIDPAlias()).toRepresentation(); idp.getConfig().put(OrganizationModel.ORGANIZATION_DOMAIN_ATTRIBUTE, "neworg.org"); @@ -664,7 +663,6 @@ public class OrganizationOIDCProtocolMapperTest extends AbstractOrganizationTest @Test public void testForceSelectingOrganizationWhenReAuthenticatingUsingDifferentClient() { - driver.manage().timeouts().pageLoadTimeout(Duration.ofDays(1)); OrganizationRepresentation orgA = createOrganization("orga", true); MemberRepresentation member = addMember(testRealm().organizations().get(orgA.getId()), "member@" + orgA.getDomains().iterator().next().getName()); OrganizationRepresentation orgB = createOrganization("orgb", true); @@ -707,7 +705,6 @@ public class OrganizationOIDCProtocolMapperTest extends AbstractOrganizationTest @Test public void testReAuthenticationUserMemberOfSingleOrganizationUsingDifferentClient() { - driver.manage().timeouts().pageLoadTimeout(Duration.ofDays(1)); OrganizationRepresentation orgA = createOrganization("orga", true); MemberRepresentation member = addMember(testRealm().organizations().get(orgA.getId()), "member@" + orgA.getDomains().iterator().next().getName()); ClientRepresentation client = testRealm().clients().findByClientId("broker-app").get(0); @@ -736,7 +733,6 @@ public class OrganizationOIDCProtocolMapperTest extends AbstractOrganizationTest @Test public void testReAuthenticationUserNotMemberOfOrganizationUsingDifferentClient() { - driver.manage().timeouts().pageLoadTimeout(Duration.ofDays(1)); OrganizationRepresentation orgA = createOrganization("orga", true); MemberRepresentation member = addMember(testRealm().organizations().get(orgA.getId()), "member@" + orgA.getDomains().iterator().next().getName()); testRealm().organizations().get(orgA.getId()).members().member(member.getId()).delete().close(); @@ -767,7 +763,6 @@ public class OrganizationOIDCProtocolMapperTest extends AbstractOrganizationTest @Test public void testDoNotAskToSelectOrganizationIfOrganizationScopeNotPresent() { - driver.manage().timeouts().pageLoadTimeout(Duration.ofDays(1)); OrganizationRepresentation orgA = createOrganization("orga", true); MemberRepresentation member = addMember(testRealm().organizations().get(orgA.getId()), "member@" + orgA.getDomains().iterator().next().getName()); OrganizationRepresentation orgB = createOrganization("orgb", true); @@ -795,7 +790,6 @@ public class OrganizationOIDCProtocolMapperTest extends AbstractOrganizationTest @Test public void testSelectDifferentOrganizationWhenReAuthenticating() { - driver.manage().timeouts().pageLoadTimeout(Duration.ofDays(1)); OrganizationRepresentation orgA = createOrganization("orga", true); MemberRepresentation member = addMember(testRealm().organizations().get(orgA.getId()), "member@" + orgA.getDomains().iterator().next().getName()); OrganizationRepresentation orgB = createOrganization("orgb", true); @@ -862,6 +856,58 @@ public class OrganizationOIDCProtocolMapperTest extends AbstractOrganizationTest assertScopeAndClaims(scopeName, orgA); } + @Test + public void testCustomOrganizationScopeNameAllOrganizations() { + OrganizationResource orga = testRealm().organizations().get(createOrganization("org-a").getId()); + OrganizationResource orgb = testRealm().organizations().get(createOrganization("org-b").getId()); + + addMember(orga); + + UserRepresentation member = getUserRepresentation(memberEmail); + + orgb.members().addMember(member.getId()).close(); + + Assert.assertTrue(orga.members().list(-1, -1).stream().map(UserRepresentation::getId).anyMatch(member.getId()::equals)); + Assert.assertTrue(orgb.members().list(-1, -1).stream().map(UserRepresentation::getId).anyMatch(member.getId()::equals)); + + ClientScopeRepresentation orgScope = testRealm().clientScopes().findAll().stream() + .filter(s -> OIDCLoginProtocolFactory.ORGANIZATION.equals(s.getName())) + .findAny() + .orElseThrow(); + ClientScopeResource orgScopeResource = testRealm().clientScopes().get(orgScope.getId()); + ProtocolMapperRepresentation orgMapper = orgScopeResource.getProtocolMappers().getMappers().stream() + .filter(m -> OIDCLoginProtocolFactory.ORGANIZATION.equals(m.getName())) + .findAny() + .orElseThrow(); + orgMapper.setId(null); + orgScope.setProtocolMappers(List.of(orgMapper)); + orgScope.setId(null); + orgScope.setName("org"); + String createdId = ApiUtil.getCreatedId(testRealm().clientScopes().create(orgScope)); + testRealm().addDefaultDefaultClientScope(createdId); + ClientRepresentation client = testRealm().clients().findByClientId("broker-app").get(0); + testRealm().clients().get(client.getId()).addDefaultClientScope(createdId); + getCleanup().addCleanup(() -> testRealm().clientScopes().get(createdId).remove()); + + oauth.client("broker-app", KcOidcBrokerConfiguration.CONSUMER_BROKER_APP_SECRET); + String scopeName = "org:*"; + oauth.scope(scopeName); + oauth.realm(bc.consumerRealmName()); + oauth.openLoginForm(); + loginPage.loginUsername(member.getEmail()); + loginPage.login(memberPassword); + + String code = oauth.parseLoginResponse().getCode(); + AccessTokenResponse response = oauth.doAccessTokenRequest(code); + assertThat(response.getScope(), containsString(scopeName)); + assertThat(List.of(response.getScope().split(" ")), not(hasItem("org"))); + AccessToken accessToken = oauth.verifyToken(response.getAccessToken()); + assertThat(accessToken.getScope(), containsString(scopeName)); + assertThat(List.of(accessToken.getScope().split(" ")), not(hasItem("org"))); + assertThat(accessToken.getOtherClaims().keySet(), hasItem(OAuth2Constants.ORGANIZATION)); + assertThat((List) accessToken.getOtherClaims().get(OAuth2Constants.ORGANIZATION), hasSize(2)); + } + @Test public void testClaimNotMappedIfUserNotMemberWhenDefaultClientScope() { OrganizationRepresentation orgARep = createOrganization("orga", true); @@ -1006,8 +1052,6 @@ public class OrganizationOIDCProtocolMapperTest extends AbstractOrganizationTest OrganizationResource orgA = testRealm().organizations().get(orgARep.getId()); MemberRepresentation member = addMember(orgA, "member@" + orgARep.getDomains().iterator().next().getName()); orgA.members().member(member.getId()).delete().close(); - driver.manage().timeouts().pageLoadTimeout(Duration.ofDays(1)); - // resolve organization based on the organization scope value oauth.client("broker-app", KcOidcBrokerConfiguration.CONSUMER_BROKER_APP_SECRET); oauth.scope(orgScope); loginPage.open(bc.consumerRealmName());