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.

Sign up for GitHub

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

Jump to bottom

Add Saml2AuthenticationRequestRepository #10060

Merged
merged 2 commits into from
Jul 27, 2021
Merged

Add Saml2AuthenticationRequestRepository #10060

merged 2 commits into from
Jul 27, 2021

Conversation

marcusdacoregio
Copy link
Contributor

Closes gh-9185

Related #9174

@marcusdacoregio marcusdacoregio added status: duplicate A duplicate of another issue type: enhancement A general enhancement in: saml2 An issue in SAML2 modules labels Jul 9, 2021
@marcusdacoregio marcusdacoregio added this to the 5.6.0-M1 milestone Jul 9, 2021
@marcusdacoregio marcusdacoregio requested a review from jzheaux July 9, 2021 19:27
@marcusdacoregio marcusdacoregio self-assigned this Jul 9, 2021
jzheaux
jzheaux requested changes Jul 12, 2021
View reviewed changes
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 for the PR, @marcusdacoregio!

In addition to the comments, please also add DSL support and docs. At a minimum, the DSL support should look for a bean of type Saml2AuthorizationRequestRepository and wire it in accordingly.

...springframework/security/saml2/provider/service/authentication/Saml2AuthenticationToken.java Show resolved Hide resolved
...security/saml2/provider/service/servlet/HttpSessionSaml2AuthenticationRequestRepository.java Outdated Show resolved Hide resolved
...security/saml2/provider/service/servlet/HttpSessionSaml2AuthenticationRequestRepository.java Outdated Show resolved Hide resolved
...security/saml2/provider/service/servlet/HttpSessionSaml2AuthenticationRequestRepository.java Outdated Show resolved Hide resolved
...security/saml2/provider/service/servlet/HttpSessionSaml2AuthenticationRequestRepository.java Show resolved Hide resolved
...ramework/security/saml2/provider/service/servlet/filter/Saml2WebSsoAuthenticationFilter.java Show resolved Hide resolved
...g/springframework/security/saml2/provider/service/web/Saml2AuthenticationTokenConverter.java Outdated Show resolved Hide resolved
...g/springframework/security/saml2/provider/service/web/Saml2AuthenticationTokenConverter.java Outdated Show resolved Hide resolved
...rity/saml2/provider/service/servlet/HttpSessionSaml2AuthenticationRequestRepositoryTest.java Outdated Show resolved Hide resolved
...springframework/security/saml2/provider/service/authentication/Saml2AuthenticationToken.java Outdated Show resolved Hide resolved
@marcusdacoregio marcusdacoregio force-pushed the gh-9185 branch 2 times, most recently from aa48eab to 270c10e Compare July 15, 2021 19:56
@marcusdacoregio
Copy link
Contributor Author

Thanks for your review @jzheaux. I've updated the PR with your suggestions and the DSL and docs addition.

@marcusdacoregio marcusdacoregio requested a review from jzheaux July 15, 2021 19:57
@eleftherias eleftherias modified the milestones: 5.6.0-M1, 5.6.0-M2 Jul 19, 2021
jzheaux
jzheaux requested changes Jul 19, 2021
View reviewed changes
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 for the updates, @marcusdacoregio! I've left some additional feedback about the changes you submitted.

...g/springframework/security/config/annotation/web/configurers/saml2/Saml2LoginConfigurer.java Outdated Show resolved Hide resolved
...g/springframework/security/config/annotation/web/configurers/saml2/Saml2LoginConfigurer.java Outdated Show resolved Hide resolved
docs/manual/src/docs/asciidoc/_includes/servlet/saml2/saml2-login.adoc Outdated Show resolved Hide resolved
...springframework/security/saml2/provider/service/authentication/Saml2AuthenticationToken.java Outdated Show resolved Hide resolved
...ramework/security/saml2/provider/service/servlet/filter/Saml2WebSsoAuthenticationFilter.java Outdated Show resolved Hide resolved
...ramework/security/saml2/provider/service/servlet/filter/Saml2WebSsoAuthenticationFilter.java Outdated Show resolved Hide resolved
...g/springframework/security/saml2/provider/service/web/Saml2AuthenticationTokenConverter.java Outdated Show resolved Hide resolved
...g/springframework/security/saml2/provider/service/web/Saml2AuthenticationTokenConverter.java Outdated Show resolved Hide resolved
@marcusdacoregio marcusdacoregio force-pushed the gh-9185 branch 2 times, most recently from b39ac5a to 6945064 Compare July 22, 2021 14:19
@marcusdacoregio
Copy link
Contributor Author

