-
Notifications
You must be signed in to change notification settings - Fork 6.1k
Add AuthorizationManagerFactory #17673
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
Add AuthorizationManagerFactory #17673
Conversation
4ef91a4
to
1de115e
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 updates @sjohnr! I've provided feedback inline.
...n/java/org/springframework/security/access/expression/AbstractSecurityExpressionHandler.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/springframework/security/access/expression/SecurityExpressionRoot.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/springframework/security/access/expression/SecurityExpressionRoot.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/springframework/security/access/expression/SecurityExpressionRoot.java
Outdated
Show resolved
Hide resolved
...main/java/org/springframework/security/authorization/DefaultAuthorizationManagerFactory.java
Outdated
Show resolved
Hide resolved
.../org/springframework/security/messaging/access/expression/MessageSecurityExpressionRoot.java
Outdated
Show resolved
Hide resolved
.../org/springframework/security/messaging/access/expression/MessageSecurityExpressionRoot.java
Outdated
Show resolved
Hide resolved
...gframework/security/messaging/access/expression/DefaultMessageSecurityExpressionHandler.java
Outdated
Show resolved
Hide resolved
.../org/springframework/security/web/access/expression/DefaultWebSecurityExpressionHandler.java
Outdated
Show resolved
Hide resolved
.../org/springframework/security/web/access/expression/DefaultWebSecurityExpressionHandler.java
Outdated
Show resolved
Hide resolved
@rwinch Thanks for the review! Changing the generic type of |
I think that this can work if we update the constructor too. For example: public class MessageSecurityExpressionRoot<T> extends SecurityExpressionRoot {
public final Message<T> message;
public MessageSecurityExpressionRoot(Authentication authentication, Message<T> message) {
...
public MessageSecurityExpressionRoot(Supplier<Authentication> authentication, Message<T> message) {
... Then this will compile: Message<?> message = ...
MessageSecurityExpressionRoot root = new MessageSecurityExpressionRoot<>(authentication, message); |
1de115e
to
5fb6f37
Compare
@rwinch I've working through the changes and it's looking really good. The only thing I'm noticing is that a few packages are missing nullability defaults.
As it stands now, classes in these packages show as having inconsistent nullability. These show up as IntelliJ inspections for me, similar to our conversation a few weeks ago. I think adding |
.../springframework/security/data/repository/query/SecurityEvaluationContextExtensionTests.java
Show resolved
Hide resolved
...pringframework/security/access/expression/method/DefaultMethodSecurityExpressionHandler.java
Show resolved
Hide resolved
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.
This is looking great @sjohnr!
I think we are very close 😄 I provided a few minor comments inline.
Now that we are almost done, please also add documentation (using include-code).
After those are addressed, I think we will be ready to merge!
...main/java/org/springframework/security/authorization/DefaultAuthorizationManagerFactory.java
Outdated
Show resolved
Hide resolved
...main/java/org/springframework/security/authorization/DefaultAuthorizationManagerFactory.java
Outdated
Show resolved
Hide resolved
...main/java/org/springframework/security/authorization/DefaultAuthorizationManagerFactory.java
Outdated
Show resolved
Hide resolved
.../springframework/security/data/repository/query/SecurityEvaluationContextExtensionTests.java
Show resolved
Hide resolved
.../java/org/springframework/security/web/access/expression/FilterInvocationExpressionRoot.java
Show resolved
Hide resolved
.../java/org/springframework/security/web/access/expression/FilterInvocationExpressionRoot.java
Show resolved
Hide resolved
We've only added nullability for crypto and core so feel free to leave as is. We'll get the rest of the module in a different ticket commit. |
5fb6f37
to
d8d5eb7
Compare
0e4933a
to
d8d5eb7
Compare
d8d5eb7
to
0e4933a
Compare
23d776d
to
913c7e8
Compare
0a9448b
to
06e5a21
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.
@rwinch I've added documentation, code examples with include-code
, and tests. I've also added two additional notes for your review below. Aside from the note below, I believe this PR is ready to merge.
I will also be creating a GH issue for the Kotlin authorizeHttpRequests
DSL, which doesn't currently support using the new AuthorizationManagerFactory
. I doubt I'll have time to work on that change though (if needed for 7.0).
...work/security/config/annotation/method/configuration/PrePostMethodSecurityConfiguration.java
Outdated
Show resolved
Hide resolved
...ingframework/security/config/annotation/web/configurers/AuthorizeHttpRequestsConfigurer.java
Show resolved
Hide resolved
@sjohnr Thanks for the PR. Overall, I think that we are ready to merge after you address my two replies to your comments.
Thanks for the heads up. We will ensure that this gets taken care of. |
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.
I've provided feedback inline. Can you please include a link in the whats-new.adoc as well?
Signed-off-by: Steve Riesenberg <5248162+sjohnr@users.noreply.github.com>
06e5a21
to
90a29ce
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.
Awesome work @sjohnr! This is ready for merge
Closes gh-17585