mirror of https://github.com/coder/coder.git
fix: stop leaking User into API handlers unless authorized
Fixes an issue where we extracted the `{user}` parameter from the URL and added it to the API Handler context regardless of whether the caller had permission to read the User.
This commit is contained in:
parent
fbabb43cbb
commit
7c71053eab
|
@ -652,7 +652,6 @@ func New(options *Options) *API {
|
|||
r.Get("/roles", api.assignableOrgRoles)
|
||||
r.Route("/{user}", func(r chi.Router) {
|
||||
r.Use(
|
||||
httpmw.ExtractUserParam(options.Database),
|
||||
httpmw.ExtractOrganizationMemberParam(options.Database),
|
||||
)
|
||||
r.Put("/roles", api.putMemberRoles)
|
||||
|
|
|
@ -5,6 +5,7 @@ import (
|
|||
"net/http"
|
||||
|
||||
"github.com/coder/coder/v2/coderd/database"
|
||||
"github.com/coder/coder/v2/coderd/database/dbauthz"
|
||||
"github.com/coder/coder/v2/coderd/httpapi"
|
||||
"github.com/coder/coder/v2/codersdk"
|
||||
)
|
||||
|
@ -25,8 +26,8 @@ func OrganizationParam(r *http.Request) database.Organization {
|
|||
|
||||
// OrganizationMemberParam returns the organization membership that allowed the query
|
||||
// from the ExtractOrganizationParam handler.
|
||||
func OrganizationMemberParam(r *http.Request) database.OrganizationMember {
|
||||
organizationMember, ok := r.Context().Value(organizationMemberParamContextKey{}).(database.OrganizationMember)
|
||||
func OrganizationMemberParam(r *http.Request) OrganizationMember {
|
||||
organizationMember, ok := r.Context().Value(organizationMemberParamContextKey{}).(OrganizationMember)
|
||||
if !ok {
|
||||
panic("developer error: organization member param middleware not provided")
|
||||
}
|
||||
|
@ -62,14 +63,31 @@ func ExtractOrganizationParam(db database.Store) func(http.Handler) http.Handler
|
|||
}
|
||||
}
|
||||
|
||||
// OrganizationMember is the database object plus the Username. Including the Username in this
|
||||
// middleware is preferable to a join at the SQL layer so that we can keep the autogenerated
|
||||
// database types as they are.
|
||||
type OrganizationMember struct {
|
||||
database.OrganizationMember
|
||||
Username string
|
||||
}
|
||||
|
||||
// ExtractOrganizationMemberParam grabs a user membership from the "organization" and "user" URL parameter.
|
||||
// This middleware requires the ExtractUser and ExtractOrganization middleware higher in the stack
|
||||
func ExtractOrganizationMemberParam(db database.Store) func(http.Handler) http.Handler {
|
||||
return func(next http.Handler) http.Handler {
|
||||
return http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) {
|
||||
ctx := r.Context()
|
||||
// We need to resolve the `{user}` URL parameter so that we can get the userID and
|
||||
// username. We do this as SystemRestricted since the caller might have permission
|
||||
// to access the OrganizationMember object, but *not* the User object. So, it is
|
||||
// very important that we do not add the User object to the request context or otherwise
|
||||
// leak it to the API handler.
|
||||
// nolint:gocritic
|
||||
user, ok := extractUserContext(dbauthz.AsSystemRestricted(ctx), db, rw, r)
|
||||
if !ok {
|
||||
return
|
||||
}
|
||||
organization := OrganizationParam(r)
|
||||
user := UserParam(r)
|
||||
|
||||
organizationMember, err := db.GetOrganizationMemberByUserID(ctx, database.GetOrganizationMemberByUserIDParams{
|
||||
OrganizationID: organization.ID,
|
||||
|
@ -87,7 +105,17 @@ func ExtractOrganizationMemberParam(db database.Store) func(http.Handler) http.H
|
|||
return
|
||||
}
|
||||
|
||||
ctx = context.WithValue(ctx, organizationMemberParamContextKey{}, organizationMember)
|
||||
ctx = context.WithValue(ctx, organizationMemberParamContextKey{}, OrganizationMember{
|
||||
OrganizationMember: organizationMember,
|
||||
// Here we're making one exception to the rule about not leaking data about the user
|
||||
// to the API handler, which is to include the username. If the caller has permission
|
||||
// to read the OrganizationMember, then we're explicitly saying here that they also
|
||||
// have permission to see the member's username, which is itself uncontroversial.
|
||||
//
|
||||
// API handlers need this information for audit logging and returning the owner's
|
||||
// username in response to creating a workspace.
|
||||
Username: user.Username,
|
||||
})
|
||||
next.ServeHTTP(rw, r.WithContext(ctx))
|
||||
})
|
||||
}
|
||||
|
|
|
@ -9,7 +9,6 @@ import (
|
|||
"github.com/google/uuid"
|
||||
|
||||
"github.com/coder/coder/v2/coderd/database"
|
||||
"github.com/coder/coder/v2/coderd/database/dbauthz"
|
||||
"github.com/coder/coder/v2/coderd/httpapi"
|
||||
"github.com/coder/coder/v2/codersdk"
|
||||
)
|
||||
|
@ -38,11 +37,7 @@ func ExtractUserParam(db database.Store) func(http.Handler) http.Handler {
|
|||
return func(next http.Handler) http.Handler {
|
||||
return http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) {
|
||||
ctx := r.Context()
|
||||
// We need to call as SystemRestricted because this middleware is called from
|
||||
// organizations/{organization}/members/{user}/ paths, and we need to allow
|
||||
// org-admins to call these paths --- they might not have sitewide read permissions on users.
|
||||
// nolint:gocritic
|
||||
user, ok := extractUserContext(dbauthz.AsSystemRestricted(ctx), db, rw, r)
|
||||
user, ok := extractUserContext(ctx, db, rw, r)
|
||||
if !ok {
|
||||
// response already handled
|
||||
return
|
||||
|
|
|
@ -31,7 +31,6 @@ import (
|
|||
func (api *API) putMemberRoles(rw http.ResponseWriter, r *http.Request) {
|
||||
var (
|
||||
ctx = r.Context()
|
||||
user = httpmw.UserParam(r)
|
||||
organization = httpmw.OrganizationParam(r)
|
||||
member = httpmw.OrganizationMemberParam(r)
|
||||
apiKey = httpmw.APIKey(r)
|
||||
|
@ -51,7 +50,7 @@ func (api *API) putMemberRoles(rw http.ResponseWriter, r *http.Request) {
|
|||
|
||||
updatedUser, err := api.updateOrganizationMemberRoles(ctx, database.UpdateMemberRolesParams{
|
||||
GrantedRoles: params.Roles,
|
||||
UserID: user.ID,
|
||||
UserID: member.UserID,
|
||||
OrgID: organization.ID,
|
||||
})
|
||||
if err != nil {
|
||||
|
|
|
@ -326,7 +326,7 @@ func TestDeleteUser(t *testing.T) {
|
|||
err := client.DeleteUser(context.Background(), firstUser.UserID)
|
||||
var apiErr *codersdk.Error
|
||||
require.ErrorAs(t, err, &apiErr)
|
||||
require.Equal(t, http.StatusForbidden, apiErr.StatusCode())
|
||||
require.Equal(t, http.StatusBadRequest, apiErr.StatusCode())
|
||||
})
|
||||
t.Run("HasWorkspaces", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
@ -930,7 +930,7 @@ func TestGrantSiteRoles(t *testing.T) {
|
|||
AssignToUser: first.UserID.String(),
|
||||
Roles: []string{},
|
||||
Error: true,
|
||||
StatusCode: http.StatusForbidden,
|
||||
StatusCode: http.StatusBadRequest,
|
||||
},
|
||||
{
|
||||
// Cannot update your own roles
|
||||
|
|
|
@ -298,9 +298,9 @@ func (api *API) postWorkspacesByOrganization(rw http.ResponseWriter, r *http.Req
|
|||
organization = httpmw.OrganizationParam(r)
|
||||
apiKey = httpmw.APIKey(r)
|
||||
auditor = api.Auditor.Load()
|
||||
user = httpmw.UserParam(r)
|
||||
member = httpmw.OrganizationMemberParam(r)
|
||||
workspaceResourceInfo = audit.AdditionalFields{
|
||||
WorkspaceOwner: user.Username,
|
||||
WorkspaceOwner: member.Username,
|
||||
}
|
||||
)
|
||||
|
||||
|
@ -321,7 +321,7 @@ func (api *API) postWorkspacesByOrganization(rw http.ResponseWriter, r *http.Req
|
|||
|
||||
// Do this upfront to save work.
|
||||
if !api.Authorize(r, rbac.ActionCreate,
|
||||
rbac.ResourceWorkspace.InOrg(organization.ID).WithOwner(user.ID.String())) {
|
||||
rbac.ResourceWorkspace.InOrg(organization.ID).WithOwner(member.UserID.String())) {
|
||||
httpapi.ResourceNotFound(rw)
|
||||
return
|
||||
}
|
||||
|
@ -438,7 +438,7 @@ func (api *API) postWorkspacesByOrganization(rw http.ResponseWriter, r *http.Req
|
|||
// read other workspaces. Ideally we check the error on create and look for
|
||||
// a postgres conflict error.
|
||||
workspace, err := api.Database.GetWorkspaceByOwnerIDAndName(ctx, database.GetWorkspaceByOwnerIDAndNameParams{
|
||||
OwnerID: user.ID,
|
||||
OwnerID: member.UserID,
|
||||
Name: createWorkspace.Name,
|
||||
})
|
||||
if err == nil {
|
||||
|
@ -471,7 +471,7 @@ func (api *API) postWorkspacesByOrganization(rw http.ResponseWriter, r *http.Req
|
|||
ID: uuid.New(),
|
||||
CreatedAt: now,
|
||||
UpdatedAt: now,
|
||||
OwnerID: user.ID,
|
||||
OwnerID: member.UserID,
|
||||
OrganizationID: template.OrganizationID,
|
||||
TemplateID: template.ID,
|
||||
Name: createWorkspace.Name,
|
||||
|
@ -537,7 +537,7 @@ func (api *API) postWorkspacesByOrganization(rw http.ResponseWriter, r *http.Req
|
|||
ProvisionerJob: *provisionerJob,
|
||||
QueuePosition: 0,
|
||||
},
|
||||
user.Username,
|
||||
member.Username,
|
||||
[]database.WorkspaceResource{},
|
||||
[]database.WorkspaceResourceMetadatum{},
|
||||
[]database.WorkspaceAgent{},
|
||||
|
@ -558,7 +558,7 @@ func (api *API) postWorkspacesByOrganization(rw http.ResponseWriter, r *http.Req
|
|||
workspace,
|
||||
apiBuild,
|
||||
template,
|
||||
user.Username,
|
||||
member.Username,
|
||||
))
|
||||
}
|
||||
|
||||
|
|
Loading…
Reference in New Issue