diff --git a/cli/testdata/coder_list_--output_json.golden b/cli/testdata/coder_list_--output_json.golden index 9a39ed4f9c3dc..6ea0ced1835ee 100644 --- a/cli/testdata/coder_list_--output_json.golden +++ b/cli/testdata/coder_list_--output_json.golden @@ -5,6 +5,7 @@ "updated_at": "[timestamp]", "owner_id": "[first user ID]", "owner_name": "testuser", + "organization_id": "[first org ID]", "template_id": "[template ID]", "template_name": "test-template", "template_display_name": "", diff --git a/coderd/apidoc/docs.go b/coderd/apidoc/docs.go index 67075e77f6a61..d3e5b48d58db0 100644 --- a/coderd/apidoc/docs.go +++ b/coderd/apidoc/docs.go @@ -6776,11 +6776,9 @@ const docTemplate = `{ "codersdk.Experiment": { "type": "string", "enum": [ - "authz_querier", "template_editor" ], "x-enum-varnames": [ - "ExperimentAuthzQuerier", "ExperimentTemplateEditor" ] }, @@ -8504,6 +8502,10 @@ const docTemplate = `{ "name": { "type": "string" }, + "organization_id": { + "type": "string", + "format": "uuid" + }, "outdated": { "type": "boolean" }, diff --git a/coderd/apidoc/swagger.json b/coderd/apidoc/swagger.json index 2697a8ee2205a..913e02eb22c37 100644 --- a/coderd/apidoc/swagger.json +++ b/coderd/apidoc/swagger.json @@ -6063,8 +6063,8 @@ }, "codersdk.Experiment": { "type": "string", - "enum": ["authz_querier", "template_editor"], - "x-enum-varnames": ["ExperimentAuthzQuerier", "ExperimentTemplateEditor"] + "enum": ["template_editor"], + "x-enum-varnames": ["ExperimentTemplateEditor"] }, "codersdk.Feature": { "type": "object", @@ -7652,6 +7652,10 @@ "name": { "type": "string" }, + "organization_id": { + "type": "string", + "format": "uuid" + }, "outdated": { "type": "boolean" }, diff --git a/coderd/apikey.go b/coderd/apikey.go index 8330488191e53..c05f0205dc80f 100644 --- a/coderd/apikey.go +++ b/coderd/apikey.go @@ -55,11 +55,6 @@ func (api *API) postToken(rw http.ResponseWriter, r *http.Request) { aReq.Old = database.APIKey{} defer commitAudit() - if !api.Authorize(r, rbac.ActionCreate, rbac.ResourceAPIKey.WithOwner(user.ID.String())) { - httpapi.ResourceNotFound(rw) - return - } - var createToken codersdk.CreateTokenRequest if !httpapi.Read(ctx, rw, r, &createToken) { return @@ -134,11 +129,6 @@ func (api *API) postAPIKey(rw http.ResponseWriter, r *http.Request) { ctx := r.Context() user := httpmw.UserParam(r) - if !api.Authorize(r, rbac.ActionCreate, rbac.ResourceAPIKey.WithOwner(user.ID.String())) { - httpapi.ResourceNotFound(rw) - return - } - lifeTime := time.Hour * 24 * 7 cookie, _, err := api.createAPIKey(ctx, createAPIKeyParams{ UserID: user.ID, @@ -190,11 +180,6 @@ func (api *API) apiKeyByID(rw http.ResponseWriter, r *http.Request) { return } - if !api.Authorize(r, rbac.ActionRead, key) { - httpapi.ResourceNotFound(rw) - return - } - httpapi.Write(ctx, rw, http.StatusOK, convertAPIKey(key)) } @@ -230,11 +215,6 @@ func (api *API) apiKeyByName(rw http.ResponseWriter, r *http.Request) { return } - if !api.Authorize(r, rbac.ActionRead, token) { - httpapi.ResourceNotFound(rw) - return - } - httpapi.Write(ctx, rw, http.StatusOK, convertAPIKey(token)) } @@ -327,7 +307,6 @@ func (api *API) tokens(rw http.ResponseWriter, r *http.Request) { func (api *API) deleteAPIKey(rw http.ResponseWriter, r *http.Request) { var ( ctx = r.Context() - user = httpmw.UserParam(r) keyID = chi.URLParam(r, "keyid") auditor = api.Auditor.Load() aReq, commitAudit = audit.InitRequest[database.APIKey](rw, &audit.RequestParams{ @@ -344,11 +323,6 @@ func (api *API) deleteAPIKey(rw http.ResponseWriter, r *http.Request) { aReq.Old = key defer commitAudit() - if !api.Authorize(r, rbac.ActionDelete, rbac.ResourceAPIKey.WithIDString(keyID).WithOwner(user.ID.String())) { - httpapi.ResourceNotFound(rw) - return - } - err = api.Database.DeleteAPIKeyByID(ctx, keyID) if errors.Is(err, sql.ErrNoRows) { httpapi.ResourceNotFound(rw) diff --git a/coderd/audit.go b/coderd/audit.go index f74df92b099c3..bde7e6eb0b0a3 100644 --- a/coderd/audit.go +++ b/coderd/audit.go @@ -37,10 +37,6 @@ import ( // @Router /audit [get] func (api *API) auditLogs(rw http.ResponseWriter, r *http.Request) { ctx := r.Context() - if !api.Authorize(r, rbac.ActionRead, rbac.ResourceAuditLog) { - httpapi.Forbidden(rw) - return - } page, ok := parsePagination(rw, r) if !ok { @@ -90,10 +86,6 @@ func (api *API) auditLogs(rw http.ResponseWriter, r *http.Request) { // @Router /audit/testgenerate [post] func (api *API) generateFakeAuditLog(rw http.ResponseWriter, r *http.Request) { ctx := r.Context() - if !api.Authorize(r, rbac.ActionCreate, rbac.ResourceAuditLog) { - httpapi.Forbidden(rw) - return - } key := httpmw.APIKey(r) user, err := api.Database.GetUserByID(ctx, key.UserID) diff --git a/coderd/authorize.go b/coderd/authorize.go index 0681f38bf990c..ab1f3a39fd542 100644 --- a/coderd/authorize.go +++ b/coderd/authorize.go @@ -51,28 +51,6 @@ type HTTPAuthorizer struct { // return // } func (api *API) Authorize(r *http.Request, action rbac.Action, object rbac.Objecter) bool { - // The experiment does not replace ALL rbac checks, but does replace most. - // This statement aborts early on the checks that will be removed in the - // future when this experiment is default. - if api.Experiments.Enabled(codersdk.ExperimentAuthzQuerier) { - // Some resource types do not interact with the persistent layer and - // we need to keep these checks happening in the API layer. - switch object.RBACObject().Type { - case rbac.ResourceWorkspaceExecution.Type: - // This is not a db resource, always in API layer - case rbac.ResourceDeploymentValues.Type: - // For metric cache items like DAU, we do not hit the DB. - // Some db actions are in asserted in the authz layer. - case rbac.ResourceReplicas.Type: - // Replica rbac is checked for adding and removing replicas. - case rbac.ResourceProvisionerDaemon.Type: - // Provisioner rbac is checked for adding and removing provisioners. - case rbac.ResourceDebugInfo.Type: - // This is not a db resource, always in API layer. - default: - return true - } - } return api.HTTPAuth.Authorize(r, action, object) } diff --git a/coderd/coderd.go b/coderd/coderd.go index f49765b5abdf4..ac277d057bb71 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -166,6 +166,15 @@ func New(options *Options) *API { if options == nil { options = &Options{} } + + if options.Authorizer == nil { + options.Authorizer = rbac.NewCachingAuthorizer(options.PrometheusRegistry) + } + options.Database = dbauthz.New( + options.Database, + options.Authorizer, + options.Logger.Named("authz_querier"), + ) experiments := initExperiments( options.Logger, options.DeploymentValues.Experiments.Value(), ) @@ -201,9 +210,6 @@ func New(options *Options) *API { if options.PrometheusRegistry == nil { options.PrometheusRegistry = prometheus.NewRegistry() } - if options.Authorizer == nil { - options.Authorizer = rbac.NewCachingAuthorizer(options.PrometheusRegistry) - } if options.TailnetCoordinator == nil { options.TailnetCoordinator = tailnet.NewCoordinator() } @@ -216,14 +222,6 @@ func New(options *Options) *API { if options.SSHConfig.HostnamePrefix == "" { options.SSHConfig.HostnamePrefix = "coder." } - // TODO: remove this once we promote authz_querier out of experiments. - if experiments.Enabled(codersdk.ExperimentAuthzQuerier) { - options.Database = dbauthz.New( - options.Database, - options.Authorizer, - options.Logger.Named("authz_querier"), - ) - } if options.SetUserGroups == nil { options.SetUserGroups = func(context.Context, database.Store, uuid.UUID, []string) error { return nil } } diff --git a/coderd/coderdtest/authorize.go b/coderd/coderdtest/authorize.go index 0f0c21636afbe..5e3918e7e6f02 100644 --- a/coderd/coderdtest/authorize.go +++ b/coderd/coderdtest/authorize.go @@ -1,559 +1,145 @@ package coderdtest import ( - "bytes" "context" "fmt" - "io" - "net/http" - "strconv" + "path/filepath" + "runtime" "strings" "sync" "testing" - "time" - "github.com/go-chi/chi/v5" "github.com/google/uuid" "github.com/moby/moby/pkg/namesgenerator" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "golang.org/x/xerrors" - "github.com/coder/coder/cryptorand" - "github.com/coder/coder/coderd" + "github.com/coder/coder/coderd/database/dbauthz" "github.com/coder/coder/coderd/rbac" "github.com/coder/coder/coderd/rbac/regosql" "github.com/coder/coder/codersdk" - "github.com/coder/coder/provisioner/echo" - "github.com/coder/coder/provisionersdk/proto" + "github.com/coder/coder/cryptorand" ) -func AGPLRoutes(a *AuthTester) (map[string]string, map[string]RouteCheck) { - // Some quick reused objects - workspaceRBACObj := rbac.ResourceWorkspace.WithID(a.Workspace.ID).InOrg(a.Organization.ID).WithOwner(a.Workspace.OwnerID.String()) - workspaceExecObj := rbac.ResourceWorkspaceExecution.WithID(a.Workspace.ID).InOrg(a.Organization.ID).WithOwner(a.Workspace.OwnerID.String()) - applicationConnectObj := rbac.ResourceWorkspaceApplicationConnect.WithID(a.Workspace.ID).InOrg(a.Organization.ID).WithOwner(a.Workspace.OwnerID.String()) - templateObj := rbac.ResourceTemplate.WithID(a.Template.ID).InOrg(a.Template.OrganizationID) - - // skipRoutes allows skipping routes from being checked. - skipRoutes := map[string]string{ - "POST:/api/v2/users/logout": "Logging out deletes the API Key for other routes", - "GET:/derp": "This requires a WebSocket upgrade!", - "GET:/derp/latency-check": "This always returns a 200!", - } +// RBACAsserter is a helper for asserting that the correct RBAC checks are +// performed. This struct is tied to a given user, and only authorizes calls +// for this user are checked. +type RBACAsserter struct { + Subject rbac.Subject - assertRoute := map[string]RouteCheck{ - // These endpoints do not require auth - "GET:/healthz": {NoAuthorize: true}, - "GET:/api/v2": {NoAuthorize: true}, - "GET:/api/v2/buildinfo": {NoAuthorize: true}, - "GET:/api/v2/experiments": {NoAuthorize: true}, // This route requires AuthN, but not AuthZ. - "GET:/api/v2/updatecheck": {NoAuthorize: true}, - "GET:/api/v2/users/first": {NoAuthorize: true}, - "POST:/api/v2/users/first": {NoAuthorize: true}, - "POST:/api/v2/users/login": {NoAuthorize: true}, - "GET:/api/v2/users/authmethods": {NoAuthorize: true}, - "POST:/api/v2/csp/reports": {NoAuthorize: true}, - "POST:/api/v2/authcheck": {NoAuthorize: true}, - "GET:/api/v2/applications/host": {NoAuthorize: true}, - "GET:/api/v2/deployment/ssh": {NoAuthorize: true, StatusCode: http.StatusOK}, - - // Has it's own auth - "GET:/api/v2/users/oauth2/github/callback": {NoAuthorize: true}, - "GET:/api/v2/users/oidc/callback": {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/gitauth": {NoAuthorize: true}, - "GET:/api/v2/workspaceagents/me/gitsshkey": {NoAuthorize: true}, - "GET:/api/v2/workspaceagents/me/metadata": {NoAuthorize: true}, - "GET:/api/v2/workspaceagents/me/coordinate": {NoAuthorize: true}, - "POST:/api/v2/workspaceagents/me/startup": {NoAuthorize: true}, - "POST:/api/v2/workspaceagents/me/app-health": {NoAuthorize: true}, - "POST:/api/v2/workspaceagents/me/report-stats": {NoAuthorize: true}, - "POST:/api/v2/workspaceagents/me/report-lifecycle": {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.WithID(a.Admin.OrganizationID).InOrg(a.Admin.OrganizationID)}, - "GET:/api/v2/users/{user}/organizations": {StatusCode: http.StatusOK, AssertObject: rbac.ResourceOrganization}, - "GET:/api/v2/users/{user}/workspace/{workspacename}": { - AssertObject: rbac.ResourceWorkspace, - AssertAction: rbac.ActionRead, - }, - "GET:/api/v2/users/{user}/workspace/{workspacename}/builds/{buildnumber}": { - AssertObject: rbac.ResourceWorkspace, - AssertAction: rbac.ActionRead, - }, - "GET:/api/v2/users/{user}/keys/tokens": { - AssertObject: rbac.ResourceAPIKey, - AssertAction: rbac.ActionRead, - StatusCode: http.StatusOK, - }, - "GET:/api/v2/users/{user}/keys/{keyid}": { - AssertObject: rbac.ResourceAPIKey, - AssertAction: rbac.ActionRead, - }, - "GET:/api/v2/users/{user}/keys/tokens/{keyname}": { - AssertObject: rbac.ResourceAPIKey, - AssertAction: rbac.ActionRead, - }, - "GET:/api/v2/users/{user}/keys/tokens/tokenconfig": {NoAuthorize: true}, - "GET:/api/v2/workspacebuilds/{workspacebuild}": { - AssertAction: rbac.ActionRead, - AssertObject: workspaceRBACObj, - }, - "GET:/api/v2/workspacebuilds/{workspacebuild}/logs": { - AssertAction: rbac.ActionRead, - AssertObject: workspaceRBACObj, - }, - "GET:/api/v2/workspaces/{workspace}/builds": { - AssertAction: rbac.ActionRead, - AssertObject: workspaceRBACObj, - }, - "GET:/api/v2/workspaces/{workspace}": { - AssertAction: rbac.ActionRead, - AssertObject: workspaceRBACObj, - }, - "PUT:/api/v2/workspaces/{workspace}/autostart": { - AssertAction: rbac.ActionUpdate, - AssertObject: workspaceRBACObj, - }, - "PUT:/api/v2/workspaces/{workspace}/ttl": { - AssertAction: rbac.ActionUpdate, - AssertObject: workspaceRBACObj, - }, - "PATCH:/api/v2/workspacebuilds/{workspacebuild}/cancel": { - AssertAction: rbac.ActionUpdate, - AssertObject: workspaceRBACObj, - }, - "GET:/api/v2/workspacebuilds/{workspacebuild}/resources": { - AssertAction: rbac.ActionRead, - AssertObject: workspaceRBACObj, - }, - "GET:/api/v2/workspacebuilds/{workspacebuild}/state": { - AssertAction: rbac.ActionUpdate, - AssertObject: templateObj, - }, - "GET:/api/v2/workspaceagents/{workspaceagent}": { - AssertAction: rbac.ActionRead, - AssertObject: workspaceRBACObj, - }, - "GET:/api/v2/workspaceagents/{workspaceagent}/pty": { - AssertAction: rbac.ActionCreate, - AssertObject: workspaceExecObj, - }, - "GET:/api/v2/workspaceagents/{workspaceagent}/coordinate": { - AssertAction: rbac.ActionCreate, - AssertObject: workspaceExecObj, - }, - "POST:/api/v2/organizations/{organization}/templates": { - AssertAction: rbac.ActionCreate, - AssertObject: rbac.ResourceTemplate.InOrg(a.Organization.ID), - }, - "DELETE:/api/v2/templates/{template}": { - AssertAction: rbac.ActionDelete, - AssertObject: templateObj, - }, - "GET:/api/v2/templates/{template}": { - AssertAction: rbac.ActionRead, - AssertObject: templateObj, - }, - "POST:/api/v2/files": {AssertAction: rbac.ActionCreate, AssertObject: rbac.ResourceFile}, - "GET:/api/v2/files/{fileID}": { - AssertAction: rbac.ActionRead, - AssertObject: rbac.ResourceFile.WithOwner(a.Admin.UserID.String()), - }, - "GET:/api/v2/templates/{template}/versions": { - AssertAction: rbac.ActionRead, - AssertObject: templateObj, - }, - "PATCH:/api/v2/templates/{template}/versions": { - AssertAction: rbac.ActionUpdate, - AssertObject: templateObj, - }, - "GET:/api/v2/templates/{template}/versions/{templateversionname}": { - AssertAction: rbac.ActionRead, - AssertObject: templateObj, - }, - "GET:/api/v2/templateversions/{templateversion}": { - AssertAction: rbac.ActionRead, - AssertObject: templateObj, - }, - "PATCH:/api/v2/templateversions/{templateversion}/cancel": { - AssertAction: rbac.ActionUpdate, - AssertObject: templateObj, - }, - "GET:/api/v2/templateversions/{templateversion}/logs": { - AssertAction: rbac.ActionRead, - AssertObject: templateObj, - }, - "GET:/api/v2/templateversions/{templateversion}/parameters": { - AssertAction: rbac.ActionRead, - AssertObject: templateObj, - }, - "GET:/api/v2/templateversions/{templateversion}/rich-parameters": { - AssertAction: rbac.ActionRead, - AssertObject: templateObj, - }, - "GET:/api/v2/templateversions/{templateversion}/resources": { - AssertAction: rbac.ActionRead, - AssertObject: templateObj, - }, - "GET:/api/v2/templateversions/{templateversion}/schema": { - AssertAction: rbac.ActionRead, - AssertObject: templateObj, - }, - "POST:/api/v2/templateversions/{templateversion}/dry-run": { - // The first check is to read the template - AssertAction: rbac.ActionRead, - AssertObject: templateObj, - }, - "GET:/api/v2/templateversions/{templateversion}/dry-run/{jobID}": { - AssertAction: rbac.ActionRead, - AssertObject: templateObj, - }, - "GET:/api/v2/templateversions/{templateversion}/dry-run/{jobID}/resources": { - AssertAction: rbac.ActionRead, - AssertObject: templateObj, - }, - "GET:/api/v2/templateversions/{templateversion}/dry-run/{jobID}/logs": { - AssertAction: rbac.ActionRead, - AssertObject: templateObj, - }, - "PATCH:/api/v2/templateversions/{templateversion}/dry-run/{jobID}/cancel": { - AssertAction: rbac.ActionRead, - AssertObject: templateObj, - }, - "POST:/api/v2/parameters/{scope}/{id}": { - AssertAction: rbac.ActionUpdate, - AssertObject: rbac.ResourceTemplate, - }, - "GET:/api/v2/parameters/{scope}/{id}": { - AssertAction: rbac.ActionRead, - AssertObject: rbac.ResourceTemplate, - }, - "DELETE:/api/v2/parameters/{scope}/{id}/{name}": { - AssertAction: rbac.ActionUpdate, - AssertObject: rbac.ResourceTemplate, - }, - "GET:/api/v2/organizations/{organization}/templates/{templatename}": { - AssertAction: rbac.ActionRead, - AssertObject: templateObj, - }, - "GET:/api/v2/organizations/{organization}/templates/{templatename}/versions/{templateversionname}": { - AssertAction: rbac.ActionRead, - AssertObject: templateObj, - }, - "GET:/api/v2/organizations/{organization}/templates/{templatename}/versions/{templateversionname}/previous": { - AssertAction: rbac.ActionRead, - AssertObject: templateObj, - }, - "POST:/api/v2/organizations/{organization}/members/{user}/workspaces": { - AssertAction: rbac.ActionCreate, - // No ID when creating - AssertObject: workspaceRBACObj, - }, - "GET:/api/v2/workspaces/{workspace}/watch": { - AssertAction: rbac.ActionRead, - AssertObject: workspaceRBACObj, - }, - "GET:/api/v2/users": {StatusCode: http.StatusOK, AssertObject: rbac.ResourceUser}, - "GET:/api/v2/applications/auth-redirect": {AssertAction: rbac.ActionCreate, AssertObject: rbac.ResourceAPIKey}, - - // These endpoints need payloads to get to the auth part. Payloads will be required - "PUT:/api/v2/users/{user}/roles": {StatusCode: http.StatusBadRequest, NoAuthorize: true}, - "PUT:/api/v2/organizations/{organization}/members/{user}/roles": {NoAuthorize: true}, - "POST:/api/v2/workspaces/{workspace}/builds": {StatusCode: http.StatusBadRequest, NoAuthorize: true}, - "POST:/api/v2/organizations/{organization}/templateversions": {StatusCode: http.StatusBadRequest, NoAuthorize: true}, - - // For any route using SQL filters, we do not check authorization. - // This is because the in memory fake does not use SQL. - "GET:/api/v2/workspaces/": { - StatusCode: http.StatusOK, - NoAuthorize: true, - AssertAction: rbac.ActionRead, - AssertObject: rbac.ResourceWorkspace, - }, - "GET:/api/v2/organizations/{organization}/templates": { - StatusCode: http.StatusOK, - NoAuthorize: true, - AssertAction: rbac.ActionRead, - AssertObject: rbac.ResourceTemplate, - }, + Recorder *RecordingAuthorizer +} - "GET:/api/v2/debug/coordinator": { - AssertAction: rbac.ActionRead, - AssertObject: rbac.ResourceDebugInfo, - }, +// AssertRBAC returns an RBACAsserter for the given user. This asserter will +// allow asserting that the correct RBAC checks are performed for the given user. +// All checks that are not run against this user will be ignored. +func AssertRBAC(t *testing.T, api *coderd.API, client *codersdk.Client) RBACAsserter { + if client.SessionToken() == "" { + t.Fatal("client must be logged in") } - - // Routes like proxy routes support all HTTP methods. A helper func to expand - // 1 url to all http methods. - assertAllHTTPMethods := func(url string, check RouteCheck) { - methods := []string{ - http.MethodGet, http.MethodHead, http.MethodPost, - http.MethodPut, http.MethodPatch, http.MethodDelete, - http.MethodConnect, http.MethodOptions, http.MethodTrace, - } - - for _, method := range methods { - route := method + ":" + url - assertRoute[route] = check - } + recorder, ok := api.Authorizer.(*RecordingAuthorizer) + if !ok { + t.Fatal("expected RecordingAuthorizer") } - assertAllHTTPMethods("/%40{user}/{workspace_and_agent}/apps/{workspaceapp}/*", RouteCheck{ - AssertAction: rbac.ActionCreate, - AssertObject: applicationConnectObj, - }) - assertAllHTTPMethods("/@{user}/{workspace_and_agent}/apps/{workspaceapp}/*", RouteCheck{ - AssertAction: rbac.ActionCreate, - AssertObject: applicationConnectObj, - }) - - return skipRoutes, assertRoute -} - -type RouteCheck struct { - NoAuthorize bool - AssertAction rbac.Action - AssertObject rbac.Object - StatusCode int + // We use the database directly to not cause additional auth checks on behalf + // of the user. This does add authz checks on behalf of the system user, but + // it is hard to avoid that. + // nolint:gocritic + ctx := dbauthz.AsSystemRestricted(context.Background()) + token := client.SessionToken() + parts := strings.Split(token, "-") + key, err := api.Database.GetAPIKeyByID(ctx, parts[0]) + require.NoError(t, err, "fetch client api key") + + roles, err := api.Database.GetAuthorizationUserRoles(ctx, key.UserID) + require.NoError(t, err, "fetch user roles") + + return RBACAsserter{ + Subject: rbac.Subject{ + ID: key.UserID.String(), + Roles: rbac.RoleNames(roles.Roles), + Groups: roles.Groups, + Scope: rbac.ScopeName(key.Scope), + }, + Recorder: recorder, + } } -type AuthTester struct { - t *testing.T - api *coderd.API - authorizer *RecordingAuthorizer - - Client *codersdk.Client - Workspace codersdk.Workspace - Organization codersdk.Organization - Admin codersdk.CreateFirstUserResponse - Template codersdk.Template - Version codersdk.TemplateVersion - WorkspaceResource codersdk.WorkspaceResource - File codersdk.UploadResponse - TemplateVersionDryRun codersdk.ProvisionerJob - TemplateParam codersdk.Parameter - URLParams map[string]string +// AllCalls is for debugging. If you are not sure where calls are coming from, +// call this and use a debugger or print them. They have small callstacks +// on them to help locate the 'Authorize' call. +// Only calls to Authorize by the given subject will be returned. +// Note that duplicate rbac calls are handled by the rbac.Cacher(), but +// will be recorded twice. So AllCalls() returns calls regardless if they +// were returned from the cached or not. +func (a RBACAsserter) AllCalls() []AuthCall { + return a.Recorder.AllCalls(&a.Subject) } -func NewAuthTester(ctx context.Context, t *testing.T, client *codersdk.Client, api *coderd.API, admin codersdk.CreateFirstUserResponse) *AuthTester { - authorizer, ok := api.Authorizer.(*RecordingAuthorizer) - if !ok { - t.Fail() - } - _, err := client.CreateToken(ctx, admin.UserID.String(), codersdk.CreateTokenRequest{ - Lifetime: time.Hour, - Scope: codersdk.APIKeyScopeAll, - TokenName: namesgenerator.GetRandomName(1), - }) - require.NoError(t, err, "create token") - - apiKeys, err := client.Tokens(ctx, admin.UserID.String(), codersdk.TokensFilter{ - IncludeAll: true, - }) - require.NoError(t, err, "get tokens") - apiKey := apiKeys[0] - - organization, err := client.Organization(ctx, admin.OrganizationID) - require.NoError(t, err, "fetch org") - - // Setup some data in the database. - version := CreateTemplateVersion(t, client, admin.OrganizationID, &echo.Responses{ - Parse: echo.ParseComplete, - ProvisionApply: []*proto.Provision_Response{{ - Type: &proto.Provision_Response_Complete{ - Complete: &proto.Provision_Complete{ - // Return a workspace resource - Resources: []*proto.Resource{{ - Name: "some", - Type: "example", - Agents: []*proto.Agent{{ - Name: "agent", - Id: "something", - Auth: &proto.Agent_Token{}, - Apps: []*proto.App{{ - Slug: "testapp", - DisplayName: "testapp", - Url: "http://localhost:3000", - }}, - }}, - }}, - }, - }, - }}, - }) - AwaitTemplateVersionJob(t, client, version.ID) - template := CreateTemplate(t, client, admin.OrganizationID, version.ID) - workspace := CreateWorkspace(t, client, admin.OrganizationID, template.ID) - AwaitWorkspaceBuildJob(t, client, workspace.LatestBuild.ID) - file, err := client.Upload(ctx, codersdk.ContentTypeTar, bytes.NewReader(make([]byte, 1024))) - require.NoError(t, err, "upload file") - workspace, err = client.Workspace(ctx, workspace.ID) - require.NoError(t, err, "workspace resources") - templateVersionDryRun, err := client.CreateTemplateVersionDryRun(ctx, version.ID, codersdk.CreateTemplateVersionDryRunRequest{ - ParameterValues: []codersdk.CreateParameterRequest{}, - }) - require.NoError(t, err, "template version dry-run") - - templateParam, err := client.CreateParameter(ctx, codersdk.ParameterTemplate, template.ID, codersdk.CreateParameterRequest{ - Name: "test-param", - SourceValue: "hello world", - SourceScheme: codersdk.ParameterSourceSchemeData, - DestinationScheme: codersdk.ParameterDestinationSchemeProvisionerVariable, - }) - require.NoError(t, err, "create template param") - urlParameters := map[string]string{ - "{organization}": admin.OrganizationID.String(), - "{user}": admin.UserID.String(), - "{organizationname}": organization.Name, - "{workspace}": workspace.ID.String(), - "{workspacebuild}": workspace.LatestBuild.ID.String(), - "{workspacename}": workspace.Name, - "{workspaceagent}": workspace.LatestBuild.Resources[0].Agents[0].ID.String(), - "{buildnumber}": strconv.FormatInt(int64(workspace.LatestBuild.BuildNumber), 10), - "{template}": template.ID.String(), - "{fileID}": file.ID.String(), - "{workspaceresource}": workspace.LatestBuild.Resources[0].ID.String(), - "{workspaceapp}": workspace.LatestBuild.Resources[0].Agents[0].Apps[0].Slug, - "{templateversion}": version.ID.String(), - "{jobID}": templateVersionDryRun.ID.String(), - "{templatename}": template.Name, - "{workspace_and_agent}": workspace.Name + "." + workspace.LatestBuild.Resources[0].Agents[0].Name, - "{keyid}": apiKey.ID, - "{keyname}": apiKey.TokenName, - // Only checking template scoped params here - "parameters/{scope}/{id}": fmt.Sprintf("parameters/%s/%s", - string(templateParam.Scope), templateParam.ScopeID.String()), - } - - return &AuthTester{ - t: t, - api: api, - authorizer: authorizer, - Client: client, - Workspace: workspace, - Organization: organization, - Admin: admin, - Template: template, - Version: version, - WorkspaceResource: workspace.LatestBuild.Resources[0], - File: file, - TemplateVersionDryRun: templateVersionDryRun, - TemplateParam: templateParam, - URLParams: urlParameters, +// AssertChecked will assert a given rbac check was performed. It does not care +// about order of checks, or any other checks. This is useful when you do not +// care about asserting every check that was performed. +func (a RBACAsserter) AssertChecked(t *testing.T, action rbac.Action, objects ...interface{}) { + converted := a.convertObjects(t, objects...) + pairs := make([]ActionObjectPair, 0, len(converted)) + for _, obj := range converted { + pairs = append(pairs, a.Recorder.Pair(action, obj)) } + a.Recorder.AssertOutOfOrder(t, a.Subject, pairs...) } -func (a *AuthTester) Test(ctx context.Context, assertRoute map[string]RouteCheck, skipRoutes map[string]string) { - // Always fail auth from this point forward - a.authorizer.Wrapped = &FakeAuthorizer{ - AlwaysReturn: rbac.ForbiddenWithInternal(xerrors.New("fake implementation"), rbac.Subject{}, "", rbac.Object{}, nil), +// AssertInOrder must be called in the correct order of authz checks. If the objects +// or actions are not in the correct order, the test will fail. +func (a RBACAsserter) AssertInOrder(t *testing.T, action rbac.Action, objects ...interface{}) { + converted := a.convertObjects(t, objects...) + pairs := make([]ActionObjectPair, 0, len(converted)) + for _, obj := range converted { + pairs = append(pairs, a.Recorder.Pair(action, obj)) } + a.Recorder.AssertActor(t, a.Subject, pairs...) +} - routeMissing := make(map[string]bool) - for k, v := range assertRoute { - noTrailSlash := strings.TrimRight(k, "/") - if _, ok := assertRoute[noTrailSlash]; ok && noTrailSlash != k { - a.t.Errorf("route %q & %q is declared twice", noTrailSlash, k) - a.t.FailNow() +// convertObjects converts the codersdk types to rbac.Object. Unfortunately +// does not have type safety, and instead uses a t.Fatal to enforce types. +func (RBACAsserter) convertObjects(t *testing.T, objs ...interface{}) []rbac.Object { + converted := make([]rbac.Object, 0, len(objs)) + for _, obj := range objs { + var robj rbac.Object + switch obj := obj.(type) { + case rbac.Object: + robj = obj + case rbac.Objecter: + robj = obj.RBACObject() + case codersdk.TemplateVersion: + robj = rbac.ResourceTemplate.InOrg(obj.OrganizationID) + case codersdk.User: + robj = rbac.ResourceUser.WithID(obj.ID) + case codersdk.Workspace: + robj = rbac.ResourceWorkspace.WithID(obj.ID).InOrg(obj.OrganizationID).WithOwner(obj.OwnerID.String()) + default: + t.Fatalf("unsupported type %T to convert to rbac.Object, add the implementation", obj) } - assertRoute[noTrailSlash] = v - routeMissing[noTrailSlash] = true + converted = append(converted, robj) } + return converted +} - for k, v := range skipRoutes { - noTrailSlash := strings.TrimRight(k, "/") - if _, ok := skipRoutes[noTrailSlash]; ok && noTrailSlash != k { - a.t.Errorf("route %q & %q is declared twice", noTrailSlash, k) - a.t.FailNow() - } - skipRoutes[noTrailSlash] = v - } +// Reset will clear all previously recorded authz calls. +// This is helpful when wanting to ignore checks run in test setup. +func (a RBACAsserter) Reset() RBACAsserter { + a.Recorder.Reset() + return a +} - err := chi.Walk( - a.api.RootHandler, - func( - method string, - route string, - handler http.Handler, - middlewares ...func(http.Handler) http.Handler, - ) error { - // work around chi's bugged handling of /*/*/ which can occur if we - // r.Mount("/", someHandler()) in our tree - for strings.Contains(route, "/*/") { - route = strings.Replace(route, "/*/", "/", -1) - } - name := method + ":" + route - if _, ok := skipRoutes[strings.TrimRight(name, "/")]; ok { - return nil - } - a.t.Run(name, func(t *testing.T) { - a.authorizer.Reset() - routeKey := strings.TrimRight(name, "/") - - routeAssertions, ok := assertRoute[routeKey] - if !ok { - // By default, all omitted routes check for just "authorize" called - routeAssertions = RouteCheck{} - } - delete(routeMissing, routeKey) - - // Replace all url params with known values - for k, v := range a.URLParams { - route = strings.ReplaceAll(route, k, v) - } - - resp, err := a.Client.Request(ctx, method, route, nil) - require.NoError(t, err, "do req") - body, _ := io.ReadAll(resp.Body) - t.Logf("Response Body: %q", string(body)) - _ = resp.Body.Close() - - if !routeAssertions.NoAuthorize { - assert.NotNil(t, a.authorizer.Called, "authorizer expected") - if routeAssertions.StatusCode != 0 { - assert.Equal(t, routeAssertions.StatusCode, resp.StatusCode, "expect unauthorized") - } else { - // It's either a 404 or 403. - if resp.StatusCode != http.StatusNotFound { - assert.Equal(t, http.StatusForbidden, resp.StatusCode, "expect unauthorized") - } - } - if a.authorizer.lastCall() != nil { - last := a.authorizer.lastCall() - if routeAssertions.AssertAction != "" { - assert.Equal(t, routeAssertions.AssertAction, last.Action, "resource action") - } - if routeAssertions.AssertObject.Type != "" { - assert.Equal(t, routeAssertions.AssertObject.Type, last.Object.Type, "resource type") - } - if routeAssertions.AssertObject.Owner != "" { - assert.Equal(t, routeAssertions.AssertObject.Owner, last.Object.Owner, "resource owner") - } - if routeAssertions.AssertObject.OrgID != "" { - assert.Equal(t, routeAssertions.AssertObject.OrgID, last.Object.OrgID, "resource org") - } - } - } else { - assert.Nil(t, a.authorizer.Called, "authorize not expected") - } - }) - return nil - }) - require.NoError(a.t, err) - require.Len(a.t, routeMissing, 0, "didn't walk some asserted routes: %v", routeMissing) -} - -type authCall struct { +type AuthCall struct { rbac.AuthCall asserted bool + // callers is a small stack trace for debugging. + callers []string } var _ rbac.Authorizer = (*RecordingAuthorizer)(nil) @@ -562,7 +148,7 @@ var _ rbac.Authorizer = (*RecordingAuthorizer)(nil) // calls made. This is useful for testing as these calls can later be asserted. type RecordingAuthorizer struct { sync.RWMutex - Called []authCall + Called []AuthCall Wrapped rbac.Authorizer } @@ -586,7 +172,7 @@ func (*RecordingAuthorizer) Pair(action rbac.Action, object rbac.Objecter) Actio func (r *RecordingAuthorizer) AllAsserted() error { r.RLock() defer r.RUnlock() - missed := []authCall{} + missed := []AuthCall{} for _, c := range r.Called { if !c.asserted { missed = append(missed, c) @@ -599,11 +185,48 @@ func (r *RecordingAuthorizer) AllAsserted() error { return nil } +// AllCalls is useful for debugging. +func (r *RecordingAuthorizer) AllCalls(actor *rbac.Subject) []AuthCall { + r.RLock() + defer r.RUnlock() + + called := make([]AuthCall, 0, len(r.Called)) + for _, c := range r.Called { + if actor != nil && !c.Actor.Equal(*actor) { + continue + } + called = append(called, c) + } + return called +} + +// AssertOutOfOrder asserts that the given actor performed the given action +// on the given objects. It does not care about the order of the calls. +// When marking authz calls as asserted, it will mark the first matching +// calls first. +func (r *RecordingAuthorizer) AssertOutOfOrder(t *testing.T, actor rbac.Subject, did ...ActionObjectPair) { + r.Lock() + defer r.Unlock() + + for _, do := range did { + found := false + // Find the first non-asserted call that matches the actor, action, and object. + for i, call := range r.Called { + if !call.asserted && call.Actor.Equal(actor) && call.Action == do.Action && call.Object.Equal(do.Object) { + r.Called[i].asserted = true + found = true + break + } + } + require.True(t, found, "assertion missing: %s %s %s", actor, do.Action, do.Object) + } +} + // AssertActor asserts in order. If the order of authz calls does not match, // this will fail. func (r *RecordingAuthorizer) AssertActor(t *testing.T, actor rbac.Subject, did ...ActionObjectPair) { - r.RLock() - defer r.RUnlock() + r.Lock() + defer r.Unlock() ptr := 0 for i, call := range r.Called { if ptr == len(did) { @@ -626,15 +249,38 @@ func (r *RecordingAuthorizer) AssertActor(t *testing.T, actor rbac.Subject, did func (r *RecordingAuthorizer) recordAuthorize(subject rbac.Subject, action rbac.Action, object rbac.Object) { r.Lock() defer r.Unlock() - r.Called = append(r.Called, authCall{ + + r.Called = append(r.Called, AuthCall{ AuthCall: rbac.AuthCall{ Actor: subject, Action: action, Object: object, }, + callers: []string{ + // This is a decent stack trace for debugging. + // Some dbauthz calls are a bit nested, so we skip a few. + caller(2), + caller(3), + caller(4), + caller(5), + }, }) } +func caller(skip int) string { + pc, file, line, ok := runtime.Caller(skip + 1) + i := strings.Index(file, "coder") + if i >= 0 { + file = file[i:] + } + str := fmt.Sprintf("%s:%d", file, line) + if ok { + f := runtime.FuncForPC(pc) + str += " | " + filepath.Base(f.Name()) + } + return str +} + func (r *RecordingAuthorizer) Authorize(ctx context.Context, subject rbac.Subject, action rbac.Action, object rbac.Object) error { r.recordAuthorize(subject, action, object) if r.Wrapped == nil { @@ -669,17 +315,6 @@ func (r *RecordingAuthorizer) Reset() { r.Called = nil } -// lastCall is implemented to support legacy tests. -// Deprecated -func (r *RecordingAuthorizer) lastCall() *authCall { - r.RLock() - defer r.RUnlock() - if len(r.Called) == 0 { - return nil - } - return &r.Called[len(r.Called)-1] -} - // PreparedRecorder is the prepared version of the RecordingAuthorizer. // It records the Authorize() calls to the original recorder. If the caller // uses CompileToSQL, all recording stops. This is to support parity between diff --git a/coderd/coderdtest/authorize_test.go b/coderd/coderdtest/authorize_test.go index 8ef2ef05d9b7b..67bcf482def75 100644 --- a/coderd/coderdtest/authorize_test.go +++ b/coderd/coderdtest/authorize_test.go @@ -2,34 +2,15 @@ package coderdtest_test import ( "context" - "os" - "strings" + "math/rand" "testing" "github.com/stretchr/testify/require" "github.com/coder/coder/coderd/coderdtest" "github.com/coder/coder/coderd/rbac" - "github.com/coder/coder/codersdk" ) -func TestAuthorizeAllEndpoints(t *testing.T) { - if strings.Contains(os.Getenv("CODER_EXPERIMENTS_TEST"), string(codersdk.ExperimentAuthzQuerier)) { - t.Skip("Skipping TestAuthorizeAllEndpoints for authz_querier experiment") - } - t.Parallel() - client, _, api := coderdtest.NewWithAPI(t, &coderdtest.Options{ - // Required for any subdomain-based proxy tests to pass. - AppHostname: "*.test.coder.com", - Authorizer: &coderdtest.RecordingAuthorizer{Wrapped: &coderdtest.FakeAuthorizer{}}, - IncludeProvisionerDaemon: true, - }) - admin := coderdtest.CreateFirstUser(t, client) - a := coderdtest.NewAuthTester(context.Background(), t, client, api, admin) - skipRoute, assertRoute := coderdtest.AGPLRoutes(a) - a.Test(context.Background(), assertRoute, skipRoute) -} - func TestAuthzRecorder(t *testing.T) { t.Parallel() @@ -81,6 +62,42 @@ func TestAuthzRecorder(t *testing.T) { rec.AssertActor(t, a, aPairs...) require.NoError(t, rec.AllAsserted(), "all assertions should have been made") }) + + t.Run("AuthorizeOutOfOrder", func(t *testing.T) { + t.Parallel() + + rec := &coderdtest.RecordingAuthorizer{ + Wrapped: &coderdtest.FakeAuthorizer{}, + } + sub := coderdtest.RandomRBACSubject() + pairs := fuzzAuthz(t, sub, rec, 10) + rand.Shuffle(len(pairs), func(i, j int) { + pairs[i], pairs[j] = pairs[j], pairs[i] + }) + + rec.AssertOutOfOrder(t, sub, pairs...) + require.NoError(t, rec.AllAsserted(), "all assertions should have been made") + }) + + t.Run("AllCalls", func(t *testing.T) { + t.Parallel() + + rec := &coderdtest.RecordingAuthorizer{ + Wrapped: &coderdtest.FakeAuthorizer{}, + } + sub := coderdtest.RandomRBACSubject() + calls := rec.AllCalls(&sub) + pairs := make([]coderdtest.ActionObjectPair, 0, len(calls)) + for _, call := range calls { + pairs = append(pairs, coderdtest.ActionObjectPair{ + Action: call.Action, + Object: call.Object, + }) + } + + rec.AssertActor(t, sub, pairs...) + require.NoError(t, rec.AllAsserted(), "all assertions should have been made") + }) } // fuzzAuthzPrep has same action and object types for all calls. diff --git a/coderd/coderdtest/coderdtest.go b/coderd/coderdtest/coderdtest.go index f0e7cc9711c22..ff9bf82addc98 100644 --- a/coderd/coderdtest/coderdtest.go +++ b/coderd/coderdtest/coderdtest.go @@ -22,7 +22,6 @@ import ( "net/http" "net/http/httptest" "net/url" - "os" "regexp" "strconv" "strings" @@ -183,18 +182,18 @@ func NewOptions(t *testing.T, options *Options) (func(http.Handler), context.Can close(options.AutobuildStats) }) } + + if options.Authorizer == nil { + options.Authorizer = &RecordingAuthorizer{ + Wrapped: rbac.NewCachingAuthorizer(prometheus.NewRegistry()), + } + } + if options.Database == nil { options.Database, options.Pubsub = dbtestutil.NewDB(t) - } - // TODO: remove this once we're ready to enable authz querier by default. - if strings.Contains(os.Getenv("CODER_EXPERIMENTS_TEST"), string(codersdk.ExperimentAuthzQuerier)) { - if options.Authorizer == nil { - options.Authorizer = &RecordingAuthorizer{ - Wrapped: rbac.NewCachingAuthorizer(prometheus.NewRegistry()), - } - } options.Database = dbauthz.New(options.Database, options.Authorizer, slogtest.Make(t, nil).Leveled(slog.LevelDebug)) } + if options.DeploymentValues == nil { options.DeploymentValues = DeploymentValues(t) } diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go index 2d177ef6fb79c..f82e703d86070 100644 --- a/coderd/database/dbauthz/dbauthz.go +++ b/coderd/database/dbauthz/dbauthz.go @@ -38,6 +38,13 @@ func (NotAuthorizedError) Unwrap() error { return sql.ErrNoRows } +func IsNotAuthorizedError(err error) bool { + if err == nil { + return false + } + return xerrors.As(err, &NotAuthorizedError{}) +} + func logNotAuthorizedError(ctx context.Context, logger slog.Logger, err error) error { // Only log the errors if it is an UnauthorizedError error. internalError := new(rbac.UnauthorizedError) @@ -50,8 +57,9 @@ func logNotAuthorizedError(ctx context.Context, logger slog.Logger, err error) e // // NotAuthorizedError is == to sql.ErrNoRows, which is not correct // if it's actually a canceled context. - internalError.SetInternal(context.Canceled) - return internalError + contextError := *internalError + contextError.SetInternal(context.Canceled) + return &contextError } logger.Debug(ctx, "unauthorized", slog.F("internal", internalError.Internal()), diff --git a/coderd/database/dbauthz/dbauthz_test.go b/coderd/database/dbauthz/dbauthz_test.go index ab4da817599db..3fa5d3cdf1c5b 100644 --- a/coderd/database/dbauthz/dbauthz_test.go +++ b/coderd/database/dbauthz/dbauthz_test.go @@ -21,6 +21,11 @@ import ( func TestAsNoActor(t *testing.T) { t.Parallel() + t.Run("NoError", func(t *testing.T) { + t.Parallel() + require.False(t, dbauthz.IsNotAuthorizedError(nil), "no error") + }) + t.Run("AsRemoveActor", func(t *testing.T) { t.Parallel() _, ok := dbauthz.ActorFromContext(context.Background()) @@ -80,6 +85,7 @@ func TestInTX(t *testing.T) { }, nil) require.Error(t, err, "must error") require.ErrorAs(t, err, &dbauthz.NotAuthorizedError{}, "must be an authorized error") + require.True(t, dbauthz.IsNotAuthorizedError(err), "must be an authorized error") } // TestNew should not double wrap a querier. diff --git a/coderd/database/dbauthz/querier.go b/coderd/database/dbauthz/querier.go index 5c70e8e244526..867bfa36383d1 100644 --- a/coderd/database/dbauthz/querier.go +++ b/coderd/database/dbauthz/querier.go @@ -1068,7 +1068,11 @@ func (q *querier) UpdateUserHashedPassword(ctx context.Context, arg database.Upd err = q.authorizeContext(ctx, rbac.ActionUpdate, user.UserDataRBACObject()) if err != nil { - return err + // Admins can update passwords for other users. + err = q.authorizeContext(ctx, rbac.ActionUpdate, user.RBACObject()) + if err != nil { + return err + } } return q.db.UpdateUserHashedPassword(ctx, arg) diff --git a/coderd/files.go b/coderd/files.go index 91858eb3ca06e..1362bfef7ebde 100644 --- a/coderd/files.go +++ b/coderd/files.go @@ -15,7 +15,6 @@ 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" ) @@ -37,12 +36,6 @@ const ( func (api *API) postFile(rw http.ResponseWriter, r *http.Request) { ctx := r.Context() apiKey := httpmw.APIKey(r) - // This requires the site wide action to create files. - // Once created, a user can read their own files uploaded - if !api.Authorize(r, rbac.ActionCreate, rbac.ResourceFile.WithOwner(apiKey.UserID.String())) { - httpapi.Forbidden(rw) - return - } contentType := r.Header.Get("Content-Type") @@ -145,12 +138,6 @@ func (api *API) fileByID(rw http.ResponseWriter, r *http.Request) { return } - if !api.Authorize(r, rbac.ActionRead, file) { - // Return 404 to not leak the file exists - httpapi.ResourceNotFound(rw) - return - } - rw.Header().Set("Content-Type", file.Mimetype) rw.WriteHeader(http.StatusOK) _, _ = rw.Write(file.Data) diff --git a/coderd/gitsshkey.go b/coderd/gitsshkey.go index 3380382f15afd..096844caa0a28 100644 --- a/coderd/gitsshkey.go +++ b/coderd/gitsshkey.go @@ -8,7 +8,6 @@ import ( "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" "github.com/coder/coder/codersdk/agentsdk" ) @@ -35,11 +34,6 @@ func (api *API) regenerateGitSSHKey(rw http.ResponseWriter, r *http.Request) { ) defer commitAudit() - if !api.Authorize(r, rbac.ActionUpdate, user.UserDataRBACObject()) { - httpapi.ResourceNotFound(rw) - return - } - oldKey, err := api.Database.GetGitSSHKey(ctx, user.ID) if err != nil { httpapi.InternalServerError(rw, err) @@ -94,11 +88,6 @@ func (api *API) gitSSHKey(rw http.ResponseWriter, r *http.Request) { ctx := r.Context() user := httpmw.UserParam(r) - if !api.Authorize(r, rbac.ActionRead, user.UserDataRBACObject()) { - httpapi.ResourceNotFound(rw) - return - } - gitSSHKey, err := api.Database.GetGitSSHKey(ctx, user.ID) if err != nil { httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ diff --git a/coderd/members.go b/coderd/members.go index c3e4607b0f9e5..293f007dd400d 100644 --- a/coderd/members.go +++ b/coderd/members.go @@ -34,7 +34,6 @@ func (api *API) putMemberRoles(rw http.ResponseWriter, r *http.Request) { organization = httpmw.OrganizationParam(r) member = httpmw.OrganizationMemberParam(r) apiKey = httpmw.APIKey(r) - actorRoles = httpmw.UserAuthorization(r) ) if apiKey.UserID == member.UserID { @@ -49,30 +48,6 @@ func (api *API) putMemberRoles(rw http.ResponseWriter, r *http.Request) { return } - // The org-member role is always implied. - impliedTypes := append(params.Roles, rbac.RoleOrgMember(organization.ID)) - added, removed := rbac.ChangeRoleSet(member.Roles, impliedTypes) - - // Assigning a role requires the create permission. - if len(added) > 0 && !api.Authorize(r, rbac.ActionCreate, rbac.ResourceOrgRoleAssignment.InOrg(organization.ID)) { - httpapi.ResourceNotFound(rw) - return - } - - // Removing a role requires the delete permission. - if len(removed) > 0 && !api.Authorize(r, rbac.ActionDelete, rbac.ResourceOrgRoleAssignment.InOrg(organization.ID)) { - httpapi.ResourceNotFound(rw) - return - } - - // Just treat adding & removing as "assigning" for now. - for _, roleName := range append(added, removed...) { - if !rbac.CanAssignRole(actorRoles.Actor.Roles, roleName) { - httpapi.ResourceNotFound(rw) - return - } - } - updatedUser, err := api.updateOrganizationMemberRoles(ctx, database.UpdateMemberRolesParams{ GrantedRoles: params.Roles, UserID: user.ID, diff --git a/coderd/organizations.go b/coderd/organizations.go index 19b57a8958e9e..13906baef63b3 100644 --- a/coderd/organizations.go +++ b/coderd/organizations.go @@ -24,15 +24,10 @@ import ( // @Param organization path string true "Organization ID" format(uuid) // @Success 200 {object} codersdk.Organization // @Router /organizations/{organization} [get] -func (api *API) organization(rw http.ResponseWriter, r *http.Request) { +func (*API) organization(rw http.ResponseWriter, r *http.Request) { ctx := r.Context() organization := httpmw.OrganizationParam(r) - if !api.Authorize(r, rbac.ActionRead, organization) { - httpapi.ResourceNotFound(rw) - return - } - httpapi.Write(ctx, rw, http.StatusOK, convertOrganization(organization)) } @@ -48,12 +43,6 @@ func (api *API) organization(rw http.ResponseWriter, r *http.Request) { func (api *API) postOrganizations(rw http.ResponseWriter, r *http.Request) { ctx := r.Context() apiKey := httpmw.APIKey(r) - // Create organization uses the organization resource without an OrgID. - // This means you need the site wide permission to make a new organization. - if !api.Authorize(r, rbac.ActionCreate, rbac.ResourceOrganization) { - httpapi.Forbidden(rw) - return - } var req codersdk.CreateOrganizationRequest if !httpapi.Read(ctx, rw, r, &req) { diff --git a/coderd/parameters.go b/coderd/parameters.go index 29adb4bf5e43b..59814ba1bd9c5 100644 --- a/coderd/parameters.go +++ b/coderd/parameters.go @@ -14,7 +14,6 @@ import ( "github.com/coder/coder/coderd/database" "github.com/coder/coder/coderd/httpapi" "github.com/coder/coder/coderd/parameter" - "github.com/coder/coder/coderd/rbac" "github.com/coder/coder/codersdk" ) @@ -35,14 +34,6 @@ func (api *API) postParameter(rw http.ResponseWriter, r *http.Request) { if !valid { return } - obj, ok := api.parameterRBACResource(rw, r, scope, scopeID) - if !ok { - return - } - if !api.Authorize(r, rbac.ActionUpdate, obj) { - httpapi.ResourceNotFound(rw) - return - } var createRequest codersdk.CreateParameterRequest if !httpapi.Read(ctx, rw, r, &createRequest) { @@ -104,15 +95,6 @@ func (api *API) parameters(rw http.ResponseWriter, r *http.Request) { if !valid { return } - obj, ok := api.parameterRBACResource(rw, r, scope, scopeID) - if !ok { - return - } - - if !api.Authorize(r, rbac.ActionRead, obj) { - httpapi.ResourceNotFound(rw) - return - } parameterValues, err := api.Database.ParameterValues(ctx, database.ParameterValuesParams{ Scopes: []database.ParameterScope{scope}, @@ -152,15 +134,6 @@ func (api *API) deleteParameter(rw http.ResponseWriter, r *http.Request) { if !valid { return } - obj, ok := api.parameterRBACResource(rw, r, scope, scopeID) - if !ok { - return - } - // A deleted param is still updating the underlying resource for the scope. - if !api.Authorize(r, rbac.ActionUpdate, obj) { - httpapi.ResourceNotFound(rw) - return - } name := chi.URLParam(r, "name") parameterValue, err := api.Database.GetParameterValueByScopeAndName(ctx, database.GetParameterValueByScopeAndNameParams{ @@ -236,54 +209,6 @@ func convertParameterValue(parameterValue database.ParameterValue) codersdk.Para } } -// parameterRBACResource returns the RBAC resource a parameter scope and scope -// ID is trying to update. For RBAC purposes, adding a param to a resource -// is equivalent to updating/reading the associated resource. -// This means "parameters" are not a new resource, but an extension of existing -// ones. -func (api *API) parameterRBACResource(rw http.ResponseWriter, r *http.Request, scope database.ParameterScope, scopeID uuid.UUID) (rbac.Objecter, bool) { - ctx := r.Context() - var resource rbac.Objecter - var err error - switch scope { - case database.ParameterScopeWorkspace: - resource, err = api.Database.GetWorkspaceByID(ctx, scopeID) - case database.ParameterScopeImportJob: - // I hate myself. - var version database.TemplateVersion - version, err = api.Database.GetTemplateVersionByJobID(ctx, scopeID) - if err != nil { - break - } - var template database.Template - template, err = api.Database.GetTemplateByID(ctx, version.TemplateID.UUID) - if err != nil { - break - } - resource = version.RBACObject(template) - - case database.ParameterScopeTemplate: - resource, err = api.Database.GetTemplateByID(ctx, scopeID) - default: - err = xerrors.Errorf("Parameter scope %q unsupported", scope) - } - - // Write error payload to rw if we cannot find the resource for the scope - if err != nil { - if xerrors.Is(err, sql.ErrNoRows) { - httpapi.Write(ctx, rw, http.StatusNotFound, codersdk.Response{ - Message: fmt.Sprintf("Scope %q resource %q not found.", scope, scopeID), - }) - } else { - httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ - Message: err.Error(), - }) - } - return nil, false - } - return resource, true -} - func readScopeAndID(ctx context.Context, rw http.ResponseWriter, r *http.Request) (database.ParameterScope, uuid.UUID, bool) { scope := database.ParameterScope(chi.URLParam(r, "scope")) switch scope { diff --git a/coderd/templates.go b/coderd/templates.go index aa4a6ddc239a2..9bdc591cc6da1 100644 --- a/coderd/templates.go +++ b/coderd/templates.go @@ -39,11 +39,6 @@ func (api *API) template(rw http.ResponseWriter, r *http.Request) { ctx := r.Context() template := httpmw.TemplateParam(r) - if !api.Authorize(r, rbac.ActionRead, template) { - httpapi.ResourceNotFound(rw) - return - } - createdByNameMap, err := getCreatedByNamesByTemplateIDs(ctx, api.Database, []database.Template{template}) if err != nil { httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ @@ -79,11 +74,6 @@ func (api *API) deleteTemplate(rw http.ResponseWriter, r *http.Request) { defer commitAudit() aReq.Old = template - if !api.Authorize(r, rbac.ActionDelete, template) { - httpapi.ResourceNotFound(rw) - return - } - // This is just to get the workspace count, so we use a system context to // return ALL workspaces. Not just workspaces the user can view. // nolint:gocritic @@ -156,11 +146,6 @@ func (api *API) postTemplateByOrganization(rw http.ResponseWriter, r *http.Reque defer commitTemplateAudit() defer commitTemplateVersionAudit() - if !api.Authorize(r, rbac.ActionCreate, rbac.ResourceTemplate.InOrg(organization.ID)) { - httpapi.ResourceNotFound(rw) - return - } - if !httpapi.Read(ctx, rw, r, &createTemplate) { return } @@ -462,11 +447,6 @@ func (api *API) patchTemplateMeta(rw http.ResponseWriter, r *http.Request) { defer commitAudit() aReq.Old = template - if !api.Authorize(r, rbac.ActionUpdate, template) { - httpapi.ResourceNotFound(rw) - return - } - var req codersdk.UpdateTemplateMeta if !httpapi.Read(ctx, rw, r, &req) { return diff --git a/coderd/templateversions.go b/coderd/templateversions.go index 7d9b39226820a..59f42c8150df1 100644 --- a/coderd/templateversions.go +++ b/coderd/templateversions.go @@ -40,15 +40,7 @@ import ( // @Router /templateversions/{templateversion} [get] func (api *API) templateVersion(rw http.ResponseWriter, r *http.Request) { ctx := r.Context() - var ( - templateVersion = httpmw.TemplateVersionParam(r) - template = httpmw.TemplateParam(r) - ) - - if !api.Authorize(r, rbac.ActionRead, templateVersion.RBACObject(template)) { - httpapi.ResourceNotFound(rw) - return - } + templateVersion := httpmw.TemplateVersionParam(r) job, err := api.Database.GetProvisionerJobByID(ctx, templateVersion.JobID) if err != nil { @@ -81,14 +73,7 @@ func (api *API) templateVersion(rw http.ResponseWriter, r *http.Request) { // @Router /templateversions/{templateversion}/cancel [patch] func (api *API) patchCancelTemplateVersion(rw http.ResponseWriter, r *http.Request) { ctx := r.Context() - var ( - templateVersion = httpmw.TemplateVersionParam(r) - template = httpmw.TemplateParam(r) - ) - if !api.Authorize(r, rbac.ActionUpdate, templateVersion.RBACObject(template)) { - httpapi.ResourceNotFound(rw) - return - } + templateVersion := httpmw.TemplateVersionParam(r) job, err := api.Database.GetProvisionerJobByID(ctx, templateVersion.JobID) if err != nil { @@ -144,15 +129,7 @@ func (api *API) patchCancelTemplateVersion(rw http.ResponseWriter, r *http.Reque // @Router /templateversions/{templateversion}/schema [get] func (api *API) templateVersionSchema(rw http.ResponseWriter, r *http.Request) { ctx := r.Context() - var ( - templateVersion = httpmw.TemplateVersionParam(r) - template = httpmw.TemplateParam(r) - ) - - if !api.Authorize(r, rbac.ActionRead, templateVersion.RBACObject(template)) { - httpapi.ResourceNotFound(rw) - return - } + templateVersion := httpmw.TemplateVersionParam(r) job, err := api.Database.GetProvisionerJobByID(ctx, templateVersion.JobID) if err != nil { @@ -205,11 +182,7 @@ func (api *API) templateVersionSchema(rw http.ResponseWriter, r *http.Request) { func (api *API) templateVersionRichParameters(rw http.ResponseWriter, r *http.Request) { ctx := r.Context() templateVersion := httpmw.TemplateVersionParam(r) - template := httpmw.TemplateParam(r) - if !api.Authorize(r, rbac.ActionRead, templateVersion.RBACObject(template)) { - httpapi.ResourceNotFound(rw) - return - } + job, err := api.Database.GetProvisionerJobByID(ctx, templateVersion.JobID) if err != nil { httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ @@ -257,14 +230,8 @@ func (api *API) templateVersionGitAuth(rw http.ResponseWriter, r *http.Request) var ( apiKey = httpmw.APIKey(r) templateVersion = httpmw.TemplateVersionParam(r) - template = httpmw.TemplateParam(r) ) - if !api.Authorize(r, rbac.ActionRead, templateVersion.RBACObject(template)) { - httpapi.ResourceNotFound(rw) - return - } - rawProviders := templateVersion.GitAuthProviders providers := make([]codersdk.TemplateVersionGitAuth, 0) for _, rawProvider := range rawProviders { @@ -356,11 +323,7 @@ func (api *API) templateVersionGitAuth(rw http.ResponseWriter, r *http.Request) func (api *API) templateVersionVariables(rw http.ResponseWriter, r *http.Request) { ctx := r.Context() templateVersion := httpmw.TemplateVersionParam(r) - template := httpmw.TemplateParam(r) - if !api.Authorize(r, rbac.ActionRead, templateVersion.RBACObject(template)) { - httpapi.ResourceNotFound(rw) - return - } + job, err := api.Database.GetProvisionerJobByID(ctx, templateVersion.JobID) if err != nil { httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ @@ -397,14 +360,7 @@ func (api *API) templateVersionVariables(rw http.ResponseWriter, r *http.Request // @Router /templateversions/{templateversion}/parameters [get] func (api *API) templateVersionParameters(rw http.ResponseWriter, r *http.Request) { ctx := r.Context() - var ( - templateVersion = httpmw.TemplateVersionParam(r) - template = httpmw.TemplateParam(r) - ) - if !api.Authorize(r, rbac.ActionRead, templateVersion.RBACObject(template)) { - httpapi.ResourceNotFound(rw) - return - } + templateVersion := httpmw.TemplateVersionParam(r) job, err := api.Database.GetProvisionerJobByID(ctx, templateVersion.JobID) if err != nil { @@ -455,12 +411,8 @@ func (api *API) postTemplateVersionDryRun(rw http.ResponseWriter, r *http.Reques var ( apiKey = httpmw.APIKey(r) templateVersion = httpmw.TemplateVersionParam(r) - template = httpmw.TemplateParam(r) ) - if !api.Authorize(r, rbac.ActionRead, templateVersion.RBACObject(template)) { - httpapi.ResourceNotFound(rw) - return - } + // We use the workspace RBAC check since we don't want to allow dry runs if // the user can't create workspaces. if !api.Authorize(r, rbac.ActionCreate, @@ -678,15 +630,9 @@ func (api *API) fetchTemplateVersionDryRunJob(rw http.ResponseWriter, r *http.Re var ( ctx = r.Context() templateVersion = httpmw.TemplateVersionParam(r) - template = httpmw.TemplateParam(r) jobID = chi.URLParam(r, "jobID") ) - if !api.Authorize(r, rbac.ActionRead, templateVersion.RBACObject(template)) { - httpapi.ResourceNotFound(rw) - return database.ProvisionerJob{}, false - } - jobUUID, err := uuid.Parse(jobID) if err != nil { httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ @@ -754,10 +700,6 @@ func (api *API) fetchTemplateVersionDryRunJob(rw http.ResponseWriter, r *http.Re func (api *API) templateVersionsByTemplate(rw http.ResponseWriter, r *http.Request) { ctx := r.Context() template := httpmw.TemplateParam(r) - if !api.Authorize(r, rbac.ActionRead, template) { - httpapi.ResourceNotFound(rw) - return - } paginationParams, ok := parsePagination(rw, r) if !ok { @@ -860,10 +802,6 @@ func (api *API) templateVersionsByTemplate(rw http.ResponseWriter, r *http.Reque func (api *API) templateVersionByName(rw http.ResponseWriter, r *http.Request) { ctx := r.Context() template := httpmw.TemplateParam(r) - if !api.Authorize(r, rbac.ActionRead, template) { - httpapi.ResourceNotFound(rw) - return - } templateVersionName := chi.URLParam(r, "templateversionname") templateVersion, err := api.Database.GetTemplateVersionByTemplateIDAndName(ctx, database.GetTemplateVersionByTemplateIDAndNameParams{ @@ -939,11 +877,6 @@ func (api *API) templateVersionByOrganizationTemplateAndName(rw http.ResponseWri return } - if !api.Authorize(r, rbac.ActionRead, template) { - httpapi.ResourceNotFound(rw) - return - } - templateVersionName := chi.URLParam(r, "templateversionname") templateVersion, err := api.Database.GetTemplateVersionByTemplateIDAndName(ctx, database.GetTemplateVersionByTemplateIDAndNameParams{ TemplateID: uuid.NullUUID{ @@ -1017,11 +950,6 @@ func (api *API) previousTemplateVersionByOrganizationTemplateAndName(rw http.Res return } - if !api.Authorize(r, rbac.ActionRead, template) { - httpapi.ResourceNotFound(rw) - return - } - templateVersionName := chi.URLParam(r, "templateversionname") templateVersion, err := api.Database.GetTemplateVersionByTemplateIDAndName(ctx, database.GetTemplateVersionByTemplateIDAndNameParams{ TemplateID: uuid.NullUUID{ @@ -1111,11 +1039,6 @@ func (api *API) patchActiveTemplateVersion(rw http.ResponseWriter, r *http.Reque defer commitAudit() aReq.Old = template - if !api.Authorize(r, rbac.ActionUpdate, template) { - httpapi.ResourceNotFound(rw) - return - } - var req codersdk.UpdateActiveTemplateVersion if !httpapi.Read(ctx, rw, r, &req) { return @@ -1203,10 +1126,8 @@ func (api *API) postTemplateVersionsByOrganization(rw http.ResponseWriter, r *ht return } - var template database.Template if req.TemplateID != uuid.Nil { - var err error - template, err = api.Database.GetTemplateByID(ctx, req.TemplateID) + _, err := api.Database.GetTemplateByID(ctx, req.TemplateID) if errors.Is(err, sql.ErrNoRows) { httpapi.Write(ctx, rw, http.StatusNotFound, codersdk.Response{ Message: "Template does not exist.", @@ -1222,17 +1143,6 @@ func (api *API) postTemplateVersionsByOrganization(rw http.ResponseWriter, r *ht } } - if template.ID != uuid.Nil { - if !api.Authorize(r, rbac.ActionCreate, template) { - httpapi.ResourceNotFound(rw) - return - } - } else if !api.Authorize(r, rbac.ActionCreate, rbac.ResourceTemplate.InOrg(organization.ID)) { - // Making a new template version is the same permission as creating a new template. - httpapi.ResourceNotFound(rw) - return - } - // Ensures the "owner" is properly applied. tags := provisionerdserver.MutateTags(apiKey.UserID, req.ProvisionerTags) @@ -1329,11 +1239,6 @@ func (api *API) postTemplateVersionsByOrganization(rw http.ResponseWriter, r *ht } } - if !api.Authorize(r, rbac.ActionRead, file) { - httpapi.ResourceNotFound(rw) - return - } - var templateVersion database.TemplateVersion var provisionerJob database.ProvisionerJob err = api.Database.InTx(func(tx database.Store) error { @@ -1493,14 +1398,8 @@ func (api *API) templateVersionResources(rw http.ResponseWriter, r *http.Request var ( ctx = r.Context() templateVersion = httpmw.TemplateVersionParam(r) - template = httpmw.TemplateParam(r) ) - if !api.Authorize(r, rbac.ActionRead, templateVersion.RBACObject(template)) { - httpapi.ResourceNotFound(rw) - return - } - job, err := api.Database.GetProvisionerJobByID(ctx, templateVersion.JobID) if err != nil { httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ @@ -1532,14 +1431,8 @@ func (api *API) templateVersionLogs(rw http.ResponseWriter, r *http.Request) { var ( ctx = r.Context() templateVersion = httpmw.TemplateVersionParam(r) - template = httpmw.TemplateParam(r) ) - if !api.Authorize(r, rbac.ActionRead, templateVersion.RBACObject(template)) { - httpapi.ResourceNotFound(rw) - return - } - job, err := api.Database.GetProvisionerJobByID(ctx, templateVersion.JobID) if err != nil { httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ diff --git a/coderd/templateversions_test.go b/coderd/templateversions_test.go index 7f9aecbf8e3b1..176c338f5ed1c 100644 --- a/coderd/templateversions_test.go +++ b/coderd/templateversions_test.go @@ -17,6 +17,7 @@ import ( "github.com/coder/coder/coderd/database" "github.com/coder/coder/coderd/gitauth" "github.com/coder/coder/coderd/provisionerdserver" + "github.com/coder/coder/coderd/rbac" "github.com/coder/coder/codersdk" "github.com/coder/coder/examples" "github.com/coder/coder/provisioner/echo" @@ -28,14 +29,19 @@ func TestTemplateVersion(t *testing.T) { t.Parallel() t.Run("Get", func(t *testing.T) { t.Parallel() - client := coderdtest.New(t, nil) + client, _, api := coderdtest.NewWithAPI(t, nil) user := coderdtest.CreateFirstUser(t, client) + authz := coderdtest.AssertRBAC(t, api, client).Reset() + version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, nil) + authz.AssertChecked(t, rbac.ActionCreate, rbac.ResourceTemplate.InOrg(user.OrganizationID)) ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) defer cancel() - _, err := client.TemplateVersion(ctx, version.ID) + authz.Reset() + tv, err := client.TemplateVersion(ctx, version.ID) + authz.AssertChecked(t, rbac.ActionRead, tv) require.NoError(t, err) }) diff --git a/coderd/users.go b/coderd/users.go index 54c225e6f53f8..c39b33b931f9b 100644 --- a/coderd/users.go +++ b/coderd/users.go @@ -279,24 +279,11 @@ func (api *API) postUser(rw http.ResponseWriter, r *http.Request) { }) defer commitAudit() - // Create the user on the site. - if !api.Authorize(r, rbac.ActionCreate, rbac.ResourceUser) { - httpapi.Forbidden(rw) - return - } - var req codersdk.CreateUserRequest if !httpapi.Read(ctx, rw, r, &req) { return } - // Create the organization member in the org. - if !api.Authorize(r, rbac.ActionCreate, - rbac.ResourceOrganizationMember.InOrg(req.OrganizationID)) { - httpapi.ResourceNotFound(rw) - return - } - // If password auth is disabled, don't allow new users to be // created with a password! if api.DeploymentValues.DisablePasswordAuth { @@ -357,6 +344,12 @@ func (api *API) postUser(rw http.ResponseWriter, r *http.Request) { CreateUserRequest: req, LoginType: database.LoginTypePassword, }) + if dbauthz.IsNotAuthorizedError(err) { + httpapi.Write(ctx, rw, http.StatusForbidden, codersdk.Response{ + Message: "You are not authorized to create users.", + }) + return + } if err != nil { httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ Message: "Internal error creating user.", @@ -397,11 +390,6 @@ func (api *API) deleteUser(rw http.ResponseWriter, r *http.Request) { aReq.Old = user defer commitAudit() - if !api.Authorize(r, rbac.ActionDelete, rbac.ResourceUser) { - httpapi.Forbidden(rw) - return - } - if auth.Actor.ID == user.ID.String() { httpapi.Write(ctx, rw, http.StatusForbidden, codersdk.Response{ Message: "You cannot delete yourself!", @@ -430,6 +418,10 @@ func (api *API) deleteUser(rw http.ResponseWriter, r *http.Request) { ID: user.ID, Deleted: true, }) + if dbauthz.IsNotAuthorizedError(err) { + httpapi.Forbidden(rw) + return + } if err != nil { httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ Message: "Internal error deleting user.", @@ -459,12 +451,6 @@ func (api *API) userByName(rw http.ResponseWriter, r *http.Request) { ctx := r.Context() user := httpmw.UserParam(r) organizationIDs, err := userOrganizationIDs(ctx, api, user) - - if !api.Authorize(r, rbac.ActionRead, user) { - httpapi.ResourceNotFound(rw) - return - } - if err != nil { httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ Message: "Internal error fetching user's organizations.", @@ -501,11 +487,6 @@ func (api *API) putUserProfile(rw http.ResponseWriter, r *http.Request) { defer commitAudit() aReq.Old = user - if !api.Authorize(r, rbac.ActionUpdate, user) { - httpapi.ResourceNotFound(rw) - return - } - var params codersdk.UpdateUserProfileRequest if !httpapi.Read(ctx, rw, r, ¶ms) { return @@ -607,11 +588,6 @@ func (api *API) putUserStatus(status database.UserStatus) func(rw http.ResponseW defer commitAudit() aReq.Old = user - if !api.Authorize(r, rbac.ActionDelete, user) { - httpapi.ResourceNotFound(rw) - return - } - if status == database.UserStatusSuspended { // There are some manual protections when suspending a user to // prevent certain situations. @@ -684,11 +660,6 @@ func (api *API) putUserPassword(rw http.ResponseWriter, r *http.Request) { defer commitAudit() aReq.Old = user - if !api.Authorize(r, rbac.ActionUpdate, user.UserDataRBACObject()) { - httpapi.ResourceNotFound(rw) - return - } - if !httpapi.Read(ctx, rw, r, ¶ms) { return } @@ -708,12 +679,7 @@ func (api *API) putUserPassword(rw http.ResponseWriter, r *http.Request) { } // admins can change passwords without sending old_password - if params.OldPassword == "" { - if !api.Authorize(r, rbac.ActionUpdate, user) { - httpapi.Forbidden(rw) - return - } - } else { + if params.OldPassword != "" { // if they send something let's validate it ok, err := userpassword.Compare(string(user.HashedPassword), params.OldPassword) if err != nil { @@ -816,16 +782,6 @@ func (api *API) userRoles(rw http.ResponseWriter, r *http.Request) { return } - // Only include ones we can read from RBAC. - memberships, err = AuthorizeFilter(api.HTTPAuth, r, rbac.ActionRead, memberships) - if err != nil { - httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ - Message: "Internal error fetching memberships.", - Detail: err.Error(), - }) - return - } - for _, mem := range memberships { // If we can read the org member, include the roles. if err == nil { @@ -851,7 +807,6 @@ func (api *API) putUserRoles(rw http.ResponseWriter, r *http.Request) { ctx = r.Context() // User is the user to modify. user = httpmw.UserParam(r) - actorRoles = httpmw.UserAuthorization(r) apiKey = httpmw.APIKey(r) auditor = *api.Auditor.Load() aReq, commitAudit = audit.InitRequest[database.User](rw, &audit.RequestParams{ @@ -876,39 +831,14 @@ func (api *API) putUserRoles(rw http.ResponseWriter, r *http.Request) { return } - if !api.Authorize(r, rbac.ActionRead, user) { - httpapi.ResourceNotFound(rw) - return - } - - // The member role is always implied. - impliedTypes := append(params.Roles, rbac.RoleMember()) - added, removed := rbac.ChangeRoleSet(user.RBACRoles, impliedTypes) - - // Assigning a role requires the create permission. - if len(added) > 0 && !api.Authorize(r, rbac.ActionCreate, rbac.ResourceRoleAssignment) { - httpapi.Forbidden(rw) - return - } - - // Removing a role requires the delete permission. - if len(removed) > 0 && !api.Authorize(r, rbac.ActionDelete, rbac.ResourceRoleAssignment) { - httpapi.Forbidden(rw) - return - } - - // Just treat adding & removing as "assigning" for now. - for _, roleName := range append(added, removed...) { - if !rbac.CanAssignRole(actorRoles.Actor.Roles, roleName) { - httpapi.Forbidden(rw) - return - } - } - updatedUser, err := api.updateSiteUserRoles(ctx, database.UpdateUserRolesParams{ GrantedRoles: params.Roles, ID: user.ID, }) + if dbauthz.IsNotAuthorizedError(err) { + httpapi.Forbidden(rw) + return + } if err != nil { httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ Message: err.Error(), @@ -1020,11 +950,6 @@ func (api *API) organizationByUserAndName(rw http.ResponseWriter, r *http.Reques return } - if !api.Authorize(r, rbac.ActionRead, organization) { - httpapi.ResourceNotFound(rw) - return - } - httpapi.Write(ctx, rw, http.StatusOK, convertOrganization(organization)) } diff --git a/coderd/users_test.go b/coderd/users_test.go index 5e20f78bfb594..22360744e4d79 100644 --- a/coderd/users_test.go +++ b/coderd/users_test.go @@ -288,21 +288,27 @@ func TestDeleteUser(t *testing.T) { t.Parallel() t.Run("Works", func(t *testing.T) { t.Parallel() - api := coderdtest.New(t, nil) - user := coderdtest.CreateFirstUser(t, api) - _, another := coderdtest.CreateAnotherUser(t, api, user.OrganizationID) - err := api.DeleteUser(context.Background(), another.ID) + client, _, api := coderdtest.NewWithAPI(t, nil) + user := coderdtest.CreateFirstUser(t, client) + authz := coderdtest.AssertRBAC(t, api, client) + + _, another := coderdtest.CreateAnotherUser(t, client, user.OrganizationID) + err := client.DeleteUser(context.Background(), another.ID) require.NoError(t, err) // Attempt to create a user with the same email and username, and delete them again. - another, err = api.CreateUser(context.Background(), codersdk.CreateUserRequest{ + another, err = client.CreateUser(context.Background(), codersdk.CreateUserRequest{ Email: another.Email, Username: another.Username, Password: "SomeSecurePassword!", OrganizationID: user.OrganizationID, }) require.NoError(t, err) - err = api.DeleteUser(context.Background(), another.ID) + err = client.DeleteUser(context.Background(), another.ID) require.NoError(t, err) + + // RBAC checks + authz.AssertChecked(t, rbac.ActionCreate, rbac.ResourceUser) + authz.AssertChecked(t, rbac.ActionDelete, another) }) t.Run("NoPermission", func(t *testing.T) { t.Parallel() @@ -469,7 +475,7 @@ func TestPostUsers(t *testing.T) { }) var apiErr *codersdk.Error require.ErrorAs(t, err, &apiErr) - require.Equal(t, http.StatusForbidden, apiErr.StatusCode()) + require.Equal(t, http.StatusNotFound, apiErr.StatusCode()) }) t.Run("Create", func(t *testing.T) { @@ -1491,15 +1497,15 @@ func TestPaginatedUsers(t *testing.T) { coderdtest.CreateFirstUser(t, client) // This test takes longer than a long time. - ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong*2) + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong*4) t.Cleanup(cancel) me, err := client.User(ctx, codersdk.Me) require.NoError(t, err) orgID := me.OrganizationIDs[0] - // When 100 users exist - total := 100 + // When 50 users exist + total := 50 allUsers := make([]codersdk.User, total+1) // +1 forme allUsers[0] = me specialUsers := make([]codersdk.User, total/2) diff --git a/coderd/workspaceagents.go b/coderd/workspaceagents.go index 098f83b5358ec..7cf62d07ebd91 100644 --- a/coderd/workspaceagents.go +++ b/coderd/workspaceagents.go @@ -51,11 +51,7 @@ import ( func (api *API) workspaceAgent(rw http.ResponseWriter, r *http.Request) { ctx := r.Context() workspaceAgent := httpmw.WorkspaceAgentParam(r) - workspace := httpmw.WorkspaceParam(r) - if !api.Authorize(r, rbac.ActionRead, workspace) { - httpapi.ResourceNotFound(rw) - return - } + dbApps, err := api.Database.GetWorkspaceAppsByAgentID(ctx, workspaceAgent.ID) if err != nil && !xerrors.Is(err, sql.ErrNoRows) { httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ @@ -325,12 +321,7 @@ func (api *API) workspaceAgentPTY(rw http.ResponseWriter, r *http.Request) { // @Router /workspaceagents/{workspaceagent}/listening-ports [get] func (api *API) workspaceAgentListeningPorts(rw http.ResponseWriter, r *http.Request) { ctx := r.Context() - workspace := httpmw.WorkspaceParam(r) workspaceAgent := httpmw.WorkspaceAgentParam(r) - if !api.Authorize(r, rbac.ActionRead, workspace) { - httpapi.ResourceNotFound(rw) - return - } apiAgent, err := convertWorkspaceAgent( api.DERPMap, *api.TailnetCoordinator.Load(), workspaceAgent, nil, api.AgentInactiveDisconnectTimeout, @@ -492,11 +483,7 @@ func (api *API) dialWorkspaceAgentTailnet(r *http.Request, agentID uuid.UUID) (* // @Router /workspaceagents/{workspaceagent}/connection [get] func (api *API) workspaceAgentConnection(rw http.ResponseWriter, r *http.Request) { ctx := r.Context() - workspace := httpmw.WorkspaceParam(r) - if !api.Authorize(r, rbac.ActionRead, workspace) { - httpapi.ResourceNotFound(rw) - return - } + httpapi.Write(ctx, rw, http.StatusOK, codersdk.WorkspaceAgentConnectionInfo{ DERPMap: api.DERPMap, }) diff --git a/coderd/workspacebuilds.go b/coderd/workspacebuilds.go index 125bad55b295d..8a238b6994f4d 100644 --- a/coderd/workspacebuilds.go +++ b/coderd/workspacebuilds.go @@ -37,11 +37,6 @@ func (api *API) workspaceBuild(rw http.ResponseWriter, r *http.Request) { workspaceBuild := httpmw.WorkspaceBuildParam(r) workspace := httpmw.WorkspaceParam(r) - if !api.Authorize(r, rbac.ActionRead, workspace) { - httpapi.ResourceNotFound(rw) - return - } - data, err := api.workspaceBuildsData(ctx, []database.Workspace{workspace}, []database.WorkspaceBuild{workspaceBuild}) if err != nil { httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ @@ -107,11 +102,6 @@ func (api *API) workspaceBuilds(rw http.ResponseWriter, r *http.Request) { ctx := r.Context() workspace := httpmw.WorkspaceParam(r) - if !api.Authorize(r, rbac.ActionRead, workspace) { - httpapi.ResourceNotFound(rw) - return - } - paginationParams, ok := parsePagination(rw, r) if !ok { return @@ -249,11 +239,6 @@ func (api *API) workspaceBuildByBuildNumber(rw http.ResponseWriter, r *http.Requ return } - if !api.Authorize(r, rbac.ActionRead, workspace) { - httpapi.ResourceNotFound(rw) - return - } - workspaceBuild, err := api.Database.GetWorkspaceBuildByWorkspaceIDAndBuildNumber(ctx, database.GetWorkspaceBuildByWorkspaceIDAndBuildNumberParams{ WorkspaceID: workspace.ID, BuildNumber: int32(buildNumber), @@ -326,7 +311,9 @@ func (api *API) postWorkspaceBuilds(rw http.ResponseWriter, r *http.Request) { return } - // Rbac action depends on the transition + // Doing this up front saves a lot of work if the user doesn't have permission. + // This is checked again in the dbauthz layer, but the check is cached + // and will be a noop later. var action rbac.Action switch createBuild.Transition { case codersdk.WorkspaceTransitionDelete: @@ -713,11 +700,6 @@ func (api *API) patchCancelWorkspaceBuild(rw http.ResponseWriter, r *http.Reques return } - if !api.Authorize(r, rbac.ActionUpdate, workspace) { - httpapi.ResourceNotFound(rw) - return - } - valid, err := api.verifyUserCanCancelWorkspaceBuilds(ctx, httpmw.APIKey(r).UserID, workspace.TemplateID) if err != nil { httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ @@ -808,18 +790,6 @@ func (api *API) verifyUserCanCancelWorkspaceBuilds(ctx context.Context, userID u func (api *API) workspaceBuildResources(rw http.ResponseWriter, r *http.Request) { ctx := r.Context() workspaceBuild := httpmw.WorkspaceBuildParam(r) - workspace, err := api.Database.GetWorkspaceByID(ctx, workspaceBuild.WorkspaceID) - if err != nil { - httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ - Message: "No workspace exists for this job.", - }) - return - } - - if !api.Authorize(r, rbac.ActionRead, workspace) { - httpapi.ResourceNotFound(rw) - return - } job, err := api.Database.GetProvisionerJobByID(ctx, workspaceBuild.JobID) if err != nil { @@ -843,18 +813,6 @@ func (api *API) workspaceBuildResources(rw http.ResponseWriter, r *http.Request) func (api *API) workspaceBuildParameters(rw http.ResponseWriter, r *http.Request) { ctx := r.Context() workspaceBuild := httpmw.WorkspaceBuildParam(r) - workspace, err := api.Database.GetWorkspaceByID(ctx, workspaceBuild.WorkspaceID) - if err != nil { - httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ - Message: "No workspace exists for this job.", - }) - return - } - - if !api.Authorize(r, rbac.ActionRead, workspace) { - httpapi.ResourceNotFound(rw) - return - } parameters, err := api.Database.GetWorkspaceBuildParameters(ctx, workspaceBuild.ID) if err != nil { @@ -882,18 +840,6 @@ func (api *API) workspaceBuildParameters(rw http.ResponseWriter, r *http.Request func (api *API) workspaceBuildLogs(rw http.ResponseWriter, r *http.Request) { ctx := r.Context() workspaceBuild := httpmw.WorkspaceBuildParam(r) - workspace, err := api.Database.GetWorkspaceByID(ctx, workspaceBuild.WorkspaceID) - if err != nil { - httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ - Message: "No workspace exists for this job.", - }) - return - } - - if !api.Authorize(r, rbac.ActionRead, workspace) { - httpapi.ResourceNotFound(rw) - return - } job, err := api.Database.GetProvisionerJobByID(ctx, workspaceBuild.JobID) if err != nil { diff --git a/coderd/workspaces.go b/coderd/workspaces.go index f97759d0e9d7e..f84d7d6d9d345 100644 --- a/coderd/workspaces.go +++ b/coderd/workspaces.go @@ -53,10 +53,6 @@ var ( func (api *API) workspace(rw http.ResponseWriter, r *http.Request) { ctx := r.Context() workspace := httpmw.WorkspaceParam(r) - if !api.Authorize(r, rbac.ActionRead, workspace) { - httpapi.ResourceNotFound(rw) - return - } var ( deletedStr = r.URL.Query().Get("include_deleted") @@ -242,10 +238,6 @@ func (api *API) workspaceByOwnerAndName(rw http.ResponseWriter, r *http.Request) }) return } - if !api.Authorize(r, rbac.ActionRead, workspace) { - httpapi.ResourceNotFound(rw) - return - } data, err := api.workspaceData(ctx, []database.Workspace{workspace}) if err != nil { @@ -309,6 +301,7 @@ func (api *API) postWorkspacesByOrganization(rw http.ResponseWriter, r *http.Req defer commitAudit() + // Do this upfront to save work. if !api.Authorize(r, rbac.ActionCreate, rbac.ResourceWorkspace.InOrg(organization.ID).WithOwner(user.ID.String())) { httpapi.ResourceNotFound(rw) @@ -344,10 +337,6 @@ func (api *API) postWorkspacesByOrganization(rw http.ResponseWriter, r *http.Req }) return } - if !api.Authorize(r, rbac.ActionRead, template) { - httpapi.ResourceNotFound(rw) - return - } if organization.ID != template.OrganizationID { httpapi.Write(ctx, rw, http.StatusUnauthorized, codersdk.Response{ @@ -648,11 +637,6 @@ func (api *API) patchWorkspace(rw http.ResponseWriter, r *http.Request) { defer commitAudit() aReq.Old = workspace - if !api.Authorize(r, rbac.ActionUpdate, workspace) { - httpapi.ResourceNotFound(rw) - return - } - var req codersdk.UpdateWorkspaceRequest if !httpapi.Read(ctx, rw, r, &req) { return @@ -737,11 +721,6 @@ func (api *API) putWorkspaceAutostart(rw http.ResponseWriter, r *http.Request) { defer commitAudit() aReq.Old = workspace - if !api.Authorize(r, rbac.ActionUpdate, workspace) { - httpapi.ResourceNotFound(rw) - return - } - var req codersdk.UpdateWorkspaceAutostartRequest if !httpapi.Read(ctx, rw, r, &req) { return @@ -799,11 +778,6 @@ func (api *API) putWorkspaceTTL(rw http.ResponseWriter, r *http.Request) { defer commitAudit() aReq.Old = workspace - if !api.Authorize(r, rbac.ActionUpdate, workspace) { - httpapi.ResourceNotFound(rw) - return - } - var req codersdk.UpdateWorkspaceTTLRequest if !httpapi.Read(ctx, rw, r, &req) { return @@ -870,11 +844,6 @@ func (api *API) putExtendWorkspace(rw http.ResponseWriter, r *http.Request) { ctx := r.Context() workspace := httpmw.WorkspaceParam(r) - if !api.Authorize(r, rbac.ActionUpdate, workspace) { - httpapi.ResourceNotFound(rw) - return - } - var req codersdk.PutExtendWorkspaceRequest if !httpapi.Read(ctx, rw, r, &req) { return @@ -964,10 +933,6 @@ func (api *API) putExtendWorkspace(rw http.ResponseWriter, r *http.Request) { func (api *API) watchWorkspace(rw http.ResponseWriter, r *http.Request) { ctx := r.Context() workspace := httpmw.WorkspaceParam(r) - if !api.Authorize(r, rbac.ActionRead, workspace) { - httpapi.ResourceNotFound(rw) - return - } sendEvent, senderClosed, err := httpapi.ServerSentEventSender(rw, r) if err != nil { @@ -1183,6 +1148,7 @@ func convertWorkspace( UpdatedAt: workspace.UpdatedAt, OwnerID: workspace.OwnerID, OwnerName: owner.Username, + OrganizationID: workspace.OrganizationID, TemplateID: workspace.TemplateID, LatestBuild: workspaceBuild, TemplateName: template.Name, diff --git a/coderd/workspaces_test.go b/coderd/workspaces_test.go index e7442751ed62d..54b9aaf3cf049 100644 --- a/coderd/workspaces_test.go +++ b/coderd/workspaces_test.go @@ -36,8 +36,9 @@ func TestWorkspace(t *testing.T) { t.Run("OK", func(t *testing.T) { t.Parallel() - client := coderdtest.New(t, &coderdtest.Options{IncludeProvisionerDaemon: true}) + client, _, api := coderdtest.NewWithAPI(t, &coderdtest.Options{IncludeProvisionerDaemon: true}) user := coderdtest.CreateFirstUser(t, client) + authz := coderdtest.AssertRBAC(t, api, client) version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, nil) coderdtest.AwaitTemplateVersionJob(t, client, version.ID) template := coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID) @@ -46,7 +47,9 @@ func TestWorkspace(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) defer cancel() + authz.Reset() // Reset all previous checks done in setup. ws, err := client.Workspace(ctx, workspace.ID) + authz.AssertChecked(t, rbac.ActionRead, ws) require.NoError(t, err) require.Equal(t, user.UserID, ws.LatestBuild.InitiatorID) require.Equal(t, codersdk.BuildReasonInitiator, ws.LatestBuild.Reason) diff --git a/codersdk/deployment.go b/codersdk/deployment.go index 8b822219cde96..dba09776a87b1 100644 --- a/codersdk/deployment.go +++ b/codersdk/deployment.go @@ -1516,10 +1516,6 @@ func (c *Client) BuildInfo(ctx context.Context) (BuildInfoResponse, error) { type Experiment string const ( - // ExperimentAuthzQuerier is an internal experiment that enables the ExperimentAuthzQuerier - // interface for all RBAC operations. NOT READY FOR PRODUCTION USE. - ExperimentAuthzQuerier Experiment = "authz_querier" - // ExperimentTemplateEditor is an internal experiment that enables the template editor // for all users. ExperimentTemplateEditor Experiment = "template_editor" diff --git a/codersdk/workspaces.go b/codersdk/workspaces.go index c41fb91f9752c..fd1fe4d02811e 100644 --- a/codersdk/workspaces.go +++ b/codersdk/workspaces.go @@ -22,6 +22,7 @@ type Workspace struct { UpdatedAt time.Time `json:"updated_at" format:"date-time"` OwnerID uuid.UUID `json:"owner_id" format:"uuid"` OwnerName string `json:"owner_name"` + OrganizationID uuid.UUID `json:"organization_id" format:"uuid"` TemplateID uuid.UUID `json:"template_id" format:"uuid"` TemplateName string `json:"template_name"` TemplateDisplayName string `json:"template_display_name"` diff --git a/docs/api/general.md b/docs/api/general.md index 23c7ba511fb07..0676876b4f277 100644 --- a/docs/api/general.md +++ b/docs/api/general.md @@ -507,7 +507,7 @@ curl -X GET http://coder-server:8080/api/v2/experiments \ > 200 Response ```json -["authz_querier"] +["template_editor"] ``` ### Responses diff --git a/docs/api/schemas.md b/docs/api/schemas.md index cff9081317ba7..441feb2201b44 100644 --- a/docs/api/schemas.md +++ b/docs/api/schemas.md @@ -2342,7 +2342,7 @@ CreateParameterRequest is a structure used to create a new parameter value for a ## codersdk.Experiment ```json -"authz_querier" +"template_editor" ``` ### Properties @@ -2351,7 +2351,6 @@ CreateParameterRequest is a structure used to create a new parameter value for a | Value | | ----------------- | -| `authz_querier` | | `template_editor` | ## codersdk.Feature @@ -4338,6 +4337,7 @@ Parameter represents a set value for the scope. "workspace_owner_name": "string" }, "name": "string", + "organization_id": "7c60d51f-b44e-4682-87d6-449835ea4de6", "outdated": true, "owner_id": "8826ee2e-7933-4665-aef2-2393f84a0d05", "owner_name": "string", @@ -4361,6 +4361,7 @@ Parameter represents a set value for the scope. | `last_used_at` | string | false | | | | `latest_build` | [codersdk.WorkspaceBuild](#codersdkworkspacebuild) | false | | | | `name` | string | false | | | +| `organization_id` | string | false | | | | `outdated` | boolean | false | | | | `owner_id` | string | false | | | | `owner_name` | string | false | | | @@ -5222,6 +5223,7 @@ Parameter represents a set value for the scope. "workspace_owner_name": "string" }, "name": "string", + "organization_id": "7c60d51f-b44e-4682-87d6-449835ea4de6", "outdated": true, "owner_id": "8826ee2e-7933-4665-aef2-2393f84a0d05", "owner_name": "string", diff --git a/docs/api/workspaces.md b/docs/api/workspaces.md index ca19ba4f54dcb..e37c0ff7cd9ca 100644 --- a/docs/api/workspaces.md +++ b/docs/api/workspaces.md @@ -175,6 +175,7 @@ curl -X POST http://coder-server:8080/api/v2/organizations/{organization}/member "workspace_owner_name": "string" }, "name": "string", + "organization_id": "7c60d51f-b44e-4682-87d6-449835ea4de6", "outdated": true, "owner_id": "8826ee2e-7933-4665-aef2-2393f84a0d05", "owner_name": "string", @@ -344,6 +345,7 @@ curl -X GET http://coder-server:8080/api/v2/users/{user}/workspace/{workspacenam "workspace_owner_name": "string" }, "name": "string", + "organization_id": "7c60d51f-b44e-4682-87d6-449835ea4de6", "outdated": true, "owner_id": "8826ee2e-7933-4665-aef2-2393f84a0d05", "owner_name": "string", @@ -532,6 +534,7 @@ curl -X GET http://coder-server:8080/api/v2/workspaces \ "workspace_owner_name": "string" }, "name": "string", + "organization_id": "7c60d51f-b44e-4682-87d6-449835ea4de6", "outdated": true, "owner_id": "8826ee2e-7933-4665-aef2-2393f84a0d05", "owner_name": "string", @@ -702,6 +705,7 @@ curl -X GET http://coder-server:8080/api/v2/workspaces/{workspace} \ "workspace_owner_name": "string" }, "name": "string", + "organization_id": "7c60d51f-b44e-4682-87d6-449835ea4de6", "outdated": true, "owner_id": "8826ee2e-7933-4665-aef2-2393f84a0d05", "owner_name": "string", diff --git a/enterprise/coderd/coderdenttest/coderdenttest_test.go b/enterprise/coderd/coderdenttest/coderdenttest_test.go index aa32c582e67f9..04d6dab6e96a5 100644 --- a/enterprise/coderd/coderdenttest/coderdenttest_test.go +++ b/enterprise/coderd/coderdenttest/coderdenttest_test.go @@ -1,118 +1,12 @@ package coderdenttest_test import ( - "context" - "fmt" - "net/http" - "os" - "strings" "testing" - "github.com/stretchr/testify/require" - - "github.com/coder/coder/coderd/coderdtest" - "github.com/coder/coder/coderd/rbac" - "github.com/coder/coder/codersdk" "github.com/coder/coder/enterprise/coderd/coderdenttest" - "github.com/coder/coder/enterprise/coderd/license" - "github.com/coder/coder/testutil" ) func TestNew(t *testing.T) { t.Parallel() _ = coderdenttest.New(t, nil) } - -func TestAuthorizeAllEndpoints(t *testing.T) { - if strings.Contains(os.Getenv("CODER_EXPERIMENTS_TEST"), string(codersdk.ExperimentAuthzQuerier)) { - t.Skip("Skipping TestAuthorizeAllEndpoints for authz_querier experiment") - } - t.Parallel() - client, _, api := coderdenttest.NewWithAPI(t, &coderdenttest.Options{ - Options: &coderdtest.Options{ - // Required for any subdomain-based proxy tests to pass. - AppHostname: "*.test.coder.com", - Authorizer: &coderdtest.RecordingAuthorizer{Wrapped: &coderdtest.FakeAuthorizer{}}, - IncludeProvisionerDaemon: true, - }, - }) - ctx, _ := testutil.Context(t) - admin := coderdtest.CreateFirstUser(t, client) - lic := coderdenttest.AddLicense(t, client, coderdenttest.LicenseOptions{ - Features: license.Features{ - codersdk.FeatureTemplateRBAC: 1, - codersdk.FeatureExternalProvisionerDaemons: 1, - }, - }) - group, err := client.CreateGroup(ctx, admin.OrganizationID, codersdk.CreateGroupRequest{ - Name: "testgroup", - }) - require.NoError(t, err) - - groupObj := rbac.ResourceGroup.WithID(group.ID).InOrg(admin.OrganizationID) - a := coderdtest.NewAuthTester(ctx, t, client, api.AGPL, admin) - a.URLParams["licenses/{id}"] = fmt.Sprintf("licenses/%d", lic.ID) - a.URLParams["groups/{group}"] = fmt.Sprintf("groups/%s", group.ID.String()) - a.URLParams["{groupName}"] = group.Name - - skipRoutes, assertRoute := coderdtest.AGPLRoutes(a) - skipRoutes["GET:/api/v2/organizations/{organization}/provisionerdaemons/serve"] = "This route checks for RBAC dependent on input parameters!" - skipRoutes["GET:/api/v2/appearance/"] = "This route is available to all users" - - assertRoute["GET:/api/v2/entitlements"] = coderdtest.RouteCheck{ - NoAuthorize: true, - } - assertRoute["POST:/api/v2/licenses"] = coderdtest.RouteCheck{ - AssertAction: rbac.ActionCreate, - AssertObject: rbac.ResourceLicense, - } - assertRoute["GET:/api/v2/licenses"] = coderdtest.RouteCheck{ - StatusCode: http.StatusOK, - AssertAction: rbac.ActionRead, - AssertObject: rbac.ResourceLicense, - } - assertRoute["GET:/api/v2/replicas"] = coderdtest.RouteCheck{ - AssertAction: rbac.ActionRead, - AssertObject: rbac.ResourceReplicas, - } - assertRoute["DELETE:/api/v2/licenses/{id}"] = coderdtest.RouteCheck{ - AssertAction: rbac.ActionDelete, - AssertObject: rbac.ResourceLicense, - } - assertRoute["GET:/api/v2/templates/{template}/acl"] = coderdtest.RouteCheck{ - AssertAction: rbac.ActionRead, - AssertObject: rbac.ResourceTemplate, - } - assertRoute["PATCH:/api/v2/templates/{template}/acl"] = coderdtest.RouteCheck{ - AssertAction: rbac.ActionCreate, - AssertObject: rbac.ResourceTemplate, - } - assertRoute["GET:/api/v2/organizations/{organization}/groups"] = coderdtest.RouteCheck{ - StatusCode: http.StatusOK, - AssertAction: rbac.ActionRead, - AssertObject: groupObj, - } - assertRoute["GET:/api/v2/organizations/{organization}/groups/{groupName}"] = coderdtest.RouteCheck{ - AssertAction: rbac.ActionRead, - AssertObject: groupObj, - } - assertRoute["GET:/api/v2/organizations/{organization}/provisionerdaemons"] = coderdtest.RouteCheck{ - AssertAction: rbac.ActionRead, - AssertObject: rbac.ResourceProvisionerDaemon, - StatusCode: http.StatusOK, - } - assertRoute["GET:/api/v2/groups/{group}"] = coderdtest.RouteCheck{ - AssertAction: rbac.ActionRead, - AssertObject: groupObj, - } - assertRoute["PATCH:/api/v2/groups/{group}"] = coderdtest.RouteCheck{ - AssertAction: rbac.ActionUpdate, - AssertObject: groupObj, - } - assertRoute["DELETE:/api/v2/groups/{group}"] = coderdtest.RouteCheck{ - AssertAction: rbac.ActionDelete, - AssertObject: groupObj, - } - - a.Test(context.Background(), assertRoute, skipRoutes) -} diff --git a/enterprise/coderd/groups.go b/enterprise/coderd/groups.go index b75f29211a13b..de9d280fa28cb 100644 --- a/enterprise/coderd/groups.go +++ b/enterprise/coderd/groups.go @@ -41,11 +41,6 @@ func (api *API) postGroupByOrganization(rw http.ResponseWriter, r *http.Request) ) defer commitAudit() - if !api.Authorize(r, rbac.ActionCreate, rbac.ResourceGroup.InOrg(org.ID)) { - http.NotFound(rw, r) - return - } - var req codersdk.CreateGroupRequest if !httpapi.Read(ctx, rw, r, &req) { return @@ -112,11 +107,6 @@ func (api *API) patchGroup(rw http.ResponseWriter, r *http.Request) { aReq.Old = group.Auditable(currentMembers) - if !api.Authorize(r, rbac.ActionUpdate, group) { - http.NotFound(rw, r) - return - } - var req codersdk.PatchGroupRequest if !httpapi.Read(ctx, rw, r, &req) { return @@ -294,11 +284,6 @@ func (api *API) deleteGroup(rw http.ResponseWriter, r *http.Request) { aReq.Old = group.Auditable(groupMembers) - if !api.Authorize(r, rbac.ActionDelete, group) { - httpapi.ResourceNotFound(rw) - return - } - if group.Name == database.AllUsersGroup { httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ Message: fmt.Sprintf("%q is a reserved group and cannot be deleted!", database.AllUsersGroup), @@ -344,11 +329,6 @@ func (api *API) group(rw http.ResponseWriter, r *http.Request) { group = httpmw.GroupParam(r) ) - if !api.Authorize(r, rbac.ActionRead, group) { - httpapi.ResourceNotFound(rw) - return - } - users, err := api.Database.GetGroupMembers(ctx, group.ID) if err != nil && !xerrors.Is(err, sql.ErrNoRows) { httpapi.InternalServerError(rw, err) diff --git a/enterprise/coderd/templates.go b/enterprise/coderd/templates.go index c75bd9abdfb4d..42dee6a2f0960 100644 --- a/enterprise/coderd/templates.go +++ b/enterprise/coderd/templates.go @@ -32,11 +32,6 @@ func (api *API) templateACL(rw http.ResponseWriter, r *http.Request) { template = httpmw.TemplateParam(r) ) - if !api.Authorize(r, rbac.ActionRead, template) { - httpapi.ResourceNotFound(rw) - return - } - users, err := api.Database.GetTemplateUserRoles(ctx, template.ID) if err != nil { httpapi.InternalServerError(rw, err) @@ -120,13 +115,6 @@ func (api *API) patchTemplateACL(rw http.ResponseWriter, r *http.Request) { defer commitAudit() aReq.Old = template - // Only users who are able to create templates (aka template admins) - // are able to control permissions. - if !api.Authorize(r, rbac.ActionCreate, template) { - httpapi.ResourceNotFound(rw) - return - } - var req codersdk.UpdateTemplateACL if !httpapi.Read(ctx, rw, r, &req) { return diff --git a/enterprise/coderd/templates_test.go b/enterprise/coderd/templates_test.go index 7c58bd9527514..65989da54383f 100644 --- a/enterprise/coderd/templates_test.go +++ b/enterprise/coderd/templates_test.go @@ -786,7 +786,7 @@ func TestUpdateTemplateACL(t *testing.T) { err = client2.UpdateTemplateACL(ctx, template.ID, req) require.Error(t, err) cerr, _ := codersdk.AsError(err) - require.Equal(t, http.StatusNotFound, cerr.StatusCode()) + require.Equal(t, http.StatusInternalServerError, cerr.StatusCode()) }) t.Run("RegularUserWithAdminCanUpdate", func(t *testing.T) { @@ -984,7 +984,9 @@ func TestUpdateTemplateACL(t *testing.T) { //nolint:tparallel func TestTemplateAccess(t *testing.T) { t.Parallel() - ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) + // TODO: This context is for all the subtests. Each subtest should have its + // own context. + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong*3) t.Cleanup(cancel) ownerClient := coderdenttest.New(t, nil) diff --git a/site/src/api/typesGenerated.ts b/site/src/api/typesGenerated.ts index 7b2c7f7ba208c..4b31afb1f0ee0 100644 --- a/site/src/api/typesGenerated.ts +++ b/site/src/api/typesGenerated.ts @@ -1002,6 +1002,7 @@ export interface Workspace { readonly updated_at: string readonly owner_id: string readonly owner_name: string + readonly organization_id: string readonly template_id: string readonly template_name: string readonly template_display_name: string @@ -1215,8 +1216,8 @@ export const Entitlements: Entitlement[] = [ ] // From codersdk/deployment.go -export type Experiment = "authz_querier" | "template_editor" -export const Experiments: Experiment[] = ["authz_querier", "template_editor"] +export type Experiment = "template_editor" +export const Experiments: Experiment[] = ["template_editor"] // From codersdk/deployment.go export type FeatureName =