diff --git a/cli/autostart_test.go b/cli/autostart_test.go index fd295fa8e49a7..f5bab5a593e78 100644 --- a/cli/autostart_test.go +++ b/cli/autostart_test.go @@ -109,7 +109,7 @@ func TestAutostart(t *testing.T) { clitest.SetupConfig(t, client, root) err := cmd.Execute() - require.ErrorContains(t, err, "status code 403: Forbidden", "unexpected error") + require.ErrorContains(t, err, "status code 404", "unexpected error") }) t.Run("Disable_NotFound", func(t *testing.T) { @@ -126,7 +126,7 @@ func TestAutostart(t *testing.T) { clitest.SetupConfig(t, client, root) err := cmd.Execute() - require.ErrorContains(t, err, "status code 403: Forbidden", "unexpected error") + require.ErrorContains(t, err, "status code 404:", "unexpected error") }) t.Run("Enable_DefaultSchedule", func(t *testing.T) { diff --git a/cli/ttl_test.go b/cli/ttl_test.go index 92ca201c81a44..6ad0c29151d86 100644 --- a/cli/ttl_test.go +++ b/cli/ttl_test.go @@ -178,7 +178,7 @@ func TestTTL(t *testing.T) { clitest.SetupConfig(t, client, root) err := cmd.Execute() - require.ErrorContains(t, err, "status code 403: Forbidden", "unexpected error") + require.ErrorContains(t, err, "status code 404:", "unexpected error") }) t.Run("Unset_NotFound", func(t *testing.T) { @@ -195,7 +195,7 @@ func TestTTL(t *testing.T) { clitest.SetupConfig(t, client, root) err := cmd.Execute() - require.ErrorContains(t, err, "status code 403: Forbidden", "unexpected error") + require.ErrorContains(t, err, "status code 404:", "unexpected error") }) t.Run("TemplateMaxTTL", func(t *testing.T) { diff --git a/coderd/authorize.go b/coderd/authorize.go index 2f36a45e16886..8bfc68ff80452 100644 --- a/coderd/authorize.go +++ b/coderd/authorize.go @@ -7,7 +7,6 @@ import ( "cdr.dev/slog" - "github.com/coder/coder/coderd/httpapi" "github.com/coder/coder/coderd/httpmw" "github.com/coder/coder/coderd/rbac" ) @@ -17,12 +16,18 @@ func AuthorizeFilter[O rbac.Objecter](api *API, r *http.Request, action rbac.Act return rbac.Filter(r.Context(), api.Authorizer, roles.ID.String(), roles.Roles, action, objects) } -func (api *API) Authorize(rw http.ResponseWriter, r *http.Request, action rbac.Action, object rbac.Objecter) bool { +// Authorize will return false if the user is not authorized to do the action. +// This function will log appropriately, but the caller must return an +// error to the api client. +// Eg: +// if !api.Authorize(...) { +// httpapi.Forbidden(rw) +// return +// } +func (api *API) Authorize(r *http.Request, action rbac.Action, object rbac.Objecter) bool { roles := httpmw.AuthorizationUserRoles(r) err := api.Authorizer.ByRoleName(r.Context(), roles.ID.String(), roles.Roles, action, object.RBACObject()) if err != nil { - httpapi.Forbidden(rw) - // Log the errors for debugging internalError := new(rbac.UnauthorizedError) logger := api.Logger diff --git a/coderd/coderd_test.go b/coderd/coderd_test.go index 7053b4ff56cb8..99d81adca1f1e 100644 --- a/coderd/coderd_test.go +++ b/coderd/coderd_test.go @@ -380,9 +380,6 @@ func TestAuthorizeAllEndpoints(t *testing.T) { // By default, all omitted routes check for just "authorize" called routeAssertions = routeCheck{} } - if routeAssertions.StatusCode == 0 { - routeAssertions.StatusCode = http.StatusForbidden - } // Replace all url params with known values route = strings.ReplaceAll(route, "{organization}", admin.OrganizationID.String()) @@ -413,7 +410,14 @@ func TestAuthorizeAllEndpoints(t *testing.T) { if !routeAssertions.NoAuthorize { assert.NotNil(t, authorizer.Called, "authorizer expected") - assert.Equal(t, routeAssertions.StatusCode, resp.StatusCode, "expect unauthorized") + if routeAssertions.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 authorizer.Called != nil { if routeAssertions.AssertAction != "" { assert.Equal(t, routeAssertions.AssertAction, authorizer.Called.Action, "resource action") diff --git a/coderd/files.go b/coderd/files.go index fb982bb99612a..cca2cb391ef5a 100644 --- a/coderd/files.go +++ b/coderd/files.go @@ -22,7 +22,8 @@ func (api *API) postFile(rw http.ResponseWriter, r *http.Request) { 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(rw, r, rbac.ActionCreate, rbac.ResourceFile) { + if !api.Authorize(r, rbac.ActionCreate, rbac.ResourceFile) { + httpapi.Forbidden(rw) return } @@ -86,7 +87,7 @@ func (api *API) fileByHash(rw http.ResponseWriter, r *http.Request) { } file, err := api.Database.GetFileByHash(r.Context(), hash) if errors.Is(err, sql.ErrNoRows) { - httpapi.Forbidden(rw) + httpapi.ResourceNotFound(rw) return } if err != nil { @@ -97,8 +98,10 @@ func (api *API) fileByHash(rw http.ResponseWriter, r *http.Request) { return } - if !api.Authorize(rw, r, rbac.ActionRead, + if !api.Authorize(r, rbac.ActionRead, rbac.ResourceFile.WithOwner(file.CreatedBy.String()).WithID(file.Hash)) { + // Return 404 to not leak the file exists + httpapi.ResourceNotFound(rw) return } diff --git a/coderd/files_test.go b/coderd/files_test.go index 19609c8611e95..016774a030c88 100644 --- a/coderd/files_test.go +++ b/coderd/files_test.go @@ -50,7 +50,7 @@ func TestDownload(t *testing.T) { _, _, err := client.Download(context.Background(), "something") 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("Insert", func(t *testing.T) { diff --git a/coderd/gitsshkey.go b/coderd/gitsshkey.go index 11184864589c6..713dfe9e1bc49 100644 --- a/coderd/gitsshkey.go +++ b/coderd/gitsshkey.go @@ -14,7 +14,8 @@ import ( func (api *API) regenerateGitSSHKey(rw http.ResponseWriter, r *http.Request) { user := httpmw.UserParam(r) - if !api.Authorize(rw, r, rbac.ActionUpdate, rbac.ResourceUserData.WithOwner(user.ID.String())) { + if !api.Authorize(r, rbac.ActionUpdate, rbac.ResourceUserData.WithOwner(user.ID.String())) { + httpapi.ResourceNotFound(rw) return } @@ -62,7 +63,8 @@ func (api *API) regenerateGitSSHKey(rw http.ResponseWriter, r *http.Request) { func (api *API) gitSSHKey(rw http.ResponseWriter, r *http.Request) { user := httpmw.UserParam(r) - if !api.Authorize(rw, r, rbac.ActionRead, rbac.ResourceUserData.WithOwner(user.ID.String())) { + if !api.Authorize(r, rbac.ActionRead, rbac.ResourceUserData.WithOwner(user.ID.String())) { + httpapi.ResourceNotFound(rw) return } diff --git a/coderd/httpapi/httpapi.go b/coderd/httpapi/httpapi.go index aeb63d8574c84..79e36f175b26b 100644 --- a/coderd/httpapi/httpapi.go +++ b/coderd/httpapi/httpapi.go @@ -76,6 +76,14 @@ type Error struct { Detail string `json:"detail" validate:"required"` } +// ResourceNotFound is intentionally vague. All 404 responses should be identical +// to prevent leaking existence of resources. +func ResourceNotFound(rw http.ResponseWriter) { + Write(rw, http.StatusNotFound, Response{ + Message: "Resource not found or you do not have access to this resource", + }) +} + func Forbidden(rw http.ResponseWriter) { Write(rw, http.StatusForbidden, Response{ Message: "Forbidden.", diff --git a/coderd/httpmw/organizationparam.go b/coderd/httpmw/organizationparam.go index fd847f2cca3d1..83ef445a9d926 100644 --- a/coderd/httpmw/organizationparam.go +++ b/coderd/httpmw/organizationparam.go @@ -4,7 +4,6 @@ import ( "context" "database/sql" "errors" - "fmt" "net/http" "github.com/coder/coder/coderd/database" @@ -45,9 +44,7 @@ func ExtractOrganizationParam(db database.Store) func(http.Handler) http.Handler organization, err := db.GetOrganizationByID(r.Context(), orgID) if errors.Is(err, sql.ErrNoRows) { - httpapi.Write(rw, http.StatusNotFound, httpapi.Response{ - Message: fmt.Sprintf("Organization %q does not exist.", orgID), - }) + httpapi.ResourceNotFound(rw) return } if err != nil { @@ -76,9 +73,7 @@ func ExtractOrganizationMemberParam(db database.Store) func(http.Handler) http.H UserID: user.ID, }) if errors.Is(err, sql.ErrNoRows) { - httpapi.Write(rw, http.StatusForbidden, httpapi.Response{ - Message: "Not a member of the organization.", - }) + httpapi.ResourceNotFound(rw) return } if err != nil { diff --git a/coderd/httpmw/organizationparam_test.go b/coderd/httpmw/organizationparam_test.go index ec01d992bd7fe..85a5ca6d3eab2 100644 --- a/coderd/httpmw/organizationparam_test.go +++ b/coderd/httpmw/organizationparam_test.go @@ -144,7 +144,7 @@ func TestOrganizationParam(t *testing.T) { rtr.ServeHTTP(rw, r) res := rw.Result() defer res.Body.Close() - require.Equal(t, http.StatusForbidden, res.StatusCode) + require.Equal(t, http.StatusNotFound, res.StatusCode) }) t.Run("Success", func(t *testing.T) { diff --git a/coderd/httpmw/templateparam.go b/coderd/httpmw/templateparam.go index 0a7cba43d8a2a..9261a346bb40f 100644 --- a/coderd/httpmw/templateparam.go +++ b/coderd/httpmw/templateparam.go @@ -4,7 +4,6 @@ import ( "context" "database/sql" "errors" - "fmt" "net/http" "github.com/go-chi/chi/v5" @@ -33,10 +32,8 @@ func ExtractTemplateParam(db database.Store) func(http.Handler) http.Handler { return } template, err := db.GetTemplateByID(r.Context(), templateID) - if errors.Is(err, sql.ErrNoRows) { - httpapi.Write(rw, http.StatusNotFound, httpapi.Response{ - Message: fmt.Sprintf("Template %q does not exist.", templateID), - }) + if errors.Is(err, sql.ErrNoRows) || (err == nil && template.Deleted) { + httpapi.ResourceNotFound(rw) return } if err != nil { @@ -47,13 +44,6 @@ func ExtractTemplateParam(db database.Store) func(http.Handler) http.Handler { return } - if template.Deleted { - httpapi.Write(rw, http.StatusNotFound, httpapi.Response{ - Message: fmt.Sprintf("Template %q does not exist.", templateID), - }) - return - } - ctx := context.WithValue(r.Context(), templateParamContextKey{}, template) chi.RouteContext(ctx).URLParams.Add("organization", template.OrganizationID.String()) next.ServeHTTP(rw, r.WithContext(ctx)) diff --git a/coderd/httpmw/templateversionparam.go b/coderd/httpmw/templateversionparam.go index 96be063defa9f..dc85383dd48e8 100644 --- a/coderd/httpmw/templateversionparam.go +++ b/coderd/httpmw/templateversionparam.go @@ -4,7 +4,6 @@ import ( "context" "database/sql" "errors" - "fmt" "net/http" "github.com/go-chi/chi/v5" @@ -34,9 +33,7 @@ func ExtractTemplateVersionParam(db database.Store) func(http.Handler) http.Hand } templateVersion, err := db.GetTemplateVersionByID(r.Context(), templateVersionID) if errors.Is(err, sql.ErrNoRows) { - httpapi.Write(rw, http.StatusNotFound, httpapi.Response{ - Message: fmt.Sprintf("Template version %q does not exist.", templateVersionID), - }) + httpapi.ResourceNotFound(rw) return } if err != nil { diff --git a/coderd/httpmw/userparam.go b/coderd/httpmw/userparam.go index 88d4adc25b98b..924f3362b9ec1 100644 --- a/coderd/httpmw/userparam.go +++ b/coderd/httpmw/userparam.go @@ -2,8 +2,11 @@ package httpmw import ( "context" + "database/sql" "net/http" + "golang.org/x/xerrors" + "github.com/go-chi/chi/v5" "github.com/google/uuid" @@ -47,6 +50,10 @@ func ExtractUserParam(db database.Store) func(http.Handler) http.Handler { if userQuery == "me" { user, err = db.GetUserByID(r.Context(), APIKey(r).UserID) + if xerrors.Is(err, sql.ErrNoRows) { + httpapi.ResourceNotFound(rw) + return + } if err != nil { httpapi.Write(rw, http.StatusInternalServerError, httpapi.Response{ Message: "Internal error fetching user.", diff --git a/coderd/httpmw/workspacebuildparam.go b/coderd/httpmw/workspacebuildparam.go index 719a2b5c842ba..33375b300e921 100644 --- a/coderd/httpmw/workspacebuildparam.go +++ b/coderd/httpmw/workspacebuildparam.go @@ -4,7 +4,6 @@ import ( "context" "database/sql" "errors" - "fmt" "net/http" "github.com/go-chi/chi/v5" @@ -34,9 +33,7 @@ func ExtractWorkspaceBuildParam(db database.Store) func(http.Handler) http.Handl } workspaceBuild, err := db.GetWorkspaceBuildByID(r.Context(), workspaceBuildID) if errors.Is(err, sql.ErrNoRows) { - httpapi.Write(rw, http.StatusNotFound, httpapi.Response{ - Message: fmt.Sprintf("Workspace build %q does not exist.", workspaceBuildID), - }) + httpapi.ResourceNotFound(rw) return } if err != nil { diff --git a/coderd/httpmw/workspaceparam.go b/coderd/httpmw/workspaceparam.go index dd2112793b320..ce36a73ff85f8 100644 --- a/coderd/httpmw/workspaceparam.go +++ b/coderd/httpmw/workspaceparam.go @@ -4,7 +4,6 @@ import ( "context" "database/sql" "errors" - "fmt" "net/http" "github.com/coder/coder/coderd/database" @@ -32,9 +31,7 @@ func ExtractWorkspaceParam(db database.Store) func(http.Handler) http.Handler { } workspace, err := db.GetWorkspaceByID(r.Context(), workspaceID) if errors.Is(err, sql.ErrNoRows) { - httpapi.Write(rw, http.StatusNotFound, httpapi.Response{ - Message: fmt.Sprintf("Workspace %q does not exist.", workspaceID), - }) + httpapi.ResourceNotFound(rw) return } if err != nil { diff --git a/coderd/members.go b/coderd/members.go index eab402b71148d..22c2d8804f873 100644 --- a/coderd/members.go +++ b/coderd/members.go @@ -39,13 +39,15 @@ func (api *API) putMemberRoles(rw http.ResponseWriter, r *http.Request) { added, removed := rbac.ChangeRoleSet(member.Roles, impliedTypes) for _, roleName := range added { // Assigning a role requires the create permission. - if !api.Authorize(rw, r, rbac.ActionCreate, rbac.ResourceOrgRoleAssignment.WithID(roleName).InOrg(organization.ID)) { + if !api.Authorize(r, rbac.ActionCreate, rbac.ResourceOrgRoleAssignment.WithID(roleName).InOrg(organization.ID)) { + httpapi.Forbidden(rw) return } } for _, roleName := range removed { // Removing a role requires the delete permission. - if !api.Authorize(rw, r, rbac.ActionDelete, rbac.ResourceOrgRoleAssignment.WithID(roleName).InOrg(organization.ID)) { + if !api.Authorize(r, rbac.ActionDelete, rbac.ResourceOrgRoleAssignment.WithID(roleName).InOrg(organization.ID)) { + httpapi.Forbidden(rw) return } } diff --git a/coderd/organizations.go b/coderd/organizations.go index ffac03d6c5f2e..30aeac30493cc 100644 --- a/coderd/organizations.go +++ b/coderd/organizations.go @@ -19,9 +19,10 @@ import ( func (api *API) organization(rw http.ResponseWriter, r *http.Request) { organization := httpmw.OrganizationParam(r) - if !api.Authorize(rw, r, rbac.ActionRead, rbac.ResourceOrganization. + if !api.Authorize(r, rbac.ActionRead, rbac.ResourceOrganization. InOrg(organization.ID). WithID(organization.ID.String())) { + httpapi.ResourceNotFound(rw) return } @@ -32,8 +33,8 @@ func (api *API) postOrganizations(rw http.ResponseWriter, r *http.Request) { 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(rw, r, rbac.ActionCreate, - rbac.ResourceOrganization) { + if !api.Authorize(r, rbac.ActionCreate, rbac.ResourceOrganization) { + httpapi.Forbidden(rw) return } diff --git a/coderd/organizations_test.go b/coderd/organizations_test.go index adc35c08d8908..d329c65d59832 100644 --- a/coderd/organizations_test.go +++ b/coderd/organizations_test.go @@ -30,7 +30,7 @@ func TestOrganizationByUserAndName(t *testing.T) { _, err := client.OrganizationByName(context.Background(), codersdk.Me, "nothing") var apiErr *codersdk.Error require.ErrorAs(t, err, &apiErr) - require.Equal(t, http.StatusForbidden, apiErr.StatusCode()) + require.Equal(t, http.StatusNotFound, apiErr.StatusCode()) }) t.Run("NoMember", func(t *testing.T) { @@ -45,7 +45,7 @@ func TestOrganizationByUserAndName(t *testing.T) { _, err = other.OrganizationByName(context.Background(), codersdk.Me, org.Name) 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("Valid", func(t *testing.T) { diff --git a/coderd/parameters.go b/coderd/parameters.go index eeb8731d2562f..615e8f84948ca 100644 --- a/coderd/parameters.go +++ b/coderd/parameters.go @@ -26,7 +26,8 @@ func (api *API) postParameter(rw http.ResponseWriter, r *http.Request) { if !ok { return } - if !api.Authorize(rw, r, rbac.ActionUpdate, obj) { + if !api.Authorize(r, rbac.ActionUpdate, obj) { + httpapi.ResourceNotFound(rw) return } @@ -85,7 +86,8 @@ func (api *API) parameters(rw http.ResponseWriter, r *http.Request) { return } - if !api.Authorize(rw, r, rbac.ActionRead, obj) { + if !api.Authorize(r, rbac.ActionRead, obj) { + httpapi.ResourceNotFound(rw) return } @@ -120,8 +122,9 @@ func (api *API) deleteParameter(rw http.ResponseWriter, r *http.Request) { if !ok { return } - // A delete param is still updating the underlying resource for the scope. - if !api.Authorize(rw, r, rbac.ActionUpdate, obj) { + // A deleted param is still updating the underlying resource for the scope. + if !api.Authorize(r, rbac.ActionUpdate, obj) { + httpapi.ResourceNotFound(rw) return } @@ -132,10 +135,7 @@ func (api *API) deleteParameter(rw http.ResponseWriter, r *http.Request) { Name: name, }) if errors.Is(err, sql.ErrNoRows) { - httpapi.Write(rw, http.StatusNotFound, httpapi.Response{ - Message: fmt.Sprintf("No parameter found at the provided scope with name %q.", name), - Detail: err.Error(), - }) + httpapi.ResourceNotFound(rw) return } if err != nil { @@ -223,7 +223,9 @@ func (api *API) parameterRBACResource(rw http.ResponseWriter, r *http.Request, s // Write error payload to rw if we cannot find the resource for the scope if err != nil { if xerrors.Is(err, sql.ErrNoRows) { - httpapi.Forbidden(rw) + httpapi.Write(rw, http.StatusNotFound, httpapi.Response{ + Message: fmt.Sprintf("Scope %q resource %q not found.", scope, scopeID), + }) } else { httpapi.Write(rw, http.StatusBadRequest, httpapi.Response{ Message: err.Error(), diff --git a/coderd/roles.go b/coderd/roles.go index ceeca65ac0984..30f78275e0a1d 100644 --- a/coderd/roles.go +++ b/coderd/roles.go @@ -16,7 +16,8 @@ func (api *API) assignableSiteRoles(rw http.ResponseWriter, r *http.Request) { // TODO: @emyrk in the future, allow granular subsets of roles to be returned based on the // role of the user. - if !api.Authorize(rw, r, rbac.ActionRead, rbac.ResourceRoleAssignment) { + if !api.Authorize(r, rbac.ActionRead, rbac.ResourceRoleAssignment) { + httpapi.Forbidden(rw) return } @@ -30,7 +31,8 @@ func (api *API) assignableOrgRoles(rw http.ResponseWriter, r *http.Request) { // role of the user. organization := httpmw.OrganizationParam(r) - if !api.Authorize(rw, r, rbac.ActionRead, rbac.ResourceOrgRoleAssignment.InOrg(organization.ID)) { + if !api.Authorize(r, rbac.ActionRead, rbac.ResourceOrgRoleAssignment.InOrg(organization.ID)) { + httpapi.Forbidden(rw) return } @@ -41,7 +43,8 @@ func (api *API) assignableOrgRoles(rw http.ResponseWriter, r *http.Request) { func (api *API) checkPermissions(rw http.ResponseWriter, r *http.Request) { user := httpmw.UserParam(r) - if !api.Authorize(rw, r, rbac.ActionRead, rbac.ResourceUser.WithID(user.ID.String())) { + if !api.Authorize(r, rbac.ActionRead, rbac.ResourceUser.WithID(user.ID.String())) { + httpapi.ResourceNotFound(rw) return } diff --git a/coderd/templates.go b/coderd/templates.go index d79bd19f70fa2..11e434bd8848f 100644 --- a/coderd/templates.go +++ b/coderd/templates.go @@ -29,6 +29,11 @@ var ( func (api *API) template(rw http.ResponseWriter, r *http.Request) { template := httpmw.TemplateParam(r) + if !api.Authorize(r, rbac.ActionRead, template) { + httpapi.ResourceNotFound(rw) + return + } + workspaceCounts, err := api.Database.GetWorkspaceOwnerCountsByTemplateIDs(r.Context(), []uuid.UUID{template.ID}) if errors.Is(err, sql.ErrNoRows) { err = nil @@ -41,7 +46,8 @@ func (api *API) template(rw http.ResponseWriter, r *http.Request) { return } - if !api.Authorize(rw, r, rbac.ActionRead, template) { + if !api.Authorize(r, rbac.ActionRead, template) { + httpapi.ResourceNotFound(rw) return } @@ -64,7 +70,8 @@ func (api *API) template(rw http.ResponseWriter, r *http.Request) { func (api *API) deleteTemplate(rw http.ResponseWriter, r *http.Request) { template := httpmw.TemplateParam(r) - if !api.Authorize(rw, r, rbac.ActionDelete, template) { + if !api.Authorize(r, rbac.ActionDelete, template) { + httpapi.ResourceNotFound(rw) return } @@ -108,7 +115,8 @@ func (api *API) postTemplateByOrganization(rw http.ResponseWriter, r *http.Reque var createTemplate codersdk.CreateTemplateRequest organization := httpmw.OrganizationParam(r) apiKey := httpmw.APIKey(r) - if !api.Authorize(rw, r, rbac.ActionCreate, rbac.ResourceTemplate.InOrg(organization.ID)) { + if !api.Authorize(r, rbac.ActionCreate, rbac.ResourceTemplate.InOrg(organization.ID)) { + httpapi.ResourceNotFound(rw) return } @@ -299,9 +307,7 @@ func (api *API) templateByOrganizationAndName(rw http.ResponseWriter, r *http.Re }) if err != nil { if errors.Is(err, sql.ErrNoRows) { - httpapi.Write(rw, http.StatusNotFound, httpapi.Response{ - Message: fmt.Sprintf("No template found by name %q in the %q organization.", templateName, organization.Name), - }) + httpapi.ResourceNotFound(rw) return } @@ -312,7 +318,8 @@ func (api *API) templateByOrganizationAndName(rw http.ResponseWriter, r *http.Re return } - if !api.Authorize(rw, r, rbac.ActionRead, template) { + if !api.Authorize(r, rbac.ActionRead, template) { + httpapi.ResourceNotFound(rw) return } @@ -347,7 +354,8 @@ func (api *API) templateByOrganizationAndName(rw http.ResponseWriter, r *http.Re func (api *API) patchTemplateMeta(rw http.ResponseWriter, r *http.Request) { template := httpmw.TemplateParam(r) - if !api.Authorize(rw, r, rbac.ActionUpdate, template) { + if !api.Authorize(r, rbac.ActionUpdate, template) { + httpapi.ResourceNotFound(rw) return } diff --git a/coderd/templateversions.go b/coderd/templateversions.go index 64345457e969c..472fd87f8d9e8 100644 --- a/coderd/templateversions.go +++ b/coderd/templateversions.go @@ -22,7 +22,8 @@ import ( func (api *API) templateVersion(rw http.ResponseWriter, r *http.Request) { templateVersion := httpmw.TemplateVersionParam(r) - if !api.Authorize(rw, r, rbac.ActionRead, templateVersion) { + if !api.Authorize(r, rbac.ActionRead, templateVersion) { + httpapi.ResourceNotFound(rw) return } @@ -40,7 +41,8 @@ func (api *API) templateVersion(rw http.ResponseWriter, r *http.Request) { func (api *API) patchCancelTemplateVersion(rw http.ResponseWriter, r *http.Request) { templateVersion := httpmw.TemplateVersionParam(r) - if !api.Authorize(rw, r, rbac.ActionUpdate, templateVersion) { + if !api.Authorize(r, rbac.ActionUpdate, templateVersion) { + httpapi.ResourceNotFound(rw) return } @@ -85,7 +87,8 @@ func (api *API) patchCancelTemplateVersion(rw http.ResponseWriter, r *http.Reque func (api *API) templateVersionSchema(rw http.ResponseWriter, r *http.Request) { templateVersion := httpmw.TemplateVersionParam(r) - if !api.Authorize(rw, r, rbac.ActionRead, templateVersion) { + if !api.Authorize(r, rbac.ActionRead, templateVersion) { + httpapi.ResourceNotFound(rw) return } @@ -132,7 +135,8 @@ func (api *API) templateVersionSchema(rw http.ResponseWriter, r *http.Request) { func (api *API) templateVersionParameters(rw http.ResponseWriter, r *http.Request) { apiKey := httpmw.APIKey(r) templateVersion := httpmw.TemplateVersionParam(r) - if !api.Authorize(rw, r, rbac.ActionRead, templateVersion) { + if !api.Authorize(r, rbac.ActionRead, templateVersion) { + httpapi.ResourceNotFound(rw) return } @@ -175,13 +179,15 @@ func (api *API) templateVersionParameters(rw http.ResponseWriter, r *http.Reques func (api *API) postTemplateVersionDryRun(rw http.ResponseWriter, r *http.Request) { apiKey := httpmw.APIKey(r) templateVersion := httpmw.TemplateVersionParam(r) - if !api.Authorize(rw, r, rbac.ActionRead, templateVersion) { + if !api.Authorize(r, rbac.ActionRead, templateVersion) { + 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(rw, r, rbac.ActionCreate, + if !api.Authorize(r, rbac.ActionCreate, rbac.ResourceWorkspace.InOrg(templateVersion.OrganizationID).WithOwner(apiKey.UserID.String())) { + httpapi.ResourceNotFound(rw) return } @@ -293,8 +299,9 @@ func (api *API) patchTemplateVersionDryRunCancel(rw http.ResponseWriter, r *http if !ok { return } - if !api.Authorize(rw, r, rbac.ActionUpdate, + if !api.Authorize(r, rbac.ActionUpdate, rbac.ResourceWorkspace.InOrg(templateVersion.OrganizationID).WithOwner(job.InitiatorID.String())) { + httpapi.ResourceNotFound(rw) return } @@ -336,7 +343,8 @@ func (api *API) fetchTemplateVersionDryRunJob(rw http.ResponseWriter, r *http.Re templateVersion = httpmw.TemplateVersionParam(r) jobID = chi.URLParam(r, "jobID") ) - if !api.Authorize(rw, r, rbac.ActionRead, templateVersion) { + if !api.Authorize(r, rbac.ActionRead, templateVersion) { + httpapi.ResourceNotFound(rw) return database.ProvisionerJob{}, false } @@ -351,7 +359,9 @@ func (api *API) fetchTemplateVersionDryRunJob(rw http.ResponseWriter, r *http.Re job, err := api.Database.GetProvisionerJobByID(r.Context(), jobUUID) if xerrors.Is(err, sql.ErrNoRows) { - httpapi.Forbidden(rw) + httpapi.Write(rw, http.StatusNotFound, httpapi.Response{ + Message: fmt.Sprintf("Provisioner job %q not found.", jobUUID), + }) return database.ProvisionerJob{}, false } if err != nil { @@ -366,8 +376,9 @@ func (api *API) fetchTemplateVersionDryRunJob(rw http.ResponseWriter, r *http.Re return database.ProvisionerJob{}, false } // Do a workspace resource check since it's basically a workspace dry-run . - if !api.Authorize(rw, r, rbac.ActionRead, + if !api.Authorize(r, rbac.ActionRead, rbac.ResourceWorkspace.InOrg(templateVersion.OrganizationID).WithOwner(job.InitiatorID.String())) { + httpapi.Forbidden(rw) return database.ProvisionerJob{}, false } @@ -391,7 +402,8 @@ func (api *API) fetchTemplateVersionDryRunJob(rw http.ResponseWriter, r *http.Re func (api *API) templateVersionsByTemplate(rw http.ResponseWriter, r *http.Request) { template := httpmw.TemplateParam(r) - if !api.Authorize(rw, r, rbac.ActionRead, template) { + if !api.Authorize(r, rbac.ActionRead, template) { + httpapi.ResourceNotFound(rw) return } @@ -478,7 +490,8 @@ func (api *API) templateVersionsByTemplate(rw http.ResponseWriter, r *http.Reque func (api *API) templateVersionByName(rw http.ResponseWriter, r *http.Request) { template := httpmw.TemplateParam(r) - if !api.Authorize(rw, r, rbac.ActionRead, template) { + if !api.Authorize(r, rbac.ActionRead, template) { + httpapi.ResourceNotFound(rw) return } @@ -517,7 +530,8 @@ func (api *API) templateVersionByName(rw http.ResponseWriter, r *http.Request) { func (api *API) patchActiveTemplateVersion(rw http.ResponseWriter, r *http.Request) { template := httpmw.TemplateParam(r) - if !api.Authorize(rw, r, rbac.ActionUpdate, template) { + if !api.Authorize(r, rbac.ActionUpdate, template) { + httpapi.ResourceNotFound(rw) return } @@ -603,11 +617,13 @@ func (api *API) postTemplateVersionsByOrganization(rw http.ResponseWriter, r *ht } // Making a new template version is the same permission as creating a new template. - if !api.Authorize(rw, r, rbac.ActionCreate, rbac.ResourceTemplate.InOrg(organization.ID)) { + if !api.Authorize(r, rbac.ActionCreate, rbac.ResourceTemplate.InOrg(organization.ID)) { + httpapi.ResourceNotFound(rw) return } - if !api.Authorize(rw, r, rbac.ActionRead, file) { + if !api.Authorize(r, rbac.ActionRead, file) { + httpapi.ResourceNotFound(rw) return } @@ -688,7 +704,8 @@ func (api *API) postTemplateVersionsByOrganization(rw http.ResponseWriter, r *ht // return agents associated with any particular workspace. func (api *API) templateVersionResources(rw http.ResponseWriter, r *http.Request) { templateVersion := httpmw.TemplateVersionParam(r) - if !api.Authorize(rw, r, rbac.ActionRead, templateVersion) { + if !api.Authorize(r, rbac.ActionRead, templateVersion) { + httpapi.ResourceNotFound(rw) return } @@ -709,7 +726,8 @@ func (api *API) templateVersionResources(rw http.ResponseWriter, r *http.Request // Eg: Logs returned from 'terraform plan' when uploading a new terraform file. func (api *API) templateVersionLogs(rw http.ResponseWriter, r *http.Request) { templateVersion := httpmw.TemplateVersionParam(r) - if !api.Authorize(rw, r, rbac.ActionRead, templateVersion) { + if !api.Authorize(r, rbac.ActionRead, templateVersion) { + httpapi.ResourceNotFound(rw) return } diff --git a/coderd/users.go b/coderd/users.go index 37cab2cd394c2..781cc213d1797 100644 --- a/coderd/users.go +++ b/coderd/users.go @@ -135,7 +135,8 @@ func (api *API) users(rw http.ResponseWriter, r *http.Request) { } // Reading all users across the site. - if !api.Authorize(rw, r, rbac.ActionRead, rbac.ResourceUser) { + if !api.Authorize(r, rbac.ActionRead, rbac.ResourceUser) { + httpapi.Forbidden(rw) return } @@ -190,7 +191,8 @@ func (api *API) users(rw http.ResponseWriter, r *http.Request) { // Creates a new user. func (api *API) postUser(rw http.ResponseWriter, r *http.Request) { // Create the user on the site. - if !api.Authorize(rw, r, rbac.ActionCreate, rbac.ResourceUser) { + if !api.Authorize(r, rbac.ActionCreate, rbac.ResourceUser) { + httpapi.Forbidden(rw) return } @@ -200,8 +202,9 @@ func (api *API) postUser(rw http.ResponseWriter, r *http.Request) { } // Create the organization member in the org. - if !api.Authorize(rw, r, rbac.ActionCreate, + if !api.Authorize(r, rbac.ActionCreate, rbac.ResourceOrganizationMember.InOrg(createUser.OrganizationID)) { + httpapi.ResourceNotFound(rw) return } @@ -258,7 +261,8 @@ func (api *API) userByName(rw http.ResponseWriter, r *http.Request) { user := httpmw.UserParam(r) organizationIDs, err := userOrganizationIDs(r.Context(), api, user) - if !api.Authorize(rw, r, rbac.ActionRead, rbac.ResourceUser.WithID(user.ID.String())) { + if !api.Authorize(r, rbac.ActionRead, rbac.ResourceUser.WithID(user.ID.String())) { + httpapi.ResourceNotFound(rw) return } @@ -276,7 +280,8 @@ func (api *API) userByName(rw http.ResponseWriter, r *http.Request) { func (api *API) putUserProfile(rw http.ResponseWriter, r *http.Request) { user := httpmw.UserParam(r) - if !api.Authorize(rw, r, rbac.ActionUpdate, rbac.ResourceUser.WithID(user.ID.String())) { + if !api.Authorize(r, rbac.ActionUpdate, rbac.ResourceUser.WithID(user.ID.String())) { + httpapi.ResourceNotFound(rw) return } @@ -343,7 +348,8 @@ func (api *API) putUserStatus(status database.UserStatus) func(rw http.ResponseW user := httpmw.UserParam(r) apiKey := httpmw.APIKey(r) - if !api.Authorize(rw, r, rbac.ActionDelete, rbac.ResourceUser.WithID(user.ID.String())) { + if !api.Authorize(r, rbac.ActionDelete, rbac.ResourceUser.WithID(user.ID.String())) { + httpapi.ResourceNotFound(rw) return } @@ -387,7 +393,8 @@ func (api *API) putUserPassword(rw http.ResponseWriter, r *http.Request) { params codersdk.UpdateUserPasswordRequest ) - if !api.Authorize(rw, r, rbac.ActionUpdate, rbac.ResourceUserData.WithOwner(user.ID.String())) { + if !api.Authorize(r, rbac.ActionUpdate, rbac.ResourceUserData.WithOwner(user.ID.String())) { + httpapi.ResourceNotFound(rw) return } @@ -411,7 +418,8 @@ func (api *API) putUserPassword(rw http.ResponseWriter, r *http.Request) { // admins can change passwords without sending old_password if params.OldPassword == "" { - if !api.Authorize(rw, r, rbac.ActionUpdate, rbac.ResourceUser.WithID(user.ID.String())) { + if !api.Authorize(r, rbac.ActionUpdate, rbac.ResourceUser.WithID(user.ID.String())) { + httpapi.Forbidden(rw) return } } else { @@ -464,8 +472,8 @@ func (api *API) putUserPassword(rw http.ResponseWriter, r *http.Request) { func (api *API) userRoles(rw http.ResponseWriter, r *http.Request) { user := httpmw.UserParam(r) - if !api.Authorize(rw, r, rbac.ActionRead, rbac.ResourceUserData. - WithOwner(user.ID.String())) { + if !api.Authorize(r, rbac.ActionRead, rbac.ResourceUserData.WithOwner(user.ID.String())) { + httpapi.ResourceNotFound(rw) return } @@ -514,18 +522,25 @@ func (api *API) putUserRoles(rw http.ResponseWriter, r *http.Request) { return } + if !api.Authorize(r, rbac.ActionRead, rbac.ResourceUser.WithID(user.ID.String())) { + httpapi.ResourceNotFound(rw) + return + } + // The member role is always implied. impliedTypes := append(params.Roles, rbac.RoleMember()) added, removed := rbac.ChangeRoleSet(roles.Roles, impliedTypes) for _, roleName := range added { // Assigning a role requires the create permission. - if !api.Authorize(rw, r, rbac.ActionCreate, rbac.ResourceRoleAssignment.WithID(roleName)) { + if !api.Authorize(r, rbac.ActionCreate, rbac.ResourceRoleAssignment.WithID(roleName)) { + httpapi.Forbidden(rw) return } } for _, roleName := range removed { // Removing a role requires the delete permission. - if !api.Authorize(rw, r, rbac.ActionDelete, rbac.ResourceRoleAssignment.WithID(roleName)) { + if !api.Authorize(r, rbac.ActionDelete, rbac.ResourceRoleAssignment.WithID(roleName)) { + httpapi.Forbidden(rw) return } } @@ -606,20 +621,22 @@ func (api *API) organizationByUserAndName(rw http.ResponseWriter, r *http.Reques organizationName := chi.URLParam(r, "organizationname") organization, err := api.Database.GetOrganizationByName(r.Context(), organizationName) if errors.Is(err, sql.ErrNoRows) { - // Return unauthorized rather than a 404 to not leak if the organization - // exists. - httpapi.Forbidden(rw) + httpapi.ResourceNotFound(rw) return } if err != nil { - httpapi.Forbidden(rw) + httpapi.Write(rw, http.StatusInternalServerError, httpapi.Response{ + Message: "Internal error fetching organization.", + Detail: err.Error(), + }) return } - if !api.Authorize(rw, r, rbac.ActionRead, + if !api.Authorize(r, rbac.ActionRead, rbac.ResourceOrganization. InOrg(organization.ID). WithID(organization.ID.String())) { + httpapi.ResourceNotFound(rw) return } @@ -684,7 +701,8 @@ func (api *API) postLogin(rw http.ResponseWriter, r *http.Request) { func (api *API) postAPIKey(rw http.ResponseWriter, r *http.Request) { user := httpmw.UserParam(r) - if !api.Authorize(rw, r, rbac.ActionCreate, rbac.ResourceAPIKey.WithOwner(user.ID.String())) { + if !api.Authorize(r, rbac.ActionCreate, rbac.ResourceAPIKey.WithOwner(user.ID.String())) { + httpapi.ResourceNotFound(rw) return } diff --git a/coderd/workspaceagents.go b/coderd/workspaceagents.go index a7de85c234ef4..0b03722e9adef 100644 --- a/coderd/workspaceagents.go +++ b/coderd/workspaceagents.go @@ -33,7 +33,8 @@ import ( func (api *API) workspaceAgent(rw http.ResponseWriter, r *http.Request) { workspaceAgent := httpmw.WorkspaceAgentParam(r) workspace := httpmw.WorkspaceParam(r) - if !api.Authorize(rw, r, rbac.ActionRead, workspace) { + if !api.Authorize(r, rbac.ActionRead, workspace) { + httpapi.ResourceNotFound(rw) return } dbApps, err := api.Database.GetWorkspaceAppsByAgentID(r.Context(), workspaceAgent.ID) @@ -64,7 +65,8 @@ func (api *API) workspaceAgentDial(rw http.ResponseWriter, r *http.Request) { workspaceAgent := httpmw.WorkspaceAgentParam(r) workspace := httpmw.WorkspaceParam(r) - if !api.Authorize(rw, r, rbac.ActionUpdate, workspace) { + if !api.Authorize(r, rbac.ActionUpdate, workspace) { + httpapi.ResourceNotFound(rw) return } apiAgent, err := convertWorkspaceAgent(workspaceAgent, nil, api.AgentConnectionUpdateFrequency) @@ -379,7 +381,8 @@ func (api *API) workspaceAgentPTY(rw http.ResponseWriter, r *http.Request) { workspaceAgent := httpmw.WorkspaceAgentParam(r) workspace := httpmw.WorkspaceParam(r) - if !api.Authorize(rw, r, rbac.ActionUpdate, workspace) { + if !api.Authorize(r, rbac.ActionUpdate, workspace) { + httpapi.ResourceNotFound(rw) return } apiAgent, err := convertWorkspaceAgent(workspaceAgent, nil, api.AgentConnectionUpdateFrequency) diff --git a/coderd/workspaceapps.go b/coderd/workspaceapps.go index 7ae72752bd232..ed14f16a97d83 100644 --- a/coderd/workspaceapps.go +++ b/coderd/workspaceapps.go @@ -32,9 +32,7 @@ func (api *API) workspaceAppsProxyPath(rw http.ResponseWriter, r *http.Request) Name: workspaceParts[0], }) if errors.Is(err, sql.ErrNoRows) { - httpapi.Write(rw, http.StatusNotFound, httpapi.Response{ - Message: "Workspace not found.", - }) + httpapi.ResourceNotFound(rw) return } if err != nil { @@ -44,7 +42,8 @@ func (api *API) workspaceAppsProxyPath(rw http.ResponseWriter, r *http.Request) }) return } - if !api.Authorize(rw, r, rbac.ActionRead, workspace) { + if !api.Authorize(r, rbac.ActionRead, workspace) { + httpapi.ResourceNotFound(rw) return } diff --git a/coderd/workspacebuilds.go b/coderd/workspacebuilds.go index cefe4ded6cdc6..d0db01e4a1722 100644 --- a/coderd/workspacebuilds.go +++ b/coderd/workspacebuilds.go @@ -24,8 +24,9 @@ func (api *API) workspaceBuild(rw http.ResponseWriter, r *http.Request) { workspaceBuild := httpmw.WorkspaceBuildParam(r) workspace := httpmw.WorkspaceParam(r) - if !api.Authorize(rw, r, rbac.ActionRead, rbac.ResourceWorkspace. + if !api.Authorize(r, rbac.ActionRead, rbac.ResourceWorkspace. InOrg(workspace.OrganizationID).WithOwner(workspace.OwnerID.String()).WithID(workspace.ID.String())) { + httpapi.ResourceNotFound(rw) return } @@ -55,8 +56,8 @@ func (api *API) workspaceBuild(rw http.ResponseWriter, r *http.Request) { func (api *API) workspaceBuilds(rw http.ResponseWriter, r *http.Request) { workspace := httpmw.WorkspaceParam(r) - if !api.Authorize(rw, r, rbac.ActionRead, rbac.ResourceWorkspace. - InOrg(workspace.OrganizationID).WithOwner(workspace.OwnerID.String()).WithID(workspace.ID.String())) { + if !api.Authorize(r, rbac.ActionRead, workspace) { + httpapi.ResourceNotFound(rw) return } @@ -178,9 +179,7 @@ func (api *API) workspaceBuildByBuildNumber(rw http.ResponseWriter, r *http.Requ Name: workspaceName, }) if errors.Is(err, sql.ErrNoRows) { - httpapi.Write(rw, http.StatusNotFound, httpapi.Response{ - Message: fmt.Sprintf("Workspace %q does not exist.", workspaceName), - }) + httpapi.ResourceNotFound(rw) return } if err != nil { @@ -191,8 +190,8 @@ func (api *API) workspaceBuildByBuildNumber(rw http.ResponseWriter, r *http.Requ return } - if !api.Authorize(rw, r, rbac.ActionRead, rbac.ResourceWorkspace. - InOrg(workspace.OrganizationID).WithOwner(workspace.OwnerID.String()).WithID(workspace.ID.String())) { + if !api.Authorize(r, rbac.ActionRead, workspace) { + httpapi.ResourceNotFound(rw) return } @@ -239,20 +238,19 @@ func (api *API) workspaceBuildByBuildNumber(rw http.ResponseWriter, r *http.Requ func (api *API) workspaceBuildByName(rw http.ResponseWriter, r *http.Request) { workspace := httpmw.WorkspaceParam(r) - if !api.Authorize(rw, r, rbac.ActionRead, rbac.ResourceWorkspace. + workspaceBuildName := chi.URLParam(r, "workspacebuildname") + if !api.Authorize(r, rbac.ActionRead, rbac.ResourceWorkspace. InOrg(workspace.OrganizationID).WithOwner(workspace.OwnerID.String()).WithID(workspace.ID.String())) { + httpapi.ResourceNotFound(rw) return } - workspaceBuildName := chi.URLParam(r, "workspacebuildname") workspaceBuild, err := api.Database.GetWorkspaceBuildByWorkspaceIDAndName(r.Context(), database.GetWorkspaceBuildByWorkspaceIDAndNameParams{ WorkspaceID: workspace.ID, Name: workspaceBuildName, }) if errors.Is(err, sql.ErrNoRows) { - httpapi.Write(rw, http.StatusNotFound, httpapi.Response{ - Message: fmt.Sprintf("No workspace build found by name %q.", workspaceBuildName), - }) + httpapi.ResourceNotFound(rw) return } if err != nil { @@ -305,8 +303,9 @@ func (api *API) postWorkspaceBuilds(rw http.ResponseWriter, r *http.Request) { }) return } - if !api.Authorize(rw, r, action, rbac.ResourceWorkspace. + if !api.Authorize(r, action, rbac.ResourceWorkspace. InOrg(workspace.OrganizationID).WithOwner(workspace.OwnerID.String()).WithID(workspace.ID.String())) { + httpapi.ResourceNotFound(rw) return } @@ -482,8 +481,9 @@ func (api *API) patchCancelWorkspaceBuild(rw http.ResponseWriter, r *http.Reques return } - if !api.Authorize(rw, r, rbac.ActionUpdate, rbac.ResourceWorkspace. + if !api.Authorize(r, rbac.ActionUpdate, rbac.ResourceWorkspace. InOrg(workspace.OrganizationID).WithOwner(workspace.OwnerID.String()).WithID(workspace.ID.String())) { + httpapi.ResourceNotFound(rw) return } @@ -536,8 +536,9 @@ func (api *API) workspaceBuildResources(rw http.ResponseWriter, r *http.Request) return } - if !api.Authorize(rw, r, rbac.ActionRead, rbac.ResourceWorkspace. + if !api.Authorize(r, rbac.ActionRead, rbac.ResourceWorkspace. InOrg(workspace.OrganizationID).WithOwner(workspace.OwnerID.String()).WithID(workspace.ID.String())) { + httpapi.ResourceNotFound(rw) return } @@ -562,8 +563,9 @@ func (api *API) workspaceBuildLogs(rw http.ResponseWriter, r *http.Request) { return } - if !api.Authorize(rw, r, rbac.ActionRead, rbac.ResourceWorkspace. + if !api.Authorize(r, rbac.ActionRead, rbac.ResourceWorkspace. InOrg(workspace.OrganizationID).WithOwner(workspace.OwnerID.String()).WithID(workspace.ID.String())) { + httpapi.ResourceNotFound(rw) return } @@ -588,8 +590,8 @@ func (api *API) workspaceBuildState(rw http.ResponseWriter, r *http.Request) { return } - if !api.Authorize(rw, r, rbac.ActionRead, rbac.ResourceWorkspace. - InOrg(workspace.OrganizationID).WithOwner(workspace.OwnerID.String()).WithID(workspace.ID.String())) { + if !api.Authorize(r, rbac.ActionRead, workspace) { + httpapi.ResourceNotFound(rw) return } diff --git a/coderd/workspacebuilds_test.go b/coderd/workspacebuilds_test.go index 3833cee09171c..37847d7bf44b4 100644 --- a/coderd/workspacebuilds_test.go +++ b/coderd/workspacebuilds_test.go @@ -92,7 +92,7 @@ func TestWorkspaceBuildByBuildNumber(t *testing.T) { var apiError *codersdk.Error require.ErrorAs(t, err, &apiError) require.Equal(t, http.StatusNotFound, apiError.StatusCode()) - require.ErrorContains(t, apiError, "Workspace \"workspaceName\" does not exist.") + require.ErrorContains(t, apiError, "Resource not found") }) t.Run("WorkspaceBuildNotFound", func(t *testing.T) { diff --git a/coderd/workspaceresources.go b/coderd/workspaceresources.go index dd5ec5d79eca5..8f38d4d9dc15a 100644 --- a/coderd/workspaceresources.go +++ b/coderd/workspaceresources.go @@ -18,7 +18,8 @@ func (api *API) workspaceResource(rw http.ResponseWriter, r *http.Request) { workspaceBuild := httpmw.WorkspaceBuildParam(r) workspaceResource := httpmw.WorkspaceResourceParam(r) workspace := httpmw.WorkspaceParam(r) - if !api.Authorize(rw, r, rbac.ActionRead, workspace) { + if !api.Authorize(r, rbac.ActionRead, workspace) { + httpapi.ResourceNotFound(rw) return } diff --git a/coderd/workspaces.go b/coderd/workspaces.go index b9e09f50513b4..5c351fd63b772 100644 --- a/coderd/workspaces.go +++ b/coderd/workspaces.go @@ -31,7 +31,8 @@ import ( func (api *API) workspace(rw http.ResponseWriter, r *http.Request) { workspace := httpmw.WorkspaceParam(r) - if !api.Authorize(rw, r, rbac.ActionRead, workspace) { + if !api.Authorize(r, rbac.ActionRead, workspace) { + httpapi.ResourceNotFound(rw) return } @@ -191,8 +192,7 @@ func (api *API) workspaceByOwnerAndName(rw http.ResponseWriter, r *http.Request) }) } if errors.Is(err, sql.ErrNoRows) { - // Do not leak information if the workspace exists or not - httpapi.Forbidden(rw) + httpapi.ResourceNotFound(rw) return } if err != nil { @@ -202,7 +202,8 @@ func (api *API) workspaceByOwnerAndName(rw http.ResponseWriter, r *http.Request) }) return } - if !api.Authorize(rw, r, rbac.ActionRead, workspace) { + if !api.Authorize(r, rbac.ActionRead, workspace) { + httpapi.ResourceNotFound(rw) return } @@ -247,8 +248,9 @@ func (api *API) workspaceByOwnerAndName(rw http.ResponseWriter, r *http.Request) func (api *API) postWorkspacesByOrganization(rw http.ResponseWriter, r *http.Request) { organization := httpmw.OrganizationParam(r) apiKey := httpmw.APIKey(r) - if !api.Authorize(rw, r, rbac.ActionCreate, + if !api.Authorize(r, rbac.ActionCreate, rbac.ResourceWorkspace.InOrg(organization.ID).WithOwner(apiKey.UserID.String())) { + httpapi.ResourceNotFound(rw) return } @@ -488,8 +490,8 @@ func (api *API) postWorkspacesByOrganization(rw http.ResponseWriter, r *http.Req func (api *API) putWorkspaceAutostart(rw http.ResponseWriter, r *http.Request) { workspace := httpmw.WorkspaceParam(r) - if !api.Authorize(rw, r, rbac.ActionUpdate, rbac.ResourceWorkspace. - InOrg(workspace.OrganizationID).WithOwner(workspace.OwnerID.String()).WithID(workspace.ID.String())) { + if !api.Authorize(r, rbac.ActionUpdate, workspace) { + httpapi.ResourceNotFound(rw) return } @@ -531,8 +533,8 @@ func (api *API) putWorkspaceAutostart(rw http.ResponseWriter, r *http.Request) { func (api *API) putWorkspaceTTL(rw http.ResponseWriter, r *http.Request) { workspace := httpmw.WorkspaceParam(r) - if !api.Authorize(rw, r, rbac.ActionUpdate, rbac.ResourceWorkspace. - InOrg(workspace.OrganizationID).WithOwner(workspace.OwnerID.String()).WithID(workspace.ID.String())) { + if !api.Authorize(r, rbac.ActionUpdate, workspace) { + httpapi.ResourceNotFound(rw) return } @@ -620,7 +622,8 @@ func (api *API) putWorkspaceTTL(rw http.ResponseWriter, r *http.Request) { func (api *API) putExtendWorkspace(rw http.ResponseWriter, r *http.Request) { workspace := httpmw.WorkspaceParam(r) - if !api.Authorize(rw, r, rbac.ActionUpdate, workspace) { + if !api.Authorize(r, rbac.ActionUpdate, workspace) { + httpapi.ResourceNotFound(rw) return } @@ -677,7 +680,8 @@ func (api *API) putExtendWorkspace(rw http.ResponseWriter, r *http.Request) { func (api *API) watchWorkspace(rw http.ResponseWriter, r *http.Request) { workspace := httpmw.WorkspaceParam(r) - if !api.Authorize(rw, r, rbac.ActionRead, workspace) { + if !api.Authorize(r, rbac.ActionRead, workspace) { + httpapi.ResourceNotFound(rw) return } diff --git a/coderd/workspaces_test.go b/coderd/workspaces_test.go index cda6688f4d7af..c61b10d61f174 100644 --- a/coderd/workspaces_test.go +++ b/coderd/workspaces_test.go @@ -2,7 +2,6 @@ package coderd_test import ( "context" - "fmt" "net/http" "testing" "time" @@ -295,7 +294,7 @@ func TestWorkspaceByOwnerAndName(t *testing.T) { // Then: // When we call without includes_deleted, we don't expect to get the workspace back _, err = client.WorkspaceByOwnerAndName(context.Background(), workspace.OwnerName, workspace.Name, codersdk.WorkspaceOptions{}) - require.ErrorContains(t, err, "403") + require.ErrorContains(t, err, "404") // Then: // When we call with includes_deleted, we should get the workspace back @@ -660,7 +659,7 @@ func TestWorkspaceUpdateAutostart(t *testing.T) { require.IsType(t, err, &codersdk.Error{}, "expected codersdk.Error") coderSDKErr, _ := err.(*codersdk.Error) //nolint:errorlint require.Equal(t, coderSDKErr.StatusCode(), 404, "expected status code 404") - require.Equal(t, fmt.Sprintf("Workspace %q does not exist.", wsid), coderSDKErr.Message, "unexpected response code") + require.Contains(t, coderSDKErr.Message, "Resource not found", "unexpected response code") }) } @@ -775,7 +774,7 @@ func TestWorkspaceUpdateTTL(t *testing.T) { require.IsType(t, err, &codersdk.Error{}, "expected codersdk.Error") coderSDKErr, _ := err.(*codersdk.Error) //nolint:errorlint require.Equal(t, coderSDKErr.StatusCode(), 404, "expected status code 404") - require.Equal(t, fmt.Sprintf("Workspace %q does not exist.", wsid), coderSDKErr.Message, "unexpected response code") + require.Contains(t, coderSDKErr.Message, "Resource not found", "unexpected response code") }) } diff --git a/scripts/rules.go b/scripts/rules.go index 7f0d61e1969b7..7bf64eb6b4b82 100644 --- a/scripts/rules.go +++ b/scripts/rules.go @@ -150,3 +150,14 @@ func HttpAPIErrorMessage(m dsl.Matcher) { At(m["m"]). Report("Field \"Message\" should be a proper sentence with a capitalized first letter and ending in punctuation. $m") } + +// ProperRBACReturn ensures we always write to the response writer after a +// call to Authorize. If we just do a return, the client will get a status code +// 200, which is incorrect. +func ProperRBACReturn(m dsl.Matcher) { + m.Match(` + if !$_.Authorize($*_) { + return + } + `).Report("Must write to 'ResponseWriter' before returning'") +}