- Notifications
You must be signed in to change notification settings - Fork1.1k
chore(coderd/rbac): addAction{Create,Delete}Agent toResourceWorkspace#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.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
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 forDev Container Agents.A note has been left in `dbauthz.go` for `InsertWorkspaceAgent`detailing why it is allowed to insert a workspace agent when noworkspace could be found.
| })) | ||
| 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.
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 Acannot create or delete an agent in org B?
- A user in org Acannot delete an agent owned by a different user?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@johnstcn Does this not already cover the tests you've suggested?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think you're right actually!
johnstcn left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Approving, but would like a second opinion from@Emyrk
| iferr:=q.authorizeContext(ctx,policy.ActionCreateAgent,workspace);err!=nil { | ||
| return database.WorkspaceAgent{},err | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
If the err issql.ErrNoRows, then this requires site wide permission. So onlyowner,system, andprovisioner 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
With regards to our currenttest 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 whyInsertWorkspaceAgent is invoked for the "Build" template flow but that is an example of where I don't think we have anactual workspace to link back to.
Emyrk left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
My comment is not blocking
3e7ff9d intomainUh oh!
There was an error while loading.Please reload this page.
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.