From: Tim Otten <totten@civicrm.org>
Date: Mon, 7 Jun 2021 10:12:10 +0000 (-0700)
Subject: (REF) Consolidate calls to `Hook::checkAccess()`. Define initial value `$granted... 
X-Git-Url: https://vcs.fsf.org/?a=commitdiff_plain;h=e294cebf4a8cf872a022eaae82250b5231dbf27e;p=civicrm-core.git

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

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 02f7eb4104..2b42b91d7d 100644
--- 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 89340462d5..07f21edfe5 100644
--- 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 e2907bd9a6..f93c14e282 100644
--- 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 902f5cbabc..e39e32e854 100644
--- 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 569346d72f..a53d36f7ef 100644
--- 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 ddc7826b17..d91aaae645 100644
--- 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];