- Notifications
You must be signed in to change notification settings - Fork1k
chore: implement 'use' verb to template object,read has less scope now#16075
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
| }) | ||
| // Auditors cannot "use" templates, they can only read them. | ||
| t.Run("Auditor",func(t*testing.T) { |
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 passes without an error onmain. Now auditors get a 403 Forbidden if they do not haveuse perms from somewhere else
Uh oh!
There was an error while loading.Please reload this page.
| funcTemplateRoleActions(role codersdk.TemplateRole) []policy.Action { | ||
| switchrole { | ||
| casecodersdk.TemplateRoleAdmin: | ||
| return []policy.Action{policy.WildcardSymbol} | ||
| casecodersdk.TemplateRoleUse: | ||
| return []policy.Action{policy.ActionRead,policy.ActionUse} | ||
| } | ||
| return []policy.Action{} | ||
| } |
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 wanted to place this incodersdk, but then I'd have to importpolicy. And we already havecodersdk.RBACAction.
TemplateRole should probably be a database enum. At present it only exists incodersdk.
| -- Instead of trying to write a complicated SQL query to update the JSONB | ||
| -- object, a string replace is much simpler and easier to understand. | ||
| -- Both pieces of text are JSON arrays, so this safe to do. | ||
| group_acl= replace(group_acl::text,'["read"]','["read", "use"]')::jsonb, |
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.
Is it possible to have ACLs like["read", "update"] in this list? Wouldn't those need to be updated?
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.
No it is not. The only values a user can pass in is in this enum list:
https://github.com/coder/coder/blob/main/codersdk/templates.go#L169-L176
Theadmin role sets['*']. So we only have 2 cases in the database today
Uh oh!
There was an error while loading.Please reload this page.
The I am also keeping the check in the api call to fail fast if the permission is missing. Not having to do all the build work if it will inevitably fail. |
| tpl,err:=q.GetTemplateByID(ctx,arg.TemplateID) | ||
| iferr!=nil { | ||
| return database.WorkspaceTable{},xerrors.Errorf("verify template by id: %w",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.
This does check the templateread perm again, but we cache all authz requests perctx. So this check is going to be a cache lookup assuming it came from a normal code path.
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.
Do we cache the resources themselves, or just the authz decisions?
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.
We only cache the authz decisions. The template fetch will happen again unfortunately (from a performance POV).
ffee414 to36d329cCompareThere 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.
LGTM
| tpl,err:=q.GetTemplateByID(ctx,arg.TemplateID) | ||
| iferr!=nil { | ||
| return database.WorkspaceTable{},xerrors.Errorf("verify template by id: %w",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.
Do we cache the resources themselves, or just the authz decisions?
read has less scope nowf34e6fd intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Closes#15891
Template
useis now a verb.useall templates (org template admins same in org)useperm from theeveryonegroup in thegroup_acl.use.templatepermission is now checkedwhen creating a workspace. If you already have a workspace, you are able to start/stop/delete/etc the workspace even withoutuse.template.