Merge pull request #364 from apache/shiro-871-19x

Apply principalSuffix only when username does NOT already contain the suffix
apache · Jun 23, 2022 · 422166d · 422166d
2 parents a259d26 + 0db440a
commit 422166d
Show file tree

Hide file tree

Showing 3 changed files with 69 additions and 1 deletion.
diff --git a/core/src/main/java/org/apache/shiro/realm/activedirectory/ActiveDirectoryRealm.java b/core/src/main/java/org/apache/shiro/realm/activedirectory/ActiveDirectoryRealm.java
@@ -163,7 +163,7 @@ protected Set<String> getRoleNamesForUser(String username, LdapContext ldapConte
         searchCtls.setSearchScope(SearchControls.SUBTREE_SCOPE);
 
         String userPrincipalName = username;
-        if (principalSuffix != null) {
+        if (principalSuffix != null && !userPrincipalName.toLowerCase(Locale.ROOT).endsWith(principalSuffix.toLowerCase(Locale.ROOT))) {
             userPrincipalName += principalSuffix;
         }
 

diff --git a/core/src/main/java/org/apache/shiro/realm/ldap/AbstractLdapRealm.java b/core/src/main/java/org/apache/shiro/realm/ldap/AbstractLdapRealm.java
@@ -63,6 +63,18 @@ public abstract class AbstractLdapRealm extends AuthorizingRealm {
     /*--------------------------------------------
     |    I N S T A N C E   V A R I A B L E S    |
     ============================================*/
+
+    /**
+     * Defines the Suffix added to the User Principal Name when looking up groups (e.g. "memberOf")
+     * AD Example:
+     * User's Principal Name be "John.Doe"
+     * User's E-Mail Address be "John.Doe@example.com"
+     *
+     * For the example below, set:
+     *      realm.principalSuffix = @example.com
+     *
+     * Only then, "John.Doe" and also "John.Doe@example.com" can authorize against groups
+     */
     protected String principalSuffix = null;
 
     protected String searchBase = null;

diff --git a/core/src/test/java/org/apache/shiro/realm/activedirectory/ActiveDirectoryRealmTest.java b/core/src/test/java/org/apache/shiro/realm/activedirectory/ActiveDirectoryRealmTest.java
@@ -24,6 +24,7 @@
 import org.apache.shiro.authz.AuthorizationInfo;
 import org.apache.shiro.authz.SimpleAuthorizationInfo;
 import org.apache.shiro.mgt.DefaultSecurityManager;
+import org.apache.shiro.mgt.SecurityManager;
 import org.apache.shiro.realm.AuthorizingRealm;
 import org.apache.shiro.realm.UserIdPrincipal;
 import org.apache.shiro.realm.UsernamePrincipal;
@@ -32,14 +33,29 @@
 import org.apache.shiro.subject.SimplePrincipalCollection;
 import org.apache.shiro.subject.Subject;
 import org.apache.shiro.util.ThreadContext;
+import org.easymock.Capture;
+import org.easymock.CaptureType;
 import org.junit.After;
+import org.junit.Assert;
 import org.junit.Before;
 import org.junit.Test;
 
+import javax.naming.NamingEnumeration;
 import javax.naming.NamingException;
+import javax.naming.directory.SearchControls;
+import javax.naming.directory.SearchResult;
+import javax.naming.ldap.LdapContext;
 import java.util.HashSet;
 import java.util.Set;
 
+import static org.easymock.EasyMock.anyObject;
+import static org.easymock.EasyMock.anyString;
+import static org.easymock.EasyMock.capture;
+import static org.easymock.EasyMock.createMock;
+import static org.easymock.EasyMock.expect;
+import static org.easymock.EasyMock.replay;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.*;
 import static org.junit.Assert.assertTrue;
 
 
@@ -97,6 +113,43 @@ public void testDefaultConfig() {
         subject.logout();
     }
 
+    @Test
+    public void testExistingUserSuffix() throws Exception {
+        assertExistingUserSuffix(USERNAME, "testuser@ExAmple.COM"); // suffix case matches configure suffix
+
+        // suffix matches user entry
+        assertExistingUserSuffix(USERNAME + "@example.com", "testuser@example.com");
+        assertExistingUserSuffix(USERNAME + "@EXAMPLE.com", "testuser@EXAMPLE.com");
+    }
+
+    public void assertExistingUserSuffix(String username, String expectedPrincipalName) throws Exception {
+
+        LdapContext ldapContext = createMock(LdapContext.class);
+        NamingEnumeration<SearchResult> results = createMock(NamingEnumeration.class);
+        Capture<Object[]> captureArgs = Capture.newInstance(CaptureType.ALL);
+        expect(ldapContext.search(anyString(), anyString(), capture(captureArgs), anyObject(SearchControls.class))).andReturn(results);
+        replay(ldapContext);
+
+        ActiveDirectoryRealm activeDirectoryRealm = new ActiveDirectoryRealm() {{
+            this.principalSuffix = "@ExAmple.COM";
+        }};
+
+        SecurityManager securityManager = new DefaultSecurityManager(activeDirectoryRealm);
+        Subject subject = new Subject.Builder(securityManager).buildSubject();
+        subject.execute(() -> {
+
+            try {
+                activeDirectoryRealm.getRoleNamesForUser(username, ldapContext);
+            } catch (NamingException e) {
+                Assert.fail("Unexpected NamingException thrown during test");
+            }
+        });
+
+        Object[] args = captureArgs.getValue();
+        assertThat(args, arrayWithSize(1));
+        assertThat(args[0], is(expectedPrincipalName));
+    }
+
     public class TestActiveDirectoryRealm extends ActiveDirectoryRealm {
 
         /*--------------------------------------------
@@ -117,6 +170,9 @@ public boolean doCredentialsMatch(AuthenticationToken object, AuthenticationInfo
             setCredentialsMatcher(credentialsMatcher);
         }
 
+        public void setPrincipalSuffix(String principalSuffix) {
+            this.principalSuffix = principalSuffix;
+        }
 
         protected AuthenticationInfo doGetAuthenticationInfo(AuthenticationToken token) throws AuthenticationException {
             SimpleAccount account = (SimpleAccount) super.doGetAuthenticationInfo(token);