From af4cccf728aaf85d323397fe43ee5c4ae2ec499f Mon Sep 17 00:00:00 2001 From: Tim Otten Date: Fri, 4 Jun 2021 11:57:55 -0700 Subject: [PATCH] Convert hook_civicrm_checkAccess to civi.api4.authorizeRecord --- CRM/Core/DAO.php | 33 ------- CRM/Utils/Hook.php | 26 ------ Civi/Api4/Event/AuthorizeRecordEvent.php | 91 +++++++++++++++++++ Civi/Api4/Utils/CoreUtil.php | 20 ++-- Civi/Core/Container.php | 1 + ext/financialacls/financialacls.php | 28 +++--- .../api/v4/Traits/CheckAccessTrait.php | 19 ++-- 7 files changed, 132 insertions(+), 86 deletions(-) create mode 100644 Civi/Api4/Event/AuthorizeRecordEvent.php diff --git a/CRM/Core/DAO.php b/CRM/Core/DAO.php index 07f21edfe5..c05658b106 100644 --- a/CRM/Core/DAO.php +++ b/CRM/Core/DAO.php @@ -3040,39 +3040,6 @@ 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 $entityName - * Ex: 'Contact' or 'Custom_Foobar' - * @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 $userID - * Contact ID of the active user (whose access we must check). NULL for anonymous. - * @return bool - * TRUE if granted. FALSE if prohibited. NULL if indeterminate. - */ - public static function checkAccess(string $entityName, string $action, array $record, $userID): ?bool { - // 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'); - } - if (method_exists(static::class, '_checkAccess')) { - return static::_checkAccess($entityName, $action, $record, $userID); - } - else { - return TRUE; - } - } - /** * ensure database name is 'safe', i.e. only contains word characters (includes underscores) * and dashes, and contains at least one [a-z] case insensitive. diff --git a/CRM/Utils/Hook.php b/CRM/Utils/Hook.php index f93c14e282..82877f3feb 100644 --- a/CRM/Utils/Hook.php +++ b/CRM/Utils/Hook.php @@ -2100,32 +2100,6 @@ 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|null $granted - * TRUE if granted. FALSE if prohibited. NULL if indeterminate. - */ - 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/Civi/Api4/Event/AuthorizeRecordEvent.php b/Civi/Api4/Event/AuthorizeRecordEvent.php new file mode 100644 index 0000000000..7e7fc15b41 --- /dev/null +++ b/Civi/Api4/Event/AuthorizeRecordEvent.php @@ -0,0 +1,91 @@ +setApiRequest($apiRequest); + $this->record = $record; + $this->userID = $userID; + } + + /** + * @inheritDoc + */ + public function getHookValues() { + return [$this->getApiRequest(), $this->record, &$this->authorized]; + } + + /** + * @return array + */ + public function getRecord(): array { + return $this->record; + } + + /** + * @return int|null + * Contact ID of the active/target user (whose access we must check). + * NULL for anonymous. + */ + public function getUserID(): ?int { + return $this->userID; + } + +} diff --git a/Civi/Api4/Utils/CoreUtil.php b/Civi/Api4/Utils/CoreUtil.php index e39e32e854..5ae51d7ac7 100644 --- a/Civi/Api4/Utils/CoreUtil.php +++ b/Civi/Api4/Utils/CoreUtil.php @@ -173,13 +173,21 @@ class CoreUtil { return (bool) $apiRequest->addSelect('id')->addWhere('id', '=', $record['id'])->execute()->count(); } - $granted = NULL; - \CRM_Utils_Hook::checkAccess($apiRequest->getEntityName(), $apiRequest->getActionName(), $record, $userID, $granted); - $baoName = self::getBAOFromApiName($apiRequest->getEntityName()); - if ($granted === NULL && $baoName) { - $granted = $baoName::checkAccess($apiRequest->getEntityName(), $apiRequest->getActionName(), $record, $userID); + $event = new \Civi\Api4\Event\AuthorizeRecordEvent($apiRequest, $record, $userID); + \Civi::dispatcher()->dispatch('civi.api4.authorizeRecord', $event); + + // Note: $bao::_checkAccess() is a quasi-listener. TODO: Convert to straight-up listener. + if ($event->isAuthorized() === NULL) { + $baoName = self::getBAOFromApiName($apiRequest->getEntityName()); + if ($baoName && method_exists($baoName, '_checkAccess')) { + $authorized = $baoName::_checkAccess($event->getEntityName(), $event->getActionName(), $event->getRecord(), $event->getUserID()); + $event->setAuthorized($authorized); + } + else { + $event->setAuthorized(TRUE); + } } - return $granted; + return $event->isAuthorized(); } /** diff --git a/Civi/Core/Container.php b/Civi/Core/Container.php index 8e2e926d1b..0632c649c4 100644 --- a/Civi/Core/Container.php +++ b/Civi/Core/Container.php @@ -384,6 +384,7 @@ class Container { }; $dispatcher->addListener('civi.api4.validate', $aliasMethodEvent('civi.api4.validate', 'getEntityName'), 100); + $dispatcher->addListener('civi.api4.authorizeRecord', $aliasMethodEvent('civi.api4.authorizeRecord', 'getEntityName'), 100); $dispatcher->addListener('civi.core.install', ['\Civi\Core\InstallationCanary', 'check']); $dispatcher->addListener('civi.core.install', ['\Civi\Core\DatabaseInitializer', 'initialize']); diff --git a/ext/financialacls/financialacls.php b/ext/financialacls/financialacls.php index a53d36f7ef..3451baeca8 100644 --- a/ext/financialacls/financialacls.php +++ b/ext/financialacls/financialacls.php @@ -14,6 +14,15 @@ function financialacls_civicrm_config(&$config) { _financialacls_civix_civicrm_config($config); } +/** + * @param \Symfony\Component\DependencyInjection\ContainerBuilder $container + */ +function financialacls_civicrm_container($container) { + $dispatcherDefn = $container->getDefinition('dispatcher'); + $container->addResource(new \Symfony\Component\Config\Resource\FileResource(__FILE__)); + $dispatcherDefn->addMethodCall('addListener', ['civi.api4.authorizeRecord::Contribution', '_financialacls_civi_api4_authorizeContribution']); +} + /** * Implements hook_civicrm_xmlMenu(). * @@ -287,27 +296,24 @@ function financialacls_civicrm_permission(&$permissions) { } /** - * @param string $entity - * @param string $action - * @param array $record - * @param int|null $contactID - * @param bool|null $granted + * Listener for 'civi.api4.authorizeRecord::Contribution' * + * @param \Civi\Api4\Event\AuthorizeRecordEvent $e * @throws \CRM_Core_Exception */ -function financialacls_civicrm_checkAccess(string $entity, string $action, array $record, ?int $contactID, ?bool &$granted) { +function _financialacls_civi_api4_authorizeContribution(\Civi\Api4\Event\AuthorizeRecordEvent $e) { if (!financialacls_is_acl_limiting_enabled()) { return; } - if ($action === 'delete' && $entity === 'Contribution') { - $contributionID = $record['id']; + if ($e->getActionName() === 'delete' && $e->getEntityName() === 'Contribution') { + $contributionID = $e->getRecord()['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) + if (!CRM_Core_Permission::check('delete contributions of type ' . $financialType, $e->getUserID()) || + !CRM_Financial_BAO_FinancialType::checkPermissionedLineItems($contributionID, 'delete', FALSE, $e->getUserID()) ) { - $granted = FALSE; + $e->setAuthorized(FALSE); } } } diff --git a/tests/phpunit/api/v4/Traits/CheckAccessTrait.php b/tests/phpunit/api/v4/Traits/CheckAccessTrait.php index d91aaae645..709be2c9ea 100644 --- a/tests/phpunit/api/v4/Traits/CheckAccessTrait.php +++ b/tests/phpunit/api/v4/Traits/CheckAccessTrait.php @@ -1,8 +1,10 @@ setCheckAccessGrants(['Contact::create' => TRUE]); @@ -28,17 +30,14 @@ trait CheckAccessTrait { protected $checkAccessCounts = []; /** - * @param string $entity - * @param string $action - * @param array $record - * @param int|null $contactID - * @param bool $granted - * @see \CRM_Utils_Hook::checkAccess() + * Listen to 'civi.api4.authorizeRecord'. Override decisions with specified grants. + * + * @param \Civi\Api4\Event\AuthorizeRecordEvent $e */ - public function hook_civicrm_checkAccess(string $entity, string $action, array $record, ?int $contactID, ?bool &$granted) { - $key = "{$entity}::{$action}"; + public function on_civi_api4_authorizeRecord(AuthorizeRecordEvent $e): void { + $key = $e->getEntityName() . '::' . $e->getActionName(); if (isset($this->checkAccessGrants[$key])) { - $granted = $this->checkAccessGrants[$key]; + $e->setAuthorized($this->checkAccessGrants[$key]); $this->checkAccessCounts[$key]++; } } -- 2.25.1