Don't create multiple GenericPrincipals per Subject

apache · Sep 18, 2024 · 29e23c6 · 29e23c6
1 parent 54160f7
commit 29e23c6
Show file tree

Hide file tree

Showing 2 changed files with 52 additions and 29 deletions.
diff --git a/java/org/apache/catalina/authenticator/jaspic/CallbackHandlerImpl.java b/java/org/apache/catalina/authenticator/jaspic/CallbackHandlerImpl.java
@@ -18,9 +18,11 @@
 
 import java.io.IOException;
 import java.security.Principal;
+import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collections;
 import java.util.List;
+import java.util.Set;
 
 import javax.security.auth.Subject;
 import javax.security.auth.callback.Callback;
@@ -58,9 +60,11 @@ public void handle(Callback[] callbacks) throws IOException, UnsupportedCallback
         String[] groups = null;
 
         if (callbacks != null) {
-            // Need to combine data from multiple callbacks so use this to hold
-            // the data
-            // Process the callbacks
+            /*
+             * There may be multiple callbacks passed to this method and/or multiple calls to this method. The Jakarta
+             * Authentication specification recommends that this class does not maintain state for individual requests
+             * and that the Subject is used to maintain state.
+             */
             for (Callback callback : callbacks) {
                 if (callback instanceof CallerPrincipalCallback) {
                     CallerPrincipalCallback cpc = (CallerPrincipalCallback) callback;
@@ -88,36 +92,50 @@ public void handle(Callback[] callbacks) throws IOException, UnsupportedCallback
                 }
             }
 
-            // Create the GenericPrincipal
-            Principal gp = getPrincipal(principal, name, groups);
-            if (subject != null && gp != null) {
-                subject.getPrivateCredentials().add(gp);
-            }
-        }
-    }
+            // If subject is null, there is nothing to do
+            if (subject != null) {
 
+                // Need a name to create a Principal
+                if (name == null && principal != null) {
+                    name = principal.getName();
+                }
 
-    private Principal getPrincipal(Principal principal, String name, String[] groups) {
-        // If the Principal is cached in the session JASPIC may simply return it
-        if (principal instanceof GenericPrincipal) {
-            return principal;
-        }
-        if (name == null && principal != null) {
-            name = principal.getName();
-        }
-        if (name == null) {
-            return null;
-        }
-        List<String> roles;
-        if (groups == null || groups.length == 0) {
-            roles = Collections.emptyList();
-        } else {
-            roles = Arrays.asList(groups);
+                if (name != null) {
+                    // If the Principal has been cached in the session, just return it.
+                    if (principal instanceof GenericPrincipal) {
+                        // Duplicates are unlikely and will be handled in AuthenticatorBase.getPrincipal()
+                        subject.getPrivateCredentials().add(principal);
+                    } else {
+                        /*
+                         * There should only be a single GenericPrincipal in the private credentials for the Subject. If
+                         * one is already present, merge the groups to create a new GenericPrincipal. The code assumes
+                         * that the name and principal (if any) will be the same.
+                         */
+                        List<String> mergedRoles = new ArrayList<>();
+
+                        Set<GenericPrincipal> gps = subject.getPrivateCredentials(GenericPrincipal.class);
+                        if (!gps.isEmpty()) {
+                            GenericPrincipal gp = gps.iterator().next();
+                            mergedRoles.addAll(Arrays.asList(gp.getRoles()));
+                            // Remove the existing GenericPrincipal
+                            subject.getPrivateCredentials().remove(gp);
+                        }
+                        if (groups != null) {
+                            mergedRoles.addAll(Arrays.asList(groups));
+                        }
+
+                        if (mergedRoles.size() == 0) {
+                            mergedRoles = Collections.emptyList();
+                        }
+
+                        subject.getPrivateCredentials().add(new GenericPrincipal(name, mergedRoles, principal));
+                    }
+                }
+            }
         }
-
-        return new GenericPrincipal(name, roles, principal);
     }
 
+
     // Contained interface methods
     @Override
     public Container getContainer() {

diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
@@ -109,7 +109,12 @@
     <changelog>
       <fix>
         Ensure that <code>ServerAuthModule.initialize()</code> is called when
-        the module is configured via <code>registerServerAuthModule()</code>.
+        a Jakarta Authentication module is configured via
+        <code>registerServerAuthModule()</code>. (markt)
+      </fix>
+      <fix>
+        Ensure that the Jakarta Authentication <code>CallbackHandler</code> only
+        creates one <code>GenericPrincipal</code> in the <code>Subject</code>.
         (markt)
       </fix>
     </changelog>