From 7efa327698ab828dd86ae88dd5ba4b0c061ca880 Mon Sep 17 00:00:00 2001 From: Bobby Ho Date: Mon, 10 Aug 2026 15:31:56 -0700 Subject: [PATCH 1/7] feat(coderd): add oauth2 scope columns and single-use delete queries Migration 000567 adds a nullable `scope text` to oauth2_provider_app_codes and oauth2_provider_app_tokens so the scope negotiated at /oauth2/authorize can travel from a code to the token it is exchanged for. No backfill, and every insert writes NULL for now, which reads as unrestricted access, so behavior is unchanged. DeleteOAuth2ProviderAppCodeByIDReturningID and DeleteAPIKeyByIDReturningID return sql.ErrNoRows when the row is already gone, letting the grant paths enforce single use without a read-then-write race. The existing blind deletes and their call sites are unchanged. Refs PLAT-478 --- coderd/database/dbauthz/dbauthz.go | 22 +++++++ coderd/database/dbauthz/dbauthz_test.go | 15 +++++ coderd/database/dbgen/dbgen.go | 2 + coderd/database/dbmetrics/querymetrics.go | 16 +++++ coderd/database/dbmock/dbmock.go | 30 +++++++++ coderd/database/dump.sql | 10 ++- .../000567_oauth2_scope_enforcement.down.sql | 3 + .../000567_oauth2_scope_enforcement.up.sql | 16 +++++ coderd/database/models.go | 4 ++ coderd/database/querier.go | 6 ++ coderd/database/queries.sql.go | 64 ++++++++++++++++--- coderd/database/queries/apikeys.sql | 9 +++ coderd/database/queries/oauth2.sql | 17 +++-- coderd/oauth2provider/authorize.go | 3 + coderd/oauth2provider/tokens.go | 6 ++ 15 files changed, 207 insertions(+), 16 deletions(-) create mode 100644 coderd/database/migrations/000567_oauth2_scope_enforcement.down.sql create mode 100644 coderd/database/migrations/000567_oauth2_scope_enforcement.up.sql diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go index 3f68893f59b8b..d30073105adbb 100644 --- a/coderd/database/dbauthz/dbauthz.go +++ b/coderd/database/dbauthz/dbauthz.go @@ -2067,6 +2067,17 @@ func (q *querier) DeleteAPIKeyByID(ctx context.Context, id string) error { return deleteQ(q.log, q.auth, q.db.GetAPIKeyByID, q.db.DeleteAPIKeyByID)(ctx, id) } +func (q *querier) DeleteAPIKeyByIDReturningID(ctx context.Context, id string) (string, error) { + key, err := q.db.GetAPIKeyByID(ctx, id) + if err != nil { + return "", err + } + if err := q.authorizeContext(ctx, policy.ActionDelete, key); err != nil { + return "", err + } + return q.db.DeleteAPIKeyByIDReturningID(ctx, id) +} + func (q *querier) DeleteAPIKeysByUserID(ctx context.Context, userID uuid.UUID) error { // TODO: This is not 100% correct because it omits apikey IDs. err := q.authorizeContext(ctx, policy.ActionDelete, @@ -2314,6 +2325,17 @@ func (q *querier) DeleteOAuth2ProviderAppCodeByID(ctx context.Context, id uuid.U return q.db.DeleteOAuth2ProviderAppCodeByID(ctx, id) } +func (q *querier) DeleteOAuth2ProviderAppCodeByIDReturningID(ctx context.Context, id uuid.UUID) (uuid.UUID, error) { + code, err := q.db.GetOAuth2ProviderAppCodeByID(ctx, id) + if err != nil { + return uuid.Nil, err + } + if err := q.authorizeContext(ctx, policy.ActionDelete, code); err != nil { + return uuid.Nil, err + } + return q.db.DeleteOAuth2ProviderAppCodeByIDReturningID(ctx, id) +} + func (q *querier) DeleteOAuth2ProviderAppCodesByAppAndUserID(ctx context.Context, arg database.DeleteOAuth2ProviderAppCodesByAppAndUserIDParams) error { if err := q.authorizeContext(ctx, policy.ActionDelete, rbac.ResourceOauth2AppCodeToken.WithOwner(arg.UserID.String())); err != nil { diff --git a/coderd/database/dbauthz/dbauthz_test.go b/coderd/database/dbauthz/dbauthz_test.go index a1dd4f79731f0..7d83e57f11eac 100644 --- a/coderd/database/dbauthz/dbauthz_test.go +++ b/coderd/database/dbauthz/dbauthz_test.go @@ -360,6 +360,12 @@ 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("DeleteAPIKeyByIDReturningID", 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() + dbm.EXPECT().DeleteAPIKeyByIDReturningID(gomock.Any(), key.ID).Return(key.ID, nil).AnyTimes() + check.Args(key.ID).Asserts(key, policy.ActionDelete).Returns(key.ID) + })) 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), @@ -6001,6 +6007,15 @@ func (s *MethodTestSuite) TestOAuth2ProviderAppCodes() { }) check.Args(code.ID).Asserts(code, policy.ActionDelete) })) + s.Run("DeleteOAuth2ProviderAppCodeByIDReturningID", s.Subtest(func(db database.Store, check *expects) { + user := dbgen.User(s.T(), db, database.User{}) + app := dbgen.OAuth2ProviderApp(s.T(), db, database.OAuth2ProviderApp{}) + code := dbgen.OAuth2ProviderAppCode(s.T(), db, database.OAuth2ProviderAppCode{ + AppID: app.ID, + UserID: user.ID, + }) + check.Args(code.ID).Asserts(code, policy.ActionDelete).Returns(code.ID) + })) s.Run("DeleteOAuth2ProviderAppCodesByAppAndUserID", s.Subtest(func(db database.Store, check *expects) { dbtestutil.DisableForeignKeysAndTriggers(s.T(), db) user := dbgen.User(s.T(), db, database.User{}) diff --git a/coderd/database/dbgen/dbgen.go b/coderd/database/dbgen/dbgen.go index 10cfe4dddff08..0e3b3ba952a81 100644 --- a/coderd/database/dbgen/dbgen.go +++ b/coderd/database/dbgen/dbgen.go @@ -1784,6 +1784,7 @@ func OAuth2ProviderAppCode(t testing.TB, db database.Store, seed database.OAuth2 CodeChallengeMethod: seed.CodeChallengeMethod, StateHash: seed.StateHash, RedirectUri: seed.RedirectUri, + Scope: seed.Scope, }) require.NoError(t, err, "insert oauth2 app code") return code @@ -1805,6 +1806,7 @@ func OAuth2ProviderAppToken(t testing.TB, db database.Store, seed database.OAuth APIKeyID: takeFirst(seed.APIKeyID, uuid.New().String()), UserID: takeFirst(seed.UserID, uuid.New()), Audience: seed.Audience, + Scope: seed.Scope, }) require.NoError(t, err, "insert oauth2 app token") return token diff --git a/coderd/database/dbmetrics/querymetrics.go b/coderd/database/dbmetrics/querymetrics.go index f27c4271dbed0..4d28437766916 100644 --- a/coderd/database/dbmetrics/querymetrics.go +++ b/coderd/database/dbmetrics/querymetrics.go @@ -425,6 +425,14 @@ func (m queryMetricsStore) DeleteAPIKeyByID(ctx context.Context, id string) erro return r0 } +func (m queryMetricsStore) DeleteAPIKeyByIDReturningID(ctx context.Context, id string) (string, error) { + start := time.Now() + r0, r1 := m.s.DeleteAPIKeyByIDReturningID(ctx, id) + m.queryLatencies.WithLabelValues("DeleteAPIKeyByIDReturningID").Observe(time.Since(start).Seconds()) + m.queryCounts.WithLabelValues(httpmw.ExtractHTTPRoute(ctx), httpmw.ExtractHTTPMethod(ctx), "DeleteAPIKeyByIDReturningID").Inc() + return r0, r1 +} + func (m queryMetricsStore) DeleteAPIKeysByUserID(ctx context.Context, userID uuid.UUID) error { start := time.Now() r0 := m.s.DeleteAPIKeysByUserID(ctx, userID) @@ -641,6 +649,14 @@ func (m queryMetricsStore) DeleteOAuth2ProviderAppCodeByID(ctx context.Context, return r0 } +func (m queryMetricsStore) DeleteOAuth2ProviderAppCodeByIDReturningID(ctx context.Context, id uuid.UUID) (uuid.UUID, error) { + start := time.Now() + r0, r1 := m.s.DeleteOAuth2ProviderAppCodeByIDReturningID(ctx, id) + m.queryLatencies.WithLabelValues("DeleteOAuth2ProviderAppCodeByIDReturningID").Observe(time.Since(start).Seconds()) + m.queryCounts.WithLabelValues(httpmw.ExtractHTTPRoute(ctx), httpmw.ExtractHTTPMethod(ctx), "DeleteOAuth2ProviderAppCodeByIDReturningID").Inc() + return r0, r1 +} + func (m queryMetricsStore) DeleteOAuth2ProviderAppCodesByAppAndUserID(ctx context.Context, arg database.DeleteOAuth2ProviderAppCodesByAppAndUserIDParams) error { start := time.Now() r0 := m.s.DeleteOAuth2ProviderAppCodesByAppAndUserID(ctx, arg) diff --git a/coderd/database/dbmock/dbmock.go b/coderd/database/dbmock/dbmock.go index f172027dad580..a02c59ccc8162 100644 --- a/coderd/database/dbmock/dbmock.go +++ b/coderd/database/dbmock/dbmock.go @@ -675,6 +675,21 @@ func (mr *MockStoreMockRecorder) DeleteAPIKeyByID(ctx, id any) *gomock.Call { return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteAPIKeyByID", reflect.TypeOf((*MockStore)(nil).DeleteAPIKeyByID), ctx, id) } +// DeleteAPIKeyByIDReturningID mocks base method. +func (m *MockStore) DeleteAPIKeyByIDReturningID(ctx context.Context, id string) (string, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "DeleteAPIKeyByIDReturningID", ctx, id) + ret0, _ := ret[0].(string) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// DeleteAPIKeyByIDReturningID indicates an expected call of DeleteAPIKeyByIDReturningID. +func (mr *MockStoreMockRecorder) DeleteAPIKeyByIDReturningID(ctx, id any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteAPIKeyByIDReturningID", reflect.TypeOf((*MockStore)(nil).DeleteAPIKeyByIDReturningID), ctx, id) +} + // DeleteAPIKeysByUserID mocks base method. func (m *MockStore) DeleteAPIKeysByUserID(ctx context.Context, userID uuid.UUID) error { m.ctrl.T.Helper() @@ -1062,6 +1077,21 @@ func (mr *MockStoreMockRecorder) DeleteOAuth2ProviderAppCodeByID(ctx, id any) *g return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteOAuth2ProviderAppCodeByID", reflect.TypeOf((*MockStore)(nil).DeleteOAuth2ProviderAppCodeByID), ctx, id) } +// DeleteOAuth2ProviderAppCodeByIDReturningID mocks base method. +func (m *MockStore) DeleteOAuth2ProviderAppCodeByIDReturningID(ctx context.Context, id uuid.UUID) (uuid.UUID, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "DeleteOAuth2ProviderAppCodeByIDReturningID", ctx, id) + ret0, _ := ret[0].(uuid.UUID) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// DeleteOAuth2ProviderAppCodeByIDReturningID indicates an expected call of DeleteOAuth2ProviderAppCodeByIDReturningID. +func (mr *MockStoreMockRecorder) DeleteOAuth2ProviderAppCodeByIDReturningID(ctx, id any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteOAuth2ProviderAppCodeByIDReturningID", reflect.TypeOf((*MockStore)(nil).DeleteOAuth2ProviderAppCodeByIDReturningID), ctx, id) +} + // DeleteOAuth2ProviderAppCodesByAppAndUserID mocks base method. func (m *MockStore) DeleteOAuth2ProviderAppCodesByAppAndUserID(ctx context.Context, arg database.DeleteOAuth2ProviderAppCodesByAppAndUserIDParams) error { m.ctrl.T.Helper() diff --git a/coderd/database/dump.sql b/coderd/database/dump.sql index 775a2b27b430c..eddf1e0346530 100644 --- a/coderd/database/dump.sql +++ b/coderd/database/dump.sql @@ -2596,7 +2596,8 @@ CREATE TABLE oauth2_provider_app_codes ( code_challenge text, code_challenge_method text, state_hash text, - redirect_uri text + redirect_uri text, + scope text ); COMMENT ON TABLE oauth2_provider_app_codes IS 'Codes are meant to be exchanged for access tokens.'; @@ -2611,6 +2612,8 @@ COMMENT ON COLUMN oauth2_provider_app_codes.state_hash IS 'SHA-256 hash of the O COMMENT ON COLUMN oauth2_provider_app_codes.redirect_uri IS 'The redirect_uri provided during authorization, to be verified during token exchange (RFC 6749 §4.1.3).'; +COMMENT ON COLUMN oauth2_provider_app_codes.scope IS 'Space-separated scope negotiated at authorization time. NULL means no scope was recorded and the exchanged token is unrestricted.'; + CREATE TABLE oauth2_provider_app_secrets ( id uuid NOT NULL, created_at timestamp with time zone NOT NULL, @@ -2633,7 +2636,8 @@ CREATE TABLE oauth2_provider_app_tokens ( api_key_id text NOT NULL, audience text, user_id uuid NOT NULL, - app_id uuid NOT NULL + app_id uuid NOT NULL, + scope text ); COMMENT ON COLUMN oauth2_provider_app_tokens.refresh_hash IS 'Refresh tokens provide a way to refresh an access token (API key). An expired API key can be refreshed if this token is not yet expired, meaning this expiry can outlive an API key.'; @@ -2644,6 +2648,8 @@ COMMENT ON COLUMN oauth2_provider_app_tokens.user_id IS 'Denormalized user ID fo COMMENT ON COLUMN oauth2_provider_app_tokens.app_id IS 'Denormalized app ID so ownership checks (e.g. revocation) do not need to join through app_secret_id, which is NULL for public clients.'; +COMMENT ON COLUMN oauth2_provider_app_tokens.scope IS 'Space-separated scope granted to this token. A refresh may narrow this but never widen it. NULL means no scope was recorded and the token is unrestricted.'; + CREATE TABLE oauth2_provider_apps ( id uuid NOT NULL, created_at timestamp with time zone NOT NULL, diff --git a/coderd/database/migrations/000567_oauth2_scope_enforcement.down.sql b/coderd/database/migrations/000567_oauth2_scope_enforcement.down.sql new file mode 100644 index 0000000000000..cc1658b160a4c --- /dev/null +++ b/coderd/database/migrations/000567_oauth2_scope_enforcement.down.sql @@ -0,0 +1,3 @@ +ALTER TABLE oauth2_provider_app_codes DROP COLUMN scope; + +ALTER TABLE oauth2_provider_app_tokens DROP COLUMN scope; diff --git a/coderd/database/migrations/000567_oauth2_scope_enforcement.up.sql b/coderd/database/migrations/000567_oauth2_scope_enforcement.up.sql new file mode 100644 index 0000000000000..00782248da4f5 --- /dev/null +++ b/coderd/database/migrations/000567_oauth2_scope_enforcement.up.sql @@ -0,0 +1,16 @@ +-- The scope negotiated at /oauth2/authorize travels with the grant itself: +-- recorded on the code when it is issued, then carried onto the token it is +-- exchanged for so a refresh can narrow against what was actually granted +-- rather than against the app's current allowlist. +-- +-- Both columns are nullable with no backfill. A NULL means "no scope was +-- recorded for this grant", which the token endpoint reads as unrestricted +-- access, so codes and tokens issued before this migration keep working. + +ALTER TABLE oauth2_provider_app_codes ADD COLUMN scope text; + +ALTER TABLE oauth2_provider_app_tokens ADD COLUMN scope text; + +COMMENT ON COLUMN oauth2_provider_app_codes.scope IS 'Space-separated scope negotiated at authorization time. NULL means no scope was recorded and the exchanged token is unrestricted.'; + +COMMENT ON COLUMN oauth2_provider_app_tokens.scope IS 'Space-separated scope granted to this token. A refresh may narrow this but never widen it. NULL means no scope was recorded and the token is unrestricted.'; diff --git a/coderd/database/models.go b/coderd/database/models.go index a68b7e54bc924..0c0bc5e4a7921 100644 --- a/coderd/database/models.go +++ b/coderd/database/models.go @@ -5583,6 +5583,8 @@ type OAuth2ProviderAppCode struct { StateHash sql.NullString `db:"state_hash" json:"state_hash"` // The redirect_uri provided during authorization, to be verified during token exchange (RFC 6749 §4.1.3). RedirectUri sql.NullString `db:"redirect_uri" json:"redirect_uri"` + // Space-separated scope negotiated at authorization time. NULL means no scope was recorded and the exchanged token is unrestricted. + Scope sql.NullString `db:"scope" json:"scope"` } type OAuth2ProviderAppSecret struct { @@ -5611,6 +5613,8 @@ type OAuth2ProviderAppToken struct { UserID uuid.UUID `db:"user_id" json:"user_id"` // Denormalized app ID so ownership checks (e.g. revocation) do not need to join through app_secret_id, which is NULL for public clients. AppID uuid.UUID `db:"app_id" json:"app_id"` + // Space-separated scope granted to this token. A refresh may narrow this but never widen it. NULL means no scope was recorded and the token is unrestricted. + Scope sql.NullString `db:"scope" json:"scope"` } type Organization struct { diff --git a/coderd/database/querier.go b/coderd/database/querier.go index a7f52a88464f2..3d25f691ce480 100644 --- a/coderd/database/querier.go +++ b/coderd/database/querier.go @@ -117,6 +117,9 @@ type sqlcQuerier interface { DeleteAIProviderByID(ctx context.Context, id uuid.UUID) error DeleteAIProviderKey(ctx context.Context, id uuid.UUID) error DeleteAPIKeyByID(ctx context.Context, id string) error + // Returns sql.ErrNoRows when the key is already gone, which lets a caller + // enforce single use of a refresh token by racing this delete. + DeleteAPIKeyByIDReturningID(ctx context.Context, id string) (string, error) DeleteAPIKeysByUserID(ctx context.Context, userID uuid.UUID) error // Deletes all heartbeat rows for the chat. Used during ownership // transitions that abandon a lease. @@ -164,6 +167,9 @@ type sqlcQuerier interface { DeleteOAuth2ProviderAppByClientID(ctx context.Context, id uuid.UUID) error DeleteOAuth2ProviderAppByID(ctx context.Context, id uuid.UUID) error DeleteOAuth2ProviderAppCodeByID(ctx context.Context, id uuid.UUID) error + // Returns sql.ErrNoRows when the code was already redeemed, which lets a + // caller enforce single use by racing this delete instead of reading first. + DeleteOAuth2ProviderAppCodeByIDReturningID(ctx context.Context, id uuid.UUID) (uuid.UUID, error) DeleteOAuth2ProviderAppCodesByAppAndUserID(ctx context.Context, arg DeleteOAuth2ProviderAppCodesByAppAndUserIDParams) error DeleteOAuth2ProviderAppSecretByID(ctx context.Context, id uuid.UUID) error // Filters directly on app_id rather than joining through app_secret_id, diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index 1682eb894a008..ab65caca44a59 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -3656,6 +3656,23 @@ func (q *sqlQuerier) DeleteAPIKeyByID(ctx context.Context, id string) error { return err } +const deleteAPIKeyByIDReturningID = `-- name: DeleteAPIKeyByIDReturningID :one +DELETE FROM + api_keys +WHERE + id = $1 +RETURNING id +` + +// Returns sql.ErrNoRows when the key is already gone, which lets a caller +// enforce single use of a refresh token by racing this delete. +func (q *sqlQuerier) DeleteAPIKeyByIDReturningID(ctx context.Context, id string) (string, error) { + row := q.db.QueryRowContext(ctx, deleteAPIKeyByIDReturningID, id) + var id_2 string + err := row.Scan(&id_2) + return id_2, err +} + const deleteAPIKeysByUserID = `-- name: DeleteAPIKeysByUserID :exec DELETE FROM api_keys @@ -18862,6 +18879,19 @@ func (q *sqlQuerier) DeleteOAuth2ProviderAppCodeByID(ctx context.Context, id uui return err } +const deleteOAuth2ProviderAppCodeByIDReturningID = `-- name: DeleteOAuth2ProviderAppCodeByIDReturningID :one +DELETE FROM oauth2_provider_app_codes WHERE id = $1 RETURNING id +` + +// Returns sql.ErrNoRows when the code was already redeemed, which lets a +// caller enforce single use by racing this delete instead of reading first. +func (q *sqlQuerier) DeleteOAuth2ProviderAppCodeByIDReturningID(ctx context.Context, id uuid.UUID) (uuid.UUID, error) { + row := q.db.QueryRowContext(ctx, deleteOAuth2ProviderAppCodeByIDReturningID, id) + var id_2 uuid.UUID + err := row.Scan(&id_2) + return id_2, err +} + const deleteOAuth2ProviderAppCodesByAppAndUserID = `-- name: DeleteOAuth2ProviderAppCodesByAppAndUserID :exec DELETE FROM oauth2_provider_app_codes WHERE app_id = $1 AND user_id = $2 ` @@ -18985,7 +19015,7 @@ func (q *sqlQuerier) GetOAuth2ProviderAppByID(ctx context.Context, id uuid.UUID) } const getOAuth2ProviderAppCodeByID = `-- name: GetOAuth2ProviderAppCodeByID :one -SELECT id, created_at, expires_at, secret_prefix, hashed_secret, user_id, app_id, resource_uri, code_challenge, code_challenge_method, state_hash, redirect_uri FROM oauth2_provider_app_codes WHERE id = $1 +SELECT id, created_at, expires_at, secret_prefix, hashed_secret, user_id, app_id, resource_uri, code_challenge, code_challenge_method, state_hash, redirect_uri, scope FROM oauth2_provider_app_codes WHERE id = $1 ` func (q *sqlQuerier) GetOAuth2ProviderAppCodeByID(ctx context.Context, id uuid.UUID) (OAuth2ProviderAppCode, error) { @@ -19004,12 +19034,13 @@ func (q *sqlQuerier) GetOAuth2ProviderAppCodeByID(ctx context.Context, id uuid.U &i.CodeChallengeMethod, &i.StateHash, &i.RedirectUri, + &i.Scope, ) return i, err } const getOAuth2ProviderAppCodeByPrefix = `-- name: GetOAuth2ProviderAppCodeByPrefix :one -SELECT id, created_at, expires_at, secret_prefix, hashed_secret, user_id, app_id, resource_uri, code_challenge, code_challenge_method, state_hash, redirect_uri FROM oauth2_provider_app_codes WHERE secret_prefix = $1 +SELECT id, created_at, expires_at, secret_prefix, hashed_secret, user_id, app_id, resource_uri, code_challenge, code_challenge_method, state_hash, redirect_uri, scope FROM oauth2_provider_app_codes WHERE secret_prefix = $1 ` func (q *sqlQuerier) GetOAuth2ProviderAppCodeByPrefix(ctx context.Context, secretPrefix []byte) (OAuth2ProviderAppCode, error) { @@ -19028,6 +19059,7 @@ func (q *sqlQuerier) GetOAuth2ProviderAppCodeByPrefix(ctx context.Context, secre &i.CodeChallengeMethod, &i.StateHash, &i.RedirectUri, + &i.Scope, ) return i, err } @@ -19106,7 +19138,7 @@ func (q *sqlQuerier) GetOAuth2ProviderAppSecretsByAppID(ctx context.Context, app } const getOAuth2ProviderAppTokenByAPIKeyID = `-- name: GetOAuth2ProviderAppTokenByAPIKeyID :one -SELECT id, created_at, expires_at, hash_prefix, refresh_hash, app_secret_id, api_key_id, audience, user_id, app_id FROM oauth2_provider_app_tokens WHERE api_key_id = $1 +SELECT id, created_at, expires_at, hash_prefix, refresh_hash, app_secret_id, api_key_id, audience, user_id, app_id, scope FROM oauth2_provider_app_tokens WHERE api_key_id = $1 ` func (q *sqlQuerier) GetOAuth2ProviderAppTokenByAPIKeyID(ctx context.Context, apiKeyID string) (OAuth2ProviderAppToken, error) { @@ -19123,12 +19155,13 @@ func (q *sqlQuerier) GetOAuth2ProviderAppTokenByAPIKeyID(ctx context.Context, ap &i.Audience, &i.UserID, &i.AppID, + &i.Scope, ) return i, err } const getOAuth2ProviderAppTokenByPrefix = `-- name: GetOAuth2ProviderAppTokenByPrefix :one -SELECT id, created_at, expires_at, hash_prefix, refresh_hash, app_secret_id, api_key_id, audience, user_id, app_id FROM oauth2_provider_app_tokens WHERE hash_prefix = $1 +SELECT id, created_at, expires_at, hash_prefix, refresh_hash, app_secret_id, api_key_id, audience, user_id, app_id, scope FROM oauth2_provider_app_tokens WHERE hash_prefix = $1 ` func (q *sqlQuerier) GetOAuth2ProviderAppTokenByPrefix(ctx context.Context, hashPrefix []byte) (OAuth2ProviderAppToken, error) { @@ -19145,6 +19178,7 @@ func (q *sqlQuerier) GetOAuth2ProviderAppTokenByPrefix(ctx context.Context, hash &i.Audience, &i.UserID, &i.AppID, + &i.Scope, ) return i, err } @@ -19436,7 +19470,8 @@ INSERT INTO oauth2_provider_app_codes ( code_challenge, code_challenge_method, state_hash, - redirect_uri + redirect_uri, + scope ) VALUES( $1, $2, @@ -19449,8 +19484,9 @@ INSERT INTO oauth2_provider_app_codes ( $9, $10, $11, - $12 -) RETURNING id, created_at, expires_at, secret_prefix, hashed_secret, user_id, app_id, resource_uri, code_challenge, code_challenge_method, state_hash, redirect_uri + $12, + $13 +) RETURNING id, created_at, expires_at, secret_prefix, hashed_secret, user_id, app_id, resource_uri, code_challenge, code_challenge_method, state_hash, redirect_uri, scope ` type InsertOAuth2ProviderAppCodeParams struct { @@ -19466,6 +19502,7 @@ type InsertOAuth2ProviderAppCodeParams struct { CodeChallengeMethod sql.NullString `db:"code_challenge_method" json:"code_challenge_method"` StateHash sql.NullString `db:"state_hash" json:"state_hash"` RedirectUri sql.NullString `db:"redirect_uri" json:"redirect_uri"` + Scope sql.NullString `db:"scope" json:"scope"` } func (q *sqlQuerier) InsertOAuth2ProviderAppCode(ctx context.Context, arg InsertOAuth2ProviderAppCodeParams) (OAuth2ProviderAppCode, error) { @@ -19482,6 +19519,7 @@ func (q *sqlQuerier) InsertOAuth2ProviderAppCode(ctx context.Context, arg Insert arg.CodeChallengeMethod, arg.StateHash, arg.RedirectUri, + arg.Scope, ) var i OAuth2ProviderAppCode err := row.Scan( @@ -19497,6 +19535,7 @@ func (q *sqlQuerier) InsertOAuth2ProviderAppCode(ctx context.Context, arg Insert &i.CodeChallengeMethod, &i.StateHash, &i.RedirectUri, + &i.Scope, ) return i, err } @@ -19561,7 +19600,8 @@ INSERT INTO oauth2_provider_app_tokens ( app_secret_id, api_key_id, user_id, - audience + audience, + scope ) VALUES( $1, $2, @@ -19572,8 +19612,9 @@ INSERT INTO oauth2_provider_app_tokens ( $7, $8, $9, - $10 -) RETURNING id, created_at, expires_at, hash_prefix, refresh_hash, app_secret_id, api_key_id, audience, user_id, app_id + $10, + $11 +) RETURNING id, created_at, expires_at, hash_prefix, refresh_hash, app_secret_id, api_key_id, audience, user_id, app_id, scope ` type InsertOAuth2ProviderAppTokenParams struct { @@ -19587,6 +19628,7 @@ type InsertOAuth2ProviderAppTokenParams struct { APIKeyID string `db:"api_key_id" json:"api_key_id"` UserID uuid.UUID `db:"user_id" json:"user_id"` Audience sql.NullString `db:"audience" json:"audience"` + Scope sql.NullString `db:"scope" json:"scope"` } func (q *sqlQuerier) InsertOAuth2ProviderAppToken(ctx context.Context, arg InsertOAuth2ProviderAppTokenParams) (OAuth2ProviderAppToken, error) { @@ -19601,6 +19643,7 @@ func (q *sqlQuerier) InsertOAuth2ProviderAppToken(ctx context.Context, arg Inser arg.APIKeyID, arg.UserID, arg.Audience, + arg.Scope, ) var i OAuth2ProviderAppToken err := row.Scan( @@ -19614,6 +19657,7 @@ func (q *sqlQuerier) InsertOAuth2ProviderAppToken(ctx context.Context, arg Inser &i.Audience, &i.UserID, &i.AppID, + &i.Scope, ) return i, err } diff --git a/coderd/database/queries/apikeys.sql b/coderd/database/queries/apikeys.sql index 90e7610cf06db..6c21f60cb9391 100644 --- a/coderd/database/queries/apikeys.sql +++ b/coderd/database/queries/apikeys.sql @@ -92,6 +92,15 @@ DELETE FROM WHERE id = $1; +-- name: DeleteAPIKeyByIDReturningID :one +-- Returns sql.ErrNoRows when the key is already gone, which lets a caller +-- enforce single use of a refresh token by racing this delete. +DELETE FROM + api_keys +WHERE + id = $1 +RETURNING id; + -- name: DeleteApplicationConnectAPIKeysByUserID :exec DELETE FROM api_keys diff --git a/coderd/database/queries/oauth2.sql b/coderd/database/queries/oauth2.sql index f9272b69ea1f6..238d890cd1b02 100644 --- a/coderd/database/queries/oauth2.sql +++ b/coderd/database/queries/oauth2.sql @@ -137,7 +137,8 @@ INSERT INTO oauth2_provider_app_codes ( code_challenge, code_challenge_method, state_hash, - redirect_uri + redirect_uri, + scope ) VALUES( $1, $2, @@ -150,12 +151,18 @@ INSERT INTO oauth2_provider_app_codes ( $9, $10, $11, - $12 + $12, + $13 ) RETURNING *; -- name: DeleteOAuth2ProviderAppCodeByID :exec DELETE FROM oauth2_provider_app_codes WHERE id = $1; +-- name: DeleteOAuth2ProviderAppCodeByIDReturningID :one +-- Returns sql.ErrNoRows when the code was already redeemed, which lets a +-- caller enforce single use by racing this delete instead of reading first. +DELETE FROM oauth2_provider_app_codes WHERE id = $1 RETURNING id; + -- name: DeleteOAuth2ProviderAppCodesByAppAndUserID :exec DELETE FROM oauth2_provider_app_codes WHERE app_id = $1 AND user_id = $2; @@ -170,7 +177,8 @@ INSERT INTO oauth2_provider_app_tokens ( app_secret_id, api_key_id, user_id, - audience + audience, + scope ) VALUES( $1, $2, @@ -181,7 +189,8 @@ INSERT INTO oauth2_provider_app_tokens ( $7, $8, $9, - $10 + $10, + $11 ) RETURNING *; -- name: GetOAuth2ProviderAppTokenByPrefix :one diff --git a/coderd/oauth2provider/authorize.go b/coderd/oauth2provider/authorize.go index 1480259c1fa75..9fee4b37eda4b 100644 --- a/coderd/oauth2provider/authorize.go +++ b/coderd/oauth2provider/authorize.go @@ -259,6 +259,9 @@ func ProcessAuthorize(db database.Store) http.HandlerFunc { CodeChallengeMethod: sql.NullString{String: params.codeChallengeMethod, Valid: params.codeChallengeMethod != ""}, StateHash: hashOAuth2State(params.state), RedirectUri: sql.NullString{String: params.redirectURL.String(), Valid: params.redirectURIProvided}, + // A NULL scope records no restriction, so the token this code + // is exchanged for gets unrestricted access. + Scope: sql.NullString{}, }) if err != nil { return xerrors.Errorf("insert oauth2 authorization code: %w", err) diff --git a/coderd/oauth2provider/tokens.go b/coderd/oauth2provider/tokens.go index 3761d1010ca4e..902784fc21555 100644 --- a/coderd/oauth2provider/tokens.go +++ b/coderd/oauth2provider/tokens.go @@ -375,6 +375,9 @@ func authorizationCodeGrant(ctx context.Context, db database.Store, app database APIKeyID: newKey.ID, UserID: dbCode.UserID, Audience: dbCode.ResourceUri, + // A NULL scope records no restriction, so this token gets + // unrestricted access. + Scope: sql.NullString{}, }) if err != nil { return xerrors.Errorf("insert oauth2 refresh token: %w", err) @@ -499,6 +502,9 @@ func refreshTokenGrant(ctx context.Context, db database.Store, app database.OAut APIKeyID: newKey.ID, UserID: dbToken.UserID, Audience: dbToken.Audience, + // A NULL scope records no restriction, so this token gets + // unrestricted access. + Scope: sql.NullString{}, }) if err != nil { return xerrors.Errorf("insert oauth2 refresh token: %w", err) From 50f3466107dc7380da1c34b0af59a6e5f91a204d Mon Sep 17 00:00:00 2001 From: Bobby Ho Date: Tue, 11 Aug 2026 08:33:12 -0700 Subject: [PATCH 2/7] fix(coderd): make oauth2 grant scope explicit and non-nullable Both scope columns were nullable with NULL meaning "unrestricted", which made the most privileged state the one a forgotten field produces: sql.NullString{} is NULL is full access, and exhaustruct is satisfied by exactly that literal. An audit of either table could not separate a deliberate legacy grant from a mint path that dropped the scope. Backfill both columns to coder:all, which records what existing rows already have in fact since apikey.Generate defaults minted OAuth2 keys to that scope, then apply NOT NULL and CHECK (scope <> ''). NOT NULL alone would not be enough: sqlc maps text NOT NULL to a Go string whose zero value inserts cleanly, so the fail-closed property needs both clauses. No DEFAULT survives, or an INSERT omitting the column would silently receive an unrestricted grant. Matches the encoding api_keys.scopes and workspace_agents.api_key_scope already use, and follows migration 000389's backfill-then-constrain shape. The two grant paths now carry the parent's scope forward (Scope: dbCode.Scope, Scope: dbToken.Scope) instead of hardcoding an empty value, which is RFC 6749 section 6's default and removes the phase-ordering hazard where a scoped token could refresh into an unrestricted one. ProcessAuthorize writes the sentinel, since persisting a requested scope before validation exists would store unvalidated client input. Refs PLAT-478 Co-Authored-By: Claude Opus 5 (1M context) --- coderd/database/check_constraint.go | 2 + coderd/database/constants.go | 10 +++ coderd/database/dbauthz/dbauthz_test.go | 2 + coderd/database/dbgen/dbgen.go | 4 +- coderd/database/dump.sql | 10 +-- .../000567_oauth2_scope_enforcement.up.sql | 28 ++++++-- coderd/database/models.go | 8 +-- coderd/database/querier_test.go | 64 +++++++++++++++++++ coderd/database/queries.sql.go | 4 +- coderd/oauth2provider/authorize.go | 8 ++- coderd/oauth2provider/tokens.go | 11 ++-- 11 files changed, 123 insertions(+), 28 deletions(-) diff --git a/coderd/database/check_constraint.go b/coderd/database/check_constraint.go index 268009cd29b76..0402e4b8ee7a7 100644 --- a/coderd/database/check_constraint.go +++ b/coderd/database/check_constraint.go @@ -44,6 +44,8 @@ const ( CheckMcpServerConfigsAuthTypeCheck CheckConstraint = "mcp_server_configs_auth_type_check" // mcp_server_configs CheckMcpServerConfigsAvailabilityCheck CheckConstraint = "mcp_server_configs_availability_check" // mcp_server_configs CheckMcpServerConfigsTransportCheck CheckConstraint = "mcp_server_configs_transport_check" // mcp_server_configs + CheckOauth2ProviderAppCodesScopeNotEmpty CheckConstraint = "oauth2_provider_app_codes_scope_not_empty" // oauth2_provider_app_codes + CheckOauth2ProviderAppTokensScopeNotEmpty CheckConstraint = "oauth2_provider_app_tokens_scope_not_empty" // oauth2_provider_app_tokens CheckOauth2ProviderAppsClientTypeCheck CheckConstraint = "oauth2_provider_apps_client_type_check" // oauth2_provider_apps CheckMaxProvisionerLogsLength CheckConstraint = "max_provisioner_logs_length" // provisioner_jobs CheckNatsPortValidTcp CheckConstraint = "nats_port_valid_tcp" // replicas diff --git a/coderd/database/constants.go b/coderd/database/constants.go index 34ad1005ee4c0..bb11f8fa531fa 100644 --- a/coderd/database/constants.go +++ b/coderd/database/constants.go @@ -10,3 +10,13 @@ import ( // for use as a uuid.UUID. Both must agree; tests pin the value to the // codersdk constant so the two cannot drift. var PrebuildsSystemUserID = uuid.MustParse(codersdk.PrebuildsSystemUserID) + +// OAuth2ScopeUnrestricted is the oauth2_provider_app_codes.scope and +// oauth2_provider_app_tokens.scope value recording a grant that carries no +// restriction. Both columns hold space-separated values from the +// api_key_scope vocabulary, so an unrestricted grant is spelled the same way +// api_keys.scopes spells it. The columns are NOT NULL: writing this constant +// is how a caller states "unrestricted" on purpose, which is what +// distinguishes a deliberate grant from a scope that was never threaded +// through. +const OAuth2ScopeUnrestricted = string(ApiKeyScopeCoderAll) diff --git a/coderd/database/dbauthz/dbauthz_test.go b/coderd/database/dbauthz/dbauthz_test.go index 7d83e57f11eac..3be11dc1e9352 100644 --- a/coderd/database/dbauthz/dbauthz_test.go +++ b/coderd/database/dbauthz/dbauthz_test.go @@ -5996,6 +5996,7 @@ func (s *MethodTestSuite) TestOAuth2ProviderAppCodes() { check.Args(database.InsertOAuth2ProviderAppCodeParams{ AppID: app.ID, UserID: user.ID, + Scope: database.OAuth2ScopeUnrestricted, }).Asserts(rbac.ResourceOauth2AppCodeToken.WithOwner(user.ID.String()), policy.ActionCreate) })) s.Run("DeleteOAuth2ProviderAppCodeByID", s.Subtest(func(db database.Store, check *expects) { @@ -6049,6 +6050,7 @@ func (s *MethodTestSuite) TestOAuth2ProviderAppTokens() { AppSecretID: uuid.NullUUID{UUID: secret.ID, Valid: true}, APIKeyID: key.ID, UserID: user.ID, + Scope: database.OAuth2ScopeUnrestricted, }).Asserts(rbac.ResourceOauth2AppCodeToken.WithOwner(user.ID.String()), policy.ActionCreate) })) s.Run("GetOAuth2ProviderAppTokenByPrefix", s.Subtest(func(db database.Store, check *expects) { diff --git a/coderd/database/dbgen/dbgen.go b/coderd/database/dbgen/dbgen.go index 0e3b3ba952a81..2a52436aa6031 100644 --- a/coderd/database/dbgen/dbgen.go +++ b/coderd/database/dbgen/dbgen.go @@ -1784,7 +1784,7 @@ func OAuth2ProviderAppCode(t testing.TB, db database.Store, seed database.OAuth2 CodeChallengeMethod: seed.CodeChallengeMethod, StateHash: seed.StateHash, RedirectUri: seed.RedirectUri, - Scope: seed.Scope, + Scope: takeFirst(seed.Scope, database.OAuth2ScopeUnrestricted), }) require.NoError(t, err, "insert oauth2 app code") return code @@ -1806,7 +1806,7 @@ func OAuth2ProviderAppToken(t testing.TB, db database.Store, seed database.OAuth APIKeyID: takeFirst(seed.APIKeyID, uuid.New().String()), UserID: takeFirst(seed.UserID, uuid.New()), Audience: seed.Audience, - Scope: seed.Scope, + Scope: takeFirst(seed.Scope, database.OAuth2ScopeUnrestricted), }) require.NoError(t, err, "insert oauth2 app token") return token diff --git a/coderd/database/dump.sql b/coderd/database/dump.sql index eddf1e0346530..63f5286220257 100644 --- a/coderd/database/dump.sql +++ b/coderd/database/dump.sql @@ -2597,7 +2597,8 @@ CREATE TABLE oauth2_provider_app_codes ( code_challenge_method text, state_hash text, redirect_uri text, - scope text + scope text NOT NULL, + CONSTRAINT oauth2_provider_app_codes_scope_not_empty CHECK ((scope <> ''::text)) ); COMMENT ON TABLE oauth2_provider_app_codes IS 'Codes are meant to be exchanged for access tokens.'; @@ -2612,7 +2613,7 @@ COMMENT ON COLUMN oauth2_provider_app_codes.state_hash IS 'SHA-256 hash of the O COMMENT ON COLUMN oauth2_provider_app_codes.redirect_uri IS 'The redirect_uri provided during authorization, to be verified during token exchange (RFC 6749 §4.1.3).'; -COMMENT ON COLUMN oauth2_provider_app_codes.scope IS 'Space-separated scope negotiated at authorization time. NULL means no scope was recorded and the exchanged token is unrestricted.'; +COMMENT ON COLUMN oauth2_provider_app_codes.scope IS 'Space-separated scope negotiated at authorization time, drawn from the api_key_scope vocabulary. Always set; coder:all records an unrestricted grant.'; CREATE TABLE oauth2_provider_app_secrets ( id uuid NOT NULL, @@ -2637,7 +2638,8 @@ CREATE TABLE oauth2_provider_app_tokens ( audience text, user_id uuid NOT NULL, app_id uuid NOT NULL, - scope text + scope text NOT NULL, + CONSTRAINT oauth2_provider_app_tokens_scope_not_empty CHECK ((scope <> ''::text)) ); COMMENT ON COLUMN oauth2_provider_app_tokens.refresh_hash IS 'Refresh tokens provide a way to refresh an access token (API key). An expired API key can be refreshed if this token is not yet expired, meaning this expiry can outlive an API key.'; @@ -2648,7 +2650,7 @@ COMMENT ON COLUMN oauth2_provider_app_tokens.user_id IS 'Denormalized user ID fo COMMENT ON COLUMN oauth2_provider_app_tokens.app_id IS 'Denormalized app ID so ownership checks (e.g. revocation) do not need to join through app_secret_id, which is NULL for public clients.'; -COMMENT ON COLUMN oauth2_provider_app_tokens.scope IS 'Space-separated scope granted to this token. A refresh may narrow this but never widen it. NULL means no scope was recorded and the token is unrestricted.'; +COMMENT ON COLUMN oauth2_provider_app_tokens.scope IS 'Space-separated scope granted to this token, drawn from the api_key_scope vocabulary. Always set; coder:all records an unrestricted grant. Later phases will narrow this on refresh and never widen it.'; CREATE TABLE oauth2_provider_apps ( id uuid NOT NULL, diff --git a/coderd/database/migrations/000567_oauth2_scope_enforcement.up.sql b/coderd/database/migrations/000567_oauth2_scope_enforcement.up.sql index 00782248da4f5..2cfb8d8315e30 100644 --- a/coderd/database/migrations/000567_oauth2_scope_enforcement.up.sql +++ b/coderd/database/migrations/000567_oauth2_scope_enforcement.up.sql @@ -1,16 +1,30 @@ -- The scope negotiated at /oauth2/authorize travels with the grant itself: -- recorded on the code when it is issued, then carried onto the token it is --- exchanged for so a refresh can narrow against what was actually granted --- rather than against the app's current allowlist. +-- exchanged for, so a refresh can be narrowed against what was actually +-- granted rather than against the app's current allowlist. -- --- Both columns are nullable with no backfill. A NULL means "no scope was --- recorded for this grant", which the token endpoint reads as unrestricted --- access, so codes and tokens issued before this migration keep working. +-- Existing rows are unrestricted in fact rather than by omission, since +-- apikey.Generate mints every OAuth2 access key with the coder:all scope. +-- The backfill writes that down. Both columns are then NOT NULL with no +-- default, so a grant's authority is always stated explicitly and a caller +-- that omits the column fails instead of silently issuing full access. ALTER TABLE oauth2_provider_app_codes ADD COLUMN scope text; ALTER TABLE oauth2_provider_app_tokens ADD COLUMN scope text; -COMMENT ON COLUMN oauth2_provider_app_codes.scope IS 'Space-separated scope negotiated at authorization time. NULL means no scope was recorded and the exchanged token is unrestricted.'; +UPDATE oauth2_provider_app_codes SET scope = 'coder:all' WHERE scope IS NULL; -COMMENT ON COLUMN oauth2_provider_app_tokens.scope IS 'Space-separated scope granted to this token. A refresh may narrow this but never widen it. NULL means no scope was recorded and the token is unrestricted.'; +UPDATE oauth2_provider_app_tokens SET scope = 'coder:all' WHERE scope IS NULL; + +ALTER TABLE oauth2_provider_app_codes + ALTER COLUMN scope SET NOT NULL, + ADD CONSTRAINT oauth2_provider_app_codes_scope_not_empty CHECK (scope <> ''); + +ALTER TABLE oauth2_provider_app_tokens + ALTER COLUMN scope SET NOT NULL, + ADD CONSTRAINT oauth2_provider_app_tokens_scope_not_empty CHECK (scope <> ''); + +COMMENT ON COLUMN oauth2_provider_app_codes.scope IS 'Space-separated scope negotiated at authorization time, drawn from the api_key_scope vocabulary. Always set; coder:all records an unrestricted grant.'; + +COMMENT ON COLUMN oauth2_provider_app_tokens.scope IS 'Space-separated scope granted to this token, drawn from the api_key_scope vocabulary. Always set; coder:all records an unrestricted grant. Later phases will narrow this on refresh and never widen it.'; diff --git a/coderd/database/models.go b/coderd/database/models.go index 0c0bc5e4a7921..c80a23665c009 100644 --- a/coderd/database/models.go +++ b/coderd/database/models.go @@ -5583,8 +5583,8 @@ type OAuth2ProviderAppCode struct { StateHash sql.NullString `db:"state_hash" json:"state_hash"` // The redirect_uri provided during authorization, to be verified during token exchange (RFC 6749 §4.1.3). RedirectUri sql.NullString `db:"redirect_uri" json:"redirect_uri"` - // Space-separated scope negotiated at authorization time. NULL means no scope was recorded and the exchanged token is unrestricted. - Scope sql.NullString `db:"scope" json:"scope"` + // Space-separated scope negotiated at authorization time, drawn from the api_key_scope vocabulary. Always set; coder:all records an unrestricted grant. + Scope string `db:"scope" json:"scope"` } type OAuth2ProviderAppSecret struct { @@ -5613,8 +5613,8 @@ type OAuth2ProviderAppToken struct { UserID uuid.UUID `db:"user_id" json:"user_id"` // Denormalized app ID so ownership checks (e.g. revocation) do not need to join through app_secret_id, which is NULL for public clients. AppID uuid.UUID `db:"app_id" json:"app_id"` - // Space-separated scope granted to this token. A refresh may narrow this but never widen it. NULL means no scope was recorded and the token is unrestricted. - Scope sql.NullString `db:"scope" json:"scope"` + // Space-separated scope granted to this token, drawn from the api_key_scope vocabulary. Always set; coder:all records an unrestricted grant. Later phases will narrow this on refresh and never widen it. + Scope string `db:"scope" json:"scope"` } type Organization struct { diff --git a/coderd/database/querier_test.go b/coderd/database/querier_test.go index e84b81b79ccab..4ec0a94f36907 100644 --- a/coderd/database/querier_test.go +++ b/coderd/database/querier_test.go @@ -18817,3 +18817,67 @@ func TestGetActiveUsersAuthorizationRolesParity(t *testing.T) { require.ElementsMatch(t, single.Groups, row.Groups, "groups diverged for user %s", row.ID) } } + +func TestOAuth2ProviderScopeNotEmpty(t *testing.T) { + t.Parallel() + if testing.Short() { + t.SkipNow() + } + + // An unrestricted grant is recorded as an explicit sentinel rather than as + // an absent value, so an insert that fails to carry the negotiated scope + // forward is rejected instead of silently issuing full access. + t.Run("Code", func(t *testing.T) { + t.Parallel() + db, _ := dbtestutil.NewDB(t) + ctx := testutil.Context(t, testutil.WaitLong) + + user := dbgen.User(t, db, database.User{}) + app := dbgen.OAuth2ProviderApp(t, db, database.OAuth2ProviderApp{}) + + _, err := db.InsertOAuth2ProviderAppCode(ctx, database.InsertOAuth2ProviderAppCodeParams{ + ID: uuid.New(), + CreatedAt: dbtime.Now(), + ExpiresAt: dbtime.Now().Add(time.Minute), + SecretPrefix: []byte("prefix"), + HashedSecret: []byte("hashed-secret"), + AppID: app.ID, + UserID: user.ID, + ResourceUri: sql.NullString{}, + CodeChallenge: sql.NullString{}, + CodeChallengeMethod: sql.NullString{}, + StateHash: sql.NullString{}, + RedirectUri: sql.NullString{}, + Scope: "", + }) + require.True(t, database.IsCheckViolation(err, database.CheckOauth2ProviderAppCodesScopeNotEmpty), + "empty scope must be rejected, got %v", err) + }) + + t.Run("Token", func(t *testing.T) { + t.Parallel() + db, _ := dbtestutil.NewDB(t) + ctx := testutil.Context(t, testutil.WaitLong) + + user := dbgen.User(t, db, database.User{}) + app := dbgen.OAuth2ProviderApp(t, db, database.OAuth2ProviderApp{}) + secret := dbgen.OAuth2ProviderAppSecret(t, db, database.OAuth2ProviderAppSecret{AppID: app.ID}) + key, _ := dbgen.APIKey(t, db, database.APIKey{UserID: user.ID}) + + _, err := db.InsertOAuth2ProviderAppToken(ctx, database.InsertOAuth2ProviderAppTokenParams{ + ID: uuid.New(), + CreatedAt: dbtime.Now(), + ExpiresAt: dbtime.Now().Add(time.Minute), + HashPrefix: []byte("prefix"), + RefreshHash: []byte("hashed-secret"), + AppID: app.ID, + AppSecretID: uuid.NullUUID{UUID: secret.ID, Valid: true}, + APIKeyID: key.ID, + UserID: user.ID, + Audience: sql.NullString{}, + Scope: "", + }) + require.True(t, database.IsCheckViolation(err, database.CheckOauth2ProviderAppTokensScopeNotEmpty), + "empty scope must be rejected, got %v", err) + }) +} diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index ab65caca44a59..fbb8ebd27acc9 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -19502,7 +19502,7 @@ type InsertOAuth2ProviderAppCodeParams struct { CodeChallengeMethod sql.NullString `db:"code_challenge_method" json:"code_challenge_method"` StateHash sql.NullString `db:"state_hash" json:"state_hash"` RedirectUri sql.NullString `db:"redirect_uri" json:"redirect_uri"` - Scope sql.NullString `db:"scope" json:"scope"` + Scope string `db:"scope" json:"scope"` } func (q *sqlQuerier) InsertOAuth2ProviderAppCode(ctx context.Context, arg InsertOAuth2ProviderAppCodeParams) (OAuth2ProviderAppCode, error) { @@ -19628,7 +19628,7 @@ type InsertOAuth2ProviderAppTokenParams struct { APIKeyID string `db:"api_key_id" json:"api_key_id"` UserID uuid.UUID `db:"user_id" json:"user_id"` Audience sql.NullString `db:"audience" json:"audience"` - Scope sql.NullString `db:"scope" json:"scope"` + Scope string `db:"scope" json:"scope"` } func (q *sqlQuerier) InsertOAuth2ProviderAppToken(ctx context.Context, arg InsertOAuth2ProviderAppTokenParams) (OAuth2ProviderAppToken, error) { diff --git a/coderd/oauth2provider/authorize.go b/coderd/oauth2provider/authorize.go index 9fee4b37eda4b..7e03c1ab860dc 100644 --- a/coderd/oauth2provider/authorize.go +++ b/coderd/oauth2provider/authorize.go @@ -259,9 +259,11 @@ func ProcessAuthorize(db database.Store) http.HandlerFunc { CodeChallengeMethod: sql.NullString{String: params.codeChallengeMethod, Valid: params.codeChallengeMethod != ""}, StateHash: hashOAuth2State(params.state), RedirectUri: sql.NullString{String: params.redirectURL.String(), Valid: params.redirectURIProvided}, - // A NULL scope records no restriction, so the token this code - // is exchanged for gets unrestricted access. - Scope: sql.NullString{}, + // Scope negotiation lands in a later phase. Until the + // requested scope is validated against the app's allowlist, + // persisting it here would store unvalidated client input, so + // the code records an unrestricted grant. + Scope: database.OAuth2ScopeUnrestricted, }) if err != nil { return xerrors.Errorf("insert oauth2 authorization code: %w", err) diff --git a/coderd/oauth2provider/tokens.go b/coderd/oauth2provider/tokens.go index 902784fc21555..bf8b11b6763e8 100644 --- a/coderd/oauth2provider/tokens.go +++ b/coderd/oauth2provider/tokens.go @@ -375,9 +375,7 @@ func authorizationCodeGrant(ctx context.Context, db database.Store, app database APIKeyID: newKey.ID, UserID: dbCode.UserID, Audience: dbCode.ResourceUri, - // A NULL scope records no restriction, so this token gets - // unrestricted access. - Scope: sql.NullString{}, + Scope: dbCode.Scope, }) if err != nil { return xerrors.Errorf("insert oauth2 refresh token: %w", err) @@ -502,9 +500,10 @@ func refreshTokenGrant(ctx context.Context, db database.Store, app database.OAut APIKeyID: newKey.ID, UserID: dbToken.UserID, Audience: dbToken.Audience, - // A NULL scope records no restriction, so this token gets - // unrestricted access. - Scope: sql.NullString{}, + // RFC 6749 §6: a refresh with no scope parameter is granted the + // originally granted scope. Later phases narrow this against + // req.Scope; they never widen it. + Scope: dbToken.Scope, }) if err != nil { return xerrors.Errorf("insert oauth2 refresh token: %w", err) From 6f5e05790cf735ab76c5c887db612140072e9739 Mon Sep 17 00:00:00 2001 From: Bobby Ho Date: Tue, 11 Aug 2026 09:03:56 -0700 Subject: [PATCH 3/7] refactor(coderd/database): return the deleted row from single-use deletes Both single-use deletes returned a bare id, which forced a hand-written dbauthz wrapper each. Returning the whole row lets them collapse into the existing fetchAndQuery generic, since that helper unifies its fetch and query on one rbac.Objecter and a bare id satisfies no such interface. Each 10-line wrapper becomes a single call, and a caller now reads the deleted row's state, including a code's negotiated scope, from the same atomic delete rather than trusting an earlier unauthorized read. Renamed to ...ByIDReturningRow, since ...ReturningID no longer describes them. Add TestSingleUseDeleteByIDReturningRow, which pins the contract both queries exist for: the first delete returns the row, a second returns sql.ErrNoRows. Neither query previously executed against a real database on its already-gone path, so converting one back to :exec or adding a soft delete would have broken single use with CI still green. The concurrent exactly-one-winner half is deliberately not covered here; it exercises Postgres row-lock semantics rather than this code. Rename migration 000567 to oauth2_scope_columns. It adds columns and constraints; enforcement lands in a later phase, and migration names freeze at merge. Refs PLAT-478 Co-Authored-By: Claude Opus 5 (1M context) --- coderd/database/dbauthz/dbauthz.go | 22 ++----- coderd/database/dbauthz/dbauthz_test.go | 10 +-- coderd/database/dbmetrics/querymetrics.go | 16 ++--- coderd/database/dbmock/dbmock.go | 28 ++++---- ...l => 000567_oauth2_scope_columns.down.sql} | 0 ...sql => 000567_oauth2_scope_columns.up.sql} | 0 coderd/database/querier.go | 14 ++-- coderd/database/querier_test.go | 50 ++++++++++++++ coderd/database/queries.sql.go | 66 ++++++++++++++----- coderd/database/queries/apikeys.sql | 8 ++- coderd/database/queries/oauth2.sql | 10 +-- 11 files changed, 150 insertions(+), 74 deletions(-) rename coderd/database/migrations/{000567_oauth2_scope_enforcement.down.sql => 000567_oauth2_scope_columns.down.sql} (100%) rename coderd/database/migrations/{000567_oauth2_scope_enforcement.up.sql => 000567_oauth2_scope_columns.up.sql} (100%) diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go index d30073105adbb..c79f8dfea5fd1 100644 --- a/coderd/database/dbauthz/dbauthz.go +++ b/coderd/database/dbauthz/dbauthz.go @@ -2067,15 +2067,8 @@ func (q *querier) DeleteAPIKeyByID(ctx context.Context, id string) error { return deleteQ(q.log, q.auth, q.db.GetAPIKeyByID, q.db.DeleteAPIKeyByID)(ctx, id) } -func (q *querier) DeleteAPIKeyByIDReturningID(ctx context.Context, id string) (string, error) { - key, err := q.db.GetAPIKeyByID(ctx, id) - if err != nil { - return "", err - } - if err := q.authorizeContext(ctx, policy.ActionDelete, key); err != nil { - return "", err - } - return q.db.DeleteAPIKeyByIDReturningID(ctx, id) +func (q *querier) DeleteAPIKeyByIDReturningRow(ctx context.Context, id string) (database.APIKey, error) { + return fetchAndQuery(q.log, q.auth, policy.ActionDelete, q.db.GetAPIKeyByID, q.db.DeleteAPIKeyByIDReturningRow)(ctx, id) } func (q *querier) DeleteAPIKeysByUserID(ctx context.Context, userID uuid.UUID) error { @@ -2325,15 +2318,8 @@ func (q *querier) DeleteOAuth2ProviderAppCodeByID(ctx context.Context, id uuid.U return q.db.DeleteOAuth2ProviderAppCodeByID(ctx, id) } -func (q *querier) DeleteOAuth2ProviderAppCodeByIDReturningID(ctx context.Context, id uuid.UUID) (uuid.UUID, error) { - code, err := q.db.GetOAuth2ProviderAppCodeByID(ctx, id) - if err != nil { - return uuid.Nil, err - } - if err := q.authorizeContext(ctx, policy.ActionDelete, code); err != nil { - return uuid.Nil, err - } - return q.db.DeleteOAuth2ProviderAppCodeByIDReturningID(ctx, id) +func (q *querier) DeleteOAuth2ProviderAppCodeByIDReturningRow(ctx context.Context, id uuid.UUID) (database.OAuth2ProviderAppCode, error) { + return fetchAndQuery(q.log, q.auth, policy.ActionDelete, q.db.GetOAuth2ProviderAppCodeByID, q.db.DeleteOAuth2ProviderAppCodeByIDReturningRow)(ctx, id) } func (q *querier) DeleteOAuth2ProviderAppCodesByAppAndUserID(ctx context.Context, arg database.DeleteOAuth2ProviderAppCodesByAppAndUserIDParams) error { diff --git a/coderd/database/dbauthz/dbauthz_test.go b/coderd/database/dbauthz/dbauthz_test.go index 3be11dc1e9352..954076f7dca12 100644 --- a/coderd/database/dbauthz/dbauthz_test.go +++ b/coderd/database/dbauthz/dbauthz_test.go @@ -360,11 +360,11 @@ 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("DeleteAPIKeyByIDReturningID", s.Mocked(func(dbm *dbmock.MockStore, faker *gofakeit.Faker, check *expects) { + s.Run("DeleteAPIKeyByIDReturningRow", 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() - dbm.EXPECT().DeleteAPIKeyByIDReturningID(gomock.Any(), key.ID).Return(key.ID, nil).AnyTimes() - check.Args(key.ID).Asserts(key, policy.ActionDelete).Returns(key.ID) + dbm.EXPECT().DeleteAPIKeyByIDReturningRow(gomock.Any(), key.ID).Return(key, nil).AnyTimes() + check.Args(key.ID).Asserts(key, policy.ActionDelete).Returns(key) })) s.Run("DeleteExpiredAPIKeys", s.Mocked(func(dbm *dbmock.MockStore, faker *gofakeit.Faker, check *expects) { args := database.DeleteExpiredAPIKeysParams{ @@ -6008,14 +6008,14 @@ func (s *MethodTestSuite) TestOAuth2ProviderAppCodes() { }) check.Args(code.ID).Asserts(code, policy.ActionDelete) })) - s.Run("DeleteOAuth2ProviderAppCodeByIDReturningID", s.Subtest(func(db database.Store, check *expects) { + s.Run("DeleteOAuth2ProviderAppCodeByIDReturningRow", s.Subtest(func(db database.Store, check *expects) { user := dbgen.User(s.T(), db, database.User{}) app := dbgen.OAuth2ProviderApp(s.T(), db, database.OAuth2ProviderApp{}) code := dbgen.OAuth2ProviderAppCode(s.T(), db, database.OAuth2ProviderAppCode{ AppID: app.ID, UserID: user.ID, }) - check.Args(code.ID).Asserts(code, policy.ActionDelete).Returns(code.ID) + check.Args(code.ID).Asserts(code, policy.ActionDelete).Returns(code) })) s.Run("DeleteOAuth2ProviderAppCodesByAppAndUserID", s.Subtest(func(db database.Store, check *expects) { dbtestutil.DisableForeignKeysAndTriggers(s.T(), db) diff --git a/coderd/database/dbmetrics/querymetrics.go b/coderd/database/dbmetrics/querymetrics.go index 4d28437766916..f16f5257d3b89 100644 --- a/coderd/database/dbmetrics/querymetrics.go +++ b/coderd/database/dbmetrics/querymetrics.go @@ -425,11 +425,11 @@ func (m queryMetricsStore) DeleteAPIKeyByID(ctx context.Context, id string) erro return r0 } -func (m queryMetricsStore) DeleteAPIKeyByIDReturningID(ctx context.Context, id string) (string, error) { +func (m queryMetricsStore) DeleteAPIKeyByIDReturningRow(ctx context.Context, id string) (database.APIKey, error) { start := time.Now() - r0, r1 := m.s.DeleteAPIKeyByIDReturningID(ctx, id) - m.queryLatencies.WithLabelValues("DeleteAPIKeyByIDReturningID").Observe(time.Since(start).Seconds()) - m.queryCounts.WithLabelValues(httpmw.ExtractHTTPRoute(ctx), httpmw.ExtractHTTPMethod(ctx), "DeleteAPIKeyByIDReturningID").Inc() + r0, r1 := m.s.DeleteAPIKeyByIDReturningRow(ctx, id) + m.queryLatencies.WithLabelValues("DeleteAPIKeyByIDReturningRow").Observe(time.Since(start).Seconds()) + m.queryCounts.WithLabelValues(httpmw.ExtractHTTPRoute(ctx), httpmw.ExtractHTTPMethod(ctx), "DeleteAPIKeyByIDReturningRow").Inc() return r0, r1 } @@ -649,11 +649,11 @@ func (m queryMetricsStore) DeleteOAuth2ProviderAppCodeByID(ctx context.Context, return r0 } -func (m queryMetricsStore) DeleteOAuth2ProviderAppCodeByIDReturningID(ctx context.Context, id uuid.UUID) (uuid.UUID, error) { +func (m queryMetricsStore) DeleteOAuth2ProviderAppCodeByIDReturningRow(ctx context.Context, id uuid.UUID) (database.OAuth2ProviderAppCode, error) { start := time.Now() - r0, r1 := m.s.DeleteOAuth2ProviderAppCodeByIDReturningID(ctx, id) - m.queryLatencies.WithLabelValues("DeleteOAuth2ProviderAppCodeByIDReturningID").Observe(time.Since(start).Seconds()) - m.queryCounts.WithLabelValues(httpmw.ExtractHTTPRoute(ctx), httpmw.ExtractHTTPMethod(ctx), "DeleteOAuth2ProviderAppCodeByIDReturningID").Inc() + r0, r1 := m.s.DeleteOAuth2ProviderAppCodeByIDReturningRow(ctx, id) + m.queryLatencies.WithLabelValues("DeleteOAuth2ProviderAppCodeByIDReturningRow").Observe(time.Since(start).Seconds()) + m.queryCounts.WithLabelValues(httpmw.ExtractHTTPRoute(ctx), httpmw.ExtractHTTPMethod(ctx), "DeleteOAuth2ProviderAppCodeByIDReturningRow").Inc() return r0, r1 } diff --git a/coderd/database/dbmock/dbmock.go b/coderd/database/dbmock/dbmock.go index a02c59ccc8162..0fcaff47a70ff 100644 --- a/coderd/database/dbmock/dbmock.go +++ b/coderd/database/dbmock/dbmock.go @@ -675,19 +675,19 @@ func (mr *MockStoreMockRecorder) DeleteAPIKeyByID(ctx, id any) *gomock.Call { return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteAPIKeyByID", reflect.TypeOf((*MockStore)(nil).DeleteAPIKeyByID), ctx, id) } -// DeleteAPIKeyByIDReturningID mocks base method. -func (m *MockStore) DeleteAPIKeyByIDReturningID(ctx context.Context, id string) (string, error) { +// DeleteAPIKeyByIDReturningRow mocks base method. +func (m *MockStore) DeleteAPIKeyByIDReturningRow(ctx context.Context, id string) (database.APIKey, error) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "DeleteAPIKeyByIDReturningID", ctx, id) - ret0, _ := ret[0].(string) + ret := m.ctrl.Call(m, "DeleteAPIKeyByIDReturningRow", ctx, id) + ret0, _ := ret[0].(database.APIKey) ret1, _ := ret[1].(error) return ret0, ret1 } -// DeleteAPIKeyByIDReturningID indicates an expected call of DeleteAPIKeyByIDReturningID. -func (mr *MockStoreMockRecorder) DeleteAPIKeyByIDReturningID(ctx, id any) *gomock.Call { +// DeleteAPIKeyByIDReturningRow indicates an expected call of DeleteAPIKeyByIDReturningRow. +func (mr *MockStoreMockRecorder) DeleteAPIKeyByIDReturningRow(ctx, id any) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteAPIKeyByIDReturningID", reflect.TypeOf((*MockStore)(nil).DeleteAPIKeyByIDReturningID), ctx, id) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteAPIKeyByIDReturningRow", reflect.TypeOf((*MockStore)(nil).DeleteAPIKeyByIDReturningRow), ctx, id) } // DeleteAPIKeysByUserID mocks base method. @@ -1077,19 +1077,19 @@ func (mr *MockStoreMockRecorder) DeleteOAuth2ProviderAppCodeByID(ctx, id any) *g return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteOAuth2ProviderAppCodeByID", reflect.TypeOf((*MockStore)(nil).DeleteOAuth2ProviderAppCodeByID), ctx, id) } -// DeleteOAuth2ProviderAppCodeByIDReturningID mocks base method. -func (m *MockStore) DeleteOAuth2ProviderAppCodeByIDReturningID(ctx context.Context, id uuid.UUID) (uuid.UUID, error) { +// DeleteOAuth2ProviderAppCodeByIDReturningRow mocks base method. +func (m *MockStore) DeleteOAuth2ProviderAppCodeByIDReturningRow(ctx context.Context, id uuid.UUID) (database.OAuth2ProviderAppCode, error) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "DeleteOAuth2ProviderAppCodeByIDReturningID", ctx, id) - ret0, _ := ret[0].(uuid.UUID) + ret := m.ctrl.Call(m, "DeleteOAuth2ProviderAppCodeByIDReturningRow", ctx, id) + ret0, _ := ret[0].(database.OAuth2ProviderAppCode) ret1, _ := ret[1].(error) return ret0, ret1 } -// DeleteOAuth2ProviderAppCodeByIDReturningID indicates an expected call of DeleteOAuth2ProviderAppCodeByIDReturningID. -func (mr *MockStoreMockRecorder) DeleteOAuth2ProviderAppCodeByIDReturningID(ctx, id any) *gomock.Call { +// DeleteOAuth2ProviderAppCodeByIDReturningRow indicates an expected call of DeleteOAuth2ProviderAppCodeByIDReturningRow. +func (mr *MockStoreMockRecorder) DeleteOAuth2ProviderAppCodeByIDReturningRow(ctx, id any) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteOAuth2ProviderAppCodeByIDReturningID", reflect.TypeOf((*MockStore)(nil).DeleteOAuth2ProviderAppCodeByIDReturningID), ctx, id) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteOAuth2ProviderAppCodeByIDReturningRow", reflect.TypeOf((*MockStore)(nil).DeleteOAuth2ProviderAppCodeByIDReturningRow), ctx, id) } // DeleteOAuth2ProviderAppCodesByAppAndUserID mocks base method. diff --git a/coderd/database/migrations/000567_oauth2_scope_enforcement.down.sql b/coderd/database/migrations/000567_oauth2_scope_columns.down.sql similarity index 100% rename from coderd/database/migrations/000567_oauth2_scope_enforcement.down.sql rename to coderd/database/migrations/000567_oauth2_scope_columns.down.sql diff --git a/coderd/database/migrations/000567_oauth2_scope_enforcement.up.sql b/coderd/database/migrations/000567_oauth2_scope_columns.up.sql similarity index 100% rename from coderd/database/migrations/000567_oauth2_scope_enforcement.up.sql rename to coderd/database/migrations/000567_oauth2_scope_columns.up.sql diff --git a/coderd/database/querier.go b/coderd/database/querier.go index 3d25f691ce480..d337ffa492b50 100644 --- a/coderd/database/querier.go +++ b/coderd/database/querier.go @@ -118,8 +118,10 @@ type sqlcQuerier interface { DeleteAIProviderKey(ctx context.Context, id uuid.UUID) error DeleteAPIKeyByID(ctx context.Context, id string) error // Returns sql.ErrNoRows when the key is already gone, which lets a caller - // enforce single use of a refresh token by racing this delete. - DeleteAPIKeyByIDReturningID(ctx context.Context, id string) (string, error) + // enforce single use of a refresh token by racing this delete. Returns the + // whole row so a caller reads the deleted key's state from the same atomic + // delete rather than trusting an earlier read. + DeleteAPIKeyByIDReturningRow(ctx context.Context, id string) (APIKey, error) DeleteAPIKeysByUserID(ctx context.Context, userID uuid.UUID) error // Deletes all heartbeat rows for the chat. Used during ownership // transitions that abandon a lease. @@ -167,9 +169,11 @@ type sqlcQuerier interface { DeleteOAuth2ProviderAppByClientID(ctx context.Context, id uuid.UUID) error DeleteOAuth2ProviderAppByID(ctx context.Context, id uuid.UUID) error DeleteOAuth2ProviderAppCodeByID(ctx context.Context, id uuid.UUID) error - // Returns sql.ErrNoRows when the code was already redeemed, which lets a - // caller enforce single use by racing this delete instead of reading first. - DeleteOAuth2ProviderAppCodeByIDReturningID(ctx context.Context, id uuid.UUID) (uuid.UUID, error) + // Returns sql.ErrNoRows when the code is already gone, which lets a caller + // enforce single use by racing this delete instead of reading first. Returns + // the whole row so a caller reads the redeemed code's negotiated scope from + // the same atomic delete rather than trusting an earlier read. + DeleteOAuth2ProviderAppCodeByIDReturningRow(ctx context.Context, id uuid.UUID) (OAuth2ProviderAppCode, error) DeleteOAuth2ProviderAppCodesByAppAndUserID(ctx context.Context, arg DeleteOAuth2ProviderAppCodesByAppAndUserIDParams) error DeleteOAuth2ProviderAppSecretByID(ctx context.Context, id uuid.UUID) error // Filters directly on app_id rather than joining through app_secret_id, diff --git a/coderd/database/querier_test.go b/coderd/database/querier_test.go index 4ec0a94f36907..c7bd428a8b428 100644 --- a/coderd/database/querier_test.go +++ b/coderd/database/querier_test.go @@ -18881,3 +18881,53 @@ func TestOAuth2ProviderScopeNotEmpty(t *testing.T) { "empty scope must be rejected, got %v", err) }) } + +func TestSingleUseDeleteByIDReturningRow(t *testing.T) { + t.Parallel() + if testing.Short() { + t.SkipNow() + } + + // These deletes are the arbiter of single use: the first caller gets the + // row, and every later caller gets sql.ErrNoRows because the row is gone. + // Converting either query back to :exec, or adding a soft delete, would + // break that guarantee silently. + t.Run("OAuth2ProviderAppCode", func(t *testing.T) { + t.Parallel() + db, _ := dbtestutil.NewDB(t) + ctx := testutil.Context(t, testutil.WaitLong) + + user := dbgen.User(t, db, database.User{}) + app := dbgen.OAuth2ProviderApp(t, db, database.OAuth2ProviderApp{}) + code := dbgen.OAuth2ProviderAppCode(t, db, database.OAuth2ProviderAppCode{ + AppID: app.ID, + UserID: user.ID, + }) + + // RETURNING * hands back the whole row, so a caller reads the + // redeemed code's negotiated scope from the delete itself rather + // than trusting an earlier read. + deleted, err := db.DeleteOAuth2ProviderAppCodeByIDReturningRow(ctx, code.ID) + require.NoError(t, err) + require.Equal(t, code, deleted) + + _, err = db.DeleteOAuth2ProviderAppCodeByIDReturningRow(ctx, code.ID) + require.ErrorIs(t, err, sql.ErrNoRows) + }) + + t.Run("APIKey", func(t *testing.T) { + t.Parallel() + db, _ := dbtestutil.NewDB(t) + ctx := testutil.Context(t, testutil.WaitLong) + + user := dbgen.User(t, db, database.User{}) + key, _ := dbgen.APIKey(t, db, database.APIKey{UserID: user.ID}) + + deleted, err := db.DeleteAPIKeyByIDReturningRow(ctx, key.ID) + require.NoError(t, err) + require.Equal(t, key, deleted) + + _, err = db.DeleteAPIKeyByIDReturningRow(ctx, key.ID) + require.ErrorIs(t, err, sql.ErrNoRows) + }) +} diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index fbb8ebd27acc9..21cc3664196c5 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -3656,21 +3656,37 @@ func (q *sqlQuerier) DeleteAPIKeyByID(ctx context.Context, id string) error { return err } -const deleteAPIKeyByIDReturningID = `-- name: DeleteAPIKeyByIDReturningID :one +const deleteAPIKeyByIDReturningRow = `-- name: DeleteAPIKeyByIDReturningRow :one DELETE FROM api_keys WHERE id = $1 -RETURNING id +RETURNING id, hashed_secret, user_id, last_used, expires_at, created_at, updated_at, login_type, lifetime_seconds, ip_address, token_name, scopes, allow_list ` // Returns sql.ErrNoRows when the key is already gone, which lets a caller -// enforce single use of a refresh token by racing this delete. -func (q *sqlQuerier) DeleteAPIKeyByIDReturningID(ctx context.Context, id string) (string, error) { - row := q.db.QueryRowContext(ctx, deleteAPIKeyByIDReturningID, id) - var id_2 string - err := row.Scan(&id_2) - return id_2, err +// enforce single use of a refresh token by racing this delete. Returns the +// whole row so a caller reads the deleted key's state from the same atomic +// delete rather than trusting an earlier read. +func (q *sqlQuerier) DeleteAPIKeyByIDReturningRow(ctx context.Context, id string) (APIKey, error) { + row := q.db.QueryRowContext(ctx, deleteAPIKeyByIDReturningRow, id) + var i APIKey + err := row.Scan( + &i.ID, + &i.HashedSecret, + &i.UserID, + &i.LastUsed, + &i.ExpiresAt, + &i.CreatedAt, + &i.UpdatedAt, + &i.LoginType, + &i.LifetimeSeconds, + &i.IPAddress, + &i.TokenName, + &i.Scopes, + &i.AllowList, + ) + return i, err } const deleteAPIKeysByUserID = `-- name: DeleteAPIKeysByUserID :exec @@ -18879,17 +18895,33 @@ func (q *sqlQuerier) DeleteOAuth2ProviderAppCodeByID(ctx context.Context, id uui return err } -const deleteOAuth2ProviderAppCodeByIDReturningID = `-- name: DeleteOAuth2ProviderAppCodeByIDReturningID :one -DELETE FROM oauth2_provider_app_codes WHERE id = $1 RETURNING id +const deleteOAuth2ProviderAppCodeByIDReturningRow = `-- name: DeleteOAuth2ProviderAppCodeByIDReturningRow :one +DELETE FROM oauth2_provider_app_codes WHERE id = $1 RETURNING id, created_at, expires_at, secret_prefix, hashed_secret, user_id, app_id, resource_uri, code_challenge, code_challenge_method, state_hash, redirect_uri, scope ` -// Returns sql.ErrNoRows when the code was already redeemed, which lets a -// caller enforce single use by racing this delete instead of reading first. -func (q *sqlQuerier) DeleteOAuth2ProviderAppCodeByIDReturningID(ctx context.Context, id uuid.UUID) (uuid.UUID, error) { - row := q.db.QueryRowContext(ctx, deleteOAuth2ProviderAppCodeByIDReturningID, id) - var id_2 uuid.UUID - err := row.Scan(&id_2) - return id_2, err +// Returns sql.ErrNoRows when the code is already gone, which lets a caller +// enforce single use by racing this delete instead of reading first. Returns +// the whole row so a caller reads the redeemed code's negotiated scope from +// the same atomic delete rather than trusting an earlier read. +func (q *sqlQuerier) DeleteOAuth2ProviderAppCodeByIDReturningRow(ctx context.Context, id uuid.UUID) (OAuth2ProviderAppCode, error) { + row := q.db.QueryRowContext(ctx, deleteOAuth2ProviderAppCodeByIDReturningRow, id) + var i OAuth2ProviderAppCode + err := row.Scan( + &i.ID, + &i.CreatedAt, + &i.ExpiresAt, + &i.SecretPrefix, + &i.HashedSecret, + &i.UserID, + &i.AppID, + &i.ResourceUri, + &i.CodeChallenge, + &i.CodeChallengeMethod, + &i.StateHash, + &i.RedirectUri, + &i.Scope, + ) + return i, err } const deleteOAuth2ProviderAppCodesByAppAndUserID = `-- name: DeleteOAuth2ProviderAppCodesByAppAndUserID :exec diff --git a/coderd/database/queries/apikeys.sql b/coderd/database/queries/apikeys.sql index 6c21f60cb9391..32539948437f1 100644 --- a/coderd/database/queries/apikeys.sql +++ b/coderd/database/queries/apikeys.sql @@ -92,14 +92,16 @@ DELETE FROM WHERE id = $1; --- name: DeleteAPIKeyByIDReturningID :one +-- name: DeleteAPIKeyByIDReturningRow :one -- Returns sql.ErrNoRows when the key is already gone, which lets a caller --- enforce single use of a refresh token by racing this delete. +-- enforce single use of a refresh token by racing this delete. Returns the +-- whole row so a caller reads the deleted key's state from the same atomic +-- delete rather than trusting an earlier read. DELETE FROM api_keys WHERE id = $1 -RETURNING id; +RETURNING *; -- name: DeleteApplicationConnectAPIKeysByUserID :exec DELETE FROM diff --git a/coderd/database/queries/oauth2.sql b/coderd/database/queries/oauth2.sql index 238d890cd1b02..e5d5c932d1629 100644 --- a/coderd/database/queries/oauth2.sql +++ b/coderd/database/queries/oauth2.sql @@ -158,10 +158,12 @@ INSERT INTO oauth2_provider_app_codes ( -- name: DeleteOAuth2ProviderAppCodeByID :exec DELETE FROM oauth2_provider_app_codes WHERE id = $1; --- name: DeleteOAuth2ProviderAppCodeByIDReturningID :one --- Returns sql.ErrNoRows when the code was already redeemed, which lets a --- caller enforce single use by racing this delete instead of reading first. -DELETE FROM oauth2_provider_app_codes WHERE id = $1 RETURNING id; +-- name: DeleteOAuth2ProviderAppCodeByIDReturningRow :one +-- Returns sql.ErrNoRows when the code is already gone, which lets a caller +-- enforce single use by racing this delete instead of reading first. Returns +-- the whole row so a caller reads the redeemed code's negotiated scope from +-- the same atomic delete rather than trusting an earlier read. +DELETE FROM oauth2_provider_app_codes WHERE id = $1 RETURNING *; -- name: DeleteOAuth2ProviderAppCodesByAppAndUserID :exec DELETE FROM oauth2_provider_app_codes WHERE app_id = $1 AND user_id = $2; From e3ca40d36e7d5fcd2347096fcb65095646b469b4 Mon Sep 17 00:00:00 2001 From: Bobby Ho Date: Tue, 11 Aug 2026 09:40:36 -0700 Subject: [PATCH 4/7] fix(coderd/database/migrations): renumber scope columns migration to 000569 origin/main merged 000567_chat_file_purge_indexes and 000568_service_account_notifications after this branch's point. CI validates the PR merge, where two files numbered 000567 coexisted and the migrate iofs driver panicked with "duplicate migration file", taking down gen, lint, sqlc-vet and every test-go-pg job. Git reports the merge as MERGEABLE because the two are different filenames; the collision is on the version number, which git cannot see. Renumbered with ./coderd/database/migrations/fix_migration_numbers.sh. Refs PLAT-478 Co-Authored-By: Claude Opus 5 (1M context) --- ...cope_columns.down.sql => 000569_oauth2_scope_columns.down.sql} | 0 ...h2_scope_columns.up.sql => 000569_oauth2_scope_columns.up.sql} | 0 2 files changed, 0 insertions(+), 0 deletions(-) rename coderd/database/migrations/{000567_oauth2_scope_columns.down.sql => 000569_oauth2_scope_columns.down.sql} (100%) rename coderd/database/migrations/{000567_oauth2_scope_columns.up.sql => 000569_oauth2_scope_columns.up.sql} (100%) diff --git a/coderd/database/migrations/000567_oauth2_scope_columns.down.sql b/coderd/database/migrations/000569_oauth2_scope_columns.down.sql similarity index 100% rename from coderd/database/migrations/000567_oauth2_scope_columns.down.sql rename to coderd/database/migrations/000569_oauth2_scope_columns.down.sql diff --git a/coderd/database/migrations/000567_oauth2_scope_columns.up.sql b/coderd/database/migrations/000569_oauth2_scope_columns.up.sql similarity index 100% rename from coderd/database/migrations/000567_oauth2_scope_columns.up.sql rename to coderd/database/migrations/000569_oauth2_scope_columns.up.sql From 3375487930b77f330ca96163feedb4ee38fb317b Mon Sep 17 00:00:00 2001 From: Bobby Ho Date: Tue, 11 Aug 2026 11:18:20 -0700 Subject: [PATCH 5/7] fix(coderd): set scope on oauth2 test inserts Two test sites built InsertOAuth2ProviderAppCodeParams and InsertOAuth2ProviderAppTokenParams without Scope, so after the columns became NOT NULL with CHECK (scope <> '') they inserted an empty string and tripped the constraint. Broke TestOAuth2ProviderTokenExchange/ExpiredCode and every TestOAuth2ProviderTokenRefresh subtest on the Linux postgres jobs. exhaustruct is disabled for _test.go (.golangci.yaml:222), so nothing forces the field in tests and the constraint is the only backstop. Audited every remaining InsertOAuth2ProviderApp{Code,Token}Params literal in the tree; these two were the only omissions, and no raw SQL inserts bypass sqlc. Refs PLAT-478 Co-Authored-By: Claude Opus 5 (1M context) --- coderd/oauth2_test.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/coderd/oauth2_test.go b/coderd/oauth2_test.go index 3a8d5917fdae5..a7e12bcf89fab 100644 --- a/coderd/oauth2_test.go +++ b/coderd/oauth2_test.go @@ -442,6 +442,7 @@ func TestOAuth2ProviderTokenExchange(t *testing.T) { HashedSecret: []byte(hashedCode), AppID: apps.Default.ID, UserID: user.ID, + Scope: database.OAuth2ScopeUnrestricted, }) return err }, @@ -732,6 +733,7 @@ func TestOAuth2ProviderTokenRefresh(t *testing.T) { AppSecretID: uuid.NullUUID{UUID: secret.ID, Valid: true}, APIKeyID: newKey.ID, UserID: user.ID, + Scope: database.OAuth2ScopeUnrestricted, }) require.NoError(t, err) From bfc9fd0e8d3ba3e12002ceaf4af7e543cbc73d3d Mon Sep 17 00:00:00 2001 From: Bobby Ho Date: Wed, 12 Aug 2026 11:26:34 -0700 Subject: [PATCH 6/7] refactor(coderd): inline oauth2 unrestricted scope constant OAuth2ScopeUnrestricted was an alias for ApiKeyScopeCoderAll, so the unrestricted grant had two spellings while every other call site (coderd/apikey.go, coderd/apikey/apikey.go, coderd/users.go) names ApiKeyScopeCoderAll directly. Use that name at the oauth2 code and token sites too, with an explicit string conversion marking where the api_key_scope enum crosses into the text columns. The alias carried no enforcement. The property that a grant's authority is always stated, and that a caller omitting the column fails rather than receiving full access, comes from NOT NULL plus CHECK (scope <> '') in migration 000569 and is unaffected. --- coderd/database/constants.go | 10 ---------- coderd/database/dbauthz/dbauthz_test.go | 4 ++-- coderd/database/dbgen/dbgen.go | 4 ++-- coderd/oauth2_test.go | 4 ++-- coderd/oauth2provider/authorize.go | 2 +- 5 files changed, 7 insertions(+), 17 deletions(-) diff --git a/coderd/database/constants.go b/coderd/database/constants.go index bb11f8fa531fa..34ad1005ee4c0 100644 --- a/coderd/database/constants.go +++ b/coderd/database/constants.go @@ -10,13 +10,3 @@ import ( // for use as a uuid.UUID. Both must agree; tests pin the value to the // codersdk constant so the two cannot drift. var PrebuildsSystemUserID = uuid.MustParse(codersdk.PrebuildsSystemUserID) - -// OAuth2ScopeUnrestricted is the oauth2_provider_app_codes.scope and -// oauth2_provider_app_tokens.scope value recording a grant that carries no -// restriction. Both columns hold space-separated values from the -// api_key_scope vocabulary, so an unrestricted grant is spelled the same way -// api_keys.scopes spells it. The columns are NOT NULL: writing this constant -// is how a caller states "unrestricted" on purpose, which is what -// distinguishes a deliberate grant from a scope that was never threaded -// through. -const OAuth2ScopeUnrestricted = string(ApiKeyScopeCoderAll) diff --git a/coderd/database/dbauthz/dbauthz_test.go b/coderd/database/dbauthz/dbauthz_test.go index 7dbd10a0d946d..6a18686b41219 100644 --- a/coderd/database/dbauthz/dbauthz_test.go +++ b/coderd/database/dbauthz/dbauthz_test.go @@ -6023,7 +6023,7 @@ func (s *MethodTestSuite) TestOAuth2ProviderAppCodes() { check.Args(database.InsertOAuth2ProviderAppCodeParams{ AppID: app.ID, UserID: user.ID, - Scope: database.OAuth2ScopeUnrestricted, + Scope: string(database.ApiKeyScopeCoderAll), }).Asserts(rbac.ResourceOauth2AppCodeToken.WithOwner(user.ID.String()), policy.ActionCreate) })) s.Run("DeleteOAuth2ProviderAppCodeByID", s.Subtest(func(db database.Store, check *expects) { @@ -6077,7 +6077,7 @@ func (s *MethodTestSuite) TestOAuth2ProviderAppTokens() { AppSecretID: uuid.NullUUID{UUID: secret.ID, Valid: true}, APIKeyID: key.ID, UserID: user.ID, - Scope: database.OAuth2ScopeUnrestricted, + Scope: string(database.ApiKeyScopeCoderAll), }).Asserts(rbac.ResourceOauth2AppCodeToken.WithOwner(user.ID.String()), policy.ActionCreate) })) s.Run("GetOAuth2ProviderAppTokenByPrefix", s.Subtest(func(db database.Store, check *expects) { diff --git a/coderd/database/dbgen/dbgen.go b/coderd/database/dbgen/dbgen.go index 2a52436aa6031..ff207099917ca 100644 --- a/coderd/database/dbgen/dbgen.go +++ b/coderd/database/dbgen/dbgen.go @@ -1784,7 +1784,7 @@ func OAuth2ProviderAppCode(t testing.TB, db database.Store, seed database.OAuth2 CodeChallengeMethod: seed.CodeChallengeMethod, StateHash: seed.StateHash, RedirectUri: seed.RedirectUri, - Scope: takeFirst(seed.Scope, database.OAuth2ScopeUnrestricted), + Scope: takeFirst(seed.Scope, string(database.ApiKeyScopeCoderAll)), }) require.NoError(t, err, "insert oauth2 app code") return code @@ -1806,7 +1806,7 @@ func OAuth2ProviderAppToken(t testing.TB, db database.Store, seed database.OAuth APIKeyID: takeFirst(seed.APIKeyID, uuid.New().String()), UserID: takeFirst(seed.UserID, uuid.New()), Audience: seed.Audience, - Scope: takeFirst(seed.Scope, database.OAuth2ScopeUnrestricted), + Scope: takeFirst(seed.Scope, string(database.ApiKeyScopeCoderAll)), }) require.NoError(t, err, "insert oauth2 app token") return token diff --git a/coderd/oauth2_test.go b/coderd/oauth2_test.go index a7e12bcf89fab..d35c63e460092 100644 --- a/coderd/oauth2_test.go +++ b/coderd/oauth2_test.go @@ -442,7 +442,7 @@ func TestOAuth2ProviderTokenExchange(t *testing.T) { HashedSecret: []byte(hashedCode), AppID: apps.Default.ID, UserID: user.ID, - Scope: database.OAuth2ScopeUnrestricted, + Scope: string(database.ApiKeyScopeCoderAll), }) return err }, @@ -733,7 +733,7 @@ func TestOAuth2ProviderTokenRefresh(t *testing.T) { AppSecretID: uuid.NullUUID{UUID: secret.ID, Valid: true}, APIKeyID: newKey.ID, UserID: user.ID, - Scope: database.OAuth2ScopeUnrestricted, + Scope: string(database.ApiKeyScopeCoderAll), }) require.NoError(t, err) diff --git a/coderd/oauth2provider/authorize.go b/coderd/oauth2provider/authorize.go index 7e03c1ab860dc..d7eb0f9138297 100644 --- a/coderd/oauth2provider/authorize.go +++ b/coderd/oauth2provider/authorize.go @@ -263,7 +263,7 @@ func ProcessAuthorize(db database.Store) http.HandlerFunc { // requested scope is validated against the app's allowlist, // persisting it here would store unvalidated client input, so // the code records an unrestricted grant. - Scope: database.OAuth2ScopeUnrestricted, + Scope: string(database.ApiKeyScopeCoderAll), }) if err != nil { return xerrors.Errorf("insert oauth2 authorization code: %w", err) From 5df995e85898f209372219f04d97456a38592cbd Mon Sep 17 00:00:00 2001 From: Bobby Ho Date: Wed, 12 Aug 2026 12:35:29 -0700 Subject: [PATCH 7/7] refactor(coderd/database): remove unused single-use delete queries DeleteAPIKeyByIDReturningRow and DeleteOAuth2ProviderAppCodeByIDReturningRow had no production caller, here or on the Phase 2 branch. Both exist for the redemption path that makes the code delete the single-use arbiter and reads the negotiated scope off the returned row, but that call-site swap is in neither phase, so the queries and the test pinning their contract were dead weight across five generated files plus two dbauthz authorization decisions no caller could exercise. The plain :exec deletes they were added alongside are untouched and remain what authorizationCodeGrant and the revoke paths call. PLAT-480 covers reintroducing the query and its contract test in the PR that switches authorizationCodeGrant over to it. Refs PLAT-478 --- coderd/database/dbauthz/dbauthz.go | 8 --- coderd/database/dbauthz/dbauthz_test.go | 15 ------ coderd/database/dbmetrics/querymetrics.go | 16 ------ coderd/database/dbmock/dbmock.go | 30 ----------- coderd/database/querier.go | 10 ---- coderd/database/querier_test.go | 50 ------------------ coderd/database/queries.sql.go | 62 ----------------------- coderd/database/queries/apikeys.sql | 11 ---- coderd/database/queries/oauth2.sql | 7 --- 9 files changed, 209 deletions(-) diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go index 9873e8df26dc3..ad1becaf35cc0 100644 --- a/coderd/database/dbauthz/dbauthz.go +++ b/coderd/database/dbauthz/dbauthz.go @@ -2060,10 +2060,6 @@ func (q *querier) DeleteAPIKeyByID(ctx context.Context, id string) error { return deleteQ(q.log, q.auth, q.db.GetAPIKeyByID, q.db.DeleteAPIKeyByID)(ctx, id) } -func (q *querier) DeleteAPIKeyByIDReturningRow(ctx context.Context, id string) (database.APIKey, error) { - return fetchAndQuery(q.log, q.auth, policy.ActionDelete, q.db.GetAPIKeyByID, q.db.DeleteAPIKeyByIDReturningRow)(ctx, id) -} - func (q *querier) DeleteAPIKeysByUserID(ctx context.Context, userID uuid.UUID) error { // TODO: This is not 100% correct because it omits apikey IDs. err := q.authorizeContext(ctx, policy.ActionDelete, @@ -2311,10 +2307,6 @@ func (q *querier) DeleteOAuth2ProviderAppCodeByID(ctx context.Context, id uuid.U return q.db.DeleteOAuth2ProviderAppCodeByID(ctx, id) } -func (q *querier) DeleteOAuth2ProviderAppCodeByIDReturningRow(ctx context.Context, id uuid.UUID) (database.OAuth2ProviderAppCode, error) { - return fetchAndQuery(q.log, q.auth, policy.ActionDelete, q.db.GetOAuth2ProviderAppCodeByID, q.db.DeleteOAuth2ProviderAppCodeByIDReturningRow)(ctx, id) -} - func (q *querier) DeleteOAuth2ProviderAppCodesByAppAndUserID(ctx context.Context, arg database.DeleteOAuth2ProviderAppCodesByAppAndUserIDParams) error { if err := q.authorizeContext(ctx, policy.ActionDelete, rbac.ResourceOauth2AppCodeToken.WithOwner(arg.UserID.String())); err != nil { diff --git a/coderd/database/dbauthz/dbauthz_test.go b/coderd/database/dbauthz/dbauthz_test.go index 5ef4716565ee7..6a4dc4c9a6c7a 100644 --- a/coderd/database/dbauthz/dbauthz_test.go +++ b/coderd/database/dbauthz/dbauthz_test.go @@ -360,12 +360,6 @@ 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("DeleteAPIKeyByIDReturningRow", 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() - dbm.EXPECT().DeleteAPIKeyByIDReturningRow(gomock.Any(), key.ID).Return(key, nil).AnyTimes() - check.Args(key.ID).Asserts(key, policy.ActionDelete).Returns(key) - })) 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), @@ -6031,15 +6025,6 @@ func (s *MethodTestSuite) TestOAuth2ProviderAppCodes() { }) check.Args(code.ID).Asserts(code, policy.ActionDelete) })) - s.Run("DeleteOAuth2ProviderAppCodeByIDReturningRow", s.Subtest(func(db database.Store, check *expects) { - user := dbgen.User(s.T(), db, database.User{}) - app := dbgen.OAuth2ProviderApp(s.T(), db, database.OAuth2ProviderApp{}) - code := dbgen.OAuth2ProviderAppCode(s.T(), db, database.OAuth2ProviderAppCode{ - AppID: app.ID, - UserID: user.ID, - }) - check.Args(code.ID).Asserts(code, policy.ActionDelete).Returns(code) - })) s.Run("DeleteOAuth2ProviderAppCodesByAppAndUserID", s.Subtest(func(db database.Store, check *expects) { dbtestutil.DisableForeignKeysAndTriggers(s.T(), db) user := dbgen.User(s.T(), db, database.User{}) diff --git a/coderd/database/dbmetrics/querymetrics.go b/coderd/database/dbmetrics/querymetrics.go index 4332b5e1e0963..795e777ee8320 100644 --- a/coderd/database/dbmetrics/querymetrics.go +++ b/coderd/database/dbmetrics/querymetrics.go @@ -417,14 +417,6 @@ func (m queryMetricsStore) DeleteAPIKeyByID(ctx context.Context, id string) erro return r0 } -func (m queryMetricsStore) DeleteAPIKeyByIDReturningRow(ctx context.Context, id string) (database.APIKey, error) { - start := time.Now() - r0, r1 := m.s.DeleteAPIKeyByIDReturningRow(ctx, id) - m.queryLatencies.WithLabelValues("DeleteAPIKeyByIDReturningRow").Observe(time.Since(start).Seconds()) - m.queryCounts.WithLabelValues(httpmw.ExtractHTTPRoute(ctx), httpmw.ExtractHTTPMethod(ctx), "DeleteAPIKeyByIDReturningRow").Inc() - return r0, r1 -} - func (m queryMetricsStore) DeleteAPIKeysByUserID(ctx context.Context, userID uuid.UUID) error { start := time.Now() r0 := m.s.DeleteAPIKeysByUserID(ctx, userID) @@ -641,14 +633,6 @@ func (m queryMetricsStore) DeleteOAuth2ProviderAppCodeByID(ctx context.Context, return r0 } -func (m queryMetricsStore) DeleteOAuth2ProviderAppCodeByIDReturningRow(ctx context.Context, id uuid.UUID) (database.OAuth2ProviderAppCode, error) { - start := time.Now() - r0, r1 := m.s.DeleteOAuth2ProviderAppCodeByIDReturningRow(ctx, id) - m.queryLatencies.WithLabelValues("DeleteOAuth2ProviderAppCodeByIDReturningRow").Observe(time.Since(start).Seconds()) - m.queryCounts.WithLabelValues(httpmw.ExtractHTTPRoute(ctx), httpmw.ExtractHTTPMethod(ctx), "DeleteOAuth2ProviderAppCodeByIDReturningRow").Inc() - return r0, r1 -} - func (m queryMetricsStore) DeleteOAuth2ProviderAppCodesByAppAndUserID(ctx context.Context, arg database.DeleteOAuth2ProviderAppCodesByAppAndUserIDParams) error { start := time.Now() r0 := m.s.DeleteOAuth2ProviderAppCodesByAppAndUserID(ctx, arg) diff --git a/coderd/database/dbmock/dbmock.go b/coderd/database/dbmock/dbmock.go index 673e5834b4fc0..418a4c205dd84 100644 --- a/coderd/database/dbmock/dbmock.go +++ b/coderd/database/dbmock/dbmock.go @@ -660,21 +660,6 @@ func (mr *MockStoreMockRecorder) DeleteAPIKeyByID(ctx, id any) *gomock.Call { return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteAPIKeyByID", reflect.TypeOf((*MockStore)(nil).DeleteAPIKeyByID), ctx, id) } -// DeleteAPIKeyByIDReturningRow mocks base method. -func (m *MockStore) DeleteAPIKeyByIDReturningRow(ctx context.Context, id string) (database.APIKey, error) { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "DeleteAPIKeyByIDReturningRow", ctx, id) - ret0, _ := ret[0].(database.APIKey) - ret1, _ := ret[1].(error) - return ret0, ret1 -} - -// DeleteAPIKeyByIDReturningRow indicates an expected call of DeleteAPIKeyByIDReturningRow. -func (mr *MockStoreMockRecorder) DeleteAPIKeyByIDReturningRow(ctx, id any) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteAPIKeyByIDReturningRow", reflect.TypeOf((*MockStore)(nil).DeleteAPIKeyByIDReturningRow), ctx, id) -} - // DeleteAPIKeysByUserID mocks base method. func (m *MockStore) DeleteAPIKeysByUserID(ctx context.Context, userID uuid.UUID) error { m.ctrl.T.Helper() @@ -1062,21 +1047,6 @@ func (mr *MockStoreMockRecorder) DeleteOAuth2ProviderAppCodeByID(ctx, id any) *g return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteOAuth2ProviderAppCodeByID", reflect.TypeOf((*MockStore)(nil).DeleteOAuth2ProviderAppCodeByID), ctx, id) } -// DeleteOAuth2ProviderAppCodeByIDReturningRow mocks base method. -func (m *MockStore) DeleteOAuth2ProviderAppCodeByIDReturningRow(ctx context.Context, id uuid.UUID) (database.OAuth2ProviderAppCode, error) { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "DeleteOAuth2ProviderAppCodeByIDReturningRow", ctx, id) - ret0, _ := ret[0].(database.OAuth2ProviderAppCode) - ret1, _ := ret[1].(error) - return ret0, ret1 -} - -// DeleteOAuth2ProviderAppCodeByIDReturningRow indicates an expected call of DeleteOAuth2ProviderAppCodeByIDReturningRow. -func (mr *MockStoreMockRecorder) DeleteOAuth2ProviderAppCodeByIDReturningRow(ctx, id any) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteOAuth2ProviderAppCodeByIDReturningRow", reflect.TypeOf((*MockStore)(nil).DeleteOAuth2ProviderAppCodeByIDReturningRow), ctx, id) -} - // DeleteOAuth2ProviderAppCodesByAppAndUserID mocks base method. func (m *MockStore) DeleteOAuth2ProviderAppCodesByAppAndUserID(ctx context.Context, arg database.DeleteOAuth2ProviderAppCodesByAppAndUserIDParams) error { m.ctrl.T.Helper() diff --git a/coderd/database/querier.go b/coderd/database/querier.go index 44e8aade3f518..bfb40a67a651c 100644 --- a/coderd/database/querier.go +++ b/coderd/database/querier.go @@ -114,11 +114,6 @@ type sqlcQuerier interface { DeleteAIProviderByID(ctx context.Context, id uuid.UUID) error DeleteAIProviderKey(ctx context.Context, id uuid.UUID) error DeleteAPIKeyByID(ctx context.Context, id string) error - // Returns sql.ErrNoRows when the key is already gone, which lets a caller - // enforce single use of a refresh token by racing this delete. Returns the - // whole row so a caller reads the deleted key's state from the same atomic - // delete rather than trusting an earlier read. - DeleteAPIKeyByIDReturningRow(ctx context.Context, id string) (APIKey, error) DeleteAPIKeysByUserID(ctx context.Context, userID uuid.UUID) error // Deletes all heartbeat rows for the chat. Used during ownership // transitions that abandon a lease. @@ -166,11 +161,6 @@ type sqlcQuerier interface { DeleteOAuth2ProviderAppByClientID(ctx context.Context, id uuid.UUID) error DeleteOAuth2ProviderAppByID(ctx context.Context, id uuid.UUID) error DeleteOAuth2ProviderAppCodeByID(ctx context.Context, id uuid.UUID) error - // Returns sql.ErrNoRows when the code is already gone, which lets a caller - // enforce single use by racing this delete instead of reading first. Returns - // the whole row so a caller reads the redeemed code's negotiated scope from - // the same atomic delete rather than trusting an earlier read. - DeleteOAuth2ProviderAppCodeByIDReturningRow(ctx context.Context, id uuid.UUID) (OAuth2ProviderAppCode, error) DeleteOAuth2ProviderAppCodesByAppAndUserID(ctx context.Context, arg DeleteOAuth2ProviderAppCodesByAppAndUserIDParams) error DeleteOAuth2ProviderAppSecretByID(ctx context.Context, id uuid.UUID) error // Filters directly on app_id rather than joining through app_secret_id, diff --git a/coderd/database/querier_test.go b/coderd/database/querier_test.go index 15c243c53804d..bbfb2b784fd1a 100644 --- a/coderd/database/querier_test.go +++ b/coderd/database/querier_test.go @@ -18945,53 +18945,3 @@ func TestOAuth2ProviderScopeNotEmpty(t *testing.T) { "empty scope must be rejected, got %v", err) }) } - -func TestSingleUseDeleteByIDReturningRow(t *testing.T) { - t.Parallel() - if testing.Short() { - t.SkipNow() - } - - // These deletes are the arbiter of single use: the first caller gets the - // row, and every later caller gets sql.ErrNoRows because the row is gone. - // Converting either query back to :exec, or adding a soft delete, would - // break that guarantee silently. - t.Run("OAuth2ProviderAppCode", func(t *testing.T) { - t.Parallel() - db, _ := dbtestutil.NewDB(t) - ctx := testutil.Context(t, testutil.WaitLong) - - user := dbgen.User(t, db, database.User{}) - app := dbgen.OAuth2ProviderApp(t, db, database.OAuth2ProviderApp{}) - code := dbgen.OAuth2ProviderAppCode(t, db, database.OAuth2ProviderAppCode{ - AppID: app.ID, - UserID: user.ID, - }) - - // RETURNING * hands back the whole row, so a caller reads the - // redeemed code's negotiated scope from the delete itself rather - // than trusting an earlier read. - deleted, err := db.DeleteOAuth2ProviderAppCodeByIDReturningRow(ctx, code.ID) - require.NoError(t, err) - require.Equal(t, code, deleted) - - _, err = db.DeleteOAuth2ProviderAppCodeByIDReturningRow(ctx, code.ID) - require.ErrorIs(t, err, sql.ErrNoRows) - }) - - t.Run("APIKey", func(t *testing.T) { - t.Parallel() - db, _ := dbtestutil.NewDB(t) - ctx := testutil.Context(t, testutil.WaitLong) - - user := dbgen.User(t, db, database.User{}) - key, _ := dbgen.APIKey(t, db, database.APIKey{UserID: user.ID}) - - deleted, err := db.DeleteAPIKeyByIDReturningRow(ctx, key.ID) - require.NoError(t, err) - require.Equal(t, key, deleted) - - _, err = db.DeleteAPIKeyByIDReturningRow(ctx, key.ID) - require.ErrorIs(t, err, sql.ErrNoRows) - }) -} diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index 8425d91670d13..b27fc9fb854ba 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -3656,39 +3656,6 @@ func (q *sqlQuerier) DeleteAPIKeyByID(ctx context.Context, id string) error { return err } -const deleteAPIKeyByIDReturningRow = `-- name: DeleteAPIKeyByIDReturningRow :one -DELETE FROM - api_keys -WHERE - id = $1 -RETURNING id, hashed_secret, user_id, last_used, expires_at, created_at, updated_at, login_type, lifetime_seconds, ip_address, token_name, scopes, allow_list -` - -// Returns sql.ErrNoRows when the key is already gone, which lets a caller -// enforce single use of a refresh token by racing this delete. Returns the -// whole row so a caller reads the deleted key's state from the same atomic -// delete rather than trusting an earlier read. -func (q *sqlQuerier) DeleteAPIKeyByIDReturningRow(ctx context.Context, id string) (APIKey, error) { - row := q.db.QueryRowContext(ctx, deleteAPIKeyByIDReturningRow, id) - var i APIKey - err := row.Scan( - &i.ID, - &i.HashedSecret, - &i.UserID, - &i.LastUsed, - &i.ExpiresAt, - &i.CreatedAt, - &i.UpdatedAt, - &i.LoginType, - &i.LifetimeSeconds, - &i.IPAddress, - &i.TokenName, - &i.Scopes, - &i.AllowList, - ) - return i, err -} - const deleteAPIKeysByUserID = `-- name: DeleteAPIKeysByUserID :exec DELETE FROM api_keys @@ -18901,35 +18868,6 @@ func (q *sqlQuerier) DeleteOAuth2ProviderAppCodeByID(ctx context.Context, id uui return err } -const deleteOAuth2ProviderAppCodeByIDReturningRow = `-- name: DeleteOAuth2ProviderAppCodeByIDReturningRow :one -DELETE FROM oauth2_provider_app_codes WHERE id = $1 RETURNING id, created_at, expires_at, secret_prefix, hashed_secret, user_id, app_id, resource_uri, code_challenge, code_challenge_method, state_hash, redirect_uri, scope -` - -// Returns sql.ErrNoRows when the code is already gone, which lets a caller -// enforce single use by racing this delete instead of reading first. Returns -// the whole row so a caller reads the redeemed code's negotiated scope from -// the same atomic delete rather than trusting an earlier read. -func (q *sqlQuerier) DeleteOAuth2ProviderAppCodeByIDReturningRow(ctx context.Context, id uuid.UUID) (OAuth2ProviderAppCode, error) { - row := q.db.QueryRowContext(ctx, deleteOAuth2ProviderAppCodeByIDReturningRow, id) - var i OAuth2ProviderAppCode - err := row.Scan( - &i.ID, - &i.CreatedAt, - &i.ExpiresAt, - &i.SecretPrefix, - &i.HashedSecret, - &i.UserID, - &i.AppID, - &i.ResourceUri, - &i.CodeChallenge, - &i.CodeChallengeMethod, - &i.StateHash, - &i.RedirectUri, - &i.Scope, - ) - return i, err -} - const deleteOAuth2ProviderAppCodesByAppAndUserID = `-- name: DeleteOAuth2ProviderAppCodesByAppAndUserID :exec DELETE FROM oauth2_provider_app_codes WHERE app_id = $1 AND user_id = $2 ` diff --git a/coderd/database/queries/apikeys.sql b/coderd/database/queries/apikeys.sql index 32539948437f1..90e7610cf06db 100644 --- a/coderd/database/queries/apikeys.sql +++ b/coderd/database/queries/apikeys.sql @@ -92,17 +92,6 @@ DELETE FROM WHERE id = $1; --- name: DeleteAPIKeyByIDReturningRow :one --- Returns sql.ErrNoRows when the key is already gone, which lets a caller --- enforce single use of a refresh token by racing this delete. Returns the --- whole row so a caller reads the deleted key's state from the same atomic --- delete rather than trusting an earlier read. -DELETE FROM - api_keys -WHERE - id = $1 -RETURNING *; - -- name: DeleteApplicationConnectAPIKeysByUserID :exec DELETE FROM api_keys diff --git a/coderd/database/queries/oauth2.sql b/coderd/database/queries/oauth2.sql index e5d5c932d1629..52a2031fbf47f 100644 --- a/coderd/database/queries/oauth2.sql +++ b/coderd/database/queries/oauth2.sql @@ -158,13 +158,6 @@ INSERT INTO oauth2_provider_app_codes ( -- name: DeleteOAuth2ProviderAppCodeByID :exec DELETE FROM oauth2_provider_app_codes WHERE id = $1; --- name: DeleteOAuth2ProviderAppCodeByIDReturningRow :one --- Returns sql.ErrNoRows when the code is already gone, which lets a caller --- enforce single use by racing this delete instead of reading first. Returns --- the whole row so a caller reads the redeemed code's negotiated scope from --- the same atomic delete rather than trusting an earlier read. -DELETE FROM oauth2_provider_app_codes WHERE id = $1 RETURNING *; - -- name: DeleteOAuth2ProviderAppCodesByAppAndUserID :exec DELETE FROM oauth2_provider_app_codes WHERE app_id = $1 AND user_id = $2;