Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rationalise Activity api ACLs for consistency, to respect the hook & improve performance #13664

Merged
merged 1 commit into from
Mar 4, 2019
Merged

Rationalise Activity api ACLs for consistency, to respect the hook & improve performance #13664

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 13 additions & 8 deletions CRM/Activity/BAO/Activity.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1125,15 +1125,20 @@ public static function deprecatedGetActivities($input) {
* @inheritDoc
*/
public function addSelectWhereClause() {
$clauses = parent::addSelectWhereClause();
if (!CRM_Core_Permission::check('view all activities')) {
$permittedActivityTypeIDs = self::getPermittedActivityTypes();
if (empty($permittedActivityTypeIDs)) {
// This just prevents a mysql fail if they have no access - should be extremely edge case.
$permittedActivityTypeIDs = [0];
}
$clauses['activity_type_id'] = ('IN (' . implode(', ', $permittedActivityTypeIDs) . ')');
$clauses = [];
$permittedActivityTypeIDs = self::getPermittedActivityTypes();
if (empty($permittedActivityTypeIDs)) {
// This just prevents a mysql fail if they have no access - should be extremely edge case.
$permittedActivityTypeIDs = [0];
}
$clauses['activity_type_id'] = ('IN (' . implode(', ', $permittedActivityTypeIDs) . ')');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be more efficient if we skip adding this clause if getPermittedActivityTypes returns the full set of types.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's filtered by component


$contactClause = CRM_Utils_SQL::mergeSubquery('Contact');
if ($contactClause) {
$contactClause = implode(' AND contact_id ', $contactClause);
$clauses['id'][] = "IN (SELECT activity_id FROM civicrm_activity_contact WHERE contact_id $contactClause)";
}
CRM_Utils_Hook::selectWhereClause($this, $clauses);
return $clauses;
}

Expand Down
3 changes: 1 addition & 2 deletions CRM/Core/Permission.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,6 @@
*
* @package CRM
* @copyright CiviCRM LLC (c) 2004-2019
* $Id$
*
*/

/**
Expand Down Expand Up @@ -1052,6 +1050,7 @@ public static function getEntityActionPermissions() {
'view all activities',
),
);
$permissions['activity_contact'] = $permissions['activity'];

// Case permissions
$permissions['case'] = array(
Expand Down
116 changes: 67 additions & 49 deletions api/v3/Activity.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -302,16 +302,6 @@ function civicrm_api3_activity_get($params) {
$options = _civicrm_api3_get_options_from_params($params, FALSE, 'Activity', 'get');
$sql = CRM_Utils_SQL_Select::fragment();

if (empty($params['target_contact_id']) && empty($params['source_contact_id'])
&& empty($params['assignee_contact_id']) &&
!empty($params['check_permissions']) && !CRM_Core_Permission::check('view all activities')
&& !CRM_Core_Permission::check('view all contacts')
) {
// Force join on the activity contact table.
// @todo get this & other acl filters to work, remove check further down.
//$params['contact_id'] = array('IS NOT NULL' => TRUE);
}

_civicrm_api3_activity_get_extraFilters($params, $sql);

// Handle is_overdue sort
Expand Down Expand Up @@ -342,15 +332,6 @@ function civicrm_api3_activity_get($params) {
return civicrm_api3_create_success($activities, $params, 'Activity', 'get');
}

if (!empty($params['check_permissions']) && !CRM_Core_Permission::check('view all activities')) {
// @todo get this to work at the query level - see contact_id join above.
foreach ($activities as $activity) {
if (!CRM_Activity_BAO_Activity::checkPermission($activity['id'], CRM_Core_Action::VIEW)) {
unset($activities[$activity['id']]);
}
}
}

$activities = _civicrm_api3_activity_get_formatResult($params, $activities, $options);
//legacy custom data get - so previous formatted response is still returned too
return civicrm_api3_create_success($activities, $params, 'Activity', 'get');
Expand Down Expand Up @@ -452,6 +433,8 @@ function _civicrm_api3_activity_get_extraFilters(&$params, &$sql) {
* @param array $params
* API request parameters.
* @param array $activities
* @param array $options
* Options array (pre-processed to extract 'return' from params).
*
* @return array
* new activities list
Expand All @@ -463,19 +446,14 @@ function _civicrm_api3_activity_get_formatResult($params, $activities, $options)

$returns = $options['return'];
foreach ($params as $n => $v) {
// @todo - the per-parsing on options should have already done this.
if (substr($n, 0, 7) == 'return.') {
$returnkey = substr($n, 7);
$returns[$returnkey] = $v;
}
}

$returns['source_contact_id'] = 1;
if (!empty($returns['target_contact_name'])) {
$returns['target_contact_id'] = 1;
}
if (!empty($returns['assignee_contact_name'])) {
$returns['assignee_contact_id'] = 1;
}
_civicrm_api3_activity_fill_activity_contact_names($activities, $params, $returns);

$tagGet = array('tag_id', 'entity_id');
$caseGet = $caseIds = array();
Expand All @@ -493,34 +471,14 @@ function _civicrm_api3_activity_get_formatResult($params, $activities, $options)
foreach ($returns as $n => $v) {
switch ($n) {
case 'assignee_contact_id':
foreach ($activities as $key => $activityArray) {
$cids = $activities[$key]['assignee_contact_id'] = CRM_Activity_BAO_ActivityAssignment::retrieveAssigneeIdsByActivityId($activityArray['id']);
if ($cids && !empty($returns['assignee_contact_name'])) {
foreach ($cids as $cid) {
$activities[$key]['assignee_contact_name'][$cid] = CRM_Core_DAO::getFieldValue('CRM_Contact_DAO_Contact', $cid, 'display_name');
}
}
}
break;

case 'target_contact_id':
foreach ($activities as $key => $activityArray) {
$cids = $activities[$key]['target_contact_id'] = CRM_Activity_BAO_ActivityTarget::retrieveTargetIdsByActivityId($activityArray['id']);
if ($cids && !empty($returns['target_contact_name'])) {
foreach ($cids as $cid) {
$activities[$key]['target_contact_name'][$cid] = CRM_Core_DAO::getFieldValue('CRM_Contact_DAO_Contact', $cid, 'display_name');
}
foreach ($activities as &$activity) {
if (!isset($activity[$n])) {
$activity[$n] = [];
}
}
break;

case 'source_contact_id':
foreach ($activities as $key => $activityArray) {
$cid = $activities[$key]['source_contact_id'] = CRM_Activity_BAO_Activity::getSourceContactID($activityArray['id']);
if ($cid && !empty($returns['source_contact_name'])) {
$activities[$key]['source_contact_name'] = CRM_Core_DAO::getFieldValue('CRM_Contact_DAO_Contact', $cid, 'display_name');
}
}
break;

case 'tag_id':
Expand Down Expand Up @@ -613,6 +571,66 @@ function _civicrm_api3_activity_get_formatResult($params, $activities, $options)
return $activities;
}

/**
* Append activity contact details to activity results.
*
* Adds id & name of activity contacts to results array if check_permissions
* does not block access to them.
*
* For historical reasons source_contact_id is always added & is not an array.
* The others are added depending on requested return params.
*
* @param array $activities
* @param array $params
* @param array $returns
*/
function _civicrm_api3_activity_fill_activity_contact_names(&$activities, $params, $returns) {
$contactTypes = array_flip(CRM_Activity_BAO_ActivityContact::buildOptions('record_type_id', 'validate'));
$assigneeType = $contactTypes['Activity Assignees'];
$targetType = $contactTypes['Activity Targets'];
$sourceType = $contactTypes['Activity Source'];
$typeMap = [
$assigneeType => 'assignee',
$sourceType => 'source',
$targetType => 'target'
];

$activityContactTypes = [$sourceType];

if (!empty($returns['target_contact_name']) || !empty($returns['target_contact_id'])) {
$activityContactTypes[] = $targetType;
}
if (!empty($returns['assignee_contact_name']) || (!empty($returns['assignee_contact_id']))) {
$activityContactTypes[] = $assigneeType;
}
$activityContactParams = [
'activity_id' => ['IN' => array_keys($activities)],
'return' => [
'activity_id',
'record_type_id',
'contact_id.display_name',
'contact_id'
],
'check_permissions' => !empty($params['check_permissions']),
];
if (count($activityContactTypes) < 3) {
$activityContactParams['record_type_id'] = ['IN' => $activityContactTypes];
}
$activityContacts = civicrm_api3('ActivityContact', 'get', $activityContactParams)['values'];
foreach ($activityContacts as $activityContact) {
$contactID = $activityContact['contact_id'];
$recordType = $typeMap[$activityContact['record_type_id']];
if (in_array($recordType, ['target', 'assignee'])) {
$activities[$activityContact['activity_id']][$recordType . '_contact_id'][] = $contactID;
$activities[$activityContact['activity_id']][$recordType . '_contact_name'][$contactID] = isset($activityContact['contact_id.display_name']) ? $activityContact['contact_id.display_name'] : '';
}
else {
$activities[$activityContact['activity_id']]['source_contact_id'] = $contactID;
$activities[$activityContact['activity_id']]['source_contact_name'] = isset($activityContact['contact_id.display_name']) ? $activityContact['contact_id.display_name'] : '';
}
}
}


/**
* Delete a specified Activity.
Expand Down
10 changes: 10 additions & 0 deletions tests/phpunit/CRMTraits/ACL/PermissionTrait.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,4 +156,14 @@ public function setupScenarioCoreACLEveryonePermittedToGroup() {
$this->setupCoreACLPermittedToGroup([$this->scenarioIDs['Group']['permitted_group']]);
}

/**
* Clean up places where permissions get cached.
*/
protected function cleanupCachedPermissions() {
if (isset(Civi::$statics['CRM_Contact_BAO_Contact_Permission'])) {
unset(Civi::$statics['CRM_Contact_BAO_Contact_Permission']);
}
CRM_Core_DAO::executeQuery('TRUNCATE civicrm_acl_contact_cache');
}

}
44 changes: 36 additions & 8 deletions tests/phpunit/api/v3/ACLPermissionTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -455,17 +455,16 @@ public function testGetActivityNoPermissions() {
/**
* View all activities is enough regardless of contact ACLs.
*/
public function testGetActivityViewAllActivitiesEnoughWithOrWithoutID() {
public function testGetActivityViewAllActivitiesDoesntCutItAnymore() {
$activity = $this->activityCreate();
$this->setPermissions(array('view all activities', 'access CiviCRM'));
$this->callAPISuccess('Activity', 'getsingle', array('check_permissions' => 1, 'id' => $activity['id']));
$this->callAPISuccess('Activity', 'getsingle', array('check_permissions' => 1));
$this->callAPISuccessGetCount('Activity', ['check_permissions' => 1, 'id' => $activity['id']], 0);
}

/**
* View all activities is required unless id is passed in.
*/
public function testGetActivityViewAllContactsEnoughWIthoutID() {
public function testGetActivityViewAllContactsEnoughWithoutID() {
$this->setPermissions(array('view all contacts', 'access CiviCRM'));
$this->callAPISuccess('Activity', 'get', array('check_permissions' => 1));
}
Expand Down Expand Up @@ -531,20 +530,49 @@ public function testGetActivityByACL() {
$activity = $this->activityCreate();

$this->hookClass->setHook('civicrm_aclWhereClause', array($this, 'aclWhereHookAllResults'));
$this->callAPISuccess('Activity', 'getsingle', array('check_permissions' => 1, 'id' => $activity['id']));
$this->callAPISuccessGetSingle('Activity', ['check_permissions' => 1, 'id' => $activity['id']]);
$this->callAPISuccessGetCount('Activity', ['check_permissions' => 1, 'id' => $activity['id']]);
}

/**
* To leverage ACL permission to view an activity you must be able to see all of the contacts.
* To leverage ACL permission to view an activity you must be able to see any of the contacts.
*/
public function testGetActivityByAclCannotViewAllContacts() {
$activity = $this->activityCreate(['assignee_contact_id' => $this->individualCreate()]);
$contacts = $this->getActivityContacts($activity);
$this->setPermissions(['access CiviCRM']);

foreach ($contacts as $role => $contact_id) {
$this->allowedContactId = $contact_id;
$this->hookClass->setHook('civicrm_aclWhereClause', array($this, 'aclWhereOnlyOne'));
$this->cleanupCachedPermissions();
$result = $this->callAPISuccessGetSingle('Activity', [
'check_permissions' => 1,
'id' => $activity['id'],
'return' => ['source_contact_id', 'target_contact_id', 'assignee_contact_id'],
]);
foreach (['source_contact', 'target_contact', 'assignee_contact'] as $roleName) {
$roleKey = $roleName . '_id';
if ($role !== $roleKey) {
$this->assertTrue(empty($result[$roleKey]), "Only contact in $role is permissioned to be returned, not $roleKey");
}
else {
$this->assertEquals([$contact_id], (array) $result[$roleKey]);
$this->assertTrue(!empty($result[$roleName . '_name']));
}
}
}
}

/**
* To leverage ACL permission to view an activity you must be able to see any of the contacts.
*/
public function testGetActivityByAclCannotViewAnyContacts() {
$activity = $this->activityCreate();
$contacts = $this->getActivityContacts($activity);
$this->setPermissions(array('access CiviCRM'));

foreach ($contacts as $contact_id) {
$this->allowedContactId = $contact_id;
$this->hookClass->setHook('civicrm_aclWhereClause', array($this, 'aclWhereOnlyOne'));
$this->callAPIFailure('Activity', 'getsingle', array('check_permissions' => 1, 'id' => $activity['id']));
}
}
Expand Down