Skip to content

Commit 307e572

Browse files
deansheatherdeansheather
committed
continue tidying/splitting up workspaceapps code
1 parent ca8b74e commit 307e572

11 files changed

+247
-316
lines changed

‎coderd/coderd.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -280,7 +280,7 @@ func New(options *Options) *API {
280280
Authorizer: options.Authorizer,
281281
Logger: options.Logger,
282282
},
283-
WorkspaceAppsProvider: workspaceapps.New(
283+
WorkspaceAppsProvider: workspaceapps.NewDBTicketProvider(
284284
options.Logger.Named("workspaceapps"),
285285
options.AccessURL,
286286
options.Authorizer,
@@ -763,7 +763,7 @@ type API struct {
763763
metricsCache *metricscache.Cache
764764
workspaceAgentCache *wsconncache.Cache
765765
updateChecker *updatecheck.Checker
766-
WorkspaceAppsProvider *workspaceapps.DBTicketProvider
766+
WorkspaceAppsProvider workspaceapps.TicketProvider
767767

768768
// Experiments contains the list of experiments currently enabled.
769769
// This is used to gate features that are not yet ready for production.

‎coderd/workspaceagents.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -550,7 +550,7 @@ func (api *API) workspaceAgentPTY(rw http.ResponseWriter, r *http.Request) {
550550
api.WebsocketWaitMutex.Unlock()
551551
defer api.WebsocketWaitGroup.Done()
552552

553-
ticket, ok := api.WorkspaceAppsProvider.ResolveRequest(rw, r, workspaceapps.Request{
553+
ticket, ok := workspaceapps.ResolveRequest(api.Logger, api.AccessURL, api.WorkspaceAppsProvider, rw, r, workspaceapps.Request{
554554
AccessMethod: workspaceapps.AccessMethodTerminal,
555555
BasePath: r.URL.Path,
556556
AgentNameOrID: chi.URLParam(r, "workspaceagent"),

‎coderd/workspaceapps.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -124,7 +124,7 @@ func (api *API) workspaceAppsProxyPath(rw http.ResponseWriter, r *http.Request)
124124
chiPath = "/" + chiPath
125125
}
126126

127-
ticket, ok := api.WorkspaceAppsProvider.ResolveRequest(rw, r, workspaceapps.Request{
127+
ticket, ok := workspaceapps.ResolveRequest(api.Logger, api.AccessURL, api.WorkspaceAppsProvider, rw, r, workspaceapps.Request{
128128
AccessMethod: workspaceapps.AccessMethodPath,
129129
BasePath: basePath,
130130
UsernameOrID: chi.URLParam(r, "user"),
@@ -247,7 +247,7 @@ func (api *API) handleSubdomainApplications(middlewares ...func(http.Handler) ht
247247
return
248248
}
249249

250-
ticket, ok := api.WorkspaceAppsProvider.ResolveRequest(rw, r, workspaceapps.Request{
250+
ticket, ok := workspaceapps.ResolveRequest(api.Logger, api.AccessURL, api.WorkspaceAppsProvider, rw, r, workspaceapps.Request{
251251
AccessMethod: workspaceapps.AccessMethodSubdomain,
252252
BasePath: "/",
253253
UsernameOrID: app.Username,

‎coderd/workspaceapps/auth.go renamed to ‎coderd/workspaceapps/db.go

Lines changed: 47 additions & 156 deletions
Original file line numberDiff line numberDiff line change
@@ -5,81 +5,66 @@ import (
55
"database/sql"
66
"fmt"
77
"net/http"
8-
"strings"
8+
"net/url"
99
"time"
1010

1111
"golang.org/x/xerrors"
1212

1313
"cdr.dev/slog"
14+
1415
"github.com/coder/coder/coderd/database"
1516
"github.com/coder/coder/coderd/database/dbauthz"
1617
"github.com/coder/coder/coderd/httpapi"
1718
"github.com/coder/coder/coderd/httpmw"
1819
"github.com/coder/coder/coderd/rbac"
1920
"github.com/coder/coder/codersdk"
20-
"github.com/coder/coder/site"
2121
)
2222

23-
const (
24-
// TODO(@deansheather): configurable expiry
25-
TicketExpiry = time.Minute
26-
27-
// RedirectURIQueryParam is the query param for the app URL to be passed
28-
// back to the API auth endpoint on the main access URL.
29-
RedirectURIQueryParam = "redirect_uri"
30-
)
23+
// DBTicketProvider provides authentication and authorization for workspace apps
24+
// by querying the database if the request is missing a valid ticket.
25+
type DBTicketProvider struct {
26+
Logger slog.Logger
27+
28+
AccessURL *url.URL
29+
Authorizer rbac.Authorizer
30+
Database database.Store
31+
DeploymentValues *codersdk.DeploymentValues
32+
OAuth2Configs *httpmw.OAuth2Configs
33+
WorkspaceAgentInactiveTimeout time.Duration
34+
TicketSigningKey []byte
35+
}
3136

32-
// TODO: remove this temporary shim
33-
func (p *DBTicketProvider) ResolveRequest(rw http.ResponseWriter, r *http.Request, appReq Request) (*Ticket, bool) {
34-
// TODO: this needs to be some sort of normalize function or something
35-
if appReq.WorkspaceAndAgent != "" {
36-
// workspace.agent
37-
workspaceAndAgent := strings.SplitN(appReq.WorkspaceAndAgent, ".", 2)
38-
appReq.WorkspaceAndAgent = ""
39-
appReq.WorkspaceNameOrID = workspaceAndAgent[0]
40-
if len(workspaceAndAgent) > 1 {
41-
appReq.AgentNameOrID = workspaceAndAgent[1]
42-
}
37+
var _ TicketProvider = &DBTicketProvider{}
4338

44-
// Sanity check.
45-
err := appReq.Validate()
46-
if err != nil {
47-
p.writeWorkspaceApp500(rw, r, &appReq, err, "invalid app request")
48-
return nil, false
49-
}
39+
func NewDBTicketProvider(log slog.Logger, accessURL *url.URL, authz rbac.Authorizer, db database.Store, cfg *codersdk.DeploymentValues, oauth2Cfgs *httpmw.OAuth2Configs, workspaceAgentInactiveTimeout time.Duration, ticketSigningKey []byte) TicketProvider {
40+
if len(ticketSigningKey) != 64 {
41+
panic("ticket signing key must be 64 bytes")
5042
}
5143

52-
ticket, ok := p.TicketFromRequest(r)
53-
if ok && ticket.MatchesRequest(appReq) {
54-
// The request has a valid ticket and it matches the request.
55-
return ticket, true
44+
if workspaceAgentInactiveTimeout == 0 {
45+
workspaceAgentInactiveTimeout = 1 * time.Minute
5646
}
5747

58-
ticket, ticketStr, ok := p.CreateTicket(r.Context(), rw, r, appReq)
59-
if !ok {
60-
return nil, false
48+
return &DBTicketProvider{
49+
Logger: log,
50+
AccessURL: accessURL,
51+
Authorizer: authz,
52+
Database: db,
53+
DeploymentValues: cfg,
54+
OAuth2Configs: oauth2Cfgs,
55+
WorkspaceAgentInactiveTimeout: workspaceAgentInactiveTimeout,
56+
TicketSigningKey: ticketSigningKey,
6157
}
62-
63-
// Write the ticket cookie. We always want this to apply to the current
64-
// hostname (even for subdomain apps, without any wildcard shenanigans,
65-
// because the ticket is only valid for a single app).
66-
http.SetCookie(rw, &http.Cookie{
67-
Name: codersdk.DevURLSessionTicketCookie,
68-
Value: ticketStr,
69-
Path: appReq.BasePath,
70-
Expires: ticket.Expiry,
71-
})
72-
73-
return ticket, true
7458
}
7559

7660
func (p *DBTicketProvider) TicketFromRequest(r *http.Request) (*Ticket, bool) {
7761
// Get the existing ticket from the request.
7862
ticketCookie, err := r.Cookie(codersdk.DevURLSessionTicketCookie)
7963
if err == nil {
80-
ticket, err := p.ParseTicket(ticketCookie.Value)
64+
ticket, err := ParseTicket(p.TicketSigningKey, ticketCookie.Value)
8165
if err == nil {
82-
err := ticket.Request.Validate()
66+
req := ticket.Request.Normalize()
67+
err := req.Validate()
8368
if err == nil {
8469
// The request has a ticket, which is a valid ticket signed by
8570
// us. The caller must check that it matches the request.
@@ -105,9 +90,11 @@ func (p *DBTicketProvider) CreateTicket(ctx context.Context, rw http.ResponseWri
10590
// // logic is handled in Provider.authorizeWorkspaceApp which directly checks the actor's
10691
// // permissions.
10792
dangerousSystemCtx := dbauthz.AsSystemRestricted(ctx)
93+
94+
appReq = appReq.Normalize()
10895
err := appReq.Validate()
10996
if err != nil {
110-
p.writeWorkspaceApp500(rw, r, &appReq, err, "invalid app request")
97+
WriteWorkspaceApp500(p.Logger, p.AccessURL, rw, r, &appReq, err, "invalid app request")
11198
return nil, "", false
11299
}
113100

@@ -135,10 +122,10 @@ func (p *DBTicketProvider) CreateTicket(ctx context.Context, rw http.ResponseWri
135122
// Lookup workspace app details from DB.
136123
dbReq, err := appReq.getDatabase(dangerousSystemCtx, p.Database)
137124
if xerrors.Is(err, sql.ErrNoRows) {
138-
p.writeWorkspaceApp404(rw, r, &appReq, err.Error())
125+
WriteWorkspaceApp404(p.Logger, p.AccessURL, rw, r, &appReq, err.Error())
139126
return nil, "", false
140127
} else if err != nil {
141-
p.writeWorkspaceApp500(rw, r, &appReq, err, "get app details from database")
128+
WriteWorkspaceApp500(p.Logger, p.AccessURL, rw, r, &appReq, err, "get app details from database")
142129
return nil, "", false
143130
}
144131
ticket.UserID = dbReq.User.ID
@@ -150,14 +137,15 @@ func (p *DBTicketProvider) CreateTicket(ctx context.Context, rw http.ResponseWri
150137
// is not running.
151138

152139
// Verify the user has access to the app.
153-
authed, ok := p.verifyAuthz(rw, r, authz, dbReq)
154-
if !ok {
140+
authed, err := p.authorizeRequest(r.Context(), authz, dbReq)
141+
if err != nil {
142+
WriteWorkspaceApp500(p.Logger, p.AccessURL, rw, r, &appReq, err, "verify authz")
155143
return nil, "", false
156144
}
157145
if !authed {
158146
if apiKey != nil {
159147
// The request has a valid API key but insufficient permissions.
160-
p.writeWorkspaceApp404(rw, r, &appReq, "insufficient permissions")
148+
WriteWorkspaceApp404(p.Logger, p.AccessURL, rw, r, &appReq, "insufficient permissions")
161149
return nil, "", false
162150
}
163151

@@ -191,28 +179,28 @@ func (p *DBTicketProvider) CreateTicket(ctx context.Context, rw http.ResponseWri
191179
// Check that the agent is online.
192180
agentStatus := dbReq.Agent.Status(p.WorkspaceAgentInactiveTimeout)
193181
if agentStatus.Status != database.WorkspaceAgentStatusConnected {
194-
p.writeWorkspaceAppOffline(rw, r, &appReq, fmt.Sprintf("Agent state is %q, not %q", agentStatus.Status, database.WorkspaceAgentStatusConnected))
182+
WriteWorkspaceAppOffline(p.Logger, p.AccessURL, rw, r, &appReq, fmt.Sprintf("Agent state is %q, not %q", agentStatus.Status, database.WorkspaceAgentStatusConnected))
195183
return nil, "", false
196184
}
197185

198186
// Check that the app is healthy.
199187
if dbReq.AppHealth != "" && dbReq.AppHealth != database.WorkspaceAppHealthDisabled && dbReq.AppHealth != database.WorkspaceAppHealthHealthy {
200-
p.writeWorkspaceAppOffline(rw, r, &appReq, fmt.Sprintf("App health is %q, not %q", dbReq.AppHealth, database.WorkspaceAppHealthHealthy))
188+
WriteWorkspaceAppOffline(p.Logger, p.AccessURL, rw, r, &appReq, fmt.Sprintf("App health is %q, not %q", dbReq.AppHealth, database.WorkspaceAppHealthHealthy))
201189
return nil, "", false
202190
}
203191

204192
// As a sanity check, ensure the ticket we just made is valid for this
205193
// request.
206194
if !ticket.MatchesRequest(appReq) {
207-
p.writeWorkspaceApp500(rw, r, &appReq, nil, "fresh ticket does not match request")
195+
WriteWorkspaceApp500(p.Logger, p.AccessURL, rw, r, &appReq, nil, "fresh ticket does not match request")
208196
return nil, "", false
209197
}
210198

211199
// Sign the ticket.
212200
ticket.Expiry = time.Now().Add(TicketExpiry)
213-
ticketStr, err := p.GenerateTicket(ticket)
201+
ticketStr, err := GenerateTicket(p.TicketSigningKey, ticket)
214202
if err != nil {
215-
p.writeWorkspaceApp500(rw, r, &appReq, err, "generate ticket")
203+
WriteWorkspaceApp500(p.Logger, p.AccessURL, rw, r, &appReq, err, "generate ticket")
216204
return nil, "", false
217205
}
218206

@@ -306,100 +294,3 @@ func (p *DBTicketProvider) authorizeRequest(ctx context.Context, roles *httpmw.A
306294
// No checks were successful.
307295
return false, nil
308296
}
309-
310-
// verifyAuthz authorizes the user using api.Authorizer for a
311-
// given app share level in the given workspace. The user's authorization status
312-
// is returned. If a server error occurs, a HTML error page is rendered and
313-
// false is returned so the caller can return early.
314-
func (p *DBTicketProvider) verifyAuthz(rw http.ResponseWriter, r *http.Request, authz *httpmw.Authorization, dbReq *databaseRequest) (authed bool, ok bool) {
315-
ok, err := p.authorizeRequest(r.Context(), authz, dbReq)
316-
if err != nil {
317-
p.Logger.Error(r.Context(), "authorize workspace app", slog.Error(err))
318-
site.RenderStaticErrorPage(rw, r, site.ErrorPageData{
319-
Status: http.StatusInternalServerError,
320-
Title: "Internal Server Error",
321-
Description: "Could not verify authorization. Please try again or contact an administrator.",
322-
RetryEnabled: true,
323-
DashboardURL: p.AccessURL.String(),
324-
})
325-
return false, false
326-
}
327-
328-
return ok, true
329-
}
330-
331-
// writeWorkspaceApp404 writes a HTML 404 error page for a workspace app. If
332-
// appReq is not nil, it will be used to log the request details at debug level.
333-
func (p *DBTicketProvider) writeWorkspaceApp404(rw http.ResponseWriter, r *http.Request, appReq *Request, msg string) {
334-
if appReq != nil {
335-
slog.Helper()
336-
p.Logger.Debug(r.Context(),
337-
"workspace app 404: "+msg,
338-
slog.F("username_or_id", appReq.UsernameOrID),
339-
slog.F("workspace_and_agent", appReq.WorkspaceAndAgent),
340-
slog.F("workspace_name_or_id", appReq.WorkspaceNameOrID),
341-
slog.F("agent_name_or_id", appReq.AgentNameOrID),
342-
slog.F("app_slug_or_port", appReq.AppSlugOrPort),
343-
)
344-
}
345-
346-
site.RenderStaticErrorPage(rw, r, site.ErrorPageData{
347-
Status: http.StatusNotFound,
348-
Title: "Application Not Found",
349-
Description: "The application or workspace you are trying to access does not exist or you do not have permission to access it.",
350-
RetryEnabled: false,
351-
DashboardURL: p.AccessURL.String(),
352-
})
353-
}
354-
355-
// writeWorkspaceApp500 writes a HTML 500 error page for a workspace app. If
356-
// appReq is not nil, it's fields will be added to the logged error message.
357-
func (p *DBTicketProvider) writeWorkspaceApp500(rw http.ResponseWriter, r *http.Request, appReq *Request, err error, msg string) {
358-
slog.Helper()
359-
ctx := r.Context()
360-
if appReq != nil {
361-
slog.With(ctx,
362-
slog.F("username_or_id", appReq.UsernameOrID),
363-
slog.F("workspace_and_agent", appReq.WorkspaceAndAgent),
364-
slog.F("workspace_name_or_id", appReq.WorkspaceNameOrID),
365-
slog.F("agent_name_or_id", appReq.AgentNameOrID),
366-
slog.F("app_name_or_port", appReq.AppSlugOrPort),
367-
)
368-
}
369-
p.Logger.Warn(ctx,
370-
"workspace app auth server error: "+msg,
371-
slog.Error(err),
372-
)
373-
374-
site.RenderStaticErrorPage(rw, r, site.ErrorPageData{
375-
Status: http.StatusInternalServerError,
376-
Title: "Internal Server Error",
377-
Description: "An internal server error occurred.",
378-
RetryEnabled: false,
379-
DashboardURL: p.AccessURL.String(),
380-
})
381-
}
382-
383-
// writeWorkspaceAppOffline writes a HTML 502 error page for a workspace app. If
384-
// appReq is not nil, it will be used to log the request details at debug level.
385-
func (p *DBTicketProvider) writeWorkspaceAppOffline(rw http.ResponseWriter, r *http.Request, appReq *Request, msg string) {
386-
if appReq != nil {
387-
slog.Helper()
388-
p.Logger.Debug(r.Context(),
389-
"workspace app unavailable: "+msg,
390-
slog.F("username_or_id", appReq.UsernameOrID),
391-
slog.F("workspace_and_agent", appReq.WorkspaceAndAgent),
392-
slog.F("workspace_name_or_id", appReq.WorkspaceNameOrID),
393-
slog.F("agent_name_or_id", appReq.AgentNameOrID),
394-
slog.F("app_slug_or_port", appReq.AppSlugOrPort),
395-
)
396-
}
397-
398-
site.RenderStaticErrorPage(rw, r, site.ErrorPageData{
399-
Status: http.StatusBadGateway,
400-
Title: "Application Unavailable",
401-
Description: msg,
402-
RetryEnabled: true,
403-
DashboardURL: p.AccessURL.String(),
404-
})
405-
}

0 commit comments

Comments
 (0)