From 4150a9953c33d7001370d267d5a44d9fd5d16bd1 Mon Sep 17 00:00:00 2001 From: Garrett Delfosse Date: Mon, 4 Dec 2023 20:04:04 +0000 Subject: [PATCH 1/7] fix: prevent deleted workspaces from updating agent stats --- coderd/database/dbauthz/dbauthz.go | 2 +- coderd/workspaceagents.go | 7 ++++ coderd/workspaceagents_test.go | 57 ++++++++++++++++++++++++++++++ 3 files changed, 65 insertions(+), 1 deletion(-) diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go index 1ba76a6c7431c..26a2a1a9a706e 100644 --- a/coderd/database/dbauthz/dbauthz.go +++ b/coderd/database/dbauthz/dbauthz.go @@ -228,7 +228,7 @@ var ( rbac.ResourceOrgRoleAssignment.Type: {rbac.ActionCreate}, rbac.ResourceUser.Type: {rbac.ActionCreate, rbac.ActionUpdate, rbac.ActionDelete}, rbac.ResourceUserData.Type: {rbac.ActionCreate, rbac.ActionUpdate}, - rbac.ResourceWorkspace.Type: {rbac.ActionUpdate}, + rbac.ResourceWorkspace.Type: {rbac.ActionUpdate, rbac.ActionDelete}, rbac.ResourceWorkspaceBuild.Type: {rbac.ActionUpdate}, rbac.ResourceWorkspaceExecution.Type: {rbac.ActionCreate}, rbac.ResourceWorkspaceProxy.Type: {rbac.ActionCreate, rbac.ActionUpdate, rbac.ActionDelete}, diff --git a/coderd/workspaceagents.go b/coderd/workspaceagents.go index c862706c5620a..c59207a904e70 100644 --- a/coderd/workspaceagents.go +++ b/coderd/workspaceagents.go @@ -1653,6 +1653,13 @@ func (api *API) workspaceAgentReportStats(rw http.ResponseWriter, r *http.Reques return } + if workspace.Deleted { + httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ + Message: "Workspace has been deleted.", + }) + return + } + var req agentsdk.Stats if !httpapi.Read(ctx, rw, r, &req) { return diff --git a/coderd/workspaceagents_test.go b/coderd/workspaceagents_test.go index c8404b2acf178..5c21d27e7d36e 100644 --- a/coderd/workspaceagents_test.go +++ b/coderd/workspaceagents_test.go @@ -26,6 +26,7 @@ import ( "github.com/coder/coder/v2/coderd" "github.com/coder/coder/v2/coderd/coderdtest" "github.com/coder/coder/v2/coderd/database" + "github.com/coder/coder/v2/coderd/database/dbauthz" "github.com/coder/coder/v2/coderd/database/dbfake" "github.com/coder/coder/v2/coderd/database/dbmem" "github.com/coder/coder/v2/coderd/database/dbtime" @@ -876,6 +877,62 @@ func TestWorkspaceAgentReportStats(t *testing.T) { "%s is not after %s", newWorkspace.LastUsedAt, r.Workspace.LastUsedAt, ) }) + + t.Run("FailDeleted", func(t *testing.T) { + t.Parallel() + + client, db := coderdtest.NewWithDatabase(t, nil) + user := coderdtest.CreateFirstUser(t, client) + r := dbfake.WorkspaceBuild(t, db, database.Workspace{ + OrganizationID: user.OrganizationID, + OwnerID: user.UserID, + }).WithAgent().Do() + + agentClient := agentsdk.New(client.URL) + agentClient.SetSessionToken(r.AgentToken) + + _, err := agentClient.PostStats(context.Background(), &agentsdk.Stats{ + ConnectionsByProto: map[string]int64{"TCP": 1}, + // Set connection count to 1 but all session counts to zero to + // assert we aren't updating last_used_at for a connections that may + // be spawned passively by the dashboard. + ConnectionCount: 1, + RxPackets: 1, + RxBytes: 1, + TxPackets: 1, + TxBytes: 1, + SessionCountVSCode: 0, + SessionCountJetBrains: 0, + SessionCountReconnectingPTY: 0, + SessionCountSSH: 0, + ConnectionMedianLatencyMS: 10, + }) + require.NoError(t, err) + + newWorkspace, err := client.Workspace(context.Background(), r.Workspace.ID) + require.NoError(t, err) + + err = db.UpdateWorkspaceDeletedByID(dbauthz.AsSystemRestricted(context.Background()), database.UpdateWorkspaceDeletedByIDParams{ + ID: newWorkspace.ID, + Deleted: true, + }) + require.NoError(t, err) + + _, err = agentClient.PostStats(context.Background(), &agentsdk.Stats{ + ConnectionsByProto: map[string]int64{"TCP": 1}, + ConnectionCount: 1, + RxPackets: 1, + RxBytes: 1, + TxPackets: 1, + TxBytes: 1, + SessionCountVSCode: 1, + SessionCountJetBrains: 0, + SessionCountReconnectingPTY: 0, + SessionCountSSH: 0, + ConnectionMedianLatencyMS: 10, + }) + require.ErrorContains(t, err, "Workspace has been deleted.") + }) } func TestWorkspaceAgent_LifecycleState(t *testing.T) { From 7721b2dd8918d23e9f2d098d9dcde2e3d631d0a5 Mon Sep 17 00:00:00 2001 From: Garrett Delfosse Date: Mon, 4 Dec 2023 20:05:32 +0000 Subject: [PATCH 2/7] remove comment --- coderd/workspaceagents_test.go | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/coderd/workspaceagents_test.go b/coderd/workspaceagents_test.go index 5c21d27e7d36e..260ef68382a46 100644 --- a/coderd/workspaceagents_test.go +++ b/coderd/workspaceagents_test.go @@ -892,10 +892,7 @@ func TestWorkspaceAgentReportStats(t *testing.T) { agentClient.SetSessionToken(r.AgentToken) _, err := agentClient.PostStats(context.Background(), &agentsdk.Stats{ - ConnectionsByProto: map[string]int64{"TCP": 1}, - // Set connection count to 1 but all session counts to zero to - // assert we aren't updating last_used_at for a connections that may - // be spawned passively by the dashboard. + ConnectionsByProto: map[string]int64{"TCP": 1}, ConnectionCount: 1, RxPackets: 1, RxBytes: 1, From f1d32799c569e29d6e7d2f26e5774b70f020bfb2 Mon Sep 17 00:00:00 2001 From: Garrett Delfosse Date: Mon, 4 Dec 2023 20:21:58 +0000 Subject: [PATCH 3/7] lint --- coderd/workspaceagents_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/coderd/workspaceagents_test.go b/coderd/workspaceagents_test.go index 260ef68382a46..694beed7997ed 100644 --- a/coderd/workspaceagents_test.go +++ b/coderd/workspaceagents_test.go @@ -909,6 +909,7 @@ func TestWorkspaceAgentReportStats(t *testing.T) { newWorkspace, err := client.Workspace(context.Background(), r.Workspace.ID) require.NoError(t, err) + // nolint:gocritic // using db directly over creating a delete job err = db.UpdateWorkspaceDeletedByID(dbauthz.AsSystemRestricted(context.Background()), database.UpdateWorkspaceDeletedByIDParams{ ID: newWorkspace.ID, Deleted: true, From f3b7b9ad290299b97e7dcc6511b099cec890e910 Mon Sep 17 00:00:00 2001 From: Garrett Delfosse Date: Thu, 7 Dec 2023 17:49:43 +0000 Subject: [PATCH 4/7] use query over route logic --- coderd/database/dbmem/dbmem.go | 3 +++ coderd/database/queries.sql.go | 5 +++-- coderd/database/queries/workspaceagents.sql | 5 +++-- coderd/workspaceagents.go | 7 ------- coderd/workspaceagents_test.go | 2 +- 5 files changed, 10 insertions(+), 12 deletions(-) diff --git a/coderd/database/dbmem/dbmem.go b/coderd/database/dbmem/dbmem.go index 4cae64776d4d9..e0abe93564292 100644 --- a/coderd/database/dbmem/dbmem.go +++ b/coderd/database/dbmem/dbmem.go @@ -3704,6 +3704,9 @@ func (q *FakeQuerier) GetWorkspaceAgentAndOwnerByAuthToken(_ context.Context, au if build.WorkspaceID != ws.ID { continue } + if ws.Deleted { + continue + } var row database.GetWorkspaceAgentAndOwnerByAuthTokenRow row.WorkspaceID = ws.ID usr, err := q.getUserByIDNoLock(ws.OwnerID) diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index c4cc4246fdc1a..3f9853709adbf 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -7736,9 +7736,10 @@ FROM users 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 + -- 2) The workspace must be running workspace_agents.auth_token = $1 +AND + workspaces.deleted = FALSE GROUP BY workspace_agents.id, workspaces.id, diff --git a/coderd/database/queries/workspaceagents.sql b/coderd/database/queries/workspaceagents.sql index 7dd2aaa29d6f7..9f2e2eaabf744 100644 --- a/coderd/database/queries/workspaceagents.sql +++ b/coderd/database/queries/workspaceagents.sql @@ -252,9 +252,10 @@ FROM users 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 + -- 2) The workspace must be running workspace_agents.auth_token = @auth_token +AND + workspaces.deleted = FALSE GROUP BY workspace_agents.id, workspaces.id, diff --git a/coderd/workspaceagents.go b/coderd/workspaceagents.go index c59207a904e70..c862706c5620a 100644 --- a/coderd/workspaceagents.go +++ b/coderd/workspaceagents.go @@ -1653,13 +1653,6 @@ func (api *API) workspaceAgentReportStats(rw http.ResponseWriter, r *http.Reques return } - if workspace.Deleted { - httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ - Message: "Workspace has been deleted.", - }) - return - } - var req agentsdk.Stats if !httpapi.Read(ctx, rw, r, &req) { return diff --git a/coderd/workspaceagents_test.go b/coderd/workspaceagents_test.go index 694beed7997ed..9f0eb3e3d7410 100644 --- a/coderd/workspaceagents_test.go +++ b/coderd/workspaceagents_test.go @@ -929,7 +929,7 @@ func TestWorkspaceAgentReportStats(t *testing.T) { SessionCountSSH: 0, ConnectionMedianLatencyMS: 10, }) - require.ErrorContains(t, err, "Workspace has been deleted.") + require.ErrorContains(t, err, "agent is invalid") }) } From 0e36f032e51aa29615ffdfa404a1e5468aba9de8 Mon Sep 17 00:00:00 2001 From: Garrett Delfosse Date: Thu, 7 Dec 2023 12:50:39 -0500 Subject: [PATCH 5/7] Apply suggestions from code review Co-authored-by: Steven Masley --- coderd/workspaceagents_test.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/coderd/workspaceagents_test.go b/coderd/workspaceagents_test.go index 9f0eb3e3d7410..980ff6e446c89 100644 --- a/coderd/workspaceagents_test.go +++ b/coderd/workspaceagents_test.go @@ -881,8 +881,9 @@ func TestWorkspaceAgentReportStats(t *testing.T) { t.Run("FailDeleted", func(t *testing.T) { t.Parallel() - client, db := coderdtest.NewWithDatabase(t, nil) - user := coderdtest.CreateFirstUser(t, client) + owner, db := coderdtest.NewWithDatabase(t, nil) + ownerUser := coderdtest.CreateFirstUser(t, owner) + client, admin := coderdtest.CreateAnotherUser(t, owner, ownerUser.OrganizationID, rbac.RoleTemplateAdmin(), rbac.RoleUserAdmin()) r := dbfake.WorkspaceBuild(t, db, database.Workspace{ OrganizationID: user.OrganizationID, OwnerID: user.UserID, @@ -910,7 +911,7 @@ func TestWorkspaceAgentReportStats(t *testing.T) { require.NoError(t, err) // nolint:gocritic // using db directly over creating a delete job - err = db.UpdateWorkspaceDeletedByID(dbauthz.AsSystemRestricted(context.Background()), database.UpdateWorkspaceDeletedByIDParams{ + err = db.UpdateWorkspaceDeletedByID(dbauthz.As(ctx, coderdtest.AuthzUserSubject(admin)), database.UpdateWorkspaceDeletedByIDParams{ ID: newWorkspace.ID, Deleted: true, }) From 44deadae69203ef6e31860b9985e7d9b1694e5f2 Mon Sep 17 00:00:00 2001 From: Garrett Delfosse Date: Thu, 7 Dec 2023 17:54:56 +0000 Subject: [PATCH 6/7] fix --- coderd/database/dbauthz/dbauthz.go | 2 +- coderd/workspaceagents_test.go | 7 ++++--- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go index 26a2a1a9a706e..1ba76a6c7431c 100644 --- a/coderd/database/dbauthz/dbauthz.go +++ b/coderd/database/dbauthz/dbauthz.go @@ -228,7 +228,7 @@ var ( rbac.ResourceOrgRoleAssignment.Type: {rbac.ActionCreate}, rbac.ResourceUser.Type: {rbac.ActionCreate, rbac.ActionUpdate, rbac.ActionDelete}, rbac.ResourceUserData.Type: {rbac.ActionCreate, rbac.ActionUpdate}, - rbac.ResourceWorkspace.Type: {rbac.ActionUpdate, rbac.ActionDelete}, + rbac.ResourceWorkspace.Type: {rbac.ActionUpdate}, rbac.ResourceWorkspaceBuild.Type: {rbac.ActionUpdate}, rbac.ResourceWorkspaceExecution.Type: {rbac.ActionCreate}, rbac.ResourceWorkspaceProxy.Type: {rbac.ActionCreate, rbac.ActionUpdate, rbac.ActionDelete}, diff --git a/coderd/workspaceagents_test.go b/coderd/workspaceagents_test.go index 980ff6e446c89..b908e2a1bdeb8 100644 --- a/coderd/workspaceagents_test.go +++ b/coderd/workspaceagents_test.go @@ -31,6 +31,7 @@ import ( "github.com/coder/coder/v2/coderd/database/dbmem" "github.com/coder/coder/v2/coderd/database/dbtime" "github.com/coder/coder/v2/coderd/database/pubsub" + "github.com/coder/coder/v2/coderd/rbac" "github.com/coder/coder/v2/codersdk" "github.com/coder/coder/v2/codersdk/agentsdk" "github.com/coder/coder/v2/provisioner/echo" @@ -885,8 +886,8 @@ func TestWorkspaceAgentReportStats(t *testing.T) { ownerUser := coderdtest.CreateFirstUser(t, owner) client, admin := coderdtest.CreateAnotherUser(t, owner, ownerUser.OrganizationID, rbac.RoleTemplateAdmin(), rbac.RoleUserAdmin()) r := dbfake.WorkspaceBuild(t, db, database.Workspace{ - OrganizationID: user.OrganizationID, - OwnerID: user.UserID, + OrganizationID: admin.OrganizationIDs[0], + OwnerID: admin.ID, }).WithAgent().Do() agentClient := agentsdk.New(client.URL) @@ -911,7 +912,7 @@ func TestWorkspaceAgentReportStats(t *testing.T) { require.NoError(t, err) // nolint:gocritic // using db directly over creating a delete job - err = db.UpdateWorkspaceDeletedByID(dbauthz.As(ctx, coderdtest.AuthzUserSubject(admin)), database.UpdateWorkspaceDeletedByIDParams{ + err = db.UpdateWorkspaceDeletedByID(dbauthz.As(context.Background(), coderdtest.AuthzUserSubject(admin)), database.UpdateWorkspaceDeletedByIDParams{ ID: newWorkspace.ID, Deleted: true, }) From dd08f2f6ab0872c777612d37a0ba31840c0e3958 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 7 Dec 2023 12:40:42 -0600 Subject: [PATCH 7/7] fix AuthzUserSubject to include member and org member role --- coderd/coderdtest/coderdtest.go | 6 +++++- coderd/templates_test.go | 2 +- coderd/workspaceagents_test.go | 10 ++++++---- 3 files changed, 12 insertions(+), 6 deletions(-) diff --git a/coderd/coderdtest/coderdtest.go b/coderd/coderdtest/coderdtest.go index f5ed26cfe97d6..eed0757e300b6 100644 --- a/coderd/coderdtest/coderdtest.go +++ b/coderd/coderdtest/coderdtest.go @@ -617,11 +617,15 @@ func CreateAnotherUserMutators(t testing.TB, client *codersdk.Client, organizati } // AuthzUserSubject does not include the user's groups. -func AuthzUserSubject(user codersdk.User) rbac.Subject { +func AuthzUserSubject(user codersdk.User, orgID uuid.UUID) rbac.Subject { roles := make(rbac.RoleNames, 0, len(user.Roles)) + // Member role is always implied + roles = append(roles, rbac.RoleMember()) for _, r := range user.Roles { roles = append(roles, r.Name) } + // We assume only 1 org exists + roles = append(roles, rbac.RoleOrgMember(orgID)) return rbac.Subject{ ID: user.ID.String(), diff --git a/coderd/templates_test.go b/coderd/templates_test.go index 4a9ba47455286..47154d1511ff0 100644 --- a/coderd/templates_test.go +++ b/coderd/templates_test.go @@ -560,7 +560,7 @@ func TestPatchTemplateMeta(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) defer cancel() // nolint:gocritic // Setting up unit test data - err := db.UpdateTemplateAccessControlByID(dbauthz.As(ctx, coderdtest.AuthzUserSubject(tplAdmin)), database.UpdateTemplateAccessControlByIDParams{ + err := db.UpdateTemplateAccessControlByID(dbauthz.As(ctx, coderdtest.AuthzUserSubject(tplAdmin, user.OrganizationID)), database.UpdateTemplateAccessControlByIDParams{ ID: template.ID, RequireActiveVersion: false, Deprecated: "Some deprecated message", diff --git a/coderd/workspaceagents_test.go b/coderd/workspaceagents_test.go index b908e2a1bdeb8..a966917cdb86c 100644 --- a/coderd/workspaceagents_test.go +++ b/coderd/workspaceagents_test.go @@ -912,10 +912,12 @@ func TestWorkspaceAgentReportStats(t *testing.T) { require.NoError(t, err) // nolint:gocritic // using db directly over creating a delete job - err = db.UpdateWorkspaceDeletedByID(dbauthz.As(context.Background(), coderdtest.AuthzUserSubject(admin)), database.UpdateWorkspaceDeletedByIDParams{ - ID: newWorkspace.ID, - Deleted: true, - }) + err = db.UpdateWorkspaceDeletedByID(dbauthz.As(context.Background(), + coderdtest.AuthzUserSubject(admin, ownerUser.OrganizationID)), + database.UpdateWorkspaceDeletedByIDParams{ + ID: newWorkspace.ID, + Deleted: true, + }) require.NoError(t, err) _, err = agentClient.PostStats(context.Background(), &agentsdk.Stats{