From 96bdaf29fe3bdac2fee68255c19988b8d114778e Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 13 Jun 2024 09:49:34 -0500 Subject: [PATCH 1/3] chore: static role assignment mapping Until a dynamic approach is created in the database, only org-admins can assign custom organization roles. --- coderd/database/dbauthz/customroles_test.go | 2 +- coderd/database/dbauthz/dbauthz.go | 26 ++++++--- coderd/database/dbauthz/dbauthz_test.go | 8 +-- coderd/members.go | 4 ++ coderd/rbac/object_gen.go | 1 + coderd/rbac/policy/policy.go | 1 + coderd/rbac/roles.go | 61 +++++++++++++-------- coderd/rbac/roles_test.go | 2 +- coderd/roles.go | 7 ++- codersdk/rbacresources_gen.go | 2 +- enterprise/coderd/roles_test.go | 1 + enterprise/coderd/users_test.go | 59 ++++++++++++++++++++ 12 files changed, 137 insertions(+), 37 deletions(-) diff --git a/coderd/database/dbauthz/customroles_test.go b/coderd/database/dbauthz/customroles_test.go index 1a9049044e0ce..4a544989c599e 100644 --- a/coderd/database/dbauthz/customroles_test.go +++ b/coderd/database/dbauthz/customroles_test.go @@ -157,7 +157,7 @@ func TestUpsertCustomRoles(t *testing.T) { org: codersdk.CreatePermissions(map[codersdk.RBACResource][]codersdk.RBACAction{ codersdk.ResourceWorkspace: {codersdk.ActionRead}, }), - errorContains: "not allowed to grant this permission", + errorContains: "forbidden", }, { name: "user-escalation", diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go index 85659751a9107..8aec14eb7bf47 100644 --- a/coderd/database/dbauthz/dbauthz.go +++ b/coderd/database/dbauthz/dbauthz.go @@ -239,10 +239,10 @@ var ( rbac.ResourceApiKey.Type: rbac.ResourceApiKey.AvailableActions(), rbac.ResourceGroup.Type: {policy.ActionCreate, policy.ActionUpdate}, rbac.ResourceAssignRole.Type: rbac.ResourceAssignRole.AvailableActions(), + rbac.ResourceAssignOrgRole.Type: rbac.ResourceAssignOrgRole.AvailableActions(), rbac.ResourceSystem.Type: {policy.WildcardSymbol}, rbac.ResourceOrganization.Type: {policy.ActionCreate, policy.ActionRead}, rbac.ResourceOrganizationMember.Type: {policy.ActionCreate}, - rbac.ResourceAssignOrgRole.Type: {policy.ActionRead, policy.ActionCreate, policy.ActionDelete}, rbac.ResourceProvisionerDaemon.Type: {policy.ActionCreate, policy.ActionUpdate}, rbac.ResourceUser.Type: rbac.ResourceUser.AvailableActions(), 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 roleAssign := rbac.ResourceAssignRole shouldBeOrgRoles := false if orgID != nil { - roleAssign = roleAssign.InOrg(*orgID) + roleAssign = rbac.ResourceAssignOrgRole.InOrg(*orgID) shouldBeOrgRoles = true } @@ -697,8 +697,14 @@ func (q *querier) canAssignRoles(ctx context.Context, orgID *uuid.UUID, added, r for _, roleName := range grantedRoles { if _, isCustom := customRolesMap[roleName]; isCustom { - // For now, use a constant name so our static assign map still works. - roleName = rbac.CustomSiteRole() + // To support a dynamic mapping of what roles can assign what, we need + // to store this in the database. For now, just use a static role so + // owners and org admins can assign roles. + if roleName.IsOrgRole() { + roleName = rbac.CustomOrganizationRole(roleName.OrganizationID) + } else { + roleName = rbac.CustomSiteRole() + } } if !rbac.CanAssignRole(actor.Roles, roleName) { @@ -3476,9 +3482,15 @@ func (q *querier) UpsertCustomRole(ctx context.Context, arg database.UpsertCusto return database.CustomRole{}, NoActorError } - // TODO: If this is an org role, check the org assign role type. - if err := q.authorizeContext(ctx, policy.ActionCreate, rbac.ResourceAssignRole); err != nil { - return database.CustomRole{}, err + // Org and site role upsert share the same query. So switch the assertion based on the org uuid. + if arg.OrganizationID.UUID != uuid.Nil { + if err := q.authorizeContext(ctx, policy.ActionCreate, rbac.ResourceAssignOrgRole.InOrg(arg.OrganizationID.UUID)); err != nil { + return database.CustomRole{}, err + } + } else { + if err := q.authorizeContext(ctx, policy.ActionCreate, rbac.ResourceAssignRole); err != nil { + return database.CustomRole{}, err + } } if arg.OrganizationID.UUID == uuid.Nil && len(arg.OrgPermissions) > 0 { diff --git a/coderd/database/dbauthz/dbauthz_test.go b/coderd/database/dbauthz/dbauthz_test.go index 44d45118ce1ea..96b0e35874186 100644 --- a/coderd/database/dbauthz/dbauthz_test.go +++ b/coderd/database/dbauthz/dbauthz_test.go @@ -625,7 +625,7 @@ func (s *MethodTestSuite) TestOrganization() { UserID: u.ID, Roles: []string{codersdk.RoleOrganizationAdmin}, }).Asserts( - rbac.ResourceAssignRole.InOrg(o.ID), policy.ActionAssign, + rbac.ResourceAssignOrgRole.InOrg(o.ID), policy.ActionAssign, rbac.ResourceOrganizationMember.InOrg(o.ID).WithID(u.ID), policy.ActionCreate) })) s.Run("UpdateOrganization", s.Subtest(func(db database.Store, check *expects) { @@ -681,8 +681,8 @@ func (s *MethodTestSuite) TestOrganization() { WithCancelled(sql.ErrNoRows.Error()). Asserts( mem, policy.ActionRead, - rbac.ResourceAssignRole.InOrg(o.ID), policy.ActionAssign, // org-mem - rbac.ResourceAssignRole.InOrg(o.ID), policy.ActionDelete, // org-admin + rbac.ResourceAssignOrgRole.InOrg(o.ID), policy.ActionAssign, // org-mem + rbac.ResourceAssignOrgRole.InOrg(o.ID), policy.ActionDelete, // org-admin ).Returns(out) })) } @@ -1257,7 +1257,7 @@ func (s *MethodTestSuite) TestUser() { }), convertSDKPerm), }).Asserts( // First check - rbac.ResourceAssignRole, policy.ActionCreate, + rbac.ResourceAssignOrgRole.InOrg(orgID), policy.ActionCreate, // Escalation checks rbac.ResourceTemplate.InOrg(orgID), policy.ActionCreate, rbac.ResourceTemplate.InOrg(orgID), policy.ActionRead, diff --git a/coderd/members.go b/coderd/members.go index eaa14ada67d8e..bd41dfa10741a 100644 --- a/coderd/members.go +++ b/coderd/members.go @@ -87,6 +87,10 @@ func (api *API) putMemberRoles(rw http.ResponseWriter, r *http.Request) { UserID: member.UserID, OrgID: organization.ID, }) + if httpapi.Is404Error(err) { + httpapi.Forbidden(rw) + return + } if err != nil { httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ Message: err.Error(), diff --git a/coderd/rbac/object_gen.go b/coderd/rbac/object_gen.go index 9ab848d795b1c..5b39b846195dd 100644 --- a/coderd/rbac/object_gen.go +++ b/coderd/rbac/object_gen.go @@ -28,6 +28,7 @@ var ( // ResourceAssignOrgRole // Valid Actions // - "ActionAssign" :: ability to assign org scoped roles + // - "ActionCreate" :: ability to create/delete/edit custom roles within an organization // - "ActionDelete" :: ability to delete org scoped roles // - "ActionRead" :: view what roles are assignable ResourceAssignOrgRole = Object{ diff --git a/coderd/rbac/policy/policy.go b/coderd/rbac/policy/policy.go index 2d3213264a514..eec8865d09317 100644 --- a/coderd/rbac/policy/policy.go +++ b/coderd/rbac/policy/policy.go @@ -218,6 +218,7 @@ var RBACPermissions = map[string]PermissionDefinition{ ActionAssign: actDef("ability to assign org scoped roles"), ActionRead: actDef("view what roles are assignable"), ActionDelete: actDef("ability to delete org scoped roles"), + ActionCreate: actDef("ability to create/delete/edit custom roles within an organization"), }, }, "oauth2_app": { diff --git a/coderd/rbac/roles.go b/coderd/rbac/roles.go index 14d18e2dd4e0e..34bbdde5622e3 100644 --- a/coderd/rbac/roles.go +++ b/coderd/rbac/roles.go @@ -24,7 +24,8 @@ const ( // customSiteRole is a placeholder for all custom site roles. // This is used for what roles can assign other roles. // TODO: Make this more dynamic to allow other roles to grant. - customSiteRole string = "custom-site-role" + customSiteRole string = "custom-site-role" + customOrganizationRole string = "custom-organization-role" orgAdmin string = "organization-admin" orgMember string = "organization-member" @@ -125,8 +126,11 @@ func (r *RoleIdentifier) UnmarshalJSON(data []byte) error { // Once we have a database implementation, the "default" roles can be defined on the // site and orgs, and these functions can be removed. -func RoleOwner() RoleIdentifier { return RoleIdentifier{Name: owner} } -func CustomSiteRole() RoleIdentifier { return RoleIdentifier{Name: customSiteRole} } +func RoleOwner() RoleIdentifier { return RoleIdentifier{Name: owner} } +func CustomSiteRole() RoleIdentifier { return RoleIdentifier{Name: customSiteRole} } +func CustomOrganizationRole(orgID uuid.UUID) RoleIdentifier { + return RoleIdentifier{Name: customOrganizationRole, OrganizationID: orgID} +} func RoleTemplateAdmin() RoleIdentifier { return RoleIdentifier{Name: templateAdmin} } func RoleUserAdmin() RoleIdentifier { return RoleIdentifier{Name: userAdmin} } func RoleMember() RoleIdentifier { return RoleIdentifier{Name: member} } @@ -307,6 +311,9 @@ func ReloadBuiltinRoles(opts *RoleOptions) { DisplayName: "User Admin", Site: Permissions(map[string][]policy.Action{ ResourceAssignRole.Type: {policy.ActionAssign, policy.ActionDelete, policy.ActionRead}, + // Need organization assign as well to create users. At present, creating a user + // will always assign them to some organization. + ResourceAssignOrgRole.Type: {policy.ActionAssign, policy.ActionDelete, policy.ActionRead}, ResourceUser.Type: { policy.ActionCreate, policy.ActionRead, policy.ActionUpdate, policy.ActionDelete, policy.ActionUpdatePersonal, policy.ActionReadPersonal, @@ -354,7 +361,7 @@ func ReloadBuiltinRoles(opts *RoleOptions) { Site: []Permission{}, Org: map[string][]Permission{ // Org admins should not have workspace exec perms. - organizationID.String(): append(allPermsExcept(ResourceWorkspace, ResourceWorkspaceDormant), Permissions(map[string][]policy.Action{ + organizationID.String(): append(allPermsExcept(ResourceWorkspace, ResourceWorkspaceDormant, ResourceAssignRole), Permissions(map[string][]policy.Action{ ResourceWorkspaceDormant.Type: {policy.ActionRead, policy.ActionDelete, policy.ActionCreate, policy.ActionUpdate, policy.ActionWorkspaceStop}, ResourceWorkspace.Type: slice.Omit(ResourceWorkspace.AvailableActions(), policy.ActionApplicationConnect, policy.ActionSSH), })...), @@ -402,32 +409,35 @@ func ReloadBuiltinRoles(opts *RoleOptions) { // map[actor_role][assign_role] var assignRoles = map[string]map[string]bool{ "system": { - owner: true, - auditor: true, - member: true, - orgAdmin: true, - orgMember: true, - templateAdmin: true, - userAdmin: true, - customSiteRole: true, + owner: true, + auditor: true, + member: true, + orgAdmin: true, + orgMember: true, + templateAdmin: true, + userAdmin: true, + customSiteRole: true, + customOrganizationRole: true, }, owner: { - owner: true, - auditor: true, - member: true, - orgAdmin: true, - orgMember: true, - templateAdmin: true, - userAdmin: true, - customSiteRole: true, + owner: true, + auditor: true, + member: true, + orgAdmin: true, + orgMember: true, + templateAdmin: true, + userAdmin: true, + customSiteRole: true, + customOrganizationRole: true, }, userAdmin: { member: true, orgMember: true, }, orgAdmin: { - orgAdmin: true, - orgMember: true, + orgAdmin: true, + orgMember: true, + customOrganizationRole: true, }, } @@ -589,6 +599,13 @@ func RoleByName(name RoleIdentifier) (Role, error) { return Role{}, xerrors.Errorf("expect a org id for role %q", name.String()) } + // This can happen if a custom role shares the same name as a built-in role. + // You could make an org role called "owner", and we should not return the + // owner role itself. + if name.OrganizationID != role.Identifier.OrganizationID { + return Role{}, xerrors.Errorf("role %q not found", name.String()) + } + return role, nil } diff --git a/coderd/rbac/roles_test.go b/coderd/rbac/roles_test.go index a1f607ac756c8..5f96775f00f07 100644 --- a/coderd/rbac/roles_test.go +++ b/coderd/rbac/roles_test.go @@ -277,7 +277,7 @@ func TestRolePermissions(t *testing.T) { }, { Name: "OrgRoleAssignment", - Actions: []policy.Action{policy.ActionAssign, policy.ActionDelete}, + Actions: []policy.Action{policy.ActionAssign, policy.ActionDelete, policy.ActionCreate}, Resource: rbac.ResourceAssignOrgRole.InOrg(orgID), AuthorizeMap: map[bool][]authSubject{ true: {owner, orgAdmin}, diff --git a/coderd/roles.go b/coderd/roles.go index f4e66b7a56a50..8c066f5fecbb3 100644 --- a/coderd/roles.go +++ b/coderd/roles.go @@ -144,9 +144,14 @@ func assignableRoles(actorRoles rbac.ExpandableRoles, roles []rbac.Role, customR } for _, role := range customRoles { + canAssign := rbac.CanAssignRole(actorRoles, rbac.CustomSiteRole()) + if role.RoleIdentifier().IsOrgRole() { + canAssign = rbac.CanAssignRole(actorRoles, rbac.CustomOrganizationRole(role.OrganizationID.UUID)) + } + assignable = append(assignable, codersdk.AssignableRoles{ Role: db2sdk.Role(role), - Assignable: rbac.CanAssignRole(actorRoles, role.RoleIdentifier()), + Assignable: canAssign, BuiltIn: false, }) } diff --git a/codersdk/rbacresources_gen.go b/codersdk/rbacresources_gen.go index 2c524e356553e..73d784b449535 100644 --- a/codersdk/rbacresources_gen.go +++ b/codersdk/rbacresources_gen.go @@ -54,7 +54,7 @@ const ( var RBACResourceActions = map[RBACResource][]RBACAction{ ResourceWildcard: {}, ResourceApiKey: {ActionCreate, ActionDelete, ActionRead, ActionUpdate}, - ResourceAssignOrgRole: {ActionAssign, ActionDelete, ActionRead}, + ResourceAssignOrgRole: {ActionAssign, ActionCreate, ActionDelete, ActionRead}, ResourceAssignRole: {ActionAssign, ActionCreate, ActionDelete, ActionRead}, ResourceAuditLog: {ActionCreate, ActionRead}, ResourceDebugInfo: {ActionRead}, diff --git a/enterprise/coderd/roles_test.go b/enterprise/coderd/roles_test.go index 239a055540075..b6f6d7d12859a 100644 --- a/enterprise/coderd/roles_test.go +++ b/enterprise/coderd/roles_test.go @@ -203,6 +203,7 @@ func TestCustomOrganizationRole(t *testing.T) { _, err := owner.PatchOrganizationRole(ctx, first.OrganizationID, codersdk.Role{ Name: "Bad_Name", // No underscores allowed DisplayName: "Testing Purposes", + OrganizationID: first.OrganizationID.String(), SitePermissions: nil, OrganizationPermissions: nil, UserPermissions: nil, diff --git a/enterprise/coderd/users_test.go b/enterprise/coderd/users_test.go index ede99551ef0ec..f611dbd5a0ab9 100644 --- a/enterprise/coderd/users_test.go +++ b/enterprise/coderd/users_test.go @@ -9,6 +9,7 @@ import ( "github.com/stretchr/testify/require" "github.com/coder/coder/v2/coderd/coderdtest" + "github.com/coder/coder/v2/coderd/rbac" "github.com/coder/coder/v2/coderd/schedule/cron" "github.com/coder/coder/v2/codersdk" "github.com/coder/coder/v2/enterprise/coderd/coderdenttest" @@ -237,3 +238,61 @@ func TestCreateFirstUser_Entitlements_Trial(t *testing.T) { require.NoError(t, err) require.True(t, entitlements.Trial, "Trial license should be immediately active.") } + +// TestAssignCustomOrgRoles verifies an organization admin (not just an owner) can create +// a custom role and assign it to an organization user. +func TestAssignCustomOrgRoles(t *testing.T) { + t.Parallel() + dv := coderdtest.DeploymentValues(t) + dv.Experiments = []string{string(codersdk.ExperimentCustomRoles)} + + ownerClient, owner := coderdenttest.New(t, &coderdenttest.Options{ + Options: &coderdtest.Options{ + DeploymentValues: dv, + IncludeProvisionerDaemon: true, + }, + LicenseOptions: &coderdenttest.LicenseOptions{ + Features: license.Features{ + codersdk.FeatureCustomRoles: 1, + }, + }, + }) + + client, _ := coderdtest.CreateAnotherUser(t, ownerClient, owner.OrganizationID, rbac.ScopedRoleOrgAdmin(owner.OrganizationID)) + tv := coderdtest.CreateTemplateVersion(t, client, owner.OrganizationID, nil) + coderdtest.AwaitTemplateVersionJobCompleted(t, client, tv.ID) + + ctx := testutil.Context(t, testutil.WaitShort) + // Create a custom role as an organization admin that allows making templates. + auditorRole, err := client.PatchOrganizationRole(ctx, owner.OrganizationID, codersdk.Role{ + Name: "template-admin", + OrganizationID: owner.OrganizationID.String(), + DisplayName: "Template Admin", + SitePermissions: nil, + OrganizationPermissions: codersdk.CreatePermissions(map[codersdk.RBACResource][]codersdk.RBACAction{ + codersdk.ResourceTemplate: codersdk.RBACResourceActions[codersdk.ResourceTemplate], // All template perms + }), + UserPermissions: nil, + }) + require.NoError(t, err) + + createTemplateReq := codersdk.CreateTemplateRequest{ + Name: "name", + DisplayName: "Template", + VersionID: tv.ID, + } + memberClient, member := coderdtest.CreateAnotherUser(t, ownerClient, owner.OrganizationID) + // Check the member cannot create a template + _, err = memberClient.CreateTemplate(ctx, owner.OrganizationID, createTemplateReq) + require.Error(t, err) + + // Assign new role to the member as the org admin + _, err = client.UpdateOrganizationMemberRoles(ctx, owner.OrganizationID, member.ID.String(), codersdk.UpdateRoles{ + Roles: []string{auditorRole.Name}, + }) + require.NoError(t, err) + + // Now the member can create the template + _, err = memberClient.CreateTemplate(ctx, owner.OrganizationID, createTemplateReq) + require.NoError(t, err) +} From f3537f4b50d75684f9d0c20e6229fe37cf8446d9 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 13 Jun 2024 15:20:55 -0500 Subject: [PATCH 2/3] fixup authz tests --- coderd/rbac/roles_test.go | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/coderd/rbac/roles_test.go b/coderd/rbac/roles_test.go index 5f96775f00f07..c49f161760235 100644 --- a/coderd/rbac/roles_test.go +++ b/coderd/rbac/roles_test.go @@ -277,7 +277,16 @@ func TestRolePermissions(t *testing.T) { }, { Name: "OrgRoleAssignment", - Actions: []policy.Action{policy.ActionAssign, policy.ActionDelete, policy.ActionCreate}, + Actions: []policy.Action{policy.ActionAssign, policy.ActionDelete}, + Resource: rbac.ResourceAssignOrgRole.InOrg(orgID), + AuthorizeMap: map[bool][]authSubject{ + true: {owner, orgAdmin, userAdmin}, + false: {orgMemberMe, otherOrgAdmin, otherOrgMember, memberMe, templateAdmin}, + }, + }, + { + Name: "CreateOrgRoleAssignment", + Actions: []policy.Action{policy.ActionCreate}, Resource: rbac.ResourceAssignOrgRole.InOrg(orgID), AuthorizeMap: map[bool][]authSubject{ true: {owner, orgAdmin}, @@ -289,8 +298,8 @@ func TestRolePermissions(t *testing.T) { Actions: []policy.Action{policy.ActionRead}, Resource: rbac.ResourceAssignOrgRole.InOrg(orgID), AuthorizeMap: map[bool][]authSubject{ - true: {owner, orgAdmin, orgMemberMe}, - false: {otherOrgAdmin, otherOrgMember, memberMe, templateAdmin, userAdmin}, + true: {owner, orgAdmin, orgMemberMe, userAdmin, userAdmin}, + false: {otherOrgAdmin, otherOrgMember, memberMe, templateAdmin}, }, }, { From a68e19dac61aa1fef3182c04156f97ebd8273152 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 13 Jun 2024 15:36:48 -0500 Subject: [PATCH 3/3] fix reserved role name in test --- enterprise/coderd/users_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/enterprise/coderd/users_test.go b/enterprise/coderd/users_test.go index f611dbd5a0ab9..4f55859cd9e4d 100644 --- a/enterprise/coderd/users_test.go +++ b/enterprise/coderd/users_test.go @@ -265,7 +265,7 @@ func TestAssignCustomOrgRoles(t *testing.T) { ctx := testutil.Context(t, testutil.WaitShort) // Create a custom role as an organization admin that allows making templates. auditorRole, err := client.PatchOrganizationRole(ctx, owner.OrganizationID, codersdk.Role{ - Name: "template-admin", + Name: "org-template-admin", OrganizationID: owner.OrganizationID.String(), DisplayName: "Template Admin", SitePermissions: nil,