chore: implement deleting custom roles #14101
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
31f7fe5 to
cb91471
Compare
43cbd73 to
938634d
Compare
cb91471 to
9225eb1
Compare
9225eb1 to
2aa9875
Compare
9d2a42f to
f4739c9
Compare
enterprise/coderd/roles.go
Outdated
| OrganizationID: organization.ID, | ||
| }, | ||
| }, | ||
| ExcludeOrgRoles: false, | ||
| OrganizationID: organization.ID, |
There was a problem hiding this comment.
weird duplication of OrganizationID 🤔
There was a problem hiding this comment.
Yea, it actually is not necessary, but we have a linter that makes you fill out all fields in a struct. This felt more intuitive that doing uuid.Nil. But you have to do something, or the linter yells at you 😢
There was a problem hiding this comment.
tldr on why this field can't be removed?
| httpapi.InternalServerError(rw, err) | ||
| return | ||
| } | ||
| aReq.New = database.CustomRole{} |
| -- When a custom role is deleted, we need to remove the assigned role | ||
| -- from all organization members that have it. | ||
| -- This action cannot be reverted, so deleting a custom role should be | ||
| -- done with caution. | ||
| CREATE OR REPLACE FUNCTION remove_organization_member_role() | ||
| RETURNS TRIGGER AS | ||
| $$ | ||
| BEGIN | ||
| -- Delete the role from all organization members that have it. | ||
| -- TODO: When site wide custom roles are supported, if the | ||
| -- organization_id is null, we should remove the role from the 'users' | ||
| -- table instead. | ||
| IF OLD.organization_id IS NOT NULL THEN | ||
| UPDATE organization_members | ||
| -- this is a noop if the role is not assigned to the member | ||
| SET roles = array_remove(roles, OLD.name) | ||
| WHERE | ||
| -- Scope to the correct organization | ||
| organization_members.organization_id = OLD.organization_id; | ||
| END IF; | ||
| RETURN OLD; | ||
| END; | ||
| $$ LANGUAGE plpgsql; | ||
|
|
||
|
|
||
| -- Attach the function to deleting the custom role | ||
| CREATE TRIGGER remove_organization_member_custom_role | ||
| BEFORE DELETE ON custom_roles FOR EACH ROW | ||
| EXECUTE PROCEDURE remove_organization_member_role(); | ||
|
|
||
|
|
||
| COMMENT ON TRIGGER | ||
| remove_organization_member_custom_role | ||
| ON custom_roles IS | ||
| 'When a custom_role is deleted, this trigger removes the role from all organization members.'; |
There was a problem hiding this comment.
I generally prefer this logic to live in app code in some sort of helper function, probably in a tx. I don't think this has to be changed here since I'm not sure how the team at large feels about this kind of design, but wanted to point out that I think it's easier to follow execution flow and also easier to update the behavior as well since changing this behavior now requires knowledge of sql triggers.
I see the benefits from a data integrity standpoint. Does this serve as some sort of performance advantage as well?
There was a problem hiding this comment.
Yea, I generally agree with you. In this case the data integrity is a security critical thing though.
Currently when making a new custom_role, you have to have a superset of the permissions to make it. So you cannot make a role that has greater privilege than yourself.
You could imagine a scenario where a role is created with a small set of perms, then deleted. Then a new role is created with the same name that has more permissions, and because a user had the old role, they are granted this new one.
At present, this would be less malicious, and more likely a mistake based on our roles, but the problem still remains.
So essentially, I choose the trigger as a security guarantee. We have some prior art for this for deleting user api keys.
A golang helper function just feels less safe imo.
enterprise/coderd/roles.go
Outdated
| OrganizationID: organization.ID, | ||
| }, | ||
| }, | ||
| ExcludeOrgRoles: false, | ||
| OrganizationID: organization.ID, |
There was a problem hiding this comment.
tldr on why this field can't be removed?
e72061a to
e8f57fe
Compare
We have a linter that requires all struct fields be explicitly set. So you could put |
When deleting a custom role, the role can still be assigned to a user.