[fix][broker] Implement authenticateAsync for AuthenticationProviderList #20132
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
michaeljmarshall
added
type/feature
michaeljmarshall
added
the
release/blocker
michaeljmarshall
requested review from
lhotari,
eolivelli,
cbornet,
codelipenghui and
nodece
cbornet
approved these changes
Apr 18, 2023
merlimat
approved these changes
Apr 18, 2023
Codecov Report
@@ Coverage Diff @@
## master #20132 +/- ##
=============================================
+ Coverage 37.75% 72.91% +35.16%
- Complexity 12534 31929 +19395
=============================================
Files 1691 1868 +177
Lines 128811 138411 +9600
Branches 14045 15236 +1191
=============================================
+ Hits 48632 100927 +52295
+ Misses 73850 29444 -44406
- Partials 6329 8040 +1711
Flags with carried forward coverage won't be shown. Click here to find out more.
|
RobertIndie
approved these changes
Apr 19, 2023
RobertIndie
pushed a commit
that referenced
this pull request
Apr 19, 2023
…ist (#20132) PIP: #12105 and #19771 ### Motivation With the implementation of asynchronous authentication in PIP 97, I missed a case in the `AuthenticationProviderList` where we need to implement the `authenticateAsync` methods. This PR is necessary for making the `AuthenticationProviderToken` and the `AuthenticationProviderOpenID` work together, which is necessary for anyone transitioning to `AuthenticationProviderOpenID`. ### Modifications * Implement `AuthenticationListState#authenticateAsync` using a recursive algorithm that first attempts to authenticate the client using the current `authState` and then tries the remaining options. * Implement `AuthenticationProviderList#authenticateAsync` using a recursive algorithm that attempts each provider sequentially. * Add test to `AuthenticationProviderListTest` that exercises this method. It didn't technically fail previously, but it's worth adding. * Add test to `AuthenticationProviderOpenIDIntegrationTest` to cover the exact failures that were causing problems. (cherry picked from commit 58ccf02)
michaeljmarshall
removed
the
release/blocker
michaeljmarshall
added a commit
to michaeljmarshall/pulsar
that referenced
this pull request
Apr 20, 2023
…ist (apache#20132) PIP: apache#12105 and apache#19771 With the implementation of asynchronous authentication in PIP 97, I missed a case in the `AuthenticationProviderList` where we need to implement the `authenticateAsync` methods. This PR is necessary for making the `AuthenticationProviderToken` and the `AuthenticationProviderOpenID` work together, which is necessary for anyone transitioning to `AuthenticationProviderOpenID`. * Implement `AuthenticationListState#authenticateAsync` using a recursive algorithm that first attempts to authenticate the client using the current `authState` and then tries the remaining options. * Implement `AuthenticationProviderList#authenticateAsync` using a recursive algorithm that attempts each provider sequentially. * Add test to `AuthenticationProviderListTest` that exercises this method. It didn't technically fail previously, but it's worth adding. * Add test to `AuthenticationProviderOpenIDIntegrationTest` to cover the exact failures that were causing problems. (cherry picked from commit 58ccf02)
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.
PIP: #12105 and #19771
Motivation
With the implementation of asynchronous authentication in PIP 97, I missed a case in the
AuthenticationProviderListwhere we need to implement theauthenticateAsyncmethods. This PR is necessary for making theAuthenticationProviderTokenand theAuthenticationProviderOpenIDwork together, which is necessary for anyone transitioning toAuthenticationProviderOpenID.Modifications
AuthenticationListState#authenticateAsyncusing a recursive algorithm that first attempts to authenticate the client using the currentauthStateand then tries the remaining options.AuthenticationProviderList#authenticateAsyncusing a recursive algorithm that attempts each provider sequentially.AuthenticationProviderListTestthat exercises this method. It didn't technically fail previously, but it's worth adding.AuthenticationProviderOpenIDIntegrationTestto cover the exact failures that were causing problems.Verifying this change
A new test is added and all existing tests pass.
Documentation
doc-not-neededMatching PR in forked repository
PR in forked repository: Skipping since I was able to run the relevant tests locally