Copilot commented on code in PR #25552:
URL: https://github.com/apache/pulsar/pull/25552#discussion_r3108156696
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/web/PulsarWebResource.java:
##########
@@ -31,6 +31,7 @@
import java.net.URI;
import java.net.URL;
import java.time.Duration;
+import java.util.List;
Review Comment:
The added `java.util.List` import appears to be unused in this file (the
only `List` usage is still fully-qualified as `java.util.List`), which will
fail style checks (unused imports) and should be removed or the fully-qualified
usages should be switched to the imported type.
```suggestion
```
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/NamespacesBase.java:
##########
@@ -3206,4 +3206,41 @@ private CompletableFuture<BundlesData>
getDefaultBundleDataAsync() {
getBundles(config().getDefaultNumberOfNamespaceBundles()));
}
+
+ protected CompletableFuture<Void>
validateBothAdminAccessForTenantAndNamespaceOperationAsync(
+ NamespaceName namespaceName, NamespaceOperation operation) {
+ final var tenantAdminValidation =
validateAdminAccessForTenantAsync(namespaceName.getTenant());
+ final var namespaceOperationValidation =
validateNamespaceOperationAsync(namespaceName, operation);
+ return FutureUtil.waitForAll(List.of(tenantAdminValidation,
namespaceOperationValidation))
+ .handle((result, err) -> {
+ if (!tenantAdminValidation.isCompletedExceptionally()
+ ||
!namespaceOperationValidation.isCompletedExceptionally()) {
+ return null;
+ }
+ if (log.isDebugEnabled()) {
+ Throwable tenantAdminValidationException = null;
+ try {
+ tenantAdminValidation.join();
+ } catch (Throwable ex) {
+ tenantAdminValidationException =
FutureUtil.unwrapCompletionException(ex);
+ }
+ Throwable namespaceOperationValidationException = null;
+ try {
+ namespaceOperationValidation.join();
+ } catch (Throwable ex) {
+ namespaceOperationValidationException =
FutureUtil.unwrapCompletionException(ex);
+ }
+
log.debug("validateBothAdminAccessForTenantAndNamespaceOperationAsync failed."
+ + " originalPrincipal={}
clientAppId={} operation={} namespace={} "
+ + "tenantAdminValidationError={}
namespaceOperationValidationError={}",
+ originalPrincipal(), clientAppId(),
operation.toString(), namespaceName,
+ tenantAdminValidationException,
namespaceOperationValidationException);
+ }
+ throw new RestException(Status.UNAUTHORIZED,
+ String.format("Unauthorized to
validateBothAdminAccessForTenantAndNamespaceOperationAsync"
+ + " for originalPrincipal [%s] and
clientAppId [%s] about operation [%s]"
+ + " on namespace [%s]",
+ originalPrincipal(), clientAppId(),
operation.toString(), namespaceName));
+ });
Review Comment:
`validateBothAdminAccessForTenantAndNamespaceOperationAsync` always starts
*both* authorization checks and waits for both to complete, even if the
tenant-admin check succeeds. This adds extra auth traffic/latency to every
namespace properties request and can make the endpoint as slow as the slower of
the two checks. Consider short-circuiting on the first successful validation
(and only running the second path if the first fails).
```suggestion
return validateAdminAccessForTenantAsync(namespaceName.getTenant())
.handle((result, tenantAdminValidationError) -> {
if (tenantAdminValidationError == null) {
return CompletableFuture.<Void>completedFuture(null);
}
final Throwable tenantAdminValidationException =
FutureUtil.unwrapCompletionException(tenantAdminValidationError);
return validateNamespaceOperationAsync(namespaceName,
operation)
.handle((namespaceResult,
namespaceOperationValidationError) -> {
if (namespaceOperationValidationError ==
null) {
return null;
}
final Throwable
namespaceOperationValidationException =
FutureUtil.unwrapCompletionException(namespaceOperationValidationError);
if (log.isDebugEnabled()) {
log.debug("validateBothAdminAccessForTenantAndNamespaceOperationAsync failed."
+ " originalPrincipal={}
clientAppId={} operation={} "
+ "namespace={}
tenantAdminValidationError={} "
+
"namespaceOperationValidationError={}",
originalPrincipal(),
clientAppId(), operation.toString(), namespaceName,
tenantAdminValidationException,
namespaceOperationValidationException);
}
throw new RestException(Status.UNAUTHORIZED,
String.format("Unauthorized to "
+
"validateBothAdminAccessForTenantAndNamespaceOperationAsync"
+ " for
originalPrincipal [%s] and clientAppId [%s] "
+ "about operation
[%s] on namespace [%s]",
originalPrincipal(),
clientAppId(), operation.toString(),
namespaceName));
});
}).thenCompose(Function.identity());
```
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/NamespacesBase.java:
##########
@@ -3206,4 +3206,41 @@ private CompletableFuture<BundlesData>
getDefaultBundleDataAsync() {
getBundles(config().getDefaultNumberOfNamespaceBundles()));
}
+
+ protected CompletableFuture<Void>
validateBothAdminAccessForTenantAndNamespaceOperationAsync(
+ NamespaceName namespaceName, NamespaceOperation operation) {
+ final var tenantAdminValidation =
validateAdminAccessForTenantAsync(namespaceName.getTenant());
+ final var namespaceOperationValidation =
validateNamespaceOperationAsync(namespaceName, operation);
+ return FutureUtil.waitForAll(List.of(tenantAdminValidation,
namespaceOperationValidation))
+ .handle((result, err) -> {
+ if (!tenantAdminValidation.isCompletedExceptionally()
+ ||
!namespaceOperationValidation.isCompletedExceptionally()) {
+ return null;
+ }
+ if (log.isDebugEnabled()) {
+ Throwable tenantAdminValidationException = null;
+ try {
+ tenantAdminValidation.join();
+ } catch (Throwable ex) {
+ tenantAdminValidationException =
FutureUtil.unwrapCompletionException(ex);
+ }
+ Throwable namespaceOperationValidationException = null;
+ try {
+ namespaceOperationValidation.join();
+ } catch (Throwable ex) {
+ namespaceOperationValidationException =
FutureUtil.unwrapCompletionException(ex);
+ }
+
log.debug("validateBothAdminAccessForTenantAndNamespaceOperationAsync failed."
+ + " originalPrincipal={}
clientAppId={} operation={} namespace={} "
+ + "tenantAdminValidationError={}
namespaceOperationValidationError={}",
+ originalPrincipal(), clientAppId(),
operation.toString(), namespaceName,
+ tenantAdminValidationException,
namespaceOperationValidationException);
+ }
+ throw new RestException(Status.UNAUTHORIZED,
+ String.format("Unauthorized to
validateBothAdminAccessForTenantAndNamespaceOperationAsync"
+ + " for originalPrincipal [%s] and
clientAppId [%s] about operation [%s]"
+ + " on namespace [%s]",
+ originalPrincipal(), clientAppId(),
operation.toString(), namespaceName));
+ });
Review Comment:
When both validations fail, this helper throws a new
`RestException(Status.UNAUTHORIZED, ...)`, which can change the HTTP status and
semantics compared to the underlying checks (e.g.,
`validateNamespaceOperationAsync` uses 403 FORBIDDEN;
`validateAdminAccessForTenantAsync` can return 404 NOT_FOUND for missing
tenants). This risks returning 401 for cases that should remain 403/404 and
also discards the original error details. Prefer propagating one of the
original exceptions (or selecting the most appropriate status) instead of
always creating a new 401.
--
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]
