coder · Emyrk · Jun 13, 2024 · Jun 13, 2024 · Jun 13, 2024 · Jun 13, 2024
diff --git 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
@@ -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
@@ -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
@@ -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
diff --git 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
@@ -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]<can_assign>
 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
@@ -279,6 +279,15 @@ func TestRolePermissions(t *testing.T) {
 			Name:     "OrgRoleAssignment",
 			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},
 				false: {orgMemberMe, otherOrgAdmin, otherOrgMember, memberMe, templateAdmin, userAdmin},
@@ -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},
 			},
 		},
 		{

diff --git 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
diff --git 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
@@ -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:            "org-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)
+}