spring-projects / spring-projects/spring-security

CachingUserDetailsService doesn't properly support expiraton of cache

Open
#16,645 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

status: waiting-for-triage type: bug
Dominant language
Java
Stars
9.6k
Forks
6.3k
Avg merge
2d 11h
Merged PRs (30d)
52

Description

Describe the bug
Using Spring Security 6.3.1 we're using LdapUserDetailsService to fetch user data and authorities. To decrease number of requests to LDAP infrastructure, we're using CachingUserDetailsService configured with LdapUserDetailsService as delegate, and SpringCacheBasedUserCache backed by Caffeine.
Caffeine is set up with "expireAfterWrite=1m" spec, so authorities of user are cached 1 minute after LDAP call.

The issue: delegate method LdapUserDetailsService::loadUserByUsername should be called every minute if user is sending requests in small intervals (<1 minute), but it's called only once per request batch - each access of user details in CachingUserDetailsService refreshes the cache with cached value:

@Override
public UserDetails loadUserByUsername(String username) {
	UserDetails user = this.userCache.getUserFromCache(username);
	if (user == null) {
		user = this.delegate.loadUserByUsername(username);
	}
	Assert.notNull(user, () -> "UserDetailsService " + this.delegate + " returned null for username " + username
			+ ". " + "This is an interface contract violation");

	// FIXME it's putting value read from cache back into the cache, thus resetting expiryAfterWrite timeout
	this.userCache.putUserInCache(user);
	return user;
}

CachingUserDetailsService

moving putUserInCache just after user is loaded from delegate inside the if block should fix the issue.

To Reproduce
Configure UserDetailsService as:

@Bean 
UserDetailsService userDetailsService(LdapUserSearch search, LdapAuthoritiesPopulator populator, LdapDetailsMapper mapper) {
    // ldap search
    var delegate = new LdapUserDetailsService(search, populator);
    delegate.setUserDetailsMapper(mapper);

    // cache definition
    var caffeine = new CaffeineCacheManager();
    caffeine.setCacheSpecification("expireAfterWrite=1m");

    //final bean
    var userDetailsService = new CachingUserDetailsService(delegate);
    userDetailsService.setUserCache(new SpringCacheBasedUserCache(caffeine.getCache("userCache")));
    return userDetailsService;
}

then require user detais more often that expiryAfterWrite cache timeout.

Expected behavior
Cache expires a minute after first requests, refreshes authorities from LDAP.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start in core/src/main/java/org/springframework/security/authentication/CachingUserDetailsService.java, especially loadUserByUsername, and reproduce the behavior with the Caffeine expireAfterWrite=1m configuration described in the issue. Done means repeated requests no longer reset the original cache expiry, so the delegate refreshes user details after the timeout.

Written by the indexing model from the issue text.

Assessment

Tech stack
java
Domain
authentication
Issue type
Bug
Difficulty
2/5
Estimated time
1-3 hours
Activity status
Stale
Clarity
Clearly specified
Newbie friendliness
55/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.