Content-Length: 15743 | pFad | http://github.com/coder/coder/pull/17621.patch

thub.com From a09b2670abe9a24178c8cfc7826469ccdacf2a2b Mon Sep 17 00:00:00 2001 From: Brett Kolodny Date: Wed, 30 Apr 2025 15:16:03 +0000 Subject: [PATCH 1/3] fix: update query and function to filter out deleted users --- coderd/database/dump.sql | 7 +- ...ting_orgs_to_filter_deleted_users.down.sql | 96 +++++++++++++++++ ...leting_orgs_to_filter_deleted_users.up.sql | 101 ++++++++++++++++++ coderd/database/queries.sql.go | 44 +++++++- coderd/database/queries/organizations.sql | 45 +++++++- 5 files changed, 282 insertions(+), 11 deletions(-) create mode 100644 coderd/database/migrations/000318_update_protect_deleting_orgs_to_filter_deleted_users.down.sql create mode 100644 coderd/database/migrations/000318_update_protect_deleting_orgs_to_filter_deleted_users.up.sql diff --git a/coderd/database/dump.sql b/coderd/database/dump.sql index 83d998b2b9a3e..968b6a24d4bf8 100644 --- a/coderd/database/dump.sql +++ b/coderd/database/dump.sql @@ -482,9 +482,14 @@ BEGIN ); member_count := ( - SELECT count(*) as count FROM organization_members + SELECT + count(*) AS count + FROM + organization_members + LEFT JOIN users ON users.id = organization_members.user_id WHERE organization_members.organization_id = OLD.id + AND users.deleted = FALSE ); provisioner_keys_count := ( diff --git a/coderd/database/migrations/000318_update_protect_deleting_orgs_to_filter_deleted_users.down.sql b/coderd/database/migrations/000318_update_protect_deleting_orgs_to_filter_deleted_users.down.sql new file mode 100644 index 0000000000000..cacafc029222c --- /dev/null +++ b/coderd/database/migrations/000318_update_protect_deleting_orgs_to_filter_deleted_users.down.sql @@ -0,0 +1,96 @@ +DROP TRIGGER IF EXISTS protect_deleting_organizations ON organizations; + +-- Replace the function with the new implementation +CREATE OR REPLACE FUNCTION protect_deleting_organizations() + RETURNS TRIGGER AS +$$ +DECLARE + workspace_count int; + template_count int; + group_count int; + member_count int; + provisioner_keys_count int; +BEGIN + workspace_count := ( + SELECT count(*) as count FROM workspaces + WHERE + workspaces.organization_id = OLD.id + AND workspaces.deleted = false + ); + + template_count := ( + SELECT count(*) as count FROM templates + WHERE + templates.organization_id = OLD.id + AND templates.deleted = false + ); + + group_count := ( + SELECT count(*) as count FROM groups + WHERE + groups.organization_id = OLD.id + ); + + member_count := ( + SELECT count(*) as count FROM organization_members + WHERE + organization_members.organization_id = OLD.id + ); + + provisioner_keys_count := ( + Select count(*) as count FROM provisioner_keys + WHERE + provisioner_keys.organization_id = OLD.id + ); + + -- Fail the deletion if one of the following: + -- * the organization has 1 or more workspaces + -- * the organization has 1 or more templates + -- * the organization has 1 or more groups other than "Everyone" group + -- * the organization has 1 or more members other than the organization owner + -- * the organization has 1 or more provisioner keys + + -- Only create error message for resources that actually exist + IF (workspace_count + template_count + provisioner_keys_count) > 0 THEN + DECLARE + error_message text := 'cannot delete organization: organization has '; + error_parts text[] := '{}'; + BEGIN + IF workspace_count > 0 THEN + error_parts := array_append(error_parts, workspace_count || ' workspaces'); + END IF; + + IF template_count > 0 THEN + error_parts := array_append(error_parts, template_count || ' templates'); + END IF; + + IF provisioner_keys_count > 0 THEN + error_parts := array_append(error_parts, provisioner_keys_count || ' provisioner keys'); + END IF; + + error_message := error_message || array_to_string(error_parts, ', ') || ' that must be deleted first'; + RAISE EXCEPTION '%', error_message; + END; + END IF; + + IF (group_count) > 1 THEN + RAISE EXCEPTION 'cannot delete organization: organization has % groups that must be deleted first', group_count - 1; + END IF; + + -- Allow 1 member to exist, because you cannot remove yourself. You can + -- remove everyone else. Ideally, we only omit the member that matches + -- the user_id of the caller, however in a trigger, the caller is unknown. + IF (member_count) > 1 THEN + RAISE EXCEPTION 'cannot delete organization: organization has % members that must be deleted first', member_count - 1; + END IF; + + RETURN NEW; +END; +$$ LANGUAGE plpgsql; + +-- Trigger to protect organizations from being soft deleted with existing resources +CREATE TRIGGER protect_deleting_organizations + BEFORE UPDATE ON organizations + FOR EACH ROW + WHEN (NEW.deleted = true AND OLD.deleted = false) + EXECUTE FUNCTION protect_deleting_organizations(); diff --git a/coderd/database/migrations/000318_update_protect_deleting_orgs_to_filter_deleted_users.up.sql b/coderd/database/migrations/000318_update_protect_deleting_orgs_to_filter_deleted_users.up.sql new file mode 100644 index 0000000000000..8db15223d92f1 --- /dev/null +++ b/coderd/database/migrations/000318_update_protect_deleting_orgs_to_filter_deleted_users.up.sql @@ -0,0 +1,101 @@ +DROP TRIGGER IF EXISTS protect_deleting_organizations ON organizations; + +-- Replace the function with the new implementation +CREATE OR REPLACE FUNCTION protect_deleting_organizations() + RETURNS TRIGGER AS +$$ +DECLARE + workspace_count int; + template_count int; + group_count int; + member_count int; + provisioner_keys_count int; +BEGIN + workspace_count := ( + SELECT count(*) as count FROM workspaces + WHERE + workspaces.organization_id = OLD.id + AND workspaces.deleted = false + ); + + template_count := ( + SELECT count(*) as count FROM templates + WHERE + templates.organization_id = OLD.id + AND templates.deleted = false + ); + + group_count := ( + SELECT count(*) as count FROM groups + WHERE + groups.organization_id = OLD.id + ); + + member_count := ( + SELECT + count(*) AS count + FROM + organization_members + LEFT JOIN users ON users.id = organization_members.user_id + WHERE + organization_members.organization_id = OLD.id + AND users.deleted = FALSE + ); + + provisioner_keys_count := ( + Select count(*) as count FROM provisioner_keys + WHERE + provisioner_keys.organization_id = OLD.id + ); + + -- Fail the deletion if one of the following: + -- * the organization has 1 or more workspaces + -- * the organization has 1 or more templates + -- * the organization has 1 or more groups other than "Everyone" group + -- * the organization has 1 or more members other than the organization owner + -- * the organization has 1 or more provisioner keys + + -- Only create error message for resources that actually exist + IF (workspace_count + template_count + provisioner_keys_count) > 0 THEN + DECLARE + error_message text := 'cannot delete organization: organization has '; + error_parts text[] := '{}'; + BEGIN + IF workspace_count > 0 THEN + error_parts := array_append(error_parts, workspace_count || ' workspaces'); + END IF; + + IF template_count > 0 THEN + error_parts := array_append(error_parts, template_count || ' templates'); + END IF; + + IF provisioner_keys_count > 0 THEN + error_parts := array_append(error_parts, provisioner_keys_count || ' provisioner keys'); + END IF; + + error_message := error_message || array_to_string(error_parts, ', ') || ' that must be deleted first'; + RAISE EXCEPTION '%', error_message; + END; + END IF; + + IF (group_count) > 1 THEN + RAISE EXCEPTION 'cannot delete organization: organization has % groups that must be deleted first', group_count - 1; + END IF; + + -- Allow 1 member to exist, because you cannot remove yourself. You can + -- remove everyone else. Ideally, we only omit the member that matches + -- the user_id of the caller, however in a trigger, the caller is unknown. + IF (member_count) > 1 THEN + RAISE EXCEPTION 'cannot delete organization: organization has % members that must be deleted first', member_count - 1; + END IF; + + RETURN NEW; +END; +$$ LANGUAGE plpgsql; + +-- Trigger to protect organizations from being soft deleted with existing resources +CREATE TRIGGER protect_deleting_organizations + BEFORE UPDATE ON organizations + FOR EACH ROW + WHEN (NEW.deleted = true AND OLD.deleted = false) + EXECUTE FUNCTION protect_deleting_organizations(); diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index 60416b1a35730..3908dab715e31 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -5586,11 +5586,45 @@ func (q *sqlQuerier) GetOrganizationByName(ctx context.Context, arg GetOrganizat const getOrganizationResourceCountByID = `-- name: GetOrganizationResourceCountByID :one SELECT - (SELECT COUNT(*) FROM workspaces WHERE workspaces.organization_id = $1 AND workspaces.deleted = false) AS workspace_count, - (SELECT COUNT(*) FROM groups WHERE groups.organization_id = $1) AS group_count, - (SELECT COUNT(*) FROM templates WHERE templates.organization_id = $1 AND templates.deleted = false) AS template_count, - (SELECT COUNT(*) FROM organization_members WHERE organization_members.organization_id = $1) AS member_count, - (SELECT COUNT(*) FROM provisioner_keys WHERE provisioner_keys.organization_id = $1) AS provisioner_key_count + ( + SELECT + count(*) + FROM + workspaces + WHERE + workspaces.organization_id = $1 + AND workspaces.deleted = FALSE) AS workspace_count, + ( + SELECT + count(*) + FROM + GROUPS + WHERE + groups.organization_id = $1) AS group_count, + ( + SELECT + count(*) + FROM + templates + WHERE + templates.organization_id = $1 + AND templates.deleted = FALSE) AS template_count, + ( + SELECT + count(*) + FROM + organization_members + LEFT JOIN users ON organization_members.user_id = users.id + WHERE + organization_members.organization_id = $1 + AND users.deleted = FALSE) AS member_count, +( + SELECT + count(*) + FROM + provisioner_keys + WHERE + provisioner_keys.organization_id = $1) AS provisioner_key_count ` type GetOrganizationResourceCountByIDRow struct { diff --git a/coderd/database/queries/organizations.sql b/coderd/database/queries/organizations.sql index d940fb1ad4dc6..89a4a7bcfcef4 100644 --- a/coderd/database/queries/organizations.sql +++ b/coderd/database/queries/organizations.sql @@ -73,11 +73,46 @@ WHERE -- name: GetOrganizationResourceCountByID :one SELECT - (SELECT COUNT(*) FROM workspaces WHERE workspaces.organization_id = $1 AND workspaces.deleted = false) AS workspace_count, - (SELECT COUNT(*) FROM groups WHERE groups.organization_id = $1) AS group_count, - (SELECT COUNT(*) FROM templates WHERE templates.organization_id = $1 AND templates.deleted = false) AS template_count, - (SELECT COUNT(*) FROM organization_members WHERE organization_members.organization_id = $1) AS member_count, - (SELECT COUNT(*) FROM provisioner_keys WHERE provisioner_keys.organization_id = $1) AS provisioner_key_count; + ( + SELECT + count(*) + FROM + workspaces + WHERE + workspaces.organization_id = $1 + AND workspaces.deleted = FALSE) AS workspace_count, + ( + SELECT + count(*) + FROM + GROUPS + WHERE + groups.organization_id = $1) AS group_count, + ( + SELECT + count(*) + FROM + templates + WHERE + templates.organization_id = $1 + AND templates.deleted = FALSE) AS template_count, + ( + SELECT + count(*) + FROM + organization_members + LEFT JOIN users ON organization_members.user_id = users.id + WHERE + organization_members.organization_id = $1 + AND users.deleted = FALSE) AS member_count, +( + SELECT + count(*) + FROM + provisioner_keys + WHERE + provisioner_keys.organization_id = $1) AS provisioner_key_count; + -- name: InsertOrganization :one INSERT INTO From 4cd79d866ddfa2e9f22508bda9346a0792e1227e Mon Sep 17 00:00:00 2001 From: Brett Kolodny Date: Wed, 30 Apr 2025 15:44:10 +0000 Subject: [PATCH 2/3] chore: add test to ensure deleted users are filtered from org deletion --- coderd/database/querier_test.go | 38 +++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/coderd/database/querier_test.go b/coderd/database/querier_test.go index 4a2edb4451c34..a4fd6389acc4d 100644 --- a/coderd/database/querier_test.go +++ b/coderd/database/querier_test.go @@ -3586,6 +3586,44 @@ func TestOrganizationDeleteTrigger(t *testing.T) { require.ErrorContains(t, err, "cannot delete organization") require.ErrorContains(t, err, "has 1 members") }) + + t.Run("UserDeletedButNotRemovedFromOrg", func(t *testing.T) { + t.Parallel() + db, _ := dbtestutil.NewDB(t) + + orgA := dbfake.Organization(t, db).Do() + + userA := dbgen.User(t, db, database.User{}) + userB := dbgen.User(t, db, database.User{}) + userC := dbgen.User(t, db, database.User{}) + + dbgen.OrganizationMember(t, db, database.OrganizationMember{ + OrganizationID: orgA.Org.ID, + UserID: userA.ID, + }) + dbgen.OrganizationMember(t, db, database.OrganizationMember{ + OrganizationID: orgA.Org.ID, + UserID: userB.ID, + }) + dbgen.OrganizationMember(t, db, database.OrganizationMember{ + OrganizationID: orgA.Org.ID, + UserID: userC.ID, + }) + + // Delete one of the users but don't remove them from the org + ctx := testutil.Context(t, testutil.WaitShort) + db.UpdateUserDeletedByID(ctx, userB.ID) + + err := db.UpdateOrganizationDeletedByID(ctx, database.UpdateOrganizationDeletedByIDParams{ + UpdatedAt: dbtime.Now(), + ID: orgA.Org.ID, + }) + require.Error(t, err) + // cannot delete organization: organization has 1 members that must be deleted first + require.ErrorContains(t, err, "cannot delete organization") + require.ErrorContains(t, err, "has 1 members") + + }) } type templateVersionWithPreset struct { From 4c5d6cc21d7d621e3cb3f64dde1f65f4c77fcadb Mon Sep 17 00:00:00 2001 From: Brett Kolodny Date: Wed, 30 Apr 2025 15:47:46 +0000 Subject: [PATCH 3/3] chore: fmt --- coderd/database/querier_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/coderd/database/querier_test.go b/coderd/database/querier_test.go index a4fd6389acc4d..b2cc20c4894d5 100644 --- a/coderd/database/querier_test.go +++ b/coderd/database/querier_test.go @@ -3622,7 +3622,6 @@ func TestOrganizationDeleteTrigger(t *testing.T) { // cannot delete organization: organization has 1 members that must be deleted first require.ErrorContains(t, err, "cannot delete organization") require.ErrorContains(t, err, "has 1 members") - }) }

pFad - (p)hone/(F)rame/(a)nonymizer/(d)eclutterfier! Saves Data!