Add SingleResultAuthorizationManager #16612
Conversation
Closes spring-projectsgh-16590 Signed-off-by: Max Batischev <[email protected]>
spring-projects-issues
added
the
status: waiting-for-triage
|
Hey @plll0123, we invite you to join the review so that your interest in contributions does not disappear :) |
There was a problem hiding this comment.
Thanks again, @franticticktick! I've left my feedback inline. Note that most if it is based on the premise that I think it class should hold an AuthorizationResult member variable, set in the constructor. This will give this class many more uses.
| * @param <C> | ||
| * @return permit-all {@link AuthorizationManager} instance | ||
| */ | ||
| public static <C> AuthorizationManager<C> PERMIT_ALL() { |
There was a problem hiding this comment.
Spring Security conventionally uses camelCase for methods, even ones that return a constant; please change this to permitAll().
| * @param <C> | ||
| * @return deny-all {@link AuthorizationManager} instance | ||
| */ | ||
| public static <C> AuthorizationManager<C> DENY_ALL() { |
There was a problem hiding this comment.
Spring Security conventionally uses camelCase for methods, even ones that return a constant; please change this to denyAll().
| * @param <C> | ||
| * @return deny-all {@link AuthorizationManager} instance | ||
| */ | ||
| public static <C> AuthorizationManager<C> DENY_ALL() { |
There was a problem hiding this comment.
I think this should return the concrete type. Just like constructors emit the concrete type. Can you please change both method to return SingleResultAuthorizationManager?
There was a problem hiding this comment.
No, we can't do this, java uses LambdaMetafactory for lambda creation. This factory creates CallSite object, which returns intenface implementation. Therefore these methods must return an interface.
There was a problem hiding this comment.
Sorry, you can ignore this comment :)
| return (a, o) -> DENY; | ||
| } | ||
|
|
||
| private SingleResultAuthorizationManager() { |
There was a problem hiding this comment.
While having this for permitAll and denyAll is great, we can expand it by taking an AuthorizationResult in the constructor and having it be public.
| * @return deny-all {@link AuthorizationManager} instance | ||
| */ | ||
| public static <C> AuthorizationManager<C> DENY_ALL() { | ||
| return (a, o) -> DENY; |
There was a problem hiding this comment.
Instead of a lambda, consider returning a static final instance of SingleResultAuthorizationManager.
|
|
||
| @Override | ||
| public AuthorizationDecision check(Supplier<Authentication> authentication, C object) { | ||
| throw new UnsupportedOperationException("Not supported"); |
There was a problem hiding this comment.
Let's still implement this. If the private member result isn't of type AuthenticationDecision, we can throw and exception.
| } | ||
|
|
||
| @Override | ||
| public AuthorizationDecision check(Supplier<Authentication> authentication, C object) { |
There was a problem hiding this comment.
Let's please implement authorize, returning whatever single result was set in the constructor.
| * @author Max Batischev | ||
| * @since 6.5 | ||
| */ | ||
| public final class SingleResultAuthorizationManager<C> implements AuthorizationManager<C> { |
There was a problem hiding this comment.
Please add tests for the class.
jzheaux
changed the title
|
Hi @jzheaux, thanks for your feedback. There are several points that need to be discussed. We can add a static method, for example This is only possible if the Generics are not available in a static context, which forces us to use a raw type. I'm not sure if this is the best pattern in this case, @jzheaux what do you think about it? |
|
@franticticktick good questions. First, I think it's okay to construct a However, since the implementation knows that it won't use the type, then it can safely use private static final SingleResultAuthorizationManager<?> DENY = new SingleResultAuthorizationManager<>(new AuthorizationDecision(false));That does require a cast in the static method, but again in practice this isn't a concern since this implementation never uses the type at runtime: public static <T> SingleResultAuthorizationManager<T> denyAll() {
return (SingleResultAuthorizationManager<T>) DENY;
} |
Closes gh-16590