Skip to content

Commit

Permalink
Partially rollback changes to $userID. Merely lay groundwork for fu…
Browse files Browse the repository at this point in the history
…ture 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.
  • Loading branch information
totten committed Jun 9, 2021
1 parent af4cccf commit 70da392
Show file tree
Hide file tree
Showing 18 changed files with 89 additions and 53 deletions.
4 changes: 2 additions & 2 deletions CRM/Contact/AccessTrait.php
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down
6 changes: 3 additions & 3 deletions CRM/Core/BAO/CustomValue.php
Original file line number Diff line number Diff line change
Expand Up @@ -228,12 +228,12 @@ public function addSelectWhereClause() {
* 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.
Expand Down
4 changes: 2 additions & 2 deletions CRM/Core/DynamicFKAccessTrait.php
Original file line number Diff line number Diff line change
Expand Up @@ -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'])) {
Expand Down
9 changes: 9 additions & 0 deletions Civi/API/Event/AuthorizeEvent.php
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -24,5 +27,11 @@
class AuthorizeEvent extends Event {

use AuthorizedTrait;
use ActiveUserTrait;

public function __construct($apiProvider, $apiRequest, $apiKernel, int $userID) {
parent::__construct($apiProvider, $apiRequest, $apiKernel);
$this->setUser($userID);
}

}
2 changes: 1 addition & 1 deletion Civi/API/Kernel.php
Original file line number Diff line number Diff line change
Expand Up @@ -218,7 +218,7 @@ public function resolve($apiRequest) {
*/
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");
}
Expand Down
43 changes: 43 additions & 0 deletions Civi/Api4/Event/ActiveUserTrait.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
<?php

namespace Civi\Api4\Event;

trait ActiveUserTrait {

/**
* Contact ID of the active/target user (whose access we must check).
* 0 for anonymous.
*
* @var int
*/
private $userID;

/**
* @param int|null $userID
* Contact ID of the active/target user (whose access we must check).
* 0 for anonymous.
* @return $this
*/
protected function setUser(int $userID) {
$loggedInContactID = \CRM_Core_Session::getLoggedInContactID() ?: 0;
if ($userID !== $loggedInContactID) {
throw new \RuntimeException("The API subsystem does not yet fully support variable user IDs.");
// Traditionally, the API events did not emit information about the current user; it was assumed
// that the user was the logged-in user. This may be expanded in the future to support some more edge-cases.
// For now, the semantics are unchanged - but we've begun reporting the active userID so that
// consumers can start adopting it.
}
$this->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;
}

}
26 changes: 5 additions & 21 deletions Civi/Api4/Event/AuthorizeRecordEvent.php
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ class AuthorizeRecordEvent extends GenericHookEvent {

use RequestTrait;
use AuthorizedTrait;
use ActiveUserTrait;

/**
* All (known/loaded) values of individual record being accessed.
Expand All @@ -40,29 +41,21 @@ 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.
*
* @param \Civi\Api4\Generic\AbstractAction $apiRequest
* @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);
}

/**
Expand All @@ -79,13 +72,4 @@ 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;
}

}
7 changes: 3 additions & 4 deletions Civi/Api4/Generic/AbstractAction.php
Original file line number Diff line number Diff line change
Expand Up @@ -389,13 +389,12 @@ public function offsetUnset($offset) {
*
* 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);
}

/**
Expand Down
2 changes: 1 addition & 1 deletion Civi/Api4/Generic/AbstractCreateAction.php
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ protected function validateValues() {
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");
}

Expand Down
2 changes: 1 addition & 1 deletion Civi/Api4/Generic/AbstractSaveAction.php
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ protected function validateValues() {
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");
}
}
Expand Down
2 changes: 1 addition & 1 deletion Civi/Api4/Generic/BasicBatchAction.php
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ public function __construct($entityName, $actionName, $select = 'id', $doer = NU
*/
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);
Expand Down
2 changes: 1 addition & 1 deletion Civi/Api4/Generic/BasicUpdateAction.php
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ public function _run(Result $result) {
$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);
Expand Down
4 changes: 2 additions & 2 deletions Civi/Api4/Generic/CheckAccessAction.php
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ public function _run(Result $result) {
$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]]);
}
Expand All @@ -62,7 +62,7 @@ public function _run(Result $result) {
*
* @return bool
*/
public function isAuthorized(?int $userID): bool {
public function isAuthorized(): bool {
return TRUE;
}

Expand Down
2 changes: 1 addition & 1 deletion Civi/Api4/Generic/DAODeleteAction.php
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ public function _run(Result $result) {

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;
Expand Down
4 changes: 2 additions & 2 deletions Civi/Api4/Generic/DAOUpdateAction.php
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ public function _run(Result $result) {
// 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];
Expand All @@ -76,7 +76,7 @@ public function _run(Result $result) {
$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");
}
}
Expand Down
14 changes: 7 additions & 7 deletions Civi/Api4/Utils/CoreUtil.php
Original file line number Diff line number Diff line change
Expand Up @@ -157,15 +157,15 @@ private static function isCustomEntity($customGroupName) {
*
* @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.
Expand Down Expand Up @@ -197,17 +197,17 @@ public static function checkAccessRecord(\Civi\Api4\Generic\AbstractAction $apiR
* @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);
Expand Down
2 changes: 1 addition & 1 deletion api/v3/Contribution.php
Original file line number Diff line number Diff line change
Expand Up @@ -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)) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,14 @@

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') {
Expand Down

0 comments on commit 70da392

Please sign in to comment.