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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
