From cdacd6abc09054bef3ff9fb3717a5f7914c88386 Mon Sep 17 00:00:00 2001 From: eileen Date: Wed, 16 Jan 2019 13:02:37 +1300 Subject: [PATCH] Rationalise Activity api ACLs We have a lot of inconsistency about how (and if) activity ACLs are applied. Note that permissions only apply when the api is being called with check_permissions = TRUE - e.g from the js layer. This PR changes the logic used for the activity.get api to be consistent with the report logic which a) is the most performant variant b) is the one with the least code complexity c) is more consistent with CiviCase d) allows hooks to modify the permissions applies e) creates consistency between api v3 & v4 f) is consistent with some site user expectations but not others - the presence of all this inconsistency is an indicator not everyone wants the same thing but given that choosing a performant & maintainable option for core seems like a good criteria. After this patch 1) the 'view all activities' permission will no longer by-pass all other ACLs. One could argue that's exactly what it means - but it doesn't do that in the UI which seems like the standard elsewhere. 2) a user will be able to view an activity via the api if they have permission to view ANY contact linked to it (before it was ALL contacts via the api) 3) a user will not see the names of any contacts they do not have permission over when requesting activity contact details in return parameters 4) getcount will no longer by-pass the api 5) performance is improved Places where permissioning applies to activities - activities listing on contact - shows actitivies & related contact names regardless of permission to view the contacts - activity search results -- shows actitivies & related contact names regardless of permission to view the contacts - activity view page - links to view the activity exist on the above 2 screens but will give access denied unless they can see ALL related contacts - activity reports - shows activities if ANY related contacts are permitted, suppresses names of unpermitted contacts Potential follow on steps 1) make the activity tab listing consistent by switching from the unperformant deprecatedGetActivities fn to the performance getActivities fn - there are no remaining blockers to that. 2) align the activity view screen & add in hook call there too 3) align activity search results screen, address performance issues there too.... --- CRM/Activity/BAO/Activity.php | 21 ++-- CRM/Core/Permission.php | 3 +- api/v3/Activity.php | 116 ++++++++++-------- .../phpunit/CRMTraits/ACL/PermissionTrait.php | 10 ++ tests/phpunit/api/v3/ACLPermissionTest.php | 44 +++++-- 5 files changed, 127 insertions(+), 67 deletions(-) diff --git a/CRM/Activity/BAO/Activity.php b/CRM/Activity/BAO/Activity.php index 090ef3b103..92656c7593 100644 --- a/CRM/Activity/BAO/Activity.php +++ b/CRM/Activity/BAO/Activity.php @@ -1125,15 +1125,20 @@ ORDER BY fixed_sort_order * @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) . ')'); + + $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; } diff --git a/CRM/Core/Permission.php b/CRM/Core/Permission.php index 7b73d6c9d9..74f74c2b78 100644 --- a/CRM/Core/Permission.php +++ b/CRM/Core/Permission.php @@ -29,8 +29,6 @@ * * @package CRM * @copyright CiviCRM LLC (c) 2004-2019 - * $Id$ - * */ /** @@ -1052,6 +1050,7 @@ class CRM_Core_Permission { 'view all activities', ), ); + $permissions['activity_contact'] = $permissions['activity']; // Case permissions $permissions['case'] = array( diff --git a/api/v3/Activity.php b/api/v3/Activity.php index fc84cc1d73..f7a8e492f1 100644 --- a/api/v3/Activity.php +++ b/api/v3/Activity.php @@ -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 @@ -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'); @@ -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 @@ -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(); @@ -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': @@ -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. diff --git a/tests/phpunit/CRMTraits/ACL/PermissionTrait.php b/tests/phpunit/CRMTraits/ACL/PermissionTrait.php index 4b37e4e229..2382636352 100644 --- a/tests/phpunit/CRMTraits/ACL/PermissionTrait.php +++ b/tests/phpunit/CRMTraits/ACL/PermissionTrait.php @@ -156,4 +156,14 @@ trait CRMTraits_ACL_PermissionTrait { $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'); + } + } diff --git a/tests/phpunit/api/v3/ACLPermissionTest.php b/tests/phpunit/api/v3/ACLPermissionTest.php index f6dab21e5d..689c0cf6c7 100644 --- a/tests/phpunit/api/v3/ACLPermissionTest.php +++ b/tests/phpunit/api/v3/ACLPermissionTest.php @@ -455,17 +455,16 @@ class api_v3_ACLPermissionTest extends CiviUnitTestCase { /** * 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)); } @@ -531,20 +530,49 @@ class api_v3_ACLPermissionTest extends CiviUnitTestCase { $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'])); } } -- 2.25.1