-
Notifications
You must be signed in to change notification settings - Fork 5.9k
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
Introduce @CurrentSecurityContext for method arguments #6562
Conversation
fdb517f
to
853afce
Compare
853afce
to
c23a30c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @clevertension! I've left some inline feedback.
Also, I'd like to see if @rwinch has some feedback, too, especially around whether the patterns that are copied from the existing argument resolvers are ones we want to propagate.
core/src/main/java/org/springframework/security/core/annotation/CurrentSecurityContext.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/springframework/security/core/annotation/CurrentSecurityContext.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/springframework/security/core/annotation/CurrentSecurityContext.java
Show resolved
Hide resolved
...g/springframework/security/web/bind/support/CurrentSecurityContextArgumentResolverTests.java
Outdated
Show resolved
Hide resolved
...k/security/web/reactive/result/method/annotation/CurrentSecurityContextArgumentResolver.java
Show resolved
Hide resolved
...g/springframework/security/web/bind/support/CurrentSecurityContextArgumentResolverTests.java
Show resolved
Hide resolved
@jzheaux thanks very much for your review feedback, i have already fix all the issues that you mentioned, so should i add extra 1 commit, or still keep only one commit by force push here? |
@clevertension Either way. Before we merge, you'll need to force push down to one commit. Some folks like to keep commits separate while the review is going on. For small PRs like this, it doesn't make a big difference. |
👍,👍,👍 |
@rwinch , can you spare time to review this PR? thank you very much |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@clevertension I took a fresh look at this, and I left a couple more comments inline.
...va/org/springframework/security/web/bind/support/CurrentSecurityContextArgumentResolver.java
Show resolved
Hide resolved
...k/security/web/reactive/result/method/annotation/CurrentSecurityContextArgumentResolver.java
Show resolved
Hide resolved
...g/springframework/security/web/bind/support/CurrentSecurityContextArgumentResolverTests.java
Show resolved
Hide resolved
418c898
to
de356cd
Compare
There was a problem hiding this 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 @clevertension! The changes look good
Thanks, @clevertension! This is now merged into |
it can both support with spring web mvc and reactive web, user can retrieve
SecurityContext
in controller easily, such as#6546