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

ReactiveAuthorizationManager + Reactive Method Security #9867

Merged
merged 1 commit into from
Aug 25, 2022
Merged

ReactiveAuthorizationManager + Reactive Method Security #9867

merged 1 commit into from
Aug 25, 2022

Conversation

evgeniycheban
Copy link
Contributor

Closes gh-9401

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jun 4, 2021
@evgeniycheban evgeniycheban force-pushed the gh-9401 branch 3 times, most recently from 0bdddc0 to 44b9720 Compare June 4, 2021 16:09
@evgeniycheban evgeniycheban force-pushed the gh-9401 branch 13 times, most recently from 3dbe286 to e78efc0 Compare June 7, 2021 01:05
jzheaux
jzheaux requested changes Jul 20, 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, @evgeniycheban, for your efforts here!

I've left some initial feedback inline.

...ork/security/config/annotation/method/configuration/ReactiveMethodSecurityConfiguration.java Outdated
ExpressionBasedPreInvocationAdvice preAdvice = new ExpressionBasedPreInvocationAdvice();
preAdvice.setExpressionHandler(handler);
return new PrePostAdviceReactiveMethodInterceptor(source, preAdvice, postAdvice);
@Role(BeanDefinition.ROLE_INFRASTRUCTURE)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is likely a bug for other branches. Would you please put this addition of the @Role annotation into a separate commit? That way, we can backport that commit to earlier branches.

...ork/security/authorization/method/PreFilterAuthorizationBeforeReactiveMethodInterceptor.java Outdated Show resolved Hide resolved
...ava/org/springframework/security/authorization/method/AbstractReactiveMethodInterceptor.java Outdated Show resolved Hide resolved
...rg/springframework/security/authorization/method/PostAuthorizeAuthorizationManagerTests.java Show resolved Hide resolved
@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 Jul 27, 2021
@jzheaux
Copy link
Contributor

jzheaux commented Oct 1, 2021

Hi, @evgeniycheban, are you able to apply the requested changes?

@evgeniycheban
Copy link
Contributor Author

Hi, @evgeniycheban, are you able to apply the requested changes?

Hi, sorry for the long time of inactivity on this PR. I've been busy at work these few months. I plan to continue working on this next week.

@evgeniycheban evgeniycheban force-pushed the gh-9401 branch 2 times, most recently from 627bedd to 3516528 Compare October 11, 2021 23:53
@evgeniycheban
Copy link
Contributor Author

@jzheaux I updated the PR according to your comments.

@evgeniycheban evgeniycheban requested a review from jzheaux October 11, 2021 23:56
@evgeniycheban evgeniycheban force-pushed the gh-9401 branch 2 times, most recently from a7aa40b to d6c2fb2 Compare October 12, 2021 00:23
@evgeniycheban
Copy link
Contributor Author

@rwinch @jzheaux I updated the PR, please take a look.

@evgeniycheban evgeniycheban requested a review from rwinch March 18, 2022 19:07
@code-uri
Copy link

@evgeniycheban I am seeing error " EL1001E: Type conversion problem, cannot convert from reactor.core.publisher.MonoJust<java.lang.Boolean> to java.lang.Boolean" when mixing a non reactive and reactive expression in @PreAuthorize.

Any clue why is this happening?

Example:-
@PreAuthorize("@authz.checkReactive() || hasRole('ADMIN')")

@evgeniycheban
Copy link
Contributor Author

@evgeniycheban I am seeing error " EL1001E: Type conversion problem, cannot convert from reactor.core.publisher.MonoJust<java.lang.Boolean> to java.lang.Boolean" when mixing a non reactive and reactive expression in @PreAuthorize.

Any clue why is this happening?

Example:- @PreAuthorize("@authz.checkReactive() || hasRole('ADMIN')")

@code-uri Interesting note.
Mixing reactive and non-reactive expressions is not currently supported.
I think we should address this to the Spring Framework team so they can decide if this should be supported in Spring expressions.

@jzheaux @rwinch What do you think? Should the user be able to mix reactive and non-reactive expressions in @PreAuthorize?

@evgeniycheban evgeniycheban force-pushed the gh-9401 branch 3 times, most recently from fe02459 to ba54a56 Compare March 29, 2022 16:00
@jzheaux
Copy link
Contributor

