mirror of
https://github.com/coder/coder.git
synced 2026-09-24 15:04:27 +08:00
fix(externalauth): prevent race condition in token refresh with optimistic 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.
This commit is contained in:
@@ -1990,7 +1990,7 @@ func (s *MethodTestSuite) TestUser() {
|
||||
}))
|
||||
s.Run("UpdateExternalAuthLinkRefreshToken", s.Mocked(func(dbm *dbmock.MockStore, faker *gofakeit.Faker, check *expects) {
|
||||
link := testutil.Fake(s.T(), faker, database.ExternalAuthLink{})
|
||||
arg := database.UpdateExternalAuthLinkRefreshTokenParams{OAuthRefreshToken: "", OAuthRefreshTokenKeyID: "", ProviderID: link.ProviderID, UserID: link.UserID, UpdatedAt: link.UpdatedAt}
|
||||
arg := database.UpdateExternalAuthLinkRefreshTokenParams{OAuthRefreshToken: "", OAuthRefreshTokenKeyID: "", ProviderID: link.ProviderID, UserID: link.UserID, UpdatedAt: link.UpdatedAt, OldOauthRefreshToken: link.OAuthRefreshToken}
|
||||
dbm.EXPECT().GetExternalAuthLink(gomock.Any(), database.GetExternalAuthLinkParams{ProviderID: link.ProviderID, UserID: link.UserID}).Return(link, nil).AnyTimes()
|
||||
dbm.EXPECT().UpdateExternalAuthLinkRefreshToken(gomock.Any(), arg).Return(nil).AnyTimes()
|
||||
check.Args(arg).Asserts(link, policy.ActionUpdatePersonal)
|
||||
|
||||
@@ -747,6 +747,10 @@ type sqlcQuerier interface {
|
||||
UpdateCryptoKeyDeletesAt(ctx context.Context, arg UpdateCryptoKeyDeletesAtParams) (CryptoKey, error)
|
||||
UpdateCustomRole(ctx context.Context, arg UpdateCustomRoleParams) (CustomRole, error)
|
||||
UpdateExternalAuthLink(ctx context.Context, arg UpdateExternalAuthLinkParams) (ExternalAuthLink, error)
|
||||
// Optimistic lock: only update the row if the refresh token in the database
|
||||
// still matches the one we read before attempting the refresh. This prevents
|
||||
// a concurrent caller that lost a token-refresh race from overwriting a valid
|
||||
// token stored by the winner.
|
||||
UpdateExternalAuthLinkRefreshToken(ctx context.Context, arg UpdateExternalAuthLinkRefreshTokenParams) error
|
||||
UpdateGitSSHKey(ctx context.Context, arg UpdateGitSSHKeyParams) (GitSSHKey, error)
|
||||
UpdateGroupByID(ctx context.Context, arg UpdateGroupByIDParams) (Group, error)
|
||||
|
||||
@@ -5325,9 +5325,11 @@ WHERE
|
||||
provider_id = $4
|
||||
AND
|
||||
user_id = $5
|
||||
AND
|
||||
oauth_refresh_token = $6
|
||||
AND
|
||||
-- Required for sqlc to generate a parameter for the oauth_refresh_token_key_id
|
||||
$6 :: text = $6 :: text
|
||||
$7 :: text = $7 :: text
|
||||
`
|
||||
|
||||
type UpdateExternalAuthLinkRefreshTokenParams struct {
|
||||
@@ -5336,9 +5338,14 @@ type UpdateExternalAuthLinkRefreshTokenParams struct {
|
||||
UpdatedAt time.Time `db:"updated_at" json:"updated_at"`
|
||||
ProviderID string `db:"provider_id" json:"provider_id"`
|
||||
UserID uuid.UUID `db:"user_id" json:"user_id"`
|
||||
OldOauthRefreshToken string `db:"old_oauth_refresh_token" json:"old_oauth_refresh_token"`
|
||||
OAuthRefreshTokenKeyID string `db:"oauth_refresh_token_key_id" json:"oauth_refresh_token_key_id"`
|
||||
}
|
||||
|
||||
// Optimistic lock: only update the row if the refresh token in the database
|
||||
// still matches the one we read before attempting the refresh. This prevents
|
||||
// a concurrent caller that lost a token-refresh race from overwriting a valid
|
||||
// token stored by the winner.
|
||||
func (q *sqlQuerier) UpdateExternalAuthLinkRefreshToken(ctx context.Context, arg UpdateExternalAuthLinkRefreshTokenParams) error {
|
||||
_, err := q.db.ExecContext(ctx, updateExternalAuthLinkRefreshToken,
|
||||
arg.OauthRefreshFailureReason,
|
||||
@@ -5346,6 +5353,7 @@ func (q *sqlQuerier) UpdateExternalAuthLinkRefreshToken(ctx context.Context, arg
|
||||
arg.UpdatedAt,
|
||||
arg.ProviderID,
|
||||
arg.UserID,
|
||||
arg.OldOauthRefreshToken,
|
||||
arg.OAuthRefreshTokenKeyID,
|
||||
)
|
||||
return err
|
||||
|
||||
@@ -48,6 +48,10 @@ UPDATE external_auth_links SET
|
||||
WHERE provider_id = $1 AND user_id = $2 RETURNING *;
|
||||
|
||||
-- name: UpdateExternalAuthLinkRefreshToken :exec
|
||||
-- Optimistic lock: only update the row if the refresh token in the database
|
||||
-- still matches the one we read before attempting the refresh. This prevents
|
||||
-- a concurrent caller that lost a token-refresh race from overwriting a valid
|
||||
-- token stored by the winner.
|
||||
UPDATE
|
||||
external_auth_links
|
||||
SET
|
||||
@@ -60,6 +64,8 @@ WHERE
|
||||
provider_id = @provider_id
|
||||
AND
|
||||
user_id = @user_id
|
||||
AND
|
||||
oauth_refresh_token = @old_oauth_refresh_token
|
||||
AND
|
||||
-- Required for sqlc to generate a parameter for the oauth_refresh_token_key_id
|
||||
@oauth_refresh_token_key_id :: text = @oauth_refresh_token_key_id :: text;
|
||||
|
||||
@@ -139,8 +139,6 @@ func IsInvalidTokenError(err error) bool {
|
||||
}
|
||||
|
||||
// RefreshToken automatically refreshes the token if expired and permitted.
|
||||
// If an error is returned, the token is either invalid, or an error occurred.
|
||||
// Use 'IsInvalidTokenError(err)' to determine the difference.
|
||||
func (c *Config) RefreshToken(ctx context.Context, db database.Store, externalAuthLink database.ExternalAuthLink) (database.ExternalAuthLink, error) {
|
||||
// If the token is expired and refresh is disabled, we prompt
|
||||
// the user to authenticate again.
|
||||
@@ -196,6 +194,9 @@ func (c *Config) RefreshToken(ctx context.Context, db database.Store, externalAu
|
||||
UpdatedAt: dbtime.Now(),
|
||||
ProviderID: externalAuthLink.ProviderID,
|
||||
UserID: externalAuthLink.UserID,
|
||||
// Optimistic lock: only clear the token if it hasn't been
|
||||
// updated by a concurrent caller that won the refresh race.
|
||||
OldOauthRefreshToken: externalAuthLink.OAuthRefreshToken,
|
||||
})
|
||||
if dbExecErr != nil {
|
||||
// This error should be rare.
|
||||
|
||||
@@ -92,6 +92,7 @@ func TestRefreshToken(t *testing.T) {
|
||||
|
||||
// Zero time used
|
||||
link.OAuthExpiry = time.Time{}
|
||||
|
||||
_, err := config.RefreshToken(ctx, nil, link)
|
||||
require.NoError(t, err)
|
||||
require.True(t, validated, "token should have been validated")
|
||||
@@ -106,6 +107,7 @@ func TestRefreshToken(t *testing.T) {
|
||||
},
|
||||
},
|
||||
}
|
||||
|
||||
_, err := config.RefreshToken(context.Background(), nil, database.ExternalAuthLink{
|
||||
OAuthExpiry: expired,
|
||||
})
|
||||
@@ -343,7 +345,6 @@ func TestRefreshToken(t *testing.T) {
|
||||
require.NoError(t, err)
|
||||
require.Equal(t, updated.OAuthAccessToken, dbLink.OAuthAccessToken, "token is updated in the DB")
|
||||
})
|
||||
|
||||
t.Run("WithExtra", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
|
||||
Reference in New Issue
Block a user