Skip to content

X509 Reactive Support #6336

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
Merged
merged 3 commits into from
Apr 26, 2019
Merged

X509 Reactive Support #6336

merged 3 commits into from
Apr 26, 2019

Conversation

alek-sys
Copy link
Contributor

No description provided.

@alek-sys alek-sys force-pushed the master branch 2 times, most recently from 6a9d888 to 3b60e55 Compare December 28, 2018 12:15
@rwinch rwinch self-assigned this Jan 7, 2019
@rwinch rwinch added in: core An issue in spring-security-core in: web An issue in web modules (web, webmvc) Reactive and removed in: core An issue in spring-security-core labels Jan 7, 2019
rwinch
rwinch requested changes Jan 7, 2019
View reviewed changes
Copy link
Member

@rwinch rwinch 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 comprehensive PR @alek-sys! I provided feedback inline.

@alek-sys alek-sys force-pushed the master branch 2 times, most recently from 093110a to 3261d4a Compare January 8, 2019 18:24
rwinch
rwinch requested changes Jan 9, 2019
View reviewed changes
Copy link
Member

@rwinch rwinch 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 fast turnaround! I found a few other small house keeping items that are I responded inline. Also can you please add documentation in https://github.com/spring-projects/spring-security/tree/master/docs/manual/src/docs/asciidoc/_includes/reactive

Thanks again for your help on this issue!

@alek-sys
Copy link
Contributor Author

Hey @rwinch, sorry for not coming back on this, I'm looking into this and hope to get it updated today or tomorrow.

@alek-sys
Copy link
Contributor Author

I'm not sure why Github shows this as "Changes requested", but I've added all the things you've asked, Rob (including documentation).

rwinch
rwinch requested changes Jan 25, 2019
View reviewed changes
Copy link
Member

@rwinch rwinch 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 the nudge. The status doesn't automatically get updated when you push changes.

I added a couple more comments inline.

Thanks again for your work on this!

@alek-sys
Copy link
Contributor Author

alek-sys commented Feb 5, 2019

I bet you knew that! 👍

Yes, there is an issue with using default authentication manager, even more of that, I've discovered that default authentication manager won't work, because PreAuthenticatedAuthenticationToken created by the converted will not be authorized. Also, to get feature parity with Servlet X509 I'd like to add PreAuthenticatedAuthenticationProvider support as well. So I keep working on this one.

rwinch
rwinch requested changes Feb 15, 2019
View reviewed changes
Copy link
Member

@rwinch rwinch 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. Comments are inline

@alek-sys alek-sys force-pushed the master branch 2 times, most recently from af30173 to 5ecd42a Compare February 18, 2019 12:04
@alek-sys
Copy link
Contributor Author

alek-sys commented Feb 18, 2019 •
edited
Loading

I've also updated the integration test to use WebTestClient with a client certificate, it makes more readable.

@alek-sys
Copy link
Contributor Author

Hey @rwinch, I'm not planning any more updates here, so feel free to review this PR whever it is a good time for you. Thanks for your help!

@alek-sys
Copy link
Contributor Author

@rwinch sorry for a nudge, do you think this PR can make it to 5.2?

rwinch
rwinch requested changes Apr 26, 2019
View reviewed changes
Copy link
Member

@rwinch rwinch left a comment

Choose a reason for hiding this comment

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

Sorry for the delays. I have one more minor comment

@alek-sys
Copy link
Contributor Author

@rwinch no worries, I've just changed the method visibility. Also, rebase on top of lastest changes in master. Thanks for your guidance and help on that!

@rwinch rwinch added this to the 5.2.0.M3 milestone Apr 26, 2019
@rwinch rwinch merged commit 0aa4805 into spring-projects:master Apr 26, 2019
@rwinch rwinch added the type: enhancement A general enhancement label May 3, 2019
@rwinch rwinch mentioned this pull request May 1, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rwinch rwinch rwinch approved these changes

Assignees

@rwinch rwinch

Labels
in: web An issue in web modules (web, webmvc) type: enhancement A general enhancement
Projects
None yet
Milestone
5.2.0.M3
Development

Successfully merging this pull request may close these issues.
2 participants
@alek-sys @rwinch