diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go index 076cd9d9de5e0..9ae6a2b12d162 100644 --- a/coderd/database/dbauthz/dbauthz.go +++ b/coderd/database/dbauthz/dbauthz.go @@ -1509,6 +1509,15 @@ func (q *querier) DeleteCustomRole(ctx context.Context, arg database.DeleteCusto return q.db.DeleteCustomRole(ctx, arg) } +func (q *querier) DeleteExpiredAPIKeys(ctx context.Context, arg database.DeleteExpiredAPIKeysParams) (int64, error) { + // Requires DELETE across all API keys. + if err := q.authorizeContext(ctx, policy.ActionDelete, rbac.ResourceApiKey); err != nil { + return 0, err + } + + return q.db.DeleteExpiredAPIKeys(ctx, arg) +} + func (q *querier) DeleteExternalAuthLink(ctx context.Context, arg database.DeleteExternalAuthLinkParams) error { return fetchAndExec(q.log, q.auth, policy.ActionUpdatePersonal, func(ctx context.Context, arg database.DeleteExternalAuthLinkParams) (database.ExternalAuthLink, error) { //nolint:gosimple diff --git a/coderd/database/dbauthz/dbauthz_test.go b/coderd/database/dbauthz/dbauthz_test.go index b53f44060c848..f3208037032f3 100644 --- a/coderd/database/dbauthz/dbauthz_test.go +++ b/coderd/database/dbauthz/dbauthz_test.go @@ -213,6 +213,14 @@ func (s *MethodTestSuite) TestAPIKey() { dbm.EXPECT().DeleteAPIKeyByID(gomock.Any(), key.ID).Return(nil).AnyTimes() check.Args(key.ID).Asserts(key, policy.ActionDelete).Returns() })) + s.Run("DeleteExpiredAPIKeys", s.Mocked(func(dbm *dbmock.MockStore, faker *gofakeit.Faker, check *expects) { + args := database.DeleteExpiredAPIKeysParams{ + Before: time.Date(2025, 11, 21, 0, 0, 0, 0, time.UTC), + LimitCount: 1000, + } + dbm.EXPECT().DeleteExpiredAPIKeys(gomock.Any(), args).Return(int64(0), nil).AnyTimes() + check.Args(args).Asserts(rbac.ResourceApiKey, policy.ActionDelete).Returns(int64(0)) + })) s.Run("GetAPIKeyByID", s.Mocked(func(dbm *dbmock.MockStore, faker *gofakeit.Faker, check *expects) { key := testutil.Fake(s.T(), faker, database.APIKey{}) dbm.EXPECT().GetAPIKeyByID(gomock.Any(), key.ID).Return(key, nil).AnyTimes() diff --git a/coderd/database/dbgen/dbgen.go b/coderd/database/dbgen/dbgen.go index 7ea1f168f2017..99e1ad8975203 100644 --- a/coderd/database/dbgen/dbgen.go +++ b/coderd/database/dbgen/dbgen.go @@ -173,6 +173,13 @@ func APIKey(t testing.TB, db database.Store, seed database.APIKey, munge ...func } } + // It does not make sense for the created_at to be after the expires_at. + // So if expires is set, change the default created_at to be 24 hours before. + var createdAt time.Time + if !seed.ExpiresAt.IsZero() && seed.CreatedAt.IsZero() { + createdAt = seed.ExpiresAt.Add(-24 * time.Hour) + } + params := database.InsertAPIKeyParams{ ID: takeFirst(seed.ID, id), // 0 defaults to 86400 at the db layer @@ -182,7 +189,7 @@ func APIKey(t testing.TB, db database.Store, seed database.APIKey, munge ...func UserID: takeFirst(seed.UserID, uuid.New()), LastUsed: takeFirst(seed.LastUsed, dbtime.Now()), ExpiresAt: takeFirst(seed.ExpiresAt, dbtime.Now().Add(time.Hour)), - CreatedAt: takeFirst(seed.CreatedAt, dbtime.Now()), + CreatedAt: takeFirst(seed.CreatedAt, createdAt, dbtime.Now()), UpdatedAt: takeFirst(seed.UpdatedAt, dbtime.Now()), LoginType: takeFirst(seed.LoginType, database.LoginTypePassword), Scope: takeFirst(seed.Scope, database.APIKeyScopeAll), diff --git a/coderd/database/dbmetrics/querymetrics.go b/coderd/database/dbmetrics/querymetrics.go index 59f2773de7d98..2b703953120f1 100644 --- a/coderd/database/dbmetrics/querymetrics.go +++ b/coderd/database/dbmetrics/querymetrics.go @@ -292,6 +292,13 @@ func (m queryMetricsStore) DeleteCustomRole(ctx context.Context, arg database.De return r0 } +func (m queryMetricsStore) DeleteExpiredAPIKeys(ctx context.Context, arg database.DeleteExpiredAPIKeysParams) (int64, error) { + start := time.Now() + r0, r1 := m.s.DeleteExpiredAPIKeys(ctx, arg) + m.queryLatencies.WithLabelValues("DeleteExpiredAPIKeys").Observe(time.Since(start).Seconds()) + return r0, r1 +} + func (m queryMetricsStore) DeleteExternalAuthLink(ctx context.Context, arg database.DeleteExternalAuthLinkParams) error { start := time.Now() r0 := m.s.DeleteExternalAuthLink(ctx, arg) diff --git a/coderd/database/dbmock/dbmock.go b/coderd/database/dbmock/dbmock.go index f899985cb5180..dfa169434662a 100644 --- a/coderd/database/dbmock/dbmock.go +++ b/coderd/database/dbmock/dbmock.go @@ -495,6 +495,21 @@ func (mr *MockStoreMockRecorder) DeleteCustomRole(ctx, arg any) *gomock.Call { return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteCustomRole", reflect.TypeOf((*MockStore)(nil).DeleteCustomRole), ctx, arg) } +// DeleteExpiredAPIKeys mocks base method. +func (m *MockStore) DeleteExpiredAPIKeys(ctx context.Context, arg database.DeleteExpiredAPIKeysParams) (int64, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "DeleteExpiredAPIKeys", ctx, arg) + ret0, _ := ret[0].(int64) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// DeleteExpiredAPIKeys indicates an expected call of DeleteExpiredAPIKeys. +func (mr *MockStoreMockRecorder) DeleteExpiredAPIKeys(ctx, arg any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteExpiredAPIKeys", reflect.TypeOf((*MockStore)(nil).DeleteExpiredAPIKeys), ctx, arg) +} + // DeleteExternalAuthLink mocks base method. func (m *MockStore) DeleteExternalAuthLink(ctx context.Context, arg database.DeleteExternalAuthLinkParams) error { m.ctrl.T.Helper() diff --git a/coderd/database/dbpurge/dbpurge.go b/coderd/database/dbpurge/dbpurge.go index b9e0023f5a6f8..60b96b056df3c 100644 --- a/coderd/database/dbpurge/dbpurge.go +++ b/coderd/database/dbpurge/dbpurge.go @@ -71,6 +71,19 @@ func New(ctx context.Context, logger slog.Logger, db database.Store, clk quartz. if err := tx.ExpirePrebuildsAPIKeys(ctx, dbtime.Time(start)); err != nil { return xerrors.Errorf("failed to expire prebuilds user api keys: %w", err) } + expiredAPIKeys, err := tx.DeleteExpiredAPIKeys(ctx, database.DeleteExpiredAPIKeysParams{ + // Leave expired keys for a week to allow the backend to know the difference + // between a 404 and an expired key. This purge code is just to bound the size of + // the table to something more reasonable. + Before: dbtime.Time(start.Add(time.Hour * 24 * 7 * -1)), + // There could be a lot of expired keys here, so set a limit to prevent this + // taking too long. + // This runs every 10 minutes, so it deletes ~1.5m keys per day at most. + LimitCount: 10000, + }) + if err != nil { + return xerrors.Errorf("failed to delete expired api keys: %w", err) + } deleteOldAuditLogConnectionEventsBefore := start.Add(-maxAuditLogConnectionEventAge) if err := tx.DeleteOldAuditLogConnectionEvents(ctx, database.DeleteOldAuditLogConnectionEventsParams{ @@ -80,7 +93,7 @@ func New(ctx context.Context, logger slog.Logger, db database.Store, clk quartz. return xerrors.Errorf("failed to delete old audit log connection events: %w", err) } - logger.Debug(ctx, "purged old database entries", slog.F("duration", clk.Since(start))) + logger.Debug(ctx, "purged old database entries", slog.F("expired_api_keys", expiredAPIKeys), slog.F("duration", clk.Since(start))) return nil }, database.DefaultTXOptions().WithID("db_purge")); err != nil { diff --git a/coderd/database/querier.go b/coderd/database/querier.go index b645ef73ae746..e7a8f8bfb9b2e 100644 --- a/coderd/database/querier.go +++ b/coderd/database/querier.go @@ -86,6 +86,7 @@ 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 + DeleteExpiredAPIKeys(ctx context.Context, arg DeleteExpiredAPIKeysParams) (int64, error) DeleteExternalAuthLink(ctx context.Context, arg DeleteExternalAuthLinkParams) error DeleteGitSSHKey(ctx context.Context, userID uuid.UUID) error DeleteGroupByID(ctx context.Context, id uuid.UUID) error diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index 7d3b446870f91..fee742122dea3 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -148,6 +148,38 @@ func (q *sqlQuerier) DeleteApplicationConnectAPIKeysByUserID(ctx context.Context return err } +const deleteExpiredAPIKeys = `-- name: DeleteExpiredAPIKeys :one +WITH expired_keys AS ( + SELECT id + FROM api_keys + -- expired keys only + WHERE expires_at < $1::timestamptz + LIMIT $2 +), +deleted_rows AS ( + DELETE FROM + api_keys + USING + expired_keys + WHERE + api_keys.id = expired_keys.id + RETURNING api_keys.id + ) +SELECT COUNT(deleted_rows.id) AS deleted_count FROM deleted_rows +` + +type DeleteExpiredAPIKeysParams struct { + Before time.Time `db:"before" json:"before"` + LimitCount int32 `db:"limit_count" json:"limit_count"` +} + +func (q *sqlQuerier) DeleteExpiredAPIKeys(ctx context.Context, arg DeleteExpiredAPIKeysParams) (int64, error) { + row := q.db.QueryRowContext(ctx, deleteExpiredAPIKeys, arg.Before, arg.LimitCount) + var deleted_count int64 + err := row.Scan(&deleted_count) + return deleted_count, err +} + const expirePrebuildsAPIKeys = `-- name: ExpirePrebuildsAPIKeys :exec WITH unexpired_prebuilds_workspace_session_tokens AS ( SELECT id, SUBSTRING(token_name FROM 38 FOR 36)::uuid AS workspace_id diff --git a/coderd/database/queries/apikeys.sql b/coderd/database/queries/apikeys.sql index 98be411ca65ea..ceac2a750b81c 100644 --- a/coderd/database/queries/apikeys.sql +++ b/coderd/database/queries/apikeys.sql @@ -84,6 +84,26 @@ DELETE FROM WHERE user_id = $1; +-- name: DeleteExpiredAPIKeys :one +WITH expired_keys AS ( + SELECT id + FROM api_keys + -- expired keys only + WHERE expires_at < @before::timestamptz + LIMIT @limit_count +), +deleted_rows AS ( + DELETE FROM + api_keys + USING + expired_keys + WHERE + api_keys.id = expired_keys.id + RETURNING api_keys.id + ) +SELECT COUNT(deleted_rows.id) AS deleted_count FROM deleted_rows; +; + -- name: ExpirePrebuildsAPIKeys :exec -- Firstly, collect api_keys owned by the prebuilds user that correlate -- to workspaces no longer owned by the prebuilds user.