Merge pull request #31 from dwnusbaum/JENKINS-48917

[JENKINS-48917] Add option to ignore LDAP servers if they are unavailable.
jenkinsci · Feb 19, 2018 · 2a78aef · 2a78aef
2 parents b4e383e + fcb7e53
commit 2a78aef
Show file tree

Hide file tree

Showing 7 changed files with 215 additions and 33 deletions.
diff --git a/.gitignore b/.gitignore
@@ -7,3 +7,5 @@
 /.project
 /.settings
 /.classpath
+/nbproject
+.DS_Store
diff --git a/src/main/java/hudson/security/LDAPSecurityRealm.java b/src/main/java/hudson/security/LDAPSecurityRealm.java
@@ -689,6 +689,7 @@ public LDAPConfiguration getConfigurationFor(String configurationId) {
                 return configurations.get(0);
             }
         }
+        LOGGER.log(Level.FINE, "Unable to find configuration for {0}", configurationId);
         return null;
     }
 
@@ -900,12 +901,11 @@ public GroupDetails loadGroupByGroupname(String groupname, boolean fetchMembers)
                     }
                     return groupDetails;
                 }
+            // Make sure we don't throw BadCredentialsException. Catch logic matches LDAPUserDetailsService#loadUserByUsername.
             } catch (DataAccessException e) {
-                LOGGER.log(Level.WARNING,
-                        String.format("Failed communication with ldap server %s (%s)",
-                                conf.getId(), conf.getServer()),
-                        e);
-                throw e;
+                throwUnlessConfigIsIgnorable(e, conf);
+            } catch (RuntimeException e) {
+                throwUnlessConfigIsIgnorable(new LdapDataAccessException("Failed to search LDAP for group: " + groupname, e), conf);
             }
         }
         throw new UsernameNotFoundException(groupname);
@@ -924,6 +924,17 @@ public DescriptorImpl getDescriptor() {
         return (DescriptorImpl) super.getDescriptor();
     }
 
+    private static <T extends Exception> void throwUnlessConfigIsIgnorable(T e, @CheckForNull LDAPConfiguration config) throws T {
+        boolean shouldThrow = config == null || !config.isIgnoreIfUnavailable();
+        LOGGER.log(Level.WARNING, String.format("Failed communication with ldap server %s (%s), will %s the next configuration",
+                config == null ? "null" : config.getId(),
+                config == null ? "null" : config.getServer(),
+                shouldThrow ? "_not_ try" : "try"), e);
+        if (shouldThrow) {
+            throw e;
+        }
+    }
+
     private static class GroupDetailsImpl extends GroupDetails {
 
         private final String dn;
@@ -989,7 +1000,7 @@ public Authentication authenticate(Authentication authentication) throws Authent
                     throw e;
                 }
             }
-            BadCredentialsException lastException = null;
+            AuthenticationException lastException = null;
             for (ManagerEntry delegate : delegates) {
                 try {
                     Authentication a = delegate.delegate.authenticate(authentication);
@@ -1003,17 +1014,18 @@ public Authentication authenticate(Authentication authentication) throws Authent
                             }
                         } catch (UsernameNotFoundException e1) {
                             lastException = e; //all is as intended, let's move along
+                        } catch (DataAccessException e1) {
+                            final LDAPConfiguration configuration = getConfigurationFor(delegate.configurationId);
+                            throwUnlessConfigIsIgnorable(e1, configuration);
+                            lastException = e;
                         }
                     } 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;
+                    throwUnlessConfigIsIgnorable(e, configuration);
+                    lastException = e;
                 }
             }
             if (lastException != null) {
@@ -1225,12 +1237,7 @@ public UserDetails loadUserByUsername(String username) throws UsernameNotFoundEx
                     lastUNFE = e;
                 } catch (DataAccessException 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;
+                    throwUnlessConfigIsIgnorable(e, configuration);
                 }
             }
             if (lastUNFE != null) {
@@ -1357,16 +1364,10 @@ public LdapUserDetails loadUserByUsername(String username) throws UsernameNotFou
                 }
 
                 return ldapUser;
-            } catch (LdapDataAccessException e) {
-                // TODO why not throw all DataAccessException up? that is why it is in the declared clause
-                LOGGER.log(Level.WARNING, "Failed to search LDAP for username="+username,e);
-                throw new UserMayOrMayNotExistException(e.getMessage(),e);
-            } catch (UsernameNotFoundException x) {
-                throw x;
-            } catch (DataAccessException x) {
+            } catch (DataAccessException | UsernameNotFoundException x) {
                 throw x;
             } catch (RuntimeException x) {
-                throw new LdapDataAccessException("Failed to search LDAP for " + username + ": " + x, x);
+                throw new LdapDataAccessException("Failed to search LDAP for " + username, x);
             }
         }
     }

