Skip to content

Commit

Permalink
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
SearchKit - Enable super-admins to disable Search Display access checks
Browse files Browse the repository at this point in the history
This allows users with 'all CiviCRM permissions and ACLs' to configure a search display
to bypass permission checks and display all records to the user.

Once a display is set to bypass ACLs, it can only be edited by a super-admin,
ordinary admin users will not be able to edit the display or the saved search.

Such a display will not automatically appear on its own page; it must be
embedded in an Afform, and the Afform will act as gatekeeper for users
to view the display.
colemanw committed Jun 15, 2021
1 parent 259207d commit 3944657
Showing 14 changed files with 288 additions and 6 deletions.
6 changes: 6 additions & 0 deletions Civi/Api4/Utils/CoreUtil.php
Original file line number Diff line number Diff line change
@@ -166,6 +166,12 @@ private static function isCustomEntity($customGroupName) {
* @throws \Civi\API\Exception\UnauthorizedException
*/
public static function checkAccessRecord(\Civi\Api4\Generic\AbstractAction $apiRequest, array $record, int $userID) {

// Super-admins always have access to everything
if (\CRM_Core_Permission::check('all CiviCRM permissions and ACLs', $userID)) {
return TRUE;
}

// For get actions, just run a get and ACLs will be applied to the query.
// It's a cheap trick and not as efficient as not running the query at all,
// but BAO::checkAccess doesn't consistently check permissions for the "get" action.
2 changes: 1 addition & 1 deletion css/civicrm.css
Original file line number Diff line number Diff line change
@@ -2781,7 +2781,7 @@ tbody.scrollContent tr.alternateRow {
}

.crm-container .disabled,
.crm-container .disabled td,
.crm-container .disabled *,
.crm-container .cancelled,
.crm-container .cancelled td,
.crm-container li.disabled a.ui-tabs-anchor,
43 changes: 43 additions & 0 deletions ext/search_kit/CRM/Search/BAO/SearchDisplay.php
Original file line number Diff line number Diff line change
@@ -14,4 +14,47 @@
*/
class CRM_Search_BAO_SearchDisplay extends CRM_Search_DAO_SearchDisplay {

/**
* Ensure only super-admins are allowed to create or update displays with acl_bypass
*
* @param string $entityName
* @param string $action
* @param array $record
* @param int $userCID
* @return bool
*/
public static function _checkAccess(string $entityName, string $action, array $record, int $userCID) {
// If we hit this function at all, the user is not a super-admin
// But they must be at least a regular administrator
if (!CRM_Core_Permission::check('administer CiviCRM data')) {
return FALSE;
}
if (in_array($action, ['create', 'update'], TRUE)) {
// Do not allow acl_bypass to be set to TRUE
if (!empty($record['acl_bypass'])) {
return FALSE;
}
// Do not allow edits to an existing record with acl_bypass = TRUE
if (!empty($record['id'])) {
return !CRM_Core_DAO::getFieldValue(__CLASS__, $record['id'], 'acl_bypass');
}
}
return TRUE;
}

/**
* Ensure only super-admins may update SavedSearches linked to displays with acl_bypass
*
* @param \Civi\Api4\Event\AuthorizeRecordEvent $e
*/
public static function savedSearchCheckAccessByDisplay(\Civi\Api4\Event\AuthorizeRecordEvent $e) {
if ($e->getActionName() === 'update') {
$id = (int) $e->getRecord()['id'];
$sql = "SELECT COUNT(id) FROM civicrm_search_display WHERE acl_bypass = 1 AND saved_search_id = $id";
if (CRM_Core_DAO::singleValueQuery($sql)) {
$e->setAuthorized(FALSE);
}
}
}

}
11 changes: 9 additions & 2 deletions ext/search_kit/Civi/Api4/Action/SearchDisplay/Run.php
Original file line number Diff line number Diff line change
@@ -75,7 +75,7 @@ class Run extends \Civi\Api4\Generic\AbstractAction {
*/
public function _run(\Civi\Api4\Generic\Result $result) {
// Only administrators can use this in unsecured "preview mode"
if (!(is_string($this->savedSearch) && is_string($this->display)) && $this->checkPermissions && !\CRM_Core_Permission::check('administer CiviCRM')) {
if (!(is_string($this->savedSearch) && is_string($this->display)) && $this->checkPermissions && !\CRM_Core_Permission::check('administer CiviCRM data')) {
throw new UnauthorizedException('Access denied');
}
if (is_string($this->savedSearch)) {
@@ -93,9 +93,16 @@ public function _run(\Civi\Api4\Generic\Result $result) {
if (!$this->savedSearch || !$this->display) {
throw new \API_Exception("Error: SearchDisplay not found.");
}
// Displays with acl_bypass must be embedded on an afform which the user has access to
if (
$this->checkPermissions && !empty($this->display['acl_bypass']) &&
!\CRM_Core_Permission::check('all CiviCRM permissions and ACLs') && !$this->loadAfform()
) {
throw new UnauthorizedException('Access denied');
}
$entityName = $this->savedSearch['api_entity'];
$apiParams =& $this->savedSearch['api_params'];
$apiParams['checkPermissions'] = $this->checkPermissions;
$apiParams['checkPermissions'] = empty($this->display['acl_bypass']);
$apiParams += ['where' => []];
$settings = $this->display['settings'];
$page = NULL;
1 change: 1 addition & 0 deletions ext/search_kit/ang/crmSearchAdmin.ang.php
Original file line number Diff line number Diff line change
@@ -16,4 +16,5 @@
'basePages' => ['civicrm/admin/search'],
'requires' => ['crmUi', 'crmUtil', 'ngRoute', 'ui.sortable', 'ui.bootstrap', 'api4', 'crmSearchTasks', 'crmRouteBinder'],
'settingsFactory' => ['\Civi\Search\Admin', 'getAdminSettings'],
'permissions' => ['all CiviCRM permissions and ACLs'],
];
1 change: 1 addition & 0 deletions ext/search_kit/ang/crmSearchAdmin.module.js
Original file line number Diff line number Diff line change
@@ -30,6 +30,7 @@
'GROUP_CONCAT(display.name ORDER BY display.id) AS display_name',
'GROUP_CONCAT(display.label ORDER BY display.id) AS display_label',
'GROUP_CONCAT(display.type:icon ORDER BY display.id) AS display_icon',
'GROUP_CONCAT(display.acl_bypass ORDER BY display.id) AS display_acl_bypass',
'GROUP_CONCAT(DISTINCT group.title) AS groups'
],
join: [['SearchDisplay AS display'], ['Group AS group']],
Original file line number Diff line number Diff line change
@@ -36,6 +36,9 @@
var ts = $scope.ts = CRM.ts('org.civicrm.search_kit'),
ctrl = this;

this.isSuperAdmin = CRM.checkPerm('all CiviCRM permissions and ACLs');
this.aclBypassHelp = ts('Only users with "all CiviCRM permissions and ACLs" can disable permission checks.');

this.preview = this.stale = false;

this.colTypes = {
13 changes: 13 additions & 0 deletions ext/search_kit/ang/crmSearchAdmin/crmSearchAdminDisplay.html
Original file line number Diff line number Diff line change
@@ -2,5 +2,18 @@
<div class="form-inline">
<label for="crm-search-admin-display-label">{{:: ts('Name') }} <span class="crm-marker">*</span></label>
<input id="crm-search-admin-display-label" type="text" class="form-control" ng-model="$ctrl.display.label" required placeholder="{{:: ts('Untitled') }}"/>
<div class="form-control" ng-class="{disabled: !$ctrl.isSuperAdmin}" title="{{:: $ctrl.aclBypassHelp }}">
<label>
<input type="checkbox" ng-if="$ctrl.isSuperAdmin" ng-model="$ctrl.display.acl_bypass" style="display: none;">
<i class="crm-i fa-lock text-success" ng-if="!$ctrl.display.acl_bypass"></i>
<i class="crm-i fa-unlock text-danger" ng-if="$ctrl.display.acl_bypass"></i>
<span>{{ $ctrl.display.acl_bypass ? ts('Bypass Permissions') : ts('Enforce Permissions') }}</span>
</label>
</div>
<div class="help-block bg-warning" ng-if="$ctrl.display.acl_bypass">
<i class="crm-i fa-unlock"></i>
{{:: ts('Anyone who can view this display will be able to see all results, regardless of their permission level.') }}
</div>
</div>

</fieldset>
1 change: 1 addition & 0 deletions ext/search_kit/ang/crmSearchAdmin/searchList.controller.js
Original file line number Diff line number Diff line change
@@ -13,6 +13,7 @@

_.each(savedSearches, function(search) {
search.entity_title = searchMeta.getEntity(search.api_entity).title_plural;
search.permissionToEdit = CRM.checkPerm('all CiviCRM permissions and ACLs') || !_.includes(search.display_acl_bypass, true);
search.afform_count = 0;
});

6 changes: 3 additions & 3 deletions ext/search_kit/ang/crmSearchAdmin/searchList.html
Original file line number Diff line number Diff line change
@@ -62,8 +62,8 @@ <h1 crm-page-title>{{:: ts('Saved Searches') }}</h1>
{{:: search.display_name.length === 1 ? ts('1 Display') : ts('%1 Displays', {1: search.display_name.length}) }} <span class="caret"></span>
</button>
<ul class="dropdown-menu" ng-if=":: search.display_name.length">
<li ng-repeat="display_name in search.display_name">
<a href="{{:: $ctrl.searchPath + '#/display/' + search.name + '/' + display_name }}" target="_blank">
<li ng-repeat="display_name in search.display_name" ng-class="{disabled: search.display_acl_bypass[$index]}" title="{{:: search.display_acl_bypass[$index] ? ts('Display has permissions disabled') : ts('View display') }}">
<a ng-href="{{:: search.display_acl_bypass[$index] ? '' : $ctrl.searchPath + '#/display/' + search.name + '/' + display_name }}" target="_blank">
<i class="fa {{:: search.display_icon[$index] }}"></i>
{{:: search.display_label[$index] }}
</a>
@@ -107,7 +107,7 @@ <h1 crm-page-title>{{:: ts('Saved Searches') }}</h1>
{{:: search['modified.display_name'] ? ts('%1 by %2', {1: formatDate(search.modified_date), 2: search['modified.display_name']}) : formatDate(search.modified_date) }}
</td>
<td class="text-right">
<a class="btn btn-xs btn-default" href="#/edit/{{:: search.id }}">{{:: ts('Edit') }}</a>
<a class="btn btn-xs btn-default" href="#/edit/{{:: search.id }}" ng-if="search.permissionToEdit">{{:: ts('Edit') }}</a>
<a class="btn btn-xs btn-default" href="#/create/{{:: search.api_entity + '?params=' + $ctrl.encode(search.api_params) }}">{{:: ts('Clone') }}</a>
<a href class="btn btn-xs btn-danger" crm-confirm="{type: 'delete', obj: search}" on-yes="$ctrl.deleteSearch(search)">{{:: ts('Delete') }}</a>
</td>
9 changes: 9 additions & 0 deletions ext/search_kit/css/crmSearchAdmin.css
Original file line number Diff line number Diff line change
@@ -29,10 +29,19 @@
margin-top: 20px;
}

#bootstrap-theme.crm-search .help-block.bg-warning {
padding: 15px;
}

#bootstrap-theme.crm-search .form-control.huge {
width: 275px;
}

#bootstrap-theme.crm-search div.form-control.disabled,
#bootstrap-theme.crm-search div.form-control.disabled * {
cursor: not-allowed;
}

#bootstrap-theme.crm-search input.ng-invalid {
border-color: #8A1F11;
}
11 changes: 11 additions & 0 deletions ext/search_kit/search_kit.php
Original file line number Diff line number Diff line change
@@ -12,6 +12,17 @@ function search_kit_civicrm_config(&$config) {
Civi::dispatcher()->addListener('hook_civicrm_alterAngular', ['\Civi\Search\AfformSearchMetadataInjector', 'preprocess'], 1000);
}

/**
* @param \Symfony\Component\DependencyInjection\ContainerBuilder $container
*/
function search_kit_civicrm_container($container) {
$container->getDefinition('dispatcher')
->addMethodCall('addListener', [
'civi.api4.authorizeRecord::SavedSearch',
['CRM_Search_BAO_SearchDisplay', 'savedSearchCheckAccessByDisplay'],
]);
}

/**
* Implements hook_civicrm_xmlMenu().
*
183 changes: 183 additions & 0 deletions ext/search_kit/tests/phpunit/api/v4/SearchDisplay/SearchRunTest.php
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
<?php
namespace api\v4\SearchDisplay;

use Civi\API\Exception\UnauthorizedException;
use Civi\Api4\Contact;
use Civi\Api4\SavedSearch;
use Civi\Api4\SearchDisplay;
@@ -200,7 +201,189 @@ public function testDisplayACLCheck() {
$this->assertCount(2, $result);
$this->assertEquals($sampleData['Three'], $result[0]['id']);
$this->assertEquals($sampleData['Four'], $result[1]['id']);
}

public function testWithACLBypass() {
$config = \CRM_Core_Config::singleton();
$config->userPermissionClass->permissions = ['all CiviCRM permissions and ACLs'];

$lastName = uniqid(__FUNCTION__);
$searchName = uniqid(__FUNCTION__);
$displayName = uniqid(__FUNCTION__);
$sampleData = [
['first_name' => 'One', 'last_name' => $lastName],
['first_name' => 'Two', 'last_name' => $lastName],
['first_name' => 'Three', 'last_name' => $lastName],
['first_name' => 'Four', 'last_name' => $lastName],
];
Contact::save()->setRecords($sampleData)->execute();

// Super admin may create a display with acl_bypass
$search = SavedSearch::create()
->setValues([
'name' => $searchName,
'title' => 'Test Saved Search',
'api_entity' => 'Contact',
'api_params' => [
'version' => 4,
'select' => ['id', 'first_name', 'last_name'],
'where' => [],
],
])
->addChain('display', SearchDisplay::create()
->setValues([
'saved_search_id' => '$id',
'name' => $displayName,
'type' => 'table',
'label' => '',
'acl_bypass' => TRUE,
'settings' => [
'limit' => 20,
'pager' => TRUE,
'columns' => [
[
'key' => 'id',
'label' => 'Contact ID',
'dataType' => 'Integer',
'type' => 'field',
],
[
'key' => 'first_name',
'label' => 'First Name',
'dataType' => 'String',
'type' => 'field',
],
[
'key' => 'last_name',
'label' => 'Last Name',
'dataType' => 'String',
'type' => 'field',
],
],
'sort' => [
['id', 'ASC'],
],
],
]))
->execute()->first();

// Super admin may update a display with acl_bypass
SearchDisplay::update()->addWhere('name', '=', $displayName)
->addValue('label', 'Test Display')
->execute();

$config->userPermissionClass->permissions = ['administer CiviCRM'];
// Ordinary admin may not edit display because it has acl_bypass
$error = NULL;
try {
SearchDisplay::update()->addWhere('name', '=', $displayName)
->addValue('label', 'Test Display')
->execute();
}
catch (UnauthorizedException $e) {
$error = $e->getMessage();
}
$this->assertContains('failed', $error);

// Ordinary admin may not change the value of acl_bypass
$error = NULL;
try {
SearchDisplay::update()->addWhere('name', '=', $displayName)
->addValue('acl_bypass', FALSE)
->execute();
}
catch (UnauthorizedException $e) {
$error = $e->getMessage();
}
$this->assertContains('failed', $error);

// Ordinary admin may not edit the search because the display has acl_bypass
$error = NULL;
try {
SavedSearch::update()->addWhere('name', '=', $searchName)
->addValue('title', 'Tested Search')
->execute();
}
catch (UnauthorizedException $e) {
$error = $e->getMessage();
}
$this->assertContains('failed', $error);

$params = [
'checkPermissions' => FALSE,
'return' => 'page:1',
'savedSearch' => $searchName,
'display' => $displayName,
'filters' => ['last_name' => $lastName],
'afform' => NULL,
];

$config->userPermissionClass->permissions = ['access CiviCRM'];

$result = civicrm_api4('SearchDisplay', 'run', $params);
$this->assertCount(4, $result);

$config->userPermissionClass->permissions = ['all CiviCRM permissions and ACLs'];
$params['checkPermissions'] = TRUE;

$result = civicrm_api4('SearchDisplay', 'run', $params);
$this->assertCount(4, $result);

$config->userPermissionClass->permissions = ['administer CiviCRM'];
$error = NULL;
try {
civicrm_api4('SearchDisplay', 'run', $params);
}
catch (UnauthorizedException $e) {
$error = $e->getMessage();
}
$this->assertContains('denied', $error);

$config->userPermissionClass->permissions = ['all CiviCRM permissions and ACLs'];

// Super users can update the acl_bypass field
SearchDisplay::update()->addWhere('name', '=', $displayName)
->addValue('acl_bypass', FALSE)
->execute();

$config->userPermissionClass->permissions = ['view all contacts'];
// And ordinary users can now run it
$result = civicrm_api4('SearchDisplay', 'run', $params);
$this->assertCount(4, $result);

// But not edit
$error = NULL;
try {
SearchDisplay::update()->addWhere('name', '=', $displayName)
->addValue('label', 'Tested Display')
->execute();
}
catch (UnauthorizedException $e) {
$error = $e->getMessage();
}
$this->assertContains('failed', $error);

$config->userPermissionClass->permissions = ['administer CiviCRM data'];

// Admins can edit the search and the display
SavedSearch::update()->addWhere('name', '=', $searchName)
->addValue('title', 'Tested Search')
->execute();
SearchDisplay::update()->addWhere('name', '=', $displayName)
->addValue('label', 'Tested Display')
->execute();

// But they can't edit the acl_bypass field
$error = NULL;
try {
SearchDisplay::update()->addWhere('name', '=', $displayName)
->addValue('acl_bypass', TRUE)
->execute();
}
catch (UnauthorizedException $e) {
$error = $e->getMessage();
}
$this->assertContains('failed', $error);
}

}
4 changes: 4 additions & 0 deletions tests/phpunit/api/v4/Traits/CheckAccessTrait.php
Original file line number Diff line number Diff line change
@@ -56,6 +56,10 @@ protected function setCheckAccessGrants($list) {

protected function resetCheckAccess() {
$this->setCheckAccessGrants([]);
// Grant the test user all permissions EXCEPT 'all CiviCRM permissions and ACLs' (which bypass ACL checks)
$allPermissions = \CRM_Core_Permission::basicPermissions(TRUE);
unset($allPermissions['all CiviCRM permissions and ACLs']);
\CRM_Core_Config::singleton()->userPermissionClass->permissions = array_keys($allPermissions);
}

}

0 comments on commit 3944657

Please sign in to comment.