Skip to content

Commit 6026b20

Browse files
aslilacaslilac
committed
Merge branch 'main' into lilac/x-authz-checks
2 parents 82b1085 + 0b58798 commit 6026b20

File tree

37 files changed

+819
-272
lines changed

37 files changed

+819
-272
lines changed

‎agent/agentscripts/agentscripts_test.go

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -102,6 +102,9 @@ func TestEnv(t *testing.T) {
102102

103103
func TestTimeout(t *testing.T) {
104104
t.Parallel()
105+
if runtime.GOOS == "darwin" {
106+
t.Skip("this test is flaky on macOS, see https://github.com/coder/internal/issues/329")
107+
}
105108
runner := setup(t, nil)
106109
defer runner.Close()
107110
aAPI := agenttest.NewFakeAgentAPI(t, testutil.Logger(t), nil, nil)

‎agent/agentssh/agentssh_test.go

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ import (
1313
"strings"
1414
"sync"
1515
"testing"
16+
"time"
1617

1718
"github.com/prometheus/client_golang/prometheus"
1819
"github.com/spf13/afero"
@@ -200,7 +201,11 @@ func TestNewServer_CloseActiveConnections(t *testing.T) {
200201
}
201202
assert.NoError(t, err)
202203

204+
// Allow the session to settle (i.e. reach echo).
203205
pty.ExpectMatchContext(ctx, "started")
206+
// Sleep a bit to ensure the sleep has started.
207+
time.Sleep(testutil.IntervalMedium)
208+
204209
close(ch)
205210

206211
err = sess.Wait()

‎cli/agent.go

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import (
44
"context"
55
"fmt"
66
"io"
7+
"net"
78
"net/http"
89
"net/http/pprof"
910
"net/url"
@@ -491,8 +492,6 @@ func (r *RootCmd) workspaceAgent() *serpent.Command {
491492
}
492493

493494
func ServeHandler(ctx context.Context, logger slog.Logger, handler http.Handler, addr, name string) (closeFunc func()) {
494-
logger.Debug(ctx, "http server listening", slog.F("addr", addr), slog.F("name", name))
495-
496495
// ReadHeaderTimeout is purposefully not enabled. It caused some issues with
497496
// websockets over the dev tunnel.
498497
// See: https://github.com/coder/coder/pull/3730
@@ -502,9 +501,15 @@ func ServeHandler(ctx context.Context, logger slog.Logger, handler http.Handler,
502501
Handler: handler,
503502
}
504503
go func() {
505-
err := srv.ListenAndServe()
506-
if err != nil && !xerrors.Is(err, http.ErrServerClosed) {
507-
logger.Error(ctx, "http server listen", slog.F("name", name), slog.Error(err))
504+
ln, err := net.Listen("tcp", addr)
505+
if err != nil {
506+
logger.Error(ctx, "http server listen", slog.F("name", name), slog.F("addr", addr), slog.Error(err))
507+
return
508+
}
509+
defer ln.Close()
510+
logger.Info(ctx, "http server listening", slog.F("addr", ln.Addr()), slog.F("name", name))
511+
if err := srv.Serve(ln); err != nil && !xerrors.Is(err, http.ErrServerClosed) {
512+
logger.Error(ctx, "http server serve", slog.F("addr", ln.Addr()), slog.F("name", name), slog.Error(err))
508513
}
509514
}()
510515

‎cli/server_test.go

Lines changed: 25 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ import (
2222
"os"
2323
"path/filepath"
2424
"reflect"
25+
"regexp"
2526
"runtime"
2627
"strconv"
2728
"strings"
@@ -1217,24 +1218,31 @@ func TestServer(t *testing.T) {
12171218
t.Parallel()
12181219

12191220
ctx := testutil.Context(t, testutil.WaitLong)
1220-
randPort := testutil.RandomPort(t)
1221-
inv, cfg := clitest.New(t,
1221+
inv, _ := clitest.New(t,
12221222
"server",
12231223
"--in-memory",
12241224
"--http-address", ":0",
12251225
"--access-url", "http://example.com",
12261226
"--provisioner-daemons", "1",
12271227
"--prometheus-enable",
1228-
"--prometheus-address", ":"+strconv.Itoa(randPort),
1228+
"--prometheus-address", ":0",
12291229
// "--prometheus-collect-db-metrics", // disabled by default
12301230
"--cache-dir", t.TempDir(),
12311231
)
12321232

1233+
pty := ptytest.New(t)
1234+
inv.Stdout = pty.Output()
1235+
inv.Stderr = pty.Output()
1236+
12331237
clitest.Start(t, inv)
1234-
_ = waitAccessURL(t, cfg)
1238+
1239+
// Wait until we see the prometheus address in the logs.
1240+
addrMatchExpr := `http server listening\s+addr=(\S+)\s+name=prometheus`
1241+
lineMatch := pty.ExpectRegexMatchContext(ctx, addrMatchExpr)
1242+
promAddr := regexp.MustCompile(addrMatchExpr).FindStringSubmatch(lineMatch)[1]
12351243

12361244
testutil.Eventually(ctx, t, func(ctx context.Context) bool {
1237-
req, err := http.NewRequestWithContext(ctx, "GET", fmt.Sprintf("http://127.0.0.1:%d/metrics", randPort), nil)
1245+
req, err := http.NewRequestWithContext(ctx, "GET", fmt.Sprintf("http://%s/metrics", promAddr), nil)
12381246
if err != nil {
12391247
t.Logf("error creating request: %s", err.Error())
12401248
return false
@@ -1272,24 +1280,31 @@ func TestServer(t *testing.T) {
12721280
t.Parallel()
12731281

12741282
ctx := testutil.Context(t, testutil.WaitLong)
1275-
randPort := testutil.RandomPort(t)
1276-
inv, cfg := clitest.New(t,
1283+
inv, _ := clitest.New(t,
12771284
"server",
12781285
"--in-memory",
12791286
"--http-address", ":0",
12801287
"--access-url", "http://example.com",
12811288
"--provisioner-daemons", "1",
12821289
"--prometheus-enable",
1283-
"--prometheus-address", ":"+strconv.Itoa(randPort),
1290+
"--prometheus-address", ":0",
12841291
"--prometheus-collect-db-metrics",
12851292
"--cache-dir", t.TempDir(),
12861293
)
12871294

1295+
pty := ptytest.New(t)
1296+
inv.Stdout = pty.Output()
1297+
inv.Stderr = pty.Output()
1298+
12881299
clitest.Start(t, inv)
1289-
_ = waitAccessURL(t, cfg)
1300+
1301+
// Wait until we see the prometheus address in the logs.
1302+
addrMatchExpr := `http server listening\s+addr=(\S+)\s+name=prometheus`
1303+
lineMatch := pty.ExpectRegexMatchContext(ctx, addrMatchExpr)
1304+
promAddr := regexp.MustCompile(addrMatchExpr).FindStringSubmatch(lineMatch)[1]
12901305

12911306
testutil.Eventually(ctx, t, func(ctx context.Context) bool {
1292-
req, err := http.NewRequestWithContext(ctx, "GET", fmt.Sprintf("http://127.0.0.1:%d/metrics", randPort), nil)
1307+
req, err := http.NewRequestWithContext(ctx, "GET", fmt.Sprintf("http://%s/metrics", promAddr), nil)
12931308
if err != nil {
12941309
t.Logf("error creating request: %s", err.Error())
12951310
return false

‎cli/support_test.go

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,8 @@ func TestSupportBundle(t *testing.T) {
5050
secretValue := uuid.NewString()
5151
seedSecretDeploymentOptions(t, &dc, secretValue)
5252
client, db := coderdtest.NewWithDatabase(t, &coderdtest.Options{
53-
DeploymentValues: dc.Values,
53+
DeploymentValues: dc.Values,
54+
HealthcheckTimeout: testutil.WaitSuperLong,
5455
})
5556
owner := coderdtest.CreateFirstUser(t, client)
5657
r := dbfake.WorkspaceBuild(t, db, database.WorkspaceTable{
@@ -113,7 +114,8 @@ func TestSupportBundle(t *testing.T) {
113114
secretValue := uuid.NewString()
114115
seedSecretDeploymentOptions(t, &dc, secretValue)
115116
client := coderdtest.New(t, &coderdtest.Options{
116-
DeploymentValues: dc.Values,
117+
DeploymentValues: dc.Values,
118+
HealthcheckTimeout: testutil.WaitSuperLong,
117119
})
118120
_ = coderdtest.CreateFirstUser(t, client)
119121

@@ -133,7 +135,8 @@ func TestSupportBundle(t *testing.T) {
133135
secretValue := uuid.NewString()
134136
seedSecretDeploymentOptions(t, &dc, secretValue)
135137
client, db := coderdtest.NewWithDatabase(t, &coderdtest.Options{
136-
DeploymentValues: dc.Values,
138+
DeploymentValues: dc.Values,
139+
HealthcheckTimeout: testutil.WaitSuperLong,
137140
})
138141
admin := coderdtest.CreateFirstUser(t, client)
139142
r := dbfake.WorkspaceBuild(t, db, database.WorkspaceTable{

‎coderd/coderd.go

Lines changed: 63 additions & 53 deletions
Original file line numberDiff line numberDiff line change
@@ -1155,64 +1155,74 @@ func New(options *Options) *API {
11551155
r.Get("/", api.AssignableSiteRoles)
11561156
})
11571157
r.Route("/{user}", func(r chi.Router) {
1158-
r.Use(httpmw.ExtractUserParam(options.Database))
1159-
r.Post("/convert-login", api.postConvertLoginType)
1160-
r.Delete("/", api.deleteUser)
1161-
r.Get("/", api.userByName)
1162-
r.Get("/autofill-parameters", api.userAutofillParameters)
1163-
r.Get("/login-type", api.userLoginType)
1164-
r.Put("/profile", api.putUserProfile)
1165-
r.Route("/status", func(r chi.Router) {
1166-
r.Put("/suspend", api.putSuspendUserAccount())
1167-
r.Put("/activate", api.putActivateUserAccount())
1158+
r.Group(func(r chi.Router) {
1159+
r.Use(httpmw.ExtractUserParamOptional(options.Database))
1160+
// Creating workspaces does not require permissions on the user, only the
1161+
// organization member. This endpoint should match the authz story of
1162+
// postWorkspacesByOrganization
1163+
r.Post("/workspaces", api.postUserWorkspaces)
11681164
})
1169-
r.Get("/appearance", api.userAppearanceSettings)
1170-
r.Put("/appearance", api.putUserAppearanceSettings)
1171-
r.Route("/password", func(r chi.Router) {
1172-
r.Use(httpmw.RateLimit(options.LoginRateLimit, time.Minute))
1173-
r.Put("/", api.putUserPassword)
1174-
})
1175-
// These roles apply to the site wide permissions.
1176-
r.Put("/roles", api.putUserRoles)
1177-
r.Get("/roles", api.userRoles)
1178-
1179-
r.Route("/keys", func(r chi.Router) {
1180-
r.Post("/", api.postAPIKey)
1181-
r.Route("/tokens", func(r chi.Router) {
1182-
r.Post("/", api.postToken)
1183-
r.Get("/", api.tokens)
1184-
r.Get("/tokenconfig", api.tokenConfig)
1185-
r.Route("/{keyname}", func(r chi.Router) {
1186-
r.Get("/", api.apiKeyByName)
1187-
})
1165+
1166+
r.Group(func(r chi.Router) {
1167+
r.Use(httpmw.ExtractUserParam(options.Database))
1168+
1169+
r.Post("/convert-login", api.postConvertLoginType)
1170+
r.Delete("/", api.deleteUser)
1171+
r.Get("/", api.userByName)
1172+
r.Get("/autofill-parameters", api.userAutofillParameters)
1173+
r.Get("/login-type", api.userLoginType)
1174+
r.Put("/profile", api.putUserProfile)
1175+
r.Route("/status", func(r chi.Router) {
1176+
r.Put("/suspend", api.putSuspendUserAccount())
1177+
r.Put("/activate", api.putActivateUserAccount())
11881178
})
1189-
r.Route("/{keyid}", func(r chi.Router) {
1190-
r.Get("/", api.apiKeyByID)
1191-
r.Delete("/", api.deleteAPIKey)
1179+
r.Get("/appearance", api.userAppearanceSettings)
1180+
r.Put("/appearance", api.putUserAppearanceSettings)
1181+
r.Route("/password", func(r chi.Router) {
1182+
r.Use(httpmw.RateLimit(options.LoginRateLimit, time.Minute))
1183+
r.Put("/", api.putUserPassword)
1184+
})
1185+
// These roles apply to the site wide permissions.
1186+
r.Put("/roles", api.putUserRoles)
1187+
r.Get("/roles", api.userRoles)
1188+
1189+
r.Route("/keys", func(r chi.Router) {
1190+
r.Post("/", api.postAPIKey)
1191+
r.Route("/tokens", func(r chi.Router) {
1192+
r.Post("/", api.postToken)
1193+
r.Get("/", api.tokens)
1194+
r.Get("/tokenconfig", api.tokenConfig)
1195+
r.Route("/{keyname}", func(r chi.Router) {
1196+
r.Get("/", api.apiKeyByName)
1197+
})
1198+
})
1199+
r.Route("/{keyid}", func(r chi.Router) {
1200+
r.Get("/", api.apiKeyByID)
1201+
r.Delete("/", api.deleteAPIKey)
1202+
})
11921203
})
1193-
})
11941204

1195-
r.Route("/organizations", func(r chi.Router) {
1196-
r.Get("/", api.organizationsByUser)
1197-
r.Get("/{organizationname}", api.organizationByUserAndName)
1198-
})
1199-
r.Post("/workspaces", api.postUserWorkspaces)
1200-
r.Route("/workspace/{workspacename}", func(r chi.Router) {
1201-
r.Get("/", api.workspaceByOwnerAndName)
1202-
r.Get("/builds/{buildnumber}", api.workspaceBuildByBuildNumber)
1203-
})
1204-
r.Get("/gitsshkey", api.gitSSHKey)
1205-
r.Put("/gitsshkey", api.regenerateGitSSHKey)
1206-
r.Route("/notifications", func(r chi.Router) {
1207-
r.Route("/preferences", func(r chi.Router) {
1208-
r.Get("/", api.userNotificationPreferences)
1209-
r.Put("/", api.putUserNotificationPreferences)
1205+
r.Route("/organizations", func(r chi.Router) {
1206+
r.Get("/", api.organizationsByUser)
1207+
r.Get("/{organizationname}", api.organizationByUserAndName)
1208+
})
1209+
r.Route("/workspace/{workspacename}", func(r chi.Router) {
1210+
r.Get("/", api.workspaceByOwnerAndName)
1211+
r.Get("/builds/{buildnumber}", api.workspaceBuildByBuildNumber)
1212+
})
1213+
r.Get("/gitsshkey", api.gitSSHKey)
1214+
r.Put("/gitsshkey", api.regenerateGitSSHKey)
1215+
r.Route("/notifications", func(r chi.Router) {
1216+
r.Route("/preferences", func(r chi.Router) {
1217+
r.Get("/", api.userNotificationPreferences)
1218+
r.Put("/", api.putUserNotificationPreferences)
1219+
})
1220+
})
1221+
r.Route("/webpush", func(r chi.Router) {
1222+
r.Post("/subscription", api.postUserWebpushSubscription)
1223+
r.Delete("/subscription", api.deleteUserWebpushSubscription)
1224+
r.Post("/test", api.postUserPushNotificationTest)
12101225
})
1211-
})
1212-
r.Route("/webpush", func(r chi.Router) {
1213-
r.Post("/subscription", api.postUserWebpushSubscription)
1214-
r.Delete("/subscription", api.deleteUserWebpushSubscription)
1215-
r.Post("/test", api.postUserPushNotificationTest)
12161226
})
12171227
})
12181228
})

‎coderd/coderdtest/authorize.go

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -81,7 +81,7 @@ func AssertRBAC(t *testing.T, api *coderd.API, client *codersdk.Client) RBACAsse
8181
// Note that duplicate rbac calls are handled by the rbac.Cacher(), but
8282
// will be recorded twice. So AllCalls() returns calls regardless if they
8383
// were returned from the cached or not.
84-
func (a RBACAsserter) AllCalls() []AuthCall {
84+
func (a RBACAsserter) AllCalls() AuthCalls {
8585
return a.Recorder.AllCalls(&a.Subject)
8686
}
8787

@@ -140,8 +140,11 @@ func (a RBACAsserter) Reset() RBACAsserter {
140140
return a
141141
}
142142

143+
type AuthCalls []AuthCall
144+
143145
type AuthCall struct {
144146
rbac.AuthCall
147+
Err error
145148

146149
asserted bool
147150
// callers is a small stack trace for debugging.
@@ -252,7 +255,7 @@ func (r *RecordingAuthorizer) AssertActor(t *testing.T, actor rbac.Subject, did
252255
}
253256

254257
// recordAuthorize is the internal method that records the Authorize() call.
255-
func (r *RecordingAuthorizer) recordAuthorize(subject rbac.Subject, action policy.Action, object rbac.Object) {
258+
func (r *RecordingAuthorizer) recordAuthorize(subject rbac.Subject, action policy.Action, object rbac.Object, authzErr error) {
256259
r.Lock()
257260
defer r.Unlock()
258261

@@ -262,6 +265,7 @@ func (r *RecordingAuthorizer) recordAuthorize(subject rbac.Subject, action polic
262265
Action: action,
263266
Object: object,
264267
},
268+
Err: authzErr,
265269
callers: []string{
266270
// This is a decent stack trace for debugging.
267271
// Some dbauthz calls are a bit nested, so we skip a few.
@@ -288,11 +292,12 @@ func caller(skip int) string {
288292
}
289293

290294
func (r *RecordingAuthorizer) Authorize(ctx context.Context, subject rbac.Subject, action policy.Action, object rbac.Object) error {
291-
r.recordAuthorize(subject, action, object)
292295
if r.Wrapped == nil {
293296
panic("Developer error: RecordingAuthorizer.Wrapped is nil")
294297
}
295-
return r.Wrapped.Authorize(ctx, subject, action, object)
298+
authzErr := r.Wrapped.Authorize(ctx, subject, action, object)
299+
r.recordAuthorize(subject, action, object, authzErr)
300+
return authzErr
296301
}
297302

298303
func (r *RecordingAuthorizer) Prepare(ctx context.Context, subject rbac.Subject, action policy.Action, objectType string) (rbac.PreparedAuthorized, error) {
@@ -339,10 +344,11 @@ func (s *PreparedRecorder) Authorize(ctx context.Context, object rbac.Object) er
339344
s.rw.Lock()
340345
defer s.rw.Unlock()
341346

347+
authzErr := s.prepped.Authorize(ctx, object)
342348
if !s.usingSQL {
343-
s.rec.recordAuthorize(s.subject, s.action, object)
349+
s.rec.recordAuthorize(s.subject, s.action, object, authzErr)
344350
}
345-
return s.prepped.Authorize(ctx, object)
351+
return authzErr
346352
}
347353

348354
func (s *PreparedRecorder) CompileToSQL(ctx context.Context, cfg regosql.ConvertConfig) (string, error) {

‎coderd/coderdtest/coderdtest.go

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -405,6 +405,12 @@ func NewOptions(t testing.TB, options *Options) (func(http.Handler), context.Can
405405
workspacestats.TrackerWithTickFlush(options.WorkspaceUsageTrackerTick, options.WorkspaceUsageTrackerFlush),
406406
)
407407

408+
// create the TempDir for the HTTP file cache BEFORE we start the server and set a t.Cleanup to close it. TempDir()
409+
// registers a Cleanup function that deletes the directory, and Cleanup functions are called in reverse order. If
410+
// we don't do this, then we could try to delete the directory before the HTTP server is done with all files in it,
411+
// which on Windows will fail (can't delete files until all programs have closed handles to them).
412+
cacheDir := t.TempDir()
413+
408414
var mutex sync.RWMutex
409415
var handler http.Handler
410416
srv := httptest.NewUnstartedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
@@ -515,7 +521,7 @@ func NewOptions(t testing.TB, options *Options) (func(http.Handler), context.Can
515521
AppHostname: options.AppHostname,
516522
AppHostnameRegex: appHostnameRegex,
517523
Logger: *options.Logger,
518-
CacheDir: t.TempDir(),
524+
CacheDir: cacheDir,
519525
RuntimeConfig: runtimeManager,
520526
Database: options.Database,
521527
Pubsub: options.Pubsub,

0 commit comments

Comments
 (0)