Introduction

I recently stumbled over the following code in our codebase:

// Security flaw!
@Cacheable("permissionsOnContracts")
public boolean isAllowedToRead(ContractId contractId) {
    return expensiveOperationOrRemoteCall(contractId); 
}

The intent of the Cacheable-Annotation was obviously to reduce calls of #expensiveOperationOrRemoteCall, which makes sense in a way, especially when #isAllowedToRead is called often. Note that the #expensiveOperationOrRemoteCall determines the calling user internally by itself (eg. by using the SecurityContext, an OAuth2-Token, …).

It also introduces a severe security problem.

The Problem

But there is one little thing missing: the Cacheable-Annotation simply stores the result ignoring the user context. And the outcome of #isAllowedToRead is – of course – highly dependent on whether or not the current user is allowed to read a given contract. Which means, if an outcome of #isAllowedToRead is cached for a given ContractId, the result is applied to every call that follows using the same ContractId.

Example: User A is allowed to read the Contract with id “123”, User B is not.

1. User A calls #isAllowedToRead(ContractId.of(“123”))
2. The cache holds no entry for “123” so the method will be executed
3. #isAllowedToRead returns “true”, this is cached by @Cacheable
4. Now User B calls #isAllowedToRead(ContractId.of(“123”))
5. The cache holds an entry for “123” (“true”) and returns it (#isAllowedToRead will not be executed)

In effect the system supplied an incorrect access control outcome for User B.

Solution 1: No caching

Simply don’t use general caching on methods handling access control concerns (make sure that this aligns with the system’s specification on performance etc.).

Solution 2: Add a UserId parameter

We can change the code as follows:

@Cacheable("permissionsOnContracts")
public boolean isAllowedToRead(ContractId contractId, UserId caller) {
    return expensiveOperationOrRemoteCall(contractId);
}

Now the (auto-generated) CacheKey will use the contractId and the caller’s id, from which point, the Cache will implicitly respect the user context.

Minor drawbacks: Every method invoking #isAllowedToRead has to determine the caller’s id (correctly!) and using ‘null’ (or some constant) will break the access control again. Furthermore the caller’s id isn’t used inside #isAllowedToRead, so there is always the risk, that it will be removed in the future (refactoring, code clean up, …).

Solution 3: AuthenticationAwareKeyGenerator

We implement a custom KeyGenerator, which is aware of the user context:

public class AuthenticationAwareKeyGenerator extends SimpleKeyGenerator {

    // note: make sure that ":" isn't a valid char in the userId 
    // nor part of the result of generateKey
    @Override
    public Object generate(Object target, Method method, Object... params) {
        UserId userId = someWayToGetTheUserId(); // eg. by using Spring's SecurityContext
        return userId.value() + ":" + generateKey(params);
    }

}

Now we can create a Bean in one of our Config-Classes, e.g.:

@Configuration
@EnableCaching
public class CacheConfig {

    @Bean
    public AuthenticationAwareKeyGenerator authenticationAwareKeyGenerator() {
        return new AuthenticationAwareKeyGenerator();
    }

}

(Maybe we can skip this step by using @Component on the class AuthenticationAwareKeyGenerator itself – I didn’t try it out)

The last step is to add the custom KeyGenerator to our method:

@Cacheable(value="permissionsOnContracts", keyGenerator="authenticationAwareKeyGenerator")
public void boolean isAllowedToRead(ContractId contractId) {
    return expensiveOperationOrRemoteCall(contractId);
}

Done.