Skip to content

Commit 643824a

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 bed62ad commit 643824a

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
@@ -1479,6 +1479,22 @@ func (q *querier) DeleteCustomRole(ctx context.Context, arg database.DeleteCusto
14791479
return q.db.DeleteCustomRole(ctx, arg)
14801480
}
14811481

1482+
func (q *querier) DeleteExpiredOAuth2ProviderAppCodes(ctx context.Context) error {
1483+
// System operation - only system can clean up expired authorization codes
1484+
if err := q.authorizeContext(ctx, policy.ActionDelete, rbac.ResourceSystem); err != nil {
1485+
return err
1486+
}
1487+
return q.db.DeleteExpiredOAuth2ProviderAppCodes(ctx)
1488+
}
1489+
1490+
func (q *querier) DeleteExpiredOAuth2ProviderAppTokens(ctx context.Context) error {
1491+
// System operation - only system can clean up expired access tokens
1492+
if err := q.authorizeContext(ctx, policy.ActionDelete, rbac.ResourceSystem); err != nil {
1493+
return err
1494+
}
1495+
return q.db.DeleteExpiredOAuth2ProviderAppTokens(ctx)
1496+
}
1497+
14821498
func (q *querier) DeleteExpiredOAuth2ProviderDeviceCodes(ctx context.Context) error {
14831499
// System operation - only system can clean up expired device codes
14841500
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
@@ -5569,6 +5569,9 @@ func (s *MethodTestSuite) TestOAuth2ProviderAppCodes() {
55695569
})
55705570
check.Args(code.SecretPrefix).Asserts(code, policy.ActionUpdate).Returns(code)
55715571
}))
5572+
s.Run("DeleteExpiredOAuth2ProviderAppCodes", s.Subtest(func(db database.Store, check *expects) {
5573+
check.Args().Asserts(rbac.ResourceSystem, policy.ActionDelete)
5574+
}))
55725575
}
55735576

55745577
func (s *MethodTestSuite) TestOAuth2ProviderAppTokens() {
@@ -5642,6 +5645,9 @@ func (s *MethodTestSuite) TestOAuth2ProviderAppTokens() {
56425645
UserID: user.ID,
56435646
}).Asserts(rbac.ResourceOauth2AppCodeToken.WithOwner(user.ID.String()), policy.ActionDelete)
56445647
}))
5648+
s.Run("DeleteExpiredOAuth2ProviderAppTokens", s.Subtest(func(db database.Store, check *expects) {
5649+
check.Args().Asserts(rbac.ResourceSystem, policy.ActionDelete)
5650+
}))
56455651
}
56465652

