APIv4 - Silently ignore non-permissioned fields instead of throwing exceptions

Previously the api would throw an exception when the user did not have permission to use a field
in the WHERE or HAVING clause.

diff --git a/Civi/Api4/Generic/AbstractAction.php b/Civi/Api4/Generic/AbstractAction.php
index d72304cd9b874607f738286b426d8deb8ab6c819..a82852bf2a53c0df00f02896afc70b5804e8eedf 100644 (file)
--- a/Civi/Api4/Generic/AbstractAction.php
+++ b/Civi/Api4/Generic/AbstractAction.php
@@ -417,9 +417,7 @@ abstract class AbstractAction implements \ArrayAccess {
    * Returns schema fields for this entity & action.
    *
    * Here we bypass the api wrapper and run the getFields action directly.
-   * This is because we DON'T want the wrapper to check permissions as this is an internal op,
-   * but we DO want permissions to be checked inside the getFields request so e.g. the api_key
-   * field can be conditionally included.
+   * This is because we DON'T want the wrapper to check permissions as this is an internal op.
    * @see \Civi\Api4\Action\Contact\GetFields
    *
    * @throws \API_Exception
@@ -433,7 +431,7 @@ abstract class AbstractAction implements \ArrayAccess {
       }
       $getFields = \Civi\API\Request::create($this->getEntityName(), 'getFields', [
         'version' => 4,
-        'checkPermissions' => $this->checkPermissions,
+        'checkPermissions' => FALSE,
         'action' => $this->getActionName(),
         'where' => [['type', 'IN', $allowedTypes]],
       ]);

diff --git a/Civi/Api4/Query/Api4SelectQuery.php b/Civi/Api4/Query/Api4SelectQuery.php
index 1c174b3954858595674b02f816e5e65db5d666b3..b84a71779726ccac03c51920963687b4ba6d2653 100644 (file)
--- a/Civi/Api4/Query/Api4SelectQuery.php
+++ b/Civi/Api4/Query/Api4SelectQuery.php
@@ -11,6 +11,7 @@
 
 namespace Civi\Api4\Query;
 
+use Civi\API\Exception\UnauthorizedException;
 use Civi\Api4\Service\Schema\Joinable\CustomGroupJoinable;
 use Civi\Api4\Utils\FormattingUtil;
 use Civi\Api4\Utils\CoreUtil;
@@ -247,7 +248,6 @@ class Api4SelectQuery {
         // Remove expressions with unknown fields without raising an error
         if (!$field || !in_array($field['type'], ['Field', 'Custom'], TRUE)) {
           $select = array_diff($select, [$item]);
-          $this->debug('undefined_fields', $fieldName);
           $valid = FALSE;
         }
       }
@@ -295,7 +295,10 @@ class Api4SelectQuery {
    */
   protected function buildHavingClause() {
     foreach ($this->getHaving() as $clause) {
-      $this->query->having($this->treeWalkClauses($clause, 'HAVING'));
+      $sql = $this->treeWalkClauses($clause, 'HAVING');
+      if ($sql) {
+        $this->query->having($sql);
+      }
     }
   }
 
@@ -325,6 +328,11 @@ class Api4SelectQuery {
       }
       // If the expression could not be rendered, it might be a field alias
       catch (\API_Exception $e) {
+        // Silently ignore fields the user lacks permission to see
+        if (is_a($e, 'Civi\API\Exception\UnauthorizedException')) {
+          $this->debug('unauthorized_fields', $item);
+          continue;
+        }
         if (!empty($this->selectAliases[$item])) {
           $column = '`' . $item . '`';
         }
@@ -399,7 +407,13 @@ class Api4SelectQuery {
         return 'NOT (' . $this->treeWalkClauses($clause[1], $type, $depth + 1) . ')';
 
       default:
-        return $this->composeClause($clause, $type, $depth);
+        try {
+          return $this->composeClause($clause, $type, $depth);
+        }
+        // Silently ignore fields the user lacks permission to see
+        catch (UnauthorizedException $e) {
+          return '';
+        }
     }
   }
 
@@ -456,7 +470,12 @@ class Api4SelectQuery {
         }
       }
       if (!isset($fieldAlias)) {
-        throw new \API_Exception("Invalid expression in HAVING clause: '$expr'. Must use a value from SELECT clause.");
+        if (in_array($expr, $this->getSelect())) {
+          throw new UnauthorizedException("Unauthorized field '$expr'");
+        }
+        else {
+          throw new \API_Exception("Invalid expression in HAVING clause: '$expr'. Must use a value from SELECT clause.");
+        }
       }
       $fieldAlias = '`' . $fieldAlias . '`';
     }
