8000 fix: improve error message when deleting organization with resources by brettkolodny · Pull Request #17049 · coder/coder · GitHub
[go: up one dir, main page]

Skip to content

fix: improve error message when deleting organization with resources #17049

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 29 additions & 0 deletions coderd/database/dbauthz/dbauthz.go
Original file line number Diff line number Diff line change
Expand Up @@ -1985,6 +1985,35 @@ func (q *querier) GetOrganizationIDsByMemberIDs(ctx context.Context, ids []uuid.
return fetchWithPostFilter(q.auth, policy.ActionRead, q.db.GetOrganizationIDsByMemberIDs)(ctx, ids)
}

func (q *querier) GetOrganizationResourceCountByID(ctx context.Context, organizationID uuid.UUID) (database.GetOrganizationResourceCountByIDRow, error) {
// Can read org members
if err := q.authorizeContext(ctx, policy.ActionRead, rbac.ResourceOrganizationMember.InOrg(organizationID)); err != nil {
return database.GetOrganizationResourceCountByIDRow{}, err
}

// Can read org workspaces
if err := q.authorizeContext(ctx, policy.ActionRead, rbac.ResourceWorkspace.InOrg(organizationID)); err != nil {
return database.GetOrganizationResourceCountByIDRow{}, err
}

// Can read org groups
if err := q.authorizeContext(ctx, policy.ActionRead, rbac.ResourceGroup.InOrg(organizationID)); err != nil {
return database.GetOrganizationResourceCountByIDRow{}, err
}

// Can read org templates
if err := q.authorizeContext(ctx, policy.ActionRead, rbac.ResourceTemplate.InOrg(organizationID)); err != nil {
return database.GetOrganizationResourceCountByIDRow{}, err
}

// Can read org provisioner daemons
if err := q.authorizeContext(ctx, policy.ActionRead, rbac.ResourceProvisionerDaemon.InOrg(organizationID)); err != nil {
return database.GetOrganizationResourceCountByIDRow{}, err
}

return q.db.GetOrganizationResourceCountByID(ctx, organizationID)
}

