[improve][authorization] Uniformly use allowTopicOperationAsync to check permissions #19461
Conversation
…eck permissions Signed-off-by: Zixuan Liu <nodeces@gmail.com>
nodece
force-pushed
the
cleanup-check-topic-permission
branch
from
5b441c5 to
897daba
Compare
michaeljmarshall
left a comment
michaeljmarshall
left a comment
There was a problem hiding this comment.
Great proposal! I think this is a good direction for Pulsar's authorization framework. I will need to come back to this tomorrow or Monday to give it a more thorough review.
|
The pr had no activity for 30 days, mark with Stale label. |
|
ping @michaeljmarshall, do you have time to review this PR? |
michaeljmarshall
left a comment
michaeljmarshall
left a comment
There was a problem hiding this comment.
@nodece - I completely agree that we should clean up these abstractions and make it easier for contributors and extension maintainers to know how to integrate with the AuthorizationService. I left some initial feedback. Let me know what you think. Thanks!
| default CompletableFuture<Boolean> canLookupAsync(TopicName topicName, String role, | ||
| AuthenticationDataSource authenticationData) { | ||
| return CompletableFuture.completedFuture(false); | ||
| } |
There was a problem hiding this comment.
Instead of providing a default implementation, I think we should just deprecate these methods. Then, in the PulsarAuthorizationProvider class, we can remove the @Override annotation and those methods become internal implementation details instead of top level methods on the interface.
| return FutureUtil.failedFuture( | ||
| new IllegalStateException("TopicOperation [" + operation.name() + "] is not supported by the Authorization" | ||
| + "provider you are using.")); |
There was a problem hiding this comment.
Are you able to explain a bit more why this is required for backwards compatibility? I think we should be able to leave this here and leave the switch statement in the PulsarAuthorizationProvider.
| if (isSuperUser) { | ||
| return CompletableFuture.completedFuture(true); | ||
| } else { | ||
| return provider.canLookupAsync(topicName, role, authenticationData); | ||
| return provider.allowTopicOperationAsync(topicName, role, TopicOperation.LOOKUP, authenticationData); |
There was a problem hiding this comment.
This is only tangentially related to your PR, but it seems like a broken abstraction that the AuthorizationService decides that the superuser privilege supersedes the AuthenticationProvider implementaiton. Instead, I think we could just call provider.allowTopicOperationAsync(topicName, role, TopicOperation.LOOKUP, authenticationData);, as you've done here.
My goal is to move that logic into the provider so that plugin implementations do not need to deal with the complexity of when to check for super user and when not to. All of this logic should be delegated to the provider, in my opinion.
That change is probably worth its own PR, though.
| return provider.canConsumeAsync(topicName, role, authenticationData, subscription); | ||
| AuthenticationDataSource subscriptionDataSource; | ||
| if (authenticationData instanceof AuthenticationDataSubscription) { | ||
| subscriptionDataSource = authenticationData; | ||
| } else { | ||
| subscriptionDataSource = new AuthenticationDataSubscription(authenticationData, subscription); | ||
| } | ||
| return provider.allowTopicOperationAsync(topicName, role, TopicOperation.CONSUME, | ||
| subscriptionDataSource); |
There was a problem hiding this comment.
Can we deprecate this canConsumeAsync method? It seems like the same kind of logic that we're deprecating in the AuthorizationProvider.
|
The pr had no activity for 30 days, mark with Stale label. |
5 participants
Motivation
#6428 adds
allowTopicOperationAsync()to check the topic authz abilities, it is a good solution, but this PR doesn't deprecate the old methods:When users customize their own
AuthorizationProvider, they will be confused, so I decided to deprecate the old methods.Modifications
canProduceAsync,canConsumeAsync, andcanLookupAsyncin theAuthorizationProvider, and add a default implementationallowTopicOperationAsyncto call the deprecated methodallowTopicOperationAsyncto check permissions in theAuthorizationServiceDocumentation
docdoc-requireddoc-not-neededdoc-complete