Skip to content
Permalink
Browse files

Merge pull request #31 from dwnusbaum/JENKINS-48917

[JENKINS-48917] Add option to ignore LDAP servers if they are unavailable.
  • Loading branch information
andresrc committed Feb 19, 2018
2 parents b4e383e + fcb7e53 commit 2a78aeff2839a57b4f20898564938fb0373659eb
@@ -7,3 +7,5 @@
/.project
/.settings
/.classpath
/nbproject
.DS_Store
@@ -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);
}
}
}
@@ -131,6 +131,12 @@
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()
@@ -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>
@@ -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>
@@ -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));
}

0 comments on commit 2a78aef

Please sign in to comment.
You can’t perform that action at this time.