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

+200
-118
lines changed

‎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 {

‎coderd/database/dbauthz/dbauthz_test.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -636,7 +636,7 @@ func (s *MethodTestSuite) TestOrganization() {
636636
check.Args(database.InsertOrganizationMemberParams{
637637
OrganizationID: o.ID,
638638
UserID: u.ID,
639-
Roles: []string{rbac.RoleOrgAdmin(o.ID)},
639+
Roles: []string{rbac.ScopedRoleOrgAdmin(o.ID)},
640640
}).Asserts(
641641
rbac.ResourceAssignRole.InOrg(o.ID), policy.ActionAssign,
642642
rbac.ResourceOrganizationMember.InOrg(o.ID).WithID(u.ID), policy.ActionCreate)
@@ -664,7 +664,7 @@ func (s *MethodTestSuite) TestOrganization() {
664664
mem := dbgen.OrganizationMember(s.T(), db, database.OrganizationMember{
665665
OrganizationID: o.ID,
666666
UserID: u.ID,
667-
Roles: []string{rbac.RoleOrgAdmin(o.ID)},
667+
Roles: []string{rbac.ScopedRoleOrgAdmin(o.ID)},
668668
})
669669
out := mem
670670
out.Roles = []string{}

‎coderd/database/dbmem/dbmem.go

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1997,7 +1997,9 @@ func (q *FakeQuerier) GetAuthorizationUserRoles(_ context.Context, userID uuid.U
19971997

19981998
for _, mem := range q.organizationMembers {
19991999
if mem.UserID == userID {
2000-
roles = append(roles, mem.Roles...)
2000+
for _, orgRole := range mem.Roles {
2001+
roles = append(roles, orgRole+":"+mem.OrganizationID.String())
2002+
}
20012003
roles = append(roles, "organization-member:"+mem.OrganizationID.String())
20022004
}
20032005
}

‎coderd/database/dump.sql

Lines changed: 1 addition & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/database/migrations/000215_scoped_org_db_roles.down.sql

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
ALTER TABLE ONLY organization_members ALTER COLUMN roles SET DEFAULT '{organization-member}';

‎coderd/database/migrations/000215_scoped_org_db_roles.up.sql

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
-- The default was 'organization-member', but we imply that in the
2+
-- 'GetAuthorizationUserRoles' query.
3+
ALTER TABLE ONLY organization_members ALTER COLUMN roles SET DEFAULT '{}';
4+
5+
-- No one should be using organization roles yet. If they are, the names in the
6+
-- database are now incorrect. Just remove them all.
7+
UPDATE organization_members SET roles = '{}';

‎coderd/database/queries.sql.go

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

‎coderd/database/queries/users.sql

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -227,12 +227,14 @@ SELECT
227227
array_append(users.rbac_roles, 'member'),
228228
(
229229
SELECT
230-
array_agg(org_roles)
230+
-- The roles are returned as a flat array, org scoped and site side.
231+
-- Concatenating the organization id scopes the organization roles.
232+
array_agg(org_roles || ':' || organization_members.organization_id::text)
231233
FROM
232234
organization_members,
233-
-- All org_members get the org-member role for their orgs
235+
-- All org_members get the organization-member role for their orgs
234236
unnest(
235-
array_append(roles, 'organization-member:' || organization_members.organization_id::text)
237+
array_append(roles, 'organization-member')
236238
) AS org_roles
237239
WHERE
238240
user_id = users.id

‎coderd/httpmw/authorize_test.go

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,7 @@ func TestExtractUserRoles(t *testing.T) {
6868
Roles: orgRoles,
6969
})
7070
require.NoError(t, err)
71-
return user, append(roles, append(orgRoles, rbac.RoleMember(), rbac.RoleOrgMember(org.ID))...), token
71+
return user, append(roles, append(orgRoles, rbac.RoleMember(), rbac.ScopedRoleOrgMember(org.ID))...), token
7272
},
7373
},
7474
{
@@ -89,7 +89,8 @@ func TestExtractUserRoles(t *testing.T) {
8989

9090
orgRoles := []string{}
9191
if i%2 == 0 {
92-
orgRoles = append(orgRoles, rbac.RoleOrgAdmin(organization.ID))
92+
orgRoles = append(orgRoles, rbac.RoleOrgAdmin())
93+
roles = append(roles, rbac.ScopedRoleOrgAdmin(organization.ID))
9394
}
9495
_, err = db.InsertOrganizationMember(context.Background(), database.InsertOrganizationMemberParams{
9596
OrganizationID: organization.ID,
@@ -99,8 +100,7 @@ func TestExtractUserRoles(t *testing.T) {
99100
Roles: orgRoles,
100101
})
101102
require.NoError(t, err)
102-
roles = append(roles, orgRoles...)
103-
roles = append(roles, rbac.RoleOrgMember(organization.ID))
103+
roles = append(roles, rbac.ScopedRoleOrgMember(organization.ID))
104104
}
105105
return user, roles, token
106106
},

‎coderd/httpmw/organizationparam_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -152,7 +152,7 @@ func TestOrganizationParam(t *testing.T) {
152152
_ = dbgen.OrganizationMember(t, db, database.OrganizationMember{
153153
OrganizationID: organization.ID,
154154
UserID: user.ID,
155-
Roles: []string{rbac.RoleOrgMember(organization.ID)},
155+
Roles: []string{rbac.ScopedRoleOrgMember(organization.ID)},
156156
})
157157
_, err := db.UpdateUserRoles(ctx, database.UpdateUserRolesParams{
158158
ID: user.ID,

‎coderd/members.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,7 @@ func convertOrganizationMember(mem database.OrganizationMember) codersdk.Organiz
6868
}
6969

7070
for _, roleName := range mem.Roles {
71-
rbacRole, _ := rbac.RoleByName(roleName)
71+
rbacRole, _ := rbac.RoleByName(rbac.RoleName(roleName, mem.OrganizationID.String()))
7272
convertedMember.Roles = append(convertedMember.Roles, db2sdk.SlimRole(rbacRole))
7373
}
7474
return convertedMember

‎coderd/organizations.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -94,7 +94,7 @@ func (api *API) postOrganizations(rw http.ResponseWriter, r *http.Request) {
9494
// come back to determining the default role of the person who
9595
// creates the org. Until that happens, all users in an organization
9696
// should be just regular members.
97-
rbac.RoleOrgMember(organization.ID),
97+
rbac.ScopedRoleOrgMember(organization.ID),
9898
},
9999
})
100100
if err != nil {

0 commit comments

Comments
 (0)