Skip to content

Set "rolePrefix" in ReactiveMethodSecurityConfiguration #6801

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

Merged
merged 1 commit into from
May 2, 2019

Conversation

ttddyy
Copy link
Contributor

@ttddyy ttddyy commented Apr 19, 2019

Currently, GrantedAuthorityDefaults is not considered in
ReactiveMethodSecurityConfiguration.
This commit updates the configuration to be aware of
GrantedAuthorityDefaults and update rolePrefix when the bean is
available.

Also, use the same instance of DefaultMethodSecurityExpressionHandler
when constructing ExpressionBasedAnnotationAttributeFactory.

@jzheaux
Copy link
Contributor

jzheaux commented Apr 25, 2019

Thanks, @ttddyy! Would you also be able to write some tests that confirm that this behavior works as expected? I think none have been added to this point, but you should be able to do something similar to GlobalMethodSecurityConfigurationTests.

And then, is there any reason that you chose to have the @Bean method depend on DefaultMethodSecurityExpressionHandler instead of MethodSecurityExpressionHandler?

@ttddyy ttddyy force-pushed the role-prefix-in-reactive branch from a6771d7 to f0105d6 Compare April 26, 2019 20:17
@ttddyy
Copy link
Contributor Author

ttddyy commented Apr 26, 2019

Hi @jzheaux

Updated the PR for

  • Added test - ReactiveMethodSecurityConfigurationTests
  • Changed to take MethodSecurityExpressionHandler on methodMetadataSource bean

Thanks,

@ttddyy ttddyy force-pushed the role-prefix-in-reactive branch from f0105d6 to 3d58d9b Compare April 26, 2019 20:52
@jzheaux jzheaux requested a review from rwinch April 26, 2019 20:56
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! I have provided feedback inline

* @since 5.0
*/
@Configuration
class ReactiveMethodSecurityConfiguration implements ImportAware {
private int advisorOrder;

@Autowired(required = false)
private GrantedAuthorityDefaults grantedAuthorityDefaults;
Copy link
Member

Choose a reason for hiding this comment

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

Please add a package private setter and move the @Autowired(required = false) on to the setter to align with gh-6624

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rwinch Updated the PR to add a package private setter for Spring Fu support.

Currently, `GrantedAuthorityDefaults` is not considered in
`ReactiveMethodSecurityConfiguration`.
This commit updates the configuration to be aware of
`GrantedAuthorityDefaults` and update `rolePrefix` when the bean is
available.

Also, use the same instance of `DefaultMethodSecurityExpressionHandler`
when constructing `ExpressionBasedAnnotationAttributeFactory`.
@ttddyy ttddyy force-pushed the role-prefix-in-reactive branch from 3d58d9b to d619434 Compare May 1, 2019 05:46
@rwinch rwinch self-assigned this May 2, 2019
@rwinch rwinch added Reactive in: core An issue in spring-security-core labels May 2, 2019
@rwinch rwinch added this to the 5.2.0.M3 milestone May 2, 2019
@rwinch rwinch merged commit aef3f51 into spring-projects:master May 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: core An issue in spring-security-core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants