From 463e521e651e258c5d42726aba7584cea775a2fd Mon Sep 17 00:00:00 2001 From: eileen Date: Mon, 12 Apr 2021 08:22:52 +1200 Subject: [PATCH] Use hook for checkAccess --- CRM/Core/DAO.php | 23 +++++++++++++-- CRM/Utils/Hook.php | 19 ++++++++++++ api/v3/Contribution.php | 22 +++++--------- ext/financialacls/financialacls.php | 29 +++++++++++++++++++ tests/phpunit/api/v3/FinancialTypeACLTest.php | 6 ++-- 5 files changed, 81 insertions(+), 18 deletions(-) diff --git a/CRM/Core/DAO.php b/CRM/Core/DAO.php index 506a971ba72a..63432693a81a 100644 --- a/CRM/Core/DAO.php +++ b/CRM/Core/DAO.php @@ -2960,7 +2960,7 @@ public function addSelectWhereClause() { $clauses[$fieldName] = CRM_Utils_SQL::mergeSubquery('Contact'); } // Clause for an entity_table/entity_id combo - if ($fieldName == 'entity_id' && isset($fields['entity_table'])) { + if ($fieldName === 'entity_id' && isset($fields['entity_table'])) { $relatedClauses = []; $relatedEntities = $this->buildOptions('entity_table', 'get'); foreach ((array) $relatedEntities as $table => $ent) { @@ -3007,9 +3007,28 @@ public static function getSelectWhereClause($tableAlias = NULL) { return $clauses; } + /** + * Check whether the given action can be performed on these values. + * + * The return is the input values array with inaccessible items filtered out. + * + * @param string $action + * @param array $values + * @param null $contactID + * + * @return array + */ + public function checkAccess(string $action, array $values, $contactID = NULL): array { + if (is_null($contactID)) { + $contactID = CRM_Core_Session::getLoggedInContactID(); + } + CRM_Utils_Hook::checkAccess(CRM_Core_DAO_AllCoreTables::getBriefName(get_class($this)), $action, $contactID, $values); + return $values; + } + /** * ensure database name is 'safe', i.e. only contains word characters (includes underscores) - * and dashes, and contains at least one [a-z] case insenstive. + * and dashes, and contains at least one [a-z] case insensitive. * * @param $database * diff --git a/CRM/Utils/Hook.php b/CRM/Utils/Hook.php index dc54996f1423..c8f731b6f9fd 100644 --- a/CRM/Utils/Hook.php +++ b/CRM/Utils/Hook.php @@ -2086,6 +2086,25 @@ public static function permission_check($permission, &$granted, $contactId) { ); } + /** + * Check whether the given contact has access to perform the given action. + * + * If the contact cannot perform the action the + * + * @param string $entity + * @param string $action + * @param int|null $contactID + * @param array $values + * + * @return mixed + */ + public static function checkAccess(string $entity, string $action, ?int $contactID, array &$values) { + return self::singleton()->invoke(['entity', 'action', 'contactID', 'values'], $entity, $action, $contactID, + $values, self::$_nullObject, self::$_nullObject, + 'civicrm_checkAccess' + ); + } + /** * Rotate the cryptographic key used in the database. * diff --git a/api/v3/Contribution.php b/api/v3/Contribution.php index 710c6ee7f073..79c2c6252c3d 100644 --- a/api/v3/Contribution.php +++ b/api/v3/Contribution.php @@ -204,23 +204,17 @@ function _civicrm_api3_contribution_create_legacy_support_45(&$params) { function civicrm_api3_contribution_delete($params) { $contributionID = !empty($params['contribution_id']) ? $params['contribution_id'] : $params['id']; - // First check contribution financial type - $financialType = CRM_Core_DAO::getFieldValue('CRM_Contribute_DAO_Contribution', $contributionID, 'financial_type_id'); - // Now check permissioned lineitems & permissioned contribution - if (!empty($params['check_permissions']) && CRM_Financial_BAO_FinancialType::isACLFinancialTypeStatus() && - ( - !CRM_Core_Permission::check('delete contributions of type ' . CRM_Contribute_PseudoConstant::financialType($financialType)) - || !CRM_Financial_BAO_FinancialType::checkPermissionedLineItems($contributionID, 'delete', FALSE) - ) - ) { - throw new API_Exception('You do not have permission to delete this contribution'); + if (!empty($params['check_permissions'])) { + $contribution = new CRM_Contribute_BAO_Contribution(); + $values = $contribution->checkAccess('delete', [$contributionID => ['id' => $contributionID]]); + if (empty($values)) { + throw new API_Exception('You do not have permission to delete this contribution'); + } } if (CRM_Contribute_BAO_Contribution::deleteContribution($contributionID)) { return civicrm_api3_create_success([$contributionID => 1]); } - else { - throw new API_Exception('Could not delete contribution'); - } + throw new API_Exception('Could not delete contribution'); } /** @@ -670,7 +664,7 @@ function _ipn_process_transaction($params, $contribution, $input, $ids) { static $domainFromName; static $domainFromEmail; if (empty($domainFromEmail) && (empty($params['receipt_from_name']) || empty($params['receipt_from_email']))) { - list($domainFromName, $domainFromEmail) = CRM_Core_BAO_Domain::getNameAndEmail(TRUE); + [$domainFromName, $domainFromEmail] = CRM_Core_BAO_Domain::getNameAndEmail(TRUE); } $input['receipt_from_name'] = CRM_Utils_Array::value('receipt_from_name', $params, $domainFromName); $input['receipt_from_email'] = CRM_Utils_Array::value('receipt_from_email', $params, $domainFromEmail); diff --git a/ext/financialacls/financialacls.php b/ext/financialacls/financialacls.php index ddd51b22a252..64023f1ac12a 100644 --- a/ext/financialacls/financialacls.php +++ b/ext/financialacls/financialacls.php @@ -286,6 +286,35 @@ function financialacls_civicrm_permission(&$permissions) { ]; } +/** + * @param string $entity + * @param string $action + * @param int|null $contactID + * @param array $values + * + * @throws \CRM_Core_Exception + */ +function financialacls_civicrm_checkAccess(string $entity, string $action, ?int $contactID, array &$values) { + if (!financialacls_is_acl_limiting_enabled()) { + return; + } + if ($action === 'delete' && $entity === 'Contribution') { + foreach ($values as $index => $details) { + $contributionID = $details['id']; + // First check contribution financial type + $financialType = CRM_Core_PseudoConstant::getName('CRM_Contribute_DAO_Contribution', 'financial_type_id', CRM_Core_DAO::getFieldValue('CRM_Contribute_DAO_Contribution', $contributionID, 'financial_type_id')); + // Now check permissioned line items & permissioned contribution + if (!CRM_Core_Permission::check('delete contributions of type ' . $financialType) || + !CRM_Financial_BAO_FinancialType::checkPermissionedLineItems($contributionID, 'delete', FALSE) + ) { + unset($values[$index]); + continue; + } + } + } + +} + /** * Remove unpermitted financial types from field Options in search context. * diff --git a/tests/phpunit/api/v3/FinancialTypeACLTest.php b/tests/phpunit/api/v3/FinancialTypeACLTest.php index 38b6eb83d78b..b019ed7eeaa3 100644 --- a/tests/phpunit/api/v3/FinancialTypeACLTest.php +++ b/tests/phpunit/api/v3/FinancialTypeACLTest.php @@ -291,8 +291,10 @@ public function testEditACLContribution() { /** * Test that acl contributions can be deleted. + * + * @throws \CRM_Core_Exception */ - public function testDeleteACLContribution() { + public function testDeleteACLContribution(): void { $this->enableFinancialACLs(); $this->setPermissions([ @@ -313,7 +315,7 @@ public function testDeleteACLContribution() { $this->addFinancialAclPermissions([['delete', 'Donation']]); $contribution = $this->callAPISuccess('Contribution', 'delete', $params); - $this->assertEquals($contribution['count'], 1); + $this->assertEquals(1, $contribution['count']); } public function testMembershipTypeACLFinancialTypeACL() {