Skip to content

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 14 commits into from
Jan 17, 2025
11 changes: 11 additions & 0 deletions coderd/database/db2sdk/db2sdk.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import (

"github.com/coder/coder/v2/coderd/database"
"github.com/coder/coder/v2/coderd/rbac"
"github.com/coder/coder/v2/coderd/rbac/policy"
"github.com/coder/coder/v2/coderd/render"
"github.com/coder/coder/v2/coderd/workspaceapps/appurl"
"github.com/coder/coder/v2/codersdk"
Expand Down Expand Up @@ -694,3 +695,13 @@ func MatchedProvisioners(provisionerDaemons []database.ProvisionerDaemon, now ti
}
return matched
}

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{}
}
Comment on lines +699 to +707
Copy link
Member Author

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 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.

8 changes: 8 additions & 0 deletions coderd/database/dbauthz/dbauthz.go
Original file line number Diff line number Diff line change
Expand Up @@ -3165,6 +3165,14 @@ func (q *querier) InsertUserLink(ctx context.Context, arg database.InsertUserLin

func (q *querier) InsertWorkspace(ctx context.Context, arg database.InsertWorkspaceParams) (database.WorkspaceTable, error) {
obj := rbac.ResourceWorkspace.WithOwner(arg.OwnerID.String()).InOrg(arg.OrganizationID)
tpl, err := q.GetTemplateByID(ctx, arg.TemplateID)
if err != nil {
return database.WorkspaceTable{}, xerrors.Errorf("verify template by id: %w", err)
}
Comment on lines +3172 to +3175
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

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?

Copy link
Member Author

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).

if err := q.authorizeContext(ctx, policy.ActionUse, tpl); err != nil {
return database.WorkspaceTable{}, xerrors.Errorf("use template for workspace: %w", err)
}

return insert(q.log, q.auth, obj, q.db.InsertWorkspace)(ctx, arg)
}

