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.

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

[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

merged 1 commit into from Mar 2, 2022

Conversation

yuruguo
Copy link
Contributor

@yuruguo yuruguo commented Jan 16, 2022

Motivation

From this comment
Role with namespace produce or consume authz can get topics.

Documentation

  • no-need-doc

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Jan 16, 2022
@yuruguo yuruguo changed the title [Authorization] Role with namespace produce authz can get topics [Authorization] Role with namespace produce authz can also get topics Jan 16, 2022
log.debug("Namespace [{}] Role [{}] exception occurred while trying to check "
+ "Consume permissions. {}", namespaceName, role, ex.getMessage());
}
finalResult.completeExceptionally(e);
Copy link
Member

Choose a reason for hiding this comment

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

@yuruguo

Something strange here, do we need e or ex ?
Besides, we need getCause() in the exception of CompletableFuture, otherwise, we will get CompletionException

Copy link
Contributor Author

@yuruguo yuruguo Jan 17, 2022

Choose a reason for hiding this comment

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

  1. we need ex in finalResult.completeExceptionally().
  2. I has used getCause().

PTAL again.

Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

good work

I left some feedback

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

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:

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;
}

@codelipenghui codelipenghui modified the milestones: 2.10.0, 2.11.0 Jan 18, 2022
@yuruguo
Copy link
Contributor Author

yuruguo commented Mar 1, 2022

@yuruguo yuruguo requested a review from eolivelli March 1, 2022 14:37
@BewareMyPower
Copy link
Contributor

Add the release/2.8.4 label since it's relied by #15501.

BewareMyPower pushed a commit that referenced this pull request Jul 28, 2022
@BewareMyPower BewareMyPower added the cherry-picked/branch-2.8 Archived: 2.8 is end of life label Jul 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants