coder · spikecurtis · Oct 11, 2023 · Oct 10, 2023
diff --git a/coderd/coderd.go b/coderd/coderd.go
@@ -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)

diff --git a/coderd/httpmw/organizationparam.go b/coderd/httpmw/organizationparam.go
@@ -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))
 		})
 	}

diff --git 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"
 )
@@ -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

diff --git a/coderd/members.go b/coderd/members.go
@@ -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 {

diff --git a/coderd/users_test.go b/coderd/users_test.go
@@ -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

diff --git a/coderd/workspaces.go b/coderd/workspaces.go
@@ -316,9 +316,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,
 		}
 	)
 
@@ -339,7 +339,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
 	}
@@ -456,7 +456,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 {
@@ -489,7 +489,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,
@@ -555,7 +555,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{},
@@ -576,7 +576,7 @@ func (api *API) postWorkspacesByOrganization(rw http.ResponseWriter, r *http.Req
 		workspace,
 		apiBuild,
 		template,
-		user.Username,
+		member.Username,
 	))
 }