Skip to content

Commit d04959c

Browse files
EmyrkEmyrk
authored
chore: implement custom role assignment for organization admins (#13570)
* chore: static role assignment mapping Until a dynamic approach is created in the database, only org-admins can assign custom organization roles.
1 parent 3d30c8d commit d04959c

File tree

12 files changed

+147
-38
lines changed

12 files changed

+147
-38
lines changed

‎coderd/database/dbauthz/customroles_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -157,7 +157,7 @@ func TestUpsertCustomRoles(t *testing.T) {
157157
org: codersdk.CreatePermissions(map[codersdk.RBACResource][]codersdk.RBACAction{
158158
codersdk.ResourceWorkspace: {codersdk.ActionRead},
159159
}),
160-
errorContains: "not allowed to grant this permission",
160+
errorContains: "forbidden",
161161
},
162162
{
163163
name: "user-escalation",

‎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/object_gen.go

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

‎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} }
@@ -314,6 +318,9 @@ func ReloadBuiltinRoles(opts *RoleOptions) {
314318
DisplayName: "User Admin",
315319
Site: Permissions(map[string][]policy.Action{
316320
ResourceAssignRole.Type: {policy.ActionAssign, policy.ActionDelete, policy.ActionRead},
321+
// Need organization assign as well to create users. At present, creating a user
322+
// will always assign them to some organization.
323+
ResourceAssignOrgRole.Type: {policy.ActionAssign, policy.ActionDelete, policy.ActionRead},
317324
ResourceUser.Type: {
318325
policy.ActionCreate, policy.ActionRead, policy.ActionUpdate, policy.ActionDelete,
319326
policy.ActionUpdatePersonal, policy.ActionReadPersonal,
@@ -361,7 +368,7 @@ func ReloadBuiltinRoles(opts *RoleOptions) {
361368
Site: []Permission{},
362369
Org: map[string][]Permission{
363370
// Org admins should not have workspace exec perms.
364-
organizationID.String(): append(allPermsExcept(ResourceWorkspace, ResourceWorkspaceDormant), Permissions(map[string][]policy.Action{
371+
organizationID.String(): append(allPermsExcept(ResourceWorkspace, ResourceWorkspaceDormant, ResourceAssignRole), Permissions(map[string][]policy.Action{
365372
ResourceWorkspaceDormant.Type: {policy.ActionRead, policy.ActionDelete, policy.ActionCreate, policy.ActionUpdate, policy.ActionWorkspaceStop},
366373
ResourceWorkspace.Type: slice.Omit(ResourceWorkspace.AvailableActions(), policy.ActionApplicationConnect, policy.ActionSSH),
367374
})...),
@@ -409,32 +416,35 @@ func ReloadBuiltinRoles(opts *RoleOptions) {
409416
// map[actor_role][assign_role]<can_assign>
410417
var assignRoles = map[string]map[string]bool{
411418
"system": {
412-
owner: true,
413-
auditor: true,
414-
member: true,
415-
orgAdmin: true,
416-
orgMember: true,
417-
templateAdmin: true,
418-
userAdmin: true,
419-
customSiteRole: true,
419+
owner: true,
420+
auditor: true,
421+
member: true,
422+
orgAdmin: true,
423+
orgMember: true,
424+
templateAdmin: true,
425+
userAdmin: true,
426+
customSiteRole: true,
427+
customOrganizationRole: true,
420428
},
421429
owner: {
422-
owner: true,
423-
auditor: true,
424-
member: true,
425-
orgAdmin: true,
426-
orgMember: true,
427-
templateAdmin: true,
428-
userAdmin: true,
429-
customSiteRole: true,
430+
owner: true,
431+
auditor: true,
432+
member: true,
433+
orgAdmin: true,
434+
orgMember: true,
435+
templateAdmin: true,
436+
userAdmin: true,
437+
customSiteRole: true,
438+
customOrganizationRole: true,
430439
},
431440
userAdmin: {
432441
member: true,
433442
orgMember: true,
434443
},
435444
orgAdmin: {
436-
orgAdmin: true,
437-
orgMember: true,
445+
orgAdmin: true,
446+
orgMember: true,
447+
customOrganizationRole: true,
438448
},
439449
}
440450

@@ -596,6 +606,13 @@ func RoleByName(name RoleIdentifier) (Role, error) {
596606
return Role{}, xerrors.Errorf("expect a org id for role %q", name.String())
597607
}
598608

609+
// This can happen if a custom role shares the same name as a built-in role.
610+
// You could make an org role called "owner", and we should not return the
611+
// owner role itself.
612+
if name.OrganizationID != role.Identifier.OrganizationID {
613+
return Role{}, xerrors.Errorf("role %q not found", name.String())
614+
}
615+
599616
return role, nil
600617
}
601618

‎coderd/rbac/roles_test.go

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -279,6 +279,15 @@ func TestRolePermissions(t *testing.T) {
279279
Name: "OrgRoleAssignment",
280280
Actions: []policy.Action{policy.ActionAssign, policy.ActionDelete},
281281
Resource: rbac.ResourceAssignOrgRole.InOrg(orgID),
282+
AuthorizeMap: map[bool][]authSubject{
283+
true: {owner, orgAdmin, userAdmin},
284+
false: {orgMemberMe, otherOrgAdmin, otherOrgMember, memberMe, templateAdmin},
285+
},
286+
},
287+
{
288+
Name: "CreateOrgRoleAssignment",
289+
Actions: []policy.Action{policy.ActionCreate},
290+
Resource: rbac.ResourceAssignOrgRole.InOrg(orgID),
282291
AuthorizeMap: map[bool][]authSubject{
283292
true: {owner, orgAdmin},
284293
false: {orgMemberMe, otherOrgAdmin, otherOrgMember, memberMe, templateAdmin, userAdmin},
@@ -289,8 +298,8 @@ func TestRolePermissions(t *testing.T) {
289298
Actions: []policy.Action{policy.ActionRead},
290299
Resource: rbac.ResourceAssignOrgRole.InOrg(orgID),
291300
AuthorizeMap: map[bool][]authSubject{
292-
true: {owner, orgAdmin, orgMemberMe},
293-
false: {otherOrgAdmin, otherOrgMember, memberMe, templateAdmin, userAdmin},
301+
true: {owner, orgAdmin, orgMemberMe, userAdmin, userAdmin},
302+
false: {otherOrgAdmin, otherOrgMember, memberMe, templateAdmin},
294303
},
295304
},
296305
{

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

‎codersdk/rbacresources_gen.go

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

‎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: "org-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)