From 44423b3b28fd57fa3150420b8704e25353c6abc9 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 9 Aug 2022 08:29:51 -0500 Subject: [PATCH 1/6] chore: Drop resource_id support in rbac system resource_id was implemented to support: - Workspace agent tokens - Roles as a resource - Workspace sharing The additional complexity is not required and will ease support of rbac moving forward. --- coderd/coderd_test.go | 50 +++--- coderd/database/modelmethods.go | 16 +- coderd/files.go | 2 +- coderd/members.go | 24 +-- coderd/organizations.go | 3 +- coderd/rbac/authz_test.go | 268 +++++++------------------------- coderd/rbac/builtin.go | 8 +- coderd/rbac/builtin_test.go | 22 +-- coderd/rbac/example_test.go | 3 - coderd/rbac/object.go | 34 ++-- coderd/rbac/policy.rego | 7 +- coderd/rbac/role.go | 1 - coderd/roles.go | 2 +- coderd/users.go | 36 ++--- coderd/workspacebuilds.go | 18 +-- 15 files changed, 155 insertions(+), 339 deletions(-) diff --git a/coderd/coderd_test.go b/coderd/coderd_test.go index 83c6155e14037..67ecfbb5e3e5e 100644 --- a/coderd/coderd_test.go +++ b/coderd/coderd_test.go @@ -215,7 +215,7 @@ func TestAuthorizeAllEndpoints(t *testing.T) { authorizer.AlwaysReturn = rbac.ForbiddenWithInternal(xerrors.New("fake implementation"), nil, nil) // Some quick reused objects - workspaceRBACObj := rbac.ResourceWorkspace.InOrg(organization.ID).WithID(workspace.ID.String()).WithOwner(workspace.OwnerID.String()) + workspaceRBACObj := rbac.ResourceWorkspace.InOrg(organization.ID).WithOwner(workspace.OwnerID.String()) // skipRoutes allows skipping routes from being checked. skipRoutes := map[string]string{ @@ -342,7 +342,7 @@ func TestAuthorizeAllEndpoints(t *testing.T) { "GET:/api/v2/organizations/{organization}/templates": { StatusCode: http.StatusOK, AssertAction: rbac.ActionRead, - AssertObject: rbac.ResourceTemplate.InOrg(template.OrganizationID).WithID(template.ID.String()), + AssertObject: rbac.ResourceTemplate.InOrg(template.OrganizationID), }, "POST:/api/v2/organizations/{organization}/templates": { AssertAction: rbac.ActionCreate, @@ -350,99 +350,99 @@ func TestAuthorizeAllEndpoints(t *testing.T) { }, "DELETE:/api/v2/templates/{template}": { AssertAction: rbac.ActionDelete, - AssertObject: rbac.ResourceTemplate.InOrg(template.OrganizationID).WithID(template.ID.String()), + AssertObject: rbac.ResourceTemplate.InOrg(template.OrganizationID), }, "GET:/api/v2/templates/{template}": { AssertAction: rbac.ActionRead, - AssertObject: rbac.ResourceTemplate.InOrg(template.OrganizationID).WithID(template.ID.String()), + AssertObject: rbac.ResourceTemplate.InOrg(template.OrganizationID), }, "POST:/api/v2/files": {AssertAction: rbac.ActionCreate, AssertObject: rbac.ResourceFile}, "GET:/api/v2/files/{fileHash}": { AssertAction: rbac.ActionRead, - AssertObject: rbac.ResourceFile.WithOwner(admin.UserID.String()).WithID(file.Hash), + AssertObject: rbac.ResourceFile.WithOwner(admin.UserID.String()), }, "GET:/api/v2/templates/{template}/versions": { AssertAction: rbac.ActionRead, - AssertObject: rbac.ResourceTemplate.InOrg(template.OrganizationID).WithID(template.ID.String()), + AssertObject: rbac.ResourceTemplate.InOrg(template.OrganizationID), }, "PATCH:/api/v2/templates/{template}/versions": { AssertAction: rbac.ActionUpdate, - AssertObject: rbac.ResourceTemplate.InOrg(template.OrganizationID).WithID(template.ID.String()), + AssertObject: rbac.ResourceTemplate.InOrg(template.OrganizationID), }, "GET:/api/v2/templates/{template}/versions/{templateversionname}": { AssertAction: rbac.ActionRead, - AssertObject: rbac.ResourceTemplate.InOrg(template.OrganizationID).WithID(template.ID.String()), + AssertObject: rbac.ResourceTemplate.InOrg(template.OrganizationID), }, "GET:/api/v2/templateversions/{templateversion}": { AssertAction: rbac.ActionRead, - AssertObject: rbac.ResourceTemplate.InOrg(template.OrganizationID).WithID(template.ID.String()), + AssertObject: rbac.ResourceTemplate.InOrg(template.OrganizationID), }, "PATCH:/api/v2/templateversions/{templateversion}/cancel": { AssertAction: rbac.ActionUpdate, - AssertObject: rbac.ResourceTemplate.InOrg(template.OrganizationID).WithID(template.ID.String()), + AssertObject: rbac.ResourceTemplate.InOrg(template.OrganizationID), }, "GET:/api/v2/templateversions/{templateversion}/logs": { AssertAction: rbac.ActionRead, - AssertObject: rbac.ResourceTemplate.InOrg(template.OrganizationID).WithID(template.ID.String()), + AssertObject: rbac.ResourceTemplate.InOrg(template.OrganizationID), }, "GET:/api/v2/templateversions/{templateversion}/parameters": { AssertAction: rbac.ActionRead, - AssertObject: rbac.ResourceTemplate.InOrg(template.OrganizationID).WithID(template.ID.String()), + AssertObject: rbac.ResourceTemplate.InOrg(template.OrganizationID), }, "GET:/api/v2/templateversions/{templateversion}/resources": { AssertAction: rbac.ActionRead, - AssertObject: rbac.ResourceTemplate.InOrg(template.OrganizationID).WithID(template.ID.String()), + AssertObject: rbac.ResourceTemplate.InOrg(template.OrganizationID), }, "GET:/api/v2/templateversions/{templateversion}/schema": { AssertAction: rbac.ActionRead, - AssertObject: rbac.ResourceTemplate.InOrg(template.OrganizationID).WithID(template.ID.String()), + AssertObject: rbac.ResourceTemplate.InOrg(template.OrganizationID), }, "POST:/api/v2/templateversions/{templateversion}/dry-run": { // The first check is to read the template AssertAction: rbac.ActionRead, - AssertObject: rbac.ResourceTemplate.InOrg(version.OrganizationID).WithID(template.ID.String()), + AssertObject: rbac.ResourceTemplate.InOrg(version.OrganizationID), }, "GET:/api/v2/templateversions/{templateversion}/dry-run/{templateversiondryrun}": { AssertAction: rbac.ActionRead, - AssertObject: rbac.ResourceTemplate.InOrg(version.OrganizationID).WithID(template.ID.String()), + AssertObject: rbac.ResourceTemplate.InOrg(version.OrganizationID), }, "GET:/api/v2/templateversions/{templateversion}/dry-run/{templateversiondryrun}/resources": { AssertAction: rbac.ActionRead, - AssertObject: rbac.ResourceTemplate.InOrg(version.OrganizationID).WithID(template.ID.String()), + AssertObject: rbac.ResourceTemplate.InOrg(version.OrganizationID), }, "GET:/api/v2/templateversions/{templateversion}/dry-run/{templateversiondryrun}/logs": { AssertAction: rbac.ActionRead, - AssertObject: rbac.ResourceTemplate.InOrg(version.OrganizationID).WithID(template.ID.String()), + AssertObject: rbac.ResourceTemplate.InOrg(version.OrganizationID), }, "PATCH:/api/v2/templateversions/{templateversion}/dry-run/{templateversiondryrun}/cancel": { AssertAction: rbac.ActionRead, - AssertObject: rbac.ResourceTemplate.InOrg(version.OrganizationID).WithID(template.ID.String()), + AssertObject: rbac.ResourceTemplate.InOrg(version.OrganizationID), }, "GET:/api/v2/provisionerdaemons": { StatusCode: http.StatusOK, - AssertObject: rbac.ResourceProvisionerDaemon.WithID(provisionerds[0].ID.String()), + AssertObject: rbac.ResourceProvisionerDaemon, }, "POST:/api/v2/parameters/{scope}/{id}": { AssertAction: rbac.ActionUpdate, - AssertObject: rbac.ResourceTemplate.WithID(template.ID.String()), + AssertObject: rbac.ResourceTemplate, }, "GET:/api/v2/parameters/{scope}/{id}": { AssertAction: rbac.ActionRead, - AssertObject: rbac.ResourceTemplate.WithID(template.ID.String()), + AssertObject: rbac.ResourceTemplate, }, "DELETE:/api/v2/parameters/{scope}/{id}/{name}": { AssertAction: rbac.ActionUpdate, - AssertObject: rbac.ResourceTemplate.WithID(template.ID.String()), + AssertObject: rbac.ResourceTemplate, }, "GET:/api/v2/organizations/{organization}/templates/{templatename}": { AssertAction: rbac.ActionRead, - AssertObject: rbac.ResourceTemplate.InOrg(template.OrganizationID).WithID(template.ID.String()), + AssertObject: rbac.ResourceTemplate.InOrg(template.OrganizationID), }, "POST:/api/v2/organizations/{organization}/workspaces": { AssertAction: rbac.ActionCreate, // No ID when creating - AssertObject: workspaceRBACObj.WithID(""), + AssertObject: workspaceRBACObj, }, "GET:/api/v2/workspaces/{workspace}/watch": { AssertAction: rbac.ActionRead, diff --git a/coderd/database/modelmethods.go b/coderd/database/modelmethods.go index a629cd63e3d4e..fd8296687c02d 100644 --- a/coderd/database/modelmethods.go +++ b/coderd/database/modelmethods.go @@ -5,37 +5,37 @@ import ( ) func (t Template) RBACObject() rbac.Object { - return rbac.ResourceTemplate.InOrg(t.OrganizationID).WithID(t.ID.String()) + return rbac.ResourceTemplate.InOrg(t.OrganizationID) } func (t TemplateVersion) RBACObject() rbac.Object { // Just use the parent template resource for controlling versions - return rbac.ResourceTemplate.InOrg(t.OrganizationID).WithID(t.TemplateID.UUID.String()) + return rbac.ResourceTemplate.InOrg(t.OrganizationID) } func (w Workspace) RBACObject() rbac.Object { - return rbac.ResourceWorkspace.InOrg(w.OrganizationID).WithID(w.ID.String()).WithOwner(w.OwnerID.String()) + return rbac.ResourceWorkspace.InOrg(w.OrganizationID).WithOwner(w.OwnerID.String()) } func (m OrganizationMember) RBACObject() rbac.Object { - return rbac.ResourceOrganizationMember.InOrg(m.OrganizationID).WithID(m.UserID.String()) + return rbac.ResourceOrganizationMember.InOrg(m.OrganizationID) } func (o Organization) RBACObject() rbac.Object { - return rbac.ResourceOrganization.InOrg(o.ID).WithID(o.ID.String()) + return rbac.ResourceOrganization.InOrg(o.ID) } func (d ProvisionerDaemon) RBACObject() rbac.Object { - return rbac.ResourceProvisionerDaemon.WithID(d.ID.String()) + return rbac.ResourceProvisionerDaemon } func (f File) RBACObject() rbac.Object { - return rbac.ResourceFile.WithID(f.Hash).WithOwner(f.CreatedBy.String()) + return rbac.ResourceFile.WithOwner(f.CreatedBy.String()) } // RBACObject returns the RBAC object for the site wide user resource. // If you are trying to get the RBAC object for the UserData, use // rbac.ResourceUserData func (u User) RBACObject() rbac.Object { - return rbac.ResourceUser.WithID(u.ID.String()) + return rbac.ResourceUser } diff --git a/coderd/files.go b/coderd/files.go index 7053e6d7c10fa..1b2728f9cc527 100644 --- a/coderd/files.go +++ b/coderd/files.go @@ -99,7 +99,7 @@ func (api *API) fileByHash(rw http.ResponseWriter, r *http.Request) { } if !api.Authorize(r, rbac.ActionRead, - rbac.ResourceFile.WithOwner(file.CreatedBy.String()).WithID(file.Hash)) { + rbac.ResourceFile.WithOwner(file.CreatedBy.String())) { // Return 404 to not leak the file exists httpapi.ResourceNotFound(rw) return diff --git a/coderd/members.go b/coderd/members.go index f0f4e4dbbbc07..fc3c1ea448bda 100644 --- a/coderd/members.go +++ b/coderd/members.go @@ -37,19 +37,19 @@ func (api *API) putMemberRoles(rw http.ResponseWriter, r *http.Request) { // The org-member role is always implied. impliedTypes := append(params.Roles, rbac.RoleOrgMember(organization.ID)) added, removed := rbac.ChangeRoleSet(member.Roles, impliedTypes) - for _, roleName := range added { - // Assigning a role requires the create permission. - if !api.Authorize(r, rbac.ActionCreate, rbac.ResourceOrgRoleAssignment.WithID(roleName).InOrg(organization.ID)) { - httpapi.Forbidden(rw) - return - } + + // TODO: Handle added and removed roles. + + // Assigning a role requires the create permission. + if !api.Authorize(r, rbac.ActionCreate, rbac.ResourceOrgRoleAssignment.InOrg(organization.ID)) { + httpapi.Forbidden(rw) + return } - for _, roleName := range removed { - // Removing a role requires the delete permission. - if !api.Authorize(r, rbac.ActionDelete, rbac.ResourceOrgRoleAssignment.WithID(roleName).InOrg(organization.ID)) { - httpapi.Forbidden(rw) - return - } + + // Removing a role requires the delete permission. + if !api.Authorize(r, rbac.ActionDelete, rbac.ResourceOrgRoleAssignment.InOrg(organization.ID)) { + httpapi.Forbidden(rw) + return } updatedUser, err := api.updateOrganizationMemberRoles(r.Context(), database.UpdateMemberRolesParams{ diff --git a/coderd/organizations.go b/coderd/organizations.go index 3755777692e75..dffedaaea6e19 100644 --- a/coderd/organizations.go +++ b/coderd/organizations.go @@ -20,8 +20,7 @@ func (api *API) organization(rw http.ResponseWriter, r *http.Request) { organization := httpmw.OrganizationParam(r) if !api.Authorize(r, rbac.ActionRead, rbac.ResourceOrganization. - InOrg(organization.ID). - WithID(organization.ID.String())) { + InOrg(organization.ID)) { httpapi.ResourceNotFound(rw) return } diff --git a/coderd/rbac/authz_test.go b/coderd/rbac/authz_test.go index e1ab601ee30eb..4c714e52e5844 100644 --- a/coderd/rbac/authz_test.go +++ b/coderd/rbac/authz_test.go @@ -3,8 +3,6 @@ package rbac_test import ( "context" "encoding/json" - "fmt" - "strconv" "testing" "github.com/google/uuid" @@ -30,9 +28,8 @@ func TestFilter(t *testing.T) { workspaceList := make([]rbac.Object, 0) fileList := make([]rbac.Object, 0) for i := 0; i < 10; i++ { - idxStr := strconv.Itoa(i) - workspace := rbac.ResourceWorkspace.WithID(idxStr).WithOwner("me") - file := rbac.ResourceFile.WithID(idxStr).WithOwner("me") + workspace := rbac.ResourceWorkspace.WithOwner("me") + file := rbac.ResourceFile.WithOwner("me") workspaceList = append(workspaceList, workspace) fileList = append(fileList, file) @@ -116,7 +113,6 @@ func TestAuthorizeDomain(t *testing.T) { t.Parallel() defOrg := uuid.New() unuseID := uuid.New() - wrkID := "1234" user := subject{ UserID: "me", @@ -127,42 +123,28 @@ func TestAuthorizeDomain(t *testing.T) { } testAuthorize(t, "Member", user, []authTestCase{ - // Org + me + id - {resource: rbac.ResourceWorkspace.InOrg(defOrg).WithOwner(user.UserID).WithID(wrkID), actions: allActions(), allow: true}, + // Org + me {resource: rbac.ResourceWorkspace.InOrg(defOrg).WithOwner(user.UserID), actions: allActions(), allow: true}, - {resource: rbac.ResourceWorkspace.InOrg(defOrg).WithID(wrkID), actions: allActions(), allow: false}, {resource: rbac.ResourceWorkspace.InOrg(defOrg), actions: allActions(), allow: false}, - {resource: rbac.ResourceWorkspace.WithOwner(user.UserID).WithID(wrkID), actions: allActions(), allow: true}, {resource: rbac.ResourceWorkspace.WithOwner(user.UserID), actions: allActions(), allow: true}, - {resource: rbac.ResourceWorkspace.WithID(wrkID), actions: allActions(), allow: false}, - {resource: rbac.ResourceWorkspace.All(), actions: allActions(), allow: false}, - // Other org + me + id - {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithOwner(user.UserID).WithID(wrkID), actions: allActions(), allow: false}, + // Other org + me {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithOwner(user.UserID), actions: allActions(), allow: false}, - {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithID(wrkID), actions: allActions(), allow: false}, {resource: rbac.ResourceWorkspace.InOrg(unuseID), actions: allActions(), allow: false}, - // Other org + other user + id - {resource: rbac.ResourceWorkspace.InOrg(defOrg).WithOwner("not-me").WithID(wrkID), actions: allActions(), allow: false}, + // Other org + other user {resource: rbac.ResourceWorkspace.InOrg(defOrg).WithOwner("not-me"), actions: allActions(), allow: false}, - {resource: rbac.ResourceWorkspace.WithOwner("not-me").WithID(wrkID), actions: allActions(), allow: false}, {resource: rbac.ResourceWorkspace.WithOwner("not-me"), actions: allActions(), allow: false}, - // Other org + other use + other id - {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithOwner("not-me").WithID("not-id"), actions: allActions(), allow: false}, + // Other org + other us {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithOwner("not-me"), actions: allActions(), allow: false}, - {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithID("not-id"), actions: allActions(), allow: false}, {resource: rbac.ResourceWorkspace.InOrg(unuseID), actions: allActions(), allow: false}, - {resource: rbac.ResourceWorkspace.WithOwner("not-me").WithID("not-id"), actions: allActions(), allow: false}, {resource: rbac.ResourceWorkspace.WithOwner("not-me"), actions: allActions(), allow: false}, - - {resource: rbac.ResourceWorkspace.WithID("not-id"), actions: allActions(), allow: false}, }) user = subject{ @@ -174,7 +156,6 @@ func TestAuthorizeDomain(t *testing.T) { { Negate: true, ResourceType: rbac.WildcardSymbol, - ResourceID: rbac.WildcardSymbol, Action: rbac.WildcardSymbol, }, }, @@ -182,42 +163,28 @@ func TestAuthorizeDomain(t *testing.T) { } testAuthorize(t, "DeletedMember", user, []authTestCase{ - // Org + me + id - {resource: rbac.ResourceWorkspace.InOrg(defOrg).WithOwner(user.UserID).WithID(wrkID), actions: allActions(), allow: false}, + // Org + me {resource: rbac.ResourceWorkspace.InOrg(defOrg).WithOwner(user.UserID), actions: allActions(), allow: false}, - {resource: rbac.ResourceWorkspace.InOrg(defOrg).WithID(wrkID), actions: allActions(), allow: false}, {resource: rbac.ResourceWorkspace.InOrg(defOrg), actions: allActions(), allow: false}, - {resource: rbac.ResourceWorkspace.WithOwner(user.UserID).WithID(wrkID), actions: allActions(), allow: false}, {resource: rbac.ResourceWorkspace.WithOwner(user.UserID), actions: allActions(), allow: false}, - {resource: rbac.ResourceWorkspace.WithID(wrkID), actions: allActions(), allow: false}, - {resource: rbac.ResourceWorkspace.All(), actions: allActions(), allow: false}, - // Other org + me + id - {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithOwner(user.UserID).WithID(wrkID), actions: allActions(), allow: false}, + // Other org + me {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithOwner(user.UserID), actions: allActions(), allow: false}, - {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithID(wrkID), actions: allActions(), allow: false}, {resource: rbac.ResourceWorkspace.InOrg(unuseID), actions: allActions(), allow: false}, - // Other org + other user + id - {resource: rbac.ResourceWorkspace.InOrg(defOrg).WithOwner("not-me").WithID(wrkID), actions: allActions(), allow: false}, + // Other org + other user {resource: rbac.ResourceWorkspace.InOrg(defOrg).WithOwner("not-me"), actions: allActions(), allow: false}, - {resource: rbac.ResourceWorkspace.WithOwner("not-me").WithID(wrkID), actions: allActions(), allow: false}, {resource: rbac.ResourceWorkspace.WithOwner("not-me"), actions: allActions(), allow: false}, - // Other org + other use + other id - {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithOwner("not-me").WithID("not-id"), actions: allActions(), allow: false}, + // Other org + other use {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithOwner("not-me"), actions: allActions(), allow: false}, - {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithID("not-id"), actions: allActions(), allow: false}, {resource: rbac.ResourceWorkspace.InOrg(unuseID), actions: allActions(), allow: false}, - {resource: rbac.ResourceWorkspace.WithOwner("not-me").WithID("not-id"), actions: allActions(), allow: false}, {resource: rbac.ResourceWorkspace.WithOwner("not-me"), actions: allActions(), allow: false}, - - {resource: rbac.ResourceWorkspace.WithID("not-id"), actions: allActions(), allow: false}, }) user = subject{ @@ -229,42 +196,28 @@ func TestAuthorizeDomain(t *testing.T) { } testAuthorize(t, "OrgAdmin", user, []authTestCase{ - // Org + me + id - {resource: rbac.ResourceWorkspace.InOrg(defOrg).WithOwner(user.UserID).WithID(wrkID), actions: allActions(), allow: true}, + // Org + me {resource: rbac.ResourceWorkspace.InOrg(defOrg).WithOwner(user.UserID), actions: allActions(), allow: true}, - {resource: rbac.ResourceWorkspace.InOrg(defOrg).WithID(wrkID), actions: allActions(), allow: true}, {resource: rbac.ResourceWorkspace.InOrg(defOrg), actions: allActions(), allow: true}, - {resource: rbac.ResourceWorkspace.WithOwner(user.UserID).WithID(wrkID), actions: allActions(), allow: true}, {resource: rbac.ResourceWorkspace.WithOwner(user.UserID), actions: allActions(), allow: true}, - {resource: rbac.ResourceWorkspace.WithID(wrkID), actions: allActions(), allow: false}, - {resource: rbac.ResourceWorkspace.All(), actions: allActions(), allow: false}, - // Other org + me + id - {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithOwner(user.UserID).WithID(wrkID), actions: allActions(), allow: false}, + // Other org + me {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithOwner(user.UserID), actions: allActions(), allow: false}, - {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithID(wrkID), actions: allActions(), allow: false}, {resource: rbac.ResourceWorkspace.InOrg(unuseID), actions: allActions(), allow: false}, - // Other org + other user + id - {resource: rbac.ResourceWorkspace.InOrg(defOrg).WithOwner("not-me").WithID(wrkID), actions: allActions(), allow: true}, + // Other org + other user {resource: rbac.ResourceWorkspace.InOrg(defOrg).WithOwner("not-me"), actions: allActions(), allow: true}, - {resource: rbac.ResourceWorkspace.WithOwner("not-me").WithID(wrkID), actions: allActions(), allow: false}, {resource: rbac.ResourceWorkspace.WithOwner("not-me"), actions: allActions(), allow: false}, - // Other org + other use + other id - {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithOwner("not-me").WithID("not-id"), actions: allActions(), allow: false}, + // Other org + other use {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithOwner("not-me"), actions: allActions(), allow: false}, - {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithID("not-id"), actions: allActions(), allow: false}, {resource: rbac.ResourceWorkspace.InOrg(unuseID), actions: allActions(), allow: false}, - {resource: rbac.ResourceWorkspace.WithOwner("not-me").WithID("not-id"), actions: allActions(), allow: false}, {resource: rbac.ResourceWorkspace.WithOwner("not-me"), actions: allActions(), allow: false}, - - {resource: rbac.ResourceWorkspace.WithID("not-id"), actions: allActions(), allow: false}, }) user = subject{ @@ -276,57 +229,44 @@ func TestAuthorizeDomain(t *testing.T) { } testAuthorize(t, "SiteAdmin", user, []authTestCase{ - // Org + me + id - {resource: rbac.ResourceWorkspace.InOrg(defOrg).WithOwner(user.UserID).WithID(wrkID), actions: allActions(), allow: true}, + // Org + me {resource: rbac.ResourceWorkspace.InOrg(defOrg).WithOwner(user.UserID), actions: allActions(), allow: true}, - {resource: rbac.ResourceWorkspace.InOrg(defOrg).WithID(wrkID), actions: allActions(), allow: true}, {resource: rbac.ResourceWorkspace.InOrg(defOrg), actions: allActions(), allow: true}, - {resource: rbac.ResourceWorkspace.WithOwner(user.UserID).WithID(wrkID), actions: allActions(), allow: true}, {resource: rbac.ResourceWorkspace.WithOwner(user.UserID), actions: allActions(), allow: true}, - {resource: rbac.ResourceWorkspace.WithID(wrkID), actions: allActions(), allow: true}, - {resource: rbac.ResourceWorkspace.All(), actions: allActions(), allow: true}, - // Other org + me + id - {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithOwner(user.UserID).WithID(wrkID), actions: allActions(), allow: true}, + // Other org + me {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithOwner(user.UserID), actions: allActions(), allow: true}, - {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithID(wrkID), actions: allActions(), allow: true}, {resource: rbac.ResourceWorkspace.InOrg(unuseID), actions: allActions(), allow: true}, - // Other org + other user + id - {resource: rbac.ResourceWorkspace.InOrg(defOrg).WithOwner("not-me").WithID(wrkID), actions: allActions(), allow: true}, + // Other org + other user {resource: rbac.ResourceWorkspace.InOrg(defOrg).WithOwner("not-me"), actions: allActions(), allow: true}, - {resource: rbac.ResourceWorkspace.WithOwner("not-me").WithID(wrkID), actions: allActions(), allow: true}, {resource: rbac.ResourceWorkspace.WithOwner("not-me"), actions: allActions(), allow: true}, - // Other org + other use + other id - {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithOwner("not-me").WithID("not-id"), actions: allActions(), allow: true}, + // Other org + other use {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithOwner("not-me"), actions: allActions(), allow: true}, - {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithID("not-id"), actions: allActions(), allow: true}, {resource: rbac.ResourceWorkspace.InOrg(unuseID), actions: allActions(), allow: true}, - {resource: rbac.ResourceWorkspace.WithOwner("not-me").WithID("not-id"), actions: allActions(), allow: true}, {resource: rbac.ResourceWorkspace.WithOwner("not-me"), actions: allActions(), allow: true}, - - {resource: rbac.ResourceWorkspace.WithID("not-id"), actions: allActions(), allow: true}, }) - // In practice this is a token scope on a regular subject + // In practice this is a token scope on a regular subject. + // So this unit test does not represent a practical role. It is just + // testing the capabilities of the RBAC system. user = subject{ UserID: "me", Roles: []rbac.Role{ { - Name: fmt.Sprintf("agent-%s", wrkID), + Name: "WorkspaceToken", // This is at the site level to prevent the token from losing access if the user // is kicked from the org Site: []rbac.Permission{ { Negate: false, ResourceType: rbac.ResourceWorkspace.Type, - ResourceID: wrkID, Action: rbac.ActionRead, }, }, @@ -334,48 +274,34 @@ func TestAuthorizeDomain(t *testing.T) { }, } - testAuthorize(t, "WorkspaceAgentToken", user, + testAuthorize(t, "WorkspaceToken", user, // Read Actions cases(func(c authTestCase) authTestCase { c.actions = []rbac.Action{rbac.ActionRead} return c }, []authTestCase{ - // Org + me + id - {resource: rbac.ResourceWorkspace.InOrg(defOrg).WithOwner(user.UserID).WithID(wrkID), allow: true}, - {resource: rbac.ResourceWorkspace.InOrg(defOrg).WithOwner(user.UserID), allow: false}, - {resource: rbac.ResourceWorkspace.InOrg(defOrg).WithID(wrkID), allow: true}, - {resource: rbac.ResourceWorkspace.InOrg(defOrg), allow: false}, - - {resource: rbac.ResourceWorkspace.WithOwner(user.UserID).WithID(wrkID), allow: true}, - {resource: rbac.ResourceWorkspace.WithOwner(user.UserID), allow: false}, + // Org + me + {resource: rbac.ResourceWorkspace.InOrg(defOrg).WithOwner(user.UserID), allow: true}, + {resource: rbac.ResourceWorkspace.InOrg(defOrg), allow: true}, - {resource: rbac.ResourceWorkspace.WithID(wrkID), allow: true}, + {resource: rbac.ResourceWorkspace.WithOwner(user.UserID), allow: true}, - {resource: rbac.ResourceWorkspace.All(), allow: false}, + {resource: rbac.ResourceWorkspace.All(), allow: true}, - // Other org + me + id - {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithOwner(user.UserID).WithID(wrkID), allow: true}, - {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithOwner(user.UserID), allow: false}, - {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithID(wrkID), allow: true}, - {resource: rbac.ResourceWorkspace.InOrg(unuseID), allow: false}, + // Other org + me + {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithOwner(user.UserID), allow: true}, + {resource: rbac.ResourceWorkspace.InOrg(unuseID), allow: true}, - // Other org + other user + id - {resource: rbac.ResourceWorkspace.InOrg(defOrg).WithOwner("not-me").WithID(wrkID), allow: true}, - {resource: rbac.ResourceWorkspace.InOrg(defOrg).WithOwner("not-me"), allow: false}, - - {resource: rbac.ResourceWorkspace.WithOwner("not-me").WithID(wrkID), allow: true}, - {resource: rbac.ResourceWorkspace.WithOwner("not-me"), allow: false}, + // Other org + other user + {resource: rbac.ResourceWorkspace.InOrg(defOrg).WithOwner("not-me"), allow: true}, - // Other org + other use + other id - {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithOwner("not-me").WithID("not-id"), allow: false}, - {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithOwner("not-me"), allow: false}, - {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithID("not-id"), allow: false}, - {resource: rbac.ResourceWorkspace.InOrg(unuseID), allow: false}, + {resource: rbac.ResourceWorkspace.WithOwner("not-me"), allow: true}, - {resource: rbac.ResourceWorkspace.WithOwner("not-me").WithID("not-id"), allow: false}, - {resource: rbac.ResourceWorkspace.WithOwner("not-me"), allow: false}, + // Other org + other use + {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithOwner("not-me"), allow: true}, + {resource: rbac.ResourceWorkspace.InOrg(unuseID), allow: true}, - {resource: rbac.ResourceWorkspace.WithID("not-id"), allow: false}, + {resource: rbac.ResourceWorkspace.WithOwner("not-me"), allow: true}, }), // Not read actions cases(func(c authTestCase) authTestCase { @@ -383,42 +309,28 @@ func TestAuthorizeDomain(t *testing.T) { c.allow = false return c }, []authTestCase{ - // Org + me + id - {resource: rbac.ResourceWorkspace.InOrg(defOrg).WithOwner(user.UserID).WithID(wrkID)}, + // Org + me {resource: rbac.ResourceWorkspace.InOrg(defOrg).WithOwner(user.UserID)}, - {resource: rbac.ResourceWorkspace.InOrg(defOrg).WithID(wrkID)}, {resource: rbac.ResourceWorkspace.InOrg(defOrg)}, - {resource: rbac.ResourceWorkspace.WithOwner(user.UserID).WithID(wrkID)}, {resource: rbac.ResourceWorkspace.WithOwner(user.UserID)}, - {resource: rbac.ResourceWorkspace.WithID(wrkID)}, - {resource: rbac.ResourceWorkspace.All()}, - // Other org + me + id - {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithOwner(user.UserID).WithID(wrkID)}, + // Other org + me {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithOwner(user.UserID)}, - {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithID(wrkID)}, {resource: rbac.ResourceWorkspace.InOrg(unuseID)}, - // Other org + other user + id - {resource: rbac.ResourceWorkspace.InOrg(defOrg).WithOwner("not-me").WithID(wrkID)}, + // Other org + other user {resource: rbac.ResourceWorkspace.InOrg(defOrg).WithOwner("not-me")}, - {resource: rbac.ResourceWorkspace.WithOwner("not-me").WithID(wrkID)}, {resource: rbac.ResourceWorkspace.WithOwner("not-me")}, - // Other org + other use + other id - {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithOwner("not-me").WithID("not-id")}, + // Other org + other use {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithOwner("not-me")}, - {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithID("not-id")}, {resource: rbac.ResourceWorkspace.InOrg(unuseID)}, - {resource: rbac.ResourceWorkspace.WithOwner("not-me").WithID("not-id")}, {resource: rbac.ResourceWorkspace.WithOwner("not-me")}, - - {resource: rbac.ResourceWorkspace.WithID("not-id")}, }), ) @@ -433,7 +345,6 @@ func TestAuthorizeDomain(t *testing.T) { defOrg.String(): {{ Negate: false, ResourceType: "*", - ResourceID: "*", Action: rbac.ActionRead, }}, }, @@ -441,7 +352,6 @@ func TestAuthorizeDomain(t *testing.T) { { Negate: false, ResourceType: "*", - ResourceID: "*", Action: rbac.ActionRead, }, }, @@ -455,42 +365,28 @@ func TestAuthorizeDomain(t *testing.T) { return c }, []authTestCase{ // Read - // Org + me + id - {resource: rbac.ResourceWorkspace.InOrg(defOrg).WithOwner(user.UserID).WithID(wrkID), allow: true}, + // Org + me {resource: rbac.ResourceWorkspace.InOrg(defOrg).WithOwner(user.UserID), allow: true}, - {resource: rbac.ResourceWorkspace.InOrg(defOrg).WithID(wrkID), allow: true}, {resource: rbac.ResourceWorkspace.InOrg(defOrg), allow: true}, - {resource: rbac.ResourceWorkspace.WithOwner(user.UserID).WithID(wrkID), allow: true}, {resource: rbac.ResourceWorkspace.WithOwner(user.UserID), allow: true}, - {resource: rbac.ResourceWorkspace.WithID(wrkID), allow: false}, - {resource: rbac.ResourceWorkspace.All(), allow: false}, - // Other org + me + id - {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithOwner(user.UserID).WithID(wrkID), allow: false}, + // Other org + me {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithOwner(user.UserID), allow: false}, - {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithID(wrkID), allow: false}, {resource: rbac.ResourceWorkspace.InOrg(unuseID), allow: false}, - // Other org + other user + id - {resource: rbac.ResourceWorkspace.InOrg(defOrg).WithOwner("not-me").WithID(wrkID), allow: true}, + // Other org + other user {resource: rbac.ResourceWorkspace.InOrg(defOrg).WithOwner("not-me"), allow: true}, - {resource: rbac.ResourceWorkspace.WithOwner("not-me").WithID(wrkID), allow: false}, {resource: rbac.ResourceWorkspace.WithOwner("not-me"), allow: false}, - // Other org + other use + other id - {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithOwner("not-me").WithID("not-id"), allow: false}, + // Other org + other use {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithOwner("not-me"), allow: false}, - {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithID("not-id"), allow: false}, {resource: rbac.ResourceWorkspace.InOrg(unuseID), allow: false}, - {resource: rbac.ResourceWorkspace.WithOwner("not-me").WithID("not-id"), allow: false}, {resource: rbac.ResourceWorkspace.WithOwner("not-me"), allow: false}, - - {resource: rbac.ResourceWorkspace.WithID("not-id"), allow: false}, }), // Pass non-read actions @@ -500,42 +396,28 @@ func TestAuthorizeDomain(t *testing.T) { return c }, []authTestCase{ // Read - // Org + me + id - {resource: rbac.ResourceWorkspace.InOrg(defOrg).WithOwner(user.UserID).WithID(wrkID)}, + // Org + me {resource: rbac.ResourceWorkspace.InOrg(defOrg).WithOwner(user.UserID)}, - {resource: rbac.ResourceWorkspace.InOrg(defOrg).WithID(wrkID)}, {resource: rbac.ResourceWorkspace.InOrg(defOrg)}, - {resource: rbac.ResourceWorkspace.WithOwner(user.UserID).WithID(wrkID)}, {resource: rbac.ResourceWorkspace.WithOwner(user.UserID)}, - {resource: rbac.ResourceWorkspace.WithID(wrkID)}, - {resource: rbac.ResourceWorkspace.All()}, - // Other org + me + id - {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithOwner(user.UserID).WithID(wrkID)}, + // Other org + me {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithOwner(user.UserID)}, - {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithID(wrkID)}, {resource: rbac.ResourceWorkspace.InOrg(unuseID)}, - // Other org + other user + id - {resource: rbac.ResourceWorkspace.InOrg(defOrg).WithOwner("not-me").WithID(wrkID)}, + // Other org + other user {resource: rbac.ResourceWorkspace.InOrg(defOrg).WithOwner("not-me")}, - {resource: rbac.ResourceWorkspace.WithOwner("not-me").WithID(wrkID)}, {resource: rbac.ResourceWorkspace.WithOwner("not-me")}, - // Other org + other use + other id - {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithOwner("not-me").WithID("not-id")}, + // Other org + other use {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithOwner("not-me")}, - {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithID("not-id")}, {resource: rbac.ResourceWorkspace.InOrg(unuseID)}, - {resource: rbac.ResourceWorkspace.WithOwner("not-me").WithID("not-id")}, {resource: rbac.ResourceWorkspace.WithOwner("not-me")}, - - {resource: rbac.ResourceWorkspace.WithID("not-id")}, })) } @@ -544,7 +426,6 @@ func TestAuthorizeDomain(t *testing.T) { func TestAuthorizeLevels(t *testing.T) { defOrg := uuid.New() unusedID := uuid.New() - wrkID := "1234" user := subject{ UserID: "me", @@ -557,7 +438,6 @@ func TestAuthorizeLevels(t *testing.T) { { Negate: true, ResourceType: "*", - ResourceID: "*", Action: "*", }, }, @@ -570,7 +450,6 @@ func TestAuthorizeLevels(t *testing.T) { { Negate: true, ResourceType: rbac.WildcardSymbol, - ResourceID: rbac.WildcardSymbol, Action: rbac.WildcardSymbol, }, }, @@ -584,42 +463,28 @@ func TestAuthorizeLevels(t *testing.T) { c.allow = true return c }, []authTestCase{ - // Org + me + id - {resource: rbac.ResourceWorkspace.InOrg(defOrg).WithOwner(user.UserID).WithID(wrkID)}, + // Org + me {resource: rbac.ResourceWorkspace.InOrg(defOrg).WithOwner(user.UserID)}, - {resource: rbac.ResourceWorkspace.InOrg(defOrg).WithID(wrkID)}, {resource: rbac.ResourceWorkspace.InOrg(defOrg)}, - {resource: rbac.ResourceWorkspace.WithOwner(user.UserID).WithID(wrkID)}, {resource: rbac.ResourceWorkspace.WithOwner(user.UserID)}, - {resource: rbac.ResourceWorkspace.WithID(wrkID)}, - {resource: rbac.ResourceWorkspace.All()}, - // Other org + me + id - {resource: rbac.ResourceWorkspace.InOrg(unusedID).WithOwner(user.UserID).WithID(wrkID)}, + // Other org + me {resource: rbac.ResourceWorkspace.InOrg(unusedID).WithOwner(user.UserID)}, - {resource: rbac.ResourceWorkspace.InOrg(unusedID).WithID(wrkID)}, {resource: rbac.ResourceWorkspace.InOrg(unusedID)}, - // Other org + other user + id - {resource: rbac.ResourceWorkspace.InOrg(defOrg).WithOwner("not-me").WithID(wrkID)}, + // Other org + other user {resource: rbac.ResourceWorkspace.InOrg(defOrg).WithOwner("not-me")}, - {resource: rbac.ResourceWorkspace.WithOwner("not-me").WithID(wrkID)}, {resource: rbac.ResourceWorkspace.WithOwner("not-me")}, - // Other org + other use + other id - {resource: rbac.ResourceWorkspace.InOrg(unusedID).WithOwner("not-me").WithID("not-id")}, + // Other org + other use {resource: rbac.ResourceWorkspace.InOrg(unusedID).WithOwner("not-me")}, - {resource: rbac.ResourceWorkspace.InOrg(unusedID).WithID("not-id")}, {resource: rbac.ResourceWorkspace.InOrg(unusedID)}, - {resource: rbac.ResourceWorkspace.WithOwner("not-me").WithID("not-id")}, {resource: rbac.ResourceWorkspace.WithOwner("not-me")}, - - {resource: rbac.ResourceWorkspace.WithID("not-id")}, })) user = subject{ @@ -631,7 +496,6 @@ func TestAuthorizeLevels(t *testing.T) { { Negate: true, ResourceType: "random", - ResourceID: rbac.WildcardSymbol, Action: rbac.WildcardSymbol, }, }, @@ -644,7 +508,6 @@ func TestAuthorizeLevels(t *testing.T) { { Negate: true, ResourceType: rbac.WildcardSymbol, - ResourceID: rbac.WildcardSymbol, Action: rbac.WildcardSymbol, }, }, @@ -657,42 +520,28 @@ func TestAuthorizeLevels(t *testing.T) { c.actions = allActions() return c }, []authTestCase{ - // Org + me + id - {resource: rbac.ResourceWorkspace.InOrg(defOrg).WithOwner(user.UserID).WithID(wrkID), allow: true}, + // Org + me {resource: rbac.ResourceWorkspace.InOrg(defOrg).WithOwner(user.UserID), allow: true}, - {resource: rbac.ResourceWorkspace.InOrg(defOrg).WithID(wrkID), allow: true}, {resource: rbac.ResourceWorkspace.InOrg(defOrg), allow: true}, - {resource: rbac.ResourceWorkspace.WithOwner(user.UserID).WithID(wrkID), allow: false}, {resource: rbac.ResourceWorkspace.WithOwner(user.UserID), allow: false}, - {resource: rbac.ResourceWorkspace.WithID(wrkID), allow: false}, - {resource: rbac.ResourceWorkspace.All(), allow: false}, - // Other org + me + id - {resource: rbac.ResourceWorkspace.InOrg(unusedID).WithOwner(user.UserID).WithID(wrkID), allow: false}, + // Other org + me {resource: rbac.ResourceWorkspace.InOrg(unusedID).WithOwner(user.UserID), allow: false}, - {resource: rbac.ResourceWorkspace.InOrg(unusedID).WithID(wrkID), allow: false}, {resource: rbac.ResourceWorkspace.InOrg(unusedID), allow: false}, - // Other org + other user + id - {resource: rbac.ResourceWorkspace.InOrg(defOrg).WithOwner("not-me").WithID(wrkID), allow: true}, + // Other org + other user {resource: rbac.ResourceWorkspace.InOrg(defOrg).WithOwner("not-me"), allow: true}, - {resource: rbac.ResourceWorkspace.WithOwner("not-me").WithID(wrkID), allow: false}, {resource: rbac.ResourceWorkspace.WithOwner("not-me"), allow: false}, - // Other org + other use + other id - {resource: rbac.ResourceWorkspace.InOrg(unusedID).WithOwner("not-me").WithID("not-id"), allow: false}, + // Other org + other use {resource: rbac.ResourceWorkspace.InOrg(unusedID).WithOwner("not-me"), allow: false}, - {resource: rbac.ResourceWorkspace.InOrg(unusedID).WithID("not-id"), allow: false}, {resource: rbac.ResourceWorkspace.InOrg(unusedID), allow: false}, - {resource: rbac.ResourceWorkspace.WithOwner("not-me").WithID("not-id"), allow: false}, {resource: rbac.ResourceWorkspace.WithOwner("not-me"), allow: false}, - - {resource: rbac.ResourceWorkspace.WithID("not-id"), allow: false}, })) } @@ -714,6 +563,7 @@ type authTestCase struct { } func testAuthorize(t *testing.T, name string, subject subject, sets ...[]authTestCase) { + t.Helper() authorizer, err := rbac.NewAuthorizer() require.NoError(t, err) for _, cases := range sets { diff --git a/coderd/rbac/builtin.go b/coderd/rbac/builtin.go index f6691c50830e7..630836db2d8b4 100644 --- a/coderd/rbac/builtin.go +++ b/coderd/rbac/builtin.go @@ -82,7 +82,7 @@ var ( // TODO: Finish the auditor as we add resources. auditor: func(_ string) Role { return Role{ - Name: "auditor", + Name: auditor, DisplayName: "Auditor", Site: permissions(map[Object][]Action{ // Should be able to read all template details, even in orgs they @@ -103,7 +103,6 @@ var ( { Negate: false, ResourceType: "*", - ResourceID: "*", Action: "*", }, }, @@ -123,24 +122,20 @@ var ( // All org members can read the other members in their org. ResourceType: ResourceOrganizationMember.Type, Action: ActionRead, - ResourceID: "*", }, { // All org members can read the organization ResourceType: ResourceOrganization.Type, Action: ActionRead, - ResourceID: "*", }, { // All org members can read templates in the org ResourceType: ResourceTemplate.Type, Action: ActionRead, - ResourceID: "*", }, { // Can read available roles. ResourceType: ResourceOrgRoleAssignment.Type, - ResourceID: "*", Action: ActionRead, }, }, @@ -292,7 +287,6 @@ func permissions(perms map[Object][]Action) []Permission { list = append(list, Permission{ Negate: false, ResourceType: k.Type, - ResourceID: WildcardSymbol, Action: act, }) } diff --git a/coderd/rbac/builtin_test.go b/coderd/rbac/builtin_test.go index d68f697719acd..1ce6e2eab6733 100644 --- a/coderd/rbac/builtin_test.go +++ b/coderd/rbac/builtin_test.go @@ -61,7 +61,7 @@ func TestRolePermissions(t *testing.T) { { Name: "MyUser", Actions: []rbac.Action{rbac.ActionRead}, - Resource: rbac.ResourceUser.WithID(currentUser.String()), + Resource: rbac.ResourceUser, AuthorizeMap: map[bool][]authSubject{ true: {admin, memberMe, orgMemberMe, orgAdmin, otherOrgMember, otherOrgAdmin}, false: {}, @@ -80,7 +80,7 @@ func TestRolePermissions(t *testing.T) { Name: "MyWorkspaceInOrg", // When creating the WithID won't be set, but it does not change the result. Actions: []rbac.Action{rbac.ActionCreate, rbac.ActionRead, rbac.ActionUpdate, rbac.ActionDelete}, - Resource: rbac.ResourceWorkspace.InOrg(orgID).WithOwner(currentUser.String()).WithID(uuid.NewString()), + Resource: rbac.ResourceWorkspace.InOrg(orgID).WithOwner(currentUser.String()), AuthorizeMap: map[bool][]authSubject{ true: {admin, orgMemberMe, orgAdmin}, false: {memberMe, otherOrgAdmin, otherOrgMember}, @@ -89,7 +89,7 @@ func TestRolePermissions(t *testing.T) { { Name: "Templates", Actions: []rbac.Action{rbac.ActionCreate, rbac.ActionUpdate, rbac.ActionDelete}, - Resource: rbac.ResourceTemplate.InOrg(orgID).WithID(uuid.NewString()), + Resource: rbac.ResourceTemplate.InOrg(orgID), AuthorizeMap: map[bool][]authSubject{ true: {admin, orgAdmin}, false: {memberMe, orgMemberMe, otherOrgAdmin, otherOrgMember}, @@ -98,7 +98,7 @@ func TestRolePermissions(t *testing.T) { { Name: "ReadTemplates", Actions: []rbac.Action{rbac.ActionRead}, - Resource: rbac.ResourceTemplate.InOrg(orgID).WithID(uuid.NewString()), + Resource: rbac.ResourceTemplate.InOrg(orgID), AuthorizeMap: map[bool][]authSubject{ true: {admin, orgMemberMe, orgAdmin}, false: {memberMe, otherOrgAdmin, otherOrgMember}, @@ -116,7 +116,7 @@ func TestRolePermissions(t *testing.T) { { Name: "MyFile", Actions: []rbac.Action{rbac.ActionRead, rbac.ActionUpdate, rbac.ActionDelete}, - Resource: rbac.ResourceFile.WithID(uuid.NewString()).WithOwner(currentUser.String()), + Resource: rbac.ResourceFile.WithOwner(currentUser.String()), AuthorizeMap: map[bool][]authSubject{ true: {admin, memberMe, orgMemberMe}, false: {orgAdmin, otherOrgAdmin, otherOrgMember}, @@ -134,7 +134,7 @@ func TestRolePermissions(t *testing.T) { { Name: "Organizations", Actions: []rbac.Action{rbac.ActionUpdate, rbac.ActionDelete}, - Resource: rbac.ResourceOrganization.InOrg(orgID).WithID(orgID.String()), + Resource: rbac.ResourceOrganization.InOrg(orgID), AuthorizeMap: map[bool][]authSubject{ true: {admin, orgAdmin}, false: {otherOrgAdmin, otherOrgMember, memberMe, orgMemberMe}, @@ -143,7 +143,7 @@ func TestRolePermissions(t *testing.T) { { Name: "ReadOrganizations", Actions: []rbac.Action{rbac.ActionRead}, - Resource: rbac.ResourceOrganization.InOrg(orgID).WithID(orgID.String()), + Resource: rbac.ResourceOrganization.InOrg(orgID), AuthorizeMap: map[bool][]authSubject{ true: {admin, orgAdmin, orgMemberMe}, false: {otherOrgAdmin, otherOrgMember, memberMe}, @@ -188,7 +188,7 @@ func TestRolePermissions(t *testing.T) { { Name: "APIKey", Actions: []rbac.Action{rbac.ActionCreate, rbac.ActionRead, rbac.ActionUpdate, rbac.ActionDelete}, - Resource: rbac.ResourceAPIKey.WithOwner(currentUser.String()).WithID(uuid.NewString()), + Resource: rbac.ResourceAPIKey.WithOwner(currentUser.String()), AuthorizeMap: map[bool][]authSubject{ true: {admin, orgMemberMe, memberMe}, false: {orgAdmin, otherOrgAdmin, otherOrgMember}, @@ -197,7 +197,7 @@ func TestRolePermissions(t *testing.T) { { Name: "UserData", Actions: []rbac.Action{rbac.ActionCreate, rbac.ActionRead, rbac.ActionUpdate, rbac.ActionDelete}, - Resource: rbac.ResourceUserData.WithOwner(currentUser.String()).WithID(currentUser.String()), + Resource: rbac.ResourceUserData.WithOwner(currentUser.String()), AuthorizeMap: map[bool][]authSubject{ true: {admin, orgMemberMe, memberMe}, false: {orgAdmin, otherOrgAdmin, otherOrgMember}, @@ -206,7 +206,7 @@ func TestRolePermissions(t *testing.T) { { Name: "ManageOrgMember", Actions: []rbac.Action{rbac.ActionCreate, rbac.ActionUpdate, rbac.ActionDelete}, - Resource: rbac.ResourceOrganizationMember.InOrg(orgID).WithID(uuid.NewString()), + Resource: rbac.ResourceOrganizationMember.InOrg(orgID), AuthorizeMap: map[bool][]authSubject{ true: {admin, orgAdmin}, false: {orgMemberMe, memberMe, otherOrgAdmin, otherOrgMember}, @@ -215,7 +215,7 @@ func TestRolePermissions(t *testing.T) { { Name: "ReadOrgMember", Actions: []rbac.Action{rbac.ActionRead}, - Resource: rbac.ResourceOrganizationMember.InOrg(orgID).WithID(uuid.NewString()), + Resource: rbac.ResourceOrganizationMember.InOrg(orgID), AuthorizeMap: map[bool][]authSubject{ true: {admin, orgAdmin, orgMemberMe}, false: {memberMe, otherOrgAdmin, otherOrgMember}, diff --git a/coderd/rbac/example_test.go b/coderd/rbac/example_test.go index 3d5847a0f5635..98b23b9303c86 100644 --- a/coderd/rbac/example_test.go +++ b/coderd/rbac/example_test.go @@ -50,9 +50,6 @@ func TestExample(t *testing.T) { // Note 'database.Workspace' could fulfill the object interface and be passed in directly err := authorizer.Authorize(ctx, user.UserID, user.Roles, rbac.ActionRead, rbac.ResourceWorkspace.InOrg(defaultOrg).WithOwner(user.UserID)) require.NoError(t, err, "this user can their workspace") - - err = authorizer.Authorize(ctx, user.UserID, user.Roles, rbac.ActionRead, rbac.ResourceWorkspace.InOrg(defaultOrg).WithOwner(user.UserID).WithID("1234")) - require.NoError(t, err, "this user can read workspace '1234'") }) } diff --git a/coderd/rbac/object.go b/coderd/rbac/object.go index 3ae9499fd856d..d7ee6932fc5db 100644 --- a/coderd/rbac/object.go +++ b/coderd/rbac/object.go @@ -108,8 +108,7 @@ var ( // that represents the set of workspaces you are trying to get access too. // Do not export this type, as it can be created from a resource type constant. type Object struct { - ResourceID string `json:"id"` - Owner string `json:"owner"` + Owner string `json:"owner"` // OrgID specifies which org the object is a part of. OrgID string `json:"org_owner"` @@ -125,39 +124,26 @@ func (z Object) RBACObject() Object { // All returns an object matching all resources of the same type. func (z Object) All() Object { return Object{ - ResourceID: "", - Owner: "", - OrgID: "", - Type: z.Type, + Owner: "", + OrgID: "", + Type: z.Type, } } // InOrg adds an org OwnerID to the resource func (z Object) InOrg(orgID uuid.UUID) Object { return Object{ - ResourceID: z.ResourceID, - Owner: z.Owner, - OrgID: orgID.String(), - Type: z.Type, + Owner: z.Owner, + OrgID: orgID.String(), + Type: z.Type, } } // WithOwner adds an OwnerID to the resource func (z Object) WithOwner(ownerID string) Object { return Object{ - ResourceID: z.ResourceID, - Owner: ownerID, - OrgID: z.OrgID, - Type: z.Type, - } -} - -// WithID adds a ResourceID to the resource -func (z Object) WithID(resourceID string) Object { - return Object{ - ResourceID: resourceID, - Owner: z.Owner, - OrgID: z.OrgID, - Type: z.Type, + Owner: ownerID, + OrgID: z.OrgID, + Type: z.Type, } } diff --git a/coderd/rbac/policy.rego b/coderd/rbac/policy.rego index 9e35b827751d7..8f5208aca138e 100644 --- a/coderd/rbac/policy.rego +++ b/coderd/rbac/policy.rego @@ -22,17 +22,16 @@ bool_flip(b) = flipped { # perms_grant returns a set of boolean values {true, false}. # True means a positive permission in the set, false is a negative permission. # It will only return `bool_flip(perm.negate)` for permissions that affect a given -# resource_type, resource_id, and action. +# resource_type, and action. # The empty set is returned if no relevant permissions are found. perms_grant(permissions) = grants { # If there are no permissions, this value is the empty set {}. grants := { x | # All permissions ... perm := permissions[_] - # Such that the permission action, type, and resource_id matches + # Such that the permission action, and type matches perm.action in [input.action, "*"] perm.resource_type in [input.object.type, "*"] - perm.resource_id in [input.object.id, "*"] x := bool_flip(perm.negate) } } @@ -137,4 +136,4 @@ allow { not false in user # And all permissions are positive user[_] -} \ No newline at end of file +} diff --git a/coderd/rbac/role.go b/coderd/rbac/role.go index 9913a1091a68c..1aa97f9db0557 100644 --- a/coderd/rbac/role.go +++ b/coderd/rbac/role.go @@ -5,7 +5,6 @@ type Permission struct { // Negate makes this a negative permission Negate bool `json:"negate"` ResourceType string `json:"resource_type"` - ResourceID string `json:"resource_id"` Action Action `json:"action"` } diff --git a/coderd/roles.go b/coderd/roles.go index 36d5d88d734dc..33a99f226c168 100644 --- a/coderd/roles.go +++ b/coderd/roles.go @@ -43,7 +43,7 @@ func (api *API) assignableOrgRoles(rw http.ResponseWriter, r *http.Request) { func (api *API) checkPermissions(rw http.ResponseWriter, r *http.Request) { user := httpmw.UserParam(r) - if !api.Authorize(r, rbac.ActionRead, rbac.ResourceUser.WithID(user.ID.String())) { + if !api.Authorize(r, rbac.ActionRead, rbac.ResourceUser) { httpapi.ResourceNotFound(rw) return } diff --git a/coderd/users.go b/coderd/users.go index c41cdee03a1fd..41c5f49766b4e 100644 --- a/coderd/users.go +++ b/coderd/users.go @@ -258,7 +258,7 @@ func (api *API) userByName(rw http.ResponseWriter, r *http.Request) { user := httpmw.UserParam(r) organizationIDs, err := userOrganizationIDs(r.Context(), api, user) - if !api.Authorize(r, rbac.ActionRead, rbac.ResourceUser.WithID(user.ID.String())) { + if !api.Authorize(r, rbac.ActionRead, rbac.ResourceUser) { httpapi.ResourceNotFound(rw) return } @@ -277,7 +277,7 @@ func (api *API) userByName(rw http.ResponseWriter, r *http.Request) { func (api *API) putUserProfile(rw http.ResponseWriter, r *http.Request) { user := httpmw.UserParam(r) - if !api.Authorize(r, rbac.ActionUpdate, rbac.ResourceUser.WithID(user.ID.String())) { + if !api.Authorize(r, rbac.ActionUpdate, rbac.ResourceUser) { httpapi.ResourceNotFound(rw) return } @@ -345,7 +345,7 @@ func (api *API) putUserStatus(status database.UserStatus) func(rw http.ResponseW user := httpmw.UserParam(r) apiKey := httpmw.APIKey(r) - if !api.Authorize(r, rbac.ActionDelete, rbac.ResourceUser.WithID(user.ID.String())) { + if !api.Authorize(r, rbac.ActionDelete, rbac.ResourceUser) { httpapi.ResourceNotFound(rw) return } @@ -415,7 +415,7 @@ func (api *API) putUserPassword(rw http.ResponseWriter, r *http.Request) { // admins can change passwords without sending old_password if params.OldPassword == "" { - if !api.Authorize(r, rbac.ActionUpdate, rbac.ResourceUser.WithID(user.ID.String())) { + if !api.Authorize(r, rbac.ActionUpdate, rbac.ResourceUser) { httpapi.Forbidden(rw) return } @@ -519,7 +519,7 @@ func (api *API) putUserRoles(rw http.ResponseWriter, r *http.Request) { return } - if !api.Authorize(r, rbac.ActionRead, rbac.ResourceUser.WithID(user.ID.String())) { + if !api.Authorize(r, rbac.ActionRead, rbac.ResourceUser) { httpapi.ResourceNotFound(rw) return } @@ -527,19 +527,18 @@ func (api *API) putUserRoles(rw http.ResponseWriter, r *http.Request) { // The member role is always implied. impliedTypes := append(params.Roles, rbac.RoleMember()) added, removed := rbac.ChangeRoleSet(roles.Roles, impliedTypes) - for _, roleName := range added { - // Assigning a role requires the create permission. - if !api.Authorize(r, rbac.ActionCreate, rbac.ResourceRoleAssignment.WithID(roleName)) { - httpapi.Forbidden(rw) - return - } + // TODO: Handle added and removed roles. + + // Assigning a role requires the create permission. + if !api.Authorize(r, rbac.ActionCreate, rbac.ResourceRoleAssignment) { + httpapi.Forbidden(rw) + return } - for _, roleName := range removed { - // Removing a role requires the delete permission. - if !api.Authorize(r, rbac.ActionDelete, rbac.ResourceRoleAssignment.WithID(roleName)) { - httpapi.Forbidden(rw) - return - } + + // Removing a role requires the delete permission. + if !api.Authorize(r, rbac.ActionDelete, rbac.ResourceRoleAssignment) { + httpapi.Forbidden(rw) + return } updatedUser, err := api.updateSiteUserRoles(r.Context(), database.UpdateUserRolesParams{ @@ -631,8 +630,7 @@ func (api *API) organizationByUserAndName(rw http.ResponseWriter, r *http.Reques if !api.Authorize(r, rbac.ActionRead, rbac.ResourceOrganization. - InOrg(organization.ID). - WithID(organization.ID.String())) { + InOrg(organization.ID)) { httpapi.ResourceNotFound(rw) return } diff --git a/coderd/workspacebuilds.go b/coderd/workspacebuilds.go index 71df22717e5c2..bfe38229cd0a3 100644 --- a/coderd/workspacebuilds.go +++ b/coderd/workspacebuilds.go @@ -25,8 +25,7 @@ func (api *API) workspaceBuild(rw http.ResponseWriter, r *http.Request) { workspaceBuild := httpmw.WorkspaceBuildParam(r) workspace := httpmw.WorkspaceParam(r) - if !api.Authorize(r, rbac.ActionRead, rbac.ResourceWorkspace. - InOrg(workspace.OrganizationID).WithOwner(workspace.OwnerID.String()).WithID(workspace.ID.String())) { + if !api.Authorize(r, rbac.ActionRead, workspace) { httpapi.ResourceNotFound(rw) return } @@ -240,8 +239,7 @@ func (api *API) workspaceBuildByBuildNumber(rw http.ResponseWriter, r *http.Requ func (api *API) workspaceBuildByName(rw http.ResponseWriter, r *http.Request) { workspace := httpmw.WorkspaceParam(r) workspaceBuildName := chi.URLParam(r, "workspacebuildname") - if !api.Authorize(r, rbac.ActionRead, rbac.ResourceWorkspace. - InOrg(workspace.OrganizationID).WithOwner(workspace.OwnerID.String()).WithID(workspace.ID.String())) { + if !api.Authorize(r, rbac.ActionRead, workspace) { httpapi.ResourceNotFound(rw) return } @@ -304,8 +302,7 @@ func (api *API) postWorkspaceBuilds(rw http.ResponseWriter, r *http.Request) { }) return } - if !api.Authorize(r, action, rbac.ResourceWorkspace. - InOrg(workspace.OrganizationID).WithOwner(workspace.OwnerID.String()).WithID(workspace.ID.String())) { + if !api.Authorize(r, action, workspace) { httpapi.ResourceNotFound(rw) return } @@ -520,8 +517,7 @@ func (api *API) patchCancelWorkspaceBuild(rw http.ResponseWriter, r *http.Reques return } - if !api.Authorize(r, rbac.ActionUpdate, rbac.ResourceWorkspace. - InOrg(workspace.OrganizationID).WithOwner(workspace.OwnerID.String()).WithID(workspace.ID.String())) { + if !api.Authorize(r, rbac.ActionUpdate, workspace) { httpapi.ResourceNotFound(rw) return } @@ -575,8 +571,7 @@ func (api *API) workspaceBuildResources(rw http.ResponseWriter, r *http.Request) return } - if !api.Authorize(r, rbac.ActionRead, rbac.ResourceWorkspace. - InOrg(workspace.OrganizationID).WithOwner(workspace.OwnerID.String()).WithID(workspace.ID.String())) { + if !api.Authorize(r, rbac.ActionRead, workspace) { httpapi.ResourceNotFound(rw) return } @@ -602,8 +597,7 @@ func (api *API) workspaceBuildLogs(rw http.ResponseWriter, r *http.Request) { return } - if !api.Authorize(r, rbac.ActionRead, rbac.ResourceWorkspace. - InOrg(workspace.OrganizationID).WithOwner(workspace.OwnerID.String()).WithID(workspace.ID.String())) { + if !api.Authorize(r, rbac.ActionRead, workspace) { httpapi.ResourceNotFound(rw) return } From 1f7d5d360eb7e037934da35ff443941aeaf0bd1a Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 9 Aug 2022 11:00:11 -0500 Subject: [PATCH 2/6] Handle assigning/removing roles --- coderd/coderd_test.go | 3 --- coderd/members.go | 12 ++++++++-- coderd/rbac/builtin.go | 52 ++++++++++++++++++++++++++++++++++++++++++ coderd/rbac/object.go | 2 +- coderd/roles.go | 7 +++--- coderd/users.go | 15 ++++++++---- 6 files changed, 77 insertions(+), 14 deletions(-) diff --git a/coderd/coderd_test.go b/coderd/coderd_test.go index 67ecfbb5e3e5e..46649438ff2d5 100644 --- a/coderd/coderd_test.go +++ b/coderd/coderd_test.go @@ -542,9 +542,6 @@ func TestAuthorizeAllEndpoints(t *testing.T) { if routeAssertions.AssertObject.OrgID != "" { assert.Equal(t, routeAssertions.AssertObject.OrgID, authorizer.Called.Object.OrgID, "resource org") } - if routeAssertions.AssertObject.ResourceID != "" { - assert.Equal(t, routeAssertions.AssertObject.ResourceID, authorizer.Called.Object.ResourceID, "resource ID") - } } } else { assert.Nil(t, authorizer.Called, "authorize not expected") diff --git a/coderd/members.go b/coderd/members.go index fc3c1ea448bda..775d9978a007b 100644 --- a/coderd/members.go +++ b/coderd/members.go @@ -21,6 +21,7 @@ func (api *API) putMemberRoles(rw http.ResponseWriter, r *http.Request) { organization := httpmw.OrganizationParam(r) member := httpmw.OrganizationMemberParam(r) apiKey := httpmw.APIKey(r) + actorRoles := httpmw.AuthorizationUserRoles(r) if apiKey.UserID == member.UserID { httpapi.Write(rw, http.StatusBadRequest, codersdk.Response{ @@ -41,17 +42,24 @@ func (api *API) putMemberRoles(rw http.ResponseWriter, r *http.Request) { // TODO: Handle added and removed roles. // Assigning a role requires the create permission. - if !api.Authorize(r, rbac.ActionCreate, rbac.ResourceOrgRoleAssignment.InOrg(organization.ID)) { + if len(added) > 0 && !api.Authorize(r, rbac.ActionCreate, rbac.ResourceOrgRoleAssignment.InOrg(organization.ID)) { httpapi.Forbidden(rw) return } // Removing a role requires the delete permission. - if !api.Authorize(r, rbac.ActionDelete, rbac.ResourceOrgRoleAssignment.InOrg(organization.ID)) { + if len(removed) > 0 && !api.Authorize(r, rbac.ActionDelete, rbac.ResourceOrgRoleAssignment.InOrg(organization.ID)) { httpapi.Forbidden(rw) return } + // Just treat adding & removing as "assigning" for now. + for _, roleName := range append(added, removed...) { + if !rbac.CanAssignRole(actorRoles.Roles, roleName) { + httpapi.Forbidden(rw) + } + } + updatedUser, err := api.updateOrganizationMemberRoles(r.Context(), database.UpdateMemberRolesParams{ GrantedRoles: params.Roles, UserID: user.ID, diff --git a/coderd/rbac/builtin.go b/coderd/rbac/builtin.go index 630836db2d8b4..cd51d88361636 100644 --- a/coderd/rbac/builtin.go +++ b/coderd/rbac/builtin.go @@ -145,6 +145,58 @@ var ( } ) +var ( + // assignRoles is a map of roles that can be assigned if a user has a given + // role. + // The first key is the actor role, the second is the roles they can assign. + // map[actor_role][assign_role] + assignRoles = map[string]map[string]bool{ + admin: { + admin: true, + auditor: true, + member: true, + orgAdmin: true, + orgMember: true, + }, + orgAdmin: { + orgAdmin: true, + orgMember: true, + }, + } +) + +// CanAssignRole is a helper function that returns true if the user can assign +// the specified role. This also can be used for removing a role. +// This is a simple implementation for now. +func CanAssignRole(roles []string, assignedRole string) bool { + assigned, assignedOrg, err := roleSplit(assignedRole) + if err != nil { + return false + } + + for _, longRole := range roles { + role, orgID, err := roleSplit(longRole) + if err != nil { + continue + } + + if orgID != "" && orgID != assignedOrg { + // Org roles only apply to the org they are assigned to. + continue + } + + allowed, ok := assignRoles[role] + if !ok { + continue + } + + if allowed[assigned] { + return true + } + } + return false +} + // RoleByName returns the permissions associated with a given role name. // This allows just the role names to be stored and expanded when required. func RoleByName(name string) (Role, error) { diff --git a/coderd/rbac/object.go b/coderd/rbac/object.go index d7ee6932fc5db..6106dd8079015 100644 --- a/coderd/rbac/object.go +++ b/coderd/rbac/object.go @@ -88,7 +88,7 @@ var ( } // ResourceOrganizationMember is a user's membership in an organization. - // Has ONLY an organization owner. The resource ID is the user's ID + // Has ONLY an organization owner. // create/delete = Create/delete member from org. // update = Update organization member // read = View member diff --git a/coderd/roles.go b/coderd/roles.go index 33a99f226c168..05013f696cd94 100644 --- a/coderd/roles.go +++ b/coderd/roles.go @@ -74,10 +74,9 @@ func (api *API) checkPermissions(rw http.ResponseWriter, r *http.Request) { } err := api.Authorizer.ByRoleName(r.Context(), roles.ID.String(), roles.Roles, rbac.Action(v.Action), rbac.Object{ - ResourceID: v.Object.ResourceID, - Owner: v.Object.OwnerID, - OrgID: v.Object.OrganizationID, - Type: v.Object.ResourceType, + Owner: v.Object.OwnerID, + OrgID: v.Object.OrganizationID, + Type: v.Object.ResourceType, }) response[k] = err == nil } diff --git a/coderd/users.go b/coderd/users.go index 41c5f49766b4e..7765dfe1085a9 100644 --- a/coderd/users.go +++ b/coderd/users.go @@ -504,7 +504,7 @@ func (api *API) userRoles(rw http.ResponseWriter, r *http.Request) { func (api *API) putUserRoles(rw http.ResponseWriter, r *http.Request) { // User is the user to modify. user := httpmw.UserParam(r) - roles := httpmw.AuthorizationUserRoles(r) + actorRoles := httpmw.AuthorizationUserRoles(r) apiKey := httpmw.APIKey(r) if apiKey.UserID == user.ID { @@ -526,21 +526,28 @@ func (api *API) putUserRoles(rw http.ResponseWriter, r *http.Request) { // The member role is always implied. impliedTypes := append(params.Roles, rbac.RoleMember()) - added, removed := rbac.ChangeRoleSet(roles.Roles, impliedTypes) + added, removed := rbac.ChangeRoleSet(user.RBACRoles, impliedTypes) // TODO: Handle added and removed roles. // Assigning a role requires the create permission. - if !api.Authorize(r, rbac.ActionCreate, rbac.ResourceRoleAssignment) { + if len(added) > 0 && !api.Authorize(r, rbac.ActionCreate, rbac.ResourceRoleAssignment) { httpapi.Forbidden(rw) return } // Removing a role requires the delete permission. - if !api.Authorize(r, rbac.ActionDelete, rbac.ResourceRoleAssignment) { + if len(removed) > 0 && !api.Authorize(r, rbac.ActionDelete, rbac.ResourceRoleAssignment) { httpapi.Forbidden(rw) return } + // Just treat adding & removing as "assigning" for now. + for _, roleName := range append(added, removed...) { + if !rbac.CanAssignRole(actorRoles.Roles, roleName) { + httpapi.Forbidden(rw) + } + } + updatedUser, err := api.updateSiteUserRoles(r.Context(), database.UpdateUserRolesParams{ GrantedRoles: params.Roles, ID: user.ID, From 0376027c0d0b4e5271996637a1734b22649507d6 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 9 Aug 2022 11:34:08 -0500 Subject: [PATCH 3/6] Update assign user roles test --- coderd/coderdtest/coderdtest.go | 9 +- coderd/users_test.go | 219 +++++++++++++++++++++----------- 2 files changed, 154 insertions(+), 74 deletions(-) diff --git a/coderd/coderdtest/coderdtest.go b/coderd/coderdtest/coderdtest.go index c858d52a68dd3..90f9482999862 100644 --- a/coderd/coderdtest/coderdtest.go +++ b/coderd/coderdtest/coderdtest.go @@ -275,10 +275,15 @@ func CreateFirstUser(t *testing.T, client *codersdk.Client) codersdk.CreateFirst // CreateAnotherUser creates and authenticates a new user. func CreateAnotherUser(t *testing.T, client *codersdk.Client, organizationID uuid.UUID, roles ...string) *codersdk.Client { + userClient, _ := createAnotherUserRetry(t, client, organizationID, 5, roles...) + return userClient +} + +func CreateAnotherUserWithUser(t *testing.T, client *codersdk.Client, organizationID uuid.UUID, roles ...string) (*codersdk.Client, codersdk.User) { return createAnotherUserRetry(t, client, organizationID, 5, roles...) } -func createAnotherUserRetry(t *testing.T, client *codersdk.Client, organizationID uuid.UUID, retries int, roles ...string) *codersdk.Client { +func createAnotherUserRetry(t *testing.T, client *codersdk.Client, organizationID uuid.UUID, retries int, roles ...string) (*codersdk.Client, codersdk.User) { req := codersdk.CreateUserRequest{ Email: namesgenerator.GetRandomName(10) + "@coder.com", Username: randomUsername(), @@ -337,7 +342,7 @@ func createAnotherUserRetry(t *testing.T, client *codersdk.Client, organizationI require.NoError(t, err, "update org membership roles") } } - return other + return other, user } // CreateTemplateVersion creates a template import provisioner job diff --git a/coderd/users_test.go b/coderd/users_test.go index 72c3ba2f99420..b196b7e63965c 100644 --- a/coderd/users_test.go +++ b/coderd/users_test.go @@ -385,7 +385,7 @@ func TestUpdateUserPassword(t *testing.T) { }) } -func TestGrantRoles(t *testing.T) { +func TestGrantSiteRoles(t *testing.T) { t.Parallel() requireStatusCode := func(t *testing.T, err error, statusCode int) { @@ -395,86 +395,161 @@ func TestGrantRoles(t *testing.T) { require.Equal(t, statusCode, e.StatusCode(), "correct status code") } - t.Run("UpdateIncorrectRoles", func(t *testing.T) { - t.Parallel() - ctx := context.Background() - var err error - - admin := coderdtest.New(t, nil) - first := coderdtest.CreateFirstUser(t, admin) - member := coderdtest.CreateAnotherUser(t, admin, first.OrganizationID) - - _, err = admin.UpdateUserRoles(ctx, codersdk.Me, codersdk.UpdateRoles{ - Roles: []string{rbac.RoleOrgAdmin(first.OrganizationID)}, - }) - require.Error(t, err, "org role in site") - requireStatusCode(t, err, http.StatusBadRequest) - - _, err = admin.UpdateUserRoles(ctx, uuid.New().String(), codersdk.UpdateRoles{ - Roles: []string{rbac.RoleOrgAdmin(first.OrganizationID)}, - }) - require.Error(t, err, "user does not exist") - requireStatusCode(t, err, http.StatusBadRequest) - - _, err = admin.UpdateOrganizationMemberRoles(ctx, first.OrganizationID, codersdk.Me, codersdk.UpdateRoles{ - Roles: []string{rbac.RoleAdmin()}, - }) - require.Error(t, err, "site role in org") - requireStatusCode(t, err, http.StatusBadRequest) - - _, err = admin.UpdateOrganizationMemberRoles(ctx, uuid.New(), codersdk.Me, codersdk.UpdateRoles{ - Roles: []string{}, - }) - require.Error(t, err, "role in org without membership") - requireStatusCode(t, err, http.StatusNotFound) + ctx := context.Background() + var err error + + admin := coderdtest.New(t, nil) + first := coderdtest.CreateFirstUser(t, admin) + member := coderdtest.CreateAnotherUser(t, admin, first.OrganizationID) + orgAdmin := coderdtest.CreateAnotherUser(t, admin, first.OrganizationID, rbac.RoleOrgAdmin(first.OrganizationID)) + randOrg, err := admin.CreateOrganization(ctx, codersdk.CreateOrganizationRequest{ + Name: "random", + }) + require.NoError(t, err) + _, randOrgUser := coderdtest.CreateAnotherUserWithUser(t, admin, randOrg.ID, rbac.RoleOrgAdmin(randOrg.ID)) - _, err = member.UpdateUserRoles(ctx, first.UserID.String(), codersdk.UpdateRoles{ - Roles: []string{}, - }) - require.Error(t, err, "member cannot change other's roles") - requireStatusCode(t, err, http.StatusForbidden) + const newUser = "newUser" - _, err = member.UpdateUserRoles(ctx, first.UserID.String(), codersdk.UpdateRoles{ - Roles: []string{}, - }) - require.Error(t, err, "member cannot change any roles") - requireStatusCode(t, err, http.StatusForbidden) + testCases := []struct { + Name string + Client *codersdk.Client + OrgID uuid.UUID + AssignToUser string + Roles []string + Error bool + StatusCode int + }{ + { + Name: "OrgRoleInSite", + Client: admin, + AssignToUser: codersdk.Me, + Roles: []string{rbac.RoleOrgAdmin(first.OrganizationID)}, + Error: true, + StatusCode: http.StatusBadRequest, + }, + { + Name: "UserNotExists", + Client: admin, + AssignToUser: uuid.NewString(), + Roles: []string{rbac.RoleAdmin()}, + Error: true, + StatusCode: http.StatusBadRequest, + }, + { + Name: "MemberCannotUpdateRoles", + Client: member, + AssignToUser: first.UserID.String(), + Roles: []string{}, + Error: true, + StatusCode: http.StatusForbidden, + }, + { + // Cannot update your own roles + Name: "AdminOnSelf", + Client: admin, + AssignToUser: first.UserID.String(), + Roles: []string{}, + Error: true, + StatusCode: http.StatusBadRequest, + }, + { + Name: "SiteRoleInOrg", + Client: admin, + OrgID: first.OrganizationID, + AssignToUser: codersdk.Me, + Roles: []string{rbac.RoleAdmin()}, + Error: true, + StatusCode: http.StatusBadRequest, + }, + { + Name: "RoleInNotMemberOrg", + Client: orgAdmin, + OrgID: randOrg.ID, + AssignToUser: randOrgUser.ID.String(), + Roles: []string{rbac.RoleOrgMember(randOrg.ID)}, + Error: true, + StatusCode: http.StatusForbidden, + }, + { + Name: "MemberAssignMember", + Client: member, + OrgID: first.OrganizationID, + AssignToUser: first.UserID.String(), + Roles: []string{}, + Error: true, + StatusCode: http.StatusForbidden, + }, + { + Name: "AdminUpdateOrgSelf", + Client: admin, + OrgID: first.OrganizationID, + AssignToUser: first.UserID.String(), + Roles: []string{}, + Error: true, + StatusCode: http.StatusBadRequest, + }, + { + Name: "OrgAdminPromote", + Client: orgAdmin, + OrgID: first.OrganizationID, + AssignToUser: newUser, + Roles: []string{rbac.RoleOrgAdmin(first.OrganizationID)}, + Error: false, + }, + } - _, err = member.UpdateOrganizationMemberRoles(ctx, first.OrganizationID, first.UserID.String(), codersdk.UpdateRoles{ - Roles: []string{}, - }) - require.Error(t, err, "member cannot change other's org roles") - requireStatusCode(t, err, http.StatusForbidden) + for _, c := range testCases { + c := c + t.Run(c.Name, func(t *testing.T) { + t.Parallel() + var err error + if c.AssignToUser == newUser { + orgID := first.OrganizationID + if c.OrgID != uuid.Nil { + orgID = c.OrgID + } + _, newUser := coderdtest.CreateAnotherUserWithUser(t, admin, orgID) + c.AssignToUser = newUser.ID.String() + } - _, err = admin.UpdateUserRoles(ctx, first.UserID.String(), codersdk.UpdateRoles{ - Roles: []string{}, - }) - require.Error(t, err, "admin cannot change self roles") - requireStatusCode(t, err, http.StatusBadRequest) + if c.OrgID != uuid.Nil { + // Org assign + _, err = c.Client.UpdateOrganizationMemberRoles(ctx, c.OrgID, c.AssignToUser, codersdk.UpdateRoles{ + Roles: c.Roles, + }) + } else { + // Site assign + _, err = c.Client.UpdateUserRoles(ctx, c.AssignToUser, codersdk.UpdateRoles{ + Roles: c.Roles, + }) + } - _, err = admin.UpdateOrganizationMemberRoles(ctx, first.OrganizationID, first.UserID.String(), codersdk.UpdateRoles{ - Roles: []string{}, + if c.Error { + require.Error(t, err) + requireStatusCode(t, err, c.StatusCode) + } else { + require.NoError(t, err) + } }) - require.Error(t, err, "admin cannot change self org roles") - requireStatusCode(t, err, http.StatusBadRequest) - }) + } +} - t.Run("FirstUserRoles", func(t *testing.T) { - t.Parallel() - ctx := context.Background() - client := coderdtest.New(t, nil) - first := coderdtest.CreateFirstUser(t, client) +// TestInitialRoles ensures the starting roles for the first user are correct. +func TestInitialRoles(t *testing.T) { + t.Parallel() + ctx := context.Background() + client := coderdtest.New(t, nil) + first := coderdtest.CreateFirstUser(t, client) - roles, err := client.GetUserRoles(ctx, codersdk.Me) - require.NoError(t, err) - require.ElementsMatch(t, roles.Roles, []string{ - rbac.RoleAdmin(), - }, "should be a member and admin") + roles, err := client.GetUserRoles(ctx, codersdk.Me) + require.NoError(t, err) + require.ElementsMatch(t, roles.Roles, []string{ + rbac.RoleAdmin(), + }, "should be a member and admin") - require.ElementsMatch(t, roles.OrganizationRoles[first.OrganizationID], []string{ - rbac.RoleOrgAdmin(first.OrganizationID), - }, "should be a member and admin") - }) + require.ElementsMatch(t, roles.OrganizationRoles[first.OrganizationID], []string{ + rbac.RoleOrgAdmin(first.OrganizationID), + }, "should be a member and admin") t.Run("GrantAdmin", func(t *testing.T) { t.Parallel() From ea41968e3728408848fcf106b6e64145270339b0 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 9 Aug 2022 11:49:04 -0500 Subject: [PATCH 4/6] Return after forbidden --- coderd/database/modelmethods.go | 4 ++-- coderd/members.go | 1 + coderd/users.go | 1 + 3 files changed, 4 insertions(+), 2 deletions(-) diff --git a/coderd/database/modelmethods.go b/coderd/database/modelmethods.go index fd8296687c02d..d83180247ac83 100644 --- a/coderd/database/modelmethods.go +++ b/coderd/database/modelmethods.go @@ -25,7 +25,7 @@ func (o Organization) RBACObject() rbac.Object { return rbac.ResourceOrganization.InOrg(o.ID) } -func (d ProvisionerDaemon) RBACObject() rbac.Object { +func (ProvisionerDaemon) RBACObject() rbac.Object { return rbac.ResourceProvisionerDaemon } @@ -36,6 +36,6 @@ func (f File) RBACObject() rbac.Object { // RBACObject returns the RBAC object for the site wide user resource. // If you are trying to get the RBAC object for the UserData, use // rbac.ResourceUserData -func (u User) RBACObject() rbac.Object { +func (User) RBACObject() rbac.Object { return rbac.ResourceUser } diff --git a/coderd/members.go b/coderd/members.go index 775d9978a007b..a01d026ead9e2 100644 --- a/coderd/members.go +++ b/coderd/members.go @@ -57,6 +57,7 @@ func (api *API) putMemberRoles(rw http.ResponseWriter, r *http.Request) { for _, roleName := range append(added, removed...) { if !rbac.CanAssignRole(actorRoles.Roles, roleName) { httpapi.Forbidden(rw) + return } } diff --git a/coderd/users.go b/coderd/users.go index 7765dfe1085a9..131171d1c5f2f 100644 --- a/coderd/users.go +++ b/coderd/users.go @@ -545,6 +545,7 @@ func (api *API) putUserRoles(rw http.ResponseWriter, r *http.Request) { for _, roleName := range append(added, removed...) { if !rbac.CanAssignRole(actorRoles.Roles, roleName) { httpapi.Forbidden(rw) + return } } From 186bfc104db69a60b80ca33d6b0068599dc119be Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 9 Aug 2022 12:45:07 -0500 Subject: [PATCH 5/6] Add benchmark for Filter --- coderd/rbac/builtin_test.go | 81 +++++++++++++++++++++++++++++++++++++ 1 file changed, 81 insertions(+) diff --git a/coderd/rbac/builtin_test.go b/coderd/rbac/builtin_test.go index 1ce6e2eab6733..9026dd7b213eb 100644 --- a/coderd/rbac/builtin_test.go +++ b/coderd/rbac/builtin_test.go @@ -12,6 +12,87 @@ import ( "github.com/coder/coder/coderd/rbac" ) +// BenchmarkRBACFilter benchmarks the rbac.Filter method. +// go test -bench BenchmarkRBACFilter -benchmem -memprofile memprofile.out -cpuprofile profile.out +func BenchmarkRBACFilter(b *testing.B) { + orgs := []uuid.UUID{ + uuid.MustParse("bf7b72bd-a2b1-4ef2-962c-1d698e0483f6"), + uuid.MustParse("e4660c6f-b9de-422d-9578-cd888983a795"), + uuid.MustParse("fb13d477-06f4-42d9-b957-f6b89bd63515"), + } + + users := []uuid.UUID{ + uuid.MustParse("10d03e62-7703-4df5-a358-4f76577d4e2f"), + uuid.MustParse("4ca78b1d-f2d2-4168-9d76-cd93b51c6c1e"), + uuid.MustParse("0632b012-49e0-4d70-a5b3-f4398f1dcd52"), + uuid.MustParse("70dbaa7a-ea9c-4f68-a781-97b08af8461d"), + } + + benchCases := []struct { + Name string + Roles []string + UserID uuid.UUID + }{ + { + Name: "NoRoles", + Roles: []string{}, + UserID: users[0], + }, + { + Name: "Admin", + // Give some extra roles that an admin might have + Roles: []string{rbac.RoleOrgMember(orgs[0]), "auditor", rbac.RoleAdmin(), rbac.RoleMember()}, + UserID: users[0], + }, + { + Name: "OrgAdmin", + Roles: []string{rbac.RoleOrgMember(orgs[0]), rbac.RoleOrgAdmin(orgs[0]), rbac.RoleMember()}, + UserID: users[0], + }, + { + Name: "OrgMember", + // Member of 2 orgs + Roles: []string{rbac.RoleOrgMember(orgs[0]), rbac.RoleOrgMember(orgs[1]), rbac.RoleMember()}, + UserID: users[0], + }, + { + Name: "ManyRoles", + // Admin of many orgs + Roles: []string{ + rbac.RoleOrgMember(orgs[0]), rbac.RoleOrgAdmin(orgs[0]), + rbac.RoleOrgMember(orgs[1]), rbac.RoleOrgAdmin(orgs[1]), + rbac.RoleOrgMember(orgs[2]), rbac.RoleOrgAdmin(orgs[2]), + rbac.RoleMember()}, + UserID: users[0], + }, + } + + authorizer, err := rbac.NewAuthorizer() + if err != nil { + require.NoError(b, err) + } + for _, c := range benchCases { + b.Run(c.Name, func(b *testing.B) { + objects := benchmarkSetup(orgs, users, b.N) + b.ResetTimer() + allowed := rbac.Filter(context.Background(), authorizer, c.UserID.String(), c.Roles, rbac.ActionRead, objects) + var _ = allowed + }) + } +} + +func benchmarkSetup(orgs []uuid.UUID, users []uuid.UUID, size int) []rbac.Object { + // Create a "random" but deterministic set of objects. + objectList := make([]rbac.Object, size) + for i := range objectList { + objectList[i] = rbac.ResourceWorkspace. + InOrg(orgs[i%len(orgs)]). + WithOwner(users[i%len(users)].String()) + } + + return objectList +} + type authSubject struct { // Name is helpful for test assertions Name string From 64a7d121626575f49d8e9a85c3c8fe575417b756 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 9 Aug 2022 13:06:05 -0500 Subject: [PATCH 6/6] Drop comments that are no longer relevent --- coderd/members.go | 2 -- coderd/users.go | 1 - 2 files changed, 3 deletions(-) diff --git a/coderd/members.go b/coderd/members.go index a01d026ead9e2..35f2ad31face7 100644 --- a/coderd/members.go +++ b/coderd/members.go @@ -39,8 +39,6 @@ func (api *API) putMemberRoles(rw http.ResponseWriter, r *http.Request) { impliedTypes := append(params.Roles, rbac.RoleOrgMember(organization.ID)) added, removed := rbac.ChangeRoleSet(member.Roles, impliedTypes) - // TODO: Handle added and removed roles. - // Assigning a role requires the create permission. if len(added) > 0 && !api.Authorize(r, rbac.ActionCreate, rbac.ResourceOrgRoleAssignment.InOrg(organization.ID)) { httpapi.Forbidden(rw) diff --git a/coderd/users.go b/coderd/users.go index 131171d1c5f2f..51509b0cc800e 100644 --- a/coderd/users.go +++ b/coderd/users.go @@ -527,7 +527,6 @@ func (api *API) putUserRoles(rw http.ResponseWriter, r *http.Request) { // The member role is always implied. impliedTypes := append(params.Roles, rbac.RoleMember()) added, removed := rbac.ChangeRoleSet(user.RBACRoles, impliedTypes) - // TODO: Handle added and removed roles. // Assigning a role requires the create permission. if len(added) > 0 && !api.Authorize(r, rbac.ActionCreate, rbac.ResourceRoleAssignment) {