chore(coderd/rbac): add Action{Create,Delete}Agent to ResourceWorkspace
#17932
Conversation
…space` This PR adds two new actions: `ActionCreateAgent` and `ActionDeleteAgent`. The former is used in this PR for `InsertWorkspaceAgent`, with the latter to be used in a follow-up PR for Dev Container Agents. A note has been left in `dbauthz.go` for `InsertWorkspaceAgent` detailing why it is allowed to insert a workspace agent when no workspace could be found.
DanielleMaywood
requested review from
Emyrk and
johnstcn
and removed request for
Emyrk
| @@ -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{}) | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Sure thing!
| { | ||
| Name: "CreateDeleteWorkspaceAgent", | ||
| Actions: []policy.Action{policy.ActionCreateAgent, policy.ActionDeleteAgent}, | ||
| Resource: rbac.ResourceWorkspace.WithID(workspaceID).InOrg(orgID).WithOwner(currentUser.String()), | ||
| AuthorizeMap: map[bool][]hasAuthSubjects{ | ||
| true: {owner, orgMemberMe, orgAdmin}, | ||
| false: {setOtherOrg, memberMe, userAdmin, templateAdmin, orgTemplateAdmin, orgUserAdmin, orgAuditor, orgMemberMeBanWorkspace}, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
@johnstcn Does this not already cover the tests you've suggested?
There was a problem hiding this comment.
I think you're right actually!
| if err := q.authorizeContext(ctx, policy.ActionCreateAgent, workspace); err != nil { | ||
| return database.WorkspaceAgent{}, err | ||
| } |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
This PR adds two new actions:
ActionCreateAgentandActionDeleteAgent. The former is used in this PR forInsertWorkspaceAgent, with the latter to be used in a follow-up PR for Dev Container Agents.A note has been left in
dbauthz.goforInsertWorkspaceAgentdetailing why it is allowed to insert a workspace agent when no workspace could be found.