diff --git a/coderd/database/dump.sql b/coderd/database/dump.sql index 687e80e1b5..1ed77d9c0b 100644 --- a/coderd/database/dump.sql +++ b/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(); diff --git a/coderd/database/migrations/000490_trigger_delete_user_secrets.down.sql b/coderd/database/migrations/000490_trigger_delete_user_secrets.down.sql new file mode 100644 index 0000000000..02bc2bde22 --- /dev/null +++ b/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; +$$; diff --git a/coderd/database/migrations/000490_trigger_delete_user_secrets.up.sql b/coderd/database/migrations/000490_trigger_delete_user_secrets.up.sql new file mode 100644 index 0000000000..0fbb5fd95c --- /dev/null +++ b/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(); diff --git a/coderd/database/querier.go b/coderd/database/querier.go index 23301e6b62..6c3a951730 100644 --- a/coderd/database/querier.go +++ b/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 diff --git a/coderd/database/querier_test.go b/coderd/database/querier_test.go index f58f428a51..41134ca12e 100644 --- a/coderd/database/querier_test.go +++ b/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() diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index 43acf4f25d..daefd542ff 100644 --- a/coderd/database/queries.sql.go +++ b/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 diff --git a/coderd/database/queries/user_secrets.sql b/coderd/database/queries/user_secrets.sql index 6bd6a14522..2bca3a0ca4 100644 --- a/coderd/database/queries/user_secrets.sql +++ b/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 diff --git a/coderd/telemetry/telemetry_test.go b/coderd/telemetry/telemetry_test.go index 5b3b0b2a6e..b3de13bff7 100644 --- a/coderd/telemetry/telemetry_test.go +++ b/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{ diff --git a/enterprise/cli/server_dbcrypt_test.go b/enterprise/cli/server_dbcrypt_test.go index 79a64ea1e3..61579db50a 100644 --- a/enterprise/cli/server_dbcrypt_test.go +++ b/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) } }