Use acl, not blanket permissions on FinancialAccount, FinancialType, …

…EntityFinancialAccount
civicrm · Aug 20, 2021 · bde5c32 · bde5c32
1 parent b2c8e47
commit bde5c32
Show file tree

Hide file tree

Showing 7 changed files with 164 additions and 33 deletions.
diff --git a/CRM/Core/Permission.php b/CRM/Core/Permission.php
@@ -1137,6 +1137,9 @@ public static function getEntityActionPermissions() {
     $permissions['line_item'] = $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'] = [

diff --git a/CRM/Financial/BAO/FinancialType.php b/CRM/Financial/BAO/FinancialType.php
@@ -9,6 +9,9 @@
  +--------------------------------------------------------------------+
  */
 
+use Civi\API\Exception\UnauthorizedException;
+use Civi\Api4\EntityFinancialAccount;
+
 /**
  *
  * @package CRM
@@ -160,35 +163,40 @@ public static function del($financialTypeId) {
   }
 
   /**
-   * 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(): array {
+    // Realistically tests are the only place where logged in contact can
+    // change during the session at this stage.
+    $key = 'income_type' . CRM_Core_Session::getLoggedInContactID();
+    if (isset(Civi::$statics[__CLASS__][$key])) {
+      return Civi::$statics[__CLASS__][$key];
+    }
+    try {
+      $types = EntityFinancialAccount::get()
+        ->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];
   }
 
   /**

diff --git a/ext/financialacls/financialacls.php b/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
 
@@ -202,20 +203,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.
  *

diff --git a/ext/financialacls/tests/phpunit/Civi/Financialacls/EntityFinancialAccountTest.php b/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']);
+      }
+    }
+  }
+
+}
diff --git a/ext/financialacls/tests/phpunit/Civi/Financialacls/FinancialAccountTest.php b/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']);
+  }
+
+}
diff --git a/ext/financialacls/tests/phpunit/Civi/Financialacls/FinancialTypeTest.php b/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);
+  }
+
 }
diff --git a/tests/phpunit/CRM/Financial/BAO/FinancialTypeTest.php b/tests/phpunit/CRM/Financial/BAO/FinancialTypeTest.php
@@ -158,8 +158,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');