Skip to content

Conversation

@Technoboy-
Copy link
Contributor

Motivation

When the client uses the proxy to connect to the broker, if enabled the authorization and set namespace subscription auth mode with Prefix, the client will always face the authorization issue :

Failed to create consumer - The subscription name needs to be prefixed by the authentication role, like abc-xxxx for topic: persistent://public/default/test

The related codes to the root cause:

private CompletableFuture<Boolean> isTopicOperationAllowed(TopicName topicName, TopicOperation operation,
AuthenticationDataSource authDataSource, AuthenticationDataSource originalAuthDataSource) {
if (!service.isAuthorizationEnabled()) {
return CompletableFuture.completedFuture(true);
}
CompletableFuture<Boolean> isProxyAuthorizedFuture;
if (originalPrincipal != null) {
isProxyAuthorizedFuture = service.getAuthorizationService().allowTopicOperationAsync(
topicName, operation, originalPrincipal,
originalAuthDataSource != null ? originalAuthDataSource : authDataSource);
} else {
isProxyAuthorizedFuture = CompletableFuture.completedFuture(true);
}
CompletableFuture<Boolean> isAuthorizedFuture = service.getAuthorizationService().allowTopicOperationAsync(
topicName, operation, authRole, authDataSource);
return isProxyAuthorizedFuture.thenCombine(isAuthorizedFuture, (isProxyAuthorized, isAuthorized) -> {
if (!isProxyAuthorized) {
log.warn("OriginalRole {} is not authorized to perform operation {} on topic {}",
originalPrincipal, operation, topicName);
}
if (!isAuthorized) {
log.warn("Role {} is not authorized to perform operation {} on topic {}",
authRole, operation, topicName);
}
return isProxyAuthorized && isAuthorized;
});
}

Verifying this change

Add a related test to cover this change.

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

@Technoboy- Technoboy- added this to the 3.4.0 milestone Jul 17, 2024
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Jul 17, 2024
@Technoboy- Technoboy- self-assigned this Jul 17, 2024
@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 73.42%. Comparing base (bbc6224) to head (cdb2e37).
Report is 456 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #23045      +/-   ##
============================================
- Coverage     73.57%   73.42%   -0.16%     
- Complexity    32624    33391     +767     
============================================
  Files          1877     1914      +37     
  Lines        139502   143634    +4132     
  Branches      15299    15669     +370     
============================================
+ Hits         102638   105458    +2820     
- Misses        28908    30111    +1203     
- Partials       7956     8065     +109     
Flag Coverage Δ
inttests 27.78% <0.00%> (+3.19%) ⬆️
systests 24.68% <0.00%> (+0.36%) ⬆️
unittests 72.48% <100.00%> (-0.37%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
...va/org/apache/pulsar/broker/service/ServerCnx.java 72.19% <100.00%> (+0.05%) ⬆️

... and 497 files with indirect coverage changes

@nodece
Copy link
Member

nodece commented Jul 22, 2024

Isn't proxy super user? Similar to #23036

@shibd shibd requested review from nodece and shibd July 25, 2024 01:10
@Technoboy- Technoboy- closed this Jul 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants