JCL-279: Implement token cache for session objects - #459
acoburn merged 5 commits into
Conversation
Sorry, something went wrong.
| !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?
Sorry, something went wrong.
There was a problem hiding this comment.
Actually, it's not case sensitive:
Sorry, something went wrong.
| 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?
Sorry, something went wrong.
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.
Sorry, something went wrong.
| .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?
Sorry, something went wrong.
There was a problem hiding this comment.
This allows us to vary the mock server response based on the presence of an auth token.
Sorry, something went wrong.
| // 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?
Sorry, something went wrong.
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.
Sorry, something went wrong.
| 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
Sorry, something went wrong.
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.
Sorry, something went wrong.
This makes use of the new ClientCache<K, V> mechanism, applying it to session credential objects.
This required inverting one section of the ReactiveAuthorization class. Before, the Authenticator::authenticate method accepted a Session parameter. Now, the Session::authenticate method accepts an Authenticator parameter:
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::fromCache works. Previously, any fromCache requests 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.