Skip to content

Commit 34d902e

Browse files
EmyrkEmyrk
authored
fix: Fix properly selecting workspace apps by agent (#3684)
1 parent dc9b415 commit 34d902e

File tree

6 files changed

+452
-103
lines changed

6 files changed

+452
-103
lines changed

‎coderd/coderd.go

+5-3
Original file line numberDiff line numberDiff line change
@@ -136,17 +136,19 @@ func New(options *Options) *API {
136136
apps := func(r chi.Router) {
137137
r.Use(
138138
httpmw.RateLimitPerMinute(options.APIRateLimit),
139+
tracing.HTTPMW(api.TracerProvider, "coderd.http"),
139140
httpmw.ExtractAPIKey(options.Database, oauthConfigs, true),
140141
httpmw.ExtractUserParam(api.Database),
141-
tracing.HTTPMW(api.TracerProvider, "coderd.http"),
142+
// Extracts the <workspace.agent> from the url
143+
httpmw.ExtractWorkspaceAndAgentParam(api.Database),
142144
)
143145
r.HandleFunc("/*", api.workspaceAppsProxyPath)
144146
}
145147
// %40 is the encoded character of the @ symbol. VS Code Web does
146148
// not handle character encoding properly, so it's safe to assume
147149
// other applications might not as well.
148-
r.Route("/%40{user}/{workspacename}/apps/{workspaceapp}", apps)
149-
r.Route("/@{user}/{workspacename}/apps/{workspaceapp}", apps)
150+
r.Route("/%40{user}/{workspace_and_agent}/apps/{workspaceapp}", apps)
151+
r.Route("/@{user}/{workspace_and_agent}/apps/{workspaceapp}", apps)
150152

151153
r.Route("/api/v2", func(r chi.Router) {
152154
r.NotFound(func(rw http.ResponseWriter, r *http.Request) {

‎coderd/coderdtest/authtest.go

+42-25
Original file line numberDiff line numberDiff line change
@@ -85,6 +85,7 @@ func NewAuthTester(ctx context.Context, t *testing.T, options *Options) *AuthTes
8585
Name: "some",
8686
Type: "example",
8787
Agents: []*proto.Agent{{
88+
Name: "agent",
8889
Id: "something",
8990
Auth: &proto.Agent_Token{},
9091
Apps: []*proto.App{{
@@ -119,22 +120,23 @@ func NewAuthTester(ctx context.Context, t *testing.T, options *Options) *AuthTes
119120
require.NoError(t, err, "create template param")
120121

121122
urlParameters := map[string]string{
122-
"{organization}": admin.OrganizationID.String(),
123-
"{user}": admin.UserID.String(),
124-
"{organizationname}": organization.Name,
125-
"{workspace}": workspace.ID.String(),
126-
"{workspacebuild}": workspace.LatestBuild.ID.String(),
127-
"{workspacename}": workspace.Name,
128-
"{workspacebuildname}": workspace.LatestBuild.Name,
129-
"{workspaceagent}": workspaceResources[0].Agents[0].ID.String(),
130-
"{buildnumber}": strconv.FormatInt(int64(workspace.LatestBuild.BuildNumber), 10),
131-
"{template}": template.ID.String(),
132-
"{hash}": file.Hash,
133-
"{workspaceresource}": workspaceResources[0].ID.String(),
134-
"{workspaceapp}": workspaceResources[0].Agents[0].Apps[0].Name,
135-
"{templateversion}": version.ID.String(),
136-
"{jobID}": templateVersionDryRun.ID.String(),
137-
"{templatename}": template.Name,
123+
"{organization}": admin.OrganizationID.String(),
124+
"{user}": admin.UserID.String(),
125+
"{organizationname}": organization.Name,
126+
"{workspace}": workspace.ID.String(),
127+
"{workspacebuild}": workspace.LatestBuild.ID.String(),
128+
"{workspacename}": workspace.Name,
129+
"{workspacebuildname}": workspace.LatestBuild.Name,
130+
"{workspaceagent}": workspaceResources[0].Agents[0].ID.String(),
131+
"{buildnumber}": strconv.FormatInt(int64(workspace.LatestBuild.BuildNumber), 10),
132+
"{template}": template.ID.String(),
133+
"{hash}": file.Hash,
134+
"{workspaceresource}": workspaceResources[0].ID.String(),
135+
"{workspaceapp}": workspaceResources[0].Agents[0].Apps[0].Name,
136+
"{templateversion}": version.ID.String(),
137+
"{jobID}": templateVersionDryRun.ID.String(),
138+
"{templatename}": template.Name,
139+
"{workspace_and_agent}": workspace.Name + "." + workspaceResources[0].Agents[0].Name,
138140
// Only checking template scoped params here
139141
"parameters/{scope}/{id}": fmt.Sprintf("parameters/%s/%s",
140142
string(templateParam.Scope), templateParam.ScopeID.String()),
@@ -178,15 +180,6 @@ func AGPLRoutes(a *AuthTester) (map[string]string, map[string]RouteCheck) {
178180
"POST:/api/v2/csp/reports": {NoAuthorize: true},
179181
"GET:/api/v2/entitlements": {NoAuthorize: true},
180182

181-
"GET:/%40{user}/{workspacename}/apps/{workspaceapp}/*": {
182-
AssertAction: rbac.ActionCreate,
183-
AssertObject: workspaceExecObj,
184-
},
185-
"GET:/@{user}/{workspacename}/apps/{workspaceapp}/*": {
186-
AssertAction: rbac.ActionCreate,
187-
AssertObject: workspaceExecObj,
188-
},
189-
190183
// Has it's own auth
191184
"GET:/api/v2/users/oauth2/github/callback": {NoAuthorize: true},
192185
"GET:/api/v2/users/oidc/callback": {NoAuthorize: true},
@@ -399,6 +392,29 @@ func AGPLRoutes(a *AuthTester) (map[string]string, map[string]RouteCheck) {
399392
"POST:/api/v2/workspaces/{workspace}/builds": {StatusCode: http.StatusBadRequest, NoAuthorize: true},
400393
"POST:/api/v2/organizations/{organization}/templateversions": {StatusCode: http.StatusBadRequest, NoAuthorize: true},
401394
}
395+
396+
// Routes like proxy routes support all HTTP methods. A helper func to expand
397+
// 1 url to all http methods.
398+
assertAllHTTPMethods := func(url string, check RouteCheck) {
399+
methods := []string{http.MethodGet, http.MethodHead, http.MethodPost,
400+
http.MethodPut, http.MethodPatch, http.MethodDelete,
401+
http.MethodConnect, http.MethodOptions, http.MethodTrace}
402+
403+
for _, method := range methods {
404+
route := method + ":" + url
405+
assertRoute[route] = check
406+
}
407+
}
408+
409+
assertAllHTTPMethods("/%40{user}/{workspace_and_agent}/apps/{workspaceapp}/*", RouteCheck{
410+
AssertAction: rbac.ActionCreate,
411+
AssertObject: workspaceExecObj,
412+
})
413+
assertAllHTTPMethods("/@{user}/{workspace_and_agent}/apps/{workspaceapp}/*", RouteCheck{
414+
AssertAction: rbac.ActionCreate,
415+
AssertObject: workspaceExecObj,
416+
})
417+
402418
return skipRoutes, assertRoute
403419
}
404420

@@ -446,6 +462,7 @@ func (a *AuthTester) Test(ctx context.Context, assertRoute map[string]RouteCheck
446462
a.t.Run(name, func(t *testing.T) {
447463
a.authorizer.reset()
448464
routeKey := strings.TrimRight(name, "/")
465+
449466
routeAssertions, ok := assertRoute[routeKey]
450467
if !ok {
451468
// By default, all omitted routes check for just "authorize" called

‎coderd/database/databasefake/databasefake.go

+2-8
Original file line numberDiff line numberDiff line change
@@ -592,14 +592,14 @@ func (q *fakeQuerier) GetLatestWorkspaceBuildByWorkspaceID(_ context.Context, wo
592592
defer q.mutex.RUnlock()
593593

594594
var row database.WorkspaceBuild
595-
var buildNum int32
595+
var buildNum int32 = -1
596596
for _, workspaceBuild := range q.workspaceBuilds {
597597
if workspaceBuild.WorkspaceID.String() == workspaceID.String() && workspaceBuild.BuildNumber > buildNum {
598598
row = workspaceBuild
599599
buildNum = workspaceBuild.BuildNumber
600600
}
601601
}
602-
if buildNum == 0 {
602+
if buildNum == -1 {
603603
return database.WorkspaceBuild{}, sql.ErrNoRows
604604
}
605605
return row, nil
@@ -1263,9 +1263,6 @@ func (q *fakeQuerier) GetWorkspaceAgentsByResourceIDs(_ context.Context, resourc
12631263
workspaceAgents = append(workspaceAgents, agent)
12641264
}
12651265
}
1266-
if len(workspaceAgents) == 0 {
1267-
return nil, sql.ErrNoRows
1268-
}
12691266
return workspaceAgents, nil
12701267
}
12711268

@@ -1347,9 +1344,6 @@ func (q *fakeQuerier) GetWorkspaceResourcesByJobID(_ context.Context, jobID uuid
13471344
}
13481345
resources = append(resources, resource)
13491346
}
1350-
if len(resources) == 0 {
1351-
return nil, sql.ErrNoRows
1352-
}
13531347
return resources, nil
13541348
}
13551349

‎coderd/httpmw/workspaceparam.go

+110
Original file line numberDiff line numberDiff line change
@@ -4,11 +4,15 @@ import (
44
"context"
55
"database/sql"
66
"errors"
7+
"fmt"
78
"net/http"
9+
"strings"
810

911
"github.com/coder/coder/coderd/database"
1012
"github.com/coder/coder/coderd/httpapi"
1113
"github.com/coder/coder/codersdk"
14+
"github.com/go-chi/chi/v5"
15+
"github.com/google/uuid"
1216
)
1317

1418
type workspaceParamContextKey struct{}
@@ -48,3 +52,109 @@ func ExtractWorkspaceParam(db database.Store) func(http.Handler) http.Handler {
4852
})
4953
}
5054
}
55+
56+
// ExtractWorkspaceAndAgentParam grabs a workspace and an agent from the
57+
// "workspace_and_agent" URL parameter. `ExtractUserParam` must be called
58+
// before this.
59+
// This can be in the form of:
60+
// - "<workspace-name>.[workspace-agent]" : If multiple agents exist
61+
// - "<workspace-name>" : If one agent exists
62+
func ExtractWorkspaceAndAgentParam(db database.Store) func(http.Handler) http.Handler {
63+
return func(next http.Handler) http.Handler {
64+
return http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) {
65+
user := UserParam(r)
66+
workspaceWithAgent := chi.URLParam(r, "workspace_and_agent")
67+
workspaceParts := strings.Split(workspaceWithAgent, ".")
68+
69+
workspace, err := db.GetWorkspaceByOwnerIDAndName(r.Context(), database.GetWorkspaceByOwnerIDAndNameParams{
70+
OwnerID: user.ID,
71+
Name: workspaceParts[0],
72+
})
73+
if err != nil {
74+
if errors.Is(err, sql.ErrNoRows) {
75+
httpapi.ResourceNotFound(rw)
76+
return
77+
}
78+
httpapi.Write(rw, http.StatusInternalServerError, codersdk.Response{
79+
Message: "Internal error fetching workspace.",
80+
Detail: err.Error(),
81+
})
82+
return
83+
}
84+
85+
build, err := db.GetLatestWorkspaceBuildByWorkspaceID(r.Context(), workspace.ID)
86+
if err != nil {
87+
httpapi.Write(rw, http.StatusInternalServerError, codersdk.Response{
88+
Message: "Internal error fetching workspace build.",
89+
Detail: err.Error(),
90+
})
91+
return
92+
}
93+
94+
resources, err := db.GetWorkspaceResourcesByJobID(r.Context(), build.JobID)
95+
if err != nil {
96+
httpapi.Write(rw, http.StatusInternalServerError, codersdk.Response{
97+
Message: "Internal error fetching workspace resources.",
98+
Detail: err.Error(),
99+
})
100+
return
101+
}
102+
resourceIDs := make([]uuid.UUID, 0)
103+
for _, resource := range resources {
104+
resourceIDs = append(resourceIDs, resource.ID)
105+
}
106+
107+
agents, err := db.GetWorkspaceAgentsByResourceIDs(r.Context(), resourceIDs)
108+
if err != nil {
109+
httpapi.Write(rw, http.StatusInternalServerError, codersdk.Response{
110+
Message: "Internal error fetching workspace agents.",
111+
Detail: err.Error(),
112+
})
113+
return
114+
}
115+
116+
if len(agents) == 0 {
117+
httpapi.Write(rw, http.StatusBadRequest, codersdk.Response{
118+
Message: "No agents exist for this workspace",
119+
})
120+
return
121+
}
122+
123+
// If we have more than 1 workspace agent, we need to specify which one to use.
124+
if len(agents) > 1 && len(workspaceParts) <= 1 {
125+
httpapi.Write(rw, http.StatusBadRequest, codersdk.Response{
126+
Message: "More than one agent exists, but no agent specified.",
127+
})
128+
return
129+
}
130+
131+
var agent database.WorkspaceAgent
132+
var found bool
133+
// If we have more than 1 workspace agent, we need to specify which one to use.
134+
// If the user specified an agent, we need to make sure that agent
135+
// actually exists.
136+
if len(workspaceParts) > 1 || len(agents) > 1 {
137+
for _, otherAgent := range agents {
138+
if otherAgent.Name == workspaceParts[1] {
139+
agent = otherAgent
140+
found = true
141+
break
142+
}
143+
}
144+
if !found {
145+
httpapi.Write(rw, http.StatusBadRequest, codersdk.Response{
146+
Message: fmt.Sprintf("No agent exists with the name %q", workspaceParts[1]),
147+
})
148+
return
149+
}
150+
} else {
151+
agent = agents[0]
152+
}
153+
154+
ctx := r.Context()
155+
ctx = context.WithValue(ctx, workspaceParamContextKey{}, workspace)
156+
ctx = context.WithValue(ctx, workspaceAgentParamContextKey{}, agent)
157+
next.ServeHTTP(rw, r.WithContext(ctx))
158+
})
159+
}
160+
}

0 commit comments

Comments
 (0)