Skip to content

chore(coderd/rbac): add Action{Create,Delete}Agent to ResourceWorkspace #17932

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
May 20, 2025
Merged

chore(coderd/rbac): add Action{Create,Delete}Agent to ResourceWorkspace #17932

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2443e19
chore(coderd/rbac): add `Action{Create,Delete}Agent` to `ResourceWork…
DanielleMaywood May 20, 2025
908f679
fix: broken test logic
DanielleMaywood May 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions coderd/apidoc/docs.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 4 additions & 0 deletions coderd/apidoc/swagger.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

25 changes: 22 additions & 3 deletions coderd/database/dbauthz/dbauthz.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,7 @@ var (
// Unsure why provisionerd needs update and read personal
rbac.ResourceUser.Type: {policy.ActionRead, policy.ActionReadPersonal, policy.ActionUpdatePersonal},
rbac.ResourceWorkspaceDormant.Type: {policy.ActionDelete, policy.ActionRead, policy.ActionUpdate, policy.ActionWorkspaceStop},
rbac.ResourceWorkspace.Type: {policy.ActionDelete, policy.ActionRead, policy.ActionUpdate, policy.ActionWorkspaceStart, policy.ActionWorkspaceStop},
rbac.ResourceWorkspace.Type: {policy.ActionDelete, policy.ActionRead, policy.ActionUpdate, policy.ActionWorkspaceStart, policy.ActionWorkspaceStop, policy.ActionCreateAgent},
rbac.ResourceApiKey.Type: {policy.WildcardSymbol},
// When org scoped provisioner credentials are implemented,
// this can be reduced to read a specific org.
Expand Down Expand Up @@ -338,7 +338,7 @@ var (
rbac.ResourceProvisionerDaemon.Type: {policy.ActionCreate, policy.ActionRead, policy.ActionUpdate},
rbac.ResourceUser.Type: rbac.ResourceUser.AvailableActions(),
rbac.ResourceWorkspaceDormant.Type: {policy.ActionUpdate, policy.ActionDelete, policy.ActionWorkspaceStop},
rbac.ResourceWorkspace.Type: {policy.ActionUpdate, policy.ActionDelete, policy.ActionWorkspaceStart, policy.ActionWorkspaceStop, policy.ActionSSH},
rbac.ResourceWorkspace.Type: {policy.ActionUpdate, policy.ActionDelete, policy.ActionWorkspaceStart, policy.ActionWorkspaceStop, policy.ActionSSH, policy.ActionCreateAgent, policy.ActionDeleteAgent},
rbac.ResourceWorkspaceProxy.Type: {policy.ActionCreate, policy.ActionUpdate, policy.ActionDelete},
rbac.ResourceDeploymentConfig.Type: {policy.ActionCreate, policy.ActionUpdate, policy.ActionDelete},
rbac.ResourceNotificationMessage.Type: {policy.ActionCreate, policy.ActionRead, policy.ActionUpdate, policy.ActionDelete},
Expand Down Expand Up @@ -3162,6 +3162,10 @@ func (q *querier) GetWorkspaceByOwnerIDAndName(ctx context.Context, arg database
return fetch(q.log, q.auth, q.db.GetWorkspaceByOwnerIDAndName)(ctx, arg)
}

func (q *querier) GetWorkspaceByResourceID(ctx context.Context, resourceID uuid.UUID) (database.Workspace, error) {
return fetch(q.log, q.auth, q.db.GetWorkspaceByResourceID)(ctx, resourceID)
}

func (q *querier) GetWorkspaceByWorkspaceAppID(ctx context.Context, workspaceAppID uuid.UUID) (database.Workspace, error) {
return fetch(q.log, q.auth, q.db.GetWorkspaceByWorkspaceAppID)(ctx, workspaceAppID)
}
Expand Down Expand Up @@ -3700,9 +3704,24 @@ func (q *querier) InsertWorkspace(ctx context.Context, arg database.InsertWorksp
}

func (q *querier) InsertWorkspaceAgent(ctx context.Context, arg database.InsertWorkspaceAgentParams) (database.WorkspaceAgent, error) {
if err := q.authorizeContext(ctx, policy.ActionCreate, rbac.ResourceSystem); err != nil {
// NOTE(DanielleMaywood):
// Currently, the only way to link a Resource back to a Workspace is by following this chain:
//
// WorkspaceResource -> WorkspaceBuild -> Workspace
//
// It is possible for this function to be called without there existing
// a `WorkspaceBuild` to link back to. This means that we want to allow
// execution to continue if there isn't a workspace found to allow this
// behavior to continue.
workspace, err := q.db.GetWorkspaceByResourceID(ctx, arg.ResourceID)
if err != nil && !errors.Is(err, sql.ErrNoRows) {
return database.WorkspaceAgent{}, err
}

if err := q.authorizeContext(ctx, policy.ActionCreateAgent, workspace); err != nil {
return database.WorkspaceAgent{}, err
}
Comment on lines +3721 to +3723
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the err is sql.ErrNoRows, then this requires site wide permission. So only owner, system, and provisioner can do it. Is that ok?

What does it mean to insert a workspace agent without a workspace build?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With regards to our current test infrastructure, this happens quite often:

  • A workspace is created
  • A provisioner job is created
  • A workspace resource is created
  • A workspace agent is created
  • Maybe A workspace build is created

Unfortunately there is no way we can link back the resource to the workspace. We could always rewrite these tests but that might be a big change.

Another situation where this goes wrong is with workspace templates:

When attempting to update a workspace template, it will fail the job because it cannot insert a workspace agent. I'm not entirely sure why InsertWorkspaceAgent is invoked for the "Build" template flow but that is an example of where I don't think we have an actual workspace to link back to.


return q.db.InsertWorkspaceAgent(ctx, arg)
}

Expand Down
33 changes: 31 additions & 2 deletions coderd/database/dbauthz/dbauthz_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1925,6 +1925,22 @@ func (s *MethodTestSuite) TestWorkspace() {
})
check.Args(ws.ID).Asserts(ws, policy.ActionRead)
}))
s.Run("GetWorkspaceByResourceID", s.Subtest(func(db database.Store, check *expects) {
u := dbgen.User(s.T(), db, database.User{})
o := dbgen.Organization(s.T(), db, database.Organization{})
j := dbgen.ProvisionerJob(s.T(), db, nil, database.ProvisionerJob{Type: database.ProvisionerJobTypeWorkspaceBuild})
tpl := dbgen.Template(s.T(), db, database.Template{CreatedBy: u.ID, OrganizationID: o.ID})
tv := dbgen.TemplateVersion(s.T(), db, database.TemplateVersion{
TemplateID: uuid.NullUUID{UUID: tpl.ID, Valid: true},
JobID: j.ID,
OrganizationID: o.ID,
CreatedBy: u.ID,
})
ws := dbgen.Workspace(s.T(), db, database.WorkspaceTable{OwnerID: u.ID, TemplateID: tpl.ID, OrganizationID: o.ID})
_ = dbgen.WorkspaceBuild(s.T(), db, database.WorkspaceBuild{WorkspaceID: ws.ID, JobID: j.ID, TemplateVersionID: tv.ID})
res := dbgen.WorkspaceResource(s.T(), db, database.WorkspaceResource{JobID: j.ID})
check.Args(res.ID).Asserts(ws, policy.ActionRead)
}))
s.Run("GetWorkspaces", s.Subtest(func(_ database.Store, check *expects) {
// No asserts here because SQLFilter.
check.Args(database.GetWorkspacesParams{}).Asserts()
Expand Down Expand Up @@ -4016,12 +4032,25 @@ func (s *MethodTestSuite) TestSystemFunctions() {
Returns(slice.New(a, b))
}))
s.Run("InsertWorkspaceAgent", s.Subtest(func(db database.Store, check *expects) {
dbtestutil.DisableForeignKeysAndTriggers(s.T(), db)
u := dbgen.User(s.T(), db, database.User{})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we also add a test to validate:

  • A user in org A cannot create or delete an agent in org B?
  • A user in org A cannot delete an agent owned by a different user?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure thing!

o := dbgen.Organization(s.T(), db, database.Organization{})
j := dbgen.ProvisionerJob(s.T(), db, nil, database.ProvisionerJob{Type: database.ProvisionerJobTypeWorkspaceBuild})
tpl := dbgen.Template(s.T(), db, database.Template{CreatedBy: u.ID, OrganizationID: o.ID})
tv := dbgen.TemplateVersion(s.T(), db, database.TemplateVersion{
TemplateID: uuid.NullUUID{UUID: tpl.ID, Valid: true},
JobID: j.ID,
OrganizationID: o.ID,
CreatedBy: u.ID,
})
ws := dbgen.Workspace(s.T(), db, database.WorkspaceTable{OwnerID: u.ID, TemplateID: tpl.ID, OrganizationID: o.ID})
_ = dbgen.WorkspaceBuild(s.T(), db, database.WorkspaceBuild{WorkspaceID: ws.ID, JobID: j.ID, TemplateVersionID: tv.ID})
res := dbgen.WorkspaceResource(s.T(), db, database.WorkspaceResource{JobID: j.ID})
check.Args(database.InsertWorkspaceAgentParams{
ID: uuid.New(),
ResourceID: res.ID,
Name: "dev",
APIKeyScope: database.AgentKeyScopeEnumAll,
}).Asserts(rbac.ResourceSystem, policy.ActionCreate)
}).Asserts(ws, policy.ActionCreateAgent)
}))
s.Run("InsertWorkspaceApp", s.Subtest(func(db database.Store, check *expects) {
dbtestutil.DisableForeignKeysAndTriggers(s.T(), db)
Expand Down
27 changes: 27 additions & 0 deletions coderd/database/dbmem/dbmem.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8035,6 +8035,33 @@ func (q *FakeQuerier) GetWorkspaceByOwnerIDAndName(_ context.Context, arg databa
return database.Workspace{}, sql.ErrNoRows
}

func (q *FakeQuerier) GetWorkspaceByResourceID(ctx context.Context, resourceID uuid.UUID) (database.Workspace, error) {
q.mutex.RLock()
defer q.mutex.RUnlock()

for _, resource := range q.workspaceResources {
if resource.ID != resourceID {
continue
}

for _, build := range q.workspaceBuilds {
if build.JobID != resource.JobID {
continue
}

for _, workspace := range q.workspaces {
if workspace.ID != build.WorkspaceID {
continue
}

return q.extendWorkspace(workspace), nil
}
}
}

return database.Workspace{}, sql.ErrNoRows
}

func (q *FakeQuerier) GetWorkspaceByWorkspaceAppID(_ context.Context, workspaceAppID uuid.UUID) (database.Workspace, error) {
if err := validateDatabaseType(workspaceAppID); err != nil {
return database.Workspace{}, err
Expand Down
7 changes: 7 additions & 0 deletions coderd/database/dbmetrics/querymetrics.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

15 changes: 15 additions & 0 deletions coderd/database/dbmock/dbmock.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions coderd/database/querier.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

59 changes: 59 additions & 0 deletions coderd/database/queries.sql.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

24 changes: 24 additions & 0 deletions coderd/database/queries/workspaces.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,30 @@ WHERE
LIMIT
1;

-- name: GetWorkspaceByResourceID :one
SELECT
*
FROM
workspaces_expanded as workspaces
WHERE
workspaces.id = (
SELECT
workspace_id
FROM
workspace_builds
WHERE
workspace_builds.job_id = (
SELECT
job_id
FROM
workspace_resources
WHERE
workspace_resources.id = @resource_id
)
)
LIMIT
1;

-- name: GetWorkspaceByWorkspaceAppID :one
SELECT
*
Expand Down
6 changes: 6 additions & 0 deletions coderd/rbac/object_gen.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 6 additions & 0 deletions coderd/rbac/policy/policy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,9 @@ const (

ActionReadPersonal Action = "read_personal"
ActionUpdatePersonal Action = "update_personal"

ActionCreateAgent Action = "create_agent"
ActionDeleteAgent Action = "delete_agent"
)

type PermissionDefinition struct {
Expand Down Expand Up @@ -67,6 +70,9 @@ var workspaceActions = map[Action]ActionDefinition{
// Running a workspace
ActionSSH: actDef("ssh into a given workspace"),
ActionApplicationConnect: actDef("connect to workspace apps via browser"),

ActionCreateAgent: actDef("create a new workspace agent"),
ActionDeleteAgent: actDef("delete an existing workspace agent"),
}

// RBACPermissions is indexed by the type
Expand Down
Loading
Loading