From 291b35b31b46e9401303c4b903fb7e3073d9ea1d Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Mon, 23 May 2022 13:27:27 -0500 Subject: [PATCH 1/7] feat: Generic Filter method for rbac objects --- coderd/coderd_test.go | 6 ++- coderd/database/modelmethods.go | 7 +++ coderd/rbac/authz.go | 18 +++++++ coderd/rbac/authz_test.go | 85 +++++++++++++++++++++++++++++++-- coderd/rbac/fake_test.go | 15 ++++++ coderd/rbac/object.go | 9 ++++ coderd/templates.go | 7 +++ 7 files changed, 143 insertions(+), 4 deletions(-) create mode 100644 coderd/database/modelmethods.go create mode 100644 coderd/rbac/fake_test.go diff --git a/coderd/coderd_test.go b/coderd/coderd_test.go index f6d555fb3d015..4eaf6c32e36d6 100644 --- a/coderd/coderd_test.go +++ b/coderd/coderd_test.go @@ -97,7 +97,6 @@ func TestAuthorizeAllEndpoints(t *testing.T) { "PUT:/api/v2/organizations/{organization}/members/{user}/roles": {NoAuthorize: true}, "GET:/api/v2/organizations/{organization}/provisionerdaemons": {NoAuthorize: true}, "POST:/api/v2/organizations/{organization}/templates": {NoAuthorize: true}, - "GET:/api/v2/organizations/{organization}/templates": {NoAuthorize: true}, "GET:/api/v2/organizations/{organization}/templates/{templatename}": {NoAuthorize: true}, "POST:/api/v2/organizations/{organization}/templateversions": {NoAuthorize: true}, "POST:/api/v2/organizations/{organization}/workspaces": {NoAuthorize: true}, @@ -183,6 +182,11 @@ func TestAuthorizeAllEndpoints(t *testing.T) { AssertAction: rbac.ActionRead, AssertObject: workspaceRBACObj, }, + "GET:/api/v2/organizations/{organization}/templates": { + StatusCode: http.StatusOK, + AssertAction: rbac.ActionRead, + AssertObject: rbac.ResourceTemplate.InOrg(template.OrganizationID).WithID(template.ID.String()), + }, // These endpoints need payloads to get to the auth part. Payloads will be required "PUT:/api/v2/users/{user}/roles": {StatusCode: http.StatusBadRequest, NoAuthorize: true}, diff --git a/coderd/database/modelmethods.go b/coderd/database/modelmethods.go new file mode 100644 index 0000000000000..a316b839041df --- /dev/null +++ b/coderd/database/modelmethods.go @@ -0,0 +1,7 @@ +package database + +import "github.com/coder/coder/coderd/rbac" + +func (t Template) RBACObject() rbac.Object { + return rbac.ResourceTemplate.InOrg(t.OrganizationID).WithID(t.ID.String()) +} diff --git a/coderd/rbac/authz.go b/coderd/rbac/authz.go index 6325a4b8c506b..9f10131613e1e 100644 --- a/coderd/rbac/authz.go +++ b/coderd/rbac/authz.go @@ -13,6 +13,24 @@ type Authorizer interface { ByRoleName(ctx context.Context, subjectID string, roleNames []string, action Action, object Object) error } +// Filter takes in a list of objects, and will filter the list removing all +// the elements the subject does not have permission for. +// Filter does not allocate a new slice, and will use the existing one +// passed in. This can cause memory leaks if the slice is held for a prolonged +// period of time. +func Filter[O IsObject](ctx context.Context, auth Authorizer, subjID string, subjRoles []string, action Action, objects []O) []O { + var currentIdx int + for i := range objects { + object := objects[i] + err := auth.ByRoleName(ctx, subjID, subjRoles, action, object.RBACObject()) + if err == nil { + objects[currentIdx] = objects[i] + currentIdx++ + } + } + return objects[:currentIdx] +} + // RegoAuthorizer will use a prepared rego query for performing authorize() type RegoAuthorizer struct { query rego.PreparedEvalQuery diff --git a/coderd/rbac/authz_test.go b/coderd/rbac/authz_test.go index 3e797b238e982..4ceb8f1a687b7 100644 --- a/coderd/rbac/authz_test.go +++ b/coderd/rbac/authz_test.go @@ -4,13 +4,12 @@ import ( "context" "encoding/json" "fmt" + "strconv" "testing" "github.com/google/uuid" - - "golang.org/x/xerrors" - "github.com/stretchr/testify/require" + "golang.org/x/xerrors" "github.com/coder/coder/coderd/rbac" ) @@ -24,6 +23,86 @@ type subject struct { Roles []rbac.Role `json:"roles"` } +func TestFilter(t *testing.T) { + t.Parallel() + + objectList := make([]rbac.Object, 0) + 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") + + workspaceList = append(workspaceList, workspace) + fileList = append(fileList, file) + + objectList = append(objectList, workspace) + objectList = append(objectList, file) + } + + testCases := []struct { + Name string + List []rbac.Object + Expected []rbac.Object + Auth func(o rbac.Object) error + }{ + { + Name: "FilterWorkspaceType", + List: objectList, + Expected: workspaceList, + Auth: func(o rbac.Object) error { + if o.Type != rbac.ResourceWorkspace.Type { + return xerrors.New("wrong type") + } + return nil + }, + }, + { + Name: "FilterFileType", + List: objectList, + Expected: fileList, + Auth: func(o rbac.Object) error { + if o.Type != rbac.ResourceFile.Type { + return xerrors.New("wrong type") + } + return nil + }, + }, + { + Name: "FilterAll", + List: objectList, + Expected: []rbac.Object{}, + Auth: func(o rbac.Object) error { + return xerrors.New("always fail") + }, + }, + { + Name: "FilterNone", + List: objectList, + Expected: objectList, + Auth: func(o rbac.Object) error { + return nil + }, + }, + } + + for _, c := range testCases { + c := c + t.Run(c.Name, func(t *testing.T) { + authorizer := fakeAuthorizer{ + AuthFunc: func(_ context.Context, _ string, _ []string, _ rbac.Action, object rbac.Object) error { + return c.Auth(object) + }, + } + + filtered := rbac.Filter(context.Background(), authorizer, "me", []string{}, rbac.ActionRead, c.List) + require.ElementsMatch(t, filtered, c.Expected, "expect same list") + require.Equal(t, len(c.Expected), len(filtered), "same length list") + }) + } +} + // TestAuthorizeDomain test the very basic roles that are commonly used. func TestAuthorizeDomain(t *testing.T) { t.Parallel() diff --git a/coderd/rbac/fake_test.go b/coderd/rbac/fake_test.go new file mode 100644 index 0000000000000..e8f6a90394cc7 --- /dev/null +++ b/coderd/rbac/fake_test.go @@ -0,0 +1,15 @@ +package rbac_test + +import ( + "context" + + "github.com/coder/coder/coderd/rbac" +) + +type fakeAuthorizer struct { + AuthFunc func(ctx context.Context, subjectID string, roleNames []string, action rbac.Action, object rbac.Object) error +} + +func (f fakeAuthorizer) ByRoleName(ctx context.Context, subjectID string, roleNames []string, action rbac.Action, object rbac.Object) error { + return f.AuthFunc(ctx, subjectID, roleNames, action, object) +} diff --git a/coderd/rbac/object.go b/coderd/rbac/object.go index 862653f50286e..3ce0ac9d39267 100644 --- a/coderd/rbac/object.go +++ b/coderd/rbac/object.go @@ -6,6 +6,11 @@ import ( const WildcardSymbol = "*" +// IsObject returns the RBAC object for itself. +type IsObject interface { + RBACObject() Object +} + // Resources are just typed objects. Making resources this way allows directly // passing them into an Authorize function and use the chaining api. var ( @@ -99,6 +104,10 @@ type Object struct { // TODO: SharedUsers? } +func (z Object) RBACObject() Object { + return z +} + // All returns an object matching all resources of the same type. func (z Object) All() Object { return Object{ diff --git a/coderd/templates.go b/coderd/templates.go index 87cbc7874d1df..57eeb1614a618 100644 --- a/coderd/templates.go +++ b/coderd/templates.go @@ -13,6 +13,7 @@ import ( "github.com/coder/coder/coderd/database" "github.com/coder/coder/coderd/httpapi" "github.com/coder/coder/coderd/httpmw" + "github.com/coder/coder/coderd/rbac" "github.com/coder/coder/codersdk" ) @@ -182,6 +183,7 @@ func (api *api) postTemplateByOrganization(rw http.ResponseWriter, r *http.Reque func (api *api) templatesByOrganization(rw http.ResponseWriter, r *http.Request) { organization := httpmw.OrganizationParam(r) + roles := httpmw.UserRoles(r) templates, err := api.Database.GetTemplatesByOrganization(r.Context(), database.GetTemplatesByOrganizationParams{ OrganizationID: organization.ID, }) @@ -194,7 +196,12 @@ func (api *api) templatesByOrganization(rw http.ResponseWriter, r *http.Request) }) return } + + // Filter templates based on rbac permissions + templates = rbac.Filter(r.Context(), api.Authorizer, roles.ID.String(), roles.Roles, rbac.ActionRead, templates) + templateIDs := make([]uuid.UUID, 0, len(templates)) + for _, template := range templates { templateIDs = append(templateIDs, template.ID) } From b825ce96e35099ce92927776fc304d67943bae92 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Mon, 23 May 2022 13:30:37 -0500 Subject: [PATCH 2/7] Make filter easier to use" --- coderd/authorize.go | 5 +++++ coderd/templates.go | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/coderd/authorize.go b/coderd/authorize.go index 69b9cf8c596c9..7423562dc6b13 100644 --- a/coderd/authorize.go +++ b/coderd/authorize.go @@ -12,6 +12,11 @@ import ( "github.com/coder/coder/coderd/rbac" ) +func AuthorizeFilter[O rbac.IsObject](api *api, r *http.Request, action rbac.Action, objects []O) []O { + roles := httpmw.UserRoles(r) + return rbac.Filter(r.Context(), api.Authorizer, roles.ID.String(), roles.Roles, action, objects) +} + func (api *api) Authorize(rw http.ResponseWriter, r *http.Request, action rbac.Action, object rbac.Object) bool { roles := httpmw.UserRoles(r) err := api.Authorizer.ByRoleName(r.Context(), roles.ID.String(), roles.Roles, action, object) diff --git a/coderd/templates.go b/coderd/templates.go index 57eeb1614a618..1ef76dc16473b 100644 --- a/coderd/templates.go +++ b/coderd/templates.go @@ -198,7 +198,7 @@ func (api *api) templatesByOrganization(rw http.ResponseWriter, r *http.Request) } // Filter templates based on rbac permissions - templates = rbac.Filter(r.Context(), api.Authorizer, roles.ID.String(), roles.Roles, rbac.ActionRead, templates) + templates = AuthorizeFilter(api, r, rbac.ActionRead, templates) templateIDs := make([]uuid.UUID, 0, len(templates)) From 561b9812682e9b179043dda12ff35a674c221f0c Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Mon, 23 May 2022 13:36:19 -0500 Subject: [PATCH 3/7] Linting changes --- coderd/rbac/authz_test.go | 1 + coderd/templates.go | 1 - 2 files changed, 1 insertion(+), 1 deletion(-) diff --git a/coderd/rbac/authz_test.go b/coderd/rbac/authz_test.go index 4ceb8f1a687b7..13875110652e8 100644 --- a/coderd/rbac/authz_test.go +++ b/coderd/rbac/authz_test.go @@ -90,6 +90,7 @@ func TestFilter(t *testing.T) { for _, c := range testCases { c := c t.Run(c.Name, func(t *testing.T) { + t.Parallel() authorizer := fakeAuthorizer{ AuthFunc: func(_ context.Context, _ string, _ []string, _ rbac.Action, object rbac.Object) error { return c.Auth(object) diff --git a/coderd/templates.go b/coderd/templates.go index 1ef76dc16473b..e1c799fc1a4d9 100644 --- a/coderd/templates.go +++ b/coderd/templates.go @@ -183,7 +183,6 @@ func (api *api) postTemplateByOrganization(rw http.ResponseWriter, r *http.Reque func (api *api) templatesByOrganization(rw http.ResponseWriter, r *http.Request) { organization := httpmw.OrganizationParam(r) - roles := httpmw.UserRoles(r) templates, err := api.Database.GetTemplatesByOrganization(r.Context(), database.GetTemplatesByOrganizationParams{ OrganizationID: organization.ID, }) From 35879635b04d53510f1fb0db2035a6af1bd329f5 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Mon, 23 May 2022 13:55:38 -0500 Subject: [PATCH 4/7] fix unit tests sharing slice --- coderd/rbac/authz.go | 1 - coderd/rbac/authz_test.go | 27 +++++++++++++++++---------- 2 files changed, 17 insertions(+), 11 deletions(-) diff --git a/coderd/rbac/authz.go b/coderd/rbac/authz.go index 9f10131613e1e..31e7716a0a151 100644 --- a/coderd/rbac/authz.go +++ b/coderd/rbac/authz.go @@ -3,7 +3,6 @@ package rbac import ( "context" _ "embed" - "golang.org/x/xerrors" "github.com/open-policy-agent/opa/rego" diff --git a/coderd/rbac/authz_test.go b/coderd/rbac/authz_test.go index 13875110652e8..e1ab601ee30eb 100644 --- a/coderd/rbac/authz_test.go +++ b/coderd/rbac/authz_test.go @@ -41,6 +41,13 @@ func TestFilter(t *testing.T) { objectList = append(objectList, file) } + // copyList is to prevent tests from sharing the same slice + copyList := func(list []rbac.Object) []rbac.Object { + tmp := make([]rbac.Object, len(list)) + copy(tmp, list) + return tmp + } + testCases := []struct { Name string List []rbac.Object @@ -49,29 +56,29 @@ func TestFilter(t *testing.T) { }{ { Name: "FilterWorkspaceType", - List: objectList, - Expected: workspaceList, + List: copyList(objectList), + Expected: copyList(workspaceList), Auth: func(o rbac.Object) error { if o.Type != rbac.ResourceWorkspace.Type { - return xerrors.New("wrong type") + return xerrors.New("only workspace") } return nil }, }, { Name: "FilterFileType", - List: objectList, - Expected: fileList, + List: copyList(objectList), + Expected: copyList(fileList), Auth: func(o rbac.Object) error { if o.Type != rbac.ResourceFile.Type { - return xerrors.New("wrong type") + return xerrors.New("only file") } return nil }, }, { Name: "FilterAll", - List: objectList, + List: copyList(objectList), Expected: []rbac.Object{}, Auth: func(o rbac.Object) error { return xerrors.New("always fail") @@ -79,8 +86,8 @@ func TestFilter(t *testing.T) { }, { Name: "FilterNone", - List: objectList, - Expected: objectList, + List: copyList(objectList), + Expected: copyList(objectList), Auth: func(o rbac.Object) error { return nil }, @@ -98,7 +105,7 @@ func TestFilter(t *testing.T) { } filtered := rbac.Filter(context.Background(), authorizer, "me", []string{}, rbac.ActionRead, c.List) - require.ElementsMatch(t, filtered, c.Expected, "expect same list") + require.ElementsMatch(t, c.Expected, filtered, "expect same list") require.Equal(t, len(c.Expected), len(filtered), "same length list") }) } From bf74b4c38f242eda8cc0ed77528681cfe16b44cf Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Mon, 23 May 2022 15:55:08 -0500 Subject: [PATCH 5/7] Use authorize filter in more places --- coderd/database/modelmethods.go | 12 +++++++++ coderd/users.go | 26 ++++++------------- coderd/workspaces.go | 45 ++++++++------------------------- 3 files changed, 31 insertions(+), 52 deletions(-) diff --git a/coderd/database/modelmethods.go b/coderd/database/modelmethods.go index a316b839041df..e00628e144d19 100644 --- a/coderd/database/modelmethods.go +++ b/coderd/database/modelmethods.go @@ -5,3 +5,15 @@ import "github.com/coder/coder/coderd/rbac" func (t Template) RBACObject() rbac.Object { return rbac.ResourceTemplate.InOrg(t.OrganizationID).WithID(t.ID.String()) } + +func (w Workspace) RBACObject() rbac.Object { + return rbac.ResourceWorkspace.InOrg(w.OrganizationID).WithID(w.ID.String()).WithOwner(w.OwnerID.String()) +} + +func (m OrganizationMember) RBACObject() rbac.Object { + return rbac.ResourceOrganizationMember.InOrg(m.OrganizationID).WithID(m.UserID.String()) +} + +func (o Organization) RBACObject() rbac.Object { + return rbac.ResourceOrganization.InOrg(o.ID).WithID(o.ID.String()) +} diff --git a/coderd/users.go b/coderd/users.go index 66ffde5202dac..2d1c1b58d1abf 100644 --- a/coderd/users.go +++ b/coderd/users.go @@ -245,7 +245,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(rw, r, rbac.ActionUpdate, rbac.ResourceUser.WithOwner(user.ID.String())) { + if !api.Authorize(rw, r, rbac.ActionUpdate, rbac.ResourceUser.WithID(user.ID.String())) { return } @@ -389,7 +389,6 @@ func (api *api) putUserPassword(rw http.ResponseWriter, r *http.Request) { func (api *api) userRoles(rw http.ResponseWriter, r *http.Request) { user := httpmw.UserParam(r) - roles := httpmw.UserRoles(r) if !api.Authorize(rw, r, rbac.ActionRead, rbac.ResourceUserData. WithOwner(user.ID.String())) { @@ -409,13 +408,10 @@ func (api *api) userRoles(rw http.ResponseWriter, r *http.Request) { return } - for _, mem := range memberships { - err := api.Authorizer.ByRoleName(r.Context(), roles.ID.String(), roles.Roles, rbac.ActionRead, - rbac.ResourceOrganizationMember. - WithID(user.ID.String()). - InOrg(mem.OrganizationID), - ) + // Only include ones we can read from RBAC + memberships = AuthorizeFilter(api, r, rbac.ActionRead, memberships) + for _, mem := range memberships { // If we can read the org member, include the roles if err == nil { resp.OrganizationRoles[mem.OrganizationID] = mem.Roles @@ -508,7 +504,6 @@ func (api *api) updateSiteUserRoles(ctx context.Context, args database.UpdateUse // Returns organizations the parameterized user has access to. func (api *api) organizationsByUser(rw http.ResponseWriter, r *http.Request) { user := httpmw.UserParam(r) - roles := httpmw.UserRoles(r) organizations, err := api.Database.GetOrganizationsByUserID(r.Context(), user.ID) if errors.Is(err, sql.ErrNoRows) { @@ -522,17 +517,12 @@ func (api *api) organizationsByUser(rw http.ResponseWriter, r *http.Request) { return } + // Only return orgs the user can read + organizations = AuthorizeFilter(api, r, rbac.ActionRead, organizations) + publicOrganizations := make([]codersdk.Organization, 0, len(organizations)) for _, organization := range organizations { - err := api.Authorizer.ByRoleName(r.Context(), roles.ID.String(), roles.Roles, rbac.ActionRead, - rbac.ResourceOrganization. - WithID(organization.ID.String()). - InOrg(organization.ID), - ) - if err == nil { - // Only return orgs the user can read - publicOrganizations = append(publicOrganizations, convertOrganization(organization)) - } + publicOrganizations = append(publicOrganizations, convertOrganization(organization)) } httpapi.Write(rw, http.StatusOK, publicOrganizations) diff --git a/coderd/workspaces.go b/coderd/workspaces.go index b9e7be07da96e..297964d6b91e0 100644 --- a/coderd/workspaces.go +++ b/coderd/workspaces.go @@ -108,7 +108,6 @@ func (api *api) workspace(rw http.ResponseWriter, r *http.Request) { func (api *api) workspacesByOrganization(rw http.ResponseWriter, r *http.Request) { organization := httpmw.OrganizationParam(r) - roles := httpmw.UserRoles(r) workspaces, err := api.Database.GetWorkspacesWithFilter(r.Context(), database.GetWorkspacesWithFilterParams{ OrganizationID: organization.ID, Deleted: false, @@ -123,17 +122,10 @@ func (api *api) workspacesByOrganization(rw http.ResponseWriter, r *http.Request return } - allowedWorkspaces := make([]database.Workspace, 0) - for _, ws := range workspaces { - ws := ws - err = api.Authorizer.ByRoleName(r.Context(), roles.ID.String(), roles.Roles, rbac.ActionRead, - rbac.ResourceWorkspace.InOrg(ws.OrganizationID).WithOwner(ws.OwnerID.String()).WithID(ws.ID.String())) - if err == nil { - allowedWorkspaces = append(allowedWorkspaces, ws) - } - } + // Rbac filter + workspaces = AuthorizeFilter(api, r, rbac.ActionRead, workspaces) - apiWorkspaces, err := convertWorkspaces(r.Context(), api.Database, allowedWorkspaces) + apiWorkspaces, err := convertWorkspaces(r.Context(), api.Database, workspaces) if err != nil { httpapi.Write(rw, http.StatusInternalServerError, httpapi.Response{ Message: fmt.Sprintf("convert workspaces: %s", err), @@ -146,7 +138,6 @@ func (api *api) workspacesByOrganization(rw http.ResponseWriter, r *http.Request // workspaces returns all workspaces a user can read. // Optional filters with query params func (api *api) workspaces(rw http.ResponseWriter, r *http.Request) { - roles := httpmw.UserRoles(r) apiKey := httpmw.APIKey(r) // Empty strings mean no filter @@ -186,24 +177,18 @@ func (api *api) workspaces(rw http.ResponseWriter, r *http.Request) { filter.OwnerID = userID } - allowedWorkspaces := make([]database.Workspace, 0) - allWorkspaces, err := api.Database.GetWorkspacesWithFilter(r.Context(), filter) + workspaces, err := api.Database.GetWorkspacesWithFilter(r.Context(), filter) if err != nil { httpapi.Write(rw, http.StatusInternalServerError, httpapi.Response{ Message: fmt.Sprintf("get workspaces for user: %s", err), }) return } - for _, ws := range allWorkspaces { - ws := ws - err = api.Authorizer.ByRoleName(r.Context(), roles.ID.String(), roles.Roles, rbac.ActionRead, - rbac.ResourceWorkspace.InOrg(ws.OrganizationID).WithOwner(ws.OwnerID.String()).WithID(ws.ID.String())) - if err == nil { - allowedWorkspaces = append(allowedWorkspaces, ws) - } - } - apiWorkspaces, err := convertWorkspaces(r.Context(), api.Database, allowedWorkspaces) + // Only return workspaces the user can read + workspaces = AuthorizeFilter(api, r, rbac.ActionRead, workspaces) + + apiWorkspaces, err := convertWorkspaces(r.Context(), api.Database, workspaces) if err != nil { httpapi.Write(rw, http.StatusInternalServerError, httpapi.Response{ Message: fmt.Sprintf("convert workspaces: %s", err), @@ -215,7 +200,6 @@ func (api *api) workspaces(rw http.ResponseWriter, r *http.Request) { func (api *api) workspacesByOwner(rw http.ResponseWriter, r *http.Request) { owner := httpmw.UserParam(r) - roles := httpmw.UserRoles(r) workspaces, err := api.Database.GetWorkspacesWithFilter(r.Context(), database.GetWorkspacesWithFilterParams{ OwnerID: owner.ID, Deleted: false, @@ -230,17 +214,10 @@ func (api *api) workspacesByOwner(rw http.ResponseWriter, r *http.Request) { return } - allowedWorkspaces := make([]database.Workspace, 0) - for _, ws := range workspaces { - ws := ws - err = api.Authorizer.ByRoleName(r.Context(), roles.ID.String(), roles.Roles, rbac.ActionRead, - rbac.ResourceWorkspace.InOrg(ws.OrganizationID).WithOwner(ws.OwnerID.String()).WithID(ws.ID.String())) - if err == nil { - allowedWorkspaces = append(allowedWorkspaces, ws) - } - } + // Only return workspaces the user can read + workspaces = AuthorizeFilter(api, r, rbac.ActionRead, workspaces) - apiWorkspaces, err := convertWorkspaces(r.Context(), api.Database, allowedWorkspaces) + apiWorkspaces, err := convertWorkspaces(r.Context(), api.Database, workspaces) if err != nil { httpapi.Write(rw, http.StatusInternalServerError, httpapi.Response{ Message: fmt.Sprintf("convert workspaces: %s", err), From e3a63ad14b87550df93e67381a8d2d7d5c205689 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Mon, 23 May 2022 16:36:10 -0500 Subject: [PATCH 6/7] RBAC template endpoints - Use new authorize filter on other endpoints - Pass in objs vs making RBACObjects --- coderd/authorize.go | 4 ++-- coderd/coderd.go | 1 + coderd/coderd_test.go | 17 ++++++++++++++--- coderd/templates.go | 18 +++++++++++++++++- coderd/workspaces.go | 11 ++--------- 5 files changed, 36 insertions(+), 15 deletions(-) diff --git a/coderd/authorize.go b/coderd/authorize.go index 7423562dc6b13..e47688100e595 100644 --- a/coderd/authorize.go +++ b/coderd/authorize.go @@ -17,9 +17,9 @@ func AuthorizeFilter[O rbac.IsObject](api *api, r *http.Request, action rbac.Act return rbac.Filter(r.Context(), api.Authorizer, roles.ID.String(), roles.Roles, action, objects) } -func (api *api) Authorize(rw http.ResponseWriter, r *http.Request, action rbac.Action, object rbac.Object) bool { +func (api *api) Authorize(rw http.ResponseWriter, r *http.Request, action rbac.Action, object rbac.IsObject) bool { roles := httpmw.UserRoles(r) - err := api.Authorizer.ByRoleName(r.Context(), roles.ID.String(), roles.Roles, action, object) + err := api.Authorizer.ByRoleName(r.Context(), roles.ID.String(), roles.Roles, action, object.RBACObject()) if err != nil { httpapi.Write(rw, http.StatusForbidden, httpapi.Response{ Message: err.Error(), diff --git a/coderd/coderd.go b/coderd/coderd.go index 343da27bf55cf..56aaa8f79544c 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -185,6 +185,7 @@ func newRouter(options *Options, a *api) chi.Router { r.Route("/templates/{template}", func(r chi.Router) { r.Use( apiKeyMiddleware, + authRolesMiddleware, httpmw.ExtractTemplateParam(options.Database), ) diff --git a/coderd/coderd_test.go b/coderd/coderd_test.go index 4eaf6c32e36d6..eeee7a93fcac9 100644 --- a/coderd/coderd_test.go +++ b/coderd/coderd_test.go @@ -96,7 +96,6 @@ func TestAuthorizeAllEndpoints(t *testing.T) { "PUT:/api/v2/organizations/{organization}/members/{user}/roles": {NoAuthorize: true}, "GET:/api/v2/organizations/{organization}/provisionerdaemons": {NoAuthorize: true}, - "POST:/api/v2/organizations/{organization}/templates": {NoAuthorize: true}, "GET:/api/v2/organizations/{organization}/templates/{templatename}": {NoAuthorize: true}, "POST:/api/v2/organizations/{organization}/templateversions": {NoAuthorize: true}, "POST:/api/v2/organizations/{organization}/workspaces": {NoAuthorize: true}, @@ -105,8 +104,6 @@ func TestAuthorizeAllEndpoints(t *testing.T) { "GET:/api/v2/parameters/{scope}/{id}": {NoAuthorize: true}, "DELETE:/api/v2/parameters/{scope}/{id}/{name}": {NoAuthorize: true}, - "DELETE:/api/v2/templates/{template}": {NoAuthorize: true}, - "GET:/api/v2/templates/{template}": {NoAuthorize: true}, "GET:/api/v2/templates/{template}/versions": {NoAuthorize: true}, "PATCH:/api/v2/templates/{template}/versions": {NoAuthorize: true}, "GET:/api/v2/templates/{template}/versions/{templateversionname}": {NoAuthorize: true}, @@ -187,6 +184,19 @@ func TestAuthorizeAllEndpoints(t *testing.T) { AssertAction: rbac.ActionRead, AssertObject: rbac.ResourceTemplate.InOrg(template.OrganizationID).WithID(template.ID.String()), }, + "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()), + }, + "GET:/api/v2/templates/{template}": { + AssertAction: rbac.ActionRead, + AssertObject: rbac.ResourceTemplate.InOrg(template.OrganizationID).WithID(template.ID.String()), + }, // These endpoints need payloads to get to the auth part. Payloads will be required "PUT:/api/v2/users/{user}/roles": {StatusCode: http.StatusBadRequest, NoAuthorize: true}, @@ -224,6 +234,7 @@ func TestAuthorizeAllEndpoints(t *testing.T) { route = strings.ReplaceAll(route, "{workspacebuild}", workspace.LatestBuild.ID.String()) route = strings.ReplaceAll(route, "{workspacename}", workspace.Name) route = strings.ReplaceAll(route, "{workspacebuildname}", workspace.LatestBuild.Name) + route = strings.ReplaceAll(route, "{template}", template.ID.String()) resp, err := client.Request(context.Background(), method, route, nil) require.NoError(t, err, "do req") diff --git a/coderd/templates.go b/coderd/templates.go index e1c799fc1a4d9..44850f817f623 100644 --- a/coderd/templates.go +++ b/coderd/templates.go @@ -31,6 +31,11 @@ func (api *api) template(rw http.ResponseWriter, r *http.Request) { }) return } + + if !api.Authorize(rw, r, rbac.ActionRead, template) { + return + } + count := uint32(0) if len(workspaceCounts) > 0 { count = uint32(workspaceCounts[0].Count) @@ -41,6 +46,9 @@ func (api *api) template(rw http.ResponseWriter, r *http.Request) { func (api *api) deleteTemplate(rw http.ResponseWriter, r *http.Request) { template := httpmw.TemplateParam(r) + if !api.Authorize(rw, r, rbac.ActionDelete, template) { + return + } workspaces, err := api.Database.GetWorkspacesByTemplateID(r.Context(), database.GetWorkspacesByTemplateIDParams{ TemplateID: template.ID, @@ -78,10 +86,14 @@ func (api *api) deleteTemplate(rw http.ResponseWriter, r *http.Request) { // Create a new template in an organization. func (api *api) postTemplateByOrganization(rw http.ResponseWriter, r *http.Request) { var createTemplate codersdk.CreateTemplateRequest + organization := httpmw.OrganizationParam(r) + if !api.Authorize(rw, r, rbac.ActionCreate, rbac.ResourceTemplate.InOrg(organization.ID)) { + return + } + if !httpapi.Read(rw, r, &createTemplate) { return } - organization := httpmw.OrganizationParam(r) _, err := api.Database.GetTemplateByOrganizationAndName(r.Context(), database.GetTemplateByOrganizationAndNameParams{ OrganizationID: organization.ID, Name: createTemplate.Name, @@ -239,6 +251,10 @@ func (api *api) templateByOrganizationAndName(rw http.ResponseWriter, r *http.Re return } + if !api.Authorize(rw, r, rbac.ActionRead, template) { + return + } + workspaceCounts, err := api.Database.GetWorkspaceOwnerCountsByTemplateIDs(r.Context(), []uuid.UUID{template.ID}) if errors.Is(err, sql.ErrNoRows) { err = nil diff --git a/coderd/workspaces.go b/coderd/workspaces.go index 297964d6b91e0..92e774bd24a8e 100644 --- a/coderd/workspaces.go +++ b/coderd/workspaces.go @@ -30,13 +30,7 @@ import ( func (api *api) workspace(rw http.ResponseWriter, r *http.Request) { workspace := httpmw.WorkspaceParam(r) - if !api.Authorize(rw, r, rbac.ActionRead, - rbac.ResourceWorkspace.InOrg(workspace.OrganizationID).WithOwner(workspace.OwnerID.String()).WithID(workspace.ID.String())) { - return - } - - if !api.Authorize(rw, r, rbac.ActionRead, - rbac.ResourceWorkspace.InOrg(workspace.OrganizationID).WithOwner(workspace.OwnerID.String()).WithID(workspace.ID.String())) { + if !api.Authorize(rw, r, rbac.ActionRead, workspace) { return } @@ -255,8 +249,7 @@ func (api *api) workspaceByOwnerAndName(rw http.ResponseWriter, r *http.Request) return } - if !api.Authorize(rw, r, rbac.ActionRead, - rbac.ResourceWorkspace.InOrg(workspace.OrganizationID).WithOwner(workspace.OwnerID.String()).WithID(workspace.ID.String())) { + if !api.Authorize(rw, r, rbac.ActionRead, workspace) { return } From f401432a776628d0de0f664a728e77eb1cbc3876 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 24 May 2022 08:28:45 -0500 Subject: [PATCH 7/7] Allocate new slice for RBAC filter instead --- coderd/authorize.go | 4 ++-- coderd/rbac/authz.go | 10 +++++----- coderd/rbac/object.go | 4 ++-- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/coderd/authorize.go b/coderd/authorize.go index e47688100e595..13374344fae1f 100644 --- a/coderd/authorize.go +++ b/coderd/authorize.go @@ -12,12 +12,12 @@ import ( "github.com/coder/coder/coderd/rbac" ) -func AuthorizeFilter[O rbac.IsObject](api *api, r *http.Request, action rbac.Action, objects []O) []O { +func AuthorizeFilter[O rbac.Objecter](api *api, r *http.Request, action rbac.Action, objects []O) []O { roles := httpmw.UserRoles(r) return rbac.Filter(r.Context(), api.Authorizer, roles.ID.String(), roles.Roles, action, objects) } -func (api *api) Authorize(rw http.ResponseWriter, r *http.Request, action rbac.Action, object rbac.IsObject) bool { +func (api *api) Authorize(rw http.ResponseWriter, r *http.Request, action rbac.Action, object rbac.Objecter) bool { roles := httpmw.UserRoles(r) err := api.Authorizer.ByRoleName(r.Context(), roles.ID.String(), roles.Roles, action, object.RBACObject()) if err != nil { diff --git a/coderd/rbac/authz.go b/coderd/rbac/authz.go index 31e7716a0a151..5e566496093b2 100644 --- a/coderd/rbac/authz.go +++ b/coderd/rbac/authz.go @@ -17,17 +17,17 @@ type Authorizer interface { // Filter does not allocate a new slice, and will use the existing one // passed in. This can cause memory leaks if the slice is held for a prolonged // period of time. -func Filter[O IsObject](ctx context.Context, auth Authorizer, subjID string, subjRoles []string, action Action, objects []O) []O { - var currentIdx int +func Filter[O Objecter](ctx context.Context, auth Authorizer, subjID string, subjRoles []string, action Action, objects []O) []O { + filtered := make([]O, 0) + for i := range objects { object := objects[i] err := auth.ByRoleName(ctx, subjID, subjRoles, action, object.RBACObject()) if err == nil { - objects[currentIdx] = objects[i] - currentIdx++ + filtered = append(filtered, object) } } - return objects[:currentIdx] + return filtered } // RegoAuthorizer will use a prepared rego query for performing authorize() diff --git a/coderd/rbac/object.go b/coderd/rbac/object.go index 3ce0ac9d39267..c40e0e9aba394 100644 --- a/coderd/rbac/object.go +++ b/coderd/rbac/object.go @@ -6,8 +6,8 @@ import ( const WildcardSymbol = "*" -// IsObject returns the RBAC object for itself. -type IsObject interface { +// Objecter returns the RBAC object for itself. +type Objecter interface { RBACObject() Object }