fix(externalauth): prevent race condition in token refresh with optimistic locking (#22904) (backport to 2.29)#24901
Merged
Merged
Conversation
…istic locking (#22904) ## Problem When multiple concurrent callers (e.g., parallel workspace builds) read the same single-use OAuth2 refresh token from the database and race to exchange it with the provider, the first caller succeeds but subsequent callers get `bad_refresh_token`. The losing caller then **clears the valid new token** from the database, permanently breaking the auth link until the user manually re-authenticates. This is reliably reproducible when launching multiple workspaces simultaneously with GitHub App external auth and user-to-server token expiration enabled. ## Solution Two layers of protection: ### 1. Singleflight deduplication (`Config.RefreshToken` + `ObtainOIDCAccessToken`) Concurrent callers for the same user/provider share a single refresh call via `golang.org/x/sync/singleflight`, keyed by `userID`. The singleflight callback re-reads the link from the database to pick up any token already refreshed by a prior in-flight call, avoiding redundant IDP round-trips entirely. ### 2. Optimistic locking on `UpdateExternalAuthLinkRefreshToken` The SQL `WHERE` clause now includes `AND oauth_refresh_token = @old_oauth_refresh_token`, so if two replicas (HA) race past singleflight, the loser's destructive UPDATE is a harmless no-op rather than overwriting the winner's valid token. ## Changes | File | Change | |------|--------| | `coderd/externalauth/externalauth.go` | Added `singleflight.Group` to `Config`; split `RefreshToken` into public wrapper + `refreshTokenInner`; pass `OldOauthRefreshToken` to DB update | | `coderd/provisionerdserver/provisionerdserver.go` | Wrapped OIDC refresh in `ObtainOIDCAccessToken` with package-level singleflight | | `coderd/database/queries/externalauth.sql` | Added optimistic lock (`WHERE ... AND oauth_refresh_token = @old_oauth_refresh_token`) | | `coderd/database/queries.sql.go` | Regenerated | | `coderd/database/querier.go` | Regenerated | | `coderd/database/dbauthz/dbauthz_test.go` | Updated test params for new field | | `coderd/externalauth/externalauth_test.go` | Added `ConcurrentRefreshDedup` test; updated existing tests for singleflight DB re-read | ## Testing - **New test `ConcurrentRefreshDedup`**: 5 goroutines call `RefreshToken` concurrently, asserts IDP refresh called exactly once, all callers get same token. - All existing `TestRefreshToken/*` subtests updated and passing. - `TestObtainOIDCAccessToken` passing. - `dbauthz` tests passing. (cherry picked from commit 53e52ae)
3e09ae9 to
d5fa784
Compare
geokat
approved these changes
May 1, 2026
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Backport of #22904 to
release/2.29.Adds an optimistic lock to
UpdateExternalAuthLinkRefreshTokenso that a concurrent caller that lost a token-refresh race cannot overwrite a valid token stored by the winner. The SQLWHEREclause now includesAND oauth_refresh_token = @old_oauth_refresh_token.Original PR: #22904
Merge commit: 53e52ae
Cherry-pick applied cleanly with no conflicts.