func (q *querier) GetOrganizations(ctx context.Context, args database.GetOrganizationsParams) ([]database.Organization, error) {
fetch := func(ctx context.Context, _ interface{}) ([]database.Organization, error) {
return q.db.GetOrganizations(ctx, args)
Expand Down
33 changes: 33 additions & 0 deletions coderd/database/dbauthz/dbauthz_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -809,6 +809,39 @@ func (s *MethodTestSuite) TestOrganization() {
o := dbgen.Organization(s.T(), db, database.Organization{})
check.Args(o.ID).Asserts(o, policy.ActionRead).Returns(o)
}))
s.Run("GetOrganizationResourceCountByID", s.Subtest(func(db database.Store, check *expects) {
u := dbgen.User(s.T(), db, database.User{})
o := dbgen.Organization(s.T(), db, database.Organization{})

t := dbgen.Template(s.T(), db, database.Template{
CreatedBy: u.ID,
OrganizationID: o.ID,
})
dbgen.Workspace(s.T(), db, database.WorkspaceTable{
OrganizationID: o.ID,
OwnerID: u.ID,
TemplateID: t.ID,
})
dbgen.Group(s.T(), db, database.Group{OrganizationID: o.ID})
dbgen.OrganizationMember(s.T(), db, database.OrganizationMember{
OrganizationID: o.ID,
UserID: u.ID,
})

check.Args(o.ID).Asserts(
rbac.ResourceOrganizationMember.InOrg(o.ID), policy.ActionRead,
rbac.ResourceWorkspace.InOrg(o.ID), policy.ActionRead,
rbac.ResourceGroup.InOrg(o.ID), policy.ActionRead,
rbac.ResourceTemplate.InOrg(o.ID), policy.ActionRead,
rbac.ResourceProvisionerDaemon.InOrg(o.ID), policy.ActionRead,
).Returns(database.GetOrganizationResourceCountByIDRow{
WorkspaceCount: 1,
GroupCount: 1,
TemplateCount: 1,
MemberCount: 1,
ProvisionerKeyCount: 0,
})
}))
s.Run("GetDefaultOrganization", s.Subtest(func(db database.Store, check *expects) {
o, _ := db.GetDefaultOrganization(context.Background())
check.Args().Asserts(o, policy.ActionRead).Returns(o)
Expand Down
48 changes: 48 additions & 0 deletions coderd/database/dbmem/dbmem.go
Original file line number Diff line number Diff line change
Expand Up @@ -3974,6 +3974,54 @@ func (q *FakeQuerier) GetOrganizationIDsByMemberIDs(_ context.Context, ids []uui
return getOrganizationIDsByMemberIDRows, nil
}

func (q *FakeQuerier) GetOrganizationResourceCountByID(_ context.Context, organizationID uuid.UUID) (database.GetOrganizationResourceCountByIDRow, error) {
q.mutex.RLock()
defer q.mutex.RUnlock()

workspacesCount := 0
for _, workspace := range q.workspaces {
if workspace.OrganizationID == organizationID {
workspacesCount++
}
}

groupsCount := 0
for _, group := range q.groups {
if group.OrganizationID == organizationID {
groupsCount++
}
}

templatesCount := 0
for _, template := range q.templates {
if template.OrganizationID == organizationID {
templatesCount++
}
}

organizationMembersCount := 0
for _, organizationMember := range q.organizationMembers {
if organizationMember.OrganizationID == organizationID {
organizationMembersCount++
}
}

provKeyCount := 0
for _, provKey := range q.provisionerKeys {
if provKey.OrganizationID == organizationID {
provKeyCount++
}
}

return database.GetOrganizationResourceCountByIDRow{
WorkspaceCount: int64(workspacesCount),
GroupCount: int64(groupsCount),
TemplateCount: int64(templatesCount),
MemberCount: int64(organizationMembersCount),
ProvisionerKeyCount: int64(provKeyCount),
}, nil
}

func (q *FakeQuerier) GetOrganizations(_ context.Context, args database.GetOrganizationsParams) ([]database.Organization, error) {
q.mutex.RLock()
defer q.mutex.RUnlock()
Expand Down
7 changes: 7 additions & 0 deletions coderd/database/dbmetrics/querymetrics.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

15 changes: 15 additions & 0 deletions coderd/database/dbmock/dbmock.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

21 changes: 20 additions & 1 deletion coderd/database/dump.sql

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it looks like right now your down migration is just a copy of your up migration.

  • up migrations should add your new things
  • down migrations should revert them

I think your down migration really only needs to be

drop trigger if exists protect_deleting_organizations on organizations;
drop function if exists protect_deleting_organizations;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I think there were some copy paste errors as I changed from another branch to this one (long story). Probably also why there is the tabs vs spaces issue.

The down migration can't just be dropping the function since the function already exists, as far as I understand I'd have to re-declare the original function from a previous migration

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually taking a closer look now the down migration is correct, or at least what I meant it to be. In it we drop the existing functions and triggers and re-create the old one from migration 296

Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
-- Drop trigger that uses this function
DROP TRIGGER IF EXISTS protect_deleting_organizations ON organizations;

-- Revert the function to its original implementation
CREATE OR REPLACE FUNCTION protect_deleting_organizations()
RETURNS TRIGGER AS
$$
DECLARE
workspace_count int;
template_count int;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

classic tabs vs spaces moment (you've got 4 spaces on one line and a tab on the next)

F438 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

goes back and forth again here, and actually a lot throughout the file 😅 I usually normalize to tabs, but I don't think we really have a consistent indentation style in sql. it is nice to pick one and stick with it within a file at least.


IF (workspace_count + template_count + provisioner_keys_count) > 0 THEN
RAISE EXCEPTION 'cannot delete organization: organization has % workspaces, % templates, and % provisioner keys that must be deleted first', workspace_count, template_count, provisioner_keys_count;
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;

-- Re-create trigger that uses this function
CREATE TRIGGER protect_deleting_organizations
BEFORE DELETE ON organizations
FOR EACH ROW
EXECUTE FUNCTION protect_deleting_organizations();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and this file also has inconsistent indentation throughout. we're really not usually picky about sql formatting, but sticking with a consistent indentation character/width is nice.

Original file line number Diff line number Diff line change
@@ -0,0 +1,96 @@
DROP TRIGGER IF EXISTS protect_deleting_organizations ON organizations;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems a bit over defensive, but also doesn't really hurt anything.

mostly I'm just wondering if you think this needs to stay or if this is just some weird thing that your little buddy dreamt up 😂


-- 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();
1 change: 1 addition & 0 deletions coderd/database/querier.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 0 additions & 1 deletion coderd/database/querier_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3396,7 +3396,6 @@ func TestOrganizationDeleteTrigger(t *testing.T) {
require.Error(t, err)
// cannot delete organization: organization has 0 workspaces and 1 templates that must be deleted first
require.ErrorContains(t, err, "cannot delete organization")
require.ErrorContains(t, err, "has 0 workspaces")
require.ErrorContains(t, err, "1 templates")
})

Expand Down
Loading
Loading
0