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

Saml2RelyingPartyAutoConfiguration ignores sign-request when metadata-url is used #33747

Closed
jzheaux opened this issue Jan 10, 2023 · 2 comments
Closed
Assignees
Labels
type: bug A general bug
Milestone

Comments

@jzheaux
Copy link
Contributor

jzheaux commented Jan 10, 2023

Related to spring-projects/spring-security#11818

The order of precedence for security.relyingparty.{id}.assertingparty.signlesignon.sign-request should be like this:

  1. Use whatever the application declares sign-request to be
  2. Use what comes back from the metadata-url query
  3. Otherwise, default to true

But it is currently like this:

  1. Use what comes back from the metadata-url query
  2. Use whatever the application declares sign-request to be
  3. Otherwise, default to true
@jzheaux jzheaux changed the title SamlRelyingPartyAutoConfiguration ignores sign-request when metadata-url is used Saml2RelyingPartyAutoConfiguration ignores sign-request when metadata-url is used Jan 10, 2023
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jan 10, 2023
@philwebb
Copy link
Member

@jzheaux Do we need to change this when check?. Do you consider this a bug or something we should only change in 3.1. I wonder if it can break existing applications if we change things now?

@philwebb philwebb added the status: waiting-for-internal-feedback An issue that needs input from a member or another Spring Team label Jan 11, 2023
@jzheaux
Copy link
Contributor Author

jzheaux commented Feb 7, 2023

The behavior that this is causing it with configurations like this:

metadata-url: ...
sign-request: ...

The sign-request property is getting ignored. I think this would be surprising and would consider it a bug. IOW, it seems clear to me that if a property is specified, it would be expected for that property to get applied. That said, there is a workaround, which is to declare a RelyingPartyRegistrationRepository bean.

Do we need to change this when check?

Possibly. IIRC, the difficulty was it being able to tell the difference between whether the signRequest property in Saml2RelyingPartyProperties is set to true or is simply unset. It seems to me that this property would need to be a Boolean so that it can be null and it can be clear that the application left it unset.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug A general bug
Projects
None yet
Development

No branches or pull requests

4 participants