Skip to content

Commit caeff49

Browse files
aslilacaslilac
authored
chore: refactor roles to support multiple permission sets scoped by org id (coder#20186)
In preparation for adding the "member" permission level, which will also be grouped by org ID, do a bit of a refactor to make room for it and the existing "org" level to live in the same `map`
1 parent 6213b30 commit caeff49

15 files changed

Lines changed: 234 additions & 203 deletions

File tree

‎coderd/database/db2sdk/db2sdk.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -693,13 +693,13 @@ func SlimRoleFromName(name string) codersdk.SlimRole {
693693
func RBACRole(role rbac.Role) codersdk.Role {
694694
slim := SlimRole(role)
695695

696-
orgPerms := role.Org[slim.OrganizationID]
696+
orgPerms := role.ByOrgID[slim.OrganizationID]
697697
return codersdk.Role{
698698
Name: slim.Name,
699699
OrganizationID: slim.OrganizationID,
700700
DisplayName: slim.DisplayName,
701701
SitePermissions: List(role.Site, RBACPermission),
702-
OrganizationPermissions: List(orgPerms, RBACPermission),
702+
OrganizationPermissions: List(orgPerms.Org, RBACPermission),
703703
UserPermissions: List(role.User, RBACPermission),
704704
}
705705
}

‎coderd/database/dbauthz/dbauthz.go‎

Lines changed: 33 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -232,8 +232,8 @@ var (
232232
// Provisionerd creates usage events
233233
rbac.ResourceUsageEvent.Type: {policy.ActionCreate},
234234
}),
235-
Org: map[string][]rbac.Permission{},
236-
User: []rbac.Permission{},
235+
User: []rbac.Permission{},
236+
ByOrgID: map[string]rbac.OrgPermissions{},
237237
},
238238
}),
239239
Scope: rbac.ScopeAll,
@@ -257,8 +257,8 @@ var (
257257
rbac.ResourceWorkspace.Type: {policy.ActionDelete, policy.ActionRead, policy.ActionUpdate, policy.ActionWorkspaceStart, policy.ActionWorkspaceStop},
258258
rbac.ResourceWorkspaceDormant.Type: {policy.ActionDelete, policy.ActionRead, policy.ActionUpdate, policy.ActionWorkspaceStop},
259259
}),
260-
Org: map[string][]rbac.Permission{},
261-
User: []rbac.Permission{},
260+
User: []rbac.Permission{},
261+
ByOrgID: map[string]rbac.OrgPermissions{},
262262
},
263263
}),
264264
Scope: rbac.ScopeAll,
@@ -280,8 +280,8 @@ var (
280280
rbac.ResourceWorkspaceDormant.Type: {policy.ActionRead, policy.ActionUpdate},
281281
rbac.ResourceProvisionerJobs.Type: {policy.ActionRead, policy.ActionUpdate},
282282
}),
283-
Org: map[string][]rbac.Permission{},
284-
User: []rbac.Permission{},
283+
User: []rbac.Permission{},
284+
ByOrgID: map[string]rbac.OrgPermissions{},
285285
},
286286
}),
287287
Scope: rbac.ScopeAll,
@@ -299,8 +299,8 @@ var (
299299
Site: rbac.Permissions(map[string][]policy.Action{
300300
rbac.ResourceCryptoKey.Type: {policy.WildcardSymbol},
301301
}),
302-
Org: map[string][]rbac.Permission{},
303-
User: []rbac.Permission{},
302+
User: []rbac.Permission{},
303+
ByOrgID: map[string]rbac.OrgPermissions{},
304304
},
305305
}),
306306
Scope: rbac.ScopeAll,
@@ -318,8 +318,8 @@ var (
318318
Site: rbac.Permissions(map[string][]policy.Action{
319319
rbac.ResourceCryptoKey.Type: {policy.WildcardSymbol},
320320
}),
321-
Org: map[string][]rbac.Permission{},
322-
User: []rbac.Permission{},
321+
User: []rbac.Permission{},
322+
ByOrgID: map[string]rbac.OrgPermissions{},
323323
},
324324
}),
325325
Scope: rbac.ScopeAll,
@@ -336,8 +336,8 @@ var (
336336
Site: rbac.Permissions(map[string][]policy.Action{
337337
rbac.ResourceConnectionLog.Type: {policy.ActionUpdate, policy.ActionRead},
338338
}),
339-
Org: map[string][]rbac.Permission{},
340-
User: []rbac.Permission{},
339+
User: []rbac.Permission{},
340+
ByOrgID: map[string]rbac.OrgPermissions{},
341341
},
342342
}),
343343
Scope: rbac.ScopeAll,
@@ -357,8 +357,8 @@ var (
357357
rbac.ResourceWebpushSubscription.Type: {policy.ActionCreate, policy.ActionRead, policy.ActionUpdate, policy.ActionDelete},
358358
rbac.ResourceDeploymentConfig.Type: {policy.ActionRead, policy.ActionUpdate}, // To read and upsert VAPID keys
359359
}),
360-
Org: map[string][]rbac.Permission{},
361-
User: []rbac.Permission{},
360+
User: []rbac.Permission{},
361+
ByOrgID: map[string]rbac.OrgPermissions{},
362362
},
363363
}),
364364
Scope: rbac.ScopeAll,
@@ -376,8 +376,8 @@ var (
376376
// The workspace monitor needs to be able to update monitors
377377
rbac.ResourceWorkspaceAgentResourceMonitor.Type: {policy.ActionUpdate},
378378
}),
379-
Org: map[string][]rbac.Permission{},
380-
User: []rbac.Permission{},
379+
User: []rbac.Permission{},
380+
ByOrgID: map[string]rbac.OrgPermissions{},
381381
},
382382
}),
383383
Scope: rbac.ScopeAll,
@@ -393,12 +393,12 @@ var (
393393
Identifier: rbac.RoleIdentifier{Name: "subagentapi"},
394394
DisplayName: "Sub Agent API",
395395
Site: []rbac.Permission{},
396-
Org: map[string][]rbac.Permission{
397-
orgID.String(): {},
398-
},
399396
User: rbac.Permissions(map[string][]policy.Action{
400397
rbac.ResourceWorkspace.Type: {policy.ActionRead, policy.ActionUpdate, policy.ActionCreateAgent, policy.ActionDeleteAgent},
401398
}),
399+
ByOrgID: map[string]rbac.OrgPermissions{
400+
orgID.String(): {},
401+
},
402402
},
403403
}),
404404
Scope: rbac.ScopeAll,
@@ -437,8 +437,8 @@ var (
437437
rbac.ResourceOauth2App.Type: {policy.ActionCreate, policy.ActionRead, policy.ActionUpdate, policy.ActionDelete},
438438
rbac.ResourceOauth2AppSecret.Type: {policy.ActionCreate, policy.ActionRead, policy.ActionUpdate, policy.ActionDelete},
439439
}),
440-
Org: map[string][]rbac.Permission{},
441-
User: []rbac.Permission{},
440+
User: []rbac.Permission{},
441+
ByOrgID: map[string]rbac.OrgPermissions{},
442442
},
443443
}),
444444
Scope: rbac.ScopeAll,
@@ -455,8 +455,8 @@ var (
455455
Site: rbac.Permissions(map[string][]policy.Action{
456456
rbac.ResourceProvisionerDaemon.Type: {policy.ActionRead},
457457
}),
458-
Org: map[string][]rbac.Permission{},
459-
User: []rbac.Permission{},
458+
User: []rbac.Permission{},
459+
ByOrgID: map[string]rbac.OrgPermissions{},
460460
},
461461
}),
462462
Scope: rbac.ScopeAll,
@@ -532,8 +532,8 @@ var (
532532
Site: rbac.Permissions(map[string][]policy.Action{
533533
rbac.ResourceFile.Type: {policy.ActionRead},
534534
}),
535-
Org: map[string][]rbac.Permission{},
536-
User: []rbac.Permission{},
535+
User: []rbac.Permission{},
536+
ByOrgID: map[string]rbac.OrgPermissions{},
537537
},
538538
}),
539539
Scope: rbac.ScopeAll,
@@ -553,8 +553,8 @@ var (
553553
// reads/processes them.
554554
rbac.ResourceUsageEvent.Type: {policy.ActionRead, policy.ActionUpdate},
555555
}),
556-
Org: map[string][]rbac.Permission{},
557-
User: []rbac.Permission{},
556+
User: []rbac.Permission{},
557+
ByOrgID: map[string]rbac.OrgPermissions{},
558558
},
559559
}),
560560
Scope: rbac.ScopeAll,
@@ -577,8 +577,8 @@ var (
577577
rbac.ResourceApiKey.Type: {policy.ActionRead}, // Validate API keys.
578578
rbac.ResourceAibridgeInterception.Type: {policy.ActionCreate, policy.ActionRead, policy.ActionUpdate},
579579
}),
580-
Org: map[string][]rbac.Permission{},
581-
User: []rbac.Permission{},
580+
User: []rbac.Permission{},
581+
ByOrgID: map[string]rbac.OrgPermissions{},
582582
},
583583
}),
584584
Scope: rbac.ScopeAll,
@@ -1254,13 +1254,13 @@ func (q *querier) customRoleCheck(ctx context.Context, role database.CustomRole)
12541254
return xerrors.Errorf("invalid role: %w", err)
12551255
}
12561256

