Skip to content

Commit 3c05576

Browse files
mafredrimafredri
committed
fix(coderd/externalauth): save refreshed token before validation
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 (now-invalid) refresh token, fail permanently, and destroy the token. Move the UpdateExternalAuthLink call from after validation to immediately after TokenSource.Token() succeeds. The existing post-validation save becomes a no-op when the early save fires.
1 parent 155e989 commit 3c05576

2 files changed

Lines changed: 182 additions & 12 deletions

File tree

‎coderd/externalauth/externalauth.go‎

Lines changed: 37 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -261,6 +261,30 @@ 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+
if db != nil && token.AccessToken != externalAuthLink.OAuthAccessToken {
271+
updatedAuthLink, err := db.UpdateExternalAuthLink(ctx, database.UpdateExternalAuthLinkParams{
272+
ProviderID: c.ID,
273+
UserID: externalAuthLink.UserID,
274+
UpdatedAt: dbtime.Now(),
275+
OAuthAccessToken: token.AccessToken,
276+
OAuthAccessTokenKeyID: sql.NullString{}, // dbcrypt will update as required
277+
OAuthRefreshToken: token.RefreshToken,
278+
OAuthRefreshTokenKeyID: sql.NullString{}, // dbcrypt will update as required
279+
OAuthExpiry: token.Expiry,
280+
OAuthExtra: extra,
281+
})
282+
if err != nil {
283+
return updatedAuthLink, xerrors.Errorf("update external auth link: %w", err)
284+
}
285+
externalAuthLink = updatedAuthLink
286+
}
287+
264288
r := retry.New(50*time.Millisecond, 200*time.Millisecond)
265289
// See the comment below why the retry and cancel is required.
266290
retryCtx, retryCtxCancel := context.WithTimeout(ctx, time.Second)
@@ -301,19 +325,20 @@ validate:
301325
return updatedAuthLink, xerrors.Errorf("update external auth link: %w", err)
302326
}
303327
externalAuthLink = updatedAuthLink
328+
}
304329

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+
// Update the associated users github.com username if the token
331+
// is for github.com and validation returned user info.
332+
if IsGithubDotComURL(c.AuthCodeURL("")) && user != nil {
333+
err = db.UpdateUserGithubComUserID(ctx, database.UpdateUserGithubComUserIDParams{
334+
ID: externalAuthLink.UserID,
335+
GithubComUserID: sql.NullInt64{
336+
Int64: user.ID,
337+
Valid: true,
338+
},
339+
})
340+
if err != nil {
341+
return externalAuthLink, xerrors.Errorf("update user github com user id: %w", err)
317342
}
318343
}
319344

‎coderd/externalauth/externalauth_test.go‎

Lines changed: 145 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,12 +2,14 @@ package externalauth_test
22

