-
Notifications
You must be signed in to change notification settings - Fork 937
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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 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 = '{}'; |
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 and RoleOrgAdmin()
?
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 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.
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 in cf93307
func RoleOrgAdmin() string { | |
return orgAdmin | |
} | |
func RoleOrgMember() string { | |
return orgMember | |
} | |
// 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. | |
func ScopedRoleOrgAdmin(organizationID uuid.UUID) string { | |
return RoleName(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. | |
func ScopedRoleOrgMember(organizationID uuid.UUID) string { | |
return RoleName(orgMember, organizationID.String()) | |
} | |
Fixing all the test apis to remove this is a lot of work for little return 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 deprecated ScopedRole.*
functions. Also it might be no harm to have some comments on the organization_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 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.
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
coderdtest
we 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
OrganizationID
as a field on orgs, rather than a naming convention.