diff --git a/Civi/Api4/Generic/AbstractAction.php b/Civi/Api4/Generic/AbstractAction.php index d72304cd9b87..a82852bf2a53 100644 --- a/Civi/Api4/Generic/AbstractAction.php +++ b/Civi/Api4/Generic/AbstractAction.php @@ -417,9 +417,7 @@ public function getPermissions() { * 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 @@ public function entityFields() { } $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 1c174b395485..b84a71779726 100644 --- 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 @@ protected function buildSelectClause($select = NULL) { // 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 @@ protected function buildWhereClause() { */ 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 @@ protected function buildOrderBy() { } // 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 @@ protected function treeWalkClauses($clause, $type, $depth = 0) { 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 @@ protected function composeClause(array $clause, string $type, int $depth) { } } 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 @@ public function getField($expr, $strict = FALSE) { $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 b6f69f4d5fb0..6bd8c3c2c988 100644 --- 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 @@ protected function initialize() { } 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 afa8fed64a27..57d6b0ab8513 100644 --- a/tests/phpunit/api/v4/Action/ContactApiKeyTest.php +++ b/tests/phpunit/api/v4/Action/ContactApiKeyTest.php @@ -70,7 +70,7 @@ public function testGetApiKey() { ->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 @@ public function testGetApiKey() { ->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 @@ public function testGetApiKey() { $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();