From b0558c215a036e34d99c230c431c55e2becb79b4 Mon Sep 17 00:00:00 2001
From: Patrick Figel <pfigel@greenpeace.org>
Date: Mon, 4 Feb 2019 16:00:03 +0100
Subject: [PATCH] dev/core#690 - Civi\API - Fix entity permission check for
 trusted calls

This changes the permission check in DynamicFKAuthorization to
short-circuit on trusted API calls (check_permissions = 0) so that
no exception is thrown when it's used with entities that are not
allowed delegates.
---
 .../API/Subscriber/DynamicFKAuthorization.php |  8 +++---
 .../Subscriber/DynamicFKAuthorizationTest.php | 26 +++++++++++++++++++
 2 files changed, 30 insertions(+), 4 deletions(-)

diff --git a/Civi/API/Subscriber/DynamicFKAuthorization.php b/Civi/API/Subscriber/DynamicFKAuthorization.php
index a00802e65e..aade5b2968 100644
--- a/Civi/API/Subscriber/DynamicFKAuthorization.php
+++ b/Civi/API/Subscriber/DynamicFKAuthorization.php
@@ -206,6 +206,10 @@ class DynamicFKAuthorization implements EventSubscriberInterface {
    * @throws \Civi\API\Exception\UnauthorizedException
    */
   public function authorizeDelegate($action, $entityTable, $entityId, $apiRequest) {
+    if ($this->isTrusted($apiRequest)) {
+      return;
+    }
+
     $entity = $this->getDelegatedEntityName($entityTable);
     if (!$entity) {
       throw new \API_Exception("Failed to run permission check: Unrecognized target entity table ($entityTable)");
@@ -214,10 +218,6 @@ class DynamicFKAuthorization implements EventSubscriberInterface {
       throw new \Civi\API\Exception\UnauthorizedException("Authorization failed on ($entity): Missing entity_id");
     }
 
-    if ($this->isTrusted($apiRequest)) {
-      return;
-    }
-
     /**
      * @var \Exception $exception
      */
diff --git a/tests/phpunit/Civi/API/Subscriber/DynamicFKAuthorizationTest.php b/tests/phpunit/Civi/API/Subscriber/DynamicFKAuthorizationTest.php
index df0b5becf5..3e0a32f470 100644
--- a/tests/phpunit/Civi/API/Subscriber/DynamicFKAuthorizationTest.php
+++ b/tests/phpunit/Civi/API/Subscriber/DynamicFKAuthorizationTest.php
@@ -11,6 +11,8 @@ class DynamicFKAuthorizationTest extends \CiviUnitTestCase {
 
   const FILE_FORBIDDEN_ID = 11;
 
+  const FILE_UNDELEGATED_ENTITY = 12;
+
   const WIDGET_ID = 20;
 
   const FORBIDDEN_ID = 30;
@@ -214,4 +216,28 @@ class DynamicFKAuthorizationTest extends \CiviUnitTestCase {
     $this->assertRegExp($expectedError, $result['error_message']);
   }
 
+  /**
+   * Test whether trusted API calls bypass the permission check
+   *
+   */
+  public function testNotDelegated() {
+    $entity = 'FakeFile';
+    $action = 'create';
+    $params = [
+      'entity_id' => self::FILE_UNDELEGATED_ENTITY,
+      'entity_table' => 'civicrm_membership',
+      'version' => 3,
+      'debug' => 1,
+      'check_permissions' => 1,
+    ];
+    // run with permission check
+    $result = $this->kernel->run('FakeFile', 'create', $params);
+    $this->assertTrue((bool) $result['is_error'], 'Undelegated entity with check_permissions = 1 should fail');
+    $this->assertRegExp('/Unrecognized target entity table \(civicrm_membership\)/', $result['error_message']);
+    // repeat without permission check
+    $params['check_permissions'] = 0;
+    $result = $this->kernel->run('FakeFile', 'create', $params);
+    $this->assertFalse((bool) $result['is_error'], 'Undelegated entity with check_permissions = 0 should succeed');
+  }
+
 }
-- 
2.25.1