jzheaux commented Mar 30, 2022

Should the user be able to mix reactive and non-reactive expressions in @PreAuthorize?

I wonder if the user could simply do @authz.checkReactive(#root, 'ADMIN') and then invoke hasRole('ADMIN') from within the authz bean.

Personally, I'm not a huge fan of embedding logic inside the annotations as it's a bit harder to test; I'd prefer to use a bean or a custom ReactiveAuthorizationManager. I'd probably want to wait for a clearer use case where mixing the two types in a SpEL expression is better than one of these two alternatives. Just my 2c, though.

@evgeniycheban
Copy link
Contributor Author

@jzheaux @rwinch Any updates on this?

@rwinch rwinch added this to the 5.8.x milestone Jun 2, 2022
@evgeniycheban evgeniycheban changed the base branch from main to 5.8.x June 9, 2022 22:00
@evgeniycheban evgeniycheban force-pushed the gh-9401 branch 3 times, most recently from 82202bc to b3786ee Compare June 9, 2022 23:41
@evgeniycheban
Copy link
Contributor Author

@rwinch @jzheaux I've rebased it on top of 5.8.x and updated @since to 5.8.

rwinch
rwinch requested changes Jun 27, 2022
View reviewed changes
Copy link
Member

@rwinch rwinch left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I've responded inline. The biggest ask is to update to remove co-routines support as discussed #9867 (review)

Please also ensure you rebase off 5.8.x as there are currently conflicts

...va/org/springframework/security/authorization/method/CoroutinesBeanFactoryPostProcessor.java Outdated Show resolved Hide resolved
...framework/security/authorization/method/PreFilterAuthorizationReactiveMethodInterceptor.java Show resolved Hide resolved
...mework/security/authorization/method/AuthorizationManagerAfterReactiveMethodInterceptor.java Show resolved Hide resolved
...ework/security/authorization/method/AuthorizationManagerBeforeReactiveMethodInterceptor.java Show resolved Hide resolved
.../springframework/security/authorization/method/CoroutinesAfterReactiveMethodInterceptor.java Outdated Show resolved Hide resolved
...springframework/security/authorization/method/PostAuthorizeReactiveAuthorizationManager.java Show resolved Hide resolved
@koenpunt
Copy link

koenpunt commented Aug 8, 2022

Any update on this?

@jens-meiss
Copy link

i tried to rebase 5.8.x, but the 5.8.x branch was broken/couldn't be build because of mission classes ... @evgeniycheban are you going to fix the stuff mentionend in the review?

@evgeniycheban
Copy link
Contributor Author

Hello @koenpunt @jens-meiss, I'm going to fix it this week.

@jzheaux
Copy link
Contributor

jzheaux commented Aug 17, 2022

Thanks, @evgeniycheban, just saw your update. Thank you for such a valuable and time-consuming contribution. I'll add any minor polish that remains and hopefully merge this week.

@jzheaux jzheaux modified the milestones: 5.8.x, 5.8.0-M3 Aug 25, 2022
@jzheaux jzheaux merged commit cbb4f40 into spring-projects:5.8.x Aug 25, 2022
@jzheaux
Copy link
Contributor

jzheaux commented Aug 25, 2022

Nice, @evgeniycheban! This is now merged into 5.8.x and main. I also added a polish commit at e990174 and a documentation commit at 070dce1.

Thanks again for all your consistent effort to this PR!

@jzheaux jzheaux mentioned this pull request Dec 22, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rwinch rwinch rwinch requested changes

@eleftherias eleftherias Awaiting requested review from eleftherias

@jzheaux jzheaux Awaiting requested review from jzheaux

Assignees

@rwinch rwinch

@jzheaux jzheaux

Labels
in: core An issue in spring-security-core type: enhancement A general enhancement
Projects
Spring Security Team
Status: Done
Milestone
5.8.0-M3
Development

Successfully merging this pull request may close these issues.

ReactiveAuthorizationManager + Reactive Method Security
8 participants
@evgeniycheban @jzheaux @code-uri @koenpunt @jens-meiss @rwinch @eleftherias @spring-projects-issues