[FIXED JENKINS-45576] AD recognizes groups by CN and sAMAccount when …

…authorities only works with CN (#81) [JENKINS-45576] Removing the ability to search groups by sammaccountname.
jenkinsci · Sep 21, 2017 · eae7cb8 · eae7cb8
1 parent e286fd3
commit eae7cb8
Show file tree

Hide file tree

Showing 3 changed files with 92 additions and 4 deletions.
diff --git a/src/main/java/hudson/plugins/active_directory/ActiveDirectoryUnixAuthenticationProvider.java b/src/main/java/hudson/plugins/active_directory/ActiveDirectoryUnixAuthenticationProvider.java
@@ -525,12 +525,23 @@ public ActiveDirectoryGroupDetails call() throws NamingException {
                                         if (group==null) {
                                             // failed to find it. Fall back to sAMAccountName.
                                             // see http://www.nabble.com/Re%3A-Hudson-AD-plug-in-td21428668.html
+                                            // new link: https://web.archive.org/web/20090321130122/http://www.nabble.com/Re%3A-Hudson-AD-plug-in-td21428668.html
                                             LOGGER.log(Level.FINE, "Failed to find {0} in cn. Trying sAMAccountName", groupname);
                                             group = new LDAPSearchBuilder(context,domainDN).subTreeScope().searchOne("(& (sAMAccountName={0})(objectCategory=group))", groupname);
-                                            if (group==null) {
-                                                // Group not found in this domain, try next
-                                                continue;
+
+                                            // https://issues.jenkins-ci.org/browse/JENKINS-45576
+                                            // Fall back to sAMAccountName for groups is causing issues with AD exchange alias.
+                                            //   -> see https://technet.microsoft.com/en-us/library/cc539081.aspx
+                                            // When a user login Jenkins, their groups are resolved from the CN (not the sAMAccountName). 
+                                            // If one AD group have different values for CN and sAMAccountName then Jenkins will consider that group as different depending
+                                            // on if you used the CN or the sAMAccountName to get the group. 
+                                            // Ignoring the sAMAccountName search seems to be the best option to avoid confusion
+                                            if (group!=null) {
+                                                String cn = group.get("CN").get().toString();
+                                                LOGGER.log(Level.WARNING, String.format("JENKINS-45576: `%s` is an Exchange alias and aliases are not currently supported. Please use the group common name `%s` instead", groupname, cn));
+                                                group = null;
                                             }
+                                            continue;
                                         }
                                         LOGGER.log(Level.FINE, "Found group {0} : {1}", new Object[] {groupname, group});
                                         return new ActiveDirectoryGroupDetails(groupname);

diff --git a/src/test/java/hudson/plugins/active_directory/docker/TheFlintstonesTest.java b/src/test/java/hudson/plugins/active_directory/docker/TheFlintstonesTest.java
@@ -38,15 +38,29 @@
 import org.junit.Before;
 import org.junit.Rule;
 import org.junit.Test;
+import org.jvnet.hudson.test.Issue;
 import org.jvnet.hudson.test.JenkinsRule;
 
 import java.util.ArrayList;
+import java.util.Collection;
 import java.util.List;
 
 import static org.junit.Assert.assertThat;
 import static org.hamcrest.Matchers.is;
 import static org.junit.Assert.assertTrue;
 
+import java.util.logging.Formatter;
+import java.util.logging.Level;
+import java.util.logging.LogRecord;
+import java.util.logging.Logger;
+import java.util.logging.SimpleFormatter;
+
+import com.google.common.base.Predicates;
+import com.google.common.collect.Collections2;
+
+import hudson.security.GroupDetails;
+import hudson.util.RingBufferLogHandler;
+
 /**
  * Integration tests with Docker
  */
@@ -101,6 +115,54 @@ public void simpleLoginFails() throws Exception {
         }
     }
 
+    @Issue("JENKINS-45576")
+    @Test
+    public void loadGroupFromGroupname() throws Exception {
+        String groupname = "The Rubbles";
+        GroupDetails group = j.jenkins.getSecurityRealm().loadGroupByGroupname(groupname);
+        assertThat(group.getName(), is("The Rubbles"));
+    }
+
+    @Issue("JENKINS-45576")
+    @Test
+    public void loadGroupFromAlias() throws Exception {
+        // required to monitor the log messages, removing this line the test will fail
+        List<String> logMessages = captureLogMessages(20);
+
+        String aliasname = "Rubbles";
+        boolean isAlias = false;
+        try {
+            j.jenkins.getSecurityRealm().loadGroupByGroupname(aliasname);
+        } catch (Exception e) {
+        } finally {
+            Collection<String> filter = Collections2.filter(logMessages, Predicates.containsPattern("JENKINS-45576"));
+            isAlias = !filter.isEmpty();
+        }
+
+        assertTrue(isAlias);
+    }
+
+    private List<String> captureLogMessages(int size) {
+        final List<String> logMessages = new ArrayList<String>(size);
+        Logger logger = Logger.getLogger("");
+        logger.setLevel(Level.ALL);
+
+        RingBufferLogHandler ringHandler = new RingBufferLogHandler(size) {
+
+            final Formatter f = new SimpleFormatter(); // placeholder instance for what should have been a static method perhaps
+            @Override
+            public synchronized void publish(LogRecord record) {
+                super.publish(record);
+                String message = f.formatMessage(record);
+                Throwable x = record.getThrown();
+                logMessages.add(message == null && x != null ? x.toString() : message);
+            }
+        };
+        logger.addHandler(ringHandler);
+
+        return logMessages;
+    }
+
     @DockerFixture(id = "ad-dc", ports= {389, 3268})
     public static class TheFlintstones extends DockerContainer {
 

diff --git a/...ources/hudson/plugins/active_directory/docker/TheFlintstonesTest/TheFlintstones/custom.sh b/...ources/hudson/plugins/active_directory/docker/TheFlintstonesTest/TheFlintstones/custom.sh
@@ -6,12 +6,27 @@
 
 # add groups
 samba-tool group add The-Flintstones
+samba-tool group add "The Rubbles"
 
 # add users
 samba-tool user add Fred ia4uV1EeKait
 samba-tool user add Wilma ia4uV1EeKait
-
+samba-tool user add Barney ia4uV1EeKait
+samba-tool user add Betty ia4uV1EeKait
 
 # add users to groups
 samba-tool group addmembers The-Flintstones Fred
 samba-tool group addmembers The-Flintstones Wilma
+samba-tool group addmembers "The Rubbles" Barney
+
+# add alias for the "The Rubbles"
+{ cat > file.ldif <<-EOF
+dn: CN=The Rubbles,cn=Users,dc=samdom,dc=example,dc=com
+changetype: modify
+replace: sAMAccountName
+sAMAccountName: Rubbles
+EOF
+} && ldapmodify -a -h 127.0.0.1 -p 389 -D "cn=Administrator,cn=Users,dc=samdom,dc=example,dc=com" -w "ia4uV1EeKait" -f file.ldif
+
+# add Betty to Rubbles alias
+samba-tool group addmembers Rubbles Betty