Move valid provider issuer check to OidcBackChannelLogoutTokenValidator constructor

chao.wang · chao.wang · commit d66fa09a9cca · 2024-09-19T15:53:09.000+08:00
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
@@ -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/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
@@ -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/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
@@ -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()

Original file line number	Diff line number	Diff line change
`@@ -603,9 +603,8 @@ String idToken(String sessionId) {`
`603`	`603`	`}`
`604`	`604`
`605`	`605`	`private String getIssuerUri() {`
`606`		`- String issuerUri = registration.getProviderDetails().getIssuerUri();`
`607`		`- if (StringUtils.hasText(issuerUri)) {`
`608`		`- return issuerUri;`
	`606`	`+ if (this.web == null) {`
	`607`	`+ return TestClientRegistrations.clientRegistration().build().getProviderDetails().getIssuerUri();`
`609`	`608`	`}`
`610`	`609`	`return this.web.url("/").toString();`
`611`	`610`	`}`
`@@ -743,7 +742,7 @@ private MockResponse toMockResponse(FluxExchangeResult<String> result) {`
`743`	`742`	`}`
`744`	`743`
`745`	`744`	`@Test`
`746`		`- void logoutWhenProviderIssuerMissingThenBadRequest() {`
	`745`	`+ void logoutWhenProviderIssuerMissingThen5xxServerError() {`
`747`	`746`	`this.spring.register(WebServerConfig.class, OidcProviderConfig.class, ProviderIssuerMissingConfig.class).autowire();`
`748`	`747`	`String registrationId = this.clientRegistration.getRegistrationId();`
`749`	`748`	`String session = login();`
`@@ -761,7 +760,7 @@ void logoutWhenProviderIssuerMissingThenBadRequest() {`
`761`	`760`	`.body(BodyInserters.fromFormData("logout_token", logoutToken))`
`762`	`761`	`.exchange()`
`763`	`762`	`.expectStatus()`
`764`		`- .isBadRequest();`
	`763`	`+ .is5xxServerError();`
`765`	`764`	`this.test.mutateWith(session(session)).get().uri("/token/logout").exchange().expectStatus().isOk();`
`766`	`765`	`}`
`767`	`766`
`@@ -774,7 +773,7 @@ static class ProviderIssuerMissingRegistrationConfig {`
`774`	`773`	`@Bean`
`775`	`774`	`ClientRegistration clientRegistration() {`
`776`	`775`	`if (this.web == null) {`
`777`		`- return TestClientRegistrations.clientRegistration().build();`
	`776`	`+ return TestClientRegistrations.clientRegistration().issuerUri(null).build();`
`778`	`777`	`}`
`779`	`778`	`String issuer = this.web.url("/").toString();`
`780`	`779`	`return TestClientRegistrations.clientRegistration()`