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 AuthorizationManager that uses ExpressionHandler #11171

Merged
merged 1 commit into from
May 13, 2022
Merged

Add AuthorizationManager that uses ExpressionHandler #11171

merged 1 commit into from
May 13, 2022

Conversation

evgeniycheban
Copy link
Contributor

@evgeniycheban evgeniycheban commented Apr 27, 2022 •
edited
Loading

Closes gh-11105

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Apr 27, 2022
@evgeniycheban evgeniycheban force-pushed the gh-11105 branch 3 times, most recently from 3ab2068 to 75ca7e6 Compare April 27, 2022 16:05
@jgrandja jgrandja requested a review from jzheaux May 4, 2022 16:34
rwinch
rwinch requested changes May 4, 2022
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 PR @evgeniycheban I've commented inline

...va/org/springframework/security/web/access/expression/WebExpressionAuthorizationManager.java Outdated

private Expression expression;

private final String expressionString;
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this needs to be tracked separately because you can just set the Expression directly and use Expression.getExpressionString() when you need to update the Expression when a new SecurityExpressionHandler is set.

This would also allow afterPropertiesSet to be removed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just wondering if it's okay to parse an expression twice when a custom SecurityExpressionHandler is set.
I personally prefer using afterPropertiesSet here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

...va/org/springframework/security/web/access/expression/WebExpressionAuthorizationManager.java Show resolved Hide resolved
.../main/java/org/springframework/security/web/access/expression/WebSecurityExpressionRoot.java Outdated
@@ -39,6 +40,17 @@ public WebSecurityExpressionRoot(Authentication a, FilterInvocation fi) {
this.request = fi.getRequest();
Copy link
Member

Choose a reason for hiding this comment

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

This constructor can use the newly added constructor this(authentication, fi.getRequest())

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@evgeniycheban evgeniycheban requested a review from rwinch May 5, 2022 11:52
@rwinch
Copy link
Member

rwinch commented May 11, 2022

Thank you for the changes. I've approved it from my side and will leave it to @jzheaux from here

jzheaux
jzheaux requested changes May 11, 2022
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, @evgeniycheban! I've left some feedback inline.

...ingframework/security/config/annotation/web/configurers/AuthorizeHttpRequestsConfigurer.java Outdated Show resolved Hide resolved
...org/springframework/security/web/access/expression/DefaultHttpSecurityExpressionHandler.java Outdated Show resolved Hide resolved
...org/springframework/security/web/access/expression/DefaultHttpSecurityExpressionHandler.java Outdated Show resolved Hide resolved
.../main/java/org/springframework/security/web/access/expression/WebSecurityExpressionRoot.java Show resolved Hide resolved
...va/org/springframework/security/web/access/expression/WebExpressionAuthorizationManager.java Outdated Show resolved Hide resolved
@jzheaux
Copy link
Contributor

jzheaux commented May 11, 2022

Also, @evgeniycheban, if you would please rebase against 5.8.x, that would be very helpful.

@jzheaux jzheaux added in: core An issue in spring-security-core type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels May 11, 2022
@jzheaux jzheaux added this to the 5.8.0-M1 milestone May 11, 2022
@evgeniycheban evgeniycheban changed the base branch from main to 5.8.x May 11, 2022 22:38
@evgeniycheban evgeniycheban force-pushed the gh-11105 branch 2 times, most recently from 6f3ff0d to 23dca8b Compare May 11, 2022 22:52
@evgeniycheban
Copy link
Contributor Author

@jzheaux Thanks for the review. I've made changes according to your comments and rebased this branch on top of 5.8.x.

@jzheaux jzheaux merged commit 07b0be3 into spring-projects:5.8.x May 13, 2022
@jzheaux
Copy link
Contributor

jzheaux commented May 13, 2022

Thanks again, @evgeniycheban! This is now merged into 5.8.x. I also added a polish to introduce support for request variables. It also adds a few more tests: ffaf5b4

@jzheaux jzheaux added the status: duplicate A duplicate of another issue label May 13, 2022
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

@jzheaux jzheaux jzheaux approved these changes

Assignees

@jzheaux jzheaux

Labels
in: core An issue in spring-security-core status: duplicate A duplicate of another issue type: enhancement A general enhancement
Projects
None yet
Milestone
5.8.0-M1
Development

Successfully merging this pull request may close these issues.

Add AuthorizationManager that uses ExpressionHandler
4 participants
@evgeniycheban @rwinch @jzheaux @spring-projects-issues