rhenar0 opened a new pull request, #13564: URL: https://github.com/apache/cloudstack/pull/13564
### Description This PR fixes a thread-safety bug in `GoogleOAuth2Provider` that breaks Google OAuth2 login under concurrent use, and can leave the provider permanently broken until the management server is restarted. `GoogleOAuth2Provider` is a singleton Spring bean, but it stored the OAuth2 `accessToken` / `refreshToken` in shared mutable instance fields: 1. **Race condition between concurrent logins**: the `StringUtils.isAnyEmpty(accessToken, refreshToken)` guard in `verifyCodeAndFetchEmail()` 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, so `verifyUser()` fails with `Unable to verify the email address with the provided secret`. 2. **Poisoned state is never cleaned up**: `clearAccessAndRefreshTokens()` was only called on the success path of `verifyUser()`, so any failure left stale tokens in the singleton, and every subsequent Google login reused them and failed, until a `cloudstack-management` restart. The token caching provided no benefit: each login carries its own one-time authorization code that must be exchanged individually. Changes: - Remove the `accessToken` / `refreshToken` instance fields; `verifyCodeAndFetchEmail()` now always exchanges the authorization code and keeps the tokens in local variables. - Remove `clearAccessAndRefreshTokens()` and its call site in `verifyUser()`. - Surface token exchange failures as a `CloudRuntimeException` with a meaningful message instead of a bare `RuntimeException`. - Adapt `GoogleOAuth2ProviderTest` to mock the token exchange flow, and add tests covering that every call performs its own token exchange and that a failed exchange raises a `CloudRuntimeException`. No change to the OAuth scopes, the userinfo call, or the email comparison logic. Fixes: #13563 ### Types of changes - [ ] Breaking change (fix or feature that would cause existing functionality to change) - [ ] New feature (non-breaking change which adds functionality) - [x] Bug fix (non-breaking change which fixes an issue) - [ ] Enhancement (improves an existing feature and functionality) - [ ] Cleanup (Code refactoring and cleanup, that may add test cases) - [ ] Build/CI - [ ] Test (unit or integration test code) ### Feature/Enhancement Scale or Bug Severity #### Feature/Enhancement Scale - [ ] Major - [ ] Minor #### Bug Severity - [ ] BLOCKER - [ ] Critical - [x] Major - [ ] Minor - [ ] Trivial ### Screenshots (if appropriate): ### How Has This Been Tested? - `mvn -pl plugins/user-authenticators/oauth2 -am clean install -DskipTests` followed by `mvn -pl plugins/user-authenticators/oauth2 test`: all 29 unit tests of the module pass, including the 8 tests of `GoogleOAuth2ProviderTest` (2 of them new). - The new `testVerifyCodeAndFetchEmailExchangesCodeOnEveryCall` test verifies that two successive calls to `verifyCodeAndFetchEmail()` each trigger their own token exchange (`flow.newTokenRequest(...)` called once per code), which is exactly the scenario broken by the shared-state guard. - The bug itself was observed and reproduced on a CloudStack 4.22.0.0 production environment (Rocky Linux management server, KVM, Google as primary login method for ~hundreds of users); see #13563 for the reproduction steps. #### How did you try to break this feature and the system with this change? - The new `testVerifyUserWithFailedTokenExchange` test submits a failing token exchange and asserts it surfaces as a `CloudRuntimeException`; with no shared state left in the singleton, a failed login can no longer affect subsequent logins. - Checked the other `UserOAuth2Authenticator` call sites: `verifyCodeAndFetchEmail()` and `verifyUser()` keep their existing signatures and semantics, and the removed members were internal to this provider (`clearAccessAndRefreshTokens()` had no other callers, and the `accessToken`/`refreshToken` fields were only referenced by the provider and its test). -- 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]
