Skip to content

GroupCache should cache when no value found for a key #115

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

GroupCache should cache when no value found for a key #115

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3e1b898
cache should cache empty result
olamy Nov 24, 2021
82886c0
do not cache such error
olamy Nov 24, 2021
393f681
fix import
olamy Nov 24, 2021
73017ee
fix other caches
olamy Nov 25, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 23 additions & 11 deletions src/main/java/hudson/plugins/active_directory/ActiveDirectoryAuthenticationProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@
import java.io.IOException;
import java.util.ArrayList;
import java.util.List;
import java.util.Optional;
import java.util.function.Function;
import java.util.logging.Level;
import java.util.logging.Logger;
Expand Down Expand Up @@ -86,12 +87,12 @@ public class ActiveDirectoryAuthenticationProvider extends AbstractActiveDirecto
/**
* The {@link UserDetails} cache.
*/
private final Cache<CacheKey, UserDetails> userCache;
private final Cache<CacheKey, Optional<UserDetails>> userCache;

/**
* The {@link ActiveDirectoryGroupDetails} cache.
*/
private final Cache<String, ActiveDirectoryGroupDetails> groupCache;
private final Cache<String, Optional<GroupDetails>> groupCache;

public ActiveDirectoryAuthenticationProvider() throws IOException {
this(null);
Expand Down Expand Up @@ -159,7 +160,7 @@ protected UserDetails retrieveUser(final String username,final UsernamePassword
}
final CacheKey cacheKey = CacheUtil.computeCacheKey(username, password, userCache.asMap().keySet());

final Function<CacheKey, UserDetails> userDetailsFunction = cacheKey1 ->
final Function<CacheKey, Optional<UserDetails>> userDetailsFunction = cacheKey1 ->
{
String dn = getDnOfUserOrGroup(username);

Expand Down Expand Up @@ -187,7 +188,7 @@ protected UserDetails retrieveUser(final String username,final UsernamePassword
throw new BadCredentialsException(msg, e);
}
if (usr == null) // the user name was in fact a group
throw new UsernameNotFoundException("User not found: "+ username);
return Optional.empty();

List<GrantedAuthority> groups = new ArrayList<>();
for( Com4jObject g : usr.groups() ) {
Expand All @@ -202,19 +203,28 @@ protected UserDetails retrieveUser(final String username,final UsernamePassword

LOGGER.log(Level.FINE, "Login successful: {0} dn={1}", new Object[] {username, dn});

return new ActiveDirectoryUserDetail(
return Optional.of(new ActiveDirectoryUserDetail(
username, "redacted",
!isAccountDisabled(usr),
true, true, true,
groups.toArray(new GrantedAuthority[0]),
getFullName(usr), getEmailAddress(usr), getTelephoneNumber(usr)
).updateUserInfo();
).updateUserInfo());
} finally {
col.disposeAll();
COM4J.removeListener(col);
}
};
return cacheKey == null ? userDetailsFunction.apply(null): userCache.get(cacheKey, userDetailsFunction);
if (cacheKey == null) {
return userDetailsFunction.apply(null).orElseThrow(() -> new UsernameNotFoundException("User not found: "+ username));
}
Optional<UserDetails> opt = userCache.get(cacheKey, userDetailsFunction);
// NPE check to make spotbugs happy
if (opt==null) {
throw new UsernameNotFoundException("User not found: "+ username);
}
return opt.orElseThrow(() -> new UsernameNotFoundException("User not found: "+ username));

} catch (Exception e) {
if (e instanceof AuthenticationException) {
throw (AuthenticationException)e;
Expand Down Expand Up @@ -289,7 +299,7 @@ private String getDnOfUserOrGroup(String userOrGroupname) throws UsernameNotFoun

public GroupDetails loadGroupByGroupname(final String groupname) {
try {
return groupCache.get(groupname, s -> {
Optional<GroupDetails> opt = groupCache.get(groupname, s -> {
ComObjectCollector col = new ComObjectCollector();
COM4J.addListener(col);
try {
Expand All @@ -303,10 +313,9 @@ public GroupDetails loadGroupByGroupname(final String groupname) {
if (group == null) {
throw new UserMayOrMayNotExistException(groupname);
}
return new ActiveDirectoryGroupDetails(groupname);
return Optional.of(new ActiveDirectoryGroupDetails(groupname));
} catch (UsernameNotFoundException e) {
// failed to convert group name to DN
throw new UsernameNotFoundException("Failed to get the DN of the group " + groupname);
return Optional.empty();
} catch (ComException e) {
// recover gracefully since AD might behave in a way we haven't anticipated
LOGGER.log(Level.WARNING, String.format("Failed to figure out details of AD group: %s", groupname), e);
Expand All @@ -316,6 +325,9 @@ public GroupDetails loadGroupByGroupname(final String groupname) {
COM4J.removeListener(col);
}
});
// NPE check to make spotbugs happy
if (opt==null) throw new UsernameNotFoundException("Failed to get the DN of the group " + groupname);
return opt.orElseThrow(() -> new UsernameNotFoundException("Failed to get the DN of the group " + groupname));
} catch (Exception e) {
if (e instanceof AuthenticationException) {
throw (AuthenticationException)e;
Expand Down
30 changes: 20 additions & 10 deletions src/main/java/hudson/plugins/active_directory/ActiveDirectoryUnixAuthenticationProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,13 +64,15 @@
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.Optional;
import java.util.Set;
import java.util.Stack;
import java.util.concurrent.ArrayBlockingQueue;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.ThreadPoolExecutor;
import java.util.concurrent.TimeUnit;
import java.util.function.Function;
import java.util.function.Supplier;
import java.util.logging.Level;
import java.util.logging.Logger;

Expand Down Expand Up @@ -105,12 +107,12 @@ public class ActiveDirectoryUnixAuthenticationProvider extends AbstractActiveDir
/**
* The {@link UserDetails} cache.
*/
private final Cache<CacheKey, UserDetails> userCache;
private final Cache<CacheKey, Optional<UserDetails>> userCache;

/**
* The {@link ActiveDirectoryGroupDetails} cache.
*/
private final Cache<String, ActiveDirectoryGroupDetails> groupCache;
private final Cache<String, Optional<GroupDetails>> groupCache;

/**
* The threadPool to update the cache on background
Expand Down Expand Up @@ -334,7 +336,7 @@ public UserDetails retrieveUser(final String username, final Password password,

try {
final ActiveDirectoryUserDetail[] cacheMiss = new ActiveDirectoryUserDetail[1];
final Function<CacheKey, UserDetails> cacheKeyUserDetailsFunction = cacheKey1 ->
final Function<CacheKey, Optional<UserDetails>> cacheKeyUserDetailsFunction = cacheKey1 ->
{
DirContext context;
boolean anonymousBind = false; // did we bind anonymously?
Expand Down Expand Up @@ -391,7 +393,7 @@ public UserDetails retrieveUser(final String username, final Password password,
LOGGER.log(Level.FINE, "Failed to find {0} in userPrincipalName. Trying sAMAccountName", userPrincipalName);
user = new LDAPSearchBuilder(context, domainDN).subTreeScope().searchOne("(& (sAMAccountName={0})(objectCategory=user))", samAccountName);
if (user == null) {
throw new UsernameNotFoundException("Authentication was successful but cannot locate the user information for " + username);
return Optional.empty();
}
}
LOGGER.fine("Found user " + username + " : " + user);
Expand Down Expand Up @@ -427,7 +429,7 @@ public UserDetails retrieveUser(final String username, final Password password,
getStringAttribute(user, "mail"),
getStringAttribute(user, "telephoneNumber")
);
return cacheMiss[0];
return Optional.of(cacheMiss[0]);
} catch (NamingException e) {
if (activeDirectoryInternalUser != null) {
throw new RuntimeException(e);
Expand All @@ -451,8 +453,12 @@ public UserDetails retrieveUser(final String username, final Password password,
closeQuietly(context);
}
};
userDetails = cacheKey == null ? cacheKeyUserDetailsFunction.apply(null) : userCache.get(cacheKey, cacheKeyUserDetailsFunction);
if (cacheMiss[0] != null || cacheKey == null) { // If a lookup was performed
Supplier<Exception> userNameNotFound = () -> new UsernameNotFoundException("Authentication was successful but cannot locate the user information for " + username);
if(cacheKey==null) return cacheKeyUserDetailsFunction.apply(null).orElseThrow(userNameNotFound);
Optional<UserDetails> opt = userCache.get(cacheKey, cacheKeyUserDetailsFunction);
if(opt==null) throw userNameNotFound.get();
userDetails = opt.orElseThrow(userNameNotFound);
if (cacheMiss[0] != null) { // If a lookup was performed
threadPoolExecutor.execute(() -> {
final String threadName = Thread.currentThread().getName();
Thread.currentThread().setName(threadName + " updating-cache-for-user-" + cacheMiss[0].getUsername());
Expand Down Expand Up @@ -511,7 +517,7 @@ private boolean userMatchesInternalDatabaseUser(String username) {

public GroupDetails loadGroupByGroupname(final String groupname) {
try {
return groupCache.get(groupname, s -> {
Optional<GroupDetails> opt = groupCache.get(groupname, s -> {
for (ActiveDirectoryDomain domain : domains) {
if (domain==null) {
throw new UserMayOrMayNotExistException("Unable to retrieve group information without bind DN/password configured");
Expand Down Expand Up @@ -550,7 +556,7 @@ public GroupDetails loadGroupByGroupname(final String groupname) {
continue;
}
LOGGER.log(Level.FINE, "Found group {0} : {1}", new Object[] {groupname, group});
return new ActiveDirectoryGroupDetails(groupname);
return Optional.of(new ActiveDirectoryGroupDetails(groupname));
} catch (NamingException e) {
LOGGER.log(Level.WARNING, String.format("Failed to retrieve user information for %s", groupname), e);
throw new BadCredentialsException("Failed to retrieve user information for "+ groupname, e);
Expand All @@ -562,13 +568,17 @@ public GroupDetails loadGroupByGroupname(final String groupname) {
} catch (AuthenticationException e) {
// something went wrong talking to the server. This should be reported
LOGGER.log(Level.WARNING, String.format("Failed to find the group %s in %s domain", groupname, domain.getName()), e);
throw e;
} finally {
Thread.currentThread().setContextClassLoader(ccl);
}
}
LOGGER.log(Level.WARNING, "Exhausted all configured domains and could not authenticate against any");
throw new UserMayOrMayNotExistException(groupname);
return Optional.empty();
});
// NPE check to make spotbugs happy...
if (opt == null) throw new UserMayOrMayNotExistException(groupname);
return opt.orElseThrow(() -> new UserMayOrMayNotExistException(groupname));
} catch (Exception e) {
if (e instanceof AuthenticationException) {
throw e;
Expand Down
13 changes: 7 additions & 6 deletions src/main/java/hudson/plugins/active_directory/CacheConfiguration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,28 +2,29 @@

import com.github.benmanes.caffeine.cache.Cache;
import com.github.benmanes.caffeine.cache.Caffeine;
import com.github.benmanes.caffeine.cache.LoadingCache;
import hudson.security.GroupDetails;
import org.acegisecurity.userdetails.UserDetails;
import org.kohsuke.stapler.DataBoundConstructor;

import java.util.Optional;
import java.util.concurrent.TimeUnit;

/**
* Cache configuration
*/
public class CacheConfiguration<K,V,E extends Exception> {
public class CacheConfiguration {
private final int size;
private final int ttl;

/**
* The {@link UserDetails} cache.
*/
private transient final Cache<CacheKey, UserDetails> userCache;
private transient final Cache<CacheKey, Optional<UserDetails>> userCache;

/**
* The {@link ActiveDirectoryGroupDetails} cache.
*/
private transient final Cache<String, ActiveDirectoryGroupDetails> groupCache;
private transient final Cache<String, Optional<GroupDetails>> groupCache;

/**
* CacheConfiguration DataBoundConstructor
Expand Down Expand Up @@ -70,7 +71,7 @@ public int getTtl() {
*
* @return the cache for users
*/
public Cache<CacheKey, UserDetails> getUserCache() {
public Cache<CacheKey, Optional<UserDetails>> getUserCache() {
return userCache;
}

Expand All @@ -79,7 +80,7 @@ public Cache<CacheKey, UserDetails> getUserCache() {
*
* @return the cache for groups
*/
public Cache<String, ActiveDirectoryGroupDetails> getGroupCache() {
public Cache<String, Optional<GroupDetails>> getGroupCache() {
return groupCache;
}
}