From 26583d3ee6a20ecb5a6edbce9e97097ef2c56bb1 Mon Sep 17 00:00:00 2001 From: eileen Date: Wed, 26 Apr 2017 10:59:12 +1200 Subject: [PATCH] CRM-20441 always do acl checks from api --- CRM/Activity/BAO/Activity.php | 33 +----------- api/v3/Activity.php | 54 +++++++------------ .../phpunit/CRM/Activity/BAO/ActivityTest.php | 12 +++++ tests/phpunit/CiviTest/CiviUnitTestCase.php | 13 +++++ tests/phpunit/api/v3/ACLPermissionTest.php | 21 ++------ 5 files changed, 48 insertions(+), 85 deletions(-) diff --git a/CRM/Activity/BAO/Activity.php b/CRM/Activity/BAO/Activity.php index f21a06988c..88a7e55bed 100644 --- a/CRM/Activity/BAO/Activity.php +++ b/CRM/Activity/BAO/Activity.php @@ -665,46 +665,15 @@ class CRM_Activity_BAO_Activity extends CRM_Activity_DAO_Activity { public static function getActivities($params, $getCount = FALSE) { $activities = array(); - // fetch all activity IDs whose target/assignee/source contact id is $params['contact_id'] - // currently cannot be done via Activity.Get API so we are using SQL query instead - if (!empty($params['contact_id'])) { - $activityIDs = CRM_Core_DAO::singleValueQuery("SELECT GROUP_CONCAT(DISTINCT activity_id SEPARATOR ',') - FROM civicrm_activity_contact - WHERE contact_id = %1", array(1 => array($params['contact_id'], 'Integer'))); - - // if no activities found for given $params['contact_id'] then return empty array - if (empty($activityIDs)) { - return $getCount ? count($activities) : $activities; - } - $activityIDs = explode(',', $activityIDs); - // CRM-20441 Check if user has access to the activities. - // This is a temporary fix we need to figure out the rules around - // the right permissions to access Activities. - // This attempts to reduce fatal errors in 4.7.19 RC. - if (!empty($activityIDs)) { - foreach ($activityIDs as $key => $activityId) { - try { - civicrm_api3('Activity', 'get', array('id' => $activityId, 'check_permissions' => 1)); - } - catch (Exception $e) { - unset($activityIDs[$key]); - } - } - } - if (empty($activityIDs)) { - return $getCount ? count($activities) : $activities; - } - } - // fetch all active activity types $activityTypes = CRM_Core_OptionGroup::values('activity_type'); // Activity.Get API params $activityParams = array( - 'id' => (!empty($activityIDs)) ? array('IN' => $activityIDs) : NULL, 'is_deleted' => 0, 'is_current_revision' => 1, 'is_test' => 0, + 'contact_id' => CRM_Utils_Array::value('contact_id', $params), 'return' => array( 'activity_date_time', 'source_record_id', diff --git a/api/v3/Activity.php b/api/v3/Activity.php index 1b34761edf..0d6a85de41 100644 --- a/api/v3/Activity.php +++ b/api/v3/Activity.php @@ -293,42 +293,6 @@ function _civicrm_api3_activity_get_spec(&$params) { * @throws \Civi\API\Exception\UnauthorizedException */ function civicrm_api3_activity_get($params) { - if (!empty($params['check_permissions']) && !CRM_Core_Permission::check('view all activities')) { - // In absence of view all activities permission it's possible to see a specific activity by ACL. - // Note still allowing view all activities to override ACLs is based on the 'don't change too much - // if you are not sure principle' and it could be argued that the ACLs should always be applied. - if (empty($params['id']) || !empty($params['contact_id'])) { - // We fall back to the original blunt permissions if we don't have an id to check or we are about - // to go to the weird place that the legacy 'contact_id' parameter takes us to. - throw new \Civi\API\Exception\UnauthorizedException( - "Cannot access activities. Required permission: 'view all activities''" - ); - } - $ids = array(); - $allowed_operators = array( - 'IN', - ); - if (is_array($params['id'])) { - foreach ($params['id'] as $operator => $values) { - if (in_array($operator, CRM_Core_DAO::acceptedSQLOperators()) && in_array($operator, $allowed_operators)) { - $ids = $values; - } - else { - throw new \API_Exception(ts('Used an unsupported sql operator with Activity.get API')); - } - } - } - else { - $ids = array($params['id']); - } - foreach ($ids as $id) { - if (!CRM_Activity_BAO_Activity::checkPermission($id, CRM_Core_Action::VIEW)) { - throw new \Civi\API\Exception\UnauthorizedException( - 'You do not have permission to view this activity' - ); - } - } - } $sql = CRM_Utils_SQL_Select::fragment(); $recordTypes = civicrm_api3('ActivityContact', 'getoptions', array('field' => 'record_type_id')); @@ -339,6 +303,16 @@ function civicrm_api3_activity_get($params) { 'source_contact_id' => array_search('Activity Source', $recordTypes), 'assignee_contact_id' => array_search('Activity Assignees', $recordTypes), ); + 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); + } + foreach ($activityContactOptions as $activityContactName => $activityContactValue) { if (!empty($params[$activityContactName])) { if (!is_array($params[$activityContactName])) { @@ -379,6 +353,14 @@ function civicrm_api3_activity_get($params) { } } $activities = _civicrm_api3_basic_get(_civicrm_api3_get_BAO(__FUNCTION__), $params, FALSE, 'Activity', $sql); + 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']]); + } + } + } $options = _civicrm_api3_get_options_from_params($params, FALSE, 'Activity', 'get'); if ($options['is_count']) { return civicrm_api3_create_success($activities, $params, 'Activity', 'get'); diff --git a/tests/phpunit/CRM/Activity/BAO/ActivityTest.php b/tests/phpunit/CRM/Activity/BAO/ActivityTest.php index cfae9c8159..33677ca53a 100644 --- a/tests/phpunit/CRM/Activity/BAO/ActivityTest.php +++ b/tests/phpunit/CRM/Activity/BAO/ActivityTest.php @@ -434,6 +434,18 @@ class CRM_Activity_BAO_ActivityTest extends CiviUnitTestCase { $this->setUpForActivityDashboardTests(); $activities = CRM_Activity_BAO_Activity::getActivities($this->_params); $this->assertEquals(0, count($activities)); + } + + /** + * Test getActivities BAO method. + */ + public function testGetActivitiesForAdminDashboardAclLimitedViewContacts() { + CRM_Core_Config::singleton()->userPermissionClass->permissions = array('access CiviCRM'); + $this->allowedContacts = array(1, 3, 4, 5); + $this->hookClass->setHook('civicrm_aclWhereClause', array($this, 'aclWhereMultipleContacts')); + $this->setUpForActivityDashboardTests(); + $activities = CRM_Activity_BAO_Activity::getActivities($this->_params); + $this->assertEquals(1, count($activities)); } diff --git a/tests/phpunit/CiviTest/CiviUnitTestCase.php b/tests/phpunit/CiviTest/CiviUnitTestCase.php index 4082bc15e5..65f05a5fac 100644 --- a/tests/phpunit/CiviTest/CiviUnitTestCase.php +++ b/tests/phpunit/CiviTest/CiviUnitTestCase.php @@ -3823,6 +3823,19 @@ AND ( TABLE_NAME LIKE 'civicrm_value_%' ) public function aclWhereHookNoResults($type, &$tables, &$whereTables, &$contactID, &$where) { } + /** + * Only specified contact returned. + * @implements CRM_Utils_Hook::aclWhereClause + * @param $type + * @param $tables + * @param $whereTables + * @param $contactID + * @param $where + */ + public function aclWhereMultipleContacts($type, &$tables, &$whereTables, &$contactID, &$where) { + $where = " contact_a.id IN (" . implode(', ', $this->allowedContacts) . ")"; + } + /** * @implements CRM_Utils_Hook::selectWhereClause * diff --git a/tests/phpunit/api/v3/ACLPermissionTest.php b/tests/phpunit/api/v3/ACLPermissionTest.php index 1618a19421..5fdf7ac53b 100644 --- a/tests/phpunit/api/v3/ACLPermissionTest.php +++ b/tests/phpunit/api/v3/ACLPermissionTest.php @@ -466,19 +466,6 @@ class api_v3_ACLPermissionTest extends CiviUnitTestCase { $where = " contact_a.id = " . $this->allowedContactId; } - /** - * Only specified contact returned. - * @implements CRM_Utils_Hook::aclWhereClause - * @param $type - * @param $tables - * @param $whereTables - * @param $contactID - * @param $where - */ - public function aclWhereMultipleContacts($type, &$tables, &$whereTables, &$contactID, &$where) { - $where = " contact_a.id IN (" . implode(', ', $this->allowedContacts) . ")"; - } - /** * Basic check that an unpermissioned call keeps working and permissioned call fails. */ @@ -501,9 +488,9 @@ class api_v3_ACLPermissionTest extends CiviUnitTestCase { /** * View all activities is required unless id is passed in. */ - public function testGetActivityViewAllContactsNotEnoughWIthoutID() { + public function testGetActivityViewAllContactsEnoughWIthoutID() { $this->setPermissions(array('view all contacts', 'access CiviCRM')); - $this->callAPIFailure('Activity', 'get', array('check_permissions' => 1)); + $this->callAPISuccess('Activity', 'get', array('check_permissions' => 1)); } /** @@ -630,8 +617,8 @@ class api_v3_ACLPermissionTest extends CiviUnitTestCase { 'id' => array('NOT IN' => array($activity['id'], $activity2['id'])), 'check_permissions' => TRUE, ); - $result = $this->callAPIFailure('activity', 'get', $params); - $this->assertEquals('Used an unsupported sql operator with Activity.get API', $result['error_message']); + $result = $this->callAPISuccess('activity', 'get', $params); + $this->assertEquals(0, $result['count']); } /** -- 2.25.1