refactor(coderd/database): return the deleted row from single-use del…

…etes 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) <noreply@anthropic.com>
BobbyHo · Aug 13, 2026 · Aug 10, 2026 · Aug 11, 2026 · Aug 11, 2026 · Aug 11, 2026
commit 6f5e05790cf735ab76c5c887db612140072e9739
diff --git 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
@@ -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
diff --git a/coderd/database/dbmock/dbmock.go b/coderd/database/dbmock/dbmock.go
diff --git a/.../000567_oauth2_scope_enforcement.down.sql → ...ions/000567_oauth2_scope_columns.down.sql b/.../000567_oauth2_scope_enforcement.down.sql → ...ions/000567_oauth2_scope_columns.down.sql
diff --git a/...ns/000567_oauth2_scope_enforcement.up.sql → ...ations/000567_oauth2_scope_columns.up.sql b/...ns/000567_oauth2_scope_enforcement.up.sql → ...ations/000567_oauth2_scope_columns.up.sql
diff --git a/coderd/database/querier.go b/coderd/database/querier.go
diff --git 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
diff --git 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
@@ -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;