Skip to content

Commit

Permalink
Merge pull request #10251 from eileenmcnaughton/4.7.19-rc2
Browse files Browse the repository at this point in the history 
CRM-20441 further fixes & tests for api + acl
  • Loading branch information
@eileenmcnaughton
eileenmcnaughton authored Apr 26, 2017
2 parents 76a46c0 + 26583d3 commit 881d82e
Show file tree
Hide file tree
Showing 6 changed files with 133 additions and 169 deletions.
34 changes: 1 addition & 33 deletions CRM/Activity/BAO/Activity.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down
54 changes: 18 additions & 36 deletions api/v3/Activity.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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'));
Expand All @@ -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])) {
Expand Down Expand Up @@ -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');
Expand Down
130 changes: 72 additions & 58 deletions tests/phpunit/CRM/Activity/BAO/ActivityTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}

/**
Expand All @@ -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,
Expand Down Expand Up @@ -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.'
);
Expand Down Expand Up @@ -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.'
);
Expand Down Expand Up @@ -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.'
);
Expand Down Expand Up @@ -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.'
);
Expand All @@ -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);
}

/**
Expand Down Expand Up @@ -381,15 +366,15 @@ 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']);

$_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']);
}
}
Expand Down Expand Up @@ -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
Expand All @@ -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.
*/
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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,
);
}

}
20 changes: 0 additions & 20 deletions tests/phpunit/CRM/Activity/BAO/activities_for_dashboard_count.xml
View file
Original file line number Diff line number Diff line change
@@ -1,26 +1,6 @@
<?xml version="1.0" encoding="UTF-8" ?>
<!-- $Id: dataset.xml 23301 2009-08-17 13:02:24Z walt $ -->
<dataset>
<!-- Source Contact for Activity: 1 -->
<civicrm_contact
id="1"
contact_type="Individual"
is_opt_out="0"
display_name="Test Contact 1"
sort_name="Test Contact 1"
first_name="Test"
last_name="Contact 1"
/>
<!-- Source Contact for Activity: 2 -->
<civicrm_contact
id="2"
contact_type="Individual"
is_opt_out="0"
display_name="Test Contact 2"
sort_name="Test Contact 2"
first_name="Test"
last_name="Contact 2"
/>
<!-- Source Contact for Activity: 3 -->
<civicrm_contact
id="3"
Expand Down
Loading

0 comments on commit 881d82e

Please sign in to comment.