From 849354a5e1bcdd1812f3848095b1571ffd082676 Mon Sep 17 00:00:00 2001 From: Tim Otten Date: Sun, 6 Jun 2021 23:28:43 -0700 Subject: [PATCH] (REF) Change CoreUtil::checkAccess() to CoreUtil::checkAccessRecord() This change invovles a few things: 1. Pass the `AbstractAction $apiRequest` instead of the tuple `string $entity, string $action`. 2. There are a couple cases where we don't actually want to re-use the current `$apiRequest`. Switch these using `checkAccessDelegated()`. 3. Always resolve the userID before calling `checkAccessRecord()`. `$userID===null` can mean two different things (ie "active user" vs "anonymous user"). By resolving this once before we do any work with `checkAccess()`, we ensure that it will consistently mean "anonymous user" (even if there are multiple rounds of delegation). 3. Change the name from `checkAccess()` to `checkAccessRecord`. There are a few flavors of `...checkAccess...`, and this makes it easier to differentiate when skimming. --- CRM/Core/DAO.php | 1 - Civi/Api4/Generic/AbstractCreateAction.php | 2 +- Civi/Api4/Generic/AbstractSaveAction.php | 2 +- Civi/Api4/Generic/BasicBatchAction.php | 2 +- Civi/Api4/Generic/BasicUpdateAction.php | 2 +- Civi/Api4/Generic/CheckAccessAction.php | 2 +- Civi/Api4/Generic/DAODeleteAction.php | 2 +- Civi/Api4/Generic/DAOUpdateAction.php | 4 +-- Civi/Api4/Utils/CoreUtil.php | 31 +++++++++++----------- 9 files changed, 24 insertions(+), 24 deletions(-) diff --git a/CRM/Core/DAO.php b/CRM/Core/DAO.php index 25d88a379f..89340462d5 100644 --- a/CRM/Core/DAO.php +++ b/CRM/Core/DAO.php @@ -3067,7 +3067,6 @@ SELECT contact_id ) { 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($entityName, $action, $record, $userID); diff --git a/Civi/Api4/Generic/AbstractCreateAction.php b/Civi/Api4/Generic/AbstractCreateAction.php index b08cb3e39c..f6bfb2f3d3 100644 --- a/Civi/Api4/Generic/AbstractCreateAction.php +++ b/Civi/Api4/Generic/AbstractCreateAction.php @@ -70,7 +70,7 @@ abstract class AbstractCreateAction extends AbstractAction { throw new \API_Exception("Mandatory values missing from Api4 {$this->getEntityName()}::{$this->getActionName()}: " . implode(", ", $unmatched), "mandatory_missing", ["fields" => $unmatched]); } - if ($this->checkPermissions && !CoreUtil::checkAccess($this->getEntityName(), $this->getActionName(), $this->getValues())) { + if ($this->checkPermissions && !CoreUtil::checkAccessRecord($this, $this->getValues(), \CRM_Core_Session::getLoggedInContactID())) { throw new UnauthorizedException("ACL check failed"); } diff --git a/Civi/Api4/Generic/AbstractSaveAction.php b/Civi/Api4/Generic/AbstractSaveAction.php index aefd3725ed..a0a65a964a 100644 --- a/Civi/Api4/Generic/AbstractSaveAction.php +++ b/Civi/Api4/Generic/AbstractSaveAction.php @@ -112,7 +112,7 @@ abstract class AbstractSaveAction extends AbstractAction { if ($this->checkPermissions) { foreach ($this->records as $record) { $action = empty($record[$this->idField]) ? 'create' : 'update'; - if (!CoreUtil::checkAccess($this->getEntityName(), $action, $record)) { + if (!CoreUtil::checkAccessDelegated($this->getEntityName(), $action, $record, \CRM_Core_Session::getLoggedInContactID())) { throw new UnauthorizedException("ACL check failed"); } } diff --git a/Civi/Api4/Generic/BasicBatchAction.php b/Civi/Api4/Generic/BasicBatchAction.php index 1806f9165b..aa0dd47541 100644 --- a/Civi/Api4/Generic/BasicBatchAction.php +++ b/Civi/Api4/Generic/BasicBatchAction.php @@ -67,7 +67,7 @@ class BasicBatchAction extends AbstractBatchAction { */ public function _run(Result $result) { foreach ($this->getBatchRecords() as $item) { - if ($this->checkPermissions && !CoreUtil::checkAccess($this->getEntityName(), $this->getActionName(), $item)) { + if ($this->checkPermissions && !CoreUtil::checkAccessRecord($this, $item, \CRM_Core_Session::getLoggedInContactID())) { throw new UnauthorizedException("ACL check failed"); } $result[] = $this->doTask($item); diff --git a/Civi/Api4/Generic/BasicUpdateAction.php b/Civi/Api4/Generic/BasicUpdateAction.php index f00dfe358f..f192978636 100644 --- a/Civi/Api4/Generic/BasicUpdateAction.php +++ b/Civi/Api4/Generic/BasicUpdateAction.php @@ -63,7 +63,7 @@ class BasicUpdateAction extends AbstractUpdateAction { $this->validateValues(); foreach ($this->getBatchRecords() as $item) { $record = $this->values + $item; - if ($this->checkPermissions && !CoreUtil::checkAccess($this->getEntityName(), $this->getActionName(), $record)) { + if ($this->checkPermissions && !CoreUtil::checkAccessRecord($this, $record, \CRM_Core_Session::getLoggedInContactID())) { throw new UnauthorizedException("ACL check failed"); } $result[] = $this->writeRecord($record); diff --git a/Civi/Api4/Generic/CheckAccessAction.php b/Civi/Api4/Generic/CheckAccessAction.php index bdbc821567..85d3e92a08 100644 --- a/Civi/Api4/Generic/CheckAccessAction.php +++ b/Civi/Api4/Generic/CheckAccessAction.php @@ -52,7 +52,7 @@ class CheckAccessAction extends AbstractAction { $granted = TRUE; } else { - $granted = CoreUtil::checkAccess($this->getEntityName(), $this->action, $this->values); + $granted = CoreUtil::checkAccessDelegated($this->getEntityName(), $this->action, $this->values, \CRM_Core_Session::getLoggedInContactID()); } $result->exchangeArray([['access' => $granted]]); } diff --git a/Civi/Api4/Generic/DAODeleteAction.php b/Civi/Api4/Generic/DAODeleteAction.php index de5ed1a07f..6101de9e80 100644 --- a/Civi/Api4/Generic/DAODeleteAction.php +++ b/Civi/Api4/Generic/DAODeleteAction.php @@ -43,7 +43,7 @@ class DAODeleteAction extends AbstractBatchAction { if ($this->getCheckPermissions()) { foreach ($items as $key => $item) { - if (!CoreUtil::checkAccess($this->getEntityName(), $this->getActionName(), $item)) { + if (!CoreUtil::checkAccessRecord($this, $item, \CRM_Core_Session::getLoggedInContactID())) { throw new UnauthorizedException("ACL check failed"); } $items[$key]['check_permissions'] = TRUE; diff --git a/Civi/Api4/Generic/DAOUpdateAction.php b/Civi/Api4/Generic/DAOUpdateAction.php index b545fbc225..a359f18890 100644 --- a/Civi/Api4/Generic/DAOUpdateAction.php +++ b/Civi/Api4/Generic/DAOUpdateAction.php @@ -63,7 +63,7 @@ class DAOUpdateAction extends AbstractUpdateAction { // Update a single record by ID unless select requires more than id if ($this->getSelect() === ['id'] && count($this->where) === 1 && $this->where[0][0] === 'id' && $this->where[0][1] === '=' && !empty($this->where[0][2])) { $this->values['id'] = $this->where[0][2]; - if ($this->checkPermissions && !CoreUtil::checkAccess($this->getEntityName(), $this->getActionName(), $this->values)) { + if ($this->checkPermissions && !CoreUtil::checkAccessRecord($this, $this->values, \CRM_Core_Session::getLoggedInContactID())) { throw new UnauthorizedException("ACL check failed"); } $items = [$this->values]; @@ -76,7 +76,7 @@ class DAOUpdateAction extends AbstractUpdateAction { $items = $this->getBatchRecords(); foreach ($items as &$item) { $item = $this->values + $item; - if ($this->checkPermissions && !CoreUtil::checkAccess($this->getEntityName(), $this->getActionName(), $item)) { + if ($this->checkPermissions && !CoreUtil::checkAccessRecord($this, $item, \CRM_Core_Session::getLoggedInContactID())) { throw new UnauthorizedException("ACL check failed"); } } diff --git a/Civi/Api4/Utils/CoreUtil.php b/Civi/Api4/Utils/CoreUtil.php index adc2a1670d..902f5cbabc 100644 --- a/Civi/Api4/Utils/CoreUtil.php +++ b/Civi/Api4/Utils/CoreUtil.php @@ -155,36 +155,33 @@ class CoreUtil { /** * Check if current user is authorized to perform specified action on a given entity. * - * @param string $entityName - * @param string $actionName + * @param \Civi\Api4\Generic\AbstractAction $apiRequest * @param array $record * @param int|null $userID - * Contact ID of the user we are testing, or NULL for the default/active user. + * Contact ID of the user we are testing, or NULL for the anonymous user. * @return bool * @throws \API_Exception * @throws \CRM_Core_Exception * @throws \Civi\API\Exception\NotImplementedException * @throws \Civi\API\Exception\UnauthorizedException */ - public static function checkAccess(string $entityName, string $actionName, array $record, $userID = NULL) { - $action = Request::create($entityName, $actionName, ['version' => 4]); - // This checks gatekeeper permissions - $granted = $action->isAuthorized($userID); + public static function checkAccessRecord(\Civi\Api4\Generic\AbstractAction $apiRequest, array $record, ?int $userID) { + $granted = TRUE; // For get actions, just run a get and ACLs will be applied to the query. // It's a cheap trick and not as efficient as not running the query at all, // but BAO::checkAccess doesn't consistently check permissions for the "get" action. - if (is_a($action, '\Civi\Api4\Generic\DAOGetAction')) { - $granted = $granted && $action->addSelect('id')->addWhere('id', '=', $record['id'])->execute()->count(); + if (is_a($apiRequest, '\Civi\Api4\Generic\DAOGetAction')) { + $granted = $granted && $apiRequest->addSelect('id')->addWhere('id', '=', $record['id'])->execute()->count(); } else { // If entity has a BAO, run the BAO::checkAccess function, which will call the hook - $baoName = self::getBAOFromApiName($entityName); + $baoName = self::getBAOFromApiName($apiRequest->getEntityName()); if ($baoName) { - $granted = $baoName::checkAccess($entityName, $actionName, $record, $userID, $granted); + $granted = $baoName::checkAccess($apiRequest->getEntityName(), $apiRequest->getActionName(), $record, $userID, $granted); } // Otherwise, call the hook directly else { - \CRM_Utils_Hook::checkAccess($entityName, $actionName, $record, $userID, $granted); + \CRM_Utils_Hook::checkAccess($apiRequest->getEntityName(), $apiRequest->getActionName(), $record, $userID, $granted); } } return $granted; @@ -204,9 +201,13 @@ class CoreUtil { * @throws \API_Exception * @throws \CRM_Core_Exception */ - public static function checkAccessDelegated(string $entityName, string $actionName, array $record, $userID = NULL) { - // FIXME: Move isAuthorized check into here. It's redundant for normal checkAccess(). - return static::checkAccess($entityName, $actionName, $record, $userID); + public static function checkAccessDelegated(string $entityName, string $actionName, array $record, ?int $userID) { + $apiRequest = Request::create($entityName, $actionName, ['version' => 4]); + // TODO: Should probably emit civi.api.authorize for checking guardian permission; but in APIv4 with std cfg, this is de-facto equivalent. + if (!$apiRequest->isAuthorized($userID)) { + return FALSE; + } + return static::checkAccessRecord($apiRequest, $record, $userID); } } -- 2.25.1