dev/core#2752 Use acl, not blanket permissions on FinancialAccount, FinancialType, EntityFinancialAccount

CRM/Contribute/BAO/ContributionPage.php

@@ -812,11 +812,13 @@ public static function buildOptions($fieldName, $context = NULL, $props = []) {
     // Special logic for fields whose options depend on context or properties
     switch ($fieldName) {
       case 'financial_type_id':
-        // @fixme - this is going to ignore context, better to get conditions, add params, and call PseudoConstant::get
-        // @fixme - https://lab.civicrm.org/dev/core/issues/547 if CiviContribute not enabled this causes an invalid query
-        //   because $relationTypeId is not set in CRM_Financial_BAO_FinancialType::getIncomeFinancialType()
+        // https://lab.civicrm.org/dev/core/issues/547 if CiviContribute not enabled this causes an invalid query
+        // @todo - the component is enabled check should be done within getIncomeFinancialType
+        // It looks to me like test cover was NOT added to cover the change
+        // that added this so we need to assume there is no test cover
         if (array_key_exists('CiviContribute', CRM_Core_Component::getEnabledComponents())) {
-          return CRM_Financial_BAO_FinancialType::getIncomeFinancialType();
+          // if check_permission has been passed in (not Null) then restrict.
+          return CRM_Financial_BAO_FinancialType::getIncomeFinancialType($props['check_permissions'] ?? TRUE);
         }
         return [];
     }

CRM/Core/Permission.php

@@ -1142,6 +1142,9 @@ public static function getEntityActionPermissions() {
     $permissions['product'] = $permissions['contribution'];
 
     $permissions['financial_item'] = $permissions['contribution'];
+    $permissions['financial_type']['get'] = $permissions['contribution']['get'];
+    $permissions['entity_financial_account']['get'] = $permissions['contribution']['get'];
+    $permissions['financial_account']['get'] = $permissions['contribution']['get'];
 
     // Payment permissions
     $permissions['payment'] = [

CRM/Event/BAO/Event.php

@@ -2361,11 +2361,12 @@ public static function buildOptions($fieldName, $context = NULL, $props = []) {
     // Special logic for fields whose options depend on context or properties
     switch ($fieldName) {
       case 'financial_type_id':
-        // @fixme - this is going to ignore context, better to get conditions, add params, and call PseudoConstant::get
-        // @fixme - https://lab.civicrm.org/dev/core/issues/547 if CiviContribute not enabled this causes an invalid query
-        //   because $relationTypeId is not set in CRM_Financial_BAO_FinancialType::getIncomeFinancialType()
+        // https://lab.civicrm.org/dev/core/issues/547 if CiviContribute not enabled this causes an invalid query
+        // @todo - the component is enabled check should be done within getIncomeFinancialType
+        // It looks to me like test cover was NOT added to cover the change
+        // that added this so we need to assume there is no test cover
         if (array_key_exists('CiviContribute', CRM_Core_Component::getEnabledComponents())) {
-          return CRM_Financial_BAO_FinancialType::getIncomeFinancialType();
+          return CRM_Financial_BAO_FinancialType::getIncomeFinancialType($props['check_permissions'] ?? TRUE);
         }
         return [];
     }

CRM/Financial/BAO/FinancialType.php

@@ -9,6 +9,9 @@
  +--------------------------------------------------------------------+
  */
 
+use Civi\API\Exception\UnauthorizedException;
+use Civi\Api4\EntityFinancialAccount;
+
 /**
  *
  * @package CRM
@@ -174,35 +177,43 @@ public static function self_hook_civicrm_post(\Civi\Core\Event\PostEvent $event)
   }
 
   /**
-   * fetch financial type having relationship as Income Account is.
-   *
+   * Fetch financial types having relationship as Income Account is.
    *
    * @return array
    *   all financial type with income account is relationship
+   *
+   * @throws \API_Exception
    */
-  public static function getIncomeFinancialType() {
-    // Financial Type
-    $financialType = CRM_Contribute_PseudoConstant::financialType();
-    $revenueFinancialType = [];
-    $relationTypeId = key(CRM_Core_PseudoConstant::accountOptionValues('account_relationship', NULL, " AND v.name LIKE 'Income Account is' "));
-    CRM_Core_PseudoConstant::populate(
-      $revenueFinancialType,
-      'CRM_Financial_DAO_EntityFinancialAccount',
-      $all = TRUE,
-      $retrieve = 'entity_id',
-      $filter = NULL,
-      "account_relationship = $relationTypeId AND entity_table = 'civicrm_financial_type' "
-    );
-
-    foreach ($financialType as $key => $financialTypeName) {
-      if (!in_array($key, $revenueFinancialType)
-        || (CRM_Financial_BAO_FinancialType::isACLFinancialTypeStatus()
-          && !CRM_Core_Permission::check('add contributions of type ' . $financialTypeName))
-      ) {
-        unset($financialType[$key]);
+  public static function getIncomeFinancialType($checkPermissions = TRUE): array {
+    // Realistically tests are the only place where logged in contact can
+    // change during the session at this stage.
+    $key = 'income_type' . (int) $checkPermissions;
+    if ($checkPermissions) {
+      $key .= '_' . CRM_Core_Session::getLoggedInContactID();
+    }
+    if (isset(Civi::$statics[__CLASS__][$key])) {
+      return Civi::$statics[__CLASS__][$key];
+    }
+    try {
+      $types = EntityFinancialAccount::get($checkPermissions)
+        ->addWhere('account_relationship:name', '=', 'Income Account is')
+        ->addWhere('entity_table', '=', 'civicrm_financial_type')
+        ->addSelect('entity_id', 'financial_type.name')
+        ->addJoin('FinancialType AS financial_type', 'LEFT', [
+          'entity_id',
+          '=',
+          'financial_type.id',
+        ])
+        ->execute()->indexBy('entity_id');
+      Civi::$statics[__CLASS__][$key] = [];
+      foreach ($types as $type) {
+        Civi::$statics[__CLASS__][$key][$type['entity_id']] = $type['financial_type.name'];
       }
     }
-    return $financialType;
+    catch (UnauthorizedException $e) {
+      Civi::$statics[__CLASS__][$key] = [];
+    }
+    return Civi::$statics[__CLASS__][$key];
   }
 
   /**

api/v3/Generic.php

@@ -434,6 +434,11 @@ function civicrm_api3_generic_getoptions($apiRequest) {
   }
   else {
     $baoName = _civicrm_api3_get_BAO($apiRequest['entity']);
+    if (!isset($apiRequest['params']['check_permissions'])) {
+      // Ensure this is set so buildOptions for ContributionPage.buildOptions
+      // can distinguish between 'who knows' and 'NO'.
+      $apiRequest['params']['check_permissions'] = FALSE;
+    }
     $options = $baoName::buildOptions($fieldName, $context, $apiRequest['params']);
   }
   if ($options === FALSE) {

ext/financialacls/financialacls.php

@@ -2,6 +2,7 @@
 
 require_once 'financialacls.civix.php';
 // phpcs:disable
+use Civi\Api4\EntityFinancialAccount;
 use CRM_Financialacls_ExtensionUtil as E;
 // phpcs:enable
 
@@ -138,20 +139,66 @@ function financialacls_civicrm_selectWhereClause($entity, &$clauses) {
     case 'LineItem':
     case 'MembershipType':
     case 'ContributionRecur':
-      $types = [];
-      CRM_Financial_BAO_FinancialType::getAvailableFinancialTypes($types);
-      if ($types) {
-        $clauses['financial_type_id'] = 'IN (' . implode(',', array_keys($types)) . ')';
-      }
-      else {
-        $clauses['financial_type_id'] = '= 0';
-      }
+      $clauses['financial_type_id'] = _financialacls_civicrm_get_type_clause();
+      break;
+
+    case 'FinancialType':
+      $clauses['id'] = _financialacls_civicrm_get_type_clause();
+      break;
+
+    case 'FinancialAccount':
+      $clauses['id'] = _financialacls_civicrm_get_accounts_clause();
       break;
 
   }
 
 }
 
+/**
+ * Get the clause to limit available types.
+ *
+ * @return string
+ */
+function _financialacls_civicrm_get_accounts_clause(): string {
+  if (!isset(Civi::$statics['financial_acls'][__FUNCTION__][CRM_Core_Session::getLoggedInContactID()])) {
+    try {
+      $clause = '= 0';
+      Civi::$statics['financial_acls'][__FUNCTION__][CRM_Core_Session::getLoggedInContactID()] = &$clause;
+      $accounts = (array) EntityFinancialAccount::get()
+        ->addWhere('account_relationship:name', '=', 'Income Account is')
+        ->addWhere('entity_table', '=', 'civicrm_financial_type')
+        ->addSelect('entity_id', 'financial_account_id')
+        ->addJoin('FinancialType AS financial_type', 'LEFT', [
+          'entity_id',
+          '=',
+          'financial_type.id',
+        ])
+        ->execute()->indexBy('financial_account_id');
+      if (!empty($accounts)) {
+        $clause = 'IN (' . implode(',', array_keys($accounts)) . ')';
+      }
+    }
+    catch (\API_Exception $e) {
+      // We've already set it to 0 so we can quietly handle this.
+    }
+  }
+  return Civi::$statics['financial_acls'][__FUNCTION__][CRM_Core_Session::getLoggedInContactID()];
+}
+
+/**
+ * Get the clause to limit available types.
+ *
+ * @return string
+ */
+function _financialacls_civicrm_get_type_clause(): string {
+  $types = [];
+  CRM_Financial_BAO_FinancialType::getAvailableFinancialTypes($types);
+  if ($types) {
+    return 'IN (' . implode(',', array_keys($types)) . ')';
+  }
+  return '= 0';
+}
+
 /**
  * Remove unpermitted options.
  *

ext/financialacls/tests/phpunit/Civi/Financialacls/EntityFinancialAccountTest.php

@@ -0,0 +1,33 @@
+<?php
+
+namespace Civi\Financialacls;
+
+use Civi\Api4\EntityFinancialAccount;
+
+// I fought the Autoloader and the autoloader won.
+require_once 'BaseTestClass.php';
+
+/**
+ * @group headless
+ */
+class EntityFinancialAccountTest extends BaseTestClass {
+
+  /**
+   * Test only accounts with permitted income types can be retrieved.
+   *
+   * @throws \API_Exception
+   */
+  public function testGetEntityFinancialAccount(): void {
+    $this->setupLoggedInUserWithLimitedFinancialTypeAccess();
+    $entityFinancialAccounts = EntityFinancialAccount::get(FALSE)->execute();
+    $this->assertCount(23, $entityFinancialAccounts);
+    $restrictedAccounts = EntityFinancialAccount::get()->execute();
+    $this->assertCount(9, $restrictedAccounts);
+    foreach ($restrictedAccounts as $restrictedAccount) {
+      if ($restrictedAccount['entity_table'] === 'civicrm_financial_type') {
+        $this->assertEquals(1, $restrictedAccount['entity_id']);
+      }
+    }
+  }
+
+}

ext/financialacls/tests/phpunit/Civi/Financialacls/FinancialAccountTest.php

@@ -0,0 +1,29 @@
+<?php
+
+namespace Civi\Financialacls;
+
+use Civi\Api4\FinancialAccount;
+
+// I fought the Autoloader and the autoloader won.
+require_once 'BaseTestClass.php';
+
+/**
+ * @group headless
+ */
+class FinancialAccountTest extends BaseTestClass {
+
+  /**
+   * Test only accounts with permitted income types can be retrieved.
+   *
+   * @throws \API_Exception
+   */
+  public function testGetFinancialAccount(): void {
+    $this->setupLoggedInUserWithLimitedFinancialTypeAccess();
+    $financialAccounts = FinancialAccount::get(FALSE)->execute();
+    $this->assertCount(14, $financialAccounts);
+    $restrictedAccounts = FinancialAccount::get()->execute();
+    $this->assertCount(1, $restrictedAccounts);
+    $this->assertEquals('Donation', $restrictedAccounts[0]['name']);
+  }
+
+}

ext/financialacls/tests/phpunit/Civi/Financialacls/FinancialTypeTest.php

@@ -62,4 +62,15 @@ public function testPermissionedFinancialTypes(): void {
     ], $permissions['administer CiviCRM Financial Types']);
   }
 
+  /**
+   * Test income financial types are acl filtered.
+   */
+  public function testGetIncomeFinancialType(): void {
+    $types = \CRM_Financial_BAO_FinancialType::getIncomeFinancialType();
+    $this->assertCount(4, $types);
+    $this->setupLoggedInUserWithLimitedFinancialTypeAccess();
+    $type = \CRM_Financial_BAO_FinancialType::getIncomeFinancialType();
+    $this->assertEquals([1 => 'Donation'], $type);
+  }
+
 }

tests/phpunit/CRM/Financial/BAO/FinancialTypeTest.php

@@ -159,8 +159,8 @@ public function testGetAvailableFinancialTypes() {
     $types = [];
     CRM_Financial_BAO_FinancialType::getAvailableFinancialTypes($types);
     $expectedResult = [
-      1 => "Donation",
-      2 => "Member Dues",
+      1 => 'Donation',
+      2 => 'Member Dues',
     ];
     $this->assertEquals($expectedResult, $types, 'Verify that only certain financial types can be retrieved');