Expand Down
2 changes: 1 addition & 1 deletion coderd/database/dbauthz/dbauthz_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2458,7 +2458,7 @@ func (s *MethodTestSuite) TestWorkspace() {
OrganizationID: o.ID,
AutomaticUpdates: database.AutomaticUpdatesNever,
TemplateID: tpl.ID,
}).Asserts(rbac.ResourceWorkspace.WithOwner(u.ID.String()).InOrg(o.ID), policy.ActionCreate)
}).Asserts(tpl, policy.ActionRead, tpl, policy.ActionUse, rbac.ResourceWorkspace.WithOwner(u.ID.String()).InOrg(o.ID), policy.ActionCreate)
}))
s.Run("Start/InsertWorkspaceBuild", s.Subtest(func(db database.Store, check *expects) {
u := dbgen.User(s.T(), db, database.User{})
Expand Down
5 changes: 3 additions & 2 deletions coderd/database/dbgen/dbgen.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,13 @@ import (
"golang.org/x/xerrors"

"github.com/coder/coder/v2/coderd/database"
"github.com/coder/coder/v2/coderd/database/db2sdk"
"github.com/coder/coder/v2/coderd/database/dbauthz"
"github.com/coder/coder/v2/coderd/database/dbtime"
"github.com/coder/coder/v2/coderd/database/provisionerjobs"
"github.com/coder/coder/v2/coderd/database/pubsub"
"github.com/coder/coder/v2/coderd/rbac"
"github.com/coder/coder/v2/coderd/rbac/policy"
"github.com/coder/coder/v2/codersdk"
"github.com/coder/coder/v2/cryptorand"
"github.com/coder/coder/v2/testutil"
)
Expand Down Expand Up @@ -75,7 +76,7 @@ func Template(t testing.TB, db database.Store, seed database.Template) database.
if seed.GroupACL == nil {
// By default, all users in the organization can read the template.
seed.GroupACL = database.TemplateACL{
seed.OrganizationID.String(): []policy.Action{policy.ActionRead},
seed.OrganizationID.String(): db2sdk.TemplateRoleActions(codersdk.TemplateRoleUse),
}
}
if seed.UserACL == nil {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
UPDATE
templates
SET
group_acl = replace(group_acl::text, '["read", "use"]', '["read"]')::jsonb,
user_acl = replace(user_acl::text, '["read", "use"]', '["read"]')::jsonb
12 changes: 12 additions & 0 deletions coderd/database/migrations/000284_template_read_to_use.up.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
-- With the "use" verb now existing for templates, we need to update the acl's to
-- include "use" where the permissions set ["read"] is present.
-- The other permission set is ["*"] which is unaffected.

UPDATE
templates
SET
-- 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,
user_acl = replace(user_acl::text, '["read"]', '["read", "use"]')::jsonb
6 changes: 3 additions & 3 deletions coderd/insights_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,12 +23,12 @@ import (
agentproto "github.com/coder/coder/v2/agent/proto"
"github.com/coder/coder/v2/coderd/coderdtest"
"github.com/coder/coder/v2/coderd/database"
"github.com/coder/coder/v2/coderd/database/db2sdk"
"github.com/coder/coder/v2/coderd/database/dbauthz"
"github.com/coder/coder/v2/coderd/database/dbgen"
"github.com/coder/coder/v2/coderd/database/dbrollup"
"github.com/coder/coder/v2/coderd/database/dbtestutil"
"github.com/coder/coder/v2/coderd/rbac"
"github.com/coder/coder/v2/coderd/rbac/policy"
"github.com/coder/coder/v2/coderd/workspaceapps"
"github.com/coder/coder/v2/coderd/workspacestats"
"github.com/coder/coder/v2/codersdk"
Expand Down Expand Up @@ -675,7 +675,7 @@ func TestTemplateInsights_Golden(t *testing.T) {
OrganizationID: firstUser.OrganizationID,
CreatedBy: firstUser.UserID,
GroupACL: database.TemplateACL{
firstUser.OrganizationID.String(): []policy.Action{policy.ActionRead},
firstUser.OrganizationID.String(): db2sdk.TemplateRoleActions(codersdk.TemplateRoleUse),
},
})
err := db.UpdateTemplateVersionByID(context.Background(), database.UpdateTemplateVersionByIDParams{
Expand Down Expand Up @@ -1573,7 +1573,7 @@ func TestUserActivityInsights_Golden(t *testing.T) {
OrganizationID: firstUser.OrganizationID,
CreatedBy: firstUser.UserID,
GroupACL: database.TemplateACL{
firstUser.OrganizationID.String(): []policy.Action{policy.ActionRead},
firstUser.OrganizationID.String(): db2sdk.TemplateRoleActions(codersdk.TemplateRoleUse),
},
})
err := db.UpdateTemplateVersionByID(context.Background(), database.UpdateTemplateVersionByIDParams{
Expand Down
1 change: 1 addition & 0 deletions coderd/rbac/object_gen.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions coderd/rbac/policy/policy.go
Original file line number Diff line number Diff line change
Expand Up @@ -133,8 +133,8 @@ var RBACPermissions = map[string]PermissionDefinition{
},
"template": {
Actions: map[Action]ActionDefinition{
ActionCreate: actDef("create a template"),
// TODO: Create a use permission maybe?
ActionCreate: actDef("create a template"),
ActionUse: actDef("use the template to initially create a workspace, then workspace lifecycle permissions take over"),
ActionRead: actDef("read template"),
ActionUpdate: actDef("update a template"),
ActionDelete: actDef("delete a template"),
Expand Down
4 changes: 2 additions & 2 deletions coderd/rbac/roles.go
Original file line number Diff line number Diff line change
Expand Up @@ -318,7 +318,7 @@ func ReloadBuiltinRoles(opts *RoleOptions) {
Identifier: RoleTemplateAdmin(),
DisplayName: "Template Admin",
Site: Permissions(map[string][]policy.Action{
ResourceTemplate.Type: {policy.ActionCreate, policy.ActionRead, policy.ActionUpdate, policy.ActionDelete, policy.ActionViewInsights},
ResourceTemplate.Type: ResourceTemplate.AvailableActions(),
// CRUD all files, even those they did not upload.
ResourceFile.Type: {policy.ActionCreate, policy.ActionRead},
ResourceWorkspace.Type: {policy.ActionRead},
Expand Down Expand Up @@ -476,7 +476,7 @@ func ReloadBuiltinRoles(opts *RoleOptions) {
Site: []Permission{},
Org: map[string][]Permission{
organizationID.String(): Permissions(map[string][]policy.Action{
ResourceTemplate.Type: {policy.ActionCreate, policy.ActionRead, policy.ActionUpdate, policy.ActionDelete, policy.ActionViewInsights},
ResourceTemplate.Type: ResourceTemplate.AvailableActions(),
ResourceFile.Type: {policy.ActionCreate, policy.ActionRead},
ResourceWorkspace.Type: {policy.ActionRead},
// Assigning template perms requires this permission.
Expand Down
11 changes: 11 additions & 0 deletions coderd/rbac/roles_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -232,6 +232,17 @@ func TestRolePermissions(t *testing.T) {
false: {setOtherOrg, orgAuditor, orgUserAdmin, memberMe, userAdmin, orgMemberMe},
},
},
{
Name: "UseTemplates",
Actions: []policy.Action{policy.ActionUse},
Resource: rbac.ResourceTemplate.InOrg(orgID).WithGroupACL(map[string][]policy.Action{
groupID.String(): {policy.ActionUse},
}),
AuthorizeMap: map[bool][]hasAuthSubjects{
true: {owner, orgAdmin, templateAdmin, orgTemplateAdmin, groupMemberMe},
false: {setOtherOrg, orgAuditor, orgUserAdmin, memberMe, userAdmin, orgMemberMe},
},
},
{
Name: "Files",
Actions: []policy.Action{policy.ActionCreate},
Expand Down
3 changes: 2 additions & 1 deletion coderd/templates.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import (
"golang.org/x/xerrors"

"cdr.dev/slog"
"github.com/coder/coder/v2/coderd/database/db2sdk"

"github.com/coder/coder/v2/coderd/audit"
"github.com/coder/coder/v2/coderd/database"
Expand Down Expand Up @@ -382,7 +383,7 @@ func (api *API) postTemplateByOrganization(rw http.ResponseWriter, r *http.Reque
if !createTemplate.DisableEveryoneGroupAccess {
// The organization ID is used as the group ID for the everyone group
// in this organization.
defaultsGroups[organization.ID.String()] = []policy.Action{policy.ActionRead}
defaultsGroups[organization.ID.String()] = db2sdk.TemplateRoleActions(codersdk.TemplateRoleUse)
}
err = api.Database.InTx(func(tx database.Store) error {
now := dbtime.Now()
Expand Down
12 changes: 12 additions & 0 deletions coderd/workspaces.go
Original file line number Diff line number Diff line change
Expand Up @@ -525,6 +525,18 @@ func createWorkspace(
httpapi.ResourceNotFound(rw)
return
}
// The user also needs permission to use the template. At this point they have
// read perms, but not necessarily "use". This is also checked in `db.InsertWorkspace`.
// Doing this up front can save some work below if the user doesn't have permission.
if !api.Authorize(r, policy.ActionUse, template) {
httpapi.Write(ctx, rw, http.StatusForbidden, codersdk.Response{
Message: fmt.Sprintf("Unauthorized access to use the template %q.", template.Name),
Detail: "Although you are able to view the template, you are unable to create a workspace using it. " +
"Please contact an administrator about your permissions if you feel this is an error.",
Validations: nil,
})
return
}

templateAccessControl := (*(api.AccessControlStore.Load())).GetTemplateAccessControl(template)
if templateAccessControl.IsDeprecated() {
Expand Down
2 changes: 1 addition & 1 deletion codersdk/rbacresources_gen.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

22 changes: 6 additions & 16 deletions enterprise/coderd/templates.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import (
"github.com/coder/coder/v2/coderd/httpapi"
"github.com/coder/coder/v2/coderd/httpmw"
"github.com/coder/coder/v2/coderd/rbac/policy"
"github.com/coder/coder/v2/coderd/util/slice"
"github.com/coder/coder/v2/codersdk"
)

Expand Down Expand Up @@ -222,7 +223,7 @@ func (api *API) patchTemplateACL(rw http.ResponseWriter, r *http.Request) {
delete(template.UserACL, id)
continue
}
template.UserACL[id] = convertSDKTemplateRole(role)
template.UserACL[id] = db2sdk.TemplateRoleActions(role)
}
}

Expand All @@ -234,7 +235,7 @@ func (api *API) patchTemplateACL(rw http.ResponseWriter, r *http.Request) {
delete(template.GroupACL, id)
continue
}
template.GroupACL[id] = convertSDKTemplateRole(role)
template.GroupACL[id] = db2sdk.TemplateRoleActions(role)
}
}

Expand Down Expand Up @@ -316,8 +317,8 @@ func convertTemplateUsers(tus []database.TemplateUser, orgIDsByUserIDs map[uuid.
}

func validateTemplateRole(role codersdk.TemplateRole) error {
actions := convertSDKTemplateRole(role)
if actions == nil && role != codersdk.TemplateRoleDeleted {
actions := db2sdk.TemplateRoleActions(role)
if len(actions) == 0 && role != codersdk.TemplateRoleDeleted {
return xerrors.Errorf("role %q is not a valid Template role", role)
}

Expand All @@ -326,7 +327,7 @@ func validateTemplateRole(role codersdk.TemplateRole) error {

func convertToTemplateRole(actions []policy.Action) codersdk.TemplateRole {
switch {
case len(actions) == 1 && actions[0] == policy.ActionRead:
case len(actions) == 2 && slice.SameElements(actions, []policy.Action{policy.ActionUse, policy.ActionRead}):
return codersdk.TemplateRoleUse
case len(actions) == 1 && actions[0] == policy.WildcardSymbol:
return codersdk.TemplateRoleAdmin
Expand All @@ -335,17 +336,6 @@ func convertToTemplateRole(actions []policy.Action) codersdk.TemplateRole {
return ""
}

func convertSDKTemplateRole(role codersdk.TemplateRole) []policy.Action {
switch role {
case codersdk.TemplateRoleAdmin:
return []policy.Action{policy.WildcardSymbol}
case codersdk.TemplateRoleUse:
return []policy.Action{policy.ActionRead}
}

return nil
}

// TODO move to api.RequireFeatureMW when we are OK with changing the behavior.
func (api *API) templateRBACEnabledMW(next http.Handler) http.Handler {
return api.RequireFeatureMW(codersdk.FeatureTemplateRBAC)(next)
Expand Down
47 changes: 47 additions & 0 deletions enterprise/coderd/workspaces_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,53 @@ 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) {
Copy link
Member Author

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 on main. Now auditors get a 403 Forbidden if they do not have use perms from somewhere else

t.Parallel()

owner, first := coderdenttest.New(t, &coderdenttest.Options{
Options: &coderdtest.Options{
IncludeProvisionerDaemon: true,
},
LicenseOptions: &coderdenttest.LicenseOptions{
Features: license.Features{
codersdk.FeatureTemplateRBAC: 1,
codersdk.FeatureMultipleOrganizations: 1,
},
},
})

// A member of the org as an auditor
auditor, _ := coderdtest.CreateAnotherUser(t, owner, first.OrganizationID, rbac.RoleAuditor())

ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
defer cancel()

// Given: a template with a version without the "use" permission on everyone
version := coderdtest.CreateTemplateVersion(t, owner, first.OrganizationID, nil)
_ = coderdtest.AwaitTemplateVersionJobCompleted(t, owner, version.ID)
template := coderdtest.CreateTemplate(t, owner, first.OrganizationID, version.ID)

//nolint:gocritic // This should be run as the owner user.
err := owner.UpdateTemplateACL(ctx, template.ID, codersdk.UpdateTemplateACL{
UserPerms: nil,
GroupPerms: map[string]codersdk.TemplateRole{
first.OrganizationID.String(): codersdk.TemplateRoleDeleted,
},
})
require.NoError(t, err)

_, err = auditor.CreateUserWorkspace(ctx, codersdk.Me, codersdk.CreateWorkspaceRequest{
TemplateID: template.ID,
Name: "workspace",
})
require.Error(t, err)
var apiErr *codersdk.Error
require.ErrorAs(t, err, &apiErr)
require.Equal(t, http.StatusForbidden, apiErr.StatusCode())
require.Contains(t, apiErr.Message, "Unauthorized access to use the template")
})
}

func TestCreateUserWorkspace(t *testing.T) {
Expand Down
1 change: 1 addition & 0 deletions site/src/api/rbacresourcesGenerated.ts
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,7 @@ export const RBACResourceActions: Partial<
delete: "delete a template",
read: "read template",
update: "update a template",
use: "use the template to initially create a workspace, then workspace lifecycle permissions take over",
view_insights: "view insights",
},
user: {
Expand Down
Loading