AuthorizationManager + Method Security Support #9350

evgeniycheban · 2021-01-18T09:28:09Z

jzheaux

Thanks, @evgeniycheban! This is a good start.

I've left some feedback inline. In addition to that, please keep classes final until there is a clear need for extension.

I have some additional thoughts, but I'm guessing they will shake out when you add Pre/PostAuthorize support.

...ork/security/access/intercept/aopalliance/MethodSecurityAuthorizationManagerInterceptor.java

...ngframework/security/config/annotation/method/configuration/MethodSecurityConfiguration.java

evgeniycheban · 2021-01-25T11:49:17Z

@jzheaux I added support for Pre/PostAuthorize, please take a look. I'm currently covering the code with tests.

jzheaux

Thanks, @evgeniycheban! I've left some additional feedback.

...ccess/expression/method/ExpressionBasedPostMethodInvocationAuthorizationManagerResolver.java

...rg/springframework/security/access/intercept/aopalliance/AuthorizationMethodInterceptor.java

...ccess/expression/method/ExpressionBasedPostMethodInvocationAuthorizationManagerResolver.java

jzheaux

Thanks, @evgeniycheban! I've left my feedback inline.

jzheaux · 2021-02-02T16:09:33Z

core/src/main/java/org/springframework/security/access/method/AfterAuthorizationManager.java

