plugin/oauth2: fix thread-safety of GoogleOAuth2Provider token handling#13564
Open
rhenar0 wants to merge 3 commits into
Open
plugin/oauth2: fix thread-safety of GoogleOAuth2Provider token handling#13564rhenar0 wants to merge 3 commits into
rhenar0 wants to merge 3 commits into
Conversation
GoogleOAuth2Provider is a Spring singleton but stored the OAuth2 access and refresh tokens in shared instance fields. Under concurrent logins, the StringUtils.isAnyEmpty() guard made a second login reuse the first login's tokens instead of exchanging its own authorization code, so Google returned the first user's email and verification failed with "Unable to verify the email address with the provided secret". Additionally, clearAccessAndRefreshTokens() was only invoked on the success path of verifyUser(), so any failure left stale tokens in the singleton and every subsequent login failed until a management server restart. Fix by removing the instance fields entirely: always exchange the authorization code in verifyCodeAndFetchEmail() and keep the tokens in local variables. Also surface token exchange failures as a CloudRuntimeException with a meaningful message instead of a bare RuntimeException. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Congratulations on your first Pull Request and welcome to the Apache CloudStack community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/cloudstack/blob/main/CONTRIBUTING.md)
|
PrashantBhanage
suggested changes
Jul 8, 2026
PrashantBhanage
left a comment
There was a problem hiding this comment.
Logic looks solid and the thread-safety fix is great. Just found a small runtime exception syntax error and a minor variable naming cleanup.
…oudstack/oauth2/google/GoogleOAuth2Provider.java Co-authored-by: prrssshhhh <prashantbhanage717@gmail.com>
…oudstack/oauth2/google/GoogleOAuth2Provider.java Co-authored-by: prrssshhhh <prashantbhanage717@gmail.com>
This file contains hidden or 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
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.
Description
This PR fixes a thread-safety bug in
GoogleOAuth2Providerthat breaks Google OAuth2 login under concurrent use, and can leave the provider permanently broken until the management server is restarted.GoogleOAuth2Provideris a singleton Spring bean, but it stored the OAuth2accessToken/refreshTokenin shared mutable instance fields:StringUtils.isAnyEmpty(accessToken, refreshToken)guard inverifyCodeAndFetchEmail()made a concurrent login B skip exchanging its own authorization code and call the userinfo endpoint with login A's tokens. Google then returns A's email, which does not match B's, soverifyUser()fails withUnable to verify the email address with the provided secret.clearAccessAndRefreshTokens()was only called on the success path ofverifyUser(), so any failure left stale tokens in the singleton, and every subsequent Google login reused them and failed, until acloudstack-managementrestart.The token caching provided no benefit: each login carries its own one-time authorization code that must be exchanged individually.
Changes:
accessToken/refreshTokeninstance fields;verifyCodeAndFetchEmail()now always exchanges the authorization code and keeps the tokens in local variables.clearAccessAndRefreshTokens()and its call site inverifyUser().CloudRuntimeExceptionwith a meaningful message instead of a bareRuntimeException.GoogleOAuth2ProviderTestto mock the token exchange flow, and add tests covering that every call performs its own token exchange and that a failed exchange raises aCloudRuntimeException.No change to the OAuth scopes, the userinfo call, or the email comparison logic.
Fixes: #13563
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
mvn -pl plugins/user-authenticators/oauth2 -am clean install -DskipTestsfollowed bymvn -pl plugins/user-authenticators/oauth2 test: all 29 unit tests of the module pass, including the 8 tests ofGoogleOAuth2ProviderTest(2 of them new).testVerifyCodeAndFetchEmailExchangesCodeOnEveryCalltest verifies that two successive calls toverifyCodeAndFetchEmail()each trigger their own token exchange (flow.newTokenRequest(...)called once per code), which is exactly the scenario broken by the shared-state guard.How did you try to break this feature and the system with this change?
testVerifyUserWithFailedTokenExchangetest submits a failing token exchange and asserts it surfaces as aCloudRuntimeException; with no shared state left in the singleton, a failed login can no longer affect subsequent logins.UserOAuth2Authenticatorcall sites:verifyCodeAndFetchEmail()andverifyUser()keep their existing signatures and semantics, and the removed members were internal to this provider (clearAccessAndRefreshTokens()had no other callers, and theaccessToken/refreshTokenfields were only referenced by the provider and its test).