Skip to content

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

Merged
merged 7 commits into from
Jun 5, 2024

Conversation

Emyrk
Copy link
Member

@Emyrk Emyrk commented Jun 4, 2024

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 need rolename:<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.

Emyrk added 2 commits June 4, 2024 15:53
Organization member's table is already scoped to an organization.
Rolename should avoid having the org_id appended
@Emyrk Emyrk changed the title chore: refactor, remove organiation_id from org_member roles chore: remove organization_id suffix from org_member roles in db Jun 4, 2024
Comment on lines +2854 to +2859
// 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)
}
Copy link
Member Author

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

@Emyrk Emyrk requested a review from johnstcn June 4, 2024 21:39
Comment on lines +1 to +7
-- 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 = '{}';
Copy link
Member

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

Comment on lines 73 to 80
func StaticRoleOrgAdmin() string {
return orgAdmin
}

func StaticRoleOrgMember() string {
return orgMember
}

Copy link
Member

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()?

Copy link
Member Author

@Emyrk Emyrk Jun 5, 2024

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.

Copy link
Member Author

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Solution

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.
@Emyrk Emyrk requested a review from johnstcn June 5, 2024 15:44
Copy link
Member

@johnstcn johnstcn left a 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"
Copy link
Member

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

Copy link
Member Author

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.

@Emyrk Emyrk changed the title chore: remove organization_id suffix from org_member roles in db chore: remove organization_id suffix from org_member roles in database Jun 5, 2024
@Emyrk Emyrk merged commit 8f62311 into main Jun 5, 2024
37 checks passed
@Emyrk Emyrk deleted the stevenmasley/org_roles_db_store branch June 5, 2024 16:25
@github-actions github-actions bot locked and limited conversation to collaborators Jun 5, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants