From 929a9585fec8683805087f134d6185a0cc84322b Mon Sep 17 00:00:00 2001 From: Coleman Watts Date: Tue, 27 Apr 2021 14:51:02 -0400 Subject: [PATCH] APIv4 - Add checkAccess action Call checkAccess action before creating, updating or deleting --- Civi/Api4/CustomValue.php | 7 ++ Civi/Api4/Generic/AbstractCreateAction.php | 8 ++ Civi/Api4/Generic/AbstractEntity.php | 7 ++ Civi/Api4/Generic/AbstractSaveAction.php | 13 +++ Civi/Api4/Generic/BasicBatchAction.php | 5 ++ Civi/Api4/Generic/BasicUpdateAction.php | 8 +- Civi/Api4/Generic/CheckAccessAction.php | 80 +++++++++++++++++++ Civi/Api4/Generic/DAODeleteAction.php | 6 ++ Civi/Api4/Generic/DAOUpdateAction.php | 9 +++ Civi/Api4/Utils/CoreUtil.php | 37 +++++++++ .../api/v4/Action/BasicActionsTest.php | 2 +- 11 files changed, 180 insertions(+), 2 deletions(-) create mode 100644 Civi/Api4/Generic/CheckAccessAction.php diff --git a/Civi/Api4/CustomValue.php b/Civi/Api4/CustomValue.php index 40b4d96160..e036aa48e8 100644 --- a/Civi/Api4/CustomValue.php +++ b/Civi/Api4/CustomValue.php @@ -122,6 +122,13 @@ class CustomValue { ->setCheckPermissions($checkPermissions); } + /** + * @return \Civi\Api4\Generic\CheckAccessAction + */ + public static function checkAccess($customGroup) { + return new Generic\CheckAccessAction("Custom_$customGroup", __FUNCTION__); + } + /** * @see \Civi\Api4\Generic\AbstractEntity::permissions() * @return array diff --git a/Civi/Api4/Generic/AbstractCreateAction.php b/Civi/Api4/Generic/AbstractCreateAction.php index 153e26ce6f..b08cb3e39c 100644 --- a/Civi/Api4/Generic/AbstractCreateAction.php +++ b/Civi/Api4/Generic/AbstractCreateAction.php @@ -20,6 +20,8 @@ namespace Civi\Api4\Generic; use Civi\Api4\Event\ValidateValuesEvent; +use Civi\API\Exception\UnauthorizedException; +use Civi\Api4\Utils\CoreUtil; /** * Base class for all `Create` api actions. @@ -59,6 +61,7 @@ abstract class AbstractCreateAction extends AbstractAction { /** * @throws \API_Exception + * @throws \Civi\API\Exception\UnauthorizedException */ protected function validateValues() { // FIXME: There should be a protocol to report a full list of errors... Perhaps a subclass of API_Exception? @@ -66,6 +69,11 @@ abstract class AbstractCreateAction extends AbstractAction { if ($unmatched) { 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())) { + throw new UnauthorizedException("ACL check failed"); + } + $e = new ValidateValuesEvent($this, [$this->getValues()], new \CRM_Utils_LazyArray(function () { return [['old' => NULL, 'new' => $this->getValues()]]; })); diff --git a/Civi/Api4/Generic/AbstractEntity.php b/Civi/Api4/Generic/AbstractEntity.php index 72ef2bab19..348015fb3c 100644 --- a/Civi/Api4/Generic/AbstractEntity.php +++ b/Civi/Api4/Generic/AbstractEntity.php @@ -55,6 +55,13 @@ abstract class AbstractEntity { */ abstract public static function getFields(); + /** + * @return \Civi\Api4\Generic\CheckAccessAction + */ + public static function checkAccess() { + return new CheckAccessAction(self::getEntityName(), __FUNCTION__); + } + /** * Returns a list of permissions needed to access the various actions in this api. * diff --git a/Civi/Api4/Generic/AbstractSaveAction.php b/Civi/Api4/Generic/AbstractSaveAction.php index 29f329cc66..aefd3725ed 100644 --- a/Civi/Api4/Generic/AbstractSaveAction.php +++ b/Civi/Api4/Generic/AbstractSaveAction.php @@ -20,6 +20,8 @@ namespace Civi\Api4\Generic; use Civi\Api4\Event\ValidateValuesEvent; +use Civi\API\Exception\UnauthorizedException; +use Civi\Api4\Utils\CoreUtil; /** * Create or update one or more $ENTITIES. @@ -93,6 +95,7 @@ abstract class AbstractSaveAction extends AbstractAction { /** * @throws \API_Exception + * @throws \Civi\API\Exception\UnauthorizedException */ protected function validateValues() { // FIXME: There should be a protocol to report a full list of errors... Perhaps a subclass of API_Exception? @@ -105,6 +108,16 @@ abstract class AbstractSaveAction extends AbstractAction { if ($unmatched) { throw new \API_Exception("Mandatory values missing from Api4 {$this->getEntityName()}::{$this->getActionName()}: " . implode(", ", $unmatched), "mandatory_missing", ["fields" => $unmatched]); } + + if ($this->checkPermissions) { + foreach ($this->records as $record) { + $action = empty($record[$this->idField]) ? 'create' : 'update'; + if (!CoreUtil::checkAccess($this->getEntityName(), $action, $record)) { + throw new UnauthorizedException("ACL check failed"); + } + } + } + $e = new ValidateValuesEvent($this, $this->records, new \CRM_Utils_LazyArray(function() { $existingIds = array_column($this->records, $this->idField); $existing = civicrm_api4($this->getEntityName(), 'get', [ diff --git a/Civi/Api4/Generic/BasicBatchAction.php b/Civi/Api4/Generic/BasicBatchAction.php index d1787bd44a..1806f9165b 100644 --- a/Civi/Api4/Generic/BasicBatchAction.php +++ b/Civi/Api4/Generic/BasicBatchAction.php @@ -20,6 +20,8 @@ namespace Civi\Api4\Generic; use Civi\API\Exception\NotImplementedException; +use Civi\API\Exception\UnauthorizedException; +use Civi\Api4\Utils\CoreUtil; /** * $ACTION one or more $ENTITIES. @@ -65,6 +67,9 @@ class BasicBatchAction extends AbstractBatchAction { */ public function _run(Result $result) { foreach ($this->getBatchRecords() as $item) { + if ($this->checkPermissions && !CoreUtil::checkAccess($this->getEntityName(), $this->getActionName(), $item)) { + 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 4fd8fe6c15..f00dfe358f 100644 --- a/Civi/Api4/Generic/BasicUpdateAction.php +++ b/Civi/Api4/Generic/BasicUpdateAction.php @@ -20,6 +20,8 @@ namespace Civi\Api4\Generic; use Civi\API\Exception\NotImplementedException; +use Civi\API\Exception\UnauthorizedException; +use Civi\Api4\Utils\CoreUtil; /** * Update one or more $ENTITY with new values. @@ -60,7 +62,11 @@ class BasicUpdateAction extends AbstractUpdateAction { $this->formatWriteValues($this->values); $this->validateValues(); foreach ($this->getBatchRecords() as $item) { - $result[] = $this->writeRecord($this->values + $item); + $record = $this->values + $item; + if ($this->checkPermissions && !CoreUtil::checkAccess($this->getEntityName(), $this->getActionName(), $record)) { + throw new UnauthorizedException("ACL check failed"); + } + $result[] = $this->writeRecord($record); } } diff --git a/Civi/Api4/Generic/CheckAccessAction.php b/Civi/Api4/Generic/CheckAccessAction.php new file mode 100644 index 0000000000..cc26a5e6e5 --- /dev/null +++ b/Civi/Api4/Generic/CheckAccessAction.php @@ -0,0 +1,80 @@ +action === 'checkAccess') { + $granted = TRUE; + } + else { + $granted = CoreUtil::checkAccess($this->getEntityName(), $this->action, $this->values); + } + $result->exchangeArray([['access' => $granted]]); + } + + /** + * This action is always allowed + * + * @return bool + */ + public function isAuthorized() { + return TRUE; + } + + /** + * Add an item to the values array + * @param string $fieldName + * @param mixed $value + * @return $this + */ + public function addValue(string $fieldName, $value) { + $this->values[$fieldName] = $value; + return $this; + } + +} diff --git a/Civi/Api4/Generic/DAODeleteAction.php b/Civi/Api4/Generic/DAODeleteAction.php index ecd9c91c13..bf335c0f40 100644 --- a/Civi/Api4/Generic/DAODeleteAction.php +++ b/Civi/Api4/Generic/DAODeleteAction.php @@ -19,6 +19,9 @@ namespace Civi\Api4\Generic; +use Civi\API\Exception\UnauthorizedException; +use Civi\Api4\Utils\CoreUtil; + /** * Delete one or more $ENTITIES. * @@ -53,6 +56,9 @@ class DAODeleteAction extends AbstractBatchAction { if ($this->getCheckPermissions()) { foreach (array_keys($items) as $key) { + if (!CoreUtil::checkAccess($this->getEntityName(), $this->getActionName(), $items[$key])) { + throw new UnauthorizedException("ACL check failed"); + } $items[$key]['check_permissions'] = TRUE; $this->checkContactPermissions($baoName, $items[$key]); } diff --git a/Civi/Api4/Generic/DAOUpdateAction.php b/Civi/Api4/Generic/DAOUpdateAction.php index aef6618729..b545fbc225 100644 --- a/Civi/Api4/Generic/DAOUpdateAction.php +++ b/Civi/Api4/Generic/DAOUpdateAction.php @@ -19,6 +19,9 @@ namespace Civi\Api4\Generic; +use Civi\API\Exception\UnauthorizedException; +use Civi\Api4\Utils\CoreUtil; + /** * Update one or more $ENTITY with new values. * @@ -60,6 +63,9 @@ 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)) { + throw new UnauthorizedException("ACL check failed"); + } $items = [$this->values]; $this->validateValues(); $result->exchangeArray($this->writeObjects($items)); @@ -70,6 +76,9 @@ 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)) { + throw new UnauthorizedException("ACL check failed"); + } } $this->validateValues(); diff --git a/Civi/Api4/Utils/CoreUtil.php b/Civi/Api4/Utils/CoreUtil.php index c5b0d5e829..65a3d81ed9 100644 --- a/Civi/Api4/Utils/CoreUtil.php +++ b/Civi/Api4/Utils/CoreUtil.php @@ -19,6 +19,7 @@ namespace Civi\Api4\Utils; +use Civi\API\Request; use CRM_Core_DAO_AllCoreTables as AllCoreTables; class CoreUtil { @@ -151,4 +152,40 @@ class CoreUtil { return $customGroupName && \CRM_Core_DAO::getFieldValue('CRM_Core_DAO_CustomGroup', $customGroupName, 'is_multiple', 'name'); } + /** + * Check if current user is authorized to perform specified action on a given entity. + * + * @param string $entityName + * @param string $actionName + * @param array $record + * @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) { + $action = Request::create($entityName, $actionName, ['version' => 4]); + // This checks gatekeeper permissions + $granted = $action->isAuthorized(); + // 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(); + } + else { + $baoName = self::getBAOFromApiName($entityName); + // If entity has a BAO, run the BAO::checkAccess function, which will call the hook + if ($baoName && strpos($baoName, '_BAO_')) { + $baoName::checkAccess($actionName, $record, NULL, $granted); + } + // Otherwise, call the hook directly + else { + \CRM_Utils_Hook::checkAccess($entityName, $actionName, $record, NULL, $granted); + } + } + return $granted; + } + } diff --git a/tests/phpunit/api/v4/Action/BasicActionsTest.php b/tests/phpunit/api/v4/Action/BasicActionsTest.php index 40048d6e4b..68fcf7c341 100644 --- a/tests/phpunit/api/v4/Action/BasicActionsTest.php +++ b/tests/phpunit/api/v4/Action/BasicActionsTest.php @@ -260,7 +260,7 @@ class BasicActionsTest extends UnitTestCase { public function testEmptyAndNullOperators() { $records = [ - [], + ['id' => NULL], ['color' => '', 'weight' => 0], ['color' => 'yellow', 'weight' => 100000000000], ]; -- 2.25.1