- Notifications
You must be signed in to change notification settings - Fork927
chore: implement deleting custom roles#14101
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Emyrk commentedAug 1, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
This stack of pull requests is managed by Graphite.Learn more about stacking. |
31f7fe5 tocb91471Compare43cbd73 to938634dComparecb91471 to9225eb1Compare9225eb1 to2aa9875CompareUh oh!
There was an error while loading.Please reload this page.
9d2a42f tof4739c9CompareThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
lgtm!
enterprise/coderd/roles.go Outdated
| OrganizationID: organization.ID, | ||
| }, | ||
| }, | ||
| ExcludeOrgRoles: false, | ||
| OrganizationID: organization.ID, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
weird duplication ofOrganizationID 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 doinguuid.Nil. But you have to do something, or the linter yells at you 😢
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
tldr on why this field can't be removed?
| httpapi.InternalServerError(rw, err) | ||
| return | ||
| } | ||
| aReq.New = database.CustomRole{} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
clever 😄
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
| -- 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 deletinguser 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
tldr on why this field can't be removed?
Uh oh!
There was an error while loading.Please reload this page.
e72061a toe8f57feCompare
We have a linter that requires all struct fields be explicitly set. So you could put |
2c13797 intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
When deleting a custom role, the role can still be assigned to a user.