AuthorizationManager should return AuthorizationResult #14846

CrazyParanoid · 2024-04-04T20:32:54Z

Added a new authorization method to AuthorizationManager that returns AuthorizationResult.

jzheaux

With check deprecated, we should no longer call it in from other non-deprecated production code in Spring Security. Can you also make those corresponding changes? Please leave tests as-is, though.

Also, please add tests to ensure that the new method works.

Finally, please take a look at AuthorizaitonManagerBeforeMethodInterceptor and other method interceptors to ensure that they are no longer casting expression values to AuthorizationDecision. Instead, they should implement the authorize method, have their check method call it, and then perform the cast there.

CrazyParanoid · 2024-04-10T14:21:47Z

Hi @jzheaux ! I added the authorize method to the AuthorizationManager and made the default implementation of the check method:

default AuthorizationDecision check(Supplier<Authentication> authentication, T object) {
		return (AuthorizationDecision) authorize(authentication, object);
	}

With this implementation, it will not be possible to leave the tests as is, because in some tests, a mock of the check method is made and its invocation is verified:

verify(this.mockAuthorizationManager).check(any(), any());

And it needs to be changed to authorize, since the mock was made for example:

given(this.mockAuthorizationManager.check(any(), any())).willReturn(new AuthorizationDecision(true));

In addition, there are several delegating components that make call check method of delegate. Here you need to mock the authorize and verify its invocation.

jzheaux

Thanks for the updates and for your patience as I got back to this PR, @CrazyParanoid. I've left my feedback inilne.

jzheaux · 2024-09-17T21:28:56Z

core/src/main/java/org/springframework/security/authorization/AuthorizationManager.java

+	 * @since 6.3
+	 */
+	@Nullable
+	AuthorizationResult authorize(Supplier<Authentication> authentication, T object);


Sorry, I may have misspoken here. We can't introduce a new non-default method to an existing interface as that would not be passive. Will you please switch this so that authorize has the default implementation?

jzheaux · 2024-09-17T21:32:21Z

...rc/main/java/org/springframework/security/authorization/AuthorizationObservationContext.java

 	 */
