michaeljmarshall opened a new pull request, #19197: URL: https://github.com/apache/pulsar/pull/19197
PIP: https://github.com/apache/pulsar/issues/12105 ### Motivation This is the first of several PRs to implement [PIP 97](https://github.com/apache/pulsar/issues/12105). This PR seeks to start to solve the fact that the `AuthenticationState` class currently authenticates `authData` twice instead of just once. This change is important to make before we are able to utilize the async methods introduced in https://github.com/apache/pulsar/pull/12104. Historical context: https://github.com/apache/pulsar/pull/14044 introduced the `AuthenticationProvider#newHttpAuthState` method. The only use case for this method in the pulsar code base is to let custom providers specify the `AuthenticationDataSource` on http request attributes. The primary problem with that implementation is that the `AuthenticationState` class currently involves authenticating the `authData` passed in to the `newHttpAuthState`. As such, this code is sub-optimal, and creates a confusing flow. I propose we deprecate the `newHttpAuthState` method and instead start using the `authenticateHttpRequestAsync` and `authenticateHttpRequest` methods to allow custom implementations to configure the desired `AuthenticationDataSource` on the request attributes. In order to simplify the diff for reviewers, this PR uses the deprecated `AuthenticationProvider#authenticateHttpRequest` method. I plan to follow up and switch to use the `AuthenticationProvider#authenticateHttpRequestAsync` method. Note that these changes are completely backwards compatible. The only risk is to users that have custom code loaded into the broker that calls the `AuthenticationProvider#authenticateHttpRequest` method. ### Modifications * Deprecate `AuthenticationService#authenticateHttpRequest(HttpServletRequest request, AuthenticationDataSource authData)`. It is no longer used. * Deprecate `AuthenticationProvider#newHttpAuthState(HttpServletRequest request)`. It is no longer used outside of the `AuthenticationProvider` interface itself. * Remove `@InterfaceStability.Unstable` annotation from the `authenticateHttpRequestAsync` method. When I introduced that annotation, I was under the impression that we didn't need it. However, in order to meet the requirements introduced in #14044, we need to let custom `AuthenticationProviders` add their own attributes. * Update the default implementation of `authenticateHttpRequest`. Because the previous implementation was unreachable by all auth providers except for the SASL implementation, this is a backwards compatible change. ### Verifying this change I added new tests. ### Does this pull request potentially affect one of the following parts: - [x] The public API This changes the public API within the broker by marking some methods as `@Deprecated`. ### Documentation - [x] `doc-not-needed` We document the `AuthenticationProvider` interface in the code. I added these docs. There is currently no where else to update docs. ### Matching PR in forked repository PR in forked repository: https://github.com/michaeljmarshall/pulsar/pull/12 -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
