[GitHub] [pulsar] michaeljmarshall commented on a change in pull request #13090: [Authorization] Optimize the logic of allowing namespace operation

[GitBox <gi...@apache.org>]

michaeljmarshall commented on a change in pull request #13090:
URL: https://github.com/apache/pulsar/pull/13090#discussion_r762118069



##########
File path: pulsar-common/src/main/java/org/apache/pulsar/common/policies/data/NamespaceOperation.java
##########
@@ -24,7 +24,6 @@
  */
 public enum NamespaceOperation {
     CREATE_TOPIC,
-    GET_TOPIC,

Review comment:
       Technically, this is a breaking change, and I don't think we should remove the value just yet. Even though it isn't used by the project, extensions of the `AuthorizationProvider` might reference it, and removing the value from the enum would break those implementations. If we think there is no use case for the value, perhaps we could somehow mark it as deprecated and eventually remove it.
   
   @KannarFr - is there a place where the `GET_TOPIC` value was supposed to be used?

##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authorization/AuthorizationProvider.java
##########
@@ -183,6 +183,16 @@ default void initialize(ServiceConfiguration conf, PulsarResources pulsarResourc
     CompletableFuture<Boolean> allowSinkOpsAsync(NamespaceName namespaceName, String role,
                                                  AuthenticationDataSource authenticationData);
 
+    /**
+     * Allow package operations with in this namespace
+     * @param namespaceName The namespace that the package operations can be executed in
+     * @param role The role to check
+     * @param authenticationData authentication data related to the role
+     * @return a boolean to determine whether authorized or not
+     */
+    CompletableFuture<Boolean> allowPackageOpsAsync(NamespaceName namespaceName, String role,

Review comment:
       @yuruguo - can you explain why we need this method in the interface? It is only used internally by the `PulsarAuthorizationProvider` and `MultiRolesTokenAuthorizationProvider`, so I don' think we need to add it to the public interface.




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