Skip to content

Commit 5c7f06a

Browse files
ThomasK33ThomasK33
committed
feat: add cleanup for expired OAuth2 provider app codes and tokens
Change-Id: I07e7c229efa6e92282885464d2193dfc4c2e1c98 Signed-off-by: Thomas Kosiewski <tk@coder.com>
1 parent 3dd0c37 commit 5c7f06a

File tree

9 files changed

+318
-2
lines changed

9 files changed

+318
-2
lines changed

‎coderd/database/dbauthz/dbauthz.go

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1446,6 +1446,22 @@ func (q *querier) DeleteCustomRole(ctx context.Context, arg database.DeleteCusto
14461446
return q.db.DeleteCustomRole(ctx, arg)
14471447
}
14481448

1449+
func (q *querier) DeleteExpiredOAuth2ProviderAppCodes(ctx context.Context) error {
1450+
// System operation - only system can clean up expired authorization codes
1451+
if err := q.authorizeContext(ctx, policy.ActionDelete, rbac.ResourceSystem); err != nil {
1452+
return err
1453+
}
1454+
return q.db.DeleteExpiredOAuth2ProviderAppCodes(ctx)
1455+
}
1456+
1457+
func (q *querier) DeleteExpiredOAuth2ProviderAppTokens(ctx context.Context) error {
1458+
// System operation - only system can clean up expired access tokens
1459+
if err := q.authorizeContext(ctx, policy.ActionDelete, rbac.ResourceSystem); err != nil {
1460+
return err
1461+
}
1462+
return q.db.DeleteExpiredOAuth2ProviderAppTokens(ctx)
1463+
}
1464+
14491465
func (q *querier) DeleteExpiredOAuth2ProviderDeviceCodes(ctx context.Context) error {
14501466
// System operation - only system can clean up expired device codes
14511467
if err := q.authorizeContext(ctx, policy.ActionDelete, rbac.ResourceSystem); err != nil {

‎coderd/database/dbauthz/dbauthz_test.go

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5460,6 +5460,9 @@ func (s *MethodTestSuite) TestOAuth2ProviderAppCodes() {
54605460
})
54615461
check.Args(code.SecretPrefix).Asserts(code, policy.ActionUpdate).Returns(code)
54625462
}))
5463+
s.Run("DeleteExpiredOAuth2ProviderAppCodes", s.Subtest(func(db database.Store, check *expects) {
5464+
check.Args().Asserts(rbac.ResourceSystem, policy.ActionDelete)
5465+
}))
54635466
}
54645467

54655468
func (s *MethodTestSuite) TestOAuth2ProviderAppTokens() {
@@ -5533,6 +5536,9 @@ func (s *MethodTestSuite) TestOAuth2ProviderAppTokens() {
55335536
UserID: user.ID,
55345537
}).Asserts(rbac.ResourceOauth2AppCodeToken.WithOwner(user.ID.String()), policy.ActionDelete)
55355538
}))
5539+
s.Run("DeleteExpiredOAuth2ProviderAppTokens", s.Subtest(func(db database.Store, check *expects) {
5540+
check.Args().Asserts(rbac.ResourceSystem, policy.ActionDelete)
5541+
}))
55365542
}
55375543

