Authorization Manager Method Security #9630

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

Merged

jzheaux merged 2 commits into spring-projects:main from jzheaux:authorization-manager-method-security

May 18, 2021

Contributor

jzheaux commented Apr 13, 2021

No description provided.

jzheaux assigned rwinch

jzheaux force-pushed the authorization-manager-method-security branch 3 times, most recently from 2adbf47 to 396ab87 Compare

April 19, 2021 22:08

rwinch requested changes

View reviewed changes

Member

rwinch left a comment

I've provided some feedback. I think we should also ensure we are addressing issues around what to do if multiple annotations are present. A few examples are:

...in/java/org/springframework/security/authorization/method/AuthorizationMethodInvocation.java Outdated Show resolved Hide resolved

...in/java/org/springframework/security/authorization/method/AuthorizationMethodInvocation.java Outdated Show resolved Hide resolved

...work/security/config/annotation/method/configuration/PrePostMethodSecurityConfiguration.java Outdated Show resolved Hide resolved

...work/security/config/annotation/method/configuration/PrePostMethodSecurityConfiguration.java Outdated Show resolved Hide resolved

...main/java/org/springframework/security/authorization/method/MethodInvocationReturnValue.java Outdated Show resolved Hide resolved

.../java/org/springframework/security/authorization/method/AuthorizationMethodInterceptors.java Outdated Show resolved Hide resolved

.../springframework/security/config/annotation/method/configuration/MethodSecuritySelector.java Outdated Show resolved Hide resolved

.../springframework/security/config/annotation/method/configuration/MethodSecuritySelector.java Outdated Show resolved Hide resolved

...n/java/org/springframework/security/authorization/method/AuthorizationMethodInterceptor.java Outdated Show resolved Hide resolved

rwinch requested changes

View reviewed changes

Member

rwinch left a comment

I've provided some additional feedback inline. I also think we need to ensure we address #9630 (review)

.../org/springframework/security/authorization/method/AbstractAuthorizationManagerRegistry.java Outdated

