AuthorizationManager should return AuthorizationResult

[franticticktick]

Added a new authorization method to AuthorizationManager that returns AuthorizationResult.

Closes gh-14843

[jzheaux]

Thanks, @CrazyParanoid.

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.

[franticticktick]

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]

@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.

[franticticktick]

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

[jzheaux]

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

[jzheaux]

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.

[franticticktick]

Hi @jzheaux , thanks for your feedback! I'll get back to working on this issue soon. But I'm still a little worried about the correctness of the chosen solution - if the authorize method is marked as default, it turns out that all lambdas will implement the check method, is this a problem and how easy will it be to switch all lambdas to the authorize method?

[jzheaux]

Good point, @CrazyParanoid, though I think this is something we'll have to take in stride. We don't want to break folks when they upgrade, which is what is driving the decision to mark authorize as default.

I believe the main consequence is that they will not be able to use a lambda and a custom implementation of AuthorizationResult together. Since that feature isn't in Spring Security yet, no one has lost anything.

Either way, it should not be an issue for very long, given that Spring Security 7 will be released next fall where we can remove the check method.

How well does that address your concern?

[franticticktick]

I'm not sure if this is type safe. For example, we started using authorize instead of check and get AuthrorizationResult. In this case, eventPublisher is implemented as a noPublish lambda, this lambda requires an object of type AuthorizationDecision, but there is no guarantee that authorize will return exactly AuthorizationDecision, it can return any AuthrorizationResult. Which will immediately result in an error in noPublish.

[franticticktick]

This concerns the publisher, I think that, as you said, it would be better to open another ticket for NoOpAuthorizationEventPublisher.

[jzheaux]

I agree that the no-publish lambda may not be able to stay. A concrete implementation will likely improve readability as well. It just needs to remain package-private or private (inline) for now.

[jzheaux]

Nice, @CrazyParanoid! This for all this work. I ended up polishing the event support a bit more than expected, so I opened a separate ticket and adjusted the commits accordingly. I also added some polish, largely to remove deprecated usages and references to things like AuthorizationResult decision.

This is now merged into main.