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.

diff --git a/CRM/Contact/AccessTrait.php b/CRM/Contact/AccessTrait.php
index d5206e04b0a18b655573dbb41e610b1a78ad30bc..d7bfd66fb9fec05dae13f526723b97a2a5718727 100644 (file)
--- 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 2b42b91d7d59801534387971cc605404220dd907..3f1760d799c0ce9fd2f2ddd1df64ea401498fdc6 100644 (file)
--- 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 ebf83825539b5f72998fae79e7953f307807ab22..041d53717d56b27422328ba427a58de5da3e0423 100644 (file)
--- 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 164677a01b241f6e327a5a0f218e1b3770a61188..d50411fb65cf06ac2f462204edf6f911b3bea096 100644 (file)
--- 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 f3cab8ed09c24d352702796fb58575711ba2bcef..70395abd76810e223b4c514092ce4e969889e23d 100644 (file)
--- 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 (file)
index 0000000..3cb8c82
--- /dev/null
+++ b/Civi/Api4/Event/ActiveUserTrait.php
@@ -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;
+  }
+
+}

diff --git a/Civi/Api4/Event/AuthorizeRecordEvent.php b/Civi/Api4/Event/AuthorizeRecordEvent.php
index 7e7fc15b41d70cc489de9185545fa38c14e10b32..ba480de842261b4125625f0dd000d430ea5eb264 100644 (file)
--- 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 275bd690774a18d1fabe871c379834214a1df544..a91e85853ffc9f872ebb0e40ef0a5e259f849877 100644 (file)
--- 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 f6bfb2f3d3002718d00a2d1de8efd6f7dccae1c0..c8b75eb870e33b5eff0c30ff50a51caeffae3d25 100644 (file)
--- 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 a0a65a964a6863b98e5b74254f47d63515950952..578e1dfc7238c0624713d61bbeba4917029086fa 100644 (file)
--- 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 aa0dd475413d947a3360951c2aa6d46aa673eaa3..25b662e56bc96a43dedfe78fbc1adc5eccd475ce 100644 (file)
--- 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 f192978636e16b84efbebb9c34349cb8b0c26bbe..1eaabfe28c3f4172af3394032c56c0a6d176e2ca 100644 (file)
--- 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 85d3e92a08464e459efbd8033b00356699b01364..c1f0e919d399270b51570f54f9cd184a9c22dd5a 100644 (file)
--- 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 6101de9e807a98c73f936808940d01fd0f66cb0e..902d28838a2d4baa261c7ecc07c9892670b56448 100644 (file)
--- 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 a359f1889002004cfc457191bf9cc375aa4d9b61..33adb73a2d9c36883a61d25a82bdc891b93dfa36 100644 (file)
--- 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 5ae51d7ac7ac79a84ba35f68ba0cc89108948c82..8399142500b78322b87776b6bb480aee79d1c736 100644 (file)
--- 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 bf5335a6e24bcf037b595e1072505dbd66486fed..80961ba5e3133339f48766b69f67f7ec49d3d13e 100644 (file)
--- 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 749b296d114f967f19bcfbe68110736a687942a0..77817ab4aa156a6dfdc7dde24b9a6a293499c5a5 100644 (file)
--- 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') {