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

OpenSaml4AuthenticationProvider should include secondary statusCode messages on error #11725

Closed
kha1989led opened this issue Aug 18, 2022 · 5 comments · Fixed by #14743
Closed
Assignees
Labels
in: saml2 An issue in SAML2 modules status: duplicate A duplicate of another issue type: enhancement A general enhancement

Comments

@kha1989led
Copy link

Expected Behavior

Two cases:

  1. When the isPassive flag is set to true, and the request is sent to an IdP that doesn't support passive mode, the expected statusCode is urn:oasis:names:tc:SAML:2.0:status:NoPassive:

  2. Similarly, with the isPassive flag, is set to true, and the request is sent to an IdP that supports passive mode, but the user doesn't have a session yet with the IdP, the expected statusCode is urn:oasis:names:tc:SAML:2.0:status:NoPassive

Current Behavior

In the createDefaultResponseValidator method, when the request goes through case 1, the samlResponse looks like this:
Screen Shot 2022-08-18 at 2 13 54 PM

However, the createDefaultResponseValidator uses the outer statusCode urn:oasis:names:tc:SAML:2.0:status:Responder:
Screen Shot 2022-08-18 at 2 07 23 PM

Similarly, in case 2, the samlResponse looks like this:
Screen Shot 2022-08-18 at 2 14 33 PM

and output from createDefaultResponseValidator is urn:oasis:names:tc:SAML:2.0:status:Requester:
Screen Shot 2022-08-18 at 3 06 09 PM

Context

How has this issue affected you?

I can't tell if the source of the error is NoPassive or something else to decide how to proceed with the sign-in flow.

What are you trying to accomplish?

I'm implementing a dynamic passive value for multi-tenants, and when the IdP doesn't support passive, or there's no session at the IdP, I'm detecting the error and using the redirectStrategy to send the user back to the main page. It's a public page that tries to passively log the user in if there's a session with the IdP.

What other alternatives have you considered?

None.

Are you aware of any workarounds?

No.

@kha1989led kha1989led added status: waiting-for-triage An issue we've not yet triaged type: enhancement A general enhancement labels Aug 18, 2022
@sjohnr sjohnr added the in: saml2 An issue in SAML2 modules label Aug 22, 2022
@jzheaux
Copy link
Contributor

jzheaux commented Aug 26, 2022

Thanks, @kha1989led, I think it makes sense to add this extra detail.

Given that a top-level status code may have many secondary status codes, I think we should make sure to include each as an individual error message.

Are you able to provide a PR to add this behavior?

@jzheaux jzheaux removed the status: waiting-for-triage An issue we've not yet triaged label Aug 26, 2022
@jzheaux jzheaux added this to the 5.8.x milestone Aug 26, 2022
@jzheaux jzheaux changed the title Improve returned statusCode in org.springframework.security.saml2.provider.service.authentication.OpenSaml4AuthenticationProvider when passive flag is used OpenSaml4AuthenticationProvider should include secondary statusCode messages on error Aug 26, 2022
@kha1989led
Copy link
Author

@jzheaux I can take stab at it. Is there a deadline for when I should provide the PR?

@jzheaux jzheaux added the status: ideal-for-contribution An issue that we actively are looking for someone to help us with label Dec 23, 2022
@jzheaux jzheaux removed this from the 5.8.x milestone Dec 23, 2022
@jzheaux
Copy link
Contributor

jzheaux commented Dec 23, 2022

Hi, @kha1989led, if you are still available, a PR would be most appreciated!

@Anubhav-2000
Copy link
Contributor

Hi, can i try to resolve this?

@Anubhav-2000
Copy link
Contributor

@jzheaux I have made a PR, can you check and merge please?

jzheaux added a commit that referenced this issue Mar 22, 2024
Adjusted code styling to avoid nested ifs

Closes gh-11725
@jzheaux jzheaux added status: duplicate A duplicate of another issue and removed status: ideal-for-contribution An issue that we actively are looking for someone to help us with labels Mar 22, 2024
jzheaux added a commit to jzheaux/spring-security that referenced this issue Jul 20, 2024
- Use test objects
- Ensure assertThat is checked

Issue spring-projectsgh-11725
jzheaux added a commit that referenced this issue Jul 20, 2024
- Use test objects
- Ensure assertThat is checked

Issue gh-11725
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: duplicate A duplicate of another issue type: enhancement A general enhancement
Projects
Status: No status
4 participants