From 1d3cbc3c535f826daed52f9e0f976cf0a815b329 Mon Sep 17 00:00:00 2001 From: Coleman Watts Date: Wed, 5 May 2021 15:30:03 -0400 Subject: [PATCH] Implement _checkAccess for Contact BAO and related entities (email, phone, etc.) Implements the _checkAccess BAO callback for contacts and the related entities listed in _civicrm_api3_check_edit_permissions. Switch APIv4 to stop using _civicrm_api3_check_edit_permissions now that the checks are implemented in the BAO. Also fixes a couple permission check functions to respect $userID variable. --- CRM/Contact/AccessTrait.php | 43 +++++++++++++++++++++ CRM/Contact/BAO/Contact.php | 28 ++++++++++++++ CRM/Contact/BAO/Contact/Permission.php | 43 ++++++++++----------- CRM/Core/BAO/Address.php | 1 + CRM/Core/BAO/Email.php | 1 + CRM/Core/BAO/IM.php | 1 + CRM/Core/BAO/OpenID.php | 1 + CRM/Core/BAO/Phone.php | 1 + CRM/Core/BAO/Website.php | 1 + Civi/Api4/Generic/DAODeleteAction.php | 19 +++++---- Civi/Api4/Generic/Traits/DAOActionTrait.php | 26 ------------- Civi/Api4/Utils/CoreUtil.php | 2 +- tests/phpunit/api/v3/ContactTest.php | 7 +++- 13 files changed, 114 insertions(+), 60 deletions(-) create mode 100644 CRM/Contact/AccessTrait.php diff --git a/CRM/Contact/AccessTrait.php b/CRM/Contact/AccessTrait.php new file mode 100644 index 0000000000..d3a5e39661 --- /dev/null +++ b/CRM/Contact/AccessTrait.php @@ -0,0 +1,43 @@ + $cid], $userID); + } + +} diff --git a/CRM/Contact/BAO/Contact.php b/CRM/Contact/BAO/Contact.php index fb35924582..5275f5a9fd 100644 --- a/CRM/Contact/BAO/Contact.php +++ b/CRM/Contact/BAO/Contact.php @@ -3728,4 +3728,32 @@ LEFT JOIN civicrm_address ON ( civicrm_address.contact_id = civicrm_contact.id ) ]; } + /** + * @param string $action + * @param array $record + * @param $userID + * @return bool + * @see CRM_Core_DAO::checkAccess + */ + public static function _checkAccess(string $action, array $record, $userID): bool { + switch ($action) { + case 'create': + return CRM_Core_Permission::check('add contacts', $userID); + + case 'get': + $actionType = CRM_Core_Permission::VIEW; + break; + + case 'delete': + $actionType = CRM_Core_Permission::DELETE; + break; + + default: + $actionType = CRM_Core_Permission::EDIT; + break; + } + + return CRM_Contact_BAO_Contact_Permission::allow($record['id'], $actionType, $userID); + } + } diff --git a/CRM/Contact/BAO/Contact/Permission.php b/CRM/Contact/BAO/Contact/Permission.php index 1aae3cea9a..5143831b6c 100644 --- a/CRM/Contact/BAO/Contact/Permission.php +++ b/CRM/Contact/BAO/Contact/Permission.php @@ -136,37 +136,39 @@ WHERE contact_id IN ({$contact_id_list}) * @param int $id * Contact id. * @param int|string $type the type of operation (view|edit) + * @param int $userID + * Contact id of user to check (defaults to current logged-in user) * * @return bool * true if the user has permission, false otherwise */ - public static function allow($id, $type = CRM_Core_Permission::VIEW) { - // get logged in user - $contactID = CRM_Core_Session::getLoggedInContactID(); + public static function allow($id, $type = CRM_Core_Permission::VIEW, $userID = NULL) { + // Default to logged in user if not supplied + $userID = $userID ?? CRM_Core_Session::getLoggedInContactID(); // first: check if contact is trying to view own contact - if ($contactID == $id && ($type == CRM_Core_Permission::VIEW && CRM_Core_Permission::check('view my contact') - || $type == CRM_Core_Permission::EDIT && CRM_Core_Permission::check('edit my contact')) + if ($userID == $id && ($type == CRM_Core_Permission::VIEW && CRM_Core_Permission::check('view my contact') + || $type == CRM_Core_Permission::EDIT && CRM_Core_Permission::check('edit my contact', $userID)) ) { return TRUE; } // FIXME: push this somewhere below, to not give this permission so many rights $isDeleted = (bool) CRM_Core_DAO::getFieldValue('CRM_Contact_DAO_Contact', $id, 'is_deleted'); - if (CRM_Core_Permission::check('access deleted contacts') && $isDeleted) { + if (CRM_Core_Permission::check('access deleted contacts', $userID) && $isDeleted) { return TRUE; } // short circuit for admin rights here so we avoid unneeeded queries // some duplication of code, but we skip 3-5 queries - if (CRM_Core_Permission::check('edit all contacts') || - ($type == CRM_ACL_API::VIEW && CRM_Core_Permission::check('view all contacts')) + if (CRM_Core_Permission::check('edit all contacts', $userID) || + ($type == CRM_Core_Permission::VIEW && CRM_Core_Permission::check('view all contacts', $userID)) ) { return TRUE; } // check permission based on relationship, CRM-2963 - if (self::relationshipList([$id], $type)) { + if (self::relationshipList([$id], $type, $userID)) { return TRUE; } @@ -175,7 +177,7 @@ WHERE contact_id IN ({$contact_id_list}) $tables = []; $whereTables = []; - $permission = CRM_ACL_API::whereClause($type, $tables, $whereTables, NULL, FALSE, FALSE, TRUE); + $permission = CRM_ACL_API::whereClause($type, $tables, $whereTables, $userID, FALSE, FALSE, TRUE); $from = CRM_Contact_BAO_Query::fromClause($whereTables); $query = " @@ -185,10 +187,7 @@ WHERE contact_a.id = %1 AND $permission LIMIT 1 "; - if (CRM_Core_DAO::singleValueQuery($query, [1 => [$id, 'Integer']])) { - return TRUE; - } - return FALSE; + return (bool) CRM_Core_DAO::singleValueQuery($query, [1 => [$id, 'Integer']]); } /** @@ -362,18 +361,18 @@ AND $operationClause /** * Filter a list of contact_ids by the ones that the - * currently active user as a permissioned relationship with + * user as a permissioned relationship with * * @param array $contact_ids * List of contact IDs to be filtered - * * @param int $type * access type CRM_Core_Permission::VIEW or CRM_Core_Permission::EDIT + * @param int $userID * * @return array * List of contact IDs that the user has permissions for */ - public static function relationshipList($contact_ids, $type) { + public static function relationshipList($contact_ids, $type, $userID = NULL) { $result_set = []; // no processing empty lists (avoid SQL errors as well) @@ -381,9 +380,9 @@ AND $operationClause return []; } - // get the currently logged in user - $contactID = CRM_Core_Session::getLoggedInContactID(); - if (empty($contactID)) { + // Default to currently logged in user + $userID = $userID ?? CRM_Core_Session::getLoggedInContactID(); + if (empty($userID)) { return []; } @@ -418,7 +417,7 @@ AND $operationClause SELECT civicrm_relationship.{$contact_id_column} AS contact_id FROM civicrm_relationship {$LEFT_JOIN_DELETED} - WHERE civicrm_relationship.{$user_id_column} = {$contactID} + WHERE civicrm_relationship.{$user_id_column} = {$userID} AND civicrm_relationship.{$contact_id_column} IN ({$contact_id_list}) AND civicrm_relationship.is_active = 1 AND civicrm_relationship.is_permission_{$direction['from']}_{$direction['to']} {$is_perm_condition} @@ -444,7 +443,7 @@ SELECT second_degree_relationship.contact_id_{$second_direction['to']} AS contac FROM civicrm_relationship first_degree_relationship LEFT JOIN civicrm_relationship second_degree_relationship ON first_degree_relationship.contact_id_{$first_direction['to']} = second_degree_relationship.contact_id_{$second_direction['from']} {$LEFT_JOIN_DELETED} - WHERE first_degree_relationship.contact_id_{$first_direction['from']} = {$contactID} + WHERE first_degree_relationship.contact_id_{$first_direction['from']} = {$userID} AND second_degree_relationship.contact_id_{$second_direction['to']} IN ({$contact_id_list}) AND first_degree_relationship.is_active = 1 AND first_degree_relationship.is_permission_{$first_direction['from']}_{$first_direction['to']} {$is_perm_condition} diff --git a/CRM/Core/BAO/Address.php b/CRM/Core/BAO/Address.php index 283709aba6..636294b426 100644 --- a/CRM/Core/BAO/Address.php +++ b/CRM/Core/BAO/Address.php @@ -19,6 +19,7 @@ * This is class to handle address related functions. */ class CRM_Core_BAO_Address extends CRM_Core_DAO_Address { + use CRM_Contact_AccessTrait; /** * Takes an associative array and creates a address. diff --git a/CRM/Core/BAO/Email.php b/CRM/Core/BAO/Email.php index f09051a9b0..1e947294a5 100644 --- a/CRM/Core/BAO/Email.php +++ b/CRM/Core/BAO/Email.php @@ -19,6 +19,7 @@ * This class contains functions for email handling. */ class CRM_Core_BAO_Email extends CRM_Core_DAO_Email { + use CRM_Contact_AccessTrait; /** * Create email address. diff --git a/CRM/Core/BAO/IM.php b/CRM/Core/BAO/IM.php index bca4ce1365..12fc6238d4 100644 --- a/CRM/Core/BAO/IM.php +++ b/CRM/Core/BAO/IM.php @@ -19,6 +19,7 @@ * This class contain function for IM handling */ class CRM_Core_BAO_IM extends CRM_Core_DAO_IM { + use CRM_Contact_AccessTrait; /** * Create or update IM record. diff --git a/CRM/Core/BAO/OpenID.php b/CRM/Core/BAO/OpenID.php index f200043573..33a7c269fc 100644 --- a/CRM/Core/BAO/OpenID.php +++ b/CRM/Core/BAO/OpenID.php @@ -19,6 +19,7 @@ * This class contains function for Open Id */ class CRM_Core_BAO_OpenID extends CRM_Core_DAO_OpenID { + use CRM_Contact_AccessTrait; /** * Create or update OpenID record. diff --git a/CRM/Core/BAO/Phone.php b/CRM/Core/BAO/Phone.php index dd73b9f9b7..c5a110e2ee 100644 --- a/CRM/Core/BAO/Phone.php +++ b/CRM/Core/BAO/Phone.php @@ -19,6 +19,7 @@ * Class contains functions for phone. */ class CRM_Core_BAO_Phone extends CRM_Core_DAO_Phone { + use CRM_Contact_AccessTrait; /** * Create phone object - note that the create function calls 'add' but diff --git a/CRM/Core/BAO/Website.php b/CRM/Core/BAO/Website.php index d5f386e8fd..d6b420a6bf 100644 --- a/CRM/Core/BAO/Website.php +++ b/CRM/Core/BAO/Website.php @@ -19,6 +19,7 @@ * This class contain function for Website handling. */ class CRM_Core_BAO_Website extends CRM_Core_DAO_Website { + use CRM_Contact_AccessTrait; /** * Create or update Website record. diff --git a/Civi/Api4/Generic/DAODeleteAction.php b/Civi/Api4/Generic/DAODeleteAction.php index bf335c0f40..de5ed1a07f 100644 --- a/Civi/Api4/Generic/DAODeleteAction.php +++ b/Civi/Api4/Generic/DAODeleteAction.php @@ -40,6 +40,15 @@ class DAODeleteAction extends AbstractBatchAction { } $items = $this->getBatchRecords(); + + if ($this->getCheckPermissions()) { + foreach ($items as $key => $item) { + if (!CoreUtil::checkAccess($this->getEntityName(), $this->getActionName(), $item)) { + throw new UnauthorizedException("ACL check failed"); + } + $items[$key]['check_permissions'] = TRUE; + } + } if ($items) { $result->exchangeArray($this->deleteObjects($items)); } @@ -54,16 +63,6 @@ class DAODeleteAction extends AbstractBatchAction { $ids = []; $baoName = $this->getBaoName(); - if ($this->getCheckPermissions()) { - foreach (array_keys($items) as $key) { - if (!CoreUtil::checkAccess($this->getEntityName(), $this->getActionName(), $items[$key])) { - throw new UnauthorizedException("ACL check failed"); - } - $items[$key]['check_permissions'] = TRUE; - $this->checkContactPermissions($baoName, $items[$key]); - } - } - if ($this->getEntityName() !== 'EntityTag' && method_exists($baoName, 'del')) { foreach ($items as $item) { $args = [$item['id']]; diff --git a/Civi/Api4/Generic/Traits/DAOActionTrait.php b/Civi/Api4/Generic/Traits/DAOActionTrait.php index 88d1a612cd..af61135448 100644 --- a/Civi/Api4/Generic/Traits/DAOActionTrait.php +++ b/Civi/Api4/Generic/Traits/DAOActionTrait.php @@ -136,10 +136,6 @@ trait DAOActionTrait { $item['contact_id'] = $entityId; } - if ($this->getCheckPermissions()) { - $this->checkContactPermissions($baoName, $item); - } - if ($this->getEntityName() === 'Address') { $createResult = $baoName::$method($item, $this->fixAddress); } @@ -259,26 +255,4 @@ trait DAOActionTrait { return isset($info[$fieldName]) ? ['suffix' => $suffix] + $info[$fieldName] : NULL; } - /** - * Check edit/delete permissions for contacts and related entities. - * - * @param string $baoName - * @param array $item - * - * @throws \Civi\API\Exception\UnauthorizedException - */ - protected function checkContactPermissions($baoName, $item) { - if ($baoName === 'CRM_Contact_BAO_Contact' && !empty($item['id'])) { - $permission = $this->getActionName() === 'delete' ? \CRM_Core_Permission::DELETE : \CRM_Core_Permission::EDIT; - if (!\CRM_Contact_BAO_Contact_Permission::allow($item['id'], $permission)) { - throw new \Civi\API\Exception\UnauthorizedException('Permission denied to modify contact record'); - } - } - else { - // Fixme: decouple from v3 - require_once 'api/v3/utils.php'; - _civicrm_api3_check_edit_permissions($baoName, $item); - } - } - } diff --git a/Civi/Api4/Utils/CoreUtil.php b/Civi/Api4/Utils/CoreUtil.php index 65a3d81ed9..e62bcfe007 100644 --- a/Civi/Api4/Utils/CoreUtil.php +++ b/Civi/Api4/Utils/CoreUtil.php @@ -178,7 +178,7 @@ class CoreUtil { $baoName = self::getBAOFromApiName($entityName); // If entity has a BAO, run the BAO::checkAccess function, which will call the hook if ($baoName && strpos($baoName, '_BAO_')) { - $baoName::checkAccess($actionName, $record, NULL, $granted); + $granted = $baoName::checkAccess($actionName, $record, NULL, $granted); } // Otherwise, call the hook directly else { diff --git a/tests/phpunit/api/v3/ContactTest.php b/tests/phpunit/api/v3/ContactTest.php index 4bce7f9333..1f381ca790 100644 --- a/tests/phpunit/api/v3/ContactTest.php +++ b/tests/phpunit/api/v3/ContactTest.php @@ -3211,7 +3211,12 @@ class api_v3_ContactTest extends CiviUnitTestCase { $config->userPermissionClass->permissions = ['access CiviCRM']; $result = $this->callAPIFailure('contact', 'update', $params); - $this->assertEquals('Permission denied to modify contact record', $result['error_message']); + if ($version == 3) { + $this->assertEquals('Permission denied to modify contact record', $result['error_message']); + } + else { + $this->assertEquals('ACL check failed', $result['error_message']); + } $config->userPermissionClass->permissions = [ 'access CiviCRM', -- 2.25.1