56475653
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
@@ -67,6 +67,15 @@ func New(ctx context.Context, logger slog.Logger, db database.Store, clk quartz.
6767
if err := tx.DeleteOldNotificationMessages(ctx); err != nil {
6868
return xerrors.Errorf("failed to delete old notification messages: %w", err)
6969
}
70+
if err := tx.DeleteExpiredOAuth2ProviderAppCodes(ctx); err != nil {
71+
return xerrors.Errorf("failed to delete expired oauth2 provider app codes: %w", err)
72+
}
73+
if err := tx.DeleteExpiredOAuth2ProviderAppTokens(ctx); err != nil {
74+
return xerrors.Errorf("failed to delete expired oauth2 provider app tokens: %w", err)
75+
}
76+
if err := tx.DeleteExpiredOAuth2ProviderDeviceCodes(ctx); err != nil {
77+
return xerrors.Errorf("failed to delete expired oauth2 provider device codes: %w", err)
78+
}
7079

7180
deleteOldAuditLogConnectionEventsBefore := start.Add(-maxAuditLogConnectionEventAge)
7281
if err := tx.DeleteOldAuditLogConnectionEvents(ctx, database.DeleteOldAuditLogConnectionEventsParams{

‎coderd/database/dbpurge/dbpurge_test.go

Lines changed: 213 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -635,3 +635,216 @@ func TestDeleteOldAuditLogConnectionEventsLimit(t *testing.T) {
635635

636636
require.Len(t, logs, 0)
637637
}
638+
639+
//nolint:paralleltest // It uses LockIDDBPurge.
640+
func TestDeleteExpiredOAuth2ProviderAppCodes(t *testing.T) {
641+
ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitShort)
642+
defer cancel()
643+
644+
clk := quartz.NewMock(t)
645+
db, _ := dbtestutil.NewDB(t, dbtestutil.WithDumpOnFailure())
646+
logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true})
647+
648+
now := dbtime.Now()
649+
clk.Set(now).MustWait(ctx)
650+
651+
// Create test data
652+
user := dbgen.User(t, db, database.User{})
653+
app := dbgen.OAuth2ProviderApp(t, db, database.OAuth2ProviderApp{
654+
Name: fmt.Sprintf("test-codes-%d", time.Now().UnixNano()),
655+
})
656+
657+
// Create expired authorization code (should be deleted)
658+
expiredCode := dbgen.OAuth2ProviderAppCode(t, db, database.OAuth2ProviderAppCode{
659+
ExpiresAt: now.Add(-1 * time.Hour), // Expired 1 hour ago
660+
AppID: app.ID,
661+
UserID: user.ID,
662+
SecretPrefix: []byte(fmt.Sprintf("expired-%d", time.Now().UnixNano())),
663+
})
664+
665+
// Create non-expired authorization code (should be retained)
666+
validCode := dbgen.OAuth2ProviderAppCode(t, db, database.OAuth2ProviderAppCode{
667+
ExpiresAt: now.Add(1 * time.Hour), // Expires in 1 hour
668+
AppID: app.ID,
669+
UserID: user.ID,
670+
SecretPrefix: []byte(fmt.Sprintf("valid-%d", time.Now().UnixNano())),
671+
})
672+
673+
// Verify codes exist initially
674+
_, err := db.GetOAuth2ProviderAppCodeByID(ctx, expiredCode.ID)
675+
require.NoError(t, err)
676+
_, err = db.GetOAuth2ProviderAppCodeByID(ctx, validCode.ID)
677+
require.NoError(t, err)
678+
679+
// Run cleanup
680+
done := awaitDoTick(ctx, t, clk)
681+
closer := dbpurge.New(ctx, logger, db, clk)
682+
defer closer.Close()
683+
<-done
684+
685+
// Verify expired code is deleted
686+
_, err = db.GetOAuth2ProviderAppCodeByID(ctx, expiredCode.ID)
687+
require.Error(t, err)
688+
require.ErrorIs(t, err, sql.ErrNoRows)
689+
690+
// Verify non-expired code is retained
691+
_, err = db.GetOAuth2ProviderAppCodeByID(ctx, validCode.ID)
692+
require.NoError(t, err)
693+
}
694+
695+
//nolint:paralleltest // It uses LockIDDBPurge.
696+
func TestDeleteExpiredOAuth2ProviderAppTokens(t *testing.T) {
697+
ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitShort)
698+
defer cancel()
699+
700+
clk := quartz.NewMock(t)
701+
db, _ := dbtestutil.NewDB(t, dbtestutil.WithDumpOnFailure())
702+
logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true})
703+
704+
now := dbtime.Now()
705+
clk.Set(now).MustWait(ctx)
706+
707+
// Create test data
708+
user := dbgen.User(t, db, database.User{})
709+
app := dbgen.OAuth2ProviderApp(t, db, database.OAuth2ProviderApp{
710+
Name: fmt.Sprintf("test-tokens-%d", time.Now().UnixNano()),
711+
})
712+
appSecret := dbgen.OAuth2ProviderAppSecret(t, db, database.OAuth2ProviderAppSecret{
713+
AppID: app.ID,
714+
})
715+
716+
// Create API keys for the tokens
717+
expiredAPIKey, _ := dbgen.APIKey(t, db, database.APIKey{
718+
UserID: user.ID,
719+
ExpiresAt: now.Add(-1 * time.Hour),
720+
})
721+
validAPIKey, _ := dbgen.APIKey(t, db, database.APIKey{
722+
UserID: user.ID,
723+
ExpiresAt: now.Add(24 * time.Hour), // Valid for 24 hours
724+
})
725+
726+
// Create expired access token (should be deleted)
727+
expiredToken := dbgen.OAuth2ProviderAppToken(t, db, database.OAuth2ProviderAppToken{
728+
ExpiresAt: now.Add(-1 * time.Hour), // Expired 1 hour ago
729+
AppSecretID: appSecret.ID,
730+
APIKeyID: expiredAPIKey.ID,
731+
UserID: user.ID,
732+
HashPrefix: []byte(fmt.Sprintf("expired-%d", time.Now().UnixNano())),
733+
})
734+
735+
// Create non-expired access token (should be retained)
736+
validToken := dbgen.OAuth2ProviderAppToken(t, db, database.OAuth2ProviderAppToken{
737+
ExpiresAt: now.Add(1 * time.Hour), // Expires in 1 hour
738+
AppSecretID: appSecret.ID,
739+
APIKeyID: validAPIKey.ID,
740+
UserID: user.ID,
741+
HashPrefix: []byte(fmt.Sprintf("valid-%d", time.Now().UnixNano())),
742+
})
743+
744+
// Verify tokens exist initially
745+
_, err := db.GetOAuth2ProviderAppTokenByPrefix(ctx, expiredToken.HashPrefix)
746+
require.NoError(t, err)
747+
_, err = db.GetOAuth2ProviderAppTokenByPrefix(ctx, validToken.HashPrefix)
748+
require.NoError(t, err)
749+
750+
// Run cleanup
751+
done := awaitDoTick(ctx, t, clk)
752+
closer := dbpurge.New(ctx, logger, db, clk)
753+
defer closer.Close()
754+
<-done
755+
756+
// Verify expired token is deleted
757+
_, err = db.GetOAuth2ProviderAppTokenByPrefix(ctx, expiredToken.HashPrefix)
758+
require.Error(t, err)
759+
require.ErrorIs(t, err, sql.ErrNoRows)
760+
761+
// Verify non-expired token is retained
762+
_, err = db.GetOAuth2ProviderAppTokenByPrefix(ctx, validToken.HashPrefix)
763+
require.NoError(t, err)
764+
}
765+
766+
//nolint:paralleltest // It uses LockIDDBPurge.
767+
func TestDeleteExpiredOAuth2ProviderDeviceCodes(t *testing.T) {
768+
ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitShort)
769+
defer cancel()
770+
771+
clk := quartz.NewMock(t)
772+
db, _ := dbtestutil.NewDB(t, dbtestutil.WithDumpOnFailure())
773+
logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true})
774+
775+
now := dbtime.Now()
776+
clk.Set(now).MustWait(ctx)
777+
778+
// Create test data
779+
app := dbgen.OAuth2ProviderApp(t, db, database.OAuth2ProviderApp{
780+
Name: fmt.Sprintf("test-device-%d", time.Now().UnixNano()),
781+
})
782+
783+
nanoTime := time.Now().UnixNano()
784+
785+
// Create expired device code with pending status (should be deleted)
786+
expiredPendingCode := dbgen.OAuth2ProviderDeviceCode(t, db, database.OAuth2ProviderDeviceCode{
787+
ExpiresAt: now.Add(-1 * time.Hour), // Expired 1 hour ago
788+
ClientID: app.ID,
789+
Status: database.OAuth2DeviceStatusPending,
790+
DeviceCodePrefix: fmt.Sprintf("EP%06d", nanoTime%1000000),
791+
UserCode: fmt.Sprintf("EP%06d", nanoTime%1000000),
792+
DeviceCodeHash: fmt.Appendf(nil, "hash-exp-pending-%d", nanoTime),
793+
})
794+
795+
// Create non-expired device code with pending status (should be retained)
796+
validPendingCode := dbgen.OAuth2ProviderDeviceCode(t, db, database.OAuth2ProviderDeviceCode{
797+
ExpiresAt: now.Add(1 * time.Hour), // Expires in 1 hour
798+
ClientID: app.ID,
799+
Status: database.OAuth2DeviceStatusPending,
800+
DeviceCodePrefix: fmt.Sprintf("VP%06d", (nanoTime+1)%1000000),
801+
UserCode: fmt.Sprintf("VP%06d", (nanoTime+1)%1000000),
802+
DeviceCodeHash: fmt.Appendf(nil, "hash-val-pending-%d", nanoTime+1),
803+
})
804+
805+
// Create expired device code with authorized status (should be deleted - all expired codes are deleted)
806+
expiredAuthorizedCode := dbgen.OAuth2ProviderDeviceCode(t, db, database.OAuth2ProviderDeviceCode{
807+
ExpiresAt: now.Add(-1 * time.Hour), // Expired 1 hour ago
808+
ClientID: app.ID,
809+
DeviceCodePrefix: fmt.Sprintf("EA%06d", (nanoTime+2)%1000000),
810+
UserCode: fmt.Sprintf("EA%06d", (nanoTime+2)%1000000),
811+
DeviceCodeHash: fmt.Appendf(nil, "hash-exp-auth-%d", nanoTime+2),
812+
})
813+
814+
// Create a user and authorize the device code
815+
user := dbgen.User(t, db, database.User{})
816+
expiredAuthorizedCode, err := db.UpdateOAuth2ProviderDeviceCodeAuthorization(ctx, database.UpdateOAuth2ProviderDeviceCodeAuthorizationParams{
817+
ID: expiredAuthorizedCode.ID,
818+
UserID: uuid.NullUUID{UUID: user.ID, Valid: true},
819+
Status: database.OAuth2DeviceStatusAuthorized,
820+
})
821+
require.NoError(t, err)
822+
823+
// Verify device codes exist initially
824+
_, err = db.GetOAuth2ProviderDeviceCodeByID(ctx, expiredPendingCode.ID)
825+
require.NoError(t, err)
826+
_, err = db.GetOAuth2ProviderDeviceCodeByID(ctx, validPendingCode.ID)
827+
require.NoError(t, err)
828+
_, err = db.GetOAuth2ProviderDeviceCodeByID(ctx, expiredAuthorizedCode.ID)
829+
require.NoError(t, err)
830+
831+
// Run cleanup
832+
done := awaitDoTick(ctx, t, clk)
833+
closer := dbpurge.New(ctx, logger, db, clk)
834+
defer closer.Close()
835+
<-done
836+
837+
// Verify expired pending device code is deleted
838+
_, err = db.GetOAuth2ProviderDeviceCodeByID(ctx, expiredPendingCode.ID)
839+
require.Error(t, err)
840+
require.ErrorIs(t, err, sql.ErrNoRows)
841+
842+
// Verify non-expired pending device code is retained
843+
_, err = db.GetOAuth2ProviderDeviceCodeByID(ctx, validPendingCode.ID)
844+
require.NoError(t, err)
845+
846+
// Verify expired authorized device code is deleted (all expired codes are deleted)
847+
_, err = db.GetOAuth2ProviderDeviceCodeByID(ctx, expiredAuthorizedCode.ID)
848+
require.Error(t, err)
849+
require.ErrorIs(t, err, sql.ErrNoRows)
850+
}

‎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)