[Fix] Share the OAuth authorization session across instances via Redis - #1960
akilarootcode wants to merge 1 commit into
Conversation
|
ThinuwanW
left a comment
ThinuwanW
left a comment
There was a problem hiding this comment.
🤖 Claude Code Review
This PR adds only spring-boot-starter-session-data-redis to the backend pom; the accompanying spring.session.data.redis.* settings live in the config submodule (application-ep-non-prd.yml / application-ep-prd.yml) and are not part of the reviewable diff. The artifact name and version management are correct for Spring Boot 4.0.3, and the change is the right fix for making the OAuth authorization session survive across instances. However, the dependency has global, classpath-driven side effects: it switches HTTP session storage from Tomcat to Redis for every profile (including the community profiles, which configure no Redis host), replaces Tomcat's session-cookie writer, and defaults to JDK serialization of the stored SecurityContext — none of which are addressed. The companion YAML also appears to use the wrong property prefix.
Found 13 new issue(s): 🔴 5 important, 🟡 5 suggestion(s), 🟣 3 nit(s)
| <groupId>org.springframework.boot</groupId> | ||
| <artifactId>spring-boot-starter-data-redis</artifactId> | ||
| </dependency> | ||
| <dependency> |
There was a problem hiding this comment.
🤖 Claude · 🔴 Important
📁 File-level observation
Wrong property prefix in the companion config, so the session settings are dead. The enterprise configs added alongside this dependency (backend/src/main/resources/config/application-ep-non-prd.yml and application-ep-prd.yml) use spring.session.data.redis.{repository-type,namespace,flush-mode}. Spring Boot / Spring Session 4.0 (the spring-session.version managed by the 4.0.3 BOM) bind these under spring.session.redis.* — the Spring Session 4.0 reference documents spring.session.redis.namespace and spring.session.redis.repository-type, and Boot's own session docs use spring.session.jdbc.table-name (not spring.session.data.jdbc.*) for the sibling store. Unknown spring.* keys are silently ignored, so nothing fails at startup, but namespace: skapp:oauth-session never takes effect and sessions are written under the default spring:session:* keyspace — in the same Redis DB already used by EpCacheServiceImpl and bucket4j rate limiting, losing the isolation the setting was meant to provide. (repository-type: default and flush-mode: on_save happen to match the built-in defaults, so only the namespace has visible impact.) Fix: rename the block to spring.session.redis.*. You can verify in one run by starting the app, doing a session sign-in, and checking whether the Redis key is skapp:oauth-session:* or spring:session:*.
| <groupId>org.springframework.boot</groupId> | ||
| <artifactId>spring-boot-starter-data-redis</artifactId> | ||
| </dependency> | ||
| <dependency> |
There was a problem hiding this comment.
🤖 Claude · 🔴 Important
📁 File-level observation
This starter is added to the community pom, but only the enterprise profiles configure Redis. application-ep-non-prd.yml / application-ep-prd.yml set spring.data.redis.host: ${REDIS_HOST}; the community profiles (application-non-prd.yml, application-prd.yml) and backend/.env.local have no Redis settings at all, so a community/self-hosted deployment silently falls back to localhost:6379. With Spring Session on the classpath, SessionRepositoryFilter wraps every request and all HTTP session storage moves to Redis globally, not just for the OAuth flow. Today this is latent rather than immediately broken — both SecurityConfig and EPSecurityConfig are STATELESS and the only request.getSession(true) is in the enterprise OAuthServiceImpl.sessionLogin — but the first community code path that touches a session will fail with RedisConnectionFailureException in an environment with no Redis. Note also that the Spring Session commit happens inside SessionRepositoryFilter, outside the DispatcherServlet, so such a failure bypasses GlobalExceptionHandler (whose DataAccessException handler would otherwise catch it) and surfaces as a raw container 500 instead of the standard ErrorResponse. Either scope this dependency to the enterprise build, or add spring.autoconfigure.exclude for the session auto-configuration in the community profiles, and document REDIS_HOST in .env.local / README if Redis is now expected for community too.
| </dependency> | ||
| <dependency> | ||
| <groupId>org.springframework.boot</groupId> | ||
| <artifactId>spring-boot-starter-session-data-redis</artifactId> |
There was a problem hiding this comment.
🤖 Claude · 🟡 Suggestion
No session serializer is configured, so sessions fall back to JDK serialization. Spring Session's Redis repository uses JdkSerializationRedisSerializer unless a bean named springSessionDefaultRedisSerializer is supplied. The session written by OAuthServiceImpl.sessionLogin holds a SecurityContextImpl wrapping a UsernamePasswordAuthenticationToken, and those classes derive their serialVersionUID from SpringSecurityCoreVersion.SERIAL_VERSION_UID, which changes on Spring Security version bumps. Result: every Spring Security upgrade invalidates in-flight sessions, and mixed-version rolling deploys produce deserialization errors mid-OAuth-flow. It is also inconsistent with the existing RedisConfig, which deliberately uses JSON serializers for the app's own RedisTemplate. Add a @Bean("springSessionDefaultRedisSerializer") returning a GenericJackson2JsonRedisSerializer built from a mapper registered with SecurityJacksonModules.getModules(...) (the same modules already used in AuthorizationServerConfig.authorizationJsonMapper()), so the stored context is version-tolerant and inspectable.
| </dependency> | ||
| <dependency> | ||
| <groupId>org.springframework.boot</groupId> | ||
| <artifactId>spring-boot-starter-session-data-redis</artifactId> |
There was a problem hiding this comment.
🤖 Claude · 🟡 Suggestion
Session cookie attributes change behaviour and are nowhere configured. Once Spring Session is active, the session cookie is written by DefaultCookieSerializer instead of Tomcat: the name changes from JSESSIONID to SESSION, the value is base64-encoded, and SameSite=Lax is now set explicitly (Tomcat previously emitted no SameSite attribute). There is no server.servlet.session.cookie.* block in any profile. EPSecurityConfig.getCorsConfigurationCookies enables allowCredentials(true) for /v1/auth/session/sign-in, /v1/auth/session/sign-out, /v1/auth/session/refresh-token, /v2/ep/auth/session/signin/sso/* and /v1/ep/auth/session/code-challenge/verify — if the frontend origin and the API are not same-site (different registrable domains, not just different subdomains), an explicit SameSite=Lax cookie will not be sent on those cross-site XHRs, so sign-out and refresh-token would silently no-op while the top-level /oauth2/authorize navigation still works, which is an easy-to-miss partial failure. Explicitly pin server.servlet.session.cookie.{name,same-site,secure,http-only} in the enterprise profiles to match the deployment topology rather than relying on the changed defaults. (Nothing in the repo greps for JSESSIONID, so the name change itself looks safe, but any CDN/ALB/nginx rule outside this repo keyed on the cookie name would need updating.)
| </dependency> | ||
| <dependency> | ||
| <groupId>org.springframework.boot</groupId> | ||
| <artifactId>spring-boot-starter-session-data-redis</artifactId> |
There was a problem hiding this comment.
🤖 Claude · 🟣 Nit
Session TTL is longer than the flow needs. Spring Session's expiry falls back to server.servlet.session.timeout, which the enterprise configs set to 30m. OAuthAuthorizationSessionFilter invalidates the session as soon as the authorize request completes, but an abandoned flow leaves a Redis key holding a full authenticated SecurityContext for 30 minutes. Consider setting spring.session.timeout to a short value (a few minutes) sized for the authorize round-trip, independent of the servlet session timeout.
| </dependency> | ||
| <dependency> | ||
| <groupId>org.springframework.boot</groupId> | ||
| <artifactId>spring-boot-starter-session-data-redis</artifactId> |
There was a problem hiding this comment.
🤖 Claude · 🟣 Nit
No test-side handling for the new auto-configuration. backend/src/test/resources/application.yml configures neither Redis nor spring.session, and no integration test exercises session sign-in → /oauth2/authorize. The existing @SpringBootTest suites should still pass, because the default repository-type is the non-indexed RedisSessionRepository (lazy — no eager connection or keyspace-notification CONFIG SET at startup) and no test creates a session (all filter chains are STATELESS, and SecurityMockMvcRequestPostProcessors.authentication(...) stores the context in a request attribute). That is an implicit dependency on a Spring Session default, though: switching repository-type to indexed later would break the whole suite with connection-refused at context startup. Make it explicit by excluding the session auto-configuration in src/test/resources/application.yml, and add at least one integration test covering the sign-in → authorize handoff against an embedded/Testcontainers Redis so the behaviour this PR enables is actually verified.
| </dependency> | ||
| <dependency> | ||
| <groupId>org.springframework.boot</groupId> | ||
| <artifactId>spring-boot-starter-session-data-redis</artifactId> |
There was a problem hiding this comment.
🤖 Claude · 🔴 Important
Unauthenticated requests now create Redis-backed sessions, because the request cache is never disabled on the STATELESS chains. SecurityConfig.java:62 and EPSecurityConfig.java:77 set SessionCreationPolicy.STATELESS but leave Spring Security's default HttpSessionRequestCache in place, and AuthorizationServerConfig.java:116 sets no session policy at all. ExceptionTranslationFilter calls requestCache.saveRequest(...) before invoking the entry point, which creates an HttpSession. Before this starter that was a short-lived heap object; now every such request writes a <namespace>:sessions:<uuid> key to Redis with the 30m server.servlet.session.timeout. Three concrete paths: (1) community builds — JwtAuthFilter.java:51-54 passes unauthenticated requests straight through, so every 401 on a protected endpoint triggers a session write, and since no community profile configures Redis the write goes to the autoconfigured localhost:6379 and fails, turning a clean 401 into an error page; (2) /oauth2/authorize on the order-1 chain is publicly reachable and is NOT covered by rate limiting (EpRateLimitInterceptor is a HandlerInterceptor, and the authorization-server endpoints are served by servlet filters that never reach handler mapping), so callers can grow the Redis keyspace at will; (3) OAuthAuthorizationSessionFilter.doFilterInternal only invalidates sessions carrying OAUTH_SESSION_TENANT_ATTRIBUTE, so these orphan sessions are never cleaned up and live out their full TTL. Note the app's own XHR traffic is spared (the default matcher skips Accept: application/json), which is exactly why this will not show up in normal testing. Fix: add .requestCache(RequestCacheConfigurer::disable) (or wire a NullRequestCache) to both JWT chains, and on the authorization-server chain either disable the request cache or scope session creation to /oauth2/authorize only.
| </dependency> | ||
| <dependency> | ||
| <groupId>org.springframework.boot</groupId> | ||
| <artifactId>spring-boot-starter-session-data-redis</artifactId> |
There was a problem hiding this comment.
🤖 Claude · 🔴 Important
Redis failures during session access are reported to clients as HTTP 404 and never reach the project's error contract. Every request.getSession(...) call — TenantFilter.java:126 (runs on essentially every non-excluded request), OAuthSessionUtil.java:17 and :36, OAuthServiceImpl.java:87-89 — now performs a Redis round trip whenever a SESSION cookie is present, and can throw RedisConnectionFailureException, QueryTimeoutException or SerializationException. Those propagate up to ExceptionLoggingFilter, whose case null, default -> branch (ExceptionLoggingFilter.java:67-71) maps ANY unrecognised exception to HttpStatus.NOT_FOUND with COMMON_ERROR_MODULE_EXCEPTION — so a Redis outage surfaces as a 404, which is both wrong for the caller and invisible to alerting. Failures raised later by SessionRepositoryFilter.commitSession() are outside both ExceptionLoggingFilter and GlobalExceptionHandler (which only sees exceptions reaching the DispatcherServlet) and return a raw container error page instead of ResponseEntityDto/ErrorResponse. This also breaks a deliberate convention: EpCacheServiceImpl.java:37,49,60 catches RedisConnectionFailureException and degrades, and EpRateLimitInterceptor.java:65-68 explicitly fails open. Fix: add an explicit branch in ExceptionLoggingFilter.handleException for org.springframework.dao.DataAccessException mapping to 503 SERVICE_UNAVAILABLE with a dedicated message constant, and make the session reads in TenantFilter/OAuthSessionUtil fail open (treat an unreachable Redis as "no session") so JWT-authenticated traffic is not taken down by a session-store outage. Worth noting the same handler is what a post-upgrade InvalidClassException on stored sessions would hit.
| </dependency> | ||
| <dependency> | ||
| <groupId>org.springframework.boot</groupId> | ||
| <artifactId>spring-boot-starter-session-data-redis</artifactId> |
There was a problem hiding this comment.
🤖 Claude · 🔴 Important
No Redis command/connect timeout is configured, and Redis is now synchronously on the request path. spring.data.redis in application-ep-non-prd.yml / application-ep-prd.yml sets only host, port and ssl, so Lettuce's default 60s command timeout applies. Until this PR that only mattered for the cache and the rate limiter, and both of those fail open (EpCacheServiceImpl, EpRateLimitInterceptor.java:65). Session reads/writes have no fail-open path and happen inside the filter chain, so a stalled (not down) Redis will park request threads for up to 60 seconds each — with spring.threads.virtual.enabled: true that will not manifest as thread exhaustion but as a fleet-wide latency cliff. Add spring.data.redis.timeout: 2s and spring.data.redis.connect-timeout: 1s (plus explicit lettuce.pool sizing if shareNativeConnection is turned off) in the same commit that adds this starter, so the new dependency ships with bounded failure behaviour.
| <groupId>org.springframework.boot</groupId> | ||
| <artifactId>spring-boot-starter-data-redis</artifactId> | ||
| </dependency> | ||
| <dependency> |
There was a problem hiding this comment.
🤖 Claude · 🟡 Suggestion
📁 File-level observation
Session keys will share a Redis database with cache entries and rate-limit buckets, where an eviction policy can silently drop them. spring.data.redis sets no database, so the new session keys land in DB 0 alongside the EpRateLimitConstants.RATE_LIMIT_REDIS_KEY_PREFIX buckets written by EpRateLimitConfig and the entries written by EpCacheServiceImpl. If that instance is provisioned as a cache (maxmemory-policy allkeys-lru/allkeys-random, common for ElastiCache defaults), live sessions get evicted under memory pressure and users are dropped mid-consent with no diagnosable error — and, per the previous finding, the eviction is indistinguishable from "no session". Either pin sessions to a dedicated spring.data.redis.database index, or document/enforce that this instance runs noeviction or volatile-ttl. Also worth confirming the OAuth session namespace does not collide with the rate-limit prefix.
| <groupId>org.springframework.boot</groupId> | ||
| <artifactId>spring-boot-starter-data-redis</artifactId> | ||
| </dependency> | ||
| <dependency> |
There was a problem hiding this comment.
🤖 Claude · 🟡 Suggestion
📁 File-level observation
Session traffic will multiplex over the same single Lettuce connection as the rate limiter. EpRateLimitConfig.java:29-40 injects the shared RedisConnectionFactory, casts it to LettuceConnectionFactory and takes the native RedisClient; Spring Session's repository is wired to that same factory, whose shareNativeConnection defaults to true. So session GET/SET, the Bucket4j CAS retry loop and cache calls all contend on one connection, and a slow session write can now delay rate-limit decisions (and vice versa). Consider declaring a dedicated RedisConnectionFactory bean annotated @SpringSessionRedisConnectionFactory so session traffic is isolated from rate limiting and can be tuned/monitored separately.
| </dependency> | ||
| <dependency> | ||
| <groupId>org.springframework.boot</groupId> | ||
| <artifactId>spring-boot-starter-session-data-redis</artifactId> |
There was a problem hiding this comment.
🤖 Claude · 🟣 Nit
Now that the HttpSession implementation is swapped out underneath it, the session handshake in OAuthServiceImpl.sessionLogin (OAuthServiceImpl.java:87-90) is worth tightening. It does getSession(true), then changeSessionId(), then re-reads with getSession(false) and dereferences the result without a null check — the result of getSession(false) immediately after changeSessionId() is implementation-specific behaviour, and this now depends on Spring Session's wrapper rather than Tomcat's. Hold the reference instead: HttpSession session = httpRequest.getSession(true); httpRequest.changeSessionId(); session.setAttribute(...). Both implementations keep the same HttpSession object across an id rotation, so this is strictly safer and removes the NPE on line 90.
| <groupId>org.springframework.boot</groupId> | ||
| <artifactId>spring-boot-starter-data-redis</artifactId> | ||
| </dependency> | ||
| <dependency> |
There was a problem hiding this comment.
🤖 Claude · 🟡 Suggestion
📁 File-level observation
No test can observe the new filter, and none is added. SessionRepositoryFilter is registered as a container-level filter, so MockMvc-based tests continue to use MockHttpServletRequest's in-memory session and will pass identically whether or not Redis sessions work — the entire OAuth session path (OAuthController, OAuthServiceImpl.sessionLogin/sessionLogout, OAuthSessionUtil, TenantFilter.tenantFromSession) has no test coverage today, so nothing in CI exercises this change. The Boot 4.0.3 BOM ships spring-boot-starter-session-data-redis-test for exactly this; add it (test scope) plus a Testcontainers-backed integration test that drives session-login → /oauth2/authorize → consent through a real filter chain and asserts the session survives a simulated second instance. Without it, the serializer, namespace and TTL choices made in this PR are unverified in production terms.
2 participants
Summary
The Claude MCP connector cannot complete authorization on production. After signing in, the browser is returned to the workspace-selection screen instead of the consent screen, with no error shown. The OAuth authorization server keeps its authenticated principal in a servlet
HttpSessionheld in JVM memory, and production core runs two instances behind a round-robin load balancer with no session affinity. This change moves that session into the Redis/Valkey instance the application already uses, so any instance can serve any hop of the flow.Root cause
Measured by issuing repeated requests to
/oauth2/authorizefrom a single cookie engine, presenting the previous session cookie each time:With two targets and round-robin routing the browser's cookie is always the one minted by the other instance, so the hit rate is 0%, not 50% — the flow fails on every attempt rather than intermittently.
POST /v1/auth/oauth/session-loginwrites the security context onto one instance. The browser's next hop,GET /oauth2/authorize, lands on the other, is treated as unauthenticated, andOAuthAuthenticationEntryPointredirects to/auth/signin?callback=…. That redirect carriescallbackbut notenantId, so the sign-in page falls back to the workspace-entry screen. Every hop is a plain 302, which is why the user sees no error.Everything else in the flow already survives across instances:
JdbcOAuth2AuthorizationServiceandJdbcOAuth2AuthorizationConsentServicepersist the authorization, itsstateand the consent decision to the database. The principal was the only instance-local part.Changes in this repo (
SkappHQ/skapp)backend/pom.xml— addspring-boot-starter-session-data-redis. No version: the Spring Boot 4.0.3 parent manages it (resolves spring-session 4.0.2).No Java changed.
OAuthServiceImpl,OAuthSessionUtil,TenantFilterandOAuthAuthorizationSessionFiltercontinue to use the servletHttpSessionAPI unchanged; only its backing store moves.Feature scope (all repos)
Branch
fix/oauth-session-redis-store:SkappHQ/skapp(super)spring-boot-starter-session-data-redisdependencyrootcodelabs/skapp-ep-be-resources-configspring.session.data.redisblock in theep-prd/ep-non-prdprofilesArchitecture / flow
flowchart TB subgraph After["After — session in Redis"] S1[POST /v1/auth/oauth/session-login] --> IA2[instance A] S2[GET /oauth2/authorize] --> IB2[instance B] IA2 --> R[("Redis / Valkey<br/>skapp:oauth-session")] IB2 --> R R --> OK[consent screen] end subgraph Before["Before — session in JVM memory"] A1[POST /v1/auth/oauth/session-login] --> IA[instance A<br/>session written to heap] A2[GET /oauth2/authorize] --> IB[instance B<br/>session not found] IB --> RE[302 /auth/signin?callback=...<br/>no tenantId] RE --> TS[workspace-selection screen] endExclusions — deliberately NOT in this PR
resolveTenant()ignores the readabletenantcookie, andbuildOAuthSignInPath/resolveSignInUrldroptenantId. These are what make the failure silent rather than an error; they do not cause it. Separate change.auth.skapp.comandauth.skapp.devcurrently serve the same RSA modulus under the samekid, so only theissclaim separates the environments. Pre-existing, raised separately.Ignored — handled elsewhere / at deployment
Deployment notes
JSESSIONIDtoSESSION(Spring Session's default). Nothing in the backend reads it by name and the frontend never touches it. Sessions in flight at deploy time are dropped, so a client mid-authorization simply restarts the flow.maxmemory-policymust not evict live sessions (allkeys-lruwould, and would reproduce the reported symptom), and because the store now gates authentication rather than only caching, the instance should require AUTH and remain network-restricted.Testing
mvn -B -DskipTests package— BUILD SUCCESS; spring-session 4.0.2 resolved via the Boot BOMmvn checkstyle:check— passVerified against a running instance with Redis inspected directly:
Set-Cookiein 5 requests from one cookie engineskapp:oauth-session:sessions:<uuid>; the cookie base64-decodes to that uuidchangeSessionId()on session-loginsession.invalidate()on session-logoutSecure; HttpOnly; SameSite=LaxSecureis set automatically:ForwardedHeaderFilteris registered at highest precedence, sorequest.isSecure()is true behind TLS termination and no explicit cookie property is needed.SameSite=Laxis new relative to the Tomcat cookie, and permits the top-levelGET /oauth2/authorizenavigation the flow depends on.The two-instance case itself is not reproducible locally (one JVM); what is verified is that the session lives in Redis rather than in the JVM. Confirmation after deploy is the same repeated-request probe against production, which should report 1 rather than 30.