Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit 2a1984f

Browse filesBrowse files
mafredrimafredri
authored
fix(coderd/externalauth): save refreshed token before validation (#24332)
GitHub rotates refresh tokens on use, invalidating the old token immediately. If post-refresh validation fails (e.g. rate-limited 403 from /user), the new token was silently discarded because the DB save only happened after successful validation. The next refresh attempt would use the stale refresh token, fail permanently, and destroy the token. Move the UpdateExternalAuthLink call to immediately after TokenSource.Token() succeeds. The post-validation save block is removed (dead code after the early save). The DB write uses a detached context (context.WithoutCancel) so a canceled request cannot prevent persistence of the already-consumed refresh token.
1 parent 2ea27e8 commit 2a1984f
Copy full SHA for 2a1984f

2 files changed

+284-29Lines changed: 284 additions & 29 deletions

File tree

Expand file treeCollapse file tree
Open diff view settings
Filter options
Expand file treeCollapse file tree
Open diff view settings
Collapse file

‎coderd/externalauth/externalauth.go‎

Copy file name to clipboardExpand all lines: coderd/externalauth/externalauth.go
+41-27Lines changed: 41 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -261,6 +261,37 @@ func (c *Config) RefreshToken(ctx context.Context, db database.Store, externalAu
261261
return externalAuthLink, xerrors.Errorf("generate token extra: %w", err)
262262
}
263263

264+
// Persist the refreshed token to the DB before validation. GitHub
265+
// rotates refresh tokens on every use, so the old refresh token is
266+
// already invalid on the IDP side. If we validated first and the
267+
// validation endpoint was unavailable (e.g. rate-limited 403), the
268+
// new token would be silently lost and the user would be forced to
269+
// re-authenticate manually.
270+
// Use a detached context for the DB write only. The IDP already
271+
// consumed the old refresh token, so if the caller's request
272+
// context is canceled mid-save, the new token would be lost.
273+
persistCtx, persistCancel := context.WithTimeout(context.WithoutCancel(ctx), 10*time.Second)
274+
defer persistCancel()
275+
276+
originalAccessToken := externalAuthLink.OAuthAccessToken
277+
if token.AccessToken != originalAccessToken {
278+
updatedAuthLink, err := db.UpdateExternalAuthLink(persistCtx, database.UpdateExternalAuthLinkParams{
279+
ProviderID: c.ID,
280+
UserID: externalAuthLink.UserID,
281+
UpdatedAt: dbtime.Now(),
282+
OAuthAccessToken: token.AccessToken,
283+
OAuthAccessTokenKeyID: sql.NullString{}, // dbcrypt will update as required
284+
OAuthRefreshToken: token.RefreshToken,
285+
OAuthRefreshTokenKeyID: sql.NullString{}, // dbcrypt will update as required
286+
OAuthExpiry: token.Expiry,
287+
OAuthExtra: extra,
288+
})
289+
if err != nil {
290+
return updatedAuthLink, xerrors.Errorf("persist refreshed token: %w", err)
291+
}
292+
externalAuthLink = updatedAuthLink
293+
}
294+
264295
r := retry.New(50*time.Millisecond, 200*time.Millisecond)
265296
// See the comment below why the retry and cancel is required.
266297
retryCtx, retryCtxCancel := context.WithTimeout(ctx, time.Second)
@@ -285,35 +316,18 @@ validate:
285316
return externalAuthLink, InvalidTokenError("token failed to validate")
286317
}
287318