Thanks for your effort on the review @jzheaux. I've updated the PR with your suggestions.

@marcusdacoregio marcusdacoregio requested a review from jzheaux July 22, 2021 14:19
jzheaux
jzheaux requested changes Jul 26, 2021
View reviewed changes
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, @marcusdacoregio! I've left some additional feedback inline.

...g/springframework/security/config/annotation/web/configurers/saml2/Saml2LoginConfigurer.java Outdated Show resolved Hide resolved
...g/springframework/security/saml2/provider/service/web/Saml2AuthenticationTokenConverter.java Outdated Show resolved Hide resolved
...g/springframework/security/config/annotation/web/configurers/saml2/Saml2LoginConfigurer.java Show resolved Hide resolved
@marcusdacoregio
Copy link
Contributor Author

Thanks @jzheaux, I've updated the PR based on your feedback.

@marcusdacoregio marcusdacoregio requested a review from jzheaux July 27, 2021 13:52
jzheaux
jzheaux requested changes Jul 27, 2021
View reviewed changes
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.

Things are really taking shape now, @marcusdacoregio, nice work.

I've left some remaining feedback about JavaDoc, docs, and tests.

...ork/security/saml2/provider/service/servlet/filter/Saml2WebSsoAuthenticationFilterTests.java Outdated Show resolved Hide resolved
...ork/security/saml2/provider/service/servlet/filter/Saml2WebSsoAuthenticationFilterTests.java Outdated Show resolved Hide resolved
...ity/saml2/provider/service/servlet/HttpSessionSaml2AuthenticationRequestRepositoryTests.java Outdated Show resolved Hide resolved
...ity/saml2/provider/service/servlet/HttpSessionSaml2AuthenticationRequestRepositoryTests.java Outdated Show resolved Hide resolved
...gframework/security/saml2/provider/service/servlet/Saml2AuthenticationRequestRepository.java Outdated Show resolved Hide resolved
docs/manual/src/docs/asciidoc/_includes/servlet/saml2/saml2-login.adoc Outdated Show resolved Hide resolved
docs/manual/src/docs/asciidoc/_includes/servlet/saml2/saml2-login.adoc Outdated Show resolved Hide resolved
@marcusdacoregio
Add Saml2AuthenticationRequestRepository
377160d 
Closes spring-projectsgh-9185
@marcusdacoregio
Fix test getting stuck
9024e86 
The tests are getting stuck when running a single test class and the mock is performed in a static variable inside an inner class

Issue spring-projectsgh-6025
@marcusdacoregio
Copy link
Contributor Author

Thank you again for the review @jzheaux. I've updated the PR.

About the docs, I've updated it to use the SAML terms, like AuthnRequest and SAMLResponse, aligned with the rest of the docs as you mentioned.

@marcusdacoregio marcusdacoregio requested a review from jzheaux July 27, 2021 18:57
@jzheaux jzheaux merged commit 662ab10 into spring-projects:main Jul 27, 2021
@jzheaux
Copy link
Contributor

jzheaux commented Jul 27, 2021

Thanks, @marcusdacoregio! This is now merged into main.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jzheaux jzheaux jzheaux approved these changes

Assignees

@marcusdacoregio marcusdacoregio

Labels
in: saml2 An issue in SAML2 modules status: duplicate A duplicate of another issue type: enhancement A general enhancement
Projects
None yet
Milestone
5.6.0-M2
Development

Successfully merging this pull request may close these issues.

Add Saml2AuthenticationRequestRepository
3 participants
@marcusdacoregio @jzheaux @eleftherias