You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2022/01/21 23:02:13 UTC
[GitHub] [pulsar] michaeljmarshall opened a new pull request #13896: [Authorization] Remove unnecessary authz provider null checks
michaeljmarshall opened a new pull request #13896:
URL: https://github.com/apache/pulsar/pull/13896
### Motivation
Clean up the `AuthorizationService` class. The `provider` in the `AuthorizationService` is never null. The class fails to initialize if it is null. Therefore, we can drop the null checks. This cleans up the code and will help simplify future refactors, which I plan to contribute.
### Modifications
* Remove null checks.
* Make `AuthorizationService#provider` final.
* Update white space.
* Remove unused import.
### Verifying this change
This is a trivial revision of the class.
### Does this pull request potentially affect one of the following parts:
It does not break any behavior.
### Documentation
- [x] `no-need-doc`
This is a completely internal change.
--
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] mattisonchao commented on a change in pull request #13896: [Authorization] Remove unnecessary authz provider null checks
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on a change in pull request #13896:
URL: https://github.com/apache/pulsar/pull/13896#discussion_r790080286
##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authorization/AuthorizationService.java
##########
@@ -184,16 +161,13 @@ public AuthorizationService(ServiceConfiguration conf, PulsarResources pulsarRes
if (!this.conf.isAuthorizationEnabled()) {
return CompletableFuture.completedFuture(true);
}
- if (provider != null) {
- return provider.isSuperUser(role, authenticationData, conf).thenComposeAsync(isSuperUser -> {
- if (isSuperUser) {
- return CompletableFuture.completedFuture(true);
- } else {
- return provider.canProduceAsync(topicName, role, authenticationData);
- }
- });
- }
- return FutureUtil.failedFuture(new IllegalStateException("No authorization provider configured"));
+ return provider.isSuperUser(role, authenticationData, conf).thenComposeAsync(isSuperUser -> {
Review comment:
I agree with your consideration.
I think we need to hear from everyone or discuss it in another PR. :-)
--
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] mattisonchao commented on a change in pull request #13896: [Authorization] Remove unnecessary authz provider null checks
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on a change in pull request #13896:
URL: https://github.com/apache/pulsar/pull/13896#discussion_r790064467
##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authorization/AuthorizationService.java
##########
@@ -184,16 +161,13 @@ public AuthorizationService(ServiceConfiguration conf, PulsarResources pulsarRes
if (!this.conf.isAuthorizationEnabled()) {
return CompletableFuture.completedFuture(true);
}
- if (provider != null) {
- return provider.isSuperUser(role, authenticationData, conf).thenComposeAsync(isSuperUser -> {
- if (isSuperUser) {
- return CompletableFuture.completedFuture(true);
- } else {
- return provider.canProduceAsync(topicName, role, authenticationData);
- }
- });
- }
- return FutureUtil.failedFuture(new IllegalStateException("No authorization provider configured"));
+ return provider.isSuperUser(role, authenticationData, conf).thenComposeAsync(isSuperUser -> {
Review comment:
Maybe there could we use specific ``executor`` instead of ``commonPool`` when ``thenComposeAsync``?
--
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on a change in pull request #13896: [Authorization] Remove unnecessary authz provider null checks
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on a change in pull request #13896:
URL: https://github.com/apache/pulsar/pull/13896#discussion_r790077394
##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authorization/AuthorizationService.java
##########
@@ -184,16 +161,13 @@ public AuthorizationService(ServiceConfiguration conf, PulsarResources pulsarRes
if (!this.conf.isAuthorizationEnabled()) {
return CompletableFuture.completedFuture(true);
}
- if (provider != null) {
- return provider.isSuperUser(role, authenticationData, conf).thenComposeAsync(isSuperUser -> {
- if (isSuperUser) {
- return CompletableFuture.completedFuture(true);
- } else {
- return provider.canProduceAsync(topicName, role, authenticationData);
- }
- });
- }
- return FutureUtil.failedFuture(new IllegalStateException("No authorization provider configured"));
+ return provider.isSuperUser(role, authenticationData, conf).thenComposeAsync(isSuperUser -> {
Review comment:
This is a fair question to ask, but I think fixing it is out of the scope of this PR. We could definitely consider adding an executor to the `AuthorizationService`, but I think we should consider refactoring this class first.
In my view, the larger problem here is that the `AuthenticationService` is making composite authorization decisions about who can produce by determining that super user or that `provider.canProduceAsync` is sufficient. I think the proper abstraction would only delegate to the `AuthenticationProvider`.
--
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] merlimat merged pull request #13896: [Authorization] Remove unnecessary authz provider null checks
Posted by GitBox <gi...@apache.org>.
merlimat merged pull request #13896:
URL: https://github.com/apache/pulsar/pull/13896
--
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org