From bed0f8f120ea094776f4a4651ea5e5bb30f86e62 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 11 May 2022 16:46:14 -0500 Subject: [PATCH 01/26] feat: Enforce authorize call on all endpoints - Make 'request()' exported for running custom requests --- coderd/coderd.go | 18 +++++--- coderd/coderd_test.go | 78 ++++++++++++++++++++++++++++++++- coderd/coderdtest/coderdtest.go | 17 +++++-- coderd/httpmw/authorize.go | 67 +++++++++++++++++++++++----- coderd/httpmw/oauth2.go | 4 +- coderd/rbac/authz.go | 4 ++ coderd/rbac/object.go | 16 +++++++ codersdk/buildinfo.go | 2 +- codersdk/client.go | 4 +- codersdk/files.go | 4 +- codersdk/gitsshkey.go | 6 +-- codersdk/organizations.go | 20 ++++----- codersdk/pagination.go | 2 +- codersdk/parameters.go | 6 +-- codersdk/provisionerdaemons.go | 4 +- codersdk/roles.go | 4 +- codersdk/templates.go | 10 ++--- codersdk/templateversions.go | 10 ++--- codersdk/users.go | 38 ++++++++-------- codersdk/workspaceagents.go | 14 +++--- codersdk/workspacebuilds.go | 8 ++-- codersdk/workspaceresources.go | 2 +- codersdk/workspaces.go | 12 ++--- 23 files changed, 254 insertions(+), 96 deletions(-) diff --git a/coderd/coderd.go b/coderd/coderd.go index 67e7b0eaeae3f..ac01dfecfd12f 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -50,7 +50,7 @@ type Options struct { SecureAuthCookie bool SSHKeygenAlgorithm gitsshkey.Algorithm TURNServer *turnconn.Server - Authorizer *rbac.RegoAuthorizer + Authorizer rbac.Authorizer } // New constructs the Coder API into an HTTP handler. @@ -127,9 +127,11 @@ func New(options *Options) (http.Handler, func()) { r.Route("/files", func(r chi.Router) { r.Use( apiKeyMiddleware, + authRolesMiddleware, // This number is arbitrary, but reading/writing // file content is expensive so it should be small. httpmw.RateLimitPerMinute(12), + httpmw.WithRBACObject(rbac.ResourceTypeFile), ) r.Get("/{hash}", api.fileByHash) r.Post("/", api.postFile) @@ -137,8 +139,8 @@ func New(options *Options) (http.Handler, func()) { r.Route("/organizations/{organization}", func(r chi.Router) { r.Use( apiKeyMiddleware, - httpmw.ExtractOrganizationParam(options.Database), authRolesMiddleware, + httpmw.ExtractOrganizationParam(options.Database), ) r.Get("/", api.organization) r.Get("/provisionerdaemons", api.provisionerDaemonsByOrganization) @@ -149,12 +151,16 @@ func New(options *Options) (http.Handler, func()) { r.Get("/{templatename}", api.templateByOrganizationAndName) }) r.Route("/workspaces", func(r chi.Router) { - r.Post("/", api.postWorkspacesByOrganization) - r.Get("/", api.workspacesByOrganization) + r.Use(httpmw.WithRBACObject(rbac.ResourceWorkspace)) + // Posting a workspace is inherently owned by the api key creating it. + r.With(httpmw.WithAPIKeyAsOwner()). + Post("/", authorize(api.postWorkspacesByOrganization, rbac.ActionCreate)) + r.Get("/", authorize(api.workspacesByOrganization, rbac.ActionRead)) r.Route("/{user}", func(r chi.Router) { r.Use(httpmw.ExtractUserParam(options.Database)) - r.Get("/{workspace}", api.workspaceByOwnerAndName) - r.Get("/", api.workspacesByOwner) + // TODO: @emyrk add the resource id to this authorize. + r.Get("/{workspace}", authorize(api.workspaceByOwnerAndName, rbac.ActionRead)) + r.Get("/", authorize(api.workspacesByOwner, rbac.ActionRead)) }) }) r.Route("/members", func(r chi.Router) { diff --git a/coderd/coderd_test.go b/coderd/coderd_test.go index 73d3c3d308def..25231d34fc2d5 100644 --- a/coderd/coderd_test.go +++ b/coderd/coderd_test.go @@ -2,14 +2,17 @@ package coderd_test import ( "context" + "net/http" "testing" - "go.uber.org/goleak" - + "github.com/go-chi/chi/v5" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "go.uber.org/goleak" "github.com/coder/coder/buildinfo" "github.com/coder/coder/coderd/coderdtest" + "github.com/coder/coder/coderd/rbac" ) func TestMain(m *testing.M) { @@ -24,3 +27,74 @@ func TestBuildInfo(t *testing.T) { require.Equal(t, buildinfo.ExternalURL(), buildInfo.ExternalURL, "external URL") require.Equal(t, buildinfo.Version(), buildInfo.Version, "version") } + +// TestAuthorizeAllEndpoints will check `authorize` is called on every endpoint registered. +func TestAuthorizeAllEndpoints(t *testing.T) { + t.Parallel() + + // skipRoutes allows skipping routes from being checked. + type routeCheck struct { + NoAuthorize bool + } + assertRoute := map[string]routeCheck{ + "GET:/api/v2": {NoAuthorize: true}, + "GET:/api/v2/buildinfo": {NoAuthorize: true}, + } + + authorizer := &fakeAuthorizer{} + srv, client := coderdtest.NewMemoryCoderd(t, &coderdtest.Options{ + Authorizer: authorizer, + }) + admin := coderdtest.CreateFirstUser(t, client) + var _ = admin + + c := srv.Config.Handler.(*chi.Mux) + err := chi.Walk(c, func(method string, route string, handler http.Handler, middlewares ...func(http.Handler) http.Handler) error { + name := method + ":" + route + t.Run(name, func(t *testing.T) { + authorizer.reset() + routeAssertions, ok := assertRoute[name] + if !ok { + // By default, all omitted routes check for just "authorize" called + routeAssertions = routeCheck{} + } + + resp, err := client.Request(context.Background(), method, route, nil) + require.NoError(t, err, "do req") + _ = resp.Body.Close() + + if !routeAssertions.NoAuthorize { + assert.NotNil(t, authorizer.Called, "authorizer expected") + } else { + assert.Nil(t, authorizer.Called, "authorize not expected") + } + }) + return nil + }) + require.NoError(t, err) +} + +type authCall struct { + SubjectID string + Roles []string + Action rbac.Action + Object rbac.Object +} + +type fakeAuthorizer struct { + Called *authCall +} + +func (f *fakeAuthorizer) AuthorizeByRoleName(ctx context.Context, subjectID string, roleNames []string, action rbac.Action, object rbac.Object) error { + f.Called = &authCall{ + SubjectID: subjectID, + Roles: roleNames, + Action: action, + Object: object, + } + return nil +} + +func (f *fakeAuthorizer) reset() { + f.Called = nil +} diff --git a/coderd/coderdtest/coderdtest.go b/coderd/coderdtest/coderdtest.go index a77f6bde5575a..1a4c8b4e7e627 100644 --- a/coderd/coderdtest/coderdtest.go +++ b/coderd/coderdtest/coderdtest.go @@ -24,6 +24,8 @@ import ( "testing" "time" + "github.com/coder/coder/coderd/rbac" + "cloud.google.com/go/compute/metadata" "github.com/fullsailor/pkcs7" "github.com/golang-jwt/jwt" @@ -57,11 +59,10 @@ type Options struct { GoogleTokenValidator *idtoken.Validator SSHKeygenAlgorithm gitsshkey.Algorithm APIRateLimit int + Authorizer rbac.Authorizer } -// New constructs an in-memory coderd instance and returns -// the connected client. -func New(t *testing.T, options *Options) *codersdk.Client { +func NewMemoryCoderd(t *testing.T, options *Options) (*httptest.Server, *codersdk.Client) { if options == nil { options = &Options{} } @@ -129,6 +130,7 @@ func New(t *testing.T, options *Options) *codersdk.Client { SSHKeygenAlgorithm: options.SSHKeygenAlgorithm, TURNServer: turnServer, APIRateLimit: options.APIRateLimit, + Authorizer: options.Authorizer, }) t.Cleanup(func() { cancelFunc() @@ -137,7 +139,14 @@ func New(t *testing.T, options *Options) *codersdk.Client { closeWait() }) - return codersdk.New(serverURL) + return srv, codersdk.New(serverURL) +} + +// New constructs an in-memory coderd instance and returns +// the connected client. +func New(t *testing.T, options *Options) *codersdk.Client { + _, cli := NewMemoryCoderd(t, options) + return cli } // NewProvisionerDaemon launches a provisionerd instance configured to work diff --git a/coderd/httpmw/authorize.go b/coderd/httpmw/authorize.go index 2eb221f1893eb..6bca6f23c1159 100644 --- a/coderd/httpmw/authorize.go +++ b/coderd/httpmw/authorize.go @@ -4,6 +4,8 @@ import ( "context" "net/http" + "github.com/google/uuid" + "golang.org/x/xerrors" "cdr.dev/slog" @@ -15,11 +17,12 @@ import ( // Authorize will enforce if the user roles can complete the action on the AuthObject. // The organization and owner are found using the ExtractOrganization and // ExtractUser middleware if present. -func Authorize(logger slog.Logger, auth *rbac.RegoAuthorizer, action rbac.Action) func(http.Handler) http.Handler { +func Authorize(logger slog.Logger, auth rbac.Authorizer, action rbac.Action) func(http.Handler) http.Handler { return func(next http.Handler) http.Handler { return http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { roles := UserRoles(r) - object := rbacObject(r) + authObject := rbacObject(r) + object := authObject.Object if object.Type == "" { panic("developer error: auth object has no type") @@ -34,12 +37,18 @@ func Authorize(logger slog.Logger, auth *rbac.RegoAuthorizer, action rbac.Action object = object.InOrg(organization.ID) } - unknownOwner := r.Context().Value(userParamContextKey{}) - if owner, castOK := unknownOwner.(database.User); unknownOwner != nil { - if !castOK { - panic("developer error: user param middleware not provided for authorize") + if authObject.WithOwner != nil { + owner := authObject.WithOwner(r) + object = object.WithOwner(owner.String()) + } else { + // Attempt to find the resource owner id + unknownOwner := r.Context().Value(userParamContextKey{}) + if owner, castOK := unknownOwner.(database.User); unknownOwner != nil { + if !castOK { + panic("developer error: user param middleware not provided for authorize") + } + object = object.WithOwner(owner.ID.String()) } - object = object.WithOwner(owner.ID.String()) } err := auth.AuthorizeByRoleName(r.Context(), roles.ID.String(), roles.Roles, action, object) @@ -70,21 +79,59 @@ func Authorize(logger slog.Logger, auth *rbac.RegoAuthorizer, action rbac.Action type authObjectKey struct{} +type AuthObject struct { + Object rbac.Object + + WithOwner func(r *http.Request) uuid.UUID +} + // APIKey returns the API key from the ExtractAPIKey handler. -func rbacObject(r *http.Request) rbac.Object { - obj, ok := r.Context().Value(authObjectKey{}).(rbac.Object) +func rbacObject(r *http.Request) AuthObject { + obj, ok := r.Context().Value(authObjectKey{}).(AuthObject) if !ok { panic("developer error: auth object middleware not provided") } return obj } +func WithAPIKeyAsOwner() func(http.Handler) http.Handler { + return WithOwner(func(r *http.Request) uuid.UUID { + key := APIKey(r) + return key.UserID + }) +} + +// WithOwner sets the object owner for 'Authorize()' for all routes handled +// by this middleware. +func WithOwner(withOwner func(r *http.Request) uuid.UUID) func(http.Handler) http.Handler { + return func(next http.Handler) http.Handler { + return http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { + obj, ok := r.Context().Value(authObjectKey{}).(AuthObject) + if ok { + obj.WithOwner = withOwner + } else { + obj = AuthObject{WithOwner: withOwner} + } + + ctx := context.WithValue(r.Context(), authObjectKey{}, obj) + next.ServeHTTP(rw, r.WithContext(ctx)) + }) + } +} + // WithRBACObject sets the object for 'Authorize()' for all routes handled // by this middleware. The important field to set is 'Type' func WithRBACObject(object rbac.Object) func(http.Handler) http.Handler { return func(next http.Handler) http.Handler { return http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { - ctx := context.WithValue(r.Context(), authObjectKey{}, object) + obj, ok := r.Context().Value(authObjectKey{}).(AuthObject) + if ok { + obj.Object = object + } else { + obj = AuthObject{Object: object} + } + + ctx := context.WithValue(r.Context(), authObjectKey{}, obj) next.ServeHTTP(rw, r.WithContext(ctx)) }) } diff --git a/coderd/httpmw/oauth2.go b/coderd/httpmw/oauth2.go index ba14f8dafdfc0..92ba5da38162b 100644 --- a/coderd/httpmw/oauth2.go +++ b/coderd/httpmw/oauth2.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "net/http" + "reflect" "golang.org/x/oauth2" @@ -46,7 +47,8 @@ func OAuth2(r *http.Request) OAuth2State { func ExtractOAuth2(config OAuth2Config) func(http.Handler) http.Handler { return func(next http.Handler) http.Handler { return http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { - if config == nil { + // Interfaces can hold a nil value + if config == nil || reflect.ValueOf(config).IsNil() { httpapi.Write(rw, http.StatusPreconditionRequired, httpapi.Response{ Message: "The oauth2 method requested is not configured!", }) diff --git a/coderd/rbac/authz.go b/coderd/rbac/authz.go index 39cd7ed102906..938989a5b71b7 100644 --- a/coderd/rbac/authz.go +++ b/coderd/rbac/authz.go @@ -9,6 +9,10 @@ import ( "github.com/open-policy-agent/opa/rego" ) +type Authorizer interface { + AuthorizeByRoleName(ctx context.Context, subjectID string, roleNames []string, action Action, object Object) error +} + // RegoAuthorizer will use a prepared rego query for performing authorize() type RegoAuthorizer struct { query rego.PreparedEvalQuery diff --git a/coderd/rbac/object.go b/coderd/rbac/object.go index dd12efc25fcb2..3888ade2ba618 100644 --- a/coderd/rbac/object.go +++ b/coderd/rbac/object.go @@ -9,6 +9,10 @@ const WildcardSymbol = "*" // Resources are just typed objects. Making resources this way allows directly // passing them into an Authorize function and use the chaining api. var ( + // ResourceWorkspace CRUD. Org + User owner + // create/delete = make or delete workspaces + // read = access workspace + // update = edit workspace variables ResourceWorkspace = Object{ Type: "workspace", } @@ -17,6 +21,18 @@ var ( Type: "template", } + ResourceTypeFile = Object{ + Type: "file", + } + + // ResourceOrganization CRUD. Org owner + // create/delete = make or delete organizations + // read = view org information + // update = ?? + ResourceOrganization = Object{ + Type: "organization", + } + // ResourceUserRole might be expanded later to allow more granular permissions // to modifying roles. For now, this covers all possible roles, so having this permission // allows granting/deleting **ALL** roles. diff --git a/codersdk/buildinfo.go b/codersdk/buildinfo.go index a3aecc1ffdfed..0233047caf98c 100644 --- a/codersdk/buildinfo.go +++ b/codersdk/buildinfo.go @@ -18,7 +18,7 @@ type BuildInfoResponse struct { // BuildInfo returns build information for this instance of Coder. func (c *Client) BuildInfo(ctx context.Context) (BuildInfoResponse, error) { - res, err := c.request(ctx, http.MethodGet, "/api/v2/buildinfo", nil) + res, err := c.Request(ctx, http.MethodGet, "/api/v2/buildinfo", nil) if err != nil { return BuildInfoResponse{}, err } diff --git a/codersdk/client.go b/codersdk/client.go index 1654c141e6827..48571adff5d0b 100644 --- a/codersdk/client.go +++ b/codersdk/client.go @@ -35,9 +35,9 @@ type Client struct { type requestOption func(*http.Request) -// request performs an HTTP request with the body provided. +// Request performs an HTTP request with the body provided. // The caller is responsible for closing the response body. -func (c *Client) request(ctx context.Context, method, path string, body interface{}, opts ...requestOption) (*http.Response, error) { +func (c *Client) Request(ctx context.Context, method, path string, body interface{}, opts ...requestOption) (*http.Response, error) { serverURL, err := c.URL.Parse(path) if err != nil { return nil, xerrors.Errorf("parse url: %w", err) diff --git a/codersdk/files.go b/codersdk/files.go index 15c30f30f87f7..52fcf0215081b 100644 --- a/codersdk/files.go +++ b/codersdk/files.go @@ -20,7 +20,7 @@ type UploadResponse struct { // Upload uploads an arbitrary file with the content type provided. // This is used to upload a source-code archive. func (c *Client) Upload(ctx context.Context, contentType string, content []byte) (UploadResponse, error) { - res, err := c.request(ctx, http.MethodPost, "/api/v2/files", content, func(r *http.Request) { + res, err := c.Request(ctx, http.MethodPost, "/api/v2/files", content, func(r *http.Request) { r.Header.Set("Content-Type", contentType) }) if err != nil { @@ -36,7 +36,7 @@ func (c *Client) Upload(ctx context.Context, contentType string, content []byte) // Download fetches a file by uploaded hash. func (c *Client) Download(ctx context.Context, hash string) ([]byte, string, error) { - res, err := c.request(ctx, http.MethodGet, fmt.Sprintf("/api/v2/files/%s", hash), nil) + res, err := c.Request(ctx, http.MethodGet, fmt.Sprintf("/api/v2/files/%s", hash), nil) if err != nil { return nil, "", err } diff --git a/codersdk/gitsshkey.go b/codersdk/gitsshkey.go index 3cc4333e735e3..c71ddd85ba630 100644 --- a/codersdk/gitsshkey.go +++ b/codersdk/gitsshkey.go @@ -25,7 +25,7 @@ type AgentGitSSHKey struct { // GitSSHKey returns the user's git SSH public key. func (c *Client) GitSSHKey(ctx context.Context, userID uuid.UUID) (GitSSHKey, error) { - res, err := c.request(ctx, http.MethodGet, fmt.Sprintf("/api/v2/users/%s/gitsshkey", uuidOrMe(userID)), nil) + res, err := c.Request(ctx, http.MethodGet, fmt.Sprintf("/api/v2/users/%s/gitsshkey", uuidOrMe(userID)), nil) if err != nil { return GitSSHKey{}, xerrors.Errorf("execute request: %w", err) } @@ -41,7 +41,7 @@ func (c *Client) GitSSHKey(ctx context.Context, userID uuid.UUID) (GitSSHKey, er // RegenerateGitSSHKey will create a new SSH key pair for the user and return it. func (c *Client) RegenerateGitSSHKey(ctx context.Context, userID uuid.UUID) (GitSSHKey, error) { - res, err := c.request(ctx, http.MethodPut, fmt.Sprintf("/api/v2/users/%s/gitsshkey", uuidOrMe(userID)), nil) + res, err := c.Request(ctx, http.MethodPut, fmt.Sprintf("/api/v2/users/%s/gitsshkey", uuidOrMe(userID)), nil) if err != nil { return GitSSHKey{}, xerrors.Errorf("execute request: %w", err) } @@ -57,7 +57,7 @@ func (c *Client) RegenerateGitSSHKey(ctx context.Context, userID uuid.UUID) (Git // AgentGitSSHKey will return the user's SSH key pair for the workspace. func (c *Client) AgentGitSSHKey(ctx context.Context) (AgentGitSSHKey, error) { - res, err := c.request(ctx, http.MethodGet, "/api/v2/workspaceagents/me/gitsshkey", nil) + res, err := c.Request(ctx, http.MethodGet, "/api/v2/workspaceagents/me/gitsshkey", nil) if err != nil { return AgentGitSSHKey{}, xerrors.Errorf("execute request: %w", err) } diff --git a/codersdk/organizations.go b/codersdk/organizations.go index ce0690c466203..8ebc4cd8ae16b 100644 --- a/codersdk/organizations.go +++ b/codersdk/organizations.go @@ -58,7 +58,7 @@ type CreateWorkspaceRequest struct { } func (c *Client) Organization(ctx context.Context, id uuid.UUID) (Organization, error) { - res, err := c.request(ctx, http.MethodGet, fmt.Sprintf("/api/v2/organizations/%s", id.String()), nil) + res, err := c.Request(ctx, http.MethodGet, fmt.Sprintf("/api/v2/organizations/%s", id.String()), nil) if err != nil { return Organization{}, xerrors.Errorf("execute request: %w", err) } @@ -74,7 +74,7 @@ func (c *Client) Organization(ctx context.Context, id uuid.UUID) (Organization, // ProvisionerDaemonsByOrganization returns provisioner daemons available for an organization. func (c *Client) ProvisionerDaemonsByOrganization(ctx context.Context, organizationID uuid.UUID) ([]ProvisionerDaemon, error) { - res, err := c.request(ctx, http.MethodGet, + res, err := c.Request(ctx, http.MethodGet, fmt.Sprintf("/api/v2/organizations/%s/provisionerdaemons", organizationID.String()), nil, ) @@ -94,7 +94,7 @@ func (c *Client) ProvisionerDaemonsByOrganization(ctx context.Context, organizat // CreateTemplateVersion processes source-code and optionally associates the version with a template. // Executing without a template is useful for validating source-code. func (c *Client) CreateTemplateVersion(ctx context.Context, organizationID uuid.UUID, req CreateTemplateVersionRequest) (TemplateVersion, error) { - res, err := c.request(ctx, http.MethodPost, + res, err := c.Request(ctx, http.MethodPost, fmt.Sprintf("/api/v2/organizations/%s/templateversions", organizationID.String()), req, ) @@ -113,7 +113,7 @@ func (c *Client) CreateTemplateVersion(ctx context.Context, organizationID uuid. // CreateTemplate creates a new template inside an organization. func (c *Client) CreateTemplate(ctx context.Context, organizationID uuid.UUID, request CreateTemplateRequest) (Template, error) { - res, err := c.request(ctx, http.MethodPost, + res, err := c.Request(ctx, http.MethodPost, fmt.Sprintf("/api/v2/organizations/%s/templates", organizationID.String()), request, ) @@ -132,7 +132,7 @@ func (c *Client) CreateTemplate(ctx context.Context, organizationID uuid.UUID, r // TemplatesByOrganization lists all templates inside of an organization. func (c *Client) TemplatesByOrganization(ctx context.Context, organizationID uuid.UUID) ([]Template, error) { - res, err := c.request(ctx, http.MethodGet, + res, err := c.Request(ctx, http.MethodGet, fmt.Sprintf("/api/v2/organizations/%s/templates", organizationID.String()), nil, ) @@ -151,7 +151,7 @@ func (c *Client) TemplatesByOrganization(ctx context.Context, organizationID uui // TemplateByName finds a template inside the organization provided with a case-insensitive name. func (c *Client) TemplateByName(ctx context.Context, organizationID uuid.UUID, name string) (Template, error) { - res, err := c.request(ctx, http.MethodGet, + res, err := c.Request(ctx, http.MethodGet, fmt.Sprintf("/api/v2/organizations/%s/templates/%s", organizationID.String(), name), nil, ) @@ -170,7 +170,7 @@ func (c *Client) TemplateByName(ctx context.Context, organizationID uuid.UUID, n // CreateWorkspace creates a new workspace for the template specified. func (c *Client) CreateWorkspace(ctx context.Context, organizationID uuid.UUID, request CreateWorkspaceRequest) (Workspace, error) { - res, err := c.request(ctx, http.MethodPost, fmt.Sprintf("/api/v2/organizations/%s/workspaces", organizationID), request) + res, err := c.Request(ctx, http.MethodPost, fmt.Sprintf("/api/v2/organizations/%s/workspaces", organizationID), request) if err != nil { return Workspace{}, err } @@ -186,7 +186,7 @@ func (c *Client) CreateWorkspace(ctx context.Context, organizationID uuid.UUID, // WorkspacesByOrganization returns all workspaces in the specified organization. func (c *Client) WorkspacesByOrganization(ctx context.Context, organizationID uuid.UUID) ([]Workspace, error) { - res, err := c.request(ctx, http.MethodGet, fmt.Sprintf("/api/v2/organizations/%s/workspaces", organizationID), nil) + res, err := c.Request(ctx, http.MethodGet, fmt.Sprintf("/api/v2/organizations/%s/workspaces", organizationID), nil) if err != nil { return nil, err } @@ -202,7 +202,7 @@ func (c *Client) WorkspacesByOrganization(ctx context.Context, organizationID uu // WorkspacesByOwner returns all workspaces contained in the organization owned by the user. func (c *Client) WorkspacesByOwner(ctx context.Context, organizationID, userID uuid.UUID) ([]Workspace, error) { - res, err := c.request(ctx, http.MethodGet, fmt.Sprintf("/api/v2/organizations/%s/workspaces/%s", organizationID, uuidOrMe(userID)), nil) + res, err := c.Request(ctx, http.MethodGet, fmt.Sprintf("/api/v2/organizations/%s/workspaces/%s", organizationID, uuidOrMe(userID)), nil) if err != nil { return nil, err } @@ -218,7 +218,7 @@ func (c *Client) WorkspacesByOwner(ctx context.Context, organizationID, userID u // WorkspaceByOwnerAndName returns a workspace by the owner's UUID and the workspace's name. func (c *Client) WorkspaceByOwnerAndName(ctx context.Context, organization, owner uuid.UUID, name string) (Workspace, error) { - res, err := c.request(ctx, http.MethodGet, fmt.Sprintf("/api/v2/organizations/%s/workspaces/%s/%s", organization, uuidOrMe(owner), name), nil) + res, err := c.Request(ctx, http.MethodGet, fmt.Sprintf("/api/v2/organizations/%s/workspaces/%s/%s", organization, uuidOrMe(owner), name), nil) if err != nil { return Workspace{}, err } diff --git a/codersdk/pagination.go b/codersdk/pagination.go index a4adee6b6e567..c059266dd34c4 100644 --- a/codersdk/pagination.go +++ b/codersdk/pagination.go @@ -26,7 +26,7 @@ type Pagination struct { Offset int `json:"offset,omitempty"` } -// asRequestOption returns a function that can be used in (*Client).request. +// asRequestOption returns a function that can be used in (*Client).Request. // It modifies the request query parameters. func (p Pagination) asRequestOption() requestOption { return func(r *http.Request) { diff --git a/codersdk/parameters.go b/codersdk/parameters.go index 4697d07c51190..9fd9d5d9cad8d 100644 --- a/codersdk/parameters.go +++ b/codersdk/parameters.go @@ -43,7 +43,7 @@ type CreateParameterRequest struct { } func (c *Client) CreateParameter(ctx context.Context, scope ParameterScope, id uuid.UUID, req CreateParameterRequest) (Parameter, error) { - res, err := c.request(ctx, http.MethodPost, fmt.Sprintf("/api/v2/parameters/%s/%s", scope, id.String()), req) + res, err := c.Request(ctx, http.MethodPost, fmt.Sprintf("/api/v2/parameters/%s/%s", scope, id.String()), req) if err != nil { return Parameter{}, err } @@ -58,7 +58,7 @@ func (c *Client) CreateParameter(ctx context.Context, scope ParameterScope, id u } func (c *Client) DeleteParameter(ctx context.Context, scope ParameterScope, id uuid.UUID, name string) error { - res, err := c.request(ctx, http.MethodDelete, fmt.Sprintf("/api/v2/parameters/%s/%s/%s", scope, id.String(), name), nil) + res, err := c.Request(ctx, http.MethodDelete, fmt.Sprintf("/api/v2/parameters/%s/%s/%s", scope, id.String(), name), nil) if err != nil { return err } @@ -73,7 +73,7 @@ func (c *Client) DeleteParameter(ctx context.Context, scope ParameterScope, id u } func (c *Client) Parameters(ctx context.Context, scope ParameterScope, id uuid.UUID) ([]Parameter, error) { - res, err := c.request(ctx, http.MethodGet, fmt.Sprintf("/api/v2/parameters/%s/%s", scope, id.String()), nil) + res, err := c.Request(ctx, http.MethodGet, fmt.Sprintf("/api/v2/parameters/%s/%s", scope, id.String()), nil) if err != nil { return nil, err } diff --git a/codersdk/provisionerdaemons.go b/codersdk/provisionerdaemons.go index c726f8f255eef..1c906fa8c23b8 100644 --- a/codersdk/provisionerdaemons.go +++ b/codersdk/provisionerdaemons.go @@ -99,7 +99,7 @@ func (c *Client) provisionerJobLogsBefore(ctx context.Context, path string, befo if !before.IsZero() { values["before"] = []string{strconv.FormatInt(before.UTC().UnixMilli(), 10)} } - res, err := c.request(ctx, http.MethodGet, fmt.Sprintf("%s?%s", path, values.Encode()), nil) + res, err := c.Request(ctx, http.MethodGet, fmt.Sprintf("%s?%s", path, values.Encode()), nil) if err != nil { return nil, err } @@ -118,7 +118,7 @@ func (c *Client) provisionerJobLogsAfter(ctx context.Context, path string, after if !after.IsZero() { afterQuery = fmt.Sprintf("&after=%d", after.UTC().UnixMilli()) } - res, err := c.request(ctx, http.MethodGet, fmt.Sprintf("%s?follow%s", path, afterQuery), nil) + res, err := c.Request(ctx, http.MethodGet, fmt.Sprintf("%s?follow%s", path, afterQuery), nil) if err != nil { return nil, err } diff --git a/codersdk/roles.go b/codersdk/roles.go index 727c78e2256c1..85ec5296e6dfa 100644 --- a/codersdk/roles.go +++ b/codersdk/roles.go @@ -17,7 +17,7 @@ type Role struct { // ListSiteRoles lists all available site wide roles. // This is not user specific. func (c *Client) ListSiteRoles(ctx context.Context) ([]Role, error) { - res, err := c.request(ctx, http.MethodGet, "/api/v2/users/roles", nil) + res, err := c.Request(ctx, http.MethodGet, "/api/v2/users/roles", nil) if err != nil { return nil, err } @@ -32,7 +32,7 @@ func (c *Client) ListSiteRoles(ctx context.Context) ([]Role, error) { // ListOrganizationRoles lists all available roles for a given organization. // This is not user specific. func (c *Client) ListOrganizationRoles(ctx context.Context, org uuid.UUID) ([]Role, error) { - res, err := c.request(ctx, http.MethodGet, fmt.Sprintf("/api/v2/organizations/%s/members/roles/", org.String()), nil) + res, err := c.Request(ctx, http.MethodGet, fmt.Sprintf("/api/v2/organizations/%s/members/roles/", org.String()), nil) if err != nil { return nil, err } diff --git a/codersdk/templates.go b/codersdk/templates.go index d1f3361c85671..dceb2bcbc2cc3 100644 --- a/codersdk/templates.go +++ b/codersdk/templates.go @@ -31,7 +31,7 @@ type UpdateActiveTemplateVersion struct { // Template returns a single template. func (c *Client) Template(ctx context.Context, template uuid.UUID) (Template, error) { - res, err := c.request(ctx, http.MethodGet, fmt.Sprintf("/api/v2/templates/%s", template), nil) + res, err := c.Request(ctx, http.MethodGet, fmt.Sprintf("/api/v2/templates/%s", template), nil) if err != nil { return Template{}, nil } @@ -44,7 +44,7 @@ func (c *Client) Template(ctx context.Context, template uuid.UUID) (Template, er } func (c *Client) DeleteTemplate(ctx context.Context, template uuid.UUID) error { - res, err := c.request(ctx, http.MethodDelete, fmt.Sprintf("/api/v2/templates/%s", template), nil) + res, err := c.Request(ctx, http.MethodDelete, fmt.Sprintf("/api/v2/templates/%s", template), nil) if err != nil { return err } @@ -58,7 +58,7 @@ func (c *Client) DeleteTemplate(ctx context.Context, template uuid.UUID) error { // UpdateActiveTemplateVersion updates the active template version to the ID provided. // The template version must be attached to the template. func (c *Client) UpdateActiveTemplateVersion(ctx context.Context, template uuid.UUID, req UpdateActiveTemplateVersion) error { - res, err := c.request(ctx, http.MethodPatch, fmt.Sprintf("/api/v2/templates/%s/versions", template), req) + res, err := c.Request(ctx, http.MethodPatch, fmt.Sprintf("/api/v2/templates/%s/versions", template), req) if err != nil { return nil } @@ -78,7 +78,7 @@ type TemplateVersionsByTemplateRequest struct { // TemplateVersionsByTemplate lists versions associated with a template. func (c *Client) TemplateVersionsByTemplate(ctx context.Context, req TemplateVersionsByTemplateRequest) ([]TemplateVersion, error) { - res, err := c.request(ctx, http.MethodGet, fmt.Sprintf("/api/v2/templates/%s/versions", req.TemplateID), nil, req.Pagination.asRequestOption()) + res, err := c.Request(ctx, http.MethodGet, fmt.Sprintf("/api/v2/templates/%s/versions", req.TemplateID), nil, req.Pagination.asRequestOption()) if err != nil { return nil, err } @@ -93,7 +93,7 @@ func (c *Client) TemplateVersionsByTemplate(ctx context.Context, req TemplateVer // TemplateVersionByName returns a template version by it's friendly name. // This is used for path-based routing. Like: /templates/example/versions/helloworld func (c *Client) TemplateVersionByName(ctx context.Context, template uuid.UUID, name string) (TemplateVersion, error) { - res, err := c.request(ctx, http.MethodGet, fmt.Sprintf("/api/v2/templates/%s/versions/%s", template, name), nil) + res, err := c.Request(ctx, http.MethodGet, fmt.Sprintf("/api/v2/templates/%s/versions/%s", template, name), nil) if err != nil { return TemplateVersion{}, err } diff --git a/codersdk/templateversions.go b/codersdk/templateversions.go index f7cf29006e514..3b8b2e21711d4 100644 --- a/codersdk/templateversions.go +++ b/codersdk/templateversions.go @@ -31,7 +31,7 @@ type TemplateVersionParameter parameter.ComputedValue // TemplateVersion returns a template version by ID. func (c *Client) TemplateVersion(ctx context.Context, id uuid.UUID) (TemplateVersion, error) { - res, err := c.request(ctx, http.MethodGet, fmt.Sprintf("/api/v2/templateversions/%s", id), nil) + res, err := c.Request(ctx, http.MethodGet, fmt.Sprintf("/api/v2/templateversions/%s", id), nil) if err != nil { return TemplateVersion{}, err } @@ -45,7 +45,7 @@ func (c *Client) TemplateVersion(ctx context.Context, id uuid.UUID) (TemplateVer // CancelTemplateVersion marks a template version job as canceled. func (c *Client) CancelTemplateVersion(ctx context.Context, version uuid.UUID) error { - res, err := c.request(ctx, http.MethodPatch, fmt.Sprintf("/api/v2/templateversions/%s/cancel", version), nil) + res, err := c.Request(ctx, http.MethodPatch, fmt.Sprintf("/api/v2/templateversions/%s/cancel", version), nil) if err != nil { return err } @@ -58,7 +58,7 @@ func (c *Client) CancelTemplateVersion(ctx context.Context, version uuid.UUID) e // TemplateVersionSchema returns schemas for a template version by ID. func (c *Client) TemplateVersionSchema(ctx context.Context, version uuid.UUID) ([]TemplateVersionParameterSchema, error) { - res, err := c.request(ctx, http.MethodGet, fmt.Sprintf("/api/v2/templateversions/%s/schema", version), nil) + res, err := c.Request(ctx, http.MethodGet, fmt.Sprintf("/api/v2/templateversions/%s/schema", version), nil) if err != nil { return nil, err } @@ -72,7 +72,7 @@ func (c *Client) TemplateVersionSchema(ctx context.Context, version uuid.UUID) ( // TemplateVersionParameters returns computed parameters for a template version. func (c *Client) TemplateVersionParameters(ctx context.Context, version uuid.UUID) ([]TemplateVersionParameter, error) { - res, err := c.request(ctx, http.MethodGet, fmt.Sprintf("/api/v2/templateversions/%s/parameters", version), nil) + res, err := c.Request(ctx, http.MethodGet, fmt.Sprintf("/api/v2/templateversions/%s/parameters", version), nil) if err != nil { return nil, err } @@ -86,7 +86,7 @@ func (c *Client) TemplateVersionParameters(ctx context.Context, version uuid.UUI // TemplateVersionResources returns resources a template version declares. func (c *Client) TemplateVersionResources(ctx context.Context, version uuid.UUID) ([]WorkspaceResource, error) { - res, err := c.request(ctx, http.MethodGet, fmt.Sprintf("/api/v2/templateversions/%s/resources", version), nil) + res, err := c.Request(ctx, http.MethodGet, fmt.Sprintf("/api/v2/templateversions/%s/resources", version), nil) if err != nil { return nil, err } diff --git a/codersdk/users.go b/codersdk/users.go index 693277608d5b8..b9c2c7361e940 100644 --- a/codersdk/users.go +++ b/codersdk/users.go @@ -104,7 +104,7 @@ type AuthMethods struct { // HasFirstUser returns whether the first user has been created. func (c *Client) HasFirstUser(ctx context.Context) (bool, error) { - res, err := c.request(ctx, http.MethodGet, "/api/v2/users/first", nil) + res, err := c.Request(ctx, http.MethodGet, "/api/v2/users/first", nil) if err != nil { return false, err } @@ -121,7 +121,7 @@ func (c *Client) HasFirstUser(ctx context.Context) (bool, error) { // CreateFirstUser attempts to create the first user on a Coder deployment. // This initial user has superadmin privileges. If >0 users exist, this request will fail. func (c *Client) CreateFirstUser(ctx context.Context, req CreateFirstUserRequest) (CreateFirstUserResponse, error) { - res, err := c.request(ctx, http.MethodPost, "/api/v2/users/first", req) + res, err := c.Request(ctx, http.MethodPost, "/api/v2/users/first", req) if err != nil { return CreateFirstUserResponse{}, err } @@ -135,7 +135,7 @@ func (c *Client) CreateFirstUser(ctx context.Context, req CreateFirstUserRequest // CreateUser creates a new user. func (c *Client) CreateUser(ctx context.Context, req CreateUserRequest) (User, error) { - res, err := c.request(ctx, http.MethodPost, "/api/v2/users", req) + res, err := c.Request(ctx, http.MethodPost, "/api/v2/users", req) if err != nil { return User{}, err } @@ -149,7 +149,7 @@ func (c *Client) CreateUser(ctx context.Context, req CreateUserRequest) (User, e // UpdateUserProfile enables callers to update profile information func (c *Client) UpdateUserProfile(ctx context.Context, userID uuid.UUID, req UpdateUserProfileRequest) (User, error) { - res, err := c.request(ctx, http.MethodPut, fmt.Sprintf("/api/v2/users/%s/profile", uuidOrMe(userID)), req) + res, err := c.Request(ctx, http.MethodPut, fmt.Sprintf("/api/v2/users/%s/profile", uuidOrMe(userID)), req) if err != nil { return User{}, err } @@ -163,7 +163,7 @@ func (c *Client) UpdateUserProfile(ctx context.Context, userID uuid.UUID, req Up // SuspendUser enables callers to suspend a user func (c *Client) SuspendUser(ctx context.Context, userID uuid.UUID) (User, error) { - res, err := c.request(ctx, http.MethodPut, fmt.Sprintf("/api/v2/users/%s/suspend", uuidOrMe(userID)), nil) + res, err := c.Request(ctx, http.MethodPut, fmt.Sprintf("/api/v2/users/%s/suspend", uuidOrMe(userID)), nil) if err != nil { return User{}, err } @@ -179,7 +179,7 @@ func (c *Client) SuspendUser(ctx context.Context, userID uuid.UUID) (User, error // UpdateUserPassword updates a user password. // It calls PUT /users/{user}/password func (c *Client) UpdateUserPassword(ctx context.Context, userID uuid.UUID, req UpdateUserPasswordRequest) error { - res, err := c.request(ctx, http.MethodPut, fmt.Sprintf("/api/v2/users/%s/password", uuidOrMe(userID)), req) + res, err := c.Request(ctx, http.MethodPut, fmt.Sprintf("/api/v2/users/%s/password", uuidOrMe(userID)), req) if err != nil { return err } @@ -193,7 +193,7 @@ func (c *Client) UpdateUserPassword(ctx context.Context, userID uuid.UUID, req U // UpdateUserRoles grants the userID the specified roles. // Include ALL roles the user has. func (c *Client) UpdateUserRoles(ctx context.Context, userID uuid.UUID, req UpdateRoles) (User, error) { - res, err := c.request(ctx, http.MethodPut, fmt.Sprintf("/api/v2/users/%s/roles", uuidOrMe(userID)), req) + res, err := c.Request(ctx, http.MethodPut, fmt.Sprintf("/api/v2/users/%s/roles", uuidOrMe(userID)), req) if err != nil { return User{}, err } @@ -208,7 +208,7 @@ func (c *Client) UpdateUserRoles(ctx context.Context, userID uuid.UUID, req Upda // UpdateOrganizationMemberRoles grants the userID the specified roles in an org. // Include ALL roles the user has. func (c *Client) UpdateOrganizationMemberRoles(ctx context.Context, organizationID, userID uuid.UUID, req UpdateRoles) (OrganizationMember, error) { - res, err := c.request(ctx, http.MethodPut, fmt.Sprintf("/api/v2/organizations/%s/members/%s/roles", organizationID, uuidOrMe(userID)), req) + res, err := c.Request(ctx, http.MethodPut, fmt.Sprintf("/api/v2/organizations/%s/members/%s/roles", organizationID, uuidOrMe(userID)), req) if err != nil { return OrganizationMember{}, err } @@ -222,7 +222,7 @@ func (c *Client) UpdateOrganizationMemberRoles(ctx context.Context, organization // GetUserRoles returns all roles the user has func (c *Client) GetUserRoles(ctx context.Context, userID uuid.UUID) (UserRoles, error) { - res, err := c.request(ctx, http.MethodGet, fmt.Sprintf("/api/v2/users/%s/roles", uuidOrMe(userID)), nil) + res, err := c.Request(ctx, http.MethodGet, fmt.Sprintf("/api/v2/users/%s/roles", uuidOrMe(userID)), nil) if err != nil { return UserRoles{}, err } @@ -236,7 +236,7 @@ func (c *Client) GetUserRoles(ctx context.Context, userID uuid.UUID) (UserRoles, // CreateAPIKey generates an API key for the user ID provided. func (c *Client) CreateAPIKey(ctx context.Context, userID uuid.UUID) (*GenerateAPIKeyResponse, error) { - res, err := c.request(ctx, http.MethodPost, fmt.Sprintf("/api/v2/users/%s/keys", uuidOrMe(userID)), nil) + res, err := c.Request(ctx, http.MethodPost, fmt.Sprintf("/api/v2/users/%s/keys", uuidOrMe(userID)), nil) if err != nil { return nil, err } @@ -251,7 +251,7 @@ func (c *Client) CreateAPIKey(ctx context.Context, userID uuid.UUID) (*GenerateA // LoginWithPassword creates a session token authenticating with an email and password. // Call `SetSessionToken()` to apply the newly acquired token to the client. func (c *Client) LoginWithPassword(ctx context.Context, req LoginWithPasswordRequest) (LoginWithPasswordResponse, error) { - res, err := c.request(ctx, http.MethodPost, "/api/v2/users/login", req) + res, err := c.Request(ctx, http.MethodPost, "/api/v2/users/login", req) if err != nil { return LoginWithPasswordResponse{}, err } @@ -272,7 +272,7 @@ func (c *Client) LoginWithPassword(ctx context.Context, req LoginWithPasswordReq func (c *Client) Logout(ctx context.Context) error { // Since `LoginWithPassword` doesn't actually set a SessionToken // (it requires a call to SetSessionToken), this is essentially a no-op - res, err := c.request(ctx, http.MethodPost, "/api/v2/users/logout", nil) + res, err := c.Request(ctx, http.MethodPost, "/api/v2/users/logout", nil) if err != nil { return err } @@ -292,7 +292,7 @@ func (c *Client) UserByUsername(ctx context.Context, username string) (User, err } func (c *Client) userByIdentifier(ctx context.Context, ident string) (User, error) { - res, err := c.request(ctx, http.MethodGet, fmt.Sprintf("/api/v2/users/%s", ident), nil) + res, err := c.Request(ctx, http.MethodGet, fmt.Sprintf("/api/v2/users/%s", ident), nil) if err != nil { return User{}, err } @@ -307,7 +307,7 @@ func (c *Client) userByIdentifier(ctx context.Context, ident string) (User, erro // Users returns all users according to the request parameters. If no parameters are set, // the default behavior is to return all users in a single page. func (c *Client) Users(ctx context.Context, req UsersRequest) ([]User, error) { - res, err := c.request(ctx, http.MethodGet, "/api/v2/users", nil, + res, err := c.Request(ctx, http.MethodGet, "/api/v2/users", nil, req.Pagination.asRequestOption(), func(r *http.Request) { q := r.URL.Query() @@ -331,7 +331,7 @@ func (c *Client) Users(ctx context.Context, req UsersRequest) ([]User, error) { // OrganizationsByUser returns all organizations the user is a member of. func (c *Client) OrganizationsByUser(ctx context.Context, userID uuid.UUID) ([]Organization, error) { - res, err := c.request(ctx, http.MethodGet, fmt.Sprintf("/api/v2/users/%s/organizations", uuidOrMe(userID)), nil) + res, err := c.Request(ctx, http.MethodGet, fmt.Sprintf("/api/v2/users/%s/organizations", uuidOrMe(userID)), nil) if err != nil { return nil, err } @@ -344,7 +344,7 @@ func (c *Client) OrganizationsByUser(ctx context.Context, userID uuid.UUID) ([]O } func (c *Client) OrganizationByName(ctx context.Context, userID uuid.UUID, name string) (Organization, error) { - res, err := c.request(ctx, http.MethodGet, fmt.Sprintf("/api/v2/users/%s/organizations/%s", uuidOrMe(userID), name), nil) + res, err := c.Request(ctx, http.MethodGet, fmt.Sprintf("/api/v2/users/%s/organizations/%s", uuidOrMe(userID), name), nil) if err != nil { return Organization{}, err } @@ -358,7 +358,7 @@ func (c *Client) OrganizationByName(ctx context.Context, userID uuid.UUID, name // CreateOrganization creates an organization and adds the provided user as an admin. func (c *Client) CreateOrganization(ctx context.Context, userID uuid.UUID, req CreateOrganizationRequest) (Organization, error) { - res, err := c.request(ctx, http.MethodPost, fmt.Sprintf("/api/v2/users/%s/organizations", uuidOrMe(userID)), req) + res, err := c.Request(ctx, http.MethodPost, fmt.Sprintf("/api/v2/users/%s/organizations", uuidOrMe(userID)), req) if err != nil { return Organization{}, err } @@ -374,7 +374,7 @@ func (c *Client) CreateOrganization(ctx context.Context, userID uuid.UUID, req C // AuthMethods returns types of authentication available to the user. func (c *Client) AuthMethods(ctx context.Context) (AuthMethods, error) { - res, err := c.request(ctx, http.MethodGet, "/api/v2/users/authmethods", nil) + res, err := c.Request(ctx, http.MethodGet, "/api/v2/users/authmethods", nil) if err != nil { return AuthMethods{}, err } @@ -390,7 +390,7 @@ func (c *Client) AuthMethods(ctx context.Context) (AuthMethods, error) { // WorkspacesByUser returns all workspaces a user has access to. func (c *Client) WorkspacesByUser(ctx context.Context, userID uuid.UUID) ([]Workspace, error) { - res, err := c.request(ctx, http.MethodGet, fmt.Sprintf("/api/v2/users/%s/workspaces", uuidOrMe(userID)), nil) + res, err := c.Request(ctx, http.MethodGet, fmt.Sprintf("/api/v2/users/%s/workspaces", uuidOrMe(userID)), nil) if err != nil { return nil, err } diff --git a/codersdk/workspaceagents.go b/codersdk/workspaceagents.go index be98b4696fd8b..d64b42bc5faaa 100644 --- a/codersdk/workspaceagents.go +++ b/codersdk/workspaceagents.go @@ -65,7 +65,7 @@ func (c *Client) AuthWorkspaceGoogleInstanceIdentity(ctx context.Context, servic if err != nil { return WorkspaceAgentAuthenticateResponse{}, xerrors.Errorf("get metadata identity: %w", err) } - res, err := c.request(ctx, http.MethodPost, "/api/v2/workspaceagents/google-instance-identity", GoogleInstanceIdentityToken{ + res, err := c.Request(ctx, http.MethodPost, "/api/v2/workspaceagents/google-instance-identity", GoogleInstanceIdentityToken{ JSONWebToken: jwt, }) if err != nil { @@ -129,7 +129,7 @@ func (c *Client) AuthWorkspaceAWSInstanceIdentity(ctx context.Context) (Workspac return WorkspaceAgentAuthenticateResponse{}, xerrors.Errorf("read token: %w", err) } - res, err = c.request(ctx, http.MethodPost, "/api/v2/workspaceagents/aws-instance-identity", AWSInstanceIdentityToken{ + res, err = c.Request(ctx, http.MethodPost, "/api/v2/workspaceagents/aws-instance-identity", AWSInstanceIdentityToken{ Signature: string(signature), Document: string(document), }) @@ -164,7 +164,7 @@ func (c *Client) AuthWorkspaceAzureInstanceIdentity(ctx context.Context) (Worksp return WorkspaceAgentAuthenticateResponse{}, err } - res, err = c.request(ctx, http.MethodPost, "/api/v2/workspaceagents/azure-instance-identity", token) + res, err = c.Request(ctx, http.MethodPost, "/api/v2/workspaceagents/azure-instance-identity", token) if err != nil { return WorkspaceAgentAuthenticateResponse{}, err } @@ -213,7 +213,7 @@ func (c *Client) ListenWorkspaceAgent(ctx context.Context, logger slog.Logger) ( } listener, err := peerbroker.Listen(session, func(ctx context.Context) ([]webrtc.ICEServer, *peer.ConnOptions, error) { // This can be cached if it adds to latency too much. - res, err := c.request(ctx, http.MethodGet, "/api/v2/workspaceagents/me/iceservers", nil) + res, err := c.Request(ctx, http.MethodGet, "/api/v2/workspaceagents/me/iceservers", nil) if err != nil { return nil, nil, err } @@ -240,7 +240,7 @@ func (c *Client) ListenWorkspaceAgent(ctx context.Context, logger slog.Logger) ( if err != nil { return agent.Metadata{}, nil, xerrors.Errorf("listen peerbroker: %w", err) } - res, err = c.request(ctx, http.MethodGet, "/api/v2/workspaceagents/me/metadata", nil) + res, err = c.Request(ctx, http.MethodGet, "/api/v2/workspaceagents/me/metadata", nil) if err != nil { return agent.Metadata{}, nil, err } @@ -292,7 +292,7 @@ func (c *Client) DialWorkspaceAgent(ctx context.Context, agentID uuid.UUID, opti return nil, xerrors.Errorf("negotiate connection: %w", err) } - res, err = c.request(ctx, http.MethodGet, fmt.Sprintf("/api/v2/workspaceagents/%s/iceservers", agentID.String()), nil) + res, err = c.Request(ctx, http.MethodGet, fmt.Sprintf("/api/v2/workspaceagents/%s/iceservers", agentID.String()), nil) if err != nil { return nil, err } @@ -326,7 +326,7 @@ func (c *Client) DialWorkspaceAgent(ctx context.Context, agentID uuid.UUID, opti // WorkspaceAgent returns an agent by ID. func (c *Client) WorkspaceAgent(ctx context.Context, id uuid.UUID) (WorkspaceAgent, error) { - res, err := c.request(ctx, http.MethodGet, fmt.Sprintf("/api/v2/workspaceagents/%s", id), nil) + res, err := c.Request(ctx, http.MethodGet, fmt.Sprintf("/api/v2/workspaceagents/%s", id), nil) if err != nil { return WorkspaceAgent{}, err } diff --git a/codersdk/workspacebuilds.go b/codersdk/workspacebuilds.go index ef6e68d6bab8f..3c1ee5154b540 100644 --- a/codersdk/workspacebuilds.go +++ b/codersdk/workspacebuilds.go @@ -32,7 +32,7 @@ type WorkspaceBuild struct { // WorkspaceBuild returns a single workspace build for a workspace. // If history is "", the latest version is returned. func (c *Client) WorkspaceBuild(ctx context.Context, id uuid.UUID) (WorkspaceBuild, error) { - res, err := c.request(ctx, http.MethodGet, fmt.Sprintf("/api/v2/workspacebuilds/%s", id), nil) + res, err := c.Request(ctx, http.MethodGet, fmt.Sprintf("/api/v2/workspacebuilds/%s", id), nil) if err != nil { return WorkspaceBuild{}, err } @@ -46,7 +46,7 @@ func (c *Client) WorkspaceBuild(ctx context.Context, id uuid.UUID) (WorkspaceBui // CancelWorkspaceBuild marks a workspace build job as canceled. func (c *Client) CancelWorkspaceBuild(ctx context.Context, id uuid.UUID) error { - res, err := c.request(ctx, http.MethodPatch, fmt.Sprintf("/api/v2/workspacebuilds/%s/cancel", id), nil) + res, err := c.Request(ctx, http.MethodPatch, fmt.Sprintf("/api/v2/workspacebuilds/%s/cancel", id), nil) if err != nil { return err } @@ -59,7 +59,7 @@ func (c *Client) CancelWorkspaceBuild(ctx context.Context, id uuid.UUID) error { // WorkspaceResourcesByBuild returns resources for a workspace build. func (c *Client) WorkspaceResourcesByBuild(ctx context.Context, build uuid.UUID) ([]WorkspaceResource, error) { - res, err := c.request(ctx, http.MethodGet, fmt.Sprintf("/api/v2/workspacebuilds/%s/resources", build), nil) + res, err := c.Request(ctx, http.MethodGet, fmt.Sprintf("/api/v2/workspacebuilds/%s/resources", build), nil) if err != nil { return nil, err } @@ -83,7 +83,7 @@ func (c *Client) WorkspaceBuildLogsAfter(ctx context.Context, build uuid.UUID, a // WorkspaceBuildState returns the provisioner state of the build. func (c *Client) WorkspaceBuildState(ctx context.Context, build uuid.UUID) ([]byte, error) { - res, err := c.request(ctx, http.MethodGet, fmt.Sprintf("/api/v2/workspacebuilds/%s/state", build), nil) + res, err := c.Request(ctx, http.MethodGet, fmt.Sprintf("/api/v2/workspacebuilds/%s/state", build), nil) if err != nil { return nil, err } diff --git a/codersdk/workspaceresources.go b/codersdk/workspaceresources.go index b21451bbc63ea..f1dc5d74a04f9 100644 --- a/codersdk/workspaceresources.go +++ b/codersdk/workspaceresources.go @@ -69,7 +69,7 @@ type WorkspaceAgentInstanceMetadata struct { } func (c *Client) WorkspaceResource(ctx context.Context, id uuid.UUID) (WorkspaceResource, error) { - res, err := c.request(ctx, http.MethodGet, fmt.Sprintf("/api/v2/workspaceresources/%s", id), nil) + res, err := c.Request(ctx, http.MethodGet, fmt.Sprintf("/api/v2/workspaceresources/%s", id), nil) if err != nil { return WorkspaceResource{}, err } diff --git a/codersdk/workspaces.go b/codersdk/workspaces.go index 9d63755ad16de..9c9d4e5197537 100644 --- a/codersdk/workspaces.go +++ b/codersdk/workspaces.go @@ -39,7 +39,7 @@ type CreateWorkspaceBuildRequest struct { // Workspace returns a single workspace. func (c *Client) Workspace(ctx context.Context, id uuid.UUID) (Workspace, error) { - res, err := c.request(ctx, http.MethodGet, fmt.Sprintf("/api/v2/workspaces/%s", id), nil) + res, err := c.Request(ctx, http.MethodGet, fmt.Sprintf("/api/v2/workspaces/%s", id), nil) if err != nil { return Workspace{}, err } @@ -52,7 +52,7 @@ func (c *Client) Workspace(ctx context.Context, id uuid.UUID) (Workspace, error) } func (c *Client) WorkspaceBuilds(ctx context.Context, workspace uuid.UUID) ([]WorkspaceBuild, error) { - res, err := c.request(ctx, http.MethodGet, fmt.Sprintf("/api/v2/workspaces/%s/builds", workspace), nil) + res, err := c.Request(ctx, http.MethodGet, fmt.Sprintf("/api/v2/workspaces/%s/builds", workspace), nil) if err != nil { return nil, err } @@ -66,7 +66,7 @@ func (c *Client) WorkspaceBuilds(ctx context.Context, workspace uuid.UUID) ([]Wo // CreateWorkspaceBuild queues a new build to occur for a workspace. func (c *Client) CreateWorkspaceBuild(ctx context.Context, workspace uuid.UUID, request CreateWorkspaceBuildRequest) (WorkspaceBuild, error) { - res, err := c.request(ctx, http.MethodPost, fmt.Sprintf("/api/v2/workspaces/%s/builds", workspace), request) + res, err := c.Request(ctx, http.MethodPost, fmt.Sprintf("/api/v2/workspaces/%s/builds", workspace), request) if err != nil { return WorkspaceBuild{}, err } @@ -79,7 +79,7 @@ func (c *Client) CreateWorkspaceBuild(ctx context.Context, workspace uuid.UUID, } func (c *Client) WorkspaceBuildByName(ctx context.Context, workspace uuid.UUID, name string) (WorkspaceBuild, error) { - res, err := c.request(ctx, http.MethodGet, fmt.Sprintf("/api/v2/workspaces/%s/builds/%s", workspace, name), nil) + res, err := c.Request(ctx, http.MethodGet, fmt.Sprintf("/api/v2/workspaces/%s/builds/%s", workspace, name), nil) if err != nil { return WorkspaceBuild{}, err } @@ -100,7 +100,7 @@ type UpdateWorkspaceAutostartRequest struct { // If the provided schedule is empty, autostart is disabled for the workspace. func (c *Client) UpdateWorkspaceAutostart(ctx context.Context, id uuid.UUID, req UpdateWorkspaceAutostartRequest) error { path := fmt.Sprintf("/api/v2/workspaces/%s/autostart", id.String()) - res, err := c.request(ctx, http.MethodPut, path, req) + res, err := c.Request(ctx, http.MethodPut, path, req) if err != nil { return xerrors.Errorf("update workspace autostart: %w", err) } @@ -120,7 +120,7 @@ type UpdateWorkspaceAutostopRequest struct { // If the provided schedule is empty, autostop is disabled for the workspace. func (c *Client) UpdateWorkspaceAutostop(ctx context.Context, id uuid.UUID, req UpdateWorkspaceAutostopRequest) error { path := fmt.Sprintf("/api/v2/workspaces/%s/autostop", id.String()) - res, err := c.request(ctx, http.MethodPut, path, req) + res, err := c.Request(ctx, http.MethodPut, path, req) if err != nil { return xerrors.Errorf("update workspace autostop: %w", err) } From af6dc5fce6104a8ff9ffb82751fbbb29ea3c30a0 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 12 May 2022 07:48:47 -0500 Subject: [PATCH 02/26] Add more endpoints to the unit test --- coderd/coderd.go | 13 ++++++---- coderd/coderd_test.go | 50 +++++++++++++++++++++++++++++--------- coderd/httpmw/authorize.go | 42 +++++++++++++++++--------------- 3 files changed, 69 insertions(+), 36 deletions(-) diff --git a/coderd/coderd.go b/coderd/coderd.go index ac01dfecfd12f..08161d6bde7b0 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -83,8 +83,8 @@ func New(options *Options) (http.Handler, func()) { // TODO: @emyrk we should just move this into 'ExtractAPIKey'. authRolesMiddleware := httpmw.ExtractUserRoles(options.Database) - authorize := func(f http.HandlerFunc, actions rbac.Action) http.HandlerFunc { - return httpmw.Authorize(api.Logger, api.Authorizer, actions)(f).ServeHTTP + authorize := func(f http.HandlerFunc, actions ...rbac.Action) http.HandlerFunc { + return httpmw.Authorize(api.Logger, api.Authorizer, actions...)(f).ServeHTTP } r := chi.NewRouter() @@ -131,10 +131,12 @@ func New(options *Options) (http.Handler, func()) { // This number is arbitrary, but reading/writing // file content is expensive so it should be small. httpmw.RateLimitPerMinute(12), + // TODO: @emyrk (rbac) Currently files are owned by the site? + // Should files be org scoped? User scoped? httpmw.WithRBACObject(rbac.ResourceTypeFile), ) - r.Get("/{hash}", api.fileByHash) - r.Post("/", api.postFile) + r.Get("/{hash}", authorize(api.fileByHash, rbac.ActionRead)) + r.Post("/", authorize(api.postFile, rbac.ActionCreate, rbac.ActionUpdate)) }) r.Route("/organizations/{organization}", func(r chi.Router) { r.Use( @@ -142,7 +144,8 @@ func New(options *Options) (http.Handler, func()) { authRolesMiddleware, httpmw.ExtractOrganizationParam(options.Database), ) - r.Get("/", api.organization) + r.With(httpmw.WithRBACObject(rbac.ResourceOrganization)). + Get("/", authorize(api.organization, rbac.ActionRead)) r.Get("/provisionerdaemons", api.provisionerDaemonsByOrganization) r.Post("/templateversions", api.postTemplateVersionsByOrganization) r.Route("/templates", func(r chi.Router) { diff --git a/coderd/coderd_test.go b/coderd/coderd_test.go index 25231d34fc2d5..209bfb4180383 100644 --- a/coderd/coderd_test.go +++ b/coderd/coderd_test.go @@ -2,7 +2,9 @@ package coderd_test import ( "context" + "fmt" "net/http" + "strings" "testing" "github.com/go-chi/chi/v5" @@ -32,15 +34,6 @@ func TestBuildInfo(t *testing.T) { func TestAuthorizeAllEndpoints(t *testing.T) { t.Parallel() - // skipRoutes allows skipping routes from being checked. - type routeCheck struct { - NoAuthorize bool - } - assertRoute := map[string]routeCheck{ - "GET:/api/v2": {NoAuthorize: true}, - "GET:/api/v2/buildinfo": {NoAuthorize: true}, - } - authorizer := &fakeAuthorizer{} srv, client := coderdtest.NewMemoryCoderd(t, &coderdtest.Options{ Authorizer: authorizer, @@ -48,23 +41,58 @@ func TestAuthorizeAllEndpoints(t *testing.T) { admin := coderdtest.CreateFirstUser(t, client) var _ = admin + // skipRoutes allows skipping routes from being checked. + type routeCheck struct { + NoAuthorize bool + AssertObject rbac.Object + } + assertRoute := map[string]routeCheck{ + "GET:/api/v2": {NoAuthorize: true}, + "GET:/api/v2/buildinfo": {NoAuthorize: true}, + "GET:/api/v2/users/first": {NoAuthorize: true}, + "POST:/api/v2/users/first": {NoAuthorize: true}, + "POST:/api/v2/users/login": {NoAuthorize: true}, + "POST:/api/v2/users/logout": {NoAuthorize: true}, + "GET:/api/v2/users/authmethods": {NoAuthorize: true}, + + // TODO: @emyrk these need to be fixed by adding authorize calls + "/api/v2/organizations/{organization}/provisionerdaemons": {NoAuthorize: true}, + "GET:/api/v2/organizations/{organization}": {AssertObject: rbac.ResourceOrganization.InOrg(admin.OrganizationID)}, + } + c := srv.Config.Handler.(*chi.Mux) err := chi.Walk(c, func(method string, route string, handler http.Handler, middlewares ...func(http.Handler) http.Handler) error { name := method + ":" + route t.Run(name, func(t *testing.T) { authorizer.reset() - routeAssertions, ok := assertRoute[name] + routeAssertions, ok := assertRoute[strings.TrimRight(name, "/")] if !ok { // By default, all omitted routes check for just "authorize" called routeAssertions = routeCheck{} } + // Replace all url params with expected + route = strings.ReplaceAll(route, "{organization}", admin.OrganizationID.String()) + resp, err := client.Request(context.Background(), method, route, nil) require.NoError(t, err, "do req") _ = resp.Body.Close() if !routeAssertions.NoAuthorize { assert.NotNil(t, authorizer.Called, "authorizer expected") + assert.Equal(t, http.StatusUnauthorized, resp.StatusCode, "expect unauthorized") + if routeAssertions.AssertObject.Type != "" { + assert.Equal(t, routeAssertions.AssertObject.Type, authorizer.Called.Object.Type, "resource type") + } + if routeAssertions.AssertObject.Owner != "" { + assert.Equal(t, routeAssertions.AssertObject.Owner, authorizer.Called.Object.Owner, "resource owner") + } + 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") } @@ -92,7 +120,7 @@ func (f *fakeAuthorizer) AuthorizeByRoleName(ctx context.Context, subjectID stri Action: action, Object: object, } - return nil + return rbac.ForbiddenWithInternal(fmt.Errorf("fake implementation"), nil, nil) } func (f *fakeAuthorizer) reset() { diff --git a/coderd/httpmw/authorize.go b/coderd/httpmw/authorize.go index 6bca6f23c1159..2ab95cca818ba 100644 --- a/coderd/httpmw/authorize.go +++ b/coderd/httpmw/authorize.go @@ -17,7 +17,7 @@ import ( // Authorize will enforce if the user roles can complete the action on the AuthObject. // The organization and owner are found using the ExtractOrganization and // ExtractUser middleware if present. -func Authorize(logger slog.Logger, auth rbac.Authorizer, action rbac.Action) func(http.Handler) http.Handler { +func Authorize(logger slog.Logger, auth rbac.Authorizer, actions ...rbac.Action) func(http.Handler) http.Handler { return func(next http.Handler) http.Handler { return http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { roles := UserRoles(r) @@ -51,26 +51,28 @@ func Authorize(logger slog.Logger, auth rbac.Authorizer, action rbac.Action) fun } } - err := auth.AuthorizeByRoleName(r.Context(), roles.ID.String(), roles.Roles, action, object) - if err != nil { - internalError := new(rbac.UnauthorizedError) - if xerrors.As(err, internalError) { - logger = logger.With(slog.F("internal", internalError.Internal())) + for _, action := range actions { + err := auth.AuthorizeByRoleName(r.Context(), roles.ID.String(), roles.Roles, action, object) + if err != nil { + internalError := new(rbac.UnauthorizedError) + if xerrors.As(err, internalError) { + logger = logger.With(slog.F("internal", internalError.Internal())) + } + // Log information for debugging. This will be very helpful + // in the early days if we over secure endpoints. + logger.Warn(r.Context(), "unauthorized", + slog.F("roles", roles.Roles), + slog.F("user_id", roles.ID), + slog.F("username", roles.Username), + slog.F("route", r.URL.Path), + slog.F("action", action), + slog.F("object", object), + ) + httpapi.Write(rw, http.StatusUnauthorized, httpapi.Response{ + Message: err.Error(), + }) + return } - // Log information for debugging. This will be very helpful - // in the early days if we over secure endpoints. - logger.Warn(r.Context(), "unauthorized", - slog.F("roles", roles.Roles), - slog.F("user_id", roles.ID), - slog.F("username", roles.Username), - slog.F("route", r.URL.Path), - slog.F("action", action), - slog.F("object", object), - ) - httpapi.Write(rw, http.StatusUnauthorized, httpapi.Response{ - Message: err.Error(), - }) - return } next.ServeHTTP(rw, r) }) From be5b0b3e4aaebe5af099a552b2166cb9294080fe Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 12 May 2022 08:44:26 -0500 Subject: [PATCH 03/26] Rbac users endpoints Skip workspace agent endpoints --- coderd/coderd.go | 53 ++++++++++++++------- coderd/coderd_test.go | 19 +++++++- coderd/coderdtest/coderdtest.go | 3 +- coderd/rbac/builtin.go | 18 +++++++- coderd/rbac/object.go | 36 +++++++++++++-- coderd/users.go | 81 +++++++++++++++++++++++---------- 6 files changed, 162 insertions(+), 48 deletions(-) diff --git a/coderd/coderd.go b/coderd/coderd.go index 08161d6bde7b0..2b8abf2d4cc06 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -133,7 +133,7 @@ func New(options *Options) (http.Handler, func()) { httpmw.RateLimitPerMinute(12), // TODO: @emyrk (rbac) Currently files are owned by the site? // Should files be org scoped? User scoped? - httpmw.WithRBACObject(rbac.ResourceTypeFile), + httpmw.WithRBACObject(rbac.ResourceFile), ) r.Get("/{hash}", authorize(api.fileByHash, rbac.ActionRead)) r.Post("/", authorize(api.postFile, rbac.ActionCreate, rbac.ActionUpdate)) @@ -237,8 +237,12 @@ func New(options *Options) (http.Handler, func()) { apiKeyMiddleware, authRolesMiddleware, ) - r.Post("/", api.postUser) - r.Get("/", api.users) + r.Group(func(r chi.Router) { + // Site wide, all users + r.Use(httpmw.WithRBACObject(rbac.ResourceUser)) + r.Post("/", authorize(api.postUser, rbac.ActionCreate)) + r.Get("/", authorize(api.users, rbac.ActionRead)) + }) // These routes query information about site wide roles. r.Route("/roles", func(r chi.Router) { r.Use(httpmw.WithRBACObject(rbac.ResourceUserRole)) @@ -246,27 +250,42 @@ func New(options *Options) (http.Handler, func()) { }) r.Route("/{user}", func(r chi.Router) { r.Use(httpmw.ExtractUserParam(options.Database)) - r.Get("/", api.userByName) - r.Put("/profile", api.putUserProfile) - r.Put("/suspend", api.putUserSuspend) - r.Route("/password", func(r chi.Router) { - r.Use(httpmw.WithRBACObject(rbac.ResourceUserPasswordRole)) - r.Put("/", authorize(api.putUserPassword, rbac.ActionUpdate)) + r.Group(func(r chi.Router) { + r.Use(httpmw.WithRBACObject(rbac.ResourceUser)) + r.Get("/", authorize(api.userByName, rbac.ActionRead)) + r.Put("/profile", authorize(api.putUserProfile, rbac.ActionUpdate)) + // suspension is deleting for a user + r.Put("/suspend", authorize(api.putUserSuspend, rbac.ActionDelete)) + r.Route("/password", func(r chi.Router) { + r.Put("/", authorize(api.putUserPassword, rbac.ActionUpdate)) + }) + // This route technically also fetches the organization member struct, but only + // returns the roles. + r.Get("/roles", authorize(api.userRoles, rbac.ActionRead)) + + // This has 2 authorize calls. The second is explicitly called + // in the handler. + r.Put("/roles", authorize(api.putUserRoles, rbac.ActionUpdate)) + + // For now, just use the "user" role for their ssh keys. + // We can always split this out to it's own resource if we need to. + r.Get("/gitsshkey", authorize(api.gitSSHKey, rbac.ActionRead)) + r.Put("/gitsshkey", authorize(api.regenerateGitSSHKey, rbac.ActionUpdate)) }) - r.Get("/organizations", api.organizationsByUser) - r.Post("/organizations", api.postOrganizationsByUser) - // These roles apply to the site wide permissions. - r.Put("/roles", api.putUserRoles) - r.Get("/roles", api.userRoles) - r.Post("/keys", api.postAPIKey) + r.With(httpmw.WithRBACObject(rbac.ResourceAPIKey)).Post("/keys", authorize(api.postAPIKey, rbac.ActionCreate)) + r.Route("/organizations", func(r chi.Router) { + // TODO: @emyrk This creates an organization, so why is it nested under {user}? + // Shouldn't this be outside the {user} param subpath? Maybe in the organizations/ + // path? r.Post("/", api.postOrganizationsByUser) + r.Get("/", api.organizationsByUser) + + // TODO: @emyrk why is this nested under {user} when the user param is not used? r.Get("/{organizationname}", api.organizationByUserAndName) }) - r.Get("/gitsshkey", api.gitSSHKey) - r.Put("/gitsshkey", api.regenerateGitSSHKey) r.Get("/workspaces", api.workspacesByUser) }) }) diff --git a/coderd/coderd_test.go b/coderd/coderd_test.go index 209bfb4180383..eef6f8830027a 100644 --- a/coderd/coderd_test.go +++ b/coderd/coderd_test.go @@ -55,6 +55,22 @@ func TestAuthorizeAllEndpoints(t *testing.T) { "POST:/api/v2/users/logout": {NoAuthorize: true}, "GET:/api/v2/users/authmethods": {NoAuthorize: true}, + // All workspaceagents endpoints do not use rbac + "POST:/api/v2/workspaceagents/aws-instance-identity": {NoAuthorize: true}, + "POST:/api/v2/workspaceagents/azure-instance-identity": {NoAuthorize: true}, + "POST:/api/v2/workspaceagents/google-instance-identity": {NoAuthorize: true}, + "GET:/api/v2/workspaceagents/me/gitsshkey": {NoAuthorize: true}, + "GET:/api/v2/workspaceagents/me/iceservers": {NoAuthorize: true}, + "GET:/api/v2/workspaceagents/me/listen": {NoAuthorize: true}, + "GET:/api/v2/workspaceagents/me/metadata": {NoAuthorize: true}, + "GET:/api/v2/workspaceagents/me/turn": {NoAuthorize: true}, + "GET:/api/v2/workspaceagents/{workspaceagent}": {NoAuthorize: true}, + "GET:/api/v2/workspaceagents/{workspaceagent}/": {NoAuthorize: true}, + "GET:/api/v2/workspaceagents/{workspaceagent}/dial": {NoAuthorize: true}, + "GET:/api/v2/workspaceagents/{workspaceagent}/iceservers": {NoAuthorize: true}, + "GET:/api/v2/workspaceagents/{workspaceagent}/pty": {NoAuthorize: true}, + "GET:/api/v2/workspaceagents/{workspaceagent}/turn": {NoAuthorize: true}, + // TODO: @emyrk these need to be fixed by adding authorize calls "/api/v2/organizations/{organization}/provisionerdaemons": {NoAuthorize: true}, "GET:/api/v2/organizations/{organization}": {AssertObject: rbac.ResourceOrganization.InOrg(admin.OrganizationID)}, @@ -71,8 +87,9 @@ func TestAuthorizeAllEndpoints(t *testing.T) { routeAssertions = routeCheck{} } - // Replace all url params with expected + // Replace all url params with known values route = strings.ReplaceAll(route, "{organization}", admin.OrganizationID.String()) + route = strings.ReplaceAll(route, "{user}", admin.UserID.String()) resp, err := client.Request(context.Background(), method, route, nil) require.NoError(t, err, "do req") diff --git a/coderd/coderdtest/coderdtest.go b/coderd/coderdtest/coderdtest.go index 0eb748d02f957..87f4da375e35f 100644 --- a/coderd/coderdtest/coderdtest.go +++ b/coderd/coderdtest/coderdtest.go @@ -55,12 +55,13 @@ import ( type Options struct { AWSCertificates awsidentity.Certificates + Authorizer rbac.Authorizer AzureCertificates x509.VerifyOptions GithubOAuth2Config *coderd.GithubOAuth2Config GoogleTokenValidator *idtoken.Validator SSHKeygenAlgorithm gitsshkey.Algorithm APIRateLimit int - Authorizer rbac.Authorizer + LifecycleTicker <-chan time.Time } // New constructs an in-memory coderd instance and returns diff --git a/coderd/rbac/builtin.go b/coderd/rbac/builtin.go index 6a85cfe3256a2..b5bc13822d6c8 100644 --- a/coderd/rbac/builtin.go +++ b/coderd/rbac/builtin.go @@ -64,6 +64,11 @@ var ( return Role{ Name: member, DisplayName: "Member", + Site: permissions(map[Object][]Action{ + // All users can read all organizations. + // TODO: @emyrk is this ok? + ResourceOrganization: {ActionRead}, + }), User: permissions(map[Object][]Action{ ResourceWildcard: {WildcardSymbol}, }), @@ -111,7 +116,18 @@ var ( Name: roleName(orgMember, organizationID), DisplayName: "Organization Member", Org: map[string][]Permission{ - organizationID: {}, + organizationID: { + { + // All org members can read the other members in their org. + ResourceType: ResourceOrganizationMember.Type, + Action: ActionRead, + }, + { + // All org members can read the organization + ResourceType: ResourceOrganization.Type, + Action: ActionRead, + }, + }, }, } }, diff --git a/coderd/rbac/object.go b/coderd/rbac/object.go index 3888ade2ba618..f1a3ea6ff5954 100644 --- a/coderd/rbac/object.go +++ b/coderd/rbac/object.go @@ -21,13 +21,13 @@ var ( Type: "template", } - ResourceTypeFile = Object{ + ResourceFile = Object{ Type: "file", } - // ResourceOrganization CRUD. Org owner + // ResourceOrganization CRUD. Always has an org owner. // create/delete = make or delete organizations - // read = view org information + // read = view org information (Can add user owner for read) // update = ?? ResourceOrganization = Object{ Type: "organization", @@ -36,12 +36,38 @@ var ( // ResourceUserRole might be expanded later to allow more granular permissions // to modifying roles. For now, this covers all possible roles, so having this permission // allows granting/deleting **ALL** roles. + // create = Assign roles + // update = ?? + // read = View available roles to assign + // delete = Remove role ResourceUserRole = Object{ Type: "user_role", } - ResourceUserPasswordRole = Object{ - Type: "user_password", + // ResourceAPIKey is owned by a user. + // create = Create a new api key for user + // update = ?? + // read = View api key + // delete = Delete api key + ResourceAPIKey = Object{ + Type: "api_key", + } + + // ResourceUser is the user in the 'users' table. + // create/delete = make or delete a new user. + // read = view all user's settings + // update = update all user field & settings + ResourceUser = Object{ + Type: "user", + } + + // ResourceOrganizationMember is a user's membership in an organization. + // Has ONLY an organization owner. + // create/delete = Create/delete member from org. + // update = Update organization member + // read = View member + ResourceOrganizationMember = Object{ + Type: "organization_member", } // ResourceWildcard represents all resource types diff --git a/coderd/users.go b/coderd/users.go index f6dc26ccb51fe..0c3ccc258d63c 100644 --- a/coderd/users.go +++ b/coderd/users.go @@ -385,22 +385,51 @@ 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 - // TODO: Until rbac authorize is implemented, only be able to change your - // own roles. This also means you can grant yourself whatever roles you want. user := httpmw.UserParam(r) - apiKey := httpmw.APIKey(r) - if apiKey.UserID != user.ID { - httpapi.Write(rw, http.StatusUnauthorized, httpapi.Response{ - Message: "modifying other users is not supported at this time", - }) - return - } + roles := httpmw.UserRoles(r) var params codersdk.UpdateRoles if !httpapi.Read(rw, r, ¶ms) { return } + has := make(map[string]struct{}) + for _, exists := range roles.Roles { + has[exists] = struct{}{} + } + + for _, roleName := range params.Roles { + // If the user already has the role, we don't need to check the permission. + if _, ok := has[roleName]; ok { + delete(has, roleName) + continue + } + + // Assigning a role requires the create permission. The middleware checks if + // we can update this user, so the combination of the 2 permissions enables + // assigning new roles. + err := api.Authorizer.AuthorizeByRoleName(r.Context(), roles.ID.String(), roles.Roles, + rbac.ActionCreate, rbac.ResourceUserRole.WithID(roleName)) + if err != nil { + httpapi.Write(rw, http.StatusUnauthorized, httpapi.Response{ + Message: "unauthorized", + }) + return + } + } + + // Any roles that were removed also need to be checked. + for roleName := range has { + err := api.Authorizer.AuthorizeByRoleName(r.Context(), roles.ID.String(), roles.Roles, + rbac.ActionDelete, rbac.ResourceUserRole.WithID(roleName)) + if err != nil { + httpapi.Write(rw, http.StatusUnauthorized, httpapi.Response{ + Message: "unauthorized", + }) + return + } + } + updatedUser, err := api.updateSiteUserRoles(r.Context(), database.UpdateUserRolesParams{ GrantedRoles: params.Roles, ID: user.ID, @@ -445,6 +474,7 @@ 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) { @@ -460,14 +490,23 @@ func (api *api) organizationsByUser(rw http.ResponseWriter, r *http.Request) { publicOrganizations := make([]codersdk.Organization, 0, len(organizations)) for _, organization := range organizations { - publicOrganizations = append(publicOrganizations, convertOrganization(organization)) + err := api.Authorizer.AuthorizeByRoleName(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)) + } } httpapi.Write(rw, http.StatusOK, publicOrganizations) } func (api *api) organizationByUserAndName(rw http.ResponseWriter, r *http.Request) { - user := httpmw.UserParam(r) + roles := httpmw.UserRoles(r) + organizationName := chi.URLParam(r, "organizationname") organization, err := api.Database.GetOrganizationByName(r.Context(), organizationName) if errors.Is(err, sql.ErrNoRows) { @@ -482,19 +521,15 @@ func (api *api) organizationByUserAndName(rw http.ResponseWriter, r *http.Reques }) return } - _, err = api.Database.GetOrganizationMemberByUserID(r.Context(), database.GetOrganizationMemberByUserIDParams{ - OrganizationID: organization.ID, - UserID: user.ID, - }) - if errors.Is(err, sql.ErrNoRows) { - httpapi.Write(rw, http.StatusUnauthorized, httpapi.Response{ - Message: "you are not a member of that organization", - }) - return - } + + err = api.Authorizer.AuthorizeByRoleName(r.Context(), roles.ID.String(), roles.Roles, rbac.ActionRead, + rbac.ResourceOrganization. + InOrg(organization.ID). + WithID(organization.ID.String()), + ) if err != nil { - httpapi.Write(rw, http.StatusInternalServerError, httpapi.Response{ - Message: fmt.Sprintf("get organization member: %s", err), + httpapi.Write(rw, http.StatusUnauthorized, httpapi.Response{ + Message: err.Error(), }) return } From 970e34539fa3c26e7cee1f38bb8a1a3785d02dcd Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 12 May 2022 09:37:39 -0500 Subject: [PATCH 04/26] Make test pass by skipping missed endpoints --- coderd/coderd.go | 4 +-- coderd/coderd_test.go | 72 ++++++++++++++++++++++++++++++++++++++----- coderd/users.go | 1 + 3 files changed, 68 insertions(+), 9 deletions(-) diff --git a/coderd/coderd.go b/coderd/coderd.go index 2b8abf2d4cc06..a974fded6dcb8 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -239,7 +239,7 @@ func New(options *Options) (http.Handler, func()) { ) r.Group(func(r chi.Router) { // Site wide, all users - r.Use(httpmw.WithRBACObject(rbac.ResourceUser)) + r.Use(httpmw.WithRBACObject(rbac.ResourceUser.All())) r.Post("/", authorize(api.postUser, rbac.ActionCreate)) r.Get("/", authorize(api.users, rbac.ActionRead)) }) @@ -274,6 +274,7 @@ func New(options *Options) (http.Handler, func()) { }) r.With(httpmw.WithRBACObject(rbac.ResourceAPIKey)).Post("/keys", authorize(api.postAPIKey, rbac.ActionCreate)) + r.Get("/workspaces", api.workspacesByUser) r.Route("/organizations", func(r chi.Router) { // TODO: @emyrk This creates an organization, so why is it nested under {user}? @@ -286,7 +287,6 @@ func New(options *Options) (http.Handler, func()) { // TODO: @emyrk why is this nested under {user} when the user param is not used? r.Get("/{organizationname}", api.organizationByUserAndName) }) - r.Get("/workspaces", api.workspacesByUser) }) }) }) diff --git a/coderd/coderd_test.go b/coderd/coderd_test.go index eef6f8830027a..1fd9cdbee673e 100644 --- a/coderd/coderd_test.go +++ b/coderd/coderd_test.go @@ -39,14 +39,20 @@ func TestAuthorizeAllEndpoints(t *testing.T) { Authorizer: authorizer, }) admin := coderdtest.CreateFirstUser(t, client) - var _ = admin + organization, err := client.Organization(context.Background(), admin.OrganizationID) + require.NoError(t, err, "fetch org") + + // Always fail auth from this point forward + authorizer.AlwaysReturn = rbac.ForbiddenWithInternal(fmt.Errorf("fake implementation"), nil, nil) // skipRoutes allows skipping routes from being checked. type routeCheck struct { NoAuthorize bool AssertObject rbac.Object + StatusCode int } assertRoute := map[string]routeCheck{ + // These endpoints do not require auth "GET:/api/v2": {NoAuthorize: true}, "GET:/api/v2/buildinfo": {NoAuthorize: true}, "GET:/api/v2/users/first": {NoAuthorize: true}, @@ -72,12 +78,59 @@ func TestAuthorizeAllEndpoints(t *testing.T) { "GET:/api/v2/workspaceagents/{workspaceagent}/turn": {NoAuthorize: true}, // TODO: @emyrk these need to be fixed by adding authorize calls - "/api/v2/organizations/{organization}/provisionerdaemons": {NoAuthorize: true}, - "GET:/api/v2/organizations/{organization}": {AssertObject: rbac.ResourceOrganization.InOrg(admin.OrganizationID)}, + "GET:/api/v2/workspaceresources/{workspaceresource}": {NoAuthorize: true}, + "GET:/api/v2/workspacebuilds/{workspacebuild}": {NoAuthorize: true}, + "GET:/api/v2/workspacebuilds/{workspacebuild}/logs": {NoAuthorize: true}, + "GET:/api/v2/workspacebuilds/{workspacebuild}/resources": {NoAuthorize: true}, + "GET:/api/v2/workspacebuilds/{workspacebuild}/state": {NoAuthorize: true}, + "PATCH:/api/v2/workspacebuilds/{workspacebuild}/cancel": {NoAuthorize: true}, + "GET:/api/v2/workspaces/{workspace}/builds/{workspacebuildname}": {NoAuthorize: true}, + + "GET:/api/v2/users/oauth2/github/callback": {NoAuthorize: true}, + + "POST:/api/v2/users/{user}/organizations/": {NoAuthorize: true}, + "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/parameters/{scope}/{id}": {NoAuthorize: true}, + "GET:/api/v2/parameters/{scope}/{id}": {NoAuthorize: true}, + "DELETE:/api/v2/parameters/{scope}/{id}/{name}": {NoAuthorize: true}, + + "GET:/api/v2/provisionerdaemons/me/listen": {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}, + + "GET:/api/v2/templateversions/{templateversion}": {NoAuthorize: true}, + "PATCH:/api/v2/templateversions/{templateversion}/cancel": {NoAuthorize: true}, + "GET:/api/v2/templateversions/{templateversion}/logs": {NoAuthorize: true}, + "GET:/api/v2/templateversions/{templateversion}/parameters": {NoAuthorize: true}, + "GET:/api/v2/templateversions/{templateversion}/resources": {NoAuthorize: true}, + "GET:/api/v2/templateversions/{templateversion}/schema": {NoAuthorize: true}, + + "POST:/api/v2/users/{user}/organizations": {NoAuthorize: true}, + + "GET:/api/v2/workspaces/{workspace}": {NoAuthorize: true}, + "PUT:/api/v2/workspaces/{workspace}/autostart": {NoAuthorize: true}, + "PUT:/api/v2/workspaces/{workspace}/autostop": {NoAuthorize: true}, + "GET:/api/v2/workspaces/{workspace}/builds": {NoAuthorize: true}, + "POST:/api/v2/workspaces/{workspace}/builds": {NoAuthorize: true}, + + // These endpoints have more assertions. This is good, add more endpoints to assert if you can! + "GET:/api/v2/organizations/{organization}": {AssertObject: rbac.ResourceOrganization.InOrg(admin.OrganizationID)}, + "GET:/api/v2/users/{user}/organizations": {StatusCode: http.StatusOK, AssertObject: rbac.ResourceOrganization}, + "GET:/api/v2/users/{user}/workspaces": {StatusCode: http.StatusOK, AssertObject: rbac.ResourceWorkspace}, } c := srv.Config.Handler.(*chi.Mux) - err := chi.Walk(c, func(method string, route string, handler http.Handler, middlewares ...func(http.Handler) http.Handler) error { + err = chi.Walk(c, func(method string, route string, handler http.Handler, middlewares ...func(http.Handler) http.Handler) error { name := method + ":" + route t.Run(name, func(t *testing.T) { authorizer.reset() @@ -86,10 +139,14 @@ func TestAuthorizeAllEndpoints(t *testing.T) { // By default, all omitted routes check for just "authorize" called routeAssertions = routeCheck{} } + if routeAssertions.StatusCode == 0 { + routeAssertions.StatusCode = http.StatusUnauthorized + } // Replace all url params with known values route = strings.ReplaceAll(route, "{organization}", admin.OrganizationID.String()) route = strings.ReplaceAll(route, "{user}", admin.UserID.String()) + route = strings.ReplaceAll(route, "{organizationname}", organization.Name) resp, err := client.Request(context.Background(), method, route, nil) require.NoError(t, err, "do req") @@ -97,7 +154,7 @@ func TestAuthorizeAllEndpoints(t *testing.T) { if !routeAssertions.NoAuthorize { assert.NotNil(t, authorizer.Called, "authorizer expected") - assert.Equal(t, http.StatusUnauthorized, resp.StatusCode, "expect unauthorized") + assert.Equal(t, routeAssertions.StatusCode, resp.StatusCode, "expect unauthorized") if routeAssertions.AssertObject.Type != "" { assert.Equal(t, routeAssertions.AssertObject.Type, authorizer.Called.Object.Type, "resource type") } @@ -127,7 +184,8 @@ type authCall struct { } type fakeAuthorizer struct { - Called *authCall + Called *authCall + AlwaysReturn error } func (f *fakeAuthorizer) AuthorizeByRoleName(ctx context.Context, subjectID string, roleNames []string, action rbac.Action, object rbac.Object) error { @@ -137,7 +195,7 @@ func (f *fakeAuthorizer) AuthorizeByRoleName(ctx context.Context, subjectID stri Action: action, Object: object, } - return rbac.ForbiddenWithInternal(fmt.Errorf("fake implementation"), nil, nil) + return f.AlwaysReturn } func (f *fakeAuthorizer) reset() { diff --git a/coderd/users.go b/coderd/users.go index 0c3ccc258d63c..f4f06d3ffde9e 100644 --- a/coderd/users.go +++ b/coderd/users.go @@ -811,6 +811,7 @@ func (api *api) createUser(ctx context.Context, req codersdk.CreateUserRequest) }) } +// func (api *api) workspacesByUser(rw http.ResponseWriter, r *http.Request) { user := httpmw.UserParam(r) roles := httpmw.UserRoles(r) From 945e9fab304ab9e0931355414454c254089d7b9c Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 12 May 2022 09:49:29 -0500 Subject: [PATCH 05/26] Fix broken tests --- coderd/coderd.go | 4 ++-- coderd/coderd_test.go | 12 ++++++++---- coderd/rbac/builtin.go | 6 +----- coderd/users_test.go | 10 ++++++---- 4 files changed, 17 insertions(+), 15 deletions(-) diff --git a/coderd/coderd.go b/coderd/coderd.go index a974fded6dcb8..2faff14f55346 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -135,8 +135,8 @@ func New(options *Options) (http.Handler, func()) { // Should files be org scoped? User scoped? httpmw.WithRBACObject(rbac.ResourceFile), ) - r.Get("/{hash}", authorize(api.fileByHash, rbac.ActionRead)) - r.Post("/", authorize(api.postFile, rbac.ActionCreate, rbac.ActionUpdate)) + r.Get("/{hash}", api.fileByHash) + r.Post("/", api.postFile) }) r.Route("/organizations/{organization}", func(r chi.Router) { r.Use( diff --git a/coderd/coderd_test.go b/coderd/coderd_test.go index 1fd9cdbee673e..8ecf8ca43334f 100644 --- a/coderd/coderd_test.go +++ b/coderd/coderd_test.go @@ -2,11 +2,12 @@ package coderd_test import ( "context" - "fmt" "net/http" "strings" "testing" + "golang.org/x/xerrors" + "github.com/go-chi/chi/v5" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -43,7 +44,7 @@ func TestAuthorizeAllEndpoints(t *testing.T) { require.NoError(t, err, "fetch org") // Always fail auth from this point forward - authorizer.AlwaysReturn = rbac.ForbiddenWithInternal(fmt.Errorf("fake implementation"), nil, nil) + authorizer.AlwaysReturn = rbac.ForbiddenWithInternal(xerrors.New("fake implementation"), nil, nil) // skipRoutes allows skipping routes from being checked. type routeCheck struct { @@ -123,13 +124,16 @@ func TestAuthorizeAllEndpoints(t *testing.T) { "GET:/api/v2/workspaces/{workspace}/builds": {NoAuthorize: true}, "POST:/api/v2/workspaces/{workspace}/builds": {NoAuthorize: true}, + "POST:/api/v2/files": {NoAuthorize: true}, + "GET:/api/v2/files/{hash}": {NoAuthorize: true}, + // These endpoints have more assertions. This is good, add more endpoints to assert if you can! "GET:/api/v2/organizations/{organization}": {AssertObject: rbac.ResourceOrganization.InOrg(admin.OrganizationID)}, "GET:/api/v2/users/{user}/organizations": {StatusCode: http.StatusOK, AssertObject: rbac.ResourceOrganization}, "GET:/api/v2/users/{user}/workspaces": {StatusCode: http.StatusOK, AssertObject: rbac.ResourceWorkspace}, } - c := srv.Config.Handler.(*chi.Mux) + c, _ := srv.Config.Handler.(*chi.Mux) err = chi.Walk(c, func(method string, route string, handler http.Handler, middlewares ...func(http.Handler) http.Handler) error { name := method + ":" + route t.Run(name, func(t *testing.T) { @@ -188,7 +192,7 @@ type fakeAuthorizer struct { AlwaysReturn error } -func (f *fakeAuthorizer) AuthorizeByRoleName(ctx context.Context, subjectID string, roleNames []string, action rbac.Action, object rbac.Object) error { +func (f *fakeAuthorizer) AuthorizeByRoleName(_ context.Context, subjectID string, roleNames []string, action rbac.Action, object rbac.Object) error { f.Called = &authCall{ SubjectID: subjectID, Roles: roleNames, diff --git a/coderd/rbac/builtin.go b/coderd/rbac/builtin.go index b5bc13822d6c8..a314addab3afb 100644 --- a/coderd/rbac/builtin.go +++ b/coderd/rbac/builtin.go @@ -64,11 +64,7 @@ var ( return Role{ Name: member, DisplayName: "Member", - Site: permissions(map[Object][]Action{ - // All users can read all organizations. - // TODO: @emyrk is this ok? - ResourceOrganization: {ActionRead}, - }), + Site: permissions(map[Object][]Action{}), User: permissions(map[Object][]Action{ ResourceWildcard: {WildcardSymbol}, }), diff --git a/coderd/users_test.go b/coderd/users_test.go index 99d1849ca6cf8..a9fd04b123897 100644 --- a/coderd/users_test.go +++ b/coderd/users_test.go @@ -401,10 +401,11 @@ func TestGrantRoles(t *testing.T) { []string{rbac.RoleOrgMember(first.OrganizationID)}, ) + memberUser, err := member.User(ctx, codersdk.Me) + require.NoError(t, err, "fetch member") + // Grant - // TODO: @emyrk this should be 'admin.UpdateUserRoles' once proper authz - // is enforced. - _, err = member.UpdateUserRoles(ctx, codersdk.Me, codersdk.UpdateRoles{ + _, err = admin.UpdateUserRoles(ctx, memberUser.ID, codersdk.UpdateRoles{ Roles: []string{ // Promote to site admin rbac.RoleMember(), @@ -588,12 +589,13 @@ func TestOrganizationByUserAndName(t *testing.T) { t.Parallel() client := coderdtest.New(t, nil) first := coderdtest.CreateFirstUser(t, client) + notMember := coderdtest.CreateAnotherUser(t, client, first.OrganizationID) other := coderdtest.CreateAnotherUser(t, client, first.OrganizationID) org, err := other.CreateOrganization(context.Background(), codersdk.Me, codersdk.CreateOrganizationRequest{ Name: "another", }) require.NoError(t, err) - _, err = client.OrganizationByName(context.Background(), codersdk.Me, org.Name) + _, err = notMember.OrganizationByName(context.Background(), codersdk.Me, org.Name) var apiErr *codersdk.Error require.ErrorAs(t, err, &apiErr) require.Equal(t, http.StatusUnauthorized, apiErr.StatusCode()) From fdfef88f4ac75ad9a9813d199d5260e20a3afac0 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 12 May 2022 09:57:50 -0500 Subject: [PATCH 06/26] Import order --- coderd/coderd_test.go | 3 +-- coderd/httpmw/authorize.go | 1 - 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/coderd/coderd_test.go b/coderd/coderd_test.go index 8ecf8ca43334f..e81234cc7131a 100644 --- a/coderd/coderd_test.go +++ b/coderd/coderd_test.go @@ -6,12 +6,11 @@ import ( "strings" "testing" - "golang.org/x/xerrors" - "github.com/go-chi/chi/v5" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "go.uber.org/goleak" + "golang.org/x/xerrors" "github.com/coder/coder/buildinfo" "github.com/coder/coder/coderd/coderdtest" diff --git a/coderd/httpmw/authorize.go b/coderd/httpmw/authorize.go index 2ab95cca818ba..797540da99610 100644 --- a/coderd/httpmw/authorize.go +++ b/coderd/httpmw/authorize.go @@ -5,7 +5,6 @@ import ( "net/http" "github.com/google/uuid" - "golang.org/x/xerrors" "cdr.dev/slog" From 89a367890c41e5f73403b9d672b02cefc3bf481e Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 12 May 2022 12:29:45 -0500 Subject: [PATCH 07/26] PR comment fixes - Rename authObject - Shorten 'AuthorizeByRoleName' --- coderd/coderd_test.go | 2 +- coderd/httpmw/authorize.go | 18 +++++++++--------- coderd/rbac/authz.go | 6 +++--- coderd/users.go | 10 +++++----- 4 files changed, 18 insertions(+), 18 deletions(-) diff --git a/coderd/coderd_test.go b/coderd/coderd_test.go index e81234cc7131a..9ddc0e0f9c3a3 100644 --- a/coderd/coderd_test.go +++ b/coderd/coderd_test.go @@ -191,7 +191,7 @@ type fakeAuthorizer struct { AlwaysReturn error } -func (f *fakeAuthorizer) AuthorizeByRoleName(_ context.Context, subjectID string, roleNames []string, action rbac.Action, object rbac.Object) error { +func (f *fakeAuthorizer) ByRoleName(_ context.Context, subjectID string, roleNames []string, action rbac.Action, object rbac.Object) error { f.Called = &authCall{ SubjectID: subjectID, Roles: roleNames, diff --git a/coderd/httpmw/authorize.go b/coderd/httpmw/authorize.go index 797540da99610..86b918111ce3a 100644 --- a/coderd/httpmw/authorize.go +++ b/coderd/httpmw/authorize.go @@ -13,7 +13,7 @@ import ( "github.com/coder/coder/coderd/rbac" ) -// Authorize will enforce if the user roles can complete the action on the AuthObject. +// Authorize will enforce if the user roles can complete the action on the RBACObject. // The organization and owner are found using the ExtractOrganization and // ExtractUser middleware if present. func Authorize(logger slog.Logger, auth rbac.Authorizer, actions ...rbac.Action) func(http.Handler) http.Handler { @@ -51,7 +51,7 @@ func Authorize(logger slog.Logger, auth rbac.Authorizer, actions ...rbac.Action) } for _, action := range actions { - err := auth.AuthorizeByRoleName(r.Context(), roles.ID.String(), roles.Roles, action, object) + err := auth.ByRoleName(r.Context(), roles.ID.String(), roles.Roles, action, object) if err != nil { internalError := new(rbac.UnauthorizedError) if xerrors.As(err, internalError) { @@ -80,15 +80,15 @@ func Authorize(logger slog.Logger, auth rbac.Authorizer, actions ...rbac.Action) type authObjectKey struct{} -type AuthObject struct { +type RBACObject struct { Object rbac.Object WithOwner func(r *http.Request) uuid.UUID } // APIKey returns the API key from the ExtractAPIKey handler. -func rbacObject(r *http.Request) AuthObject { - obj, ok := r.Context().Value(authObjectKey{}).(AuthObject) +func rbacObject(r *http.Request) RBACObject { + obj, ok := r.Context().Value(authObjectKey{}).(RBACObject) if !ok { panic("developer error: auth object middleware not provided") } @@ -107,11 +107,11 @@ func WithAPIKeyAsOwner() func(http.Handler) http.Handler { func WithOwner(withOwner func(r *http.Request) uuid.UUID) func(http.Handler) http.Handler { return func(next http.Handler) http.Handler { return http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { - obj, ok := r.Context().Value(authObjectKey{}).(AuthObject) + obj, ok := r.Context().Value(authObjectKey{}).(RBACObject) if ok { obj.WithOwner = withOwner } else { - obj = AuthObject{WithOwner: withOwner} + obj = RBACObject{WithOwner: withOwner} } ctx := context.WithValue(r.Context(), authObjectKey{}, obj) @@ -125,11 +125,11 @@ func WithOwner(withOwner func(r *http.Request) uuid.UUID) func(http.Handler) htt func WithRBACObject(object rbac.Object) func(http.Handler) http.Handler { return func(next http.Handler) http.Handler { return http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { - obj, ok := r.Context().Value(authObjectKey{}).(AuthObject) + obj, ok := r.Context().Value(authObjectKey{}).(RBACObject) if ok { obj.Object = object } else { - obj = AuthObject{Object: object} + obj = RBACObject{Object: object} } ctx := context.WithValue(r.Context(), authObjectKey{}, obj) diff --git a/coderd/rbac/authz.go b/coderd/rbac/authz.go index 938989a5b71b7..6325a4b8c506b 100644 --- a/coderd/rbac/authz.go +++ b/coderd/rbac/authz.go @@ -10,7 +10,7 @@ import ( ) type Authorizer interface { - AuthorizeByRoleName(ctx context.Context, subjectID string, roleNames []string, action Action, object Object) error + ByRoleName(ctx context.Context, subjectID string, roleNames []string, action Action, object Object) error } // RegoAuthorizer will use a prepared rego query for performing authorize() @@ -42,10 +42,10 @@ type authSubject struct { Roles []Role `json:"roles"` } -// AuthorizeByRoleName will expand all roleNames into roles before calling Authorize(). +// ByRoleName will expand all roleNames into roles before calling Authorize(). // This is the function intended to be used outside this package. // The role is fetched from the builtin map located in memory. -func (a RegoAuthorizer) AuthorizeByRoleName(ctx context.Context, subjectID string, roleNames []string, action Action, object Object) error { +func (a RegoAuthorizer) ByRoleName(ctx context.Context, subjectID string, roleNames []string, action Action, object Object) error { roles := make([]Role, 0, len(roleNames)) for _, n := range roleNames { r, err := RoleByName(n) diff --git a/coderd/users.go b/coderd/users.go index f4f06d3ffde9e..252ff4c4e9b49 100644 --- a/coderd/users.go +++ b/coderd/users.go @@ -408,7 +408,7 @@ func (api *api) putUserRoles(rw http.ResponseWriter, r *http.Request) { // Assigning a role requires the create permission. The middleware checks if // we can update this user, so the combination of the 2 permissions enables // assigning new roles. - err := api.Authorizer.AuthorizeByRoleName(r.Context(), roles.ID.String(), roles.Roles, + err := api.Authorizer.ByRoleName(r.Context(), roles.ID.String(), roles.Roles, rbac.ActionCreate, rbac.ResourceUserRole.WithID(roleName)) if err != nil { httpapi.Write(rw, http.StatusUnauthorized, httpapi.Response{ @@ -420,7 +420,7 @@ func (api *api) putUserRoles(rw http.ResponseWriter, r *http.Request) { // Any roles that were removed also need to be checked. for roleName := range has { - err := api.Authorizer.AuthorizeByRoleName(r.Context(), roles.ID.String(), roles.Roles, + err := api.Authorizer.ByRoleName(r.Context(), roles.ID.String(), roles.Roles, rbac.ActionDelete, rbac.ResourceUserRole.WithID(roleName)) if err != nil { httpapi.Write(rw, http.StatusUnauthorized, httpapi.Response{ @@ -490,7 +490,7 @@ func (api *api) organizationsByUser(rw http.ResponseWriter, r *http.Request) { publicOrganizations := make([]codersdk.Organization, 0, len(organizations)) for _, organization := range organizations { - err := api.Authorizer.AuthorizeByRoleName(r.Context(), roles.ID.String(), roles.Roles, rbac.ActionRead, + err := api.Authorizer.ByRoleName(r.Context(), roles.ID.String(), roles.Roles, rbac.ActionRead, rbac.ResourceOrganization. WithID(organization.ID.String()). InOrg(organization.ID), @@ -522,7 +522,7 @@ func (api *api) organizationByUserAndName(rw http.ResponseWriter, r *http.Reques return } - err = api.Authorizer.AuthorizeByRoleName(r.Context(), roles.ID.String(), roles.Roles, rbac.ActionRead, + err = api.Authorizer.ByRoleName(r.Context(), roles.ID.String(), roles.Roles, rbac.ActionRead, rbac.ResourceOrganization. InOrg(organization.ID). WithID(organization.ID.String()), @@ -825,7 +825,7 @@ func (api *api) workspacesByUser(rw http.ResponseWriter, r *http.Request) { } organizationIDs := make([]uuid.UUID, 0) for _, organization := range organizations { - err = api.Authorizer.AuthorizeByRoleName(r.Context(), user.ID.String(), roles.Roles, rbac.ActionRead, rbac.ResourceWorkspace.All().InOrg(organization.ID)) + err = api.Authorizer.ByRoleName(r.Context(), user.ID.String(), roles.Roles, rbac.ActionRead, rbac.ResourceWorkspace.All().InOrg(organization.ID)) var apiErr *rbac.UnauthorizedError if xerrors.As(err, &apiErr) { continue From 63727e0e29d5e6fb5b7f71332972e5eb2e39507d Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 13 May 2022 11:30:09 -0500 Subject: [PATCH 08/26] omit another endpoint --- coderd/coderd_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/coderd/coderd_test.go b/coderd/coderd_test.go index 9ddc0e0f9c3a3..7ac5736dedf27 100644 --- a/coderd/coderd_test.go +++ b/coderd/coderd_test.go @@ -95,6 +95,7 @@ func TestAuthorizeAllEndpoints(t *testing.T) { "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}, "POST:/api/v2/parameters/{scope}/{id}": {NoAuthorize: true}, "GET:/api/v2/parameters/{scope}/{id}": {NoAuthorize: true}, From 96a57275e53a5093573a6383fc3c8946f81d47d5 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 13 May 2022 13:12:06 -0500 Subject: [PATCH 09/26] Cleanup comments --- coderd/httpmw/authorize.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/coderd/httpmw/authorize.go b/coderd/httpmw/authorize.go index 86b918111ce3a..44fdb8a2b0e04 100644 --- a/coderd/httpmw/authorize.go +++ b/coderd/httpmw/authorize.go @@ -83,10 +83,12 @@ type authObjectKey struct{} type RBACObject struct { Object rbac.Object + // WithOwner will set the Object.Owner field based on the request. + // This allows the Owner field to be set dynamically based on the context + // of the request. WithOwner func(r *http.Request) uuid.UUID } -// APIKey returns the API key from the ExtractAPIKey handler. func rbacObject(r *http.Request) RBACObject { obj, ok := r.Context().Value(authObjectKey{}).(RBACObject) if !ok { From 4b6c9b069d6cbe56fe21436af8f73af125ad0372 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 13 May 2022 13:20:30 -0500 Subject: [PATCH 10/26] Do not leak if an organization name exists --- coderd/users.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/coderd/users.go b/coderd/users.go index 252ff4c4e9b49..be99ca20c87e3 100644 --- a/coderd/users.go +++ b/coderd/users.go @@ -510,8 +510,10 @@ func (api *api) organizationByUserAndName(rw http.ResponseWriter, r *http.Reques organizationName := chi.URLParam(r, "organizationname") organization, err := api.Database.GetOrganizationByName(r.Context(), organizationName) if errors.Is(err, sql.ErrNoRows) { - httpapi.Write(rw, http.StatusNotFound, httpapi.Response{ - Message: fmt.Sprintf("no organization found by name %q", organizationName), + // Return unauthorized rather than a 404 to not leak if the organization + // exists. + httpapi.Write(rw, http.StatusUnauthorized, httpapi.Response{ + Message: "unauthorized", }) return } From cd2fda70566d797618ea36126623f577c69fd7ec Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 13 May 2022 13:26:40 -0500 Subject: [PATCH 11/26] Update comment --- coderd/coderd.go | 2 +- coderd/users.go | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/coderd/coderd.go b/coderd/coderd.go index 4fc7feabacd2a..8a62e4cb7f11f 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -240,7 +240,7 @@ func New(options *Options) (http.Handler, func()) { ) r.Group(func(r chi.Router) { // Site wide, all users - r.Use(httpmw.WithRBACObject(rbac.ResourceUser.All())) + r.Use(httpmw.WithRBACObject(rbac.ResourceUser)) r.Post("/", authorize(api.postUser, rbac.ActionCreate)) r.Get("/", authorize(api.users, rbac.ActionRead)) }) diff --git a/coderd/users.go b/coderd/users.go index be99ca20c87e3..ead9e4d5ef74d 100644 --- a/coderd/users.go +++ b/coderd/users.go @@ -399,7 +399,9 @@ func (api *api) putUserRoles(rw http.ResponseWriter, r *http.Request) { } for _, roleName := range params.Roles { - // If the user already has the role, we don't need to check the permission. + // If the user already has the role assigned, we don't need to check the permission + // to reassign it. Only run permission checks on the difference in the set of + // roles. if _, ok := has[roleName]; ok { delete(has, roleName) continue From 62ec87e91630b9096c75c8522a44297c3d4fc453 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 13 May 2022 13:49:36 -0500 Subject: [PATCH 12/26] feat: Implement authorize for each endpoint --- coderd/authorize.go | 21 ++++++++++++ coderd/coderd.go | 78 ++++++++++++++------------------------------- coderd/users.go | 12 +++++++ 3 files changed, 57 insertions(+), 54 deletions(-) create mode 100644 coderd/authorize.go diff --git a/coderd/authorize.go b/coderd/authorize.go new file mode 100644 index 0000000000000..9ea96e25afe95 --- /dev/null +++ b/coderd/authorize.go @@ -0,0 +1,21 @@ +package coderd + +import ( + "net/http" + + "github.com/coder/coder/coderd/httpapi" + "github.com/coder/coder/coderd/httpmw" + "github.com/coder/coder/coderd/rbac" +) + +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) + if err != nil { + httpapi.Write(rw, http.StatusUnauthorized, httpapi.Response{ + Message: err.Error(), + }) + return false + } + return true +} diff --git a/coderd/coderd.go b/coderd/coderd.go index 8a62e4cb7f11f..d059606b208b2 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -83,8 +83,8 @@ func New(options *Options) (http.Handler, func()) { // TODO: @emyrk we should just move this into 'ExtractAPIKey'. authRolesMiddleware := httpmw.ExtractUserRoles(options.Database) - authorize := func(f http.HandlerFunc, actions ...rbac.Action) http.HandlerFunc { - return httpmw.Authorize(api.Logger, api.Authorizer, actions...)(f).ServeHTTP + authorize := func(f http.HandlerFunc, actions rbac.Action) http.HandlerFunc { + return httpmw.Authorize(api.Logger, api.Authorizer, actions)(f).ServeHTTP } r := chi.NewRouter() @@ -127,13 +127,9 @@ func New(options *Options) (http.Handler, func()) { r.Route("/files", func(r chi.Router) { r.Use( apiKeyMiddleware, - authRolesMiddleware, // This number is arbitrary, but reading/writing // file content is expensive so it should be small. httpmw.RateLimitPerMinute(12), - // TODO: @emyrk (rbac) Currently files are owned by the site? - // Should files be org scoped? User scoped? - httpmw.WithRBACObject(rbac.ResourceFile), ) r.Get("/{hash}", api.fileByHash) r.Post("/", api.postFile) @@ -141,11 +137,10 @@ func New(options *Options) (http.Handler, func()) { r.Route("/organizations/{organization}", func(r chi.Router) { r.Use( apiKeyMiddleware, - authRolesMiddleware, httpmw.ExtractOrganizationParam(options.Database), + authRolesMiddleware, ) - r.With(httpmw.WithRBACObject(rbac.ResourceOrganization)). - Get("/", authorize(api.organization, rbac.ActionRead)) + r.Get("/", api.organization) r.Get("/provisionerdaemons", api.provisionerDaemonsByOrganization) r.Post("/templateversions", api.postTemplateVersionsByOrganization) r.Route("/templates", func(r chi.Router) { @@ -154,17 +149,12 @@ func New(options *Options) (http.Handler, func()) { r.Get("/{templatename}", api.templateByOrganizationAndName) }) r.Route("/workspaces", func(r chi.Router) { - r.Use(httpmw.WithRBACObject(rbac.ResourceWorkspace)) - // Posting a workspace is inherently owned by the api key creating it. - r.With(httpmw.WithAPIKeyAsOwner()). - Post("/", authorize(api.postWorkspacesByOrganization, rbac.ActionCreate)) - r.Get("/", authorize(api.workspacesByOrganization, rbac.ActionRead)) r.Post("/", api.postWorkspacesByOrganization) + r.Get("/", api.workspacesByOrganization) r.Route("/{user}", func(r chi.Router) { r.Use(httpmw.ExtractUserParam(options.Database)) - // TODO: @emyrk add the resource id to this authorize. - r.Get("/{workspace}", authorize(api.workspaceByOwnerAndName, rbac.ActionRead)) - r.Get("/", authorize(api.workspacesByOwner, rbac.ActionRead)) + r.Get("/{workspace}", api.workspaceByOwnerAndName) + r.Get("/", api.workspacesByOwner) }) }) r.Route("/members", func(r chi.Router) { @@ -238,12 +228,8 @@ func New(options *Options) (http.Handler, func()) { apiKeyMiddleware, authRolesMiddleware, ) - r.Group(func(r chi.Router) { - // Site wide, all users - r.Use(httpmw.WithRBACObject(rbac.ResourceUser)) - r.Post("/", authorize(api.postUser, rbac.ActionCreate)) - r.Get("/", authorize(api.users, rbac.ActionRead)) - }) + r.Post("/", api.postUser) + r.Get("/", api.users) // These routes query information about site wide roles. r.Route("/roles", func(r chi.Router) { r.Use(httpmw.WithRBACObject(rbac.ResourceUserRole)) @@ -251,45 +237,29 @@ func New(options *Options) (http.Handler, func()) { }) r.Route("/{user}", func(r chi.Router) { r.Use(httpmw.ExtractUserParam(options.Database)) - r.Group(func(r chi.Router) { - r.Use(httpmw.WithRBACObject(rbac.ResourceUser)) - r.Get("/", authorize(api.userByName, rbac.ActionRead)) - r.Put("/profile", authorize(api.putUserProfile, rbac.ActionUpdate)) - // suspension is deleting for a user - r.Put("/suspend", authorize(api.putUserSuspend, rbac.ActionDelete)) - r.Route("/password", func(r chi.Router) { - r.Put("/", authorize(api.putUserPassword, rbac.ActionUpdate)) - }) - // This route technically also fetches the organization member struct, but only - // returns the roles. - r.Get("/roles", authorize(api.userRoles, rbac.ActionRead)) - - // This has 2 authorize calls. The second is explicitly called - // in the handler. - r.Put("/roles", authorize(api.putUserRoles, rbac.ActionUpdate)) - - // For now, just use the "user" role for their ssh keys. - // We can always split this out to it's own resource if we need to. - r.Get("/gitsshkey", authorize(api.gitSSHKey, rbac.ActionRead)) - r.Put("/gitsshkey", authorize(api.regenerateGitSSHKey, rbac.ActionUpdate)) - - r.Post("/authorization", authorize(api.checkPermissions, rbac.ActionRead)) + r.Get("/", api.userByName) + r.Put("/profile", api.putUserProfile) + r.Put("/suspend", api.putUserSuspend) + r.Route("/password", func(r chi.Router) { + r.Put("/", authorize(api.putUserPassword, rbac.ActionUpdate)) }) + r.Get("/organizations", api.organizationsByUser) + r.Post("/organizations", api.postOrganizationsByUser) + // These roles apply to the site wide permissions. + r.Put("/roles", api.putUserRoles) + r.Get("/roles", api.userRoles) - r.With(httpmw.WithRBACObject(rbac.ResourceAPIKey)).Post("/keys", authorize(api.postAPIKey, rbac.ActionCreate)) - r.Get("/workspaces", api.workspacesByUser) + r.Post("/authorization", api.checkPermissions) + r.Post("/keys", api.postAPIKey) r.Route("/organizations", func(r chi.Router) { - // TODO: @emyrk This creates an organization, so why is it nested under {user}? - // Shouldn't this be outside the {user} param subpath? Maybe in the organizations/ - // path? r.Post("/", api.postOrganizationsByUser) - r.Get("/", api.organizationsByUser) - - // TODO: @emyrk why is this nested under {user} when the user param is not used? r.Get("/{organizationname}", api.organizationByUserAndName) }) + r.Get("/gitsshkey", api.gitSSHKey) + r.Put("/gitsshkey", api.regenerateGitSSHKey) + r.Get("/workspaces", api.workspacesByUser) }) }) }) diff --git a/coderd/users.go b/coderd/users.go index ead9e4d5ef74d..8745fd27fadc3 100644 --- a/coderd/users.go +++ b/coderd/users.go @@ -163,6 +163,18 @@ func (api *api) postUser(rw http.ResponseWriter, r *http.Request) { if !httpapi.Read(rw, r, &createUser) { return } + + // Create the user on the site + if !api.Authorize(rw, r, rbac.ActionCreate, rbac.ResourceUser) { + return + } + + // Create the organization member in the org. + if !api.Authorize(rw, r, rbac.ActionCreate, + rbac.ResourceOrganizationMember.InOrg(createUser.OrganizationID)) { + return + } + _, err := api.Database.GetUserByEmailOrUsername(r.Context(), database.GetUserByEmailOrUsernameParams{ Username: createUser.Username, Email: createUser.Email, From 452c72d8e00fde267728c7f37d1d8ac1f9ad0277 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 13 May 2022 13:56:45 -0500 Subject: [PATCH 13/26] Authorize per endpoint --- coderd/authorize.go | 21 +++++++++++++++++++++ coderd/coderd.go | 14 +++----------- coderd/roles.go | 1 + coderd/users.go | 19 +++++++++++++++++++ 4 files changed, 44 insertions(+), 11 deletions(-) diff --git a/coderd/authorize.go b/coderd/authorize.go index 9ea96e25afe95..f893b1a776614 100644 --- a/coderd/authorize.go +++ b/coderd/authorize.go @@ -3,6 +3,9 @@ package coderd import ( "net/http" + "cdr.dev/slog" + "golang.org/x/xerrors" + "github.com/coder/coder/coderd/httpapi" "github.com/coder/coder/coderd/httpmw" "github.com/coder/coder/coderd/rbac" @@ -15,6 +18,24 @@ func (api *api) Authorize(rw http.ResponseWriter, r *http.Request, action rbac.A httpapi.Write(rw, http.StatusUnauthorized, httpapi.Response{ Message: err.Error(), }) + + // Log the errors for debugging + internalError := new(rbac.UnauthorizedError) + logger := api.Logger + if xerrors.As(err, internalError) { + logger = api.Logger.With(slog.F("internal", internalError.Internal())) + } + // Log information for debugging. This will be very helpful + // in the early days + logger.Warn(r.Context(), "unauthorized", + slog.F("roles", roles.Roles), + slog.F("user_id", roles.ID), + slog.F("username", roles.Username), + slog.F("route", r.URL.Path), + slog.F("action", action), + slog.F("object", object), + ) + return false } return true diff --git a/coderd/coderd.go b/coderd/coderd.go index d059606b208b2..e13886a1c2c10 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -83,10 +83,6 @@ func New(options *Options) (http.Handler, func()) { // TODO: @emyrk we should just move this into 'ExtractAPIKey'. authRolesMiddleware := httpmw.ExtractUserRoles(options.Database) - authorize := func(f http.HandlerFunc, actions rbac.Action) http.HandlerFunc { - return httpmw.Authorize(api.Logger, api.Authorizer, actions)(f).ServeHTTP - } - r := chi.NewRouter() r.Use( @@ -158,10 +154,7 @@ func New(options *Options) (http.Handler, func()) { }) }) r.Route("/members", func(r chi.Router) { - r.Route("/roles", func(r chi.Router) { - r.Use(httpmw.WithRBACObject(rbac.ResourceUserRole)) - r.Get("/", authorize(api.assignableOrgRoles, rbac.ActionRead)) - }) + r.Get("/roles", api.assignableOrgRoles) r.Route("/{user}", func(r chi.Router) { r.Use( httpmw.ExtractUserParam(options.Database), @@ -232,8 +225,7 @@ func New(options *Options) (http.Handler, func()) { r.Get("/", api.users) // These routes query information about site wide roles. r.Route("/roles", func(r chi.Router) { - r.Use(httpmw.WithRBACObject(rbac.ResourceUserRole)) - r.Get("/", authorize(api.assignableSiteRoles, rbac.ActionRead)) + r.Get("/", api.assignableSiteRoles) }) r.Route("/{user}", func(r chi.Router) { r.Use(httpmw.ExtractUserParam(options.Database)) @@ -241,7 +233,7 @@ func New(options *Options) (http.Handler, func()) { r.Put("/profile", api.putUserProfile) r.Put("/suspend", api.putUserSuspend) r.Route("/password", func(r chi.Router) { - r.Put("/", authorize(api.putUserPassword, rbac.ActionUpdate)) + r.Put("/", api.putUserPassword) }) r.Get("/organizations", api.organizationsByUser) r.Post("/organizations", api.postOrganizationsByUser) diff --git a/coderd/roles.go b/coderd/roles.go index 31bf5875be052..81515254c6fea 100644 --- a/coderd/roles.go +++ b/coderd/roles.go @@ -14,6 +14,7 @@ import ( func (*api) assignableSiteRoles(rw http.ResponseWriter, _ *http.Request) { // TODO: @emyrk in the future, allow granular subsets of roles to be returned based on the // role of the user. + roles := rbac.SiteRoles() httpapi.Write(rw, http.StatusOK, convertRoles(roles)) } diff --git a/coderd/users.go b/coderd/users.go index 8745fd27fadc3..0af8977c19def 100644 --- a/coderd/users.go +++ b/coderd/users.go @@ -109,6 +109,11 @@ func (api *api) users(rw http.ResponseWriter, r *http.Request) { statusFilter = r.URL.Query().Get("status") ) + // Reading all users across the site + if !api.Authorize(rw, r, rbac.ActionRead, rbac.ResourceUser) { + return + } + paginationParams, ok := parsePagination(rw, r) if !ok { return @@ -175,6 +180,8 @@ func (api *api) postUser(rw http.ResponseWriter, r *http.Request) { return } + // TODO: @emyrk Authorize the organization create if the createUser will do that. + _, err := api.Database.GetUserByEmailOrUsername(r.Context(), database.GetUserByEmailOrUsernameParams{ Username: createUser.Username, Email: createUser.Email, @@ -253,6 +260,10 @@ 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.WithID(user.ID.String())) { + return + } + var params codersdk.UpdateUserProfileRequest if !httpapi.Read(rw, r, ¶ms) { return @@ -318,6 +329,10 @@ func (api *api) putUserProfile(rw http.ResponseWriter, r *http.Request) { func (api *api) putUserSuspend(rw http.ResponseWriter, r *http.Request) { user := httpmw.UserParam(r) + if !api.Authorize(rw, r, rbac.ActionUpdate, rbac.ResourceUser.WithID(user.ID.String())) { + return + } + suspendedUser, err := api.Database.UpdateUserStatus(r.Context(), database.UpdateUserStatusParams{ ID: user.ID, Status: database.UserStatusSuspended, @@ -351,6 +366,10 @@ func (api *api) putUserPassword(rw http.ResponseWriter, r *http.Request) { return } + if !api.Authorize(rw, r, rbac.ActionUpdate, rbac.ResourceUser.WithID(user.ID.String())) { + return + } + hashedPassword, err := userpassword.Hash(params.Password) if err != nil { httpapi.Write(rw, http.StatusInternalServerError, httpapi.Response{ From 32af1e6cde8c64c203b5a5182bae864febbd3936 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Mon, 16 May 2022 08:03:14 -0500 Subject: [PATCH 14/26] feat: Move all authorize calls into each handler --- coderd/coderd_test.go | 38 ++++++++++++-------- coderd/gitsshkey.go | 12 +++++++ coderd/organizations.go | 47 +++++++++++++++++++++--- coderd/rbac/object.go | 7 ++++ coderd/roles.go | 22 +++++++----- coderd/users.go | 79 +++++++++++++++++++++-------------------- 6 files changed, 140 insertions(+), 65 deletions(-) diff --git a/coderd/coderd_test.go b/coderd/coderd_test.go index 7ac5736dedf27..dd07f0360168c 100644 --- a/coderd/coderd_test.go +++ b/coderd/coderd_test.go @@ -42,6 +42,13 @@ func TestAuthorizeAllEndpoints(t *testing.T) { organization, err := client.Organization(context.Background(), admin.OrganizationID) require.NoError(t, err, "fetch org") + // Setup some data in the database. + coderdtest.NewProvisionerDaemon(t, client) + version := coderdtest.CreateTemplateVersion(t, client, admin.OrganizationID, nil) + coderdtest.AwaitTemplateVersionJob(t, client, version.ID) + template := coderdtest.CreateTemplate(t, client, admin.OrganizationID, version.ID) + coderdtest.CreateWorkspace(t, client, admin.OrganizationID, template.ID) + // Always fail auth from this point forward authorizer.AlwaysReturn = rbac.ForbiddenWithInternal(xerrors.New("fake implementation"), nil, nil) @@ -128,9 +135,10 @@ func TestAuthorizeAllEndpoints(t *testing.T) { "GET:/api/v2/files/{hash}": {NoAuthorize: true}, // These endpoints have more assertions. This is good, add more endpoints to assert if you can! - "GET:/api/v2/organizations/{organization}": {AssertObject: rbac.ResourceOrganization.InOrg(admin.OrganizationID)}, - "GET:/api/v2/users/{user}/organizations": {StatusCode: http.StatusOK, AssertObject: rbac.ResourceOrganization}, - "GET:/api/v2/users/{user}/workspaces": {StatusCode: http.StatusOK, AssertObject: rbac.ResourceWorkspace}, + "GET:/api/v2/organizations/{organization}": {AssertObject: rbac.ResourceOrganization.InOrg(admin.OrganizationID)}, + "GET:/api/v2/users/{user}/organizations": {StatusCode: http.StatusOK, AssertObject: rbac.ResourceOrganization}, + "GET:/api/v2/users/{user}/workspaces": {StatusCode: http.StatusOK, AssertObject: rbac.ResourceWorkspace}, + "GET:/api/v2/organizations/{organization}/workspaces/{user}": {StatusCode: http.StatusOK, AssertObject: rbac.ResourceWorkspace}, } c, _ := srv.Config.Handler.(*chi.Mux) @@ -159,17 +167,19 @@ func TestAuthorizeAllEndpoints(t *testing.T) { if !routeAssertions.NoAuthorize { assert.NotNil(t, authorizer.Called, "authorizer expected") assert.Equal(t, routeAssertions.StatusCode, resp.StatusCode, "expect unauthorized") - if routeAssertions.AssertObject.Type != "" { - assert.Equal(t, routeAssertions.AssertObject.Type, authorizer.Called.Object.Type, "resource type") - } - if routeAssertions.AssertObject.Owner != "" { - assert.Equal(t, routeAssertions.AssertObject.Owner, authorizer.Called.Object.Owner, "resource owner") - } - 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") + if authorizer.Called != nil { + if routeAssertions.AssertObject.Type != "" { + assert.Equal(t, routeAssertions.AssertObject.Type, authorizer.Called.Object.Type, "resource type") + } + if routeAssertions.AssertObject.Owner != "" { + assert.Equal(t, routeAssertions.AssertObject.Owner, authorizer.Called.Object.Owner, "resource owner") + } + 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/gitsshkey.go b/coderd/gitsshkey.go index 1543980ab6eb2..2c520486bbdf9 100644 --- a/coderd/gitsshkey.go +++ b/coderd/gitsshkey.go @@ -4,6 +4,8 @@ import ( "fmt" "net/http" + "github.com/coder/coder/coderd/rbac" + "github.com/coder/coder/coderd/database" "github.com/coder/coder/coderd/gitsshkey" "github.com/coder/coder/coderd/httpapi" @@ -13,6 +15,11 @@ import ( func (api *api) regenerateGitSSHKey(rw http.ResponseWriter, r *http.Request) { user := httpmw.UserParam(r) + + if !api.Authorize(rw, r, rbac.ActionUpdate, rbac.ResourceUserData.WithOwner(user.ID.String())) { + return + } + privateKey, publicKey, err := gitsshkey.Generate(api.SSHKeygenAlgorithm) if err != nil { httpapi.Write(rw, http.StatusInternalServerError, httpapi.Response{ @@ -53,6 +60,11 @@ func (api *api) regenerateGitSSHKey(rw http.ResponseWriter, r *http.Request) { func (api *api) gitSSHKey(rw http.ResponseWriter, r *http.Request) { user := httpmw.UserParam(r) + + if !api.Authorize(rw, r, rbac.ActionRead, rbac.ResourceUserData.WithOwner(user.ID.String())) { + return + } + gitSSHKey, err := api.Database.GetGitSSHKey(r.Context(), user.ID) if err != nil { httpapi.Write(rw, http.StatusInternalServerError, httpapi.Response{ diff --git a/coderd/organizations.go b/coderd/organizations.go index 43bdf5432d898..7dd7b395544ed 100644 --- a/coderd/organizations.go +++ b/coderd/organizations.go @@ -7,6 +7,8 @@ import ( "fmt" "net/http" + "github.com/coder/coder/coderd/rbac" + "github.com/go-chi/chi/v5" "github.com/google/uuid" "github.com/moby/moby/pkg/namesgenerator" @@ -18,8 +20,15 @@ import ( "github.com/coder/coder/codersdk" ) -func (*api) organization(rw http.ResponseWriter, r *http.Request) { +func (api *api) organization(rw http.ResponseWriter, r *http.Request) { organization := httpmw.OrganizationParam(r) + + if !api.Authorize(rw, r, rbac.ActionRead, rbac.ResourceOrganization. + InOrg(organization.ID). + WithID(organization.ID.String())) { + return + } + httpapi.Write(rw, http.StatusOK, convertOrganization(organization)) } @@ -327,6 +336,11 @@ func (api *api) templateByOrganizationAndName(rw http.ResponseWriter, r *http.Re func (api *api) workspacesByOrganization(rw http.ResponseWriter, r *http.Request) { organization := httpmw.OrganizationParam(r) + + if !api.Authorize(rw, r, rbac.ActionRead, rbac.ResourceWorkspace.InOrg(organization.ID)) { + return + } + workspaces, err := api.Database.GetWorkspacesByOrganizationID(r.Context(), database.GetWorkspacesByOrganizationIDParams{ OrganizationID: organization.ID, Deleted: false, @@ -352,6 +366,8 @@ func (api *api) workspacesByOrganization(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.GetWorkspacesByOwnerID(r.Context(), database.GetWorkspacesByOwnerIDParams{ OwnerID: owner.ID, }) @@ -364,7 +380,19 @@ func (api *api) workspacesByOwner(rw http.ResponseWriter, r *http.Request) { }) return } - apiWorkspaces, err := convertWorkspaces(r.Context(), api.Database, workspaces) + + allowed := make([]database.Workspace, 0) + for i := range workspaces { + w := workspaces[i] + err := api.Authorizer.ByRoleName(r.Context(), roles.ID.String(), roles.Roles, rbac.ActionRead, + rbac.ResourceWorkspace.InOrg(w.OrganizationID).WithOwner(w.OwnerID.String()).WithID(w.ID.String())) + + if err == nil { + allowed = append(allowed, w) + } + } + + apiWorkspaces, err := convertWorkspaces(r.Context(), api.Database, allowed) if err != nil { httpapi.Write(rw, http.StatusInternalServerError, httpapi.Response{ Message: fmt.Sprintf("convert workspaces: %s", err), @@ -379,6 +407,10 @@ func (api *api) workspaceByOwnerAndName(rw http.ResponseWriter, r *http.Request) organization := httpmw.OrganizationParam(r) workspaceName := chi.URLParam(r, "workspace") + if !api.Authorize(rw, r, rbac.ActionRead, rbac.ResourceWorkspace.InOrg(organization.ID).WithOwner(owner.ID.String())) { + return + } + workspace, err := api.Database.GetWorkspaceByOwnerIDAndName(r.Context(), database.GetWorkspaceByOwnerIDAndNameParams{ OwnerID: owner.ID, Name: workspaceName, @@ -431,11 +463,18 @@ func (api *api) workspaceByOwnerAndName(rw http.ResponseWriter, r *http.Request) // Create a new workspace for the currently authenticated user. func (api *api) postWorkspacesByOrganization(rw http.ResponseWriter, r *http.Request) { + organization := httpmw.OrganizationParam(r) + apiKey := httpmw.APIKey(r) + var createWorkspace codersdk.CreateWorkspaceRequest if !httpapi.Read(rw, r, &createWorkspace) { return } - apiKey := httpmw.APIKey(r) + + if !api.Authorize(rw, r, rbac.ActionCreate, rbac.ResourceWorkspace.InOrg(organization.ID).WithOwner(apiKey.UserID.String())) { + return + } + template, err := api.Database.GetTemplateByID(r.Context(), createWorkspace.TemplateID) if errors.Is(err, sql.ErrNoRows) { httpapi.Write(rw, http.StatusBadRequest, httpapi.Response{ @@ -453,7 +492,7 @@ func (api *api) postWorkspacesByOrganization(rw http.ResponseWriter, r *http.Req }) return } - organization := httpmw.OrganizationParam(r) + if organization.ID != template.OrganizationID { httpapi.Write(rw, http.StatusUnauthorized, httpapi.Response{ Message: fmt.Sprintf("template is not in organization %q", organization.Name), diff --git a/coderd/rbac/object.go b/coderd/rbac/object.go index f1a3ea6ff5954..e1391dec5b9dc 100644 --- a/coderd/rbac/object.go +++ b/coderd/rbac/object.go @@ -54,6 +54,7 @@ var ( } // ResourceUser is the user in the 'users' table. + // ResourceUser never has any owners or in an org, as it's site wide. // create/delete = make or delete a new user. // read = view all user's settings // update = update all user field & settings @@ -61,6 +62,12 @@ var ( Type: "user", } + // ResourceUserData is any data associated with a user. A user has control + // over their data (profile, password, etc). So this resource has an owner. + ResourceUserData = Object{ + Type: "user_data", + } + // ResourceOrganizationMember is a user's membership in an organization. // Has ONLY an organization owner. // create/delete = Create/delete member from org. diff --git a/coderd/roles.go b/coderd/roles.go index 81515254c6fea..b78dfb0fc2587 100644 --- a/coderd/roles.go +++ b/coderd/roles.go @@ -11,19 +11,28 @@ import ( ) // assignableSiteRoles returns all site wide roles that can be assigned. -func (*api) assignableSiteRoles(rw http.ResponseWriter, _ *http.Request) { +func (api *api) assignableSiteRoles(rw http.ResponseWriter, r *http.Request) { // TODO: @emyrk in the future, allow granular subsets of roles to be returned based on the // role of the user. + if !api.Authorize(rw, r, rbac.ActionRead, rbac.ResourceUserRole) { + return + } + roles := rbac.SiteRoles() httpapi.Write(rw, http.StatusOK, convertRoles(roles)) } // assignableSiteRoles returns all site wide roles that can be assigned. -func (*api) assignableOrgRoles(rw http.ResponseWriter, r *http.Request) { +func (api *api) assignableOrgRoles(rw http.ResponseWriter, r *http.Request) { // TODO: @emyrk in the future, allow granular subsets of roles to be returned based on the // role of the user. organization := httpmw.OrganizationParam(r) + + if !api.Authorize(rw, r, rbac.ActionRead, rbac.ResourceUserRole.InOrg(organization.ID)) { + return + } + roles := rbac.OrganizationRoles(organization.ID) httpapi.Write(rw, http.StatusOK, convertRoles(roles)) } @@ -31,13 +40,8 @@ func (*api) assignableOrgRoles(rw http.ResponseWriter, r *http.Request) { func (api *api) checkPermissions(rw http.ResponseWriter, r *http.Request) { roles := httpmw.UserRoles(r) user := httpmw.UserParam(r) - if user.ID != roles.ID { - httpapi.Write(rw, http.StatusBadRequest, httpapi.Response{ - // TODO: @Emyrk in the future we could have an rbac check here. - // If the user can masquerade/impersonate as the user passed in, - // we could allow this or something like that. - Message: "only allowed to check permissions on yourself", - }) + + if !api.Authorize(rw, r, rbac.ActionRead, rbac.ResourceUserData.WithOwner(user.ID.String())) { return } diff --git a/coderd/users.go b/coderd/users.go index 0af8977c19def..fd0ab280c7274 100644 --- a/coderd/users.go +++ b/coderd/users.go @@ -164,13 +164,13 @@ func (api *api) users(rw http.ResponseWriter, r *http.Request) { func (api *api) postUser(rw http.ResponseWriter, r *http.Request) { apiKey := httpmw.APIKey(r) - var createUser codersdk.CreateUserRequest - if !httpapi.Read(rw, r, &createUser) { + // Create the user on the site + if !api.Authorize(rw, r, rbac.ActionCreate, rbac.ResourceUser) { return } - // Create the user on the site - if !api.Authorize(rw, r, rbac.ActionCreate, rbac.ResourceUser) { + var createUser codersdk.CreateUserRequest + if !httpapi.Read(rw, r, &createUser) { return } @@ -247,6 +247,10 @@ func (api *api) userByName(rw http.ResponseWriter, r *http.Request) { user := httpmw.UserParam(r) organizationIDs, err := userOrganizationIDs(r.Context(), api, user) + if !api.Authorize(rw, r, rbac.ActionRead, rbac.ResourceUser.WithID(user.ID.String())) { + return + } + if err != nil { httpapi.Write(rw, http.StatusInternalServerError, httpapi.Response{ Message: fmt.Sprintf("get organization IDs: %s", err.Error()), @@ -260,7 +264,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.WithID(user.ID.String())) { + if !api.Authorize(rw, r, rbac.ActionUpdate, rbac.ResourceUserData.WithOwner(user.ID.String())) { return } @@ -329,7 +333,7 @@ func (api *api) putUserProfile(rw http.ResponseWriter, r *http.Request) { func (api *api) putUserSuspend(rw http.ResponseWriter, r *http.Request) { user := httpmw.UserParam(r) - if !api.Authorize(rw, r, rbac.ActionUpdate, rbac.ResourceUser.WithID(user.ID.String())) { + if !api.Authorize(rw, r, rbac.ActionDelete, rbac.ResourceUser.WithID(user.ID.String())) { return } @@ -362,11 +366,12 @@ func (api *api) putUserPassword(rw http.ResponseWriter, r *http.Request) { user = httpmw.UserParam(r) params codersdk.UpdateUserPasswordRequest ) - if !httpapi.Read(rw, r, ¶ms) { + + if !api.Authorize(rw, r, rbac.ActionUpdate, rbac.ResourceUserData.WithOwner(user.ID.String())) { return } - if !api.Authorize(rw, r, rbac.ActionUpdate, rbac.ResourceUser.WithID(user.ID.String())) { + if !httpapi.Read(rw, r, ¶ms) { return } @@ -393,6 +398,12 @@ 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())) { + return + } resp := codersdk.UserRoles{ Roles: user.RBACRoles, @@ -408,7 +419,16 @@ func (api *api) userRoles(rw http.ResponseWriter, r *http.Request) { } for _, mem := range memberships { - resp.OrganizationRoles[mem.OrganizationID] = mem.Roles + err := api.Authorizer.ByRoleName(r.Context(), roles.ID.String(), roles.Roles, rbac.ActionRead, + rbac.ResourceOrganizationMember. + WithID(user.ID.String()). + InOrg(mem.OrganizationID), + ) + + // If we can read the org member, include the roles + if err == nil { + resp.OrganizationRoles[mem.OrganizationID] = mem.Roles + } } httpapi.Write(rw, http.StatusOK, resp) @@ -419,6 +439,10 @@ func (api *api) putUserRoles(rw http.ResponseWriter, r *http.Request) { user := httpmw.UserParam(r) roles := httpmw.UserRoles(r) + if !api.Authorize(rw, r, rbac.ActionUpdate, rbac.ResourceUserData.WithOwner(user.ID.String())) { + return + } + var params codersdk.UpdateRoles if !httpapi.Read(rw, r, ¶ms) { return @@ -438,27 +462,15 @@ func (api *api) putUserRoles(rw http.ResponseWriter, r *http.Request) { continue } - // Assigning a role requires the create permission. The middleware checks if - // we can update this user, so the combination of the 2 permissions enables - // assigning new roles. - err := api.Authorizer.ByRoleName(r.Context(), roles.ID.String(), roles.Roles, - rbac.ActionCreate, rbac.ResourceUserRole.WithID(roleName)) - if err != nil { - httpapi.Write(rw, http.StatusUnauthorized, httpapi.Response{ - Message: "unauthorized", - }) + // Assigning a role requires the create permission. + if !api.Authorize(rw, r, rbac.ActionCreate, rbac.ResourceUserRole.WithID(roleName)) { return } } // Any roles that were removed also need to be checked. for roleName := range has { - err := api.Authorizer.ByRoleName(r.Context(), roles.ID.String(), roles.Roles, - rbac.ActionDelete, rbac.ResourceUserRole.WithID(roleName)) - if err != nil { - httpapi.Write(rw, http.StatusUnauthorized, httpapi.Response{ - Message: "unauthorized", - }) + if !api.Authorize(rw, r, rbac.ActionDelete, rbac.ResourceUserRole.WithID(roleName)) { return } } @@ -485,6 +497,8 @@ func (api *api) putUserRoles(rw http.ResponseWriter, r *http.Request) { httpapi.Write(rw, http.StatusOK, convertUser(updatedUser, organizationIDs)) } +// updateSiteUserRoles will ensure only site wide roles are passed in as arguments. +// If an organization role is included, an error is returned. func (api *api) updateSiteUserRoles(ctx context.Context, args database.UpdateUserRolesParams) (database.User, error) { // Enforce only site wide roles for _, r := range args.GrantedRoles { @@ -538,8 +552,6 @@ func (api *api) organizationsByUser(rw http.ResponseWriter, r *http.Request) { } func (api *api) organizationByUserAndName(rw http.ResponseWriter, r *http.Request) { - roles := httpmw.UserRoles(r) - organizationName := chi.URLParam(r, "organizationname") organization, err := api.Database.GetOrganizationByName(r.Context(), organizationName) if errors.Is(err, sql.ErrNoRows) { @@ -557,15 +569,10 @@ func (api *api) organizationByUserAndName(rw http.ResponseWriter, r *http.Reques return } - err = api.Authorizer.ByRoleName(r.Context(), roles.ID.String(), roles.Roles, rbac.ActionRead, + if !api.Authorize(rw, r, rbac.ActionRead, rbac.ResourceOrganization. InOrg(organization.ID). - WithID(organization.ID.String()), - ) - if err != nil { - httpapi.Write(rw, http.StatusUnauthorized, httpapi.Response{ - Message: err.Error(), - }) + WithID(organization.ID.String())) { return } @@ -678,12 +685,8 @@ func (api *api) postLogin(rw http.ResponseWriter, r *http.Request) { // Creates a new session key, used for logging in via the CLI func (api *api) postAPIKey(rw http.ResponseWriter, r *http.Request) { user := httpmw.UserParam(r) - apiKey := httpmw.APIKey(r) - if user.ID != apiKey.UserID { - httpapi.Write(rw, http.StatusUnauthorized, httpapi.Response{ - Message: "Keys can only be generated for the authenticated user", - }) + if !api.Authorize(rw, r, rbac.ActionCreate, rbac.ResourceAPIKey.WithOwner(user.ID.String())) { return } From 28a099fab5e9f0f51672e44d48e0d4712f39d717 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Mon, 16 May 2022 08:06:15 -0500 Subject: [PATCH 15/26] Delete unused code --- coderd/gitsshkey.go | 3 +- coderd/httpmw/authorize.go | 132 ------------------------------------- coderd/organizations.go | 3 +- 3 files changed, 2 insertions(+), 136 deletions(-) diff --git a/coderd/gitsshkey.go b/coderd/gitsshkey.go index 2c520486bbdf9..d5b2b049f892c 100644 --- a/coderd/gitsshkey.go +++ b/coderd/gitsshkey.go @@ -4,12 +4,11 @@ import ( "fmt" "net/http" - "github.com/coder/coder/coderd/rbac" - "github.com/coder/coder/coderd/database" "github.com/coder/coder/coderd/gitsshkey" "github.com/coder/coder/coderd/httpapi" "github.com/coder/coder/coderd/httpmw" + "github.com/coder/coder/coderd/rbac" "github.com/coder/coder/codersdk" ) diff --git a/coderd/httpmw/authorize.go b/coderd/httpmw/authorize.go index 44fdb8a2b0e04..84bf7cbfa04b4 100644 --- a/coderd/httpmw/authorize.go +++ b/coderd/httpmw/authorize.go @@ -4,142 +4,10 @@ import ( "context" "net/http" - "github.com/google/uuid" - "golang.org/x/xerrors" - - "cdr.dev/slog" "github.com/coder/coder/coderd/database" "github.com/coder/coder/coderd/httpapi" - "github.com/coder/coder/coderd/rbac" ) -// Authorize will enforce if the user roles can complete the action on the RBACObject. -// The organization and owner are found using the ExtractOrganization and -// ExtractUser middleware if present. -func Authorize(logger slog.Logger, auth rbac.Authorizer, actions ...rbac.Action) func(http.Handler) http.Handler { - return func(next http.Handler) http.Handler { - return http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { - roles := UserRoles(r) - authObject := rbacObject(r) - object := authObject.Object - - if object.Type == "" { - panic("developer error: auth object has no type") - } - - // First extract the object's owner and organization if present. - unknownOrg := r.Context().Value(organizationParamContextKey{}) - if organization, castOK := unknownOrg.(database.Organization); unknownOrg != nil { - if !castOK { - panic("developer error: organization param middleware not provided for authorize") - } - object = object.InOrg(organization.ID) - } - - if authObject.WithOwner != nil { - owner := authObject.WithOwner(r) - object = object.WithOwner(owner.String()) - } else { - // Attempt to find the resource owner id - unknownOwner := r.Context().Value(userParamContextKey{}) - if owner, castOK := unknownOwner.(database.User); unknownOwner != nil { - if !castOK { - panic("developer error: user param middleware not provided for authorize") - } - object = object.WithOwner(owner.ID.String()) - } - } - - for _, action := range actions { - err := auth.ByRoleName(r.Context(), roles.ID.String(), roles.Roles, action, object) - if err != nil { - internalError := new(rbac.UnauthorizedError) - if xerrors.As(err, internalError) { - logger = logger.With(slog.F("internal", internalError.Internal())) - } - // Log information for debugging. This will be very helpful - // in the early days if we over secure endpoints. - logger.Warn(r.Context(), "unauthorized", - slog.F("roles", roles.Roles), - slog.F("user_id", roles.ID), - slog.F("username", roles.Username), - slog.F("route", r.URL.Path), - slog.F("action", action), - slog.F("object", object), - ) - httpapi.Write(rw, http.StatusUnauthorized, httpapi.Response{ - Message: err.Error(), - }) - return - } - } - next.ServeHTTP(rw, r) - }) - } -} - -type authObjectKey struct{} - -type RBACObject struct { - Object rbac.Object - - // WithOwner will set the Object.Owner field based on the request. - // This allows the Owner field to be set dynamically based on the context - // of the request. - WithOwner func(r *http.Request) uuid.UUID -} - -func rbacObject(r *http.Request) RBACObject { - obj, ok := r.Context().Value(authObjectKey{}).(RBACObject) - if !ok { - panic("developer error: auth object middleware not provided") - } - return obj -} - -func WithAPIKeyAsOwner() func(http.Handler) http.Handler { - return WithOwner(func(r *http.Request) uuid.UUID { - key := APIKey(r) - return key.UserID - }) -} - -// WithOwner sets the object owner for 'Authorize()' for all routes handled -// by this middleware. -func WithOwner(withOwner func(r *http.Request) uuid.UUID) func(http.Handler) http.Handler { - return func(next http.Handler) http.Handler { - return http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { - obj, ok := r.Context().Value(authObjectKey{}).(RBACObject) - if ok { - obj.WithOwner = withOwner - } else { - obj = RBACObject{WithOwner: withOwner} - } - - ctx := context.WithValue(r.Context(), authObjectKey{}, obj) - next.ServeHTTP(rw, r.WithContext(ctx)) - }) - } -} - -// WithRBACObject sets the object for 'Authorize()' for all routes handled -// by this middleware. The important field to set is 'Type' -func WithRBACObject(object rbac.Object) func(http.Handler) http.Handler { - return func(next http.Handler) http.Handler { - return http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { - obj, ok := r.Context().Value(authObjectKey{}).(RBACObject) - if ok { - obj.Object = object - } else { - obj = RBACObject{Object: object} - } - - ctx := context.WithValue(r.Context(), authObjectKey{}, obj) - next.ServeHTTP(rw, r.WithContext(ctx)) - }) - } -} - // User roles are the 'subject' field of Authorize() type userRolesKey struct{} diff --git a/coderd/organizations.go b/coderd/organizations.go index 7dd7b395544ed..8b9f2c7d6df67 100644 --- a/coderd/organizations.go +++ b/coderd/organizations.go @@ -7,8 +7,6 @@ import ( "fmt" "net/http" - "github.com/coder/coder/coderd/rbac" - "github.com/go-chi/chi/v5" "github.com/google/uuid" "github.com/moby/moby/pkg/namesgenerator" @@ -17,6 +15,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" ) From ff7bd81794a2d1eb7d2d3e228742c445146fdd96 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Mon, 16 May 2022 08:39:35 -0500 Subject: [PATCH 16/26] feat: Add some perms to users --- coderd/rbac/builtin.go | 8 +++++++- coderd/users.go | 21 +-------------------- coderd/users_test.go | 6 ++++-- codersdk/roles.go | 2 +- 4 files changed, 13 insertions(+), 24 deletions(-) diff --git a/coderd/rbac/builtin.go b/coderd/rbac/builtin.go index a314addab3afb..86bf84c402bf0 100644 --- a/coderd/rbac/builtin.go +++ b/coderd/rbac/builtin.go @@ -64,7 +64,11 @@ var ( return Role{ Name: member, DisplayName: "Member", - Site: permissions(map[Object][]Action{}), + Site: permissions(map[Object][]Action{ + // TODO: @emyrk in EE we should restrict this to only certain fields. + // All users can read all other users and know they exist. + ResourceUser: {ActionRead}, + }), User: permissions(map[Object][]Action{ ResourceWildcard: {WildcardSymbol}, }), @@ -117,11 +121,13 @@ 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: "*", }, }, }, diff --git a/coderd/users.go b/coderd/users.go index fd0ab280c7274..5850bb9b1cd4d 100644 --- a/coderd/users.go +++ b/coderd/users.go @@ -162,8 +162,6 @@ func (api *api) users(rw http.ResponseWriter, r *http.Request) { // Creates a new user. func (api *api) postUser(rw http.ResponseWriter, r *http.Request) { - apiKey := httpmw.APIKey(r) - // Create the user on the site if !api.Authorize(rw, r, rbac.ActionCreate, rbac.ResourceUser) { return @@ -199,7 +197,7 @@ func (api *api) postUser(rw http.ResponseWriter, r *http.Request) { return } - organization, err := api.Database.GetOrganizationByID(r.Context(), createUser.OrganizationID) + _, err = api.Database.GetOrganizationByID(r.Context(), createUser.OrganizationID) if errors.Is(err, sql.ErrNoRows) { httpapi.Write(rw, http.StatusNotFound, httpapi.Response{ Message: "organization does not exist with the provided id", @@ -212,23 +210,6 @@ func (api *api) postUser(rw http.ResponseWriter, r *http.Request) { }) return } - // Check if the caller has permissions to the organization requested. - _, err = api.Database.GetOrganizationMemberByUserID(r.Context(), database.GetOrganizationMemberByUserIDParams{ - OrganizationID: organization.ID, - UserID: apiKey.UserID, - }) - if errors.Is(err, sql.ErrNoRows) { - httpapi.Write(rw, http.StatusUnauthorized, httpapi.Response{ - Message: "you are not authorized to add members to that organization", - }) - return - } - if err != nil { - httpapi.Write(rw, http.StatusInternalServerError, httpapi.Response{ - Message: fmt.Sprintf("get organization member: %s", err), - }) - return - } user, _, err := api.createUser(r.Context(), createUser) if err != nil { diff --git a/coderd/users_test.go b/coderd/users_test.go index a9fd04b123897..2b02ad7b0e48c 100644 --- a/coderd/users_test.go +++ b/coderd/users_test.go @@ -172,13 +172,14 @@ func TestPostUsers(t *testing.T) { t.Parallel() client := coderdtest.New(t, nil) first := coderdtest.CreateFirstUser(t, client) + notInOrg := coderdtest.CreateAnotherUser(t, client, first.OrganizationID) other := coderdtest.CreateAnotherUser(t, client, first.OrganizationID) org, err := other.CreateOrganization(context.Background(), codersdk.Me, codersdk.CreateOrganizationRequest{ Name: "another", }) require.NoError(t, err) - _, err = client.CreateUser(context.Background(), codersdk.CreateUserRequest{ + _, err = notInOrg.CreateUser(context.Background(), codersdk.CreateUserRequest{ Email: "some@domain.com", Username: "anotheruser", Password: "testing", @@ -582,7 +583,8 @@ func TestOrganizationByUserAndName(t *testing.T) { _, err := client.OrganizationByName(context.Background(), codersdk.Me, "nothing") var apiErr *codersdk.Error require.ErrorAs(t, err, &apiErr) - require.Equal(t, http.StatusNotFound, apiErr.StatusCode()) + // Returns unauthorized to not leak if the org exists or not + require.Equal(t, http.StatusUnauthorized, apiErr.StatusCode()) }) t.Run("NoMember", func(t *testing.T) { diff --git a/codersdk/roles.go b/codersdk/roles.go index dbbf8e7e53555..c9f916733e4eb 100644 --- a/codersdk/roles.go +++ b/codersdk/roles.go @@ -32,7 +32,7 @@ func (c *Client) ListSiteRoles(ctx context.Context) ([]Role, error) { // ListOrganizationRoles lists all available roles for a given organization. // This is not user specific. func (c *Client) ListOrganizationRoles(ctx context.Context, org uuid.UUID) ([]Role, error) { - res, err := c.Request(ctx, http.MethodGet, fmt.Sprintf("/api/v2/organizations/%s/members/roles/", org.String()), nil) + res, err := c.Request(ctx, http.MethodGet, fmt.Sprintf("/api/v2/organizations/%s/members/roles", org.String()), nil) if err != nil { return nil, err } From d123b9fafe42ebe65b19c82cc4cefd8ec09de4de Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Mon, 16 May 2022 11:02:25 -0500 Subject: [PATCH 17/26] Drop comment --- coderd/rbac/builtin.go | 1 - 1 file changed, 1 deletion(-) diff --git a/coderd/rbac/builtin.go b/coderd/rbac/builtin.go index 86bf84c402bf0..71b0f059f628d 100644 --- a/coderd/rbac/builtin.go +++ b/coderd/rbac/builtin.go @@ -65,7 +65,6 @@ var ( Name: member, DisplayName: "Member", Site: permissions(map[Object][]Action{ - // TODO: @emyrk in EE we should restrict this to only certain fields. // All users can read all other users and know they exist. ResourceUser: {ActionRead}, }), From 186eb5f4ed440a18221642a272ec0af9409e1bc5 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Mon, 16 May 2022 14:24:08 -0500 Subject: [PATCH 18/26] Fix 401 -> 403 Fix comments --- coderd/authorize.go | 2 +- coderd/httpapi/httpapi.go | 6 ++++++ coderd/organizations.go | 6 +++--- coderd/rbac/error.go | 2 +- coderd/rbac/object.go | 6 +++--- coderd/roles.go | 10 ++++++++-- coderd/users.go | 8 ++------ 7 files changed, 24 insertions(+), 16 deletions(-) diff --git a/coderd/authorize.go b/coderd/authorize.go index f893b1a776614..9823408d6e290 100644 --- a/coderd/authorize.go +++ b/coderd/authorize.go @@ -15,7 +15,7 @@ func (api *api) Authorize(rw http.ResponseWriter, r *http.Request, action rbac.A roles := httpmw.UserRoles(r) err := api.Authorizer.ByRoleName(r.Context(), roles.ID.String(), roles.Roles, action, object) if err != nil { - httpapi.Write(rw, http.StatusUnauthorized, httpapi.Response{ + httpapi.Write(rw, http.StatusForbidden, httpapi.Response{ Message: err.Error(), }) diff --git a/coderd/httpapi/httpapi.go b/coderd/httpapi/httpapi.go index 331ec527e4328..afdb4685ed063 100644 --- a/coderd/httpapi/httpapi.go +++ b/coderd/httpapi/httpapi.go @@ -62,6 +62,12 @@ type Error struct { Detail string `json:"detail" validate:"required"` } +func Forbidden(rw http.ResponseWriter) { + Write(rw, http.StatusForbidden, Response{ + Message: "forbidden", + }) +} + // Write outputs a standardized format to an HTTP response body. func Write(rw http.ResponseWriter, status int, response interface{}) { buf := &bytes.Buffer{} diff --git a/coderd/organizations.go b/coderd/organizations.go index 8b9f2c7d6df67..2a43790cbbf59 100644 --- a/coderd/organizations.go +++ b/coderd/organizations.go @@ -428,7 +428,7 @@ func (api *api) workspaceByOwnerAndName(rw http.ResponseWriter, r *http.Request) } if workspace.OrganizationID != organization.ID { - httpapi.Write(rw, http.StatusUnauthorized, httpapi.Response{ + httpapi.Write(rw, http.StatusForbidden, httpapi.Response{ Message: fmt.Sprintf("workspace is not owned by organization %q", organization.Name), }) return @@ -493,7 +493,7 @@ func (api *api) postWorkspacesByOrganization(rw http.ResponseWriter, r *http.Req } if organization.ID != template.OrganizationID { - httpapi.Write(rw, http.StatusUnauthorized, httpapi.Response{ + httpapi.Write(rw, http.StatusForbidden, httpapi.Response{ Message: fmt.Sprintf("template is not in organization %q", organization.Name), }) return @@ -503,7 +503,7 @@ func (api *api) postWorkspacesByOrganization(rw http.ResponseWriter, r *http.Req UserID: apiKey.UserID, }) if errors.Is(err, sql.ErrNoRows) { - httpapi.Write(rw, http.StatusUnauthorized, httpapi.Response{ + httpapi.Write(rw, http.StatusForbidden, httpapi.Response{ Message: "you aren't allowed to access templates in that organization", }) return diff --git a/coderd/rbac/error.go b/coderd/rbac/error.go index 593ca4d0fc23a..6b63bb88602db 100644 --- a/coderd/rbac/error.go +++ b/coderd/rbac/error.go @@ -6,7 +6,7 @@ const ( // errUnauthorized is the error message that should be returned to // clients when an action is forbidden. It is intentionally vague to prevent // disclosing information that a client should not have access to. - errUnauthorized = "unauthorized" + errUnauthorized = "forbidden" ) // UnauthorizedError is the error type for authorization errors diff --git a/coderd/rbac/object.go b/coderd/rbac/object.go index e1391dec5b9dc..9ae04c3f0c459 100644 --- a/coderd/rbac/object.go +++ b/coderd/rbac/object.go @@ -25,7 +25,7 @@ var ( Type: "file", } - // ResourceOrganization CRUD. Always has an org owner. + // ResourceOrganization CRUD. Has an org owner on all but 'create'. // create/delete = make or delete organizations // read = view org information (Can add user owner for read) // update = ?? @@ -56,8 +56,8 @@ var ( // ResourceUser is the user in the 'users' table. // ResourceUser never has any owners or in an org, as it's site wide. // create/delete = make or delete a new user. - // read = view all user's settings - // update = update all user field & settings + // read = view all 'user' table data + // update = update all 'user' table data ResourceUser = Object{ Type: "user", } diff --git a/coderd/roles.go b/coderd/roles.go index b78dfb0fc2587..897cf450ce517 100644 --- a/coderd/roles.go +++ b/coderd/roles.go @@ -38,10 +38,16 @@ func (api *api) assignableOrgRoles(rw http.ResponseWriter, r *http.Request) { } func (api *api) checkPermissions(rw http.ResponseWriter, r *http.Request) { - roles := httpmw.UserRoles(r) user := httpmw.UserParam(r) - if !api.Authorize(rw, r, rbac.ActionRead, rbac.ResourceUserData.WithOwner(user.ID.String())) { + if !api.Authorize(rw, r, rbac.ActionRead, rbac.ResourceUser.WithOwner(user.ID.String())) { + return + } + + // use the roles of the user specified, not the person making the request. + roles, err := api.Database.GetAllUserRoles(r.Context(), user.ID) + if err != nil { + httpapi.Forbidden(rw) return } diff --git a/coderd/users.go b/coderd/users.go index 5850bb9b1cd4d..35f75c953bdce 100644 --- a/coderd/users.go +++ b/coderd/users.go @@ -538,15 +538,11 @@ func (api *api) organizationByUserAndName(rw http.ResponseWriter, r *http.Reques if errors.Is(err, sql.ErrNoRows) { // Return unauthorized rather than a 404 to not leak if the organization // exists. - httpapi.Write(rw, http.StatusUnauthorized, httpapi.Response{ - Message: "unauthorized", - }) + httpapi.Forbidden(rw) return } if err != nil { - httpapi.Write(rw, http.StatusInternalServerError, httpapi.Response{ - Message: fmt.Sprintf("get organization by name: %s", err), - }) + httpapi.Forbidden(rw) return } From 5d32d9d97401df1c36c5f321eb9bfb9b12e21312 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Mon, 16 May 2022 14:26:45 -0500 Subject: [PATCH 19/26] Fix using User over UserData --- coderd/rbac/object.go | 2 +- coderd/users.go | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/coderd/rbac/object.go b/coderd/rbac/object.go index 9ae04c3f0c459..d6a4a1528b1bc 100644 --- a/coderd/rbac/object.go +++ b/coderd/rbac/object.go @@ -69,7 +69,7 @@ var ( } // ResourceOrganizationMember is a user's membership in an organization. - // Has ONLY an organization owner. + // Has ONLY an organization owner. The resource ID is the user's ID // create/delete = Create/delete member from org. // update = Update organization member // read = View member diff --git a/coderd/users.go b/coderd/users.go index 35f75c953bdce..3956a08a9cef8 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.ResourceUserData.WithOwner(user.ID.String())) { + if !api.Authorize(rw, r, rbac.ActionUpdate, rbac.ResourceUser.WithOwner(user.ID.String())) { return } @@ -420,7 +420,7 @@ func (api *api) putUserRoles(rw http.ResponseWriter, r *http.Request) { user := httpmw.UserParam(r) roles := httpmw.UserRoles(r) - if !api.Authorize(rw, r, rbac.ActionUpdate, rbac.ResourceUserData.WithOwner(user.ID.String())) { + if !api.Authorize(rw, r, rbac.ActionUpdate, rbac.ResourceUser.WithOwner(user.ID.String())) { return } From 301d42a38119bb3b87f84598ecb97337be33a8d2 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Mon, 16 May 2022 14:29:42 -0500 Subject: [PATCH 20/26] Rename UserRole to RoleAssignment --- coderd/rbac/object.go | 6 +++--- coderd/roles.go | 4 ++-- coderd/users.go | 4 ++-- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/coderd/rbac/object.go b/coderd/rbac/object.go index d6a4a1528b1bc..862653f50286e 100644 --- a/coderd/rbac/object.go +++ b/coderd/rbac/object.go @@ -33,15 +33,15 @@ var ( Type: "organization", } - // ResourceUserRole might be expanded later to allow more granular permissions + // ResourceRoleAssignment might be expanded later to allow more granular permissions // to modifying roles. For now, this covers all possible roles, so having this permission // allows granting/deleting **ALL** roles. // create = Assign roles // update = ?? // read = View available roles to assign // delete = Remove role - ResourceUserRole = Object{ - Type: "user_role", + ResourceRoleAssignment = Object{ + Type: "assign_role", } // ResourceAPIKey is owned by a user. diff --git a/coderd/roles.go b/coderd/roles.go index 897cf450ce517..308b1bf791984 100644 --- a/coderd/roles.go +++ b/coderd/roles.go @@ -15,7 +15,7 @@ func (api *api) assignableSiteRoles(rw http.ResponseWriter, r *http.Request) { // TODO: @emyrk in the future, allow granular subsets of roles to be returned based on the // role of the user. - if !api.Authorize(rw, r, rbac.ActionRead, rbac.ResourceUserRole) { + if !api.Authorize(rw, r, rbac.ActionRead, rbac.ResourceRoleAssignment) { return } @@ -29,7 +29,7 @@ func (api *api) assignableOrgRoles(rw http.ResponseWriter, r *http.Request) { // role of the user. organization := httpmw.OrganizationParam(r) - if !api.Authorize(rw, r, rbac.ActionRead, rbac.ResourceUserRole.InOrg(organization.ID)) { + if !api.Authorize(rw, r, rbac.ActionRead, rbac.ResourceRoleAssignment.InOrg(organization.ID)) { return } diff --git a/coderd/users.go b/coderd/users.go index 3956a08a9cef8..fc79db1c92b48 100644 --- a/coderd/users.go +++ b/coderd/users.go @@ -444,14 +444,14 @@ func (api *api) putUserRoles(rw http.ResponseWriter, r *http.Request) { } // Assigning a role requires the create permission. - if !api.Authorize(rw, r, rbac.ActionCreate, rbac.ResourceUserRole.WithID(roleName)) { + if !api.Authorize(rw, r, rbac.ActionCreate, rbac.ResourceRoleAssignment.WithID(roleName)) { return } } // Any roles that were removed also need to be checked. for roleName := range has { - if !api.Authorize(rw, r, rbac.ActionDelete, rbac.ResourceUserRole.WithID(roleName)) { + if !api.Authorize(rw, r, rbac.ActionDelete, rbac.ResourceRoleAssignment.WithID(roleName)) { return } } From a989224ae32bbf6720a71eba6e65fd7081cf9579 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Mon, 16 May 2022 14:33:03 -0500 Subject: [PATCH 21/26] Refactor workspacesByUser --- coderd/users.go | 46 +++++----------------------------------------- 1 file changed, 5 insertions(+), 41 deletions(-) diff --git a/coderd/users.go b/coderd/users.go index fc79db1c92b48..3bdafdf93cc05 100644 --- a/coderd/users.go +++ b/coderd/users.go @@ -826,47 +826,11 @@ func (api *api) createUser(ctx context.Context, req codersdk.CreateUserRequest) }) } -// func (api *api) workspacesByUser(rw http.ResponseWriter, r *http.Request) { user := httpmw.UserParam(r) roles := httpmw.UserRoles(r) - organizations, err := api.Database.GetOrganizationsByUserID(r.Context(), user.ID) - if err != nil { - httpapi.Write(rw, http.StatusInternalServerError, httpapi.Response{ - Message: fmt.Sprintf("get organizations: %s", err), - }) - return - } - organizationIDs := make([]uuid.UUID, 0) - for _, organization := range organizations { - err = api.Authorizer.ByRoleName(r.Context(), user.ID.String(), roles.Roles, rbac.ActionRead, rbac.ResourceWorkspace.All().InOrg(organization.ID)) - var apiErr *rbac.UnauthorizedError - if xerrors.As(err, &apiErr) { - continue - } - if err != nil { - httpapi.Write(rw, http.StatusInternalServerError, httpapi.Response{ - Message: fmt.Sprintf("authorize: %s", err), - }) - return - } - organizationIDs = append(organizationIDs, organization.ID) - } - - workspaceIDs := map[uuid.UUID]struct{}{} - allWorkspaces, err := api.Database.GetWorkspacesByOrganizationIDs(r.Context(), database.GetWorkspacesByOrganizationIDsParams{ - Ids: organizationIDs, - }) - if err != nil { - httpapi.Write(rw, http.StatusInternalServerError, httpapi.Response{ - Message: fmt.Sprintf("get workspaces for organizations: %s", err), - }) - return - } - for _, ws := range allWorkspaces { - workspaceIDs[ws.ID] = struct{}{} - } + allWorkspaces := make([]database.Workspace, 0) userWorkspaces, err := api.Database.GetWorkspacesByOwnerID(r.Context(), database.GetWorkspacesByOwnerIDParams{ OwnerID: user.ID, }) @@ -877,11 +841,11 @@ func (api *api) workspacesByUser(rw http.ResponseWriter, r *http.Request) { return } for _, ws := range userWorkspaces { - _, exists := workspaceIDs[ws.ID] - if exists { - continue + err = api.Authorizer.ByRoleName(r.Context(), user.ID.String(), roles.Roles, rbac.ActionRead, + rbac.ResourceWorkspace.InOrg(ws.OrganizationID).WithOwner(ws.OwnerID.String()).WithID(ws.ID.String())) + if err == nil { + allWorkspaces = append(allWorkspaces, ws) } - allWorkspaces = append(allWorkspaces, ws) } apiWorkspaces, err := convertWorkspaces(r.Context(), api.Database, allWorkspaces) From 1047391501c00e2d48e63eb43e01507438571faf Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Mon, 16 May 2022 18:35:47 -0500 Subject: [PATCH 22/26] Fix some routes --- coderd/coderd_test.go | 9 +++++++-- coderd/workspaces.go | 44 ++++++++++++++++++++++++++++++++++++++----- 2 files changed, 46 insertions(+), 7 deletions(-) diff --git a/coderd/coderd_test.go b/coderd/coderd_test.go index dd07f0360168c..826aee5c04fbe 100644 --- a/coderd/coderd_test.go +++ b/coderd/coderd_test.go @@ -47,7 +47,7 @@ func TestAuthorizeAllEndpoints(t *testing.T) { version := coderdtest.CreateTemplateVersion(t, client, admin.OrganizationID, nil) coderdtest.AwaitTemplateVersionJob(t, client, version.ID) template := coderdtest.CreateTemplate(t, client, admin.OrganizationID, version.ID) - coderdtest.CreateWorkspace(t, client, admin.OrganizationID, template.ID) + workspace := coderdtest.CreateWorkspace(t, client, admin.OrganizationID, template.ID) // Always fail auth from this point forward authorizer.AlwaysReturn = rbac.ForbiddenWithInternal(xerrors.New("fake implementation"), nil, nil) @@ -139,6 +139,10 @@ func TestAuthorizeAllEndpoints(t *testing.T) { "GET:/api/v2/users/{user}/organizations": {StatusCode: http.StatusOK, AssertObject: rbac.ResourceOrganization}, "GET:/api/v2/users/{user}/workspaces": {StatusCode: http.StatusOK, AssertObject: rbac.ResourceWorkspace}, "GET:/api/v2/organizations/{organization}/workspaces/{user}": {StatusCode: http.StatusOK, AssertObject: rbac.ResourceWorkspace}, + "GET:/api/v2/organizations/{organization}/workspaces/{user}/{workspace}": { + AssertObject: rbac.ResourceWorkspace.InOrg(organization.ID).WithID(workspace.ID.String()).WithOwner(workspace.OwnerID.String()), + }, + "GET:/api/v2/organizations/{organization}/workspaces": {StatusCode: http.StatusOK, AssertObject: rbac.ResourceWorkspace}, } c, _ := srv.Config.Handler.(*chi.Mux) @@ -152,13 +156,14 @@ func TestAuthorizeAllEndpoints(t *testing.T) { routeAssertions = routeCheck{} } if routeAssertions.StatusCode == 0 { - routeAssertions.StatusCode = http.StatusUnauthorized + routeAssertions.StatusCode = http.StatusForbidden } // Replace all url params with known values route = strings.ReplaceAll(route, "{organization}", admin.OrganizationID.String()) route = strings.ReplaceAll(route, "{user}", admin.UserID.String()) route = strings.ReplaceAll(route, "{organizationname}", organization.Name) + route = strings.ReplaceAll(route, "{workspace}", workspace.Name) resp, err := client.Request(context.Background(), method, route, nil) require.NoError(t, err, "do req") diff --git a/coderd/workspaces.go b/coderd/workspaces.go index 51bd006fb9df6..0e473fbdbcd79 100644 --- a/coderd/workspaces.go +++ b/coderd/workspaces.go @@ -58,12 +58,18 @@ func (api *api) workspace(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())) { + return + } + httpapi.Write(rw, http.StatusOK, convertWorkspace(workspace, convertWorkspaceBuild(build, convertProvisionerJob(job)), template, owner)) } func (api *api) workspacesByOrganization(rw http.ResponseWriter, r *http.Request) { organization := httpmw.OrganizationParam(r) + roles := httpmw.UserRoles(r) workspaces, err := api.Database.GetWorkspacesByOrganizationID(r.Context(), database.GetWorkspacesByOrganizationIDParams{ OrganizationID: organization.ID, Deleted: false, @@ -77,7 +83,18 @@ func (api *api) workspacesByOrganization(rw http.ResponseWriter, r *http.Request }) return } - apiWorkspaces, err := convertWorkspaces(r.Context(), api.Database, workspaces) + + 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) + } + } + + apiWorkspaces, err := convertWorkspaces(r.Context(), api.Database, allowedWorkspaces) if err != nil { httpapi.Write(rw, http.StatusInternalServerError, httpapi.Response{ Message: fmt.Sprintf("convert workspaces: %s", err), @@ -102,6 +119,7 @@ func (api *api) workspacesByUser(rw http.ResponseWriter, r *http.Request) { return } for _, ws := range userWorkspaces { + ws := ws err = api.Authorizer.ByRoleName(r.Context(), user.ID.String(), roles.Roles, rbac.ActionRead, rbac.ResourceWorkspace.InOrg(ws.OrganizationID).WithOwner(ws.OwnerID.String()).WithID(ws.ID.String())) if err == nil { @@ -121,6 +139,7 @@ func (api *api) workspacesByUser(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.GetWorkspacesByOwnerID(r.Context(), database.GetWorkspacesByOwnerIDParams{ OwnerID: owner.ID, }) @@ -133,7 +152,18 @@ func (api *api) workspacesByOwner(rw http.ResponseWriter, r *http.Request) { }) return } - apiWorkspaces, err := convertWorkspaces(r.Context(), api.Database, workspaces) + + 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) + } + } + + apiWorkspaces, err := convertWorkspaces(r.Context(), api.Database, allowedWorkspaces) if err != nil { httpapi.Write(rw, http.StatusInternalServerError, httpapi.Response{ Message: fmt.Sprintf("convert workspaces: %s", err), @@ -153,9 +183,8 @@ func (api *api) workspaceByOwnerAndName(rw http.ResponseWriter, r *http.Request) Name: workspaceName, }) if errors.Is(err, sql.ErrNoRows) { - httpapi.Write(rw, http.StatusNotFound, httpapi.Response{ - Message: fmt.Sprintf("no workspace found by name %q", workspaceName), - }) + // Do not leak information if the workspace exists or not + httpapi.Forbidden(rw) return } if err != nil { @@ -172,6 +201,11 @@ 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())) { + return + } + build, err := api.Database.GetWorkspaceBuildByWorkspaceIDWithoutAfter(r.Context(), workspace.ID) if err != nil { httpapi.Write(rw, http.StatusInternalServerError, httpapi.Response{ From 7ad069ee5b82bbcc3b613b3d6473bc2f7ab95cab Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Mon, 16 May 2022 18:47:03 -0500 Subject: [PATCH 23/26] Drop update User auth check from assign roles --- coderd/coderd_test.go | 3 +++ coderd/users.go | 4 ---- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/coderd/coderd_test.go b/coderd/coderd_test.go index 826aee5c04fbe..d096682957538 100644 --- a/coderd/coderd_test.go +++ b/coderd/coderd_test.go @@ -143,6 +143,9 @@ func TestAuthorizeAllEndpoints(t *testing.T) { AssertObject: rbac.ResourceWorkspace.InOrg(organization.ID).WithID(workspace.ID.String()).WithOwner(workspace.OwnerID.String()), }, "GET:/api/v2/organizations/{organization}/workspaces": {StatusCode: http.StatusOK, AssertObject: rbac.ResourceWorkspace}, + + // These endpoints need payloads to get to the auth part. + "PUT:/api/v2/users/{user}/roles": {StatusCode: http.StatusBadRequest, NoAuthorize: true}, } c, _ := srv.Config.Handler.(*chi.Mux) diff --git a/coderd/users.go b/coderd/users.go index 73eba19e748ff..fbbbde5e250c1 100644 --- a/coderd/users.go +++ b/coderd/users.go @@ -430,10 +430,6 @@ func (api *api) putUserRoles(rw http.ResponseWriter, r *http.Request) { user := httpmw.UserParam(r) roles := httpmw.UserRoles(r) - if !api.Authorize(rw, r, rbac.ActionUpdate, rbac.ResourceUser.WithOwner(user.ID.String())) { - return - } - var params codersdk.UpdateRoles if !httpapi.Read(rw, r, ¶ms) { return From e68fdbf61dad8a86d7efa791da617a4b9fd1ae9c Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 17 May 2022 09:06:20 -0500 Subject: [PATCH 24/26] Correct unit tests --- coderd/coderd.go | 1 + coderd/organizations_test.go | 8 ++++---- coderd/users_test.go | 2 +- coderd/workspaces_test.go | 2 +- 4 files changed, 7 insertions(+), 6 deletions(-) diff --git a/coderd/coderd.go b/coderd/coderd.go index a3cfc90d0b4c0..6bf4aa8920189 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -293,6 +293,7 @@ func New(options *Options) (http.Handler, func()) { r.Route("/workspaces/{workspace}", func(r chi.Router) { r.Use( apiKeyMiddleware, + authRolesMiddleware, httpmw.ExtractWorkspaceParam(options.Database), ) r.Get("/", api.workspace) diff --git a/coderd/organizations_test.go b/coderd/organizations_test.go index e6338f61cb7fe..01b5822a15611 100644 --- a/coderd/organizations_test.go +++ b/coderd/organizations_test.go @@ -30,7 +30,7 @@ func TestOrganizationByUserAndName(t *testing.T) { _, err := client.OrganizationByName(context.Background(), codersdk.Me, "nothing") var apiErr *codersdk.Error require.ErrorAs(t, err, &apiErr) - require.Equal(t, http.StatusNotFound, apiErr.StatusCode()) + require.Equal(t, http.StatusForbidden, apiErr.StatusCode()) }) t.Run("NoMember", func(t *testing.T) { @@ -38,14 +38,14 @@ func TestOrganizationByUserAndName(t *testing.T) { client := coderdtest.New(t, nil) first := coderdtest.CreateFirstUser(t, client) other := coderdtest.CreateAnotherUser(t, client, first.OrganizationID) - org, err := other.CreateOrganization(context.Background(), codersdk.Me, codersdk.CreateOrganizationRequest{ + org, err := client.CreateOrganization(context.Background(), codersdk.Me, codersdk.CreateOrganizationRequest{ Name: "another", }) require.NoError(t, err) - _, err = client.OrganizationByName(context.Background(), codersdk.Me, org.Name) + _, err = other.OrganizationByName(context.Background(), codersdk.Me, org.Name) var apiErr *codersdk.Error require.ErrorAs(t, err, &apiErr) - require.Equal(t, http.StatusUnauthorized, apiErr.StatusCode()) + require.Equal(t, http.StatusForbidden, apiErr.StatusCode()) }) t.Run("Valid", func(t *testing.T) { diff --git a/coderd/users_test.go b/coderd/users_test.go index 49512b24273d4..a39f733cf87d0 100644 --- a/coderd/users_test.go +++ b/coderd/users_test.go @@ -187,7 +187,7 @@ func TestPostUsers(t *testing.T) { }) var apiErr *codersdk.Error require.ErrorAs(t, err, &apiErr) - require.Equal(t, http.StatusUnauthorized, apiErr.StatusCode()) + require.Equal(t, http.StatusForbidden, apiErr.StatusCode()) }) t.Run("Create", func(t *testing.T) { diff --git a/coderd/workspaces_test.go b/coderd/workspaces_test.go index 3e4d8b57244c5..6183b853c1733 100644 --- a/coderd/workspaces_test.go +++ b/coderd/workspaces_test.go @@ -158,7 +158,7 @@ func TestWorkspaceByOwnerAndName(t *testing.T) { _, err := client.WorkspaceByOwnerAndName(context.Background(), user.OrganizationID, codersdk.Me, "something") var apiErr *codersdk.Error require.ErrorAs(t, err, &apiErr) - require.Equal(t, http.StatusNotFound, apiErr.StatusCode()) + require.Equal(t, http.StatusForbidden, apiErr.StatusCode()) }) t.Run("Get", func(t *testing.T) { t.Parallel() From 1a4e7e15ea12f41a3932bf4db48be24a802d019e Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 17 May 2022 10:10:17 -0500 Subject: [PATCH 25/26] Unit tests use 403 vs 401 --- cli/autostart_test.go | 4 ++-- cli/autostop_test.go | 2 +- coderd/httpmw/organizationparam.go | 2 +- coderd/httpmw/organizationparam_test.go | 2 +- coderd/roles_test.go | 4 ++-- 5 files changed, 7 insertions(+), 7 deletions(-) diff --git a/cli/autostart_test.go b/cli/autostart_test.go index 78f11a5380145..8c9ff40ee25d0 100644 --- a/cli/autostart_test.go +++ b/cli/autostart_test.go @@ -110,7 +110,7 @@ func TestAutostart(t *testing.T) { clitest.SetupConfig(t, client, root) err := cmd.Execute() - require.ErrorContains(t, err, "status code 404: no workspace found by name", "unexpected error") + require.ErrorContains(t, err, "status code 403: forbidden", "unexpected error") }) t.Run("Disable_NotFound", func(t *testing.T) { @@ -128,7 +128,7 @@ func TestAutostart(t *testing.T) { clitest.SetupConfig(t, client, root) err := cmd.Execute() - require.ErrorContains(t, err, "status code 404: no workspace found by name", "unexpected error") + require.ErrorContains(t, err, "status code 403: forbidden", "unexpected error") }) t.Run("Enable_DefaultSchedule", func(t *testing.T) { diff --git a/cli/autostop_test.go b/cli/autostop_test.go index 2d5205ad9c732..082b490491a28 100644 --- a/cli/autostop_test.go +++ b/cli/autostop_test.go @@ -127,7 +127,7 @@ func TestAutostop(t *testing.T) { clitest.SetupConfig(t, client, root) err := cmd.Execute() - require.ErrorContains(t, err, "status code 404: no workspace found by name", "unexpected error") + require.ErrorContains(t, err, "status code 403: forbidden", "unexpected error") }) t.Run("Enable_DefaultSchedule", func(t *testing.T) { diff --git a/coderd/httpmw/organizationparam.go b/coderd/httpmw/organizationparam.go index d66e49236672e..8b088ee76b4d8 100644 --- a/coderd/httpmw/organizationparam.go +++ b/coderd/httpmw/organizationparam.go @@ -63,7 +63,7 @@ func ExtractOrganizationParam(db database.Store) func(http.Handler) http.Handler UserID: apiKey.UserID, }) if errors.Is(err, sql.ErrNoRows) { - httpapi.Write(rw, http.StatusUnauthorized, httpapi.Response{ + httpapi.Write(rw, http.StatusForbidden, httpapi.Response{ Message: "not a member of the organization", }) return diff --git a/coderd/httpmw/organizationparam_test.go b/coderd/httpmw/organizationparam_test.go index 2e4a8eddf4414..b062e63bc3819 100644 --- a/coderd/httpmw/organizationparam_test.go +++ b/coderd/httpmw/organizationparam_test.go @@ -141,7 +141,7 @@ func TestOrganizationParam(t *testing.T) { rtr.ServeHTTP(rw, r) res := rw.Result() defer res.Body.Close() - require.Equal(t, http.StatusUnauthorized, res.StatusCode) + require.Equal(t, http.StatusForbidden, res.StatusCode) }) t.Run("Success", func(t *testing.T) { diff --git a/coderd/roles_test.go b/coderd/roles_test.go index 83d4f1f23d83a..0350bcf835377 100644 --- a/coderd/roles_test.go +++ b/coderd/roles_test.go @@ -112,7 +112,7 @@ func TestListRoles(t *testing.T) { }) require.NoError(t, err, "create org") - const unauth = "unauthorized" + const unauth = "forbidden" const notMember = "not a member of the organization" testCases := []struct { @@ -191,7 +191,7 @@ func TestListRoles(t *testing.T) { if c.AuthorizedError != "" { var apiErr *codersdk.Error require.ErrorAs(t, err, &apiErr) - require.Equal(t, http.StatusUnauthorized, apiErr.StatusCode()) + require.Equal(t, http.StatusForbidden, apiErr.StatusCode()) require.Contains(t, apiErr.Message, c.AuthorizedError) } else { require.NoError(t, err) From f250fbe57a239c938a807f9c6fe98561bbb452b5 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 17 May 2022 10:37:45 -0500 Subject: [PATCH 26/26] 401 -> 403 in unit test --- cli/autostop_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cli/autostop_test.go b/cli/autostop_test.go index 082b490491a28..14447ac037ee4 100644 --- a/cli/autostop_test.go +++ b/cli/autostop_test.go @@ -109,7 +109,7 @@ func TestAutostop(t *testing.T) { clitest.SetupConfig(t, client, root) err := cmd.Execute() - require.ErrorContains(t, err, "status code 404: no workspace found by name", "unexpected error") + require.ErrorContains(t, err, "status code 403: forbidden", "unexpected error") }) t.Run("Disable_NotFound", func(t *testing.T) {