Skip to content

Commit

Permalink
Merge pull request #20607 from colemanw/searchDisplayAccessBypass
Browse files Browse the repository at this point in the history
SearchKit - Allow super admins to disable Search Display access checks
  • Loading branch information
seamuslee001 authored Jun 17, 2021
2 parents 13ea910 + 5623bf2 commit 183ca91
Show file tree
Hide file tree
Showing 26 changed files with 395 additions and 75 deletions.
4 changes: 4 additions & 0 deletions CRM/Core/Permission.php
Original file line number Diff line number Diff line change
Expand Up @@ -1488,6 +1488,10 @@ public static function getEntityActionPermissions() {
],
];

$permissions['saved_search'] = [
'default' => ['administer CiviCRM data'],
];

// Profile permissions
$permissions['profile'] = [
// the profile will take care of this
Expand Down
5 changes: 4 additions & 1 deletion Civi/Api4/Query/SqlFunctionCOUNT.php
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,12 @@ class SqlFunctionCOUNT extends SqlFunction {
*
* @see \Civi\Api4\Utils\FormattingUtil::formatOutputValues
* @param string $value
* @param string $dataType
* @return string|array
*/
public function formatOutputValue($value) {
public function formatOutputValue($value, &$dataType) {
// Count is always an integer
$dataType = 'Integer';
return (int) $value;
}

Expand Down
8 changes: 7 additions & 1 deletion Civi/Api4/Query/SqlFunctionGROUP_CONCAT.php
Original file line number Diff line number Diff line change
Expand Up @@ -51,13 +51,19 @@ class SqlFunctionGROUP_CONCAT extends SqlFunction {
*
* @see \Civi\Api4\Utils\FormattingUtil::formatOutputValues
* @param string $value
* @param string $dataType
* @return string|array
*/
public function formatOutputValue($value) {
public function formatOutputValue($value, &$dataType) {
$exprArgs = $this->getArgs();
// By default, values are split into an array and formatted according to the field's dataType
if (!$exprArgs[2]['prefix']) {
$value = explode(\CRM_Core_DAO::VALUE_SEPARATOR, $value);
}
// If using custom separator, unset $dataType to preserve raw string
else {
$dataType = NULL;
}
return $value;
}

Expand Down
6 changes: 6 additions & 0 deletions Civi/Api4/Utils/CoreUtil.php
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
5 changes: 2 additions & 3 deletions Civi/Api4/Utils/FormattingUtil.php
Original file line number Diff line number Diff line change
Expand Up @@ -201,10 +201,9 @@ public static function formatOutputValues(&$results, $fields, $entity, $action =
$fieldName = \CRM_Utils_Array::first($fieldExpr->getFields());
$field = $fieldName && isset($fields[$fieldName]) ? $fields[$fieldName] : NULL;
$dataType = $field['data_type'] ?? ($fieldName == 'id' ? 'Integer' : NULL);
// If Sql Function e.g. GROUP_CONCAT or COUNT wants to do its own formatting, apply and skip dataType conversion
// If Sql Function e.g. GROUP_CONCAT or COUNT wants to do its own formatting, apply
if (method_exists($fieldExpr, 'formatOutputValue') && is_string($value)) {
$result[$key] = $value = $fieldExpr->formatOutputValue($value);
$dataType = NULL;
$result[$key] = $value = $fieldExpr->formatOutputValue($value, $dataType);
}
if (!$field) {
continue;
Expand Down
2 changes: 1 addition & 1 deletion css/civicrm.css
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
43 changes: 43 additions & 0 deletions ext/search_kit/CRM/Search/BAO/SearchDisplay.php
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
}

}
25 changes: 24 additions & 1 deletion ext/search_kit/CRM/Search/DAO/SearchDisplay.php
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
*
* Generated from org.civicrm.search_kit/xml/schema/CRM/Search/SearchDisplay.xml
* DO NOT EDIT. Generated by CRM_Core_CodeGen
* (GenCodeChecksum:6042d1e2bee9eaed4c3a7c59c34ad224)
* (GenCodeChecksum:6d44d0c212e3f352182cac098e4d44ac)
*/
use CRM_Search_ExtensionUtil as E;

Expand Down Expand Up @@ -73,6 +73,13 @@ class CRM_Search_DAO_SearchDisplay extends CRM_Core_DAO {
*/
public $settings;

/**
* Skip permission checks and ACLs when running this display.
*
* @var bool
*/
public $acl_bypass;

/**
* Class constructor.
*/
Expand Down Expand Up @@ -214,6 +221,22 @@ public static function &fields() {
'serialize' => self::SERIALIZE_JSON,
'add' => '1.0',
],
'acl_bypass' => [
'name' => 'acl_bypass',
'type' => CRM_Utils_Type::T_BOOLEAN,
'title' => E::ts('Bypass ACL Permissions'),
'description' => E::ts('Skip permission checks and ACLs when running this display.'),
'where' => 'civicrm_search_display.acl_bypass',
'default' => '0',
'table_name' => 'civicrm_search_display',
'entity' => 'SearchDisplay',
'bao' => 'CRM_Search_DAO_SearchDisplay',
'localizable' => 0,
'html' => [
'type' => 'Checkbox',
],
'add' => '5.40',
],
];
CRM_Core_DAO_AllCoreTables::invoke(__CLASS__, 'fields_callback', Civi::$statics[__CLASS__]['fields']);
}
Expand Down
32 changes: 31 additions & 1 deletion ext/search_kit/CRM/Search/Upgrader.php
Original file line number Diff line number Diff line change
Expand Up @@ -139,9 +139,39 @@ public function upgrade_1003() {
* @return bool
*/
public function upgrade_1004() {
$this->ctx->log->info('Applying update 1000 - fix menu permission.');
$this->ctx->log->info('Applying update 1004 - fix menu permission.');
CRM_Core_DAO::executeQuery("UPDATE civicrm_navigation SET permission = 'administer CiviCRM data' WHERE url = 'civicrm/admin/search'");
return TRUE;
}

/**
* Upgrade 1005 - add acl_bypass column.
* @return bool
*/
public function upgrade_1005() {
$this->ctx->log->info('Applying update 1005 - add acl_bypass column.');
$this->addTask('Add Cancel Button Setting to the Profile', 'addColumn',
'civicrm_search_display', 'acl_bypass', "tinyint DEFAULT 0 COMMENT 'Skip permission checks and ACLs when running this display.'");
return TRUE;
}

/**
* Add a column to a table if it doesn't already exist
*
* FIXME: Move to a shared class, delegate to CRM_Upgrade_Incremental_Base::addColumn
*
* @param string $table
* @param string $column
* @param string $properties
*
* @return bool
*/
public static function addColumn($table, $column, $properties) {
if (!CRM_Core_BAO_SchemaHandler::checkIfFieldExists($table, $column, FALSE)) {
$query = "ALTER TABLE `$table` ADD COLUMN `$column` $properties";
CRM_Core_DAO::executeQuery($query, [], TRUE, NULL, FALSE, FALSE);
}
return TRUE;
}

}
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
Expand Up @@ -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)) {
Expand All @@ -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;
Expand Down
1 change: 1 addition & 0 deletions ext/search_kit/Civi/Api4/SearchDisplay.php
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ public static function getSearchTasks($checkPermissions = TRUE) {

public static function permissions() {
$permissions = parent::permissions();
$permissions['default'] = ['administer CiviCRM data'];
$permissions['run'] = [];
return $permissions;
}
Expand Down
1 change: 1 addition & 0 deletions ext/search_kit/ang/crmSearchAdmin.ang.php
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Up @@ -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']],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 = {
Expand Down
13 changes: 13 additions & 0 deletions ext/search_kit/ang/crmSearchAdmin/crmSearchAdminDisplay.html
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Up @@ -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;
});

Expand Down
6 changes: 3 additions & 3 deletions ext/search_kit/ang/crmSearchAdmin/searchList.html
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down Expand Up @@ -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>
Expand Down
9 changes: 9 additions & 0 deletions ext/search_kit/css/crmSearchAdmin.css
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
30 changes: 3 additions & 27 deletions ext/search_kit/search_kit.civix.php
Original file line number Diff line number Diff line change
Expand Up @@ -221,40 +221,16 @@ function _search_kit_civix_upgrader() {
* Search directory tree for files which match a glob pattern.
*
* Note: Dot-directories (like "..", ".git", or ".svn") will be ignored.
* Note: In Civi 4.3+, delegate to CRM_Utils_File::findFiles()
* Note: Delegate to CRM_Utils_File::findFiles(), this function kept only
* for backward compatibility of extension code that uses it.
*
* @param string $dir base dir
* @param string $pattern , glob pattern, eg "*.txt"
*
* @return array
*/
function _search_kit_civix_find_files($dir, $pattern) {
if (is_callable(['CRM_Utils_File', 'findFiles'])) {
return CRM_Utils_File::findFiles($dir, $pattern);
}

$todos = [$dir];
$result = [];
while (!empty($todos)) {
$subdir = array_shift($todos);
foreach (_search_kit_civix_glob("$subdir/$pattern") as $match) {
if (!is_dir($match)) {
$result[] = $match;
}
}
if ($dh = opendir($subdir)) {
while (FALSE !== ($entry = readdir($dh))) {
$path = $subdir . DIRECTORY_SEPARATOR . $entry;
if ($entry[0] == '.') {
}
elseif (is_dir($path)) {
$todos[] = $path;
}
}
closedir($dh);
}
}
return $result;
return CRM_Utils_File::findFiles($dir, $pattern);
}

/**
Expand Down
Loading

0 comments on commit 183ca91

Please sign in to comment.