fixup tests and include migrations

coder · Emyrk · Aug 13, 2024 · Aug 8, 2024 · Aug 8, 2024 · Aug 8, 2024
commit f4d504ec96eaedad4150bff38f26addd54eee98b
diff --git a/coderd/database/dbauthz/customroles_test.go b/coderd/database/dbauthz/customroles_test.go
@@ -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()
@@ -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,

diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go
@@ -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)
 }
@@ -2579,86 +2659,6 @@ func (q *querier) InsertCustomRole(ctx context.Context, arg database.InsertCusto
 	return q.db.InsertCustomRole(ctx, arg)
 }
 
-// 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) InsertDBCryptKey(ctx context.Context, arg database.InsertDBCryptKeyParams) error {
 	if err := q.authorizeContext(ctx, policy.ActionCreate, rbac.ResourceSystem); err != nil {
 		return err
@@ -3111,7 +3111,30 @@ func (q *querier) UpdateAPIKeyByID(ctx context.Context, arg database.UpdateAPIKe
 }
 
 func (q *querier) UpdateCustomRole(ctx context.Context, arg database.UpdateCustomRoleParams) (database.CustomRole, error) {
-	panic("not implemented")
+	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) {

diff --git a/coderd/database/dbauthz/dbauthz_test.go b/coderd/database/dbauthz/dbauthz_test.go
@@ -1282,18 +1282,86 @@ 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,
 			OrgPermissions:  nil,
 			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,
 		)
 	}))
 }