[feat][broker] Update AuthenticationProvider to simplify HTTP Authn #12
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
2 tasks
michaeljmarshall
added a commit
to apache/pulsar
that referenced
this pull request
Jan 19, 2023
…19197) PIP: #12105 ### Motivation This is the first of several PRs to implement [PIP 97](#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 #12104. Historical context: #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. * Implement changes for the `AuthenticationProviderToken` so that it no longer relies on `newHttpAuthState`. ### 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: michaeljmarshall#12 ### Additional motivation from PR discussion My primary motivation for this PR is to implement PIP 97. If we want authentication to be asynchronous, we cannot assume that when the `AuthenticationState` object is initialized, the `authenticationDataSource` and the `authRole` are present because the authentication might not yet be completed. My goal with this PR is to break an unnecessary relationship between `AuthenticationState` and http request authentication that was created by #14044.
michaeljmarshall
added a commit
that referenced
this pull request
Apr 19, 2023
…pache#19197) PIP: apache#12105 This is the first of several PRs to implement [PIP 97](apache#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 apache#12104. Historical context: apache#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. * 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 apache#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. * Implement changes for the `AuthenticationProviderToken` so that it no longer relies on `newHttpAuthState`. I added new tests. - [x] The public API This changes the public API within the broker by marking some methods as `@Deprecated`. - [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. PR in forked repository: #12 My primary motivation for this PR is to implement PIP 97. If we want authentication to be asynchronous, we cannot assume that when the `AuthenticationState` object is initialized, the `authenticationDataSource` and the `authRole` are present because the authentication might not yet be completed. My goal with this PR is to break an unnecessary relationship between `AuthenticationState` and http request authentication that was created by apache#14044. (cherry picked from commit 3c38ed5)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR to run tests