From 6a140950c5c2a3e11110475af4e4929c3b87d532 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/Contribute/BAO/ContributionPage.php | 10 +-- CRM/Core/Permission.php | 3 + CRM/Event/BAO/Event.php | 9 +-- CRM/Financial/BAO/FinancialType.php | 57 ++++++++++------- api/v3/Generic.php | 5 ++ ext/financialacls/financialacls.php | 63 ++++++++++++++++--- .../EntityFinancialAccountTest.php | 33 ++++++++++ .../Financialacls/FinancialAccountTest.php | 29 +++++++++ .../Civi/Financialacls/FinancialTypeTest.php | 11 ++++ .../CRM/Financial/BAO/FinancialTypeTest.php | 4 +- 10 files changed, 183 insertions(+), 41 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/Contribute/BAO/ContributionPage.php b/CRM/Contribute/BAO/ContributionPage.php index 447f38395f21..48d0828efd53 100644 --- a/CRM/Contribute/BAO/ContributionPage.php +++ b/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 []; } diff --git a/CRM/Core/Permission.php b/CRM/Core/Permission.php index 2ad9e9977e06..07eb1abf13ed 100644 --- a/CRM/Core/Permission.php +++ b/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'] = [ diff --git a/CRM/Event/BAO/Event.php b/CRM/Event/BAO/Event.php index d25115408594..160c5e1ea40e 100644 --- a/CRM/Event/BAO/Event.php +++ b/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 []; } diff --git a/CRM/Financial/BAO/FinancialType.php b/CRM/Financial/BAO/FinancialType.php index ecdb421af55a..88143c0fb6a3 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 @@ -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]; } /** diff --git a/api/v3/Generic.php b/api/v3/Generic.php index 9a5554760736..34ba7db2a139 100644 --- a/api/v3/Generic.php +++ b/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) { diff --git a/ext/financialacls/financialacls.php b/ext/financialacls/financialacls.php index bbbbd9c52f3d..ccd3722205a8 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 @@ -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. * 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 8b2e82a3381b..2f3cbcb3812a 100644 --- a/tests/phpunit/CRM/Financial/BAO/FinancialTypeTest.php +++ b/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');