Move valid provider issuer check to OidcBackChannelLogoutTokenValidat…

…or constructor
spring-projects · Sep 19, 2024 · d66fa09 · d66fa09
1 parent 5161d96
commit d66fa09
Show file tree

Hide file tree

Showing 4 changed files with 23 additions and 26 deletions.
diff --git a/.../config/annotation/web/configurers/oauth2/client/OidcBackChannelLogoutTokenValidator.java b/.../config/annotation/web/configurers/oauth2/client/OidcBackChannelLogoutTokenValidator.java
@@ -30,7 +30,7 @@
 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.StringUtils;
+import org.springframework.util.Assert;
 
 /**
  * A {@link OAuth2TokenValidator} that validates OIDC Logout Token claims in conformance
@@ -58,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
@@ -78,9 +80,6 @@ else if (events.get(BACK_CHANNEL_LOGOUT_EVENT) == null) {
 		if (issuer == null) {
 			errors.add(invalidLogoutToken("iss claim must not be null"));
 		}
-		else if (!StringUtils.hasText(this.issuer)) {
-			errors.add(invalidLogoutToken("Provider issuer must not be null"));
-		}
 		else if (!this.issuer.equals(issuer)) {
 			errors.add(invalidLogoutToken(
 					"iss claim value must match `ClientRegistration#getProviderDetails#getIssuerUri`"));

diff --git a/...a/org/springframework/security/config/web/server/OidcBackChannelLogoutTokenValidator.java b/...a/org/springframework/security/config/web/server/OidcBackChannelLogoutTokenValidator.java
@@ -30,7 +30,7 @@
 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.StringUtils;
+import org.springframework.util.Assert;
 
 /**
  * A {@link OAuth2TokenValidator} that validates OIDC Logout Token claims in conformance
@@ -58,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
@@ -78,9 +80,6 @@ else if (events.get(BACK_CHANNEL_LOGOUT_EVENT) == null) {
 		if (issuer == null) {
 			errors.add(invalidLogoutToken("iss claim must not be null"));
 		}
-		else if (!StringUtils.hasText(this.issuer)) {
-			errors.add(invalidLogoutToken("Provider issuer must not be null"));
-		}
 		else if (!this.issuer.equals(issuer)) {
 			errors.add(invalidLogoutToken(
 					"iss claim value must match `ClientRegistration#getProviderDetails#getIssuerUri`"));

diff --git a/...k/security/config/annotation/web/configurers/oauth2/client/OidcLogoutConfigurerTests.java b/...k/security/config/annotation/web/configurers/oauth2/client/OidcLogoutConfigurerTests.java
@@ -92,6 +92,7 @@
 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;
@@ -506,9 +507,8 @@ String idToken(String sessionId) {
 		}
 
 		private String getIssuerUri() {
-			String issuerUri = registration.getProviderDetails().getIssuerUri();
-			if (StringUtils.hasText(issuerUri)) {
-				return issuerUri;
+			if (this.web == null) {
+				return TestClientRegistrations.clientRegistration().build().getProviderDetails().getIssuerUri();
 			}
 			return this.web.url("/").toString();
 		}
@@ -651,7 +651,7 @@ private String getContentAsString(MockHttpServletResponse response) {
 	}
 
 	@Test
-	void logoutWhenProviderIssuerMissingThenBadRequest() throws Exception {
+	void logoutWhenProviderIssuerMissingThenThrowIllegalArgumentException() throws Exception {
 		this.spring.register(WebServerConfig.class, OidcProviderConfig.class, ProviderIssuerMissingConfig.class).autowire();
 		String registrationId = this.clientRegistration.getRegistrationId();
 		MockHttpSession session = login();
@@ -660,11 +660,11 @@ void logoutWhenProviderIssuerMissingThenBadRequest() throws Exception {
 				.andReturn()
 				.getResponse()
 				.getContentAsString();
-		this.mvc
-				.perform(post(this.web.url("/logout/connect/back-channel/" + registrationId).toString())
-						.param("logout_token", logoutToken))
-				.andExpect(status().isBadRequest());
-		this.mvc.perform(get("/token/logout").session(session)).andExpect(status().isOk());
+		assertThatIllegalArgumentException().isThrownBy(() -> {
+			this.mvc
+					.perform(post(this.web.url("/logout/connect/back-channel/" + registrationId).toString())
+							.param("logout_token", logoutToken));
+		});
 	}
 
 	@Configuration
@@ -676,7 +676,7 @@ static class ProviderIssuerMissingRegistrationConfig {
 		@Bean
 		ClientRegistration clientRegistration() {
 			if (this.web == null) {
-				return TestClientRegistrations.clientRegistration().build();
+				return TestClientRegistrations.clientRegistration().issuerUri(null).build();
 			}
 			String issuer = this.web.url("/").toString();
 			return TestClientRegistrations.clientRegistration()

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
@@ -603,9 +603,8 @@ String idToken(String sessionId) {
 		}
 
 		private String getIssuerUri() {
-			String issuerUri = registration.getProviderDetails().getIssuerUri();
-			if (StringUtils.hasText(issuerUri)) {
-				return issuerUri;
+			if (this.web == null) {
+				return TestClientRegistrations.clientRegistration().build().getProviderDetails().getIssuerUri();
 			}
 			return this.web.url("/").toString();
 		}
@@ -743,7 +742,7 @@ private MockResponse toMockResponse(FluxExchangeResult<String> result) {
 	}
 
 	@Test
-	void logoutWhenProviderIssuerMissingThenBadRequest() {
+	void logoutWhenProviderIssuerMissingThen5xxServerError() {
 		this.spring.register(WebServerConfig.class, OidcProviderConfig.class, ProviderIssuerMissingConfig.class).autowire();
 		String registrationId = this.clientRegistration.getRegistrationId();
 		String session = login();
@@ -761,7 +760,7 @@ void logoutWhenProviderIssuerMissingThenBadRequest() {
 				.body(BodyInserters.fromFormData("logout_token", logoutToken))
 				.exchange()
 				.expectStatus()
-				.isBadRequest();
+				.is5xxServerError();
 		this.test.mutateWith(session(session)).get().uri("/token/logout").exchange().expectStatus().isOk();
 	}
 
@@ -774,7 +773,7 @@ static class ProviderIssuerMissingRegistrationConfig {
 		@Bean
 		ClientRegistration clientRegistration() {
 			if (this.web == null) {
-				return TestClientRegistrations.clientRegistration().build();
+				return TestClientRegistrations.clientRegistration().issuerUri(null).build();
 			}
 			String issuer = this.web.url("/").toString();
 			return TestClientRegistrations.clientRegistration()