-	public AuthorizationDecision getDecision() {
+	public AuthorizationResult getDecision() {


To remain binary compatible, we cannot change return types. Instead, please add a new getter (called getAuthorizationResult) and deprecate this getter.

jzheaux · 2024-09-17T21:32:43Z

...rc/main/java/org/springframework/security/authorization/AuthorizationObservationContext.java

 	 */
-	public void setDecision(AuthorizationDecision decision) {
+	public void setDecision(AuthorizationResult decision) {


To remain binary compatible, we cannot change return types. Instead, please add a new setter (called setAuthorizationResult) and deprecate this setter.

jzheaux · 2024-09-17T21:34:03Z

...rc/main/java/org/springframework/security/authorization/ObservationAuthorizationManager.java

@@ -69,16 +69,16 @@ public ObservationAuthorizationManager(ObservationRegistry registry, Authorizati
 	}

 	@Override
-	public AuthorizationDecision check(Supplier<Authentication> authentication, T object) {


I believe once you change to authorize being the default method, you will be fine to leave implementations as they are.

jzheaux · 2024-09-17T21:36:35Z

...ringframework/security/authorization/method/AuthorizationManagerBeforeMethodInterceptor.java

-		AuthorizationDecision decision = this.authorizationManager.check(this::getAuthentication, mi);
-		this.eventPublisher.publishAuthorizationEvent(this::getAuthentication, mi, decision);
+		AuthorizationResult decision = this.authorizationManager.authorize(this::getAuthentication, mi);
+		this.eventPublisher.publishAuthorizationEvent(this::getAuthentication, mi, (AuthorizationDecision) decision);


Let's consider also adding a publishAuthorizationEvent implementation that uses AuthorizeResult.

I can add a new method:

default <T> void publishAuthorizationEvent(Supplier<Authentication> authentication, T object, AuthorizationResult decision) { publishAuthorizationEvent(authentication, object,(AuthorizationDecision) decision); }

Method with AuthorizationDecision semantics can be declared as deprecated. This will not break backward compatibility with existing AuthorizationEventPublisher implementations

@jzheaux I suggest adding an AuthorizationEventPublisher implementation that will not publish events, like NoopAuthorizationEventPublisher:

public final class NoopAuthorizationEventPublisher implements AuthorizationEventPublisher { @Override public <T> void publishAuthorizationEvent(Supplier<Authentication> authentication, T object, AuthorizationDecision decision) { } @Override public <T> void publishAuthorizationEvent(Supplier<Authentication> authentication, T object, AuthorizationResult decision) { } }

AuthorizationEventPublisher is not marked as a functional interface, but in fact it is one. AuthorizationManagerAfterMethodInterceptor and AuthorizationFilter use noPublish method to implement the publisher. This means that calling this.eventPublisher.publishAuthorizationEvent(this::getAuthentication, request, decision); will require the AuthorizationDecision type explicitly. At the same time, we cannot remove the publishAuthorizationEvent method with AuthorizationDecision semantic. Therefore, it makes sense to replace eventPublisher in these components with NoopAuthorizationEventPublisher.

jzheaux · 2024-09-17T21:43:37Z

@CrazyParanoid, also if you have time, once we are aligned on the servlet changes, it would be great if the PR could have the reactive bits as well. I'm happy to add a polish for that if needed.

CrazyParanoid · 2024-09-19T09:51:07Z

Hi @jzheaux. Thanks for your feedback! I will complete this issue in the next few days.

Closes spring-projectsgh-14843

jzheaux

Looking good, @CrazyParanoid, we're nearly there. I've left some additional feedback inline.

jzheaux · 2024-10-01T00:20:52Z

core/src/main/java/org/springframework/security/authorization/AuthorizationEventPublisher.java

+	 */
+	default <T> void publishAuthorizationEvent(Supplier<Authentication> authentication, T object,
+			AuthorizationResult decision) {
+		publishAuthorizationEvent(authentication, object,(AuthorizationDecision) decision);


Even though it's a default implementation, we should perform a type-safe check. Maybe you could do the following:

if (!(result instanceof AuthorizationDecision decision)) { throw new UnsupportedOperationException("result must be of type AuthorizationDecision"); } publishAuthorizationEvent(authentication, object, decision);

jzheaux · 2024-10-01T00:21:41Z

...rc/main/java/org/springframework/security/authorization/AuthorizationObservationContext.java

@@ -73,15 +73,32 @@ public T getObject() {
 	 * @return the observed {@link AuthorizationDecision}
 	 */
 	public AuthorizationDecision getDecision() {


Let's deprecate this as well.

jzheaux · 2024-10-01T00:21:57Z

...rc/main/java/org/springframework/security/authorization/AuthorizationObservationContext.java

@@ -73,15 +73,32 @@ public T getObject() {
 	 * @return the observed {@link AuthorizationDecision}
 	 */
 	public AuthorizationDecision getDecision() {
-		return this.decision;
+		return (AuthorizationDecision) this.authorizationResult;
 	}

 	/**
 	 * Set the observed {@link AuthorizationDecision}
 	 * @param decision the observed {@link AuthorizationDecision}
 	 */
 	public void setDecision(AuthorizationDecision decision) {


Let's deprecate this as well

jzheaux · 2024-10-01T00:24:15Z

...rc/main/java/org/springframework/security/authorization/AuthorizationObservationContext.java

@@ -73,15 +73,32 @@ public T getObject() {
 	 * @return the observed {@link AuthorizationDecision}
 	 */
 	public AuthorizationDecision getDecision() {
-		return this.decision;
+		return (AuthorizationDecision) this.authorizationResult;


Please perform a type check here. The reason is so that the application can give a more informative error than ClassCastException. Maybe something like:

Assert.isInstanceOf(AuthorizationDecision.class, "please call getAuthorizationResult instead. If you must call getDecision, please ensure that the result you provide is of type AuthorizationDecision"); return (AuthorizationDecision) this.authorizationResult;

jzheaux · 2024-10-01T00:26:00Z

...rc/main/java/org/springframework/security/authorization/NoopAuthorizationEventPublisher.java

+ * @author Max Batischev
+ * @since 6.4
+ */
+public final class NoopAuthorizationEventPublisher implements AuthorizationEventPublisher {


I'd rather not introduce this in this PR. If you are able to make this package-private, I don't mind leaving it. Otherwise, let's stick with private inner classes for now, even though that's a bit of duplication.

jzheaux · 2024-10-01T00:31:18Z

...ramework/security/authorization/method/AuthorizationManagerBeforeMethodInterceptorTests.java

@@ -72,7 +72,7 @@ public void beforeWhenMockAuthorizationManagerThenCheck() throws Throwable {
 		AuthorizationManagerBeforeMethodInterceptor advice = new AuthorizationManagerBeforeMethodInterceptor(
 				Pointcut.TRUE, mockAuthorizationManager);
 		advice.invoke(mockMethodInvocation);
-		verify(mockAuthorizationManager).check(any(Supplier.class), eq(mockMethodInvocation));
+		verify(mockAuthorizationManager).authorize(any(Supplier.class), eq(mockMethodInvocation));


I'd prefer these change to have Mockito call the real authorize method, thus confirming that folks upgrading can have the same expectations. For example:

given(mockAuthorizationManager.authorize(any(), any()).willCallRealMethod();

Then, this last line in the test shouldn't have to change.

Please see if this strategy works and, if so, apply it to the remaining tests so that their verification steps stay the same.

jzheaux · 2024-10-01T00:34:31Z

Additionally, the build appears to be failing due to formatting concerns. Please try running from the command line the following:

./gradlew format && ./gradlew checkstyleMain checkstyleTest

This will correct what it can and then give you a report of the things that you need to change manually from a code convention standpoint.

spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Apr 4, 2024

jzheaux requested changes Apr 5, 2024

View reviewed changes

jzheaux self-assigned this Apr 5, 2024

CrazyParanoid force-pushed the gh-14843 branch 3 times, most recently from a368cd1 to 8ee5742 Compare April 7, 2024 15:33

CrazyParanoid requested a review from jzheaux April 10, 2024 14:22

jzheaux changed the title ~~Add support AuthorizationResult for AuthorizationManager~~ AuthorizationManager should return AuthorizationResult Apr 17, 2024

jzheaux requested changes Sep 17, 2024

View reviewed changes

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 Sep 24, 2024

Add AuthorizationResult Support to AuthorizationManager

e014a6a

Closes spring-projectsgh-14843

CrazyParanoid force-pushed the gh-14843 branch from 8ee5742 to e014a6a Compare September 26, 2024 22:16

jzheaux requested changes Oct 1, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AuthorizationManager should return AuthorizationResult #14846

AuthorizationManager should return AuthorizationResult #14846

CrazyParanoid commented Apr 4, 2024

jzheaux left a comment

CrazyParanoid commented Apr 10, 2024

jzheaux left a comment

jzheaux Sep 17, 2024

jzheaux Sep 17, 2024

jzheaux Sep 17, 2024

jzheaux Sep 17, 2024

jzheaux Sep 17, 2024

CrazyParanoid Sep 23, 2024

CrazyParanoid Sep 25, 2024

jzheaux commented Sep 17, 2024

CrazyParanoid commented Sep 19, 2024

jzheaux left a comment

jzheaux Oct 1, 2024

jzheaux Oct 1, 2024

jzheaux Oct 1, 2024

jzheaux Oct 1, 2024

jzheaux Oct 1, 2024

jzheaux Oct 1, 2024

jzheaux commented Oct 1, 2024

AuthorizationManager should return AuthorizationResult #14846

Are you sure you want to change the base?

AuthorizationManager should return AuthorizationResult #14846

Conversation

CrazyParanoid commented Apr 4, 2024

jzheaux left a comment

Choose a reason for hiding this comment

CrazyParanoid commented Apr 10, 2024

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

jzheaux commented Sep 17, 2024

CrazyParanoid commented Sep 19, 2024

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

jzheaux commented Oct 1, 2024