From: eileen <emcnaughton@wikimedia.org>
Date: Thu, 4 Jan 2024 05:26:25 +0000 (+1300)
Subject: Add report instance permission check & test
X-Git-Url: https://vcs.fsf.org/?a=commitdiff_plain;h=f965077fe9366a5095682160364921f626545cda;p=civicrm-core.git

Add report instance permission check & test
---

diff --git a/CRM/Report/BAO/ReportInstance.php b/CRM/Report/BAO/ReportInstance.php
index 12717ed113..ddbfe9fd16 100644
--- a/CRM/Report/BAO/ReportInstance.php
+++ b/CRM/Report/BAO/ReportInstance.php
@@ -258,7 +258,7 @@ class CRM_Report_BAO_ReportInstance extends CRM_Report_DAO_ReportInstance implem
    *   Url to redirect the browser to on fail.
    * @param string $successRedirect
    */
-  public static function doFormDelete($instanceId, $bounceTo = 'civicrm/report/list?reset=1', $successRedirect = NULL) {
+  public static function doFormDelete($instanceId, $bounceTo = 'civicrm/report/list?reset=1', $successRedirect = NULL): void {
     if (!CRM_Core_Permission::check('administer Reports')) {
       $statusMessage = ts('You do not have permission to Delete Report.');
       CRM_Core_Error::statusBounce($statusMessage, $bounceTo);
@@ -272,6 +272,34 @@ class CRM_Report_BAO_ReportInstance extends CRM_Report_DAO_ReportInstance implem
     }
   }
 
+  /**
+   * Apply permission field check to ReportInstance.
+   *
+   * Note that we just check all the individual found permissions & then use the
+   * 'OK' ones as a filter. The volume should be low enough for this to be OK
+   * and the table holds exactly one permission for each instance.
+   *
+   * @param string|null $entityName
+   * @param int|null $userId
+   * @param array $conditions
+   *
+   * @inheritDoc
+   */
+  public function addSelectWhereClause(string $entityName = NULL, int $userId = NULL, array $conditions = []): array {
+    $permissions = CRM_Core_DAO::executeQuery('SELECT DISTINCT permission FROM civicrm_report_instance');
+    $validPermissions = [];
+    while ($permissions->fetch()) {
+      $permission = $permissions->permission;
+      if ($permission && CRM_Core_Permission::check($permission)) {
+        $validPermissions[] = $permission;
+      }
+    }
+    if (!$validPermissions) {
+      return ['permission' => ['IS NULL']];
+    }
+    return ['permission' => ['IN ("' . implode('", "', $validPermissions) . '")']];
+  }
+
   /**
    * Get the metadata of actions available for this entity.
    *
@@ -287,7 +315,7 @@ class CRM_Report_BAO_ReportInstance extends CRM_Report_DAO_ReportInstance implem
    *    wrong, but at the php level it worked https://github.com/civicrm/civicrm-core/pull/8529#issuecomment-227639091
    *  - general script-add.
    */
-  public static function getActionMetadata() {
+  public static function getActionMetadata(): array {
     $actions = [];
     if (CRM_Core_Permission::check('save Report Criteria')) {
       $actions['report_instance.save'] = ['title' => ts('Save')];
diff --git a/ext/civi_report/Civi/Api4/ReportInstance.php b/ext/civi_report/Civi/Api4/ReportInstance.php
index aaa1f89774..3226662272 100644
--- a/ext/civi_report/Civi/Api4/ReportInstance.php
+++ b/ext/civi_report/Civi/Api4/ReportInstance.php
@@ -20,4 +20,27 @@ namespace Civi\Api4;
 class ReportInstance extends Generic\DAOEntity {
   use Generic\Traits\ManagedEntity;
 
+  /**
+   * Specify the permissions to access ReportInstance.
+   *
+   * Function exists to set the get permission on report instance get to access CiviCRM.
+   *
+   *  This allows the permission configured on the report to be implemented in
+   *  the selectWhere hook.
+   *
+   *  Note this might be better as TRUE rather than access CiviCRM but the latter
+   *  feels safer given we are deprecating civi-report and should err on the
+   *  side of stricter security.
+   *
+   * @return array[]
+   */
+  public static function permissions(): array {
+    return [
+      'get' => [
+        'access CiviCRM',
+      ],
+      // @todo - set criteria for create & update - save criteria
+    ];
+  }
+
 }
diff --git a/ext/civi_report/tests/phpunit/ReportInstanceTest.php b/ext/civi_report/tests/phpunit/ReportInstanceTest.php
new file mode 100644
index 0000000000..18ea98ed33
--- /dev/null
+++ b/ext/civi_report/tests/phpunit/ReportInstanceTest.php
@@ -0,0 +1,87 @@
+<?php
+
+declare(strict_types = 1);
+
+namespace Civi\ext\civi_report\tests\phpunit;
+
+use Civi\API\Exception\UnauthorizedException;
+use Civi\Api4\ReportInstance;
+use Civi\Test;
+use Civi\Test\CiviEnvBuilder;
+use Civi\Test\HeadlessInterface;
+use Civi\Test\HookInterface;
+use Civi\Test\TransactionalInterface;
+use CRM_Core_Config;
+use PHPUnit\Framework\TestCase;
+
+/**
+ * Test CiviReportInstance functionality.
+ *
+ * Tips:
+ *  - With HookInterface, you may implement CiviCRM hooks directly in the test class.
+ *    Simply create corresponding functions (e.g. "hook_civicrm_post(...)" or similar).
+ *  - With TransactionalInterface, any data changes made by setUp() or test****() functions will
+ *    rollback automatically -- as long as you don't manipulate schema or truncate tables.
+ *    If this test needs to manipulate schema or truncate tables, then either:
+ *       a. Do all that using setupHeadless() and Civi\Test.
+ *       b. Disable TransactionalInterface, and handle all setup/teardown yourself.
+ *
+ * @group headless
+ */
+class ReportInstanceTest extends TestCase implements HeadlessInterface, HookInterface, TransactionalInterface {
+
+  /**
+   * Setup used when HeadlessInterface is implemented.
+   *
+   * Civi\Test has many helpers, like install(), uninstall(), sql(), and sqlFile().
+   *
+   * @link https://github.com/civicrm/org.civicrm.testapalooza/blob/master/civi-test.md
+   *
+   * @return \Civi\Test\CiviEnvBuilder
+   *
+   * @throws \CRM_Extension_Exception_ParseException
+   */
+  public function setUpHeadless(): CiviEnvBuilder {
+    return Test::headless()
+      ->installMe(__DIR__)
+      ->apply();
+  }
+
+  /**
+   * Test that configured permissions are applied when retrieving a report instance.
+   *
+   * @return void
+   *
+   * @throws \CRM_Core_Exception
+   * @throws \Civi\API\Exception\UnauthorizedException
+   */
+  public function testPermissions(): void {
+    $instance = ReportInstance::get(FALSE)
+      ->addWhere('report_id', '=', 'contact/summary')
+      ->execute()->first();
+
+    CRM_Core_Config::singleton()->userPermissionClass->permissions = ['view event info'];
+    try {
+      ReportInstance::get()
+        ->addWhere('id', '=', $instance['id'])
+        ->execute();
+      $this->fail('Expected an exception as permissions do not permit access here');
+    }
+    catch (UnauthorizedException $e) {
+    }
+    CRM_Core_Config::singleton()->userPermissionClass->permissions = ['access CiviCRM'];
+    $permittedRetrieval = ReportInstance::get()
+      ->addWhere('id', '=', $instance['id'])
+      ->execute();
+    $this->assertCount(0, $permittedRetrieval);
+
+    ReportInstance::update(FALSE)->addWhere('id', '=', $instance['id'])
+      ->setValues(['permission' => 'access CiviCRM'])->execute();
+
+    $permittedRetrieval = ReportInstance::get()
+      ->addWhere('id', '=', $instance['id'])
+      ->execute();
+    $this->assertCount(1, $permittedRetrieval);
+  }
+
+}
diff --git a/ext/civi_report/tests/phpunit/ReportTest.php b/ext/civi_report/tests/phpunit/ReportTest.php
index b2d377ca32..44c875421e 100644
--- a/ext/civi_report/tests/phpunit/ReportTest.php
+++ b/ext/civi_report/tests/phpunit/ReportTest.php
@@ -1,5 +1,7 @@
 <?php
 
+declare(strict_types = 1);
+
 use Civi\Test;
 use Civi\Test\CiviEnvBuilder;
 use Civi\Test\HeadlessInterface;