From fe867d02e08833a79d3b564c65d6e41c83e9d482 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 20 Dec 2023 11:38:49 -0600 Subject: [PATCH] fix: correct perms for forbidden error in TemplateScheduleStore.Load (#11286) * chore: TemplateScheduleStore.Load() throwing forbidden error * fix: workspace agent scope to include template --- coderd/agentapi/stats.go | 2 +- coderd/database/dbmem/dbmem.go | 2 ++ coderd/database/queries.sql.go | 23 ++++++++++++------- coderd/database/queries/workspaceagents.sql | 5 +++- coderd/httpmw/workspaceagent.go | 7 +++++- .../prometheusmetrics_test.go | 2 ++ coderd/rbac/scopes.go | 22 ++++++++++++++---- coderd/workspaceagents.go | 2 +- 8 files changed, 49 insertions(+), 16 deletions(-) diff --git a/coderd/agentapi/stats.go b/coderd/agentapi/stats.go index 05c1b744f2..d2098f2cce 100644 --- a/coderd/agentapi/stats.go +++ b/coderd/agentapi/stats.go @@ -61,7 +61,7 @@ func (a *StatsAPI) UpdateStats(ctx context.Context, req *agentproto.UpdateStatsR templateSchedule, err := (*(a.TemplateScheduleStore.Load())).Get(ctx, a.Database, workspace.TemplateID) // If the template schedule fails to load, just default to bumping without the next trasition and log it. if err != nil { - a.Log.Warn(ctx, "failed to load template schedule bumping activity, defaulting to bumping by 60min", + a.Log.Error(ctx, "failed to load template schedule bumping activity, defaulting to bumping by 60min", slog.F("workspace_id", workspace.ID), slog.F("template_id", workspace.TemplateID), slog.Error(err), diff --git a/coderd/database/dbmem/dbmem.go b/coderd/database/dbmem/dbmem.go index 216e718939..66f369b1a4 100644 --- a/coderd/database/dbmem/dbmem.go +++ b/coderd/database/dbmem/dbmem.go @@ -3796,6 +3796,7 @@ func (q *FakeQuerier) GetWorkspaceAgentAndOwnerByAuthToken(_ context.Context, au } var row database.GetWorkspaceAgentAndOwnerByAuthTokenRow row.WorkspaceID = ws.ID + row.TemplateID = ws.TemplateID usr, err := q.getUserByIDNoLock(ws.OwnerID) if err != nil { return database.GetWorkspaceAgentAndOwnerByAuthTokenRow{}, sql.ErrNoRows @@ -3805,6 +3806,7 @@ func (q *FakeQuerier) GetWorkspaceAgentAndOwnerByAuthToken(_ context.Context, au // We also need to get org roles for the user row.OwnerName = usr.Username row.WorkspaceAgent = agt + row.TemplateVersionID = build.TemplateVersionID for _, mem := range q.organizationMembers { if mem.UserID == usr.ID { row.OwnerRoles = append(row.OwnerRoles, fmt.Sprintf("organization-member:%s", mem.OrganizationID.String())) diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index cd7c115e39..213c17ddbe 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -7781,6 +7781,8 @@ SELECT users.id AS owner_id, users.username AS owner_name, users.status AS owner_status, + workspaces.template_id AS template_id, + workspace_builds.template_version_id AS template_version_id, array_cat( array_append(users.rbac_roles, 'member'), array_append(ARRAY[]::text[], 'organization-member:' || organization_members.organization_id::text) @@ -7823,20 +7825,23 @@ GROUP BY workspaces.id, users.id, organization_members.organization_id, - workspace_builds.build_number + workspace_builds.build_number, + workspace_builds.template_version_id ORDER BY workspace_builds.build_number DESC 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"` + 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"` + TemplateID uuid.UUID `db:"template_id" json:"template_id"` + TemplateVersionID uuid.UUID `db:"template_version_id" json:"template_version_id"` + 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) { @@ -7877,6 +7882,8 @@ func (q *sqlQuerier) GetWorkspaceAgentAndOwnerByAuthToken(ctx context.Context, a &i.OwnerID, &i.OwnerName, &i.OwnerStatus, + &i.TemplateID, + &i.TemplateVersionID, pq.Array(&i.OwnerRoles), pq.Array(&i.OwnerGroups), ) diff --git a/coderd/database/queries/workspaceagents.sql b/coderd/database/queries/workspaceagents.sql index 9f2e2eaabf..9b972687e2 100644 --- a/coderd/database/queries/workspaceagents.sql +++ b/coderd/database/queries/workspaceagents.sql @@ -219,6 +219,8 @@ SELECT users.id AS owner_id, users.username AS owner_name, users.status AS owner_status, + workspaces.template_id AS template_id, + workspace_builds.template_version_id AS template_version_id, array_cat( array_append(users.rbac_roles, 'member'), array_append(ARRAY[]::text[], 'organization-member:' || organization_members.organization_id::text) @@ -261,7 +263,8 @@ GROUP BY workspaces.id, users.id, organization_members.organization_id, - workspace_builds.build_number + workspace_builds.build_number, + workspace_builds.template_version_id ORDER BY workspace_builds.build_number DESC LIMIT 1; diff --git a/coderd/httpmw/workspaceagent.go b/coderd/httpmw/workspaceagent.go index 883a54e404..31caad0b7c 100644 --- a/coderd/httpmw/workspaceagent.go +++ b/coderd/httpmw/workspaceagent.go @@ -97,7 +97,12 @@ func ExtractWorkspaceAgent(opts ExtractWorkspaceAgentConfig) func(http.Handler) ID: row.OwnerID.String(), Roles: rbac.RoleNames(row.OwnerRoles), Groups: row.OwnerGroups, - Scope: rbac.WorkspaceAgentScope(row.WorkspaceID, row.OwnerID), + Scope: rbac.WorkspaceAgentScope(rbac.WorkspaceAgentScopeParams{ + WorkspaceID: row.WorkspaceID, + OwnerID: row.OwnerID, + TemplateID: row.TemplateID, + VersionID: row.TemplateVersionID, + }), }.WithCachedASTValue() ctx = context.WithValue(ctx, workspaceAgentContextKey{}, row.WorkspaceAgent) diff --git a/coderd/prometheusmetrics/prometheusmetrics_test.go b/coderd/prometheusmetrics/prometheusmetrics_test.go index 8256c2f7e2..645f179256 100644 --- a/coderd/prometheusmetrics/prometheusmetrics_test.go +++ b/coderd/prometheusmetrics/prometheusmetrics_test.go @@ -394,12 +394,14 @@ func TestAgentStats(t *testing.T) { require.NoError(t, err, "create stats batcher failed") t.Cleanup(closeBatcher) + tLogger := slogtest.Make(t, nil) // Build sample workspaces with test agents and fake agent client client, _, _ := coderdtest.NewWithAPI(t, &coderdtest.Options{ Database: db, IncludeProvisionerDaemon: true, Pubsub: pubsub, StatsBatcher: batcher, + Logger: &tLogger, }) user := coderdtest.CreateFirstUser(t, client) diff --git a/coderd/rbac/scopes.go b/coderd/rbac/scopes.go index 76ec79ed0b..fe85be6763 100644 --- a/coderd/rbac/scopes.go +++ b/coderd/rbac/scopes.go @@ -8,10 +8,21 @@ import ( "golang.org/x/xerrors" ) +type WorkspaceAgentScopeParams struct { + WorkspaceID uuid.UUID + OwnerID uuid.UUID + TemplateID uuid.UUID + VersionID uuid.UUID +} + // WorkspaceAgentScope returns a scope that is the same as ScopeAll but can only // affect resources in the allow list. Only a scope is returned as the roles // should come from the workspace owner. -func WorkspaceAgentScope(workspaceID, ownerID uuid.UUID) Scope { +func WorkspaceAgentScope(params WorkspaceAgentScopeParams) Scope { + if params.WorkspaceID == uuid.Nil || params.OwnerID == uuid.Nil || params.TemplateID == uuid.Nil || params.VersionID == uuid.Nil { + panic("all uuids must be non-nil, this is a developer error") + } + allScope, err := ScopeAll.Expand() if err != nil { panic("failed to expand scope all, this should never happen") @@ -23,10 +34,13 @@ func WorkspaceAgentScope(workspaceID, ownerID uuid.UUID) Scope { // and evolving. Role: allScope.Role, // This prevents the agent from being able to access any other resource. + // Include the list of IDs of anything that is required for the + // agent to function. AllowIDList: []string{ - workspaceID.String(), - ownerID.String(), - // TODO: Might want to include the template the workspace uses too? + params.WorkspaceID.String(), + params.TemplateID.String(), + params.VersionID.String(), + params.OwnerID.String(), }, } } diff --git a/coderd/workspaceagents.go b/coderd/workspaceagents.go index 04428aed28..dd47275a4f 100644 --- a/coderd/workspaceagents.go +++ b/coderd/workspaceagents.go @@ -1488,7 +1488,7 @@ func (api *API) workspaceAgentReportStats(rw http.ResponseWriter, r *http.Reques templateSchedule, err := (*(api.TemplateScheduleStore.Load())).Get(ctx, api.Database, workspace.TemplateID) // If the template schedule fails to load, just default to bumping without the next transition and log it. if err != nil { - api.Logger.Warn(ctx, "failed to load template schedule bumping activity, defaulting to bumping by 60min", + api.Logger.Error(ctx, "failed to load template schedule bumping activity, defaulting to bumping by 60min", slog.F("workspace_id", workspace.ID), slog.F("template_id", workspace.TemplateID), slog.Error(err),