288-
if token.AccessToken != externalAuthLink.OAuthAccessToken {
289-
updatedAuthLink, err := db.UpdateExternalAuthLink(ctx, database.UpdateExternalAuthLinkParams{
290-
ProviderID: c.ID,
291-
UserID: externalAuthLink.UserID,
292-
UpdatedAt: dbtime.Now(),
293-
OAuthAccessToken: token.AccessToken,
294-
OAuthAccessTokenKeyID: sql.NullString{}, // dbcrypt will update as required
295-
OAuthRefreshToken: token.RefreshToken,
296-
OAuthRefreshTokenKeyID: sql.NullString{}, // dbcrypt will update as required
297-
OAuthExpiry: token.Expiry,
298-
OAuthExtra: extra,
319+
// Update the associated user's github.com user ID if the token
320+
// is for github.com and validation returned user info.
321+
if token.AccessToken != originalAccessToken && IsGithubDotComURL(c.AuthCodeURL("")) && user != nil {
322+
err = db.UpdateUserGithubComUserID(ctx, database.UpdateUserGithubComUserIDParams{
323+
ID: externalAuthLink.UserID,
324+
GithubComUserID: sql.NullInt64{
325+
Int64: user.ID,
326+
Valid: true,
327+
},
299328
})
300329
if err != nil {
301-
return updatedAuthLink, xerrors.Errorf("update external auth link: %w", err)
302-
}
303-
externalAuthLink = updatedAuthLink
304-
305-
// Update the associated users github.com username if the token is for github.com.
306-
if IsGithubDotComURL(c.AuthCodeURL("")) && user != nil {
307-
err = db.UpdateUserGithubComUserID(ctx, database.UpdateUserGithubComUserIDParams{
308-
ID: externalAuthLink.UserID,
309-
GithubComUserID: sql.NullInt64{
310-
Int64: user.ID,
311-
Valid: true,
312-
},
313-
})
314-
if err != nil {
315-
return externalAuthLink, xerrors.Errorf("update user github com user id: %w", err)
316-
}
330+
return externalAuthLink, xerrors.Errorf("update user github com user id: %w", err)
317331
}
318332
}
319333

Collapse file

‎coderd/externalauth/externalauth_test.go‎

Copy file name to clipboardExpand all lines: coderd/externalauth/externalauth_test.go
+243-2Lines changed: 243 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import (
88
"net/http/httptest"
99
"net/url"
1010
"strings"
11+
"sync/atomic"
1112
"testing"
1213
"time"
1314

@@ -26,6 +27,7 @@ import (
2627
"github.com/coder/coder/v2/coderd/database"
2728
"github.com/coder/coder/v2/coderd/database/dbmock"
2829
"github.com/coder/coder/v2/coderd/database/dbtestutil"
30+
"github.com/coder/coder/v2/coderd/database/dbtime"
2931
"github.com/coder/coder/v2/coderd/externalauth"
3032
"github.com/coder/coder/v2/coderd/promoauth"
3133
"github.com/coder/coder/v2/codersdk"
@@ -119,6 +121,11 @@ func TestRefreshToken(t *testing.T) {
119121
t.Run("ValidateServerError", func(t *testing.T) {
120122
t.Parallel()
121123

124+
ctrl := gomock.NewController(t)
125+
mDB := dbmock.NewMockStore(ctrl)
126+
mDB.EXPECT().UpdateExternalAuthLink(gomock.Any(), gomock.Any()).
127+
Return(database.ExternalAuthLink{}, nil).AnyTimes()
128+
122129
const staticError = "static error"
123130
validated := false
124131
fake, config, link := setupOauth2Test(t, testConfig{
@@ -135,7 +142,7 @@ func TestRefreshToken(t *testing.T) {
135142
ctx := oidc.ClientContext(context.Background(), fake.HTTPClient(nil))
136143
link.OAuthExpiry = expired
137144

138-
_, err := config.RefreshToken(ctx, nil, link)
145+
_, err := config.RefreshToken(ctx, mDB, link)
139146
require.ErrorContains(t, err, staticError)
140147
// Unsure if this should be the correct behavior. It's an invalid token because
141148
// 'ValidateToken()' failed with a runtime error. This was the previous behavior,
@@ -222,6 +229,11 @@ func TestRefreshToken(t *testing.T) {
222229
t.Run("ValidateFailure", func(t *testing.T) {
223230
t.Parallel()
224231

232+
ctrl := gomock.NewController(t)
233+
mDB := dbmock.NewMockStore(ctrl)
234+
mDB.EXPECT().UpdateExternalAuthLink(gomock.Any(), gomock.Any()).
235+
Return(database.ExternalAuthLink{}, nil).AnyTimes()
236+
225237
const staticError = "static error"
226238
validated := false
227239
fake, config, link := setupOauth2Test(t, testConfig{
@@ -238,7 +250,7 @@ func TestRefreshToken(t *testing.T) {
238250
ctx := oidc.ClientContext(context.Background(), fake.HTTPClient(nil))
239251
link.OAuthExpiry = expired
240252

241-
_, err := config.RefreshToken(ctx, nil, link)
253+
_, err := config.RefreshToken(ctx, mDB, link)
242254
require.ErrorContains(t, err, "token failed to validate")
243255
require.True(t, externalauth.IsInvalidTokenError(err))
244256
require.True(t, validated, "token should have been attempted to be validated")
@@ -379,6 +391,235 @@ func TestRefreshToken(t *testing.T) {
379391
require.True(t, ok)
380392
require.Equal(t, updated.OAuthAccessToken, mapping["access_token"])
381393
})
394+
395+
// SaveBeforeValidate tests that a successfully refreshed token is
396+
// persisted to the DB even when post-refresh validation fails. This
397+
// prevents the data-loss scenario where GitHub rotates the refresh
398+
// token on use but the new token is silently discarded because a
399+
// rate-limited validation endpoint returns 403.
400+
t.Run("SaveBeforeValidate", func(t *testing.T) {
401+
t.Parallel()
402+
403+
db, _ := dbtestutil.NewDB(t)
404+
405+
// simulateRateLimit controls whether the validate endpoint
406+
// returns 403 (true) or 200 (false).
407+
var simulateRateLimit atomic.Bool
408+
simulateRateLimit.Store(true)
409+
410+
var refreshCalls atomic.Int64
411+
fake, config, link := setupOauth2Test(t, testConfig{
412+
FakeIDPOpts: []oidctest.FakeIDPOpt{
413+
oidctest.WithRefresh(func(_ string) error {
414+
refreshCalls.Add(1)
415+
return nil
416+
}),
417+
oidctest.WithDynamicUserInfo(func(_ string) (jwt.MapClaims, error) {
418+
if simulateRateLimit.Load() {
419+
return jwt.MapClaims{}, oidctest.StatusError(http.StatusForbidden, xerrors.New("rate limit exceeded"))
420+
}
421+
return jwt.MapClaims{}, nil
422+
}),
423+
},
424+
ExternalAuthOpt: func(cfg *externalauth.Config) {
425+
cfg.Type = codersdk.EnhancedExternalAuthProviderGitHub.String()
426+
},
427+
DB: db,
428+
})
429+
430+
ctx := oidc.ClientContext(context.Background(), fake.HTTPClient(nil))
431+
432+
oldAccessToken := link.OAuthAccessToken
433+
oldRefreshToken := link.OAuthRefreshToken
434+
435+
// Expire the token to force a refresh.
436+
link.OAuthExpiry = expired
437+
438+
// First call: refresh succeeds, validation fails (403).
439+
_, err := config.RefreshToken(ctx, db, link)
440+
require.Error(t, err, "expected error because validation returned 403")
441+
require.True(t, externalauth.IsInvalidTokenError(err))
442+
require.Equal(t, int64(1), refreshCalls.Load(), "IDP refresh should have been called exactly once")
443+
444+
// Critical assertion: the DB must contain the NEW tokens from the
445+
// successful refresh, not the old (now-stale) ones.
446+
dbLink, err := db.GetExternalAuthLink(context.Background(), database.GetExternalAuthLinkParams{
447+
ProviderID: link.ProviderID,
448+
UserID: link.UserID,
449+
})
450+
require.NoError(t, err)
451+
require.NotEqual(t, oldAccessToken, dbLink.OAuthAccessToken,
452+
"DB should have the new access token from the successful refresh")
453+
require.NotEqual(t, oldRefreshToken, dbLink.OAuthRefreshToken,
454+
"DB should have the new refresh token (old one was rotated by the IDP)")
455+
456+
// Second call: uses the saved token from DB, no re-refresh.
457+
// The saved token has a future expiry, so TokenSource should return
458+
// it without contacting the IDP. Validation should succeed now.
459+
simulateRateLimit.Store(false)
460+
updated, err := config.RefreshToken(ctx, db, dbLink)
461+
require.NoError(t, err, "second call should succeed because rate limit lifted")
462+
require.Equal(t, int64(1), refreshCalls.Load(),
463+
"IDP refresh should NOT have been called again; the saved token is not expired")
464+
require.Equal(t, dbLink.OAuthAccessToken, updated.OAuthAccessToken,
465+
"returned token should match what was saved in the DB")
466+
})
467+
468+
// SaveBeforeValidate_ContextCanceled verifies the early DB save
469+
// uses a detached context. The parent context is canceled inside
470+
// the refresh hook (after TokenSource.Token() but before the DB
471+
// write), and the test asserts the new token is still persisted.
472+
t.Run("SaveBeforeValidate_ContextCanceled", func(t *testing.T) {
473+
t.Parallel()
474+
475+
db, _ := dbtestutil.NewDB(t)
476+
477+
var refreshCalls atomic.Int64
478+
cancelOnRefresh, cancel := context.WithCancel(context.Background())
479+
defer cancel()
480+
481+
fake, config, link := setupOauth2Test(t, testConfig{
482+
FakeIDPOpts: []oidctest.FakeIDPOpt{
483+
oidctest.WithRefresh(func(_ string) error {
484+
refreshCalls.Add(1)
485+
// Cancel the parent context after refresh succeeds
486+
// but before the DB save and validation.
487+
cancel()
488+
return nil
489+
}),
490+
oidctest.WithDynamicUserInfo(func(_ string) (jwt.MapClaims, error) {
491+
return jwt.MapClaims{}, nil
492+
}),
493+
},
494+
ExternalAuthOpt: func(cfg *externalauth.Config) {
495+
cfg.Type = codersdk.EnhancedExternalAuthProviderGitHub.String()
496+
},
497+
DB: db,
498+
})
499+
500+
ctx := oidc.ClientContext(cancelOnRefresh, fake.HTTPClient(nil))
501+
502+
oldAccessToken := link.OAuthAccessToken
503+
oldRefreshToken := link.OAuthRefreshToken
504+
link.OAuthExpiry = expired
505+
506+
_, err := config.RefreshToken(ctx, db, link)
507+
require.NoError(t, err)
508+
require.Equal(t, int64(1), refreshCalls.Load())
509+
510+
dbLink, err := db.GetExternalAuthLink(context.Background(), database.GetExternalAuthLinkParams{
511+
ProviderID: link.ProviderID,
512+
UserID: link.UserID,
513+
})
514+
require.NoError(t, err)
515+
require.NotEqual(t, oldAccessToken, dbLink.OAuthAccessToken,
516+
"DB should have the new access token despite context cancellation")
517+
require.NotEqual(t, oldRefreshToken, dbLink.OAuthRefreshToken,
518+
"DB should have the new refresh token despite context cancellation")
519+
})
520+
521+
// SaveBeforeValidate_DBError tests that when the early DB save
522+
// fails after a successful IDP refresh, the error is surfaced
523+
// as a non-InvalidTokenError. This is a degraded state (token
524+
// issued by IDP but not persisted), and callers should see a
525+
// real error, not a "please re-authenticate" prompt.
526+
t.Run("SaveBeforeValidate_DBError", func(t *testing.T) {
527+
t.Parallel()
528+
529+
ctrl := gomock.NewController(t)
530+
mDB := dbmock.NewMockStore(ctrl)
531+
532+
fake, config, link := setupOauth2Test(t, testConfig{
533+
FakeIDPOpts: []oidctest.FakeIDPOpt{
534+
oidctest.WithRefresh(func(_ string) error {
535+
return nil
536+
}),
537+
},
538+
ExternalAuthOpt: func(cfg *externalauth.Config) {
539+
cfg.Type = codersdk.EnhancedExternalAuthProviderGitHub.String()
540+
},
541+
})
542+
543+
ctx := oidc.ClientContext(context.Background(), fake.HTTPClient(nil))
544+
link.OAuthExpiry = expired
545+
546+
mDB.EXPECT().
547+
UpdateExternalAuthLink(gomock.Any(), gomock.Any()).
548+
Return(database.ExternalAuthLink{}, xerrors.New("db connection lost"))
549+
550+
_, err := config.RefreshToken(ctx, mDB, link)
551+
require.Error(t, err)
552+
require.Contains(t, err.Error(), "persist refreshed token")
553+
require.False(t, externalauth.IsInvalidTokenError(err),
554+
"DB errors should not be treated as invalid token")
555+
})
556+
557+
// OptimisticLockPreventsStaleOverwrite verifies that the
558+
// UpdateExternalAuthLinkRefreshToken WHERE clause prevents a
559+
// stale caller from overwriting a valid refresh token saved
560+
// by a concurrent winner.
561+
t.Run("OptimisticLockPreventsStaleOverwrite", func(t *testing.T) {
562+
t.Parallel()
563+
564+
db, _ := dbtestutil.NewDB(t)
565+
566+
fake, config, link := setupOauth2Test(t, testConfig{
567+
FakeIDPOpts: []oidctest.FakeIDPOpt{
568+
oidctest.WithRefresh(func(_ string) error {
569+
return nil
570+
}),
571+
oidctest.WithDynamicUserInfo(func(_ string) (jwt.MapClaims, error) {
572+
return jwt.MapClaims{}, nil
573+
}),
574+
},
575+
ExternalAuthOpt: func(cfg *externalauth.Config) {
576+
cfg.Type = codersdk.EnhancedExternalAuthProviderGitHub.String()
577+
},
578+
DB: db,
579+
})
580+
581+
ctx := oidc.ClientContext(context.Background(), fake.HTTPClient(nil))
582+
583+
// Snapshot the original tokens before any refresh.
584+
oldRefreshToken := link.OAuthRefreshToken
585+
586+
// Expire the token to force a refresh.
587+
link.OAuthExpiry = expired
588+
589+
// Caller A: refresh and save successfully.
590+
updated, err := config.RefreshToken(ctx, db, link)
591+
require.NoError(t, err)
592+
require.NotEqual(t, oldRefreshToken, updated.OAuthRefreshToken,
593+
"caller A should have a new refresh token")
594+
595+
// Caller B had a stale read of the original link. It tries to
596+
// destroy the refresh token using the OLD refresh token in the
597+
// optimistic lock. Because caller A already wrote a different
598+
// refresh token, this WHERE clause matches nothing.
599+
err = db.UpdateExternalAuthLinkRefreshToken(ctx, database.UpdateExternalAuthLinkRefreshTokenParams{
600+
OauthRefreshFailureReason: "simulated failure from stale caller B",
601+
OAuthRefreshToken: "",
602+
OAuthRefreshTokenKeyID: "",
603+
UpdatedAt: dbtime.Now(),
604+
ProviderID: link.ProviderID,
605+
UserID: link.UserID,
606+
OldOauthRefreshToken: oldRefreshToken,
607+
})
608+
require.NoError(t, err, "optimistic lock write should not error, it is a no-op")
609+
610+
// Verify DB still has caller A's valid token.
611+
dbLink, err := db.GetExternalAuthLink(context.Background(), database.GetExternalAuthLinkParams{
612+
ProviderID: link.ProviderID,
613+
UserID: link.UserID,
614+
})
615+
require.NoError(t, err)
616+
require.Equal(t, updated.OAuthAccessToken, dbLink.OAuthAccessToken,
617+
"caller A's access token should still be in DB")
618+
require.Equal(t, updated.OAuthRefreshToken, dbLink.OAuthRefreshToken,
619+
"caller A's refresh token should still be in DB")
620+
require.Empty(t, dbLink.OauthRefreshFailureReason,
621+
"caller B's failure reason should not have been written")
622+
})
382623
}
383624

384625
func TestRevokeToken(t *testing.T) {

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.