From a92f132f988090b0f1b6c9ea4eb8b96979bf2ddd Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 8 Jun 2022 18:45:20 -0500 Subject: [PATCH 01/13] feat: Return more 404s vs 403s --- coderd/files.go | 4 +++- coderd/parameters.go | 4 +++- coderd/templateversions.go | 4 +++- coderd/users.go | 11 +++++++---- coderd/workspaces.go | 5 +++-- 5 files changed, 19 insertions(+), 9 deletions(-) diff --git a/coderd/files.go b/coderd/files.go index fb982bb99612a..92d4c396c9cca 100644 --- a/coderd/files.go +++ b/coderd/files.go @@ -86,7 +86,9 @@ 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.Write(rw, http.StatusNotFound, httpapi.Response{ + Message: fmt.Sprintf("File %q not found.", hash), + }) return } if err != nil { diff --git a/coderd/parameters.go b/coderd/parameters.go index eeb8731d2562f..e420e6a8f4a8c 100644 --- a/coderd/parameters.go +++ b/coderd/parameters.go @@ -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/templateversions.go b/coderd/templateversions.go index 64345457e969c..ee3da5417d328 100644 --- a/coderd/templateversions.go +++ b/coderd/templateversions.go @@ -351,7 +351,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 { diff --git a/coderd/users.go b/coderd/users.go index 5ef87e7ead824..708fb1069bd3d 100644 --- a/coderd/users.go +++ b/coderd/users.go @@ -604,13 +604,16 @@ 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.Write(rw, http.StatusNotFound, httpapi.Response{ + Message: fmt.Sprintf("Organization %q not found.", organizationName), + }) return } if err != nil { - httpapi.Forbidden(rw) + httpapi.Write(rw, http.StatusInternalServerError, httpapi.Response{ + Message: "Internal error fetching organization.", + Detail: err.Error(), + }) return } diff --git a/coderd/workspaces.go b/coderd/workspaces.go index 079f880ed44f3..d8b23a0f8e55a 100644 --- a/coderd/workspaces.go +++ b/coderd/workspaces.go @@ -192,8 +192,9 @@ 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.Write(rw, http.StatusNotFound, httpapi.Response{ + Message: fmt.Sprintf("Workspace %q not found.", workspaceName), + }) return } if err != nil { From 4e23ea740065d46dd732cbc2e8ce0fa257e01cbc Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 8 Jun 2022 18:55:46 -0500 Subject: [PATCH 02/13] Fix unit test assertion --- cli/autostart_test.go | 4 ++-- cli/ttl_test.go | 4 ++-- coderd/files_test.go | 2 +- coderd/organizations_test.go | 2 +- coderd/workspaces_test.go | 2 +- 5 files changed, 7 insertions(+), 7 deletions(-) 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 00a0f29fd3811..f19b364d76d94 100644 --- a/cli/ttl_test.go +++ b/cli/ttl_test.go @@ -149,7 +149,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) { @@ -166,7 +166,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/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/organizations_test.go b/coderd/organizations_test.go index adc35c08d8908..2f063a7044440 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) { diff --git a/coderd/workspaces_test.go b/coderd/workspaces_test.go index 73fed89c80c79..b7f44a3d4efee 100644 --- a/coderd/workspaces_test.go +++ b/coderd/workspaces_test.go @@ -295,7 +295,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.WorkspaceByOwnerAndNameParams{}) - require.ErrorContains(t, err, "403") + require.ErrorContains(t, err, "404") // Then: // When we call with includes_deleted, we should get the workspace back From 0562d2c9b5cc6a64fb6abbeed9e6c2cb550015f7 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 9 Jun 2022 11:08:24 -0500 Subject: [PATCH 03/13] return 404 when member of org not found --- coderd/httpmw/organizationparam.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/coderd/httpmw/organizationparam.go b/coderd/httpmw/organizationparam.go index fd847f2cca3d1..a34618bb13213 100644 --- a/coderd/httpmw/organizationparam.go +++ b/coderd/httpmw/organizationparam.go @@ -76,7 +76,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{ + httpapi.Write(rw, http.StatusNotFound, httpapi.Response{ Message: "Not a member of the organization.", }) return From c7f976efdb15705c329676cec06bb245c443b76f Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 9 Jun 2022 11:09:01 -0500 Subject: [PATCH 04/13] fixup! return 404 when member of org not found --- coderd/httpmw/organizationparam_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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) { From 6ef164758574120c22ac2e28af0e311ac5e909ca Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 10 Jun 2022 09:44:14 -0500 Subject: [PATCH 05/13] chore: Manually forbidden after Authorize --- coderd/authorize.go | 12 ++++--- coderd/files.go | 11 +++--- coderd/gitsshkey.go | 6 ++-- coderd/httpapi/httpapi.go | 6 ++++ coderd/httpmw/organizationparam.go | 4 +-- coderd/httpmw/templateparam.go | 4 +-- coderd/httpmw/templateversionparam.go | 4 +-- coderd/httpmw/workspacebuildparam.go | 4 +-- coderd/httpmw/workspaceparam.go | 4 +-- coderd/members.go | 6 ++-- coderd/organizations.go | 6 ++-- coderd/parameters.go | 16 ++++----- coderd/roles.go | 9 +++-- coderd/templates.go | 25 ++++++++------ coderd/templateversions.go | 48 ++++++++++++++++++--------- coderd/users.go | 40 +++++++++++++--------- coderd/workspaceapps.go | 7 ++-- coderd/workspacebuilds.go | 32 ++++++++++-------- coderd/workspaceresources.go | 3 +- coderd/workspaces.go | 27 ++++++++------- scripts/rules.go | 11 ++++++ 21 files changed, 171 insertions(+), 114 deletions(-) diff --git a/coderd/authorize.go b/coderd/authorize.go index 2f36a45e16886..6b6bf2c539d0a 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,17 @@ 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) +// } +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/files.go b/coderd/files.go index 92d4c396c9cca..59dc6cf2eee42 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,9 +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.Write(rw, http.StatusNotFound, httpapi.Response{ - Message: fmt.Sprintf("File %q not found.", hash), - }) + httpapi.ResourceNotFound(rw, fmt.Sprintf("File %s", hash)) return } if err != nil { @@ -99,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, fmt.Sprintf("File %s", hash)) return } diff --git a/coderd/gitsshkey.go b/coderd/gitsshkey.go index 11184864589c6..90aede47ce7b1 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.Forbidden(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.Forbidden(rw) return } diff --git a/coderd/httpapi/httpapi.go b/coderd/httpapi/httpapi.go index aeb63d8574c84..0c3b3e083b456 100644 --- a/coderd/httpapi/httpapi.go +++ b/coderd/httpapi/httpapi.go @@ -76,6 +76,12 @@ type Error struct { Detail string `json:"detail" validate:"required"` } +func ResourceNotFound(rw http.ResponseWriter, resource string) { + Write(rw, http.StatusNotFound, Response{ + Message: fmt.Sprintf("%s does not exist.", 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 a34618bb13213..e7c065d7dd591 100644 --- a/coderd/httpmw/organizationparam.go +++ b/coderd/httpmw/organizationparam.go @@ -45,9 +45,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, fmt.Sprintf("Organization %q", orgID)) return } if err != nil { diff --git a/coderd/httpmw/templateparam.go b/coderd/httpmw/templateparam.go index 05884304b1566..ffb095f374f79 100644 --- a/coderd/httpmw/templateparam.go +++ b/coderd/httpmw/templateparam.go @@ -47,9 +47,7 @@ func ExtractTemplateParam(db database.Store) func(http.Handler) http.Handler { } if template.Deleted { - httpapi.Write(rw, http.StatusNotFound, httpapi.Response{ - Message: fmt.Sprintf("Template %q does not exist.", templateID), - }) + httpapi.ResourceNotFound(rw, fmt.Sprintf("Template %s", templateID)) return } diff --git a/coderd/httpmw/templateversionparam.go b/coderd/httpmw/templateversionparam.go index 96be063defa9f..cf3e9823c6fc7 100644 --- a/coderd/httpmw/templateversionparam.go +++ b/coderd/httpmw/templateversionparam.go @@ -34,9 +34,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, fmt.Sprintf("Template version %s", templateVersionID)) return } if err != nil { diff --git a/coderd/httpmw/workspacebuildparam.go b/coderd/httpmw/workspacebuildparam.go index 719a2b5c842ba..bfef9b0684431 100644 --- a/coderd/httpmw/workspacebuildparam.go +++ b/coderd/httpmw/workspacebuildparam.go @@ -34,9 +34,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, fmt.Sprintf("Workspace build %q", workspaceBuildID)) return } if err != nil { diff --git a/coderd/httpmw/workspaceparam.go b/coderd/httpmw/workspaceparam.go index dd2112793b320..62585ff28e439 100644 --- a/coderd/httpmw/workspaceparam.go +++ b/coderd/httpmw/workspaceparam.go @@ -32,9 +32,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, fmt.Sprintf("Workspace %s", workspaceID)) 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..26c29d952f5f3 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, fmt.Sprintf("Organization %q", organization.ID)) return } @@ -32,8 +33,9 @@ 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, + if !api.Authorize(r, rbac.ActionCreate, rbac.ResourceOrganization) { + httpapi.Forbidden(rw) return } diff --git a/coderd/parameters.go b/coderd/parameters.go index e420e6a8f4a8c..c2e1fd924e87e 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.Forbidden(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.Forbidden(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, fmt.Sprintf("Parameter %s with scope %s", scopeID, scope)) 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, fmt.Sprintf("Parameter %s with scope %s", scopeID, scope)) return } if err != nil { diff --git a/coderd/roles.go b/coderd/roles.go index ceeca65ac0984..c46a9d4dd05c1 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.Forbidden(rw) return } diff --git a/coderd/templates.go b/coderd/templates.go index 386a360111960..39f08c5876b3e 100644 --- a/coderd/templates.go +++ b/coderd/templates.go @@ -28,6 +28,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, fmt.Sprintf("Template %s", template.ID)) + return + } + workspaceCounts, err := api.Database.GetWorkspaceOwnerCountsByTemplateIDs(r.Context(), []uuid.UUID{template.ID}) if errors.Is(err, sql.ErrNoRows) { err = nil @@ -40,10 +45,6 @@ func (api *API) template(rw http.ResponseWriter, r *http.Request) { return } - if !api.Authorize(rw, r, rbac.ActionRead, template) { - return - } - count := uint32(0) if len(workspaceCounts) > 0 { count = uint32(workspaceCounts[0].Count) @@ -54,7 +55,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, fmt.Sprintf("Template %s", template.ID)) return } @@ -97,7 +99,8 @@ func (api *API) deleteTemplate(rw http.ResponseWriter, r *http.Request) { func (api *API) postTemplateByOrganization(rw http.ResponseWriter, r *http.Request) { var createTemplate codersdk.CreateTemplateRequest organization := httpmw.OrganizationParam(r) - if !api.Authorize(rw, r, rbac.ActionCreate, rbac.ResourceTemplate.InOrg(organization.ID)) { + if !api.Authorize(r, rbac.ActionCreate, rbac.ResourceTemplate.InOrg(organization.ID)) { + httpapi.Forbidden(rw) return } @@ -270,9 +273,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, fmt.Sprintf("Template %s in organization %s", templateName, organization.Name)) return } @@ -283,7 +284,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, fmt.Sprintf("Template %s in organization %s", templateName, organization.Name)) return } @@ -309,7 +311,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, fmt.Sprintf("Template %s", template.ID)) return } diff --git a/coderd/templateversions.go b/coderd/templateversions.go index ee3da5417d328..942f28f01b52a 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, fmt.Sprintf("Template version %s", templateVersion.ID)) 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, fmt.Sprintf("Template version %s", templateVersion.ID)) 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, fmt.Sprintf("Template version %s", templateVersion.ID)) 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, fmt.Sprintf("Template version %s", templateVersion.ID)) 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, fmt.Sprintf("Template version %s", templateVersion.ID)) 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.Forbidden(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, fmt.Sprintf("Template version %s", templateVersion.ID)) 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, fmt.Sprintf("Template version %s", templateVersion.ID)) return database.ProvisionerJob{}, false } @@ -368,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 } @@ -393,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, fmt.Sprintf("Template %s", template.ID)) return } @@ -480,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, fmt.Sprintf("Template %s", template.ID)) return } @@ -519,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, fmt.Sprintf("Template %s", template.ID)) return } @@ -605,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.Forbidden(rw) return } - if !api.Authorize(rw, r, rbac.ActionRead, file) { + if !api.Authorize(r, rbac.ActionRead, file) { + httpapi.Forbidden(rw) return } @@ -690,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, fmt.Sprintf("Template version %s", templateVersion.ID)) return } @@ -711,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, fmt.Sprintf("Template version %s", templateVersion.ID)) return } diff --git a/coderd/users.go b/coderd/users.go index 708fb1069bd3d..e1c84a587e5ef 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.Forbidden(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.Forbidden(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.Forbidden(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.Forbidden(rw) return } @@ -388,7 +394,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.Forbidden(rw) return } @@ -462,8 +469,9 @@ 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. + if !api.Authorize(r, rbac.ActionRead, rbac.ResourceUserData. WithOwner(user.ID.String())) { + httpapi.Forbidden(rw) return } @@ -517,13 +525,15 @@ func (api *API) putUserRoles(rw http.ResponseWriter, r *http.Request) { 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 } } @@ -604,9 +614,7 @@ func (api *API) organizationByUserAndName(rw http.ResponseWriter, r *http.Reques organizationName := chi.URLParam(r, "organizationname") organization, err := api.Database.GetOrganizationByName(r.Context(), organizationName) if errors.Is(err, sql.ErrNoRows) { - httpapi.Write(rw, http.StatusNotFound, httpapi.Response{ - Message: fmt.Sprintf("Organization %q not found.", organizationName), - }) + httpapi.ResourceNotFound(rw, fmt.Sprintf("Organization %q", organizationName)) return } if err != nil { @@ -617,10 +625,11 @@ func (api *API) organizationByUserAndName(rw http.ResponseWriter, r *http.Reques 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, fmt.Sprintf("Organization %q", organizationName)) return } @@ -685,7 +694,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.Forbidden(rw) return } diff --git a/coderd/workspaceapps.go b/coderd/workspaceapps.go index 7ae72752bd232..0b1f7a9c5c89d 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, fmt.Sprintf("Workspace")) 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, fmt.Sprintf("Workspace")) return } diff --git a/coderd/workspacebuilds.go b/coderd/workspacebuilds.go index 5a2f567572f84..542c209d01ea7 100644 --- a/coderd/workspacebuilds.go +++ b/coderd/workspacebuilds.go @@ -23,8 +23,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, fmt.Sprintf("Workspace build %s", workspaceBuild.ID)) return } @@ -52,8 +53,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, fmt.Sprintf("Workspace %s", workspace.ID)) return } @@ -154,20 +155,19 @@ func (api *API) workspaceBuilds(rw http.ResponseWriter, r *http.Request) { 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, fmt.Sprintf("Workspace build %s", workspaceBuildName)) 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, fmt.Sprintf("Workspace build %s", workspaceBuildName)) return } if err != nil { @@ -218,8 +218,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, fmt.Sprintf("Workspace %s", workspace.ID)) return } @@ -391,8 +392,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, fmt.Sprintf("Workspace build %q", workspaceBuild.ID)) return } @@ -445,8 +447,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, fmt.Sprintf("Workspace build %s", workspaceBuild.ID)) return } @@ -471,8 +474,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, fmt.Sprintf("Workspace build %s", workspaceBuild.ID)) return } @@ -497,8 +501,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, fmt.Sprintf("Workspace build %s", workspaceBuild.ID)) return } diff --git a/coderd/workspaceresources.go b/coderd/workspaceresources.go index dd5ec5d79eca5..89736045d42f1 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.Forbidden(rw) return } diff --git a/coderd/workspaces.go b/coderd/workspaces.go index d8b23a0f8e55a..2a5a4af1bdb65 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, fmt.Sprintf("Workspace %s", workspace.ID)) return } @@ -192,9 +193,7 @@ func (api *API) workspaceByOwnerAndName(rw http.ResponseWriter, r *http.Request) }) } if errors.Is(err, sql.ErrNoRows) { - httpapi.Write(rw, http.StatusNotFound, httpapi.Response{ - Message: fmt.Sprintf("Workspace %q not found.", workspaceName), - }) + httpapi.ResourceNotFound(rw, fmt.Sprintf("Workspace %s", workspaceName)) return } if err != nil { @@ -204,7 +203,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, fmt.Sprintf("Workspace %s", workspace.Name)) return } @@ -240,8 +240,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.Forbidden(rw) return } @@ -480,8 +481,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, fmt.Sprintf("Workspace %s", workspace.ID)) return } @@ -523,8 +524,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, fmt.Sprintf("Workspace %s", workspace.ID)) return } @@ -572,7 +573,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, fmt.Sprintf("Workspace %s", workspace.ID)) return } @@ -629,7 +631,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, fmt.Sprintf("Workspace %s", workspace.ID)) return } 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'") +} From 8f90af4acbaa78541291a77e1e4927fe0135847e Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 10 Jun 2022 09:50:58 -0500 Subject: [PATCH 06/13] Use %q over %s --- coderd/authorize.go | 1 + coderd/files.go | 4 ++-- coderd/httpmw/templateparam.go | 2 +- coderd/httpmw/templateversionparam.go | 2 +- coderd/httpmw/workspaceparam.go | 2 +- coderd/organizations_test.go | 2 +- coderd/parameters.go | 4 ++-- coderd/templates.go | 10 +++++----- coderd/templateversions.go | 24 ++++++++++++------------ coderd/workspacebuilds.go | 16 ++++++++-------- coderd/workspaces.go | 14 +++++++------- 11 files changed, 41 insertions(+), 40 deletions(-) diff --git a/coderd/authorize.go b/coderd/authorize.go index 6b6bf2c539d0a..8bfc68ff80452 100644 --- a/coderd/authorize.go +++ b/coderd/authorize.go @@ -22,6 +22,7 @@ func AuthorizeFilter[O rbac.Objecter](api *API, r *http.Request, action rbac.Act // 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) diff --git a/coderd/files.go b/coderd/files.go index 59dc6cf2eee42..5132bca8e5fd3 100644 --- a/coderd/files.go +++ b/coderd/files.go @@ -87,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.ResourceNotFound(rw, fmt.Sprintf("File %s", hash)) + httpapi.ResourceNotFound(rw, fmt.Sprintf("File %q", hash)) return } if err != nil { @@ -101,7 +101,7 @@ func (api *API) fileByHash(rw http.ResponseWriter, r *http.Request) { if !api.Authorize(r, rbac.ActionRead, rbac.ResourceFile.WithOwner(file.CreatedBy.String()).WithID(file.Hash)) { // Return 404 to not leak the file exists - httpapi.ResourceNotFound(rw, fmt.Sprintf("File %s", hash)) + httpapi.ResourceNotFound(rw, fmt.Sprintf("File %q", hash)) return } diff --git a/coderd/httpmw/templateparam.go b/coderd/httpmw/templateparam.go index ffb095f374f79..e7655892c5e18 100644 --- a/coderd/httpmw/templateparam.go +++ b/coderd/httpmw/templateparam.go @@ -47,7 +47,7 @@ func ExtractTemplateParam(db database.Store) func(http.Handler) http.Handler { } if template.Deleted { - httpapi.ResourceNotFound(rw, fmt.Sprintf("Template %s", templateID)) + httpapi.ResourceNotFound(rw, fmt.Sprintf("Template %q", templateID)) return } diff --git a/coderd/httpmw/templateversionparam.go b/coderd/httpmw/templateversionparam.go index cf3e9823c6fc7..423301a20e06a 100644 --- a/coderd/httpmw/templateversionparam.go +++ b/coderd/httpmw/templateversionparam.go @@ -34,7 +34,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.ResourceNotFound(rw, fmt.Sprintf("Template version %s", templateVersionID)) + httpapi.ResourceNotFound(rw, fmt.Sprintf("Template version %q", templateVersionID)) return } if err != nil { diff --git a/coderd/httpmw/workspaceparam.go b/coderd/httpmw/workspaceparam.go index 62585ff28e439..226d5615c9c33 100644 --- a/coderd/httpmw/workspaceparam.go +++ b/coderd/httpmw/workspaceparam.go @@ -32,7 +32,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.ResourceNotFound(rw, fmt.Sprintf("Workspace %s", workspaceID)) + httpapi.ResourceNotFound(rw, fmt.Sprintf("Workspace %q", workspaceID)) return } if err != nil { diff --git a/coderd/organizations_test.go b/coderd/organizations_test.go index 2f063a7044440..d329c65d59832 100644 --- a/coderd/organizations_test.go +++ b/coderd/organizations_test.go @@ -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 c2e1fd924e87e..ae8d0a98085ed 100644 --- a/coderd/parameters.go +++ b/coderd/parameters.go @@ -124,7 +124,7 @@ func (api *API) deleteParameter(rw http.ResponseWriter, r *http.Request) { } // A deleted param is still updating the underlying resource for the scope. if !api.Authorize(r, rbac.ActionUpdate, obj) { - httpapi.ResourceNotFound(rw, fmt.Sprintf("Parameter %s with scope %s", scopeID, scope)) + httpapi.ResourceNotFound(rw, fmt.Sprintf("Parameter %q with scope %q", scopeID, scope)) return } @@ -135,7 +135,7 @@ func (api *API) deleteParameter(rw http.ResponseWriter, r *http.Request) { Name: name, }) if errors.Is(err, sql.ErrNoRows) { - httpapi.ResourceNotFound(rw, fmt.Sprintf("Parameter %s with scope %s", scopeID, scope)) + httpapi.ResourceNotFound(rw, fmt.Sprintf("Parameter %q with scope %q", scopeID, scope)) return } if err != nil { diff --git a/coderd/templates.go b/coderd/templates.go index 39f08c5876b3e..4687667d6d9d9 100644 --- a/coderd/templates.go +++ b/coderd/templates.go @@ -29,7 +29,7 @@ func (api *API) template(rw http.ResponseWriter, r *http.Request) { template := httpmw.TemplateParam(r) if !api.Authorize(r, rbac.ActionRead, template) { - httpapi.ResourceNotFound(rw, fmt.Sprintf("Template %s", template.ID)) + httpapi.ResourceNotFound(rw, fmt.Sprintf("Template %q", template.ID)) return } @@ -56,7 +56,7 @@ 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(r, rbac.ActionDelete, template) { - httpapi.ResourceNotFound(rw, fmt.Sprintf("Template %s", template.ID)) + httpapi.ResourceNotFound(rw, fmt.Sprintf("Template %q", template.ID)) return } @@ -273,7 +273,7 @@ func (api *API) templateByOrganizationAndName(rw http.ResponseWriter, r *http.Re }) if err != nil { if errors.Is(err, sql.ErrNoRows) { - httpapi.ResourceNotFound(rw, fmt.Sprintf("Template %s in organization %s", templateName, organization.Name)) + httpapi.ResourceNotFound(rw, fmt.Sprintf("Template %q in organization %q", templateName, organization.Name)) return } @@ -285,7 +285,7 @@ func (api *API) templateByOrganizationAndName(rw http.ResponseWriter, r *http.Re } if !api.Authorize(r, rbac.ActionRead, template) { - httpapi.ResourceNotFound(rw, fmt.Sprintf("Template %s in organization %s", templateName, organization.Name)) + httpapi.ResourceNotFound(rw, fmt.Sprintf("Template %q in organization %q", templateName, organization.Name)) return } @@ -312,7 +312,7 @@ 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(r, rbac.ActionUpdate, template) { - httpapi.ResourceNotFound(rw, fmt.Sprintf("Template %s", template.ID)) + httpapi.ResourceNotFound(rw, fmt.Sprintf("Template %q", template.ID)) return } diff --git a/coderd/templateversions.go b/coderd/templateversions.go index 942f28f01b52a..55b9fd8799034 100644 --- a/coderd/templateversions.go +++ b/coderd/templateversions.go @@ -23,7 +23,7 @@ import ( func (api *API) templateVersion(rw http.ResponseWriter, r *http.Request) { templateVersion := httpmw.TemplateVersionParam(r) if !api.Authorize(r, rbac.ActionRead, templateVersion) { - httpapi.ResourceNotFound(rw, fmt.Sprintf("Template version %s", templateVersion.ID)) + httpapi.ResourceNotFound(rw, fmt.Sprintf("Template version %q", templateVersion.ID)) return } @@ -42,7 +42,7 @@ 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(r, rbac.ActionUpdate, templateVersion) { - httpapi.ResourceNotFound(rw, fmt.Sprintf("Template version %s", templateVersion.ID)) + httpapi.ResourceNotFound(rw, fmt.Sprintf("Template version %q", templateVersion.ID)) return } @@ -88,7 +88,7 @@ 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(r, rbac.ActionRead, templateVersion) { - httpapi.ResourceNotFound(rw, fmt.Sprintf("Template version %s", templateVersion.ID)) + httpapi.ResourceNotFound(rw, fmt.Sprintf("Template version %q", templateVersion.ID)) return } @@ -136,7 +136,7 @@ func (api *API) templateVersionParameters(rw http.ResponseWriter, r *http.Reques apiKey := httpmw.APIKey(r) templateVersion := httpmw.TemplateVersionParam(r) if !api.Authorize(r, rbac.ActionRead, templateVersion) { - httpapi.ResourceNotFound(rw, fmt.Sprintf("Template version %s", templateVersion.ID)) + httpapi.ResourceNotFound(rw, fmt.Sprintf("Template version %q", templateVersion.ID)) return } @@ -180,7 +180,7 @@ func (api *API) postTemplateVersionDryRun(rw http.ResponseWriter, r *http.Reques apiKey := httpmw.APIKey(r) templateVersion := httpmw.TemplateVersionParam(r) if !api.Authorize(r, rbac.ActionRead, templateVersion) { - httpapi.ResourceNotFound(rw, fmt.Sprintf("Template version %s", templateVersion.ID)) + httpapi.ResourceNotFound(rw, fmt.Sprintf("Template version %q", templateVersion.ID)) return } // We use the workspace RBAC check since we don't want to allow dry runs if @@ -301,7 +301,7 @@ func (api *API) patchTemplateVersionDryRunCancel(rw http.ResponseWriter, r *http } if !api.Authorize(r, rbac.ActionUpdate, rbac.ResourceWorkspace.InOrg(templateVersion.OrganizationID).WithOwner(job.InitiatorID.String())) { - httpapi.ResourceNotFound(rw, fmt.Sprintf("Template version %s", templateVersion.ID)) + httpapi.ResourceNotFound(rw, fmt.Sprintf("Template version %q", templateVersion.ID)) return } @@ -344,7 +344,7 @@ func (api *API) fetchTemplateVersionDryRunJob(rw http.ResponseWriter, r *http.Re jobID = chi.URLParam(r, "jobID") ) if !api.Authorize(r, rbac.ActionRead, templateVersion) { - httpapi.ResourceNotFound(rw, fmt.Sprintf("Template version %s", templateVersion.ID)) + httpapi.ResourceNotFound(rw, fmt.Sprintf("Template version %q", templateVersion.ID)) return database.ProvisionerJob{}, false } @@ -403,7 +403,7 @@ 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(r, rbac.ActionRead, template) { - httpapi.ResourceNotFound(rw, fmt.Sprintf("Template %s", template.ID)) + httpapi.ResourceNotFound(rw, fmt.Sprintf("Template %q", template.ID)) return } @@ -491,7 +491,7 @@ 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(r, rbac.ActionRead, template) { - httpapi.ResourceNotFound(rw, fmt.Sprintf("Template %s", template.ID)) + httpapi.ResourceNotFound(rw, fmt.Sprintf("Template %q", template.ID)) return } @@ -531,7 +531,7 @@ 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(r, rbac.ActionUpdate, template) { - httpapi.ResourceNotFound(rw, fmt.Sprintf("Template %s", template.ID)) + httpapi.ResourceNotFound(rw, fmt.Sprintf("Template %q", template.ID)) return } @@ -705,7 +705,7 @@ func (api *API) postTemplateVersionsByOrganization(rw http.ResponseWriter, r *ht func (api *API) templateVersionResources(rw http.ResponseWriter, r *http.Request) { templateVersion := httpmw.TemplateVersionParam(r) if !api.Authorize(r, rbac.ActionRead, templateVersion) { - httpapi.ResourceNotFound(rw, fmt.Sprintf("Template version %s", templateVersion.ID)) + httpapi.ResourceNotFound(rw, fmt.Sprintf("Template version %q", templateVersion.ID)) return } @@ -727,7 +727,7 @@ func (api *API) templateVersionResources(rw http.ResponseWriter, r *http.Request func (api *API) templateVersionLogs(rw http.ResponseWriter, r *http.Request) { templateVersion := httpmw.TemplateVersionParam(r) if !api.Authorize(r, rbac.ActionRead, templateVersion) { - httpapi.ResourceNotFound(rw, fmt.Sprintf("Template version %s", templateVersion.ID)) + httpapi.ResourceNotFound(rw, fmt.Sprintf("Template version %q", templateVersion.ID)) return } diff --git a/coderd/workspacebuilds.go b/coderd/workspacebuilds.go index 542c209d01ea7..cc41663ca1371 100644 --- a/coderd/workspacebuilds.go +++ b/coderd/workspacebuilds.go @@ -25,7 +25,7 @@ func (api *API) workspaceBuild(rw http.ResponseWriter, r *http.Request) { if !api.Authorize(r, rbac.ActionRead, rbac.ResourceWorkspace. InOrg(workspace.OrganizationID).WithOwner(workspace.OwnerID.String()).WithID(workspace.ID.String())) { - httpapi.ResourceNotFound(rw, fmt.Sprintf("Workspace build %s", workspaceBuild.ID)) + httpapi.ResourceNotFound(rw, fmt.Sprintf("Workspace build %q", workspaceBuild.ID)) return } @@ -54,7 +54,7 @@ func (api *API) workspaceBuilds(rw http.ResponseWriter, r *http.Request) { workspace := httpmw.WorkspaceParam(r) if !api.Authorize(r, rbac.ActionRead, workspace) { - httpapi.ResourceNotFound(rw, fmt.Sprintf("Workspace %s", workspace.ID)) + httpapi.ResourceNotFound(rw, fmt.Sprintf("Workspace %q", workspace.ID)) return } @@ -158,7 +158,7 @@ func (api *API) workspaceBuildByName(rw http.ResponseWriter, r *http.Request) { 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, fmt.Sprintf("Workspace build %s", workspaceBuildName)) + httpapi.ResourceNotFound(rw, fmt.Sprintf("Workspace build %q", workspaceBuildName)) return } @@ -167,7 +167,7 @@ func (api *API) workspaceBuildByName(rw http.ResponseWriter, r *http.Request) { Name: workspaceBuildName, }) if errors.Is(err, sql.ErrNoRows) { - httpapi.ResourceNotFound(rw, fmt.Sprintf("Workspace build %s", workspaceBuildName)) + httpapi.ResourceNotFound(rw, fmt.Sprintf("Workspace build %q", workspaceBuildName)) return } if err != nil { @@ -220,7 +220,7 @@ func (api *API) postWorkspaceBuilds(rw http.ResponseWriter, r *http.Request) { } if !api.Authorize(r, action, rbac.ResourceWorkspace. InOrg(workspace.OrganizationID).WithOwner(workspace.OwnerID.String()).WithID(workspace.ID.String())) { - httpapi.ResourceNotFound(rw, fmt.Sprintf("Workspace %s", workspace.ID)) + httpapi.ResourceNotFound(rw, fmt.Sprintf("Workspace %q", workspace.ID)) return } @@ -449,7 +449,7 @@ func (api *API) workspaceBuildResources(rw http.ResponseWriter, r *http.Request) if !api.Authorize(r, rbac.ActionRead, rbac.ResourceWorkspace. InOrg(workspace.OrganizationID).WithOwner(workspace.OwnerID.String()).WithID(workspace.ID.String())) { - httpapi.ResourceNotFound(rw, fmt.Sprintf("Workspace build %s", workspaceBuild.ID)) + httpapi.ResourceNotFound(rw, fmt.Sprintf("Workspace build %q", workspaceBuild.ID)) return } @@ -476,7 +476,7 @@ func (api *API) workspaceBuildLogs(rw http.ResponseWriter, r *http.Request) { if !api.Authorize(r, rbac.ActionRead, rbac.ResourceWorkspace. InOrg(workspace.OrganizationID).WithOwner(workspace.OwnerID.String()).WithID(workspace.ID.String())) { - httpapi.ResourceNotFound(rw, fmt.Sprintf("Workspace build %s", workspaceBuild.ID)) + httpapi.ResourceNotFound(rw, fmt.Sprintf("Workspace build %q", workspaceBuild.ID)) return } @@ -502,7 +502,7 @@ func (api *API) workspaceBuildState(rw http.ResponseWriter, r *http.Request) { } if !api.Authorize(r, rbac.ActionRead, workspace) { - httpapi.ResourceNotFound(rw, fmt.Sprintf("Workspace build %s", workspaceBuild.ID)) + httpapi.ResourceNotFound(rw, fmt.Sprintf("Workspace build %q", workspaceBuild.ID)) return } diff --git a/coderd/workspaces.go b/coderd/workspaces.go index 2a5a4af1bdb65..a1e99ce53b755 100644 --- a/coderd/workspaces.go +++ b/coderd/workspaces.go @@ -32,7 +32,7 @@ import ( func (api *API) workspace(rw http.ResponseWriter, r *http.Request) { workspace := httpmw.WorkspaceParam(r) if !api.Authorize(r, rbac.ActionRead, workspace) { - httpapi.ResourceNotFound(rw, fmt.Sprintf("Workspace %s", workspace.ID)) + httpapi.ResourceNotFound(rw, fmt.Sprintf("Workspace %q", workspace.ID)) return } @@ -193,7 +193,7 @@ func (api *API) workspaceByOwnerAndName(rw http.ResponseWriter, r *http.Request) }) } if errors.Is(err, sql.ErrNoRows) { - httpapi.ResourceNotFound(rw, fmt.Sprintf("Workspace %s", workspaceName)) + httpapi.ResourceNotFound(rw, fmt.Sprintf("Workspace %q", workspaceName)) return } if err != nil { @@ -204,7 +204,7 @@ func (api *API) workspaceByOwnerAndName(rw http.ResponseWriter, r *http.Request) return } if !api.Authorize(r, rbac.ActionRead, workspace) { - httpapi.ResourceNotFound(rw, fmt.Sprintf("Workspace %s", workspace.Name)) + httpapi.ResourceNotFound(rw, fmt.Sprintf("Workspace %q", workspace.Name)) return } @@ -482,7 +482,7 @@ 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(r, rbac.ActionUpdate, workspace) { - httpapi.ResourceNotFound(rw, fmt.Sprintf("Workspace %s", workspace.ID)) + httpapi.ResourceNotFound(rw, fmt.Sprintf("Workspace %q", workspace.ID)) return } @@ -525,7 +525,7 @@ 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(r, rbac.ActionUpdate, workspace) { - httpapi.ResourceNotFound(rw, fmt.Sprintf("Workspace %s", workspace.ID)) + httpapi.ResourceNotFound(rw, fmt.Sprintf("Workspace %q", workspace.ID)) return } @@ -574,7 +574,7 @@ func (api *API) putExtendWorkspace(rw http.ResponseWriter, r *http.Request) { workspace := httpmw.WorkspaceParam(r) if !api.Authorize(r, rbac.ActionUpdate, workspace) { - httpapi.ResourceNotFound(rw, fmt.Sprintf("Workspace %s", workspace.ID)) + httpapi.ResourceNotFound(rw, fmt.Sprintf("Workspace %q", workspace.ID)) return } @@ -632,7 +632,7 @@ 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(r, rbac.ActionRead, workspace) { - httpapi.ResourceNotFound(rw, fmt.Sprintf("Workspace %s", workspace.ID)) + httpapi.ResourceNotFound(rw, fmt.Sprintf("Workspace %q", workspace.ID)) return } From a6d15edcb64a18864289e57829563672ed94ce0f Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 10 Jun 2022 09:57:09 -0500 Subject: [PATCH 07/13] Fix authoirze test --- coderd/coderd_test.go | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/coderd/coderd_test.go b/coderd/coderd_test.go index dbf3a5cede234..e6543cb0ae79d 100644 --- a/coderd/coderd_test.go +++ b/coderd/coderd_test.go @@ -366,9 +366,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()) @@ -397,7 +394,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") From 815baf96b4757dbc5221fac4a5a554513e4b2ad1 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 10 Jun 2022 11:37:42 -0500 Subject: [PATCH 08/13] Return vague 404 --- coderd/files.go | 4 ++-- coderd/gitsshkey.go | 4 ++-- coderd/httpapi/httpapi.go | 6 ++++-- coderd/httpmw/organizationparam.go | 3 +-- coderd/httpmw/templateparam.go | 2 +- coderd/httpmw/templateversionparam.go | 3 +-- coderd/httpmw/userparam.go | 7 +++++++ coderd/httpmw/workspacebuildparam.go | 3 +-- coderd/httpmw/workspaceparam.go | 3 +-- coderd/organizations.go | 5 ++--- coderd/parameters.go | 8 +++---- coderd/roles.go | 2 +- coderd/templates.go | 12 +++++------ coderd/templateversions.go | 30 +++++++++++++-------------- coderd/users.go | 19 ++++++++--------- coderd/workspaceapps.go | 4 ++-- coderd/workspacebuilds.go | 18 ++++++++-------- coderd/workspaceresources.go | 2 +- coderd/workspaces.go | 16 +++++++------- coderd/workspaces_test.go | 5 ++--- 20 files changed, 79 insertions(+), 77 deletions(-) diff --git a/coderd/files.go b/coderd/files.go index 5132bca8e5fd3..cca2cb391ef5a 100644 --- a/coderd/files.go +++ b/coderd/files.go @@ -87,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.ResourceNotFound(rw, fmt.Sprintf("File %q", hash)) + httpapi.ResourceNotFound(rw) return } if err != nil { @@ -101,7 +101,7 @@ func (api *API) fileByHash(rw http.ResponseWriter, r *http.Request) { if !api.Authorize(r, rbac.ActionRead, rbac.ResourceFile.WithOwner(file.CreatedBy.String()).WithID(file.Hash)) { // Return 404 to not leak the file exists - httpapi.ResourceNotFound(rw, fmt.Sprintf("File %q", hash)) + httpapi.ResourceNotFound(rw) return } diff --git a/coderd/gitsshkey.go b/coderd/gitsshkey.go index 90aede47ce7b1..713dfe9e1bc49 100644 --- a/coderd/gitsshkey.go +++ b/coderd/gitsshkey.go @@ -15,7 +15,7 @@ func (api *API) regenerateGitSSHKey(rw http.ResponseWriter, r *http.Request) { user := httpmw.UserParam(r) if !api.Authorize(r, rbac.ActionUpdate, rbac.ResourceUserData.WithOwner(user.ID.String())) { - httpapi.Forbidden(rw) + httpapi.ResourceNotFound(rw) return } @@ -64,7 +64,7 @@ func (api *API) gitSSHKey(rw http.ResponseWriter, r *http.Request) { user := httpmw.UserParam(r) if !api.Authorize(r, rbac.ActionRead, rbac.ResourceUserData.WithOwner(user.ID.String())) { - httpapi.Forbidden(rw) + httpapi.ResourceNotFound(rw) return } diff --git a/coderd/httpapi/httpapi.go b/coderd/httpapi/httpapi.go index 0c3b3e083b456..9db701866b4de 100644 --- a/coderd/httpapi/httpapi.go +++ b/coderd/httpapi/httpapi.go @@ -76,9 +76,11 @@ type Error struct { Detail string `json:"detail" validate:"required"` } -func ResourceNotFound(rw http.ResponseWriter, resource string) { +// 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: fmt.Sprintf("%s does not exist.", resource), + Message: fmt.Sprintf("Resource not found"), }) } diff --git a/coderd/httpmw/organizationparam.go b/coderd/httpmw/organizationparam.go index e7c065d7dd591..c6a36e2613d97 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,7 +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.ResourceNotFound(rw, fmt.Sprintf("Organization %q", orgID)) + httpapi.ResourceNotFound(rw) return } if err != nil { diff --git a/coderd/httpmw/templateparam.go b/coderd/httpmw/templateparam.go index e7655892c5e18..fb28a10e90a09 100644 --- a/coderd/httpmw/templateparam.go +++ b/coderd/httpmw/templateparam.go @@ -47,7 +47,7 @@ func ExtractTemplateParam(db database.Store) func(http.Handler) http.Handler { } if template.Deleted { - httpapi.ResourceNotFound(rw, fmt.Sprintf("Template %q", templateID)) + httpapi.ResourceNotFound(rw) return } diff --git a/coderd/httpmw/templateversionparam.go b/coderd/httpmw/templateversionparam.go index 423301a20e06a..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,7 +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.ResourceNotFound(rw, fmt.Sprintf("Template version %q", 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 bfef9b0684431..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,7 +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.ResourceNotFound(rw, fmt.Sprintf("Workspace build %q", workspaceBuildID)) + httpapi.ResourceNotFound(rw) return } if err != nil { diff --git a/coderd/httpmw/workspaceparam.go b/coderd/httpmw/workspaceparam.go index 226d5615c9c33..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,7 +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.ResourceNotFound(rw, fmt.Sprintf("Workspace %q", workspaceID)) + httpapi.ResourceNotFound(rw) return } if err != nil { diff --git a/coderd/organizations.go b/coderd/organizations.go index 26c29d952f5f3..30aeac30493cc 100644 --- a/coderd/organizations.go +++ b/coderd/organizations.go @@ -22,7 +22,7 @@ func (api *API) organization(rw http.ResponseWriter, r *http.Request) { if !api.Authorize(r, rbac.ActionRead, rbac.ResourceOrganization. InOrg(organization.ID). WithID(organization.ID.String())) { - httpapi.ResourceNotFound(rw, fmt.Sprintf("Organization %q", organization.ID)) + httpapi.ResourceNotFound(rw) return } @@ -33,8 +33,7 @@ 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(r, rbac.ActionCreate, - rbac.ResourceOrganization) { + if !api.Authorize(r, rbac.ActionCreate, rbac.ResourceOrganization) { httpapi.Forbidden(rw) return } diff --git a/coderd/parameters.go b/coderd/parameters.go index ae8d0a98085ed..615e8f84948ca 100644 --- a/coderd/parameters.go +++ b/coderd/parameters.go @@ -27,7 +27,7 @@ func (api *API) postParameter(rw http.ResponseWriter, r *http.Request) { return } if !api.Authorize(r, rbac.ActionUpdate, obj) { - httpapi.Forbidden(rw) + httpapi.ResourceNotFound(rw) return } @@ -87,7 +87,7 @@ func (api *API) parameters(rw http.ResponseWriter, r *http.Request) { } if !api.Authorize(r, rbac.ActionRead, obj) { - httpapi.Forbidden(rw) + httpapi.ResourceNotFound(rw) return } @@ -124,7 +124,7 @@ func (api *API) deleteParameter(rw http.ResponseWriter, r *http.Request) { } // A deleted param is still updating the underlying resource for the scope. if !api.Authorize(r, rbac.ActionUpdate, obj) { - httpapi.ResourceNotFound(rw, fmt.Sprintf("Parameter %q with scope %q", scopeID, scope)) + httpapi.ResourceNotFound(rw) return } @@ -135,7 +135,7 @@ func (api *API) deleteParameter(rw http.ResponseWriter, r *http.Request) { Name: name, }) if errors.Is(err, sql.ErrNoRows) { - httpapi.ResourceNotFound(rw, fmt.Sprintf("Parameter %q with scope %q", scopeID, scope)) + httpapi.ResourceNotFound(rw) return } if err != nil { diff --git a/coderd/roles.go b/coderd/roles.go index c46a9d4dd05c1..30f78275e0a1d 100644 --- a/coderd/roles.go +++ b/coderd/roles.go @@ -44,7 +44,7 @@ func (api *API) checkPermissions(rw http.ResponseWriter, r *http.Request) { user := httpmw.UserParam(r) if !api.Authorize(r, rbac.ActionRead, rbac.ResourceUser.WithID(user.ID.String())) { - httpapi.Forbidden(rw) + httpapi.ResourceNotFound(rw) return } diff --git a/coderd/templates.go b/coderd/templates.go index 4687667d6d9d9..06ce42c9c436b 100644 --- a/coderd/templates.go +++ b/coderd/templates.go @@ -29,7 +29,7 @@ func (api *API) template(rw http.ResponseWriter, r *http.Request) { template := httpmw.TemplateParam(r) if !api.Authorize(r, rbac.ActionRead, template) { - httpapi.ResourceNotFound(rw, fmt.Sprintf("Template %q", template.ID)) + httpapi.ResourceNotFound(rw) return } @@ -56,7 +56,7 @@ 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(r, rbac.ActionDelete, template) { - httpapi.ResourceNotFound(rw, fmt.Sprintf("Template %q", template.ID)) + httpapi.ResourceNotFound(rw) return } @@ -100,7 +100,7 @@ func (api *API) postTemplateByOrganization(rw http.ResponseWriter, r *http.Reque var createTemplate codersdk.CreateTemplateRequest organization := httpmw.OrganizationParam(r) if !api.Authorize(r, rbac.ActionCreate, rbac.ResourceTemplate.InOrg(organization.ID)) { - httpapi.Forbidden(rw) + httpapi.ResourceNotFound(rw) return } @@ -273,7 +273,7 @@ func (api *API) templateByOrganizationAndName(rw http.ResponseWriter, r *http.Re }) if err != nil { if errors.Is(err, sql.ErrNoRows) { - httpapi.ResourceNotFound(rw, fmt.Sprintf("Template %q in organization %q", templateName, organization.Name)) + httpapi.ResourceNotFound(rw) return } @@ -285,7 +285,7 @@ func (api *API) templateByOrganizationAndName(rw http.ResponseWriter, r *http.Re } if !api.Authorize(r, rbac.ActionRead, template) { - httpapi.ResourceNotFound(rw, fmt.Sprintf("Template %q in organization %q", templateName, organization.Name)) + httpapi.ResourceNotFound(rw) return } @@ -312,7 +312,7 @@ 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(r, rbac.ActionUpdate, template) { - httpapi.ResourceNotFound(rw, fmt.Sprintf("Template %q", template.ID)) + httpapi.ResourceNotFound(rw) return } diff --git a/coderd/templateversions.go b/coderd/templateversions.go index 55b9fd8799034..472fd87f8d9e8 100644 --- a/coderd/templateversions.go +++ b/coderd/templateversions.go @@ -23,7 +23,7 @@ import ( func (api *API) templateVersion(rw http.ResponseWriter, r *http.Request) { templateVersion := httpmw.TemplateVersionParam(r) if !api.Authorize(r, rbac.ActionRead, templateVersion) { - httpapi.ResourceNotFound(rw, fmt.Sprintf("Template version %q", templateVersion.ID)) + httpapi.ResourceNotFound(rw) return } @@ -42,7 +42,7 @@ 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(r, rbac.ActionUpdate, templateVersion) { - httpapi.ResourceNotFound(rw, fmt.Sprintf("Template version %q", templateVersion.ID)) + httpapi.ResourceNotFound(rw) return } @@ -88,7 +88,7 @@ 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(r, rbac.ActionRead, templateVersion) { - httpapi.ResourceNotFound(rw, fmt.Sprintf("Template version %q", templateVersion.ID)) + httpapi.ResourceNotFound(rw) return } @@ -136,7 +136,7 @@ func (api *API) templateVersionParameters(rw http.ResponseWriter, r *http.Reques apiKey := httpmw.APIKey(r) templateVersion := httpmw.TemplateVersionParam(r) if !api.Authorize(r, rbac.ActionRead, templateVersion) { - httpapi.ResourceNotFound(rw, fmt.Sprintf("Template version %q", templateVersion.ID)) + httpapi.ResourceNotFound(rw) return } @@ -180,14 +180,14 @@ func (api *API) postTemplateVersionDryRun(rw http.ResponseWriter, r *http.Reques apiKey := httpmw.APIKey(r) templateVersion := httpmw.TemplateVersionParam(r) if !api.Authorize(r, rbac.ActionRead, templateVersion) { - httpapi.ResourceNotFound(rw, fmt.Sprintf("Template version %q", templateVersion.ID)) + 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, rbac.ResourceWorkspace.InOrg(templateVersion.OrganizationID).WithOwner(apiKey.UserID.String())) { - httpapi.Forbidden(rw) + httpapi.ResourceNotFound(rw) return } @@ -301,7 +301,7 @@ func (api *API) patchTemplateVersionDryRunCancel(rw http.ResponseWriter, r *http } if !api.Authorize(r, rbac.ActionUpdate, rbac.ResourceWorkspace.InOrg(templateVersion.OrganizationID).WithOwner(job.InitiatorID.String())) { - httpapi.ResourceNotFound(rw, fmt.Sprintf("Template version %q", templateVersion.ID)) + httpapi.ResourceNotFound(rw) return } @@ -344,7 +344,7 @@ func (api *API) fetchTemplateVersionDryRunJob(rw http.ResponseWriter, r *http.Re jobID = chi.URLParam(r, "jobID") ) if !api.Authorize(r, rbac.ActionRead, templateVersion) { - httpapi.ResourceNotFound(rw, fmt.Sprintf("Template version %q", templateVersion.ID)) + httpapi.ResourceNotFound(rw) return database.ProvisionerJob{}, false } @@ -403,7 +403,7 @@ 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(r, rbac.ActionRead, template) { - httpapi.ResourceNotFound(rw, fmt.Sprintf("Template %q", template.ID)) + httpapi.ResourceNotFound(rw) return } @@ -491,7 +491,7 @@ 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(r, rbac.ActionRead, template) { - httpapi.ResourceNotFound(rw, fmt.Sprintf("Template %q", template.ID)) + httpapi.ResourceNotFound(rw) return } @@ -531,7 +531,7 @@ 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(r, rbac.ActionUpdate, template) { - httpapi.ResourceNotFound(rw, fmt.Sprintf("Template %q", template.ID)) + httpapi.ResourceNotFound(rw) return } @@ -618,12 +618,12 @@ 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(r, rbac.ActionCreate, rbac.ResourceTemplate.InOrg(organization.ID)) { - httpapi.Forbidden(rw) + httpapi.ResourceNotFound(rw) return } if !api.Authorize(r, rbac.ActionRead, file) { - httpapi.Forbidden(rw) + httpapi.ResourceNotFound(rw) return } @@ -705,7 +705,7 @@ func (api *API) postTemplateVersionsByOrganization(rw http.ResponseWriter, r *ht func (api *API) templateVersionResources(rw http.ResponseWriter, r *http.Request) { templateVersion := httpmw.TemplateVersionParam(r) if !api.Authorize(r, rbac.ActionRead, templateVersion) { - httpapi.ResourceNotFound(rw, fmt.Sprintf("Template version %q", templateVersion.ID)) + httpapi.ResourceNotFound(rw) return } @@ -727,7 +727,7 @@ func (api *API) templateVersionResources(rw http.ResponseWriter, r *http.Request func (api *API) templateVersionLogs(rw http.ResponseWriter, r *http.Request) { templateVersion := httpmw.TemplateVersionParam(r) if !api.Authorize(r, rbac.ActionRead, templateVersion) { - httpapi.ResourceNotFound(rw, fmt.Sprintf("Template version %q", templateVersion.ID)) + httpapi.ResourceNotFound(rw) return } diff --git a/coderd/users.go b/coderd/users.go index e1c84a587e5ef..89f25cb93d3ab 100644 --- a/coderd/users.go +++ b/coderd/users.go @@ -262,7 +262,7 @@ func (api *API) userByName(rw http.ResponseWriter, r *http.Request) { organizationIDs, err := userOrganizationIDs(r.Context(), api, user) if !api.Authorize(r, rbac.ActionRead, rbac.ResourceUser.WithID(user.ID.String())) { - httpapi.Forbidden(rw) + httpapi.ResourceNotFound(rw) return } @@ -281,7 +281,7 @@ func (api *API) putUserProfile(rw http.ResponseWriter, r *http.Request) { user := httpmw.UserParam(r) if !api.Authorize(r, rbac.ActionUpdate, rbac.ResourceUser.WithID(user.ID.String())) { - httpapi.Forbidden(rw) + httpapi.ResourceNotFound(rw) return } @@ -349,7 +349,7 @@ func (api *API) putUserStatus(status database.UserStatus) func(rw http.ResponseW apiKey := httpmw.APIKey(r) if !api.Authorize(r, rbac.ActionDelete, rbac.ResourceUser.WithID(user.ID.String())) { - httpapi.Forbidden(rw) + httpapi.ResourceNotFound(rw) return } @@ -395,7 +395,7 @@ func (api *API) putUserPassword(rw http.ResponseWriter, r *http.Request) { ) if !api.Authorize(r, rbac.ActionUpdate, rbac.ResourceUserData.WithOwner(user.ID.String())) { - httpapi.Forbidden(rw) + httpapi.ResourceNotFound(rw) return } @@ -469,9 +469,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(r, rbac.ActionRead, rbac.ResourceUserData. - WithOwner(user.ID.String())) { - httpapi.Forbidden(rw) + if !api.Authorize(r, rbac.ActionRead, rbac.ResourceUserData.WithOwner(user.ID.String())) { + httpapi.ResourceNotFound(rw) return } @@ -614,7 +613,7 @@ func (api *API) organizationByUserAndName(rw http.ResponseWriter, r *http.Reques organizationName := chi.URLParam(r, "organizationname") organization, err := api.Database.GetOrganizationByName(r.Context(), organizationName) if errors.Is(err, sql.ErrNoRows) { - httpapi.ResourceNotFound(rw, fmt.Sprintf("Organization %q", organizationName)) + httpapi.ResourceNotFound(rw) return } if err != nil { @@ -629,7 +628,7 @@ func (api *API) organizationByUserAndName(rw http.ResponseWriter, r *http.Reques rbac.ResourceOrganization. InOrg(organization.ID). WithID(organization.ID.String())) { - httpapi.ResourceNotFound(rw, fmt.Sprintf("Organization %q", organizationName)) + httpapi.ResourceNotFound(rw) return } @@ -695,7 +694,7 @@ func (api *API) postAPIKey(rw http.ResponseWriter, r *http.Request) { user := httpmw.UserParam(r) if !api.Authorize(r, rbac.ActionCreate, rbac.ResourceAPIKey.WithOwner(user.ID.String())) { - httpapi.Forbidden(rw) + httpapi.ResourceNotFound(rw) return } diff --git a/coderd/workspaceapps.go b/coderd/workspaceapps.go index 0b1f7a9c5c89d..ed14f16a97d83 100644 --- a/coderd/workspaceapps.go +++ b/coderd/workspaceapps.go @@ -32,7 +32,7 @@ func (api *API) workspaceAppsProxyPath(rw http.ResponseWriter, r *http.Request) Name: workspaceParts[0], }) if errors.Is(err, sql.ErrNoRows) { - httpapi.ResourceNotFound(rw, fmt.Sprintf("Workspace")) + httpapi.ResourceNotFound(rw) return } if err != nil { @@ -43,7 +43,7 @@ func (api *API) workspaceAppsProxyPath(rw http.ResponseWriter, r *http.Request) return } if !api.Authorize(r, rbac.ActionRead, workspace) { - httpapi.ResourceNotFound(rw, fmt.Sprintf("Workspace")) + httpapi.ResourceNotFound(rw) return } diff --git a/coderd/workspacebuilds.go b/coderd/workspacebuilds.go index cc41663ca1371..37d072bb5ded1 100644 --- a/coderd/workspacebuilds.go +++ b/coderd/workspacebuilds.go @@ -25,7 +25,7 @@ func (api *API) workspaceBuild(rw http.ResponseWriter, r *http.Request) { if !api.Authorize(r, rbac.ActionRead, rbac.ResourceWorkspace. InOrg(workspace.OrganizationID).WithOwner(workspace.OwnerID.String()).WithID(workspace.ID.String())) { - httpapi.ResourceNotFound(rw, fmt.Sprintf("Workspace build %q", workspaceBuild.ID)) + httpapi.ResourceNotFound(rw) return } @@ -54,7 +54,7 @@ func (api *API) workspaceBuilds(rw http.ResponseWriter, r *http.Request) { workspace := httpmw.WorkspaceParam(r) if !api.Authorize(r, rbac.ActionRead, workspace) { - httpapi.ResourceNotFound(rw, fmt.Sprintf("Workspace %q", workspace.ID)) + httpapi.ResourceNotFound(rw) return } @@ -158,7 +158,7 @@ func (api *API) workspaceBuildByName(rw http.ResponseWriter, r *http.Request) { 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, fmt.Sprintf("Workspace build %q", workspaceBuildName)) + httpapi.ResourceNotFound(rw) return } @@ -167,7 +167,7 @@ func (api *API) workspaceBuildByName(rw http.ResponseWriter, r *http.Request) { Name: workspaceBuildName, }) if errors.Is(err, sql.ErrNoRows) { - httpapi.ResourceNotFound(rw, fmt.Sprintf("Workspace build %q", workspaceBuildName)) + httpapi.ResourceNotFound(rw) return } if err != nil { @@ -220,7 +220,7 @@ func (api *API) postWorkspaceBuilds(rw http.ResponseWriter, r *http.Request) { } if !api.Authorize(r, action, rbac.ResourceWorkspace. InOrg(workspace.OrganizationID).WithOwner(workspace.OwnerID.String()).WithID(workspace.ID.String())) { - httpapi.ResourceNotFound(rw, fmt.Sprintf("Workspace %q", workspace.ID)) + httpapi.ResourceNotFound(rw) return } @@ -394,7 +394,7 @@ func (api *API) patchCancelWorkspaceBuild(rw http.ResponseWriter, r *http.Reques if !api.Authorize(r, rbac.ActionUpdate, rbac.ResourceWorkspace. InOrg(workspace.OrganizationID).WithOwner(workspace.OwnerID.String()).WithID(workspace.ID.String())) { - httpapi.ResourceNotFound(rw, fmt.Sprintf("Workspace build %q", workspaceBuild.ID)) + httpapi.ResourceNotFound(rw) return } @@ -449,7 +449,7 @@ func (api *API) workspaceBuildResources(rw http.ResponseWriter, r *http.Request) if !api.Authorize(r, rbac.ActionRead, rbac.ResourceWorkspace. InOrg(workspace.OrganizationID).WithOwner(workspace.OwnerID.String()).WithID(workspace.ID.String())) { - httpapi.ResourceNotFound(rw, fmt.Sprintf("Workspace build %q", workspaceBuild.ID)) + httpapi.ResourceNotFound(rw) return } @@ -476,7 +476,7 @@ func (api *API) workspaceBuildLogs(rw http.ResponseWriter, r *http.Request) { if !api.Authorize(r, rbac.ActionRead, rbac.ResourceWorkspace. InOrg(workspace.OrganizationID).WithOwner(workspace.OwnerID.String()).WithID(workspace.ID.String())) { - httpapi.ResourceNotFound(rw, fmt.Sprintf("Workspace build %q", workspaceBuild.ID)) + httpapi.ResourceNotFound(rw) return } @@ -502,7 +502,7 @@ func (api *API) workspaceBuildState(rw http.ResponseWriter, r *http.Request) { } if !api.Authorize(r, rbac.ActionRead, workspace) { - httpapi.ResourceNotFound(rw, fmt.Sprintf("Workspace build %q", workspaceBuild.ID)) + httpapi.ResourceNotFound(rw) return } diff --git a/coderd/workspaceresources.go b/coderd/workspaceresources.go index 89736045d42f1..8f38d4d9dc15a 100644 --- a/coderd/workspaceresources.go +++ b/coderd/workspaceresources.go @@ -19,7 +19,7 @@ func (api *API) workspaceResource(rw http.ResponseWriter, r *http.Request) { workspaceResource := httpmw.WorkspaceResourceParam(r) workspace := httpmw.WorkspaceParam(r) if !api.Authorize(r, rbac.ActionRead, workspace) { - httpapi.Forbidden(rw) + httpapi.ResourceNotFound(rw) return } diff --git a/coderd/workspaces.go b/coderd/workspaces.go index a1e99ce53b755..c44a12b194819 100644 --- a/coderd/workspaces.go +++ b/coderd/workspaces.go @@ -32,7 +32,7 @@ import ( func (api *API) workspace(rw http.ResponseWriter, r *http.Request) { workspace := httpmw.WorkspaceParam(r) if !api.Authorize(r, rbac.ActionRead, workspace) { - httpapi.ResourceNotFound(rw, fmt.Sprintf("Workspace %q", workspace.ID)) + httpapi.ResourceNotFound(rw) return } @@ -193,7 +193,7 @@ func (api *API) workspaceByOwnerAndName(rw http.ResponseWriter, r *http.Request) }) } if errors.Is(err, sql.ErrNoRows) { - httpapi.ResourceNotFound(rw, fmt.Sprintf("Workspace %q", workspaceName)) + httpapi.ResourceNotFound(rw) return } if err != nil { @@ -204,7 +204,7 @@ func (api *API) workspaceByOwnerAndName(rw http.ResponseWriter, r *http.Request) return } if !api.Authorize(r, rbac.ActionRead, workspace) { - httpapi.ResourceNotFound(rw, fmt.Sprintf("Workspace %q", workspace.Name)) + httpapi.ResourceNotFound(rw) return } @@ -242,7 +242,7 @@ func (api *API) postWorkspacesByOrganization(rw http.ResponseWriter, r *http.Req apiKey := httpmw.APIKey(r) if !api.Authorize(r, rbac.ActionCreate, rbac.ResourceWorkspace.InOrg(organization.ID).WithOwner(apiKey.UserID.String())) { - httpapi.Forbidden(rw) + httpapi.ResourceNotFound(rw) return } @@ -482,7 +482,7 @@ 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(r, rbac.ActionUpdate, workspace) { - httpapi.ResourceNotFound(rw, fmt.Sprintf("Workspace %q", workspace.ID)) + httpapi.ResourceNotFound(rw) return } @@ -525,7 +525,7 @@ 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(r, rbac.ActionUpdate, workspace) { - httpapi.ResourceNotFound(rw, fmt.Sprintf("Workspace %q", workspace.ID)) + httpapi.ResourceNotFound(rw) return } @@ -574,7 +574,7 @@ func (api *API) putExtendWorkspace(rw http.ResponseWriter, r *http.Request) { workspace := httpmw.WorkspaceParam(r) if !api.Authorize(r, rbac.ActionUpdate, workspace) { - httpapi.ResourceNotFound(rw, fmt.Sprintf("Workspace %q", workspace.ID)) + httpapi.ResourceNotFound(rw) return } @@ -632,7 +632,7 @@ 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(r, rbac.ActionRead, workspace) { - httpapi.ResourceNotFound(rw, fmt.Sprintf("Workspace %q", workspace.ID)) + httpapi.ResourceNotFound(rw) return } diff --git a/coderd/workspaces_test.go b/coderd/workspaces_test.go index b7f44a3d4efee..f93c9b9c94c91 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" @@ -632,7 +631,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.Equal(t, "Resource not found", coderSDKErr.Message, "unexpected response code") }) } @@ -733,7 +732,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.Equal(t, "Resource not found", coderSDKErr.Message, "unexpected response code") }) } From 8bfe1f7dfdc2e8697d11245c9e2dc60e4a8b23f5 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 10 Jun 2022 12:55:16 -0500 Subject: [PATCH 09/13] fix test assert --- coderd/httpapi/httpapi.go | 2 +- coderd/workspacebuilds_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/coderd/httpapi/httpapi.go b/coderd/httpapi/httpapi.go index 9db701866b4de..c12bd1cfe4402 100644 --- a/coderd/httpapi/httpapi.go +++ b/coderd/httpapi/httpapi.go @@ -80,7 +80,7 @@ type Error struct { // to prevent leaking existence of resources. func ResourceNotFound(rw http.ResponseWriter) { Write(rw, http.StatusNotFound, Response{ - Message: fmt.Sprintf("Resource not found"), + Message: "Resource not found", }) } 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) { From 7aa76f7561de51d46c867a93f2524627a638b5c2 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 10 Jun 2022 15:59:31 -0500 Subject: [PATCH 10/13] Fix compile issue --- coderd/templates.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/coderd/templates.go b/coderd/templates.go index c3c817e4cbab6..11e434bd8848f 100644 --- a/coderd/templates.go +++ b/coderd/templates.go @@ -1,6 +1,7 @@ package coderd import ( + "context" "database/sql" "errors" "fmt" @@ -45,7 +46,7 @@ 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 } From 3f077382db64380a98bf00b3ba8d207e1f321312 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 10 Jun 2022 16:02:09 -0500 Subject: [PATCH 11/13] Modify 404 message --- coderd/httpapi/httpapi.go | 2 +- coderd/workspaces_test.go | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/coderd/httpapi/httpapi.go b/coderd/httpapi/httpapi.go index c12bd1cfe4402..79e36f175b26b 100644 --- a/coderd/httpapi/httpapi.go +++ b/coderd/httpapi/httpapi.go @@ -80,7 +80,7 @@ type Error struct { // to prevent leaking existence of resources. func ResourceNotFound(rw http.ResponseWriter) { Write(rw, http.StatusNotFound, Response{ - Message: "Resource not found", + Message: "Resource not found or you do not have access to this resource", }) } diff --git a/coderd/workspaces_test.go b/coderd/workspaces_test.go index 29c612c35349d..c61b10d61f174 100644 --- a/coderd/workspaces_test.go +++ b/coderd/workspaces_test.go @@ -659,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, "Resource not found", coderSDKErr.Message, "unexpected response code") + require.Contains(t, coderSDKErr.Message, "Resource not found", "unexpected response code") }) } @@ -774,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, "Resource not found", coderSDKErr.Message, "unexpected response code") + require.Contains(t, coderSDKErr.Message, "Resource not found", "unexpected response code") }) } From 2c1ac1c3021eab944d82b8b0365fe3c58b79552f Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Mon, 13 Jun 2022 14:07:43 -0500 Subject: [PATCH 12/13] Use proper 404s --- coderd/httpmw/organizationparam.go | 4 +--- coderd/users.go | 2 +- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/coderd/httpmw/organizationparam.go b/coderd/httpmw/organizationparam.go index c6a36e2613d97..83ef445a9d926 100644 --- a/coderd/httpmw/organizationparam.go +++ b/coderd/httpmw/organizationparam.go @@ -73,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.StatusNotFound, httpapi.Response{ - Message: "Not a member of the organization.", - }) + httpapi.ResourceNotFound(rw) return } if err != nil { diff --git a/coderd/users.go b/coderd/users.go index 47515acdd2614..6aaa835c52793 100644 --- a/coderd/users.go +++ b/coderd/users.go @@ -204,7 +204,7 @@ func (api *API) postUser(rw http.ResponseWriter, r *http.Request) { // Create the organization member in the org. if !api.Authorize(r, rbac.ActionCreate, rbac.ResourceOrganizationMember.InOrg(createUser.OrganizationID)) { - httpapi.Forbidden(rw) + httpapi.ResourceNotFound(rw) return } From 93c04dbeee28122a0c52e40db2bee89cf3b2003a Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Mon, 13 Jun 2022 15:39:48 -0500 Subject: [PATCH 13/13] If cannot read user, return 404 --- coderd/users.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/coderd/users.go b/coderd/users.go index 6aaa835c52793..781cc213d1797 100644 --- a/coderd/users.go +++ b/coderd/users.go @@ -522,6 +522,11 @@ 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)