Conversation
To allow multiple launches to happen concurrently we key the session storage off the state value for the launch (which should be unique), and this way multiple tools can launch concurrently. Apply suggestions from code review Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Updates the LTI 1.3 launch authorization-request persistence to be keyed by OAuth state (instead of relying on browser session/cookies) so multiple launches can proceed concurrently.
Changes:
- Added
MultiStateCacheAuthorizationRequestRepository, a short-lived in-memory Guava cache keyed bystate, with optional remote-IP matching. - Added
StatelessOptimisticAuthorizationRequestRepositoryto force “no working session” semantics while delegating persistence to the state-keyed cache. - Wired the new repositories into
CustomLti13Configurerand added targeted unit tests.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
src/main/java/uk/ac/ox/ctl/ltiauth/MultiStateCacheAuthorizationRequestRepository.java |
Implements state-keyed authorization request storage with TTL, max size, and optional IP enforcement. |
src/main/java/uk/ac/ox/ctl/ltiauth/StatelessOptimisticAuthorizationRequestRepository.java |
Adapts optimistic repository behavior to a cookie-free/state-cache flow by forcing hasWorkingSession() false. |
src/main/java/uk/ac/ox/ctl/ltiauth/CustomLti13Configurer.java |
Configures the LTI flow to use the new state-keyed cache-backed repository. |
src/test/java/uk/ac/ox/ctl/ltiauth/MultiStateCacheAuthorizationRequestRepositoryTest.java |
Adds tests for concurrent state storage, removal behavior, max-size eviction, TTL expiry, and IP matching. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| protected OptimisticAuthorizationRequestRepository configureRequestRepository() { | ||
| MultiStateCacheAuthorizationRequestRepository repository = | ||
| new MultiStateCacheAuthorizationRequestRepository(Duration.ofMinutes(1), 2); | ||
| repository.setLimitIpAddress(limitIpAddresses); | ||
| return new StatelessOptimisticAuthorizationRequestRepository(repository); | ||
| } |
There was a problem hiding this comment.
This repository is an in-memory per-JVM cache. In a multi-instance deployment (multiple pods/EC2s behind a load balancer), the authorization redirect and the callback can land on different instances and the state lookup will fail. If that topology is possible, consider a shared backing store (e.g., Redis) or ensure load balancer affinity for the duration of the launch flow.
| void expiresEntriesAfterTtl() throws InterruptedException { | ||
| MultiStateCacheAuthorizationRequestRepository shortLivedRepository = | ||
| new MultiStateCacheAuthorizationRequestRepository(Duration.ofMillis(20), 1); | ||
| MockHttpServletResponse response = new MockHttpServletResponse(); | ||
|
|
||
| shortLivedRepository.saveAuthorizationRequest(authorizationRequest("short-lived"), new MockHttpServletRequest(), response); | ||
| Thread.sleep(50); | ||
|
|
||
| assertEquals(0, shortLivedRepository.size()); | ||
| assertNull(shortLivedRepository.loadAuthorizationRequest(callbackRequest("short-lived"))); | ||
| } |
There was a problem hiding this comment.
Time-based assertions using Thread.sleep are prone to CI flakiness (scheduler delays, noisy neighbors). Consider using Guava’s FakeTicker (by allowing a ticker to be injected into the repository/cache builder) or increasing margins and polling until expiry with a timeout.
There was a problem hiding this comment.
I think this seems to work fine and we should only add this complexity if needed.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
To allow multiple launches to happen concurrently we key the session storage off the state value for the launch (which should be unique), and this way multiple tools can launch concurrently.
Apply suggestions from code review