mirror of
https://github.com/coder/coder.git
synced 2026-06-02 20:48:20 +00:00
fix(coderd/database): clean up org memberships when user is soft-deleted (#25149)
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
<details>
<summary>Implementation notes</summary>
**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
</details>
This commit is contained in:
Seth Shelnutt
Generated
+6
@@ -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;
|
||||
|
||||
@@ -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;
|
||||
$$;
|
||||
@@ -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;
|
||||
$$;
|
||||
@@ -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()
|
||||
|
||||
|
||||
Reference in New Issue
Block a user