diff --git a/src/main/java/jenkins/security/plugins/ldap/LDAPConfiguration.java b/src/main/java/jenkins/security/plugins/ldap/LDAPConfiguration.java
@@ -131,6 +131,12 @@ public class LDAPConfiguration extends AbstractDescribableImpl<LDAPConfiguration
     private final Secret managerPasswordSecret;
     private String displayNameAttributeName;
     private String mailAddressAttributeName;
+    /**
+     * If true, then any operation using this configuration which fails to connect to the server will try
+     * again using the next configuration. This could be a security issue if the same username exists in
+     * multiple LDAP configurations but should not correspond to the same Jenkins user, so it defaults to false.
+     */
+    private boolean ignoreIfUnavailable;
     private Map<String,String> extraEnvVars;
     /**
      * Set in {@link #createApplicationContext(LDAPSecurityRealm, boolean)}
@@ -332,6 +338,15 @@ public void setMailAddressAttributeName(String mailAddressAttributeName) {
         this.mailAddressAttributeName = mailAddressAttributeName;
     }
 
+    public boolean isIgnoreIfUnavailable() {
+        return ignoreIfUnavailable;
+    }
+
+    @DataBoundSetter
+    public void setIgnoreIfUnavailable(boolean ignoreIfUnavailable) {
+        this.ignoreIfUnavailable = ignoreIfUnavailable;
+    }
+
     public Map<String,String> getExtraEnvVars() {
         return extraEnvVars == null || extraEnvVars.isEmpty()
                 ? Collections.<String,String>emptyMap()

diff --git a/src/main/resources/jenkins/security/plugins/ldap/LDAPConfiguration/config.jelly b/src/main/resources/jenkins/security/plugins/ldap/LDAPConfiguration/config.jelly
@@ -47,5 +47,8 @@
                 </f:entry>
             </f:repeatableProperty>
         </f:entry>
+        <f:entry field="ignoreIfUnavailable" title="${%Ignore if Unavailable}">
+            <f:checkbox />
+        </f:entry>
     </f:advanced>
 </j:jelly>
diff --git a/...n/resources/jenkins/security/plugins/ldap/LDAPConfiguration/help-ignoreIfUnavailable.html b/...n/resources/jenkins/security/plugins/ldap/LDAPConfiguration/help-ignoreIfUnavailable.html
@@ -0,0 +1,18 @@
+<div>
+    <p>
+    If checked, this option indicates that authentication and user/group membership lookups
+    which fail due to communication errors (e.g., LDAP server is unreachable, manager password is
+    incorrect) will be reattempted using the next configuration. Other types of failures, such as
+    a user attempting to authenticate with an incorrect password, will not be reattempted.
+    </p>
+    <p>
+    If unchecked (the default), authentication and user/group membership lookups which fail due to
+    communication errors will not be reattempted using the next configuration.
+    </p>
+    <p>
+    The default is intended to prevent users or groups with the same name in distinct LDAP domains
+    from being inadvertently mapped to the same user or group in Jenkins if one of the domains is
+    unavailable. Consequently, this option should only be enabled if all configured domains have
+    distinct user and group names.
+    </p>
+</div>
diff --git a/src/test/java/hudson/security/LDAPSecurityRealmTest.java b/src/test/java/hudson/security/LDAPSecurityRealmTest.java
@@ -275,6 +275,7 @@ public void configRoundTripTwo() throws Exception {
             TestConf conf = confs[i];
             final LDAPConfiguration configuration = new LDAPConfiguration(conf.server, conf.rootDN, false, conf.managerDN, Secret.fromString(conf.managerSecret));
             configuration.setUserSearchBase(conf.userSearchBase);
+            configuration.setIgnoreIfUnavailable(i % 2 == 0);
             ldapConfigurations.add(configuration);
         }
         final LDAPSecurityRealm realm = new LDAPSecurityRealm(ldapConfigurations,
@@ -301,6 +302,7 @@ public void configRoundTripTwo() throws Exception {
             assertEquals(LDAPSecurityRealm.DescriptorImpl.DEFAULT_USER_SEARCH, config.getUserSearch());
             assertEquals(LDAPSecurityRealm.DescriptorImpl.DEFAULT_DISPLAYNAME_ATTRIBUTE_NAME, config.getDisplayNameAttributeName());
             assertEquals(LDAPSecurityRealm.DescriptorImpl.DEFAULT_MAILADDRESS_ATTRIBUTE_NAME, config.getMailAddressAttributeName());
+            assertEquals(i % 2 == 0, config.isIgnoreIfUnavailable());
         }
         assertThat(newRealm.getUserIdStrategy(), instanceOf(IdStrategy.CaseInsensitive.class));
     }