Skip to content

Commit

Permalink
APIv4 - Silently ignore non-permissioned fields instead of throwing e…
Browse files Browse the repository at this point in the history 
…xceptions

Previously the api would throw an exception when the user did not have permission to use a field
in the WHERE or HAVING clause.
  • Loading branch information
@colemanw @seamuslee001
colemanw authored and seamuslee001 committed Jun 30, 2021
1 parent 962e1ff commit 80aabc6
Show file tree
Hide file tree
Showing 4 changed files with 118 additions and 12 deletions.
6 changes: 2 additions & 4 deletions Civi/Api4/Generic/AbstractAction.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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]],
]);
Expand Down
35 changes: 30 additions & 5 deletions Civi/Api4/Query/Api4SelectQuery.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
}
}
Expand Down Expand Up @@ -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);
}
}
}

Expand Down Expand Up @@ -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 . '`';
}
Expand Down Expand Up @@ -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 '';
}
}
}

Expand Down Expand Up @@ -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 . '`';
}
Expand Down Expand Up @@ -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;
}
Expand Down
7 changes: 6 additions & 1 deletion Civi/Api4/Query/SqlField.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@

namespace Civi\Api4\Query;

use Civi\API\Exception\UnauthorizedException;

/**
* Sql column expression
*/
Expand All @@ -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'];
}

Expand Down
82 changes: 80 additions & 2 deletions tests/phpunit/api/v4/Action/ContactApiKeyTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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));

Expand All @@ -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));

Expand All @@ -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();
Expand Down

0 comments on commit 80aabc6

Please sign in to comment.