-
Notifications
You must be signed in to change notification settings - Fork 6
JCL-279: Implement token cache for session objects #459
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.
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
Changes from all commits
4751815
b9a6fb6
9637928
2ac4a88
ade59b9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -133,7 +133,7 @@ private void setupMocks() { | |
| .withHeader("User-Agent", equalTo(USER_AGENT)) | ||
| .withRequestBody(matching("Test String 1")) | ||
| .withHeader(CONTENT_TYPE, containing(TEXT_PLAIN)) | ||
| .withHeader("Authorization", containing("Bearer token-67890")) | ||
| .withHeader("Authorization", containing("Bearer eyJ")) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For my own understanding, why was this required?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This allows us to vary the mock server response based on the presence of an auth token. |
||
| .withHeader("DPoP", absent()) | ||
| .willReturn(aResponse() | ||
| .withStatus(201))); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -24,4 +24,4 @@ | |
|
|
||
| public class UmaAccessGrantScenarioTest extends AccessGrantScenarios { | ||
|
|
||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -29,6 +29,7 @@ | |||
|
|
||||
| import java.util.Optional; | ||||
| import java.util.Set; | ||||
| import java.util.TreeSet; | ||||
| import java.util.concurrent.CompletableFuture; | ||||
| import java.util.concurrent.CompletionStage; | ||||
|
|
||||
|
|
@@ -38,11 +39,15 @@ | |||
| public class OpenIdAuthenticationProvider implements AuthenticationProvider { | ||||
|
|
||||
| private static final String BEARER = "Bearer"; | ||||
| private static final String DPOP = "DPoP"; | ||||
|
|
||||
| private final int priorityLevel; | ||||
| private final Set<String> schemes = new TreeSet<>(String.CASE_INSENSITIVE_ORDER); | ||||
|
|
||||
| public OpenIdAuthenticationProvider() { | ||||
| this(50); | ||||
| schemes.add(BEARER); | ||||
| schemes.add(DPOP); | ||||
| } | ||||
|
|
||||
| /** | ||||
|
|
@@ -59,15 +64,20 @@ public String getScheme() { | |||
| return BEARER; | ||||
| } | ||||
|
|
||||
| @Override | ||||
| public Set<String> getSchemes() { | ||||
| return schemes; | ||||
| } | ||||
|
|
||||
| @Override | ||||
| public Authenticator getAuthenticator(final Challenge challenge) { | ||||
| validate(challenge); | ||||
| return new OpenIdAuthenticator(priorityLevel); | ||||
| } | ||||
|
|
||||
| static void validate(final Challenge challenge) { | ||||
| void validate(final Challenge challenge) { | ||||
| if (challenge == null || | ||||
| !BEARER.equalsIgnoreCase(challenge.getScheme())) { | ||||
| !schemes.contains(challenge.getScheme())) { | ||||
|
Comment on lines
-70
to
+80
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This makes the test case-sensitive, is it a problem?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, it's not case sensitive: solid-client-java/openid/src/main/java/com/inrupt/client/openid/OpenIdAuthenticationProvider.java Line 45 in 2ac4a88
|
||||
| throw new OpenIdException("Invalid challenge for OpenID authentication"); | ||||
| } | ||||
| } | ||||
|
|
||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,13 +22,17 @@ | |
|
|
||
| import static java.nio.charset.StandardCharsets.UTF_8; | ||
|
|
||
| import com.inrupt.client.ClientCache; | ||
| import com.inrupt.client.Request; | ||
| import com.inrupt.client.auth.Authenticator; | ||
| import com.inrupt.client.auth.Credential; | ||
| import com.inrupt.client.auth.DPoP; | ||
| import com.inrupt.client.auth.Session; | ||
| import com.inrupt.client.spi.ServiceProvider; | ||
|
|
||
| import java.net.URI; | ||
| import java.security.MessageDigest; | ||
| import java.time.Duration; | ||
| import java.time.Instant; | ||
| import java.util.Collection; | ||
| import java.util.Collections; | ||
|
|
@@ -75,16 +79,19 @@ public final class OpenIdSession implements Session { | |
| private final AtomicReference<Credential> credential = new AtomicReference<>(); | ||
| private final ForkJoinPool executor = new ForkJoinPool(1); | ||
| private final DPoP dpop; | ||
| private final ClientCache<URI, Boolean> requestCache; | ||
|
|
||
| private OpenIdSession(final String id, final DPoP dpop, | ||
| final Supplier<CompletionStage<Credential>> authenticator) { | ||
| this.id = Objects.requireNonNull(id, "Session id may not be null!"); | ||
| this.authenticator = Objects.requireNonNull(authenticator, "OpenID authenticator may not be null!"); | ||
| this.dpop = Objects.requireNonNull(dpop); | ||
| this.requestCache = ServiceProvider.getCacheBuilder().build(1000, Duration.ofMinutes(5)); | ||
|
|
||
| // Support case-insensitive lookups | ||
| final Set<String> schemeNames = new TreeSet<>(String.CASE_INSENSITIVE_ORDER); | ||
| schemeNames.add("Bearer"); | ||
| schemeNames.add("DPoP"); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it worth using constants from
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
|
|
||
| this.schemes = Collections.unmodifiableSet(schemeNames); | ||
| } | ||
|
|
@@ -216,16 +223,29 @@ public Optional<String> generateProof(final String jkt, final Request request) { | |
| @Override | ||
| public Optional<Credential> fromCache(final Request request) { | ||
| final Credential c = credential.get(); | ||
| if (!hasExpired(c)) { | ||
| if (!hasExpired(c) && request != null && requestCache.get(request.uri()) != null) { | ||
| LOGGER.debug("Using cached token for request: {}", request.uri()); | ||
| return Optional.of(c); | ||
| } | ||
| return Optional.empty(); | ||
| } | ||
|
|
||
| @Override | ||
| public CompletionStage<Optional<Credential>> authenticate(final Authenticator auth, | ||
| final Request request, final Set<String> algorithms) { | ||
| final Optional<Credential> credential = getCredential(ID_TOKEN, null); | ||
| if (credential.isPresent() && request != null) { | ||
| LOGGER.debug("Setting cache entry for request: {}", request.uri()); | ||
| requestCache.put(request.uri(), Boolean.TRUE); | ||
| } | ||
| return CompletableFuture.completedFuture(credential); | ||
| } | ||
|
|
||
| /* deprecated */ | ||
| @Override | ||
| public CompletionStage<Optional<Credential>> authenticate(final Request request, | ||
| final Set<String> algorithms) { | ||
| return CompletableFuture.completedFuture(getCredential(ID_TOKEN, null)); | ||
| return authenticate(null, request, algorithms); | ||
| } | ||
|
|
||
| boolean hasExpired(final Credential credential) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -157,7 +157,7 @@ void testClientCredentials() { | |
| assertFalse(session.fromCache(null).isPresent()); | ||
| final Optional<URI> principal = session.getPrincipal(); | ||
| assertEquals(Optional.of(URI.create(WEBID)), principal); | ||
| assertTrue(session.fromCache(null).isPresent()); | ||
| assertFalse(session.fromCache(null).isPresent()); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Duplicates assertion from 3 lines above
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the past, the cache would be populated at this point (which was incorrect). This assertion tests that the cache is still not populated. |
||
| final Optional<Credential> credential = session.authenticate(null, Collections.emptySet()) | ||
| .toCompletableFuture().join(); | ||
| assertEquals(Optional.of(URI.create(WEBID)), credential.flatMap(Credential::getPrincipal)); | ||
|
|
@@ -176,7 +176,7 @@ void testClientCredentialsWithConfig() { | |
| assertFalse(session.fromCache(null).isPresent()); | ||
| final Optional<URI> principal = session.getPrincipal(); | ||
| assertEquals(Optional.of(URI.create(WEBID)), principal); | ||
| assertTrue(session.fromCache(null).isPresent()); | ||
| assertFalse(session.fromCache(null).isPresent()); | ||
| final Optional<Credential> credential = session.authenticate(null, Collections.emptySet()) | ||
| .toCompletableFuture().join(); | ||
| assertEquals(Optional.of(URI.create(WEBID)), credential.flatMap(Credential::getPrincipal)); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the token is cached, but expired, should it be removed from cache?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.