fix: wipe user secrets when user is soft-deleted (#24985)

Extend the delete_deleted_user_resources() trigger so that secrets
belonging to a soft-deleted user are removed in the same transaction as
the existing api_keys and user_links cleanup.

user_secrets.user_id has ON DELETE CASCADE, but Coder soft-deletes users
by flipping users.deleted rather than removing the row, so the foreign key
cascade never fires and secrets would otherwise survive deletion.

Assisted by Coder Agents.

coderd/database/dump.sql

@@ -762,6 +762,12 @@ BEGIN
 		-- email if the account is undeleted. Although that is not a guarantee.
 		DELETE FROM user_links
 		WHERE user_id = OLD.id;
+
+		-- Remove their user_secrets.
+		-- user_secrets.user_id has ON DELETE CASCADE, but soft-delete
+		-- does not remove the users row so the FK cascade never fires.
+		DELETE FROM user_secrets
+		WHERE user_id = OLD.id;
 	END IF;
 	RETURN NEW;
 END;
@@ -888,6 +894,21 @@ BEGIN
 END;
 $$;
 
+CREATE FUNCTION insert_user_secret_fail_if_user_deleted() RETURNS trigger
+    LANGUAGE plpgsql
+    AS $$
+
+DECLARE
+BEGIN
+	IF (NEW.user_id IS NOT NULL) THEN
+		IF (SELECT deleted FROM users WHERE id = NEW.user_id LIMIT 1) THEN
+			RAISE EXCEPTION 'Cannot create user_secret for deleted user';
+		END IF;
+	END IF;
+	RETURN NEW;
+END;
+$$;
+
 CREATE FUNCTION nullify_next_start_at_on_workspace_autostart_modification() RETURNS trigger
     LANGUAGE plpgsql
     AS $$
@@ -4105,6 +4126,8 @@ CREATE TRIGGER trigger_update_users AFTER INSERT OR UPDATE ON users FOR EACH ROW
 
 CREATE TRIGGER trigger_upsert_user_links BEFORE INSERT OR UPDATE ON user_links FOR EACH ROW EXECUTE FUNCTION insert_user_links_fail_if_user_deleted();
 
+CREATE TRIGGER trigger_upsert_user_secrets BEFORE INSERT OR UPDATE ON user_secrets FOR EACH ROW EXECUTE FUNCTION insert_user_secret_fail_if_user_deleted();
+
 CREATE TRIGGER update_notification_message_dedupe_hash BEFORE INSERT OR UPDATE ON notification_messages FOR EACH ROW EXECUTE FUNCTION compute_notification_message_dedupe_hash();
 
 CREATE TRIGGER user_status_change_trigger AFTER INSERT OR UPDATE ON users FOR EACH ROW EXECUTE FUNCTION record_user_status_change();

coderd/database/migrations/000490_trigger_delete_user_secrets.down.sql

@@ -0,0 +1,27 @@
+-- Drop the BEFORE INSERT/UPDATE guard added by 000489.
+DROP TRIGGER IF EXISTS trigger_upsert_user_secrets ON user_secrets;
+DROP FUNCTION IF EXISTS insert_user_secret_fail_if_user_deleted;
+
+-- Restore the previous body of delete_deleted_user_resources() from
+-- 000194_trigger_delete_user_user_link.up.sql, dropping the
+-- user_secrets cleanup added by 000489.
+CREATE OR REPLACE FUNCTION delete_deleted_user_resources() RETURNS trigger
+	LANGUAGE plpgsql
+AS $$
+DECLARE
+BEGIN
+	IF (NEW.deleted) THEN
+		-- Remove their api_keys
+		DELETE FROM api_keys
+		WHERE user_id = OLD.id;
+
+		-- Remove their user_links
+		-- Their login_type is preserved in the users table.
+		-- Matching this user back to the link can still be done by their
+		-- email if the account is undeleted. Although that is not a guarantee.
+		DELETE FROM user_links
+		WHERE user_id = OLD.id;
+	END IF;
+	RETURN NEW;
+END;
+$$;

coderd/database/migrations/000490_trigger_delete_user_secrets.up.sql

@@ -0,0 +1,64 @@
+-- Extend the soft-delete cleanup trigger to also wipe user_secrets.
+-- user_secrets.user_id has ON DELETE CASCADE, but Coder soft-deletes
+-- users by flipping users.deleted instead of removing the row, so the
+-- FK cascade never fires and secrets would otherwise survive deletion.
+--
+-- Backfill any rows that belonged to already-soft-deleted users before
+-- replacing the function.
+DELETE FROM
+	user_secrets
+WHERE
+	user_id
+		IN (
+		SELECT id FROM users WHERE deleted
+	);
+
+CREATE OR REPLACE FUNCTION delete_deleted_user_resources() RETURNS trigger
+	LANGUAGE plpgsql
+AS $$
+DECLARE
+BEGIN
+	IF (NEW.deleted) THEN
+		-- Remove their api_keys
+		DELETE FROM api_keys
+		WHERE user_id = OLD.id;
+
+		-- Remove their user_links
+		-- Their login_type is preserved in the users table.
+		-- Matching this user back to the link can still be done by their
+		-- email if the account is undeleted. Although that is not a guarantee.
+		DELETE FROM user_links
+		WHERE user_id = OLD.id;
+
+		-- Remove their user_secrets.
+		-- user_secrets.user_id has ON DELETE CASCADE, but soft-delete
+		-- does not remove the users row so the FK cascade never fires.
+		DELETE FROM user_secrets
+		WHERE user_id = OLD.id;
+	END IF;
+	RETURN NEW;
+END;
+$$;
+
+-- Prevent adding new user_secrets for soft-deleted users.
+-- Closes the window between an in-flight CreateUserSecret request
+-- and the soft-delete UPDATE committing.
+CREATE FUNCTION insert_user_secret_fail_if_user_deleted() RETURNS trigger
+	LANGUAGE plpgsql
+AS $$
+
+DECLARE
+BEGIN
+	IF (NEW.user_id IS NOT NULL) THEN
+		IF (SELECT deleted FROM users WHERE id = NEW.user_id LIMIT 1) THEN
+			RAISE EXCEPTION 'Cannot create user_secret for deleted user';
+		END IF;
+	END IF;
+	RETURN NEW;
+END;
+$$;
+
+CREATE TRIGGER trigger_upsert_user_secrets
+	BEFORE INSERT OR UPDATE ON user_secrets
+	FOR EACH ROW
+EXECUTE PROCEDURE insert_user_secret_fail_if_user_deleted();

coderd/database/querier.go

@@ -736,8 +736,10 @@ type sqlcQuerier interface {
 	// distribution is active non-system users. Specifically:
 	//
 	//   * deleted = false: Coder soft-deletes by flipping users.deleted
-	//     rather than removing rows, so secrets persist after delete but
-	//     are unreachable.
+	//     rather than removing rows. The delete_deleted_user_resources()
+	//     trigger now removes their user_secrets, but soft-deleted users
+	//     are still excluded here so they don't dilute the percentile
+	//     distribution as zero-secret entries.
 	//   * status = 'active': dormant users (no recent activity) and
 	//     suspended users (explicitly disabled) cannot use secrets, so
 	//     they shouldn't dilute the percentile distribution as

coderd/database/querier_test.go

@@ -7558,6 +7558,78 @@ func TestUserSecretsCRUDOperations(t *testing.T) {
 	})
 }
 
