[GitHub] [pulsar] michaeljmarshall commented on a diff in pull request #20145: [fix][broker] Move superuser/tenantAdmin checks to AuthorizationService from PulsarAuthorizationProvider

["michaeljmarshall (via GitHub)" <gi...@apache.org>]

michaeljmarshall commented on code in PR #20145:
URL: https://github.com/apache/pulsar/pull/20145#discussion_r1171931338


##########
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authorization/AuthorizationService.java:
##########
@@ -183,7 +183,15 @@ public CompletableFuture<Boolean> canProduceAsync(TopicName topicName, String ro
         if (!this.conf.isAuthorizationEnabled()) {
             return CompletableFuture.completedFuture(true);
         }
-        return provider.allowTopicOperationAsync(topicName, role, TopicOperation.PRODUCE, authenticationData);
+
+        return isSuperUserOrAdmin(topicName.getTenant(), role, authenticationData)

Review Comment:
   I do not think we should move in this direction. I think our target design should be to let the `AuthorizationProvider` implementation decide what level of permission is required. This line here removes that abstraction from the `AuthorizationProvider` and puts it in the `AuthorizationService`. I concede that there are some places where we use these checks outside of the `AuthorizationProvider`, however, those are legacy details that we have to support until we find a way to move them into the `AuthorizationProvider`.



##########
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authorization/PulsarAuthorizationProvider.java:
##########
@@ -451,42 +448,31 @@ public CompletableFuture<Boolean> allowNamespaceOperationAsync(NamespaceName nam
             log.debug("Check allowNamespaceOperationAsync [{}] on [{}].", operation.name(), namespaceName);
         }
 
-        return validateTenantAdminAccess(namespaceName.getTenant(), role, authData)

Review Comment:
   I think this call rightly belongs in the `AuthorizationProvider` impelmentation.



-- 
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-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org