- Notifications
You must be signed in to change notification settings - Fork928
chore: introduceResourceWorkspaceAgent RBAC resource#17824
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
ResourceWorkspaceAgentResourceWorkspaceAgent RBAC resourceThere 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.
Will the roles change/introduce new one later for creation as essentially in the future won't the agent be creating new agents?
Other than the above question, seems OK to me but will defer to someone deeper in the RBAC trenches. 😄
I'll be honest in saying I'm not entirely sure. I know that creating a new subject, specifically for the new devcontainer agent API work, with permissions to Where my RBAC knowledge is more limited isdoes this lock it down enough?. This PR should be better than the status quo (abusing |
@DanielleMaywood feel free to throw a time slot on my calendar if you want to chat this through. I've found these conversations done async tend to draw out too long, as they are a bit nuanced. Our RBAC system is not very flexible, so it often feels like trying to fit a square peg into a round hole. It was just the easiest shape to carve out that fits in most hole well enough 😆 |
| "workspace_agent": { | ||
| Actions: map[Action]ActionDefinition{ | ||
| ActionRead: actDef("read workspace agent"), | ||
| ActionDelete: actDef("delete workspace agent"), |
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 don't think we actually ever delete workspace agents, so this istechnically not required (yet)
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.
Correct, we don'tyet, although we will be in the upcoming devcontainer agents work. I'm happy to remove theDelete action now if that makes more sense?
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.
That's a great reason to leave it in :)
| @@ -3014,14 +3017,14 @@ func (q *querier) GetWorkspaceAgentUsageStatsAndLabels(ctx context.Context, crea | |||
| // GetWorkspaceAgentsByResourceIDs | |||
| // The workspace/job is already fetched. | |||
| func (q *querier) GetWorkspaceAgentsByResourceIDs(ctx context.Context, ids []uuid.UUID) ([]database.WorkspaceAgent, error) { | |||
| if err := q.authorizeContext(ctx, policy.ActionRead, rbac.ResourceSystem); err != nil { | |||
| if err := q.authorizeContext(ctx, policy.ActionRead, rbac.ResourceWorkspaceAgent); err != nil { | |||
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 this needs to be scoped by who ultimately "owns" the workspace agent (which is generally the owner of the associated workspace).
Can a member read an owner's workspace agent?
Can a member read another owner's workspace agent?
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.
You're absolutely right. As is, the change is status quo over the previous implementation but ultimately it would make more sense to make itbetter. Thanks 👍
| @@ -3691,7 +3694,7 @@ 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 { | |||
| if err := q.authorizeContext(ctx, policy.ActionCreate, rbac.ResourceWorkspaceAgent); err != nil { | |||
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 a member insert a workspace agent belonging to another 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.
This would possibly allow that, yes. I'll have a chat with@Emyrk to ensure that doesn't happen.
DanielleMaywood commentedMay 14, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@Emyrk Thanks! Will do 😄 |
After a discussion with@Emyrk last night, I've decided to take a slightly different approach to this. Will close this PR and open a new one 👍 |
This PR introduces a new
WorkspaceAgentRBAC resource as part ofcoder/internal#619.I've replaced a few instances of
ResourceSystemwith the newResourceWorkspaceAgentwhere applicable to move us away from abusingResourceSystem.Related#17823