Skip to content

Saml2WebSsoAuthenticationFilter adds authentication details #10306

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

Merged
merged 1 commit into from
Sep 27, 2021

Conversation

Kehrlann
Copy link
Contributor

Closes gh-7722.

@Kehrlann
Copy link
Contributor Author

Kehrlann commented Sep 21, 2021

Hi there!

We've been needing authentication details in our product, specifically WebAuthenticationDetails. I saw gh-7722 and decided to submit this.

Notes:

  1. I added a test in Saml2WebSsoAuthenticationFilterTests + OpenSaml4AuthenticationProviderTests rather than more integration-style Saml2LoginConfigurerTests, but I am unsure whether it is the right place. Maybe they should be in both?
    1. This being the default setup, I feel it does not have a place in the Saml2LoginConfigurerTests , but on the other hand an integration tests would give greater confidence that it actually works.
  2. In Saml2WebSsoAuthenticationFilter there is a cast to AbstractAuthenticationToken
    1. Technically the authentication converter returns a Saml2AuthenticationToken by default.
    2. However, that is not required.
    3. I can't really imagine a setup where a user provides both their own converter AND their own auth provider (they should probably provide their own filter if they do this), and do not even rely on AbstractAuthenticationToken...
    4. But I with a type check just in case and upcast to the simplest possible thing. Please let me know if that is too cautious.
  3. I have not backported this change to OpenSamlAuthenticationProvider as it is deprecated, but I can trivially port it back if required.

@Kehrlann Kehrlann marked this pull request as draft September 21, 2021 13:32
@Kehrlann Kehrlann marked this pull request as ready for review September 22, 2021 12:36
@jzheaux jzheaux assigned marcusdacoregio and unassigned jzheaux Sep 22, 2021
@jzheaux jzheaux added in: saml2 An issue in SAML2 modules status: duplicate A duplicate of another issue type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Sep 22, 2021
Copy link
Contributor

@marcusdacoregio marcusdacoregio left a comment

Choose a reason for hiding this comment

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

Hi @Kehrlann, thank you very much for the PR. I've left some feedback inline.

Copy link
Contributor

@marcusdacoregio marcusdacoregio left a comment

Choose a reason for hiding this comment

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

Thank you @Kehrlann.

I've left a couple more suggestions in line. Once you fix them we can merge the PR!

@marcusdacoregio marcusdacoregio merged commit 2fb8e66 into spring-projects:main Sep 27, 2021
@marcusdacoregio
Copy link
Contributor

Thank you very much @Kehrlann. This is now merged into main.

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
None yet
Development

Successfully merging this pull request may close these issues.

Saml2WebSsoAuthenticationFilter ignores the authentication details
4 participants