Skip to content

Commit 839bb9a

Browse files
EmyrkEmyrk
committed
fixup tests and include migrations
1 parent 8b9dfc9 commit 839bb9a

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
}
@@ -2571,86 +2651,6 @@ func (q *querier) InsertCustomRole(ctx context.Context, arg database.InsertCusto
25712651
return q.db.InsertCustomRole(ctx, arg)
25722652
}
25732653

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

31053105
func (q *querier) UpdateCustomRole(ctx context.Context, arg database.UpdateCustomRoleParams) (database.CustomRole, error) {
3106-
panic("not implemented")
3106+
if arg.OrganizationID.UUID != uuid.Nil {
3107+
if err := q.authorizeContext(ctx, policy.ActionUpdate, rbac.ResourceAssignOrgRole.InOrg(arg.OrganizationID.UUID)); err != nil {
3108+
return database.CustomRole{}, err
3109+
}
3110+
} else {
3111+
if err := q.authorizeContext(ctx, policy.ActionUpdate, rbac.ResourceAssignRole); err != nil {
3112+
return database.CustomRole{}, err
3113+
}
3114+
}
3115+
3116+
if err := q.customRoleCheck(ctx, database.CustomRole{
3117+
Name: arg.Name,
3118+
DisplayName: arg.DisplayName,
3119+
SitePermissions: arg.SitePermissions,
3120+
OrgPermissions: arg.OrgPermissions,
3121+
UserPermissions: arg.UserPermissions,
3122+
CreatedAt: time.Now(),
3123+
UpdatedAt: time.Now(),
3124+
OrganizationID: arg.OrganizationID,
3125+
ID: uuid.New(),
3126+
}); err != nil {
3127+
return database.CustomRole{}, err
3128+
}
3129+
return q.db.UpdateCustomRole(ctx, arg)
31073130
}
31083131

