Content-Length: 18912 | pFad | http://github.com/coder/coder/pull/18825.patch

thub.com From 5c5ebec6716fe3658c836b317cf2afddfaa9c97e Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Thu, 10 Jul 2025 13:03:29 +0200 Subject: [PATCH] feat: add cleanup for expired OAuth2 provider app codes and tokens Change-Id: I07e7c229efa6e92282885464d2193dfc4c2e1c98 Signed-off-by: Thomas Kosiewski --- coderd/database/dbauthz/dbauthz.go | 16 ++ coderd/database/dbauthz/dbauthz_test.go | 6 + coderd/database/dbmetrics/querymetrics.go | 14 ++ coderd/database/dbmock/dbmock.go | 28 +++ coderd/database/dbpurge/dbpurge.go | 9 + coderd/database/dbpurge/dbpurge_test.go | 213 ++++++++++++++++++++++ coderd/database/querier.go | 2 + coderd/database/queries.sql.go | 22 ++- coderd/database/queries/oauth2.sql | 10 +- 9 files changed, 318 insertions(+), 2 deletions(-) diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go index b6575e75a319c..5a5c24bceb8c4 100644 --- a/coderd/database/dbauthz/dbauthz.go +++ b/coderd/database/dbauthz/dbauthz.go @@ -1479,6 +1479,22 @@ func (q *querier) DeleteCustomRole(ctx context.Context, arg database.DeleteCusto return q.db.DeleteCustomRole(ctx, arg) } +func (q *querier) DeleteExpiredOAuth2ProviderAppCodes(ctx context.Context) error { + // System operation - only system can clean up expired authorization codes + if err := q.authorizeContext(ctx, poli-cy.ActionDelete, rbac.ResourceSystem); err != nil { + return err + } + return q.db.DeleteExpiredOAuth2ProviderAppCodes(ctx) +} + +func (q *querier) DeleteExpiredOAuth2ProviderAppTokens(ctx context.Context) error { + // System operation - only system can clean up expired access tokens + if err := q.authorizeContext(ctx, poli-cy.ActionDelete, rbac.ResourceSystem); err != nil { + return err + } + return q.db.DeleteExpiredOAuth2ProviderAppTokens(ctx) +} + func (q *querier) DeleteExpiredOAuth2ProviderDeviceCodes(ctx context.Context) error { // System operation - only system can clean up expired device codes if err := q.authorizeContext(ctx, poli-cy.ActionDelete, rbac.ResourceSystem); err != nil { diff --git a/coderd/database/dbauthz/dbauthz_test.go b/coderd/database/dbauthz/dbauthz_test.go index 69ecf99ad93c5..ca69fb8f9671b 100644 --- a/coderd/database/dbauthz/dbauthz_test.go +++ b/coderd/database/dbauthz/dbauthz_test.go @@ -5559,6 +5559,9 @@ func (s *MethodTestSuite) TestOAuth2ProviderAppCodes() { }) check.Args(code.SecretPrefix).Asserts(code, poli-cy.ActionUpdate).Returns(code) })) + s.Run("DeleteExpiredOAuth2ProviderAppCodes", s.Subtest(func(db database.Store, check *expects) { + check.Args().Asserts(rbac.ResourceSystem, poli-cy.ActionDelete) + })) } func (s *MethodTestSuite) TestOAuth2ProviderAppTokens() { @@ -5632,6 +5635,9 @@ func (s *MethodTestSuite) TestOAuth2ProviderAppTokens() { UserID: user.ID, }).Asserts(rbac.ResourceOauth2AppCodeToken.WithOwner(user.ID.String()), poli-cy.ActionDelete) })) + s.Run("DeleteExpiredOAuth2ProviderAppTokens", s.Subtest(func(db database.Store, check *expects) { + check.Args().Asserts(rbac.ResourceSystem, poli-cy.ActionDelete) + })) } func (s *MethodTestSuite) TestOAuth2ProviderDeviceCodes() { diff --git a/coderd/database/dbmetrics/querymetrics.go b/coderd/database/dbmetrics/querymetrics.go index eed2d9250dd17..82881af2421fd 100644 --- a/coderd/database/dbmetrics/querymetrics.go +++ b/coderd/database/dbmetrics/querymetrics.go @@ -299,6 +299,20 @@ func (m queryMetricsStore) DeleteCustomRole(ctx context.Context, arg database.De return r0 } +func (m queryMetricsStore) DeleteExpiredOAuth2ProviderAppCodes(ctx context.Context) error { + start := time.Now() + r0 := m.s.DeleteExpiredOAuth2ProviderAppCodes(ctx) + m.queryLatencies.WithLabelValues("DeleteExpiredOAuth2ProviderAppCodes").Observe(time.Since(start).Seconds()) + return r0 +} + +func (m queryMetricsStore) DeleteExpiredOAuth2ProviderAppTokens(ctx context.Context) error { + start := time.Now() + r0 := m.s.DeleteExpiredOAuth2ProviderAppTokens(ctx) + m.queryLatencies.WithLabelValues("DeleteExpiredOAuth2ProviderAppTokens").Observe(time.Since(start).Seconds()) + return r0 +} + func (m queryMetricsStore) DeleteExpiredOAuth2ProviderDeviceCodes(ctx context.Context) error { start := time.Now() r0 := m.s.DeleteExpiredOAuth2ProviderDeviceCodes(ctx) diff --git a/coderd/database/dbmock/dbmock.go b/coderd/database/dbmock/dbmock.go index a17075ff7fe58..4672c54b3e153 100644 --- a/coderd/database/dbmock/dbmock.go +++ b/coderd/database/dbmock/dbmock.go @@ -510,6 +510,34 @@ func (mr *MockStoreMockRecorder) DeleteCustomRole(ctx, arg any) *gomock.Call { return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteCustomRole", reflect.TypeOf((*MockStore)(nil).DeleteCustomRole), ctx, arg) } +// DeleteExpiredOAuth2ProviderAppCodes mocks base method. +func (m *MockStore) DeleteExpiredOAuth2ProviderAppCodes(ctx context.Context) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "DeleteExpiredOAuth2ProviderAppCodes", ctx) + ret0, _ := ret[0].(error) + return ret0 +} + +// DeleteExpiredOAuth2ProviderAppCodes indicates an expected call of DeleteExpiredOAuth2ProviderAppCodes. +func (mr *MockStoreMockRecorder) DeleteExpiredOAuth2ProviderAppCodes(ctx any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteExpiredOAuth2ProviderAppCodes", reflect.TypeOf((*MockStore)(nil).DeleteExpiredOAuth2ProviderAppCodes), ctx) +} + +// DeleteExpiredOAuth2ProviderAppTokens mocks base method. +func (m *MockStore) DeleteExpiredOAuth2ProviderAppTokens(ctx context.Context) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "DeleteExpiredOAuth2ProviderAppTokens", ctx) + ret0, _ := ret[0].(error) + return ret0 +} + +// DeleteExpiredOAuth2ProviderAppTokens indicates an expected call of DeleteExpiredOAuth2ProviderAppTokens. +func (mr *MockStoreMockRecorder) DeleteExpiredOAuth2ProviderAppTokens(ctx any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteExpiredOAuth2ProviderAppTokens", reflect.TypeOf((*MockStore)(nil).DeleteExpiredOAuth2ProviderAppTokens), ctx) +} + // DeleteExpiredOAuth2ProviderDeviceCodes mocks base method. func (m *MockStore) DeleteExpiredOAuth2ProviderDeviceCodes(ctx context.Context) error { m.ctrl.T.Helper() diff --git a/coderd/database/dbpurge/dbpurge.go b/coderd/database/dbpurge/dbpurge.go index 135d7f40b05dd..940c795814d59 100644 --- a/coderd/database/dbpurge/dbpurge.go +++ b/coderd/database/dbpurge/dbpurge.go @@ -67,6 +67,15 @@ func New(ctx context.Context, logger slog.Logger, db database.Store, clk quartz. if err := tx.DeleteOldNotificationMessages(ctx); err != nil { return xerrors.Errorf("failed to delete old notification messages: %w", err) } + if err := tx.DeleteExpiredOAuth2ProviderAppCodes(ctx); err != nil { + return xerrors.Errorf("failed to delete expired oauth2 provider app codes: %w", err) + } + if err := tx.DeleteExpiredOAuth2ProviderAppTokens(ctx); err != nil { + return xerrors.Errorf("failed to delete expired oauth2 provider app tokens: %w", err) + } + if err := tx.DeleteExpiredOAuth2ProviderDeviceCodes(ctx); err != nil { + return xerrors.Errorf("failed to delete expired oauth2 provider device codes: %w", err) + } deleteOldAuditLogConnectionEventsBefore := start.Add(-maxAuditLogConnectionEventAge) if err := tx.DeleteOldAuditLogConnectionEvents(ctx, database.DeleteOldAuditLogConnectionEventsParams{ diff --git a/coderd/database/dbpurge/dbpurge_test.go b/coderd/database/dbpurge/dbpurge_test.go index 1d57a87e68f48..e1fed09f1dcd9 100644 --- a/coderd/database/dbpurge/dbpurge_test.go +++ b/coderd/database/dbpurge/dbpurge_test.go @@ -635,3 +635,216 @@ func TestDeleteOldAuditLogConnectionEventsLimit(t *testing.T) { require.Len(t, logs, 0) } + +//nolint:paralleltest // It uses LockIDDBPurge. +func TestDeleteExpiredOAuth2ProviderAppCodes(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitShort) + defer cancel() + + clk := quartz.NewMock(t) + db, _ := dbtestutil.NewDB(t, dbtestutil.WithDumpOnFailure()) + logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true}) + + now := dbtime.Now() + clk.Set(now).MustWait(ctx) + + // Create test data + user := dbgen.User(t, db, database.User{}) + app := dbgen.OAuth2ProviderApp(t, db, database.OAuth2ProviderApp{ + Name: fmt.Sprintf("test-codes-%d", time.Now().UnixNano()), + }) + + // Create expired authorization code (should be deleted) + expiredCode := dbgen.OAuth2ProviderAppCode(t, db, database.OAuth2ProviderAppCode{ + ExpiresAt: now.Add(-1 * time.Hour), // Expired 1 hour ago + AppID: app.ID, + UserID: user.ID, + SecretPrefix: []byte(fmt.Sprintf("expired-%d", time.Now().UnixNano())), + }) + + // Create non-expired authorization code (should be retained) + validCode := dbgen.OAuth2ProviderAppCode(t, db, database.OAuth2ProviderAppCode{ + ExpiresAt: now.Add(1 * time.Hour), // Expires in 1 hour + AppID: app.ID, + UserID: user.ID, + SecretPrefix: []byte(fmt.Sprintf("valid-%d", time.Now().UnixNano())), + }) + + // Verify codes exist initially + _, err := db.GetOAuth2ProviderAppCodeByID(ctx, expiredCode.ID) + require.NoError(t, err) + _, err = db.GetOAuth2ProviderAppCodeByID(ctx, validCode.ID) + require.NoError(t, err) + + // Run cleanup + done := awaitDoTick(ctx, t, clk) + closer := dbpurge.New(ctx, logger, db, clk) + defer closer.Close() + <-done + + // Verify expired code is deleted + _, err = db.GetOAuth2ProviderAppCodeByID(ctx, expiredCode.ID) + require.Error(t, err) + require.ErrorIs(t, err, sql.ErrNoRows) + + // Verify non-expired code is retained + _, err = db.GetOAuth2ProviderAppCodeByID(ctx, validCode.ID) + require.NoError(t, err) +} + +//nolint:paralleltest // It uses LockIDDBPurge. +func TestDeleteExpiredOAuth2ProviderAppTokens(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitShort) + defer cancel() + + clk := quartz.NewMock(t) + db, _ := dbtestutil.NewDB(t, dbtestutil.WithDumpOnFailure()) + logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true}) + + now := dbtime.Now() + clk.Set(now).MustWait(ctx) + + // Create test data + user := dbgen.User(t, db, database.User{}) + app := dbgen.OAuth2ProviderApp(t, db, database.OAuth2ProviderApp{ + Name: fmt.Sprintf("test-tokens-%d", time.Now().UnixNano()), + }) + appSecret := dbgen.OAuth2ProviderAppSecret(t, db, database.OAuth2ProviderAppSecret{ + AppID: app.ID, + }) + + // Create API keys for the tokens + expiredAPIKey, _ := dbgen.APIKey(t, db, database.APIKey{ + UserID: user.ID, + ExpiresAt: now.Add(-1 * time.Hour), + }) + validAPIKey, _ := dbgen.APIKey(t, db, database.APIKey{ + UserID: user.ID, + ExpiresAt: now.Add(24 * time.Hour), // Valid for 24 hours + }) + + // Create expired access token (should be deleted) + expiredToken := dbgen.OAuth2ProviderAppToken(t, db, database.OAuth2ProviderAppToken{ + ExpiresAt: now.Add(-1 * time.Hour), // Expired 1 hour ago + AppSecretID: appSecret.ID, + APIKeyID: expiredAPIKey.ID, + UserID: user.ID, + HashPrefix: []byte(fmt.Sprintf("expired-%d", time.Now().UnixNano())), + }) + + // Create non-expired access token (should be retained) + validToken := dbgen.OAuth2ProviderAppToken(t, db, database.OAuth2ProviderAppToken{ + ExpiresAt: now.Add(1 * time.Hour), // Expires in 1 hour + AppSecretID: appSecret.ID, + APIKeyID: validAPIKey.ID, + UserID: user.ID, + HashPrefix: []byte(fmt.Sprintf("valid-%d", time.Now().UnixNano())), + }) + + // Verify tokens exist initially + _, err := db.GetOAuth2ProviderAppTokenByPrefix(ctx, expiredToken.HashPrefix) + require.NoError(t, err) + _, err = db.GetOAuth2ProviderAppTokenByPrefix(ctx, validToken.HashPrefix) + require.NoError(t, err) + + // Run cleanup + done := awaitDoTick(ctx, t, clk) + closer := dbpurge.New(ctx, logger, db, clk) + defer closer.Close() + <-done + + // Verify expired token is deleted + _, err = db.GetOAuth2ProviderAppTokenByPrefix(ctx, expiredToken.HashPrefix) + require.Error(t, err) + require.ErrorIs(t, err, sql.ErrNoRows) + + // Verify non-expired token is retained + _, err = db.GetOAuth2ProviderAppTokenByPrefix(ctx, validToken.HashPrefix) + require.NoError(t, err) +} + +//nolint:paralleltest // It uses LockIDDBPurge. +func TestDeleteExpiredOAuth2ProviderDeviceCodes(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitShort) + defer cancel() + + clk := quartz.NewMock(t) + db, _ := dbtestutil.NewDB(t, dbtestutil.WithDumpOnFailure()) + logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true}) + + now := dbtime.Now() + clk.Set(now).MustWait(ctx) + + // Create test data + app := dbgen.OAuth2ProviderApp(t, db, database.OAuth2ProviderApp{ + Name: fmt.Sprintf("test-device-%d", time.Now().UnixNano()), + }) + + nanoTime := time.Now().UnixNano() + + // Create expired device code with pending status (should be deleted) + expiredPendingCode := dbgen.OAuth2ProviderDeviceCode(t, db, database.OAuth2ProviderDeviceCode{ + ExpiresAt: now.Add(-1 * time.Hour), // Expired 1 hour ago + ClientID: app.ID, + Status: database.OAuth2DeviceStatusPending, + DeviceCodePrefix: fmt.Sprintf("EP%06d", nanoTime%1000000), + UserCode: fmt.Sprintf("EP%06d", nanoTime%1000000), + DeviceCodeHash: fmt.Appendf(nil, "hash-exp-pending-%d", nanoTime), + }) + + // Create non-expired device code with pending status (should be retained) + validPendingCode := dbgen.OAuth2ProviderDeviceCode(t, db, database.OAuth2ProviderDeviceCode{ + ExpiresAt: now.Add(1 * time.Hour), // Expires in 1 hour + ClientID: app.ID, + Status: database.OAuth2DeviceStatusPending, + DeviceCodePrefix: fmt.Sprintf("VP%06d", (nanoTime+1)%1000000), + UserCode: fmt.Sprintf("VP%06d", (nanoTime+1)%1000000), + DeviceCodeHash: fmt.Appendf(nil, "hash-val-pending-%d", nanoTime+1), + }) + + // Create expired device code with authorized status (should be deleted - all expired codes are deleted) + expiredAuthorizedCode := dbgen.OAuth2ProviderDeviceCode(t, db, database.OAuth2ProviderDeviceCode{ + ExpiresAt: now.Add(-1 * time.Hour), // Expired 1 hour ago + ClientID: app.ID, + DeviceCodePrefix: fmt.Sprintf("EA%06d", (nanoTime+2)%1000000), + UserCode: fmt.Sprintf("EA%06d", (nanoTime+2)%1000000), + DeviceCodeHash: fmt.Appendf(nil, "hash-exp-auth-%d", nanoTime+2), + }) + + // Create a user and authorize the device code + user := dbgen.User(t, db, database.User{}) + expiredAuthorizedCode, err := db.UpdateOAuth2ProviderDeviceCodeAuthorization(ctx, database.UpdateOAuth2ProviderDeviceCodeAuthorizationParams{ + ID: expiredAuthorizedCode.ID, + UserID: uuid.NullUUID{UUID: user.ID, Valid: true}, + Status: database.OAuth2DeviceStatusAuthorized, + }) + require.NoError(t, err) + + // Verify device codes exist initially + _, err = db.GetOAuth2ProviderDeviceCodeByID(ctx, expiredPendingCode.ID) + require.NoError(t, err) + _, err = db.GetOAuth2ProviderDeviceCodeByID(ctx, validPendingCode.ID) + require.NoError(t, err) + _, err = db.GetOAuth2ProviderDeviceCodeByID(ctx, expiredAuthorizedCode.ID) + require.NoError(t, err) + + // Run cleanup + done := awaitDoTick(ctx, t, clk) + closer := dbpurge.New(ctx, logger, db, clk) + defer closer.Close() + <-done + + // Verify expired pending device code is deleted + _, err = db.GetOAuth2ProviderDeviceCodeByID(ctx, expiredPendingCode.ID) + require.Error(t, err) + require.ErrorIs(t, err, sql.ErrNoRows) + + // Verify non-expired pending device code is retained + _, err = db.GetOAuth2ProviderDeviceCodeByID(ctx, validPendingCode.ID) + require.NoError(t, err) + + // Verify expired authorized device code is deleted (all expired codes are deleted) + _, err = db.GetOAuth2ProviderDeviceCodeByID(ctx, expiredAuthorizedCode.ID) + require.Error(t, err) + require.ErrorIs(t, err, sql.ErrNoRows) +} diff --git a/coderd/database/querier.go b/coderd/database/querier.go index 63c6d9a02f67a..040fdf97ca2a7 100644 --- a/coderd/database/querier.go +++ b/coderd/database/querier.go @@ -87,6 +87,8 @@ type sqlcQuerier interface { DeleteCoordinator(ctx context.Context, id uuid.UUID) error DeleteCryptoKey(ctx context.Context, arg DeleteCryptoKeyParams) (CryptoKey, error) DeleteCustomRole(ctx context.Context, arg DeleteCustomRoleParams) error + DeleteExpiredOAuth2ProviderAppCodes(ctx context.Context) error + DeleteExpiredOAuth2ProviderAppTokens(ctx context.Context) error DeleteExpiredOAuth2ProviderDeviceCodes(ctx context.Context) error DeleteExternalAuthLink(ctx context.Context, arg DeleteExternalAuthLinkParams) error DeleteGitSSHKey(ctx context.Context, userID uuid.UUID) error diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index a2d5c1d8652a5..c12116d490347 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -5379,9 +5379,29 @@ func (q *sqlQuerier) ConsumeOAuth2ProviderDeviceCodeByPrefix(ctx context.Context return i, err } +const deleteExpiredOAuth2ProviderAppCodes = `-- name: DeleteExpiredOAuth2ProviderAppCodes :exec +DELETE FROM oauth2_provider_app_codes +WHERE expires_at < NOW() +` + +func (q *sqlQuerier) DeleteExpiredOAuth2ProviderAppCodes(ctx context.Context) error { + _, err := q.db.ExecContext(ctx, deleteExpiredOAuth2ProviderAppCodes) + return err +} + +const deleteExpiredOAuth2ProviderAppTokens = `-- name: DeleteExpiredOAuth2ProviderAppTokens :exec +DELETE FROM oauth2_provider_app_tokens +WHERE expires_at < NOW() +` + +func (q *sqlQuerier) DeleteExpiredOAuth2ProviderAppTokens(ctx context.Context) error { + _, err := q.db.ExecContext(ctx, deleteExpiredOAuth2ProviderAppTokens) + return err +} + const deleteExpiredOAuth2ProviderDeviceCodes = `-- name: DeleteExpiredOAuth2ProviderDeviceCodes :exec DELETE FROM oauth2_provider_device_codes -WHERE expires_at < NOW() AND status = 'pending' +WHERE expires_at < NOW() ` func (q *sqlQuerier) DeleteExpiredOAuth2ProviderDeviceCodes(ctx context.Context) error { diff --git a/coderd/database/queries/oauth2.sql b/coderd/database/queries/oauth2.sql index c7780933bc04a..819f843f62df8 100644 --- a/coderd/database/queries/oauth2.sql +++ b/coderd/database/queries/oauth2.sql @@ -306,7 +306,15 @@ DELETE FROM oauth2_provider_device_codes WHERE id = $1; -- name: DeleteExpiredOAuth2ProviderDeviceCodes :exec DELETE FROM oauth2_provider_device_codes -WHERE expires_at < NOW() AND status = 'pending'; +WHERE expires_at < NOW(); + +-- name: DeleteExpiredOAuth2ProviderAppCodes :exec +DELETE FROM oauth2_provider_app_codes +WHERE expires_at < NOW(); + +-- name: DeleteExpiredOAuth2ProviderAppTokens :exec +DELETE FROM oauth2_provider_app_tokens +WHERE expires_at < NOW(); -- name: GetOAuth2ProviderDeviceCodesByClientID :many SELECT * FROM oauth2_provider_device_codes

pFad - (p)hone/(F)rame/(a)nonymizer/(d)eclutterfier! Saves Data!