michaeljmarshall commented on code in PR #19461:
URL: https://github.com/apache/pulsar/pull/19461#discussion_r1167295009
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authorization/AuthorizationProvider.java:
##########
@@ -129,11 +142,15 @@ CompletableFuture<Boolean> canConsumeAsync(TopicName
topicName, String role,
*
* @param topicName
* @param role
- * @return
- * @throws Exception
+ *
+ * @deprecated
+ * Use {@link #allowTopicOperationAsync(TopicName, String, TopicOperation,
AuthenticationDataSource)} instead.
*/
- CompletableFuture<Boolean> canLookupAsync(TopicName topicName, String role,
- AuthenticationDataSource authenticationData);
+ @Deprecated
+ default CompletableFuture<Boolean> canLookupAsync(TopicName topicName,
String role,
+ AuthenticationDataSource authenticationData) {
+ return CompletableFuture.completedFuture(false);
+ }
Review 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.
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authorization/AuthorizationProvider.java:
##########
@@ -345,9 +362,41 @@ default CompletableFuture<Boolean>
allowTopicOperationAsync(TopicName topic,
String role,
TopicOperation
operation,
AuthenticationDataSource authData) {
- return FutureUtil.failedFuture(
- new IllegalStateException("TopicOperation [" + operation.name() +
"] is not supported by the Authorization"
- + "provider you are using."));
Review 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`.
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authorization/AuthorizationService.java:
##########
@@ -273,7 +281,7 @@ public CompletableFuture<Boolean> canLookupAsync(TopicName
topicName, String rol
if (isSuperUser) {
return CompletableFuture.completedFuture(true);
} else {
- return provider.canLookupAsync(topicName, role,
authenticationData);
+ return provider.allowTopicOperationAsync(topicName, role,
TopicOperation.LOOKUP, authenticationData);
Review 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.
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authorization/AuthorizationService.java:
##########
@@ -191,7 +192,14 @@ public CompletableFuture<Boolean>
canConsumeAsync(TopicName topicName, String ro
if (isSuperUser) {
return CompletableFuture.completedFuture(true);
} else {
- 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);
Review Comment:
Can we deprecate this `canConsumeAsync` method? It seems like the same kind
of logic that we're deprecating in the `AuthorizationProvider`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
