From 1c5787ad47eb79388ad78ac46ee978c82612aeeb Mon Sep 17 00:00:00 2001 From: Eileen McNaughton Date: Thu, 13 Jan 2022 13:27:44 +1300 Subject: [PATCH 1/2] Clean up buildPermissionedWhereClause --- CRM/Contact/BAO/Query.php | 6 ++- CRM/Financial/BAO/FinancialType.php | 37 +++++++++---------- .../Civi/Financialacls/FinancialTypeTest.php | 23 +++++++++++- 3 files changed, 44 insertions(+), 22 deletions(-) diff --git a/CRM/Contact/BAO/Query.php b/CRM/Contact/BAO/Query.php index a487b37af4d0..27b5de1bfc27 100644 --- 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/Financial/BAO/FinancialType.php b/CRM/Financial/BAO/FinancialType.php index d5e8618961b5..e5489e8ff923 100644 --- a/CRM/Financial/BAO/FinancialType.php +++ b/CRM/Financial/BAO/FinancialType.php @@ -343,35 +343,34 @@ 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') { + $types = array_keys(self::getAllEnabledAvailableFinancialTypes()); + if (empty($types)) { + $types = [0]; + } + $clauses[] = ' civicrm_contribution.financial_type_id IN (' . implode(',', $types) . ')'; } - 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/tests/phpunit/Civi/Financialacls/FinancialTypeTest.php b/ext/financialacls/tests/phpunit/Civi/Financialacls/FinancialTypeTest.php index bb68b08eabbc..4c07331e88a4 100644 --- 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 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 IN (1,4,2)', $whereClause); + } + } From 00d50ac2512daed1567fc1fc309d3c3085f80699 Mon Sep 17 00:00:00 2001 From: Eileen McNaughton Date: Thu, 13 Jan 2022 13:52:14 +1300 Subject: [PATCH 2/2] Move permission checks from Query & BAO to financialacl extension --- CRM/Contribute/BAO/Contribution.php | 29 +------------------ CRM/Financial/BAO/FinancialType.php | 6 +--- ext/financialacls/financialacls.php | 1 + .../Civi/Financialacls/FinancialTypeTest.php | 4 +-- .../CRM/Financial/BAO/FinancialTypeTest.php | 22 -------------- 5 files changed, 5 insertions(+), 57 deletions(-) diff --git a/CRM/Contribute/BAO/Contribution.php b/CRM/Contribute/BAO/Contribution.php index a44d125fb65a..4601737ae03b 100644 --- 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 index e5489e8ff923..2b3d81f5ece6 100644 --- a/CRM/Financial/BAO/FinancialType.php +++ b/CRM/Financial/BAO/FinancialType.php @@ -355,11 +355,7 @@ public static function buildPermissionedClause(string $component): string { return ''; } if ($component === 'contribution') { - $types = array_keys(self::getAllEnabledAvailableFinancialTypes()); - if (empty($types)) { - $types = [0]; - } - $clauses[] = ' civicrm_contribution.financial_type_id IN (' . implode(',', $types) . ')'; + $clauses = CRM_Contribute_BAO_Contribution::getSelectWhereClause(); } if ($component === 'membership') { self::getAvailableMembershipTypes($types, CRM_Core_Action::VIEW); diff --git a/ext/financialacls/financialacls.php b/ext/financialacls/financialacls.php index ccd3722205a8..d0b445b3baa2 100644 --- 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 index 4c07331e88a4..31594b86c2fc 100644 --- a/ext/financialacls/tests/phpunit/Civi/Financialacls/FinancialTypeTest.php +++ b/ext/financialacls/tests/phpunit/Civi/Financialacls/FinancialTypeTest.php @@ -81,7 +81,7 @@ public function testBuildPermissionedClause(): void { 'view contributions of type Member Dues', ]); $whereClause = \CRM_Financial_BAO_FinancialType::buildPermissionedClause('contribution'); - $this->assertEquals(' civicrm_contribution.financial_type_id IN (1,2)', $whereClause); + $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', @@ -89,7 +89,7 @@ public function testBuildPermissionedClause(): void { ]); $whereClause = \CRM_Financial_BAO_FinancialType::buildPermissionedClause('contribution'); - $this->assertEquals(' civicrm_contribution.financial_type_id IN (1,4,2)', $whereClause); + $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 index 2f3cbcb3812a..0e1ef1d8705d 100644 --- 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)'); - } - }