Skip to content

Commit 6ef1647

Browse files
EmyrkEmyrk
committed
chore: Manually forbidden after Authorize
1 parent c7f976e commit 6ef1647

21 files changed

+171
-114
lines changed

‎coderd/authorize.go

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@ import (
77

88
"cdr.dev/slog"
99

10-
"github.com/coder/coder/coderd/httpapi"
1110
"github.com/coder/coder/coderd/httpmw"
1211
"github.com/coder/coder/coderd/rbac"
1312
)
@@ -17,12 +16,17 @@ func AuthorizeFilter[O rbac.Objecter](api *API, r *http.Request, action rbac.Act
1716
return rbac.Filter(r.Context(), api.Authorizer, roles.ID.String(), roles.Roles, action, objects)
1817
}
1918

20-
func (api *API) Authorize(rw http.ResponseWriter, r *http.Request, action rbac.Action, object rbac.Objecter) bool {
19+
// Authorize will return false if the user is not authorized to do the action.
20+
// This function will log appropriately, but the caller must return an
21+
// error to the api client.
22+
// Eg:
23+
// if !api.Authorize(...) {
24+
// httpapi.Forbidden(rw)
25+
// }
26+
func (api *API) Authorize(r *http.Request, action rbac.Action, object rbac.Objecter) bool {
2127
roles := httpmw.AuthorizationUserRoles(r)
2228
err := api.Authorizer.ByRoleName(r.Context(), roles.ID.String(), roles.Roles, action, object.RBACObject())
2329
if err != nil {
24-
httpapi.Forbidden(rw)
25-
2630
// Log the errors for debugging
2731
internalError := new(rbac.UnauthorizedError)
2832
logger := api.Logger

‎coderd/files.go

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,8 @@ func (api *API) postFile(rw http.ResponseWriter, r *http.Request) {
2222
apiKey := httpmw.APIKey(r)
2323
// This requires the site wide action to create files.
2424
// Once created, a user can read their own files uploaded
25-
if !api.Authorize(rw, r, rbac.ActionCreate, rbac.ResourceFile) {
25+
if !api.Authorize(r, rbac.ActionCreate, rbac.ResourceFile) {
26+
httpapi.Forbidden(rw)
2627
return
2728
}
2829

@@ -86,9 +87,7 @@ func (api *API) fileByHash(rw http.ResponseWriter, r *http.Request) {
8687
}
8788
file, err := api.Database.GetFileByHash(r.Context(), hash)
8889
if errors.Is(err, sql.ErrNoRows) {
89-
httpapi.Write(rw, http.StatusNotFound, httpapi.Response{
90-
Message: fmt.Sprintf("File %q not found.", hash),
91-
})
90+
httpapi.ResourceNotFound(rw, fmt.Sprintf("File %s", hash))
9291
return
9392
}
9493
if err != nil {
@@ -99,8 +98,10 @@ func (api *API) fileByHash(rw http.ResponseWriter, r *http.Request) {
9998
return
10099
}
101100

102-
if !api.Authorize(rw, r, rbac.ActionRead,
101+
if !api.Authorize(r, rbac.ActionRead,
103102
rbac.ResourceFile.WithOwner(file.CreatedBy.String()).WithID(file.Hash)) {
103+
// Return 404 to not leak the file exists
104+
httpapi.ResourceNotFound(rw, fmt.Sprintf("File %s", hash))
104105
return
105106
}
106107

‎coderd/gitsshkey.go

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,8 @@ import (
1414
func (api *API) regenerateGitSSHKey(rw http.ResponseWriter, r *http.Request) {
1515
user := httpmw.UserParam(r)
1616

17-
if !api.Authorize(rw, r, rbac.ActionUpdate, rbac.ResourceUserData.WithOwner(user.ID.String())) {
17+
if !api.Authorize(r, rbac.ActionUpdate, rbac.ResourceUserData.WithOwner(user.ID.String())) {
18+
httpapi.Forbidden(rw)
1819
return
1920
}
2021

@@ -62,7 +63,8 @@ func (api *API) regenerateGitSSHKey(rw http.ResponseWriter, r *http.Request) {
6263
func (api *API) gitSSHKey(rw http.ResponseWriter, r *http.Request) {
6364
user := httpmw.UserParam(r)
6465

65-
if !api.Authorize(rw, r, rbac.ActionRead, rbac.ResourceUserData.WithOwner(user.ID.String())) {
66+
if !api.Authorize(r, rbac.ActionRead, rbac.ResourceUserData.WithOwner(user.ID.String())) {
67+
httpapi.Forbidden(rw)
6668
return
6769
}
6870

‎coderd/httpapi/httpapi.go

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -76,6 +76,12 @@ type Error struct {
7676
Detail string `json:"detail" validate:"required"`
7777
}
7878

79+
func ResourceNotFound(rw http.ResponseWriter, resource string) {
80+
Write(rw, http.StatusNotFound, Response{
81+
Message: fmt.Sprintf("%s does not exist.", resource),
82+
})
83+
}
84+
7985
func Forbidden(rw http.ResponseWriter) {
8086
Write(rw, http.StatusForbidden, Response{
8187
Message: "Forbidden.",

‎coderd/httpmw/organizationparam.go

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -45,9 +45,7 @@ func ExtractOrganizationParam(db database.Store) func(http.Handler) http.Handler
4545

4646
organization, err := db.GetOrganizationByID(r.Context(), orgID)
4747
if errors.Is(err, sql.ErrNoRows) {
48-
httpapi.Write(rw, http.StatusNotFound, httpapi.Response{
49-
Message: fmt.Sprintf("Organization %q does not exist.", orgID),
50-
})
48+
httpapi.ResourceNotFound(rw, fmt.Sprintf("Organization %q", orgID))
5149
return
5250
}
5351
if err != nil {

‎coderd/httpmw/templateparam.go

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -47,9 +47,7 @@ func ExtractTemplateParam(db database.Store) func(http.Handler) http.Handler {
4747
}
4848

4949
if template.Deleted {
50-
httpapi.Write(rw, http.StatusNotFound, httpapi.Response{
51-
Message: fmt.Sprintf("Template %q does not exist.", templateID),
52-
})
50+
httpapi.ResourceNotFound(rw, fmt.Sprintf("Template %s", templateID))
5351
return
5452
}
5553

‎coderd/httpmw/templateversionparam.go

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -34,9 +34,7 @@ func ExtractTemplateVersionParam(db database.Store) func(http.Handler) http.Hand
3434
}
3535
templateVersion, err := db.GetTemplateVersionByID(r.Context(), templateVersionID)
3636
if errors.Is(err, sql.ErrNoRows) {
37-
httpapi.Write(rw, http.StatusNotFound, httpapi.Response{
38-
Message: fmt.Sprintf("Template version %q does not exist.", templateVersionID),
39-
})
37+
httpapi.ResourceNotFound(rw, fmt.Sprintf("Template version %s", templateVersionID))
4038
return
4139
}
4240
if err != nil {

‎coderd/httpmw/workspacebuildparam.go

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -34,9 +34,7 @@ func ExtractWorkspaceBuildParam(db database.Store) func(http.Handler) http.Handl
3434
}
3535
workspaceBuild, err := db.GetWorkspaceBuildByID(r.Context(), workspaceBuildID)
3636
if errors.Is(err, sql.ErrNoRows) {
37-
httpapi.Write(rw, http.StatusNotFound, httpapi.Response{
38-
Message: fmt.Sprintf("Workspace build %q does not exist.", workspaceBuildID),
39-
})
37+
httpapi.ResourceNotFound(rw, fmt.Sprintf("Workspace build %q", workspaceBuildID))
4038
return
4139
}
4240
if err != nil {

‎coderd/httpmw/workspaceparam.go

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -32,9 +32,7 @@ func ExtractWorkspaceParam(db database.Store) func(http.Handler) http.Handler {
3232
}
3333
workspace, err := db.GetWorkspaceByID(r.Context(), workspaceID)
3434
if errors.Is(err, sql.ErrNoRows) {
35-
httpapi.Write(rw, http.StatusNotFound, httpapi.Response{
36-
Message: fmt.Sprintf("Workspace %q does not exist.", workspaceID),
37-
})
35+
httpapi.ResourceNotFound(rw, fmt.Sprintf("Workspace %s", workspaceID))
3836
return
3937
}
4038
if err != nil {

‎coderd/members.go

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -39,13 +39,15 @@ func (api *API) putMemberRoles(rw http.ResponseWriter, r *http.Request) {
3939
added, removed := rbac.ChangeRoleSet(member.Roles, impliedTypes)
4040
for _, roleName := range added {
4141
// Assigning a role requires the create permission.
42-
if !api.Authorize(rw, r, rbac.ActionCreate, rbac.ResourceOrgRoleAssignment.WithID(roleName).InOrg(organization.ID)) {
42+
if !api.Authorize(r, rbac.ActionCreate, rbac.ResourceOrgRoleAssignment.WithID(roleName).InOrg(organization.ID)) {
43+
httpapi.Forbidden(rw)
4344
return
4445
}
4546
}
4647
for _, roleName := range removed {
4748
// Removing a role requires the delete permission.
48-
if !api.Authorize(rw, r, rbac.ActionDelete, rbac.ResourceOrgRoleAssignment.WithID(roleName).InOrg(organization.ID)) {
49+
if !api.Authorize(r, rbac.ActionDelete, rbac.ResourceOrgRoleAssignment.WithID(roleName).InOrg(organization.ID)) {
50+
httpapi.Forbidden(rw)
4951
return
5052
}
5153
}

‎coderd/organizations.go

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -19,9 +19,10 @@ import (
1919
func (api *API) organization(rw http.ResponseWriter, r *http.Request) {
2020
organization := httpmw.OrganizationParam(r)
2121

22-
if !api.Authorize(rw, r, rbac.ActionRead, rbac.ResourceOrganization.
22+
if !api.Authorize(r, rbac.ActionRead, rbac.ResourceOrganization.
2323
InOrg(organization.ID).
2424
WithID(organization.ID.String())) {
25+
httpapi.ResourceNotFound(rw, fmt.Sprintf("Organization %q", organization.ID))
2526
return
2627
}
2728

@@ -32,8 +33,9 @@ func (api *API) postOrganizations(rw http.ResponseWriter, r *http.Request) {
3233
apiKey := httpmw.APIKey(r)
3334
// Create organization uses the organization resource without an OrgID.
3435
// This means you need the site wide permission to make a new organization.
35-
if !api.Authorize(rw, r, rbac.ActionCreate,
36+
if !api.Authorize(r, rbac.ActionCreate,
3637
rbac.ResourceOrganization) {
38+
httpapi.Forbidden(rw)
3739
return
3840
}
3941

‎coderd/parameters.go

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,8 @@ func (api *API) postParameter(rw http.ResponseWriter, r *http.Request) {
2626
if !ok {
2727
return
2828
}
29-
if !api.Authorize(rw, r, rbac.ActionUpdate, obj) {
29+
if !api.Authorize(r, rbac.ActionUpdate, obj) {
30+
httpapi.Forbidden(rw)
3031
return
3132
}
3233

@@ -85,7 +86,8 @@ func (api *API) parameters(rw http.ResponseWriter, r *http.Request) {
8586
return
8687
}
8788

88-
if !api.Authorize(rw, r, rbac.ActionRead, obj) {
89+
if !api.Authorize(r, rbac.ActionRead, obj) {
90+
httpapi.Forbidden(rw)
8991
return
9092
}
9193

@@ -120,8 +122,9 @@ func (api *API) deleteParameter(rw http.ResponseWriter, r *http.Request) {
120122
if !ok {
121123
return
122124
}
123-
// A delete param is still updating the underlying resource for the scope.
124-
if !api.Authorize(rw, r, rbac.ActionUpdate, obj) {
125+
// A deleted param is still updating the underlying resource for the scope.
126+
if !api.Authorize(r, rbac.ActionUpdate, obj) {
127+
httpapi.ResourceNotFound(rw, fmt.Sprintf("Parameter %s with scope %s", scopeID, scope))
125128
return
126129
}
127130

@@ -132,10 +135,7 @@ func (api *API) deleteParameter(rw http.ResponseWriter, r *http.Request) {
132135
Name: name,
133136
})
134137
if errors.Is(err, sql.ErrNoRows) {
135-
httpapi.Write(rw, http.StatusNotFound, httpapi.Response{
136-
Message: fmt.Sprintf("No parameter found at the provided scope with name %q.", name),
137-
Detail: err.Error(),
138-
})
138+
httpapi.ResourceNotFound(rw, fmt.Sprintf("Parameter %s with scope %s", scopeID, scope))
139139
return
140140
}
141141
if err != nil {

‎coderd/roles.go

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,8 @@ func (api *API) assignableSiteRoles(rw http.ResponseWriter, r *http.Request) {
1616
// TODO: @emyrk in the future, allow granular subsets of roles to be returned based on the
1717
// role of the user.
1818

19-
if !api.Authorize(rw, r, rbac.ActionRead, rbac.ResourceRoleAssignment) {
19+
if !api.Authorize(r, rbac.ActionRead, rbac.ResourceRoleAssignment) {
20+
httpapi.Forbidden(rw)
2021
return
2122
}
2223

@@ -30,7 +31,8 @@ func (api *API) assignableOrgRoles(rw http.ResponseWriter, r *http.Request) {
3031
// role of the user.
3132
organization := httpmw.OrganizationParam(r)
3233

33-
if !api.Authorize(rw, r, rbac.ActionRead, rbac.ResourceOrgRoleAssignment.InOrg(organization.ID)) {
34+
if !api.Authorize(r, rbac.ActionRead, rbac.ResourceOrgRoleAssignment.InOrg(organization.ID)) {
35+
httpapi.Forbidden(rw)
3436
return
3537
}
3638

@@ -41,7 +43,8 @@ func (api *API) assignableOrgRoles(rw http.ResponseWriter, r *http.Request) {
4143
func (api *API) checkPermissions(rw http.ResponseWriter, r *http.Request) {
4244
user := httpmw.UserParam(r)
4345

44-
if !api.Authorize(rw, r, rbac.ActionRead, rbac.ResourceUser.WithID(user.ID.String())) {
46+
if !api.Authorize(r, rbac.ActionRead, rbac.ResourceUser.WithID(user.ID.String())) {
47+
httpapi.Forbidden(rw)
4548
return
4649
}
4750

‎coderd/templates.go

Lines changed: 14 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,11 @@ var (
2828
func (api *API) template(rw http.ResponseWriter, r *http.Request) {
2929
template := httpmw.TemplateParam(r)
3030

31+
if !api.Authorize(r, rbac.ActionRead, template) {
32+
httpapi.ResourceNotFound(rw, fmt.Sprintf("Template %s", template.ID))
33+
return
34+
}
35+
3136
workspaceCounts, err := api.Database.GetWorkspaceOwnerCountsByTemplateIDs(r.Context(), []uuid.UUID{template.ID})
3237
if errors.Is(err, sql.ErrNoRows) {
3338
err = nil
@@ -40,10 +45,6 @@ func (api *API) template(rw http.ResponseWriter, r *http.Request) {
4045
return
4146
}
4247

43-
if !api.Authorize(rw, r, rbac.ActionRead, template) {
44-
return
45-
}
46-
4748
count := uint32(0)
4849
if len(workspaceCounts) > 0 {
4950
count = uint32(workspaceCounts[0].Count)
@@ -54,7 +55,8 @@ func (api *API) template(rw http.ResponseWriter, r *http.Request) {
5455

5556
func (api *API) deleteTemplate(rw http.ResponseWriter, r *http.Request) {
5657
template := httpmw.TemplateParam(r)
57-
if !api.Authorize(rw, r, rbac.ActionDelete, template) {
58+
if !api.Authorize(r, rbac.ActionDelete, template) {
59+
httpapi.ResourceNotFound(rw, fmt.Sprintf("Template %s", template.ID))
5860
return
5961
}
6062

@@ -97,7 +99,8 @@ func (api *API) deleteTemplate(rw http.ResponseWriter, r *http.Request) {
9799
func (api *API) postTemplateByOrganization(rw http.ResponseWriter, r *http.Request) {
98100
var createTemplate codersdk.CreateTemplateRequest
99101
organization := httpmw.OrganizationParam(r)
100-
if !api.Authorize(rw, r, rbac.ActionCreate, rbac.ResourceTemplate.InOrg(organization.ID)) {
102+
if !api.Authorize(r, rbac.ActionCreate, rbac.ResourceTemplate.InOrg(organization.ID)) {
103+
httpapi.Forbidden(rw)
101104
return
102105
}
103106

@@ -270,9 +273,7 @@ func (api *API) templateByOrganizationAndName(rw http.ResponseWriter, r *http.Re
270273
})
271274
if err != nil {
272275
if errors.Is(err, sql.ErrNoRows) {
273-
httpapi.Write(rw, http.StatusNotFound, httpapi.Response{
274-
Message: fmt.Sprintf("No template found by name %q in the %q organization.", templateName, organization.Name),
275-
})
276+
httpapi.ResourceNotFound(rw, fmt.Sprintf("Template %s in organization %s", templateName, organization.Name))
276277
return
277278
}
278279

@@ -283,7 +284,8 @@ func (api *API) templateByOrganizationAndName(rw http.ResponseWriter, r *http.Re
283284
return
284285
}
285286

286-
if !api.Authorize(rw, r, rbac.ActionRead, template) {
287+
if !api.Authorize(r, rbac.ActionRead, template) {
288+
httpapi.ResourceNotFound(rw, fmt.Sprintf("Template %s in organization %s", templateName, organization.Name))
287289
return
288290
}
289291

@@ -309,7 +311,8 @@ func (api *API) templateByOrganizationAndName(rw http.ResponseWriter, r *http.Re
309311

310312
func (api *API) patchTemplateMeta(rw http.ResponseWriter, r *http.Request) {
311313
template := httpmw.TemplateParam(r)
312-
if !api.Authorize(rw, r, rbac.ActionUpdate, template) {
314+
if !api.Authorize(r, rbac.ActionUpdate, template) {
315+
httpapi.ResourceNotFound(rw, fmt.Sprintf("Template %s", template.ID))
313316
return
314317
}
315318

0 commit comments

Comments
 (0)