Merge pull request #22495 from eileenmcnaughton/fin2

Move permission checks from Query & BAO to financialacl extension
civicrm · Jan 14, 2022 · 31f306c · 31f306c
2 parents be0c852 + 00d50ac
commit 31f306c
Show file tree

Hide file tree

Showing 6 changed files with 42 additions and 72 deletions.
diff --git a/CRM/Contact/BAO/Query.php b/CRM/Contact/BAO/Query.php
@@ -581,7 +581,11 @@ public function initialize($apiEntity = NULL) {
     }
     if (isset($component) && !$this->_skipPermission) {
       // Unit test coverage in api_v3_FinancialTypeACLTest::testGetACLContribution.
-      CRM_Financial_BAO_FinancialType::buildPermissionedClause($this->_whereClause, $component);
+      $clauses = CRM_Financial_BAO_FinancialType::buildPermissionedClause($component);
+      if (!empty($this->_whereClause) && !empty($clauses)) {
+        $this->_whereClause .= ' AND ';
+      }
+      $this->_whereClause .= $clauses;
     }
 
     $this->_fromClause = self::fromClause($this->_tables, NULL, NULL, $this->_primaryLocation, $this->_mode, $apiEntity);

diff --git a/CRM/Contribute/BAO/Contribution.php b/CRM/Contribute/BAO/Contribution.php
@@ -1157,34 +1157,7 @@ protected static function disconnectPledgePaymentsIfCancelled(int $pledgePayment
   }
 
   /**
-   * @inheritDoc
-   */
-  public function addSelectWhereClause() {
-    $whereClauses = parent::addSelectWhereClause();
-    if ($whereClauses !== []) {
-      // In this case permisssions have been applied & we assume the
-      // financialaclreport is applying these
-      // https://github.com/JMAConsulting/biz.jmaconsulting.financialaclreport/blob/master/financialaclreport.php#L107
-      return $whereClauses;
-    }
-
-    if (!CRM_Financial_BAO_FinancialType::isACLFinancialTypeStatus()) {
-      return $whereClauses;
-    }
-    $types = CRM_Financial_BAO_FinancialType::getAllEnabledAvailableFinancialTypes();
-    if (empty($types)) {
-      $whereClauses['financial_type_id'] = 'IN (0)';
-    }
-    else {
-      $whereClauses['financial_type_id'] = [
-        'IN (' . implode(',', array_keys($types)) . ')',
-      ];
-    }
-    return $whereClauses;
-  }
-
-  /**
-   * @param null $status
+   * @param string $status
    * @param null $startDate
    * @param null $endDate
    *

diff --git a/CRM/Financial/BAO/FinancialType.php b/CRM/Financial/BAO/FinancialType.php
@@ -343,35 +343,30 @@ public static function addACLClausesToWhereClauses(&$whereClauses) {
   /**
    * Function to build a permissioned sql where clause based on available financial types.
    *
-   * @param array $whereClauses
-   *   (reference ) an array of clauses
    * @param string $component
    *   the type of component
-   * @param string $alias
-   *   the alias to use
    *
+   * @return string $clauses
    */
