michaeljmarshall commented on code in PR #20478:
URL: https://github.com/apache/pulsar/pull/20478#discussion_r1214791647
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authorization/PulsarAuthorizationProvider.java:
##########
@@ -263,7 +263,10 @@ public CompletableFuture<Void>
revokePermissionAsync(TopicName topicName, String
policies.auth_policies.getTopicAuthentication()
.computeIfPresent(topicName.toString(), (k, v)
-> {
v.remove(role);
- return null;
+ if (v.isEmpty()) {
+ return null;
+ }
+ return v;
Review Comment:
Was this regression caught by one of the tests? If not, we should add a test
to make sure this can't happen again.
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authorization/PulsarAuthorizationProvider.java:
##########
@@ -263,7 +263,10 @@ public CompletableFuture<Void>
revokePermissionAsync(TopicName topicName, String
policies.auth_policies.getTopicAuthentication()
.computeIfPresent(topicName.toString(), (k, v)
-> {
v.remove(role);
- return null;
+ if (v.isEmpty()) {
Review Comment:
I would be helpful to keep the meaningful variable names for `k` and `v`
here, like the `PersistentTopicsBase` has.
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java:
##########
@@ -319,59 +319,25 @@ protected void internalGrantPermissionsOnTopic(final
AsyncResponse asyncResponse
});
}
- private CompletableFuture<Void> revokePermissionsAsync(String topicUri,
String role, boolean force) {
- return
namespaceResources().getPoliciesAsync(namespaceName).thenCompose(
- policiesOptional -> {
- Policies policies = policiesOptional.orElseThrow(() ->
- new RestException(Status.NOT_FOUND, "Namespace
does not exist"));
- if
(!policies.auth_policies.getTopicAuthentication().containsKey(topicUri)
- ||
!policies.auth_policies.getTopicAuthentication().get(topicUri).containsKey(role))
{
- log.warn("[{}] Failed to revoke permission from role
{} on topic: Not set at topic level {}",
- clientAppId(), role, topicUri);
- if (force) {
- return CompletableFuture.completedFuture(null);
- } else {
- return FutureUtil.failedFuture(new
RestException(Status.PRECONDITION_FAILED,
- "Permissions are not set at the topic
level"));
- }
- }
Review Comment:
What is the justification for removing the check that the policies exist?
This appears to remove the need for the `force` argument.
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/NamespacesBase.java:
##########
@@ -650,29 +650,18 @@ protected CompletableFuture<Void>
internalGrantPermissionOnSubscriptionAsync(Str
protected CompletableFuture<Void>
internalRevokePermissionsOnNamespaceAsync(String role) {
return validateNamespaceOperationAsync(namespaceName,
NamespaceOperation.REVOKE_PERMISSION)
.thenAccept(__ -> checkNotNull(role, "Role should not be
null"))
- .thenCompose(__ -> validatePoliciesReadOnlyAccessAsync())
- .thenCompose(__ -> updatePoliciesAsync(namespaceName, policies
-> {
-
policies.auth_policies.getNamespaceAuthentication().remove(role);
- return policies;
- }));
+ .thenCompose(__ ->
getAuthorizationService().revokePermissionAsync(namespaceName, role));
}
protected CompletableFuture<Void>
internalRevokePermissionsOnSubscriptionAsync(String subscriptionName,
String role) {
- AuthorizationService authService =
pulsar().getBrokerService().getAuthorizationService();
- if (null != authService) {
Review Comment:
Why are we removing this check? There are still 2 other references in this
file where we fail if the authorization service is null. Seems like we should
maintain a consistent design and either keep it or remove it completely. I'd
prefer to make the failure clearer and fail right explicitly instead of due to
an NPE.
--
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]
