From 433705d03fc57c3808b96f4ed167f865d96ca76c Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 24 Aug 2022 14:53:14 -0400 Subject: [PATCH 1/9] fix: Fix properly naming workspace agents --- coderd/httpmw/chiparams.go | 1 + coderd/workspaceapps.go | 23 ++++++++++++++++++++--- 2 files changed, 21 insertions(+), 3 deletions(-) create mode 100644 coderd/httpmw/chiparams.go diff --git a/coderd/httpmw/chiparams.go b/coderd/httpmw/chiparams.go new file mode 100644 index 0000000000000..934d5031cd0ff --- /dev/null +++ b/coderd/httpmw/chiparams.go @@ -0,0 +1 @@ +package httpmw diff --git a/coderd/workspaceapps.go b/coderd/workspaceapps.go index 859232072b038..65011571d3a2d 100644 --- a/coderd/workspaceapps.go +++ b/coderd/workspaceapps.go @@ -85,14 +85,31 @@ func (api *API) workspaceAppsProxyPath(rw http.ResponseWriter, r *http.Request) return } - agent := agents[0] - if len(workspaceParts) > 1 { + // 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 + } + + // If we have more than 1 workspace agent, we need to specify which one to use. + var agent *database.WorkspaceAgent + if len(agents) > 1 { for _, otherAgent := range agents { if otherAgent.Name == workspaceParts[1] { - agent = otherAgent + agent = &otherAgent break } } + if agent == nil { + httpapi.Write(rw, http.StatusBadRequest, codersdk.Response{ + Message: fmt.Sprintf("No agent exists with the name %s", workspaceParts[1]), + }) + return + } + } else { + agent = &agents[0] } app, err := api.Database.GetWorkspaceAppByAgentIDAndName(r.Context(), database.GetWorkspaceAppByAgentIDAndNameParams{ From 7d0bc561c266d4a3d20de800c9033cf158f1011f Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 24 Aug 2022 15:07:22 -0400 Subject: [PATCH 2/9] chore: Make workspace and agent extract a middleware --- coderd/coderd.go | 5 +- coderd/httpmw/chiparams.go | 1 - coderd/httpmw/workspaceparam.go | 107 ++++++++++++++++++++++++++++++++ coderd/workspaceapps.go | 86 +------------------------ 4 files changed, 112 insertions(+), 87 deletions(-) delete mode 100644 coderd/httpmw/chiparams.go diff --git a/coderd/coderd.go b/coderd/coderd.go index a2062b736227a..72d7f34b4f33d 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -138,14 +138,15 @@ func New(options *Options) *API { httpmw.RateLimitPerMinute(options.APIRateLimit), httpmw.ExtractAPIKey(options.Database, oauthConfigs, true), httpmw.ExtractUserParam(api.Database), + 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}/{workspacename_and_agent}/apps/{workspaceapp}", apps) + r.Route("/@{user}/{workspacename_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/httpmw/chiparams.go b/coderd/httpmw/chiparams.go deleted file mode 100644 index 934d5031cd0ff..0000000000000 --- a/coderd/httpmw/chiparams.go +++ /dev/null @@ -1 +0,0 @@ -package httpmw diff --git a/coderd/httpmw/workspaceparam.go b/coderd/httpmw/workspaceparam.go index cd0fa8625813c..103dc60390ffe 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,106 @@ func ExtractWorkspaceParam(db database.Store) func(http.Handler) http.Handler { }) } } + +// ExtractWorkspaceAndAgentParam grabs a workspace and an agent from the +// "workspacename_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, "workspacename_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 + } + + // If we have more than 1 workspace agent, we need to specify which one to use. + var agent database.WorkspaceAgent + var found bool + if 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 %s", workspaceParts[1]), + }) + return + } + } else { + agent = agents[0] + } + + ctx := context.WithValue(r.Context(), workspaceParamContextKey{}, workspace) + ctx = context.WithValue(r.Context(), workspaceAgentContextKey{}, agent) + next.ServeHTTP(rw, r.WithContext(ctx)) + }) + } +} diff --git a/coderd/workspaceapps.go b/coderd/workspaceapps.go index 65011571d3a2d..8a808f0a9b5a9 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" @@ -23,95 +22,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 - } - - // 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 - } - - // If we have more than 1 workspace agent, we need to specify which one to use. - var agent *database.WorkspaceAgent - if len(agents) > 1 { - for _, otherAgent := range agents { - if otherAgent.Name == workspaceParts[1] { - agent = &otherAgent - break - } - } - if agent == nil { - httpapi.Write(rw, http.StatusBadRequest, codersdk.Response{ - Message: fmt.Sprintf("No agent exists with the name %s", workspaceParts[1]), - }) - return - } - } else { - agent = &agents[0] - } - app, err := api.Database.GetWorkspaceAppByAgentIDAndName(r.Context(), database.GetWorkspaceAppByAgentIDAndNameParams{ AgentID: agent.ID, Name: chi.URLParam(r, "workspaceapp"), From 0bb51b46ee4d99d212028adcbce7d47001897a1a Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 24 Aug 2022 15:47:02 -0400 Subject: [PATCH 3/9] chore: Write unit tests for agent selector Fix fake database implementation for returning no rows --- coderd/coderd.go | 1 + coderd/database/databasefake/databasefake.go | 10 +- coderd/httpmw/workspaceparam.go | 5 +- coderd/httpmw/workspaceparam_test.go | 240 +++++++++++++++++++ 4 files changed, 246 insertions(+), 10 deletions(-) diff --git a/coderd/coderd.go b/coderd/coderd.go index 72d7f34b4f33d..24a24a69a95dc 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -138,6 +138,7 @@ func New(options *Options) *API { httpmw.RateLimitPerMinute(options.APIRateLimit), httpmw.ExtractAPIKey(options.Database, oauthConfigs, true), httpmw.ExtractUserParam(api.Database), + // Extracts the from the url httpmw.ExtractWorkspaceAndAgentParam(api.Database), ) r.HandleFunc("/*", api.workspaceAppsProxyPath) diff --git a/coderd/database/databasefake/databasefake.go b/coderd/database/databasefake/databasefake.go index b92d066749569..70f2c4f57a3f7 100644 --- a/coderd/database/databasefake/databasefake.go +++ b/coderd/database/databasefake/databasefake.go @@ -591,14 +591,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 @@ -1262,9 +1262,6 @@ func (q *fakeQuerier) GetWorkspaceAgentsByResourceIDs(_ context.Context, resourc workspaceAgents = append(workspaceAgents, agent) } } - if len(workspaceAgents) == 0 { - return nil, sql.ErrNoRows - } return workspaceAgents, nil } @@ -1346,9 +1343,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 103dc60390ffe..5aa026a41e97f 100644 --- a/coderd/httpmw/workspaceparam.go +++ b/coderd/httpmw/workspaceparam.go @@ -149,8 +149,9 @@ func ExtractWorkspaceAndAgentParam(db database.Store) func(http.Handler) http.Ha agent = agents[0] } - ctx := context.WithValue(r.Context(), workspaceParamContextKey{}, workspace) - ctx = context.WithValue(r.Context(), workspaceAgentContextKey{}, agent) + 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 ecbebf13c5a4c..c65dc665580d8 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,240 @@ 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: "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", + }, + + // 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: "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) { + db, r := setupWorkspaceWithAgents(t, setupConfig{ + WorkspaceName: c.WorkspaceName, + Agents: c.Agents, + }) + + chi.RouteContext(r.Context()).URLParams.Add("workspacename_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 +} From 8b3f46d50a3713a9153d162364ecff090b304850 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 24 Aug 2022 16:20:12 -0400 Subject: [PATCH 4/9] Add unit test cases --- coderd/httpmw/workspaceparam.go | 8 +++++--- coderd/httpmw/workspaceparam_test.go | 30 ++++++++++++++++++++++++++++ 2 files changed, 35 insertions(+), 3 deletions(-) diff --git a/coderd/httpmw/workspaceparam.go b/coderd/httpmw/workspaceparam.go index 5aa026a41e97f..79c548d156062 100644 --- a/coderd/httpmw/workspaceparam.go +++ b/coderd/httpmw/workspaceparam.go @@ -128,10 +128,12 @@ func ExtractWorkspaceAndAgentParam(db database.Store) func(http.Handler) http.Ha return } - // If we have more than 1 workspace agent, we need to specify which one to use. var agent database.WorkspaceAgent var found bool - if len(agents) > 1 { + // 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 @@ -141,7 +143,7 @@ func ExtractWorkspaceAndAgentParam(db database.Store) func(http.Handler) http.Ha } if !found { httpapi.Write(rw, http.StatusBadRequest, codersdk.Response{ - Message: fmt.Sprintf("No agent exists with the name %s", workspaceParts[1]), + Message: fmt.Sprintf("No agent exists with the name %q", workspaceParts[1]), }) return } diff --git a/coderd/httpmw/workspaceparam_test.go b/coderd/httpmw/workspaceparam_test.go index c65dc665580d8..06e8a6208da1f 100644 --- a/coderd/httpmw/workspaceparam_test.go +++ b/coderd/httpmw/workspaceparam_test.go @@ -175,6 +175,36 @@ func TestWorkspaceAgentByNameParam(t *testing.T) { 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 { From f44f5c05eb4e6466a52ce23751db9de437941ddc Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 24 Aug 2022 16:21:29 -0400 Subject: [PATCH 5/9] Linting --- coderd/httpmw/workspaceparam_test.go | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/coderd/httpmw/workspaceparam_test.go b/coderd/httpmw/workspaceparam_test.go index 06e8a6208da1f..d18d12a2e5b60 100644 --- a/coderd/httpmw/workspaceparam_test.go +++ b/coderd/httpmw/workspaceparam_test.go @@ -133,7 +133,7 @@ func TestWorkspaceAgentByNameParam(t *testing.T) { Name string // Agents are mapped to a resource Agents map[string][]string - UrlParam string + URLParam string WorkspaceName string ExpectedAgent string ExpectedStatusCode int @@ -143,7 +143,7 @@ func TestWorkspaceAgentByNameParam(t *testing.T) { Name: "NoAgents", WorkspaceName: "dev", Agents: map[string][]string{}, - UrlParam: "dev", + URLParam: "dev", ExpectedError: "No agents exist", ExpectedStatusCode: http.StatusBadRequest, }, @@ -156,7 +156,7 @@ func TestWorkspaceAgentByNameParam(t *testing.T) { "agent-two", }, }, - UrlParam: "dev", + URLParam: "dev", ExpectedStatusCode: http.StatusBadRequest, ExpectedError: "More than one agent exists, but no agent specified", }, @@ -171,7 +171,7 @@ func TestWorkspaceAgentByNameParam(t *testing.T) { "agent-two", }, }, - UrlParam: "dev", + URLParam: "dev", ExpectedStatusCode: http.StatusBadRequest, ExpectedError: "More than one agent exists, but no agent specified", }, @@ -183,7 +183,7 @@ func TestWorkspaceAgentByNameParam(t *testing.T) { "agent-one", }, }, - UrlParam: "dev.not-exists", + URLParam: "dev.not-exists", ExpectedStatusCode: http.StatusBadRequest, ExpectedError: "No agent exists with the name", }, @@ -201,7 +201,7 @@ func TestWorkspaceAgentByNameParam(t *testing.T) { "agent-three", }, }, - UrlParam: "dev.not-exists", + URLParam: "dev.not-exists", ExpectedStatusCode: http.StatusBadRequest, ExpectedError: "No agent exists with the name", }, @@ -216,7 +216,7 @@ func TestWorkspaceAgentByNameParam(t *testing.T) { "agent-one", }, }, - UrlParam: "dev", + URLParam: "dev", ExpectedAgent: "agent-one", ExpectedStatusCode: http.StatusOK, }, @@ -228,7 +228,7 @@ func TestWorkspaceAgentByNameParam(t *testing.T) { "agent-one", }, }, - UrlParam: "dev", + URLParam: "dev", ExpectedAgent: "agent-one", ExpectedStatusCode: http.StatusOK, }, @@ -242,7 +242,7 @@ func TestWorkspaceAgentByNameParam(t *testing.T) { "agent-selected", }, }, - UrlParam: "dev.agent-selected", + URLParam: "dev.agent-selected", ExpectedAgent: "agent-selected", ExpectedStatusCode: http.StatusOK, }, @@ -261,7 +261,7 @@ func TestWorkspaceAgentByNameParam(t *testing.T) { "agent-three", }, }, - UrlParam: "dev.agent-selected", + URLParam: "dev.agent-selected", ExpectedAgent: "agent-selected", ExpectedStatusCode: http.StatusOK, }, @@ -270,12 +270,13 @@ func TestWorkspaceAgentByNameParam(t *testing.T) { 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("workspacename_and_agent", c.UrlParam) + chi.RouteContext(r.Context()).URLParams.Add("workspacename_and_agent", c.URLParam) rtr := chi.NewRouter() rtr.Use( From 712906e249522a0cbfae7c1fd5164997b55a4863 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 24 Aug 2022 16:23:52 -0400 Subject: [PATCH 6/9] Fix authorize unit test --- coderd/coderd_test.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/coderd/coderd_test.go b/coderd/coderd_test.go index c042855687cd3..c119976bd4b31 100644 --- a/coderd/coderd_test.go +++ b/coderd/coderd_test.go @@ -244,11 +244,11 @@ func TestAuthorizeAllEndpoints(t *testing.T) { "POST:/api/v2/csp/reports": {NoAuthorize: true}, "GET:/api/v2/entitlements": {NoAuthorize: true}, - "GET:/%40{user}/{workspacename}/apps/{application}/*": { + "GET:/%40{user}/{workspacename_and_agent}/apps/{application}/*": { AssertAction: rbac.ActionRead, AssertObject: workspaceRBACObj, }, - "GET:/@{user}/{workspacename}/apps/{application}/*": { + "GET:/@{user}/{workspacename_and_agent}/apps/{application}/*": { AssertAction: rbac.ActionRead, AssertObject: workspaceRBACObj, }, @@ -508,6 +508,7 @@ func TestAuthorizeAllEndpoints(t *testing.T) { route = strings.ReplaceAll(route, "{workspace}", workspace.ID.String()) route = strings.ReplaceAll(route, "{workspacebuild}", workspace.LatestBuild.ID.String()) route = strings.ReplaceAll(route, "{workspacename}", workspace.Name) + route = strings.ReplaceAll(route, "{workspacename_and_agent}", workspace.Name+"."+workspaceResources[0].Agents[0].Name) route = strings.ReplaceAll(route, "{workspacebuildname}", workspace.LatestBuild.Name) route = strings.ReplaceAll(route, "{workspaceagent}", workspaceResources[0].Agents[0].ID.String()) route = strings.ReplaceAll(route, "{buildnumber}", strconv.FormatInt(int64(workspace.LatestBuild.BuildNumber), 10)) From 40586651b4879efea915426312a9013a59914039 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 25 Aug 2022 09:14:38 -0400 Subject: [PATCH 7/9] Add test cases --- coderd/httpmw/workspaceparam_test.go | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/coderd/httpmw/workspaceparam_test.go b/coderd/httpmw/workspaceparam_test.go index d18d12a2e5b60..a2932ed1effc6 100644 --- a/coderd/httpmw/workspaceparam_test.go +++ b/coderd/httpmw/workspaceparam_test.go @@ -147,6 +147,14 @@ func TestWorkspaceAgentByNameParam(t *testing.T) { 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", @@ -232,6 +240,18 @@ func TestWorkspaceAgentByNameParam(t *testing.T) { 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", From 991448debeef47247920de476637fc04c0941100 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 26 Aug 2022 09:46:10 -0400 Subject: [PATCH 8/9] Fix auth test on all http method types --- coderd/coderdtest/authtest.go | 68 +++++++++++++++++++++-------------- 1 file changed, 42 insertions(+), 26 deletions(-) diff --git a/coderd/coderdtest/authtest.go b/coderd/coderdtest/authtest.go index 6f4673a186c01..3bed54033e9dc 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,23 +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, - "workspacename_and_agent": workspace.Name + "." + workspaceResources[0].Agents[0].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, + "{workspacename_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()), @@ -179,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_and_agent}/apps/{workspaceapp}/*": { - AssertAction: rbac.ActionCreate, - AssertObject: workspaceExecObj, - }, - "GET:/@{user}/{workspacename_and_agent}/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}, @@ -400,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}/{workspacename_and_agent}/apps/{workspaceapp}/*", RouteCheck{ + AssertAction: rbac.ActionCreate, + AssertObject: workspaceExecObj, + }) + assertAllHTTPMethods("/@{user}/{workspacename_and_agent}/apps/{workspaceapp}/*", RouteCheck{ + AssertAction: rbac.ActionCreate, + AssertObject: workspaceExecObj, + }) + return skipRoutes, assertRoute } @@ -447,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 From 2a30b23e8cd0f69dd34eb10bf13d544817494c98 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Mon, 29 Aug 2022 08:32:18 -0400 Subject: [PATCH 9/9] Rename url variable --- coderd/coderd.go | 4 +-- coderd/coderdtest/authtest.go | 38 ++++++++++++++-------------- coderd/httpmw/workspaceparam.go | 4 +-- coderd/httpmw/workspaceparam_test.go | 2 +- 4 files changed, 24 insertions(+), 24 deletions(-) diff --git a/coderd/coderd.go b/coderd/coderd.go index 90863e5a826f5..f7b8603367b5e 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -147,8 +147,8 @@ func New(options *Options) *API { // %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_and_agent}/apps/{workspaceapp}", apps) - r.Route("/@{user}/{workspacename_and_agent}/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 3bed54033e9dc..f88fd21f20849 100644 --- a/coderd/coderdtest/authtest.go +++ b/coderd/coderdtest/authtest.go @@ -120,23 +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, - "{workspacename_and_agent}": workspace.Name + "." + workspaceResources[0].Agents[0].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()), @@ -406,11 +406,11 @@ func AGPLRoutes(a *AuthTester) (map[string]string, map[string]RouteCheck) { } } - assertAllHTTPMethods("/%40{user}/{workspacename_and_agent}/apps/{workspaceapp}/*", RouteCheck{ + assertAllHTTPMethods("/%40{user}/{workspace_and_agent}/apps/{workspaceapp}/*", RouteCheck{ AssertAction: rbac.ActionCreate, AssertObject: workspaceExecObj, }) - assertAllHTTPMethods("/@{user}/{workspacename_and_agent}/apps/{workspaceapp}/*", RouteCheck{ + assertAllHTTPMethods("/@{user}/{workspace_and_agent}/apps/{workspaceapp}/*", RouteCheck{ AssertAction: rbac.ActionCreate, AssertObject: workspaceExecObj, }) diff --git a/coderd/httpmw/workspaceparam.go b/coderd/httpmw/workspaceparam.go index 79c548d156062..1ce23a20d9f40 100644 --- a/coderd/httpmw/workspaceparam.go +++ b/coderd/httpmw/workspaceparam.go @@ -54,7 +54,7 @@ func ExtractWorkspaceParam(db database.Store) func(http.Handler) http.Handler { } // ExtractWorkspaceAndAgentParam grabs a workspace and an agent from the -// "workspacename_and_agent" URL parameter. `ExtractUserParam` must be called +// "workspace_and_agent" URL parameter. `ExtractUserParam` must be called // before this. // This can be in the form of: // - ".[workspace-agent]" : If multiple agents exist @@ -63,7 +63,7 @@ func ExtractWorkspaceAndAgentParam(db database.Store) func(http.Handler) http.Ha return func(next http.Handler) http.Handler { return http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { user := UserParam(r) - workspaceWithAgent := chi.URLParam(r, "workspacename_and_agent") + workspaceWithAgent := chi.URLParam(r, "workspace_and_agent") workspaceParts := strings.Split(workspaceWithAgent, ".") workspace, err := db.GetWorkspaceByOwnerIDAndName(r.Context(), database.GetWorkspaceByOwnerIDAndNameParams{ diff --git a/coderd/httpmw/workspaceparam_test.go b/coderd/httpmw/workspaceparam_test.go index a2932ed1effc6..5b3f9a5830bc7 100644 --- a/coderd/httpmw/workspaceparam_test.go +++ b/coderd/httpmw/workspaceparam_test.go @@ -296,7 +296,7 @@ func TestWorkspaceAgentByNameParam(t *testing.T) { Agents: c.Agents, }) - chi.RouteContext(r.Context()).URLParams.Add("workspacename_and_agent", c.URLParam) + chi.RouteContext(r.Context()).URLParams.Add("workspace_and_agent", c.URLParam) rtr := chi.NewRouter() rtr.Use(