-  public static function buildPermissionedClause(&$whereClauses, $component = NULL, $alias = NULL) {
+  public static function buildPermissionedClause(string $component): string {
+    $clauses = [];
     // @todo the relevant addSelectWhere clause should be called.
     if (!self::isACLFinancialTypeStatus()) {
-      return FALSE;
+      return '';
     }
-    if ($component == 'contribution') {
-      $types = self::getAllEnabledAvailableFinancialTypes();
-      $column = "financial_type_id";
+    if ($component === 'contribution') {
+      $clauses = CRM_Contribute_BAO_Contribution::getSelectWhereClause();
     }
-    if ($component == 'membership') {
+    if ($component === 'membership') {
       self::getAvailableMembershipTypes($types, CRM_Core_Action::VIEW);
-      $column = "membership_type_id";
-    }
-    if (!empty($whereClauses)) {
-      $whereClauses .= ' AND ';
-    }
-    if (empty($types)) {
-      $whereClauses .= " civicrm_{$component}.{$column} IN (0)";
-      return;
+      $types = array_keys($types);
+      if (empty($types)) {
+        $types = [0];
+      }
+      $clauses[] = ' civicrm_membership.membership_type_id IN (' . implode(',', $types) . ')';
+
     }
-    $whereClauses .= " civicrm_{$component}.{$column} IN (" . implode(',', array_keys($types)) . ")";
+    return implode(' AND ', $clauses);
   }
 
   /**

diff --git a/ext/financialacls/financialacls.php b/ext/financialacls/financialacls.php
@@ -139,6 +139,7 @@ function financialacls_civicrm_selectWhereClause($entity, &$clauses) {
     case 'LineItem':
     case 'MembershipType':
     case 'ContributionRecur':
+    case 'Contribution':
       $clauses['financial_type_id'] = _financialacls_civicrm_get_type_clause();
       break;
 

diff --git a/ext/financialacls/tests/phpunit/Civi/Financialacls/FinancialTypeTest.php b/ext/financialacls/tests/phpunit/Civi/Financialacls/FinancialTypeTest.php
@@ -16,8 +16,6 @@ class FinancialTypeTest extends BaseTestClass {
   /**
    * Test that a message is put in session when changing the name of a
    * financial type.
-   *
-   * @throws \CRM_Core_Exception
    */
   public function testChangeFinancialTypeName(): void {
     Civi::settings()->set('acl_financial_type', TRUE);
@@ -73,4 +71,25 @@ public function testGetIncomeFinancialType(): void {
     $this->assertEquals([1 => 'Donation'], $type);
   }
 
+  /**
+   * Check method test buildPermissionedClause()
+   */
+  public function testBuildPermissionedClause(): void {
+    Civi::settings()->set('acl_financial_type', 1);
+    $this->setPermissions([
+      'view contributions of type Donation',
+      'view contributions of type Member Dues',
+    ]);
+    $whereClause = \CRM_Financial_BAO_FinancialType::buildPermissionedClause('contribution');
+    $this->assertEquals('(`civicrm_contribution`.`financial_type_id` IS NULL OR (`civicrm_contribution`.`financial_type_id` IN (1,2)))', $whereClause);
+    $this->setPermissions([
+      'view contributions of type Donation',
+      'view contributions of type Member Dues',
+      'view contributions of type Event Fee',
+    ]);
+
+    $whereClause = \CRM_Financial_BAO_FinancialType::buildPermissionedClause('contribution');
+    $this->assertEquals('(`civicrm_contribution`.`financial_type_id` IS NULL OR (`civicrm_contribution`.`financial_type_id` IN (1,4,2)))', $whereClause);
+  }
+
 }
diff --git a/tests/phpunit/CRM/Financial/BAO/FinancialTypeTest.php b/tests/phpunit/CRM/Financial/BAO/FinancialTypeTest.php
@@ -302,26 +302,4 @@ public function testCheckPermissionedLineItems() {
     $this->assertEquals($perm, TRUE, 'Verify that lineitems now have permission.');
   }
 
-  /**
-   * Check method testisACLFinancialTypeStatus()
-   */
-  public function testBuildPermissionedClause() {
-    $this->setACL();
-    $this->setPermissions([
-      'view contributions of type Donation',
-      'view contributions of type Member Dues',
-    ]);
-    CRM_Financial_BAO_FinancialType::buildPermissionedClause($whereClause, 'contribution');
-    $this->assertEquals($whereClause, ' civicrm_contribution.financial_type_id IN (1,2)');
-    $this->setPermissions([
-      'view contributions of type Donation',
-      'view contributions of type Member Dues',
-      'view contributions of type Event Fee',
-    ]);
-    $whereClause = NULL;
-
-    CRM_Financial_BAO_FinancialType::buildPermissionedClause($whereClause, 'contribution');
-    $this->assertEquals($whereClause, ' civicrm_contribution.financial_type_id IN (1,4,2)');
-  }
-
 }