- Notifications
You must be signed in to change notification settings - Fork927
chore: remove organization_id suffix from org_member roles in database#13473
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
Organization member's table is already scoped to an organization.Rolename should avoid having the org_id appended
| // 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This is not required, but added as a safety check to catch dev errors from using the previousrolename:<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 = '{}'; |
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.
another good thing to call out
coderd/rbac/roles.go Outdated
| func StaticRoleOrgAdmin() string { | ||
| return orgAdmin | ||
| } | ||
| func StaticRoleOrgMember() string { | ||
| return orgMember | ||
| } | ||
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.
What's the difference between "Static" org admin andRoleOrgAdmin()?
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.
This is super annoying, but essentially I should makeRoleOrgAdmin(<org_id>) not take an organization ID. But there is >80 usages ofRoleOrgAdmin() andRoleOrgMember(). 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, butStatic is just the rolename without the org id. The original function is dynamic.
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Solution
Lines 73 to 94 incf93307
| funcRoleOrgAdmin()string { | |
| returnorgAdmin | |
| } | |
| funcRoleOrgMember()string { | |
| returnorgMember | |
| } | |
| // ScopedRoleOrgAdmin is the org role with the organization ID | |
| // Deprecated This was used before organization scope was included as a | |
| // field in all user facing APIs. Usage of 'ScopedRoleOrgAdmin()' is preferred. | |
| funcScopedRoleOrgAdmin(organizationID uuid.UUID)string { | |
| returnRoleName(orgAdmin,organizationID.String()) | |
| } | |
| // ScopedRoleOrgMember is the org role with the organization ID | |
| // Deprecated This was used before organization scope was included as a | |
| // field in all user facing APIs. Usage of 'ScopedRoleOrgMember()' is preferred. | |
| funcScopedRoleOrgMember(organizationID uuid.UUID)string { | |
| returnRoleName(orgMember,organizationID.String()) | |
| } | |
Fixing all the test apis to remove this is a lot of work for littlereturn atm.
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.
LGTM, but we should have a follow-up to remove the usage of these deprecatedScopedRole.* functions. Also it might be no harm to have some comments on theorganization_id fields in our API docs regarding the current experimental status.
| @@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I wish the docs would read theomitempty 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.
8f62311 intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
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.