@@ -607,9 +626,15 @@ class Api4SelectQuery {
       $this->autoJoinFK($fieldName);
     }
     $field = $this->apiFieldSpec[$fieldName] ?? NULL;
-    if ($strict && !$field) {
+    if (!$field) {
+      $this->debug($field === FALSE ? 'unauthorized_fields' : 'undefined_fields', $fieldName);
+    }
+    if ($strict && $field === NULL) {
       throw new \API_Exception("Invalid field '$fieldName'");
     }
+    if ($strict && $field === FALSE) {
+      throw new UnauthorizedException("Unauthorized field '$fieldName'");
+    }
     if ($field) {
       $this->apiFieldSpec[$expr] = $field;
     }

diff --git a/Civi/Api4/Query/SqlField.php b/Civi/Api4/Query/SqlField.php
index b6f69f4d5fb0744df03265abd522fa3b9c1796b5..6bd8c3c2c9883d3a8c9311c224c342086791eeb8 100644 (file)
--- a/Civi/Api4/Query/SqlField.php
+++ b/Civi/Api4/Query/SqlField.php
@@ -11,6 +11,8 @@
 
 namespace Civi\Api4\Query;
 
+use Civi\API\Exception\UnauthorizedException;
+
 /**
  * Sql column expression
  */
@@ -26,9 +28,12 @@ class SqlField extends SqlExpression {
   }
 
   public function render(array $fieldList): string {
-    if (empty($fieldList[$this->expr])) {
+    if (!isset($fieldList[$this->expr])) {
       throw new \API_Exception("Invalid field '{$this->expr}'");
     }
+    if ($fieldList[$this->expr] === FALSE) {
+      throw new UnauthorizedException("Unauthorized field '{$this->expr}'");
+    }
     return $fieldList[$this->expr]['sql_name'];
   }
 

diff --git a/tests/phpunit/api/v4/Action/ContactApiKeyTest.php b/tests/phpunit/api/v4/Action/ContactApiKeyTest.php
index afa8fed64a271964bd5e1a56774caf8c220abd62..57d6b0ab851381a8d11622c7208c8216ba58e29b 100644 (file)
--- a/tests/phpunit/api/v4/Action/ContactApiKeyTest.php
+++ b/tests/phpunit/api/v4/Action/ContactApiKeyTest.php
@@ -70,7 +70,7 @@ class ContactApiKeyTest extends \api\v4\UnitTestCase {
       ->addSelect('api_key')
       ->setDebug(TRUE)
       ->execute();
-    $this->assertContains('api_key', $result->debug['undefined_fields']);
+    $this->assertContains('api_key', $result->debug['unauthorized_fields']);
     $this->assertArrayNotHasKey('api_key', $result[0]);
     $this->assertTrue($isSafe($result[0]), "Should NOT reveal secret details ($key): " . var_export($result[0], 1));
 
@@ -80,7 +80,7 @@ class ContactApiKeyTest extends \api\v4\UnitTestCase {
       ->addWhere('id', '=', $contact['email']['id'])
       ->setDebug(TRUE)
       ->execute();
-    $this->assertContains('contact_id.api_key', $email->debug['undefined_fields']);
+    $this->assertContains('contact_id.api_key', $email->debug['unauthorized_fields']);
     $this->assertArrayNotHasKey('contact_id.api_key', $email[0]);
     $this->assertTrue($isSafe($email[0]), "Should NOT reveal secret details ($key): " . var_export($email[0], 1));
 
@@ -92,6 +92,84 @@ class ContactApiKeyTest extends \api\v4\UnitTestCase {
     $this->assertTrue($isSafe($result), "Should NOT reveal secret details ($key): " . var_export($result, 1));
   }
 
+  public function testApiKeyInWhereAndOrderBy() {
+    \CRM_Core_Config::singleton()->userPermissionClass->permissions = ['access CiviCRM', 'add contacts', 'edit api keys', 'view all contacts', 'edit all contacts'];
+    $keyA = 'a' . \CRM_Utils_String::createRandom(15, \CRM_Utils_String::ALPHANUMERIC);
+    $keyB = 'b' . \CRM_Utils_String::createRandom(15, \CRM_Utils_String::ALPHANUMERIC);
+
+    $firstName = uniqid('name');
+
+    $contactA = Contact::create()
+      ->addValue('first_name', $firstName)
+      ->addValue('last_name', 'KeyA')
+      ->addValue('api_key', $keyA)
+      ->execute()
+      ->first();
+
+    $contactB = Contact::create()
+      ->addValue('first_name', $firstName)
+      ->addValue('last_name', 'KeyB')
+      ->addValue('api_key', $keyB)
+      ->execute()
+      ->first();
+
+    // With sufficient permission we can ORDER BY the key
+    $result = Contact::get()
+      ->addSelect('id')
+      ->addWhere('first_name', '=', $firstName)
+      ->addOrderBy('api_key', 'DESC')
+      ->addOrderBy('id', 'ASC')
+      ->execute();
+    $this->assertEquals($contactB['id'], $result[0]['id']);
+
+    // We can also use the key in WHERE clause
+    $result = Contact::get()
+      ->addSelect('id')
+      ->addWhere('api_key', '=', $keyB)
+      ->execute();
+    $this->assertEquals($contactB['id'], $result->single()['id']);
+
+    // We can also use the key in HAVING clause
+    $result = Contact::get()
+      ->addSelect('id', 'api_key')
+      ->addHaving('api_key', '=', $keyA)
+      ->execute();
+    $this->assertEquals($contactA['id'], $result->single()['id']);
+
+    // Remove permission
+    \CRM_Core_Config::singleton()->userPermissionClass->permissions = ['access CiviCRM', 'view debug output', 'view all contacts'];
+
+    // Assert we cannot ORDER BY the key
+    $result = Contact::get()
+      ->addSelect('id')
+      ->addWhere('first_name', '=', $firstName)
+      ->addOrderBy('api_key', 'DESC')
+      ->addOrderBy('id', 'ASC')
+      ->setDebug(TRUE)
+      ->execute();
+    $this->assertEquals($contactA['id'], $result[0]['id']);
+    $this->assertContains('api_key', $result->debug['unauthorized_fields']);
+
+    // Assert we cannot use the key in WHERE clause
+    $result = Contact::get()
+      ->addSelect('id')
+      ->addWhere('api_key', '=', $keyB)
+      ->setDebug(TRUE)
+      ->execute();
+    $this->assertGreaterThan(1, $result->count());
+    $this->assertContains('api_key', $result->debug['unauthorized_fields']);
+
+    // Assert we cannot use the key in HAVING clause
+    $result = Contact::get()
+      ->addSelect('id', 'api_key')
+      ->addHaving('api_key', '=', $keyA)
+      ->setDebug(TRUE)
+      ->execute();
+    $this->assertGreaterThan(1, $result->count());
+    $this->assertContains('api_key', $result->debug['unauthorized_fields']);
+
+  }
+
   public function testCreateWithInsufficientPermissions() {
     \CRM_Core_Config::singleton()->userPermissionClass->permissions = ['access CiviCRM', 'add contacts'];
     $key = uniqid();