From 2f99b5818324c0cf660d8dbd6aa9b47f61238ec9 Mon Sep 17 00:00:00 2001 From: colemanw Date: Sun, 16 Jul 2023 12:27:06 -0400 Subject: [PATCH] SearchKit - Improve performance of checking link permissions --- CRM/Utils/Hook.php | 2 +- Civi/Api4/Utils/CoreUtil.php | 10 +-- .../SearchDisplay/AbstractRunAction.php | 66 +++++++++++++++---- .../api/v4/SearchDisplay/SearchRunTest.php | 34 ++++++++++ 4 files changed, 94 insertions(+), 18 deletions(-) diff --git a/CRM/Utils/Hook.php b/CRM/Utils/Hook.php index 5e6005b77b..9ce4f5b980 100644 --- a/CRM/Utils/Hook.php +++ b/CRM/Utils/Hook.php @@ -569,7 +569,7 @@ abstract class CRM_Utils_Hook { * visibility of contacts to the logged in user * * @param int $type - * The type of permission needed. + * Action being taken (CRM_Core_Permission::VIEW or CRM_Core_Permission::EDIT) * @param array $tables * (reference ) add the tables that are needed for the select clause. * @param array $whereTables diff --git a/Civi/Api4/Utils/CoreUtil.php b/Civi/Api4/Utils/CoreUtil.php index 37f1ef6125..865aa7be14 100644 --- a/Civi/Api4/Utils/CoreUtil.php +++ b/Civi/Api4/Utils/CoreUtil.php @@ -15,6 +15,7 @@ namespace Civi\Api4\Utils; use Civi\API\Exception\NotImplementedException; use Civi\API\Exception\UnauthorizedException; use Civi\API\Request; +use Civi\Api4\Generic\AbstractAction; use CRM_Core_DAO_AllCoreTables as AllCoreTables; class CoreUtil { @@ -172,14 +173,13 @@ class CoreUtil { * * @param \Civi\Api4\Generic\AbstractAction $apiRequest * @param array $record - * @param int|string $userID - * Contact ID of the user we are testing,. 0 for the anonymous user. + * @param int|null $userID + * Contact ID of the user we are testing, 0 for the anonymous user. * @return bool * @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(AbstractAction $apiRequest, array $record, int $userID = NULL) { + $userID = $userID ?? \CRM_Core_Session::getLoggedInContactID() ?? 0; // Super-admins always have access to everything if (\CRM_Core_Permission::check('all CiviCRM permissions and ACLs', $userID)) { diff --git a/ext/search_kit/Civi/Api4/Action/SearchDisplay/AbstractRunAction.php b/ext/search_kit/Civi/Api4/Action/SearchDisplay/AbstractRunAction.php index ea4475628e..6c1269f2e1 100644 --- a/ext/search_kit/Civi/Api4/Action/SearchDisplay/AbstractRunAction.php +++ b/ext/search_kit/Civi/Api4/Action/SearchDisplay/AbstractRunAction.php @@ -3,6 +3,7 @@ namespace Civi\Api4\Action\SearchDisplay; use Civi\API\Exception\UnauthorizedException; +use Civi\API\Request; use Civi\Api4\Query\SqlField; use Civi\Api4\SearchDisplay; use Civi\Api4\Utils\CoreUtil; @@ -81,6 +82,11 @@ abstract class AbstractRunAction extends \Civi\Api4\Generic\AbstractAction { */ private $tasks; + /** + * @var array + */ + private $entityActions; + /** * Override execute method to change the result object type * @return \Civi\Api4\Result\SearchDisplayRunResult @@ -469,9 +475,10 @@ abstract class AbstractRunAction extends \Civi\Api4\Generic\AbstractAction { } /** - * Check access for edit/update/delete links + * Check if a link should be visible to the user based on their permissions * - * (presumably if a record is shown in SearchKit the user already has view access, and the check is expensive) + * Checks ACLs for all links other than VIEW (presumably if a record is shown in + * SearchKit then the user already has view access, and the check is expensive). * * @param array $link * @param array $data @@ -484,7 +491,11 @@ abstract class AbstractRunAction extends \Civi\Api4\Generic\AbstractAction { if (empty($link['path']) && empty($link['task'])) { return FALSE; } - if ($link['entity'] && !empty($link['action']) && !in_array($link['action'], ['view', 'preview'], TRUE)) { + if ($link['entity'] && !empty($link['action']) && !in_array($link['action'], ['view', 'preview'], TRUE) && $this->getCheckPermissions()) { + $actionName = $this->getPermittedLinkAction($link['entity'], $link['action']); + if (!$actionName) { + return FALSE; + } $idField = CoreUtil::getIdFieldName($link['entity']); $idKey = $this->getIdKeyName($link['entity']); $id = $data[$link['prefix'] . $idKey] ?? NULL; @@ -495,20 +506,51 @@ abstract class AbstractRunAction extends \Civi\Api4\Generic\AbstractAction { if (!is_array($data[$link['prefix'] . $idKey])) { $values += \CRM_Utils_Array::filterByPrefix($data, $link['prefix']); } - $isApiAction = civicrm_api4($link['entity'], 'getActions', [ - 'checkPermissions' => FALSE, - 'where' => [['name', '=', $link['action']]], - ])->count(); - return civicrm_api4($link['entity'], 'checkAccess', [ - // Fudge links with funny action names to check 'update' - 'action' => $isApiAction ? $link['action'] : 'update', - 'values' => $values, - ], 0)['access']; + // These 2 lines are the heart of the `checkAccess` api action. + // Calling this directly is more performant than going through the api wrapper + $apiRequest = Request::create($link['entity'], $actionName, ['version' => 4]); + return CoreUtil::checkAccessRecord($apiRequest, $values); } } return TRUE; } + /** + * Given entity/action name, return the api action name if the user is allowed to run it. + * + * This function serves 2 purposes: + * 1. Efficiently check api gatekeeper permissions (reuses a single getActions api call for every link). + * 2. Transform funny action names (some links have non-api-standard actions like "detach" or "copy"). + * + * @param string $entityName + * @param string $actionName + * @return string|null + */ + private function getPermittedLinkAction(string $entityName, string $actionName): ?string { + // Load api actions and cache for performance (this function may be called hundreds of times per request) + if (!isset($this->entityActions[$entityName])) { + $this->entityActions[$entityName] = [ + 'all' => civicrm_api4($entityName, 'getActions', ['checkPermissions' => FALSE])->column('name'), + 'allowed' => civicrm_api4($entityName, 'getActions', ['checkPermissions' => TRUE])->column('name'), + ]; + } + // Action exists and is permitted + if (in_array($actionName, $this->entityActions[$entityName]['allowed'], TRUE)) { + return $actionName; + } + // Action exists but is not permitted + elseif (in_array($actionName, $this->entityActions[$entityName]['all'], TRUE)) { + return NULL; + } + // Api action does not exist, so it's a link with a weird action name like "detach". + // Fall-back on "update" + elseif (in_array('update', $this->entityActions[$entityName]['allowed'], TRUE)) { + return 'update'; + } + // Api action does not exist and user does not have permission to "update". + return NULL; + } + /** * Check if a link should be shown based on its conditions. * diff --git a/ext/search_kit/tests/phpunit/api/v4/SearchDisplay/SearchRunTest.php b/ext/search_kit/tests/phpunit/api/v4/SearchDisplay/SearchRunTest.php index 45aa3883f9..2790bf143d 100644 --- a/ext/search_kit/tests/phpunit/api/v4/SearchDisplay/SearchRunTest.php +++ b/ext/search_kit/tests/phpunit/api/v4/SearchDisplay/SearchRunTest.php @@ -667,6 +667,10 @@ class SearchRunTest extends Api4TestBase implements TransactionalInterface { 'label' => 'First Name', 'dataType' => 'String', 'type' => 'field', + 'link' => [ + 'entity' => 'Contact', + 'action' => 'update', + ], ], [ 'key' => 'last_name', @@ -706,6 +710,16 @@ class SearchRunTest extends Api4TestBase implements TransactionalInterface { $this->assertCount(2, $result); $this->assertEquals($sampleData['Three'], $result[0]['data']['id']); $this->assertEquals($sampleData['Four'], $result[1]['data']['id']); + + // Ensure edit link is only shown for contacts we have permission to edit + $hooks->setHook('civicrm_aclWhereClause', [$this, 'aclViewAllEditOne']); + $this->cleanupCachedPermissions(); + $result = civicrm_api4('SearchDisplay', 'run', $params); + $this->assertCount(4, $result); + $this->assertNotEmpty($result[1]['columns'][1]['links']); + $this->assertTrue(empty($result[1]['columns'][0]['links'])); + $this->assertTrue(empty($result[1]['columns'][2]['links'])); + $this->assertTrue(empty($result[1]['columns'][3]['links'])); } public function testWithACLBypass() { @@ -1729,4 +1743,24 @@ class SearchRunTest extends Api4TestBase implements TransactionalInterface { $this->assertEquals('fa-star', $result[2]['columns'][0]['icons'][0]['class']); } + /** + * Returns all contacts in VIEW mode but only specified contact for EDIT. + * + * @implements CRM_Utils_Hook::aclWhereClause + * + * @param int $type + * @param array $tables + * @param array $whereTables + * @param int $contactID + * @param string|null $where + */ + public function aclViewAllEditOne(int $type, array &$tables, array &$whereTables, int &$contactID, ?string &$where): void { + if ($type === \CRM_Core_Permission::VIEW) { + $where = ' (1) '; + } + elseif ($type === \CRM_Core_Permission::EDIT) { + $where = ' contact_a.id = ' . $this->allowedContactId; + } + } + } -- 2.25.1