Merge branch 'master' into JENKINS-21784

jenkinsci · Dec 13, 2017 · 7440833 · 7440833
2 parents b520d73 + a62fbf7
commit 7440833
Show file tree

Hide file tree

Showing 3 changed files with 205 additions and 9 deletions.
diff --git a/pom.xml b/pom.xml
@@ -9,7 +9,7 @@
   </parent>
 
   <artifactId>ldap</artifactId>
-  <version>1.18-SNAPSHOT</version>
+  <version>1.19-SNAPSHOT</version>
   <packaging>hpi</packaging>
 
   <name>LDAP Plugin</name>
@@ -27,6 +27,7 @@
     <apacheds.version>2.0.0-M23</apacheds.version>
     <apacheds.jdbm.version>2.0.0-M3</apacheds.jdbm.version>
     <jenkins.version>1.651.3</jenkins.version>
+    <jenkins-test-harness.version>2.32</jenkins-test-harness.version>
   </properties>
 
   <scm>

diff --git a/src/main/java/hudson/security/LDAPSecurityRealm.java b/src/main/java/hudson/security/LDAPSecurityRealm.java
@@ -51,6 +51,7 @@
 import org.acegisecurity.Authentication;
 import org.acegisecurity.AuthenticationException;
 import org.acegisecurity.AuthenticationManager;
+import org.acegisecurity.AuthenticationServiceException;
 import org.acegisecurity.BadCredentialsException;
 import org.acegisecurity.GrantedAuthority;
 import org.acegisecurity.GrantedAuthorityImpl;
@@ -691,6 +692,16 @@ public LDAPConfiguration getConfigurationFor(String configurationId) {
         return null;
     }
 
