From aa1565de929fe2bb29dca9bb753075194c8c9bf6 Mon Sep 17 00:00:00 2001 From: Eileen McNaughton Date: Thu, 19 Aug 2021 12:25:17 +1200 Subject: [PATCH] Use acl, not blanket permissions on FinancialAccount, FinancialType, EntityFinancialAccount --- CRM/Core/Permission.php | 3 + CRM/Financial/BAO/FinancialType.php | 55 +++++++++------- ext/financialacls/financialacls.php | 63 ++++++++++++++++--- .../EntityFinancialAccountTest.php | 33 ++++++++++ .../Financialacls/FinancialAccountTest.php | 29 +++++++++ .../Civi/Financialacls/FinancialTypeTest.php | 11 ++++ .../CRM/Financial/BAO/FinancialTypeTest.php | 4 +- 7 files changed, 164 insertions(+), 34 deletions(-) create mode 100644 ext/financialacls/tests/phpunit/Civi/Financialacls/EntityFinancialAccountTest.php create mode 100644 ext/financialacls/tests/phpunit/Civi/Financialacls/FinancialAccountTest.php diff --git a/CRM/Core/Permission.php b/CRM/Core/Permission.php index 6e167ea48aea..2002273dd5ea 100644 --- 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 index 0444ba96032c..273503ebfc5e 100644 --- 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,39 @@ 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]; } - return $financialType; + 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'); + } + catch (UnauthorizedException $e) { + Civi::$statics[__CLASS__][$key] = []; + } + foreach ($types as $type) { + Civi::$statics[__CLASS__][$key][$type['id']] = $type['financial_type.name']; + } + return Civi::$statics[__CLASS__][$key]; } /** diff --git a/ext/financialacls/financialacls.php b/ext/financialacls/financialacls.php index a0d94540682f..722027f03001 100644 --- 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 new file mode 100644 index 000000000000..41f43b2a5ffa --- /dev/null +++ b/ext/financialacls/tests/phpunit/Civi/Financialacls/EntityFinancialAccountTest.php @@ -0,0 +1,33 @@ +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 new file mode 100644 index 000000000000..cc071f93afbf --- /dev/null +++ b/ext/financialacls/tests/phpunit/Civi/Financialacls/FinancialAccountTest.php @@ -0,0 +1,29 @@ +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 index c43511981fcb..bb68b08eabbc 100644 --- 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 index 0258437bd505..4d91724d26b5 100644 --- 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');