(REF) Consolidate calls to `Hook::checkAccess()`. Define initial value `$granted...

Regarding invocations:

* Before: There are three different ways `Hook::checkAccess()` may be invoked, e.g.
    * `CRM_Core_DAO::checkAccess()`, which sprinkles in a call to `static::_checkAccess()` before `Hook::checkAccess()`
    * `CRM_Core_BAO_CustomValue::checkAccess()`, which sprinkles in a call to `checkAccessDelegated()` after `Hook::checkAccess()`
    * `CoreUtil::checkAccessRecord()`, which delegates to one of the above (if appropriate) or else calls `Hook::checkAccess()`
    * `CoreUtil::checkAccessRecord()` is the most general entry-point
* After: There is one way to invoke `Hook::checkAccess()`, and it incorporates some qausi/unofficial listeners.
    * `CoreUtil::checkAccessRecord()` is still the most general entry-point.
    * `CoreUtil::checkAccessRecord()` fires `Hook::checkAccess()` unconditionally
    * `CoreUtil::checkAccessRecord()` calls `CRM_Core_DAO::checkAccess()` and/or `CRM_Core_BAO_CustomValue::_checkAccess()`,
      which are now quasi/unofficial listeners for the hook

Regarding initialization and passing of `$granted`:

* Before: The value of `$granted` defaults to `TRUE`. Listeners may flip between `TRUE`/`FALSE`. The value of `$granted` is passed to each listener.
* After: The value of `$granted` defaults to `NULL`. Listeners may flip to `TRUE`/`FALSE`. If it remains `NULL` until the end, then it's treated as `TRUE`.
  The value of `$granted` is not passed to each listener.
* Comment: IMHO, this is an overall simplification.  If you pass in `$granted`, then each listener has to decide
  whether/how to mix the inputted value with its own decision.  (Ex: Should it be `return $grantedInput &&
  $myGrantedDecision` or `return $grantedInput || $myGrantedDecision` or `return $myGrantedDecision`? That choice appears to be
  carefully informed by the context of what steps ran before.) In the updated protocol, each `_checkAccess()` a smaller scope.

diff --git a/CRM/Core/BAO/CustomValue.php b/CRM/Core/BAO/CustomValue.php
index 02f7eb41043d00439dec8663ccfaea22e44336f8..2b42b91d7d59801534387971cc605404220dd907 100644 (file)
--- a/CRM/Core/BAO/CustomValue.php
+++ b/CRM/Core/BAO/CustomValue.php
@@ -230,10 +230,10 @@ class CRM_Core_BAO_CustomValue extends CRM_Core_DAO {
    * @param array $record
    * @param int|null $userID
    *   Contact ID of the active user (whose access we must check). NULL for anonymous.
-   * @param bool $granted
    * @return bool
+   *   TRUE if granted. FALSE if prohibited. NULL if indeterminate.
    */
-  public static function checkAccess(string $entityName, string $action, array $record, $userID, $granted = TRUE): bool {
+  public static function _checkAccess(string $entityName, string $action, array $record, $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.
@@ -251,13 +251,7 @@ class CRM_Core_BAO_CustomValue extends CRM_Core_DAO {
       $cid = CRM_Core_DAO::singleValueQuery("SELECT entity_id FROM `$tableName` WHERE id = " . (int) $record['id']);
     }
 
-    // Dispatch to hook
-    $granted = NULL;
-    CRM_Utils_Hook::checkAccess("Custom_$groupName", $action, $record, $userID, $granted);
-    if ($granted === NULL) {
-      $granted = \Civi\Api4\Utils\CoreUtil::checkAccessDelegated('Contact', 'update', ['id' => $cid], $userID);
-    }
-    return $granted;
+    return \Civi\Api4\Utils\CoreUtil::checkAccessDelegated('Contact', 'update', ['id' => $cid], $userID);
   }
 
 }

diff --git a/CRM/Core/DAO.php b/CRM/Core/DAO.php
index 89340462d5fe5a7794956deccb6aeb4c3613719c..07f21edfe552ac6b67c9b5207909fdbaf157ab5f 100644 (file)
--- a/CRM/Core/DAO.php
+++ b/CRM/Core/DAO.php
@@ -3055,25 +3055,22 @@ SELECT contact_id
    *   The record should provide an 'id' but may otherwise be incomplete; guard accordingly.
    * @param int|null $userID
    *   Contact ID of the active user (whose access we must check). NULL for anonymous.
-   * @param bool $granted
-   *   Initial value (usually TRUE, but the API might pass FALSE if gatekeeper permissions fail)
-   *
    * @return bool
+   *   TRUE if granted. FALSE if prohibited. NULL if indeterminate.
    */
-  public static function checkAccess(string $entityName, string $action, array $record, $userID, $granted = TRUE): bool {
+  public static function checkAccess(string $entityName, string $action, array $record, $userID): ?bool {
     // Ensure this function was either called on a BAO class or a DAO that has no BAO
     if (!$entityName ||
       (!strpos(static::class, '_BAO_') && CRM_Core_DAO_AllCoreTables::getBAOClassName(static::class) !== static::class)
     ) {
       throw new CRM_Core_Exception('Function checkAccess must be called on a BAO class');
     }
-    // Dispatch to protected function _checkAccess in this BAO
-    if ($granted && method_exists(static::class, '_checkAccess')) {
-      $granted = static::_checkAccess($entityName, $action, $record, $userID);
+    if (method_exists(static::class, '_checkAccess')) {
+      return static::_checkAccess($entityName, $action, $record, $userID);
+    }
+    else {
+      return TRUE;
     }
-    // Dispatch to hook
-    CRM_Utils_Hook::checkAccess($entityName, $action, $record, $userID, $granted);
-    return $granted;
   }
 
   /**

diff --git a/CRM/Utils/Hook.php b/CRM/Utils/Hook.php
index e2907bd9a6fdcccad798c03f2de3d81cd8ee4451..f93c14e28226ee96c71d7b8aeb11b5395edfae8d 100644 (file)
--- a/CRM/Utils/Hook.php
+++ b/CRM/Utils/Hook.php
@@ -2116,9 +2116,10 @@ abstract class CRM_Utils_Hook {
    *   The record should provide an 'id' but may otherwise be incomplete; guard accordingly.
    * @param int|null $contactID
    *   Contact ID of the active user (whose access we must check).
-   * @param bool $granted
+   * @param bool|null $granted
+   *   TRUE if granted. FALSE if prohibited. NULL if indeterminate.
    */
-  public static function checkAccess(string $entity, string $action, array $record, ?int $contactID, bool &$granted) {
+  public static function checkAccess(string $entity, string $action, array $record, ?int $contactID, ?bool &$granted) {
     self::singleton()->invoke(['entity', 'action', 'record', 'contactID', 'granted'], $entity, $action, $record,
       $contactID, $granted, self::$_nullObject,
       'civicrm_checkAccess'

diff --git a/Civi/Api4/Utils/CoreUtil.php b/Civi/Api4/Utils/CoreUtil.php
index 902f5cbabcee9b18c0ce75091fe9c813c10567ce..e39e32e854ac0ee1fb8e1316d94b36d317e139f0 100644 (file)
--- a/Civi/Api4/Utils/CoreUtil.php
+++ b/Civi/Api4/Utils/CoreUtil.php
@@ -166,23 +166,18 @@ class CoreUtil {
    * @throws \Civi\API\Exception\UnauthorizedException
    */
   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($apiRequest, '\Civi\Api4\Generic\DAOGetAction')) {
-      $granted = $granted && $apiRequest->addSelect('id')->addWhere('id', '=', $record['id'])->execute()->count();
+      return (bool) $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($apiRequest->getEntityName());
-      if ($baoName) {
-        $granted = $baoName::checkAccess($apiRequest->getEntityName(), $apiRequest->getActionName(), $record, $userID, $granted);
-      }
-      // Otherwise, call the hook directly
-      else {
-        \CRM_Utils_Hook::checkAccess($apiRequest->getEntityName(), $apiRequest->getActionName(), $record, $userID, $granted);
-      }
+
+    $granted = NULL;
+    \CRM_Utils_Hook::checkAccess($apiRequest->getEntityName(), $apiRequest->getActionName(), $record, $userID, $granted);
+    $baoName = self::getBAOFromApiName($apiRequest->getEntityName());
+    if ($granted === NULL && $baoName) {
+      $granted = $baoName::checkAccess($apiRequest->getEntityName(), $apiRequest->getActionName(), $record, $userID);
     }
     return $granted;
   }

diff --git a/ext/financialacls/financialacls.php b/ext/financialacls/financialacls.php
index 569346d72fcc9ead74364ebadeb4bd3c517c6868..a53d36f7effd3bb45bacf3af61aeb3da3526d90c 100644 (file)
--- a/ext/financialacls/financialacls.php
+++ b/ext/financialacls/financialacls.php
@@ -291,11 +291,11 @@ function financialacls_civicrm_permission(&$permissions) {
  * @param string $action
  * @param array $record
  * @param int|null $contactID
- * @param bool $granted
+ * @param bool|null $granted
  *
  * @throws \CRM_Core_Exception
  */
-function financialacls_civicrm_checkAccess(string $entity, string $action, array $record, ?int $contactID, bool &$granted) {
+function financialacls_civicrm_checkAccess(string $entity, string $action, array $record, ?int $contactID, ?bool &$granted) {
   if (!financialacls_is_acl_limiting_enabled()) {
     return;
   }

diff --git a/tests/phpunit/api/v4/Traits/CheckAccessTrait.php b/tests/phpunit/api/v4/Traits/CheckAccessTrait.php
index ddc7826b17f741023115ec536200a51024f8d5e2..d91aaae6452366ef49b1af95bb5c3a34081975db 100644 (file)
--- a/tests/phpunit/api/v4/Traits/CheckAccessTrait.php
+++ b/tests/phpunit/api/v4/Traits/CheckAccessTrait.php
@@ -35,7 +35,7 @@ trait CheckAccessTrait {
    * @param bool $granted
    * @see \CRM_Utils_Hook::checkAccess()
    */
-  public function hook_civicrm_checkAccess(string $entity, string $action, array $record, ?int $contactID, bool &$granted) {
+  public function hook_civicrm_checkAccess(string $entity, string $action, array $record, ?int $contactID, ?bool &$granted) {
     $key = "{$entity}::{$action}";
     if (isset($this->checkAccessGrants[$key])) {
       $granted = $this->checkAccessGrants[$key];