You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by "michaeljmarshall (via GitHub)" <gi...@apache.org> on 2023/06/09 04:59:36 UTC
[GitHub] [pulsar] michaeljmarshall commented on a diff in pull request #20496: [improve][broker] Support get/remove permissions for AuthorizationProvider
michaeljmarshall commented on code in PR #20496:
URL: https://github.com/apache/pulsar/pull/20496#discussion_r1223837851
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authorization/AuthorizationProvider.java:
##########
@@ -428,4 +429,48 @@ default Boolean allowTopicPolicyOperation(TopicName topicName,
throw new RestException(e.getCause());
}
}
+
+ /**
+ * Remove authorization-action permissions on a topic to the given client.
Review Comment:
Nit: this is not for a given client, but for all clients.
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authorization/AuthorizationProvider.java:
##########
@@ -428,4 +429,48 @@ default Boolean allowTopicPolicyOperation(TopicName topicName,
throw new RestException(e.getCause());
}
}
+
+ /**
+ * Remove authorization-action permissions on a topic to the given client.
+ * @param topicName
+ * @return CompletableFuture<Void>
+ */
+ default CompletableFuture<Void> removePermissionsAsync(TopicName topicName) {
+ return FutureUtil.failedFuture(new IllegalStateException(
+ String.format("removePermissionsAsync on topicName %s is not supported by the Authorization",
+ topicName)));
+ }
+
+ /**
+ * Get authorization-action permissions on a topic to the given client.
+ * @param topicName
+ * @return CompletableFuture<Map<String, Set<AuthAction>>>
+ */
+ default CompletableFuture<Map<String, Set<AuthAction>>> getPermissionsAsync(TopicName topicName) {
Review Comment:
One interesting way we could consider expanding this method is to add the role of the user requesting this operation. As it is currently implemented, the broker will first verify that the role is a tenant admin or a superuser and then will call this method. It seems simpler to defer that kind of checking to the authentication provider. However, we don't do that kind of thing for any of the other methods, so that might not be consistent with the current design. What do you think @Technoboy-?
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authorization/AuthorizationProvider.java:
##########
@@ -428,4 +429,48 @@ default Boolean allowTopicPolicyOperation(TopicName topicName,
throw new RestException(e.getCause());
}
}
+
+ /**
+ * Remove authorization-action permissions on a topic to the given client.
+ * @param topicName
+ * @return CompletableFuture<Void>
+ */
+ default CompletableFuture<Void> removePermissionsAsync(TopicName topicName) {
+ return FutureUtil.failedFuture(new IllegalStateException(
+ String.format("removePermissionsAsync on topicName %s is not supported by the Authorization",
+ topicName)));
+ }
+
+ /**
+ * Get authorization-action permissions on a topic to the given client.
+ * @param topicName
+ * @return CompletableFuture<Map<String, Set<AuthAction>>>
+ */
+ default CompletableFuture<Map<String, Set<AuthAction>>> getPermissionsAsync(TopicName topicName) {
+ return FutureUtil.failedFuture(new IllegalStateException(
+ String.format("getPermissionsAsync on topicName %s is not supported by the Authorization",
+ topicName)));
+ }
+
+ /**
+ * Get authorization-action permissions on a topic to the given client.
Review Comment:
Nit: it's for all clients.
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authorization/AuthorizationProvider.java:
##########
@@ -428,4 +429,48 @@ default Boolean allowTopicPolicyOperation(TopicName topicName,
throw new RestException(e.getCause());
}
}
+
+ /**
+ * Remove authorization-action permissions on a topic to the given client.
+ * @param topicName
+ * @return CompletableFuture<Void>
+ */
+ default CompletableFuture<Void> removePermissionsAsync(TopicName topicName) {
+ return FutureUtil.failedFuture(new IllegalStateException(
+ String.format("removePermissionsAsync on topicName %s is not supported by the Authorization",
+ topicName)));
+ }
+
+ /**
+ * Get authorization-action permissions on a topic to the given client.
+ * @param topicName
+ * @return CompletableFuture<Map<String, Set<AuthAction>>>
+ */
+ default CompletableFuture<Map<String, Set<AuthAction>>> getPermissionsAsync(TopicName topicName) {
+ return FutureUtil.failedFuture(new IllegalStateException(
+ String.format("getPermissionsAsync on topicName %s is not supported by the Authorization",
+ topicName)));
+ }
+
+ /**
+ * Get authorization-action permissions on a topic to the given client.
+ * @param namespaceName
+ * @return CompletableFuture<Map<String, Set<String>>>
+ */
+ default CompletableFuture<Map<String, Set<String>>> getSubscriptionPermissionsAsync(NamespaceName namespaceName) {
+ return FutureUtil.failedFuture(new IllegalStateException(
+ String.format("getSubscriptionPermissionsAsync on namespace %s is not supported by the Authorization",
+ namespaceName)));
+ }
+
+ /**
+ * Get authorization-action permissions on a namespace to the given client.
Review Comment:
Nit: for all clients.
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/transaction/buffer/TransactionBufferCloseTest.java:
##########
@@ -61,7 +61,7 @@ public Object[][] isPartition() {
};
}
- @Test(timeOut = 10_000, dataProvider = "isPartition")
+ @Test(dataProvider = "isPartition")
Review Comment:
This seems like it belongs in a separate PR.
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authorization/AuthorizationProvider.java:
##########
@@ -428,4 +429,48 @@ default Boolean allowTopicPolicyOperation(TopicName topicName,
throw new RestException(e.getCause());
}
}
+
+ /**
+ * Remove authorization-action permissions on a topic to the given client.
+ * @param topicName
+ * @return CompletableFuture<Void>
+ */
+ default CompletableFuture<Void> removePermissionsAsync(TopicName topicName) {
+ return FutureUtil.failedFuture(new IllegalStateException(
+ String.format("removePermissionsAsync on topicName %s is not supported by the Authorization",
+ topicName)));
+ }
+
+ /**
+ * Get authorization-action permissions on a topic to the given client.
Review Comment:
Nit: this is the permission for the whole topic, not just for a given client.
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java:
##########
@@ -843,16 +813,9 @@ private CompletableFuture<Void> internalRemovePartitionsTopicNoAutocreationDisab
}).collect(Collectors.toList()));
}
- private CompletableFuture<Void> internalRemovePartitionsAuthenticationPoliciesAsync(int numPartitions) {
+ private CompletableFuture<Void> internalRemovePartitionsAuthenticationPoliciesAsync() {
Review Comment:
What is the motivation for removing the partition logic here? I don't think we can do that here, but I might be missing some context.
My understanding is that we need to remove permission for the parent and all of its sub partitions.
--
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