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

CRM-20441 further fixes & tests for api + acl #10251

Merged
merged 3 commits into from
Apr 26, 2017
Merged

CRM-20441 further fixes & tests for api + acl #10251

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
cbcedb3
Test preparatory cleanup
eileenmcnaughton Apr 25, 2017
75d842f
CRM-20441 add test for no activities viewable and add test fix
eileenmcnaughton Apr 25, 2017
26583d3
CRM-20441 always do acl checks from api
eileenmcnaughton Apr 25, 2017
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
34 changes: 1 addition & 33 deletions CRM/Activity/BAO/Activity.php
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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