From f355e010e89a937af92970e83c2364d4511ce8d4 Mon Sep 17 00:00:00 2001 From: Seth Shelnutt Date: Tue, 12 May 2026 16:20:25 -0400 Subject: [PATCH] fix(coderd/database): clean up org memberships when user is soft-deleted (#25149) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The soft-delete cleanup trigger (`delete_deleted_user_resources`) removed `api_keys`, `user_links`, and `user_secrets` but left `organization_members` rows intact. When a new user was created with a previously-deleted user's email, both user IDs had org membership rows in the same organization, producing duplicate-email members. Extend the trigger to also delete `organization_members` for the soft-deleted user. This cascades through the existing `trigger_delete_group_members_on_org_member_delete`, which cleans up group memberships automatically. The migration backfills by removing zombie rows for already-deleted users. Fixes ENG-831 > [!NOTE] > 🤖 Generated by Coder Agents
Implementation notes **Root cause**: `GetOrganizationIDsByMemberIDs` does not join on `users.deleted = false`, so stale org membership rows for soft-deleted users were visible to internal queries. Even the filtered queries (`OrganizationMembers`, `PaginatedOrganizationMembers`) could surface duplicate emails when a new active user reused a deleted user's email. **What changed**: - Migration 000491 extends `delete_deleted_user_resources()` to `DELETE FROM organization_members WHERE user_id = OLD.id` - Backfill removes existing zombie org memberships for soft-deleted users - `TestOrgMembersSoftDeleteTrigger` covers org membership removal, raw row cleanup, and cascading group membership cleanup
--- coderd/database/dump.sql | 6 + ...e_org_members_on_user_soft_delete.down.sql | 28 +++ ...ete_org_members_on_user_soft_delete.up.sql | 50 +++++ coderd/database/querier_test.go | 171 ++++++++++++++++++ 4 files changed, 255 insertions(+) create mode 100644 coderd/database/migrations/000492_delete_org_members_on_user_soft_delete.down.sql create mode 100644 coderd/database/migrations/000492_delete_org_members_on_user_soft_delete.up.sql diff --git a/coderd/database/dump.sql b/coderd/database/dump.sql index 4c9ee415cc..ef76f01353 100644 --- a/coderd/database/dump.sql +++ b/coderd/database/dump.sql @@ -768,6 +768,12 @@ BEGIN -- does not remove the users row so the FK cascade never fires. DELETE FROM user_secrets WHERE user_id = OLD.id; + + -- Remove their organization memberships. + -- This also triggers group membership cleanup via + -- trigger_delete_group_members_on_org_member_delete. + DELETE FROM organization_members + WHERE user_id = OLD.id; END IF; RETURN NEW; END; diff --git a/coderd/database/migrations/000492_delete_org_members_on_user_soft_delete.down.sql b/coderd/database/migrations/000492_delete_org_members_on_user_soft_delete.down.sql new file mode 100644 index 0000000000..e615a28915 --- /dev/null +++ b/coderd/database/migrations/000492_delete_org_members_on_user_soft_delete.down.sql @@ -0,0 +1,28 @@ +-- Restore the previous body of delete_deleted_user_resources() from +-- migration 000490 (without the organization_members cleanup). +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; +$$; diff --git a/coderd/database/migrations/000492_delete_org_members_on_user_soft_delete.up.sql b/coderd/database/migrations/000492_delete_org_members_on_user_soft_delete.up.sql new file mode 100644 index 0000000000..abc4682494 --- /dev/null +++ b/coderd/database/migrations/000492_delete_org_members_on_user_soft_delete.up.sql @@ -0,0 +1,50 @@ +-- Extend the soft-delete cleanup trigger to also remove organization_members. +-- organization_members.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 memberships would otherwise survive deletion. +-- Removing an org membership also fires +-- trigger_delete_group_members_on_org_member_delete, which cleans up +-- the user's group memberships in that organization automatically. +-- +-- Backfill any rows that belonged to already-soft-deleted users before +-- replacing the function. +DELETE FROM + organization_members +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; + + -- Remove their organization memberships. + -- This also triggers group membership cleanup via + -- trigger_delete_group_members_on_org_member_delete. + DELETE FROM organization_members + WHERE user_id = OLD.id; + END IF; + RETURN NEW; +END; +$$; diff --git a/coderd/database/querier_test.go b/coderd/database/querier_test.go index 2d6981dbbf..ebe000b21d 100644 --- a/coderd/database/querier_test.go +++ b/coderd/database/querier_test.go @@ -7811,6 +7811,177 @@ func TestUserSecretsSoftDeleteTrigger(t *testing.T) { require.Contains(t, err.Error(), "Cannot create user_secret for deleted user") } +// TestOrgMembersSoftDeleteTrigger verifies that a user's organization +// memberships (and transitively their group memberships) are deleted +// when the user is soft-deleted. +func TestOrgMembersSoftDeleteTrigger(t *testing.T) { + t.Parallel() + + // SingleOrg verifies the basic case: one org, one group, and a + // control user whose membership must survive. + t.Run("SingleOrg", func(t *testing.T) { + t.Parallel() + + db, _ := dbtestutil.NewDB(t) + ctx := testutil.Context(t, testutil.WaitMedium) + + org := dbgen.Organization(t, db, database.Organization{}) + + // userA will be soft-deleted. + userA := dbgen.User(t, db, database.User{}) + dbgen.OrganizationMember(t, db, database.OrganizationMember{ + OrganizationID: org.ID, + UserID: userA.ID, + }) + + // Add userA to a group in the org (should be cleaned up transitively). + group := dbgen.Group(t, db, database.Group{OrganizationID: org.ID}) + dbgen.GroupMember(t, db, database.GroupMemberTable{ + UserID: userA.ID, + GroupID: group.ID, + }) + + // userB is a control; their membership must not be touched. + userB := dbgen.User(t, db, database.User{}) + dbgen.OrganizationMember(t, db, database.OrganizationMember{ + OrganizationID: org.ID, + UserID: userB.ID, + }) + dbgen.GroupMember(t, db, database.GroupMemberTable{ + UserID: userB.ID, + GroupID: group.ID, + }) + + // Soft-delete userA. + require.NoError(t, db.UpdateUserDeletedByID(ctx, userA.ID)) + + // userA should no longer appear in the organization. + orgMembers, err := db.OrganizationMembers(ctx, database.OrganizationMembersParams{ + OrganizationID: org.ID, + }) + require.NoError(t, err) + var memberIDs []uuid.UUID + for _, m := range orgMembers { + memberIDs = append(memberIDs, m.OrganizationMember.UserID) + } + require.NotContains(t, memberIDs, userA.ID) + require.Contains(t, memberIDs, userB.ID) + + // The raw org membership rows should also be gone (not just hidden). + rawOrgs, err := db.GetOrganizationIDsByMemberIDs(ctx, []uuid.UUID{userA.ID}) + require.NoError(t, err) + require.Empty(t, rawOrgs, "zombie org membership rows should not exist after soft-delete") + + // userA's group membership should also be removed by the cascading trigger. + groupMembers, err := db.GetGroupMembersByGroupID(ctx, database.GetGroupMembersByGroupIDParams{ + GroupID: group.ID, + IncludeSystem: true, + }) + require.NoError(t, err) + var groupMemberIDs []uuid.UUID + for _, gm := range groupMembers { + groupMemberIDs = append(groupMemberIDs, gm.UserID) + } + require.NotContains(t, groupMemberIDs, userA.ID) + require.Contains(t, groupMemberIDs, userB.ID) + }) + + // MultipleOrgs verifies that memberships are cleaned up across + // every organization the deleted user belonged to. + t.Run("MultipleOrgs", func(t *testing.T) { + t.Parallel() + + db, _ := dbtestutil.NewDB(t) + ctx := testutil.Context(t, testutil.WaitMedium) + + org1 := dbgen.Organization(t, db, database.Organization{}) + org2 := dbgen.Organization(t, db, database.Organization{}) + + // userA will be soft-deleted. They belong to both orgs. + userA := dbgen.User(t, db, database.User{}) + dbgen.OrganizationMember(t, db, database.OrganizationMember{ + OrganizationID: org1.ID, + UserID: userA.ID, + }) + dbgen.OrganizationMember(t, db, database.OrganizationMember{ + OrganizationID: org2.ID, + UserID: userA.ID, + }) + + // Add userA to a group in each org. + group1 := dbgen.Group(t, db, database.Group{OrganizationID: org1.ID}) + dbgen.GroupMember(t, db, database.GroupMemberTable{ + UserID: userA.ID, + GroupID: group1.ID, + }) + group2 := dbgen.Group(t, db, database.Group{OrganizationID: org2.ID}) + dbgen.GroupMember(t, db, database.GroupMemberTable{ + UserID: userA.ID, + GroupID: group2.ID, + }) + + // userB stays in org1 as a control. + userB := dbgen.User(t, db, database.User{}) + dbgen.OrganizationMember(t, db, database.OrganizationMember{ + OrganizationID: org1.ID, + UserID: userB.ID, + }) + dbgen.GroupMember(t, db, database.GroupMemberTable{ + UserID: userB.ID, + GroupID: group1.ID, + }) + + // Soft-delete userA. + require.NoError(t, db.UpdateUserDeletedByID(ctx, userA.ID)) + + // userA should be gone from both orgs. + for _, org := range []database.Organization{org1, org2} { + members, err := db.OrganizationMembers(ctx, database.OrganizationMembersParams{ + OrganizationID: org.ID, + }) + require.NoError(t, err) + for _, m := range members { + require.NotEqual(t, userA.ID, m.OrganizationMember.UserID, + "userA should not appear in org %s", org.ID) + } + } + + // No raw org membership rows should remain. + rawOrgs, err := db.GetOrganizationIDsByMemberIDs(ctx, []uuid.UUID{userA.ID}) + require.NoError(t, err) + require.Empty(t, rawOrgs, "zombie org membership rows should not exist after soft-delete") + + // Group memberships in both orgs should be cleaned up. + for _, g := range []struct { + name string + groupID uuid.UUID + }{ + {"org1-group", group1.ID}, + {"org2-group", group2.ID}, + } { + groupMembers, err := db.GetGroupMembersByGroupID(ctx, database.GetGroupMembersByGroupIDParams{ + GroupID: g.groupID, + IncludeSystem: true, + }) + require.NoError(t, err, g.name) + for _, gm := range groupMembers { + require.NotEqual(t, userA.ID, gm.UserID, g.name) + } + } + + // userB's memberships are unaffected. + org1Members, err := db.OrganizationMembers(ctx, database.OrganizationMembersParams{ + OrganizationID: org1.ID, + }) + require.NoError(t, err) + var org1MemberIDs []uuid.UUID + for _, m := range org1Members { + org1MemberIDs = append(org1MemberIDs, m.OrganizationMember.UserID) + } + require.Contains(t, org1MemberIDs, userB.ID) + }) +} + func TestUserSecretsAuthorization(t *testing.T) { t.Parallel()