From a1325910240e20abe26c43b4afef009da794ce1d Mon Sep 17 00:00:00 2001 From: Peter Hoffmann <954078+p-hoffmann@users.noreply.github.com> Date: Fri, 27 Mar 2026 14:39:38 +0800 Subject: [PATCH 1/8] add oidc support --- docker/auth-test/docker-compose.yml | 4 +- .../auth-tests.postman_collection.json | 199 +++++++---- .../security/authc/LoginController.java | 92 +++++ .../webapi/security/authc/OidcAuthConfig.java | 327 ++++++++++++++++++ .../webapi/security/authc/UserOrigin.java | 2 +- .../security/authz/AuthorizationService.java | 14 + src/main/resources/application.yaml | 1 + 7 files changed, 564 insertions(+), 75 deletions(-) create mode 100644 src/main/java/org/ohdsi/webapi/security/authc/OidcAuthConfig.java diff --git a/docker/auth-test/docker-compose.yml b/docker/auth-test/docker-compose.yml index dc830c763..8ef6f2f62 100644 --- a/docker/auth-test/docker-compose.yml +++ b/docker/auth-test/docker-compose.yml @@ -19,7 +19,7 @@ services: container_name: mock-oauth2 environment: - SERVER_PORT=9090 - - JSON_CONFIG={"interactiveLogin":true,"httpServer":"NettyWrapper","tokenCallbacks":[{"issuerId":"default","tokenExpiry":3600,"requestMappings":[{"requestParam":"grant_type","match":"*","claims":{"sub":"testuser","email":"testuser@example.com","name":"Test User"}}]}]} + - JSON_CONFIG={"interactiveLogin":true,"httpServer":"NettyWrapper","tokenCallbacks":[{"issuerId":"default","tokenExpiry":3600,"requestMappings":[{"requestParam":"grant_type","match":"*","claims":{"sub":"testuser","email":"testuser@example.com","name":"Test User","roles":["Atlas users"]}}]}]} ports: - "9090:9090" networks: @@ -68,6 +68,8 @@ services: - SECURITY_AUTH_OPENID_EXTERNALURL=http://localhost:9090/default - SECURITY_AUTH_OPENID_LOGOUTURL=http://localhost:9090/default/endsession - SECURITY_AUTH_OPENID_EXTRASCOPES=profile email + - SECURITY_AUTH_OPENID_ROLESCLAIM=roles + - SECURITY_DEFAULTROLES=Atlas users - SECURITY_AUTH_OAUTH_CALLBACK_UI=http://localhost:18080/WebAPI/#/welcome - SECURITY_AUTH_OAUTH_CALLBACK_API=http://localhost:18080/WebAPI/user/oauth/callback - SECURITY_AUTH_OAUTH_CALLBACK_URLRESOLVER=query diff --git a/docker/auth-test/postman/auth-tests.postman_collection.json b/docker/auth-test/postman/auth-tests.postman_collection.json index 45d1b5e04..805f4db17 100644 --- a/docker/auth-test/postman/auth-tests.postman_collection.json +++ b/docker/auth-test/postman/auth-tests.postman_collection.json @@ -46,22 +46,19 @@ { "name": "OIDC Discovery Endpoint", "event": [ - { - "listen": "prerequest", - "script": { - "exec": [ - "// OIDC tests disabled - skip this request", - "pm.execution.skipRequest();" - ], - "type": "text/javascript" - } - }, { "listen": "test", "script": { "exec": [ - "pm.test('SKIPPED - OIDC not yet implemented', function() {", - " pm.expect(true).to.be.true;", + "pm.test('OIDC discovery endpoint returns 200', function() {", + " pm.response.to.have.status(200);", + "});", + "", + "pm.test('Discovery document contains required endpoints', function() {", + " const doc = pm.response.json();", + " pm.expect(doc).to.have.property('authorization_endpoint');", + " pm.expect(doc).to.have.property('token_endpoint');", + " pm.expect(doc).to.have.property('issuer');", "});" ], "type": "text/javascript" @@ -86,22 +83,19 @@ { "name": "Auth Providers Endpoint", "event": [ - { - "listen": "prerequest", - "script": { - "exec": [ - "// OIDC tests disabled - skip this request", - "pm.execution.skipRequest();" - ], - "type": "text/javascript" - } - }, { "listen": "test", "script": { "exec": [ - "pm.test('SKIPPED - OIDC not yet implemented', function() {", - " pm.expect(true).to.be.true;", + "pm.test('Auth providers returns 200', function() {", + " pm.response.to.have.status(200);", + "});", + "", + "pm.test('OpenID provider is listed', function() {", + " const providers = pm.response.json();", + " const oidc = providers.find(p => p.name === 'OpenID');", + " pm.expect(oidc).to.not.be.undefined;", + " pm.expect(oidc.url).to.equal('user/login/openid');", "});" ], "type": "text/javascript" @@ -196,23 +190,32 @@ { "name": "1. Start OIDC Login", "event": [ - { - "listen": "prerequest", - "script": { - "exec": [ - "// OIDC tests disabled - skip this request", - "pm.execution.skipRequest();" - ], - "type": "text/javascript" - } - }, { "listen": "test", "script": { "exec": [ - "pm.test('SKIPPED - OIDC not yet implemented', function() {", - " pm.expect(true).to.be.true;", - "});" + "pm.test('OIDC login returns 302 redirect', function() {", + " pm.response.to.have.status(302);", + "});", + "", + "const location = pm.response.headers.get('Location');", + "pm.test('Location header contains authorization endpoint', function() {", + " pm.expect(location).to.not.be.undefined;", + " pm.expect(location).to.include('client_id=');", + " pm.expect(location).to.include('state=');", + " pm.expect(location).to.include('response_type=code');", + "});", + "", + "// Extract the state parameter and full auth URL for subsequent requests", + "if (location) {", + " const url = new URL(location);", + " const state = url.searchParams.get('state');", + " pm.collectionVariables.set('oidc_state', state);", + " // The mock-oauth2-server's interactive login is at the authorization endpoint", + " // Newman needs the internal Docker URL, so replace localhost with mock-oauth2", + " const internalUrl = location.replace('localhost:9090', 'mock-oauth2:9090');", + " pm.collectionVariables.set('oidc_full_auth_url', internalUrl);", + "}" ], "type": "text/javascript" } @@ -240,23 +243,31 @@ { "name": "2. Simulate IdP Login", "event": [ - { - "listen": "prerequest", - "script": { - "exec": [ - "// OIDC tests disabled - skip this request", - "pm.execution.skipRequest();" - ], - "type": "text/javascript" - } - }, { "listen": "test", "script": { "exec": [ - "pm.test('SKIPPED - OIDC not yet implemented', function() {", - " pm.expect(true).to.be.true;", - "});" + "pm.test('IdP login returns redirect with code', function() {", + " pm.expect(pm.response.code).to.be.oneOf([302, 303]);", + "});", + "", + "const location = pm.response.headers.get('Location');", + "pm.test('Redirect contains authorization code', function() {", + " pm.expect(location).to.not.be.undefined;", + " pm.expect(location).to.include('code=');", + "});", + "", + "// Extract code and state from redirect", + "if (location) {", + " const url = new URL(location);", + " const code = url.searchParams.get('code');", + " const state = url.searchParams.get('state');", + " pm.collectionVariables.set('oidc_auth_code', code);", + " // State should match what we sent", + " if (state) {", + " pm.collectionVariables.set('oidc_state', state);", + " }", + "}" ], "type": "text/javascript" } @@ -293,23 +304,33 @@ { "name": "3. Complete OAuth Callback", "event": [ - { - "listen": "prerequest", - "script": { - "exec": [ - "// OIDC tests disabled - skip this request", - "pm.execution.skipRequest();" - ], - "type": "text/javascript" - } - }, { "listen": "test", "script": { "exec": [ - "pm.test('SKIPPED - OIDC not yet implemented', function() {", - " pm.expect(true).to.be.true;", - "});" + "pm.test('Callback returns 302 redirect to frontend', function() {", + " pm.response.to.have.status(302);", + "});", + "", + "const location = pm.response.headers.get('Location');", + "pm.test('Redirect contains JWT token', function() {", + " pm.expect(location).to.not.be.undefined;", + " pm.expect(location).to.include('token=');", + "});", + "", + "// Extract JWT from redirect URL", + "if (location) {", + " const url = new URL(location);", + " const token = url.searchParams.get('token');", + " if (token) {", + " pm.collectionVariables.set('oidc_jwt_token', token);", + " pm.test('JWT token is well-formed', function() {", + " pm.expect(token).to.include('.');", + " const parts = token.split('.');", + " pm.expect(parts.length).to.equal(3);", + " });", + " }", + "}" ], "type": "text/javascript" } @@ -351,8 +372,13 @@ "listen": "prerequest", "script": { "exec": [ - "// OIDC tests disabled - skip this request", - "pm.execution.skipRequest();" + "const token = pm.collectionVariables.get('oidc_jwt_token');", + "if (token) {", + " pm.request.headers.add({", + " key: 'Authorization',", + " value: 'Bearer ' + token", + " });", + "}" ], "type": "text/javascript" } @@ -361,9 +387,26 @@ "listen": "test", "script": { "exec": [ - "pm.test('SKIPPED - OIDC not yet implemented', function() {", - " pm.expect(true).to.be.true;", - "});" + "const token = pm.collectionVariables.get('oidc_jwt_token');", + "if (!token) {", + " pm.test.skip('No OIDC token available');", + "} else {", + " pm.test('Refresh returns 200 with new JWT', function() {", + " pm.response.to.have.status(200);", + " });", + "", + " const jsonData = pm.response.json();", + " pm.test('Refresh response contains login and jwt', function() {", + " pm.expect(jsonData).to.have.property('login');", + " pm.expect(jsonData).to.have.property('jwt');", + " pm.expect(jsonData.login).to.equal('testuser');", + " });", + "", + " // Update token for subsequent requests", + " if (jsonData.jwt) {", + " pm.collectionVariables.set('oidc_jwt_token', jsonData.jwt);", + " }", + "}" ], "type": "text/javascript" } @@ -391,8 +434,13 @@ "listen": "prerequest", "script": { "exec": [ - "// OIDC tests disabled - skip this request", - "pm.execution.skipRequest();" + "const token = pm.collectionVariables.get('oidc_jwt_token');", + "if (token) {", + " pm.request.headers.add({", + " key: 'Authorization',", + " value: 'Bearer ' + token", + " });", + "}" ], "type": "text/javascript" } @@ -401,9 +449,14 @@ "listen": "test", "script": { "exec": [ - "pm.test('SKIPPED - OIDC not yet implemented', function() {", - " pm.expect(true).to.be.true;", - "});" + "const token = pm.collectionVariables.get('oidc_jwt_token');", + "if (!token) {", + " pm.test.skip('No OIDC token available');", + "} else {", + " pm.test('Protected endpoint accessible with OIDC token', function() {", + " pm.expect(pm.response.code).to.be.oneOf([200, 403]);", + " });", + "}" ], "type": "text/javascript" } diff --git a/src/main/java/org/ohdsi/webapi/security/authc/LoginController.java b/src/main/java/org/ohdsi/webapi/security/authc/LoginController.java index 768b788f2..f94387c1e 100644 --- a/src/main/java/org/ohdsi/webapi/security/authc/LoginController.java +++ b/src/main/java/org/ohdsi/webapi/security/authc/LoginController.java @@ -1,10 +1,13 @@ package org.ohdsi.webapi.security.authc; +import java.net.URI; import java.util.List; +import org.ohdsi.webapi.security.authz.AuthorizationService; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.springframework.beans.factory.annotation.Qualifier; +import org.springframework.beans.factory.annotation.Value; import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty; import org.springframework.http.HttpStatus; import org.springframework.http.ResponseEntity; @@ -13,6 +16,9 @@ import org.springframework.security.core.Authentication; import org.springframework.security.core.AuthenticationException; import org.springframework.security.core.GrantedAuthority; +import org.springframework.security.core.authority.SimpleGrantedAuthority; +import org.springframework.security.oauth2.jwt.Jwt; +import org.springframework.http.HttpHeaders; import org.springframework.http.MediaType; import org.springframework.web.bind.annotation.*; @@ -133,4 +139,90 @@ public LoginService.Result login(Authentication authentication) { return loginSvc.onSuccess(authentication); } } + + /** + * OpenID Connect authentication controller. + * Implements the Authorization Code flow for SPA frontends: + * 1. /user/login/openid - redirects to the IdP authorization endpoint + * 2. /user/oauth/callback - exchanges the code for tokens, creates session, redirects to SPA + */ + @RestController + @ConditionalOnProperty(prefix = "security.auth.openId", name = "enabled", havingValue = "true") + public static class OpenId { + + private static final Logger log = LoggerFactory.getLogger(OpenId.class); + + private final LoginService loginSvc; + private final OidcAuthConfig oidcConfig; + private final AuthorizationService authorizationService; + + @Value("${security.defaultRoles:}") + private List defaultRoles; + + public OpenId(LoginService loginSvc, OidcAuthConfig oidcConfig, AuthorizationService authorizationService) { + this.loginSvc = loginSvc; + this.oidcConfig = oidcConfig; + this.authorizationService = authorizationService; + } + + @GetMapping("/user/login/openid") + public ResponseEntity login() { + String state = oidcConfig.createState(); + String authUrl = oidcConfig.buildAuthorizationUrl(state); + + log.info("OIDC: Redirecting to IdP authorization endpoint"); + return ResponseEntity.status(HttpStatus.FOUND) + .header(HttpHeaders.LOCATION, authUrl) + .build(); + } + + @GetMapping("/user/oauth/callback") + public ResponseEntity callback( + @RequestParam String code, + @RequestParam String state) { + + // Validate state for CSRF protection + if (!oidcConfig.validateState(state)) { + log.warn("OIDC: Invalid or expired state parameter"); + return ResponseEntity.status(HttpStatus.BAD_REQUEST).build(); + } + + // Exchange authorization code for ID token + Jwt idToken = oidcConfig.exchangeCodeForIdToken(code); + + String login = idToken.getSubject(); + String name = idToken.getClaimAsString("name"); + String email = idToken.getClaimAsString("email"); + if (name == null || name.isBlank()) { + name = email != null ? email : login; + } + + log.info("OIDC: Authenticated user sub={}, name={}, email={}", login, name, email); + + // Ensure the user exists in WebAPI + List filteredDefaults = defaultRoles.stream().filter(s -> !s.isBlank()).toList(); + authorizationService.ensureUserExists(login, name, UserOrigin.OPENID, filteredDefaults); + + // Extract and sync roles from the ID token + List idpRoles = oidcConfig.extractRoles(idToken); + if (!idpRoles.isEmpty()) { + log.info("OIDC: Syncing roles from token for user {}: {}", login, idpRoles); + oidcConfig.syncRoles(login, idpRoles); + } + + // Build an Authentication object and call loginSvc.onSuccess to create session + mint JWT + List authorities = idpRoles.stream() + .map(SimpleGrantedAuthority::new) + .toList(); + Authentication auth = new UsernamePasswordAuthenticationToken(login, null, authorities); + LoginService.Result result = loginSvc.onSuccess(auth); + + // Redirect to the frontend with the JWT + String redirectUrl = oidcConfig.getCallbackUi() + "?token=" + result.jwt(); + + return ResponseEntity.status(HttpStatus.FOUND) + .header(HttpHeaders.LOCATION, redirectUrl) + .build(); + } + } } diff --git a/src/main/java/org/ohdsi/webapi/security/authc/OidcAuthConfig.java b/src/main/java/org/ohdsi/webapi/security/authc/OidcAuthConfig.java new file mode 100644 index 000000000..6b4bf341a --- /dev/null +++ b/src/main/java/org/ohdsi/webapi/security/authc/OidcAuthConfig.java @@ -0,0 +1,327 @@ +package org.ohdsi.webapi.security.authc; + +import java.net.URI; +import java.net.URLEncoder; +import java.nio.charset.StandardCharsets; +import java.time.Instant; +import java.util.ArrayList; +import java.util.Collections; +import java.util.Iterator; +import java.util.List; +import java.util.Map; +import java.util.UUID; +import java.util.concurrent.ConcurrentHashMap; + +import org.ohdsi.webapi.security.authz.AuthorizationService; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +import org.springframework.beans.factory.annotation.Value; +import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty; +import org.springframework.context.annotation.Bean; +import org.springframework.context.annotation.Configuration; +import org.springframework.core.ParameterizedTypeReference; +import org.springframework.core.annotation.Order; +import org.springframework.http.HttpEntity; +import org.springframework.http.HttpHeaders; +import org.springframework.http.HttpMethod; +import org.springframework.http.MediaType; +import org.springframework.http.ResponseEntity; +import org.springframework.security.config.annotation.web.builders.HttpSecurity; +import org.springframework.security.oauth2.jwt.Jwt; +import org.springframework.security.oauth2.jwt.JwtDecoder; +import org.springframework.security.oauth2.jwt.NimbusJwtDecoder; +import org.springframework.security.web.SecurityFilterChain; +import org.springframework.util.LinkedMultiValueMap; +import org.springframework.util.MultiValueMap; +import org.springframework.web.client.RestTemplate; + +import jakarta.annotation.PostConstruct; + +@Configuration +@ConditionalOnProperty(prefix = "security.auth.openId", name = "enabled", havingValue = "true") +public class OidcAuthConfig { + + private static final Logger log = LoggerFactory.getLogger(OidcAuthConfig.class); + private static final long STATE_TTL_SECONDS = 300; // 5 minutes + + private final HttpSecurityShared httpSecurityShared; + private final AuthorizationService authorizationService; + + @Value("${security.auth.openId.clientId}") + private String clientId; + + @Value("${security.auth.openId.apiSecret}") + private String clientSecret; + + @Value("${security.auth.openId.url}") + private String discoveryUrl; + + @Value("${security.auth.openId.externalUrl:}") + private String externalUrl; + + @Value("${security.auth.openId.extraScopes:}") + private String extraScopes; + + @Value("${security.auth.openId.rolesClaim:}") + private String rolesClaim; + + @Value("${security.auth.oauth.callback.api}") + private String callbackApi; + + @Value("${security.auth.oauth.callback.ui}") + private String callbackUi; + + // Discovered endpoints + private String authorizationEndpoint; + private String tokenEndpoint; + private String jwksUri; + private String issuer; + private JwtDecoder idTokenDecoder; + + // State store for CSRF protection + private final ConcurrentHashMap stateStore = new ConcurrentHashMap<>(); + + public OidcAuthConfig(HttpSecurityShared httpSecurityShared, AuthorizationService authorizationService) { + this.httpSecurityShared = httpSecurityShared; + this.authorizationService = authorizationService; + } + + @PostConstruct + void discover() { + log.info("OIDC: Fetching discovery document from {}", discoveryUrl); + try { + RestTemplate rest = new RestTemplate(); + Map doc = rest.exchange( + discoveryUrl, HttpMethod.GET, null, + new ParameterizedTypeReference>() {}).getBody(); + + if (doc == null) { + throw new IllegalStateException("OIDC discovery document is empty"); + } + + authorizationEndpoint = (String) doc.get("authorization_endpoint"); + tokenEndpoint = (String) doc.get("token_endpoint"); + jwksUri = (String) doc.get("jwks_uri"); + issuer = (String) doc.get("issuer"); + + if (authorizationEndpoint == null || tokenEndpoint == null || jwksUri == null) { + throw new IllegalStateException( + "OIDC discovery document missing required endpoints (authorization_endpoint, token_endpoint, jwks_uri)"); + } + + idTokenDecoder = NimbusJwtDecoder.withJwkSetUri(jwksUri).build(); + + log.info("OIDC: Discovery complete. issuer={}, authz={}, token={}", issuer, authorizationEndpoint, tokenEndpoint); + } catch (Exception e) { + log.error("OIDC: Failed to fetch discovery document from {}. OIDC login will not work.", discoveryUrl, e); + } + } + + @Bean + @Order(2) + public SecurityFilterChain oidcAuthChain(HttpSecurity http) throws Exception { + httpSecurityShared.configureDefaults(http); + http.securityMatcher("/user/login/openid", "/user/oauth/callback") + .authorizeHttpRequests(auth -> auth.anyRequest().permitAll()); + return http.build(); + } + + /** + * Generate a state token and store it for CSRF validation. + */ + public String createState() { + evictExpiredStates(); + String state = UUID.randomUUID().toString(); + stateStore.put(state, Instant.now()); + return state; + } + + /** + * Validate and consume a state token. Returns true if valid. + */ + public boolean validateState(String state) { + Instant created = stateStore.remove(state); + if (created == null) { + return false; + } + return Instant.now().isBefore(created.plusSeconds(STATE_TTL_SECONDS)); + } + + /** + * Build the authorization URL for redirecting the user to the IdP. + */ + public String buildAuthorizationUrl(String state) { + // Use externalUrl for the authorization endpoint if configured (browser-accessible URL) + String authEndpoint = resolveExternalAuthEndpoint(); + + StringBuilder url = new StringBuilder(authEndpoint); + url.append("?response_type=code"); + url.append("&client_id=").append(encode(clientId)); + url.append("&redirect_uri=").append(encode(callbackApi)); + url.append("&state=").append(encode(state)); + + String scope = "openid"; + if (extraScopes != null && !extraScopes.isBlank()) { + scope = scope + " " + extraScopes.trim(); + } + url.append("&scope=").append(encode(scope)); + + return url.toString(); + } + + /** + * Exchange an authorization code for tokens, decode the ID token, and return it. + */ + public Jwt exchangeCodeForIdToken(String code) { + RestTemplate rest = new RestTemplate(); + + HttpHeaders headers = new HttpHeaders(); + headers.setContentType(MediaType.APPLICATION_FORM_URLENCODED); + + MultiValueMap params = new LinkedMultiValueMap<>(); + params.add("grant_type", "authorization_code"); + params.add("code", code); + params.add("redirect_uri", callbackApi); + params.add("client_id", clientId); + params.add("client_secret", clientSecret); + + HttpEntity> request = new HttpEntity<>(params, headers); + + ResponseEntity> response = rest.exchange( + tokenEndpoint, HttpMethod.POST, request, + new ParameterizedTypeReference>() {}); + + Map body = response.getBody(); + if (body == null || !body.containsKey("id_token")) { + throw new IllegalStateException("OIDC token response missing id_token"); + } + + String idTokenStr = (String) body.get("id_token"); + return idTokenDecoder.decode(idTokenStr); + } + + /** + * Extract roles from the ID token using the configured rolesClaim. + * Supports dot-notation for nested claims (e.g., "realm_access.roles"). + */ + @SuppressWarnings("unchecked") + public List extractRoles(Jwt idToken) { + if (rolesClaim == null || rolesClaim.isBlank()) { + return Collections.emptyList(); + } + + String[] parts = rolesClaim.split("\\."); + Object current = idToken.getClaims(); + + for (String part : parts) { + if (current instanceof Map) { + current = ((Map) current).get(part); + } else { + log.warn("OIDC: Cannot traverse claim path '{}' - intermediate value is not a map", rolesClaim); + return Collections.emptyList(); + } + if (current == null) { + log.debug("OIDC: Claim '{}' not found in ID token", rolesClaim); + return Collections.emptyList(); + } + } + + if (current instanceof List list) { + List roles = new ArrayList<>(); + for (Object item : list) { + if (item instanceof String s) { + roles.add(s); + } + } + return roles; + } else if (current instanceof String s) { + return List.of(s); + } + + log.warn("OIDC: Claim '{}' is not a list or string: {}", rolesClaim, current.getClass().getName()); + return Collections.emptyList(); + } + + /** + * Full-sync roles from the IdP token to the user in the database. + * Adds roles present in the token, removes OPENID-origin roles not in the token. + * Only touches roles with OPENID origin. + */ + public void syncRoles(String login, List idpRoles) { + List currentOidcRoleNames = getOidcOriginRoleNames(login); + + // Add new roles from the token + for (String roleName : idpRoles) { + if (!currentOidcRoleNames.contains(roleName)) { + try { + authorizationService.addUserToRole(roleName, login, UserOrigin.OPENID); + log.info("OIDC: Added role '{}' to user '{}'", roleName, login); + } catch (Exception e) { + log.warn("OIDC: Could not add role '{}' to user '{}': {}", roleName, login, e.getMessage()); + } + } + } + + // Remove OPENID-origin roles no longer in the token + for (String roleName : currentOidcRoleNames) { + if (!idpRoles.contains(roleName)) { + try { + authorizationService.removeUserFromRole(roleName, login, UserOrigin.OPENID); + log.info("OIDC: Removed role '{}' from user '{}'", roleName, login); + } catch (Exception e) { + log.warn("OIDC: Could not remove role '{}' from user '{}': {}", roleName, login, e.getMessage()); + } + } + } + } + + public String getCallbackUi() { + return callbackUi; + } + + // --- private helpers --- + + private List getOidcOriginRoleNames(String login) { + try { + return authorizationService.getOidcOriginRoles(login); + } catch (Exception e) { + log.warn("OIDC: Could not fetch OPENID-origin roles for user {}: {}", login, e.getMessage()); + return Collections.emptyList(); + } + } + + private String resolveExternalAuthEndpoint() { + if (externalUrl != null && !externalUrl.isBlank() && authorizationEndpoint != null) { + // Replace the host portion of the authorization endpoint with the external URL + // e.g., internal: http://mock-oauth2:9090/default/authorize + // external: http://localhost:9090/default + // result: http://localhost:9090/default/authorize + try { + URI authUri = URI.create(authorizationEndpoint); + URI extUri = URI.create(externalUrl); + // Take the path suffix from authorizationEndpoint that extends beyond the discovery path + String discoveryBase = discoveryUrl.replace("/.well-known/openid-configuration", ""); + URI discoveryBaseUri = URI.create(discoveryBase); + String pathSuffix = authUri.getPath().substring(discoveryBaseUri.getPath().length()); + return extUri.toString() + pathSuffix; + } catch (Exception e) { + log.warn("OIDC: Could not resolve external auth endpoint, using discovery value", e); + } + } + return authorizationEndpoint; + } + + private void evictExpiredStates() { + Instant cutoff = Instant.now().minusSeconds(STATE_TTL_SECONDS); + Iterator> it = stateStore.entrySet().iterator(); + while (it.hasNext()) { + if (it.next().getValue().isBefore(cutoff)) { + it.remove(); + } + } + } + + private static String encode(String value) { + return URLEncoder.encode(value, StandardCharsets.UTF_8); + } +} diff --git a/src/main/java/org/ohdsi/webapi/security/authc/UserOrigin.java b/src/main/java/org/ohdsi/webapi/security/authc/UserOrigin.java index 960fb7676..61138422e 100644 --- a/src/main/java/org/ohdsi/webapi/security/authc/UserOrigin.java +++ b/src/main/java/org/ohdsi/webapi/security/authc/UserOrigin.java @@ -3,7 +3,7 @@ import org.ohdsi.webapi.security.provisioning.model.LdapProviderType; public enum UserOrigin { - SYSTEM, AD, LDAP, WINDOWS, KERBEROS, GOOGLE, FACEBOOK, DATABASE; + SYSTEM, AD, LDAP, WINDOWS, KERBEROS, GOOGLE, FACEBOOK, DATABASE, OPENID; public static UserOrigin getFrom(LdapProviderType ldapProviderType) { switch (ldapProviderType) { diff --git a/src/main/java/org/ohdsi/webapi/security/authz/AuthorizationService.java b/src/main/java/org/ohdsi/webapi/security/authz/AuthorizationService.java index 4ef423ed4..ac8d063a1 100644 --- a/src/main/java/org/ohdsi/webapi/security/authz/AuthorizationService.java +++ b/src/main/java/org/ohdsi/webapi/security/authz/AuthorizationService.java @@ -152,6 +152,20 @@ public void removeUserFromRole(String roleName, String login, UserOrigin origin) this.roleService.removeUserFromRole(login, roleName, origin); } + /** + * Get the names of system roles assigned to a user with a specific origin. + * Used for OIDC role sync to identify which roles were granted by the IdP. + */ + public List getOidcOriginRoles(String login) { + UserEntity user = userService.getUserByLogin(login).orElseThrow(); + return user.getUserRoles().stream() + .filter(ur -> ur.getOrigin() == UserOrigin.OPENID) + .map(ur -> ur.getRole()) + .filter(role -> Boolean.TRUE.equals(role.isSystemRole())) + .map(role -> role.getName()) + .toList(); + } + public void addUserToRole(String roleName, String login, UserOrigin origin) { this.roleService.addUserToRole(login, roleName, origin); } diff --git a/src/main/resources/application.yaml b/src/main/resources/application.yaml index 5a4ebbcef..a5548a8bf 100644 --- a/src/main/resources/application.yaml +++ b/src/main/resources/application.yaml @@ -224,6 +224,7 @@ security: extraScopes: "" logoutUrl: "" redirectUrl: http://localhost/index.html#/welcome/ + rolesClaim: "" url: "" saml: # SAML (Security Assertion Markup Language) From 17edae60deaff6fde9194d0ebd8a44ca430da2b0 Mon Sep 17 00:00:00 2001 From: Peter Hoffmann <954078+p-hoffmann@users.noreply.github.com> Date: Fri, 24 Apr 2026 00:34:48 +0800 Subject: [PATCH 2/8] cleanup --- .../auth-tests.postman_collection.json | 5 +- .../security/authc/LoginController.java | 93 +---- .../webapi/security/authc/LoginService.java | 16 +- .../webapi/security/authc/OidcAuthConfig.java | 381 ++++++++---------- .../security/authz/AuthorizationService.java | 6 +- 5 files changed, 181 insertions(+), 320 deletions(-) diff --git a/docker/auth-test/postman/auth-tests.postman_collection.json b/docker/auth-test/postman/auth-tests.postman_collection.json index 805f4db17..c8419e78f 100644 --- a/docker/auth-test/postman/auth-tests.postman_collection.json +++ b/docker/auth-test/postman/auth-tests.postman_collection.json @@ -343,14 +343,15 @@ "method": "GET", "header": [], "url": { - "raw": "{{base_url}}/user/oauth/callback?code={{oidc_auth_code}}&state={{oidc_state}}", + "raw": "{{base_url}}/user/oauth/callback/openid?code={{oidc_auth_code}}&state={{oidc_state}}", "host": [ "{{base_url}}" ], "path": [ "user", "oauth", - "callback" + "callback", + "openid" ], "query": [ { diff --git a/src/main/java/org/ohdsi/webapi/security/authc/LoginController.java b/src/main/java/org/ohdsi/webapi/security/authc/LoginController.java index f94387c1e..f506e5579 100644 --- a/src/main/java/org/ohdsi/webapi/security/authc/LoginController.java +++ b/src/main/java/org/ohdsi/webapi/security/authc/LoginController.java @@ -1,25 +1,19 @@ package org.ohdsi.webapi.security.authc; -import java.net.URI; import java.util.List; -import org.ohdsi.webapi.security.authz.AuthorizationService; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.springframework.beans.factory.annotation.Qualifier; -import org.springframework.beans.factory.annotation.Value; import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty; import org.springframework.http.HttpStatus; +import org.springframework.http.MediaType; import org.springframework.http.ResponseEntity; import org.springframework.security.authentication.AuthenticationManager; import org.springframework.security.authentication.UsernamePasswordAuthenticationToken; import org.springframework.security.core.Authentication; import org.springframework.security.core.AuthenticationException; import org.springframework.security.core.GrantedAuthority; -import org.springframework.security.core.authority.SimpleGrantedAuthority; -import org.springframework.security.oauth2.jwt.Jwt; -import org.springframework.http.HttpHeaders; -import org.springframework.http.MediaType; import org.springframework.web.bind.annotation.*; /** @@ -140,89 +134,4 @@ public LoginService.Result login(Authentication authentication) { } } - /** - * OpenID Connect authentication controller. - * Implements the Authorization Code flow for SPA frontends: - * 1. /user/login/openid - redirects to the IdP authorization endpoint - * 2. /user/oauth/callback - exchanges the code for tokens, creates session, redirects to SPA - */ - @RestController - @ConditionalOnProperty(prefix = "security.auth.openId", name = "enabled", havingValue = "true") - public static class OpenId { - - private static final Logger log = LoggerFactory.getLogger(OpenId.class); - - private final LoginService loginSvc; - private final OidcAuthConfig oidcConfig; - private final AuthorizationService authorizationService; - - @Value("${security.defaultRoles:}") - private List defaultRoles; - - public OpenId(LoginService loginSvc, OidcAuthConfig oidcConfig, AuthorizationService authorizationService) { - this.loginSvc = loginSvc; - this.oidcConfig = oidcConfig; - this.authorizationService = authorizationService; - } - - @GetMapping("/user/login/openid") - public ResponseEntity login() { - String state = oidcConfig.createState(); - String authUrl = oidcConfig.buildAuthorizationUrl(state); - - log.info("OIDC: Redirecting to IdP authorization endpoint"); - return ResponseEntity.status(HttpStatus.FOUND) - .header(HttpHeaders.LOCATION, authUrl) - .build(); - } - - @GetMapping("/user/oauth/callback") - public ResponseEntity callback( - @RequestParam String code, - @RequestParam String state) { - - // Validate state for CSRF protection - if (!oidcConfig.validateState(state)) { - log.warn("OIDC: Invalid or expired state parameter"); - return ResponseEntity.status(HttpStatus.BAD_REQUEST).build(); - } - - // Exchange authorization code for ID token - Jwt idToken = oidcConfig.exchangeCodeForIdToken(code); - - String login = idToken.getSubject(); - String name = idToken.getClaimAsString("name"); - String email = idToken.getClaimAsString("email"); - if (name == null || name.isBlank()) { - name = email != null ? email : login; - } - - log.info("OIDC: Authenticated user sub={}, name={}, email={}", login, name, email); - - // Ensure the user exists in WebAPI - List filteredDefaults = defaultRoles.stream().filter(s -> !s.isBlank()).toList(); - authorizationService.ensureUserExists(login, name, UserOrigin.OPENID, filteredDefaults); - - // Extract and sync roles from the ID token - List idpRoles = oidcConfig.extractRoles(idToken); - if (!idpRoles.isEmpty()) { - log.info("OIDC: Syncing roles from token for user {}: {}", login, idpRoles); - oidcConfig.syncRoles(login, idpRoles); - } - - // Build an Authentication object and call loginSvc.onSuccess to create session + mint JWT - List authorities = idpRoles.stream() - .map(SimpleGrantedAuthority::new) - .toList(); - Authentication auth = new UsernamePasswordAuthenticationToken(login, null, authorities); - LoginService.Result result = loginSvc.onSuccess(auth); - - // Redirect to the frontend with the JWT - String redirectUrl = oidcConfig.getCallbackUi() + "?token=" + result.jwt(); - - return ResponseEntity.status(HttpStatus.FOUND) - .header(HttpHeaders.LOCATION, redirectUrl) - .build(); - } - } } diff --git a/src/main/java/org/ohdsi/webapi/security/authc/LoginService.java b/src/main/java/org/ohdsi/webapi/security/authc/LoginService.java index b4c23e4d5..78061f3d2 100644 --- a/src/main/java/org/ohdsi/webapi/security/authc/LoginService.java +++ b/src/main/java/org/ohdsi/webapi/security/authc/LoginService.java @@ -52,24 +52,22 @@ public LoginService( } public Result onSuccess(Authentication authentication) { + String login = authentication.getName().toLowerCase(); + authorizationService.ensureUserExists(login, login, null, this.defaultRoles); + return mintSession(authentication); + } + // Skips ensureUserExists; for callers (e.g. OIDC) that provision the user themselves. + public Result mintSession(Authentication authentication) { String login = authentication.getName().toLowerCase(); - log.info("LoginService: onSuccess: " + login); + log.info("LoginService: mintSession: " + login); String[] roles = authentication.getAuthorities().stream() .map(GrantedAuthority::getAuthority) .toArray(String[]::new); - // ensure the user exists - authorizationService.ensureUserExists(login, login, null, this.defaultRoles); - - // Generate a unique session ID and store session UUID sessionId = sessionService.createSession(login); - - // Calculate expiration for JWT (same as session) Instant expiresAt = Instant.now().plus(sessionProps.getExpiration()); - - // mint the JWT String jwt = jwtService.generateToken(login, sessionId.toString(), Date.from(expiresAt)); return new Result(login, jwt, roles, "Login successful"); diff --git a/src/main/java/org/ohdsi/webapi/security/authc/OidcAuthConfig.java b/src/main/java/org/ohdsi/webapi/security/authc/OidcAuthConfig.java index 6b4bf341a..c3fb1cd31 100644 --- a/src/main/java/org/ohdsi/webapi/security/authc/OidcAuthConfig.java +++ b/src/main/java/org/ohdsi/webapi/security/authc/OidcAuthConfig.java @@ -1,16 +1,12 @@ package org.ohdsi.webapi.security.authc; +import java.io.IOException; import java.net.URI; -import java.net.URLEncoder; -import java.nio.charset.StandardCharsets; -import java.time.Instant; import java.util.ArrayList; -import java.util.Collections; -import java.util.Iterator; +import java.util.LinkedHashSet; import java.util.List; import java.util.Map; -import java.util.UUID; -import java.util.concurrent.ConcurrentHashMap; +import java.util.Set; import org.ohdsi.webapi.security.authz.AuthorizationService; import org.slf4j.Logger; @@ -19,33 +15,32 @@ import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty; import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; -import org.springframework.core.ParameterizedTypeReference; import org.springframework.core.annotation.Order; -import org.springframework.http.HttpEntity; -import org.springframework.http.HttpHeaders; -import org.springframework.http.HttpMethod; -import org.springframework.http.MediaType; -import org.springframework.http.ResponseEntity; +import org.springframework.security.authentication.UsernamePasswordAuthenticationToken; import org.springframework.security.config.annotation.web.builders.HttpSecurity; -import org.springframework.security.oauth2.jwt.Jwt; -import org.springframework.security.oauth2.jwt.JwtDecoder; -import org.springframework.security.oauth2.jwt.NimbusJwtDecoder; +import org.springframework.security.core.Authentication; +import org.springframework.security.core.authority.SimpleGrantedAuthority; +import org.springframework.security.oauth2.client.registration.ClientRegistration; +import org.springframework.security.oauth2.client.registration.ClientRegistrationRepository; +import org.springframework.security.oauth2.client.registration.ClientRegistrations; +import org.springframework.security.oauth2.client.registration.InMemoryClientRegistrationRepository; +import org.springframework.security.oauth2.core.oidc.user.OidcUser; import org.springframework.security.web.SecurityFilterChain; -import org.springframework.util.LinkedMultiValueMap; -import org.springframework.util.MultiValueMap; -import org.springframework.web.client.RestTemplate; -import jakarta.annotation.PostConstruct; +import jakarta.servlet.http.HttpServletRequest; +import jakarta.servlet.http.HttpServletResponse; @Configuration @ConditionalOnProperty(prefix = "security.auth.openId", name = "enabled", havingValue = "true") public class OidcAuthConfig { private static final Logger log = LoggerFactory.getLogger(OidcAuthConfig.class); - private static final long STATE_TTL_SECONDS = 300; // 5 minutes + private static final String REGISTRATION_ID = "openid"; + private static final String DISCOVERY_SUFFIX = "/.well-known/openid-configuration"; private final HttpSecurityShared httpSecurityShared; private final AuthorizationService authorizationService; + private final LoginService loginService; @Value("${security.auth.openId.clientId}") private String clientId; @@ -54,7 +49,7 @@ public class OidcAuthConfig { private String clientSecret; @Value("${security.auth.openId.url}") - private String discoveryUrl; + private String discoveryOrIssuerUrl; @Value("${security.auth.openId.externalUrl:}") private String externalUrl; @@ -65,192 +60,107 @@ public class OidcAuthConfig { @Value("${security.auth.openId.rolesClaim:}") private String rolesClaim; + // Default true to mirror LdapAuthConfig's SimpleGrantedAuthority upper-casing, so roles from + // the two IdPs collide in sec_role by name rather than creating parallel mixed-case duplicates. + @Value("${security.auth.openId.rolesToUpperCase:true}") + private boolean rolesToUpperCase; + @Value("${security.auth.oauth.callback.api}") private String callbackApi; @Value("${security.auth.oauth.callback.ui}") private String callbackUi; - // Discovered endpoints - private String authorizationEndpoint; - private String tokenEndpoint; - private String jwksUri; - private String issuer; - private JwtDecoder idTokenDecoder; - - // State store for CSRF protection - private final ConcurrentHashMap stateStore = new ConcurrentHashMap<>(); + @Value("${security.defaultRoles:}") + private List defaultRoles; - public OidcAuthConfig(HttpSecurityShared httpSecurityShared, AuthorizationService authorizationService) { + public OidcAuthConfig(HttpSecurityShared httpSecurityShared, + AuthorizationService authorizationService, + LoginService loginService) { this.httpSecurityShared = httpSecurityShared; this.authorizationService = authorizationService; + this.loginService = loginService; } - @PostConstruct - void discover() { - log.info("OIDC: Fetching discovery document from {}", discoveryUrl); - try { - RestTemplate rest = new RestTemplate(); - Map doc = rest.exchange( - discoveryUrl, HttpMethod.GET, null, - new ParameterizedTypeReference>() {}).getBody(); - - if (doc == null) { - throw new IllegalStateException("OIDC discovery document is empty"); - } - - authorizationEndpoint = (String) doc.get("authorization_endpoint"); - tokenEndpoint = (String) doc.get("token_endpoint"); - jwksUri = (String) doc.get("jwks_uri"); - issuer = (String) doc.get("issuer"); - - if (authorizationEndpoint == null || tokenEndpoint == null || jwksUri == null) { - throw new IllegalStateException( - "OIDC discovery document missing required endpoints (authorization_endpoint, token_endpoint, jwks_uri)"); - } - - idTokenDecoder = NimbusJwtDecoder.withJwkSetUri(jwksUri).build(); - - log.info("OIDC: Discovery complete. issuer={}, authz={}, token={}", issuer, authorizationEndpoint, tokenEndpoint); - } catch (Exception e) { - log.error("OIDC: Failed to fetch discovery document from {}. OIDC login will not work.", discoveryUrl, e); + @Bean + public ClientRegistrationRepository oidcClientRegistrationRepository() { + String issuer = stripDiscoverySuffix(discoveryOrIssuerUrl); + log.info("OIDC: Discovering provider metadata from issuer {}", issuer); + + ClientRegistration.Builder builder = ClientRegistrations.fromIssuerLocation(issuer) + .registrationId(REGISTRATION_ID) + .clientId(clientId) + .clientSecret(clientSecret) + .redirectUri(joinPath(callbackApi, REGISTRATION_ID)) + .scope(buildScopes()); + + if (externalUrl != null && !externalUrl.isBlank()) { + String discoveredAuthUri = builder.build().getProviderDetails().getAuthorizationUri(); + String rewritten = rewriteAuthorizationUri(discoveredAuthUri); + builder.authorizationUri(rewritten); + log.info("OIDC: Using external authorization URI {}", rewritten); } + + return new InMemoryClientRegistrationRepository(builder.build()); } @Bean - @Order(2) + @Order(1) public SecurityFilterChain oidcAuthChain(HttpSecurity http) throws Exception { httpSecurityShared.configureDefaults(http); - http.securityMatcher("/user/login/openid", "/user/oauth/callback") - .authorizeHttpRequests(auth -> auth.anyRequest().permitAll()); + http + .securityMatcher("/user/login/" + REGISTRATION_ID, "/user/oauth/callback/**") + .authorizeHttpRequests(auth -> auth.anyRequest().permitAll()) + .oauth2Login(oauth -> oauth + .authorizationEndpoint(authz -> authz.baseUri("/user/login")) + .redirectionEndpoint(redir -> redir.baseUri("/user/oauth/callback/*")) + .successHandler(this::handleSuccess) + .failureHandler((req, res, ex) -> { + log.warn("OIDC: Authentication failed: {}", ex.getMessage()); + res.sendRedirect(appendQueryParam(callbackUi, "error", "oidc_failed")); + })); return http.build(); } - /** - * Generate a state token and store it for CSRF validation. - */ - public String createState() { - evictExpiredStates(); - String state = UUID.randomUUID().toString(); - stateStore.put(state, Instant.now()); - return state; - } - - /** - * Validate and consume a state token. Returns true if valid. - */ - public boolean validateState(String state) { - Instant created = stateStore.remove(state); - if (created == null) { - return false; - } - return Instant.now().isBefore(created.plusSeconds(STATE_TTL_SECONDS)); - } + private void handleSuccess(HttpServletRequest request, + HttpServletResponse response, + Authentication authentication) throws IOException { + OidcUser oidcUser = (OidcUser) authentication.getPrincipal(); + // Lowercase to match LoginService.mintSession which normalizes via Authentication.getName().toLowerCase(); + // a mixed-case sub would otherwise create the DB row as-is while the JWT encodes the lowercased form. + String login = oidcUser.getSubject().toLowerCase(); + String name = firstNonBlank(oidcUser.getFullName(), oidcUser.getEmail(), login); - /** - * Build the authorization URL for redirecting the user to the IdP. - */ - public String buildAuthorizationUrl(String state) { - // Use externalUrl for the authorization endpoint if configured (browser-accessible URL) - String authEndpoint = resolveExternalAuthEndpoint(); + log.info("OIDC: Authenticated user sub={}", login); - StringBuilder url = new StringBuilder(authEndpoint); - url.append("?response_type=code"); - url.append("&client_id=").append(encode(clientId)); - url.append("&redirect_uri=").append(encode(callbackApi)); - url.append("&state=").append(encode(state)); + List filteredDefaults = defaultRoles.stream().filter(s -> !s.isBlank()).toList(); + authorizationService.ensureUserExists(login, name, UserOrigin.OPENID, filteredDefaults); - String scope = "openid"; - if (extraScopes != null && !extraScopes.isBlank()) { - scope = scope + " " + extraScopes.trim(); + List idpRoles = extractRoles(oidcUser.getClaims(), rolesClaim, rolesToUpperCase); + if (!idpRoles.isEmpty()) { + log.info("OIDC: Syncing roles from token for user {}: {}", login, idpRoles); + syncRoles(login, idpRoles); } - url.append("&scope=").append(encode(scope)); - - return url.toString(); - } - /** - * Exchange an authorization code for tokens, decode the ID token, and return it. - */ - public Jwt exchangeCodeForIdToken(String code) { - RestTemplate rest = new RestTemplate(); + List authorities = idpRoles.stream() + .map(SimpleGrantedAuthority::new) + .toList(); +Authentication wrapped = new UsernamePasswordAuthenticationToken(login, null, authorities); + // mintSession — not onSuccess — so onSuccess's ensureUserExists doesn't overwrite the display name. + LoginService.Result result = loginService.mintSession(wrapped); - HttpHeaders headers = new HttpHeaders(); - headers.setContentType(MediaType.APPLICATION_FORM_URLENCODED); - - MultiValueMap params = new LinkedMultiValueMap<>(); - params.add("grant_type", "authorization_code"); - params.add("code", code); - params.add("redirect_uri", callbackApi); - params.add("client_id", clientId); - params.add("client_secret", clientSecret); - - HttpEntity> request = new HttpEntity<>(params, headers); - - ResponseEntity> response = rest.exchange( - tokenEndpoint, HttpMethod.POST, request, - new ParameterizedTypeReference>() {}); - - Map body = response.getBody(); - if (body == null || !body.containsKey("id_token")) { - throw new IllegalStateException("OIDC token response missing id_token"); - } - - String idTokenStr = (String) body.get("id_token"); - return idTokenDecoder.decode(idTokenStr); + response.sendRedirect(appendQueryParam(callbackUi, "token", result.jwt())); } - /** - * Extract roles from the ID token using the configured rolesClaim. - * Supports dot-notation for nested claims (e.g., "realm_access.roles"). - */ - @SuppressWarnings("unchecked") - public List extractRoles(Jwt idToken) { - if (rolesClaim == null || rolesClaim.isBlank()) { - return Collections.emptyList(); - } - - String[] parts = rolesClaim.split("\\."); - Object current = idToken.getClaims(); - - for (String part : parts) { - if (current instanceof Map) { - current = ((Map) current).get(part); - } else { - log.warn("OIDC: Cannot traverse claim path '{}' - intermediate value is not a map", rolesClaim); - return Collections.emptyList(); - } - if (current == null) { - log.debug("OIDC: Claim '{}' not found in ID token", rolesClaim); - return Collections.emptyList(); - } - } - - if (current instanceof List list) { - List roles = new ArrayList<>(); - for (Object item : list) { - if (item instanceof String s) { - roles.add(s); - } - } - return roles; - } else if (current instanceof String s) { - return List.of(s); + private void syncRoles(String login, List idpRoles) { + List currentOidcRoleNames; + try { + currentOidcRoleNames = authorizationService.getOidcOriginRoles(login); + } catch (Exception e) { + log.warn("OIDC: Could not fetch OPENID-origin roles for user {}: {}", login, e.getMessage()); + return; } - log.warn("OIDC: Claim '{}' is not a list or string: {}", rolesClaim, current.getClass().getName()); - return Collections.emptyList(); - } - - /** - * Full-sync roles from the IdP token to the user in the database. - * Adds roles present in the token, removes OPENID-origin roles not in the token. - * Only touches roles with OPENID origin. - */ - public void syncRoles(String login, List idpRoles) { - List currentOidcRoleNames = getOidcOriginRoleNames(login); - - // Add new roles from the token for (String roleName : idpRoles) { if (!currentOidcRoleNames.contains(roleName)) { try { @@ -262,7 +172,6 @@ public void syncRoles(String login, List idpRoles) { } } - // Remove OPENID-origin roles no longer in the token for (String roleName : currentOidcRoleNames) { if (!idpRoles.contains(roleName)) { try { @@ -275,53 +184,99 @@ public void syncRoles(String login, List idpRoles) { } } - public String getCallbackUi() { - return callbackUi; + private Set buildScopes() { + Set scopes = new LinkedHashSet<>(); + scopes.add("openid"); + scopes.add("profile"); + scopes.add("email"); + if (extraScopes != null && !extraScopes.isBlank()) { + for (String s : extraScopes.trim().split("\\s+")) { + if (!s.isBlank()) { + scopes.add(s); + } + } + } + return scopes; + } + + private static String joinPath(String base, String segment) { + return (base.endsWith("/") ? base : base + "/") + segment; } - // --- private helpers --- + private static String appendQueryParam(String url, String key, String value) { + String separator = url.contains("?") ? "&" : "?"; + return url + separator + key + "=" + value; + } - private List getOidcOriginRoleNames(String login) { + private String stripDiscoverySuffix(String url) { + if (url == null || url.isBlank()) { + throw new IllegalStateException("security.auth.openId.url must be configured when OpenID is enabled"); + } + if (url.endsWith(DISCOVERY_SUFFIX)) { + return url.substring(0, url.length() - DISCOVERY_SUFFIX.length()); + } + return url; + } + + private String rewriteAuthorizationUri(String authorizationUri) { try { - return authorizationService.getOidcOriginRoles(login); + URI authUri = URI.create(authorizationUri); + URI extUri = URI.create(externalUrl); + String discoveryBase = stripDiscoverySuffix(discoveryOrIssuerUrl); + URI discoveryBaseUri = URI.create(discoveryBase); + String pathSuffix = authUri.getPath().substring(discoveryBaseUri.getPath().length()); + String extBase = extUri.toString(); + if (extBase.endsWith("/")) { + extBase = extBase.substring(0, extBase.length() - 1); + } + return extBase + pathSuffix; } catch (Exception e) { - log.warn("OIDC: Could not fetch OPENID-origin roles for user {}: {}", login, e.getMessage()); - return Collections.emptyList(); + log.warn("OIDC: Could not rewrite authorization URI with externalUrl '{}', using discovered value '{}'", + externalUrl, authorizationUri, e); + return authorizationUri; } } - private String resolveExternalAuthEndpoint() { - if (externalUrl != null && !externalUrl.isBlank() && authorizationEndpoint != null) { - // Replace the host portion of the authorization endpoint with the external URL - // e.g., internal: http://mock-oauth2:9090/default/authorize - // external: http://localhost:9090/default - // result: http://localhost:9090/default/authorize - try { - URI authUri = URI.create(authorizationEndpoint); - URI extUri = URI.create(externalUrl); - // Take the path suffix from authorizationEndpoint that extends beyond the discovery path - String discoveryBase = discoveryUrl.replace("/.well-known/openid-configuration", ""); - URI discoveryBaseUri = URI.create(discoveryBase); - String pathSuffix = authUri.getPath().substring(discoveryBaseUri.getPath().length()); - return extUri.toString() + pathSuffix; - } catch (Exception e) { - log.warn("OIDC: Could not resolve external auth endpoint, using discovery value", e); - } + private static String firstNonBlank(String... values) { + if (values == null) return null; + for (String v : values) { + if (v != null && !v.isBlank()) return v; } - return authorizationEndpoint; + return null; } - private void evictExpiredStates() { - Instant cutoff = Instant.now().minusSeconds(STATE_TTL_SECONDS); - Iterator> it = stateStore.entrySet().iterator(); - while (it.hasNext()) { - if (it.next().getValue().isBefore(cutoff)) { - it.remove(); + static List extractRoles(Map claims, String claimPath, boolean toUpperCase) { + if (claimPath == null || claimPath.isBlank() || claims == null) { + return List.of(); + } + String[] parts = claimPath.split("\\."); + Object current = claims; + for (String part : parts) { + if (current instanceof Map map) { + current = map.get(part); + } else { + log.warn("OIDC: Cannot traverse claim path '{}' - intermediate value is not a map", claimPath); + return List.of(); + } + if (current == null) { + log.debug("OIDC: Claim '{}' not found in ID token", claimPath); + return List.of(); } } - } - private static String encode(String value) { - return URLEncoder.encode(value, StandardCharsets.UTF_8); + if (current instanceof List list) { + List roles = new ArrayList<>(); + for (Object item : list) { + if (item instanceof String s) { + roles.add(toUpperCase ? s.toUpperCase() : s); + } + } + return roles; + } + if (current instanceof String s) { + return List.of(toUpperCase ? s.toUpperCase() : s); + } + log.warn("OIDC: Claim '{}' is not a list or string: {}", claimPath, current.getClass().getName()); + return List.of(); } } diff --git a/src/main/java/org/ohdsi/webapi/security/authz/AuthorizationService.java b/src/main/java/org/ohdsi/webapi/security/authz/AuthorizationService.java index 0c81a47ae..03b0e4e83 100644 --- a/src/main/java/org/ohdsi/webapi/security/authz/AuthorizationService.java +++ b/src/main/java/org/ohdsi/webapi/security/authz/AuthorizationService.java @@ -175,10 +175,8 @@ public void removeUserFromRole(String roleName, String login, UserOrigin origin) this.roleService.removeUserFromRole(login, roleName, origin); } - /** - * Get the names of system roles assigned to a user with a specific origin. - * Used for OIDC role sync to identify which roles were granted by the IdP. - */ + // @Transactional required: UserEntity.userRoles is FetchType.LAZY and getUserByLogin closes its own txn. + @Transactional(readOnly = true) public List getOidcOriginRoles(String login) { UserEntity user = userService.getUserByLogin(login).orElseThrow(); return user.getUserRoles().stream() From d539461e819a6196de07594760b0725cdac78430 Mon Sep 17 00:00:00 2001 From: Peter Hoffmann <954078+p-hoffmann@users.noreply.github.com> Date: Fri, 24 Apr 2026 00:44:55 +0800 Subject: [PATCH 3/8] align naming --- .../webapi/auth/AuthProviderService.java | 4 +-- .../webapi/security/authc/OidcAuthConfig.java | 26 +++++++++---------- .../webapi/security/authc/UserOrigin.java | 2 +- .../security/authz/AuthorizationService.java | 2 +- src/main/resources/application.yaml | 2 +- 5 files changed, 18 insertions(+), 18 deletions(-) diff --git a/src/main/java/org/ohdsi/webapi/auth/AuthProviderService.java b/src/main/java/org/ohdsi/webapi/auth/AuthProviderService.java index 94264c6ad..19a5921fb 100644 --- a/src/main/java/org/ohdsi/webapi/auth/AuthProviderService.java +++ b/src/main/java/org/ohdsi/webapi/auth/AuthProviderService.java @@ -50,7 +50,7 @@ public class AuthProviderService { @Value("${security.auth.cas.enabled}") private boolean casAuthEnabled; - @Value("${security.auth.openId.enabled}") + @Value("${security.auth.oidc.enabled}") private boolean openidAuthEnabled; @Value("${security.auth.oauth.facebook.enabled}") @@ -65,7 +65,7 @@ public class AuthProviderService { @Value("${security.auth.saml.enabled}") private boolean samlAuthEnabled; - @Value("${security.auth.openId.logoutUrl:}") + @Value("${security.auth.oidc.logoutUrl:}") private String oidcLogoutUrl; /** diff --git a/src/main/java/org/ohdsi/webapi/security/authc/OidcAuthConfig.java b/src/main/java/org/ohdsi/webapi/security/authc/OidcAuthConfig.java index c3fb1cd31..9dc6b7142 100644 --- a/src/main/java/org/ohdsi/webapi/security/authc/OidcAuthConfig.java +++ b/src/main/java/org/ohdsi/webapi/security/authc/OidcAuthConfig.java @@ -31,7 +31,7 @@ import jakarta.servlet.http.HttpServletResponse; @Configuration -@ConditionalOnProperty(prefix = "security.auth.openId", name = "enabled", havingValue = "true") +@ConditionalOnProperty(prefix = "security.auth.oidc", name = "enabled", havingValue = "true") public class OidcAuthConfig { private static final Logger log = LoggerFactory.getLogger(OidcAuthConfig.class); @@ -42,27 +42,27 @@ public class OidcAuthConfig { private final AuthorizationService authorizationService; private final LoginService loginService; - @Value("${security.auth.openId.clientId}") + @Value("${security.auth.oidc.clientId}") private String clientId; - @Value("${security.auth.openId.apiSecret}") + @Value("${security.auth.oidc.apiSecret}") private String clientSecret; - @Value("${security.auth.openId.url}") + @Value("${security.auth.oidc.url}") private String discoveryOrIssuerUrl; - @Value("${security.auth.openId.externalUrl:}") + @Value("${security.auth.oidc.externalUrl:}") private String externalUrl; - @Value("${security.auth.openId.extraScopes:}") + @Value("${security.auth.oidc.extraScopes:}") private String extraScopes; - @Value("${security.auth.openId.rolesClaim:}") + @Value("${security.auth.oidc.rolesClaim:}") private String rolesClaim; // Default true to mirror LdapAuthConfig's SimpleGrantedAuthority upper-casing, so roles from // the two IdPs collide in sec_role by name rather than creating parallel mixed-case duplicates. - @Value("${security.auth.openId.rolesToUpperCase:true}") + @Value("${security.auth.oidc.rolesToUpperCase:true}") private boolean rolesToUpperCase; @Value("${security.auth.oauth.callback.api}") @@ -134,7 +134,7 @@ private void handleSuccess(HttpServletRequest request, log.info("OIDC: Authenticated user sub={}", login); List filteredDefaults = defaultRoles.stream().filter(s -> !s.isBlank()).toList(); - authorizationService.ensureUserExists(login, name, UserOrigin.OPENID, filteredDefaults); + authorizationService.ensureUserExists(login, name, UserOrigin.OIDC, filteredDefaults); List idpRoles = extractRoles(oidcUser.getClaims(), rolesClaim, rolesToUpperCase); if (!idpRoles.isEmpty()) { @@ -157,14 +157,14 @@ private void syncRoles(String login, List idpRoles) { try { currentOidcRoleNames = authorizationService.getOidcOriginRoles(login); } catch (Exception e) { - log.warn("OIDC: Could not fetch OPENID-origin roles for user {}: {}", login, e.getMessage()); + log.warn("OIDC: Could not fetch OIDC-origin roles for user {}: {}", login, e.getMessage()); return; } for (String roleName : idpRoles) { if (!currentOidcRoleNames.contains(roleName)) { try { - authorizationService.addUserToRole(roleName, login, UserOrigin.OPENID); + authorizationService.addUserToRole(roleName, login, UserOrigin.OIDC); log.info("OIDC: Added role '{}' to user '{}'", roleName, login); } catch (Exception e) { log.warn("OIDC: Could not add role '{}' to user '{}': {}", roleName, login, e.getMessage()); @@ -175,7 +175,7 @@ private void syncRoles(String login, List idpRoles) { for (String roleName : currentOidcRoleNames) { if (!idpRoles.contains(roleName)) { try { - authorizationService.removeUserFromRole(roleName, login, UserOrigin.OPENID); + authorizationService.removeUserFromRole(roleName, login, UserOrigin.OIDC); log.info("OIDC: Removed role '{}' from user '{}'", roleName, login); } catch (Exception e) { log.warn("OIDC: Could not remove role '{}' from user '{}': {}", roleName, login, e.getMessage()); @@ -210,7 +210,7 @@ private static String appendQueryParam(String url, String key, String value) { private String stripDiscoverySuffix(String url) { if (url == null || url.isBlank()) { - throw new IllegalStateException("security.auth.openId.url must be configured when OpenID is enabled"); + throw new IllegalStateException("security.auth.oidc.url must be configured when OIDC is enabled"); } if (url.endsWith(DISCOVERY_SUFFIX)) { return url.substring(0, url.length() - DISCOVERY_SUFFIX.length()); diff --git a/src/main/java/org/ohdsi/webapi/security/authc/UserOrigin.java b/src/main/java/org/ohdsi/webapi/security/authc/UserOrigin.java index 61138422e..1d9bd51f0 100644 --- a/src/main/java/org/ohdsi/webapi/security/authc/UserOrigin.java +++ b/src/main/java/org/ohdsi/webapi/security/authc/UserOrigin.java @@ -3,7 +3,7 @@ import org.ohdsi.webapi.security.provisioning.model.LdapProviderType; public enum UserOrigin { - SYSTEM, AD, LDAP, WINDOWS, KERBEROS, GOOGLE, FACEBOOK, DATABASE, OPENID; + SYSTEM, AD, LDAP, WINDOWS, KERBEROS, GOOGLE, FACEBOOK, DATABASE, OIDC; public static UserOrigin getFrom(LdapProviderType ldapProviderType) { switch (ldapProviderType) { diff --git a/src/main/java/org/ohdsi/webapi/security/authz/AuthorizationService.java b/src/main/java/org/ohdsi/webapi/security/authz/AuthorizationService.java index 03b0e4e83..7af85000e 100644 --- a/src/main/java/org/ohdsi/webapi/security/authz/AuthorizationService.java +++ b/src/main/java/org/ohdsi/webapi/security/authz/AuthorizationService.java @@ -180,7 +180,7 @@ public void removeUserFromRole(String roleName, String login, UserOrigin origin) public List getOidcOriginRoles(String login) { UserEntity user = userService.getUserByLogin(login).orElseThrow(); return user.getUserRoles().stream() - .filter(ur -> ur.getOrigin() == UserOrigin.OPENID) + .filter(ur -> ur.getOrigin() == UserOrigin.OIDC) .map(ur -> ur.getRole()) .filter(role -> Boolean.TRUE.equals(role.isSystemRole())) .map(role -> role.getName()) diff --git a/src/main/resources/application.yaml b/src/main/resources/application.yaml index 2d05179bc..35b5b2d79 100644 --- a/src/main/resources/application.yaml +++ b/src/main/resources/application.yaml @@ -217,7 +217,7 @@ security: apiKey: "" apiSecret: "" - openId: # OpenID + oidc: # OpenID Connect enabled: false apiSecret: "" clientId: "" From ea038005f1979b3ce59cb845834107f1605d89a0 Mon Sep 17 00:00:00 2001 From: Peter Hoffmann <954078+p-hoffmann@users.noreply.github.com> Date: Fri, 24 Apr 2026 00:56:29 +0800 Subject: [PATCH 4/8] fix tests --- docker/auth-test/docker-compose.yml | 16 ++++++++-------- docker/integration-test/docker-compose.yml | 14 +++++++------- 2 files changed, 15 insertions(+), 15 deletions(-) diff --git a/docker/auth-test/docker-compose.yml b/docker/auth-test/docker-compose.yml index 8ef6f2f62..5fe807cec 100644 --- a/docker/auth-test/docker-compose.yml +++ b/docker/auth-test/docker-compose.yml @@ -52,7 +52,7 @@ services: - SPRING_FLYWAY_SCHEMAS=webapi - SPRING_FLYWAY_PLACEHOLDERS_OHDSISCHEMA=webapi - SECURITY_PROVIDER=AtlasRegularSecurity - - SECURITY_AUTH_OPENID_ENABLED=true + - SECURITY_AUTH_OIDC_ENABLED=true - SECURITY_AUTH_DB_ENABLED=true - SECURITY_AUTH_LDAP_ENABLED=false - SECURITY_AUTH_AD_ENABLED=false @@ -62,13 +62,13 @@ services: - SECURITY_AUTH_OAUTH_GOOGLE_ENABLED=false - SECURITY_AUTH_OAUTH_FACEBOOK_ENABLED=false - SECURITY_AUTH_OAUTH_GITHUB_ENABLED=false - - SECURITY_AUTH_OPENID_CLIENTID=webapi-client - - SECURITY_AUTH_OPENID_APISECRET=webapi-secret - - SECURITY_AUTH_OPENID_URL=http://mock-oauth2:9090/default/.well-known/openid-configuration - - SECURITY_AUTH_OPENID_EXTERNALURL=http://localhost:9090/default - - SECURITY_AUTH_OPENID_LOGOUTURL=http://localhost:9090/default/endsession - - SECURITY_AUTH_OPENID_EXTRASCOPES=profile email - - SECURITY_AUTH_OPENID_ROLESCLAIM=roles + - SECURITY_AUTH_OIDC_CLIENTID=webapi-client + - SECURITY_AUTH_OIDC_APISECRET=webapi-secret + - SECURITY_AUTH_OIDC_URL=http://mock-oauth2:9090/default/.well-known/openid-configuration + - SECURITY_AUTH_OIDC_EXTERNALURL=http://localhost:9090/default + - SECURITY_AUTH_OIDC_LOGOUTURL=http://localhost:9090/default/endsession + - SECURITY_AUTH_OIDC_EXTRASCOPES=profile email + - SECURITY_AUTH_OIDC_ROLESCLAIM=roles - SECURITY_DEFAULTROLES=Atlas users - SECURITY_AUTH_OAUTH_CALLBACK_UI=http://localhost:18080/WebAPI/#/welcome - SECURITY_AUTH_OAUTH_CALLBACK_API=http://localhost:18080/WebAPI/user/oauth/callback diff --git a/docker/integration-test/docker-compose.yml b/docker/integration-test/docker-compose.yml index eabdec711..f93b31a76 100644 --- a/docker/integration-test/docker-compose.yml +++ b/docker/integration-test/docker-compose.yml @@ -79,7 +79,7 @@ services: - SPRING_FLYWAY_SCHEMAS=webapi - SPRING_FLYWAY_PLACEHOLDERS_OHDSISCHEMA=webapi - SECURITY_PROVIDER=AtlasRegularSecurity - - SECURITY_AUTH_OPENID_ENABLED=true + - SECURITY_AUTH_OIDC_ENABLED=true - SECURITY_AUTH_DB_ENABLED=true - SECURITY_AUTH_LDAP_ENABLED=false - SECURITY_AUTH_AD_ENABLED=false @@ -89,12 +89,12 @@ services: - SECURITY_AUTH_OAUTH_GOOGLE_ENABLED=false - SECURITY_AUTH_OAUTH_FACEBOOK_ENABLED=false - SECURITY_AUTH_OAUTH_GITHUB_ENABLED=false - - SECURITY_AUTH_OPENID_CLIENTID=webapi-client - - SECURITY_AUTH_OPENID_APISECRET=${OIDC_CLIENT_SECRET:-webapi-secret} - - SECURITY_AUTH_OPENID_URL=http://mock-oauth2:9090/default/.well-known/openid-configuration - - SECURITY_AUTH_OPENID_EXTERNALURL=http://localhost:9090/default - - SECURITY_AUTH_OPENID_LOGOUTURL=http://localhost:9090/default/endsession - - SECURITY_AUTH_OPENID_EXTRASCOPES=profile email + - SECURITY_AUTH_OIDC_CLIENTID=webapi-client + - SECURITY_AUTH_OIDC_APISECRET=${OIDC_CLIENT_SECRET:-webapi-secret} + - SECURITY_AUTH_OIDC_URL=http://mock-oauth2:9090/default/.well-known/openid-configuration + - SECURITY_AUTH_OIDC_EXTERNALURL=http://localhost:9090/default + - SECURITY_AUTH_OIDC_LOGOUTURL=http://localhost:9090/default/endsession + - SECURITY_AUTH_OIDC_EXTRASCOPES=profile email - SECURITY_AUTH_OAUTH_CALLBACK_UI=http://localhost:18080/WebAPI/#/welcome - SECURITY_AUTH_OAUTH_CALLBACK_API=http://localhost:18080/WebAPI/user/oauth/callback - SECURITY_AUTH_OAUTH_CALLBACK_URLRESOLVER=query From 72ff81a140631ea3a800ecf66cc4c297a6ba6ab7 Mon Sep 17 00:00:00 2001 From: Peter Hoffmann <954078+p-hoffmann@users.noreply.github.com> Date: Fri, 24 Apr 2026 01:07:10 +0800 Subject: [PATCH 5/8] fix --- .../ohdsi/webapi/security/authc/OidcAuthConfig.java | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/src/main/java/org/ohdsi/webapi/security/authc/OidcAuthConfig.java b/src/main/java/org/ohdsi/webapi/security/authc/OidcAuthConfig.java index 9dc6b7142..c1d72e2fa 100644 --- a/src/main/java/org/ohdsi/webapi/security/authc/OidcAuthConfig.java +++ b/src/main/java/org/ohdsi/webapi/security/authc/OidcAuthConfig.java @@ -60,8 +60,6 @@ public class OidcAuthConfig { @Value("${security.auth.oidc.rolesClaim:}") private String rolesClaim; - // Default true to mirror LdapAuthConfig's SimpleGrantedAuthority upper-casing, so roles from - // the two IdPs collide in sec_role by name rather than creating parallel mixed-case duplicates. @Value("${security.auth.oidc.rolesToUpperCase:true}") private boolean rolesToUpperCase; @@ -126,8 +124,7 @@ private void handleSuccess(HttpServletRequest request, HttpServletResponse response, Authentication authentication) throws IOException { OidcUser oidcUser = (OidcUser) authentication.getPrincipal(); - // Lowercase to match LoginService.mintSession which normalizes via Authentication.getName().toLowerCase(); - // a mixed-case sub would otherwise create the DB row as-is while the JWT encodes the lowercased form. + // Lowercase so the DB row login matches mintSession's JWT subject (which lowercases via getName()). String login = oidcUser.getSubject().toLowerCase(); String name = firstNonBlank(oidcUser.getFullName(), oidcUser.getEmail(), login); @@ -204,8 +201,12 @@ private static String joinPath(String base, String segment) { } private static String appendQueryParam(String url, String key, String value) { - String separator = url.contains("?") ? "&" : "?"; - return url + separator + key + "=" + value; + // Insert before any URL fragment so SPA hash-route callbacks keep the param queryable. + int fragmentIdx = url.indexOf('#'); + String base = fragmentIdx >= 0 ? url.substring(0, fragmentIdx) : url; + String fragment = fragmentIdx >= 0 ? url.substring(fragmentIdx) : ""; + String separator = base.contains("?") ? "&" : "?"; + return base + separator + key + "=" + value + fragment; } private String stripDiscoverySuffix(String url) { From 9155f9bb27c7dd3ead66fa5dc706470c2bcf0a43 Mon Sep 17 00:00:00 2001 From: Peter Hoffmann <954078+p-hoffmann@users.noreply.github.com> Date: Fri, 24 Apr 2026 01:48:20 +0800 Subject: [PATCH 6/8] fix --- .../postman/auth-tests.postman_collection.json | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/docker/auth-test/postman/auth-tests.postman_collection.json b/docker/auth-test/postman/auth-tests.postman_collection.json index c8419e78f..23b2bf38f 100644 --- a/docker/auth-test/postman/auth-tests.postman_collection.json +++ b/docker/auth-test/postman/auth-tests.postman_collection.json @@ -208,8 +208,8 @@ "", "// Extract the state parameter and full auth URL for subsequent requests", "if (location) {", - " const url = new URL(location);", - " const state = url.searchParams.get('state');", + " const stateMatch = location.match(/[?&#]state=([^&#]+)/);", + " const state = stateMatch ? stateMatch[1] : null;", " pm.collectionVariables.set('oidc_state', state);", " // The mock-oauth2-server's interactive login is at the authorization endpoint", " // Newman needs the internal Docker URL, so replace localhost with mock-oauth2", @@ -259,9 +259,10 @@ "", "// Extract code and state from redirect", "if (location) {", - " const url = new URL(location);", - " const code = url.searchParams.get('code');", - " const state = url.searchParams.get('state');", + " const codeMatch = location.match(/[?&#]code=([^&#]+)/);", + " const stateMatch = location.match(/[?&#]state=([^&#]+)/);", + " const code = codeMatch ? codeMatch[1] : null;", + " const state = stateMatch ? stateMatch[1] : null;", " pm.collectionVariables.set('oidc_auth_code', code);", " // State should match what we sent", " if (state) {", @@ -320,8 +321,8 @@ "", "// Extract JWT from redirect URL", "if (location) {", - " const url = new URL(location);", - " const token = url.searchParams.get('token');", + " const tokenMatch = location.match(/[?&#]token=([^&#]+)/);", + " const token = tokenMatch ? tokenMatch[1] : null;", " if (token) {", " pm.collectionVariables.set('oidc_jwt_token', token);", " pm.test('JWT token is well-formed', function() {", From 7100536c09b932da084f662b3960497650dd2f6c Mon Sep 17 00:00:00 2001 From: Peter Hoffmann <954078+p-hoffmann@users.noreply.github.com> Date: Sat, 25 Apr 2026 15:22:12 +0800 Subject: [PATCH 7/8] fix --- .../webapi/security/authc/OidcAuthConfig.java | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/src/main/java/org/ohdsi/webapi/security/authc/OidcAuthConfig.java b/src/main/java/org/ohdsi/webapi/security/authc/OidcAuthConfig.java index c1d72e2fa..807f133b2 100644 --- a/src/main/java/org/ohdsi/webapi/security/authc/OidcAuthConfig.java +++ b/src/main/java/org/ohdsi/webapi/security/authc/OidcAuthConfig.java @@ -146,7 +146,10 @@ private void handleSuccess(HttpServletRequest request, // mintSession — not onSuccess — so onSuccess's ensureUserExists doesn't overwrite the display name. LoginService.Result result = loginService.mintSession(wrapped); - response.sendRedirect(appendQueryParam(callbackUi, "token", result.jwt())); + // Deliver the session JWT in the URL fragment so it is not transmitted to + // servers, recorded in access logs, or leaked via the Referer header + // (RFC 6750 §5.3, OAuth 2.0 Security BCP §4.3.2). + response.sendRedirect(appendFragmentParam(callbackUi, "token", result.jwt())); } private void syncRoles(String login, List idpRoles) { @@ -209,6 +212,17 @@ private static String appendQueryParam(String url, String key, String value) { return base + separator + key + "=" + value + fragment; } + private static String appendFragmentParam(String url, String key, String value) { + int fragmentIdx = url.indexOf('#'); + if (fragmentIdx < 0) { + return url + "#" + key + "=" + value; + } + String base = url.substring(0, fragmentIdx); + String fragment = url.substring(fragmentIdx + 1); + String separator = fragment.isEmpty() ? "" : "&"; + return base + "#" + fragment + separator + key + "=" + value; + } + private String stripDiscoverySuffix(String url) { if (url == null || url.isBlank()) { throw new IllegalStateException("security.auth.oidc.url must be configured when OIDC is enabled"); From fd2955f2ce0c634fc4f69c9b59f36bac5f75f521 Mon Sep 17 00:00:00 2001 From: Peter Hoffmann <954078+p-hoffmann@users.noreply.github.com> Date: Sat, 25 Apr 2026 15:25:04 +0800 Subject: [PATCH 8/8] cleanup --- .../java/org/ohdsi/webapi/security/authc/OidcAuthConfig.java | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/main/java/org/ohdsi/webapi/security/authc/OidcAuthConfig.java b/src/main/java/org/ohdsi/webapi/security/authc/OidcAuthConfig.java index 807f133b2..83eacfbe4 100644 --- a/src/main/java/org/ohdsi/webapi/security/authc/OidcAuthConfig.java +++ b/src/main/java/org/ohdsi/webapi/security/authc/OidcAuthConfig.java @@ -146,9 +146,6 @@ private void handleSuccess(HttpServletRequest request, // mintSession — not onSuccess — so onSuccess's ensureUserExists doesn't overwrite the display name. LoginService.Result result = loginService.mintSession(wrapped); - // Deliver the session JWT in the URL fragment so it is not transmitted to - // servers, recorded in access logs, or leaked via the Referer header - // (RFC 6750 §5.3, OAuth 2.0 Security BCP §4.3.2). response.sendRedirect(appendFragmentParam(callbackUi, "token", result.jwt())); }