chore: implement 'use' verb to template object, read has less scope now
#16075
Conversation
| @@ -191,6 +191,51 @@ func TestCreateWorkspace(t *testing.T) { | |||
| require.Equal(t, http.StatusBadRequest, apiErr.StatusCode()) | |||
| require.Contains(t, apiErr.Message, "doesn't exist") | |||
| }) | |||
|
|
|||
| // Auditors cannot "use" templates, they can only read them. | |||
| t.Run("Auditor", func(t *testing.T) { | |||
There was a problem hiding this comment.
This passes without an error on main. Now auditors get a 403 Forbidden if they do not have use perms from somewhere else
| func TemplateRoleActions(role codersdk.TemplateRole) []policy.Action { | ||
| switch role { | ||
| case codersdk.TemplateRoleAdmin: | ||
| return []policy.Action{policy.WildcardSymbol} | ||
| case codersdk.TemplateRoleUse: | ||
| return []policy.Action{policy.ActionRead, policy.ActionUse} | ||
| } | ||
| return []policy.Action{} | ||
| } |
There was a problem hiding this comment.
I wanted to place this in codersdk, but then I'd have to import policy. And we already have codersdk.RBACAction.
TemplateRole should probably be a database enum. At present it only exists in codersdk.
| -- 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.
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.
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
The admin role sets ['*']. So we only have 2 cases in the database today
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) | ||
| if err != nil { | ||
| return database.WorkspaceTable{}, xerrors.Errorf("verify template by id: %w", err) | ||
| } |
There was a problem hiding this comment.
This does check the template read perm again, but we cache all authz requests per ctx. 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.
Do we cache the resources themselves, or just the authz decisions?
There was a problem hiding this comment.
We only cache the authz decisions. The template fetch will happen again unfortunately (from a performance POV).
ffee414 to
36d329c
Compare
| tpl, err := q.GetTemplateByID(ctx, arg.TemplateID) | ||
| if err != nil { | ||
| return database.WorkspaceTable{}, xerrors.Errorf("verify template by id: %w", err) | ||
| } |
There was a problem hiding this comment.
Do we cache the resources themselves, or just the authz decisions?
read has less scope now
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 checked when creating a workspace. If you already have a workspace, you are able to start/stop/delete/etc the workspace even withoutuse.template.