Skip to content

Commit 166467c

Browse files
aslilacaslilac
authored
fix: don't require organization_id in body when updating a custom role (coder#14102)
1 parent e2cec45 commit 166467c

11 files changed

+56
-58
lines changed

‎cli/organizationroles.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -203,7 +203,7 @@ func (r *RootCmd) editOrganizationRole(orgContext *OrganizationContext) *serpent
203203
// Do not actually post
204204
updated = customRole
205205
} else {
206-
updated, err = client.PatchOrganizationRole(ctx, org.ID, customRole)
206+
updated, err = client.PatchOrganizationRole(ctx, customRole)
207207
if err != nil {
208208
return xerrors.Errorf("patch role: %w", err)
209209
}

‎coderd/roles.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -20,12 +20,12 @@ import (
2020
// roles. Ideally only included in the enterprise package, but the routes are
2121
// intermixed with AGPL endpoints.
2222
type CustomRoleHandler interface {
23-
PatchOrganizationRole(ctx context.Context, rw http.ResponseWriter, r *http.Request, orgID uuid.UUID, role codersdk.Role) (codersdk.Role, bool)
23+
PatchOrganizationRole(ctx context.Context, rw http.ResponseWriter, r *http.Request, orgID uuid.UUID, role codersdk.PatchRoleRequest) (codersdk.Role, bool)
2424
}
2525

2626
type agplCustomRoleHandler struct{}
2727

28-
func (agplCustomRoleHandler) PatchOrganizationRole(ctx context.Context, rw http.ResponseWriter, _ *http.Request, _ uuid.UUID, _ codersdk.Role) (codersdk.Role, bool) {
28+
func (agplCustomRoleHandler) PatchOrganizationRole(ctx context.Context, rw http.ResponseWriter, _ *http.Request, _ uuid.UUID, _ codersdk.PatchRoleRequest) (codersdk.Role, bool) {
2929
httpapi.Write(ctx, rw, http.StatusForbidden, codersdk.Response{
3030
Message: "Creating and updating custom roles is an Enterprise feature. Contact sales!",
3131
})
@@ -49,7 +49,7 @@ func (api *API) patchOrgRoles(rw http.ResponseWriter, r *http.Request) {
4949
organization = httpmw.OrganizationParam(r)
5050
)
5151

52-
var req codersdk.Role
52+
var req codersdk.PatchRoleRequest
5353
if !httpapi.Read(ctx, rw, r, &req) {
5454
return
5555
}

‎codersdk/roles.go

Lines changed: 23 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,7 @@ type Permission struct {
5050
Action RBACAction `json:"action"`
5151
}
5252

53-
// Role is a longer form of SlimRole used to edit custom roles.
53+
// Role is a longer form of SlimRole that includes permissions details.
5454
type Role struct {
5555
Name string `json:"name" table:"name,default_sort" validate:"username"`
5656
OrganizationID string `json:"organization_id,omitempty" table:"organization_id" format:"uuid"`
@@ -61,6 +61,16 @@ type Role struct {
6161
UserPermissions []Permission `json:"user_permissions" table:"user_permissions"`
6262
}
6363

64+
// PatchRoleRequest is used to edit custom roles.
65+
type PatchRoleRequest struct {
66+
Name string `json:"name" table:"name,default_sort" validate:"username"`
67+
DisplayName string `json:"display_name" table:"display_name"`
68+
SitePermissions []Permission `json:"site_permissions" table:"site_permissions"`
69+
// OrganizationPermissions are specific to the organization the role belongs to.
70+
OrganizationPermissions []Permission `json:"organization_permissions" table:"organization_permissions"`
71+
UserPermissions []Permission `json:"user_permissions" table:"user_permissions"`
72+
}
73+
6474
// FullName returns the role name scoped to the organization ID. This is useful if
6575
// printing a set of roles from different scopes, as duplicated names across multiple
6676
// scopes will become unique.
@@ -73,18 +83,26 @@ func (r Role) FullName() string {
7383
}
7484

7585
// PatchOrganizationRole will upsert a custom organization role
76-
func (c *Client) PatchOrganizationRole(ctx context.Context, organizationID uuid.UUID, req Role) (Role, error) {
86+
func (c *Client) PatchOrganizationRole(ctx context.Context, role Role) (Role, error) {
87+
req := PatchRoleRequest{
88+
Name: role.Name,
89+
DisplayName: role.DisplayName,
90+
SitePermissions: role.SitePermissions,
91+
OrganizationPermissions: role.OrganizationPermissions,
92+
UserPermissions: role.UserPermissions,
93+
}
94+
7795
res, err := c.Request(ctx, http.MethodPatch,
78-
fmt.Sprintf("/api/v2/organizations/%s/members/roles", organizationID.String()), req)
96+
fmt.Sprintf("/api/v2/organizations/%s/members/roles", role.OrganizationID), req)
7997
if err != nil {
8098
return Role{}, err
8199
}
82100
defer res.Body.Close()
83101
if res.StatusCode != http.StatusOK {
84102
return Role{}, ReadBodyAsError(res)
85103
}
86-
var role Role
87-
return role, json.NewDecoder(res.Body).Decode(&role)
104+
var r Role
105+
return r, json.NewDecoder(res.Body).Decode(&r)
88106
}
89107

90108
// ListSiteRoles lists all assignable site wide roles.

‎enterprise/cli/organizationmembers_test.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@ func TestEnterpriseListOrganizationMembers(t *testing.T) {
3636

3737
ctx := testutil.Context(t, testutil.WaitMedium)
3838
//nolint:gocritic // only owners can patch roles
39-
customRole, err := ownerClient.PatchOrganizationRole(ctx, owner.OrganizationID, codersdk.Role{
39+
customRole, err := ownerClient.PatchOrganizationRole(ctx, codersdk.Role{
4040
Name: "custom",
4141
OrganizationID: owner.OrganizationID.String(),
4242
DisplayName: "Custom Role",
@@ -89,7 +89,7 @@ func TestAssignOrganizationMemberRole(t *testing.T) {
8989

9090
ctx := testutil.Context(t, testutil.WaitMedium)
9191
// nolint:gocritic // requires owner role to create
92-
customRole, err := ownerClient.PatchOrganizationRole(ctx, owner.OrganizationID, codersdk.Role{
92+
customRole, err := ownerClient.PatchOrganizationRole(ctx, codersdk.Role{
9393
Name: "custom-role",
9494
OrganizationID: owner.OrganizationID.String(),
9595
DisplayName: "Custom Role",

‎enterprise/cli/templatecreate_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -168,7 +168,7 @@ func TestTemplateCreate(t *testing.T) {
168168
ctx := testutil.Context(t, testutil.WaitMedium)
169169

170170
//nolint:gocritic // owner required to make custom roles
171-
orgTemplateAdminRole, err := ownerClient.PatchOrganizationRole(ctx, secondOrg.ID, codersdk.Role{
171+
orgTemplateAdminRole, err := ownerClient.PatchOrganizationRole(ctx, codersdk.Role{
172172
Name: "org-template-admin",
173173
OrganizationID: secondOrg.ID.String(),
174174
OrganizationPermissions: codersdk.CreatePermissions(map[codersdk.RBACResource][]codersdk.RBACAction{

‎enterprise/coderd/roles.go

Lines changed: 1 addition & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ type enterpriseCustomRoleHandler struct {
2121
Enabled bool
2222
}
2323

24-
func (h enterpriseCustomRoleHandler) PatchOrganizationRole(ctx context.Context, rw http.ResponseWriter, r *http.Request, orgID uuid.UUID, role codersdk.Role) (codersdk.Role, bool) {
24+
func (h enterpriseCustomRoleHandler) PatchOrganizationRole(ctx context.Context, rw http.ResponseWriter, r *http.Request, orgID uuid.UUID, role codersdk.PatchRoleRequest) (codersdk.Role, bool) {
2525
if !h.Enabled {
2626
httpapi.Write(ctx, rw, http.StatusForbidden, codersdk.Response{
2727
Message: "Custom roles are not enabled",
@@ -77,14 +77,6 @@ func (h enterpriseCustomRoleHandler) PatchOrganizationRole(ctx context.Context,
7777
return codersdk.Role{}, false
7878
}
7979

80-
if role.OrganizationID != orgID.String() {
81-
httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
82-
Message: "Invalid request, organization in role and url must match",
83-
Detail: fmt.Sprintf("role organization=%q does not match URL=%q", role.OrganizationID, orgID.String()),
84-
})
85-
return codersdk.Role{}, false
86-
}
87-
8880
originalRoles, err := db.CustomRoles(ctx, database.CustomRolesParams{
8981
LookupRoles: []database.NameOrganizationPair{
9082
{

‎enterprise/coderd/roles_test.go

Lines changed: 13 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,7 @@ func TestCustomOrganizationRole(t *testing.T) {
5757
ctx := testutil.Context(t, testutil.WaitMedium)
5858

5959
//nolint:gocritic // owner is required for this
60-
role, err := owner.PatchOrganizationRole(ctx, first.OrganizationID, templateAdminCustom(first.OrganizationID))
60+
role, err := owner.PatchOrganizationRole(ctx, templateAdminCustom(first.OrganizationID))
6161
require.NoError(t, err, "upsert role")
6262

6363
// Assign the custom template admin role
@@ -111,7 +111,7 @@ func TestCustomOrganizationRole(t *testing.T) {
111111
ctx := testutil.Context(t, testutil.WaitMedium)
112112

113113
//nolint:gocritic // owner is required for this
114-
role, err := owner.PatchOrganizationRole(ctx, first.OrganizationID, templateAdminCustom(first.OrganizationID))
114+
role, err := owner.PatchOrganizationRole(ctx, templateAdminCustom(first.OrganizationID))
115115
require.NoError(t, err, "upsert role")
116116

117117
// Remove the license to block enterprise functionality
@@ -124,7 +124,7 @@ func TestCustomOrganizationRole(t *testing.T) {
124124
}
125125

126126
// Verify functionality is lost
127-
_, err = owner.PatchOrganizationRole(ctx, first.OrganizationID, templateAdminCustom(first.OrganizationID))
127+
_, err = owner.PatchOrganizationRole(ctx, templateAdminCustom(first.OrganizationID))
128128
require.ErrorContains(t, err, "roles are not enabled")
129129

130130
// Assign the custom template admin role
@@ -152,7 +152,7 @@ func TestCustomOrganizationRole(t *testing.T) {
152152

153153
ctx := testutil.Context(t, testutil.WaitMedium)
154154
//nolint:gocritic // owner is required for this
155-
role, err := owner.PatchOrganizationRole(ctx, first.OrganizationID, templateAdminCustom(first.OrganizationID))
155+
role, err := owner.PatchOrganizationRole(ctx, templateAdminCustom(first.OrganizationID))
156156
require.NoError(t, err, "upsert role")
157157

158158
// Assign the custom template admin role
@@ -169,7 +169,7 @@ func TestCustomOrganizationRole(t *testing.T) {
169169
newRole.SitePermissions = nil
170170
newRole.OrganizationPermissions = nil
171171
newRole.UserPermissions = nil
172-
_, err = owner.PatchOrganizationRole(ctx, first.OrganizationID, newRole)
172+
_, err = owner.PatchOrganizationRole(ctx, newRole)
173173
require.NoError(t, err, "upsert role with override")
174174

175175
// The role should no longer have template perms
@@ -203,7 +203,7 @@ func TestCustomOrganizationRole(t *testing.T) {
203203
ctx := testutil.Context(t, testutil.WaitMedium)
204204

205205
//nolint:gocritic // owner is required for this
206-
_, err := owner.PatchOrganizationRole(ctx, first.OrganizationID, codersdk.Role{
206+
_, err := owner.PatchOrganizationRole(ctx, codersdk.Role{
207207
Name: "Bad_Name", // No underscores allowed
208208
DisplayName: "Testing Purposes",
209209
OrganizationID: first.OrganizationID.String(),
@@ -232,38 +232,17 @@ func TestCustomOrganizationRole(t *testing.T) {
232232
ctx := testutil.Context(t, testutil.WaitMedium)
233233

234234
//nolint:gocritic // owner is required for this
235-
_, err := owner.PatchOrganizationRole(ctx, first.OrganizationID, codersdk.Role{
235+
_, err := owner.PatchOrganizationRole(ctx, codersdk.Role{
236236
Name: "owner", // Reserved
237237
DisplayName: "Testing Purposes",
238+
OrganizationID: first.OrganizationID.String(),
238239
SitePermissions: nil,
239240
OrganizationPermissions: nil,
240241
UserPermissions: nil,
241242
})
242243
require.ErrorContains(t, err, "Reserved")
243244
})
244245

245-
t.Run("MismatchedOrganizations", func(t *testing.T) {
246-
t.Parallel()
247-
dv := coderdtest.DeploymentValues(t)
248-
dv.Experiments = []string{string(codersdk.ExperimentCustomRoles)}
249-
owner, first := coderdenttest.New(t, &coderdenttest.Options{
250-
Options: &coderdtest.Options{
251-
DeploymentValues: dv,
252-
},
253-
LicenseOptions: &coderdenttest.LicenseOptions{
254-
Features: license.Features{
255-
codersdk.FeatureCustomRoles: 1,
256-
},
257-
},
258-
})
259-
260-
ctx := testutil.Context(t, testutil.WaitMedium)
261-
262-
//nolint:gocritic // owner is required for this
263-
_, err := owner.PatchOrganizationRole(ctx, first.OrganizationID, templateAdminCustom(uuid.New()))
264-
require.ErrorContains(t, err, "does not match")
265-
})
266-
267246
// Attempt to add site & user permissions, which is not allowed
268247
t.Run("ExcessPermissions", func(t *testing.T) {
269248
t.Parallel()
@@ -291,7 +270,7 @@ func TestCustomOrganizationRole(t *testing.T) {
291270
}
292271

293272
//nolint:gocritic // owner is required for this
294-
_, err := owner.PatchOrganizationRole(ctx, first.OrganizationID, siteRole)
273+
_, err := owner.PatchOrganizationRole(ctx, siteRole)
295274
require.ErrorContains(t, err, "site wide permissions")
296275

297276
userRole := templateAdminCustom(first.OrganizationID)
@@ -303,11 +282,11 @@ func TestCustomOrganizationRole(t *testing.T) {
303282
}
304283

305284
//nolint:gocritic // owner is required for this
306-
_, err = owner.PatchOrganizationRole(ctx, first.OrganizationID, userRole)
285+
_, err = owner.PatchOrganizationRole(ctx, userRole)
307286
require.ErrorContains(t, err, "not allowed to assign user permissions")
308287
})
309288

310-
t.Run("InvalidUUID", func(t *testing.T) {
289+
t.Run("NotFound", func(t *testing.T) {
311290
t.Parallel()
312291
dv := coderdtest.DeploymentValues(t)
313292
dv.Experiments = []string{string(codersdk.ExperimentCustomRoles)}
@@ -328,8 +307,8 @@ func TestCustomOrganizationRole(t *testing.T) {
328307
newRole.OrganizationID = "0000" // This is not a valid uuid
329308

330309
//nolint:gocritic // owner is required for this
331-
_, err := owner.PatchOrganizationRole(ctx, first.OrganizationID, newRole)
332-
require.ErrorContains(t, err, "Invalid request")
310+
_, err := owner.PatchOrganizationRole(ctx, newRole)
311+
require.ErrorContains(t, err, "Resource not found")
333312
})
334313
}
335314

‎enterprise/coderd/templates_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -751,7 +751,7 @@ func TestTemplates(t *testing.T) {
751751
})
752752

753753
//nolint:gocritic // owner required to make custom roles
754-
orgTemplateAdminRole, err := ownerClient.PatchOrganizationRole(ctx, secondOrg.ID, codersdk.Role{
754+
orgTemplateAdminRole, err := ownerClient.PatchOrganizationRole(ctx, codersdk.Role{
755755
Name: "org-template-admin",
756756
OrganizationID: secondOrg.ID.String(),
757757
OrganizationPermissions: codersdk.CreatePermissions(map[codersdk.RBACResource][]codersdk.RBACAction{

‎enterprise/coderd/userauth_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -705,7 +705,7 @@ func TestEnterpriseUserLogin(t *testing.T) {
705705

706706
ctx := testutil.Context(t, testutil.WaitShort)
707707
//nolint:gocritic // owner required
708-
customRole, err := ownerClient.PatchOrganizationRole(ctx, owner.OrganizationID, codersdk.Role{
708+
customRole, err := ownerClient.PatchOrganizationRole(ctx, codersdk.Role{
709709
Name: "custom-role",
710710
OrganizationID: owner.OrganizationID.String(),
711711
OrganizationPermissions: []codersdk.Permission{},

‎enterprise/coderd/users_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -271,7 +271,7 @@ func TestAssignCustomOrgRoles(t *testing.T) {
271271

272272
ctx := testutil.Context(t, testutil.WaitShort)
273273
// Create a custom role as an organization admin that allows making templates.
274-
auditorRole, err := client.PatchOrganizationRole(ctx, owner.OrganizationID, codersdk.Role{
274+
auditorRole, err := client.PatchOrganizationRole(ctx, codersdk.Role{
275275
Name: "org-template-admin",
276276
OrganizationID: owner.OrganizationID.String(),
277277
DisplayName: "Template Admin",

‎site/src/api/typesGenerated.ts

Lines changed: 9 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)