31093132
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
@@ -1272,18 +1272,86 @@ func (s *MethodTestSuite) TestUser() {
12721272
}).Asserts(
12731273
rbac.ResourceAssignRole, policy.ActionDelete)
12741274
}))
1275-
s.Run("Blank/UpsertCustomRole", s.Subtest(func(db database.Store, check *expects) {
1275+
s.Run("Blank/UpdateCustomRole", s.Subtest(func(db database.Store, check *expects) {
1276+
customRole := dbgen.CustomRole(s.T(), db, database.CustomRole{})
12761277
// Blank is no perms in the role
1277-
check.Args(database.UpsertCustomRoleParams{
1278+
check.Args(database.UpdateCustomRoleParams{
1279+
Name: customRole.Name,
1280+
DisplayName: "Test Name",
1281+
SitePermissions: nil,
1282+
OrgPermissions: nil,
1283+
UserPermissions: nil,
1284+
}).Asserts(rbac.ResourceAssignRole, policy.ActionUpdate)
1285+
}))
1286+
s.Run("SitePermissions/UpdateCustomRole", s.Subtest(func(db database.Store, check *expects) {
1287+
customRole := dbgen.CustomRole(s.T(), db, database.CustomRole{
1288+
OrganizationID: uuid.NullUUID{
1289+
UUID: uuid.Nil,
1290+
Valid: false,
1291+
},
1292+
})
1293+
check.Args(database.UpdateCustomRoleParams{
1294+
Name: customRole.Name,
1295+
OrganizationID: customRole.OrganizationID,
1296+
DisplayName: "Test Name",
1297+
SitePermissions: db2sdk.List(codersdk.CreatePermissions(map[codersdk.RBACResource][]codersdk.RBACAction{
1298+
codersdk.ResourceTemplate: {codersdk.ActionCreate, codersdk.ActionRead, codersdk.ActionUpdate, codersdk.ActionDelete, codersdk.ActionViewInsights},
1299+
}), convertSDKPerm),
1300+
OrgPermissions: nil,
1301+
UserPermissions: db2sdk.List(codersdk.CreatePermissions(map[codersdk.RBACResource][]codersdk.RBACAction{
1302+
codersdk.ResourceWorkspace: {codersdk.ActionRead},
1303+
}), convertSDKPerm),
1304+
}).Asserts(
1305+
// First check
1306+
rbac.ResourceAssignRole, policy.ActionUpdate,
1307+
// Escalation checks
1308+
rbac.ResourceTemplate, policy.ActionCreate,
1309+
rbac.ResourceTemplate, policy.ActionRead,
1310+
rbac.ResourceTemplate, policy.ActionUpdate,
1311+
rbac.ResourceTemplate, policy.ActionDelete,
1312+
rbac.ResourceTemplate, policy.ActionViewInsights,
1313+
1314+
rbac.ResourceWorkspace.WithOwner(testActorID.String()), policy.ActionRead,
1315+
)
1316+
}))
1317+
s.Run("OrgPermissions/UpdateCustomRole", s.Subtest(func(db database.Store, check *expects) {
1318+
orgID := uuid.New()
1319+
customRole := dbgen.CustomRole(s.T(), db, database.CustomRole{
1320+
OrganizationID: uuid.NullUUID{
1321+
UUID: orgID,
1322+
Valid: true,
1323+
},
1324+
})
1325+
1326+
check.Args(database.UpdateCustomRoleParams{
1327+
Name: customRole.Name,
1328+
DisplayName: "Test Name",
1329+
OrganizationID: customRole.OrganizationID,
1330+
SitePermissions: nil,
1331+
OrgPermissions: db2sdk.List(codersdk.CreatePermissions(map[codersdk.RBACResource][]codersdk.RBACAction{
1332+
codersdk.ResourceTemplate: {codersdk.ActionCreate, codersdk.ActionRead},
1333+
}), convertSDKPerm),
1334+
UserPermissions: nil,
1335+
}).Asserts(
1336+
// First check
1337+
rbac.ResourceAssignOrgRole.InOrg(orgID), policy.ActionUpdate,
1338+
// Escalation checks
1339+
rbac.ResourceTemplate.InOrg(orgID), policy.ActionCreate,
1340+
rbac.ResourceTemplate.InOrg(orgID), policy.ActionRead,
1341+
)
1342+
}))
1343+
s.Run("Blank/InsertCustomRole", s.Subtest(func(db database.Store, check *expects) {
1344+
// Blank is no perms in the role
1345+
check.Args(database.InsertCustomRoleParams{
12781346
Name: "test",
12791347
DisplayName: "Test Name",
12801348
SitePermissions: nil,
12811349
OrgPermissions: nil,
12821350
UserPermissions: nil,
12831351
}).Asserts(rbac.ResourceAssignRole, policy.ActionCreate)
12841352
}))
1285-
s.Run("SitePermissions/UpsertCustomRole", s.Subtest(func(db database.Store, check *expects) {
1286-
check.Args(database.UpsertCustomRoleParams{
1353+
s.Run("SitePermissions/InsertCustomRole", s.Subtest(func(db database.Store, check *expects) {
1354+
check.Args(database.InsertCustomRoleParams{
12871355
Name: "test",
12881356
DisplayName: "Test Name",
12891357
SitePermissions: db2sdk.List(codersdk.CreatePermissions(map[codersdk.RBACResource][]codersdk.RBACAction{
@@ -1306,9 +1374,9 @@ func (s *MethodTestSuite) TestUser() {
13061374
rbac.ResourceWorkspace.WithOwner(testActorID.String()), policy.ActionRead,
13071375
)
13081376
}))
1309-
s.Run("OrgPermissions/UpsertCustomRole", s.Subtest(func(db database.Store, check *expects) {
1377+
s.Run("OrgPermissions/InsertCustomRole", s.Subtest(func(db database.Store, check *expects) {
13101378
orgID := uuid.New()
1311-
check.Args(database.UpsertCustomRoleParams{
1379+
check.Args(database.InsertCustomRoleParams{
13121380
Name: "test",
13131381
DisplayName: "Test Name",
13141382
OrganizationID: uuid.NullUUID{
@@ -1319,17 +1387,13 @@ func (s *MethodTestSuite) TestUser() {
13191387
OrgPermissions: db2sdk.List(codersdk.CreatePermissions(map[codersdk.RBACResource][]codersdk.RBACAction{
13201388
codersdk.ResourceTemplate: {codersdk.ActionCreate, codersdk.ActionRead},
13211389
}), convertSDKPerm),
1322-
UserPermissions: db2sdk.List(codersdk.CreatePermissions(map[codersdk.RBACResource][]codersdk.RBACAction{
1323-
codersdk.ResourceWorkspace: {codersdk.ActionRead},
1324-
}), convertSDKPerm),
1390+
UserPermissions: nil,
13251391
}).Asserts(
13261392
// First check
13271393
rbac.ResourceAssignOrgRole.InOrg(orgID), policy.ActionCreate,
13281394
// Escalation checks
13291395
rbac.ResourceTemplate.InOrg(orgID), policy.ActionCreate,
13301396
rbac.ResourceTemplate.InOrg(orgID), policy.ActionRead,
1331-
1332-
rbac.ResourceWorkspace.WithOwner(testActorID.String()), policy.ActionRead,
13331397
)
13341398
}))
13351399
}

0 commit comments

Comments
 (0)