Skip to content

Commit f4d504e

Browse files
EmyrkEmyrk
committed
fixup tests and include migrations
1 parent b1776c1 commit f4d504e

File tree

6 files changed

+223
-110
lines changed

6 files changed

+223
-110
lines changed

‎coderd/database/dbauthz/customroles_test.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -19,8 +19,8 @@ import (
1919
"github.com/coder/coder/v2/testutil"
2020
)
2121

22-
// TestUpsertCustomRoles verifies creating custom roles cannot escalate permissions.
23-
func TestUpsertCustomRoles(t *testing.T) {
22+
// TestInsertCustomRoles verifies creating custom roles cannot escalate permissions.
23+
func TestInsertCustomRoles(t *testing.T) {
2424
t.Parallel()
2525

2626
userID := uuid.New()
@@ -231,7 +231,7 @@ func TestUpsertCustomRoles(t *testing.T) {
231231
ctx := testutil.Context(t, testutil.WaitMedium)
232232
ctx = dbauthz.As(ctx, subject)
233233

234-
_, err := az.UpsertCustomRole(ctx, database.UpsertCustomRoleParams{
234+
_, err := az.InsertCustomRole(ctx, database.InsertCustomRoleParams{
235235
Name: "test-role",
236236
DisplayName: "",
237237
OrganizationID: tc.organizationID,

‎coderd/database/dbauthz/dbauthz.go

Lines changed: 104 additions & 81 deletions
Original file line numberDiff line numberDiff line change
@@ -815,6 +815,86 @@ func (q *querier) customRoleEscalationCheck(ctx context.Context, actor rbac.Subj
815815
return nil
816816
}
817817

818+
// customRoleCheck will validate a custom role for inserting or updating.
819+
// If the role is not valid, an error will be returned.
820+
// - Check custom roles are valid for their resource types + actions
821+
// - Check the actor can create the custom role
822+
// - Check the custom role does not grant perms the actor does not have
823+
// - Prevent negative perms
824+
// - Prevent roles with site and org permissions.
825+
func (q *querier) customRoleCheck(ctx context.Context, role database.CustomRole) error {
826+
act, ok := ActorFromContext(ctx)
827+
if !ok {
828+
return NoActorError
829+
}
830+
831+
// Org permissions require an org role
832+
if role.OrganizationID.UUID == uuid.Nil && len(role.OrgPermissions) > 0 {
833+
return xerrors.Errorf("organization permissions require specifying an organization id")
834+
}
835+
836+
// Org roles can only specify org permissions
837+
if role.OrganizationID.UUID != uuid.Nil && (len(role.SitePermissions) > 0 || len(role.UserPermissions) > 0) {
838+
return xerrors.Errorf("organization roles specify site or user permissions")
839+
}
840+
841+
// The rbac.Role has a 'Valid()' function on it that will do a lot
842+
// of checks.
843+
rbacRole, err := rolestore.ConvertDBRole(database.CustomRole{
844+
Name: role.Name,
845+
DisplayName: role.DisplayName,
846+
SitePermissions: role.SitePermissions,
847+
OrgPermissions: role.OrgPermissions,
848+
UserPermissions: role.UserPermissions,
849+
OrganizationID: role.OrganizationID,
850+
})
851+
if err != nil {
852+
return xerrors.Errorf("invalid args: %w", err)
853+
}
854+
855+
err = rbacRole.Valid()
856+
if err != nil {
857+
return xerrors.Errorf("invalid role: %w", err)
858+
}
859+
860+
if len(rbacRole.Org) > 0 && len(rbacRole.Site) > 0 {
861+
// This is a choice to keep roles simple. If we allow mixing site and org scoped perms, then knowing who can
862+
// do what gets more complicated.
863+
return xerrors.Errorf("invalid custom role, cannot assign both org and site permissions at the same time")
864+
}
865+
866+
if len(rbacRole.Org) > 1 {
867+
// Again to avoid more complexity in our roles
868+
return xerrors.Errorf("invalid custom role, cannot assign permissions to more than 1 org at a time")
869+
}
870+
871+
// Prevent escalation
872+
for _, sitePerm := range rbacRole.Site {
873+
err := q.customRoleEscalationCheck(ctx, act, sitePerm, rbac.Object{Type: sitePerm.ResourceType})
874+
if err != nil {
875+
return xerrors.Errorf("site permission: %w", err)
876+
}
877+
}
878+
879+
for orgID, perms := range rbacRole.Org {
880+
for _, orgPerm := range perms {
881+
err := q.customRoleEscalationCheck(ctx, act, orgPerm, rbac.Object{OrgID: orgID, Type: orgPerm.ResourceType})
882+
if err != nil {
883+
return xerrors.Errorf("org=%q: %w", orgID, err)
884+
}
885+
}
886+
}
887+
888+
for _, userPerm := range rbacRole.User {
889+
err := q.customRoleEscalationCheck(ctx, act, userPerm, rbac.Object{Type: userPerm.ResourceType, Owner: act.ID})
890+
if err != nil {
891+
return xerrors.Errorf("user permission: %w", err)
892+
}
893+
}
894+
895+
return nil
896+
}
897+
818898
func (q *querier) AcquireLock(ctx context.Context, id int64) error {
819899
return q.db.AcquireLock(ctx, id)
820900
}
@@ -2579,86 +2659,6 @@ func (q *querier) InsertCustomRole(ctx context.Context, arg database.InsertCusto
25792659
return q.db.InsertCustomRole(ctx, arg)
25802660
}
25812661

2582-
// customRoleCheck will validate a custom role for inserting or updating.
2583-
// If the role is not valid, an error will be returned.
2584-
// - Check custom roles are valid for their resource types + actions
2585-
// - Check the actor can create the custom role
2586-
// - Check the custom role does not grant perms the actor does not have
2587-
// - Prevent negative perms
2588-
// - Prevent roles with site and org permissions.
2589-
func (q *querier) customRoleCheck(ctx context.Context, role database.CustomRole) error {
2590-
act, ok := ActorFromContext(ctx)
2591-
if !ok {
2592-
return NoActorError
2593-
}
2594-
2595-
// Org permissions require an org role
2596-
if role.OrganizationID.UUID == uuid.Nil && len(role.OrgPermissions) > 0 {
2597-
return xerrors.Errorf("organization permissions require specifying an organization id")
2598-
}
2599-
2600-
// Org roles can only specify org permissions
2601-
if role.OrganizationID.UUID != uuid.Nil && (len(role.SitePermissions) > 0 || len(role.UserPermissions) > 0) {
2602-
return xerrors.Errorf("organization roles specify site or user permissions")
2603-
}
2604-
2605-
// The rbac.Role has a 'Valid()' function on it that will do a lot
2606-
// of checks.
2607-
rbacRole, err := rolestore.ConvertDBRole(database.CustomRole{
2608-
Name: role.Name,
2609-
DisplayName: role.DisplayName,
2610-
SitePermissions: role.SitePermissions,
2611-
OrgPermissions: role.OrgPermissions,
2612-
UserPermissions: role.UserPermissions,
2613-
OrganizationID: role.OrganizationID,
2614-
})
2615-
if err != nil {
2616-
return xerrors.Errorf("invalid args: %w", err)
2617-
}
2618-
2619-
err = rbacRole.Valid()
2620-
if err != nil {
2621-
return xerrors.Errorf("invalid role: %w", err)
2622-
}
2623-
2624-
if len(rbacRole.Org) > 0 && len(rbacRole.Site) > 0 {
2625-
// This is a choice to keep roles simple. If we allow mixing site and org scoped perms, then knowing who can
2626-
// do what gets more complicated.
2627-
return xerrors.Errorf("invalid custom role, cannot assign both org and site permissions at the same time")
2628-
}
2629-
2630-
if len(rbacRole.Org) > 1 {
2631-
// Again to avoid more complexity in our roles
2632-
return xerrors.Errorf("invalid custom role, cannot assign permissions to more than 1 org at a time")
2633-
}
2634-
2635-
// Prevent escalation
2636-
for _, sitePerm := range rbacRole.Site {
2637-
err := q.customRoleEscalationCheck(ctx, act, sitePerm, rbac.Object{Type: sitePerm.ResourceType})
2638-
if err != nil {
2639-
return xerrors.Errorf("site permission: %w", err)
2640-
}
2641-
}
2642-
2643-
for orgID, perms := range rbacRole.Org {
2644-
for _, orgPerm := range perms {
2645-
err := q.customRoleEscalationCheck(ctx, act, orgPerm, rbac.Object{OrgID: orgID, Type: orgPerm.ResourceType})
2646-
if err != nil {
2647-
return xerrors.Errorf("org=%q: %w", orgID, err)
2648-
}
2649-
}
2650-
}
2651-
2652-
for _, userPerm := range rbacRole.User {
2653-
err := q.customRoleEscalationCheck(ctx, act, userPerm, rbac.Object{Type: userPerm.ResourceType, Owner: act.ID})
2654-
if err != nil {
2655-
return xerrors.Errorf("user permission: %w", err)
2656-
}
2657-
}
2658-
2659-
return nil
2660-
}
2661-
26622662
func (q *querier) InsertDBCryptKey(ctx context.Context, arg database.InsertDBCryptKeyParams) error {
26632663
if err := q.authorizeContext(ctx, policy.ActionCreate, rbac.ResourceSystem); err != nil {
26642664
return err
@@ -3111,7 +3111,30 @@ func (q *querier) UpdateAPIKeyByID(ctx context.Context, arg database.UpdateAPIKe
31113111
}
31123112

31133113
func (q *querier) UpdateCustomRole(ctx context.Context, arg database.UpdateCustomRoleParams) (database.CustomRole, error) {
3114-
panic("not implemented")
3114+
if arg.OrganizationID.UUID != uuid.Nil {
3115+
if err := q.authorizeContext(ctx, policy.ActionUpdate, rbac.ResourceAssignOrgRole.InOrg(arg.OrganizationID.UUID)); err != nil {
3116+
return database.CustomRole{}, err
3117+
}
3118+
} else {
3119+
if err := q.authorizeContext(ctx, policy.ActionUpdate, rbac.ResourceAssignRole); err != nil {
3120+
return database.CustomRole{}, err
3121+
}
3122+
}
3123+
3124+
if err := q.customRoleCheck(ctx, database.CustomRole{
3125+
Name: arg.Name,
3126+
DisplayName: arg.DisplayName,
3127+
SitePermissions: arg.SitePermissions,
3128+
OrgPermissions: arg.OrgPermissions,
3129+
UserPermissions: arg.UserPermissions,
3130+
CreatedAt: time.Now(),
3131+
UpdatedAt: time.Now(),
3132+
OrganizationID: arg.OrganizationID,
3133+
ID: uuid.New(),
3134+
}); err != nil {
3135+
return database.CustomRole{}, err
3136+
}
3137+
return q.db.UpdateCustomRole(ctx, arg)
31153138
}
31163139

31173140
func (q *querier) UpdateExternalAuthLink(ctx context.Context, arg database.UpdateExternalAuthLinkParams) (database.ExternalAuthLink, error) {

‎coderd/database/dbauthz/dbauthz_test.go

Lines changed: 75 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1282,18 +1282,86 @@ func (s *MethodTestSuite) TestUser() {
12821282
}).Asserts(
12831283
rbac.ResourceAssignRole, policy.ActionDelete)
12841284
}))
1285-
s.Run("Blank/UpsertCustomRole", s.Subtest(func(db database.Store, check *expects) {
1285+
s.Run("Blank/UpdateCustomRole", s.Subtest(func(db database.Store, check *expects) {
1286+
customRole := dbgen.CustomRole(s.T(), db, database.CustomRole{})
12861287
// Blank is no perms in the role
1287-
check.Args(database.UpsertCustomRoleParams{
1288+
check.Args(database.UpdateCustomRoleParams{
1289+
Name: customRole.Name,
1290+
DisplayName: "Test Name",
1291+
SitePermissions: nil,
1292+
OrgPermissions: nil,
1293+
UserPermissions: nil,
1294+
}).Asserts(rbac.ResourceAssignRole, policy.ActionUpdate)
1295+
}))
1296+
s.Run("SitePermissions/UpdateCustomRole", s.Subtest(func(db database.Store, check *expects) {
1297+
customRole := dbgen.CustomRole(s.T(), db, database.CustomRole{
1298+
OrganizationID: uuid.NullUUID{
1299+
UUID: uuid.Nil,
1300+
Valid: false,
1301+
},
1302+
})
1303+
check.Args(database.UpdateCustomRoleParams{
1304+
Name: customRole.Name,
1305+
OrganizationID: customRole.OrganizationID,
1306+
DisplayName: "Test Name",
1307+
SitePermissions: db2sdk.List(codersdk.CreatePermissions(map[codersdk.RBACResource][]codersdk.RBACAction{
1308+
codersdk.ResourceTemplate: {codersdk.ActionCreate, codersdk.ActionRead, codersdk.ActionUpdate, codersdk.ActionDelete, codersdk.ActionViewInsights},
1309+
}), convertSDKPerm),
1310+
OrgPermissions: nil,
1311+
UserPermissions: db2sdk.List(codersdk.CreatePermissions(map[codersdk.RBACResource][]codersdk.RBACAction{
1312+
codersdk.ResourceWorkspace: {codersdk.ActionRead},
1313+
}), convertSDKPerm),
1314+
}).Asserts(
1315+
// First check
1316+
rbac.ResourceAssignRole, policy.ActionUpdate,
1317+
// Escalation checks
1318+
rbac.ResourceTemplate, policy.ActionCreate,
1319+
rbac.ResourceTemplate, policy.ActionRead,
1320+
rbac.ResourceTemplate, policy.ActionUpdate,
1321+
rbac.ResourceTemplate, policy.ActionDelete,
1322+
rbac.ResourceTemplate, policy.ActionViewInsights,
1323+
1324+
rbac.ResourceWorkspace.WithOwner(testActorID.String()), policy.ActionRead,
1325+
)
1326+
}))
1327+
s.Run("OrgPermissions/UpdateCustomRole", s.Subtest(func(db database.Store, check *expects) {
1328+
orgID := uuid.New()
1329+
customRole := dbgen.CustomRole(s.T(), db, database.CustomRole{
1330+
OrganizationID: uuid.NullUUID{
1331+
UUID: orgID,
1332+
Valid: true,
1333+
},
1334+
})
1335+
1336+
check.Args(database.UpdateCustomRoleParams{
1337+
Name: customRole.Name,
1338+
DisplayName: "Test Name",
1339+
OrganizationID: customRole.OrganizationID,
1340+
SitePermissions: nil,
1341+
OrgPermissions: db2sdk.List(codersdk.CreatePermissions(map[codersdk.RBACResource][]codersdk.RBACAction{
1342+
codersdk.ResourceTemplate: {codersdk.ActionCreate, codersdk.ActionRead},
1343+
}), convertSDKPerm),
1344+
UserPermissions: nil,
1345+
}).Asserts(
1346+
// First check
1347+
rbac.ResourceAssignOrgRole.InOrg(orgID), policy.ActionUpdate,
1348+
// Escalation checks
1349+
rbac.ResourceTemplate.InOrg(orgID), policy.ActionCreate,
1350+
rbac.ResourceTemplate.InOrg(orgID), policy.ActionRead,
1351+
)
1352+
}))
1353+
s.Run("Blank/InsertCustomRole", s.Subtest(func(db database.Store, check *expects) {
1354+
// Blank is no perms in the role
1355+
check.Args(database.InsertCustomRoleParams{
12881356
Name: "test",
12891357
DisplayName: "Test Name",
12901358
SitePermissions: nil,
12911359
OrgPermissions: nil,
12921360
UserPermissions: nil,
12931361
}).Asserts(rbac.ResourceAssignRole, policy.ActionCreate)
12941362
}))
1295-
s.Run("SitePermissions/UpsertCustomRole", s.Subtest(func(db database.Store, check *expects) {
1296-
check.Args(database.UpsertCustomRoleParams{
1363+
s.Run("SitePermissions/InsertCustomRole", s.Subtest(func(db database.Store, check *expects) {
1364+
check.Args(database.InsertCustomRoleParams{
12971365
Name: "test",
12981366
DisplayName: "Test Name",
12991367
SitePermissions: db2sdk.List(codersdk.CreatePermissions(map[codersdk.RBACResource][]codersdk.RBACAction{
@@ -1316,9 +1384,9 @@ func (s *MethodTestSuite) TestUser() {
13161384
rbac.ResourceWorkspace.WithOwner(testActorID.String()), policy.ActionRead,
13171385
)
13181386
}))
1319-
s.Run("OrgPermissions/UpsertCustomRole", s.Subtest(func(db database.Store, check *expects) {
1387+
s.Run("OrgPermissions/InsertCustomRole", s.Subtest(func(db database.Store, check *expects) {
13201388
orgID := uuid.New()
1321-
check.Args(database.UpsertCustomRoleParams{
1389+
check.Args(database.InsertCustomRoleParams{
13221390
Name: "test",
13231391
DisplayName: "Test Name",
13241392
OrganizationID: uuid.NullUUID{
@@ -1329,17 +1397,13 @@ func (s *MethodTestSuite) TestUser() {
13291397
OrgPermissions: db2sdk.List(codersdk.CreatePermissions(map[codersdk.RBACResource][]codersdk.RBACAction{
13301398
codersdk.ResourceTemplate: {codersdk.ActionCreate, codersdk.ActionRead},
13311399
}), convertSDKPerm),
1332-
UserPermissions: db2sdk.List(codersdk.CreatePermissions(map[codersdk.RBACResource][]codersdk.RBACAction{
1333-
codersdk.ResourceWorkspace: {codersdk.ActionRead},
1334-
}), convertSDKPerm),
1400+
UserPermissions: nil,
13351401
}).Asserts(
13361402
// First check
13371403
rbac.ResourceAssignOrgRole.InOrg(orgID), policy.ActionCreate,
13381404
// Escalation checks
13391405
rbac.ResourceTemplate.InOrg(orgID), policy.ActionCreate,
13401406
rbac.ResourceTemplate.InOrg(orgID), policy.ActionRead,
1341-
1342-
rbac.ResourceWorkspace.WithOwner(testActorID.String()), policy.ActionRead,
13431407
)
13441408
}))
13451409
}

0 commit comments

Comments
 (0)