1257-
if len(rbacRole.Org) > 0 && len(rbacRole.Site) > 0 {
1257+
if len(rbacRole.ByOrgID) > 0 && len(rbacRole.Site) > 0 {
12581258
// This is a choice to keep roles simple. If we allow mixing site and org scoped perms, then knowing who can
12591259
// do what gets more complicated.
12601260
return xerrors.Errorf("invalid custom role, cannot assign both org and site permissions at the same time")
12611261
}
12621262

1263-
if len(rbacRole.Org) > 1 {
1263+
if len(rbacRole.ByOrgID) > 1 {
12641264
// Again to avoid more complexity in our roles
12651265
return xerrors.Errorf("invalid custom role, cannot assign permissions to more than 1 org at a time")
12661266
}
@@ -1273,8 +1273,8 @@ func (q *querier) customRoleCheck(ctx context.Context, role database.CustomRole)
12731273
}
12741274
}
12751275

1276-
for orgID, perms := range rbacRole.Org {
1277-
for _, orgPerm := range perms {
1276+
for orgID, perms := range rbacRole.ByOrgID {
1277+
for _, orgPerm := range perms.Org {
12781278
err := q.customRoleEscalationCheck(ctx, act, orgPerm, rbac.Object{OrgID: orgID, Type: orgPerm.ResourceType})
12791279
if err != nil {
12801280
return xerrors.Errorf("org=%q: %w", orgID, err)

‎coderd/database/modelmethods.go‎

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -176,8 +176,8 @@ func (s APIKeyScopes) expandRBACScope() (rbac.Scope, error) {
176176
// Identifier is informational; not used in policy evaluation.
177177
Identifier: rbac.RoleIdentifier{Name: "Scope_Multiple"},
178178
Site: nil,
179-
Org: map[string][]rbac.Permission{},
180179
User: nil,
180+
ByOrgID: map[string]rbac.OrgPermissions{},
181181
}
182182

183183
// Collect allow lists for a union after expanding all scopes.
@@ -191,8 +191,10 @@ func (s APIKeyScopes) expandRBACScope() (rbac.Scope, error) {
191191

192192
// Merge role permissions: union by simple concatenation.
193193
merged.Site = append(merged.Site, expanded.Site...)
194-
for orgID, perms := range expanded.Org {
195-
merged.Org[orgID] = append(merged.Org[orgID], perms...)
194+
for orgID, perms := range expanded.ByOrgID {
195+
orgPerms := merged.ByOrgID[orgID]
196+
orgPerms.Org = append(orgPerms.Org, perms.Org...)
197+
merged.ByOrgID[orgID] = orgPerms
196198
}
197199
merged.User = append(merged.User, expanded.User...)
198200

@@ -201,10 +203,11 @@ func (s APIKeyScopes) expandRBACScope() (rbac.Scope, error) {
201203

202204
// De-duplicate permissions across Site/Org/User
203205
merged.Site = rbac.DeduplicatePermissions(merged.Site)
204-
for orgID, perms := range merged.Org {
205-
merged.Org[orgID] = rbac.DeduplicatePermissions(perms)
206-
}
207206
merged.User = rbac.DeduplicatePermissions(merged.User)
207+
for orgID, perms := range merged.ByOrgID {
208+
perms.Org = rbac.DeduplicatePermissions(perms.Org)
209+
merged.ByOrgID[orgID] = perms
210+
}
208211

209212
union, err := rbac.UnionAllowLists(allowLists...)
210213
if err != nil {

‎coderd/rbac/astvalue.go‎

Lines changed: 14 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -157,23 +157,30 @@ func (role Role) regoValue() ast.Value {
157157
if role.cachedRegoValue != nil {
158158
return role.cachedRegoValue
159159
}
160-
orgMap := ast.NewObject()
161-
for k, p := range role.Org {
162-
orgMap.Insert(ast.StringTerm(k), ast.NewTerm(regoSlice(p)))
160+
byOrgIDMap := ast.NewObject()
161+
for k, p := range role.ByOrgID {
162+
byOrgIDMap.Insert(ast.StringTerm(k), ast.NewTerm(
163+
ast.NewObject(
164+
[2]*ast.Term{
165+
ast.StringTerm("org"),
166+
ast.NewTerm(regoSlice(p.Org)),
167+
},
168+
),
169+
))
163170
}
164171
return ast.NewObject(
165172
[2]*ast.Term{
166173
ast.StringTerm("site"),
167174
ast.NewTerm(regoSlice(role.Site)),
168175
},
169-
[2]*ast.Term{
170-
ast.StringTerm("org"),
171-
ast.NewTerm(orgMap),
172-
},
173176
[2]*ast.Term{
174177
ast.StringTerm("user"),
175178
ast.NewTerm(regoSlice(role.User)),
176179
},
180+
[2]*ast.Term{
181+
ast.StringTerm("by_org_id"),
182+
ast.NewTerm(byOrgIDMap),
183+
},
177184
)
178185
}
179186

‎coderd/rbac/authz_internal_test.go‎

Lines changed: 27 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -633,20 +633,22 @@ func TestAuthorizeDomain(t *testing.T) {
633633
{
634634
Identifier: RoleIdentifier{Name: "ReadOnlyOrgAndUser"},
635635
Site: []Permission{},
636-
Org: map[string][]Permission{
637-
defOrg.String(): {{
638-
Negate: false,
639-
ResourceType: "*",
640-
Action: policy.ActionRead,
641-
}},
642-
},
643636
User: []Permission{
644637
{
645638
Negate: false,
646639
ResourceType: "*",
647640
Action: policy.ActionRead,
648641
},
649642
},
643+
ByOrgID: map[string]OrgPermissions{
644+
defOrg.String(): {
645+
Org: []Permission{{
646+
Negate: false,
647+
ResourceType: "*",
648+
Action: policy.ActionRead,
649+
}},
650+
},
651+
},
650652
},
651653
},
652654
}
@@ -726,12 +728,14 @@ func TestAuthorizeLevels(t *testing.T) {
726728
must(RoleByName(RoleOwner())),
727729
{
728730
Identifier: RoleIdentifier{Name: "org-deny:", OrganizationID: defOrg},
729-
Org: map[string][]Permission{
731+
ByOrgID: map[string]OrgPermissions{
730732
defOrg.String(): {
731-
{
732-
Negate: true,
733-
ResourceType: "*",
734-
Action: "*",
733+
Org: []Permission{
734+
{
735+
Negate: true,
736+
ResourceType: "*",
737+
Action: "*",
738+
},
735739
},
736740
},
737741
},
@@ -926,8 +930,8 @@ func TestAuthorizeScope(t *testing.T) {
926930
// Only read access for workspaces.
927931
ResourceWorkspace.Type: {policy.ActionRead},
928932
}),
929-
Org: map[string][]Permission{},
930-
User: []Permission{},
933+
User: []Permission{},
934+
ByOrgID: map[string]OrgPermissions{},
931935
},
932936
AllowIDList: []AllowListElement{{Type: ResourceWorkspace.Type, ID: workspaceID.String()}},
933937
},
@@ -1015,8 +1019,8 @@ func TestAuthorizeScope(t *testing.T) {
10151019
// Only read access for workspaces.
10161020
ResourceWorkspace.Type: {policy.ActionCreate},
10171021
}),
1018-
Org: map[string][]Permission{},
1019-
User: []Permission{},
1022+
User: []Permission{},
1023+
ByOrgID: map[string]OrgPermissions{},
10201024
},
10211025
// Empty string allow_list is allowed for actions like 'create'
10221026
AllowIDList: []AllowListElement{{
@@ -1138,14 +1142,16 @@ func TestAuthorizeScope(t *testing.T) {
11381142
},
11391143
DisplayName: "OrgAndUserScope",
11401144
Site: nil,
1141-
Org: map[string][]Permission{
1142-
defOrg.String(): Permissions(map[string][]policy.Action{
1143-
ResourceWorkspace.Type: {policy.ActionRead},
1144-
}),
1145-
},
11461145
User: Permissions(map[string][]policy.Action{
11471146
ResourceUser.Type: {policy.ActionRead},
11481147
}),
1148+
ByOrgID: map[string]OrgPermissions{
1149+
defOrg.String(): {
1150+
Org: Permissions(map[string][]policy.Action{
1151+
ResourceWorkspace.Type: {policy.ActionRead},
1152+
}),
1153+
},
1154+
},
11491155
},
11501156
AllowIDList: []AllowListElement{AllowListAll()},
11511157
},

0 commit comments

Comments
 (0)