From 06124e3e18f34e251da9676f1e20ab5ddebba5dc Mon Sep 17 00:00:00 2001 From: colemanw Date: Sun, 27 Aug 2023 22:09:19 -0400 Subject: [PATCH] APIv4 - Proper ACLs for relationship entity Before: APIv3 and v4 require 'edit all contacts' to create/update/delete relationships After: APIv3 unchanged, but v4 uses ACLs instead so the permission is no longer needed. --- CRM/Contact/BAO/Relationship.php | 21 ++++++++ Civi/Api4/Relationship.php | 12 +++++ Civi/Test/ACLPermissionTrait.php | 15 ++++++ .../phpunit/CRM/Activity/BAO/ActivityTest.php | 4 +- tests/phpunit/CRM/Group/Page/AjaxTest.php | 5 -- tests/phpunit/CiviTest/CiviUnitTestCase.php | 15 ------ .../api/v4/Entity/RelationshipTest.php | 48 +++++++++++++++++++ 7 files changed, 98 insertions(+), 22 deletions(-) diff --git a/CRM/Contact/BAO/Relationship.php b/CRM/Contact/BAO/Relationship.php index 03b91fbf66..6405520474 100644 --- a/CRM/Contact/BAO/Relationship.php +++ b/CRM/Contact/BAO/Relationship.php @@ -2274,4 +2274,25 @@ SELECT count(*) } } + /** + * @param string $entityName + * @param string $action + * @param array $record + * @param int $userID + * @return bool + * @see CRM_Core_DAO::checkAccess + */ + public static function _checkAccess(string $entityName, string $action, array $record, int $userID): bool { + // Delegate relationship permissions to contacts a & b + foreach (['a', 'b'] as $ab) { + if (empty($record["contact_id_$ab"]) && !empty($record['id'])) { + $record["contact_id_$ab"] = CRM_Core_DAO::getFieldValue(__CLASS__, $record['id'], "contact_id_$ab"); + } + if (!\Civi\Api4\Utils\CoreUtil::checkAccessDelegated('Contact', 'update', ['id' => $record["contact_id_$ab"]], $userID)) { + return FALSE; + } + } + return TRUE; + } + } diff --git a/Civi/Api4/Relationship.php b/Civi/Api4/Relationship.php index 2532d3a65c..0be9f893ef 100644 --- a/Civi/Api4/Relationship.php +++ b/Civi/Api4/Relationship.php @@ -30,4 +30,16 @@ class Relationship extends Generic\DAOEntity { ->setCheckPermissions($checkPermissions); } + /** + * @return array + */ + public static function permissions(): array { + return [ + 'meta' => ['access CiviCRM'], + // get managed by CRM_Core_BAO::addSelectWhereClause + // create/update/delete managed by CRM_Contact_BAO_Relationship::_checkAccess + 'default' => [], + ]; + } + } diff --git a/Civi/Test/ACLPermissionTrait.php b/Civi/Test/ACLPermissionTrait.php index 415a8f6aae..23368a02a9 100644 --- a/Civi/Test/ACLPermissionTrait.php +++ b/Civi/Test/ACLPermissionTrait.php @@ -120,6 +120,21 @@ trait ACLPermissionTrait { $where = ' contact_a.id > ' . $this->allowedContactId; } + /** + * Only specified contact returned. + * + * @implements CRM_Utils_Hook::aclWhereClause + * + * @param $type + * @param $tables + * @param $whereTables + * @param $contactID + * @param $where + */ + public function aclWhereMultipleContacts($type, &$tables, &$whereTables, &$contactID, &$where) { + $where = " contact_a.id IN (" . implode(', ', $this->allowedContacts) . ")"; + } + /** * Set up a core ACL. * diff --git a/tests/phpunit/CRM/Activity/BAO/ActivityTest.php b/tests/phpunit/CRM/Activity/BAO/ActivityTest.php index 50392eb0ba..478f36112a 100644 --- a/tests/phpunit/CRM/Activity/BAO/ActivityTest.php +++ b/tests/phpunit/CRM/Activity/BAO/ActivityTest.php @@ -9,14 +9,14 @@ use Civi\Api4\OptionValue; */ class CRM_Activity_BAO_ActivityTest extends CiviUnitTestCase { + use Civi\Test\ACLPermissionTrait; + private $allowedContactsACL = []; private $loggedInUserId = NULL; private $someContacts = []; - protected $allowedContacts = []; - /** * Set up for test. * diff --git a/tests/phpunit/CRM/Group/Page/AjaxTest.php b/tests/phpunit/CRM/Group/Page/AjaxTest.php index 8ea08a0990..6b2e8225fd 100644 --- a/tests/phpunit/CRM/Group/Page/AjaxTest.php +++ b/tests/phpunit/CRM/Group/Page/AjaxTest.php @@ -18,11 +18,6 @@ class CRM_Group_Page_AjaxTest extends CiviUnitTestCase { */ protected $_permissionedDisabledGroup; - /** - * @var CRM_Utils_Hook_UnitTests - */ - public $hookClass; - protected $_params = []; public function setUp(): void { diff --git a/tests/phpunit/CiviTest/CiviUnitTestCase.php b/tests/phpunit/CiviTest/CiviUnitTestCase.php index b63a80c6fe..f2fc560d36 100644 --- a/tests/phpunit/CiviTest/CiviUnitTestCase.php +++ b/tests/phpunit/CiviTest/CiviUnitTestCase.php @@ -2871,21 +2871,6 @@ class CiviUnitTestCase extends PHPUnit\Framework\TestCase { $this->_ids['membership_type'] = $membershipTypeID; } - /** - * Only specified contact returned. - * - * @implements CRM_Utils_Hook::aclWhereClause - * - * @param $type - * @param $tables - * @param $whereTables - * @param $contactID - * @param $where - */ - public function aclWhereMultipleContacts($type, &$tables, &$whereTables, &$contactID, &$where) { - $where = " contact_a.id IN (" . implode(', ', $this->allowedContacts) . ")"; - } - /** * @implements CRM_Utils_Hook::selectWhereClause * diff --git a/tests/phpunit/api/v4/Entity/RelationshipTest.php b/tests/phpunit/api/v4/Entity/RelationshipTest.php index a73f637eb1..84c4f75f02 100644 --- a/tests/phpunit/api/v4/Entity/RelationshipTest.php +++ b/tests/phpunit/api/v4/Entity/RelationshipTest.php @@ -18,6 +18,7 @@ namespace api\v4\Entity; +use Civi\API\Exception\UnauthorizedException; use Civi\Api4\Contact; use api\v4\Api4TestBase; use Civi\Api4\Relationship; @@ -34,6 +35,8 @@ use DateTime; */ class RelationshipTest extends Api4TestBase implements TransactionalInterface { + use \Civi\Test\ACLPermissionTrait; + /** * Test relationship cache tracks created relationships. * @@ -151,4 +154,49 @@ class RelationshipTest extends Api4TestBase implements TransactionalInterface { $this->assertEquals($future->format('Y-m-d'), $cacheRecord['end_date']); } + public function testRelationshipCheckAccess(): void { + $cid = $this->saveTestRecords('Contact', ['records' => 4])->column('id'); + $this->allowedContacts = array_slice($cid, 1); + \CRM_Core_Config::singleton()->userPermissionClass->permissions = ['access CiviCRM']; + \CRM_Utils_Hook::singleton()->setHook('civicrm_aclWhereClause', [$this, 'aclWhereMultipleContacts']); + $check = Relationship::checkAccess() + ->setAction('create') + ->setValues([ + 'contact_id_a' => $cid[0], + 'contact_id_b' => $cid[1], + 'relationship_type_id' => 1, + ]) + ->execute()->first(); + $this->assertFalse($check['access']); + + try { + Relationship::create()->setValues([ + 'contact_id_a' => $cid[1], + 'contact_id_b' => $cid[0], + 'relationship_type_id' => 1, + ])->execute(); + $this->fail(); + } + catch (UnauthorizedException $e) { + Relationship::create(FALSE)->setValues([ + 'contact_id_a' => $cid[1], + 'contact_id_b' => $cid[0], + 'relationship_type_id' => 1, + ])->execute(); + } + Relationship::create()->setValues([ + 'contact_id_a' => $cid[1], + 'contact_id_b' => $cid[2], + 'relationship_type_id' => 1, + ])->execute(); + + $this->assertCount(2, Relationship::get(FALSE)->addWhere('contact_id_a', '=', $cid[1])->execute()); + $this->assertCount(1, Relationship::get()->addWhere('contact_id_a', '=', $cid[1])->execute()); + + Relationship::delete() + ->addWhere('contact_id_a', '=', $cid[1]) + ->execute()->single(); + + } + } -- 2.25.1