JCL-279: Implement token cache for session objects #459
Conversation
acoburn
force-pushed
the
JCL-279-session-cache-support
branch
from
cb1e822 to
b9a6fb6
Compare
| !BEARER.equalsIgnoreCase(challenge.getScheme())) { | ||
| !schemes.contains(challenge.getScheme())) { |
There was a problem hiding this comment.
This makes the test case-sensitive, is it a problem?
There was a problem hiding this comment.
Actually, it's not case sensitive:
| public Optional<Credential> fromCache(final Request request) { | ||
| // TODO add cache | ||
| final Credential cachedToken = tokenCache.get(request.uri()); | ||
| if (cachedToken != null && cachedToken.getExpiration().isAfter(Instant.now())) { |
There was a problem hiding this comment.
If the token is cached, but expired, should it be removed from cache?
There was a problem hiding this comment.
I considered this, but I was concerned that there would be a race condition:
If multiple threads are reading/updating the cache, one thread could have fetched the token from the cache (line 168) and before line 169 another thread could have updated the cache. So if the first thread then invalidates the cache, it would have done so unnecessarily, as it would have already been updated by another thread.
| .withRequestBody(matching("Test String 1")) | ||
| .withHeader(CONTENT_TYPE, containing(TEXT_PLAIN)) | ||
| .withHeader("Authorization", containing("Bearer token-67890")) | ||
| .withHeader("Authorization", containing("Bearer eyJ")) |
There was a problem hiding this comment.
For my own understanding, why was this required?
There was a problem hiding this comment.
This allows us to vary the mock server response based on the presence of an auth token.
| // Support case-insensitive lookups | ||
| final Set<String> schemeNames = new TreeSet<>(String.CASE_INSENSITIVE_ORDER); | ||
| schemeNames.add("Bearer"); | ||
| schemeNames.add("DPoP"); |
There was a problem hiding this comment.
Is it worth using constants from OpenIdAuthenticationProvider instead?
There was a problem hiding this comment.
Possibly -- I've generally been moving away from storing string constants in a single file and using them elsewhere. It makes no difference to the JVM, since there will only ever be a single instance of the string in memory.
| final Optional<URI> principal = session.getPrincipal(); | ||
| assertEquals(Optional.of(URI.create(WEBID)), principal); | ||
| assertTrue(session.fromCache(null).isPresent()); | ||
| assertFalse(session.fromCache(null).isPresent()); |
There was a problem hiding this comment.
Duplicates assertion from 3 lines above
There was a problem hiding this comment.
In the past, the cache would be populated at this point (which was incorrect). This assertion tests that the cache is still not populated.
This makes use of the new
ClientCache<K, V>mechanism, applying it to session credential objects.This required inverting one section of the
ReactiveAuthorizationclass. Before, theAuthenticator::authenticatemethod accepted aSessionparameter. Now, theSession::authenticatemethod accepts anAuthenticatorparameter:solid-client-java/api/src/main/java/com/inrupt/client/auth/ReactiveAuthorization.java
Line 98 in 17d799b
The previously used method is now deprecated.
This also changes the way the
OpenIdSession::fromCacheworks. Previously, anyfromCacherequests returned an OpenId credential, which incorrectly bypasses reactive authorization. In this case, the is still only a single credential, but the internal cache keeps track of which URIs have performed reactive authorization and (provided that the credential is still valid), that ID Token credential will be provided.