From e6747bfd23c9c411e7d7c54bde39e16bb7580c5e Mon Sep 17 00:00:00 2001 From: Marek Posolda Date: Fri, 12 Apr 2024 14:13:03 +0200 Subject: [PATCH] Adjust priority of SubMapper (#28663) closes #28661 Co-authored-by: andymunro <48995441+andymunro@users.noreply.github.com> --- .../topics/clients/con-protocol-mappers.adoc | 6 +++++- .../upgrading/topics/changes/changes-25_0_0.adoc | 4 +++- .../keycloak/protocol/ProtocolMapperUtils.java | 15 ++++----------- .../oidc/mappers/SHA256PairwiseSubMapper.java | 3 +-- .../keycloak/protocol/oidc/mappers/SubMapper.java | 6 ++++++ .../OIDCPairwiseClientRegistrationTest.java | 5 ++++- 6 files changed, 23 insertions(+), 16 deletions(-) diff --git a/docs/documentation/server_admin/topics/clients/con-protocol-mappers.adoc b/docs/documentation/server_admin/topics/clients/con-protocol-mappers.adoc index 4fe5aa7adf..7e910a4fd5 100644 --- a/docs/documentation/server_admin/topics/clients/con-protocol-mappers.adoc +++ b/docs/documentation/server_admin/topics/clients/con-protocol-mappers.adoc @@ -33,6 +33,7 @@ You can use most OIDC mappers to control where the claim gets placed. You opt to include::proc-creating-mappers.adoc[] +[[_protocol-mappers_priority]] == Priority order Mapper implementations have _priority order_. _Priority order_ is not the configuration property of the mapper. It is the property of the concrete implementation of the mapper. @@ -71,7 +72,10 @@ When scripts deploy, you should be able to select the deployed scripts from the Subject claim _sub_ is mapped by default by *Subject (sub)* protocol mapper in the default client scope *basic*. -To use a pairwise subject identifier by using a protocol mapper such as *Pairwise subject identifier*, remove the *Subject (sub)* protocol mapper from the *basic* client scope. +To use a pairwise subject identifier by using a protocol mapper such as *Pairwise subject identifier*, you can remove the *Subject (sub)* protocol mapper from the *basic* client scope. +However it is not strictly needed as the *Subject (sub)* protocol mapper is executed before the *Pairwise subject identifier* mapper and hence the pairwise value will override the value added +by the Subject mapper. This is due to the <<_protocol-mappers_priority, priority>> of the Subject mapper. So the only advantage of removing the built-in *Subject (sub)* mapper might be to +save a little bit of performance by avoiding the use of the protocol mapper, which may not have any effect. [[_using_lightweight_access_token]] == Using lightweight access token diff --git a/docs/documentation/upgrading/topics/changes/changes-25_0_0.adoc b/docs/documentation/upgrading/topics/changes/changes-25_0_0.adoc index a041dad407..642bb29028 100644 --- a/docs/documentation/upgrading/topics/changes/changes-25_0_0.adoc +++ b/docs/documentation/upgrading/topics/changes/changes-25_0_0.adoc @@ -104,7 +104,9 @@ The `sub` claim, which was always added to the access token, is now added by def The `Subject (sub)` mapper is configured by default in the `basic` client scope. Therefore, no extra configuration is required after upgrading to this version. -Only in the case you are using `Pairwise subject identifier` mapper to map `sub` claim for access token you should disable or remove `Subject (sub)` mapper. +If you are using the `Pairwise subject identifier` mapper to map a `sub` claim for an access token, you can consider disabling or removing the `Subject (sub)` mapper, however it is not strictly needed +as the `Subject (sub)` protocol mapper is executed before the `Pairwise subject identifier` mapper and hence the `pairwise` value will override the value added by `Subject (sub)` mapper. +This may apply also to other custom protocol mapper implementations, which override the `sub` claim, as the `Subject (sub)` mapper is currently executed as first protocol mapper. You can use the `Subject (sub)` mapper to configure the `sub` claim only for access token, lightweight access token, and introspection response. IDToken and Userinfo always contain `sub` claim. diff --git a/services/src/main/java/org/keycloak/protocol/ProtocolMapperUtils.java b/services/src/main/java/org/keycloak/protocol/ProtocolMapperUtils.java index c61867a10e..8196177a25 100755 --- a/services/src/main/java/org/keycloak/protocol/ProtocolMapperUtils.java +++ b/services/src/main/java/org/keycloak/protocol/ProtocolMapperUtils.java @@ -68,6 +68,9 @@ public class ProtocolMapperUtils { public static final String MULTIVALUED_HELP_TEXT = "multivalued.tooltip"; public static final String AGGREGATE_ATTRS_HELP_TEXT = "aggregate.attrs.tooltip"; + // Priority of SubMapper. It should be first to allow other mappers override the `sub` claim + public static final int SUB_MAPPER = -10; + // Role name mapper can move some roles to different positions public static final int PRIORITY_ROLE_NAMES_MAPPER = 10; @@ -122,17 +125,7 @@ public class ProtocolMapperUtils { public static Stream> getSortedProtocolMappers(KeycloakSession session, ClientSessionContext ctx) { - KeycloakSessionFactory sessionFactory = session.getKeycloakSessionFactory(); - return ctx.getProtocolMappersStream() - .>map(mapperModel -> { - ProtocolMapper mapper = (ProtocolMapper) sessionFactory.getProviderFactory(ProtocolMapper.class, mapperModel.getProtocolMapper()); - if (mapper == null) { - return null; - } - return new AbstractMap.SimpleEntry<>(mapperModel, mapper); - }) - .filter(Objects::nonNull) - .sorted(Comparator.comparing(ProtocolMapperUtils::compare)); + return getSortedProtocolMappers(session, ctx, entry -> true); } public static Stream> getSortedProtocolMappers(KeycloakSession session, ClientSessionContext ctx, Predicate> filter) { diff --git a/services/src/main/java/org/keycloak/protocol/oidc/mappers/SHA256PairwiseSubMapper.java b/services/src/main/java/org/keycloak/protocol/oidc/mappers/SHA256PairwiseSubMapper.java index 8b28d472c7..1565fff319 100644 --- a/services/src/main/java/org/keycloak/protocol/oidc/mappers/SHA256PairwiseSubMapper.java +++ b/services/src/main/java/org/keycloak/protocol/oidc/mappers/SHA256PairwiseSubMapper.java @@ -58,8 +58,7 @@ public class SHA256PairwiseSubMapper extends AbstractPairwiseSubMapper { @Override public String getHelpText() { - return "Calculates a pairwise subject identifier using a salted sha-256 hash and adds it to the 'sub' claim. It is recommended to remove built-in 'sub' mapper when this one is used. " + - "See OpenID Connect specification for more info about pairwise subject identifiers."; + return "Calculates a pairwise subject identifier using a salted sha-256 hash and adds it to the 'sub' claim. See OpenID Connect specification for more info about pairwise subject identifiers."; } @Override diff --git a/services/src/main/java/org/keycloak/protocol/oidc/mappers/SubMapper.java b/services/src/main/java/org/keycloak/protocol/oidc/mappers/SubMapper.java index 7ecd01ada4..d267156297 100644 --- a/services/src/main/java/org/keycloak/protocol/oidc/mappers/SubMapper.java +++ b/services/src/main/java/org/keycloak/protocol/oidc/mappers/SubMapper.java @@ -22,6 +22,7 @@ import org.keycloak.models.ClientSessionContext; import org.keycloak.models.KeycloakSession; import org.keycloak.models.ProtocolMapperModel; import org.keycloak.models.UserSessionModel; +import org.keycloak.protocol.ProtocolMapperUtils; import org.keycloak.protocol.oidc.OIDCLoginProtocol; import org.keycloak.provider.ProviderConfigProperty; import org.keycloak.representations.IDToken; @@ -79,6 +80,11 @@ public class SubMapper extends AbstractOIDCProtocolMapper implements OIDCAccessT } } + @Override + public int getPriority() { + return ProtocolMapperUtils.SUB_MAPPER; + } + public static ProtocolMapperModel create(String name, boolean accessToken, boolean introspectionEndpoint) { ProtocolMapperModel mapper = new ProtocolMapperModel(); mapper.setName(name); diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/OIDCPairwiseClientRegistrationTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/OIDCPairwiseClientRegistrationTest.java index 9ad69ecc8d..f12ca3813f 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/OIDCPairwiseClientRegistrationTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/OIDCPairwiseClientRegistrationTest.java @@ -91,7 +91,10 @@ public class OIDCPairwiseClientRegistrationTest extends AbstractClientRegistrati OIDCClientRepresentation clientRep = createRep(); clientRep.setSubjectType("pairwise"); OIDCClientRepresentation pairwiseClient = reg.oidc().create(clientRep); - removeDefaultBasicClientScope(pairwiseClient.getClientId()); + + // No need to remove default sub mapper. As the pairwise sub mapper should be executed after the default one. + //removeDefaultBasicClientScope(pairwiseClient.getClientId()); + return pairwiseClient; }