fix: don't require organization_id in body when updating a custom role (coder#14102)

Author: aslilac

cli/organizationroles.go

@@ -203,7 +203,7 @@ func (r *RootCmd) editOrganizationRole(orgContext *OrganizationContext) *serpent
 				// Do not actually post
 				updated = customRole
 			} else {
-				updated, err = client.PatchOrganizationRole(ctx, org.ID, customRole)
+				updated, err = client.PatchOrganizationRole(ctx, customRole)
 				if err != nil {
 					return xerrors.Errorf("patch role: %w", err)
 				}

coderd/roles.go

@@ -20,12 +20,12 @@ import (
 // roles. Ideally only included in the enterprise package, but the routes are
 // intermixed with AGPL endpoints.
 type CustomRoleHandler interface {
-	PatchOrganizationRole(ctx context.Context, rw http.ResponseWriter, r *http.Request, orgID uuid.UUID, role codersdk.Role) (codersdk.Role, bool)
+	PatchOrganizationRole(ctx context.Context, rw http.ResponseWriter, r *http.Request, orgID uuid.UUID, role codersdk.PatchRoleRequest) (codersdk.Role, bool)
 }
 
 type agplCustomRoleHandler struct{}
 
-func (agplCustomRoleHandler) PatchOrganizationRole(ctx context.Context, rw http.ResponseWriter, _ *http.Request, _ uuid.UUID, _ codersdk.Role) (codersdk.Role, bool) {
+func (agplCustomRoleHandler) PatchOrganizationRole(ctx context.Context, rw http.ResponseWriter, _ *http.Request, _ uuid.UUID, _ codersdk.PatchRoleRequest) (codersdk.Role, bool) {
 	httpapi.Write(ctx, rw, http.StatusForbidden, codersdk.Response{
 		Message: "Creating and updating custom roles is an Enterprise feature. Contact sales!",
 	})
@@ -49,7 +49,7 @@ func (api *API) patchOrgRoles(rw http.ResponseWriter, r *http.Request) {
 		organization = httpmw.OrganizationParam(r)
 	)
 
-	var req codersdk.Role
+	var req codersdk.PatchRoleRequest
 	if !httpapi.Read(ctx, rw, r, &req) {
 		return
 	}

codersdk/roles.go

@@ -50,7 +50,7 @@ type Permission struct {
 	Action       RBACAction   `json:"action"`
 }
 
-// Role is a longer form of SlimRole used to edit custom roles.
+// Role is a longer form of SlimRole that includes permissions details.
 type Role struct {
 	Name            string       `json:"name" table:"name,default_sort" validate:"username"`
 	OrganizationID  string       `json:"organization_id,omitempty" table:"organization_id" format:"uuid"`
@@ -61,6 +61,16 @@ type Role struct {
 	UserPermissions         []Permission `json:"user_permissions" table:"user_permissions"`
 }
 
+// PatchRoleRequest is used to edit custom roles.
+type PatchRoleRequest struct {
+	Name            string       `json:"name" table:"name,default_sort" validate:"username"`
+	DisplayName     string       `json:"display_name" table:"display_name"`
+	SitePermissions []Permission `json:"site_permissions" table:"site_permissions"`
+	// OrganizationPermissions are specific to the organization the role belongs to.
+	OrganizationPermissions []Permission `json:"organization_permissions" table:"organization_permissions"`
+	UserPermissions         []Permission `json:"user_permissions" table:"user_permissions"`
+}
+
 // FullName returns the role name scoped to the organization ID. This is useful if
 // printing a set of roles from different scopes, as duplicated names across multiple
 // scopes will become unique.
@@ -73,18 +83,26 @@ func (r Role) FullName() string {
 }
 
 // PatchOrganizationRole will upsert a custom organization role
-func (c *Client) PatchOrganizationRole(ctx context.Context, organizationID uuid.UUID, req Role) (Role, error) {
+func (c *Client) PatchOrganizationRole(ctx context.Context, role Role) (Role, error) {
+	req := PatchRoleRequest{
+		Name:                    role.Name,
+		DisplayName:             role.DisplayName,
+		SitePermissions:         role.SitePermissions,
+		OrganizationPermissions: role.OrganizationPermissions,
+		UserPermissions:         role.UserPermissions,
+	}
+
 	res, err := c.Request(ctx, http.MethodPatch,
-		fmt.Sprintf("/api/v2/organizations/%s/members/roles", organizationID.String()), req)
+		fmt.Sprintf("/api/v2/organizations/%s/members/roles", role.OrganizationID), req)
 	if err != nil {
 		return Role{}, err
 	}
 	defer res.Body.Close()
 	if res.StatusCode != http.StatusOK {
 		return Role{}, ReadBodyAsError(res)
 	}
-	var role Role
-	return role, json.NewDecoder(res.Body).Decode(&role)
+	var r Role
+	return r, json.NewDecoder(res.Body).Decode(&r)
 }
 
 // ListSiteRoles lists all assignable site wide roles.

enterprise/cli/organizationmembers_test.go

@@ -36,7 +36,7 @@ func TestEnterpriseListOrganizationMembers(t *testing.T) {
 
 		ctx := testutil.Context(t, testutil.WaitMedium)
 		//nolint:gocritic // only owners can patch roles
-		customRole, err := ownerClient.PatchOrganizationRole(ctx, owner.OrganizationID, codersdk.Role{
+		customRole, err := ownerClient.PatchOrganizationRole(ctx, codersdk.Role{
 			Name:            "custom",
 			OrganizationID:  owner.OrganizationID.String(),
 			DisplayName:     "Custom Role",
@@ -89,7 +89,7 @@ func TestAssignOrganizationMemberRole(t *testing.T) {
 
 		ctx := testutil.Context(t, testutil.WaitMedium)
 		// nolint:gocritic // requires owner role to create
-		customRole, err := ownerClient.PatchOrganizationRole(ctx, owner.OrganizationID, codersdk.Role{
+		customRole, err := ownerClient.PatchOrganizationRole(ctx, codersdk.Role{
 			Name:            "custom-role",
 			OrganizationID:  owner.OrganizationID.String(),
 			DisplayName:     "Custom Role",

enterprise/cli/templatecreate_test.go

@@ -168,7 +168,7 @@ func TestTemplateCreate(t *testing.T) {
 		ctx := testutil.Context(t, testutil.WaitMedium)
 
 		//nolint:gocritic // owner required to make custom roles
-		orgTemplateAdminRole, err := ownerClient.PatchOrganizationRole(ctx, secondOrg.ID, codersdk.Role{
+		orgTemplateAdminRole, err := ownerClient.PatchOrganizationRole(ctx, codersdk.Role{
 			Name:           "org-template-admin",
 			OrganizationID: secondOrg.ID.String(),
 			OrganizationPermissions: codersdk.CreatePermissions(map[codersdk.RBACResource][]codersdk.RBACAction{

enterprise/coderd/roles.go

@@ -21,7 +21,7 @@ type enterpriseCustomRoleHandler struct {
 	Enabled bool
 }
 
-func (h enterpriseCustomRoleHandler) PatchOrganizationRole(ctx context.Context, rw http.ResponseWriter, r *http.Request, orgID uuid.UUID, role codersdk.Role) (codersdk.Role, bool) {
+func (h enterpriseCustomRoleHandler) PatchOrganizationRole(ctx context.Context, rw http.ResponseWriter, r *http.Request, orgID uuid.UUID, role codersdk.PatchRoleRequest) (codersdk.Role, bool) {
 	if !h.Enabled {
 		httpapi.Write(ctx, rw, http.StatusForbidden, codersdk.Response{
 			Message: "Custom roles are not enabled",
@@ -77,14 +77,6 @@ func (h enterpriseCustomRoleHandler) PatchOrganizationRole(ctx context.Context,
 		return codersdk.Role{}, false
 	}
 
-	if role.OrganizationID != orgID.String() {
-		httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
-			Message: "Invalid request, organization in role and url must match",
-			Detail:  fmt.Sprintf("role organization=%q does not match URL=%q", role.OrganizationID, orgID.String()),
-		})
-		return codersdk.Role{}, false
-	}
-
 	originalRoles, err := db.CustomRoles(ctx, database.CustomRolesParams{
 		LookupRoles: []database.NameOrganizationPair{
 			{

enterprise/coderd/roles_test.go

@@ -57,7 +57,7 @@ func TestCustomOrganizationRole(t *testing.T) {
 		ctx := testutil.Context(t, testutil.WaitMedium)
 
 		//nolint:gocritic // owner is required for this
-		role, err := owner.PatchOrganizationRole(ctx, first.OrganizationID, templateAdminCustom(first.OrganizationID))
+		role, err := owner.PatchOrganizationRole(ctx, templateAdminCustom(first.OrganizationID))
 		require.NoError(t, err, "upsert role")
 
 		// Assign the custom template admin role
@@ -111,7 +111,7 @@ func TestCustomOrganizationRole(t *testing.T) {
 		ctx := testutil.Context(t, testutil.WaitMedium)
 
 		//nolint:gocritic // owner is required for this
-		role, err := owner.PatchOrganizationRole(ctx, first.OrganizationID, templateAdminCustom(first.OrganizationID))
+		role, err := owner.PatchOrganizationRole(ctx, templateAdminCustom(first.OrganizationID))
 		require.NoError(t, err, "upsert role")
 
 		// Remove the license to block enterprise functionality
@@ -124,7 +124,7 @@ func TestCustomOrganizationRole(t *testing.T) {
 		}
 
 		// Verify functionality is lost
-		_, err = owner.PatchOrganizationRole(ctx, first.OrganizationID, templateAdminCustom(first.OrganizationID))
+		_, err = owner.PatchOrganizationRole(ctx, templateAdminCustom(first.OrganizationID))
 		require.ErrorContains(t, err, "roles are not enabled")
 
 		// Assign the custom template admin role
@@ -152,7 +152,7 @@ func TestCustomOrganizationRole(t *testing.T) {
 
 		ctx := testutil.Context(t, testutil.WaitMedium)
 		//nolint:gocritic // owner is required for this
-		role, err := owner.PatchOrganizationRole(ctx, first.OrganizationID, templateAdminCustom(first.OrganizationID))
+		role, err := owner.PatchOrganizationRole(ctx, templateAdminCustom(first.OrganizationID))
 		require.NoError(t, err, "upsert role")
 
 		// Assign the custom template admin role
@@ -169,7 +169,7 @@ func TestCustomOrganizationRole(t *testing.T) {
 		newRole.SitePermissions = nil
 		newRole.OrganizationPermissions = nil
 		newRole.UserPermissions = nil
-		_, err = owner.PatchOrganizationRole(ctx, first.OrganizationID, newRole)
+		_, err = owner.PatchOrganizationRole(ctx, newRole)
 		require.NoError(t, err, "upsert role with override")
 
 		// The role should no longer have template perms
@@ -203,7 +203,7 @@ func TestCustomOrganizationRole(t *testing.T) {
 		ctx := testutil.Context(t, testutil.WaitMedium)
 
 		//nolint:gocritic // owner is required for this
-		_, err := owner.PatchOrganizationRole(ctx, first.OrganizationID, codersdk.Role{
+		_, err := owner.PatchOrganizationRole(ctx, codersdk.Role{
 			Name:                    "Bad_Name", // No underscores allowed
 			DisplayName:             "Testing Purposes",
 			OrganizationID:          first.OrganizationID.String(),
@@ -232,38 +232,17 @@ func TestCustomOrganizationRole(t *testing.T) {
 		ctx := testutil.Context(t, testutil.WaitMedium)
 
 		//nolint:gocritic // owner is required for this
-		_, err := owner.PatchOrganizationRole(ctx, first.OrganizationID, codersdk.Role{
+		_, err := owner.PatchOrganizationRole(ctx, codersdk.Role{
 			Name:                    "owner", // Reserved
 			DisplayName:             "Testing Purposes",
+			OrganizationID:          first.OrganizationID.String(),
 			SitePermissions:         nil,
 			OrganizationPermissions: nil,
 			UserPermissions:         nil,
 		})
 		require.ErrorContains(t, err, "Reserved")
 	})
 
-	t.Run("MismatchedOrganizations", func(t *testing.T) {
-		t.Parallel()
-		dv := coderdtest.DeploymentValues(t)
-		dv.Experiments = []string{string(codersdk.ExperimentCustomRoles)}
-		owner, first := coderdenttest.New(t, &coderdenttest.Options{
-			Options: &coderdtest.Options{
-				DeploymentValues: dv,
-			},
-			LicenseOptions: &coderdenttest.LicenseOptions{
-				Features: license.Features{
-					codersdk.FeatureCustomRoles: 1,
-				},
-			},
-		})
-
-		ctx := testutil.Context(t, testutil.WaitMedium)
-
-		//nolint:gocritic // owner is required for this
-		_, err := owner.PatchOrganizationRole(ctx, first.OrganizationID, templateAdminCustom(uuid.New()))
-		require.ErrorContains(t, err, "does not match")
-	})
-
 	// Attempt to add site & user permissions, which is not allowed
 	t.Run("ExcessPermissions", func(t *testing.T) {
 		t.Parallel()
@@ -291,7 +270,7 @@ func TestCustomOrganizationRole(t *testing.T) {
 		}
 
 		//nolint:gocritic // owner is required for this
-		_, err := owner.PatchOrganizationRole(ctx, first.OrganizationID, siteRole)
+		_, err := owner.PatchOrganizationRole(ctx, siteRole)
 		require.ErrorContains(t, err, "site wide permissions")
 
 		userRole := templateAdminCustom(first.OrganizationID)
@@ -303,11 +282,11 @@ func TestCustomOrganizationRole(t *testing.T) {
 		}
 
 		//nolint:gocritic // owner is required for this
-		_, err = owner.PatchOrganizationRole(ctx, first.OrganizationID, userRole)
+		_, err = owner.PatchOrganizationRole(ctx, userRole)
 		require.ErrorContains(t, err, "not allowed to assign user permissions")
 	})
 
-	t.Run("InvalidUUID", func(t *testing.T) {
+	t.Run("NotFound", func(t *testing.T) {
 		t.Parallel()
 		dv := coderdtest.DeploymentValues(t)
 		dv.Experiments = []string{string(codersdk.ExperimentCustomRoles)}
@@ -328,8 +307,8 @@ func TestCustomOrganizationRole(t *testing.T) {
 		newRole.OrganizationID = "0000" // This is not a valid uuid
 
 		//nolint:gocritic // owner is required for this
-		_, err := owner.PatchOrganizationRole(ctx, first.OrganizationID, newRole)
-		require.ErrorContains(t, err, "Invalid request")
+		_, err := owner.PatchOrganizationRole(ctx, newRole)
+		require.ErrorContains(t, err, "Resource not found")
 	})
 }
 

enterprise/coderd/templates_test.go

@@ -751,7 +751,7 @@ func TestTemplates(t *testing.T) {
 		})
 
 		//nolint:gocritic // owner required to make custom roles
-		orgTemplateAdminRole, err := ownerClient.PatchOrganizationRole(ctx, secondOrg.ID, codersdk.Role{
+		orgTemplateAdminRole, err := ownerClient.PatchOrganizationRole(ctx, codersdk.Role{
 			Name:           "org-template-admin",
 			OrganizationID: secondOrg.ID.String(),
 			OrganizationPermissions: codersdk.CreatePermissions(map[codersdk.RBACResource][]codersdk.RBACAction{

enterprise/coderd/userauth_test.go

@@ -705,7 +705,7 @@ func TestEnterpriseUserLogin(t *testing.T) {
 
 		ctx := testutil.Context(t, testutil.WaitShort)
 		//nolint:gocritic // owner required
-		customRole, err := ownerClient.PatchOrganizationRole(ctx, owner.OrganizationID, codersdk.Role{
+		customRole, err := ownerClient.PatchOrganizationRole(ctx, codersdk.Role{
 			Name:                    "custom-role",
 			OrganizationID:          owner.OrganizationID.String(),
 			OrganizationPermissions: []codersdk.Permission{},

enterprise/coderd/users_test.go

@@ -271,7 +271,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{
+	auditorRole, err := client.PatchOrganizationRole(ctx, codersdk.Role{
 		Name:            "org-template-admin",
 		OrganizationID:  owner.OrganizationID.String(),
 		DisplayName:     "Template Admin",