Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Authorization] Role with namespace produce authz can also get topics #13773

Merged
merged 1 commit into from
Mar 2, 2022
Merged

[Authorization] Role with namespace produce authz can also get topics #13773

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
35 changes: 35 additions & 0 deletions ...mon/src/main/java/org/apache/pulsar/broker/authorization/PulsarAuthorizationProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -229,6 +229,40 @@ public CompletableFuture<Boolean> allowSinkOpsAsync(NamespaceName namespaceName,
return allowTheSpecifiedActionOpsAsync(namespaceName, role, authenticationData, AuthAction.sinks);
}

private CompletableFuture<Boolean> allowConsumeOrProduceOpsAsync(NamespaceName namespaceName,
String role,
AuthenticationDataSource authenticationData) {
CompletableFuture<Boolean> finalResult = new CompletableFuture<>();
allowTheSpecifiedActionOpsAsync(namespaceName, role, authenticationData, AuthAction.consume)
.whenComplete((consumeAuthorized, e) -> {
if (e == null) {
if (consumeAuthorized) {
finalResult.complete(consumeAuthorized);
return;
}
} else {
if (log.isDebugEnabled()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we cannot let pass every exception
we should fall back to checking the second action only in case of missing authentication, otherwise if there is a system error (system overloaded?) we are going to generate the error twice

Copy link
Contributor Author

@yuruguo yuruguo Jan 21, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we cannot let pass every exception

If there is an exception or error when checking the first action and we end the subsequent checks, so even if the user has the second permission and it will not take effect. Does this not meet expectations?

we should fall back to checking the second action only in case of missing authentication

I traced the implementation of allowTheSpecifiedActionOpsAsync and not found exceptions related to authentication, so I can't tell whether to perform the second check.

In addition, the implementation logic of this PR is similar canLookupAsync, as follows:

pulsar/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authorization/PulsarAuthorizationProvider.java

Lines 179 to 211 in d972990

public CompletableFuture<Boolean> canLookupAsync(TopicName topicName, String role,
AuthenticationDataSource authenticationData) {
CompletableFuture<Boolean> finalResult = new CompletableFuture<Boolean>();
canProduceAsync(topicName, role, authenticationData).whenComplete((produceAuthorized, ex) -> {
if (ex == null) {
if (produceAuthorized) {
finalResult.complete(produceAuthorized);
return;
}
} else {
if (log.isDebugEnabled()) {
log.debug(
"Topic [{}] Role [{}] exception occurred while trying to check Produce permissions. {}",
topicName.toString(), role, ex.getMessage());
}
}
canConsumeAsync(topicName, role, authenticationData, null).whenComplete((consumeAuthorized, e)
-> {
if (e == null) {
finalResult.complete(consumeAuthorized);
} else {
if (log.isDebugEnabled()) {
log.debug(
"Topic [{}] Role [{}] exception occurred while trying to check Consume permissions. {}",
topicName.toString(), role, e.getMessage());
}
finalResult.completeExceptionally(e);
}
});
});
return finalResult;
}

log.debug("Namespace [{}] Role [{}] exception occurred while trying to check Consume "
+ "permission. {}", namespaceName, role, e.getCause());
}
}
allowTheSpecifiedActionOpsAsync(namespaceName, role, authenticationData, AuthAction.produce)
.whenComplete((produceAuthorized, ex) -> {
if (ex == null) {
finalResult.complete(produceAuthorized);
} else {
if (log.isDebugEnabled()) {
log.debug("Namespace [{}] Role [{}] exception occurred while trying to check "
+ "Produce permission. {}", namespaceName, role, ex.getCause());
}
finalResult.completeExceptionally(ex.getCause());
}
});
});

return finalResult;
}

private CompletableFuture<Boolean> allowTheSpecifiedActionOpsAsync(NamespaceName namespaceName, String role,
AuthenticationDataSource authenticationData,
AuthAction authAction) {
Expand Down Expand Up @@ -568,6 +602,7 @@ public CompletableFuture<Boolean> allowNamespaceOperationAsync(NamespaceName nam
namespaceName, role, authData, AuthAction.packages);
case GET_TOPIC:
case GET_TOPICS:
return allowConsumeOrProduceOpsAsync(namespaceName, role, authData);
case UNSUBSCRIBE:
case CLEAR_BACKLOG:
return allowTheSpecifiedActionOpsAsync(
Expand Down
6 changes: 6 additions & 0 deletions ...-broker/src/test/java/org/apache/pulsar/client/api/AuthorizationProducerConsumerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -451,6 +451,12 @@ public void testClearBacklogPermission() throws Exception {
assertEquals(sub1Admin.topics().getStats(topicName + "-partition-0").getSubscriptions()
.get(subscriptionName).getMsgBacklog(), 0);

superAdmin.namespaces().revokePermissionsOnNamespace(namespace, subscriptionRole);
superAdmin.namespaces().grantPermissionOnNamespace(namespace, subscriptionRole,
Sets.newHashSet(AuthAction.produce));
assertEquals(sub1Admin.topics().getPartitionedTopicList(namespace),
Lists.newArrayList(topicName));

log.info("-- Exiting {} test --", methodName);
}

Expand Down