From 34d902ebf19ea294db0b906e28c22e89c698b67a Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Mon, 29 Aug 2022 08:56:52 -0400 Subject: [PATCH] fix: Fix properly selecting workspace apps by agent (#3684) --- coderd/coderd.go | 8 +- coderd/coderdtest/authtest.go | 67 +++-- coderd/database/databasefake/databasefake.go | 10 +- coderd/httpmw/workspaceparam.go | 110 +++++++ coderd/httpmw/workspaceparam_test.go | 291 +++++++++++++++++++ coderd/workspaceapps.go | 69 +---- 6 files changed, 452 insertions(+), 103 deletions(-) diff --git a/coderd/coderd.go b/coderd/coderd.go index 7bfdffe1d2..f7b8603367 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -136,17 +136,19 @@ func New(options *Options) *API { apps := func(r chi.Router) { r.Use( httpmw.RateLimitPerMinute(options.APIRateLimit), + tracing.HTTPMW(api.TracerProvider, "coderd.http"), httpmw.ExtractAPIKey(options.Database, oauthConfigs, true), httpmw.ExtractUserParam(api.Database), - tracing.HTTPMW(api.TracerProvider, "coderd.http"), + // Extracts the from the url + httpmw.ExtractWorkspaceAndAgentParam(api.Database), ) r.HandleFunc("/*", api.workspaceAppsProxyPath) } // %40 is the encoded character of the @ symbol. VS Code Web does // not handle character encoding properly, so it's safe to assume // other applications might not as well. - r.Route("/%40{user}/{workspacename}/apps/{workspaceapp}", apps) - r.Route("/@{user}/{workspacename}/apps/{workspaceapp}", apps) + r.Route("/%40{user}/{workspace_and_agent}/apps/{workspaceapp}", apps) + r.Route("/@{user}/{workspace_and_agent}/apps/{workspaceapp}", apps) r.Route("/api/v2", func(r chi.Router) { r.NotFound(func(rw http.ResponseWriter, r *http.Request) { diff --git a/coderd/coderdtest/authtest.go b/coderd/coderdtest/authtest.go index d143f0af50..f88fd21f20 100644 --- a/coderd/coderdtest/authtest.go +++ b/coderd/coderdtest/authtest.go @@ -85,6 +85,7 @@ func NewAuthTester(ctx context.Context, t *testing.T, options *Options) *AuthTes Name: "some", Type: "example", Agents: []*proto.Agent{{ + Name: "agent", Id: "something", Auth: &proto.Agent_Token{}, Apps: []*proto.App{{ @@ -119,22 +120,23 @@ func NewAuthTester(ctx context.Context, t *testing.T, options *Options) *AuthTes require.NoError(t, err, "create template param") urlParameters := map[string]string{ - "{organization}": admin.OrganizationID.String(), - "{user}": admin.UserID.String(), - "{organizationname}": organization.Name, - "{workspace}": workspace.ID.String(), - "{workspacebuild}": workspace.LatestBuild.ID.String(), - "{workspacename}": workspace.Name, - "{workspacebuildname}": workspace.LatestBuild.Name, - "{workspaceagent}": workspaceResources[0].Agents[0].ID.String(), - "{buildnumber}": strconv.FormatInt(int64(workspace.LatestBuild.BuildNumber), 10), - "{template}": template.ID.String(), - "{hash}": file.Hash, - "{workspaceresource}": workspaceResources[0].ID.String(), - "{workspaceapp}": workspaceResources[0].Agents[0].Apps[0].Name, - "{templateversion}": version.ID.String(), - "{jobID}": templateVersionDryRun.ID.String(), - "{templatename}": template.Name, + "{organization}": admin.OrganizationID.String(), + "{user}": admin.UserID.String(), + "{organizationname}": organization.Name, + "{workspace}": workspace.ID.String(), + "{workspacebuild}": workspace.LatestBuild.ID.String(), + "{workspacename}": workspace.Name, + "{workspacebuildname}": workspace.LatestBuild.Name, + "{workspaceagent}": workspaceResources[0].Agents[0].ID.String(), + "{buildnumber}": strconv.FormatInt(int64(workspace.LatestBuild.BuildNumber), 10), + "{template}": template.ID.String(), + "{hash}": file.Hash, + "{workspaceresource}": workspaceResources[0].ID.String(), + "{workspaceapp}": workspaceResources[0].Agents[0].Apps[0].Name, + "{templateversion}": version.ID.String(), + "{jobID}": templateVersionDryRun.ID.String(), + "{templatename}": template.Name, + "{workspace_and_agent}": workspace.Name + "." + workspaceResources[0].Agents[0].Name, // Only checking template scoped params here "parameters/{scope}/{id}": fmt.Sprintf("parameters/%s/%s", string(templateParam.Scope), templateParam.ScopeID.String()), @@ -178,15 +180,6 @@ func AGPLRoutes(a *AuthTester) (map[string]string, map[string]RouteCheck) { "POST:/api/v2/csp/reports": {NoAuthorize: true}, "GET:/api/v2/entitlements": {NoAuthorize: true}, - "GET:/%40{user}/{workspacename}/apps/{workspaceapp}/*": { - AssertAction: rbac.ActionCreate, - AssertObject: workspaceExecObj, - }, - "GET:/@{user}/{workspacename}/apps/{workspaceapp}/*": { - AssertAction: rbac.ActionCreate, - AssertObject: workspaceExecObj, - }, - // Has it's own auth "GET:/api/v2/users/oauth2/github/callback": {NoAuthorize: true}, "GET:/api/v2/users/oidc/callback": {NoAuthorize: true}, @@ -399,6 +392,29 @@ func AGPLRoutes(a *AuthTester) (map[string]string, map[string]RouteCheck) { "POST:/api/v2/workspaces/{workspace}/builds": {StatusCode: http.StatusBadRequest, NoAuthorize: true}, "POST:/api/v2/organizations/{organization}/templateversions": {StatusCode: http.StatusBadRequest, NoAuthorize: true}, } + + // Routes like proxy routes support all HTTP methods. A helper func to expand + // 1 url to all http methods. + assertAllHTTPMethods := func(url string, check RouteCheck) { + methods := []string{http.MethodGet, http.MethodHead, http.MethodPost, + http.MethodPut, http.MethodPatch, http.MethodDelete, + http.MethodConnect, http.MethodOptions, http.MethodTrace} + + for _, method := range methods { + route := method + ":" + url + assertRoute[route] = check + } + } + + assertAllHTTPMethods("/%40{user}/{workspace_and_agent}/apps/{workspaceapp}/*", RouteCheck{ + AssertAction: rbac.ActionCreate, + AssertObject: workspaceExecObj, + }) + assertAllHTTPMethods("/@{user}/{workspace_and_agent}/apps/{workspaceapp}/*", RouteCheck{ + AssertAction: rbac.ActionCreate, + AssertObject: workspaceExecObj, + }) + return skipRoutes, assertRoute } @@ -446,6 +462,7 @@ func (a *AuthTester) Test(ctx context.Context, assertRoute map[string]RouteCheck a.t.Run(name, func(t *testing.T) { a.authorizer.reset() routeKey := strings.TrimRight(name, "/") + routeAssertions, ok := assertRoute[routeKey] if !ok { // By default, all omitted routes check for just "authorize" called diff --git a/coderd/database/databasefake/databasefake.go b/coderd/database/databasefake/databasefake.go index 21634eab29..590d3c5685 100644 --- a/coderd/database/databasefake/databasefake.go +++ b/coderd/database/databasefake/databasefake.go @@ -592,14 +592,14 @@ func (q *fakeQuerier) GetLatestWorkspaceBuildByWorkspaceID(_ context.Context, wo defer q.mutex.RUnlock() var row database.WorkspaceBuild - var buildNum int32 + var buildNum int32 = -1 for _, workspaceBuild := range q.workspaceBuilds { if workspaceBuild.WorkspaceID.String() == workspaceID.String() && workspaceBuild.BuildNumber > buildNum { row = workspaceBuild buildNum = workspaceBuild.BuildNumber } } - if buildNum == 0 { + if buildNum == -1 { return database.WorkspaceBuild{}, sql.ErrNoRows } return row, nil @@ -1263,9 +1263,6 @@ func (q *fakeQuerier) GetWorkspaceAgentsByResourceIDs(_ context.Context, resourc workspaceAgents = append(workspaceAgents, agent) } } - if len(workspaceAgents) == 0 { - return nil, sql.ErrNoRows - } return workspaceAgents, nil } @@ -1347,9 +1344,6 @@ func (q *fakeQuerier) GetWorkspaceResourcesByJobID(_ context.Context, jobID uuid } resources = append(resources, resource) } - if len(resources) == 0 { - return nil, sql.ErrNoRows - } return resources, nil } diff --git a/coderd/httpmw/workspaceparam.go b/coderd/httpmw/workspaceparam.go index cd0fa86258..1ce23a20d9 100644 --- a/coderd/httpmw/workspaceparam.go +++ b/coderd/httpmw/workspaceparam.go @@ -4,11 +4,15 @@ import ( "context" "database/sql" "errors" + "fmt" "net/http" + "strings" "github.com/coder/coder/coderd/database" "github.com/coder/coder/coderd/httpapi" "github.com/coder/coder/codersdk" + "github.com/go-chi/chi/v5" + "github.com/google/uuid" ) type workspaceParamContextKey struct{} @@ -48,3 +52,109 @@ func ExtractWorkspaceParam(db database.Store) func(http.Handler) http.Handler { }) } } + +// ExtractWorkspaceAndAgentParam grabs a workspace and an agent from the +// "workspace_and_agent" URL parameter. `ExtractUserParam` must be called +// before this. +// This can be in the form of: +// - ".[workspace-agent]" : If multiple agents exist +// - "" : If one agent exists +func ExtractWorkspaceAndAgentParam(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) { + user := UserParam(r) + workspaceWithAgent := chi.URLParam(r, "workspace_and_agent") + workspaceParts := strings.Split(workspaceWithAgent, ".") + + workspace, err := db.GetWorkspaceByOwnerIDAndName(r.Context(), database.GetWorkspaceByOwnerIDAndNameParams{ + OwnerID: user.ID, + Name: workspaceParts[0], + }) + if err != nil { + if errors.Is(err, sql.ErrNoRows) { + httpapi.ResourceNotFound(rw) + return + } + httpapi.Write(rw, http.StatusInternalServerError, codersdk.Response{ + Message: "Internal error fetching workspace.", + Detail: err.Error(), + }) + return + } + + build, err := db.GetLatestWorkspaceBuildByWorkspaceID(r.Context(), workspace.ID) + if err != nil { + httpapi.Write(rw, http.StatusInternalServerError, codersdk.Response{ + Message: "Internal error fetching workspace build.", + Detail: err.Error(), + }) + return + } + + resources, err := db.GetWorkspaceResourcesByJobID(r.Context(), build.JobID) + if err != nil { + httpapi.Write(rw, http.StatusInternalServerError, codersdk.Response{ + Message: "Internal error fetching workspace resources.", + Detail: err.Error(), + }) + return + } + resourceIDs := make([]uuid.UUID, 0) + for _, resource := range resources { + resourceIDs = append(resourceIDs, resource.ID) + } + + agents, err := db.GetWorkspaceAgentsByResourceIDs(r.Context(), resourceIDs) + if err != nil { + httpapi.Write(rw, http.StatusInternalServerError, codersdk.Response{ + Message: "Internal error fetching workspace agents.", + Detail: err.Error(), + }) + return + } + + if len(agents) == 0 { + httpapi.Write(rw, http.StatusBadRequest, codersdk.Response{ + Message: "No agents exist for this workspace", + }) + return + } + + // If we have more than 1 workspace agent, we need to specify which one to use. + if len(agents) > 1 && len(workspaceParts) <= 1 { + httpapi.Write(rw, http.StatusBadRequest, codersdk.Response{ + Message: "More than one agent exists, but no agent specified.", + }) + return + } + + var agent database.WorkspaceAgent + var found bool + // If we have more than 1 workspace agent, we need to specify which one to use. + // If the user specified an agent, we need to make sure that agent + // actually exists. + if len(workspaceParts) > 1 || len(agents) > 1 { + for _, otherAgent := range agents { + if otherAgent.Name == workspaceParts[1] { + agent = otherAgent + found = true + break + } + } + if !found { + httpapi.Write(rw, http.StatusBadRequest, codersdk.Response{ + Message: fmt.Sprintf("No agent exists with the name %q", workspaceParts[1]), + }) + return + } + } else { + agent = agents[0] + } + + ctx := r.Context() + ctx = context.WithValue(ctx, workspaceParamContextKey{}, workspace) + ctx = context.WithValue(ctx, workspaceAgentParamContextKey{}, agent) + next.ServeHTTP(rw, r.WithContext(ctx)) + }) + } +} diff --git a/coderd/httpmw/workspaceparam_test.go b/coderd/httpmw/workspaceparam_test.go index ecbebf13c5..5b3f9a5830 100644 --- a/coderd/httpmw/workspaceparam_test.go +++ b/coderd/httpmw/workspaceparam_test.go @@ -3,12 +3,15 @@ package httpmw_test import ( "context" "crypto/sha256" + "encoding/json" "fmt" "net/http" "net/http/httptest" "testing" "time" + "github.com/stretchr/testify/assert" + "github.com/go-chi/chi/v5" "github.com/google/uuid" "github.com/stretchr/testify/require" @@ -122,3 +125,291 @@ func TestWorkspaceParam(t *testing.T) { require.Equal(t, http.StatusOK, res.StatusCode) }) } + +func TestWorkspaceAgentByNameParam(t *testing.T) { + t.Parallel() + + testCases := []struct { + Name string + // Agents are mapped to a resource + Agents map[string][]string + URLParam string + WorkspaceName string + ExpectedAgent string + ExpectedStatusCode int + ExpectedError string + }{ + { + Name: "NoAgents", + WorkspaceName: "dev", + Agents: map[string][]string{}, + URLParam: "dev", + ExpectedError: "No agents exist", + ExpectedStatusCode: http.StatusBadRequest, + }, + { + Name: "NoAgentsSpecify", + WorkspaceName: "dev", + Agents: map[string][]string{}, + URLParam: "dev.agent", + ExpectedError: "No agents exist", + ExpectedStatusCode: http.StatusBadRequest, + }, + { + Name: "MultipleAgents", + WorkspaceName: "dev", + Agents: map[string][]string{ + "resource-a": { + "agent-one", + "agent-two", + }, + }, + URLParam: "dev", + ExpectedStatusCode: http.StatusBadRequest, + ExpectedError: "More than one agent exists, but no agent specified", + }, + { + Name: "MultipleResources", + WorkspaceName: "dev", + Agents: map[string][]string{ + "resource-a": { + "agent-one", + }, + "resource-b": { + "agent-two", + }, + }, + URLParam: "dev", + ExpectedStatusCode: http.StatusBadRequest, + ExpectedError: "More than one agent exists, but no agent specified", + }, + { + Name: "NotExistsOneAgent", + WorkspaceName: "dev", + Agents: map[string][]string{ + "resource-a": { + "agent-one", + }, + }, + URLParam: "dev.not-exists", + ExpectedStatusCode: http.StatusBadRequest, + ExpectedError: "No agent exists with the name", + }, + { + Name: "NotExistsMultipleAgents", + WorkspaceName: "dev", + Agents: map[string][]string{ + "resource-a": { + "agent-one", + }, + "resource-b": { + "agent-two", + }, + "resource-c": { + "agent-three", + }, + }, + URLParam: "dev.not-exists", + ExpectedStatusCode: http.StatusBadRequest, + ExpectedError: "No agent exists with the name", + }, + + // OKs + { + Name: "MultipleResourcesOneAgent", + WorkspaceName: "dev", + Agents: map[string][]string{ + "resource-a": {}, + "resource-b": { + "agent-one", + }, + }, + URLParam: "dev", + ExpectedAgent: "agent-one", + ExpectedStatusCode: http.StatusOK, + }, + { + Name: "OneAgent", + WorkspaceName: "dev", + Agents: map[string][]string{ + "resource-a": { + "agent-one", + }, + }, + URLParam: "dev", + ExpectedAgent: "agent-one", + ExpectedStatusCode: http.StatusOK, + }, + { + Name: "OneAgentSelected", + WorkspaceName: "dev", + Agents: map[string][]string{ + "resource-a": { + "agent-one", + }, + }, + URLParam: "dev.agent-one", + ExpectedAgent: "agent-one", + ExpectedStatusCode: http.StatusOK, + }, + { + Name: "MultipleAgentSelectOne", + WorkspaceName: "dev", + Agents: map[string][]string{ + "resource-a": { + "agent-one", + "agent-two", + "agent-selected", + }, + }, + URLParam: "dev.agent-selected", + ExpectedAgent: "agent-selected", + ExpectedStatusCode: http.StatusOK, + }, + { + Name: "MultipleResourcesSelectOne", + WorkspaceName: "dev", + Agents: map[string][]string{ + "resource-a": { + "agent-one", + }, + "resource-b": { + "agent-two", + }, + "resource-c": { + "agent-selected", + "agent-three", + }, + }, + URLParam: "dev.agent-selected", + ExpectedAgent: "agent-selected", + ExpectedStatusCode: http.StatusOK, + }, + } + + for _, c := range testCases { + c := c + t.Run(c.Name, func(t *testing.T) { + t.Parallel() + db, r := setupWorkspaceWithAgents(t, setupConfig{ + WorkspaceName: c.WorkspaceName, + Agents: c.Agents, + }) + + chi.RouteContext(r.Context()).URLParams.Add("workspace_and_agent", c.URLParam) + + rtr := chi.NewRouter() + rtr.Use( + httpmw.ExtractAPIKey(db, nil, true), + httpmw.ExtractUserParam(db), + httpmw.ExtractWorkspaceAndAgentParam(db), + ) + rtr.Get("/", func(w http.ResponseWriter, r *http.Request) { + workspace := httpmw.WorkspaceParam(r) + agent := httpmw.WorkspaceAgentParam(r) + + assert.Equal(t, c.ExpectedAgent, agent.Name, "expected agent name") + assert.Equal(t, c.WorkspaceName, workspace.Name, "expected workspace name") + }) + + rw := httptest.NewRecorder() + rtr.ServeHTTP(rw, r) + res := rw.Result() + var coderResp codersdk.Response + _ = json.NewDecoder(res.Body).Decode(&coderResp) + res.Body.Close() + require.Equal(t, c.ExpectedStatusCode, res.StatusCode) + if c.ExpectedError != "" { + require.Contains(t, coderResp.Message, c.ExpectedError) + } + }) + } +} + +type setupConfig struct { + WorkspaceName string + // Agents are mapped to a resource + Agents map[string][]string +} + +func setupWorkspaceWithAgents(t testing.TB, cfg setupConfig) (database.Store, *http.Request) { + t.Helper() + db := databasefake.New() + var ( + id, secret = randomAPIKeyParts() + hashed = sha256.Sum256([]byte(secret)) + ) + r := httptest.NewRequest("GET", "/", nil) + r.AddCookie(&http.Cookie{ + Name: codersdk.SessionTokenKey, + Value: fmt.Sprintf("%s-%s", id, secret), + }) + + userID := uuid.New() + username, err := cryptorand.String(8) + require.NoError(t, err) + user, err := db.InsertUser(r.Context(), database.InsertUserParams{ + ID: userID, + Email: "testaccount@coder.com", + HashedPassword: hashed[:], + Username: username, + CreatedAt: database.Now(), + UpdatedAt: database.Now(), + }) + require.NoError(t, err) + + _, err = db.InsertAPIKey(r.Context(), database.InsertAPIKeyParams{ + ID: id, + UserID: user.ID, + HashedSecret: hashed[:], + LastUsed: database.Now(), + ExpiresAt: database.Now().Add(time.Minute), + LoginType: database.LoginTypePassword, + }) + require.NoError(t, err) + + workspace, err := db.InsertWorkspace(context.Background(), database.InsertWorkspaceParams{ + ID: uuid.New(), + TemplateID: uuid.New(), + OwnerID: user.ID, + Name: cfg.WorkspaceName, + }) + require.NoError(t, err) + + build, err := db.InsertWorkspaceBuild(context.Background(), database.InsertWorkspaceBuildParams{ + ID: uuid.New(), + WorkspaceID: workspace.ID, + JobID: uuid.New(), + }) + require.NoError(t, err) + + job, err := db.InsertProvisionerJob(context.Background(), database.InsertProvisionerJobParams{ + ID: build.JobID, + Type: database.ProvisionerJobTypeWorkspaceBuild, + }) + require.NoError(t, err) + + for resourceName, agentNames := range cfg.Agents { + resource, err := db.InsertWorkspaceResource(context.Background(), database.InsertWorkspaceResourceParams{ + ID: uuid.New(), + JobID: job.ID, + Name: resourceName, + }) + require.NoError(t, err) + + for _, name := range agentNames { + _, err = db.InsertWorkspaceAgent(context.Background(), database.InsertWorkspaceAgentParams{ + ID: uuid.New(), + ResourceID: resource.ID, + Name: name, + }) + require.NoError(t, err) + } + } + + ctx := chi.NewRouteContext() + ctx.URLParams.Add("user", codersdk.Me) + r = r.WithContext(context.WithValue(r.Context(), chi.RouteCtxKey, ctx)) + + return db, r +} diff --git a/coderd/workspaceapps.go b/coderd/workspaceapps.go index 4f9ef2d6ae..de05d27789 100644 --- a/coderd/workspaceapps.go +++ b/coderd/workspaceapps.go @@ -10,7 +10,6 @@ import ( "strings" "github.com/go-chi/chi/v5" - "github.com/google/uuid" "github.com/coder/coder/coderd/database" "github.com/coder/coder/coderd/httpapi" @@ -24,78 +23,14 @@ import ( // workspaceAppsProxyPath proxies requests to a workspace application // through a relative URL path. func (api *API) workspaceAppsProxyPath(rw http.ResponseWriter, r *http.Request) { - user := httpmw.UserParam(r) - // This can be in the form of: ".[workspace-agent]" or "" - workspaceWithAgent := chi.URLParam(r, "workspacename") - workspaceParts := strings.Split(workspaceWithAgent, ".") - - workspace, err := api.Database.GetWorkspaceByOwnerIDAndName(r.Context(), database.GetWorkspaceByOwnerIDAndNameParams{ - OwnerID: user.ID, - Name: workspaceParts[0], - }) - if errors.Is(err, sql.ErrNoRows) { - httpapi.ResourceNotFound(rw) - return - } - if err != nil { - httpapi.Write(rw, http.StatusInternalServerError, codersdk.Response{ - Message: "Internal error fetching workspace.", - Detail: err.Error(), - }) - return - } + workspace := httpmw.WorkspaceParam(r) + agent := httpmw.WorkspaceAgentParam(r) if !api.Authorize(r, rbac.ActionCreate, workspace.ExecutionRBAC()) { httpapi.ResourceNotFound(rw) return } - build, err := api.Database.GetLatestWorkspaceBuildByWorkspaceID(r.Context(), workspace.ID) - if err != nil { - httpapi.Write(rw, http.StatusInternalServerError, codersdk.Response{ - Message: "Internal error fetching workspace build.", - Detail: err.Error(), - }) - return - } - - resources, err := api.Database.GetWorkspaceResourcesByJobID(r.Context(), build.JobID) - if err != nil { - httpapi.Write(rw, http.StatusInternalServerError, codersdk.Response{ - Message: "Internal error fetching workspace resources.", - Detail: err.Error(), - }) - return - } - resourceIDs := make([]uuid.UUID, 0) - for _, resource := range resources { - resourceIDs = append(resourceIDs, resource.ID) - } - agents, err := api.Database.GetWorkspaceAgentsByResourceIDs(r.Context(), resourceIDs) - if err != nil { - httpapi.Write(rw, http.StatusInternalServerError, codersdk.Response{ - Message: "Internal error fetching workspace agents.", - Detail: err.Error(), - }) - return - } - if len(agents) == 0 { - httpapi.Write(rw, http.StatusBadRequest, codersdk.Response{ - Message: "No agents exist.", - }) - return - } - - agent := agents[0] - if len(workspaceParts) > 1 { - for _, otherAgent := range agents { - if otherAgent.Name == workspaceParts[1] { - agent = otherAgent - break - } - } - } - app, err := api.Database.GetWorkspaceAppByAgentIDAndName(r.Context(), database.GetWorkspaceAppByAgentIDAndNameParams{ AgentID: agent.ID, Name: chi.URLParam(r, "workspaceapp"),