chore: remove organization_id suffix from org_member roles in database #13473
Conversation
Organization member's table is already scoped to an organization. Rolename should avoid having the org_id appended
Emyrk
changed the title
| // This check is a developer safety check. Old code might try to invoke this code path with | ||
| // organization id suffixes. Catch this and return a nice error so it can be fixed. | ||
| _, foundOrg, _ := rbac.RoleSplit(grantedRole) | ||
| if foundOrg != "" { | ||
| return database.OrganizationMember{}, xerrors.Errorf("attempt to assign a role %q, remove the ':<organization_id> suffix", grantedRole) | ||
| } |
There was a problem hiding this comment.
This is not required, but added as a safety check to catch dev errors from using the previous rolename:<org_id> syntax
| -- The default was 'organization-member', but we imply that in the | ||
| -- 'GetAuthorizationUserRoles' query. | ||
| ALTER TABLE ONLY organization_members ALTER COLUMN roles SET DEFAULT '{}'; | ||
|
|
||
| -- No one should be using organization roles yet. If they are, the names in the | ||
| -- database are now incorrect. Just remove them all. | ||
| UPDATE organization_members SET roles = '{}'; |
coderd/rbac/roles.go
Outdated
| func StaticRoleOrgAdmin() string { | ||
| return orgAdmin | ||
| } | ||
|
|
||
| func StaticRoleOrgMember() string { | ||
| return orgMember | ||
| } | ||
|
|
There was a problem hiding this comment.
What's the difference between "Static" org admin and RoleOrgAdmin()?
There was a problem hiding this comment.
This is super annoying, but essentially I should make RoleOrgAdmin(<org_id>) not take an organization ID. But there is >80 usages of RoleOrgAdmin() and RoleOrgMember(). All our test APIs just take []string{} for roles, but now they should take, []string, map[<org_id>][]string.
That kind of refactor would be a much larger diff, and this would only serve to affect unit tests. I'll add a comment, but Static is just the rolename without the org id. The original function is dynamic.
There was a problem hiding this comment.
I have another idea. I will rename the old ones to something else and put up a deprecated comment.
There was a problem hiding this comment.
Solution
Lines 73 to 94 in cf93307
Fixing all the test apis to remove this is a lot of work for little return atm.
| @@ -1600,7 +1600,8 @@ curl -X PATCH http://coder-server:8080/api/v2/scim/v2/Users/{id} \ | |||
| "roles": [ | |||
| { | |||
| "display_name": "string", | |||
| "name": "string" | |||
| "name": "string", | |||
| "organization_id": "string" | |||
There was a problem hiding this comment.
We should probably also drop a comment here regarding the experimental status of this thing until it's ready for prime time
There was a problem hiding this comment.
I wish the docs would read the omitempty field like our type generator does. No organization roles are currently returned by any api in use by our frontend, so the field is always excluded in the json responses.
Emyrk
changed the title
Any existing organization role assignments are wiped. No one should be using these
Organization member's table is already scoped to an organization.
Rolename should avoid having the org_id appended.
When adding custom roles, this makes more sense to keep role names consistent across different tables. The only awkward thing is that in
coderdtestwe pass site and org roles as strings. So these strings still needrolename:<org_id>for the function to know which roles are org scoped, and which are site for assigning. This is only in tests though, which can use the rbac rolename helper functions.I did change the inputs from
organization-admin:432475a4-6ca2-4a33-b8f7-2b26e8aa729d->organization-admin. I added a new error to tell the caller what the change they need to make it. We only used this in unit tests. No UI, no cli, and no one should be using this endpoint, so it is not a breaking change.The custom roles effort is adding
OrganizationIDas a field on orgs, rather than a naming convention.