Skip to content

Commit a8fe429

Browse files
EmyrkEmyrk
committed
chore: static role assignment mapping
Until a dynamic approach is created in the database, only org-admins can assign custom organization roles.
1 parent c587af7 commit a8fe429

File tree

9 files changed

+134
-35
lines changed

9 files changed

+134
-35
lines changed

‎coderd/database/dbauthz/dbauthz.go

Lines changed: 19 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -239,10 +239,10 @@ var (
239239
rbac.ResourceApiKey.Type: rbac.ResourceApiKey.AvailableActions(),
240240
rbac.ResourceGroup.Type: {policy.ActionCreate, policy.ActionUpdate},
241241
rbac.ResourceAssignRole.Type: rbac.ResourceAssignRole.AvailableActions(),
242+
rbac.ResourceAssignOrgRole.Type: rbac.ResourceAssignOrgRole.AvailableActions(),
242243
rbac.ResourceSystem.Type: {policy.WildcardSymbol},
243244
rbac.ResourceOrganization.Type: {policy.ActionCreate, policy.ActionRead},
244245
rbac.ResourceOrganizationMember.Type: {policy.ActionCreate},
245-
rbac.ResourceAssignOrgRole.Type: {policy.ActionRead, policy.ActionCreate, policy.ActionDelete},
246246
rbac.ResourceProvisionerDaemon.Type: {policy.ActionCreate, policy.ActionUpdate},
247247
rbac.ResourceUser.Type: rbac.ResourceUser.AvailableActions(),
248248
rbac.ResourceWorkspaceDormant.Type: {policy.ActionUpdate, policy.ActionDelete, policy.ActionWorkspaceStop},
@@ -622,7 +622,7 @@ func (q *querier) canAssignRoles(ctx context.Context, orgID *uuid.UUID, added, r
622622
roleAssign := rbac.ResourceAssignRole
623623
shouldBeOrgRoles := false
624624
if orgID != nil {
625-
roleAssign = roleAssign.InOrg(*orgID)
625+
roleAssign = rbac.ResourceAssignOrgRole.InOrg(*orgID)
626626
shouldBeOrgRoles = true
627627
}
628628

@@ -697,8 +697,14 @@ func (q *querier) canAssignRoles(ctx context.Context, orgID *uuid.UUID, added, r
697697

698698
for _, roleName := range grantedRoles {
699699
if _, isCustom := customRolesMap[roleName]; isCustom {
700-
// For now, use a constant name so our static assign map still works.
701-
roleName = rbac.CustomSiteRole()
700+
// To support a dynamic mapping of what roles can assign what, we need
701+
// to store this in the database. For now, just use a static role so
702+
// owners and org admins can assign roles.
703+
if roleName.IsOrgRole() {
704+
roleName = rbac.CustomOrganizationRole(roleName.OrganizationID)
705+
} else {
706+
roleName = rbac.CustomSiteRole()
707+
}
702708
}
703709

704710
if !rbac.CanAssignRole(actor.Roles, roleName) {
@@ -3476,9 +3482,15 @@ func (q *querier) UpsertCustomRole(ctx context.Context, arg database.UpsertCusto
34763482
return database.CustomRole{}, NoActorError
34773483
}
34783484

3479-
// TODO: If this is an org role, check the org assign role type.
3480-
if err := q.authorizeContext(ctx, policy.ActionCreate, rbac.ResourceAssignRole); err != nil {
3481-
return database.CustomRole{}, err
3485+
// Org and site role upsert share the same query. So switch the assertion based on the org uuid.
3486+
if arg.OrganizationID.UUID != uuid.Nil {
3487+
if err := q.authorizeContext(ctx, policy.ActionCreate, rbac.ResourceAssignOrgRole.InOrg(arg.OrganizationID.UUID)); err != nil {
3488+
return database.CustomRole{}, err
3489+
}
3490+
} else {
3491+
if err := q.authorizeContext(ctx, policy.ActionCreate, rbac.ResourceAssignRole); err != nil {
3492+
return database.CustomRole{}, err
3493+
}
34823494
}
34833495

34843496
if arg.OrganizationID.UUID == uuid.Nil && len(arg.OrgPermissions) > 0 {

‎coderd/database/dbauthz/dbauthz_test.go

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -625,7 +625,7 @@ func (s *MethodTestSuite) TestOrganization() {
625625
UserID: u.ID,
626626
Roles: []string{codersdk.RoleOrganizationAdmin},
627627
}).Asserts(
628-
rbac.ResourceAssignRole.InOrg(o.ID), policy.ActionAssign,
628+
rbac.ResourceAssignOrgRole.InOrg(o.ID), policy.ActionAssign,
629629
rbac.ResourceOrganizationMember.InOrg(o.ID).WithID(u.ID), policy.ActionCreate)
630630
}))
631631
s.Run("UpdateOrganization", s.Subtest(func(db database.Store, check *expects) {
@@ -681,8 +681,8 @@ func (s *MethodTestSuite) TestOrganization() {
681681
WithCancelled(sql.ErrNoRows.Error()).
682682
Asserts(
683683
mem, policy.ActionRead,
684-
rbac.ResourceAssignRole.InOrg(o.ID), policy.ActionAssign, // org-mem
685-
rbac.ResourceAssignRole.InOrg(o.ID), policy.ActionDelete, // org-admin
684+
rbac.ResourceAssignOrgRole.InOrg(o.ID), policy.ActionAssign, // org-mem
685+
rbac.ResourceAssignOrgRole.InOrg(o.ID), policy.ActionDelete, // org-admin
686686
).Returns(out)
687687
}))
688688
}
@@ -1257,7 +1257,7 @@ func (s *MethodTestSuite) TestUser() {
12571257
}), convertSDKPerm),
12581258
}).Asserts(
12591259
// First check
1260-
rbac.ResourceAssignRole, policy.ActionCreate,
1260+
rbac.ResourceAssignOrgRole.InOrg(orgID), policy.ActionCreate,
12611261
// Escalation checks
12621262
rbac.ResourceTemplate.InOrg(orgID), policy.ActionCreate,
12631263
rbac.ResourceTemplate.InOrg(orgID), policy.ActionRead,

‎coderd/members.go

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -87,6 +87,10 @@ func (api *API) putMemberRoles(rw http.ResponseWriter, r *http.Request) {
8787
UserID: member.UserID,
8888
OrgID: organization.ID,
8989
})
90+
if httpapi.Is404Error(err) {
91+
httpapi.Forbidden(rw)
92+
return
93+
}
9094
if err != nil {
9195
httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
9296
Message: err.Error(),

‎coderd/rbac/policy/policy.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -218,6 +218,7 @@ var RBACPermissions = map[string]PermissionDefinition{
218218
ActionAssign: actDef("ability to assign org scoped roles"),
219219
ActionRead: actDef("view what roles are assignable"),
220220
ActionDelete: actDef("ability to delete org scoped roles"),
221+
ActionCreate: actDef("ability to create/delete/edit custom roles within an organization"),
221222
},
222223
},
223224
"oauth2_app": {

‎coderd/rbac/roles.go

Lines changed: 39 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,8 @@ const (
2424
// customSiteRole is a placeholder for all custom site roles.
2525
// This is used for what roles can assign other roles.
2626
// TODO: Make this more dynamic to allow other roles to grant.
27-
customSiteRole string = "custom-site-role"
27+
customSiteRole string = "custom-site-role"
28+
customOrganizationRole string = "custom-organization-role"
2829

2930
orgAdmin string = "organization-admin"
3031
orgMember string = "organization-member"
@@ -125,8 +126,11 @@ func (r *RoleIdentifier) UnmarshalJSON(data []byte) error {
125126
// Once we have a database implementation, the "default" roles can be defined on the
126127
// site and orgs, and these functions can be removed.
127128

128-
func RoleOwner() RoleIdentifier { return RoleIdentifier{Name: owner} }
129-
func CustomSiteRole() RoleIdentifier { return RoleIdentifier{Name: customSiteRole} }
129+
func RoleOwner() RoleIdentifier { return RoleIdentifier{Name: owner} }
130+
func CustomSiteRole() RoleIdentifier { return RoleIdentifier{Name: customSiteRole} }
131+
func CustomOrganizationRole(orgID uuid.UUID) RoleIdentifier {
132+
return RoleIdentifier{Name: customOrganizationRole, OrganizationID: orgID}
133+
}
130134
func RoleTemplateAdmin() RoleIdentifier { return RoleIdentifier{Name: templateAdmin} }
131135
func RoleUserAdmin() RoleIdentifier { return RoleIdentifier{Name: userAdmin} }
132136
func RoleMember() RoleIdentifier { return RoleIdentifier{Name: member} }
@@ -307,6 +311,9 @@ func ReloadBuiltinRoles(opts *RoleOptions) {
307311
DisplayName: "User Admin",
308312
Site: Permissions(map[string][]policy.Action{
309313
ResourceAssignRole.Type: {policy.ActionAssign, policy.ActionDelete, policy.ActionRead},
314+
// Need organization assign as well to create users. At present, creating a user
315+
// will always assign them to some organization.
316+
ResourceAssignOrgRole.Type: {policy.ActionAssign, policy.ActionDelete, policy.ActionRead},
310317
ResourceUser.Type: {
311318
policy.ActionCreate, policy.ActionRead, policy.ActionUpdate, policy.ActionDelete,
312319
policy.ActionUpdatePersonal, policy.ActionReadPersonal,
@@ -354,7 +361,7 @@ func ReloadBuiltinRoles(opts *RoleOptions) {
354361
Site: []Permission{},
355362
Org: map[string][]Permission{
356363
// Org admins should not have workspace exec perms.
357-
organizationID.String(): append(allPermsExcept(ResourceWorkspace, ResourceWorkspaceDormant), Permissions(map[string][]policy.Action{
364+
organizationID.String(): append(allPermsExcept(ResourceWorkspace, ResourceWorkspaceDormant, ResourceAssignRole), Permissions(map[string][]policy.Action{
358365
ResourceWorkspaceDormant.Type: {policy.ActionRead, policy.ActionDelete, policy.ActionCreate, policy.ActionUpdate, policy.ActionWorkspaceStop},
359366
ResourceWorkspace.Type: slice.Omit(ResourceWorkspace.AvailableActions(), policy.ActionApplicationConnect, policy.ActionSSH),
360367
})...),
@@ -402,32 +409,35 @@ func ReloadBuiltinRoles(opts *RoleOptions) {
402409
// map[actor_role][assign_role]<can_assign>
403410
var assignRoles = map[string]map[string]bool{
404411
"system": {
405-
owner: true,
406-
auditor: true,
407-
member: true,
408-
orgAdmin: true,
409-
orgMember: true,
410-
templateAdmin: true,
411-
userAdmin: true,
412-
customSiteRole: true,
412+
owner: true,
413+
auditor: true,
414+
member: true,
415+
orgAdmin: true,
416+
orgMember: true,
417+
templateAdmin: true,
418+
userAdmin: true,
419+
customSiteRole: true,
420+
customOrganizationRole: true,
413421
},
414422
owner: {
415-
owner: true,
416-
auditor: true,
417-
member: true,
418-
orgAdmin: true,
419-
orgMember: true,
420-
templateAdmin: true,
421-
userAdmin: true,
422-
customSiteRole: true,
423+
owner: true,
424+
auditor: true,
425+
member: true,
426+
orgAdmin: true,
427+
orgMember: true,
428+
templateAdmin: true,
429+
userAdmin: true,
430+
customSiteRole: true,
431+
customOrganizationRole: true,
423432
},
424433
userAdmin: {
425434
member: true,
426435
orgMember: true,
427436
},
428437
orgAdmin: {
429-
orgAdmin: true,
430-
orgMember: true,
438+
orgAdmin: true,
439+
orgMember: true,
440+
customOrganizationRole: true,
431441
},
432442
}
433443

@@ -589,6 +599,13 @@ func RoleByName(name RoleIdentifier) (Role, error) {
589599
return Role{}, xerrors.Errorf("expect a org id for role %q", name.String())
590600
}
591601

602+
// This can happen if a custom role shares the same name as a built-in role.
603+
// You could make an org role called "owner", and we should not return the
604+
// owner role itself.
605+
if name.OrganizationID != role.Identifier.OrganizationID {
606+
return Role{}, xerrors.Errorf("role %q not found", name.String())
607+
}
608+
592609
return role, nil
593610
}
594611

‎coderd/rbac/roles_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -277,7 +277,7 @@ func TestRolePermissions(t *testing.T) {
277277
},
278278
{
279279
Name: "OrgRoleAssignment",
280-
Actions: []policy.Action{policy.ActionAssign, policy.ActionDelete},
280+
Actions: []policy.Action{policy.ActionAssign, policy.ActionDelete, policy.ActionCreate},
281281
Resource: rbac.ResourceAssignOrgRole.InOrg(orgID),
282282
AuthorizeMap: map[bool][]authSubject{
283283
true: {owner, orgAdmin},

‎coderd/roles.go

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -144,9 +144,14 @@ func assignableRoles(actorRoles rbac.ExpandableRoles, roles []rbac.Role, customR
144144
}
145145

146146
for _, role := range customRoles {
147+
canAssign := rbac.CanAssignRole(actorRoles, rbac.CustomSiteRole())
148+
if role.RoleIdentifier().IsOrgRole() {
149+
canAssign = rbac.CanAssignRole(actorRoles, rbac.CustomOrganizationRole(role.OrganizationID.UUID))
150+
}
151+
147152
assignable = append(assignable, codersdk.AssignableRoles{
148153
Role: db2sdk.Role(role),
149-
Assignable: rbac.CanAssignRole(actorRoles, role.RoleIdentifier()),
154+
Assignable: canAssign,
150155
BuiltIn: false,
151156
})
152157
}

‎enterprise/coderd/roles_test.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -203,6 +203,7 @@ func TestCustomOrganizationRole(t *testing.T) {
203203
_, err := owner.PatchOrganizationRole(ctx, first.OrganizationID, codersdk.Role{
204204
Name: "Bad_Name", // No underscores allowed
205205
DisplayName: "Testing Purposes",
206+
OrganizationID: first.OrganizationID.String(),
206207
SitePermissions: nil,
207208
OrganizationPermissions: nil,
208209
UserPermissions: nil,

‎enterprise/coderd/users_test.go

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import (
99
"github.com/stretchr/testify/require"
1010

1111
"github.com/coder/coder/v2/coderd/coderdtest"
12+
"github.com/coder/coder/v2/coderd/rbac"
1213
"github.com/coder/coder/v2/coderd/schedule/cron"
1314
"github.com/coder/coder/v2/codersdk"
1415
"github.com/coder/coder/v2/enterprise/coderd/coderdenttest"
@@ -237,3 +238,61 @@ func TestCreateFirstUser_Entitlements_Trial(t *testing.T) {
237238
require.NoError(t, err)
238239
require.True(t, entitlements.Trial, "Trial license should be immediately active.")
239240
}
241+
242+
// TestAssignCustomOrgRoles verifies an organization admin (not just an owner) can create
243+
// a custom role and assign it to an organization user.
244+
func TestAssignCustomOrgRoles(t *testing.T) {
245+
t.Parallel()
246+
dv := coderdtest.DeploymentValues(t)
247+
dv.Experiments = []string{string(codersdk.ExperimentCustomRoles)}
248+
249+
ownerClient, owner := coderdenttest.New(t, &coderdenttest.Options{
250+
Options: &coderdtest.Options{
251+
DeploymentValues: dv,
252+
IncludeProvisionerDaemon: true,
253+
},
254+
LicenseOptions: &coderdenttest.LicenseOptions{
255+
Features: license.Features{
256+
codersdk.FeatureCustomRoles: 1,
257+
},
258+
},
259+
})
260+
261+
client, _ := coderdtest.CreateAnotherUser(t, ownerClient, owner.OrganizationID, rbac.ScopedRoleOrgAdmin(owner.OrganizationID))
262+
tv := coderdtest.CreateTemplateVersion(t, client, owner.OrganizationID, nil)
263+
coderdtest.AwaitTemplateVersionJobCompleted(t, client, tv.ID)
264+
265+
ctx := testutil.Context(t, testutil.WaitShort)
266+
// Create a custom role as an organization admin that allows making templates.
267+
auditorRole, err := client.PatchOrganizationRole(ctx, owner.OrganizationID, codersdk.Role{
268+
Name: "template-admin",
269+
OrganizationID: owner.OrganizationID.String(),
270+
DisplayName: "Template Admin",
271+
SitePermissions: nil,
272+
OrganizationPermissions: codersdk.CreatePermissions(map[codersdk.RBACResource][]codersdk.RBACAction{
273+
codersdk.ResourceTemplate: codersdk.RBACResourceActions[codersdk.ResourceTemplate], // All template perms
274+
}),
275+
UserPermissions: nil,
276+
})
277+
require.NoError(t, err)
278+
279+
createTemplateReq := codersdk.CreateTemplateRequest{
280+
Name: "name",
281+
DisplayName: "Template",
282+
VersionID: tv.ID,
283+
}
284+
memberClient, member := coderdtest.CreateAnotherUser(t, ownerClient, owner.OrganizationID)
285+
// Check the member cannot create a template
286+
_, err = memberClient.CreateTemplate(ctx, owner.OrganizationID, createTemplateReq)
287+
require.Error(t, err)
288+
289+
// Assign new role to the member as the org admin
290+
_, err = client.UpdateOrganizationMemberRoles(ctx, owner.OrganizationID, member.ID.String(), codersdk.UpdateRoles{
291+
Roles: []string{auditorRole.Name},
292+
})
293+
require.NoError(t, err)
294+
295+
// Now the member can create the template
296+
_, err = memberClient.CreateTemplate(ctx, owner.OrganizationID, createTemplateReq)
297+
require.NoError(t, err)
298+
}

0 commit comments

Comments
 (0)