Skip to content

Commit b7e08ba

Browse files
brettkolodnybrettkolodny
authored
fix: filter out deleted users when attempting to delete an organization (#17621)
Closes [coder/internal#601](coder/internal#601)
1 parent cae4fa8 commit b7e08ba

6 files changed

+319
-11
lines changed

‎coderd/database/dump.sql

+6-1
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/database/migrations/000318_update_protect_deleting_orgs_to_filter_deleted_users.down.sql

+96
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,96 @@
1+
DROP TRIGGER IF EXISTS protect_deleting_organizations ON organizations;
2+
3+
-- Replace the function with the new implementation
4+
CREATE OR REPLACE FUNCTION protect_deleting_organizations()
5+
RETURNS TRIGGER AS
6+
$$
7+
DECLARE
8+
workspace_count int;
9+
template_count int;
10+
group_count int;
11+
member_count int;
12+
provisioner_keys_count int;
13+
BEGIN
14+
workspace_count := (
15+
SELECT count(*) as count FROM workspaces
16+
WHERE
17+
workspaces.organization_id = OLD.id
18+
AND workspaces.deleted = false
19+
);
20+
21+
template_count := (
22+
SELECT count(*) as count FROM templates
23+
WHERE
24+
templates.organization_id = OLD.id
25+
AND templates.deleted = false
26+
);
27+
28+
group_count := (
29+
SELECT count(*) as count FROM groups
30+
WHERE
31+
groups.organization_id = OLD.id
32+
);
33+
34+
member_count := (
35+
SELECT count(*) as count FROM organization_members
36+
WHERE
37+
organization_members.organization_id = OLD.id
38+
);
39+
40+
provisioner_keys_count := (
41+
Select count(*) as count FROM provisioner_keys
42+
WHERE
43+
provisioner_keys.organization_id = OLD.id
44+
);
45+
46+
-- Fail the deletion if one of the following:
47+
-- * the organization has 1 or more workspaces
48+
-- * the organization has 1 or more templates
49+
-- * the organization has 1 or more groups other than "Everyone" group
50+
-- * the organization has 1 or more members other than the organization owner
51+
-- * the organization has 1 or more provisioner keys
52+
53+
-- Only create error message for resources that actually exist
54+
IF (workspace_count + template_count + provisioner_keys_count) > 0 THEN
55+
DECLARE
56+
error_message text := 'cannot delete organization: organization has ';
57+
error_parts text[] := '{}';
58+
BEGIN
59+
IF workspace_count > 0 THEN
60+
error_parts := array_append(error_parts, workspace_count || ' workspaces');
61+
END IF;
62+
63+
IF template_count > 0 THEN
64+
error_parts := array_append(error_parts, template_count || ' templates');
65+
END IF;
66+
67+
IF provisioner_keys_count > 0 THEN
68+
error_parts := array_append(error_parts, provisioner_keys_count || ' provisioner keys');
69+
END IF;
70+
71+
error_message := error_message || array_to_string(error_parts, ', ') || ' that must be deleted first';
72+
RAISE EXCEPTION '%', error_message;
73+
END;
74+
END IF;
75+
76+
IF (group_count) > 1 THEN
77+
RAISE EXCEPTION 'cannot delete organization: organization has % groups that must be deleted first', group_count - 1;
78+
END IF;
79+
80+
-- Allow 1 member to exist, because you cannot remove yourself. You can
81+
-- remove everyone else. Ideally, we only omit the member that matches
82+
-- the user_id of the caller, however in a trigger, the caller is unknown.
83+
IF (member_count) > 1 THEN
84+
RAISE EXCEPTION 'cannot delete organization: organization has % members that must be deleted first', member_count - 1;
85+
END IF;
86+
87+
RETURN NEW;
88+
END;
89+
$$ LANGUAGE plpgsql;
90+
91+
-- Trigger to protect organizations from being soft deleted with existing resources
92+
CREATE TRIGGER protect_deleting_organizations
93+
BEFORE UPDATE ON organizations
94+
FOR EACH ROW
95+
WHEN (NEW.deleted = true AND OLD.deleted = false)
96+
EXECUTE FUNCTION protect_deleting_organizations();

‎coderd/database/migrations/000318_update_protect_deleting_orgs_to_filter_deleted_users.up.sql

+101
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,101 @@
1+
DROP TRIGGER IF EXISTS protect_deleting_organizations ON organizations;
2+
3+
-- Replace the function with the new implementation
4+
CREATE OR REPLACE FUNCTION protect_deleting_organizations()
5+
RETURNS TRIGGER AS
6+
$$
7+
DECLARE
8+
workspace_count int;
9+
template_count int;
10+
group_count int;
11+
member_count int;
12+
provisioner_keys_count int;
13+
BEGIN
14+
workspace_count := (
15+
SELECT count(*) as count FROM workspaces
16+
WHERE
17+
workspaces.organization_id = OLD.id
18+
AND workspaces.deleted = false
19+
);
20+
21+
template_count := (
22+
SELECT count(*) as count FROM templates
23+
WHERE
24+
templates.organization_id = OLD.id
25+
AND templates.deleted = false
26+
);
27+
28+
group_count := (
29+
SELECT count(*) as count FROM groups
30+
WHERE
31+
groups.organization_id = OLD.id
32+
);
33+
34+
member_count := (
35+
SELECT
36+
count(*) AS count
37+
FROM
38+
organization_members
39+
LEFT JOIN users ON users.id = organization_members.user_id
40+
WHERE
41+
organization_members.organization_id = OLD.id
42+
AND users.deleted = FALSE
43+
);
44+
45+
provisioner_keys_count := (
46+
Select count(*) as count FROM provisioner_keys
47+
WHERE
48+
provisioner_keys.organization_id = OLD.id
49+
);
50+
51+
-- Fail the deletion if one of the following:
52+
-- * the organization has 1 or more workspaces
53+
-- * the organization has 1 or more templates
54+
-- * the organization has 1 or more groups other than "Everyone" group
55+
-- * the organization has 1 or more members other than the organization owner
56+
-- * the organization has 1 or more provisioner keys
57+
58+
-- Only create error message for resources that actually exist
59+
IF (workspace_count + template_count + provisioner_keys_count) > 0 THEN
60+
DECLARE
61+
error_message text := 'cannot delete organization: organization has ';
62+
error_parts text[] := '{}';
63+
BEGIN
64+
IF workspace_count > 0 THEN
65+
error_parts := array_append(error_parts, workspace_count || ' workspaces');
66+
END IF;
67+
68+
IF template_count > 0 THEN
69+
error_parts := array_append(error_parts, template_count || ' templates');
70+
END IF;
71+
72+
IF provisioner_keys_count > 0 THEN
73+
error_parts := array_append(error_parts, provisioner_keys_count || ' provisioner keys');
74+
END IF;
75+
76+
error_message := error_message || array_to_string(error_parts, ', ') || ' that must be deleted first';
77+
RAISE EXCEPTION '%', error_message;
78+
END;
79+
END IF;
80+
81+
IF (group_count) > 1 THEN
82+
RAISE EXCEPTION 'cannot delete organization: organization has % groups that must be deleted first', group_count - 1;
83+
END IF;
84+
85+
-- Allow 1 member to exist, because you cannot remove yourself. You can
86+
-- remove everyone else. Ideally, we only omit the member that matches
87+
-- the user_id of the caller, however in a trigger, the caller is unknown.
88+
IF (member_count) > 1 THEN
89+
RAISE EXCEPTION 'cannot delete organization: organization has % members that must be deleted first', member_count - 1;
90+
END IF;
91+
92+
RETURN NEW;
93+
END;
94+
$$ LANGUAGE plpgsql;
95+
96+
-- Trigger to protect organizations from being soft deleted with existing resources
97+
CREATE TRIGGER protect_deleting_organizations
98+
BEFORE UPDATE ON organizations
99+
FOR EACH ROW
100+
WHEN (NEW.deleted = true AND OLD.deleted = false)
101+
EXECUTE FUNCTION protect_deleting_organizations();

‎coderd/database/querier_test.go

+37
Original file line numberDiff line numberDiff line change
@@ -3586,6 +3586,43 @@ func TestOrganizationDeleteTrigger(t *testing.T) {
35863586
require.ErrorContains(t, err, "cannot delete organization")
35873587
require.ErrorContains(t, err, "has 1 members")
35883588
})
3589+
3590+
t.Run("UserDeletedButNotRemovedFromOrg", func(t *testing.T) {
3591+
t.Parallel()
3592+
db, _ := dbtestutil.NewDB(t)
3593+
3594+
orgA := dbfake.Organization(t, db).Do()
3595+
3596+
userA := dbgen.User(t, db, database.User{})
3597+
userB := dbgen.User(t, db, database.User{})
3598+
userC := dbgen.User(t, db, database.User{})
3599+
3600+
dbgen.OrganizationMember(t, db, database.OrganizationMember{
3601+
OrganizationID: orgA.Org.ID,
3602+
UserID: userA.ID,
3603+
})
3604+
dbgen.OrganizationMember(t, db, database.OrganizationMember{
3605+
OrganizationID: orgA.Org.ID,
3606+
UserID: userB.ID,
3607+
})
3608+
dbgen.OrganizationMember(t, db, database.OrganizationMember{
3609+
OrganizationID: orgA.Org.ID,
3610+
UserID: userC.ID,
3611+
})
3612+
3613+
// Delete one of the users but don't remove them from the org
3614+
ctx := testutil.Context(t, testutil.WaitShort)
3615+
db.UpdateUserDeletedByID(ctx, userB.ID)
3616+
3617+
err := db.UpdateOrganizationDeletedByID(ctx, database.UpdateOrganizationDeletedByIDParams{
3618+
UpdatedAt: dbtime.Now(),
3619+
ID: orgA.Org.ID,
3620+
})
3621+
require.Error(t, err)
3622+
// cannot delete organization: organization has 1 members that must be deleted first
3623+
require.ErrorContains(t, err, "cannot delete organization")
3624+
require.ErrorContains(t, err, "has 1 members")
3625+
})
35893626
}
35903627

