From 690e012fb199b8d54bd15b32d2560a1298208066 Mon Sep 17 00:00:00 2001 From: "chao.wang" Date: Wed, 18 Sep 2024 22:44:20 +0800 Subject: [PATCH] Improve OidcBackChannelLogoutTokenValidator error when provider issuer is missing Closes gh-15771 --- .../OidcBackChannelLogoutTokenValidator.java | 7 +- .../OidcBackChannelLogoutTokenValidator.java | 7 +- .../client/OidcLogoutConfigurerTests.java | 81 ++++++++++++++++- .../web/server/OidcLogoutSpecTests.java | 86 ++++++++++++++++++- 4 files changed, 173 insertions(+), 8 deletions(-) diff --git a/config/src/main/java/org/springframework/security/config/annotation/web/configurers/oauth2/client/OidcBackChannelLogoutTokenValidator.java b/config/src/main/java/org/springframework/security/config/annotation/web/configurers/oauth2/client/OidcBackChannelLogoutTokenValidator.java index 7b6634f933..7a65826ab9 100644 --- a/config/src/main/java/org/springframework/security/config/annotation/web/configurers/oauth2/client/OidcBackChannelLogoutTokenValidator.java +++ b/config/src/main/java/org/springframework/security/config/annotation/web/configurers/oauth2/client/OidcBackChannelLogoutTokenValidator.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2023 the original author or authors. + * Copyright 2002-2024 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -30,6 +30,7 @@ import org.springframework.security.oauth2.core.OAuth2ErrorCodes; import org.springframework.security.oauth2.core.OAuth2TokenValidator; import org.springframework.security.oauth2.core.OAuth2TokenValidatorResult; import org.springframework.security.oauth2.jwt.Jwt; +import org.springframework.util.Assert; /** * A {@link OAuth2TokenValidator} that validates OIDC Logout Token claims in conformance @@ -57,7 +58,9 @@ final class OidcBackChannelLogoutTokenValidator implements OAuth2TokenValidator< OidcBackChannelLogoutTokenValidator(ClientRegistration clientRegistration) { this.audience = clientRegistration.getClientId(); - this.issuer = clientRegistration.getProviderDetails().getIssuerUri(); + String issuer = clientRegistration.getProviderDetails().getIssuerUri(); + Assert.hasText(issuer, "Provider issuer cannot be null"); + this.issuer = issuer; } @Override diff --git a/config/src/main/java/org/springframework/security/config/web/server/OidcBackChannelLogoutTokenValidator.java b/config/src/main/java/org/springframework/security/config/web/server/OidcBackChannelLogoutTokenValidator.java index 7053689171..c1709e1ec7 100644 --- a/config/src/main/java/org/springframework/security/config/web/server/OidcBackChannelLogoutTokenValidator.java +++ b/config/src/main/java/org/springframework/security/config/web/server/OidcBackChannelLogoutTokenValidator.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2023 the original author or authors. + * Copyright 2002-2024 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -30,6 +30,7 @@ import org.springframework.security.oauth2.core.OAuth2ErrorCodes; import org.springframework.security.oauth2.core.OAuth2TokenValidator; import org.springframework.security.oauth2.core.OAuth2TokenValidatorResult; import org.springframework.security.oauth2.jwt.Jwt; +import org.springframework.util.Assert; /** * A {@link OAuth2TokenValidator} that validates OIDC Logout Token claims in conformance @@ -57,7 +58,9 @@ final class OidcBackChannelLogoutTokenValidator implements OAuth2TokenValidator< OidcBackChannelLogoutTokenValidator(ClientRegistration clientRegistration) { this.audience = clientRegistration.getClientId(); - this.issuer = clientRegistration.getProviderDetails().getIssuerUri(); + String issuer = clientRegistration.getProviderDetails().getIssuerUri(); + Assert.hasText(issuer, "Provider issuer cannot be null"); + this.issuer = issuer; } @Override diff --git a/config/src/test/java/org/springframework/security/config/annotation/web/configurers/oauth2/client/OidcLogoutConfigurerTests.java b/config/src/test/java/org/springframework/security/config/annotation/web/configurers/oauth2/client/OidcLogoutConfigurerTests.java index eccf675f3d..541942212f 100644 --- a/config/src/test/java/org/springframework/security/config/annotation/web/configurers/oauth2/client/OidcLogoutConfigurerTests.java +++ b/config/src/test/java/org/springframework/security/config/annotation/web/configurers/oauth2/client/OidcLogoutConfigurerTests.java @@ -85,12 +85,14 @@ import org.springframework.security.web.authentication.logout.LogoutHandler; import org.springframework.test.web.servlet.MockMvc; import org.springframework.test.web.servlet.MvcResult; import org.springframework.test.web.servlet.request.MockHttpServletRequestBuilder; +import org.springframework.util.StringUtils; import org.springframework.web.bind.annotation.GetMapping; import org.springframework.web.bind.annotation.PostMapping; import org.springframework.web.bind.annotation.RequestParam; import org.springframework.web.bind.annotation.RestController; import org.springframework.web.servlet.config.annotation.EnableWebMvc; +import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException; import static org.hamcrest.Matchers.containsString; import static org.mockito.ArgumentMatchers.any; import static org.mockito.BDDMockito.willThrow; @@ -448,6 +450,9 @@ public class OidcLogoutConfigurerTests { @Autowired ClientRegistration registration; + @Autowired(required = false) + MockWebServer web; + @Bean @Order(0) SecurityFilterChain authorizationServer(HttpSecurity http, ClientRegistration registration) throws Exception { @@ -484,7 +489,7 @@ public class OidcLogoutConfigurerTests { HttpSession session = request.getSession(); JwtEncoderParameters parameters = JwtEncoderParameters .from(JwtClaimsSet.builder().id("id").subject(this.username) - .issuer(this.registration.getProviderDetails().getIssuerUri()).issuedAt(Instant.now()) + .issuer(getIssuerUri()).issuedAt(Instant.now()) .expiresAt(Instant.now().plusSeconds(86400)).claim("scope", "openid").build()); String token = this.encoder.encode(parameters).getTokenValue(); return new OIDCTokens(idToken(session.getId()), new BearerAccessToken(token, 86400, new Scope("openid")), null) @@ -492,7 +497,7 @@ public class OidcLogoutConfigurerTests { } String idToken(String sessionId) { - OidcIdToken token = TestOidcIdTokens.idToken().issuer(this.registration.getProviderDetails().getIssuerUri()) + OidcIdToken token = TestOidcIdTokens.idToken().issuer(getIssuerUri()) .subject(this.username).expiresAt(Instant.now().plusSeconds(86400)) .audience(List.of(this.registration.getClientId())).nonce(this.nonce) .claim(LogoutTokenClaimNames.SID, sessionId).build(); @@ -501,6 +506,13 @@ public class OidcLogoutConfigurerTests { return this.encoder.encode(parameters).getTokenValue(); } + private String getIssuerUri() { + if (this.web == null) { + return TestClientRegistrations.clientRegistration().build().getProviderDetails().getIssuerUri(); + } + return this.web.url("/").toString(); + } + @GetMapping("/user") Map userinfo() { return Map.of("sub", this.username, "id", this.username); @@ -638,4 +650,69 @@ public class OidcLogoutConfigurerTests { } + @Test + void logoutWhenProviderIssuerMissingThenThrowIllegalArgumentException() throws Exception { + this.spring.register(WebServerConfig.class, OidcProviderConfig.class, ProviderIssuerMissingConfig.class).autowire(); + String registrationId = this.clientRegistration.getRegistrationId(); + MockHttpSession session = login(); + String logoutToken = this.mvc.perform(get("/token/logout").session(session)) + .andExpect(status().isOk()) + .andReturn() + .getResponse() + .getContentAsString(); + assertThatIllegalArgumentException().isThrownBy(() -> { + this.mvc + .perform(post(this.web.url("/logout/connect/back-channel/" + registrationId).toString()) + .param("logout_token", logoutToken)); + }); + } + + @Configuration + static class ProviderIssuerMissingRegistrationConfig { + + @Autowired(required = false) + MockWebServer web; + + @Bean + ClientRegistration clientRegistration() { + if (this.web == null) { + return TestClientRegistrations.clientRegistration().issuerUri(null).build(); + } + String issuer = this.web.url("/").toString(); + return TestClientRegistrations.clientRegistration() + .issuerUri(null) + .jwkSetUri(issuer + "jwks") + .tokenUri(issuer + "token") + .userInfoUri(issuer + "user") + .scope("openid") + .build(); + } + + @Bean + ClientRegistrationRepository clientRegistrationRepository(ClientRegistration clientRegistration) { + return new InMemoryClientRegistrationRepository(clientRegistration); + } + + } + + @Configuration + @EnableWebSecurity + @Import(ProviderIssuerMissingRegistrationConfig.class) + static class ProviderIssuerMissingConfig { + + @Bean + @Order(1) + SecurityFilterChain filters(HttpSecurity http) throws Exception { + // @formatter:off + http + .authorizeHttpRequests((authorize) -> authorize.anyRequest().authenticated()) + .oauth2Login(Customizer.withDefaults()) + .oidcLogout((oidc) -> oidc.backChannel(Customizer.withDefaults())); + // @formatter:on + + return http.build(); + } + + } + } diff --git a/config/src/test/java/org/springframework/security/config/web/server/OidcLogoutSpecTests.java b/config/src/test/java/org/springframework/security/config/web/server/OidcLogoutSpecTests.java index 9f5627db79..711481c280 100644 --- a/config/src/test/java/org/springframework/security/config/web/server/OidcLogoutSpecTests.java +++ b/config/src/test/java/org/springframework/security/config/web/server/OidcLogoutSpecTests.java @@ -86,6 +86,7 @@ import org.springframework.security.web.server.util.matcher.ServerWebExchangeMat import org.springframework.test.web.reactive.server.FluxExchangeResult; import org.springframework.test.web.reactive.server.WebTestClient; import org.springframework.test.web.reactive.server.WebTestClientConfigurer; +import org.springframework.util.StringUtils; import org.springframework.web.bind.annotation.GetMapping; import org.springframework.web.bind.annotation.PostMapping; import org.springframework.web.bind.annotation.RequestParam; @@ -537,6 +538,9 @@ public class OidcLogoutSpecTests { @Autowired ClientRegistration registration; + @Autowired(required = false) + MockWebServer web; + static ServerWebExchangeMatcher or(String... patterns) { List matchers = new ArrayList<>(); for (String pattern : patterns) { @@ -581,7 +585,7 @@ public class OidcLogoutSpecTests { Map accessToken(WebSession session) { JwtEncoderParameters parameters = JwtEncoderParameters .from(JwtClaimsSet.builder().id("id").subject(this.username) - .issuer(this.registration.getProviderDetails().getIssuerUri()).issuedAt(Instant.now()) + .issuer(getIssuerUri()).issuedAt(Instant.now()) .expiresAt(Instant.now().plusSeconds(86400)).claim("scope", "openid").build()); String token = this.encoder.encode(parameters).getTokenValue(); return new OIDCTokens(idToken(session.getId()), new BearerAccessToken(token, 86400, new Scope("openid")), null) @@ -589,7 +593,7 @@ public class OidcLogoutSpecTests { } String idToken(String sessionId) { - OidcIdToken token = TestOidcIdTokens.idToken().issuer(this.registration.getProviderDetails().getIssuerUri()) + OidcIdToken token = TestOidcIdTokens.idToken().issuer(getIssuerUri()) .subject(this.username).expiresAt(Instant.now().plusSeconds(86400)) .audience(List.of(this.registration.getClientId())).nonce(this.nonce) .claim(LogoutTokenClaimNames.SID, sessionId).build(); @@ -598,6 +602,13 @@ public class OidcLogoutSpecTests { return this.encoder.encode(parameters).getTokenValue(); } + private String getIssuerUri() { + if (this.web == null) { + return TestClientRegistrations.clientRegistration().build().getProviderDetails().getIssuerUri(); + } + return this.web.url("/").toString(); + } + @GetMapping("/user") Map userinfo() { return Map.of("sub", this.username, "id", this.username); @@ -730,4 +741,75 @@ public class OidcLogoutSpecTests { } + @Test + void logoutWhenProviderIssuerMissingThen5xxServerError() { + this.spring.register(WebServerConfig.class, OidcProviderConfig.class, ProviderIssuerMissingConfig.class).autowire(); + String registrationId = this.clientRegistration.getRegistrationId(); + String session = login(); + String logoutToken = this.test.mutateWith(session(session)) + .get() + .uri("/token/logout") + .exchange() + .expectStatus() + .isOk() + .returnResult(String.class) + .getResponseBody() + .blockFirst(); + this.test.post() + .uri(this.web.url("/logout/connect/back-channel/" + registrationId).toString()) + .body(BodyInserters.fromFormData("logout_token", logoutToken)) + .exchange() + .expectStatus() + .is5xxServerError(); + this.test.mutateWith(session(session)).get().uri("/token/logout").exchange().expectStatus().isOk(); + } + + @Configuration + static class ProviderIssuerMissingRegistrationConfig { + + @Autowired(required = false) + MockWebServer web; + + @Bean + ClientRegistration clientRegistration() { + if (this.web == null) { + return TestClientRegistrations.clientRegistration().issuerUri(null).build(); + } + String issuer = this.web.url("/").toString(); + return TestClientRegistrations.clientRegistration() + .issuerUri(null) + .jwkSetUri(issuer + "jwks") + .tokenUri(issuer + "token") + .userInfoUri(issuer + "user") + .scope("openid") + .build(); + } + + @Bean + ReactiveClientRegistrationRepository clientRegistrationRepository(ClientRegistration clientRegistration) { + return new InMemoryReactiveClientRegistrationRepository(clientRegistration); + } + + } + + @Configuration + @EnableWebFluxSecurity + @Import(ProviderIssuerMissingRegistrationConfig.class) + static class ProviderIssuerMissingConfig { + + @Bean + @Order(1) + SecurityWebFilterChain filters(ServerHttpSecurity http) throws Exception { + // @formatter:off + http + .authorizeExchange((authorize) -> authorize.anyExchange().authenticated()) + .oauth2Login(Customizer.withDefaults()) + .oidcLogout((oidc) -> oidc.backChannel(Customizer.withDefaults())); + // @formatter:on + + return http.build(); + } + + } + }