+// TestUserSecretsSoftDeleteTrigger verifies that a user's secrets
+// are deleted when the user is soft-deleted.
+func TestUserSecretsSoftDeleteTrigger(t *testing.T) {
+	t.Parallel()
+
+	db, _ := dbtestutil.NewDB(t)
+	ctx := testutil.Context(t, testutil.WaitMedium)
+
+	// userA will be soft-deleted.
+	userA := dbgen.User(t, db, database.User{})
+	secretA1 := dbgen.UserSecret(t, db, database.UserSecret{
+		UserID:   userA.ID,
+		Name:     "secret-a-1",
+		Value:    "value-a-1",
+		EnvName:  "SECRET_A_1",
+		FilePath: "/secrets/a/1",
+	})
+	secretA2 := dbgen.UserSecret(t, db, database.UserSecret{
+		UserID:   userA.ID,
+		Name:     "secret-a-2",
+		Value:    "value-a-2",
+		EnvName:  "SECRET_A_2",
+		FilePath: "/secrets/a/2",
+	})
+
+	// Sanity-check the existing trigger behavior. An API key for
+	// userA should also be wiped on soft-delete.
+	_, _ = dbgen.APIKey(t, db, database.APIKey{UserID: userA.ID})
+
+	userB := dbgen.User(t, db, database.User{})
+	secretB := dbgen.UserSecret(t, db, database.UserSecret{
+		UserID:   userB.ID,
+		Name:     "secret-b",
+		Value:    "value-b",
+		EnvName:  "SECRET_B",
+		FilePath: "/secrets/b",
+	})
+
+	require.NoError(t, db.UpdateUserDeletedByID(ctx, userA.ID))
+
+	// userA's secrets are removed after soft-deletion.
+	_, err := db.GetUserSecretByID(ctx, secretA1.ID)
+	require.ErrorIs(t, err, sql.ErrNoRows)
+	_, err = db.GetUserSecretByID(ctx, secretA2.ID)
+	require.ErrorIs(t, err, sql.ErrNoRows)
+
+	// userA's API key is also removed.
+	apiKeysA, err := db.GetAPIKeysByUserID(ctx, database.GetAPIKeysByUserIDParams{
+		UserID:    userA.ID,
+		LoginType: userA.LoginType,
+	})
+	require.NoError(t, err)
+	require.Empty(t, apiKeysA)
+
+	// userB's secret is unaffected.
+	got, err := db.GetUserSecretByID(ctx, secretB.ID)
+	require.NoError(t, err)
+	require.Equal(t, secretB.ID, got.ID)
+
+	// Trying to insert a new secret for the soft-deleted userA must fail.
+	_, err = db.CreateUserSecret(ctx, database.CreateUserSecretParams{
+		ID:       uuid.New(),
+		UserID:   userA.ID,
+		Name:     "post-delete",
+		Value:    "value",
+		EnvName:  "POST_DELETE_ENV",
+		FilePath: "/secrets/post-delete",
+	})
+	require.Error(t, err)
+	require.Contains(t, err.Error(), "Cannot create user_secret for deleted user")
+}
+
 func TestUserSecretsAuthorization(t *testing.T) {
 	t.Parallel()
 

coderd/database/queries.sql.go

@@ -25153,8 +25153,10 @@ type GetUserSecretsTelemetrySummaryRow struct {
 // distribution is active non-system users. Specifically:
 //
 //   - deleted = false: Coder soft-deletes by flipping users.deleted
-//     rather than removing rows, so secrets persist after delete but
-//     are unreachable.
+//     rather than removing rows. The delete_deleted_user_resources()
+//     trigger now removes their user_secrets, but soft-deleted users
+//     are still excluded here so they don't dilute the percentile
+//     distribution as zero-secret entries.
 //   - status = 'active': dormant users (no recent activity) and
 //     suspended users (explicitly disabled) cannot use secrets, so
 //     they shouldn't dilute the percentile distribution as

coderd/database/queries/user_secrets.sql

@@ -73,8 +73,10 @@ RETURNING *;
 -- distribution is active non-system users. Specifically:
 --
 --   * deleted = false: Coder soft-deletes by flipping users.deleted
---     rather than removing rows, so secrets persist after delete but
---     are unreachable.
+--     rather than removing rows. The delete_deleted_user_resources()
+--     trigger now removes their user_secrets, but soft-deleted users
+--     are still excluded here so they don't dilute the percentile
+--     distribution as zero-secret entries.
 --   * status = 'active': dormant users (no recent activity) and
 --     suspended users (explicitly disabled) cannot use secrets, so
 --     they shouldn't dilute the percentile distribution as

coderd/telemetry/telemetry_test.go

@@ -2147,18 +2147,6 @@ func TestUserSecretsTelemetry(t *testing.T) {
 			p.FilePath = "/home/coder/active.file"
 		})
 
-		// Soft-deleted user. user_secrets has ON DELETE CASCADE on
-		// users, but Coder soft-deletes by setting users.deleted, so
-		// the secret row persists. The summary should ignore it.
-		deleted := dbgen.User(t, db, database.User{Deleted: true})
-		_ = dbgen.UserSecret(t, db, database.UserSecret{
-			UserID: deleted.ID,
-			Name:   "deleted-secret",
-		}, func(p *database.CreateUserSecretParams) {
-			p.EnvName = "DELETED_ENV"
-			p.FilePath = ""
-		})
-
 		// User secret owned by a dormant user should be excluded.
 		dormant := dbgen.User(t, db, database.User{Status: database.UserStatusDormant})
 		_ = dbgen.UserSecret(t, db, database.UserSecret{

enterprise/cli/server_dbcrypt_test.go

@@ -234,7 +234,7 @@ func genData(t *testing.T, db database.Store) []database.User {
 					OAuthAccessToken:  "access-" + usr.ID.String(),
 					OAuthRefreshToken: "refresh-" + usr.ID.String(),
 				})
-				// Deleted users cannot have user_links
+				// Deleted users cannot have user_links or user_secrets.
 				if !deleted {
 					// Fun fact: our schema allows _all_ login types to have
 					// a user_link. Even though I'm not sure how it could occur
@@ -245,15 +245,15 @@ func genData(t *testing.T, db database.Store) []database.User {
 						OAuthAccessToken:  "access-" + usr.ID.String(),
 						OAuthRefreshToken: "refresh-" + usr.ID.String(),
 					})
-				}
 
-				_ = dbgen.UserSecret(t, db, database.UserSecret{
-					UserID:   usr.ID,
-					Name:     "secret-" + usr.ID.String(),
-					Value:    "value-" + usr.ID.String(),
-					EnvName:  "",
-					FilePath: "",
-				})
+					_ = dbgen.UserSecret(t, db, database.UserSecret{
+						UserID:   usr.ID,
+						Name:     "secret-" + usr.ID.String(),
+						Value:    "value-" + usr.ID.String(),
+						EnvName:  "",
+						FilePath: "",
+					})
+				}
 				users = append(users, usr)
 			}
 		}