From fc4a6f2fd4196c6c9684bd7a4e6a24deb9308db5 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 7 May 2025 13:42:43 -0500 Subject: [PATCH 1/6] chore: fetching workspaces by username requires site `user.read` perms --- enterprise/coderd/workspaces_test.go | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/enterprise/coderd/workspaces_test.go b/enterprise/coderd/workspaces_test.go index 72859c5460fa7..85b414960f85c 100644 --- a/enterprise/coderd/workspaces_test.go +++ b/enterprise/coderd/workspaces_test.go @@ -289,11 +289,17 @@ func TestCreateUserWorkspace(t *testing.T) { ctx = testutil.Context(t, testutil.WaitLong*1000) // Reset the context to avoid timeouts. - _, err = creator.CreateUserWorkspace(ctx, adminID.ID.String(), codersdk.CreateWorkspaceRequest{ + wrk, err := creator.CreateUserWorkspace(ctx, adminID.ID.String(), codersdk.CreateWorkspaceRequest{ TemplateID: template.ID, Name: "workspace", }) require.NoError(t, err) + coderdtest.AwaitWorkspaceBuildJobCompleted(t, admin, wrk.LatestBuild.ID) + + _, err = creator.WorkspaceByOwnerAndName(ctx, adminID.Username, wrk.Name, codersdk.WorkspaceOptions{ + IncludeDeleted: false, + }) + require.NoError(t, err) }) // Asserting some authz calls when creating a workspace. From abb460caa56546367e11c71721b1e5d21df6f2c6 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 7 May 2025 14:53:30 -0500 Subject: [PATCH 2/6] chore: do not require site wide permission to use workspace apis Workspace apis scoped to an org should not require site wide perms. Site wide perms should still work if the owner is not in an organzation --- coderd/coderd.go | 23 ++-- coderd/httpmw/organizationparam.go | 146 ++++++++++++++++++++---- coderd/httpmw/organizationparam_test.go | 17 ++- coderd/httpmw/userparam.go | 55 +++++++++ coderd/workspacebuilds.go | 4 +- coderd/workspaces.go | 63 ++++------ 6 files changed, 231 insertions(+), 77 deletions(-) diff --git a/coderd/coderd.go b/coderd/coderd.go index 123e58feb642a..44e81a12b91ad 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -1076,7 +1076,7 @@ func New(options *Options) *API { r.Group(func(r chi.Router) { r.Use( - httpmw.ExtractOrganizationMemberParam(options.Database), + httpmw.ExtractOrganizationMemberParam(options.Database, api.HTTPAuth.Authorize), ) r.Delete("/", api.deleteOrganizationMember) r.Put("/roles", api.putMemberRoles) @@ -1189,15 +1189,25 @@ func New(options *Options) *API { }) r.Route("/{user}", func(r chi.Router) { r.Group(func(r chi.Router) { - r.Use(httpmw.ExtractUserParamOptional(options.Database)) + r.Use(httpmw.ExtractOrganizationMembersParam(options.Database, api.HTTPAuth.Authorize)) // Creating workspaces does not require permissions on the user, only the // organization member. This endpoint should match the authz story of // postWorkspacesByOrganization r.Post("/workspaces", api.postUserWorkspaces) + r.Route("/workspace/{workspacename}", func(r chi.Router) { + r.Get("/", api.workspaceByOwnerAndName) + r.Get("/builds/{buildnumber}", api.workspaceBuildByBuildNumber) + }) + }) + + r.Group(func(r chi.Router) { + r.Use(httpmw.ExtractUserParam(options.Database)) // Similarly to creating a workspace, evaluating parameters for a // new workspace should also match the authz story of // postWorkspacesByOrganization + // TODO: Do not require site wide read user permission. Make this work + // with org member permissions. r.Route("/templateversions/{templateversion}", func(r chi.Router) { r.Use( httpmw.ExtractTemplateVersionParam(options.Database), @@ -1205,10 +1215,6 @@ func New(options *Options) *API { ) r.Get("/parameters", api.templateVersionDynamicParameters) }) - }) - - r.Group(func(r chi.Router) { - r.Use(httpmw.ExtractUserParam(options.Database)) r.Post("/convert-login", api.postConvertLoginType) r.Delete("/", api.deleteUser) @@ -1250,10 +1256,7 @@ func New(options *Options) *API { r.Get("/", api.organizationsByUser) r.Get("/{organizationname}", api.organizationByUserAndName) }) - r.Route("/workspace/{workspacename}", func(r chi.Router) { - r.Get("/", api.workspaceByOwnerAndName) - r.Get("/builds/{buildnumber}", api.workspaceBuildByBuildNumber) - }) + r.Get("/gitsshkey", api.gitSSHKey) r.Put("/gitsshkey", api.regenerateGitSSHKey) r.Route("/notifications", func(r chi.Router) { diff --git a/coderd/httpmw/organizationparam.go b/coderd/httpmw/organizationparam.go index 782a0d37e1985..a94c92dcb3efd 100644 --- a/coderd/httpmw/organizationparam.go +++ b/coderd/httpmw/organizationparam.go @@ -11,12 +11,15 @@ import ( "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/coderd/rbac" + "github.com/coder/coder/v2/coderd/rbac/policy" "github.com/coder/coder/v2/codersdk" ) type ( - organizationParamContextKey struct{} - organizationMemberParamContextKey struct{} + organizationParamContextKey struct{} + organizationMemberParamContextKey struct{} + organizationMembersParamContextKey struct{} ) // OrganizationParam returns the organization from the ExtractOrganizationParam handler. @@ -38,6 +41,14 @@ func OrganizationMemberParam(r *http.Request) OrganizationMember { return organizationMember } +func OrganizationMembersParam(r *http.Request) OrganizationMembers { + organizationMembers, ok := r.Context().Value(organizationMembersParamContextKey{}).(OrganizationMembers) + if !ok { + panic("developer error: organization members param middleware not provided") + } + return organizationMembers +} + // ExtractOrganizationParam grabs an organization from the "organization" URL parameter. // This middleware requires the API key middleware higher in the call stack for authentication. func ExtractOrganizationParam(db database.Store) func(http.Handler) http.Handler { @@ -107,39 +118,27 @@ type OrganizationMember struct { // 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 { +func ExtractOrganizationMemberParam(db database.Store, auth func(r *http.Request, action policy.Action, object rbac.Objecter) bool) 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) - - organizationMember, err := database.ExpectOne(db.OrganizationMembers(ctx, database.OrganizationMembersParams{ - OrganizationID: organization.ID, - UserID: user.ID, - IncludeSystem: false, - })) - if httpapi.Is404Error(err) { - httpapi.ResourceNotFound(rw) + _, members, done := ExtractOrganizationMember(ctx, auth, rw, r, db, organization.ID) + if done { return } - if err != nil { + + if len(members) != 1 { httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ Message: "Internal error fetching organization member.", - Detail: err.Error(), + // This is a developer error and should never happen. + Detail: fmt.Sprintf("Expected exactly one organization member, but got %d.", len(members)), }) return } + organizationMember := members[0] + ctx = context.WithValue(ctx, organizationMemberParamContextKey{}, OrganizationMember{ OrganizationMember: organizationMember.OrganizationMember, // Here we're making two exceptions to the rule about not leaking data about the user @@ -151,8 +150,105 @@ func ExtractOrganizationMemberParam(db database.Store) func(http.Handler) http.H // API handlers need this information for audit logging and returning the owner's // username in response to creating a workspace. Additionally, the frontend consumes // the Avatar URL and this allows the FE to avoid an extra request. - Username: user.Username, - AvatarURL: user.AvatarURL, + Username: organizationMember.Username, + AvatarURL: organizationMember.AvatarURL, + }) + + next.ServeHTTP(rw, r.WithContext(ctx)) + }) + } +} + +// ExtractOrganizationMember extracts all user memberships from the "user" URL +// parameter. If orgID is uuid.Nil, then it will return all memberships for the +// user, otherwise it will only return memberships to the org. +// +// If `user` is returned, that means the caller can use the data. This is returned because +// it is possible to have a user with 0 organizations. So the user != nil, with 0 memberships. +func ExtractOrganizationMember(ctx context.Context, auth func(r *http.Request, action policy.Action, object rbac.Objecter) bool, rw http.ResponseWriter, r *http.Request, db database.Store, orgID uuid.UUID) (*database.User, []database.OrganizationMembersRow, bool) { + // 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 nil, nil, true + } + + organizationMembers, err := db.OrganizationMembers(ctx, database.OrganizationMembersParams{ + OrganizationID: orgID, + UserID: user.ID, + IncludeSystem: false, + }) + if httpapi.Is404Error(err) { + httpapi.ResourceNotFound(rw) + return nil, nil, true + } + if err != nil { + httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ + Message: "Internal error fetching organization member.", + Detail: err.Error(), + }) + return nil, nil, true + } + + if auth(r, policy.ActionRead, user) { + return &user, organizationMembers, true + } + + // If the user cannot be read and 0 memberships exist, throw a 404 to not + // leak the user existance. + if len(organizationMembers) == 0 { + httpapi.ResourceNotFound(rw) + return nil, nil, true + } + + return nil, organizationMembers, false +} + +type OrganizationMembers struct { + User *database.User + Memberships []OrganizationMember +} + +func (om OrganizationMembers) UserID() uuid.UUID { + if om.User != nil { + return om.User.ID + } + + if len(om.Memberships) > 0 { + return om.Memberships[0].UserID + } + return uuid.Nil +} + +// ExtractOrganizationMembersParam grabs all user organization memberships. +// Only requires the "user" URL parameter. +func ExtractOrganizationMembersParam(db database.Store, auth func(r *http.Request, action policy.Action, object rbac.Objecter) bool) 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() + + // Fetch all memberships + user, members, done := ExtractOrganizationMember(ctx, auth, rw, r, db, uuid.Nil) + if done { + return + } + + orgMembers := make([]OrganizationMember, 0, len(members)) + for _, organizationMember := range members { + orgMembers = append(orgMembers, OrganizationMember{ + OrganizationMember: organizationMember.OrganizationMember, + Username: organizationMember.Username, + AvatarURL: organizationMember.AvatarURL, + }) + } + + ctx = context.WithValue(ctx, organizationMembersParamContextKey{}, OrganizationMembers{ + User: user, + Memberships: orgMembers, }) next.ServeHTTP(rw, r.WithContext(ctx)) }) diff --git a/coderd/httpmw/organizationparam_test.go b/coderd/httpmw/organizationparam_test.go index ca3adcabbae01..8d0c6b7f9ab9b 100644 --- a/coderd/httpmw/organizationparam_test.go +++ b/coderd/httpmw/organizationparam_test.go @@ -16,6 +16,8 @@ import ( "github.com/coder/coder/v2/coderd/database/dbmem" "github.com/coder/coder/v2/coderd/database/dbtime" "github.com/coder/coder/v2/coderd/httpmw" + "github.com/coder/coder/v2/coderd/rbac" + "github.com/coder/coder/v2/coderd/rbac/policy" "github.com/coder/coder/v2/codersdk" "github.com/coder/coder/v2/testutil" ) @@ -129,7 +131,9 @@ func TestOrganizationParam(t *testing.T) { }), httpmw.ExtractUserParam(db), httpmw.ExtractOrganizationParam(db), - httpmw.ExtractOrganizationMemberParam(db), + httpmw.ExtractOrganizationMemberParam(db, func(r *http.Request, _ policy.Action, _ rbac.Objecter) bool { + return true + }), ) rtr.Get("/", nil) rtr.ServeHTTP(rw, r) @@ -166,7 +170,12 @@ func TestOrganizationParam(t *testing.T) { }), httpmw.ExtractOrganizationParam(db), httpmw.ExtractUserParam(db), - httpmw.ExtractOrganizationMemberParam(db), + httpmw.ExtractOrganizationMemberParam(db, func(r *http.Request, _ policy.Action, _ rbac.Objecter) bool { + return true + }), + httpmw.ExtractOrganizationMembersParam(db, func(r *http.Request, _ policy.Action, _ rbac.Objecter) bool { + return true + }), ) rtr.Get("/", func(rw http.ResponseWriter, r *http.Request) { org := httpmw.OrganizationParam(r) @@ -190,6 +199,10 @@ func TestOrganizationParam(t *testing.T) { assert.NotEmpty(t, orgMem.OrganizationMember.UpdatedAt) assert.NotEmpty(t, orgMem.OrganizationMember.UserID) assert.NotEmpty(t, orgMem.OrganizationMember.Roles) + + orgMems := httpmw.OrganizationMembersParam(r) + assert.NotZero(t, orgMems) + assert.Equal(t, orgMem.UserID, orgMems[0].UserID) }) // Try by ID diff --git a/coderd/httpmw/userparam.go b/coderd/httpmw/userparam.go index 2fbcc458489f9..4f97653747d67 100644 --- a/coderd/httpmw/userparam.go +++ b/coderd/httpmw/userparam.go @@ -9,6 +9,7 @@ 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" ) @@ -128,3 +129,57 @@ func ExtractUserContext(ctx context.Context, db database.Store, rw http.Response } return user, true } + +// ExtractUserID will work if the requester can access any OrganizationMember that +// belongs to the user. +func ExtractUserID(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) + + organizationMember, err := database.ExpectOne(db.OrganizationMembers(ctx, database.OrganizationMembersParams{ + OrganizationID: organization.ID, + UserID: user.ID, + IncludeSystem: false, + })) + if httpapi.Is404Error(err) { + httpapi.ResourceNotFound(rw) + return + } + if err != nil { + httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ + Message: "Internal error fetching organization member.", + Detail: err.Error(), + }) + return + } + + ctx = context.WithValue(ctx, organizationMemberParamContextKey{}, OrganizationMember{ + OrganizationMember: organizationMember.OrganizationMember, + // Here we're making two exceptions to the rule about not leaking data about the user + // to the API handler, which is to include the username and avatar URL. + // 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 and avatar. + // This is OK! + // + // API handlers need this information for audit logging and returning the owner's + // username in response to creating a workspace. Additionally, the frontend consumes + // the Avatar URL and this allows the FE to avoid an extra request. + Username: user.Username, + AvatarURL: user.AvatarURL, + }) + next.ServeHTTP(rw, r.WithContext(ctx)) + }) + } +} diff --git a/coderd/workspacebuilds.go b/coderd/workspacebuilds.go index 94f1822df797c..719d4e2a48123 100644 --- a/coderd/workspacebuilds.go +++ b/coderd/workspacebuilds.go @@ -232,7 +232,7 @@ func (api *API) workspaceBuilds(rw http.ResponseWriter, r *http.Request) { // @Router /users/{user}/workspace/{workspacename}/builds/{buildnumber} [get] func (api *API) workspaceBuildByBuildNumber(rw http.ResponseWriter, r *http.Request) { ctx := r.Context() - owner := httpmw.UserParam(r) + mems := httpmw.OrganizationMembersParam(r) workspaceName := chi.URLParam(r, "workspacename") buildNumber, err := strconv.ParseInt(chi.URLParam(r, "buildnumber"), 10, 32) if err != nil { @@ -244,7 +244,7 @@ func (api *API) workspaceBuildByBuildNumber(rw http.ResponseWriter, r *http.Requ } workspace, err := api.Database.GetWorkspaceByOwnerIDAndName(ctx, database.GetWorkspaceByOwnerIDAndNameParams{ - OwnerID: owner.ID, + OwnerID: mems.UserID(), Name: workspaceName, }) if httpapi.Is404Error(err) { diff --git a/coderd/workspaces.go b/coderd/workspaces.go index 2ac432d905ae6..bf8cc6dd05571 100644 --- a/coderd/workspaces.go +++ b/coderd/workspaces.go @@ -253,7 +253,8 @@ func (api *API) workspaces(rw http.ResponseWriter, r *http.Request) { // @Router /users/{user}/workspace/{workspacename} [get] func (api *API) workspaceByOwnerAndName(rw http.ResponseWriter, r *http.Request) { ctx := r.Context() - owner := httpmw.UserParam(r) + + mems := httpmw.OrganizationMembersParam(r) workspaceName := chi.URLParam(r, "workspacename") apiKey := httpmw.APIKey(r) @@ -273,12 +274,12 @@ func (api *API) workspaceByOwnerAndName(rw http.ResponseWriter, r *http.Request) } workspace, err := api.Database.GetWorkspaceByOwnerIDAndName(ctx, database.GetWorkspaceByOwnerIDAndNameParams{ - OwnerID: owner.ID, + OwnerID: mems.UserID(), Name: workspaceName, }) if includeDeleted && errors.Is(err, sql.ErrNoRows) { workspace, err = api.Database.GetWorkspaceByOwnerIDAndName(ctx, database.GetWorkspaceByOwnerIDAndNameParams{ - OwnerID: owner.ID, + OwnerID: mems.UserID(), Name: workspaceName, Deleted: includeDeleted, }) @@ -408,6 +409,7 @@ func (api *API) postUserWorkspaces(rw http.ResponseWriter, r *http.Request) { ctx = r.Context() apiKey = httpmw.APIKey(r) auditor = api.Auditor.Load() + mems = httpmw.OrganizationMembersParam(r) ) var req codersdk.CreateWorkspaceRequest @@ -416,17 +418,16 @@ func (api *API) postUserWorkspaces(rw http.ResponseWriter, r *http.Request) { } var owner workspaceOwner - // This user fetch is an optimization path for the most common case of creating a - // workspace for 'Me'. - // - // This is also required to allow `owners` to create workspaces for users - // that are not in an organization. - user, ok := httpmw.UserParamOptional(r) - if ok { + if mems.User != nil { + // This user fetch is an optimization path for the most common case of creating a + // workspace for 'Me'. + // + // This is also required to allow `owners` to create workspaces for users + // that are not in an organization. owner = workspaceOwner{ - ID: user.ID, - Username: user.Username, - AvatarURL: user.AvatarURL, + ID: mems.User.ID, + Username: mems.User.Username, + AvatarURL: mems.User.AvatarURL, } } else { // A workspace can still be created if the caller can read the organization @@ -443,35 +444,21 @@ func (api *API) postUserWorkspaces(rw http.ResponseWriter, r *http.Request) { return } - // We need to fetch the original user as a system user to fetch the - // user_id. 'ExtractUserContext' handles all cases like usernames, - // 'Me', etc. - // nolint:gocritic // The user_id needs to be fetched. This handles all those cases. - user, ok := httpmw.ExtractUserContext(dbauthz.AsSystemRestricted(ctx), api.Database, rw, r) - if !ok { - return - } - - organizationMember, err := database.ExpectOne(api.Database.OrganizationMembers(ctx, database.OrganizationMembersParams{ - OrganizationID: template.OrganizationID, - UserID: user.ID, - IncludeSystem: false, - })) - if httpapi.Is404Error(err) { + // If the caller can find the organization membership in the same org + // as the template, then they can continue. + orgIndex := slices.IndexFunc(mems.Memberships, func(mem httpmw.OrganizationMember) bool { + return mem.OrganizationID == template.ID + }) + if orgIndex == -1 { httpapi.ResourceNotFound(rw) return } - if err != nil { - httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ - Message: "Internal error fetching organization member.", - Detail: err.Error(), - }) - return - } + + member := mems.Memberships[orgIndex] owner = workspaceOwner{ - ID: organizationMember.OrganizationMember.UserID, - Username: organizationMember.Username, - AvatarURL: organizationMember.AvatarURL, + ID: member.UserID, + Username: member.Username, + AvatarURL: member.AvatarURL, } } From e0b8af3903e718e17dc3643603f6f8f936513d77 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 7 May 2025 15:01:51 -0500 Subject: [PATCH 3/6] fixup comments --- coderd/coderd.go | 2 +- coderd/httpmw/organizationparam.go | 17 ++++++++++++----- coderd/httpmw/organizationparam_test.go | 14 ++++++-------- 3 files changed, 19 insertions(+), 14 deletions(-) diff --git a/coderd/coderd.go b/coderd/coderd.go index 44e81a12b91ad..d0d3471cdd771 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -1076,7 +1076,7 @@ func New(options *Options) *API { r.Group(func(r chi.Router) { r.Use( - httpmw.ExtractOrganizationMemberParam(options.Database, api.HTTPAuth.Authorize), + httpmw.ExtractOrganizationMemberParam(options.Database), ) r.Delete("/", api.deleteOrganizationMember) r.Put("/roles", api.putMemberRoles) diff --git a/coderd/httpmw/organizationparam.go b/coderd/httpmw/organizationparam.go index a94c92dcb3efd..670f2c0085a15 100644 --- a/coderd/httpmw/organizationparam.go +++ b/coderd/httpmw/organizationparam.go @@ -118,12 +118,12 @@ type OrganizationMember struct { // 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, auth func(r *http.Request, action policy.Action, object rbac.Objecter) bool) func(http.Handler) http.Handler { +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() organization := OrganizationParam(r) - _, members, done := ExtractOrganizationMember(ctx, auth, rw, r, db, organization.ID) + _, members, done := ExtractOrganizationMember(ctx, nil, rw, r, db, organization.ID) if done { return } @@ -194,12 +194,12 @@ func ExtractOrganizationMember(ctx context.Context, auth func(r *http.Request, a return nil, nil, true } - if auth(r, policy.ActionRead, user) { + if auth != nil && auth(r, policy.ActionRead, user) { return &user, organizationMembers, true } // If the user cannot be read and 0 memberships exist, throw a 404 to not - // leak the user existance. + // leak the user existence. if len(organizationMembers) == 0 { httpapi.ResourceNotFound(rw) return nil, nil, true @@ -209,7 +209,11 @@ func ExtractOrganizationMember(ctx context.Context, auth func(r *http.Request, a } type OrganizationMembers struct { - User *database.User + // User is `nil` if the caller is not allowed access to the site wide + // user object. + User *database.User + // Memberships can only be length 0 if `user != nil`. If `user == nil`, then + // memberships will be at least length 1. Memberships []OrganizationMember } @@ -226,6 +230,9 @@ func (om OrganizationMembers) UserID() uuid.UUID { // ExtractOrganizationMembersParam grabs all user organization memberships. // Only requires the "user" URL parameter. +// +// Use this if you want to grab as much information for a user as you can. +// From an organization context, site wide user information might not available. func ExtractOrganizationMembersParam(db database.Store, auth func(r *http.Request, action policy.Action, object rbac.Objecter) bool) func(http.Handler) http.Handler { return func(next http.Handler) http.Handler { return http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { diff --git a/coderd/httpmw/organizationparam_test.go b/coderd/httpmw/organizationparam_test.go index 8d0c6b7f9ab9b..68cc314abd26f 100644 --- a/coderd/httpmw/organizationparam_test.go +++ b/coderd/httpmw/organizationparam_test.go @@ -131,9 +131,7 @@ func TestOrganizationParam(t *testing.T) { }), httpmw.ExtractUserParam(db), httpmw.ExtractOrganizationParam(db), - httpmw.ExtractOrganizationMemberParam(db, func(r *http.Request, _ policy.Action, _ rbac.Objecter) bool { - return true - }), + httpmw.ExtractOrganizationMemberParam(db), ) rtr.Get("/", nil) rtr.ServeHTTP(rw, r) @@ -170,11 +168,10 @@ func TestOrganizationParam(t *testing.T) { }), httpmw.ExtractOrganizationParam(db), httpmw.ExtractUserParam(db), - httpmw.ExtractOrganizationMemberParam(db, func(r *http.Request, _ policy.Action, _ rbac.Objecter) bool { - return true - }), + httpmw.ExtractOrganizationMemberParam(db), httpmw.ExtractOrganizationMembersParam(db, func(r *http.Request, _ policy.Action, _ rbac.Objecter) bool { - return true + // Assume the caller cannot read the member + return false }), ) rtr.Get("/", func(rw http.ResponseWriter, r *http.Request) { @@ -202,7 +199,8 @@ func TestOrganizationParam(t *testing.T) { orgMems := httpmw.OrganizationMembersParam(r) assert.NotZero(t, orgMems) - assert.Equal(t, orgMem.UserID, orgMems[0].UserID) + assert.Equal(t, orgMem.UserID, orgMems.Memberships[0].UserID) + assert.Nil(t, orgMems.User, "user data should not be available, hard coded false authorize") }) // Try by ID From 18f4015cea83243af4cc59d7977e1dd5af3fafec Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 7 May 2025 15:14:08 -0500 Subject: [PATCH 4/6] fix boolean return --- 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 670f2c0085a15..93e9d99f306d5 100644 --- a/coderd/httpmw/organizationparam.go +++ b/coderd/httpmw/organizationparam.go @@ -195,7 +195,7 @@ func ExtractOrganizationMember(ctx context.Context, auth func(r *http.Request, a } if auth != nil && auth(r, policy.ActionRead, user) { - return &user, organizationMembers, true + return &user, organizationMembers, false } // If the user cannot be read and 0 memberships exist, throw a 404 to not From ce151e6def10068622b86c64940378c5264bdfae Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 8 May 2025 11:29:14 -0500 Subject: [PATCH 5/6] use org id, not template id --- coderd/workspaces.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/coderd/workspaces.go b/coderd/workspaces.go index bf8cc6dd05571..6b187e241e80f 100644 --- a/coderd/workspaces.go +++ b/coderd/workspaces.go @@ -447,7 +447,7 @@ func (api *API) postUserWorkspaces(rw http.ResponseWriter, r *http.Request) { // If the caller can find the organization membership in the same org // as the template, then they can continue. orgIndex := slices.IndexFunc(mems.Memberships, func(mem httpmw.OrganizationMember) bool { - return mem.OrganizationID == template.ID + return mem.OrganizationID == template.OrganizationID }) if orgIndex == -1 { httpapi.ResourceNotFound(rw) From 4815eae48972e53d3850c42f9033e7e6cf92d3d4 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 8 May 2025 11:51:40 -0500 Subject: [PATCH 6/6] remove dead code --- coderd/httpmw/organizationparam.go | 1 + coderd/httpmw/userparam.go | 55 ------------------------------ 2 files changed, 1 insertion(+), 55 deletions(-) diff --git a/coderd/httpmw/organizationparam.go b/coderd/httpmw/organizationparam.go index 93e9d99f306d5..efedc3a764591 100644 --- a/coderd/httpmw/organizationparam.go +++ b/coderd/httpmw/organizationparam.go @@ -194,6 +194,7 @@ func ExtractOrganizationMember(ctx context.Context, auth func(r *http.Request, a return nil, nil, true } + // Only return the user data if the caller can read the user object. if auth != nil && auth(r, policy.ActionRead, user) { return &user, organizationMembers, false } diff --git a/coderd/httpmw/userparam.go b/coderd/httpmw/userparam.go index 4f97653747d67..2fbcc458489f9 100644 --- a/coderd/httpmw/userparam.go +++ b/coderd/httpmw/userparam.go @@ -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" ) @@ -129,57 +128,3 @@ func ExtractUserContext(ctx context.Context, db database.Store, rw http.Response } return user, true } - -// ExtractUserID will work if the requester can access any OrganizationMember that -// belongs to the user. -func ExtractUserID(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) - - organizationMember, err := database.ExpectOne(db.OrganizationMembers(ctx, database.OrganizationMembersParams{ - OrganizationID: organization.ID, - UserID: user.ID, - IncludeSystem: false, - })) - if httpapi.Is404Error(err) { - httpapi.ResourceNotFound(rw) - return - } - if err != nil { - httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ - Message: "Internal error fetching organization member.", - Detail: err.Error(), - }) - return - } - - ctx = context.WithValue(ctx, organizationMemberParamContextKey{}, OrganizationMember{ - OrganizationMember: organizationMember.OrganizationMember, - // Here we're making two exceptions to the rule about not leaking data about the user - // to the API handler, which is to include the username and avatar URL. - // 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 and avatar. - // This is OK! - // - // API handlers need this information for audit logging and returning the owner's - // username in response to creating a workspace. Additionally, the frontend consumes - // the Avatar URL and this allows the FE to avoid an extra request. - Username: user.Username, - AvatarURL: user.AvatarURL, - }) - next.ServeHTTP(rw, r.WithContext(ctx)) - }) - } -}