Skip to content

chore: Drop resource_id support in rbac system #3426

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Aug 9, 2022
Merged

chore: Drop resource_id support in rbac system #3426

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
44423b3
chore: Drop resource_id support in rbac system
Emyrk Aug 9, 2022
1f7d5d3
Handle assigning/removing roles
Emyrk Aug 9, 2022
0376027
Update assign user roles test
Emyrk Aug 9, 2022
ea41968
Return after forbidden
Emyrk Aug 9, 2022
7a06b27
Merge remote-tracking branch 'origin/main' into stevenmasley/rbac_no_ids
Emyrk Aug 9, 2022
186bfc1
Add benchmark for Filter
Emyrk Aug 9, 2022
64a7d12
Drop comments that are no longer relevent
Emyrk Aug 9, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 25 additions & 28 deletions coderd/coderd_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,7 +219,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{
Expand Down Expand Up @@ -346,107 +346,107 @@ 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,
AssertObject: rbac.ResourceTemplate.InOrg(organization.ID),
},
"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,
Expand Down Expand Up @@ -546,9 +546,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")
Expand Down
9 changes: 7 additions & 2 deletions coderd/coderdtest/coderdtest.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Expand Down Expand Up @@ -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
Expand Down
20 changes: 10 additions & 10 deletions coderd/database/modelmethods.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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())
func (ProvisionerDaemon) RBACObject() rbac.Object {
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())
func (User) RBACObject() rbac.Object {
return rbac.ResourceUser
}
2 changes: 1 addition & 1 deletion coderd/files.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
25 changes: 16 additions & 9 deletions coderd/members.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand All @@ -37,16 +38,22 @@ 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
}

// Assigning a role requires the create permission.
if len(added) > 0 && !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)) {

// Removing a role requires the delete permission.
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)
return
}
Expand Down
3 changes: 1 addition & 2 deletions coderd/organizations.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
Loading