Send saml logout response even when validation errors happen #14676
Conversation
spring-projects-issues
added
the
status: waiting-for-triage
|
Thanks for the PR, @1livv. This is going to take a little bit more work to maintain backward compatibility and also ensure that the APIs progress naturally. I'll get back to you soon with recommendations. |
| * 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.
Let's please reuse INVALID_REQUEST
| * 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.
Please reuse INVALID_RESPONSE
There was a problem hiding this comment.
Ended up refactoring a bit and didn't need this
| * 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.
Please reuse INVALID_REQUEST
| } | ||
| } | ||
|
|
||
| public void setLogoutRequestMatcher(RequestMatcher logoutRequestMatcher) { |
There was a problem hiding this comment.
See if you can avoid moving these, so that it's easier to identify the changes to fix the bug.
...k/security/saml2/provider/service/web/authentication/logout/Saml2LogoutResponseResolver.java
Outdated
Show resolved
Hide resolved
| @@ -371,7 +371,7 @@ public void saml2LogoutRequestWhenLowercaseEncodingAndDifferentQueryParamOrderTh | |||
| } | |||
|
|
|||
| @Test | |||
| public void saml2LogoutRequestWhenNoRegistrationThen400() throws Exception { | |||
| public void saml2LogoutRequestWhenNoRegistrationThen401() throws Exception { | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Reverted it this came from
Saml2LogoutRequestValidatorParameters parameters;
try {
parameters = this.logoutRequestResolver.resolve(request, authentication);
}
catch (Saml2AuthenticationException ex) {
this.logger.trace("Did not process logout request since failed to find requested RelyingPartyRegistration");
response.sendError(HttpServletResponse.SC_BAD_REQUEST);
return;
}
from Saml2LogoutRequestFilter
| * 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.
Let's please do something more like SERVER_ERROR.
There was a problem hiding this comment.
Chose INVALID_DESTINATION
| 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.
Please have this include the error message from the exception so that it gives the same information that this did
| verifyNoInteractions(getBean(LogoutHandler.class)); | ||
| } | ||
|
|
||
| @Test | ||
| public void saml2LogoutRequestWhenInvalidSamlRequestThen401() throws Exception { | ||
| public void saml2LogoutRequestWhenInvalidSamlRequestThen302Redirect() throws Exception { |
There was a problem hiding this comment.
Thank you, this makes sense to change this test since it is what the bug is about
jzheaux
added
status: waiting-for-feedback
|
Hi, @1livv, are you able to make the requested changes? |
|
Hey @jzheaux this slipped off my radar, i'll take a look in the next days. |
spring-projects-issues
added
status: feedback-provided
|
@1livv I'd be willing to take care of the change requests if you are ok with this |
|
Yes, that would be great thank you @jan-knoblich !! |
|
@1livv Could you Sync your repository with the main one? Otherwise I will have to create a new one. There are some changes that impact the changes in this PR |
|
@1livv and @jan-knoblich, I've rebased the PR, thanks for your patience. Would either of you like to apply the listed changes? If I don't hear from you one way or the other by next week, I'll go ahead and provide a polish commit myself. |
jzheaux
added
status: waiting-for-feedback
Signed-off-by: Liviu Gheorghe <[email protected]>
Signed-off-by: Liviu Gheorghe <[email protected]>
Signed-off-by: Liviu Gheorghe <[email protected]>
Signed-off-by: Liviu Gheorghe <[email protected]>
solves #14635