Skip to content
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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

1livv
Copy link
Contributor

@1livv 1livv commented Mar 3, 2024

solves #14635

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Mar 3, 2024
@jzheaux jzheaux self-assigned this Mar 6, 2024
@jzheaux
Copy link
Contributor

jzheaux commented Mar 6, 2024

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.

Copy link
Contributor

@jzheaux jzheaux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @1livv! This is very helpful. I've left my feedback inline.

* The saml logout request failed validation
* @since 6.3
*/
public static final String INVALID_LOGOUT_REQUEST = "invalid_logout_request";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please reuse INVALID_RESPONSE

* The saml response or logout request was delivered via an invalid binding
* @since 6.3
*/
public static final String INVALID_BINDING = "invalid_binding";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please reuse INVALID_REQUEST

}
}

public void setLogoutRequestMatcher(RequestMatcher logoutRequestMatcher) {
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

* processed
* @return a signed and serialized SAML 2.0 Logout Response
*/
Saml2LogoutResponse resolve(HttpServletRequest request, Authentication authentication,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please make this a default method so as to remain backward compatible. Given your implementation, I think returning null as a default would work.

@@ -371,7 +371,7 @@ public void saml2LogoutRequestWhenLowercaseEncodingAndDifferentQueryParamOrderTh
}

@Test
public void saml2LogoutRequestWhenNoRegistrationThen400() throws Exception {
public void saml2LogoutRequestWhenNoRegistrationThen401() throws Exception {
Copy link
Contributor

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.

* 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";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's please do something more like SERVER_ERROR.

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);
Copy link
Contributor

Choose a reason for hiding this comment

The 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

verifyNoInteractions(getBean(LogoutHandler.class));
}

@Test
public void saml2LogoutRequestWhenInvalidSamlRequestThen401() throws Exception {
public void saml2LogoutRequestWhenInvalidSamlRequestThen302Redirect() throws Exception {
Copy link
Contributor

Choose a reason for hiding this comment

The 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

@jzheaux jzheaux added status: waiting-for-feedback We need additional information before we can continue type: enhancement A general enhancement in: saml2 An issue in SAML2 modules and removed status: waiting-for-triage An issue we've not yet triaged labels Apr 6, 2024
@jzheaux
Copy link
Contributor

jzheaux commented Apr 19, 2024

Hi, @1livv, are you able to make the requested changes?

@1livv
Copy link
Contributor Author

1livv commented Apr 19, 2024

Hey @jzheaux this slipped off my radar, i'll take a look in the next days.
Sorry for the delay

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Apr 19, 2024
@jan-knoblich
Copy link

@1livv I'd be willing to take care of the change requests if you are ok with this

@1livv
Copy link
Contributor Author

1livv commented Aug 19, 2024

Yes, that would be great thank you @jan-knoblich !!

@jan-knoblich
Copy link

@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
This explains how to sync: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: saml2 An issue in SAML2 modules status: feedback-provided Feedback has been provided type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants