refactor(active-directory): unify username handling with suffix and DN support

fix warnings

Author: lprimak

core/src/main/java/org/apache/shiro/realm/activedirectory/ActiveDirectoryRealm.java

@@ -24,6 +24,7 @@
 import org.apache.shiro.authc.UsernamePasswordToken;
 import org.apache.shiro.authz.AuthorizationInfo;
 import org.apache.shiro.authz.SimpleAuthorizationInfo;
+import org.apache.shiro.lang.util.StringUtils;
 import org.apache.shiro.realm.Realm;
 import org.apache.shiro.realm.ldap.AbstractLdapRealm;
 import org.apache.shiro.realm.ldap.LdapContextFactory;
@@ -110,7 +111,7 @@ protected AuthenticationInfo queryForAuthenticationInfo(AuthenticationToken toke
         // Binds using the username and password provided by the user.
         LdapContext ctx = null;
         try {
-            ctx = ldapContextFactory.getLdapContext(getUsernameForAuthentication(upToken.getUsername()),
+            ctx = ldapContextFactory.getLdapContext(getUsernameWithSuffixOrFullDN(upToken.getUsername()),
                     String.valueOf(upToken.getPassword()));
         } finally {
             LdapUtils.closeContext(ctx);
@@ -169,7 +170,7 @@ protected Set<String> getRoleNamesForUser(String username, LdapContext ldapConte
         SearchControls searchControls = new SearchControls();
         searchControls.setSearchScope(SearchControls.SUBTREE_SCOPE);
 
-        String userPrincipalName = getUsernameForAuthentication(username);
+        String userPrincipalName = getUsernameWithSuffixOrFullDN(username);
 
         Object[] searchArguments = new Object[] {userPrincipalName};
 
@@ -235,6 +236,43 @@ protected Collection<String> getRoleNamesForGroups(Collection<String> groupNames
         return roleNames;
     }
 
+    /**
+     * Returns the username to use for authentication.
+     * If {@link #principalSuffix} is configured, the sanitized username with appended suffix will be returned.
+     * If {@link #principalSuffix} is not configured, the method will check if the username is a valid LDAP DN.
+     * If it is a valid LDAP DN, the username will be returned as-is,
+     * otherwise the sanitized username with appended suffix will be returned.
+     *
+     * @param username input to check and sanitize
+     * @return the sanitized username with optional suffix to use for authentication
+     */
+    protected String getUsernameWithSuffixOrFullDN(String username) {
+        if (!StringUtils.hasText(principalSuffix)) {
+            try {
+                LdapName ldapName = new LdapName(username);
+                // Full LDAP DN needs to have more than one RDN, so we can return the username as-is
+                if (ldapName.size() > 1) {
+                    return username;
+                }
+            } catch (javax.naming.InvalidNameException e) {
+                // Not a valid LDAP DN, so treat it as a regular username.
+            }
+        }
+
+        return getUsernameWithSuffix(username);
+    }
+
+    /**
+     * Returns the sanitized username with appended suffix if {@link #principalSuffix} is configured
+     * and the username does not already end with it.
+     * If {@link #principalSuffix} is not configured, the sanitized username will be returned
+     * <p>
+     * NOTE: {@link #getUsernameWithSuffixOrFullDN(String)} should be used instead of this method
+     * to handle full LDAP DNs correctly.
+     *
+     * @param username input to sanitize and append suffix
+     * @return the sanitized username with optional suffix
+     */
     protected String getUsernameWithSuffix(String username) {
         String sanitizedUsername = Rdn.escapeValue(username);
         if (principalSuffix != null
@@ -243,18 +281,4 @@ protected String getUsernameWithSuffix(String username) {
         }
         return sanitizedUsername;
     }
-
-    protected String getUsernameForAuthentication(String username) {
-        try {
-            LdapName ldapName = new LdapName(username);
-            if (ldapName.size() > 1) {
-                return username;
-            }
-        } catch (javax.naming.InvalidNameException e) {
-            // Not a valid LDAP DN, so treat it as a regular username.
-        }
-
-        return getUsernameWithSuffix(username);
-    }
-
 }

core/src/test/java/org/apache/shiro/mgt/AbstractRememberMeManagerObjectInputFilterTest.java

@@ -19,18 +19,21 @@
 package org.apache.shiro.mgt;
 
 import org.apache.shiro.lang.io.Serializer;
+import org.apache.shiro.subject.ImmutablePrincipalCollection;
 import org.apache.shiro.subject.PrincipalCollection;
-import org.apache.shiro.subject.SimplePrincipalCollection;
 import org.apache.shiro.subject.Subject;
 import org.apache.shiro.subject.SubjectContext;
 import org.apache.shiro.subject.support.DefaultSubjectContext;
 import org.junit.jupiter.api.Test;
 
+import java.io.ByteArrayInputStream;
 import java.io.ByteArrayOutputStream;
 import java.io.IOException;
 import java.io.InvalidClassException;
 import java.io.ObjectInputFilter;
+import java.io.ObjectInputStream;
 import java.io.ObjectOutputStream;
+import java.io.Serial;
 import java.io.Serializable;
 import java.util.ArrayList;
 import java.util.List;
@@ -51,7 +54,7 @@ class AbstractRememberMeManagerObjectInputFilterTest {
     @Test
     void testLegitimatePrincipalsRoundTripUnderDefaultFilter() {
         InMemoryRememberMeManager rmm = new InMemoryRememberMeManager();
-        PrincipalCollection principals = new SimplePrincipalCollection("joecool", "myRealm");
+        PrincipalCollection principals = ImmutablePrincipalCollection.ofSinglePrincipal("joecool", "myRealm");
 
         rmm.rememberIdentity(null, principals);
         PrincipalCollection remembered = rmm.getRememberedPrincipals(new DefaultSubjectContext());
@@ -95,18 +98,18 @@ void testDefaultFilterRejectsOversizedPayloadBeforeFullConstruction() {
     @Test
     void testCustomStricterAllowListFilterCanBeConfigured() {
         // Documented override path (see AbstractRememberMeManager#getSerializer javadoc): replace the default
-        // serializer's filter with a strict class allow-list. Only SimplePrincipalCollection,
+        // serializer's filter with a strict class allow-list. Only ImmutablePrincipalCollection,
         // AbstractRememberMeManager.RememberedIdentity, and JDK collection/primitive/java.time plumbing are let
         // through. Note: java.time types (e.g. Instant) don't serialize themselves directly - they writeReplace()
         // to an internal java.time serialization proxy class, which is what actually appears in the stream.
         InMemoryRememberMeManager rmm = new InMemoryRememberMeManager();
         rmm.getSerializer()
                 .setObjectInputFilter(ObjectInputFilter.Config.createFilter(
-                        "org.apache.shiro.subject.SimplePrincipalCollection;"
+                        "org.apache.shiro.subject.ImmutablePrincipalCollection;"
                                 + "org.apache.shiro.mgt.AbstractRememberMeManager$RememberedIdentity;"
                                 + "java.time.*;java.util.*;java.lang.*;!*"));
 
-        PrincipalCollection principals = new SimplePrincipalCollection("joecool", "myRealm");
+        PrincipalCollection principals = ImmutablePrincipalCollection.ofSinglePrincipal("joecool", "myRealm");
         rmm.rememberIdentity(null, principals);
         PrincipalCollection remembered = rmm.getRememberedPrincipals(new DefaultSubjectContext());
         assertThat(remembered.getPrimaryPrincipal()).isEqualTo("joecool");
@@ -126,25 +129,24 @@ void testCustomSerializerIsUnaffectedByDefaultFilterMachinery() {
         // A caller-supplied Serializer implementation (not a DefaultSerializer) must keep working exactly as
         // before this feature existed - AbstractRememberMeManager only touches the filter on its own default
         // DefaultSerializer instance, never on a replaced Serializer.
-        InMemoryRememberMeManager rmm = new InMemoryRememberMeManager();
-        rmm.setSerializer(new Serializer<AbstractRememberMeManager.RememberedIdentity>() {
+        var rmm = new InMemoryRememberMeManager();
+        rmm.setSerializer(new Serializer<>() {
             @Override
             public byte[] serialize(AbstractRememberMeManager.RememberedIdentity o) {
                 return plainJdkSerialize(o);
             }
 
             @Override
-            @SuppressWarnings("unchecked")
             public AbstractRememberMeManager.RememberedIdentity deserialize(byte[] serialized) {
-                try (var ois = new java.io.ObjectInputStream(new java.io.ByteArrayInputStream(serialized))) {
+                try (var ois = new ObjectInputStream(new ByteArrayInputStream(serialized))) {
                     return (AbstractRememberMeManager.RememberedIdentity) ois.readObject();
                 } catch (IOException | ClassNotFoundException e) {
                     throw new RuntimeException(e);
                 }
             }
         });
 
-        PrincipalCollection principals = new SimplePrincipalCollection("joecool", "myRealm");
+        PrincipalCollection principals = ImmutablePrincipalCollection.ofSinglePrincipal("joecool", "myRealm");
         rmm.rememberIdentity(null, principals);
         PrincipalCollection remembered = rmm.getRememberedPrincipals(new DefaultSubjectContext());
 
@@ -164,6 +166,7 @@ private static byte[] plainJdkSerialize(Object o) {
     }
 
     public static class NotAllowlisted implements Serializable {
+        @Serial
         private static final long serialVersionUID = 1L;
     }
 

core/src/test/java/org/apache/shiro/realm/activedirectory/ActiveDirectoryRealmTest.java

@@ -55,6 +55,7 @@
 import javax.naming.directory.SearchControls;
 import javax.naming.directory.SearchResult;
 import javax.naming.ldap.LdapContext;
+import javax.naming.ldap.Rdn;
 import java.util.HashSet;
 import java.util.Set;
 
@@ -153,30 +154,37 @@ void testInitialization() {
     }
 
     @Test
-    void testUsernameForAuthenticationWithDn() {
-        ActiveDirectoryRealm activeDirectoryRealm = new ActiveDirectoryRealm() {{
-            this.principalSuffix = "@example.com";
-        }};
+    void usernameForAuthenticationWithDnAndSuffix() {
+        var activeDirectoryRealm = new ActiveDirectoryRealm();
+        activeDirectoryRealm.setPrincipalSuffix("@example.com");
 
         String dn = "CN=my_name,OU=Development,OU=Special Accounts,DC=mycompany,DC=com";
 
-        assertThat(activeDirectoryRealm.getUsernameForAuthentication(dn))
+        assertThat(activeDirectoryRealm.getUsernameWithSuffixOrFullDN(dn))
+                .isEqualTo(Rdn.escapeValue(dn) + "@example.com");
+    }
+
+    @Test
+    void usernameForAuthenticationWithDn() {
+        var activeDirectoryRealm = new ActiveDirectoryRealm();
+
+        String dn = "CN=my_name,OU=Development,OU=Special Accounts,DC=mycompany,DC=com";
+
+        assertThat(activeDirectoryRealm.getUsernameWithSuffixOrFullDN(dn))
                 .isEqualTo(dn);
     }
 
     @Test
     void testUsernameForAuthenticationWithUsername() {
-        ActiveDirectoryRealm activeDirectoryRealm = new ActiveDirectoryRealm();
+        var activeDirectoryRealm = new ActiveDirectoryRealm();
 
-        assertThat(activeDirectoryRealm.getUsernameForAuthentication("test,user"))
+        assertThat(activeDirectoryRealm.getUsernameWithSuffixOrFullDN("test,user"))
                 .isEqualTo("test\\,user");
     }
 
     @Test
-    void testAuthenticationUsesDnWithoutEscaping() throws Exception {
-        ActiveDirectoryRealm activeDirectoryRealm = new ActiveDirectoryRealm() {{
-            this.principalSuffix = "@example.com";
-        }};
+    void authenticationUsesDnWithoutEscaping() throws Exception {
+        var activeDirectoryRealm = new ActiveDirectoryRealm();
         LdapContextFactory factory = createMock(LdapContextFactory.class);
         LdapContext ldapContext = createNiceMock(LdapContext.class);
 
@@ -193,10 +201,8 @@ void testAuthenticationUsesDnWithoutEscaping() throws Exception {
     }
 
     @Test
-    void testAuthorizationUsesDnWithoutEscaping() throws Exception {
-        ActiveDirectoryRealm activeDirectoryRealm = new ActiveDirectoryRealm() {{
-            this.principalSuffix = "@example.com";
-        }};
+    void authorizationUsesDnWithoutEscaping() throws Exception {
+        var activeDirectoryRealm = new ActiveDirectoryRealm();
 
         LdapContext ldapContext = createNiceMock(LdapContext.class);
         NamingEnumeration<SearchResult> results = createNiceMock(NamingEnumeration.class);
@@ -232,9 +238,8 @@ public void assertExistingUserSuffix(String username, String expectedPrincipalNa
                 .andReturn(results);
         replay(ldapContext);
 
-        ActiveDirectoryRealm activeDirectoryRealm = new ActiveDirectoryRealm() {{
-            this.principalSuffix = "@ExAmple.COM";
-        }};
+        var activeDirectoryRealm = new ActiveDirectoryRealm();
+        activeDirectoryRealm.setPrincipalSuffix("@ExAmple.COM");
 
         SecurityManager securityManager = new DefaultSecurityManager(activeDirectoryRealm);
         Subject subject = new Subject.Builder(securityManager).buildSubject();
@@ -277,10 +282,6 @@ public Optional<AuthenticationInfo> createSimulatedCredentials() {
             setCredentialsMatcher(credentialsMatcher);
         }
 
-        public void setPrincipalSuffix(String principalSuffix) {
-            this.principalSuffix = principalSuffix;
-        }
-
         protected AuthenticationInfo doGetAuthenticationInfo(AuthenticationToken token) throws AuthenticationException {
             SimpleAccount account = (SimpleAccount) super.doGetAuthenticationInfo(token);
 
@@ -296,14 +297,14 @@ protected AuthenticationInfo doGetAuthenticationInfo(AuthenticationToken token)
         }
 
         protected AuthorizationInfo doGetAuthorizationInfo(PrincipalCollection principals) {
-            Set<String> roles = new HashSet<String>();
+            Set<String> roles = new HashSet<>();
             roles.add(ROLE);
             return new SimpleAuthorizationInfo(roles);
         }
 
         // override ldap query because i don't care about testing that piece in this case
-        protected AuthenticationInfo queryForAuthenticationInfo(AuthenticationToken token, LdapContextFactory ldapContextFactory)
-                throws NamingException {
+        protected AuthenticationInfo queryForAuthenticationInfo(AuthenticationToken token,
+                                                                LdapContextFactory ldapContextFactory) {
             return new SimpleAccount(token.getPrincipal(), token.getCredentials(), getName());
         }
     }