fixup test to assign org role instead of site

coder · Emyrk · May 29, 2024 · May 23, 2024 · May 23, 2024 · May 24, 2024
commit d231cdf02de606c41f9a6d8335db04e0b89ba459
diff --git a/coderd/coderd.go b/coderd/coderd.go
@@ -826,9 +826,12 @@ func New(options *Options) *API {
 					})
 				})
 				r.Route("/members", func(r chi.Router) {
-					r.Get("/roles", api.assignableOrgRoles)
-					r.With(httpmw.RequireExperiment(api.Experiments, codersdk.ExperimentCustomRoles)).
-						Patch("/roles", api.patchOrgRoles)
+					r.Route("/roles", func(r chi.Router) {
+						r.Get("/", api.assignableOrgRoles)
+						r.With(httpmw.RequireExperiment(api.Experiments, codersdk.ExperimentCustomRoles)).
+							Patch("/", api.patchOrgRoles)
+					})
+
 					r.Route("/{user}", func(r chi.Router) {
 						r.Use(
 							httpmw.ExtractOrganizationMemberParam(options.Database),

diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go
@@ -600,14 +600,25 @@ func (q *querier) canAssignRoles(ctx context.Context, orgID *uuid.UUID, added, r
 	customRoles := make([]string, 0)
 	// Validate that the roles being assigned are valid.
 	for _, r := range grantedRoles {
-		_, isOrgRole := rbac.IsOrgRole(r)
+		roleOrgIDStr, isOrgRole := rbac.IsOrgRole(r)
 		if shouldBeOrgRoles && !isOrgRole {
 			return xerrors.Errorf("Must only update org roles")
 		}
 		if !shouldBeOrgRoles && isOrgRole {
 			return xerrors.Errorf("Must only update site wide roles")
 		}
 
+		if shouldBeOrgRoles {
+			roleOrgID, err := uuid.Parse(roleOrgIDStr)
+			if err != nil {
+				return xerrors.Errorf("role %q has invalid uuid for org: %w", r, err)
+			}
+
+			if roleOrgID != *orgID {
+				return xerrors.Errorf("attempted to assign role from a different org, role %q to %q", r, (*orgID).String())
+			}
+		}
+
 		// All roles should be valid roles
 		if _, err := rbac.RoleByName(r); err != nil {
 			customRoles = append(customRoles, r)

diff --git a/coderd/members.go b/coderd/members.go
@@ -1,13 +1,8 @@
 package coderd
 
 import (
-	"context"
 	"net/http"
 
-	"github.com/google/uuid"
-
-	"golang.org/x/xerrors"
-
 	"github.com/coder/coder/v2/coderd/database/db2sdk"
 	"github.com/coder/coder/v2/coderd/rbac"
 
@@ -48,7 +43,7 @@ func (api *API) putMemberRoles(rw http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	updatedUser, err := api.updateOrganizationMemberRoles(ctx, database.UpdateMemberRolesParams{
+	updatedUser, err := api.Database.UpdateMemberRoles(ctx, database.UpdateMemberRolesParams{
 		GrantedRoles: params.Roles,
 		UserID:       member.UserID,
 		OrgID:        organization.ID,
@@ -63,36 +58,6 @@ func (api *API) putMemberRoles(rw http.ResponseWriter, r *http.Request) {
 	httpapi.Write(ctx, rw, http.StatusOK, convertOrganizationMember(updatedUser))
 }
 
-func (api *API) updateOrganizationMemberRoles(ctx context.Context, args database.UpdateMemberRolesParams) (database.OrganizationMember, error) {
-	// Enforce only site wide roles
-	for _, r := range args.GrantedRoles {
-		// Must be an org role for the org in the args
-		orgID, ok := rbac.IsOrgRole(r)
-		if !ok {
-			return database.OrganizationMember{}, xerrors.Errorf("must only update organization roles")
-		}
-
-		roleOrg, err := uuid.Parse(orgID)
-		if err != nil {
-			return database.OrganizationMember{}, xerrors.Errorf("Role must have proper UUIDs for organization, %q does not", r)
-		}
-
-		if roleOrg != args.OrgID {
-			return database.OrganizationMember{}, xerrors.Errorf("Must only pass roles for org %q", args.OrgID.String())
-		}
-
-		if _, err := rbac.RoleByName(r); err != nil {
-			return database.OrganizationMember{}, xerrors.Errorf("%q is not a supported organization role", r)
-		}
-	}
-
-	updatedUser, err := api.Database.UpdateMemberRoles(ctx, args)
-	if err != nil {
-		return database.OrganizationMember{}, xerrors.Errorf("Update site roles: %w", err)
-	}
-	return updatedUser, nil
-}
-
 func convertOrganizationMember(mem database.OrganizationMember) codersdk.OrganizationMember {
 	convertedMember := codersdk.OrganizationMember{
 		UserID:         mem.UserID,

diff --git a/codersdk/roles.go b/codersdk/roles.go
@@ -44,9 +44,21 @@ type Role struct {
 	UserPermissions         []Permission            `json:"user_permissions" table:"user_permissions"`
 }
 
-// PatchRole will upsert a custom site wide role
-func (c *Client) PatchRole(ctx context.Context, req Role) (Role, error) {
-	res, err := c.Request(ctx, http.MethodPatch, "/api/v2/users/roles", req)
+// 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.
+// In practice, this is primarily used in testing.
+func (r Role) FullName() string {
+	if r.OrganizationID == "" {
+		return r.Name
+	}
+	return r.Name + ":" + r.OrganizationID
+}
+
+// PatchOrganizationRole will upsert a custom organization role
+func (c *Client) PatchOrganizationRole(ctx context.Context, organizationID uuid.UUID, req Role) (Role, error) {
+	res, err := c.Request(ctx, http.MethodPatch,
+		fmt.Sprintf("/api/v2/organizations/%s/members/roles", organizationID.String()), req)
 	if err != nil {
 		return Role{}, err
 	}

diff --git a/enterprise/coderd/coderd.go b/enterprise/coderd/coderd.go
@@ -327,22 +327,6 @@ func New(ctx context.Context, options *Options) (_ *API, err error) {
 			})
 		})
 
-		r.Route("/users/roles", func(r chi.Router) {
-			r.Use(
-				apiKeyMiddleware,
-			)
-			r.Group(func(r chi.Router) {
-				r.Use(
-					api.customRolesEnabledMW,
-				)
-				r.Patch("/", api.patchRole)
-			})
-			// Unfortunate, but this r.Route overrides the AGPL roles route.
-			// The AGPL does not have the entitlements to block the licensed
-			// routes, so we need to duplicate the AGPL here.
-			r.Get("/", api.AGPL.AssignableSiteRoles)
-		})
-
 		r.Route("/users/{user}/quiet-hours", func(r chi.Router) {
 			r.Use(
 				api.autostopRequirementEnabledMW,

diff --git a/enterprise/coderd/roles.go b/enterprise/coderd/roles.go
@@ -107,80 +107,3 @@ func (h enterpriseCustomRoleHandler) PatchOrganizationRole(ctx context.Context,
 
 	return db2sdk.Role(convertedInsert), true
 }
-
-// patchRole will allow creating a custom role
-//
-// @Summary Upsert a custom site-wide role
-// @ID upsert-a-custom-site-wide-role
-// @Security CoderSessionToken
-// @Produce json
-// @Tags Members
-// @Success 200 {array} codersdk.Role
-// @Router /users/roles [patch]
-func (api *API) patchRole(rw http.ResponseWriter, r *http.Request) {
-	ctx := r.Context()
-
-	var req codersdk.Role
-	if !httpapi.Read(ctx, rw, r, &req) {
-		return
-	}
-
-	if err := httpapi.NameValid(req.Name); err != nil {
-		httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
-			Message: "Invalid role name",
-			Detail:  err.Error(),
-		})
-		return
-	}
-
-	if len(req.OrganizationPermissions) > 0 {
-		// Org perms should be assigned only in org specific roles. Otherwise,
-		// it gets complicated to keep track of who can do what.
-		httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
-			Message: "Invalid request, not allowed to assign organization permissions for a site wide role.",
-			Detail:  "site wide roles may not contain organization specific permissions",
-		})
-		return
-	}
-
-	// Make sure all permissions inputted are valid according to our policy.
-	rbacRole := db2sdk.RoleToRBAC(req)
-	args, err := rolestore.ConvertRoleToDB(rbacRole)
-	if err != nil {
-		httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
-			Message: "Invalid request",
-			Detail:  err.Error(),
-		})
-		return
-	}
-
-	inserted, err := api.Database.UpsertCustomRole(ctx, database.UpsertCustomRoleParams{
-		Name:            args.Name,
-		DisplayName:     args.DisplayName,
-		SitePermissions: args.SitePermissions,
-		OrgPermissions:  args.OrgPermissions,
-		UserPermissions: args.UserPermissions,
-	})
-	if httpapi.Is404Error(err) {
-		httpapi.ResourceNotFound(rw)
-		return
-	}
-	if err != nil {
-		httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
-			Message: "Failed to update role permissions",
-			Detail:  err.Error(),
-		})
-		return
-	}
-
-	convertedInsert, err := rolestore.ConvertDBRole(inserted)
-	if err != nil {
-		httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
-			Message: "Permissions were updated, unable to read them back out of the database.",
-			Detail:  err.Error(),
-		})
-		return
-	}
-
-	httpapi.Write(ctx, rw, http.StatusOK, db2sdk.Role(convertedInsert))
-}