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 2021/09/16 21:40:44 UTC
[GitHub] [pulsar] michaeljmarshall opened a new pull request #12068: [Broker Auth SASL] Remove unnecessary authenticate method definition
michaeljmarshall opened a new pull request #12068:
URL: https://github.com/apache/pulsar/pull/12068
### Motivation
Remove `AuthenticationProviderSasl`'s overridden method declaration for `authenticate`. The default implementation will be used, which always throws an `AuthenticationException`.
I remove it first because the `AuthenticationProvider` interface's Javadoc indicates that the method should not be implemented in the case of multi-stage authentication providers. In those cases, the authentication provider ought to implement the `AuthenticationState` class, which the `pulsar-broker-auth-sasl` module does do (`AuthenticationProviderSasl`). Here is a reference to the Javadoc: https://github.com/apache/pulsar/blob/23ffdb778e25169bfc7343920912521b41bf95f5/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationProvider.java#L54-L67
Second, this `authenticate` method is never called successfully. By removing it, it should make the implementation a bit easier to understand. Here is an enumeration of the method calls to `AuthenticationProvider#authenticate`:
1. https://github.com/apache/pulsar/blob/23ffdb778e25169bfc7343920912521b41bf95f5/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationService.java#L92
2. https://github.com/apache/pulsar/blob/23ffdb778e25169bfc7343920912521b41bf95f5/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationService.java#L113
3. https://github.com/apache/pulsar/blob/23ffdb778e25169bfc7343920912521b41bf95f5/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationService.java#L125
4. https://github.com/apache/pulsar/blob/23ffdb778e25169bfc7343920912521b41bf95f5/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/OneStageAuthenticationState.java#L46
5. https://github.com/apache/pulsar/blob/23ffdb778e25169bfc7343920912521b41bf95f5/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationProviderList.java#L159
In case 1 above, the calling method `AuthenticationService#authenticate` is only called from the `DiscoveryService`'s class. This class does not create an `SaslAuthenticationDataSource`, so the call will fail. In cases 2 and 3 above, the `AuthenticationDataSource` that is passed in is always of type `AuthenticationDataHttps`, so the call will fail. In case 4 above, that is used in `OneStageAuthenticationState`, so it is never called when using sasl. In çase 5 above, the `AuthenticationProviderList` is recursively implementing the `authenticate` method, so it is covered by cases 1 through 4.
### Modifications
* Remove overriding of `authenticate` method in the sasl authentication provider implementation.
* Add Javadoc explaining why it is not overridden.
### Verifying this change
* Tests pass for the `pulsar-broker-auth-sasl` module
* Since this change does not _actually_ change any behavior, I don't see any need to add more tests.
### Does this pull request potentially affect one of the following parts:
No.
### Documentation
This is an internal change. I did leave some comments for future reference. I don't believe any additional documentation is required.
--
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 pull request #12068: [Broker Auth SASL] Remove unnecessary authenticate method definition
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on pull request #12068:
URL: https://github.com/apache/pulsar/pull/12068#issuecomment-921275110
@jiazhai - PTAL. I know you were the original author of this code, so I would greatly appreciate your review. Thanks.
--
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 pull request #12068: [Broker Auth SASL] Remove unnecessary authenticate method definition
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on pull request #12068:
URL: https://github.com/apache/pulsar/pull/12068#issuecomment-921311175
/pulsarbot run-failure-checks
--
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 #12068: [Broker Auth SASL] Remove unnecessary authenticate method definition
Posted by GitBox <gi...@apache.org>.
merlimat merged pull request #12068:
URL: https://github.com/apache/pulsar/pull/12068
--
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