35913628
type templateVersionWithPreset struct {

‎coderd/database/queries.sql.go

+39-5
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/database/queries/organizations.sql

+40-5
Original file line numberDiff line numberDiff line change
@@ -73,11 +73,46 @@ WHERE
7373

7474
-- name: GetOrganizationResourceCountByID :one
7575
SELECT
76-
(SELECT COUNT(*) FROM workspaces WHERE workspaces.organization_id = $1 AND workspaces.deleted = false) AS workspace_count,
77-
(SELECT COUNT(*) FROM groups WHERE groups.organization_id = $1) AS group_count,
78-
(SELECT COUNT(*) FROM templates WHERE templates.organization_id = $1 AND templates.deleted = false) AS template_count,
79-
(SELECT COUNT(*) FROM organization_members WHERE organization_members.organization_id = $1) AS member_count,
80-
(SELECT COUNT(*) FROM provisioner_keys WHERE provisioner_keys.organization_id = $1) AS provisioner_key_count;
76+
(
77+
SELECT
78+
count(*)
79+
FROM
80+
workspaces
81+
WHERE
82+
workspaces.organization_id = $1
83+
AND workspaces.deleted = FALSE) AS workspace_count,
84+
(
85+
SELECT
86+
count(*)
87+
FROM
88+
GROUPS
89+
WHERE
90+
groups.organization_id = $1) AS group_count,
91+
(
92+
SELECT
93+
count(*)
94+
FROM
95+
templates
96+
WHERE
97+
templates.organization_id = $1
98+
AND templates.deleted = FALSE) AS template_count,
99+
(
100+
SELECT
101+
count(*)
102+
FROM
103+
organization_members
104+
LEFT JOIN users ON organization_members.user_id = users.id
105+
WHERE
106+
organization_members.organization_id = $1
107+
AND users.deleted = FALSE) AS member_count,
108+
(
109+
SELECT
110+
count(*)
111+
FROM
112+
provisioner_keys
113+
WHERE
114+
provisioner_keys.organization_id = $1) AS provisioner_key_count;
115+
81116

82117
-- name: InsertOrganization :one
83118
INSERT INTO

0 commit comments

Comments
 (0)