From 70da392777d9c663ea79755b43cfec2e347b5f04 Mon Sep 17 00:00:00 2001 From: Tim Otten Date: Tue, 8 Jun 2021 19:46:17 -0700 Subject: [PATCH] Partially rollback changes to `$userID`. Merely lay groundwork for future update. Context: AuthorizeEvent did not allow tracking userID. AuthorizeRecordEvent is spec'd to track userID. This is a step toward supporting checks when the target user is non-present (ie not the user in the browser/session). However, this step is not *sufficient* - additional work is also needed to support non-present users. Original: AuthorizeEvent and AbstractAction::isAuthorized did not report current userID. However, the wiring for AuthorizeRecordEvent is spec'd to allow userID. Previous: Made a breaking change in the signature of AuthorizeEvent/AbstractAction::isAuthorized() to report userID. However, even with the break, it's not clear if this is the best approach. Revised: * Both AuthorizeEvent and AuthorizeRecordEvent report `userID`. This allows consumers to start using this information -- laying the groundwork for future changes. * If an existing event-consumer ignores the `userID`, it will still work as correctly as before. This is because we guarantee that the userID matches the session-user. * The signature of `AbstractAction::isAuthorized()` matches its original. No BC break. However, the method is flagged `@internal` to warn about the prospect of future changes. * In the future, after we do more legwork on to ensure that the overall system makes sense, we may flip this and start doing non-present users. --- CRM/Contact/AccessTrait.php | 4 +- CRM/Core/BAO/CustomValue.php | 6 +-- CRM/Core/DynamicFKAccessTrait.php | 4 +- Civi/API/Event/AuthorizeEvent.php | 9 ++++ Civi/API/Kernel.php | 2 +- Civi/Api4/Event/ActiveUserTrait.php | 43 +++++++++++++++++++ Civi/Api4/Event/AuthorizeRecordEvent.php | 26 +++-------- Civi/Api4/Generic/AbstractAction.php | 7 ++- 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 | 4 +- Civi/Api4/Generic/DAODeleteAction.php | 2 +- Civi/Api4/Generic/DAOUpdateAction.php | 4 +- Civi/Api4/Utils/CoreUtil.php | 14 +++--- api/v3/Contribution.php | 2 +- .../OnlyModifyOwnTokensTrait.php | 7 +-- 18 files changed, 89 insertions(+), 53 deletions(-) create mode 100644 Civi/Api4/Event/ActiveUserTrait.php diff --git a/CRM/Contact/AccessTrait.php b/CRM/Contact/AccessTrait.php index d5206e04b0..d7bfd66fb9 100644 --- a/CRM/Contact/AccessTrait.php +++ b/CRM/Contact/AccessTrait.php @@ -24,11 +24,11 @@ trait CRM_Contact_AccessTrait { * @param string $entityName * @param string $action * @param array $record - * @param int|NULL $userID + * @param int $userID * @return bool * @see CRM_Core_DAO::checkAccess */ - public static function _checkAccess(string $entityName, string $action, array $record, $userID) { + public static function _checkAccess(string $entityName, string $action, array $record, int $userID) { $cid = $record['contact_id'] ?? NULL; if (!$cid && !empty($record['id'])) { $cid = CRM_Core_DAO::getFieldValue(__CLASS__, $record['id'], 'contact_id'); diff --git a/CRM/Core/BAO/CustomValue.php b/CRM/Core/BAO/CustomValue.php index 2b42b91d7d..3f1760d799 100644 --- a/CRM/Core/BAO/CustomValue.php +++ b/CRM/Core/BAO/CustomValue.php @@ -228,12 +228,12 @@ class CRM_Core_BAO_CustomValue extends CRM_Core_DAO { * Ex: 'Contact' or 'Custom_Foobar' * @param string $action * @param array $record - * @param int|null $userID - * Contact ID of the active user (whose access we must check). NULL for anonymous. + * @param int $userID + * Contact ID of the active user (whose access we must check). 0 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 { + public static function _checkAccess(string $entityName, string $action, array $record, int $userID): ?bool { $groupName = substr($entityName, 0, 7) === 'Custom_' ? substr($entityName, 7) : NULL; if (!$groupName) { // $groupName is required but the function signature has to match the parent. diff --git a/CRM/Core/DynamicFKAccessTrait.php b/CRM/Core/DynamicFKAccessTrait.php index ebf8382553..041d53717d 100644 --- a/CRM/Core/DynamicFKAccessTrait.php +++ b/CRM/Core/DynamicFKAccessTrait.php @@ -24,11 +24,11 @@ trait CRM_Core_DynamicFKAccessTrait { * @param string $entityName * @param string $action * @param array $record - * @param int|NULL $userID + * @param int $userID * @return bool * @see CRM_Core_DAO::checkAccess */ - public static function _checkAccess(string $entityName, string $action, array $record, $userID): bool { + public static function _checkAccess(string $entityName, string $action, array $record, int $userID): bool { $eid = $record['entity_id'] ?? NULL; $table = $record['entity_table'] ?? NULL; if (!$eid && !empty($record['id'])) { diff --git a/Civi/API/Event/AuthorizeEvent.php b/Civi/API/Event/AuthorizeEvent.php index 164677a01b..d50411fb65 100644 --- a/Civi/API/Event/AuthorizeEvent.php +++ b/Civi/API/Event/AuthorizeEvent.php @@ -11,8 +11,11 @@ namespace Civi\API\Event; +use Civi\Api4\Event\ActiveUserTrait; + /** * Class AuthorizeEvent + * * @package Civi\API\Event * * Determine whether the API request is allowed for the current user. @@ -24,5 +27,11 @@ namespace Civi\API\Event; class AuthorizeEvent extends Event { use AuthorizedTrait; + use ActiveUserTrait; + + public function __construct($apiProvider, $apiRequest, $apiKernel, int $userID) { + parent::__construct($apiProvider, $apiRequest, $apiKernel); + $this->setUser($userID); + } } diff --git a/Civi/API/Kernel.php b/Civi/API/Kernel.php index f3cab8ed09..70395abd76 100644 --- a/Civi/API/Kernel.php +++ b/Civi/API/Kernel.php @@ -218,7 +218,7 @@ class Kernel { */ public function authorize($apiProvider, $apiRequest) { /** @var \Civi\API\Event\AuthorizeEvent $event */ - $event = $this->dispatcher->dispatch('civi.api.authorize', new AuthorizeEvent($apiProvider, $apiRequest, $this)); + $event = $this->dispatcher->dispatch('civi.api.authorize', new AuthorizeEvent($apiProvider, $apiRequest, $this, \CRM_Core_Session::getLoggedInContactID() ?: 0)); if (!$event->isAuthorized()) { throw new \Civi\API\Exception\UnauthorizedException("Authorization failed"); } diff --git a/Civi/Api4/Event/ActiveUserTrait.php b/Civi/Api4/Event/ActiveUserTrait.php new file mode 100644 index 0000000000..3cb8c82352 --- /dev/null +++ b/Civi/Api4/Event/ActiveUserTrait.php @@ -0,0 +1,43 @@ +userID = $userID; + return $this; + } + + /** + * @return int + * Contact ID of the active/target user (whose access we must check). + * 0 for anonymous. + */ + public function getUserID(): int { + return $this->userID; + } + +} diff --git a/Civi/Api4/Event/AuthorizeRecordEvent.php b/Civi/Api4/Event/AuthorizeRecordEvent.php index 7e7fc15b41..ba480de842 100644 --- a/Civi/Api4/Event/AuthorizeRecordEvent.php +++ b/Civi/Api4/Event/AuthorizeRecordEvent.php @@ -31,6 +31,7 @@ class AuthorizeRecordEvent extends GenericHookEvent { use RequestTrait; use AuthorizedTrait; + use ActiveUserTrait; /** * All (known/loaded) values of individual record being accessed. @@ -40,14 +41,6 @@ class AuthorizeRecordEvent extends GenericHookEvent { */ private $record; - /** - * Contact ID of the active/target user (whose access we must check). - * NULL for anonymous. - * - * @var int|null - */ - private $userID; - /** * CheckAccessEvent constructor. * @@ -55,14 +48,14 @@ class AuthorizeRecordEvent extends GenericHookEvent { * @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 + * @param int $userID * Contact ID of the active/target user (whose access we must check). - * NULL for anonymous. + * 0 for anonymous. */ - public function __construct($apiRequest, array $record, ?int $userID) { + public function __construct($apiRequest, array $record, int $userID) { $this->setApiRequest($apiRequest); $this->record = $record; - $this->userID = $userID; + $this->setUser($userID); } /** @@ -79,13 +72,4 @@ class AuthorizeRecordEvent extends GenericHookEvent { 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/Generic/AbstractAction.php b/Civi/Api4/Generic/AbstractAction.php index 275bd69077..a91e85853f 100644 --- a/Civi/Api4/Generic/AbstractAction.php +++ b/Civi/Api4/Generic/AbstractAction.php @@ -389,13 +389,12 @@ abstract class AbstractAction implements \ArrayAccess { * * This function is called if checkPermissions is set to true. * - * @param int|null $userID - * Contact ID of the user we are testing, or NULL for the default/active user. * @return bool + * @internal Implement/override in civicrm-core.git only. Signature may evolve. */ - public function isAuthorized(?int $userID): bool { + public function isAuthorized(): bool { $permissions = $this->getPermissions(); - return \CRM_Core_Permission::check($permissions, $userID); + return \CRM_Core_Permission::check($permissions); } /** diff --git a/Civi/Api4/Generic/AbstractCreateAction.php b/Civi/Api4/Generic/AbstractCreateAction.php index f6bfb2f3d3..c8b75eb870 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::checkAccessRecord($this, $this->getValues(), \CRM_Core_Session::getLoggedInContactID())) { + if ($this->checkPermissions && !CoreUtil::checkAccessRecord($this, $this->getValues(), \CRM_Core_Session::getLoggedInContactID() ?: 0)) { throw new UnauthorizedException("ACL check failed"); } diff --git a/Civi/Api4/Generic/AbstractSaveAction.php b/Civi/Api4/Generic/AbstractSaveAction.php index a0a65a964a..578e1dfc72 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::checkAccessDelegated($this->getEntityName(), $action, $record, \CRM_Core_Session::getLoggedInContactID())) { + if (!CoreUtil::checkAccessDelegated($this->getEntityName(), $action, $record, \CRM_Core_Session::getLoggedInContactID() ?: 0)) { throw new UnauthorizedException("ACL check failed"); } } diff --git a/Civi/Api4/Generic/BasicBatchAction.php b/Civi/Api4/Generic/BasicBatchAction.php index aa0dd47541..25b662e56b 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::checkAccessRecord($this, $item, \CRM_Core_Session::getLoggedInContactID())) { + if ($this->checkPermissions && !CoreUtil::checkAccessRecord($this, $item, \CRM_Core_Session::getLoggedInContactID() ?: 0)) { 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 f192978636..1eaabfe28c 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::checkAccessRecord($this, $record, \CRM_Core_Session::getLoggedInContactID())) { + if ($this->checkPermissions && !CoreUtil::checkAccessRecord($this, $record, \CRM_Core_Session::getLoggedInContactID() ?: 0)) { 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 85d3e92a08..c1f0e919d3 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::checkAccessDelegated($this->getEntityName(), $this->action, $this->values, \CRM_Core_Session::getLoggedInContactID()); + $granted = CoreUtil::checkAccessDelegated($this->getEntityName(), $this->action, $this->values, \CRM_Core_Session::getLoggedInContactID() ?: 0); } $result->exchangeArray([['access' => $granted]]); } @@ -62,7 +62,7 @@ class CheckAccessAction extends AbstractAction { * * @return bool */ - public function isAuthorized(?int $userID): bool { + public function isAuthorized(): bool { return TRUE; } diff --git a/Civi/Api4/Generic/DAODeleteAction.php b/Civi/Api4/Generic/DAODeleteAction.php index 6101de9e80..902d28838a 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::checkAccessRecord($this, $item, \CRM_Core_Session::getLoggedInContactID())) { + if (!CoreUtil::checkAccessRecord($this, $item, \CRM_Core_Session::getLoggedInContactID() ?: 0)) { 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 a359f18890..33adb73a2d 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::checkAccessRecord($this, $this->values, \CRM_Core_Session::getLoggedInContactID())) { + if ($this->checkPermissions && !CoreUtil::checkAccessRecord($this, $this->values, \CRM_Core_Session::getLoggedInContactID() ?: 0)) { 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::checkAccessRecord($this, $item, \CRM_Core_Session::getLoggedInContactID())) { + if ($this->checkPermissions && !CoreUtil::checkAccessRecord($this, $item, \CRM_Core_Session::getLoggedInContactID() ?: 0)) { throw new UnauthorizedException("ACL check failed"); } } diff --git a/Civi/Api4/Utils/CoreUtil.php b/Civi/Api4/Utils/CoreUtil.php index 5ae51d7ac7..8399142500 100644 --- a/Civi/Api4/Utils/CoreUtil.php +++ b/Civi/Api4/Utils/CoreUtil.php @@ -157,15 +157,15 @@ class CoreUtil { * * @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 anonymous user. + * @param int|string $userID + * Contact ID of the user we are testing,. 0 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 checkAccessRecord(\Civi\Api4\Generic\AbstractAction $apiRequest, array $record, ?int $userID) { + public static function checkAccessRecord(\Civi\Api4\Generic\AbstractAction $apiRequest, array $record, int $userID) { // 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. @@ -197,17 +197,17 @@ class CoreUtil { * @param string $entityName * @param string $actionName * @param array $record - * @param int|null $userID - * Contact ID of the user we are testing, or NULL for the anonymous user. + * @param int $userID + * Contact ID of the user we are testing, or 0 for the anonymous user. * * @return bool * @throws \API_Exception * @throws \CRM_Core_Exception */ - public static function checkAccessDelegated(string $entityName, string $actionName, array $record, ?int $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)) { + if (!$apiRequest->isAuthorized()) { return FALSE; } return static::checkAccessRecord($apiRequest, $record, $userID); diff --git a/api/v3/Contribution.php b/api/v3/Contribution.php index bf5335a6e2..80961ba5e3 100644 --- a/api/v3/Contribution.php +++ b/api/v3/Contribution.php @@ -204,7 +204,7 @@ 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']; - if (!empty($params['check_permissions']) && !\Civi\Api4\Utils\CoreUtil::checkAccessDelegated('Contribution', 'delete', ['id' => $contributionID], CRM_Core_Session::getLoggedInContactID())) { + if (!empty($params['check_permissions']) && !\Civi\Api4\Utils\CoreUtil::checkAccessDelegated('Contribution', 'delete', ['id' => $contributionID], CRM_Core_Session::getLoggedInContactID() ?: 0)) { throw new API_Exception('You do not have permission to delete this contribution'); } if (CRM_Contribute_BAO_Contribution::deleteContribution($contributionID)) { diff --git a/ext/oauth-client/Civi/Api4/Action/OAuthContactToken/OnlyModifyOwnTokensTrait.php b/ext/oauth-client/Civi/Api4/Action/OAuthContactToken/OnlyModifyOwnTokensTrait.php index 749b296d11..77817ab4aa 100644 --- a/ext/oauth-client/Civi/Api4/Action/OAuthContactToken/OnlyModifyOwnTokensTrait.php +++ b/ext/oauth-client/Civi/Api4/Action/OAuthContactToken/OnlyModifyOwnTokensTrait.php @@ -4,13 +4,14 @@ namespace Civi\Api4\Action\OAuthContactToken; trait OnlyModifyOwnTokensTrait { - public function isAuthorized(?int $loggedInContactID): bool { - if (\CRM_Core_Permission::check(['manage all OAuth contact tokens'], $loggedInContactID)) { + public function isAuthorized(): bool { + if (\CRM_Core_Permission::check(['manage all OAuth contact tokens'])) { return TRUE; } - if (!\CRM_Core_Permission::check(['manage my OAuth contact tokens'], $loggedInContactID)) { + if (!\CRM_Core_Permission::check(['manage my OAuth contact tokens'])) { return FALSE; } + $loggedInContactID = \CRM_Core_Session::singleton()->getLoggedInContactID(); foreach ($this->where as $clause) { [$field, $op, $val] = $clause; if ($field !== 'contact_id') { -- 2.25.1