+    @CheckForNull
+    private static LDAPConfiguration _getConfigurationFor(String configurationId) {
+        final SecurityRealm securityRealm = Jenkins.getActiveInstance().getSecurityRealm();
+        if (securityRealm instanceof LDAPSecurityRealm) {
+            return ((LDAPSecurityRealm) securityRealm).getConfigurationFor(configurationId);
+        }
+
+        return null;
+    }
+
     @Restricted(NoExternalUse.class)
     public static String toProviderUrl(String serverUrl, String rootDN) {
         StringBuilder buf = new StringBuilder();
@@ -964,7 +975,12 @@ private void addDelegate(final AuthenticationManager delegate, final String conf
 
         public Authentication authenticate(Authentication authentication) throws AuthenticationException {
             if (delegates.size() == 1) {
-                return updateUserDetails(delegates.get(0).delegate.authenticate(authentication));
+                try {
+                    return updateUserDetails(delegates.get(0).delegate.authenticate(authentication));
+                } catch (AuthenticationServiceException e) {
+                    LOGGER.log(Level.WARNING, "Failed communication with ldap server.", e);
+                    throw e;
+                }
             }
             BadCredentialsException lastException = null;
             for (ManagerEntry delegate : delegates) {
@@ -984,6 +1000,13 @@ public Authentication authenticate(Authentication authentication) throws Authent
                     } else {
                         lastException = e;
                     }
+                } catch (AuthenticationServiceException e) {
+                    final LDAPConfiguration configuration = getConfigurationFor(delegate.configurationId);
+                    LOGGER.log(Level.WARNING,
+                            String.format("Failed communication with ldap server %s (%s)",
+                                    delegate.configurationId, configuration != null ? configuration.getServer() : "null"),
+                            e);
+                    throw e;
                 }
             }
             if (lastException != null) {
@@ -1160,11 +1183,20 @@ public boolean contains(LDAPUserDetailsService delegate) {
         public DelegatedLdapUserDetails loadUserByUsername(String configurationId, String username) throws UsernameNotFoundException, DataAccessException {
             for (LDAPUserDetailsService delegate : delegates) {
                 if (delegate.configurationId.equals(configurationId)) {
-                    LdapUserDetails userDetails = delegate.loadUserByUsername(username);
-                    if (userDetails instanceof DelegatedLdapUserDetails) {
-                        return (DelegatedLdapUserDetails)userDetails;
-                    } else {
-                        return new DelegatedLdapUserDetails(userDetails, delegate.configurationId);
+                    try {
+                        LdapUserDetails userDetails = delegate.loadUserByUsername(username);
+                        if (userDetails instanceof DelegatedLdapUserDetails) {
+                            return (DelegatedLdapUserDetails)userDetails;
+                        } else {
+                            return new DelegatedLdapUserDetails(userDetails, delegate.configurationId);
+                        }
+                    } catch (DataAccessException e) {
+                        final LDAPConfiguration configuration = _getConfigurationFor(delegate.configurationId);
+                        LOGGER.log(Level.WARNING,
+                                String.format("Failed communication with ldap server %s (%s)",
+                                        delegate.configurationId, configuration != null ? configuration.getServer() : "null"),
+                                e);
+                        throw e;
                     }
                 }
             }
@@ -1185,7 +1217,12 @@ public UserDetails loadUserByUsername(String username) throws UsernameNotFoundEx
                 } catch (UsernameNotFoundException e) {
                     lastUNFE = e;
                 } catch (DataAccessException e) {
-                    LOGGER.log(Level.WARNING, "LDAP connection " + delegate.configurationId + " seems to be broken, will _not_ try the next configuration.", e);
+                    LDAPConfiguration configuration = _getConfigurationFor(delegate.configurationId);
+                    LOGGER.log(Level.WARNING,
+                            "LDAP connection "
+                                    + delegate.configurationId
+                                    + (configuration != null ? "("+configuration.getServer()+")" : "")
+                                    + " seems to be broken, will _not_ try the next configuration.", e);
                     throw e;
                 }
             }

diff --git a/src/test/java/hudson/security/LdapMultiEmbeddedTest.java b/src/test/java/hudson/security/LdapMultiEmbeddedTest.java
@@ -1,24 +1,34 @@
 package hudson.security;
 
+import com.gargoylesoftware.htmlunit.FailingHttpStatusCodeException;
 import hudson.model.User;
 import hudson.tasks.Mailer;
 import hudson.util.Secret;
 import jenkins.model.IdStrategy;
 import jenkins.security.plugins.ldap.*;
+import org.hamcrest.CoreMatchers;
 import org.junit.Before;
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.rules.RuleChain;
+import org.jvnet.hudson.test.LoggerRule;
 import org.jvnet.hudson.test.JenkinsRule;
+import org.springframework.dao.DataAccessException;
 
 import java.util.ArrayList;
 import java.util.Arrays;
+import java.util.List;
+import java.util.logging.Level;
 
+import static org.hamcrest.CoreMatchers.allOf;
 import static org.hamcrest.Matchers.*;
 import static org.hamcrest.core.StringContains.containsString;
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertThat;
+import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
+import static org.jvnet.hudson.test.LoggerRule.recorded;
 
 /**
  * Tests connecting to two different embedded servers using slightly different configurations.
@@ -28,8 +38,9 @@ public class LdapMultiEmbeddedTest {
     public LDAPRule sevenSeas = new LDAPRule();
     public LDAPRule planetExpress = new LDAPRule();
     public JenkinsRule r = new JenkinsRule();
+    public LoggerRule log = new LoggerRule().record(LDAPSecurityRealm.class, Level.WARNING).capture(100);
     @Rule
-    public RuleChain chain = RuleChain.outerRule(sevenSeas).around(planetExpress).around(r);
+    public RuleChain chain = RuleChain.outerRule(sevenSeas).around(planetExpress).around(r).around(log);
 
     @Before
     public void setup() throws Exception {
@@ -135,4 +146,151 @@ public void loginWithBrokenServerInTheMiddle() throws Exception {
         }
 
     }
+
+    public static final String FAILED_COMMUNICATION_WITH_LDAP_SERVER = "Failed communication with ldap server";
+
+    private void setBadPwd(LDAPRule rule) {
+        final LDAPSecurityRealm realm = (LDAPSecurityRealm)r.jenkins.getSecurityRealm();
+        LDAPConfiguration repl = null;
+        int index = -1;
+        final List<LDAPConfiguration> configurations = realm.getConfigurations();
+        for (int i = 0; i < configurations.size(); i++) {
+            LDAPConfiguration configuration = configurations.get(i);
+            if (configuration.getServer().equals(rule.getUrl())) {
+                repl = configuration;
+                index = i;
+                break;
+            }
+        }
+        assertNotNull(repl);
+        assertTrue(index >= 0);
+
+        LDAPConfiguration nc = new LDAPConfiguration(
+                repl.getServer(),
+                repl.getRootDN(),
+                true,
+                repl.getManagerDN(),
+                Secret.fromString("something completely wrong"));
+        configurations.set(index, nc);
+        r.jenkins.setSecurityRealm(new LDAPSecurityRealm(configurations,
+                realm.disableMailAddressResolver,
+                realm.getCache(),
+                realm.getUserIdStrategy(),
+                realm.getGroupIdStrategy()));
+    }
+
+    @Test
+    public void when_first_is_wrong_and_login_on_first_then_log() throws Exception {
+        setBadPwd(planetExpress);
+        try {
+            r.createWebClient().login("fry", "fry");
+            fail("Login should fail");
+        } catch (FailingHttpStatusCodeException e) {
+            assertEquals(500, e.getStatusCode());
+        }
+        assertThat(log, recorded(Level.WARNING,
+                allOf(containsString(FAILED_COMMUNICATION_WITH_LDAP_SERVER),
+                        containsString(planetExpress.getUrl())),
+                CoreMatchers.<Throwable>instanceOf(DataAccessException.class)));
+    }
+
+    @Test
+    public void when_first_is_wrong_and_login_on_second_then_log() throws Exception {
+        setBadPwd(planetExpress);
+        try {
+            r.createWebClient().login("hnelson", "pass");
+            fail("Login should fail");
+        } catch (FailingHttpStatusCodeException e) {
+            assertEquals(500, e.getStatusCode());
+        }
+        assertThat(log, recorded(Level.WARNING,
+                allOf(containsString(FAILED_COMMUNICATION_WITH_LDAP_SERVER),
+                        containsString(planetExpress.getUrl())),
+                CoreMatchers.<Throwable>instanceOf(DataAccessException.class)));
+    }
+
+    @Test
+    public void when_second_is_wrong_and_login_on_second_then_log() throws Exception {
+        setBadPwd(sevenSeas);
+        try {
+            r.createWebClient().login("hnelson", "pass");
+            fail("Login should fail");
+        } catch (FailingHttpStatusCodeException e) {
+            assertEquals(500, e.getStatusCode());
+        }
+        assertThat(log, recorded(Level.WARNING,
+                allOf(containsString(FAILED_COMMUNICATION_WITH_LDAP_SERVER),
+                        containsString(sevenSeas.getUrl())),
+                CoreMatchers.<Throwable>instanceOf(DataAccessException.class)));
+    }
+
+    @Test
+    public void when_second_is_wrong_and_login_on_first_no_log() throws Exception {
+        setBadPwd(sevenSeas);
+        r.createWebClient().login("fry", "fry");
+
+        assertThat(log, not(recorded(Level.WARNING,
+                containsString(FAILED_COMMUNICATION_WITH_LDAP_SERVER))));
+    }
+
+
+    @Test
+    public void when_first_is_wrong_and_lookup_on_first_then_log() throws Exception {
+        setBadPwd(planetExpress);
+
+        try {
+            r.jenkins.getSecurityRealm().loadUserByUsername("fry");
+        } catch (DataAccessException _) {
+            //all is as expected
+        }
+        assertThat(log, recorded(Level.WARNING,
+                allOf(containsString("LDAP connection"),
+                        containsString("seems to be broken, will _not_ try the next configuration"),
+                        containsString(planetExpress.getUrl())),
+                CoreMatchers.<Throwable>instanceOf(DataAccessException.class)));
+    }
+
+    @Test
+    public void when_first_is_wrong_and_lookup_on_second_then_log() throws Exception {
+        setBadPwd(planetExpress);
+        try {
+            r.jenkins.getSecurityRealm().loadUserByUsername("hnelson");
+            fail("Expected a DataAccessException");
+        } catch (DataAccessException _) {
+            //all is as expected
+        }
+        assertThat(log, recorded(Level.WARNING,
+                allOf(containsString("LDAP connection"),
+                        containsString("seems to be broken, will _not_ try the next configuration"),
+                        containsString(planetExpress.getUrl())),
+                CoreMatchers.<Throwable>instanceOf(DataAccessException.class)));
+    }
+
+    @Test
+    public void when_second_is_wrong_and_lookup_on_second_then_log() throws Exception {
+        setBadPwd(sevenSeas);
+        try {
+            r.jenkins.getSecurityRealm().loadUserByUsername("hnelson");
+            fail("Expected a DataAccessException");
+        } catch (DataAccessException _) {
+            //all is as expected
+        }
+        assertThat(log, recorded(Level.WARNING,
+                allOf(containsString("LDAP connection"),
+                        containsString("seems to be broken, will _not_ try the next configuration"),
+                        containsString(sevenSeas.getUrl())),
+                CoreMatchers.<Throwable>instanceOf(DataAccessException.class)));
+    }
+
+    @Test
+    public void when_second_is_wrong_and_lookup_on_first_no_log() throws Exception {
+        setBadPwd(sevenSeas);
+        try {
+            r.jenkins.getSecurityRealm().loadUserByUsername("fry");
+        } catch (DataAccessException _) {
+            fail("No exception should be thrown when first is working");
+        }
+        assertThat(log, not(recorded(Level.WARNING,
+                containsString(FAILED_COMMUNICATION_WITH_LDAP_SERVER))));
+    }
 }