chore: refactor patch custom organization route to live in enterprise

This was before realizing single routes could be overwritten
coder · Emyrk · Aug 5, 2024 · Aug 1, 2024 · Aug 1, 2024 · Aug 2, 2024
commit 009b9fc35e9c39e5904977156d9588cc43a07715
diff --git a/coderd/coderd.go b/coderd/coderd.go
@@ -464,7 +464,6 @@ func New(options *Options) *API {
 		TemplateScheduleStore:       options.TemplateScheduleStore,
 		UserQuietHoursScheduleStore: options.UserQuietHoursScheduleStore,
 		AccessControlStore:          options.AccessControlStore,
-		CustomRoleHandler:           atomic.Pointer[CustomRoleHandler]{},
 		Experiments:                 experiments,
 		healthCheckGroup:            &singleflight.Group[string, *healthsdk.HealthcheckReport]{},
 		Acquirer: provisionerdserver.NewAcquirer(
@@ -476,8 +475,6 @@ func New(options *Options) *API {
 		dbRolluper: options.DatabaseRolluper,
 	}
 
-	var customRoleHandler CustomRoleHandler = &agplCustomRoleHandler{}
-	api.CustomRoleHandler.Store(&customRoleHandler)
 	api.AppearanceFetcher.Store(&appearance.DefaultFetcher)
 	api.PortSharer.Store(&portsharing.DefaultPortSharer)
 	buildInfo := codersdk.BuildInfoResponse{
@@ -887,8 +884,6 @@ func New(options *Options) *API {
 					r.Get("/", api.listMembers)
 					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) {
@@ -1327,8 +1322,6 @@ type API struct {
 	// passed to dbauthz.
 	AccessControlStore *atomic.Pointer[dbauthz.AccessControlStore]
 	PortSharer         atomic.Pointer[portsharing.PortSharer]
-	// CustomRoleHandler is the AGPL/Enterprise implementation for custom roles.
-	CustomRoleHandler atomic.Pointer[CustomRoleHandler]
 
 	HTTPAuth *HTTPAuthorizer
 

diff --git a/coderd/roles.go b/coderd/roles.go
@@ -1,7 +1,6 @@
 package coderd
 
 import (
-	"context"
 	"net/http"
 
 	"github.com/google/uuid"
@@ -16,52 +15,6 @@ import (
 	"github.com/coder/coder/v2/coderd/rbac"
 )
 
-// CustomRoleHandler handles AGPL/Enterprise interface for handling custom
-// 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.PatchRoleRequest) (codersdk.Role, bool)
-}
-
-type agplCustomRoleHandler struct{}
-
-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!",
-	})
-	return codersdk.Role{}, false
-}
-
-// patchRole will allow creating a custom organization role
-//
-// @Summary Upsert a custom organization role
-// @ID upsert-a-custom-organization-role
-// @Security CoderSessionToken
-// @Produce json
-// @Param organization path string true "Organization ID" format(uuid)
-// @Tags Members
-// @Success 200 {array} codersdk.Role
-// @Router /organizations/{organization}/members/roles [patch]
-func (api *API) patchOrgRoles(rw http.ResponseWriter, r *http.Request) {
-	var (
-		ctx          = r.Context()
-		handler      = *api.CustomRoleHandler.Load()
-		organization = httpmw.OrganizationParam(r)
-	)
-
-	var req codersdk.PatchRoleRequest
-	if !httpapi.Read(ctx, rw, r, &req) {
-		return
-	}
-
-	updated, ok := handler.PatchOrganizationRole(ctx, rw, r, organization.ID, req)
-	if !ok {
-		return
-	}
-
-	httpapi.Write(ctx, rw, http.StatusOK, updated)
-}
-
 // AssignableSiteRoles returns all site wide roles that can be assigned.
 //
 // @Summary Get site member roles

diff --git a/enterprise/coderd/coderd.go b/enterprise/coderd/coderd.go
@@ -261,6 +261,17 @@ func New(ctx context.Context, options *Options) (_ *API, err error) {
 			r.Delete("/organizations/{organization}", api.deleteOrganization)
 		})
 
+		r.Group(func(r chi.Router) {
+			r.Use(
+				apiKeyMiddleware,
+				api.RequireFeatureMW(codersdk.FeatureCustomRoles),
+				httpmw.RequireExperiment(api.AGPL.Experiments, codersdk.ExperimentCustomRoles),
+				httpmw.ExtractOrganizationParam(api.Database),
+			)
+			r.Patch("/organizations/{organization}/members/roles", api.patchOrgRoles)
+
+		})
+
 		r.Route("/organizations/{organization}/groups", func(r chi.Router) {
 			r.Use(
 				apiKeyMiddleware,
@@ -787,16 +798,6 @@ func (api *API) updateEntitlements(ctx context.Context) error {
 		api.AGPL.PortSharer.Store(&ps)
 	}
 
-	if initial, changed, enabled := featureChanged(codersdk.FeatureCustomRoles); shouldUpdate(initial, changed, enabled) {
-		var handler coderd.CustomRoleHandler = &enterpriseCustomRoleHandler{API: api, Enabled: enabled}
-		api.AGPL.CustomRoleHandler.Store(&handler)
-	}
-
-	if initial, changed, enabled := featureChanged(codersdk.FeatureMultipleOrganizations); shouldUpdate(initial, changed, enabled) {
-		var handler coderd.CustomRoleHandler = &enterpriseCustomRoleHandler{API: api, Enabled: enabled}
-		api.AGPL.CustomRoleHandler.Store(&handler)
-	}
-
 	// External token encryption is soft-enforced
 	featureExternalTokenEncryption := entitlements.Features[codersdk.FeatureExternalTokenEncryption]
 	featureExternalTokenEncryption.Enabled = len(api.ExternalTokenEncryption) > 0

diff --git a/enterprise/coderd/roles.go b/enterprise/coderd/roles.go
@@ -1,7 +1,6 @@
 package coderd
 
 import (
-	"context"
 	"fmt"
 	"net/http"
 
@@ -11,117 +10,123 @@ import (
 	"github.com/coder/coder/v2/coderd/database"
 	"github.com/coder/coder/v2/coderd/database/db2sdk"
 	"github.com/coder/coder/v2/coderd/httpapi"
+	"github.com/coder/coder/v2/coderd/httpmw"
 	"github.com/coder/coder/v2/coderd/rbac"
 	"github.com/coder/coder/v2/coderd/rbac/policy"
 	"github.com/coder/coder/v2/codersdk"
 )
 
-type enterpriseCustomRoleHandler struct {
-	API     *API
-	Enabled 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",
-		})
-		return codersdk.Role{}, false
-	}
-
+// patchRole will allow creating a custom organization role
+//
+// @Summary Upsert a custom organization role
+// @ID upsert-a-custom-organization-role
+// @Security CoderSessionToken
+// @Produce json
+// @Param organization path string true "Organization ID" format(uuid)
+// @Tags Members
+// @Success 200 {array} codersdk.Role
+// @Router /organizations/{organization}/members/roles [patch]
+func (api *API) patchOrgRoles(rw http.ResponseWriter, r *http.Request) {
 	var (
-		db                = h.API.Database
-		auditor           = h.API.AGPL.Auditor.Load()
+		ctx               = r.Context()
+		db                = api.Database
+		auditor           = api.AGPL.Auditor.Load()
+		organization      = httpmw.OrganizationParam(r)
 		aReq, commitAudit = audit.InitRequest[database.CustomRole](rw, &audit.RequestParams{
 			Audit:          *auditor,
-			Log:            h.API.Logger,
+			Log:            api.Logger,
 			Request:        r,
 			Action:         database.AuditActionWrite,
-			OrganizationID: orgID,
+			OrganizationID: organization.ID,
 		})
 	)
 	defer commitAudit()
 
+	var req codersdk.Role
+	if !httpapi.Read(ctx, rw, r, &req) {
+		return
+	}
+
 	// This check is not ideal, but we cannot enforce a unique role name in the db against
 	// the built-in role names.
-	if rbac.ReservedRoleName(role.Name) {
+	if rbac.ReservedRoleName(req.Name) {
 		httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
 			Message: "Reserved role name",
-			Detail:  fmt.Sprintf("%q is a reserved role name, and not allowed to be used", role.Name),
+			Detail:  fmt.Sprintf("%q is a reserved role name, and not allowed to be used", req.Name),
 		})
-		return codersdk.Role{}, false
+		return
 	}
 
-	if err := httpapi.NameValid(role.Name); err != nil {
+	if err := httpapi.NameValid(req.Name); err != nil {
 		httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
 			Message: "Invalid role name",
 			Detail:  err.Error(),
 		})
-		return codersdk.Role{}, false
+		return
 	}
 
 	// Only organization permissions are allowed to be granted
-	if len(role.SitePermissions) > 0 {
+	if len(req.SitePermissions) > 0 {
 		httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
 			Message: "Invalid request, not allowed to assign site wide permissions for an organization role.",
 			Detail:  "organization scoped roles may not contain site wide permissions",
 		})
-		return codersdk.Role{}, false
+		return
 	}
 
-	if len(role.UserPermissions) > 0 {
+	if len(req.UserPermissions) > 0 {
 		httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
 			Message: "Invalid request, not allowed to assign user permissions for an organization role.",
 			Detail:  "organization scoped roles may not contain user permissions",
 		})
-		return codersdk.Role{}, false
+		return
 	}
 
 	originalRoles, err := db.CustomRoles(ctx, database.CustomRolesParams{
 		LookupRoles: []database.NameOrganizationPair{
 			{
-				Name:           role.Name,
-				OrganizationID: orgID,
+				Name:           req.Name,
+				OrganizationID: organization.ID,
 			},
 		},
 		ExcludeOrgRoles: false,
-		OrganizationID:  orgID,
+		OrganizationID:  organization.ID,
 	})
 	// If it is a 404 (not found) error, ignore it.
 	if err != nil && !httpapi.Is404Error(err) {
 		httpapi.InternalServerError(rw, err)
-		return codersdk.Role{}, false
+		return
 	}
 	if len(originalRoles) == 1 {
 		// For auditing changes to a role.
 		aReq.Old = originalRoles[0]
 	}
 
 	inserted, err := db.UpsertCustomRole(ctx, database.UpsertCustomRoleParams{
-		Name:        role.Name,
-		DisplayName: role.DisplayName,
+		Name:        req.Name,
+		DisplayName: req.DisplayName,
 		OrganizationID: uuid.NullUUID{
-			UUID:  orgID,
+			UUID:  organization.ID,
 			Valid: true,
 		},
-		SitePermissions: db2sdk.List(role.SitePermissions, sdkPermissionToDB),
-		OrgPermissions:  db2sdk.List(role.OrganizationPermissions, sdkPermissionToDB),
-		UserPermissions: db2sdk.List(role.UserPermissions, sdkPermissionToDB),
+		SitePermissions: db2sdk.List(req.SitePermissions, sdkPermissionToDB),
+		OrgPermissions:  db2sdk.List(req.OrganizationPermissions, sdkPermissionToDB),
+		UserPermissions: db2sdk.List(req.UserPermissions, sdkPermissionToDB),
 	})
 	if httpapi.Is404Error(err) {
 		httpapi.ResourceNotFound(rw)
-		return codersdk.Role{}, false
+		return
 	}
 	if err != nil {
 		httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
 			Message: "Failed to update role permissions",
 			Detail:  err.Error(),
 		})
