michaeljmarshall commented on a change in pull request #11172:
URL: https://github.com/apache/pulsar/pull/11172#discussion_r680224433
##########
File path:
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java
##########
@@ -1722,30 +1723,97 @@ public void readEntryFailed(ManagedLedgerException
exception, Object ctx) {
});
}
+ private CompletableFuture<Boolean>
isNamespaceOperationAllowed(NamespaceName namespaceName,
Review comment:
It seems like this method and the `isTopicOperationAllowed` method share
a lot of core logic with just a couple places that would branch for looking up
`allowNamespaceOperationAsync` vs `allowNamespaceOperationAsync`. It'd be good
to consolidate this logic, if possible.
##########
File path:
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java
##########
@@ -1722,30 +1723,97 @@ public void readEntryFailed(ManagedLedgerException
exception, Object ctx) {
});
}
+ private CompletableFuture<Boolean>
isNamespaceOperationAllowed(NamespaceName namespaceName,
+
NamespaceOperation operation) {
+ CompletableFuture<Boolean> isProxyAuthorizedFuture;
+ CompletableFuture<Boolean> isAuthorizedFuture;
+ if (service.isAuthorizationEnabled()) {
+ if (originalPrincipal != null) {
+ isProxyAuthorizedFuture =
service.getAuthorizationService().allowNamespaceOperationAsync(
+ namespaceName, operation, originalPrincipal,
getAuthenticationData());
+ } else {
+ isProxyAuthorizedFuture =
CompletableFuture.completedFuture(true);
+ }
+ isAuthorizedFuture =
service.getAuthorizationService().allowNamespaceOperationAsync(
+ namespaceName, operation, authRole, authenticationData);
+ } else {
+ isProxyAuthorizedFuture = CompletableFuture.completedFuture(true);
+ isAuthorizedFuture = CompletableFuture.completedFuture(true);
+ }
+ return isProxyAuthorizedFuture.thenCombine(isAuthorizedFuture,
(isProxyAuthorized, isAuthorized) -> {
+ if (!isProxyAuthorized) {
+ log.warn("OriginalRole {} is not authorized to perform
operation {} on namespace {}",
+ originalPrincipal, operation, namespaceName);
+ }
+ if (!isAuthorized) {
+ log.warn("Role {} is not authorized to perform operation {} on
namespace {}",
+ authRole, operation, namespaceName);
+ }
+ return isProxyAuthorized && isAuthorized;
+ });
+ }
+
@Override
protected void handleGetTopicsOfNamespace(CommandGetTopicsOfNamespace
commandGetTopicsOfNamespace) {
final long requestId = commandGetTopicsOfNamespace.getRequestId();
final String namespace = commandGetTopicsOfNamespace.getNamespace();
final CommandGetTopicsOfNamespace.Mode mode =
commandGetTopicsOfNamespace.getMode();
final NamespaceName namespaceName = NamespaceName.get(namespace);
-
getBrokerService().pulsar().getNamespaceService().getListOfTopics(namespaceName,
mode)
- .thenAccept(topics -> {
- if (log.isDebugEnabled()) {
- log.debug("[{}] Received CommandGetTopicsOfNamespace
for namespace [//{}] by {}, size:{}",
- remoteAddress, namespace, requestId,
topics.size());
- }
- commandSender.sendGetTopicsOfNamespaceResponse(topics,
requestId);
- })
- .exceptionally(ex -> {
- log.warn("[{}] Error GetTopicsOfNamespace for namespace
[//{}] by {}",
- remoteAddress, namespace, requestId);
- commandSender.sendErrorResponse(requestId,
- BrokerServiceException.getClientErrorCode(new
ServerMetadataException(ex)),
- ex.getMessage());
-
- return null;
- });
+ final Semaphore lookupSemaphore = service.getLookupRequestSemaphore();
Review comment:
I'm seeing some code duplication in this file where we call
`service.getLookupRequestSemaphore()`. It could be good to come back later and
consolidate the code.
--
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-unsubscr...@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
