diff --git a/CRM/Activity/BAO/Activity.php b/CRM/Activity/BAO/Activity.php index 885a47be9197..88a7e55bedfb 100644 --- a/CRM/Activity/BAO/Activity.php +++ b/CRM/Activity/BAO/Activity.php @@ -665,47 +665,15 @@ public static function logActivityAction($activity, $logMessage = NULL) { 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 attpemts 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 1b34761edf49..0d6a85de41c4 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 644e175b375e..33677ca53a25 100644 --- a/tests/phpunit/CRM/Activity/BAO/ActivityTest.php +++ b/tests/phpunit/CRM/Activity/BAO/ActivityTest.php @@ -7,12 +7,18 @@ class CRM_Activity_BAO_ActivityTest extends CiviUnitTestCase { public function setUp() { parent::setUp(); + $this->prepareForACLs(); + CRM_Core_Config::singleton()->userPermissionClass->permissions = array('view all contacts', 'access CiviCRM'); } + /** + * Clean up after tests. + */ public function tearDown() { - // truncate a few tables - $tablesToTruncate = array('civicrm_contact', 'civicrm_activity', 'civicrm_activity_contact'); + $tablesToTruncate = array('civicrm_activity', 'civicrm_activity_contact'); $this->quickCleanup($tablesToTruncate); + $this->cleanUpAfterACLs(); + parent::tearDown(); } /** @@ -33,8 +39,7 @@ public function testCreate() { 'subject', 'Database check for created activity.' ); - // Now call create() to modify an existing Activity - + // Now call create() to modify an existing Activity. $params = array( 'id' => $activityId, 'source_contact_id' => $contactId, @@ -114,11 +119,11 @@ public function testRetrieve() { CRM_Activity_BAO_Activity::create($params); - $activityId = $this->assertDBNotNull('CRM_Activity_DAO_Activity', 'Scheduling Meeting', 'id', + $this->assertDBNotNull('CRM_Activity_DAO_Activity', 'Scheduling Meeting', 'id', 'subject', 'Database check for created activity.' ); - $activityTargetId = $this->assertDBNotNull('CRM_Activity_DAO_ActivityContact', $targetContactId, + $this->assertDBNotNull('CRM_Activity_DAO_ActivityContact', $targetContactId, 'id', 'contact_id', 'Database check for created activity target.' ); @@ -163,11 +168,11 @@ public function testDeleteActivity() { CRM_Activity_BAO_Activity::create($params); - $activityId = $this->assertDBNotNull('CRM_Activity_DAO_Activity', 'Scheduling Meeting', 'id', + $this->assertDBNotNull('CRM_Activity_DAO_Activity', 'Scheduling Meeting', 'id', 'subject', 'Database check for created activity.' ); - $activityTargetId = $this->assertDBNotNull('CRM_Activity_DAO_ActivityContact', $targetContactId, + $this->assertDBNotNull('CRM_Activity_DAO_ActivityContact', $targetContactId, 'id', 'contact_id', 'Database check for created activity target.' ); @@ -214,7 +219,7 @@ public function testDeleteActivityTarget() { 'subject', 'Database check for created activity.' ); - $activityTargetId = $this->assertDBNotNull('CRM_Activity_DAO_ActivityContact', $targetContactId, + $this->assertDBNotNull('CRM_Activity_DAO_ActivityContact', $targetContactId, 'id', 'contact_id', 'Database check for created activity target.' ); @@ -256,7 +261,7 @@ public function testDeleteActivityAssignment() { 'subject', 'Database check for created activity.' ); - $activityAssignmentId = $this->assertDBNotNull('CRM_Activity_DAO_ActivityContact', + $this->assertDBNotNull('CRM_Activity_DAO_ActivityContact', $assigneeContactId, 'id', 'contact_id', 'Database check for created activity assignment.' ); @@ -274,30 +279,10 @@ public function testDeleteActivityAssignment() { /** * Test getActivities BAO method for getting count. */ - public function testGetActivitiesCountforAdminDashboard() { - $op = new PHPUnit_Extensions_Database_Operation_Insert(); - $op->execute($this->_dbconn, - $this->createFlatXMLDataSet( - dirname(__FILE__) . '/activities_for_dashboard_count.xml' - ) - ); - - $params = array( - 'contact_id' => NULL, - 'admin' => TRUE, - 'caseId' => NULL, - 'context' => 'home', - 'activity_type_id' => NULL, - 'offset' => 0, - 'rowCount' => 0, - 'sort' => NULL, - ); - $activityCount = CRM_Activity_BAO_Activity::getActivities($params, TRUE); - - //since we are loading activities from dataset, we know total number of activities - // 8 schedule activities that should be shown on dashboard - $count = 8; - $this->assertEquals($count, $activityCount); + public function testGetActivitiesCountForAdminDashboard() { + $this->setUpForActivityDashboardTests(); + $activityCount = CRM_Activity_BAO_Activity::getActivities($this->_params, TRUE); + $this->assertEquals(8, $activityCount); } /** @@ -381,7 +366,7 @@ public function testActivityFilters() { $activities = $obj->getContactActivity(); // This should include activities of type Meeting only. - foreach ($activities['data'] as $key => $value) { + foreach ($activities['data'] as $value) { $this->assertContains('Meeting', $value['activity_type']); } unset($_GET['activity_type_id']); @@ -389,7 +374,7 @@ public function testActivityFilters() { $_GET['activity_type_exclude_id'] = 1; $activities = $obj->getContactActivity(); // None of the activities should be of type Meeting. - foreach ($activities['data'] as $key => $value) { + foreach ($activities['data'] as $value) { $this->assertNotEquals('Meeting', $value['activity_type']); } } @@ -425,25 +410,9 @@ public function testGetActivitiesCountforContactSummaryWithNoActivities() { /** * Test getActivities BAO method. */ - public function testGetActivitiesforAdminDashboard() { - $op = new PHPUnit_Extensions_Database_Operation_Insert(); - $op->execute($this->_dbconn, - $this->createFlatXMLDataSet( - dirname(__FILE__) . '/activities_for_dashboard_count.xml' - ) - ); - - $params = array( - 'contact_id' => NULL, - 'admin' => TRUE, - 'caseId' => NULL, - 'context' => 'home', - 'activity_type_id' => NULL, - 'offset' => 0, - 'rowCount' => 0, - 'sort' => NULL, - ); - $activities = CRM_Activity_BAO_Activity::getActivities($params); + public function testGetActivitiesForAdminDashboard() { + $this->setUpForActivityDashboardTests(); + $activities = CRM_Activity_BAO_Activity::getActivities($this->_params); //since we are loading activities from dataset, we know total number of activities // with no contact ID and there should be 8 schedule activities shown on dashboard @@ -457,6 +426,29 @@ public function testGetActivitiesforAdminDashboard() { } } + /** + * Test getActivities BAO method. + */ + public function testGetActivitiesForAdminDashboardNoViewContacts() { + CRM_Core_Config::singleton()->userPermissionClass->permissions = array('access CiviCRM'); + $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)); + + } + /** * Test getActivities BAO method. */ @@ -506,10 +498,11 @@ public function testGetActivitiesforNonAdminDashboard() { public function testTargetCountforContactSummary() { $targetCount = 5; $contactId = $this->individualCreate(); + $targetContactIDs = array(); for ($i = 0; $i < $targetCount; $i++) { $targetContactIDs[] = $this->individualCreate(array(), $i); } - // create activities with 5 target contacts + // Create activities with 5 target contacts. $activityParams = array( 'source_contact_id' => $contactId, 'target_contact_id' => $targetContactIDs, @@ -733,7 +726,6 @@ public function testEmailAddressOfActivityCopy() { $formAddress = CRM_Case_BAO_Case::getReceiptFrom($activity['id']); $expectedFromAddress = sprintf("%s <%s>", $sourceDisplayName, $sourceContactParams['email']); $this->assertEquals($expectedFromAddress, $formAddress); - // ----------------------- End of Case 1 --------------------------- // Case 2: System Default From Address // but first erase the email address of existing source contact ID @@ -762,10 +754,32 @@ public function testEmailAddressOfActivityCopy() { // TODO: due to unknown reason the following assertion fails on // test.civicrm.org test build but works fine on local // $this->assertEquals($expectedFromAddress, $formAddress); - // ----------------------- End of Case 2 --------------------------- // TODO: Case 4 about checking the $formAddress on basis of logged contact ID respectively needs, // to change the domain setting, which isn't straight forward in test environment } + /** + * Set up for testing activity queries. + */ + protected function setUpForActivityDashboardTests() { + $op = new PHPUnit_Extensions_Database_Operation_Insert(); + $op->execute($this->_dbconn, + $this->createFlatXMLDataSet( + dirname(__FILE__) . '/activities_for_dashboard_count.xml' + ) + ); + + $this->_params = array( + 'contact_id' => NULL, + 'admin' => TRUE, + 'caseId' => NULL, + 'context' => 'home', + 'activity_type_id' => NULL, + 'offset' => 0, + 'rowCount' => 0, + 'sort' => NULL, + ); + } + } diff --git a/tests/phpunit/CRM/Activity/BAO/activities_for_dashboard_count.xml b/tests/phpunit/CRM/Activity/BAO/activities_for_dashboard_count.xml index 4ce2c20d4a0e..f954a3b7e47b 100644 --- a/tests/phpunit/CRM/Activity/BAO/activities_for_dashboard_count.xml +++ b/tests/phpunit/CRM/Activity/BAO/activities_for_dashboard_count.xml @@ -1,26 +1,6 @@ - - - - userPermissionClass->permissions = array(); + } + + /** + * Reset after ACLs. + */ + protected function cleanUpAfterACLs() { + CRM_Utils_Hook::singleton()->reset(); + $tablesToTruncate = array( + 'civicrm_acl', + 'civicrm_acl_cache', + 'civicrm_acl_entity_role', + 'civicrm_acl_contact_cache', + ); + $this->quickCleanup($tablesToTruncate); + $config = CRM_Core_Config::singleton(); + unset($config->userPermissionClass->permissions); + } /** * Create a smart group. * @@ -3800,6 +3823,19 @@ public function createPriceSetWithPage($entity = NULL, $params = array()) { 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 6d86ad8f8705..5fdf7ac53b00 100644 --- a/tests/phpunit/api/v3/ACLPermissionTest.php +++ b/tests/phpunit/api/v3/ACLPermissionTest.php @@ -44,8 +44,7 @@ public function setUp() { $baoObj = new CRM_Core_DAO(); $baoObj->createTestObject('CRM_Pledge_BAO_Pledge', array(), 1, 0); $baoObj->createTestObject('CRM_Core_BAO_Phone', array(), 1, 0); - $config = CRM_Core_Config::singleton(); - $config->userPermissionClass->permissions = array(); + $this->prepareForACLs(); } /** @@ -53,7 +52,7 @@ public function setUp() { * @see CiviUnitTestCase::tearDown() */ public function tearDown() { - CRM_Utils_Hook::singleton()->reset(); + $this->cleanUpAfterACLs(); $tablesToTruncate = array( 'civicrm_contact', 'civicrm_group_contact', @@ -72,8 +71,6 @@ public function tearDown() { 'civicrm_tag', ); $this->quickCleanup($tablesToTruncate); - $config = CRM_Core_Config::singleton(); - unset($config->userPermissionClass->permissions); } /** @@ -469,19 +466,6 @@ public function aclWhereOnlyOne($type, &$tables, &$whereTables, &$contactID, &$w $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. */ @@ -504,9 +488,9 @@ public function testGetActivityViewAllActivitiesEnoughWithOrWithoutID() { /** * 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)); } /** @@ -633,8 +617,8 @@ public function testActivitiesGetMultipleIdsCheckPermissionsNotIN() { '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']); } /**