chore: remove UpsertCustomRole in favor of Insert + Update (#14217)

* chore: remove UpsertCustomRole in favor of Insert + Update

---------

Co-authored-by: Jaayden Halko <jaayden.halko@gmail.com>
This commit is contained in:
Steven Masley
2024-08-13 12:53:47 -05:00
committed by GitHub
parent 712a1b50d8
commit 84fdfd2a18
39 changed files with 1085 additions and 452 deletions

View File

@ -19,8 +19,8 @@ import (
"github.com/coder/coder/v2/testutil"
)
// TestUpsertCustomRoles verifies creating custom roles cannot escalate permissions.
func TestUpsertCustomRoles(t *testing.T) {
// TestInsertCustomRoles verifies creating custom roles cannot escalate permissions.
func TestInsertCustomRoles(t *testing.T) {
t.Parallel()
userID := uuid.New()
@ -98,7 +98,7 @@ func TestUpsertCustomRoles(t *testing.T) {
org: codersdk.CreatePermissions(map[codersdk.RBACResource][]codersdk.RBACAction{
codersdk.ResourceWorkspace: {codersdk.ActionRead},
}),
errorContains: "cannot assign both org and site permissions",
errorContains: "organization roles specify site or user permissions",
},
{
name: "invalid-action",
@ -231,7 +231,7 @@ func TestUpsertCustomRoles(t *testing.T) {
ctx := testutil.Context(t, testutil.WaitMedium)
ctx = dbauthz.As(ctx, subject)
_, err := az.UpsertCustomRole(ctx, database.UpsertCustomRoleParams{
_, err := az.InsertCustomRole(ctx, database.InsertCustomRoleParams{
Name: "test-role",
DisplayName: "",
OrganizationID: tc.organizationID,

View File

@ -815,6 +815,86 @@ func (q *querier) customRoleEscalationCheck(ctx context.Context, actor rbac.Subj
return nil
}
// customRoleCheck will validate a custom role for inserting or updating.
// If the role is not valid, an error will be returned.
// - Check custom roles are valid for their resource types + actions
// - Check the actor can create the custom role
// - Check the custom role does not grant perms the actor does not have
// - Prevent negative perms
// - Prevent roles with site and org permissions.
func (q *querier) customRoleCheck(ctx context.Context, role database.CustomRole) error {
act, ok := ActorFromContext(ctx)
if !ok {
return NoActorError
}
// Org permissions require an org role
if role.OrganizationID.UUID == uuid.Nil && len(role.OrgPermissions) > 0 {
return xerrors.Errorf("organization permissions require specifying an organization id")
}
// Org roles can only specify org permissions
if role.OrganizationID.UUID != uuid.Nil && (len(role.SitePermissions) > 0 || len(role.UserPermissions) > 0) {
return xerrors.Errorf("organization roles specify site or user permissions")
}
// The rbac.Role has a 'Valid()' function on it that will do a lot
// of checks.
rbacRole, err := rolestore.ConvertDBRole(database.CustomRole{
Name: role.Name,
DisplayName: role.DisplayName,
SitePermissions: role.SitePermissions,
OrgPermissions: role.OrgPermissions,
UserPermissions: role.UserPermissions,
OrganizationID: role.OrganizationID,
})
if err != nil {
return xerrors.Errorf("invalid args: %w", err)
}
err = rbacRole.Valid()
if err != nil {
return xerrors.Errorf("invalid role: %w", err)
}
if len(rbacRole.Org) > 0 && len(rbacRole.Site) > 0 {
// This is a choice to keep roles simple. If we allow mixing site and org scoped perms, then knowing who can
// do what gets more complicated.
return xerrors.Errorf("invalid custom role, cannot assign both org and site permissions at the same time")
}
if len(rbacRole.Org) > 1 {
// Again to avoid more complexity in our roles
return xerrors.Errorf("invalid custom role, cannot assign permissions to more than 1 org at a time")
}
// Prevent escalation
for _, sitePerm := range rbacRole.Site {
err := q.customRoleEscalationCheck(ctx, act, sitePerm, rbac.Object{Type: sitePerm.ResourceType})
if err != nil {
return xerrors.Errorf("site permission: %w", err)
}
}
for orgID, perms := range rbacRole.Org {
for _, orgPerm := range perms {
err := q.customRoleEscalationCheck(ctx, act, orgPerm, rbac.Object{OrgID: orgID, Type: orgPerm.ResourceType})
if err != nil {
return xerrors.Errorf("org=%q: %w", orgID, err)
}
}
}
for _, userPerm := range rbacRole.User {
err := q.customRoleEscalationCheck(ctx, act, userPerm, rbac.Object{Type: userPerm.ResourceType, Owner: act.ID})
if err != nil {
return xerrors.Errorf("user permission: %w", err)
}
}
return nil
}
func (q *querier) AcquireLock(ctx context.Context, id int64) error {
return q.db.AcquireLock(ctx, id)
}
@ -2551,6 +2631,34 @@ func (q *querier) InsertAuditLog(ctx context.Context, arg database.InsertAuditLo
return insert(q.log, q.auth, rbac.ResourceAuditLog, q.db.InsertAuditLog)(ctx, arg)
}
func (q *querier) InsertCustomRole(ctx context.Context, arg database.InsertCustomRoleParams) (database.CustomRole, error) {
// Org and site role upsert share the same query. So switch the assertion based on the org uuid.
if arg.OrganizationID.UUID != uuid.Nil {
if err := q.authorizeContext(ctx, policy.ActionCreate, rbac.ResourceAssignOrgRole.InOrg(arg.OrganizationID.UUID)); err != nil {
return database.CustomRole{}, err
}
} else {
if err := q.authorizeContext(ctx, policy.ActionCreate, rbac.ResourceAssignRole); err != nil {
return database.CustomRole{}, err
}
}
if err := q.customRoleCheck(ctx, database.CustomRole{
Name: arg.Name,
DisplayName: arg.DisplayName,
SitePermissions: arg.SitePermissions,
OrgPermissions: arg.OrgPermissions,
UserPermissions: arg.UserPermissions,
CreatedAt: time.Now(),
UpdatedAt: time.Now(),
OrganizationID: arg.OrganizationID,
ID: uuid.New(),
}); err != nil {
return database.CustomRole{}, err
}
return q.db.InsertCustomRole(ctx, arg)
}
func (q *querier) InsertDBCryptKey(ctx context.Context, arg database.InsertDBCryptKeyParams) error {
if err := q.authorizeContext(ctx, policy.ActionCreate, rbac.ResourceSystem); err != nil {
return err
@ -3002,6 +3110,33 @@ func (q *querier) UpdateAPIKeyByID(ctx context.Context, arg database.UpdateAPIKe
return update(q.log, q.auth, fetch, q.db.UpdateAPIKeyByID)(ctx, arg)
}
func (q *querier) UpdateCustomRole(ctx context.Context, arg database.UpdateCustomRoleParams) (database.CustomRole, error) {
if arg.OrganizationID.UUID != uuid.Nil {
if err := q.authorizeContext(ctx, policy.ActionUpdate, rbac.ResourceAssignOrgRole.InOrg(arg.OrganizationID.UUID)); err != nil {
return database.CustomRole{}, err
}
} else {
if err := q.authorizeContext(ctx, policy.ActionUpdate, rbac.ResourceAssignRole); err != nil {
return database.CustomRole{}, err
}
}
if err := q.customRoleCheck(ctx, database.CustomRole{
Name: arg.Name,
DisplayName: arg.DisplayName,
SitePermissions: arg.SitePermissions,
OrgPermissions: arg.OrgPermissions,
UserPermissions: arg.UserPermissions,
CreatedAt: time.Now(),
UpdatedAt: time.Now(),
OrganizationID: arg.OrganizationID,
ID: uuid.New(),
}); err != nil {
return database.CustomRole{}, err
}
return q.db.UpdateCustomRole(ctx, arg)
}
func (q *querier) UpdateExternalAuthLink(ctx context.Context, arg database.UpdateExternalAuthLinkParams) (database.ExternalAuthLink, error) {
fetch := func(ctx context.Context, arg database.UpdateExternalAuthLinkParams) (database.ExternalAuthLink, error) {
return q.db.GetExternalAuthLink(ctx, database.GetExternalAuthLinkParams{UserID: arg.UserID, ProviderID: arg.ProviderID})
@ -3664,91 +3799,6 @@ func (q *querier) UpsertApplicationName(ctx context.Context, value string) error
return q.db.UpsertApplicationName(ctx, value)
}
// UpsertCustomRole does a series of authz checks to protect custom roles.
// - Check custom roles are valid for their resource types + actions
// - Check the actor can create the custom role
// - Check the custom role does not grant perms the actor does not have
// - Prevent negative perms
// - Prevent roles with site and org permissions.
func (q *querier) UpsertCustomRole(ctx context.Context, arg database.UpsertCustomRoleParams) (database.CustomRole, error) {
act, ok := ActorFromContext(ctx)
if !ok {
return database.CustomRole{}, NoActorError
}
// Org and site role upsert share the same query. So switch the assertion based on the org uuid.
if arg.OrganizationID.UUID != uuid.Nil {
if err := q.authorizeContext(ctx, policy.ActionCreate, rbac.ResourceAssignOrgRole.InOrg(arg.OrganizationID.UUID)); err != nil {
return database.CustomRole{}, err
}
} else {
if err := q.authorizeContext(ctx, policy.ActionCreate, rbac.ResourceAssignRole); err != nil {
return database.CustomRole{}, err
}
}
if arg.OrganizationID.UUID == uuid.Nil && len(arg.OrgPermissions) > 0 {
return database.CustomRole{}, xerrors.Errorf("organization permissions require specifying an organization id")
}
// There is quite a bit of validation we should do here.
// The rbac.Role has a 'Valid()' function on it that will do a lot
// of checks.
rbacRole, err := rolestore.ConvertDBRole(database.CustomRole{
Name: arg.Name,
DisplayName: arg.DisplayName,
SitePermissions: arg.SitePermissions,
OrgPermissions: arg.OrgPermissions,
UserPermissions: arg.UserPermissions,
OrganizationID: arg.OrganizationID,
})
if err != nil {
return database.CustomRole{}, xerrors.Errorf("invalid args: %w", err)
}
err = rbacRole.Valid()
if err != nil {
return database.CustomRole{}, xerrors.Errorf("invalid role: %w", err)
}
if len(rbacRole.Org) > 0 && len(rbacRole.Site) > 0 {
// This is a choice to keep roles simple. If we allow mixing site and org scoped perms, then knowing who can
// do what gets more complicated.
return database.CustomRole{}, xerrors.Errorf("invalid custom role, cannot assign both org and site permissions at the same time")
}
if len(rbacRole.Org) > 1 {
// Again to avoid more complexity in our roles
return database.CustomRole{}, xerrors.Errorf("invalid custom role, cannot assign permissions to more than 1 org at a time")
}
// Prevent escalation
for _, sitePerm := range rbacRole.Site {
err := q.customRoleEscalationCheck(ctx, act, sitePerm, rbac.Object{Type: sitePerm.ResourceType})
if err != nil {
return database.CustomRole{}, xerrors.Errorf("site permission: %w", err)
}
}
for orgID, perms := range rbacRole.Org {
for _, orgPerm := range perms {
err := q.customRoleEscalationCheck(ctx, act, orgPerm, rbac.Object{OrgID: orgID, Type: orgPerm.ResourceType})
if err != nil {
return database.CustomRole{}, xerrors.Errorf("org=%q: %w", orgID, err)
}
}
}
for _, userPerm := range rbacRole.User {
err := q.customRoleEscalationCheck(ctx, act, userPerm, rbac.Object{Type: userPerm.ResourceType, Owner: act.ID})
if err != nil {
return database.CustomRole{}, xerrors.Errorf("user permission: %w", err)
}
}
return q.db.UpsertCustomRole(ctx, arg)
}
func (q *querier) UpsertDefaultProxy(ctx context.Context, arg database.UpsertDefaultProxyParams) error {
if err := q.authorizeContext(ctx, policy.ActionUpdate, rbac.ResourceSystem); err != nil {
return err

View File

@ -1282,9 +1282,77 @@ func (s *MethodTestSuite) TestUser() {
}).Asserts(
rbac.ResourceAssignRole, policy.ActionDelete)
}))
s.Run("Blank/UpsertCustomRole", s.Subtest(func(db database.Store, check *expects) {
s.Run("Blank/UpdateCustomRole", s.Subtest(func(db database.Store, check *expects) {
customRole := dbgen.CustomRole(s.T(), db, database.CustomRole{})
// Blank is no perms in the role
check.Args(database.UpsertCustomRoleParams{
check.Args(database.UpdateCustomRoleParams{
Name: customRole.Name,
DisplayName: "Test Name",
SitePermissions: nil,
OrgPermissions: nil,
UserPermissions: nil,
}).Asserts(rbac.ResourceAssignRole, policy.ActionUpdate)
}))
s.Run("SitePermissions/UpdateCustomRole", s.Subtest(func(db database.Store, check *expects) {
customRole := dbgen.CustomRole(s.T(), db, database.CustomRole{
OrganizationID: uuid.NullUUID{
UUID: uuid.Nil,
Valid: false,
},
})
check.Args(database.UpdateCustomRoleParams{
Name: customRole.Name,
OrganizationID: customRole.OrganizationID,
DisplayName: "Test Name",
SitePermissions: db2sdk.List(codersdk.CreatePermissions(map[codersdk.RBACResource][]codersdk.RBACAction{
codersdk.ResourceTemplate: {codersdk.ActionCreate, codersdk.ActionRead, codersdk.ActionUpdate, codersdk.ActionDelete, codersdk.ActionViewInsights},
}), convertSDKPerm),
OrgPermissions: nil,
UserPermissions: db2sdk.List(codersdk.CreatePermissions(map[codersdk.RBACResource][]codersdk.RBACAction{
codersdk.ResourceWorkspace: {codersdk.ActionRead},
}), convertSDKPerm),
}).Asserts(
// First check
rbac.ResourceAssignRole, policy.ActionUpdate,
// Escalation checks
rbac.ResourceTemplate, policy.ActionCreate,
rbac.ResourceTemplate, policy.ActionRead,
rbac.ResourceTemplate, policy.ActionUpdate,
rbac.ResourceTemplate, policy.ActionDelete,
rbac.ResourceTemplate, policy.ActionViewInsights,
rbac.ResourceWorkspace.WithOwner(testActorID.String()), policy.ActionRead,
)
}))
s.Run("OrgPermissions/UpdateCustomRole", s.Subtest(func(db database.Store, check *expects) {
orgID := uuid.New()
customRole := dbgen.CustomRole(s.T(), db, database.CustomRole{
OrganizationID: uuid.NullUUID{
UUID: orgID,
Valid: true,
},
})
check.Args(database.UpdateCustomRoleParams{
Name: customRole.Name,
DisplayName: "Test Name",
OrganizationID: customRole.OrganizationID,
SitePermissions: nil,
OrgPermissions: db2sdk.List(codersdk.CreatePermissions(map[codersdk.RBACResource][]codersdk.RBACAction{
codersdk.ResourceTemplate: {codersdk.ActionCreate, codersdk.ActionRead},
}), convertSDKPerm),
UserPermissions: nil,
}).Asserts(
// First check
rbac.ResourceAssignOrgRole.InOrg(orgID), policy.ActionUpdate,
// Escalation checks
rbac.ResourceTemplate.InOrg(orgID), policy.ActionCreate,
rbac.ResourceTemplate.InOrg(orgID), policy.ActionRead,
)
}))
s.Run("Blank/InsertCustomRole", s.Subtest(func(db database.Store, check *expects) {
// Blank is no perms in the role
check.Args(database.InsertCustomRoleParams{
Name: "test",
DisplayName: "Test Name",
SitePermissions: nil,
@ -1292,8 +1360,8 @@ func (s *MethodTestSuite) TestUser() {
UserPermissions: nil,
}).Asserts(rbac.ResourceAssignRole, policy.ActionCreate)
}))
s.Run("SitePermissions/UpsertCustomRole", s.Subtest(func(db database.Store, check *expects) {
check.Args(database.UpsertCustomRoleParams{
s.Run("SitePermissions/InsertCustomRole", s.Subtest(func(db database.Store, check *expects) {
check.Args(database.InsertCustomRoleParams{
Name: "test",
DisplayName: "Test Name",
SitePermissions: db2sdk.List(codersdk.CreatePermissions(map[codersdk.RBACResource][]codersdk.RBACAction{
@ -1316,9 +1384,9 @@ func (s *MethodTestSuite) TestUser() {
rbac.ResourceWorkspace.WithOwner(testActorID.String()), policy.ActionRead,
)
}))
s.Run("OrgPermissions/UpsertCustomRole", s.Subtest(func(db database.Store, check *expects) {
s.Run("OrgPermissions/InsertCustomRole", s.Subtest(func(db database.Store, check *expects) {
orgID := uuid.New()
check.Args(database.UpsertCustomRoleParams{
check.Args(database.InsertCustomRoleParams{
Name: "test",
DisplayName: "Test Name",
OrganizationID: uuid.NullUUID{
@ -1329,17 +1397,13 @@ func (s *MethodTestSuite) TestUser() {
OrgPermissions: db2sdk.List(codersdk.CreatePermissions(map[codersdk.RBACResource][]codersdk.RBACAction{
codersdk.ResourceTemplate: {codersdk.ActionCreate, codersdk.ActionRead},
}), convertSDKPerm),
UserPermissions: db2sdk.List(codersdk.CreatePermissions(map[codersdk.RBACResource][]codersdk.RBACAction{
codersdk.ResourceWorkspace: {codersdk.ActionRead},
}), convertSDKPerm),
UserPermissions: nil,
}).Asserts(
// First check
rbac.ResourceAssignOrgRole.InOrg(orgID), policy.ActionCreate,
// Escalation checks
rbac.ResourceTemplate.InOrg(orgID), policy.ActionCreate,
rbac.ResourceTemplate.InOrg(orgID), policy.ActionRead,
rbac.ResourceWorkspace.WithOwner(testActorID.String()), policy.ActionRead,
)
}))
}