Adjust priority of SubMapper (#28663)

closes #28661


Co-authored-by: andymunro <48995441+andymunro@users.noreply.github.com>

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

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.
 

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<Entry<ProtocolMapperModel, ProtocolMapper>> getSortedProtocolMappers(KeycloakSession session, ClientSessionContext ctx) {
-        KeycloakSessionFactory sessionFactory = session.getKeycloakSessionFactory();
+        return getSortedProtocolMappers(session, ctx, entry -> true);
-        return ctx.getProtocolMappersStream()
-                .<Entry<ProtocolMapperModel, ProtocolMapper>>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));
     }
 
     public static Stream<Entry<ProtocolMapperModel, ProtocolMapper>> getSortedProtocolMappers(KeycloakSession session, ClientSessionContext ctx, Predicate<Entry<ProtocolMapperModel, ProtocolMapper>> filter) {

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. " +
+        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.";
-                "See OpenID Connect specification for more info about pairwise subject identifiers.";
     }
 
     @Override

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);

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;
     }