+ *
+ * @author Evgeniy Cheban
+ */
+public interface AfterAuthorizationManager {


I like this interface, though I wonder if a better name is AuthorizationManagerAfterAdvice. The reason for that is that it's clear from the name that it is intercepting and perhaps mutating the return value.

Renamed to AuthorizationManagerAfterAdvice.

...org/springframework/security/access/method/MethodInvocationAuthorizationManagerResolver.java

jzheaux

This looks great, @evgeniycheban! It's really starting to take shape. I've left some feedback inline.

jzheaux · 2021-02-05T18:58:06Z

...amework/security/access/intercept/aopalliance/MethodSecurityAuthorizationManagerAdvisor.java

+			AuthorizationManagerBeforeAdvice<MethodInvocation> beforeAdvice,
+			AuthorizationManagerAfterAdvice<MethodInvocation> afterAdvice) {
+		this.advice = advice;
+		this.beforeAdvice = beforeAdvice;


I think you could instead do:

this.pointcut = Pointcuts.union(this.beforeAdvice, this.afterAdvice);

Then, you can remove the inner class.

jzheaux · 2021-02-05T19:08:01Z

...ngframework/security/config/annotation/method/configuration/MethodSecurityConfiguration.java

+	 * </p>
+	 * @return the {@link MethodSecurityExpressionHandler} to use
+	 */
+	protected MethodSecurityExpressionHandler methodSecurityExpressionHandler() {


I know Spring Security did a lot of this in the past, but I'd like to see if this can be changed to be bean-based instead of based on extending a class.

One way that might work would be to do:

@Autowired(required = false) public void setMethodSecurityExpressionHandler(MethodSecurityExpressionHandler custom) { this.methodSecurityExpressionHandler = custom; }

If successful, then let's also make the class final

The @Configuration class cannot be final because Spring creates a CGLIB proxy for it. I think we could make it just package private.

In fact, setting the proxyBeanMethods attribute to false allows the @Configuration class to be final.

jzheaux · 2021-02-05T19:16:36Z

...ngframework/security/config/annotation/method/configuration/MethodSecurityConfiguration.java

+
+	@Bean
+	@Role(BeanDefinition.ROLE_INFRASTRUCTURE)
+	AuthorizationManagerAfterAdvice<MethodInvocation> authorizationManagerAfterAdvice() {


It might not go into this PR, but it would be nice if an application could do:

@Bean public AuthorizationManagerBeforeAdvice<MethodInvocation> myBeforeAdvice() { // .... } @Bean public AuthorizationManagerAfterAdvice<MethodInvocation> myAfterAdvice() { // ... }

And MethodSecurityConfiguration would take that one instead of its own.

This would be the equivalent of supplying a custom SecurityMetadataSource

jzheaux · 2021-02-05T19:23:02Z

...mework/security/access/expression/method/ExpressionBasedAuthorizationManagerAfterAdvice.java

+ *
+ * @author Evgeniy Cheban
+ */
+public final class ExpressionBasedAuthorizationManagerAfterAdvice


While the class is expression-based, it would be ambiguous if other expression-based ways to give advice were added later on to the framework. We might want to call this PreAnnotationAuthorizationManagerAfterAdvice.

jzheaux · 2021-02-05T19:42:45Z

...a/org/springframework/security/access/method/DelegatingAuthorizationManagerBeforeAdvice.java

+				targetClass);
+		for (AuthorizationManagerBeforeAdvice<MethodAuthorizationContext> delegate : this.delegates) {
+			AuthorizationDecision decision = delegate.check(authentication, methodAuthorizationContext);
+			if (decision != null) {


I think this should match the AffirmativeBased matcher, since that's the default in @EnableGlobalMethodSecurity. It returns the first voter that grants and only denies if one of the voters denied.

Done. In the current version the DelegatingAuthorizationManagerBeforeAdvice returns the first granted AuthorizationDecision and the denied AuthorizationDecision only if one of the DelegatingAuthorizationManagerBeforeAdvices denied. If multiple DelegatingAuthorizationManagerBeforeAdvices denies then the first denied AuthorizationDecision is returned.

jzheaux · 2021-02-05T19:57:51Z

...mework/security/access/expression/method/ExpressionBasedAuthorizationManagerAfterAdvice.java

+	 * Creates an instance.
+	 * @param expressionHandler the {@link MethodSecurityExpressionHandler} to use
+	 */
+	public ExpressionBasedAuthorizationManagerAfterAdvice(MethodSecurityExpressionHandler expressionHandler) {


I'd recommend that this change to a setter. Typically, Spring Security places required parameters in the constructor and optional ones as setters. Since there's a default handler that will work in most cases, using the default constructor will make it more convenient for applications to use.

evgeniycheban · 2021-02-09T00:54:01Z

@jzheaux I've updated the PR according to your comments.

jzheaux

Thanks for your patience, @evgeniycheban, while I got back to you.

I've left some additional feedback inline.

jzheaux · 2021-03-10T22:57:07Z

...rg/springframework/security/access/intercept/aopalliance/AuthorizationMethodInterceptor.java

+	 * @param beforeAdvice the {@link AuthorizationManagerBeforeAdvice} to use
+	 * @param afterAdvice the {@link AuthorizationManagerAfterAdvice} to use
+	 */
+	public AuthorizationMethodInterceptor(AuthorizationManagerBeforeAdvice<MethodInvocation> beforeAdvice,


@rwinch made a good point that it would be nice to see if we can change AuthorizationManagerBeforeAdvice so that it doesn't extend AuthorizationManager.

Two ideas come to mind:

Change this constructor to

public AuthorizationMethodInterceptor(AuthorizationManagerBeforeAdvice<MethodInvocation> beforeAdvice, AuthorizationManager<MethodInvocation> beforeManager, AuthorizationManagerAfterAdvice<MethodInvocation> afterAdvice, AuthorizationManager<MethodInvocation> afterManager)

and then PreAnnotationAuthorizationManagerBeforeAdvice would split into two classes --PreFilterAuthorizationManagerBeforeAdvice and PreAuthorizeAuthorizationManager -- and likewise for PostAnnotationXXX
2. Keep the contract as-is, but split PreAnnotationAuthorizationManagerBeforeAdvice into PreFilterAuthorizationManagerBeforeAdvice and something like MethodMatcherAuthorizationManagerBeforeAdvice. The latter would could have an instance of AuthorizationManager (e.g. PreAuthorizeAuthorizationManager) that it would delegate to if the MethodMatcher matches.

The second seems ideal, but I'm listing both for completeness.

In both cases, it seems like the before advice contract would change to return void since not all advices would return an AuthorizationDecision anymore.

jzheaux

Thanks for the updates, @evgeniycheban!

I've left some additional feedback inline.

Also, we're probably close enough to add @since 5.5 to new classes.

...rg/springframework/security/config/annotation/method/configuration/EnableMethodSecurity.java

jzheaux · 2021-03-30T19:17:36Z

...amework/security/access/intercept/aopalliance/MethodSecurityAuthorizationManagerAdvisor.java

+ *
+ * @author Evgeniy Cheban
+ */
+public final class MethodSecurityAuthorizationManagerAdvisor extends AbstractPointcutAdvisor {


I think this class is no longer needed. DefaultPointcutAdvisor should suffice.

...ngframework/security/config/annotation/method/configuration/MethodSecurityConfiguration.java

jzheaux · 2021-03-30T19:22:44Z

...org/springframework/security/access/expression/method/PostAuthorizeAuthorizationManager.java

+ * limitations under the License.
+ */
+
+package org.springframework.security.access.expression.method;


Let's please move all these classes to org.springframework.security.authorization.method.

jzheaux · 2021-03-30T19:25:14Z

...ava/org/springframework/security/access/method/DelegatingAuthorizationMethodAfterAdvice.java

+ *
+ * @author Evgeniy Cheban
+ */
+public final class DelegatingAuthorizationMethodAfterAdvice


I'm thinking it would be better to have this use MethodAuthorizationContext instead of MethodInvocation. That way, only one MethodAuthorizationContext needs to be generated in AuthorizationMethodInterceptor.

evgeniycheban · 2021-03-31T23:26:39Z

@jzheaux I updated the PR with the JSR-250 security annotations support.

Closes spring-projectsgh-9289

jzheaux · 2021-04-10T00:50:45Z

Thanks again for all the hard work on this PR, @evgeniycheban, it's much appreciated! It's been merged in 20778f7. I also added some polish commits relative to documentation and some small bug fixes.

Finally, since we are close to release time, I polished the AOP structure, changing the advice classes to interceptors. These match very nicely with the surrounding interceptor and remove a number of classes needed to support the previous structure.

spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jan 18, 2021

jzheaux requested changes Jan 21, 2021

View reviewed changes

...ork/security/access/intercept/aopalliance/MethodSecurityAuthorizationManagerInterceptor.java Outdated Show resolved Hide resolved

...ngframework/security/config/annotation/method/configuration/MethodSecurityConfiguration.java Outdated Show resolved Hide resolved

evgeniycheban force-pushed the gh-9289 branch from 2f7ca1f to a2ae4a6 Compare January 25, 2021 11:18

evgeniycheban requested a review from jzheaux January 25, 2021 11:49

evgeniycheban force-pushed the gh-9289 branch from a2ae4a6 to 05a35ee Compare January 25, 2021 12:35

jzheaux requested changes Jan 25, 2021

View reviewed changes

jzheaux self-assigned this Jan 26, 2021

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 Jan 26, 2021

evgeniycheban force-pushed the gh-9289 branch 2 times, most recently from fb11b02 to d0244a5 Compare January 27, 2021 22:26

evgeniycheban requested a review from jzheaux January 27, 2021 22:36

jzheaux requested changes Feb 2, 2021

View reviewed changes

evgeniycheban force-pushed the gh-9289 branch 7 times, most recently from d50798a to 3822746 Compare February 4, 2021 11:00

evgeniycheban requested a review from jzheaux February 4, 2021 11:09

jzheaux requested changes Feb 5, 2021

View reviewed changes

evgeniycheban force-pushed the gh-9289 branch from 3822746 to abc9965 Compare February 8, 2021 21:57

evgeniycheban marked this pull request as ready for review February 8, 2021 21:57

evgeniycheban force-pushed the gh-9289 branch 2 times, most recently from 26a3ab8 to cb62099 Compare February 9, 2021 00:11

evgeniycheban requested a review from jzheaux February 9, 2021 00:31

evgeniycheban force-pushed the gh-9289 branch from cb62099 to af92fd5 Compare February 9, 2021 00:44

evgeniycheban force-pushed the gh-9289 branch 5 times, most recently from a5b27e9 to 2b7e042 Compare February 11, 2021 00:45

This was referenced Feb 23, 2021

Allow to override the ExpressionBasedAnnotationAttributeFactory #9470

Closed

Expose *InvocationExpressionAttribute classes #9206

Closed

jzheaux requested changes Mar 10, 2021

View reviewed changes

evgeniycheban force-pushed the gh-9289 branch 2 times, most recently from 1029660 to 60294fe Compare March 24, 2021 01:15

evgeniycheban requested a review from jzheaux March 24, 2021 01:20

evgeniycheban force-pushed the gh-9289 branch 4 times, most recently from 806d203 to 1689a72 Compare March 29, 2021 11:52

jzheaux requested changes Mar 30, 2021

View reviewed changes

evgeniycheban force-pushed the gh-9289 branch from 1689a72 to 6fc3236 Compare March 31, 2021 23:23

evgeniycheban requested a review from jzheaux March 31, 2021 23:53

evgeniycheban force-pushed the gh-9289 branch from 6fc3236 to 5fd9061 Compare April 1, 2021 00:03

Consider AuthorizationManager for Method Security

00d3e8f

Closes spring-projectsgh-9289

evgeniycheban force-pushed the gh-9289 branch from 5fd9061 to 00d3e8f Compare April 1, 2021 00:10

jzheaux added this to the 5.5.0-RC1 milestone Apr 10, 2021

jzheaux closed this Apr 10, 2021

rwinch changed the title ~~Consider AuthorizationManager for Method Security~~ AuthorizationManager + Method Security Support Apr 12, 2021

rwinch removed this from the 5.5.0-RC1 milestone Apr 12, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AuthorizationManager + Method Security Support #9350

AuthorizationManager + Method Security Support #9350

evgeniycheban commented Jan 18, 2021

jzheaux left a comment

evgeniycheban commented Jan 25, 2021

jzheaux left a comment

jzheaux left a comment

jzheaux Feb 2, 2021

evgeniycheban Feb 4, 2021

jzheaux left a comment

jzheaux Feb 5, 2021

evgeniycheban Feb 9, 2021

jzheaux Feb 5, 2021

jzheaux Feb 5, 2021

evgeniycheban Feb 5, 2021

evgeniycheban Feb 5, 2021

evgeniycheban Feb 9, 2021

jzheaux Feb 5, 2021

evgeniycheban Feb 9, 2021

jzheaux Feb 5, 2021

evgeniycheban Feb 9, 2021

jzheaux Feb 5, 2021

evgeniycheban Feb 9, 2021

jzheaux Feb 5, 2021

evgeniycheban Feb 9, 2021

evgeniycheban commented Feb 9, 2021

jzheaux left a comment

jzheaux Mar 10, 2021

jzheaux left a comment

jzheaux Mar 30, 2021

evgeniycheban Mar 31, 2021

jzheaux Mar 30, 2021

evgeniycheban Mar 31, 2021

jzheaux Mar 30, 2021

evgeniycheban Mar 31, 2021

evgeniycheban commented Mar 31, 2021

jzheaux commented Apr 10, 2021

AuthorizationManager + Method Security Support #9350

AuthorizationManager + Method Security Support #9350

Conversation

evgeniycheban commented Jan 18, 2021

jzheaux left a comment

Choose a reason for hiding this comment

evgeniycheban commented Jan 25, 2021

jzheaux left a comment

Choose a reason for hiding this comment

jzheaux left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jzheaux left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evgeniycheban commented Feb 9, 2021

jzheaux left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jzheaux left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evgeniycheban commented Mar 31, 2021

jzheaux commented Apr 10, 2021