Skip to content

Commit

Permalink
[fix][broker] Only validate superuser access if authz enabled (apache…
Browse files Browse the repository at this point in the history
…#19989)

### Motivation

In apache#19455, I added a requirement that only the proxy role could supply an original principal. That check is only supposed to apply when the broker has authorization enabled. However, in one case, that was not the case. This PR does a check and returns early when authorization is not enabled in the broker.

See apache#19830 (comment) for additional motivation.

### Modifications

* Update the `PulsarWebResource#validateSuperUserAccessAsync` to only validate when authentication and authorization are enabled in the configuration.

### Verifying this change

This is a trivial change. It'd be good to add tests, but I didn't include them here because this is a somewhat urgent fix. There was one test that broke because of this change, so there is at least some existing coverage.

### Documentation

- [x] `doc-not-needed`

### Matching PR in forked repository

PR in forked repository: michaeljmarshall#39
  • Loading branch information
michaeljmarshall authored Apr 5, 2023
1 parent be7e890 commit 1a6c28d
Show file tree
Hide file tree
Showing 2 changed files with 12 additions and 18 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -185,8 +185,8 @@ protected boolean hasSuperUserAccess() {
return true;
}

public CompletableFuture<Void> validateSuperUserAccessAsync(){
if (!config().isAuthenticationEnabled()) {
public CompletableFuture<Void> validateSuperUserAccessAsync() {
if (!config().isAuthenticationEnabled() || !config().isAuthorizationEnabled()) {
return CompletableFuture.completedFuture(null);
}
String appId = clientAppId();
Expand Down Expand Up @@ -221,22 +221,15 @@ public CompletableFuture<Void> validateSuperUserAccessAsync(){
}
});
} else {
if (config().isAuthorizationEnabled()) {
return pulsar.getBrokerService()
.getAuthorizationService()
.isSuperUser(appId, clientAuthData())
.thenAccept(proxyAuthorizationSuccess -> {
if (!proxyAuthorizationSuccess) {
throw new RestException(Status.UNAUTHORIZED,
"This operation requires super-user access");
}
});
}
if (log.isDebugEnabled()) {
log.debug("Successfully authorized {} as super-user",
appId);
}
return CompletableFuture.completedFuture(null);
return pulsar.getBrokerService()
.getAuthorizationService()
.isSuperUser(appId, clientAuthData())
.thenAccept(proxyAuthorizationSuccess -> {
if (!proxyAuthorizationSuccess) {
throw new RestException(Status.UNAUTHORIZED,
"This operation requires super-user access");
}
});
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -630,6 +630,7 @@ public void testUpdateTransactionCoordinatorNumber() throws Exception {
Awaitility.await().until(() -> pulsar.getTransactionMetadataStoreService().getStores().size() ==
coordinatorSize * 2);
pulsar.getConfiguration().setAuthenticationEnabled(true);
pulsar.getConfiguration().setAuthorizationEnabled(true);
Set<String> proxyRoles = spy(Set.class);
doReturn(true).when(proxyRoles).contains(any());
pulsar.getConfiguration().setProxyRoles(proxyRoles);
Expand Down

0 comments on commit 1a6c28d

Please sign in to comment.