From 44aad4b4273a71c9cd1087482bf0dd1f9e14470d Mon Sep 17 00:00:00 2001 From: Spike Curtis Date: Tue, 10 Oct 2023 13:36:49 +0400 Subject: [PATCH 1/2] chore: drop unused redirectToLoginOnMe parameter --- coderd/coderd.go | 4 ++-- coderd/httpmw/organizationparam_test.go | 4 ++-- coderd/httpmw/userparam.go | 15 +++------------ coderd/httpmw/userparam_test.go | 6 +++--- coderd/httpmw/workspaceparam_test.go | 2 +- enterprise/coderd/coderd.go | 4 ++-- 6 files changed, 13 insertions(+), 22 deletions(-) diff --git a/coderd/coderd.go b/coderd/coderd.go index cabf63c34bd98..6cb43b71dd7a7 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -652,7 +652,7 @@ func New(options *Options) *API { r.Get("/roles", api.assignableOrgRoles) r.Route("/{user}", func(r chi.Router) { r.Use( - httpmw.ExtractUserParam(options.Database, false), + httpmw.ExtractUserParam(options.Database), httpmw.ExtractOrganizationMemberParam(options.Database), ) r.Put("/roles", api.putMemberRoles) @@ -741,7 +741,7 @@ func New(options *Options) *API { r.Get("/", api.assignableSiteRoles) }) r.Route("/{user}", func(r chi.Router) { - r.Use(httpmw.ExtractUserParam(options.Database, false)) + r.Use(httpmw.ExtractUserParam(options.Database)) r.Post("/convert-login", api.postConvertLoginType) r.Delete("/", api.deleteUser) r.Get("/", api.userByName) diff --git a/coderd/httpmw/organizationparam_test.go b/coderd/httpmw/organizationparam_test.go index f375b13bb0a85..0457168132e9a 100644 --- a/coderd/httpmw/organizationparam_test.go +++ b/coderd/httpmw/organizationparam_test.go @@ -125,7 +125,7 @@ func TestOrganizationParam(t *testing.T) { DB: db, RedirectToLogin: false, }), - httpmw.ExtractUserParam(db, false), + httpmw.ExtractUserParam(db), httpmw.ExtractOrganizationParam(db), httpmw.ExtractOrganizationMemberParam(db), ) @@ -157,7 +157,7 @@ func TestOrganizationParam(t *testing.T) { RedirectToLogin: false, }), httpmw.ExtractOrganizationParam(db), - httpmw.ExtractUserParam(db, false), + httpmw.ExtractUserParam(db), httpmw.ExtractOrganizationMemberParam(db), ) rtr.Get("/", func(rw http.ResponseWriter, r *http.Request) { diff --git a/coderd/httpmw/userparam.go b/coderd/httpmw/userparam.go index e58f3c7bc512f..8a8310672cb93 100644 --- a/coderd/httpmw/userparam.go +++ b/coderd/httpmw/userparam.go @@ -34,9 +34,7 @@ func UserParam(r *http.Request) database.User { // ExtractUserParam extracts a user from an ID/username in the {user} URL // parameter. -// -//nolint:revive -func ExtractUserParam(db database.Store, redirectToLoginOnMe bool) func(http.Handler) http.Handler { +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() @@ -44,7 +42,7 @@ func ExtractUserParam(db database.Store, redirectToLoginOnMe bool) func(http.Han // 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, redirectToLoginOnMe) + user, ok := extractUserContext(dbauthz.AsSystemRestricted(ctx), db, rw, r) if !ok { // response already handled return @@ -56,9 +54,7 @@ func ExtractUserParam(db database.Store, redirectToLoginOnMe bool) func(http.Han } // extractUserContext queries the database for the parameterized `{user}` from the request URL. -// -//nolint:revive -func extractUserContext(ctx context.Context, db database.Store, rw http.ResponseWriter, r *http.Request, redirectToLoginOnMe bool) (user database.User, ok bool) { +func extractUserContext(ctx context.Context, db database.Store, rw http.ResponseWriter, r *http.Request) (user database.User, ok bool) { // userQuery is either a uuid, a username, or 'me' userQuery := chi.URLParam(r, "user") if userQuery == "" { @@ -71,11 +67,6 @@ func extractUserContext(ctx context.Context, db database.Store, rw http.Response if userQuery == "me" { apiKey, ok := APIKeyOptional(r) if !ok { - if redirectToLoginOnMe { - RedirectToLogin(rw, r, nil, SignedOutErrorMessage) - return database.User{}, false - } - httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ Message: "Cannot use \"me\" without a valid session.", }) diff --git a/coderd/httpmw/userparam_test.go b/coderd/httpmw/userparam_test.go index bd1b5b2b277c7..040948ff60cf3 100644 --- a/coderd/httpmw/userparam_test.go +++ b/coderd/httpmw/userparam_test.go @@ -44,7 +44,7 @@ func TestUserParam(t *testing.T) { r = returnedRequest })).ServeHTTP(rw, r) - httpmw.ExtractUserParam(db, false)(http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { + httpmw.ExtractUserParam(db)(http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { rw.WriteHeader(http.StatusOK) })).ServeHTTP(rw, r) res := rw.Result() @@ -66,7 +66,7 @@ func TestUserParam(t *testing.T) { routeContext := chi.NewRouteContext() routeContext.URLParams.Add("user", "ben") r = r.WithContext(context.WithValue(r.Context(), chi.RouteCtxKey, routeContext)) - httpmw.ExtractUserParam(db, false)(http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { + httpmw.ExtractUserParam(db)(http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { rw.WriteHeader(http.StatusOK) })).ServeHTTP(rw, r) res := rw.Result() @@ -88,7 +88,7 @@ func TestUserParam(t *testing.T) { routeContext := chi.NewRouteContext() routeContext.URLParams.Add("user", "me") r = r.WithContext(context.WithValue(r.Context(), chi.RouteCtxKey, routeContext)) - httpmw.ExtractUserParam(db, false)(http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { + httpmw.ExtractUserParam(db)(http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { _ = httpmw.UserParam(r) rw.WriteHeader(http.StatusOK) })).ServeHTTP(rw, r) diff --git a/coderd/httpmw/workspaceparam_test.go b/coderd/httpmw/workspaceparam_test.go index 7e3be16b89a34..d65fb53f8f28d 100644 --- a/coderd/httpmw/workspaceparam_test.go +++ b/coderd/httpmw/workspaceparam_test.go @@ -315,7 +315,7 @@ func TestWorkspaceAgentByNameParam(t *testing.T) { DB: db, RedirectToLogin: true, }), - httpmw.ExtractUserParam(db, false), + httpmw.ExtractUserParam(db), httpmw.ExtractWorkspaceAndAgentParam(db), ) rtr.Get("/", func(w http.ResponseWriter, r *http.Request) { diff --git a/enterprise/coderd/coderd.go b/enterprise/coderd/coderd.go index eea08a488f567..7d0017a0af98f 100644 --- a/enterprise/coderd/coderd.go +++ b/enterprise/coderd/coderd.go @@ -269,7 +269,7 @@ func New(ctx context.Context, options *Options) (_ *API, err error) { apiKeyMiddleware, ) r.Route("/{user}", func(r chi.Router) { - r.Use(httpmw.ExtractUserParam(options.Database, false)) + r.Use(httpmw.ExtractUserParam(options.Database)) r.Get("/", api.workspaceQuota) }) }) @@ -296,7 +296,7 @@ func New(ctx context.Context, options *Options) (_ *API, err error) { r.Use( api.autostopRequirementEnabledMW, apiKeyMiddleware, - httpmw.ExtractUserParam(options.Database, false), + httpmw.ExtractUserParam(options.Database), ) r.Get("/", api.userQuietHoursSchedule) From 048f3106dee0499439d6e7a5d801634464094571 Mon Sep 17 00:00:00 2001 From: Spike Curtis Date: Tue, 10 Oct 2023 15:53:55 +0400 Subject: [PATCH 2/2] chore: refactor workspace conversion to accept ownerName --- coderd/users.go | 10 +++--- coderd/workspaceagents.go | 12 ++++---- coderd/workspacebuilds.go | 51 ++++++++++++++++++++---------- coderd/workspaces.go | 65 ++++++++++++++++++++++++++------------- 4 files changed, 90 insertions(+), 48 deletions(-) diff --git a/coderd/users.go b/coderd/users.go index 7633290a80edf..9fa71c05633ea 100644 --- a/coderd/users.go +++ b/coderd/users.go @@ -1177,13 +1177,13 @@ func userOrganizationIDs(ctx context.Context, api *API, user database.User) ([]u return member.OrganizationIDs, nil } -func findUser(id uuid.UUID, users []database.User) *database.User { - for _, u := range users { - if u.ID == id { - return &u +func usernameWithID(id uuid.UUID, users []database.User) (string, bool) { + for _, user := range users { + if id == user.ID { + return user.Username, true } } - return nil + return "", false } func convertAPIKey(k database.APIKey) codersdk.APIKey { diff --git a/coderd/workspaceagents.go b/coderd/workspaceagents.go index ce5d0de0f3886..44f047bb3ac48 100644 --- a/coderd/workspaceagents.go +++ b/coderd/workspaceagents.go @@ -121,7 +121,7 @@ func (api *API) workspaceAgent(rw http.ResponseWriter, r *http.Request) { } apiAgent, err := convertWorkspaceAgent( - api.DERPMap(), *api.TailnetCoordinator.Load(), workspaceAgent, convertApps(dbApps, workspaceAgent, owner, workspace), convertScripts(scripts), convertLogSources(logSources), api.AgentInactiveDisconnectTimeout, + api.DERPMap(), *api.TailnetCoordinator.Load(), workspaceAgent, convertApps(dbApps, workspaceAgent, owner.Username, workspace), convertScripts(scripts), convertLogSources(logSources), api.AgentInactiveDisconnectTimeout, api.DeploymentValues.AgentFallbackTroubleshootingURL.String(), ) if err != nil { @@ -235,7 +235,7 @@ func (api *API) workspaceAgentManifest(rw http.ResponseWriter, r *http.Request) httpapi.Write(ctx, rw, http.StatusOK, agentsdk.Manifest{ AgentID: apiAgent.ID, - Apps: convertApps(dbApps, workspaceAgent, owner, workspace), + Apps: convertApps(dbApps, workspaceAgent, owner.Username, workspace), Scripts: convertScripts(scripts), DERPMap: api.DERPMap(), DERPForceWebSockets: api.DeploymentValues.DERP.Config.ForceWebSockets.Value(), @@ -1404,14 +1404,14 @@ func (api *API) workspaceAgentClientCoordinate(rw http.ResponseWriter, r *http.R // convertProvisionedApps converts applications that are in the middle of provisioning process. // It means that they may not have an agent or workspace assigned (dry-run job). func convertProvisionedApps(dbApps []database.WorkspaceApp) []codersdk.WorkspaceApp { - return convertApps(dbApps, database.WorkspaceAgent{}, database.User{}, database.Workspace{}) + return convertApps(dbApps, database.WorkspaceAgent{}, "", database.Workspace{}) } -func convertApps(dbApps []database.WorkspaceApp, agent database.WorkspaceAgent, owner database.User, workspace database.Workspace) []codersdk.WorkspaceApp { +func convertApps(dbApps []database.WorkspaceApp, agent database.WorkspaceAgent, ownerName string, workspace database.Workspace) []codersdk.WorkspaceApp { apps := make([]codersdk.WorkspaceApp, 0) for _, dbApp := range dbApps { var subdomainName string - if dbApp.Subdomain && agent.Name != "" && owner.Username != "" && workspace.Name != "" { + if dbApp.Subdomain && agent.Name != "" && ownerName != "" && workspace.Name != "" { appSlug := dbApp.Slug if appSlug == "" { appSlug = dbApp.DisplayName @@ -1420,7 +1420,7 @@ func convertApps(dbApps []database.WorkspaceApp, agent database.WorkspaceAgent, AppSlugOrPort: appSlug, AgentName: agent.Name, WorkspaceName: workspace.Name, - Username: owner.Username, + Username: ownerName, }.String() } diff --git a/coderd/workspacebuilds.go b/coderd/workspacebuilds.go index 998bf37ce157b..16326f9945fb2 100644 --- a/coderd/workspacebuilds.go +++ b/coderd/workspacebuilds.go @@ -68,12 +68,20 @@ func (api *API) workspaceBuild(rw http.ResponseWriter, r *http.Request) { }) return } + ownerName, ok := usernameWithID(workspace.OwnerID, data.users) + if !ok { + httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ + Message: "Internal error converting workspace build.", + Detail: "owner not found for workspace", + }) + return + } apiBuild, err := api.convertWorkspaceBuild( workspaceBuild, workspace, data.jobs[0], - data.users, + ownerName, data.resources, data.metadata, data.agents, @@ -274,12 +282,20 @@ func (api *API) workspaceBuildByBuildNumber(rw http.ResponseWriter, r *http.Requ }) return } + ownerName, ok := usernameWithID(workspace.OwnerID, data.users) + if !ok { + httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ + Message: "Internal error converting workspace build.", + Detail: "owner not found for workspace", + }) + return + } apiBuild, err := api.convertWorkspaceBuild( workspaceBuild, workspace, data.jobs[0], - data.users, + ownerName, data.resources, data.metadata, data.agents, @@ -398,6 +414,14 @@ func (api *API) postWorkspaceBuilds(rw http.ResponseWriter, r *http.Request) { }) return } + ownerName, exists := usernameWithID(workspace.OwnerID, users) + if !exists { + httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ + Message: "Internal error converting workspace build.", + Detail: "owner not found for workspace", + }) + return + } apiBuild, err := api.convertWorkspaceBuild( *workspaceBuild, @@ -406,7 +430,7 @@ func (api *API) postWorkspaceBuilds(rw http.ResponseWriter, r *http.Request) { ProvisionerJob: *provisionerJob, QueuePosition: 0, }, - users, + ownerName, []database.WorkspaceResource{}, []database.WorkspaceResourceMetadatum{}, []database.WorkspaceAgent{}, @@ -807,12 +831,16 @@ func (api *API) convertWorkspaceBuilds( if !exists { return nil, xerrors.New("template version not found") } + ownerName, exists := usernameWithID(workspace.OwnerID, users) + if !exists { + return nil, xerrors.Errorf("owner not found for workspace: %q", workspace.Name) + } apiBuild, err := api.convertWorkspaceBuild( build, workspace, job, - users, + ownerName, workspaceResources, resourceMetadata, resourceAgents, @@ -835,7 +863,7 @@ func (api *API) convertWorkspaceBuild( build database.WorkspaceBuild, workspace database.Workspace, job database.GetProvisionerJobsByIDsWithQueuePositionRow, - users []database.User, + ownerName string, workspaceResources []database.WorkspaceResource, resourceMetadata []database.WorkspaceResourceMetadatum, resourceAgents []database.WorkspaceAgent, @@ -844,10 +872,6 @@ func (api *API) convertWorkspaceBuild( agentLogSources []database.WorkspaceAgentLogSource, templateVersion database.TemplateVersion, ) (codersdk.WorkspaceBuild, error) { - userByID := map[uuid.UUID]database.User{} - for _, user := range users { - userByID[user.ID] = user - } resourcesByJobID := map[uuid.UUID][]database.WorkspaceResource{} for _, resource := range workspaceResources { resourcesByJobID[resource.JobID] = append(resourcesByJobID[resource.JobID], resource) @@ -873,11 +897,6 @@ func (api *API) convertWorkspaceBuild( logSourcesByAgentID[logSource.WorkspaceAgentID] = append(logSourcesByAgentID[logSource.WorkspaceAgentID], logSource) } - owner, exists := userByID[workspace.OwnerID] - if !exists { - return codersdk.WorkspaceBuild{}, xerrors.Errorf("owner not found for workspace: %q", workspace.Name) - } - resources := resourcesByJobID[job.ProvisionerJob.ID] apiResources := make([]codersdk.WorkspaceResource, 0) for _, resource := range resources { @@ -888,7 +907,7 @@ func (api *API) convertWorkspaceBuild( scripts := scriptsByAgentID[agent.ID] logSources := logSourcesByAgentID[agent.ID] apiAgent, err := convertWorkspaceAgent( - api.DERPMap(), *api.TailnetCoordinator.Load(), agent, convertApps(apps, agent, owner, workspace), convertScripts(scripts), convertLogSources(logSources), api.AgentInactiveDisconnectTimeout, + api.DERPMap(), *api.TailnetCoordinator.Load(), agent, convertApps(apps, agent, ownerName, workspace), convertScripts(scripts), convertLogSources(logSources), api.AgentInactiveDisconnectTimeout, api.DeploymentValues.AgentFallbackTroubleshootingURL.String(), ) if err != nil { @@ -906,7 +925,7 @@ func (api *API) convertWorkspaceBuild( CreatedAt: build.CreatedAt, UpdatedAt: build.UpdatedAt, WorkspaceOwnerID: workspace.OwnerID, - WorkspaceOwnerName: owner.Username, + WorkspaceOwnerName: ownerName, WorkspaceID: build.WorkspaceID, WorkspaceName: workspace.Name, TemplateVersionID: build.TemplateVersionID, diff --git a/coderd/workspaces.go b/coderd/workspaces.go index a95bf2afe7af3..b4f971fe974e6 100644 --- a/coderd/workspaces.go +++ b/coderd/workspaces.go @@ -92,12 +92,19 @@ func (api *API) workspace(rw http.ResponseWriter, r *http.Request) { httpapi.Forbidden(rw) return } - + ownerName, ok := usernameWithID(workspace.OwnerID, data.users) + if !ok { + httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ + Message: "Internal error fetching workspace resources.", + Detail: "unable to find workspace owner's username", + }) + return + } httpapi.Write(ctx, rw, http.StatusOK, convertWorkspace( workspace, data.builds[0], data.templates[0], - findUser(workspace.OwnerID, data.users), + ownerName, )) } @@ -274,12 +281,19 @@ func (api *API) workspaceByOwnerAndName(rw http.ResponseWriter, r *http.Request) httpapi.ResourceNotFound(rw) return } - + ownerName, ok := usernameWithID(workspace.OwnerID, data.users) + if !ok { + httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ + Message: "Internal error fetching workspace resources.", + Detail: "unable to find workspace owner's username", + }) + return + } httpapi.Write(ctx, rw, http.StatusOK, convertWorkspace( workspace, data.builds[0], data.templates[0], - findUser(workspace.OwnerID, data.users), + ownerName, )) } @@ -529,21 +543,11 @@ func (api *API) postWorkspacesByOrganization(rw http.ResponseWriter, r *http.Req } aReq.New = workspace - initiator, err := api.Database.GetUserByID(ctx, workspaceBuild.InitiatorID) - if err != nil { - httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ - Message: "Internal error fetching user.", - Detail: err.Error(), - }) - return - } - api.Telemetry.Report(&telemetry.Snapshot{ Workspaces: []telemetry.Workspace{telemetry.ConvertWorkspace(workspace)}, WorkspaceBuilds: []telemetry.WorkspaceBuild{telemetry.ConvertWorkspaceBuild(*workspaceBuild)}, }) - users := []database.User{user, initiator} apiBuild, err := api.convertWorkspaceBuild( *workspaceBuild, workspace, @@ -551,7 +555,7 @@ func (api *API) postWorkspacesByOrganization(rw http.ResponseWriter, r *http.Req ProvisionerJob: *provisionerJob, QueuePosition: 0, }, - users, + user.Username, []database.WorkspaceResource{}, []database.WorkspaceResourceMetadatum{}, []database.WorkspaceAgent{}, @@ -572,7 +576,7 @@ func (api *API) postWorkspacesByOrganization(rw http.ResponseWriter, r *http.Req workspace, apiBuild, template, - findUser(user.ID, users), + user.Username, )) } @@ -885,6 +889,14 @@ func (api *API) putWorkspaceDormant(rw http.ResponseWriter, r *http.Request) { }) return } + ownerName, ok := usernameWithID(workspace.OwnerID, data.users) + if !ok { + httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ + Message: "Internal error fetching workspace resources.", + Detail: "unable to find workspace owner's username", + }) + return + } if len(data.templates) == 0 { httpapi.Forbidden(rw) @@ -896,7 +908,7 @@ func (api *API) putWorkspaceDormant(rw http.ResponseWriter, r *http.Request) { workspace, data.builds[0], data.templates[0], - findUser(workspace.OwnerID, data.users), + ownerName, )) } @@ -1111,13 +1123,24 @@ func (api *API) watchWorkspace(rw http.ResponseWriter, r *http.Request) { return } + ownerName, ok := usernameWithID(workspace.OwnerID, data.users) + if !ok { + _ = sendEvent(ctx, codersdk.ServerSentEvent{ + Type: codersdk.ServerSentEventTypeError, + Data: codersdk.Response{ + Message: "Internal error fetching workspace resources.", + Detail: "unable to find workspace owner's username", + }, + }) + return + } _ = sendEvent(ctx, codersdk.ServerSentEvent{ Type: codersdk.ServerSentEventTypeData, Data: convertWorkspace( workspace, data.builds[0], data.templates[0], - findUser(workspace.OwnerID, data.users), + ownerName, ), }) } @@ -1267,7 +1290,7 @@ func convertWorkspaces(workspaces []database.Workspace, data workspaceData) ([]c workspace, build, template, - &owner, + owner.Username, )) } return apiWorkspaces, nil @@ -1277,7 +1300,7 @@ func convertWorkspace( workspace database.Workspace, workspaceBuild codersdk.WorkspaceBuild, template database.Template, - owner *database.User, + ownerName string, ) codersdk.Workspace { var autostartSchedule *string if workspace.AutostartSchedule.Valid { @@ -1310,7 +1333,7 @@ func convertWorkspace( CreatedAt: workspace.CreatedAt, UpdatedAt: workspace.UpdatedAt, OwnerID: workspace.OwnerID, - OwnerName: owner.Username, + OwnerName: ownerName, OrganizationID: workspace.OrganizationID, TemplateID: workspace.TemplateID, LatestBuild: workspaceBuild,