Skip to content

Commit db665e7

Browse files
EmyrkEmyrk
authored
chore: Drop resource_id support in rbac system (#3426)
1 parent ccf6f4e commit db665e7

File tree

17 files changed

+460
-471
lines changed

17 files changed

+460
-471
lines changed

‎coderd/coderd_test.go‎

Lines changed: 25 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -219,7 +219,7 @@ func TestAuthorizeAllEndpoints(t *testing.T) {
219219
authorizer.AlwaysReturn = rbac.ForbiddenWithInternal(xerrors.New("fake implementation"), nil, nil)
220220

221221
// Some quick reused objects
222-
workspaceRBACObj := rbac.ResourceWorkspace.InOrg(organization.ID).WithID(workspace.ID.String()).WithOwner(workspace.OwnerID.String())
222+
workspaceRBACObj := rbac.ResourceWorkspace.InOrg(organization.ID).WithOwner(workspace.OwnerID.String())
223223

224224
// skipRoutes allows skipping routes from being checked.
225225
skipRoutes := map[string]string{
@@ -346,107 +346,107 @@ func TestAuthorizeAllEndpoints(t *testing.T) {
346346
"GET:/api/v2/organizations/{organization}/templates": {
347347
StatusCode: http.StatusOK,
348348
AssertAction: rbac.ActionRead,
349-
AssertObject: rbac.ResourceTemplate.InOrg(template.OrganizationID).WithID(template.ID.String()),
349+
AssertObject: rbac.ResourceTemplate.InOrg(template.OrganizationID),
350350
},
351351
"POST:/api/v2/organizations/{organization}/templates": {
352352
AssertAction: rbac.ActionCreate,
353353
AssertObject: rbac.ResourceTemplate.InOrg(organization.ID),
354354
},
355355
"DELETE:/api/v2/templates/{template}": {
356356
AssertAction: rbac.ActionDelete,
357-
AssertObject: rbac.ResourceTemplate.InOrg(template.OrganizationID).WithID(template.ID.String()),
357+
AssertObject: rbac.ResourceTemplate.InOrg(template.OrganizationID),
358358
},
359359
"GET:/api/v2/templates/{template}": {
360360
AssertAction: rbac.ActionRead,
361-
AssertObject: rbac.ResourceTemplate.InOrg(template.OrganizationID).WithID(template.ID.String()),
361+
AssertObject: rbac.ResourceTemplate.InOrg(template.OrganizationID),
362362
},
363363
"POST:/api/v2/files": {AssertAction: rbac.ActionCreate, AssertObject: rbac.ResourceFile},
364364
"GET:/api/v2/files/{fileHash}": {
365365
AssertAction: rbac.ActionRead,
366-
AssertObject: rbac.ResourceFile.WithOwner(admin.UserID.String()).WithID(file.Hash),
366+
AssertObject: rbac.ResourceFile.WithOwner(admin.UserID.String()),
367367
},
368368
"GET:/api/v2/templates/{template}/versions": {
369369
AssertAction: rbac.ActionRead,
370-
AssertObject: rbac.ResourceTemplate.InOrg(template.OrganizationID).WithID(template.ID.String()),
370+
AssertObject: rbac.ResourceTemplate.InOrg(template.OrganizationID),
371371
},
372372
"PATCH:/api/v2/templates/{template}/versions": {
373373
AssertAction: rbac.ActionUpdate,
374-
AssertObject: rbac.ResourceTemplate.InOrg(template.OrganizationID).WithID(template.ID.String()),
374+
AssertObject: rbac.ResourceTemplate.InOrg(template.OrganizationID),
375375
},
376376
"GET:/api/v2/templates/{template}/versions/{templateversionname}": {
377377
AssertAction: rbac.ActionRead,
378-
AssertObject: rbac.ResourceTemplate.InOrg(template.OrganizationID).WithID(template.ID.String()),
378+
AssertObject: rbac.ResourceTemplate.InOrg(template.OrganizationID),
379379
},
380380
"GET:/api/v2/templateversions/{templateversion}": {
381381
AssertAction: rbac.ActionRead,
382-
AssertObject: rbac.ResourceTemplate.InOrg(template.OrganizationID).WithID(template.ID.String()),
382+
AssertObject: rbac.ResourceTemplate.InOrg(template.OrganizationID),
383383
},
384384
"PATCH:/api/v2/templateversions/{templateversion}/cancel": {
385385
AssertAction: rbac.ActionUpdate,
386-
AssertObject: rbac.ResourceTemplate.InOrg(template.OrganizationID).WithID(template.ID.String()),
386+
AssertObject: rbac.ResourceTemplate.InOrg(template.OrganizationID),
387387
},
388388
"GET:/api/v2/templateversions/{templateversion}/logs": {
389389
AssertAction: rbac.ActionRead,
390-
AssertObject: rbac.ResourceTemplate.InOrg(template.OrganizationID).WithID(template.ID.String()),
390+
AssertObject: rbac.ResourceTemplate.InOrg(template.OrganizationID),
391391
},
392392
"GET:/api/v2/templateversions/{templateversion}/parameters": {
393393
AssertAction: rbac.ActionRead,
394-
AssertObject: rbac.ResourceTemplate.InOrg(template.OrganizationID).WithID(template.ID.String()),
394+
AssertObject: rbac.ResourceTemplate.InOrg(template.OrganizationID),
395395
},
396396
"GET:/api/v2/templateversions/{templateversion}/resources": {
397397
AssertAction: rbac.ActionRead,
398-
AssertObject: rbac.ResourceTemplate.InOrg(template.OrganizationID).WithID(template.ID.String()),
398+
AssertObject: rbac.ResourceTemplate.InOrg(template.OrganizationID),
399399
},
400400
"GET:/api/v2/templateversions/{templateversion}/schema": {
401401
AssertAction: rbac.ActionRead,
402-
AssertObject: rbac.ResourceTemplate.InOrg(template.OrganizationID).WithID(template.ID.String()),
402+
AssertObject: rbac.ResourceTemplate.InOrg(template.OrganizationID),
403403
},
404404
"POST:/api/v2/templateversions/{templateversion}/dry-run": {
405405
// The first check is to read the template
406406
AssertAction: rbac.ActionRead,
407-
AssertObject: rbac.ResourceTemplate.InOrg(version.OrganizationID).WithID(template.ID.String()),
407+
AssertObject: rbac.ResourceTemplate.InOrg(version.OrganizationID),
408408
},
409409
"GET:/api/v2/templateversions/{templateversion}/dry-run/{templateversiondryrun}": {
410410
AssertAction: rbac.ActionRead,
411-
AssertObject: rbac.ResourceTemplate.InOrg(version.OrganizationID).WithID(template.ID.String()),
411+
AssertObject: rbac.ResourceTemplate.InOrg(version.OrganizationID),
412412
},
413413
"GET:/api/v2/templateversions/{templateversion}/dry-run/{templateversiondryrun}/resources": {
414414
AssertAction: rbac.ActionRead,
415-
AssertObject: rbac.ResourceTemplate.InOrg(version.OrganizationID).WithID(template.ID.String()),
415+
AssertObject: rbac.ResourceTemplate.InOrg(version.OrganizationID),
416416
},
417417
"GET:/api/v2/templateversions/{templateversion}/dry-run/{templateversiondryrun}/logs": {
418418
AssertAction: rbac.ActionRead,
419-
AssertObject: rbac.ResourceTemplate.InOrg(version.OrganizationID).WithID(template.ID.String()),
419+
AssertObject: rbac.ResourceTemplate.InOrg(version.OrganizationID),
420420
},
421421
"PATCH:/api/v2/templateversions/{templateversion}/dry-run/{templateversiondryrun}/cancel": {
422422
AssertAction: rbac.ActionRead,
423-
AssertObject: rbac.ResourceTemplate.InOrg(version.OrganizationID).WithID(template.ID.String()),
423+
AssertObject: rbac.ResourceTemplate.InOrg(version.OrganizationID),
424424
},
425425
"GET:/api/v2/provisionerdaemons": {
426426
StatusCode: http.StatusOK,
427-
AssertObject: rbac.ResourceProvisionerDaemon.WithID(provisionerds[0].ID.String()),
427+
AssertObject: rbac.ResourceProvisionerDaemon,
428428
},
429429

430430
"POST:/api/v2/parameters/{scope}/{id}": {
431431
AssertAction: rbac.ActionUpdate,
432-
AssertObject: rbac.ResourceTemplate.WithID(template.ID.String()),
432+
AssertObject: rbac.ResourceTemplate,
433433
},
434434
"GET:/api/v2/parameters/{scope}/{id}": {
435435
AssertAction: rbac.ActionRead,
436-
AssertObject: rbac.ResourceTemplate.WithID(template.ID.String()),
436+
AssertObject: rbac.ResourceTemplate,
437437
},
438438
"DELETE:/api/v2/parameters/{scope}/{id}/{name}": {
439439
AssertAction: rbac.ActionUpdate,
440-
AssertObject: rbac.ResourceTemplate.WithID(template.ID.String()),
440+
AssertObject: rbac.ResourceTemplate,
441441
},
442442
"GET:/api/v2/organizations/{organization}/templates/{templatename}": {
443443
AssertAction: rbac.ActionRead,
444-
AssertObject: rbac.ResourceTemplate.InOrg(template.OrganizationID).WithID(template.ID.String()),
444+
AssertObject: rbac.ResourceTemplate.InOrg(template.OrganizationID),
445445
},
446446
"POST:/api/v2/organizations/{organization}/workspaces": {
447447
AssertAction: rbac.ActionCreate,
448448
// No ID when creating
449-
AssertObject: workspaceRBACObj.WithID(""),
449+
AssertObject: workspaceRBACObj,
450450
},
451451
"GET:/api/v2/workspaces/{workspace}/watch": {
452452
AssertAction: rbac.ActionRead,
@@ -546,9 +546,6 @@ func TestAuthorizeAllEndpoints(t *testing.T) {
546546
if routeAssertions.AssertObject.OrgID != "" {
547547
assert.Equal(t, routeAssertions.AssertObject.OrgID, authorizer.Called.Object.OrgID, "resource org")
548548
}
549-
if routeAssertions.AssertObject.ResourceID != "" {
550-
assert.Equal(t, routeAssertions.AssertObject.ResourceID, authorizer.Called.Object.ResourceID, "resource ID")
551-
}
552549
}
553550
} else {
554551
assert.Nil(t, authorizer.Called, "authorize not expected")

‎coderd/coderdtest/coderdtest.go‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -275,10 +275,15 @@ func CreateFirstUser(t *testing.T, client *codersdk.Client) codersdk.CreateFirst
275275

276276
// CreateAnotherUser creates and authenticates a new user.
277277
func CreateAnotherUser(t *testing.T, client *codersdk.Client, organizationID uuid.UUID, roles ...string) *codersdk.Client {
278+
userClient, _ := createAnotherUserRetry(t, client, organizationID, 5, roles...)
279+
return userClient
280+
}
281+
282+
func CreateAnotherUserWithUser(t *testing.T, client *codersdk.Client, organizationID uuid.UUID, roles ...string) (*codersdk.Client, codersdk.User) {
278283
return createAnotherUserRetry(t, client, organizationID, 5, roles...)
279284
}
280285

281-
func createAnotherUserRetry(t *testing.T, client *codersdk.Client, organizationID uuid.UUID, retries int, roles ...string) *codersdk.Client {
286+
func createAnotherUserRetry(t *testing.T, client *codersdk.Client, organizationID uuid.UUID, retries int, roles ...string) (*codersdk.Client, codersdk.User) {
282287
req := codersdk.CreateUserRequest{
283288
Email: namesgenerator.GetRandomName(10) + "@coder.com",
284289
Username: randomUsername(),
@@ -337,7 +342,7 @@ func createAnotherUserRetry(t *testing.T, client *codersdk.Client, organizationI
337342
require.NoError(t, err, "update org membership roles")
338343
}
339344
}
340-
return other
345+
return other, user
341346
}
342347

343348
// CreateTemplateVersion creates a template import provisioner job

‎coderd/database/modelmethods.go‎

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -5,37 +5,37 @@ import (
55
)
66

77
func (t Template) RBACObject() rbac.Object {
8-
return rbac.ResourceTemplate.InOrg(t.OrganizationID).WithID(t.ID.String())
8+
return rbac.ResourceTemplate.InOrg(t.OrganizationID)
99
}
1010

1111
func (t TemplateVersion) RBACObject() rbac.Object {
1212
// Just use the parent template resource for controlling versions
13-
return rbac.ResourceTemplate.InOrg(t.OrganizationID).WithID(t.TemplateID.UUID.String())
13+
return rbac.ResourceTemplate.InOrg(t.OrganizationID)
1414
}
1515

1616
func (w Workspace) RBACObject() rbac.Object {
17-
return rbac.ResourceWorkspace.InOrg(w.OrganizationID).WithID(w.ID.String()).WithOwner(w.OwnerID.String())
17+
return rbac.ResourceWorkspace.InOrg(w.OrganizationID).WithOwner(w.OwnerID.String())
1818
}
1919

2020
func (m OrganizationMember) RBACObject() rbac.Object {
21-
return rbac.ResourceOrganizationMember.InOrg(m.OrganizationID).WithID(m.UserID.String())
21+
return rbac.ResourceOrganizationMember.InOrg(m.OrganizationID)
2222
}
2323

2424
func (o Organization) RBACObject() rbac.Object {
25-
return rbac.ResourceOrganization.InOrg(o.ID).WithID(o.ID.String())
25+
return rbac.ResourceOrganization.InOrg(o.ID)
2626
}
2727

28-
func (d ProvisionerDaemon) RBACObject() rbac.Object {
29-
return rbac.ResourceProvisionerDaemon.WithID(d.ID.String())
28+
func (ProvisionerDaemon) RBACObject() rbac.Object {
29+
return rbac.ResourceProvisionerDaemon
3030
}
3131

3232
func (f File) RBACObject() rbac.Object {
33-
return rbac.ResourceFile.WithID(f.Hash).WithOwner(f.CreatedBy.String())
33+
return rbac.ResourceFile.WithOwner(f.CreatedBy.String())
3434
}
3535

3636
// RBACObject returns the RBAC object for the site wide user resource.
3737
// If you are trying to get the RBAC object for the UserData, use
3838
// rbac.ResourceUserData
39-
func (u User) RBACObject() rbac.Object {
40-
return rbac.ResourceUser.WithID(u.ID.String())
39+
func (User) RBACObject() rbac.Object {
40+
return rbac.ResourceUser
4141
}

‎coderd/files.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -99,7 +99,7 @@ func (api *API) fileByHash(rw http.ResponseWriter, r *http.Request) {
9999
}
100100

101101
if !api.Authorize(r, rbac.ActionRead,
102-
rbac.ResourceFile.WithOwner(file.CreatedBy.String()).WithID(file.Hash)) {
102+
rbac.ResourceFile.WithOwner(file.CreatedBy.String())) {
103103
// Return 404 to not leak the file exists
104104
httpapi.ResourceNotFound(rw)
105105
return

‎coderd/members.go‎

Lines changed: 16 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ func (api *API) putMemberRoles(rw http.ResponseWriter, r *http.Request) {
2121
organization := httpmw.OrganizationParam(r)
2222
member := httpmw.OrganizationMemberParam(r)
2323
apiKey := httpmw.APIKey(r)
24+
actorRoles := httpmw.AuthorizationUserRoles(r)
2425

2526
if apiKey.UserID == member.UserID {
2627
httpapi.Write(rw, http.StatusBadRequest, codersdk.Response{
@@ -37,16 +38,22 @@ func (api *API) putMemberRoles(rw http.ResponseWriter, r *http.Request) {
3738
// The org-member role is always implied.
3839
impliedTypes := append(params.Roles, rbac.RoleOrgMember(organization.ID))
3940
added, removed := rbac.ChangeRoleSet(member.Roles, impliedTypes)
40-
for _, roleName := range added {
41-
// Assigning a role requires the create permission.
42-
if !api.Authorize(r, rbac.ActionCreate, rbac.ResourceOrgRoleAssignment.WithID(roleName).InOrg(organization.ID)) {
43-
httpapi.Forbidden(rw)
44-
return
45-
}
41+
42+
// Assigning a role requires the create permission.
43+
if len(added) > 0 && !api.Authorize(r, rbac.ActionCreate, rbac.ResourceOrgRoleAssignment.InOrg(organization.ID)) {
44+
httpapi.Forbidden(rw)
45+
return
4646
}
47-
for _, roleName := range removed {
48-
// Removing a role requires the delete permission.
49-
if !api.Authorize(r, rbac.ActionDelete, rbac.ResourceOrgRoleAssignment.WithID(roleName).InOrg(organization.ID)) {
47+
48+
// Removing a role requires the delete permission.
49+
if len(removed) > 0 && !api.Authorize(r, rbac.ActionDelete, rbac.ResourceOrgRoleAssignment.InOrg(organization.ID)) {
50+
httpapi.Forbidden(rw)
51+
return
52+
}
53+
54+
// Just treat adding & removing as "assigning" for now.
55+
for _, roleName := range append(added, removed...) {
56+
if !rbac.CanAssignRole(actorRoles.Roles, roleName) {
5057
httpapi.Forbidden(rw)
5158
return
5259
}

‎coderd/organizations.go‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -20,8 +20,7 @@ func (api *API) organization(rw http.ResponseWriter, r *http.Request) {
2020
organization := httpmw.OrganizationParam(r)
2121

2222
if !api.Authorize(r, rbac.ActionRead, rbac.ResourceOrganization.
23-
InOrg(organization.ID).
24-
WithID(organization.ID.String())) {
23+
InOrg(organization.ID)) {
2524
httpapi.ResourceNotFound(rw)
2625
return
2726
}

0 commit comments

Comments
 (0)