55385544
func (s *MethodTestSuite) TestOAuth2ProviderDeviceCodes() {

‎coderd/database/dbmetrics/querymetrics.go

Lines changed: 14 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/database/dbmock/dbmock.go

Lines changed: 28 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/database/dbpurge/dbpurge.go

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,15 @@ func New(ctx context.Context, logger slog.Logger, db database.Store, clk quartz.
6262
if err := tx.DeleteOldNotificationMessages(ctx); err != nil {
6363
return xerrors.Errorf("failed to delete old notification messages: %w", err)
6464
}
65+
if err := tx.DeleteExpiredOAuth2ProviderAppCodes(ctx); err != nil {
66+
return xerrors.Errorf("failed to delete expired oauth2 provider app codes: %w", err)
67+
}
68+
if err := tx.DeleteExpiredOAuth2ProviderAppTokens(ctx); err != nil {
69+
return xerrors.Errorf("failed to delete expired oauth2 provider app tokens: %w", err)
70+
}
71+
if err := tx.DeleteExpiredOAuth2ProviderDeviceCodes(ctx); err != nil {
72+
return xerrors.Errorf("failed to delete expired oauth2 provider device codes: %w", err)
73+
}
6574

6675
logger.Debug(ctx, "purged old database entries", slog.F("duration", clk.Since(start)))
6776

‎coderd/database/dbpurge/dbpurge_test.go

Lines changed: 213 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -490,3 +490,216 @@ func containsProvisionerDaemon(daemons []database.ProvisionerDaemon, name string
490490
return d.Name == name
491491
})
492492
}
493+
494+
//nolint:paralleltest // It uses LockIDDBPurge.
495+
func TestDeleteExpiredOAuth2ProviderAppCodes(t *testing.T) {
496+
ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitShort)
497+
defer cancel()
498+
499+
clk := quartz.NewMock(t)
500+
db, _ := dbtestutil.NewDB(t, dbtestutil.WithDumpOnFailure())
501+
logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true})
502+
503+
now := dbtime.Now()
504+
clk.Set(now).MustWait(ctx)
505+
506+
// Create test data
507+
user := dbgen.User(t, db, database.User{})
508+
app := dbgen.OAuth2ProviderApp(t, db, database.OAuth2ProviderApp{
509+
Name: fmt.Sprintf("test-codes-%d", time.Now().UnixNano()),
510+
})
511+
512+
// Create expired authorization code (should be deleted)
513+
expiredCode := dbgen.OAuth2ProviderAppCode(t, db, database.OAuth2ProviderAppCode{
514+
ExpiresAt: now.Add(-1 * time.Hour), // Expired 1 hour ago
515+
AppID: app.ID,
516+
UserID: user.ID,
517+
SecretPrefix: []byte(fmt.Sprintf("expired-%d", time.Now().UnixNano())),
518+
})
519+
520+
// Create non-expired authorization code (should be retained)
521+
validCode := dbgen.OAuth2ProviderAppCode(t, db, database.OAuth2ProviderAppCode{
522+
ExpiresAt: now.Add(1 * time.Hour), // Expires in 1 hour
523+
AppID: app.ID,
524+
UserID: user.ID,
525+
SecretPrefix: []byte(fmt.Sprintf("valid-%d", time.Now().UnixNano())),
526+
})
527+
528+
// Verify codes exist initially
529+
_, err := db.GetOAuth2ProviderAppCodeByID(ctx, expiredCode.ID)
530+
require.NoError(t, err)
531+
_, err = db.GetOAuth2ProviderAppCodeByID(ctx, validCode.ID)
532+
require.NoError(t, err)
533+
534+
// Run cleanup
535+
done := awaitDoTick(ctx, t, clk)
536+
closer := dbpurge.New(ctx, logger, db, clk)
537+
defer closer.Close()
538+
<-done
539+
540+
// Verify expired code is deleted
541+
_, err = db.GetOAuth2ProviderAppCodeByID(ctx, expiredCode.ID)
542+
require.Error(t, err)
543+
require.ErrorIs(t, err, sql.ErrNoRows)
544+
545+
// Verify non-expired code is retained
546+
_, err = db.GetOAuth2ProviderAppCodeByID(ctx, validCode.ID)
547+
require.NoError(t, err)
548+
}
549+
550+
//nolint:paralleltest // It uses LockIDDBPurge.
551+
func TestDeleteExpiredOAuth2ProviderAppTokens(t *testing.T) {
552+
ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitShort)
553+
defer cancel()
554+
555+
clk := quartz.NewMock(t)
556+
db, _ := dbtestutil.NewDB(t, dbtestutil.WithDumpOnFailure())
557+
logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true})
558+
559+
now := dbtime.Now()
560+
clk.Set(now).MustWait(ctx)
561+
562+
// Create test data
563+
user := dbgen.User(t, db, database.User{})
564+
app := dbgen.OAuth2ProviderApp(t, db, database.OAuth2ProviderApp{
565+
Name: fmt.Sprintf("test-tokens-%d", time.Now().UnixNano()),
566+
})
567+
appSecret := dbgen.OAuth2ProviderAppSecret(t, db, database.OAuth2ProviderAppSecret{
568+
AppID: app.ID,
569+
})
570+
571+
// Create API keys for the tokens
572+
expiredAPIKey, _ := dbgen.APIKey(t, db, database.APIKey{
573+
UserID: user.ID,
574+
ExpiresAt: now.Add(-1 * time.Hour),
575+
})
576+
validAPIKey, _ := dbgen.APIKey(t, db, database.APIKey{
577+
UserID: user.ID,
578+
ExpiresAt: now.Add(24 * time.Hour), // Valid for 24 hours
579+
})
580+
581+
// Create expired access token (should be deleted)
582+
expiredToken := dbgen.OAuth2ProviderAppToken(t, db, database.OAuth2ProviderAppToken{
583+
ExpiresAt: now.Add(-1 * time.Hour), // Expired 1 hour ago
584+
AppSecretID: appSecret.ID,
585+
APIKeyID: expiredAPIKey.ID,
586+
UserID: user.ID,
587+
HashPrefix: []byte(fmt.Sprintf("expired-%d", time.Now().UnixNano())),
588+
})
589+
590+
// Create non-expired access token (should be retained)
591+
validToken := dbgen.OAuth2ProviderAppToken(t, db, database.OAuth2ProviderAppToken{
592+
ExpiresAt: now.Add(1 * time.Hour), // Expires in 1 hour
593+
AppSecretID: appSecret.ID,
594+
APIKeyID: validAPIKey.ID,
595+
UserID: user.ID,
596+
HashPrefix: []byte(fmt.Sprintf("valid-%d", time.Now().UnixNano())),
597+
})
598+
599+
// Verify tokens exist initially
600+
_, err := db.GetOAuth2ProviderAppTokenByPrefix(ctx, expiredToken.HashPrefix)
601+
require.NoError(t, err)
602+
_, err = db.GetOAuth2ProviderAppTokenByPrefix(ctx, validToken.HashPrefix)
603+
require.NoError(t, err)
604+
605+
// Run cleanup
606+
done := awaitDoTick(ctx, t, clk)
607+
closer := dbpurge.New(ctx, logger, db, clk)
608+
defer closer.Close()
609+
<-done
610+
611+
// Verify expired token is deleted
612+
_, err = db.GetOAuth2ProviderAppTokenByPrefix(ctx, expiredToken.HashPrefix)
613+
require.Error(t, err)
614+
require.ErrorIs(t, err, sql.ErrNoRows)
615+
616+
// Verify non-expired token is retained
617+
_, err = db.GetOAuth2ProviderAppTokenByPrefix(ctx, validToken.HashPrefix)
618+
require.NoError(t, err)
619+
}
620+
621+
//nolint:paralleltest // It uses LockIDDBPurge.
622+
func TestDeleteExpiredOAuth2ProviderDeviceCodes(t *testing.T) {
623+
ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitShort)
624+
defer cancel()
625+
626+
clk := quartz.NewMock(t)
627+
db, _ := dbtestutil.NewDB(t, dbtestutil.WithDumpOnFailure())
628+
logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true})
629+
630+
now := dbtime.Now()
631+
clk.Set(now).MustWait(ctx)
632+
633+
// Create test data
634+
app := dbgen.OAuth2ProviderApp(t, db, database.OAuth2ProviderApp{
635+
Name: fmt.Sprintf("test-device-%d", time.Now().UnixNano()),
636+
})
637+
638+
nanoTime := time.Now().UnixNano()
639+
640+
// Create expired device code with pending status (should be deleted)
641+
expiredPendingCode := dbgen.OAuth2ProviderDeviceCode(t, db, database.OAuth2ProviderDeviceCode{
642+
ExpiresAt: now.Add(-1 * time.Hour), // Expired 1 hour ago
643+
ClientID: app.ID,
644+
Status: database.OAuth2DeviceStatusPending,
645+
DeviceCodePrefix: fmt.Sprintf("exp-pending-%d", nanoTime),
646+
UserCode: fmt.Sprintf("EP%06d", nanoTime%1000000),
647+
DeviceCodeHash: []byte(fmt.Sprintf("hash-exp-pending-%d", nanoTime)),
648+
})
649+
650+
// Create non-expired device code with pending status (should be retained)
651+
validPendingCode := dbgen.OAuth2ProviderDeviceCode(t, db, database.OAuth2ProviderDeviceCode{
652+
ExpiresAt: now.Add(1 * time.Hour), // Expires in 1 hour
653+
ClientID: app.ID,
654+
Status: database.OAuth2DeviceStatusPending,
655+
DeviceCodePrefix: fmt.Sprintf("val-pending-%d", nanoTime+1),
656+
UserCode: fmt.Sprintf("VP%06d", (nanoTime+1)%1000000),
657+
DeviceCodeHash: []byte(fmt.Sprintf("hash-val-pending-%d", nanoTime+1)),
658+
})
659+
660+
// Create expired device code with authorized status (should be deleted - all expired codes are deleted)
661+
expiredAuthorizedCode := dbgen.OAuth2ProviderDeviceCode(t, db, database.OAuth2ProviderDeviceCode{
662+
ExpiresAt: now.Add(-1 * time.Hour), // Expired 1 hour ago
663+
ClientID: app.ID,
664+
DeviceCodePrefix: fmt.Sprintf("exp-auth-%d", nanoTime+2),
665+
UserCode: fmt.Sprintf("EA%06d", (nanoTime+2)%1000000),
666+
DeviceCodeHash: []byte(fmt.Sprintf("hash-exp-auth-%d", nanoTime+2)),
667+
})
668+
669+
// Create a user and authorize the device code
670+
user := dbgen.User(t, db, database.User{})
671+
expiredAuthorizedCode, err := db.UpdateOAuth2ProviderDeviceCodeAuthorization(ctx, database.UpdateOAuth2ProviderDeviceCodeAuthorizationParams{
672+
ID: expiredAuthorizedCode.ID,
673+
UserID: uuid.NullUUID{UUID: user.ID, Valid: true},
674+
Status: database.OAuth2DeviceStatusAuthorized,
675+
})
676+
require.NoError(t, err)
677+
678+
// Verify device codes exist initially
679+
_, err = db.GetOAuth2ProviderDeviceCodeByID(ctx, expiredPendingCode.ID)
680+
require.NoError(t, err)
681+
_, err = db.GetOAuth2ProviderDeviceCodeByID(ctx, validPendingCode.ID)
682+
require.NoError(t, err)
683+
_, err = db.GetOAuth2ProviderDeviceCodeByID(ctx, expiredAuthorizedCode.ID)
684+
require.NoError(t, err)
685+
686+
// Run cleanup
687+
done := awaitDoTick(ctx, t, clk)
688+
closer := dbpurge.New(ctx, logger, db, clk)
689+
defer closer.Close()
690+
<-done
691+
692+
// Verify expired pending device code is deleted
693+
_, err = db.GetOAuth2ProviderDeviceCodeByID(ctx, expiredPendingCode.ID)
694+
require.Error(t, err)
695+
require.ErrorIs(t, err, sql.ErrNoRows)
696+
697+
// Verify non-expired pending device code is retained
698+
_, err = db.GetOAuth2ProviderDeviceCodeByID(ctx, validPendingCode.ID)
699+
require.NoError(t, err)
700+
701+
// Verify expired authorized device code is deleted (all expired codes are deleted)
702+
_, err = db.GetOAuth2ProviderDeviceCodeByID(ctx, expiredAuthorizedCode.ID)
703+
require.Error(t, err)
704+
require.ErrorIs(t, err, sql.ErrNoRows)
705+
}

‎coderd/database/querier.go

Lines changed: 2 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/database/queries.sql.go

Lines changed: 21 additions & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/database/queries/oauth2.sql

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -305,7 +305,15 @@ DELETE FROM oauth2_provider_device_codes WHERE id = $1;
305305

306306
-- name: DeleteExpiredOAuth2ProviderDeviceCodes :exec
307307
DELETE FROM oauth2_provider_device_codes
308-
WHERE expires_at < NOW() AND status = 'pending';
308+
WHERE expires_at < NOW();
309+
310+
-- name: DeleteExpiredOAuth2ProviderAppCodes :exec
311+
DELETE FROM oauth2_provider_app_codes
312+
WHERE expires_at < NOW();
313+
314+
-- name: DeleteExpiredOAuth2ProviderAppTokens :exec
315+
DELETE FROM oauth2_provider_app_tokens
316+
WHERE expires_at < NOW();
309317

310318
-- name: GetOAuth2ProviderDeviceCodesByClientID :many
311319
SELECT * FROM oauth2_provider_device_codes

0 commit comments

Comments
 (0)