From 9cef60668e2373de629f43d8c32c9c8a16964b18 Mon Sep 17 00:00:00 2001 From: eileen Date: Mon, 12 Apr 2021 08:22:52 +1200 Subject: [PATCH] Add BAO function and hook for checkAccess This adds a static ::checkAccess function to all BAOs, which dispatches to a protected _checkAccess function in that BAO, as well as a new hook: hook_civicrm_checkAccess($entity, $action, $record, $contactID, &$granted) --- CRM/Core/DAO.php | 40 ++++++++++++++++++- CRM/Core/DAO/AllCoreTables.php | 2 +- CRM/Financial/BAO/FinancialType.php | 6 +-- CRM/Utils/Hook.php | 25 ++++++++++++ api/v3/Contribution.php | 16 ++------ ext/financialacls/financialacls.php | 26 ++++++++++++ tests/phpunit/api/v3/FinancialTypeACLTest.php | 6 ++- 7 files changed, 100 insertions(+), 21 deletions(-) diff --git a/CRM/Core/DAO.php b/CRM/Core/DAO.php index 80271dd449..d8dbad2760 100644 --- a/CRM/Core/DAO.php +++ b/CRM/Core/DAO.php @@ -2993,7 +2993,7 @@ SELECT contact_id $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) { @@ -3040,9 +3040,45 @@ SELECT contact_id return $clauses; } + /** + * Check whether action can be performed on a given record. + * + * Dispatches to internal BAO function ('static::_checkAccess())` and `hook_civicrm_checkAccess`. + * + * @param string $action + * APIv4 action name. + * Ex: 'create', 'get', 'delete' + * @param array $record + * All (known/loaded) values of individual record being accessed. + * The record should provide an 'id' but may otherwise be incomplete; guard accordingly. + * @param int $userID + * Contact ID of the active user (whose access we must check). + * @param bool $granted + * Initial value (usually TRUE, but the API might pass FALSE if gatekeeper permissions fail) + * + * @return bool + */ + public static function checkAccess(string $action, array $record, $userID = NULL, $granted = TRUE): bool { + $entityName = CRM_Core_DAO_AllCoreTables::getBriefName(static::class); + // Ensure this function was either called on a BAO class or a DAO that has no BAO + if (!$entityName || + (!strpos(static::class, '_BAO_') && CRM_Core_DAO_AllCoreTables::getBAOClassName(static::class) !== static::class) + ) { + throw new CRM_Core_Exception('Function checkAccess must be called on a BAO class'); + } + $userID = isset($userID) ? (int) $userID : CRM_Core_Session::getLoggedInContactID(); + // Dispatch to protected function _checkAccess in this BAO + if ($granted && method_exists(static::class, '_checkAccess')) { + $granted = static::_checkAccess($action, $record, $userID); + } + // Dispatch to hook + CRM_Utils_Hook::checkAccess($entityName, $action, $record, $userID, $granted); + return $granted; + } + /** * 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/Core/DAO/AllCoreTables.php b/CRM/Core/DAO/AllCoreTables.php index 04e4fe32dc..f117d68148 100644 --- a/CRM/Core/DAO/AllCoreTables.php +++ b/CRM/Core/DAO/AllCoreTables.php @@ -203,7 +203,7 @@ class CRM_Core_DAO_AllCoreTables { */ public static function getBAOClassName($daoName) { $baoName = str_replace('_DAO_', '_BAO_', $daoName); - return class_exists($baoName) ? $baoName : $daoName; + return $daoName === $baoName || class_exists($baoName) ? $baoName : $daoName; } /** diff --git a/CRM/Financial/BAO/FinancialType.php b/CRM/Financial/BAO/FinancialType.php index 9003eb8d16..86d3528066 100644 --- a/CRM/Financial/BAO/FinancialType.php +++ b/CRM/Financial/BAO/FinancialType.php @@ -420,20 +420,20 @@ class CRM_Financial_BAO_FinancialType extends CRM_Financial_DAO_FinancialType { * @param string $op * the mode of operation, can be add, view, edit, delete * @param bool $force + * @param int $contactID * * @return bool */ - public static function checkPermissionedLineItems($id, $op, $force = TRUE) { + public static function checkPermissionedLineItems($id, $op, $force = TRUE, $contactID = NULL) { if (!self::isACLFinancialTypeStatus()) { return TRUE; } $lineItems = CRM_Price_BAO_LineItem::getLineItemsByContributionID($id); $flag = FALSE; foreach ($lineItems as $items) { - if (!CRM_Core_Permission::check($op . ' contributions of type ' . CRM_Contribute_PseudoConstant::financialType($items['financial_type_id']))) { + if (!CRM_Core_Permission::check($op . ' contributions of type ' . CRM_Contribute_PseudoConstant::financialType($items['financial_type_id']), $contactID)) { if ($force) { throw new CRM_Core_Exception(ts('You do not have permission to access this page.')); - break; } $flag = FALSE; break; diff --git a/CRM/Utils/Hook.php b/CRM/Utils/Hook.php index 82877f3feb..e2907bd9a6 100644 --- a/CRM/Utils/Hook.php +++ b/CRM/Utils/Hook.php @@ -2100,6 +2100,31 @@ abstract class CRM_Utils_Hook { ); } + /** + * Check whether the given contact has access to perform the given action. + * + * If the contact cannot perform the action the + * + * @param string $entity + * APIv4 entity name. + * Ex: 'Contact', 'Email', 'Event' + * @param string $action + * APIv4 action name. + * Ex: 'create', 'get', 'delete' + * @param array $record + * All (known/loaded) values of individual record being accessed. + * The record should provide an 'id' but may otherwise be incomplete; guard accordingly. + * @param int|null $contactID + * Contact ID of the active user (whose access we must check). + * @param bool $granted + */ + public static function checkAccess(string $entity, string $action, array $record, ?int $contactID, bool &$granted) { + self::singleton()->invoke(['entity', 'action', 'record', 'contactID', 'granted'], $entity, $action, $record, + $contactID, $granted, 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 a4a3581361..eeae8e5855 100644 --- a/api/v3/Contribution.php +++ b/api/v3/Contribution.php @@ -204,23 +204,13 @@ 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) - ) - ) { + if (!empty($params['check_permissions']) && !CRM_Contribute_BAO_Contribution::checkAccess('delete', ['id' => $contributionID])) { 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'); } /** @@ -671,7 +661,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 ddd51b22a2..569346d72f 100644 --- a/ext/financialacls/financialacls.php +++ b/ext/financialacls/financialacls.php @@ -286,6 +286,32 @@ function financialacls_civicrm_permission(&$permissions) { ]; } +/** + * @param string $entity + * @param string $action + * @param array $record + * @param int|null $contactID + * @param bool $granted + * + * @throws \CRM_Core_Exception + */ +function financialacls_civicrm_checkAccess(string $entity, string $action, array $record, ?int $contactID, bool &$granted) { + if (!financialacls_is_acl_limiting_enabled()) { + return; + } + if ($action === 'delete' && $entity === 'Contribution') { + $contributionID = $record['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, $contactID) || + !CRM_Financial_BAO_FinancialType::checkPermissionedLineItems($contributionID, 'delete', FALSE, $contactID) + ) { + $granted = FALSE; + } + } +} + /** * 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 38b6eb83d7..b019ed7eea 100644 --- a/tests/phpunit/api/v3/FinancialTypeACLTest.php +++ b/tests/phpunit/api/v3/FinancialTypeACLTest.php @@ -291,8 +291,10 @@ class api_v3_FinancialTypeACLTest extends CiviUnitTestCase { /** * 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 @@ class api_v3_FinancialTypeACLTest extends CiviUnitTestCase { $this->addFinancialAclPermissions([['delete', 'Donation']]); $contribution = $this->callAPISuccess('Contribution', 'delete', $params); - $this->assertEquals($contribution['count'], 1); + $this->assertEquals(1, $contribution['count']); } public function testMembershipTypeACLFinancialTypeACL() { -- 2.25.1