-		return codersdk.Role{}, false
+		return
 	}
 	aReq.New = inserted
 
-	return db2sdk.Role(inserted), true
+	httpapi.Write(ctx, rw, http.StatusOK, db2sdk.Role(inserted))
 }
 
 func sdkPermissionToDB(p codersdk.Permission) database.CustomRolePermission {

diff --git a/enterprise/coderd/roles_test.go b/enterprise/coderd/roles_test.go
@@ -125,7 +125,7 @@ func TestCustomOrganizationRole(t *testing.T) {
 
 		// Verify functionality is lost
 		_, err = owner.PatchOrganizationRole(ctx, templateAdminCustom(first.OrganizationID))
-		require.ErrorContains(t, err, "roles are not enabled")
+		require.ErrorContains(t, err, "Custom Roles is an Enterprise feature")
 
 		// Assign the custom template admin role
 		tmplAdmin, _ := coderdtest.CreateAnotherUser(t, owner, first.OrganizationID, rbac.RoleIdentifier{Name: role.Name, OrganizationID: first.OrganizationID})
@@ -308,7 +308,7 @@ func TestCustomOrganizationRole(t *testing.T) {
 
 		//nolint:gocritic // owner is required for this
 		_, err := owner.PatchOrganizationRole(ctx, newRole)
-		require.ErrorContains(t, err, "Resource not found")
+		require.ErrorContains(t, err, "Invalid request")
 	})
 }