Add SAML-based SSO to UI & secure API calls#54
Conversation
…ontend guards and tests Agent-Logs-Url: https://github.com/MaximumTrainer/OpenDataMask/sessions/2dcca1c1-b66b-4760-8bef-db04af8c9658 Co-authored-by: MaximumTrainer <1376575+MaximumTrainer@users.noreply.github.com>
…larify auth/me null check and duplicate matchers Agent-Logs-Url: https://github.com/MaximumTrainer/OpenDataMask/sessions/2dcca1c1-b66b-4760-8bef-db04af8c9658 Co-authored-by: MaximumTrainer <1376575+MaximumTrainer@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds an optional SAML2 SSO flow alongside the existing JWT auth, aiming to (a) redirect unauthenticated browser users to the IdP while (b) keeping /api/** endpoints non-redirecting (401) and usable from the SPA with cookies/CSRF.
Changes:
- Backend: split Spring Security into multiple ordered filter chains, add conditional SAML chain + CORS configuration, and add
/api/auth/mesession-check endpoint. - Frontend: enable cookie-based requests + CSRF header injection, add SAML-aware 401 handling and navigation redirects, and initialize auth state from either local JWT storage or
/api/auth/me. - Tests/config: add frontend store tests, backend filter-chain tests, and test-profile CORS config.
Reviewed changes
Copilot reviewed 18 out of 19 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/src/views/LoginView.vue | Adds a conditional “Sign in with SSO” entry point when SAML is enabled. |
| frontend/src/views/tests/auth.store.test.ts | Adds vitest coverage for new auth-store session initialization behavior. |
| frontend/src/store/auth.ts | Introduces initializeFromSession() and adjusts auth state derivation to support SAML sessions. |
| frontend/src/router/index.ts | Updates route guard to redirect to SAML IdP when SAML is enabled. |
| frontend/src/App.vue | Switches initialization from storage-only to session-aware initialization. |
| frontend/src/api/client.ts | Enables withCredentials, adds CSRF header injection, and SAML-aware 401 redirect handling. |
| frontend/src/api/auth.ts | Adds /auth/me client helper returning a minimal User for session-based auth. |
| frontend/package.json | Adds jsdom for browser-like test environment. |
| frontend/package-lock.json | Locks jsdom and transitive dependencies. |
| backend/src/test/resources/application-test.yml | Adds test-profile CORS allowed-origins config. |
| backend/src/test/kotlin/com/opendatamask/infrastructure/security/SecurityFilterChainTest.kt | Adds integration tests to validate 401 vs permitted behavior for the new filter chains. |
| backend/src/test/kotlin/com/opendatamask/adapter/input/rest/AuthControllerTest.kt | Adds coverage for the new /api/auth/me endpoint behavior. |
| backend/src/main/resources/application.yml | Adds SAML relying party configuration placeholders and CORS allowed-origins config. |
| backend/src/main/resources/application-test.yml | Adds test-profile CORS allowed-origins config. |
| backend/src/main/kotlin/com/opendatamask/infrastructure/config/SecurityConfig.kt | Introduces separate API vs web filter chains with ordering and CORS/CSRF policy differences. |
| backend/src/main/kotlin/com/opendatamask/infrastructure/config/SamlSecurityConfig.kt | Adds a conditional SAML2 login/logout filter chain (order=2). |
| backend/src/main/kotlin/com/opendatamask/infrastructure/config/CorsConfig.kt | Adds a shared CORS configuration source using opendatamask.cors.allowed-origins. |
| backend/src/main/kotlin/com/opendatamask/adapter/input/rest/AuthController.kt | Adds /api/auth/me session-check endpoint. |
| backend/build.gradle.kts | Documents (commented) SAML dependency + repository needed for OpenSAML transitive deps. |
Files not reviewed (1)
- frontend/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| .securityMatcher("/api/**") | ||
| .cors { it.configurationSource(corsConfig.corsConfigurationSource()) } | ||
| .csrf { it.disable() } | ||
| .sessionManagement { it.sessionCreationPolicy(SessionCreationPolicy.STATELESS) } | ||
| .authorizeHttpRequests { auth -> |
There was a problem hiding this comment.
/api/** is configured as STATELESS with CSRF disabled, but the frontend now uses withCredentials + CSRF headers and AuthController.me is intended to detect a SAML session. With SessionCreationPolicy.STATELESS, Spring Security will not load the SecurityContext from the HTTP session, so SAML-authenticated browser sessions won’t be recognized on /api/** (likely causing /api/auth/me and other API calls to return 401 and potentially creating redirect loops). Consider using a session policy that can read an existing session (e.g., IF_REQUIRED) and enabling CSRF for cookie-based auth while still ignoring CSRF for Bearer-token requests.
| @GetMapping("/me") | ||
| fun me(@AuthenticationPrincipal principal: UserDetails?): ResponseEntity<Map<String, String>> { | ||
| if (principal == null) { | ||
| return ResponseEntity.status(HttpStatus.UNAUTHORIZED).build() | ||
| } |
There was a problem hiding this comment.
@AuthenticationPrincipal principal: UserDetails? will only be populated for authentication types whose principal implements UserDetails (e.g., your JWT flow). For SAML2, the principal is typically a Saml2AuthenticatedPrincipal, so this parameter will be null and the endpoint will incorrectly return 401 even when the user is authenticated via SAML. Use a more general type (e.g., Principal, Authentication, or AuthenticatedPrincipal) and extract the name/attributes accordingly.
| @Configuration | ||
| @ConditionalOnClass(name = ["org.springframework.security.saml2.provider.service.registration.RelyingPartyRegistrationRepository"]) | ||
| @ConditionalOnProperty(prefix = "spring.security.saml2.relyingparty.registration", name = ["default.assertingparty.metadata-uri"]) | ||
| class SamlSecurityConfig( |
There was a problem hiding this comment.
@ConditionalOnProperty here only checks for property presence, not that it’s non-empty. Since application.yml defines spring.security.saml2...metadata-uri with a default empty value (${SAML_IDP_METADATA_URI:}), this condition will evaluate true whenever the SAML classes are on the classpath—even when SAML isn’t configured. That undermines the “zero-impact when unconfigured” goal and may fail startup once the dependency is enabled. Consider conditioning on an explicit boolean flag, or use a condition that checks the metadata-uri is not blank, and/or avoid defining the property with an empty default in the baseline config.
| val result = mockMvc.perform(get("/api/workspaces")).andReturn() | ||
| val responseStatus = result.response.status | ||
| assert(responseStatus != 401 && responseStatus != 403) { | ||
| "Expected authenticated request to be permitted but got HTTP $responseStatus" | ||
| } |
There was a problem hiding this comment.
This test uses Kotlin’s assert(...) for verification. JVM assertions are typically disabled unless tests are run with -ea, so this check may be skipped entirely in CI and locally. Prefer JUnit assertions (e.g., Assertions.assertNotEquals / assertTrue) or a MockMvc andExpect matcher to ensure the test fails reliably when the status is 401/403.
| // Handle 401 globally – clear local credentials and redirect to SAML IdP | ||
| apiClient.interceptors.response.use( | ||
| (response) => response, | ||
| (error) => { | ||
| if (error.response?.status === 401) { | ||
| localStorage.removeItem('token') | ||
| localStorage.removeItem('user') | ||
| router.push('/login') | ||
|
|
||
| // If a SAML IdP is configured (detected by the presence of the | ||
| // SAML auth endpoint path in our app), redirect to the IdP. | ||
| // Otherwise fall back to the local login page. | ||
| const samlEnabled = import.meta.env.VITE_SAML_ENABLED === 'true' | ||
| if (samlEnabled) { | ||
| window.location.href = SAML_AUTH_ENDPOINT | ||
| } else { | ||
| router.push('/login') | ||
| } | ||
| } |
There was a problem hiding this comment.
The global 401 interceptor redirects on any 401, including calls to /api/auth/me and potentially /api/auth/login/register. Since App.vue now calls initializeFromSession() on mount, an unauthenticated page load will trigger /auth/me → 401 → redirect to /login, which breaks access to routes marked public (e.g., /register) and can also prevent the login form from showing “invalid credentials” errors (it will redirect instead). Consider exempting auth endpoints from the global redirect (or letting callers handle 401 for /auth/me/login), and only redirecting for 401s from protected API calls.
|
@copilot apply changes based on the comments in this thread |
… ConditionalOnProperty, JUnit assertions, 401 auth exemptions Agent-Logs-Url: https://github.com/MaximumTrainer/OpenDataMask/sessions/36e94f78-88b6-4b49-825f-84f2061837f9 Co-authored-by: MaximumTrainer <1376575+MaximumTrainer@users.noreply.github.com>
All five review issues addressed in commit
|
|
@copilot apply changes based on the comments in this thread |
All five changes from the review thread were already applied in commit
|
build.gradle.ktsCorsConfig.ktwith CORS configurationSecurityConfig.kt— two ordered filter chains (API order=1, web fallback order=3)SessionCreationPolicy.STATELESS→NEVERso existing SAML sessions are read by the API chain without creating new sessionsSamlSecurityConfig.kt— conditional SAML filter chain (order=2)@ConditionalOnPropertyto use full property name withmatchIfMissing = false; removed empty-default SAML section fromapplication.ymlso the property is absent unless the env var is explicitly setAuthController.kt—/api/auth/meendpoint@AuthenticationPrincipal UserDetails?→Authentication?parameter so both JWT (UserDetails) and SAML (Saml2AuthenticatedPrincipal) principals work viaAuthentication.getName()application.yml— removedspring.security.saml2block with empty defaults; added documentation comment describing required env varsSecurityFilterChainTest.ktassert()withAssertions.assertNotEquals()(JUnit) so assertions are reliable in CI without-eaflagAuthControllerTest.kt— tests for/api/auth/meapi/client.ts/auth/login,/auth/register,/auth/me) are now exempted from the global 401 redirect, so login errors surface correctly andinitializeFromSession()can detect "no session" silently without breaking public routesapi/auth.ts—me()functionstore/auth.ts—initializeFromSession()router/index.ts— SAML-aware navigation guardApp.vue— session init on mountLoginView.vue— SSO buttonauth.store.test.ts— 6 SAML auth store tests