-
Notifications
You must be signed in to change notification settings - Fork 5.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Send saml logout response even when validation errors happen #14676
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,5 @@ | ||
/* | ||
* Copyright 2002-2022 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. | ||
|
@@ -371,7 +371,7 @@ public void saml2LogoutRequestWhenLowercaseEncodingAndDifferentQueryParamOrderTh | |
} | ||
|
||
@Test | ||
public void saml2LogoutRequestWhenNoRegistrationThen400() throws Exception { | ||
public void saml2LogoutRequestWhenNoRegistrationThen401() throws Exception { | ||
this.spring.register(Saml2LogoutDefaultsConfig.class).autowire(); | ||
DefaultSaml2AuthenticatedPrincipal principal = new DefaultSaml2AuthenticatedPrincipal("user", | ||
Collections.emptyMap()); | ||
|
@@ -384,19 +384,19 @@ public void saml2LogoutRequestWhenNoRegistrationThen400() throws Exception { | |
.param("SigAlg", this.apLogoutRequestSigAlg) | ||
.param("Signature", this.apLogoutRequestSignature) | ||
.with(authentication(user))) | ||
.andExpect(status().isBadRequest()); | ||
.andExpect(status().isUnauthorized()); | ||
verifyNoInteractions(getBean(LogoutHandler.class)); | ||
} | ||
|
||
@Test | ||
public void saml2LogoutRequestWhenInvalidSamlRequestThen401() throws Exception { | ||
public void saml2LogoutRequestWhenInvalidSamlRequestThen302Redirect() throws Exception { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you, this makes sense to change this test since it is what the bug is about |
||
this.spring.register(Saml2LogoutDefaultsConfig.class).autowire(); | ||
this.mvc | ||
.perform(get("/logout/saml2/slo").param("SAMLRequest", this.apLogoutRequest) | ||
.param("RelayState", this.apLogoutRequestRelayState) | ||
.param("SigAlg", this.apLogoutRequestSigAlg) | ||
.with(authentication(this.user))) | ||
.andExpect(status().isUnauthorized()); | ||
.andExpect(status().isFound()); | ||
verifyNoInteractions(getBean(LogoutHandler.class)); | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,5 @@ | ||
/* | ||
* Copyright 2002-2022 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. | ||
|
@@ -130,6 +130,30 @@ public final class Saml2ErrorCodes { | |
*/ | ||
public static final String INVALID_IN_RESPONSE_TO = "invalid_in_response_to"; | ||
|
||
/** | ||
* The RP registration does not have configured a logout request endpoint | ||
* @since 6.3 | ||
*/ | ||
public static final String MISSING_LOGOUT_REQUEST_ENDPOINT = "missing_logout_request_endpoint"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's please do something more like |
||
|
||
/** | ||
* The saml response or logout request was delivered via an invalid binding | ||
* @since 6.3 | ||
*/ | ||
public static final String INVALID_BINDING = "invalid_binding"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please reuse |
||
|
||
/** | ||
* The saml logout request failed validation | ||
* @since 6.3 | ||
*/ | ||
public static final String INVALID_LOGOUT_REQUEST = "invalid_logout_request"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's please reuse |
||
|
||
/** | ||
* The saml logout response could not be generated | ||
* @since 6.3 | ||
*/ | ||
public static final String FAILED_TO_GENERATE_LOGOUT_RESPONSE = "failed_to_generate_logout_response"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please reuse |
||
|
||
private Saml2ErrorCodes() { | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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. | ||
|
@@ -113,47 +113,84 @@ public Saml2LogoutRequestFilter(RelyingPartyRegistrationResolver relyingPartyReg | |
protected void doFilterInternal(HttpServletRequest request, HttpServletResponse response, FilterChain chain) | ||
throws ServletException, IOException { | ||
Authentication authentication = this.securityContextHolderStrategy.getContext().getAuthentication(); | ||
Saml2LogoutRequestValidatorParameters parameters; | ||
try { | ||
parameters = this.logoutRequestResolver.resolve(request, authentication); | ||
Saml2LogoutRequestValidatorParameters parameters = this.logoutRequestResolver.resolve(request, | ||
authentication); | ||
if (parameters == null) { | ||
chain.doFilter(request, response); | ||
return; | ||
} | ||
|
||
Saml2LogoutResponse logoutResponse = processLogoutRequest(request, response, authentication, parameters); | ||
sendLogoutResponse(request, response, logoutResponse); | ||
} | ||
catch (Saml2AuthenticationException ex) { | ||
this.logger.trace("Did not process logout request since failed to find requested RelyingPartyRegistration"); | ||
response.sendError(HttpServletResponse.SC_BAD_REQUEST); | ||
return; | ||
} | ||
if (parameters == null) { | ||
chain.doFilter(request, response); | ||
return; | ||
Saml2LogoutResponse errorLogoutResponse = this.logoutResponseResolver.resolve(request, authentication, ex); | ||
if (errorLogoutResponse == null) { | ||
this.logger.trace("Returning error since no error logout response could be generated", ex); | ||
response.sendError(HttpServletResponse.SC_UNAUTHORIZED); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please have this include the error message from the exception so that it gives the same information that this did |
||
return; | ||
} | ||
|
||
sendLogoutResponse(request, response, errorLogoutResponse); | ||
} | ||
} | ||
|
||
public void setLogoutRequestMatcher(RequestMatcher logoutRequestMatcher) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See if you can avoid moving these, so that it's easier to identify the changes to fix the bug. |
||
Assert.notNull(logoutRequestMatcher, "logoutRequestMatcher cannot be null"); | ||
Assert.isInstanceOf(Saml2AssertingPartyLogoutRequestResolver.class, this.logoutRequestResolver, | ||
"saml2LogoutRequestResolver and logoutRequestMatcher cannot both be set. Please set the request matcher in the saml2LogoutRequestResolver itself."); | ||
((Saml2AssertingPartyLogoutRequestResolver) this.logoutRequestResolver) | ||
.setLogoutRequestMatcher(logoutRequestMatcher); | ||
} | ||
|
||
/** | ||
* Sets the {@link SecurityContextHolderStrategy} to use. The default action is to use | ||
* the {@link SecurityContextHolderStrategy} stored in {@link SecurityContextHolder}. | ||
* | ||
* @since 5.8 | ||
*/ | ||
public void setSecurityContextHolderStrategy(SecurityContextHolderStrategy securityContextHolderStrategy) { | ||
Assert.notNull(securityContextHolderStrategy, "securityContextHolderStrategy cannot be null"); | ||
this.securityContextHolderStrategy = securityContextHolderStrategy; | ||
} | ||
|
||
private Saml2LogoutResponse processLogoutRequest(HttpServletRequest request, HttpServletResponse response, | ||
Authentication authentication, Saml2LogoutRequestValidatorParameters parameters) { | ||
RelyingPartyRegistration registration = parameters.getRelyingPartyRegistration(); | ||
if (registration.getSingleLogoutServiceLocation() == null) { | ||
this.logger.trace( | ||
"Did not process logout request since RelyingPartyRegistration has not been configured with a logout request endpoint"); | ||
response.sendError(HttpServletResponse.SC_UNAUTHORIZED); | ||
return; | ||
throw new Saml2AuthenticationException(new Saml2Error(Saml2ErrorCodes.MISSING_LOGOUT_REQUEST_ENDPOINT, | ||
"RelyingPartyRegistration has not been configured with a logout request endpoint")); | ||
} | ||
|
||
Saml2MessageBinding saml2MessageBinding = Saml2MessageBindingUtils.resolveBinding(request); | ||
if (!registration.getSingleLogoutServiceBindings().contains(saml2MessageBinding)) { | ||
this.logger.trace("Did not process logout request since used incorrect binding"); | ||
response.sendError(HttpServletResponse.SC_UNAUTHORIZED); | ||
return; | ||
throw new Saml2AuthenticationException( | ||
new Saml2Error(Saml2ErrorCodes.INVALID_BINDING, "Logout request used invalid binding")); | ||
} | ||
|
||
Saml2LogoutValidatorResult result = this.logoutRequestValidator.validate(parameters); | ||
if (result.hasErrors()) { | ||
response.sendError(HttpServletResponse.SC_UNAUTHORIZED, result.getErrors().iterator().next().toString()); | ||
this.logger.debug(LogMessage.format("Failed to validate LogoutRequest: %s", result.getErrors())); | ||
return; | ||
throw new Saml2AuthenticationException( | ||
new Saml2Error(Saml2ErrorCodes.INVALID_LOGOUT_REQUEST, "Failed to validate the logout request")); | ||
} | ||
|
||
this.handler.logout(request, response, authentication); | ||
Saml2LogoutResponse logoutResponse = this.logoutResponseResolver.resolve(request, authentication); | ||
if (logoutResponse == null) { | ||
this.logger.trace("Returning 401 since no logout response generated"); | ||
response.setStatus(HttpServletResponse.SC_UNAUTHORIZED); | ||
return; | ||
this.logger.trace("Returning error since no logout response generated"); | ||
throw new Saml2AuthenticationException(new Saml2Error(Saml2ErrorCodes.FAILED_TO_GENERATE_LOGOUT_RESPONSE, | ||
"Could not generated logout response")); | ||
} | ||
return logoutResponse; | ||
} | ||
|
||
private void sendLogoutResponse(HttpServletRequest request, HttpServletResponse response, | ||
Saml2LogoutResponse logoutResponse) throws IOException { | ||
if (logoutResponse.getBinding() == Saml2MessageBinding.REDIRECT) { | ||
doRedirect(request, response, logoutResponse); | ||
} | ||
|
@@ -162,25 +199,6 @@ protected void doFilterInternal(HttpServletRequest request, HttpServletResponse | |
} | ||
} | ||
|
||
public void setLogoutRequestMatcher(RequestMatcher logoutRequestMatcher) { | ||
Assert.notNull(logoutRequestMatcher, "logoutRequestMatcher cannot be null"); | ||
Assert.isInstanceOf(Saml2AssertingPartyLogoutRequestResolver.class, this.logoutRequestResolver, | ||
"saml2LogoutRequestResolver and logoutRequestMatcher cannot both be set. Please set the request matcher in the saml2LogoutRequestResolver itself."); | ||
((Saml2AssertingPartyLogoutRequestResolver) this.logoutRequestResolver) | ||
.setLogoutRequestMatcher(logoutRequestMatcher); | ||
} | ||
|
||
/** | ||
* Sets the {@link SecurityContextHolderStrategy} to use. The default action is to use | ||
* the {@link SecurityContextHolderStrategy} stored in {@link SecurityContextHolder}. | ||
* | ||
* @since 5.8 | ||
*/ | ||
public void setSecurityContextHolderStrategy(SecurityContextHolderStrategy securityContextHolderStrategy) { | ||
Assert.notNull(securityContextHolderStrategy, "securityContextHolderStrategy cannot be null"); | ||
this.securityContextHolderStrategy = securityContextHolderStrategy; | ||
} | ||
|
||
private void doRedirect(HttpServletRequest request, HttpServletResponse response, | ||
Saml2LogoutResponse logoutResponse) throws IOException { | ||
String location = logoutResponse.getResponseLocation(); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,5 @@ | ||
/* | ||
* Copyright 2002-2021 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. | ||
|
@@ -19,6 +19,7 @@ | |
import jakarta.servlet.http.HttpServletRequest; | ||
|
||
import org.springframework.security.core.Authentication; | ||
import org.springframework.security.saml2.provider.service.authentication.Saml2AuthenticationException; | ||
import org.springframework.security.saml2.provider.service.authentication.logout.Saml2LogoutResponse; | ||
import org.springframework.security.saml2.provider.service.registration.RelyingPartyRegistration; | ||
|
||
|
@@ -44,4 +45,15 @@ public interface Saml2LogoutResponseResolver { | |
*/ | ||
Saml2LogoutResponse resolve(HttpServletRequest request, Authentication authentication); | ||
|
||
/** | ||
* Prepare to create, sign, and serialize a SAML 2.0 Error Logout Response. | ||
* @param request the HTTP request | ||
* @param authentication the current user | ||
* @param authenticationException the thrown exception when the logout request was | ||
* processed | ||
* @return a signed and serialized SAML 2.0 Logout Response | ||
*/ | ||
Saml2LogoutResponse resolve(HttpServletRequest request, Authentication authentication, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please make this a |
||
Saml2AuthenticationException authenticationException); | ||
|
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not understanding the logic of this change. Can you please elaborate? Ideally, we leave tests as they are unless it is a bug.