Skip to content

Commit 8f62311

Browse files
EmyrkEmyrk
authored
chore: remove organization_id suffix from org_member roles in database (coder#13473)
Organization member's table is already scoped to an organization. Rolename should avoid having the org_id appended. Wipes all existing organization role assignments, which should not be used anyway.
1 parent fade8ba commit 8f62311

38 files changed

Lines changed: 200 additions & 118 deletions

‎cli/server_createadminuser.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -222,7 +222,7 @@ func (r *RootCmd) newCreateAdminUserCommand() *serpent.Command {
222222
UserID: newUser.ID,
223223
CreatedAt: dbtime.Now(),
224224
UpdatedAt: dbtime.Now(),
225-
Roles: []string{rbac.RoleOrgAdmin(org.ID)},
225+
Roles: []string{rbac.ScopedRoleOrgAdmin(org.ID)},
226226
})
227227
if err != nil {
228228
return xerrors.Errorf("insert organization member: %w", err)

‎cli/server_createadminuser_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,7 @@ func TestServerCreateAdminUser(t *testing.T) {
7171
orgIDs2 := make(map[uuid.UUID]struct{}, len(orgMemberships))
7272
for _, membership := range orgMemberships {
7373
orgIDs2[membership.OrganizationID] = struct{}{}
74-
assert.Equal(t, []string{rbac.RoleOrgAdmin(membership.OrganizationID)}, membership.Roles, "user is not org admin")
74+
assert.Equal(t, []string{rbac.ScopedRoleOrgAdmin(membership.OrganizationID)}, membership.Roles, "user is not org admin")
7575
}
7676

7777
require.Equal(t, orgIDs, orgIDs2, "user is not in all orgs")

‎coderd/apidoc/docs.go‎

Lines changed: 3 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/apidoc/swagger.json‎

Lines changed: 3 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/authorize_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@ func TestCheckPermissions(t *testing.T) {
2727
memberClient, _ := coderdtest.CreateAnotherUser(t, adminClient, adminUser.OrganizationID)
2828
memberUser, err := memberClient.User(ctx, codersdk.Me)
2929
require.NoError(t, err)
30-
orgAdminClient, _ := coderdtest.CreateAnotherUser(t, adminClient, adminUser.OrganizationID, rbac.RoleOrgAdmin(adminUser.OrganizationID))
30+
orgAdminClient, _ := coderdtest.CreateAnotherUser(t, adminClient, adminUser.OrganizationID, rbac.ScopedRoleOrgAdmin(adminUser.OrganizationID))
3131
orgAdminUser, err := orgAdminClient.User(ctx, codersdk.Me)
3232
require.NoError(t, err)
3333

‎coderd/batchstats/batcher_internal_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -177,7 +177,7 @@ func setupDeps(t *testing.T, store database.Store, ps pubsub.Pubsub) deps {
177177
_, err := store.InsertOrganizationMember(context.Background(), database.InsertOrganizationMemberParams{
178178
OrganizationID: org.ID,
179179
UserID: user.ID,
180-
Roles: []string{rbac.RoleOrgMember(org.ID)},
180+
Roles: []string{rbac.ScopedRoleOrgMember(org.ID)},
181181
})
182182
require.NoError(t, err)
183183
tv := dbgen.TemplateVersion(t, store, database.TemplateVersion{

‎coderd/coderdtest/coderdtest.go‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -663,6 +663,7 @@ func CreateFirstUser(t testing.TB, client *codersdk.Client) codersdk.CreateFirst
663663
}
664664

665665
// CreateAnotherUser creates and authenticates a new user.
666+
// Roles can include org scoped roles with 'roleName:<organization_id>'
666667
func CreateAnotherUser(t testing.TB, client *codersdk.Client, organizationID uuid.UUID, roles ...string) (*codersdk.Client, codersdk.User) {
667668
return createAnotherUserRetry(t, client, organizationID, 5, roles)
668669
}
@@ -680,7 +681,7 @@ func AuthzUserSubject(user codersdk.User, orgID uuid.UUID) rbac.Subject {
680681
roles = append(roles, r.Name)
681682
}
682683
// We assume only 1 org exists
683-
roles = append(roles, rbac.RoleOrgMember(orgID))
684+
roles = append(roles, rbac.ScopedRoleOrgMember(orgID))
684685

685686
return rbac.Subject{
686687
ID: user.ID.String(),
@@ -754,6 +755,8 @@ func createAnotherUserRetry(t testing.TB, client *codersdk.Client, organizationI
754755
for _, roleName := range roles {
755756
roleName := roleName
756757
orgID, ok := rbac.IsOrgRole(roleName)
758+
roleName, _, err = rbac.RoleSplit(roleName)
759+
require.NoError(t, err, "split org role name")
757760
if ok {
758761
orgRoles[orgID] = append(orgRoles[orgID], roleName)
759762
} else {

‎coderd/database/db2sdk/db2sdk.go‎

Lines changed: 13 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -204,13 +204,6 @@ func Group(group database.Group, members []database.User) codersdk.Group {
204204
}
205205
}
206206

207-
func SlimRole(role rbac.Role) codersdk.SlimRole {
208-
return codersdk.SlimRole{
209-
DisplayName: role.DisplayName,
210-
Name: role.Name,
211-
}
212-
}
213-
214207
func TemplateInsightsParameters(parameterRows []database.GetTemplateParameterInsightsRow) ([]codersdk.TemplateParameterUsage, error) {
215208
// Use a stable sort, similarly to how we would sort in the query, note that
216209
// we don't sort in the query because order varies depending on the table
@@ -525,6 +518,19 @@ func ProvisionerDaemon(dbDaemon database.ProvisionerDaemon) codersdk.Provisioner
525518
return result
526519
}
527520

521+
func SlimRole(role rbac.Role) codersdk.SlimRole {
522+
roleName, orgIDStr, err := rbac.RoleSplit(role.Name)
523+
if err != nil {
524+
roleName = role.Name
525+
}
526+
527+
return codersdk.SlimRole{
528+
DisplayName: role.DisplayName,
529+
Name: roleName,
530+
OrganizationID: orgIDStr,
531+
}
532+
}
533+
528534
func RBACRole(role rbac.Role) codersdk.Role {
529535
roleName, orgIDStr, err := rbac.RoleSplit(role.Name)
530536
if err != nil {

‎coderd/database/dbauthz/customroles_test.go‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -153,7 +153,7 @@ func TestUpsertCustomRoles(t *testing.T) {
153153
UUID: uuid.New(),
154154
Valid: true,
155155
},
156-
subject: merge(canAssignRole, rbac.RoleOrgAdmin(orgID.UUID)),
156+
subject: merge(canAssignRole, rbac.ScopedRoleOrgAdmin(orgID.UUID)),
157157
org: codersdk.CreatePermissions(map[codersdk.RBACResource][]codersdk.RBACAction{
158158
codersdk.ResourceWorkspace: {codersdk.ActionRead},
159159
}),
@@ -162,7 +162,7 @@ func TestUpsertCustomRoles(t *testing.T) {
162162
{
163163
name: "user-escalation",
164164
// These roles do not grant user perms
165-
subject: merge(canAssignRole, rbac.RoleOrgAdmin(orgID.UUID)),
165+
subject: merge(canAssignRole, rbac.ScopedRoleOrgAdmin(orgID.UUID)),
166166
user: codersdk.CreatePermissions(map[codersdk.RBACResource][]codersdk.RBACAction{
167167
codersdk.ResourceWorkspace: {codersdk.ActionRead},
168168
}),
@@ -190,7 +190,7 @@ func TestUpsertCustomRoles(t *testing.T) {
190190
},
191191
{
192192
name: "read-workspace-in-org",
193-
subject: merge(canAssignRole, rbac.RoleOrgAdmin(orgID.UUID)),
193+
subject: merge(canAssignRole, rbac.ScopedRoleOrgAdmin(orgID.UUID)),
194194
organizationID: orgID,
195195
org: codersdk.CreatePermissions(map[codersdk.RBACResource][]codersdk.RBACAction{
196196
codersdk.ResourceWorkspace: {codersdk.ActionRead},

‎coderd/database/dbauthz/dbauthz.go‎

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2472,7 +2472,7 @@ func (q *querier) InsertOrganization(ctx context.Context, arg database.InsertOrg
24722472

24732473
func (q *querier) InsertOrganizationMember(ctx context.Context, arg database.InsertOrganizationMemberParams) (database.OrganizationMember, error) {
24742474
// All roles are added roles. Org member is always implied.
2475-
addedRoles := append(arg.Roles, rbac.RoleOrgMember(arg.OrganizationID))
2475+
addedRoles := append(arg.Roles, rbac.ScopedRoleOrgMember(arg.OrganizationID))
24762476
err := q.canAssignRoles(ctx, &arg.OrganizationID, addedRoles, []string{})
24772477
if err != nil {
24782478
return database.OrganizationMember{}, err
@@ -2847,8 +2847,22 @@ func (q *querier) UpdateMemberRoles(ctx context.Context, arg database.UpdateMemb
28472847
return database.OrganizationMember{}, err
28482848
}
28492849

2850+
// The 'rbac' package expects role names to be scoped.
2851+
// Convert the argument roles for validation.
2852+
scopedGranted := make([]string, 0, len(arg.GrantedRoles))
2853+
for _, grantedRole := range arg.GrantedRoles {
2854+
// This check is a developer safety check. Old code might try to invoke this code path with
2855+
// organization id suffixes. Catch this and return a nice error so it can be fixed.
2856+
_, foundOrg, _ := rbac.RoleSplit(grantedRole)
2857+
if foundOrg != "" {
2858+
return database.OrganizationMember{}, xerrors.Errorf("attempt to assign a role %q, remove the ':<organization_id> suffix", grantedRole)
2859+
}
2860+
2861+
scopedGranted = append(scopedGranted, rbac.RoleName(grantedRole, arg.OrgID.String()))
2862+
}
2863+
28502864
// The org member role is always implied.
2851-
impliedTypes := append(arg.GrantedRoles, rbac.RoleOrgMember(arg.OrgID))
2865+
impliedTypes := append(scopedGranted, rbac.ScopedRoleOrgMember(arg.OrgID))
28522866
added, removed := rbac.ChangeRoleSet(member.Roles, impliedTypes)
28532867
err = q.canAssignRoles(ctx, &arg.OrgID, added, removed)
28542868
if err != nil {

0 commit comments

Comments
 (0)