From 305b5f4dd379f9258f2df55183fdea8c522d3835 Mon Sep 17 00:00:00 2001 From: colemanw Date: Sun, 17 Sep 2023 22:58:59 -0400 Subject: [PATCH] Note - Deprecate hook_civicrm_notePrivacy in favor of hook_civicrm_selectWhereClause This enables note privacy to be enforced by SearchKit and the API, and gives an upgrade path for implementations of hook_civicrm_notePrivacy to be refactored out. --- CRM/Core/BAO/Note.php | 16 ++++++- .../Incremental/php/FiveSixtySeven.php | 17 +++++++ CRM/Utils/Hook.php | 12 ++--- Civi/Api4/Action/Note/Get.php | 47 +++++++++++++++++++ Civi/Api4/Generic/AbstractAction.php | 2 +- Civi/Api4/Note.php | 9 ++++ tests/phpunit/api/v4/Entity/NoteTest.php | 36 ++++++++++++++ 7 files changed, 131 insertions(+), 8 deletions(-) create mode 100644 Civi/Api4/Action/Note/Get.php diff --git a/CRM/Core/BAO/Note.php b/CRM/Core/BAO/Note.php index bcb71d37ec..ffd6e3246c 100644 --- a/CRM/Core/BAO/Note.php +++ b/CRM/Core/BAO/Note.php @@ -50,7 +50,7 @@ class CRM_Core_BAO_Note extends CRM_Core_DAO_Note implements \Civi\Core\HookInte * * @return bool * TRUE if the note should be hidden, otherwise FALSE - * + * @deprecated in favor of selectWhereClause */ public static function getNotePrivacyHidden($note) { if (CRM_Core_Permission::check('view all notes')) { @@ -61,6 +61,9 @@ class CRM_Core_BAO_Note extends CRM_Core_DAO_Note implements \Civi\Core\HookInte if (is_object($note) && get_class($note) === 'CRM_Core_DAO_Note') { CRM_Core_DAO::storeValues($note, $noteValues); } + elseif (is_array($note)) { + $noteValues = $note; + } else { $noteDAO = new CRM_Core_DAO_Note(); $noteDAO->id = $note; @@ -533,6 +536,17 @@ WHERE participant.contact_id = %1 AND note.entity_table = 'civicrm_participant' // Nested array will be joined with OR $clauses['entity_table'] = [$relatedClauses]; } + // Enforce note privacy setting + if (!CRM_Core_Permission::check('view all notes')) { + $clauses['privacy'] = [ + [ + '= 0', + // OR + '= 1 AND {contact_id} = ' . (int) CRM_Core_Session::getLoggedInContactID(), + ], + ]; + } + CRM_Utils_Hook::selectWhereClause($this, $clauses); return $clauses; } diff --git a/CRM/Upgrade/Incremental/php/FiveSixtySeven.php b/CRM/Upgrade/Incremental/php/FiveSixtySeven.php index 429809a1a4..2a694782b1 100644 --- a/CRM/Upgrade/Incremental/php/FiveSixtySeven.php +++ b/CRM/Upgrade/Incremental/php/FiveSixtySeven.php @@ -21,6 +21,23 @@ */ class CRM_Upgrade_Incremental_php_FiveSixtySeven extends CRM_Upgrade_Incremental_Base { + public function setPreUpgradeMessage(&$preUpgradeMessage, $rev, $currentVer = NULL) { + parent::setPreUpgradeMessage($preUpgradeMessage, $rev, $currentVer); + if ($rev === '5.67.alpha1') { + $customPrivacy = CRM_Core_DAO::executeQuery(' + SELECT value, label + FROM civicrm_option_value + WHERE is_active = 1 AND value NOT IN ("0", "1") + AND option_group_id = (SELECT id FROM civicrm_option_group WHERE name = "note_privacy")') + ->fetchMap('value', 'label'); + if ($customPrivacy) { + $preUpgradeMessage .= '

' + . ts('This site has custom note privacy options (%1) which may not work correctly after the upgrade, due to the deprecation of hook_civicrm_notePrivacy. If you are using this hook, see developer documentation on updating your code.', [1 => '"' . implode('", "', $customPrivacy) . '"', 2 => 'href="https://docs.civicrm.org/dev/en/latest/hooks/hook_civicrm_notePrivacy/" target="_blank"']) . + '

'; + } + } + } + /** * Upgrade step; adds tasks including 'runSql'. * diff --git a/CRM/Utils/Hook.php b/CRM/Utils/Hook.php index b263f0e6fe..4d5703a546 100644 --- a/CRM/Utils/Hook.php +++ b/CRM/Utils/Hook.php @@ -1548,19 +1548,19 @@ abstract class CRM_Utils_Hook { } /** - * This hook provides a way to override the default privacy behavior for notes. - * + * Deprecated: use hook_civicrm_selectWhereClause instead. + * @deprecated * @param array &$noteValues - * Associative array of values for this note - * - * @return mixed */ public static function notePrivacy(&$noteValues) { $null = NULL; - return self::singleton()->invoke(['noteValues'], $noteValues, + self::singleton()->invoke(['noteValues'], $noteValues, $null, $null, $null, $null, $null, 'civicrm_notePrivacy' ); + if (isset($noteValues['notePrivacy_hidden'])) { + CRM_Core_Error::deprecatedFunctionWarning('hook_civicrm_selectWhereClause', 'hook_civicrm_notePrivacy'); + } } /** diff --git a/Civi/Api4/Action/Note/Get.php b/Civi/Api4/Action/Note/Get.php new file mode 100644 index 0000000000..cf4a3a70d8 --- /dev/null +++ b/Civi/Api4/Action/Note/Get.php @@ -0,0 +1,47 @@ +getSelect() === ['row_count']; + if (!$onlyCount && $this->getCheckPermissions() && !$this->getGroupBy() && $this->getSelect()) { + // Hack in some extra selects for fields needed by the deprecated hook + $this->addSelect('privacy', 'entity_table', 'entity_id', 'contact_id'); + } + parent::getObjects($result); + // Here comes the really bad part... + // Silver lining, this will emit a noisy deprecation notice if the hook gets used + if ($this->getCheckPermissions() && !$onlyCount) { + $allowedRows = []; + foreach ($result as $row) { + if (!\CRM_Core_BAO_Note::getNotePrivacyHidden($row)) { + $allowedRows[] = $row; + } + } + $result->exchangeArray($allowedRows); + } + } + +} diff --git a/Civi/Api4/Generic/AbstractAction.php b/Civi/Api4/Generic/AbstractAction.php index 6e2d48ace8..43563bae3e 100644 --- a/Civi/Api4/Generic/AbstractAction.php +++ b/Civi/Api4/Generic/AbstractAction.php @@ -28,7 +28,7 @@ use Civi\Api4\Utils\ReflectionUtils; * - Require a value for the param if you add the "@required" annotation. * * @method bool getCheckPermissions() - * @method $this setDebug(bool $value) Enable/disable debug output + * @method $this setDebug(bool $debug) Enable/disable debug output * @method bool getDebug() * @method $this setChain(array $chain) * @method array getChain() diff --git a/Civi/Api4/Note.php b/Civi/Api4/Note.php index 7437bafc02..31e3f56b81 100644 --- a/Civi/Api4/Note.php +++ b/Civi/Api4/Note.php @@ -19,4 +19,13 @@ namespace Civi\Api4; */ class Note extends Generic\DAOEntity { + /** + * @param bool $checkPermissions + * @return Action\Note\Get + */ + public static function get($checkPermissions = TRUE) { + return (new Action\Note\Get(__CLASS__, __FUNCTION__)) + ->setCheckPermissions($checkPermissions); + } + } diff --git a/tests/phpunit/api/v4/Entity/NoteTest.php b/tests/phpunit/api/v4/Entity/NoteTest.php index dfdf46502b..aaa1d24527 100644 --- a/tests/phpunit/api/v4/Entity/NoteTest.php +++ b/tests/phpunit/api/v4/Entity/NoteTest.php @@ -21,6 +21,7 @@ namespace api\v4\Entity; use api\v4\Api4TestBase; use Civi\Api4\Contact; use Civi\Api4\Note; +use Civi\Test\ACLPermissionTrait; use Civi\Test\TransactionalInterface; /** @@ -28,6 +29,8 @@ use Civi\Test\TransactionalInterface; */ class NoteTest extends Api4TestBase implements TransactionalInterface { + use ACLPermissionTrait; + public function testDeleteWithChildren(): void { $c1 = $this->createTestRecord('Contact'); @@ -113,4 +116,37 @@ class NoteTest extends Api4TestBase implements TransactionalInterface { $this->assertEquals($userId, $results['Note3']['Contact_Note.contact_id']); } + public function testNotePermissions(): void { + $userId = $this->createLoggedInUser(); + $cid1 = $this->createTestRecord('Contact')['id']; + $cid2 = $this->createTestRecord('Contact')['id']; + $this->allowedContacts = [$cid1]; + \CRM_Core_Config::singleton()->userPermissionClass->permissions = ['access CiviCRM', 'view debug output']; + \CRM_Utils_Hook::singleton()->setHook('civicrm_aclWhereClause', [$this, 'aclWhereMultipleContacts']); + // Create 2 notes for $c1 and 1 for $c2. + $notes = Note::save(FALSE) + ->setRecords([ + ['note' => 'Not private'], + ['note' => 'Private', 'privacy' => 1], + ['note' => 'Private not mine', 'privacy' => 1, 'contact_id' => $cid2], + ['note' => 'C2 note', 'entity_id' => $cid2], + ['note' => 'C2 private note', 'privacy' => 1, 'entity_id' => $cid2], + ]) + ->setDefaults([ + 'entity_id' => $cid1, + 'entity_table' => 'civicrm_contact', + ])->execute()->column('id'); + $visibleNotes = Note::get() + ->addWhere('entity_id', 'IN', [$cid1, $cid2]) + ->addWhere('entity_table', 'IN', ['civicrm_contact']) + ->setDebug(TRUE) + ->execute() + ->indexBy('id'); + $this->assertCount(2, $visibleNotes); + $this->assertEquals('Not private', $visibleNotes[$notes[0]]['note']); + $this->assertEquals('Private', $visibleNotes[$notes[1]]['note']); + // ACL clause should have been inserted only once because entity_table was specified + $this->assertEquals(1, substr_count($visibleNotes->debug['sql'][0], 'civicrm_acl_contact_cache')); + } + } -- 2.25.1