From 009b9fc35e9c39e5904977156d9588cc43a07715 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 1 Aug 2024 14:00:59 -0500 Subject: [PATCH 1/5] chore: refactor patch custom organization route to live in enterprise This was before realizing single routes could be overwritten --- coderd/coderd.go | 7 --- coderd/roles.go | 47 ------------------ enterprise/coderd/coderd.go | 21 ++++---- enterprise/coderd/roles.go | 85 +++++++++++++++++---------------- enterprise/coderd/roles_test.go | 4 +- 5 files changed, 58 insertions(+), 106 deletions(-) diff --git a/coderd/coderd.go b/coderd/coderd.go index 6f8a59ad6efc6..22fe51ac27034 100644 --- 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 index 7bc67df7d8a52..89e6a964aba31 100644 --- 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 index e9e8d7d196af0..63e6c931d9f6b 100644 --- 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 index bebd36da0de14..0c80878468abd 100644 --- a/enterprise/coderd/roles.go +++ b/enterprise/coderd/roles.go @@ -1,7 +1,6 @@ package coderd import ( - "context" "fmt" "net/http" @@ -11,86 +10,92 @@ 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. @@ -98,30 +103,30 @@ func (h enterpriseCustomRoleHandler) PatchOrganizationRole(ctx context.Context, } 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 index 50c7c5cad08bb..bcc50ac4aaeb0 100644 --- 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") }) } From 85b49fd4bd05a0c6a6f6c19bc7530d905a23ec3e Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 1 Aug 2024 14:18:11 -0500 Subject: [PATCH 2/5] fmt --- enterprise/coderd/coderd.go | 1 - 1 file changed, 1 deletion(-) diff --git a/enterprise/coderd/coderd.go b/enterprise/coderd/coderd.go index 63e6c931d9f6b..39ea3e410c03c 100644 --- a/enterprise/coderd/coderd.go +++ b/enterprise/coderd/coderd.go @@ -269,7 +269,6 @@ func New(ctx context.Context, options *Options) (_ *API, err error) { httpmw.ExtractOrganizationParam(api.Database), ) r.Patch("/organizations/{organization}/members/roles", api.patchOrgRoles) - }) r.Route("/organizations/{organization}/groups", func(r chi.Router) { From 938634d5cd04f7151e189befda6a182b6670d598 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 2 Aug 2024 14:42:40 -0500 Subject: [PATCH 3/5] rebase --- enterprise/coderd/roles.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/enterprise/coderd/roles.go b/enterprise/coderd/roles.go index 0c80878468abd..53f5134ce1fc3 100644 --- a/enterprise/coderd/roles.go +++ b/enterprise/coderd/roles.go @@ -23,6 +23,7 @@ import ( // @Security CoderSessionToken // @Produce json // @Param organization path string true "Organization ID" format(uuid) +// @Param request body codersdk.PatchRoleRequest true "Upsert role request" // @Tags Members // @Success 200 {array} codersdk.Role // @Router /organizations/{organization}/members/roles [patch] @@ -42,7 +43,7 @@ func (api *API) patchOrgRoles(rw http.ResponseWriter, r *http.Request) { ) defer commitAudit() - var req codersdk.Role + var req codersdk.PatchRoleRequest if !httpapi.Read(ctx, rw, r, &req) { return } From 5ebb5734e51b5079d55e57ce4ba5185d46bba7b9 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 2 Aug 2024 14:50:38 -0500 Subject: [PATCH 4/5] make gen --- coderd/apidoc/docs.go | 39 ++++++++++++++++++++++++++++++++ coderd/apidoc/swagger.json | 39 ++++++++++++++++++++++++++++++++ docs/api/members.md | 38 ++++++++++++++++++++++++++++--- docs/api/schemas.md | 40 +++++++++++++++++++++++++++++++++ enterprise/coderd/roles_test.go | 2 +- 5 files changed, 154 insertions(+), 4 deletions(-) diff --git a/coderd/apidoc/docs.go b/coderd/apidoc/docs.go index 981be686df469..82bd5a01dae17 100644 --- a/coderd/apidoc/docs.go +++ b/coderd/apidoc/docs.go @@ -2432,6 +2432,15 @@ const docTemplate = `{ "name": "organization", "in": "path", "required": true + }, + { + "description": "Upsert role request", + "name": "request", + "in": "body", + "required": true, + "schema": { + "$ref": "#/definitions/codersdk.PatchRoleRequest" + } } ], "responses": { @@ -10747,6 +10756,36 @@ const docTemplate = `{ } } }, + "codersdk.PatchRoleRequest": { + "type": "object", + "properties": { + "display_name": { + "type": "string" + }, + "name": { + "type": "string" + }, + "organization_permissions": { + "description": "OrganizationPermissions are specific to the organization the role belongs to.", + "type": "array", + "items": { + "$ref": "#/definitions/codersdk.Permission" + } + }, + "site_permissions": { + "type": "array", + "items": { + "$ref": "#/definitions/codersdk.Permission" + } + }, + "user_permissions": { + "type": "array", + "items": { + "$ref": "#/definitions/codersdk.Permission" + } + } + } + }, "codersdk.PatchTemplateVersionRequest": { "type": "object", "properties": { diff --git a/coderd/apidoc/swagger.json b/coderd/apidoc/swagger.json index 14efc71711687..9f24fe1d41540 100644 --- a/coderd/apidoc/swagger.json +++ b/coderd/apidoc/swagger.json @@ -2124,6 +2124,15 @@ "name": "organization", "in": "path", "required": true + }, + { + "description": "Upsert role request", + "name": "request", + "in": "body", + "required": true, + "schema": { + "$ref": "#/definitions/codersdk.PatchRoleRequest" + } } ], "responses": { @@ -9683,6 +9692,36 @@ } } }, + "codersdk.PatchRoleRequest": { + "type": "object", + "properties": { + "display_name": { + "type": "string" + }, + "name": { + "type": "string" + }, + "organization_permissions": { + "description": "OrganizationPermissions are specific to the organization the role belongs to.", + "type": "array", + "items": { + "$ref": "#/definitions/codersdk.Permission" + } + }, + "site_permissions": { + "type": "array", + "items": { + "$ref": "#/definitions/codersdk.Permission" + } + }, + "user_permissions": { + "type": "array", + "items": { + "$ref": "#/definitions/codersdk.Permission" + } + } + } + }, "codersdk.PatchTemplateVersionRequest": { "type": "object", "properties": { diff --git a/docs/api/members.md b/docs/api/members.md index 1ecf490738f00..8dc8dea0aa707 100644 --- a/docs/api/members.md +++ b/docs/api/members.md @@ -215,17 +215,49 @@ To perform this operation, you must be authenticated. [Learn more](authenticatio ```shell # Example request using curl curl -X PATCH http://coder-server:8080/api/v2/organizations/{organization}/members/roles \ + -H 'Content-Type: application/json' \ -H 'Accept: application/json' \ -H 'Coder-Session-Token: API_KEY' ``` `PATCH /organizations/{organization}/members/roles` +> Body parameter + +```json +{ + "display_name": "string", + "name": "string", + "organization_permissions": [ + { + "action": "application_connect", + "negate": true, + "resource_type": "*" + } + ], + "site_permissions": [ + { + "action": "application_connect", + "negate": true, + "resource_type": "*" + } + ], + "user_permissions": [ + { + "action": "application_connect", + "negate": true, + "resource_type": "*" + } + ] +} +``` + ### Parameters -| Name | In | Type | Required | Description | -| -------------- | ---- | ------------ | -------- | --------------- | -| `organization` | path | string(uuid) | true | Organization ID | +| Name | In | Type | Required | Description | +| -------------- | ---- | ---------------------------------------------------------------- | -------- | ------------------- | +| `organization` | path | string(uuid) | true | Organization ID | +| `body` | body | [codersdk.PatchRoleRequest](schemas.md#codersdkpatchrolerequest) | true | Upsert role request | ### Example responses diff --git a/docs/api/schemas.md b/docs/api/schemas.md index 53ad820daf60c..8af3dd64b9ced 100644 --- a/docs/api/schemas.md +++ b/docs/api/schemas.md @@ -3698,6 +3698,46 @@ CreateWorkspaceRequest provides options for creating a new workspace. Only one o | `quota_allowance` | integer | false | | | | `remove_users` | array of string | false | | | +## codersdk.PatchRoleRequest + +```json +{ + "display_name": "string", + "name": "string", + "organization_permissions": [ + { + "action": "application_connect", + "negate": true, + "resource_type": "*" + } + ], + "site_permissions": [ + { + "action": "application_connect", + "negate": true, + "resource_type": "*" + } + ], + "user_permissions": [ + { + "action": "application_connect", + "negate": true, + "resource_type": "*" + } + ] +} +``` + +### Properties + +| Name | Type | Required | Restrictions | Description | +| -------------------------- | --------------------------------------------------- | -------- | ------------ | ------------------------------------------------------------------------------ | +| `display_name` | string | false | | | +| `name` | string | false | | | +| `organization_permissions` | array of [codersdk.Permission](#codersdkpermission) | false | | Organization permissions are specific to the organization the role belongs to. | +| `site_permissions` | array of [codersdk.Permission](#codersdkpermission) | false | | | +| `user_permissions` | array of [codersdk.Permission](#codersdkpermission) | false | | | + ## codersdk.PatchTemplateVersionRequest ```json diff --git a/enterprise/coderd/roles_test.go b/enterprise/coderd/roles_test.go index bcc50ac4aaeb0..05073efd05986 100644 --- a/enterprise/coderd/roles_test.go +++ b/enterprise/coderd/roles_test.go @@ -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, "Invalid request") + require.ErrorContains(t, err, "Resource not found") }) } From a8aa2bd2ac5b06542df2bcb5b93ba72e6a04ec3b Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 2 Aug 2024 15:15:56 -0500 Subject: [PATCH 5/5] swagger --- coderd/apidoc/docs.go | 3 +++ coderd/apidoc/swagger.json | 1 + enterprise/coderd/roles.go | 1 + 3 files changed, 5 insertions(+) diff --git a/coderd/apidoc/docs.go b/coderd/apidoc/docs.go index 82bd5a01dae17..674aab497d2c9 100644 --- a/coderd/apidoc/docs.go +++ b/coderd/apidoc/docs.go @@ -2416,6 +2416,9 @@ const docTemplate = `{ "CoderSessionToken": [] } ], + "consumes": [ + "application/json" + ], "produces": [ "application/json" ], diff --git a/coderd/apidoc/swagger.json b/coderd/apidoc/swagger.json index 9f24fe1d41540..e8c001abf453b 100644 --- a/coderd/apidoc/swagger.json +++ b/coderd/apidoc/swagger.json @@ -2112,6 +2112,7 @@ "CoderSessionToken": [] } ], + "consumes": ["application/json"], "produces": ["application/json"], "tags": ["Members"], "summary": "Upsert a custom organization role", diff --git a/enterprise/coderd/roles.go b/enterprise/coderd/roles.go index 53f5134ce1fc3..80bce8a1e975b 100644 --- a/enterprise/coderd/roles.go +++ b/enterprise/coderd/roles.go @@ -21,6 +21,7 @@ import ( // @Summary Upsert a custom organization role // @ID upsert-a-custom-organization-role // @Security CoderSessionToken +// @Accept json // @Produce json // @Param organization path string true "Organization ID" format(uuid) // @Param request body codersdk.PatchRoleRequest true "Upsert role request"