33
import (
44
"context"
5+
"database/sql"
56
"encoding/json"
67
"fmt"
78
"net/http"
89
"net/http/httptest"
910
"net/url"
1011
"strings"
12+
"sync/atomic"
1113
"testing"
1214
"time"
1315

@@ -26,6 +28,7 @@ import (
2628
"github.com/coder/coder/v2/coderd/database"
2729
"github.com/coder/coder/v2/coderd/database/dbmock"
2830
"github.com/coder/coder/v2/coderd/database/dbtestutil"
31+
"github.com/coder/coder/v2/coderd/database/dbtime"
2932
"github.com/coder/coder/v2/coderd/externalauth"
3033
"github.com/coder/coder/v2/coderd/promoauth"
3134
"github.com/coder/coder/v2/codersdk"
@@ -379,6 +382,148 @@ func TestRefreshToken(t *testing.T) {
379382
require.True(t, ok)
380383
require.Equal(t, updated.OAuthAccessToken, mapping["access_token"])
381384
})
385+
386+
// SaveBeforeValidate tests that a successfully refreshed token is
387+
// persisted to the DB even when post-refresh validation fails. This
388+
// prevents the data-loss scenario where GitHub rotates the refresh
389+
// token on use but the new token is silently discarded because a
390+
// rate-limited validation endpoint returns 403.
391+
t.Run("SaveBeforeValidate", func(t *testing.T) {
392+
t.Parallel()
393+
394+
db, _ := dbtestutil.NewDB(t)
395+
396+
// simulateRateLimit controls whether the validate endpoint
397+
// returns 403 (true) or 200 (false).
398+
var simulateRateLimit atomic.Bool
399+
simulateRateLimit.Store(true)
400+
401+
var refreshCalls atomic.Int64
402+
fake, config, link := setupOauth2Test(t, testConfig{
403+
FakeIDPOpts: []oidctest.FakeIDPOpt{
404+
oidctest.WithRefresh(func(_ string) error {
405+
refreshCalls.Add(1)
406+
return nil
407+
}),
408+
oidctest.WithDynamicUserInfo(func(_ string) (jwt.MapClaims, error) {
409+
if simulateRateLimit.Load() {
410+
return jwt.MapClaims{}, oidctest.StatusError(http.StatusForbidden, xerrors.New("rate limit exceeded"))
411+
}
412+
return jwt.MapClaims{}, nil
413+
}),
414+
},
415+
ExternalAuthOpt: func(cfg *externalauth.Config) {
416+
cfg.Type = codersdk.EnhancedExternalAuthProviderGitHub.String()
417+
},
418+
DB: db,
419+
})
420+
421+
ctx := oidc.ClientContext(context.Background(), fake.HTTPClient(nil))
422+
423+
oldAccessToken := link.OAuthAccessToken
424+
oldRefreshToken := link.OAuthRefreshToken
425+
426+
// Expire the token to force a refresh.
427+
link.OAuthExpiry = expired
428+
429+
// --- First call: refresh succeeds, validation fails (403). ---
430+
_, err := config.RefreshToken(ctx, db, link)
431+
require.Error(t, err, "expected error because validation returned 403")
432+
require.True(t, externalauth.IsInvalidTokenError(err))
433+
require.Equal(t, int64(1), refreshCalls.Load(), "IDP refresh should have been called exactly once")
434+
435+
// Critical assertion: the DB must contain the NEW tokens from the
436+
// successful refresh, not the old (now-stale) ones.
437+
dbLink, err := db.GetExternalAuthLink(context.Background(), database.GetExternalAuthLinkParams{
438+
ProviderID: link.ProviderID,
439+
UserID: link.UserID,
440+
})
441+
require.NoError(t, err)
442+
require.NotEqual(t, oldAccessToken, dbLink.OAuthAccessToken,
443+
"DB should have the new access token from the successful refresh")
444+
require.NotEqual(t, oldRefreshToken, dbLink.OAuthRefreshToken,
445+
"DB should have the new refresh token (old one was rotated by the IDP)")
446+
447+
// --- Second call: uses the saved token from DB, no re-refresh. ---
448+
// The saved token has a future expiry, so TokenSource should return
449+
// it without contacting the IDP. Validation should succeed now.
450+
simulateRateLimit.Store(false)
451+
updated, err := config.RefreshToken(ctx, db, dbLink)
452+
require.NoError(t, err, "second call should succeed because rate limit lifted")
453+
require.Equal(t, int64(1), refreshCalls.Load(),
454+
"IDP refresh should NOT have been called again; the saved token is not expired")
455+
require.Equal(t, dbLink.OAuthAccessToken, updated.OAuthAccessToken,
456+
"returned token should match what was saved in the DB")
457+
})
458+
459+
// ConcurrentRefreshOptimisticLock verifies that when caller A saves
460+
// a successfully refreshed token (early save), caller B's subsequent
461+
// attempt to destroy the refresh token via
462+
// UpdateExternalAuthLinkRefreshToken is a no-op. The optimistic lock
463+
// (WHERE oauth_refresh_token = @old) prevents B from overwriting
464+
// A's valid token.
465+
t.Run("ConcurrentRefreshOptimisticLock", func(t *testing.T) {
466+
t.Parallel()
467+
468+
db, _ := dbtestutil.NewDB(t)
469+
470+
fake, config, link := setupOauth2Test(t, testConfig{
471+
FakeIDPOpts: []oidctest.FakeIDPOpt{
472+
oidctest.WithRefresh(func(_ string) error {
473+
return nil
474+
}),
475+
oidctest.WithDynamicUserInfo(func(_ string) (jwt.MapClaims, error) {
476+
return jwt.MapClaims{}, nil
477+
}),
478+
},
479+
ExternalAuthOpt: func(cfg *externalauth.Config) {
480+
cfg.Type = codersdk.EnhancedExternalAuthProviderGitHub.String()
481+
},
482+
DB: db,
483+
})
484+
485+
ctx := oidc.ClientContext(context.Background(), fake.HTTPClient(nil))
486+
487+
// Snapshot the original tokens before any refresh.
488+
oldRefreshToken := link.OAuthRefreshToken
489+
490+
// Expire the token to force a refresh.
491+
link.OAuthExpiry = expired
492+
493+
// Caller A: refresh and save successfully.
494+
updated, err := config.RefreshToken(ctx, db, link)
495+
require.NoError(t, err)
496+
require.NotEqual(t, oldRefreshToken, updated.OAuthRefreshToken,
497+
"caller A should have a new refresh token")
498+
499+
// Caller B had a stale read of the original link. It tries to
500+
// destroy the refresh token using the OLD refresh token in the
501+
// optimistic lock. Because caller A already wrote a different
502+
// refresh token, this WHERE clause matches nothing.
503+
err = db.UpdateExternalAuthLinkRefreshToken(ctx, database.UpdateExternalAuthLinkRefreshTokenParams{
504+
OauthRefreshFailureReason: "simulated failure from stale caller B",
505+
OAuthRefreshToken: "",
506+
OAuthRefreshTokenKeyID: sql.NullString{}.String,
507+
UpdatedAt: dbtime.Now(),
508+
ProviderID: link.ProviderID,
509+
UserID: link.UserID,
510+
OldOauthRefreshToken: oldRefreshToken,
511+
})
512+
require.NoError(t, err, "optimistic lock write should not error, it is a no-op")
513+
514+
// Verify DB still has caller A's valid token.
515+
dbLink, err := db.GetExternalAuthLink(context.Background(), database.GetExternalAuthLinkParams{
516+
ProviderID: link.ProviderID,
517+
UserID: link.UserID,
518+
})
519+
require.NoError(t, err)
520+
require.Equal(t, updated.OAuthAccessToken, dbLink.OAuthAccessToken,
521+
"caller A's access token should still be in DB")
522+
require.Equal(t, updated.OAuthRefreshToken, dbLink.OAuthRefreshToken,
523+
"caller A's refresh token should still be in DB")
524+
require.Empty(t, dbLink.OauthRefreshFailureReason,
525+
"caller B's failure reason should not have been written")
526+
})
382527
}
383528

384529
func TestRevokeToken(t *testing.T) {

0 commit comments

Comments
 (0)