From 11c20d694e1dda3eff777100352d919c62f4fa09 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Tue, 15 Aug 2023 14:04:09 +0100 Subject: [PATCH 01/18] use dbtestutil in httpmw test, refactor --- coderd/httpmw/workspaceagent_test.go | 116 ++++++++++++++++----------- 1 file changed, 69 insertions(+), 47 deletions(-) diff --git a/coderd/httpmw/workspaceagent_test.go b/coderd/httpmw/workspaceagent_test.go index 5b50aa14b4802..a100fad7cf8f0 100644 --- a/coderd/httpmw/workspaceagent_test.go +++ b/coderd/httpmw/workspaceagent_test.go @@ -10,8 +10,8 @@ import ( "github.com/stretchr/testify/require" "github.com/coder/coder/coderd/database" - "github.com/coder/coder/coderd/database/dbfake" "github.com/coder/coder/coderd/database/dbgen" + "github.com/coder/coder/coderd/database/dbtestutil" "github.com/coder/coder/coderd/httpmw" "github.com/coder/coder/codersdk" ) @@ -19,26 +19,22 @@ import ( func TestWorkspaceAgent(t *testing.T) { t.Parallel() - setup := func(db database.Store, token uuid.UUID) *http.Request { - r := httptest.NewRequest("GET", "/", nil) - r.Header.Set(codersdk.SessionTokenHeader, token.String()) - return r - } - t.Run("None", func(t *testing.T) { t.Parallel() - db := dbfake.New() - rtr := chi.NewRouter() - rtr.Use( - httpmw.ExtractWorkspaceAgent(httpmw.ExtractWorkspaceAgentConfig{ + db, pubsub := dbtestutil.NewDB(t) + t.Cleanup(func() { + _ = pubsub.Close() + }) + + req, rtr := setup(t, db, uuid.New(), httpmw.ExtractWorkspaceAgent( + httpmw.ExtractWorkspaceAgentConfig{ DB: db, Optional: false, - }), - ) - rtr.Get("/", nil) - r := setup(db, uuid.New()) + })) + rw := httptest.NewRecorder() - rtr.ServeHTTP(rw, r) + req.Header.Set(codersdk.SessionTokenHeader, uuid.New().String()) + rtr.ServeHTTP(rw, req) res := rw.Result() defer res.Body.Close() @@ -47,42 +43,68 @@ func TestWorkspaceAgent(t *testing.T) { t.Run("Found", func(t *testing.T) { t.Parallel() - db := dbfake.New() - var ( - user = dbgen.User(t, db, database.User{}) - workspace = dbgen.Workspace(t, db, database.Workspace{ - OwnerID: user.ID, - }) - job = dbgen.ProvisionerJob(t, db, database.ProvisionerJob{}) - resource = dbgen.WorkspaceResource(t, db, database.WorkspaceResource{ - JobID: job.ID, - }) - _ = dbgen.WorkspaceBuild(t, db, database.WorkspaceBuild{ - WorkspaceID: workspace.ID, - JobID: job.ID, - }) - agent = dbgen.WorkspaceAgent(t, db, database.WorkspaceAgent{ - ResourceID: resource.ID, - }) - ) - - rtr := chi.NewRouter() - rtr.Use( - httpmw.ExtractWorkspaceAgent(httpmw.ExtractWorkspaceAgentConfig{ + db, pubsub := dbtestutil.NewDB(t) + t.Cleanup(func() { + _ = pubsub.Close() + }) + authToken := uuid.New() + req, rtr := setup(t, db, authToken, httpmw.ExtractWorkspaceAgent( + httpmw.ExtractWorkspaceAgentConfig{ DB: db, Optional: false, - }), - ) - rtr.Get("/", func(rw http.ResponseWriter, r *http.Request) { - _ = httpmw.WorkspaceAgent(r) - rw.WriteHeader(http.StatusOK) - }) - r := setup(db, agent.AuthToken) + })) + rw := httptest.NewRecorder() - rtr.ServeHTTP(rw, r) + req.Header.Set(codersdk.SessionTokenHeader, authToken.String()) + rtr.ServeHTTP(rw, req) res := rw.Result() - defer res.Body.Close() + t.Cleanup(func() { _ = res.Body.Close() }) require.Equal(t, http.StatusOK, res.StatusCode) }) } + +func setup(t testing.TB, db database.Store, authToken uuid.UUID, mw func(http.Handler) http.Handler) (*http.Request, http.Handler) { + t.Helper() + org := dbgen.Organization(t, db, database.Organization{}) + user := dbgen.User(t, db, database.User{}) + templateVersion := dbgen.TemplateVersion(t, db, database.TemplateVersion{ + OrganizationID: org.ID, + CreatedBy: user.ID, + }) + template := dbgen.Template(t, db, database.Template{ + OrganizationID: org.ID, + ActiveVersionID: templateVersion.ID, + CreatedBy: user.ID, + }) + workspace := dbgen.Workspace(t, db, database.Workspace{ + OwnerID: user.ID, + OrganizationID: org.ID, + TemplateID: template.ID, + }) + job := dbgen.ProvisionerJob(t, db, database.ProvisionerJob{ + OrganizationID: org.ID, + }) + resource := dbgen.WorkspaceResource(t, db, database.WorkspaceResource{ + JobID: job.ID, + }) + _ = dbgen.WorkspaceBuild(t, db, database.WorkspaceBuild{ + WorkspaceID: workspace.ID, + JobID: job.ID, + TemplateVersionID: templateVersion.ID, + }) + _ = dbgen.WorkspaceAgent(t, db, database.WorkspaceAgent{ + ResourceID: resource.ID, + AuthToken: authToken, + }) + + req := httptest.NewRequest("GET", "/", nil) + rtr := chi.NewRouter() + rtr.Use(mw) + rtr.Get("/", func(rw http.ResponseWriter, r *http.Request) { + _ = httpmw.WorkspaceAgent(r) + rw.WriteHeader(http.StatusOK) + }) + + return req, rtr +} From da03c4bb93ae7220667595d32ca1bd98807cefcc Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Tue, 15 Aug 2023 15:35:45 +0100 Subject: [PATCH 02/18] add new query --- coderd/database/dbauthz/dbauthz.go | 8 ++ coderd/database/dbfake/dbfake.go | 42 ++++++++ coderd/database/dbmetrics/dbmetrics.go | 7 ++ coderd/database/dbmock/dbmock.go | 15 +++ coderd/database/querier.go | 1 + coderd/database/queries.sql.go | 112 ++++++++++++++++++++ coderd/database/queries/workspaceagents.sql | 54 ++++++++++ coderd/httpmw/workspaceagent.go | 23 ++-- 8 files changed, 250 insertions(+), 12 deletions(-) diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go index 38766121c443c..850c87187d4f2 100644 --- a/coderd/database/dbauthz/dbauthz.go +++ b/coderd/database/dbauthz/dbauthz.go @@ -1474,6 +1474,14 @@ func (q *querier) GetUsersByIDs(ctx context.Context, ids []uuid.UUID) ([]databas return q.db.GetUsersByIDs(ctx, ids) } +func (q *querier) GetWorkspaceAgentAndOwnerByAuthToken(ctx context.Context, agentID uuid.UUID) (database.GetWorkspaceAgentAndOwnerByAuthTokenRow, error) { + // This is a system function + if err := q.authorizeContext(ctx, rbac.ActionRead, rbac.ResourceSystem); err != nil { + return database.GetWorkspaceAgentAndOwnerByAuthTokenRow{}, err + } + return q.db.GetWorkspaceAgentAndOwnerByAuthToken(ctx, agentID) +} + // GetWorkspaceAgentByAuthToken is used in http middleware to get the workspace agent. // This should only be used by a system user in that middleware. func (q *querier) GetWorkspaceAgentByAuthToken(ctx context.Context, authToken uuid.UUID) (database.WorkspaceAgent, error) { diff --git a/coderd/database/dbfake/dbfake.go b/coderd/database/dbfake/dbfake.go index 818656496184f..45c96eb3906b1 100644 --- a/coderd/database/dbfake/dbfake.go +++ b/coderd/database/dbfake/dbfake.go @@ -651,6 +651,48 @@ func (q *FakeQuerier) isEveryoneGroup(id uuid.UUID) bool { return false } +func (q *FakeQuerier) GetWorkspaceAgentAndOwnerByAuthToken(ctx context.Context, authToken uuid.UUID) (database.GetWorkspaceAgentAndOwnerByAuthTokenRow, error) { + q.mutex.RLock() + defer q.mutex.RUnlock() + var resp database.GetWorkspaceAgentAndOwnerByAuthTokenRow + var found bool + for _, agt := range q.workspaceAgents { + if agt.AuthToken == authToken { + resp.WorkspaceAgent = agt + found = true + break + } + } + if !found { + return resp, sql.ErrNoRows + } + + // get the related workspace and user + for _, res := range q.workspaceResources { + if resp.WorkspaceAgent.ResourceID != res.ID { + continue + } + for _, build := range q.workspaceBuilds { + if build.JobID != res.JobID { + continue + } + for _, ws := range q.workspaces { + if build.WorkspaceID != ws.ID { + continue + } + resp.WorkspaceID = ws.ID + if usr, err := q.getUserByIDNoLock(ws.OwnerID); err == nil { + resp.OwnerID = usr.ID + resp.OwnerRoles = usr.RBACRoles + resp.OwnerName = usr.Username + return resp, nil + } + } + } + } + return database.GetWorkspaceAgentAndOwnerByAuthTokenRow{}, sql.ErrNoRows +} + func (*FakeQuerier) AcquireLock(_ context.Context, _ int64) error { return xerrors.New("AcquireLock must only be called within a transaction") } diff --git a/coderd/database/dbmetrics/dbmetrics.go b/coderd/database/dbmetrics/dbmetrics.go index f2c98f6594c43..e03ea9292804e 100644 --- a/coderd/database/dbmetrics/dbmetrics.go +++ b/coderd/database/dbmetrics/dbmetrics.go @@ -781,6 +781,13 @@ func (m metricsStore) GetUsersByIDs(ctx context.Context, ids []uuid.UUID) ([]dat return users, err } +func (m metricsStore) GetWorkspaceAgentAndOwnerByAuthToken(ctx context.Context, authToken uuid.UUID) (database.GetWorkspaceAgentAndOwnerByAuthTokenRow, error) { + start := time.Now() + r0, r1 := m.s.GetWorkspaceAgentAndOwnerByAuthToken(ctx, authToken) + m.queryLatencies.WithLabelValues("GetWorkspaceAgentAndOwnerByAuthToken").Observe(time.Since(start).Seconds()) + return r0, r1 +} + func (m metricsStore) GetWorkspaceAgentByAuthToken(ctx context.Context, authToken uuid.UUID) (database.WorkspaceAgent, error) { start := time.Now() agent, err := m.s.GetWorkspaceAgentByAuthToken(ctx, authToken) diff --git a/coderd/database/dbmock/dbmock.go b/coderd/database/dbmock/dbmock.go index 45db2e6bd4f58..8d2c018f543ea 100644 --- a/coderd/database/dbmock/dbmock.go +++ b/coderd/database/dbmock/dbmock.go @@ -1736,6 +1736,21 @@ func (mr *MockStoreMockRecorder) GetWorkspaceAgentStatsAndLabels(arg0, arg1 inte return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetWorkspaceAgentStatsAndLabels", reflect.TypeOf((*MockStore)(nil).GetWorkspaceAgentStatsAndLabels), arg0, arg1) } +// GetWorkspaceAgentWithOwnerByAgentToken mocks base method. +func (m *MockStore) GetWorkspaceAgentWithOwnerByAgentToken(arg0 context.Context, arg1 uuid.UUID) (database.GetWorkspaceAgentWithOwnerByAgentTokenRow, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "GetWorkspaceAgentWithOwnerByAgentToken", arg0, arg1) + ret0, _ := ret[0].(database.GetWorkspaceAgentWithOwnerByAgentTokenRow) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// GetWorkspaceAgentWithOwnerByAgentToken indicates an expected call of GetWorkspaceAgentWithOwnerByAgentToken. +func (mr *MockStoreMockRecorder) GetWorkspaceAgentWithOwnerByAgentToken(arg0, arg1 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetWorkspaceAgentWithOwnerByAgentToken", reflect.TypeOf((*MockStore)(nil).GetWorkspaceAgentWithOwnerByAgentToken), arg0, arg1) +} + // GetWorkspaceAgentsByResourceIDs mocks base method. func (m *MockStore) GetWorkspaceAgentsByResourceIDs(arg0 context.Context, arg1 []uuid.UUID) ([]database.WorkspaceAgent, error) { m.ctrl.T.Helper() diff --git a/coderd/database/querier.go b/coderd/database/querier.go index 3a8f97307114d..4307b0effd7c8 100644 --- a/coderd/database/querier.go +++ b/coderd/database/querier.go @@ -151,6 +151,7 @@ type sqlcQuerier interface { // to look up references to actions. eg. a user could build a workspace // for another user, then be deleted... we still want them to appear! GetUsersByIDs(ctx context.Context, ids []uuid.UUID) ([]User, error) + GetWorkspaceAgentAndOwnerByAuthToken(ctx context.Context, authToken uuid.UUID) (GetWorkspaceAgentAndOwnerByAuthTokenRow, error) GetWorkspaceAgentByAuthToken(ctx context.Context, authToken uuid.UUID) (WorkspaceAgent, error) GetWorkspaceAgentByID(ctx context.Context, id uuid.UUID) (WorkspaceAgent, error) GetWorkspaceAgentByInstanceID(ctx context.Context, authInstanceID string) (WorkspaceAgent, error) diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index 1a8c7598f7f59..7f6af1d3f0db4 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -6248,6 +6248,118 @@ func (q *sqlQuerier) DeleteOldWorkspaceAgentLogs(ctx context.Context) error { return err } +const getWorkspaceAgentAndOwnerByAuthToken = `-- name: GetWorkspaceAgentAndOwnerByAuthToken :one +SELECT + workspace_agents.id, workspace_agents.created_at, workspace_agents.updated_at, workspace_agents.name, workspace_agents.first_connected_at, workspace_agents.last_connected_at, workspace_agents.disconnected_at, workspace_agents.resource_id, workspace_agents.auth_token, workspace_agents.auth_instance_id, workspace_agents.architecture, workspace_agents.environment_variables, workspace_agents.operating_system, workspace_agents.startup_script, workspace_agents.instance_metadata, workspace_agents.resource_metadata, workspace_agents.directory, workspace_agents.version, workspace_agents.last_connected_replica_id, workspace_agents.connection_timeout_seconds, workspace_agents.troubleshooting_url, workspace_agents.motd_file, workspace_agents.lifecycle_state, workspace_agents.startup_script_timeout_seconds, workspace_agents.expanded_directory, workspace_agents.shutdown_script, workspace_agents.shutdown_script_timeout_seconds, workspace_agents.logs_length, workspace_agents.logs_overflowed, workspace_agents.startup_script_behavior, workspace_agents.started_at, workspace_agents.ready_at, workspace_agents.subsystems, + workspaces.id AS workspace_id, + users.id AS owner_id, + users.username AS owner_name, + users.status AS owner_status, + array_cat( + -- All users are members + array_append(users.rbac_roles, 'member'), + ( + SELECT + array_agg(org_roles) + FROM + organization_members, + -- All org_members get the org-member role for their orgs + unnest( + array_append(roles, 'organization-member:' || organization_members.organization_id::text) + ) AS org_roles + WHERE + user_id = users.id + ) + ) :: text[] AS owner_roles, + ( + SELECT + array_agg( + group_members.group_id :: text + ) + FROM + group_members + WHERE + user_id = users.id + ) :: text[] AS owner_groups +FROM users + INNER JOIN + workspaces + ON + workspaces.owner_id = users.id + INNER JOIN + workspace_builds + ON + workspace_builds.workspace_id = workspaces.id + INNER JOIN + workspace_resources + ON + workspace_resources.job_id = workspace_builds.job_id + INNER JOIN + workspace_agents + ON + workspace_agents.resource_id = workspace_resources.id +WHERE + workspace_agents.auth_token = $1 +LIMIT 1 +` + +type GetWorkspaceAgentAndOwnerByAuthTokenRow struct { + WorkspaceAgent WorkspaceAgent `db:"workspace_agent" json:"workspace_agent"` + WorkspaceID uuid.UUID `db:"workspace_id" json:"workspace_id"` + OwnerID uuid.UUID `db:"owner_id" json:"owner_id"` + OwnerName string `db:"owner_name" json:"owner_name"` + OwnerStatus UserStatus `db:"owner_status" json:"owner_status"` + OwnerRoles []string `db:"owner_roles" json:"owner_roles"` + OwnerGroups []string `db:"owner_groups" json:"owner_groups"` +} + +func (q *sqlQuerier) GetWorkspaceAgentAndOwnerByAuthToken(ctx context.Context, authToken uuid.UUID) (GetWorkspaceAgentAndOwnerByAuthTokenRow, error) { + row := q.db.QueryRowContext(ctx, getWorkspaceAgentAndOwnerByAuthToken, authToken) + var i GetWorkspaceAgentAndOwnerByAuthTokenRow + err := row.Scan( + &i.WorkspaceAgent.ID, + &i.WorkspaceAgent.CreatedAt, + &i.WorkspaceAgent.UpdatedAt, + &i.WorkspaceAgent.Name, + &i.WorkspaceAgent.FirstConnectedAt, + &i.WorkspaceAgent.LastConnectedAt, + &i.WorkspaceAgent.DisconnectedAt, + &i.WorkspaceAgent.ResourceID, + &i.WorkspaceAgent.AuthToken, + &i.WorkspaceAgent.AuthInstanceID, + &i.WorkspaceAgent.Architecture, + &i.WorkspaceAgent.EnvironmentVariables, + &i.WorkspaceAgent.OperatingSystem, + &i.WorkspaceAgent.StartupScript, + &i.WorkspaceAgent.InstanceMetadata, + &i.WorkspaceAgent.ResourceMetadata, + &i.WorkspaceAgent.Directory, + &i.WorkspaceAgent.Version, + &i.WorkspaceAgent.LastConnectedReplicaID, + &i.WorkspaceAgent.ConnectionTimeoutSeconds, + &i.WorkspaceAgent.TroubleshootingURL, + &i.WorkspaceAgent.MOTDFile, + &i.WorkspaceAgent.LifecycleState, + &i.WorkspaceAgent.StartupScriptTimeoutSeconds, + &i.WorkspaceAgent.ExpandedDirectory, + &i.WorkspaceAgent.ShutdownScript, + &i.WorkspaceAgent.ShutdownScriptTimeoutSeconds, + &i.WorkspaceAgent.LogsLength, + &i.WorkspaceAgent.LogsOverflowed, + &i.WorkspaceAgent.StartupScriptBehavior, + &i.WorkspaceAgent.StartedAt, + &i.WorkspaceAgent.ReadyAt, + pq.Array(&i.WorkspaceAgent.Subsystems), + &i.WorkspaceID, + &i.OwnerID, + &i.OwnerName, + &i.OwnerStatus, + pq.Array(&i.OwnerRoles), + pq.Array(&i.OwnerGroups), + ) + return i, err +} + const getWorkspaceAgentByAuthToken = `-- name: GetWorkspaceAgentByAuthToken :one SELECT id, created_at, updated_at, name, first_connected_at, last_connected_at, disconnected_at, resource_id, auth_token, auth_instance_id, architecture, environment_variables, operating_system, startup_script, instance_metadata, resource_metadata, directory, version, last_connected_replica_id, connection_timeout_seconds, troubleshooting_url, motd_file, lifecycle_state, startup_script_timeout_seconds, expanded_directory, shutdown_script, shutdown_script_timeout_seconds, logs_length, logs_overflowed, startup_script_behavior, started_at, ready_at, subsystems diff --git a/coderd/database/queries/workspaceagents.sql b/coderd/database/queries/workspaceagents.sql index dcc15081615e2..ebd44bd98591a 100644 --- a/coderd/database/queries/workspaceagents.sql +++ b/coderd/database/queries/workspaceagents.sql @@ -200,3 +200,57 @@ WHERE WHERE wb.workspace_id = @workspace_id :: uuid ); + +-- name: GetWorkspaceAgentAndOwnerByAuthToken :one +SELECT + sqlc.embed(workspace_agents), + workspaces.id AS workspace_id, + users.id AS owner_id, + users.username AS owner_name, + users.status AS owner_status, + array_cat( + -- All users are members + array_append(users.rbac_roles, 'member'), + ( + SELECT + array_agg(org_roles) + FROM + organization_members, + -- All org_members get the org-member role for their orgs + unnest( + array_append(roles, 'organization-member:' || organization_members.organization_id::text) + ) AS org_roles + WHERE + user_id = users.id + ) + ) :: text[] AS owner_roles, + ( + SELECT + array_agg( + group_members.group_id :: text + ) + FROM + group_members + WHERE + user_id = users.id + ) :: text[] AS owner_groups +FROM users + INNER JOIN + workspaces + ON + workspaces.owner_id = users.id + INNER JOIN + workspace_builds + ON + workspace_builds.workspace_id = workspaces.id + INNER JOIN + workspace_resources + ON + workspace_resources.job_id = workspace_builds.job_id + INNER JOIN + workspace_agents + ON + workspace_agents.resource_id = workspace_resources.id +WHERE + workspace_agents.auth_token = @auth_token +LIMIT 1; diff --git a/coderd/httpmw/workspaceagent.go b/coderd/httpmw/workspaceagent.go index f039c6bbf7afb..d7d1405d2471c 100644 --- a/coderd/httpmw/workspaceagent.go +++ b/coderd/httpmw/workspaceagent.go @@ -74,8 +74,9 @@ func ExtractWorkspaceAgent(opts ExtractWorkspaceAgentConfig) func(http.Handler) }) return } + //nolint:gocritic // System needs to be able to get workspace agents. - agent, err := opts.DB.GetWorkspaceAgentByAuthToken(dbauthz.AsSystemRestricted(ctx), token) + row, err := opts.DB.GetWorkspaceAgentAndOwnerByAuthToken(dbauthz.AsSystemRestricted(ctx), token) if err != nil { if errors.Is(err, sql.ErrNoRows) { optionalWrite(http.StatusUnauthorized, codersdk.Response{ @@ -86,23 +87,21 @@ func ExtractWorkspaceAgent(opts ExtractWorkspaceAgentConfig) func(http.Handler) } httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ - Message: "Internal error fetching workspace agent.", + Message: "Internal error authorizing workspace agent.", Detail: err.Error(), }) return } - //nolint:gocritic // System needs to be able to get workspace agents. - subject, err := getAgentSubject(dbauthz.AsSystemRestricted(ctx), opts.DB, agent) - if err != nil { - httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ - Message: "Internal error fetching workspace agent.", - Detail: err.Error(), - }) - return - } + subject := rbac.Subject{ + ID: row.OwnerID.String(), + Roles: rbac.RoleNames(row.OwnerRoles), + Groups: row.OwnerGroups, + // Note: this is generated as a NullUUID even though it shouldn't be nullable based on the query. + Scope: rbac.WorkspaceAgentScope(row.WorkspaceID, row.OwnerID), + }.WithCachedASTValue() - ctx = context.WithValue(ctx, workspaceAgentContextKey{}, agent) + ctx = context.WithValue(ctx, workspaceAgentContextKey{}, row.WorkspaceAgent) // Also set the dbauthz actor for the request. ctx = dbauthz.As(ctx, subject) next.ServeHTTP(rw, r.WithContext(ctx)) From 317537ca4fd8a1418d2a1baaa77c51b05173c3a9 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Tue, 15 Aug 2023 15:37:15 +0100 Subject: [PATCH 03/18] remove old query --- coderd/database/dbauthz/dbauthz.go | 9 ---- coderd/database/dbauthz/dbauthz_test.go | 4 -- coderd/database/dbfake/dbfake.go | 42 ++++++++++++++--- coderd/database/dbmetrics/dbmetrics.go | 7 --- coderd/database/querier.go | 1 - coderd/database/queries.sql.go | 52 --------------------- coderd/database/queries/workspaceagents.sql | 10 ---- coderd/httpmw/workspaceagent.go | 33 +------------ 8 files changed, 36 insertions(+), 122 deletions(-) diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go index 850c87187d4f2..ea78c26ae1099 100644 --- a/coderd/database/dbauthz/dbauthz.go +++ b/coderd/database/dbauthz/dbauthz.go @@ -1482,15 +1482,6 @@ func (q *querier) GetWorkspaceAgentAndOwnerByAuthToken(ctx context.Context, agen return q.db.GetWorkspaceAgentAndOwnerByAuthToken(ctx, agentID) } -// GetWorkspaceAgentByAuthToken is used in http middleware to get the workspace agent. -// This should only be used by a system user in that middleware. -func (q *querier) GetWorkspaceAgentByAuthToken(ctx context.Context, authToken uuid.UUID) (database.WorkspaceAgent, error) { - if err := q.authorizeContext(ctx, rbac.ActionRead, rbac.ResourceSystem); err != nil { - return database.WorkspaceAgent{}, err - } - return q.db.GetWorkspaceAgentByAuthToken(ctx, authToken) -} - func (q *querier) GetWorkspaceAgentByID(ctx context.Context, id uuid.UUID) (database.WorkspaceAgent, error) { if _, err := q.GetWorkspaceByAgentID(ctx, id); err != nil { return database.WorkspaceAgent{}, err diff --git a/coderd/database/dbauthz/dbauthz_test.go b/coderd/database/dbauthz/dbauthz_test.go index d6ad41f51408a..b6a76f6bd7c65 100644 --- a/coderd/database/dbauthz/dbauthz_test.go +++ b/coderd/database/dbauthz/dbauthz_test.go @@ -1319,10 +1319,6 @@ func (s *MethodTestSuite) TestSystemFunctions() { dbgen.WorkspaceBuild(s.T(), db, database.WorkspaceBuild{}) check.Args().Asserts(rbac.ResourceSystem, rbac.ActionRead) })) - s.Run("GetWorkspaceAgentByAuthToken", s.Subtest(func(db database.Store, check *expects) { - agt := dbgen.WorkspaceAgent(s.T(), db, database.WorkspaceAgent{}) - check.Args(agt.AuthToken).Asserts(rbac.ResourceSystem, rbac.ActionRead).Returns(agt) - })) s.Run("GetActiveUserCount", s.Subtest(func(db database.Store, check *expects) { check.Args().Asserts(rbac.ResourceSystem, rbac.ActionRead).Returns(int64(0)) })) diff --git a/coderd/database/dbfake/dbfake.go b/coderd/database/dbfake/dbfake.go index 45c96eb3906b1..3413b2b9f0fee 100644 --- a/coderd/database/dbfake/dbfake.go +++ b/coderd/database/dbfake/dbfake.go @@ -2833,18 +2833,46 @@ func (q *FakeQuerier) GetUsersByIDs(_ context.Context, ids []uuid.UUID) ([]datab return users, nil } -func (q *FakeQuerier) GetWorkspaceAgentByAuthToken(_ context.Context, authToken uuid.UUID) (database.WorkspaceAgent, error) { +func (q *FakeQuerier) GetWorkspaceAgentAndOwnerByAuthToken(ctx context.Context, authToken uuid.UUID) (database.GetWorkspaceAgentAndOwnerByAuthTokenRow, error) { q.mutex.RLock() defer q.mutex.RUnlock() + var resp database.GetWorkspaceAgentAndOwnerByAuthTokenRow + var found bool + for _, agt := range q.workspaceAgents { + if agt.AuthToken == authToken { + resp.WorkspaceAgent = agt + found = true + break + } + } + if !found { + return resp, sql.ErrNoRows + } - // The schema sorts this by created at, so we iterate the array backwards. - for i := len(q.workspaceAgents) - 1; i >= 0; i-- { - agent := q.workspaceAgents[i] - if agent.AuthToken == authToken { - return agent, nil + // get the related workspace and user + for _, res := range q.workspaceResources { + if resp.WorkspaceAgent.ResourceID != res.ID { + continue + } + for _, build := range q.workspaceBuilds { + if build.JobID != res.JobID { + continue + } + for _, ws := range q.workspaces { + if build.WorkspaceID != ws.ID { + continue + } + resp.WorkspaceID = ws.ID + if usr, err := q.getUserByIDNoLock(ws.OwnerID); err == nil { + resp.OwnerID = usr.ID + resp.OwnerRoles = usr.RBACRoles + resp.OwnerName = usr.Username + return resp, nil + } + } } } - return database.WorkspaceAgent{}, sql.ErrNoRows + return database.GetWorkspaceAgentAndOwnerByAuthTokenRow{}, sql.ErrNoRows } func (q *FakeQuerier) GetWorkspaceAgentByID(ctx context.Context, id uuid.UUID) (database.WorkspaceAgent, error) { diff --git a/coderd/database/dbmetrics/dbmetrics.go b/coderd/database/dbmetrics/dbmetrics.go index e03ea9292804e..580bfff9fe9db 100644 --- a/coderd/database/dbmetrics/dbmetrics.go +++ b/coderd/database/dbmetrics/dbmetrics.go @@ -788,13 +788,6 @@ func (m metricsStore) GetWorkspaceAgentAndOwnerByAuthToken(ctx context.Context, return r0, r1 } -func (m metricsStore) GetWorkspaceAgentByAuthToken(ctx context.Context, authToken uuid.UUID) (database.WorkspaceAgent, error) { - start := time.Now() - agent, err := m.s.GetWorkspaceAgentByAuthToken(ctx, authToken) - m.queryLatencies.WithLabelValues("GetWorkspaceAgentByAuthToken").Observe(time.Since(start).Seconds()) - return agent, err -} - func (m metricsStore) GetWorkspaceAgentByID(ctx context.Context, id uuid.UUID) (database.WorkspaceAgent, error) { start := time.Now() agent, err := m.s.GetWorkspaceAgentByID(ctx, id) diff --git a/coderd/database/querier.go b/coderd/database/querier.go index 4307b0effd7c8..6ddaeffe2d9ff 100644 --- a/coderd/database/querier.go +++ b/coderd/database/querier.go @@ -152,7 +152,6 @@ type sqlcQuerier interface { // for another user, then be deleted... we still want them to appear! GetUsersByIDs(ctx context.Context, ids []uuid.UUID) ([]User, error) GetWorkspaceAgentAndOwnerByAuthToken(ctx context.Context, authToken uuid.UUID) (GetWorkspaceAgentAndOwnerByAuthTokenRow, error) - GetWorkspaceAgentByAuthToken(ctx context.Context, authToken uuid.UUID) (WorkspaceAgent, error) GetWorkspaceAgentByID(ctx context.Context, id uuid.UUID) (WorkspaceAgent, error) GetWorkspaceAgentByInstanceID(ctx context.Context, authInstanceID string) (WorkspaceAgent, error) GetWorkspaceAgentLifecycleStateByID(ctx context.Context, id uuid.UUID) (GetWorkspaceAgentLifecycleStateByIDRow, error) diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index 7f6af1d3f0db4..d480d7fc7678a 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -6360,58 +6360,6 @@ func (q *sqlQuerier) GetWorkspaceAgentAndOwnerByAuthToken(ctx context.Context, a return i, err } -const getWorkspaceAgentByAuthToken = `-- name: GetWorkspaceAgentByAuthToken :one -SELECT - id, created_at, updated_at, name, first_connected_at, last_connected_at, disconnected_at, resource_id, auth_token, auth_instance_id, architecture, environment_variables, operating_system, startup_script, instance_metadata, resource_metadata, directory, version, last_connected_replica_id, connection_timeout_seconds, troubleshooting_url, motd_file, lifecycle_state, startup_script_timeout_seconds, expanded_directory, shutdown_script, shutdown_script_timeout_seconds, logs_length, logs_overflowed, startup_script_behavior, started_at, ready_at, subsystems -FROM - workspace_agents -WHERE - auth_token = $1 -ORDER BY - created_at DESC -` - -func (q *sqlQuerier) GetWorkspaceAgentByAuthToken(ctx context.Context, authToken uuid.UUID) (WorkspaceAgent, error) { - row := q.db.QueryRowContext(ctx, getWorkspaceAgentByAuthToken, authToken) - var i WorkspaceAgent - err := row.Scan( - &i.ID, - &i.CreatedAt, - &i.UpdatedAt, - &i.Name, - &i.FirstConnectedAt, - &i.LastConnectedAt, - &i.DisconnectedAt, - &i.ResourceID, - &i.AuthToken, - &i.AuthInstanceID, - &i.Architecture, - &i.EnvironmentVariables, - &i.OperatingSystem, - &i.StartupScript, - &i.InstanceMetadata, - &i.ResourceMetadata, - &i.Directory, - &i.Version, - &i.LastConnectedReplicaID, - &i.ConnectionTimeoutSeconds, - &i.TroubleshootingURL, - &i.MOTDFile, - &i.LifecycleState, - &i.StartupScriptTimeoutSeconds, - &i.ExpandedDirectory, - &i.ShutdownScript, - &i.ShutdownScriptTimeoutSeconds, - &i.LogsLength, - &i.LogsOverflowed, - &i.StartupScriptBehavior, - &i.StartedAt, - &i.ReadyAt, - pq.Array(&i.Subsystems), - ) - return i, err -} - const getWorkspaceAgentByID = `-- name: GetWorkspaceAgentByID :one SELECT id, created_at, updated_at, name, first_connected_at, last_connected_at, disconnected_at, resource_id, auth_token, auth_instance_id, architecture, environment_variables, operating_system, startup_script, instance_metadata, resource_metadata, directory, version, last_connected_replica_id, connection_timeout_seconds, troubleshooting_url, motd_file, lifecycle_state, startup_script_timeout_seconds, expanded_directory, shutdown_script, shutdown_script_timeout_seconds, logs_length, logs_overflowed, startup_script_behavior, started_at, ready_at, subsystems diff --git a/coderd/database/queries/workspaceagents.sql b/coderd/database/queries/workspaceagents.sql index ebd44bd98591a..71c24a74df660 100644 --- a/coderd/database/queries/workspaceagents.sql +++ b/coderd/database/queries/workspaceagents.sql @@ -1,13 +1,3 @@ --- name: GetWorkspaceAgentByAuthToken :one -SELECT - * -FROM - workspace_agents -WHERE - auth_token = $1 -ORDER BY - created_at DESC; - -- name: GetWorkspaceAgentByID :one SELECT * diff --git a/coderd/httpmw/workspaceagent.go b/coderd/httpmw/workspaceagent.go index d7d1405d2471c..36a085199d16b 100644 --- a/coderd/httpmw/workspaceagent.go +++ b/coderd/httpmw/workspaceagent.go @@ -97,8 +97,7 @@ func ExtractWorkspaceAgent(opts ExtractWorkspaceAgentConfig) func(http.Handler) ID: row.OwnerID.String(), Roles: rbac.RoleNames(row.OwnerRoles), Groups: row.OwnerGroups, - // Note: this is generated as a NullUUID even though it shouldn't be nullable based on the query. - Scope: rbac.WorkspaceAgentScope(row.WorkspaceID, row.OwnerID), + Scope: rbac.WorkspaceAgentScope(row.WorkspaceID, row.OwnerID), }.WithCachedASTValue() ctx = context.WithValue(ctx, workspaceAgentContextKey{}, row.WorkspaceAgent) @@ -108,33 +107,3 @@ func ExtractWorkspaceAgent(opts ExtractWorkspaceAgentConfig) func(http.Handler) }) } } - -func getAgentSubject(ctx context.Context, db database.Store, agent database.WorkspaceAgent) (rbac.Subject, error) { - // TODO: make a different query that gets the workspace owner and roles along with the agent. - workspace, err := db.GetWorkspaceByAgentID(ctx, agent.ID) - if err != nil { - return rbac.Subject{}, err - } - - user, err := db.GetUserByID(ctx, workspace.OwnerID) - if err != nil { - return rbac.Subject{}, err - } - - roles, err := db.GetAuthorizationUserRoles(ctx, user.ID) - if err != nil { - return rbac.Subject{}, err - } - - // A user that creates a workspace can use this agent auth token and - // impersonate the workspace. So to prevent privilege escalation, the - // subject inherits the roles of the user that owns the workspace. - // We then add a workspace-agent scope to limit the permissions - // to only what the workspace agent needs. - return rbac.Subject{ - ID: user.ID.String(), - Roles: rbac.RoleNames(roles.Roles), - Groups: roles.Groups, - Scope: rbac.WorkspaceAgentScope(workspace.ID, user.ID), - }.WithCachedASTValue(), nil -} From 8e2430d765104b17e3808002b0adb537d6f4ef3b Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Wed, 16 Aug 2023 15:36:27 +0100 Subject: [PATCH 04/18] alter query --- coderd/database/queries.sql.go | 39 ++++++++------------- coderd/database/queries/workspaceagents.sql | 39 ++++++++------------- coderd/httpmw/workspaceagent_test.go | 8 ++++- 3 files changed, 37 insertions(+), 49 deletions(-) diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index d480d7fc7678a..829edc45e63e9 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -6256,31 +6256,10 @@ SELECT users.username AS owner_name, users.status AS owner_status, array_cat( - -- All users are members array_append(users.rbac_roles, 'member'), - ( - SELECT - array_agg(org_roles) - FROM - organization_members, - -- All org_members get the org-member role for their orgs - unnest( - array_append(roles, 'organization-member:' || organization_members.organization_id::text) - ) AS org_roles - WHERE - user_id = users.id - ) - ) :: text[] AS owner_roles, - ( - SELECT - array_agg( - group_members.group_id :: text - ) - FROM - group_members - WHERE - user_id = users.id - ) :: text[] AS owner_groups + array_append(ARRAY[]::text[], 'organization-member:' || organization_members.organization_id::text) + )::text[] as owner_roles, + array_agg(COALESCE(group_members.group_id::text, ''))::text[] AS owner_groups FROM users INNER JOIN workspaces @@ -6298,8 +6277,20 @@ FROM users workspace_agents ON workspace_agents.resource_id = workspace_resources.id + INNER JOIN -- every user is a member of some org + organization_members + ON + organization_members.user_id = users.id + LEFT JOIN -- as they may not be a member of any groups + group_members + ON + group_members.user_id = users.id WHERE workspace_agents.auth_token = $1 + AND + users.status = 'active' -- workspaces that belong to inactive users should not be +GROUP BY + workspace_agents.id, workspaces.id, users.id, organization_members.organization_id LIMIT 1 ` diff --git a/coderd/database/queries/workspaceagents.sql b/coderd/database/queries/workspaceagents.sql index 71c24a74df660..190df60b88e51 100644 --- a/coderd/database/queries/workspaceagents.sql +++ b/coderd/database/queries/workspaceagents.sql @@ -199,31 +199,10 @@ SELECT users.username AS owner_name, users.status AS owner_status, array_cat( - -- All users are members array_append(users.rbac_roles, 'member'), - ( - SELECT - array_agg(org_roles) - FROM - organization_members, - -- All org_members get the org-member role for their orgs - unnest( - array_append(roles, 'organization-member:' || organization_members.organization_id::text) - ) AS org_roles - WHERE - user_id = users.id - ) - ) :: text[] AS owner_roles, - ( - SELECT - array_agg( - group_members.group_id :: text - ) - FROM - group_members - WHERE - user_id = users.id - ) :: text[] AS owner_groups + array_append(ARRAY[]::text[], 'organization-member:' || organization_members.organization_id::text) + )::text[] as owner_roles, + array_agg(COALESCE(group_members.group_id::text, ''))::text[] AS owner_groups FROM users INNER JOIN workspaces @@ -241,6 +220,18 @@ FROM users workspace_agents ON workspace_agents.resource_id = workspace_resources.id + INNER JOIN -- every user is a member of some org + organization_members + ON + organization_members.user_id = users.id + LEFT JOIN -- as they may not be a member of any groups + group_members + ON + group_members.user_id = users.id WHERE workspace_agents.auth_token = @auth_token + AND + users.status = 'active' -- workspaces that belong to inactive users should not be +GROUP BY + workspace_agents.id, workspaces.id, users.id, organization_members.organization_id LIMIT 1; diff --git a/coderd/httpmw/workspaceagent_test.go b/coderd/httpmw/workspaceagent_test.go index a100fad7cf8f0..2ec70a4c059d8 100644 --- a/coderd/httpmw/workspaceagent_test.go +++ b/coderd/httpmw/workspaceagent_test.go @@ -67,7 +67,13 @@ func TestWorkspaceAgent(t *testing.T) { func setup(t testing.TB, db database.Store, authToken uuid.UUID, mw func(http.Handler) http.Handler) (*http.Request, http.Handler) { t.Helper() org := dbgen.Organization(t, db, database.Organization{}) - user := dbgen.User(t, db, database.User{}) + user := dbgen.User(t, db, database.User{ + Status: database.UserStatusActive, + }) + _ = dbgen.OrganizationMember(t, db, database.OrganizationMember{ + UserID: user.ID, + OrganizationID: org.ID, + }) templateVersion := dbgen.TemplateVersion(t, db, database.TemplateVersion{ OrganizationID: org.ID, CreatedBy: user.ID, From 4aa23c6292ca29b39fb4369a6d21ead4d9ebd419 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Wed, 16 Aug 2023 15:44:53 +0100 Subject: [PATCH 05/18] mockgen --- coderd/database/dbmock/dbmock.go | 29 +++++++---------------------- 1 file changed, 7 insertions(+), 22 deletions(-) diff --git a/coderd/database/dbmock/dbmock.go b/coderd/database/dbmock/dbmock.go index 8d2c018f543ea..7f3cf5cab772c 100644 --- a/coderd/database/dbmock/dbmock.go +++ b/coderd/database/dbmock/dbmock.go @@ -1616,19 +1616,19 @@ func (mr *MockStoreMockRecorder) GetUsersByIDs(arg0, arg1 interface{}) *gomock.C return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetUsersByIDs", reflect.TypeOf((*MockStore)(nil).GetUsersByIDs), arg0, arg1) } -// GetWorkspaceAgentByAuthToken mocks base method. -func (m *MockStore) GetWorkspaceAgentByAuthToken(arg0 context.Context, arg1 uuid.UUID) (database.WorkspaceAgent, error) { +// GetWorkspaceAgentAndOwnerByAuthToken mocks base method. +func (m *MockStore) GetWorkspaceAgentAndOwnerByAuthToken(arg0 context.Context, arg1 uuid.UUID) (database.GetWorkspaceAgentAndOwnerByAuthTokenRow, error) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "GetWorkspaceAgentByAuthToken", arg0, arg1) - ret0, _ := ret[0].(database.WorkspaceAgent) + ret := m.ctrl.Call(m, "GetWorkspaceAgentAndOwnerByAuthToken", arg0, arg1) + ret0, _ := ret[0].(database.GetWorkspaceAgentAndOwnerByAuthTokenRow) ret1, _ := ret[1].(error) return ret0, ret1 } -// GetWorkspaceAgentByAuthToken indicates an expected call of GetWorkspaceAgentByAuthToken. -func (mr *MockStoreMockRecorder) GetWorkspaceAgentByAuthToken(arg0, arg1 interface{}) *gomock.Call { +// GetWorkspaceAgentAndOwnerByAuthToken indicates an expected call of GetWorkspaceAgentAndOwnerByAuthToken. +func (mr *MockStoreMockRecorder) GetWorkspaceAgentAndOwnerByAuthToken(arg0, arg1 interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetWorkspaceAgentByAuthToken", reflect.TypeOf((*MockStore)(nil).GetWorkspaceAgentByAuthToken), arg0, arg1) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetWorkspaceAgentAndOwnerByAuthToken", reflect.TypeOf((*MockStore)(nil).GetWorkspaceAgentAndOwnerByAuthToken), arg0, arg1) } // GetWorkspaceAgentByID mocks base method. @@ -1736,21 +1736,6 @@ func (mr *MockStoreMockRecorder) GetWorkspaceAgentStatsAndLabels(arg0, arg1 inte return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetWorkspaceAgentStatsAndLabels", reflect.TypeOf((*MockStore)(nil).GetWorkspaceAgentStatsAndLabels), arg0, arg1) } -// GetWorkspaceAgentWithOwnerByAgentToken mocks base method. -func (m *MockStore) GetWorkspaceAgentWithOwnerByAgentToken(arg0 context.Context, arg1 uuid.UUID) (database.GetWorkspaceAgentWithOwnerByAgentTokenRow, error) { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "GetWorkspaceAgentWithOwnerByAgentToken", arg0, arg1) - ret0, _ := ret[0].(database.GetWorkspaceAgentWithOwnerByAgentTokenRow) - ret1, _ := ret[1].(error) - return ret0, ret1 -} - -// GetWorkspaceAgentWithOwnerByAgentToken indicates an expected call of GetWorkspaceAgentWithOwnerByAgentToken. -func (mr *MockStoreMockRecorder) GetWorkspaceAgentWithOwnerByAgentToken(arg0, arg1 interface{}) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetWorkspaceAgentWithOwnerByAgentToken", reflect.TypeOf((*MockStore)(nil).GetWorkspaceAgentWithOwnerByAgentToken), arg0, arg1) -} - // GetWorkspaceAgentsByResourceIDs mocks base method. func (m *MockStore) GetWorkspaceAgentsByResourceIDs(arg0 context.Context, arg1 []uuid.UUID) ([]database.WorkspaceAgent, error) { m.ctrl.T.Helper() From 41561b9244a52e5d6325166fd915a40bd6ae89fe Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Wed, 16 Aug 2023 18:36:47 +0100 Subject: [PATCH 06/18] rm unused tx in dbfake --- coderd/database/dbfake/dbfake.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/coderd/database/dbfake/dbfake.go b/coderd/database/dbfake/dbfake.go index 3413b2b9f0fee..c601c671b79c0 100644 --- a/coderd/database/dbfake/dbfake.go +++ b/coderd/database/dbfake/dbfake.go @@ -2833,7 +2833,7 @@ func (q *FakeQuerier) GetUsersByIDs(_ context.Context, ids []uuid.UUID) ([]datab return users, nil } -func (q *FakeQuerier) GetWorkspaceAgentAndOwnerByAuthToken(ctx context.Context, authToken uuid.UUID) (database.GetWorkspaceAgentAndOwnerByAuthTokenRow, error) { +func (q *FakeQuerier) GetWorkspaceAgentAndOwnerByAuthToken(_ context.Context, authToken uuid.UUID) (database.GetWorkspaceAgentAndOwnerByAuthTokenRow, error) { q.mutex.RLock() defer q.mutex.RUnlock() var resp database.GetWorkspaceAgentAndOwnerByAuthTokenRow From 0cb0d186808d2941feeee40b0f399cd8a52892a9 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Thu, 17 Aug 2023 11:13:15 +0100 Subject: [PATCH 07/18] add TODO --- cli/agent_test.go | 1 + coderd/database/dbfake/dbfake.go | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/cli/agent_test.go b/cli/agent_test.go index 34f04b70705b2..b30ff714c57bc 100644 --- a/cli/agent_test.go +++ b/cli/agent_test.go @@ -22,6 +22,7 @@ import ( "github.com/coder/coder/pty/ptytest" ) +// TODO(Cian): These ones are failing but only with dbfake func TestWorkspaceAgent(t *testing.T) { t.Parallel() diff --git a/coderd/database/dbfake/dbfake.go b/coderd/database/dbfake/dbfake.go index c601c671b79c0..6d6ecefd13eaa 100644 --- a/coderd/database/dbfake/dbfake.go +++ b/coderd/database/dbfake/dbfake.go @@ -2855,7 +2855,7 @@ func (q *FakeQuerier) GetWorkspaceAgentAndOwnerByAuthToken(_ context.Context, au continue } for _, build := range q.workspaceBuilds { - if build.JobID != res.JobID { + if build.JobID != res.JobID { // <-- jobID does not match up continue } for _, ws := range q.workspaces { From e6ce0f53a8d23c12e48163a370cc94c86e5bf943 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Fri, 18 Aug 2023 16:26:17 +0000 Subject: [PATCH 08/18] fix dbfake --- coderd/database/dbfake/dbfake.go | 104 +++++++------------- coderd/database/queries.sql.go | 6 +- coderd/database/queries/workspaceagents.sql | 6 +- 3 files changed, 45 insertions(+), 71 deletions(-) diff --git a/coderd/database/dbfake/dbfake.go b/coderd/database/dbfake/dbfake.go index 6d6ecefd13eaa..b17c428d63072 100644 --- a/coderd/database/dbfake/dbfake.go +++ b/coderd/database/dbfake/dbfake.go @@ -651,48 +651,6 @@ func (q *FakeQuerier) isEveryoneGroup(id uuid.UUID) bool { return false } -func (q *FakeQuerier) GetWorkspaceAgentAndOwnerByAuthToken(ctx context.Context, authToken uuid.UUID) (database.GetWorkspaceAgentAndOwnerByAuthTokenRow, error) { - q.mutex.RLock() - defer q.mutex.RUnlock() - var resp database.GetWorkspaceAgentAndOwnerByAuthTokenRow - var found bool - for _, agt := range q.workspaceAgents { - if agt.AuthToken == authToken { - resp.WorkspaceAgent = agt - found = true - break - } - } - if !found { - return resp, sql.ErrNoRows - } - - // get the related workspace and user - for _, res := range q.workspaceResources { - if resp.WorkspaceAgent.ResourceID != res.ID { - continue - } - for _, build := range q.workspaceBuilds { - if build.JobID != res.JobID { - continue - } - for _, ws := range q.workspaces { - if build.WorkspaceID != ws.ID { - continue - } - resp.WorkspaceID = ws.ID - if usr, err := q.getUserByIDNoLock(ws.OwnerID); err == nil { - resp.OwnerID = usr.ID - resp.OwnerRoles = usr.RBACRoles - resp.OwnerName = usr.Username - return resp, nil - } - } - } - } - return database.GetWorkspaceAgentAndOwnerByAuthTokenRow{}, sql.ErrNoRows -} - func (*FakeQuerier) AcquireLock(_ context.Context, _ int64) error { return xerrors.New("AcquireLock must only be called within a transaction") } @@ -2837,36 +2795,48 @@ func (q *FakeQuerier) GetWorkspaceAgentAndOwnerByAuthToken(_ context.Context, au q.mutex.RLock() defer q.mutex.RUnlock() var resp database.GetWorkspaceAgentAndOwnerByAuthTokenRow - var found bool +AgentLoop: for _, agt := range q.workspaceAgents { - if agt.AuthToken == authToken { - resp.WorkspaceAgent = agt - found = true - break - } - } - if !found { - return resp, sql.ErrNoRows - } - - // get the related workspace and user - for _, res := range q.workspaceResources { - if resp.WorkspaceAgent.ResourceID != res.ID { - continue - } - for _, build := range q.workspaceBuilds { - if build.JobID != res.JobID { // <-- jobID does not match up - continue + if agt.AuthToken != authToken { + continue AgentLoop + } + // get the related workspace and user + ResourceLoop: + for _, res := range q.workspaceResources { + if agt.ResourceID != res.ID { + continue ResourceLoop } - for _, ws := range q.workspaces { - if build.WorkspaceID != ws.ID { - continue + BuildLoop: + for _, build := range q.workspaceBuilds { + if build.JobID != res.JobID { + continue BuildLoop } - resp.WorkspaceID = ws.ID - if usr, err := q.getUserByIDNoLock(ws.OwnerID); err == nil { + WorkspaceLoop: + for _, ws := range q.workspaces { + if build.WorkspaceID != ws.ID { + continue WorkspaceLoop + } + resp.WorkspaceID = ws.ID + usr, err := q.getUserByIDNoLock(ws.OwnerID) + if err != nil { + return database.GetWorkspaceAgentAndOwnerByAuthTokenRow{}, sql.ErrNoRows + } resp.OwnerID = usr.ID - resp.OwnerRoles = usr.RBACRoles + resp.OwnerRoles = append(usr.RBACRoles, "member") + // We also need to get org roles for the user resp.OwnerName = usr.Username + resp.WorkspaceAgent = agt + for _, mem := range q.organizationMembers { + if mem.UserID == usr.ID { + resp.OwnerRoles = append(resp.OwnerRoles, fmt.Sprintf("organization-member:%s", mem.OrganizationID.String())) + } + } + // And group memberships + for _, groupMem := range q.groupMembers { + if groupMem.UserID == usr.ID { + resp.OwnerGroups = append(resp.OwnerGroups, groupMem.GroupID.String()) + } + } return resp, nil } } diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index 829edc45e63e9..50c8202d1d015 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -6286,9 +6286,11 @@ FROM users ON group_members.user_id = users.id WHERE + -- TODO: we can add more conditions here, such as: + -- 1) The user must be active + -- 2) The user must not be deleted + -- 3) The workspace must be running workspace_agents.auth_token = $1 - AND - users.status = 'active' -- workspaces that belong to inactive users should not be GROUP BY workspace_agents.id, workspaces.id, users.id, organization_members.organization_id LIMIT 1 diff --git a/coderd/database/queries/workspaceagents.sql b/coderd/database/queries/workspaceagents.sql index 190df60b88e51..b7ec7d0b643fd 100644 --- a/coderd/database/queries/workspaceagents.sql +++ b/coderd/database/queries/workspaceagents.sql @@ -229,9 +229,11 @@ FROM users ON group_members.user_id = users.id WHERE + -- TODO: we can add more conditions here, such as: + -- 1) The user must be active + -- 2) The user must not be deleted + -- 3) The workspace must be running workspace_agents.auth_token = @auth_token - AND - users.status = 'active' -- workspaces that belong to inactive users should not be GROUP BY workspace_agents.id, workspaces.id, users.id, organization_members.organization_id LIMIT 1; From b1d4084fb2494db3a404978bc172c03bff968c23 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Fri, 18 Aug 2023 17:15:55 +0000 Subject: [PATCH 09/18] return latest workspace build --- coderd/database/dbfake/dbfake.go | 5 +- coderd/database/queries.sql.go | 58 ++++++++++++--------- coderd/database/queries/workspaceagents.sql | 58 ++++++++++++--------- 3 files changed, 68 insertions(+), 53 deletions(-) diff --git a/coderd/database/dbfake/dbfake.go b/coderd/database/dbfake/dbfake.go index b17c428d63072..4b1e20e3333a7 100644 --- a/coderd/database/dbfake/dbfake.go +++ b/coderd/database/dbfake/dbfake.go @@ -2791,7 +2791,7 @@ func (q *FakeQuerier) GetUsersByIDs(_ context.Context, ids []uuid.UUID) ([]datab return users, nil } -func (q *FakeQuerier) GetWorkspaceAgentAndOwnerByAuthToken(_ context.Context, authToken uuid.UUID) (database.GetWorkspaceAgentAndOwnerByAuthTokenRow, error) { +func (q *FakeQuerier) GetWorkspaceAgentAndOwnerByAuthToken(ctx context.Context, authToken uuid.UUID) (database.GetWorkspaceAgentAndOwnerByAuthTokenRow, error) { q.mutex.RLock() defer q.mutex.RUnlock() var resp database.GetWorkspaceAgentAndOwnerByAuthTokenRow @@ -2816,6 +2816,9 @@ AgentLoop: if build.WorkspaceID != ws.ID { continue WorkspaceLoop } + if latestBuild, err := q.getLatestWorkspaceBuildByWorkspaceIDNoLock(ctx, ws.ID); err == nil && latestBuild.ID != build.ID { + continue BuildLoop + } resp.WorkspaceID = ws.ID usr, err := q.getUserByIDNoLock(ws.OwnerID) if err != nil { diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index 50c8202d1d015..272c5fe74f424 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -6261,38 +6261,44 @@ SELECT )::text[] as owner_roles, array_agg(COALESCE(group_members.group_id::text, ''))::text[] AS owner_groups FROM users - INNER JOIN - workspaces - ON - workspaces.owner_id = users.id - INNER JOIN - workspace_builds - ON - workspace_builds.workspace_id = workspaces.id - INNER JOIN - workspace_resources - ON - workspace_resources.job_id = workspace_builds.job_id - INNER JOIN - workspace_agents - ON - workspace_agents.resource_id = workspace_resources.id - INNER JOIN -- every user is a member of some org - organization_members - ON - organization_members.user_id = users.id - LEFT JOIN -- as they may not be a member of any groups - group_members - ON - group_members.user_id = users.id + INNER JOIN + workspaces + ON + workspaces.owner_id = users.id + INNER JOIN + workspace_builds + ON + workspace_builds.workspace_id = workspaces.id + INNER JOIN + workspace_resources + ON + workspace_resources.job_id = workspace_builds.job_id + INNER JOIN + workspace_agents + ON + workspace_agents.resource_id = workspace_resources.id + INNER JOIN -- every user is a member of some org + organization_members + ON + organization_members.user_id = users.id + LEFT JOIN -- as they may not be a member of any groups + group_members + ON + group_members.user_id = users.id WHERE -- TODO: we can add more conditions here, such as: -- 1) The user must be active -- 2) The user must not be deleted -- 3) The workspace must be running - workspace_agents.auth_token = $1 + workspace_agents.auth_token = $1 GROUP BY - workspace_agents.id, workspaces.id, users.id, organization_members.organization_id + workspace_agents.id, + workspaces.id, + users.id, + organization_members.organization_id, + workspace_builds.build_number +ORDER BY + workspace_builds.build_number DESC LIMIT 1 ` diff --git a/coderd/database/queries/workspaceagents.sql b/coderd/database/queries/workspaceagents.sql index b7ec7d0b643fd..8afd9343cb1f0 100644 --- a/coderd/database/queries/workspaceagents.sql +++ b/coderd/database/queries/workspaceagents.sql @@ -204,36 +204,42 @@ SELECT )::text[] as owner_roles, array_agg(COALESCE(group_members.group_id::text, ''))::text[] AS owner_groups FROM users - INNER JOIN - workspaces - ON - workspaces.owner_id = users.id - INNER JOIN - workspace_builds - ON - workspace_builds.workspace_id = workspaces.id - INNER JOIN - workspace_resources - ON - workspace_resources.job_id = workspace_builds.job_id - INNER JOIN - workspace_agents - ON - workspace_agents.resource_id = workspace_resources.id - INNER JOIN -- every user is a member of some org - organization_members - ON - organization_members.user_id = users.id - LEFT JOIN -- as they may not be a member of any groups - group_members - ON - group_members.user_id = users.id + INNER JOIN + workspaces + ON + workspaces.owner_id = users.id + INNER JOIN + workspace_builds + ON + workspace_builds.workspace_id = workspaces.id + INNER JOIN + workspace_resources + ON + workspace_resources.job_id = workspace_builds.job_id + INNER JOIN + workspace_agents + ON + workspace_agents.resource_id = workspace_resources.id + INNER JOIN -- every user is a member of some org + organization_members + ON + organization_members.user_id = users.id + LEFT JOIN -- as they may not be a member of any groups + group_members + ON + group_members.user_id = users.id WHERE -- TODO: we can add more conditions here, such as: -- 1) The user must be active -- 2) The user must not be deleted -- 3) The workspace must be running - workspace_agents.auth_token = @auth_token + workspace_agents.auth_token = @auth_token GROUP BY - workspace_agents.id, workspaces.id, users.id, organization_members.organization_id + workspace_agents.id, + workspaces.id, + users.id, + organization_members.organization_id, + workspace_builds.build_number +ORDER BY + workspace_builds.build_number DESC LIMIT 1; From b64a8700a59a48033a9a24dcd16202ea7f8b78fc Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Fri, 18 Aug 2023 17:16:51 +0000 Subject: [PATCH 10/18] fixup! return latest workspace build --- cli/agent_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/cli/agent_test.go b/cli/agent_test.go index b30ff714c57bc..34f04b70705b2 100644 --- a/cli/agent_test.go +++ b/cli/agent_test.go @@ -22,7 +22,6 @@ import ( "github.com/coder/coder/pty/ptytest" ) -// TODO(Cian): These ones are failing but only with dbfake func TestWorkspaceAgent(t *testing.T) { t.Parallel() From 73de5525854de95a73b7031d3cc2b6b1efbd07fb Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Fri, 18 Aug 2023 17:24:38 +0000 Subject: [PATCH 11/18] fix TestWorkspaceAgentListen/FailNonLatestBuild --- coderd/workspaceagents_test.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/coderd/workspaceagents_test.go b/coderd/workspaceagents_test.go index b85dd9d3550d9..ea687f04427d2 100644 --- a/coderd/workspaceagents_test.go +++ b/coderd/workspaceagents_test.go @@ -406,7 +406,9 @@ func TestWorkspaceAgentListen(t *testing.T) { _, err = agentClient.Listen(ctx) require.Error(t, err) - require.ErrorContains(t, err, "build is outdated") + var sdkErr *codersdk.Error + require.ErrorAs(t, err, &sdkErr) + require.Equal(t, http.StatusUnauthorized, sdkErr.StatusCode()) }) } From 1218fa48b69087d52cc2694117c4e389629f390a Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Fri, 18 Aug 2023 18:00:22 +0000 Subject: [PATCH 12/18] the bug was inside you all along --- coderd/database/dbfake/dbfake.go | 37 +++++++++++++++++++++----------- coderd/workspaceagents_test.go | 2 +- 2 files changed, 25 insertions(+), 14 deletions(-) diff --git a/coderd/database/dbfake/dbfake.go b/coderd/database/dbfake/dbfake.go index 4b1e20e3333a7..8172e4193bdaf 100644 --- a/coderd/database/dbfake/dbfake.go +++ b/coderd/database/dbfake/dbfake.go @@ -2794,7 +2794,7 @@ func (q *FakeQuerier) GetUsersByIDs(_ context.Context, ids []uuid.UUID) ([]datab func (q *FakeQuerier) GetWorkspaceAgentAndOwnerByAuthToken(ctx context.Context, authToken uuid.UUID) (database.GetWorkspaceAgentAndOwnerByAuthTokenRow, error) { q.mutex.RLock() defer q.mutex.RUnlock() - var resp database.GetWorkspaceAgentAndOwnerByAuthTokenRow + var rows []database.GetWorkspaceAgentAndOwnerByAuthTokenRow AgentLoop: for _, agt := range q.workspaceAgents { if agt.AuthToken != authToken { @@ -2816,36 +2816,47 @@ AgentLoop: if build.WorkspaceID != ws.ID { continue WorkspaceLoop } - if latestBuild, err := q.getLatestWorkspaceBuildByWorkspaceIDNoLock(ctx, ws.ID); err == nil && latestBuild.ID != build.ID { - continue BuildLoop - } - resp.WorkspaceID = ws.ID + var row database.GetWorkspaceAgentAndOwnerByAuthTokenRow + //if latestBuild, err := q.getLatestWorkspaceBuildByWorkspaceIDNoLock(ctx, ws.ID); err == nil && latestBuild.ID != build.ID { + // continue BuildLoop + //} + row.WorkspaceID = ws.ID usr, err := q.getUserByIDNoLock(ws.OwnerID) if err != nil { return database.GetWorkspaceAgentAndOwnerByAuthTokenRow{}, sql.ErrNoRows } - resp.OwnerID = usr.ID - resp.OwnerRoles = append(usr.RBACRoles, "member") + row.OwnerID = usr.ID + row.OwnerRoles = append(usr.RBACRoles, "member") // We also need to get org roles for the user - resp.OwnerName = usr.Username - resp.WorkspaceAgent = agt + row.OwnerName = usr.Username + row.WorkspaceAgent = agt for _, mem := range q.organizationMembers { if mem.UserID == usr.ID { - resp.OwnerRoles = append(resp.OwnerRoles, fmt.Sprintf("organization-member:%s", mem.OrganizationID.String())) + row.OwnerRoles = append(row.OwnerRoles, fmt.Sprintf("organization-member:%s", mem.OrganizationID.String())) } } // And group memberships for _, groupMem := range q.groupMembers { if groupMem.UserID == usr.ID { - resp.OwnerGroups = append(resp.OwnerGroups, groupMem.GroupID.String()) + row.OwnerGroups = append(row.OwnerGroups, groupMem.GroupID.String()) } } - return resp, nil + rows = append(rows, row) } } } } - return database.GetWorkspaceAgentAndOwnerByAuthTokenRow{}, sql.ErrNoRows + + // Sort rows by build ID descending + sort.Slice(rows, func(i, j int) bool { + return rows[i].WorkspaceAgent.CreatedAt.After(rows[j].WorkspaceAgent.CreatedAt) + }) + + var err error + if len(rows) == 0 { + return database.GetWorkspaceAgentAndOwnerByAuthTokenRow{}, sql.ErrNoRows + } + return rows[0], err } func (q *FakeQuerier) GetWorkspaceAgentByID(ctx context.Context, id uuid.UUID) (database.WorkspaceAgent, error) { diff --git a/coderd/workspaceagents_test.go b/coderd/workspaceagents_test.go index ea687f04427d2..522d5393bce9b 100644 --- a/coderd/workspaceagents_test.go +++ b/coderd/workspaceagents_test.go @@ -408,7 +408,7 @@ func TestWorkspaceAgentListen(t *testing.T) { require.Error(t, err) var sdkErr *codersdk.Error require.ErrorAs(t, err, &sdkErr) - require.Equal(t, http.StatusUnauthorized, sdkErr.StatusCode()) + require.Equal(t, http.StatusForbidden, sdkErr.StatusCode()) }) } From 8e222e3318f5383123664e12e1204559e67a6415 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Fri, 18 Aug 2023 19:10:18 +0100 Subject: [PATCH 13/18] linter --- coderd/database/dbfake/dbfake.go | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/coderd/database/dbfake/dbfake.go b/coderd/database/dbfake/dbfake.go index 8172e4193bdaf..c36c4e00e7a15 100644 --- a/coderd/database/dbfake/dbfake.go +++ b/coderd/database/dbfake/dbfake.go @@ -2791,7 +2791,7 @@ func (q *FakeQuerier) GetUsersByIDs(_ context.Context, ids []uuid.UUID) ([]datab return users, nil } -func (q *FakeQuerier) GetWorkspaceAgentAndOwnerByAuthToken(ctx context.Context, authToken uuid.UUID) (database.GetWorkspaceAgentAndOwnerByAuthTokenRow, error) { +func (q *FakeQuerier) GetWorkspaceAgentAndOwnerByAuthToken(_ context.Context, authToken uuid.UUID) (database.GetWorkspaceAgentAndOwnerByAuthTokenRow, error) { q.mutex.RLock() defer q.mutex.RUnlock() var rows []database.GetWorkspaceAgentAndOwnerByAuthTokenRow @@ -2817,9 +2817,6 @@ AgentLoop: continue WorkspaceLoop } var row database.GetWorkspaceAgentAndOwnerByAuthTokenRow - //if latestBuild, err := q.getLatestWorkspaceBuildByWorkspaceIDNoLock(ctx, ws.ID); err == nil && latestBuild.ID != build.ID { - // continue BuildLoop - //} row.WorkspaceID = ws.ID usr, err := q.getUserByIDNoLock(ws.OwnerID) if err != nil { From 06bbda2492381a266ec8030bf1891dd240f4c2c4 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Mon, 21 Aug 2023 12:41:26 +0100 Subject: [PATCH 14/18] address comments in dbfake impl --- coderd/database/dbfake/dbfake.go | 37 +++++++++++++++++--------------- 1 file changed, 20 insertions(+), 17 deletions(-) diff --git a/coderd/database/dbfake/dbfake.go b/coderd/database/dbfake/dbfake.go index 24e20635c5a20..162aa9195cb91 100644 --- a/coderd/database/dbfake/dbfake.go +++ b/coderd/database/dbfake/dbfake.go @@ -2794,27 +2794,29 @@ func (q *FakeQuerier) GetUsersByIDs(_ context.Context, ids []uuid.UUID) ([]datab func (q *FakeQuerier) GetWorkspaceAgentAndOwnerByAuthToken(_ context.Context, authToken uuid.UUID) (database.GetWorkspaceAgentAndOwnerByAuthTokenRow, error) { q.mutex.RLock() defer q.mutex.RUnlock() - var rows []database.GetWorkspaceAgentAndOwnerByAuthTokenRow -AgentLoop: + + // map of build number -> row + rows := make(map[int32]database.GetWorkspaceAgentAndOwnerByAuthTokenRow) + + // We want to return the latest build number + var latestBuildNumber int32 + for _, agt := range q.workspaceAgents { if agt.AuthToken != authToken { - continue AgentLoop + continue } // get the related workspace and user - ResourceLoop: for _, res := range q.workspaceResources { if agt.ResourceID != res.ID { - continue ResourceLoop + continue } - BuildLoop: for _, build := range q.workspaceBuilds { if build.JobID != res.JobID { - continue BuildLoop + continue } - WorkspaceLoop: for _, ws := range q.workspaces { if build.WorkspaceID != ws.ID { - continue WorkspaceLoop + continue } var row database.GetWorkspaceAgentAndOwnerByAuthTokenRow row.WorkspaceID = ws.ID @@ -2838,22 +2840,23 @@ AgentLoop: row.OwnerGroups = append(row.OwnerGroups, groupMem.GroupID.String()) } } - rows = append(rows, row) + + // Keep track of the latest build number + rows[build.BuildNumber] = row + if build.BuildNumber > latestBuildNumber { + latestBuildNumber = build.BuildNumber + } } } } } - // Sort rows by build ID descending - sort.Slice(rows, func(i, j int) bool { - return rows[i].WorkspaceAgent.CreatedAt.After(rows[j].WorkspaceAgent.CreatedAt) - }) - - var err error if len(rows) == 0 { return database.GetWorkspaceAgentAndOwnerByAuthTokenRow{}, sql.ErrNoRows } - return rows[0], err + + // Return the row related to the latest build + return rows[latestBuildNumber], nil } func (q *FakeQuerier) GetWorkspaceAgentByID(ctx context.Context, id uuid.UUID) (database.WorkspaceAgent, error) { From 3e21545c67d9ec7351f475366aea918afe85706d Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Mon, 21 Aug 2023 12:42:25 +0100 Subject: [PATCH 15/18] match indentation --- coderd/database/queries.sql.go | 2 +- coderd/database/queries/workspaceagents.sql | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index 272c5fe74f424..fe51d8cd6d244 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -6258,7 +6258,7 @@ SELECT array_cat( array_append(users.rbac_roles, 'member'), array_append(ARRAY[]::text[], 'organization-member:' || organization_members.organization_id::text) - )::text[] as owner_roles, + )::text[] as owner_roles, array_agg(COALESCE(group_members.group_id::text, ''))::text[] AS owner_groups FROM users INNER JOIN diff --git a/coderd/database/queries/workspaceagents.sql b/coderd/database/queries/workspaceagents.sql index 8afd9343cb1f0..9906d367e7bcf 100644 --- a/coderd/database/queries/workspaceagents.sql +++ b/coderd/database/queries/workspaceagents.sql @@ -201,7 +201,7 @@ SELECT array_cat( array_append(users.rbac_roles, 'member'), array_append(ARRAY[]::text[], 'organization-member:' || organization_members.organization_id::text) - )::text[] as owner_roles, + )::text[] as owner_roles, array_agg(COALESCE(group_members.group_id::text, ''))::text[] AS owner_groups FROM users INNER JOIN From 3a603fce1bf9b7411cf50750c4efb1a5e6db2691 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Mon, 21 Aug 2023 12:43:17 +0100 Subject: [PATCH 16/18] reword error --- coderd/httpmw/workspaceagent.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/coderd/httpmw/workspaceagent.go b/coderd/httpmw/workspaceagent.go index 5ced290a8a9d5..883a54e404c4e 100644 --- a/coderd/httpmw/workspaceagent.go +++ b/coderd/httpmw/workspaceagent.go @@ -87,7 +87,7 @@ func ExtractWorkspaceAgent(opts ExtractWorkspaceAgentConfig) func(http.Handler) } httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ - Message: "Internal error authorizing workspace agent.", + Message: "Internal error checking workspace agent authorization.", Detail: err.Error(), }) return From 267aa6caedec7da2e1230b5aa0a65d0dc0c78572 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Mon, 21 Aug 2023 12:44:34 +0100 Subject: [PATCH 17/18] rm unnecessary pubsub close --- coderd/httpmw/workspaceagent_test.go | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/coderd/httpmw/workspaceagent_test.go b/coderd/httpmw/workspaceagent_test.go index d0fd722f09584..62472fe13513d 100644 --- a/coderd/httpmw/workspaceagent_test.go +++ b/coderd/httpmw/workspaceagent_test.go @@ -21,10 +21,7 @@ func TestWorkspaceAgent(t *testing.T) { t.Run("None", func(t *testing.T) { t.Parallel() - db, pubsub := dbtestutil.NewDB(t) - t.Cleanup(func() { - _ = pubsub.Close() - }) + db, _ := dbtestutil.NewDB(t) req, rtr := setup(t, db, uuid.New(), httpmw.ExtractWorkspaceAgent( httpmw.ExtractWorkspaceAgentConfig{ @@ -43,10 +40,7 @@ func TestWorkspaceAgent(t *testing.T) { t.Run("Found", func(t *testing.T) { t.Parallel() - db, pubsub := dbtestutil.NewDB(t) - t.Cleanup(func() { - _ = pubsub.Close() - }) + db, _ := dbtestutil.NewDB(t) authToken := uuid.New() req, rtr := setup(t, db, authToken, httpmw.ExtractWorkspaceAgent( httpmw.ExtractWorkspaceAgentConfig{ From 6e0d3b3eb3f37cb2e0f3edbc259727c7d220f1b5 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Mon, 21 Aug 2023 12:46:00 +0100 Subject: [PATCH 18/18] rename misleading argument --- coderd/database/dbauthz/dbauthz.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go index 1a540ebadc07b..9123c7fba5135 100644 --- a/coderd/database/dbauthz/dbauthz.go +++ b/coderd/database/dbauthz/dbauthz.go @@ -1474,12 +1474,12 @@ func (q *querier) GetUsersByIDs(ctx context.Context, ids []uuid.UUID) ([]databas return q.db.GetUsersByIDs(ctx, ids) } -func (q *querier) GetWorkspaceAgentAndOwnerByAuthToken(ctx context.Context, agentID uuid.UUID) (database.GetWorkspaceAgentAndOwnerByAuthTokenRow, error) { +func (q *querier) GetWorkspaceAgentAndOwnerByAuthToken(ctx context.Context, authToken uuid.UUID) (database.GetWorkspaceAgentAndOwnerByAuthTokenRow, error) { // This is a system function if err := q.authorizeContext(ctx, rbac.ActionRead, rbac.ResourceSystem); err != nil { return database.GetWorkspaceAgentAndOwnerByAuthTokenRow{}, err } - return q.db.GetWorkspaceAgentAndOwnerByAuthToken(ctx, agentID) + return q.db.GetWorkspaceAgentAndOwnerByAuthToken(ctx, authToken) } func (q *querier) GetWorkspaceAgentByID(ctx context.Context, id uuid.UUID) (database.WorkspaceAgent, error) {