+              	/**
+              	 * Returns an {@link AuthorizationManager} for the
+              	 * {@link AuthorizationMethodInvocation}.

Member

rwinch Apr 21, 2021

AuthorizationMethodInvocation does not exist, so please remove references to it

core/src/main/java/org/springframework/security/authorization/method/AuthorizationAdvisors.java Outdated

+               * @see SecuredAuthorizationManager
+               * @see Jsr250AuthorizationManager
+               */
+              public final class AuthorizationAdvisors {

Member

rwinch Apr 21, 2021

I think this could be removed in favor of constructors on the *Interceptors that match the static methods. We could put the DEFAULT_ORDER constant on the *Interceptor instances and default the value to it. Doing this would mean the users who customize the *Interceptors wouldn't need to remember to set the value to the default.

It would also make things a bit more consistent. The use of these constants seems inconsistent. PRE_FILTER_ADVISOR_ORDER is used by PreFilterAuthorizationMethodInterceptor directly. However, PRE_AUTHORIZE_ADVISOR_ORDER is only used in AuthorizationAdvisors.

...ava/org/springframework/security/authorization/method/PostAuthorizeAuthorizationManager.java Show resolved Hide resolved

jzheaux force-pushed the authorization-manager-method-security branch from 74e1e47 to 04d8184 Compare

April 28, 2021 04:06

rwinch requested changes

View reviewed changes

Member

rwinch left a comment •

edited

Loading

Overall it looks good. I've provided some comments inline. Can we close the related issues and use this to supersede now? I think we should also add XML support

docs/manual/src/docs/asciidoc/_includes/servlet/authorization/method-security.adoc

    
              	@PreAuthorize("hasRole('USER')")

                  Account[] findAccounts();

              	@PreAuthorize("hasRole('TELLER')")

Member

rwinch Apr 28, 2021

Spacing issues here?

docs/manual/src/docs/asciidoc/_includes/servlet/authorization/method-security.adoc Outdated

    
              [source,java]

              ----

              @Bean

              MethodSecurityExpressionHandler methodSecurityExpressionHandler() {

Member

rwinch Apr 28, 2021

make this method static as it will be initialized very early on for AOP and static methods ensure that only this method is invoked vs initializing the entire configuration class

...ain/java/org/springframework/security/authorization/method/AuthorizationAnnotationUtils.java Show resolved Hide resolved

docs/manual/src/docs/asciidoc/_includes/servlet/authorization/method-security.adoc Outdated

    
            @@ -6,6 +6,213 @@ It provides support for JSR-250 annotation security as well as the framework's o
          
              From 3.0 you can also make use of new <<el-access,expression-based annotations>>.

              You can apply security to a single bean, using the `intercept-methods` element to decorate the bean declaration, or you can secure multiple beans across the entire service layer using the AspectJ style pointcuts.

              === EnableMethodSecurity

              In 5.6, we can enable annotation-based security using the `@EnableMethodSecurity` annotation on any `@Configuration` instance.

Member

rwinch Apr 28, 2021

I think it is worth describing how this differs from @EnableGlobalMethodSecurity and that users should prefer this new support.

docs/manual/src/docs/asciidoc/_includes/servlet/authorization/method-security.adoc

    
              In 5.6, we can enable annotation-based security using the `@EnableMethodSecurity` annotation on any `@Configuration` instance.

              [NOTE]

Member

rwinch Apr 28, 2021

Similar to how how we use { for if blocks, we use ==== for admonitions

docs/manual/src/docs/asciidoc/_includes/servlet/authorization/method-security.adoc

    
            @@ -6,6 +6,213 @@ It provides support for JSR-250 annotation security as well as the framework's o
          
              From 3.0 you can also make use of new <<el-access,expression-based annotations>>.

              You can apply security to a single bean, using the `intercept-methods` element to decorate the bean declaration, or you can secure multiple beans across the entire service layer using the AspectJ style pointcuts.

              === EnableMethodSecurity

Member

rwinch Apr 28, 2021

Please add a reference to this in What's New

docs/manual/src/docs/asciidoc/_includes/servlet/authorization/method-security.adoc Outdated

    
            @@ -6,6 +6,213 @@ It provides support for JSR-250 annotation security as well as the framework's o
          
              From 3.0 you can also make use of new <<el-access,expression-based annotations>>.

              You can apply security to a single bean, using the `intercept-methods` element to decorate the bean declaration, or you can secure multiple beans across the entire service layer using the AspectJ style pointcuts.

              === EnableMethodSecurity

              In 5.6, we can enable annotation-based security using the `@EnableMethodSecurity` annotation on any `@Configuration` instance.

Member

rwinch Apr 28, 2021

Perhaps change to "In Spring Security 5.6"

jzheaux force-pushed the authorization-manager-method-security branch from 04d8184 to 75fd108 Compare

April 30, 2021 00:05

This was referenced May 3, 2021

Allow MethodSecurityInterceptor.publishAuthorizationSuccess to be configured #9705

Open

ReactiveAuthorizationManager + Reactive Method Security #9401

Closed

jzheaux force-pushed the authorization-manager-method-security branch from 75fd108 to c5515b9 Compare

May 3, 2021 20:09

rwinch approved these changes

View reviewed changes

Member

rwinch left a comment

Thanks @jzheaux The changes look good to me. We will get this into 5.6 right after 5.5 goes ga

rwinch added this to the 5.6.0-M1 milestone

rwinch added in: core type: enhancement labels

jzheaux force-pushed the authorization-manager-method-security branch from c5515b9 to 041fce8 Compare

May 4, 2021 22:41

evgeniycheban and others added 2 commits

May 18, 2021 17:06


          Consider AuthorizationManager for Method Security

94e049d

Closes spring-projectsgh-9289


          Polish AuthorizationManager Method Security

4d5397f

- Removed consolidated pointcut advisor in favor of each interceptor
being an advisor. This allows Spring AOP to do more of the heavy
lifting of selecting the set of interceptors that applies
- Created new method context for after interceptors instead of
modifying existing one
- Added documentation
- Added XML support
- Added AuthorizationInterceptorsOrder to simplify interceptor
ordering
- Adjusted annotation lookup to comply with JSR-250 spec
- Adjusted annotation lookup to exhaustively search for duplicate
annotations
- Separated into three @configuration classes, one for each set of
authorization annotations

Issue spring-projectsgh-9289

jzheaux force-pushed the authorization-manager-method-security branch from 041fce8 to 4d5397f Compare

May 18, 2021 23:06

This was referenced May 18, 2021

GlobalMethodSecurity and multiple annotation ordering #4103

Closed

Allow multiple security annotations on a method (combining result of evaluations with AND operator) #4003

Closed

SEC-2974: Multiple pre post annotations fail silently #3183

Closed

jzheaux merged commit 67e5c05 into spring-projects:main

jzheaux deleted the authorization-manager-method-security branch

May 18, 2021 23:34

jzheaux mentioned this pull request

Multiple Pre or PostAuthorization Annotations #9452

Closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

in: core type: enhancement