From f818ece6f64c2e60b4e829893738941e3bd4288c Mon Sep 17 00:00:00 2001
From: Coleman Watts <coleman@civicrm.org>
Date: Tue, 15 Jun 2021 15:04:06 -0400
Subject: [PATCH] APIv4 - Make LineItem, EntityFinancialTrxn and FinancialTrxn
 read-only

Adds a new ReadOnly trait which annotates write methods as @internal
and sets write permissions to ALWAYS_DENY.

This effectively hides the write actions from the API Explorer,
and restricts use of the write methods to when `checkPermissions = FALSE`.
---
 Civi/Api4/EntityFinancialTrxn.php             |  1 +
 Civi/Api4/FinancialItem.php                   | 15 +---
 Civi/Api4/FinancialTrxn.php                   |  1 +
 Civi/Api4/Generic/Traits/ReadOnly.php         | 78 +++++++++++++++++++
 Civi/Api4/LineItem.php                        |  1 +
 .../phpunit/api/v4/Entity/ConformanceTest.php | 29 ++++---
 6 files changed, 102 insertions(+), 23 deletions(-)
 create mode 100644 Civi/Api4/Generic/Traits/ReadOnly.php

diff --git a/Civi/Api4/EntityFinancialTrxn.php b/Civi/Api4/EntityFinancialTrxn.php
index f63803793a..568f57d482 100644
--- a/Civi/Api4/EntityFinancialTrxn.php
+++ b/Civi/Api4/EntityFinancialTrxn.php
@@ -29,6 +29,7 @@ namespace Civi\Api4;
  */
 class EntityFinancialTrxn extends Generic\DAOEntity {
   use Generic\Traits\EntityBridge;
+  use Generic\Traits\ReadOnly;
 
   /**
    * @return array
diff --git a/Civi/Api4/FinancialItem.php b/Civi/Api4/FinancialItem.php
index 7f1c3966e9..ced033b3c4 100644
--- a/Civi/Api4/FinancialItem.php
+++ b/Civi/Api4/FinancialItem.php
@@ -30,19 +30,6 @@ namespace Civi\Api4;
  * @package Civi\Api4
  */
 class FinancialItem extends Generic\DAOEntity {
-
-  /**
-   * @see \Civi\Api4\Generic\AbstractEntity::permissions()
-   * @return array
-   */
-  public static function permissions() {
-    $permissions = \CRM_Core_Permission::getEntityActionPermissions()['financial_item'] ?? [];
-
-    // Merge permissions for this entity with the defaults
-    return array_merge($permissions, [
-      'create' => [\CRM_Core_Permission::ALWAYS_DENY_PERMISSION],
-      'update' => [\CRM_Core_Permission::ALWAYS_DENY_PERMISSION],
-    ]);
-  }
+  use Generic\Traits\ReadOnly;
 
 }
diff --git a/Civi/Api4/FinancialTrxn.php b/Civi/Api4/FinancialTrxn.php
index f04a37e399..f96f515196 100644
--- a/Civi/Api4/FinancialTrxn.php
+++ b/Civi/Api4/FinancialTrxn.php
@@ -32,5 +32,6 @@ namespace Civi\Api4;
  * @package Civi\Api4
  */
 class FinancialTrxn extends Generic\DAOEntity {
+  use Generic\Traits\ReadOnly;
 
 }
diff --git a/Civi/Api4/Generic/Traits/ReadOnly.php b/Civi/Api4/Generic/Traits/ReadOnly.php
new file mode 100644
index 0000000000..5c9abe2ce9
--- /dev/null
+++ b/Civi/Api4/Generic/Traits/ReadOnly.php
@@ -0,0 +1,78 @@
+<?php
+/*
+ +--------------------------------------------------------------------+
+ | Copyright CiviCRM LLC. All rights reserved.                        |
+ |                                                                    |
+ | This work is published under the GNU AGPLv3 license with some      |
+ | permitted exceptions and without any warranty. For full license    |
+ | and copyright information, see https://civicrm.org/licensing       |
+ +--------------------------------------------------------------------+
+ */
+
+namespace Civi\Api4\Generic\Traits;
+
+/**
+ * Trait for Entities not intended to be publicly writable.
+ */
+trait ReadOnly {
+
+  /**
+   * Not intended to be used outside CiviCRM core code.
+   *
+   * @inheritDoc
+   * @internal
+   */
+  public static function save($checkPermissions = TRUE) {
+    return parent::save($checkPermissions);
+  }
+
+  /**
+   * Not intended to be used outside CiviCRM core code.
+   *
+   * @inheritDoc
+   * @internal
+   */
+  public static function create($checkPermissions = TRUE) {
+    return parent::create($checkPermissions);
+  }
+
+  /**
+   * Not intended to be used outside CiviCRM core code.
+   *
+   * @inheritDoc
+   * @internal
+   */
+  public static function update($checkPermissions = TRUE) {
+    return parent::update($checkPermissions);
+  }
+
+  /**
+   * Not intended to be used outside CiviCRM core code.
+   *
+   * @inheritDoc
+   * @internal
+   */
+  public static function delete($checkPermissions = TRUE) {
+    return parent::delete($checkPermissions);
+  }
+
+  /**
+   * Not intended to be used outside CiviCRM core code.
+   *
+   * @inheritDoc
+   * @internal
+   */
+  public static function replace($checkPermissions = TRUE) {
+    return parent::replace($checkPermissions);
+  }
+
+  /**
+   * @return array
+   */
+  public static function permissions() {
+    $permissions = parent::permissions();
+    $permissions['create'] = $permissions['update'] = $permissions['delete'] = \CRM_Core_Permission::ALWAYS_DENY_PERMISSION;
+    return $permissions;
+  }
+
+}
diff --git a/Civi/Api4/LineItem.php b/Civi/Api4/LineItem.php
index 724484be1e..ca425f9278 100644
--- a/Civi/Api4/LineItem.php
+++ b/Civi/Api4/LineItem.php
@@ -25,5 +25,6 @@ namespace Civi\Api4;
  * @package Civi\Api4
  */
 class LineItem extends Generic\DAOEntity {
+  use Generic\Traits\ReadOnly;
 
 }
diff --git a/tests/phpunit/api/v4/Entity/ConformanceTest.php b/tests/phpunit/api/v4/Entity/ConformanceTest.php
index 893af3b00b..9c1408a400 100644
--- a/tests/phpunit/api/v4/Entity/ConformanceTest.php
+++ b/tests/phpunit/api/v4/Entity/ConformanceTest.php
@@ -31,8 +31,6 @@ use Civi\Test\HookInterface;
  */
 class ConformanceTest extends UnitTestCase implements HookInterface {
 
-  const READ_ONLY_ENTITIES = '/^(FinancialItem)$/';
-
   use \api\v4\Traits\CheckAccessTrait;
   use \api\v4\Traits\TableDropperTrait;
   use \api\v4\Traits\OptionCleanupTrait {
@@ -215,7 +213,7 @@ class ConformanceTest extends UnitTestCase implements HookInterface {
    * @return mixed
    */
   protected function checkCreation($entity, $entityClass) {
-    $isReadOnly = preg_match(static::READ_ONLY_ENTITIES, $entity);
+    $isReadOnly = $this->isReadOnly($entityClass);
 
     $hookLog = [];
     $onValidate = function(ValidateValuesEvent $e) use (&$hookLog) {
@@ -272,7 +270,7 @@ class ConformanceTest extends UnitTestCase implements HookInterface {
     catch (UnauthorizedException $e) {
       // OK, expected exception
     }
-    if (!preg_match(static::READ_ONLY_ENTITIES, $entity)) {
+    if (!$this->isReadOnly($entityClass)) {
       $this->assertEquals(1, $this->checkAccessCounts["{$entity}::create"]);
     }
     $this->resetCheckAccess();
@@ -353,15 +351,14 @@ class ConformanceTest extends UnitTestCase implements HookInterface {
    * @param \Civi\Api4\Generic\AbstractEntity|string $entityClass
    */
   protected function checkDeleteWithNoId($entityClass) {
-    $exceptionThrown = '';
     try {
       $entityClass::delete()
         ->execute();
+      $this->fail("$entityClass should require ID to delete.");
     }
     catch (\API_Exception $e) {
-      $exceptionThrown = $e->getMessage();
+      // OK
     }
-    $this->assertStringContainsString('required', $exceptionThrown);
   }
 
   /**
@@ -391,14 +388,18 @@ class ConformanceTest extends UnitTestCase implements HookInterface {
   protected function checkDeletionAllowed($entityClass, $id, $entity) {
     $this->setCheckAccessGrants(["{$entity}::delete" => TRUE]);
     $this->assertEquals(0, $this->checkAccessCounts["{$entity}::delete"]);
+    $isReadOnly = $this->isReadOnly($entityClass);
 
     $deleteResult = $entityClass::delete()
+      ->setCheckPermissions(!$isReadOnly)
       ->addWhere('id', '=', $id)
       ->execute();
 
     // should get back an array of deleted id
     $this->assertEquals([['id' => $id]], (array) $deleteResult);
-    $this->assertEquals(1, $this->checkAccessCounts["{$entity}::delete"]);
+    if (!$isReadOnly) {
+      $this->assertEquals(1, $this->checkAccessCounts["{$entity}::delete"]);
+    }
     $this->resetCheckAccess();
   }
 
@@ -423,7 +424,9 @@ class ConformanceTest extends UnitTestCase implements HookInterface {
       // OK
     }
 
-    $this->assertEquals(1, $this->checkAccessCounts["{$entity}::delete"]);
+    if (!$this->isReadOnly($entityClass)) {
+      $this->assertEquals(1, $this->checkAccessCounts["{$entity}::delete"]);
+    }
     $this->resetCheckAccess();
   }
 
@@ -459,4 +462,12 @@ class ConformanceTest extends UnitTestCase implements HookInterface {
     return $result;
   }
 
+  /**
+   * @param \Civi\Api4\Generic\AbstractEntity|string $entityClass
+   * @return bool
+   */
+  protected function isReadOnly($entityClass) {
+    return in_array('ReadOnly', $entityClass::getInfo()['type'], TRUE);
+  }
+
 }
-- 
2.25.1