[CRM-20441] Fatal error on contact summary for ACL'd user (from activity tab count) - CiviCRM Issue Tracker

Details

Type: Bug
Status: Done/Fixed
Priority: Blocker
Resolution: Fixed/Completed
Affects Version/s: 4.7.19
Fix Version/s: 4.7.19
Component/s: Core CiviCRM
Labels:
- ACL
- Activities
- PatchSubmitted
- fatal

Versioning Impact:
Patch (backwards-compatible bug fixes)
Documentation Required?:
None
Funding Source:
Contributed Code
Verified?:
Yes

Description

When an ACL'd user tries to view the contact summary for a permitted contact who has activities, a fatal error occurs. Backtrace shows that the error occurs when generating the count for the Activities tab.

Steps to replicate (in Drupal)

Create role with just "access CiviCRM" permission.
Create a user X with only the above role.
Create Civi ACL group "ACL Test" and add contact X.
Create Civi group "Visible contacts" and add a subset of contacts.
Create Civi ACL role "ACL Test" and assign to group "ACL Test".
Create ACL permitting "ACL Test" to view group "Visible contacts".
Logged in as user X, do an unconstrained activity search.
Click on the target contact for any of the retrieved activities.

Expected Result

See contact summary for target contact.

Actual result

A fatal error was triggered: One of parameters (value: ) is not of the type Integer.

Also tested on stock 4.7.18, problem did not occur there.

Backtrace

#0 .../dmaster/sites/all/modules/civicrm/CRM/Core/Error.php(336): CRM_Core_Error::backtrace("backTrace", TRUE)
#1 .../dmaster/sites/all/modules/civicrm/CRM/Utils/Type.php(476): CRM_Core_Error::fatal("One of parameters (value: ) is not of the type Integer")
#2 .../dmaster/sites/all/modules/civicrm/CRM/Core/DAO.php(1377): CRM_Utils_Type::validate((Array:1), "Integer")
#3 .../dmaster/sites/all/modules/civicrm/CRM/Core/DAO.php(1341): CRM_Core_DAO::composeQuery("SELECT id FROM civicrm_case_activity WHERE activity_id = %1", (Array:1), TRUE)
#4 .../dmaster/sites/all/modules/civicrm/CRM/Case/BAO/Case.php(2826): CRM_Core_DAO::singleValueQuery("SELECT id FROM civicrm_case_activity WHERE activity_id = %1", (Array:1))
#5 .../dmaster/sites/all/modules/civicrm/CRM/Activity/BAO/Activity.php(2121): CRM_Case_BAO_Case::isCaseActivity((Array:1))
#6 .../dmaster/sites/all/modules/civicrm/api/v3/Activity.php(310): CRM_Activity_BAO_Activity::checkPermission((Array:1), 4)
#7 .../dmaster/sites/all/modules/civicrm/Civi/API/Provider/MagicFunctionProvider.php(89): civicrm_api3_activity_get((Array:9))
#8 .../dmaster/sites/all/modules/civicrm/Civi/API/Kernel.php(169): Civi\API\Provider\MagicFunctionProvider->invoke((Array:9))
#9 .../dmaster/sites/all/modules/civicrm/Civi/API/Kernel.php(100): Civi\API\Kernel->runRequest((Array:7))
#10 .../dmaster/sites/all/modules/civicrm/api/api.php(43): Civi\API\Kernel->runSafe("Activity", "Get", (Array:9))
#11 .../dmaster/sites/all/modules/civicrm/CRM/Activity/BAO/Activity.php(776): civicrm_api3("Activity", "Get", (Array:8))
#12 .../dmaster/sites/all/modules/civicrm/CRM/Contact/BAO/Contact.php(2619): CRM_Activity_BAO_Activity::getActivities((Array:4), TRUE)
#13 .../dmaster/sites/all/modules/civicrm/CRM/Contact/Page/View/Summary.php(358): CRM_Contact_BAO_Contact::getCountComponent("activity", "101")
#14 .../dmaster/sites/all/modules/civicrm/CRM/Contact/Page/View/Summary.php(93): CRM_Contact_Page_View_Summary->view()
#15 .../dmaster/sites/all/modules/civicrm/CRM/Core/Invoke.php(310): CRM_Contact_Page_View_Summary->run((Array:3), NULL)
#16 .../dmaster/sites/all/modules/civicrm/CRM/Core/Invoke.php(84): CRM_Core_Invoke::runItem((Array:14))
#17 .../dmaster/sites/all/modules/civicrm/CRM/Core/Invoke.php(52): CRM_Core_Invoke::_invoke((Array:3))
#18 .../dmaster/sites/all/modules/civicrm/drupal/civicrm.module(448): CRM_Core_Invoke::invoke((Array:3))
#19 [internal function](): civicrm_invoke("contact", "view")
#20 .../dmaster/includes/menu.inc(527): call_user_func_array("civicrm_invoke", (Array:2))
#21 .../dmaster/index.php(21): menu_execute_active_handler()
#22 {main}vicrm/CRM/Core/Error.php(336): CRM_Core_Error::backtrace("backTrace", TRUE)

Discussed on Mattermost. Diagnosis there from Monish...

---8<---

the issue occurred as Activity.get throws error, when in API $params you have multiple activity IDs with check_permission = TRUE parameters. Lemme explain how this is related to your issue cited in the backtrace:

Due to recent improvements made under ~~CRM-20207~~ where the underlying Activity BAO function getActivities() used to fetch activities, is using Activity.get API instead of SQL here https://github.com/civicrm/civicrm-core/blob/master/CRM/Activity/BAO/Activity.php#L776.
So when the API use multiple Activity IDs with check_permissions = TRUE here https://github.com/civicrm/civicrm-core/blob/master/CRM/Activity/BAO/Activity.php#L687 . It eventually got tripped at https://github.com/civicrm/civicrm-core/blob/master/api/v3/Activity.php#L310. The issue which ya encountered
As CRM_Activity_BAO_Activity::checkPermission($params['id'], CRM_Core_Action::VIEW)) expect $params['id'] to be integer value Activity ID not array of activity IDs with advance filter which is in our case Array('IN' => array(..Activity IDs.))

In my opinion we need to handle CRM_Activity_BAO_Activity::checkPermission(...) for multiple activity IDs , @coleman whats your thought ?

---8<---

Attachments

Options
- Sort By Name
- Sort By Date
- Ascending
- Descending
- Download All

Attachments

backtrace-crm-20441-2017-04-25-11-35.txt
2017-04-25 10:44
13 kB
Dave Jenkins

Issue Links

is supplemented by

CRM-20481 Refactor Activity api to more efficient query

Won't Do

links to

CRM-20441 Fix issue where multiple activity Ids were not supported when check permissions was used

PR: civicrm-core#10231: CRM-20441 Limit acceptable operators to IN only

PR: civicrm-core#10239: CRM-20441: Fatal error on contact summary for ACL'd user (from activity tab count)

PR: civicrm-core#10247: CRM-20441 Fix tests following latest fix

PR: civicrm-core#10251: CRM-20441 add test for no activities viewable

(3 links to)

Activity

[CRM-20441] Fatal error on contact summary for ACL'd user (from activity tab count)

Seamus Lee added a comment - 2017-04-22 23:17

I am closing this as the PR has been merged and the PR contains tests similar to steps to replicate

Dave Jenkins added a comment - 2017-04-24 14:28

I'm not sure this is fixed, I'm now getting a different fatal error with current 4.7.19-rc branch - see https://github.com/civicrm/civicrm-core/pull/10212#issuecomment-296609236

Eileen McNaughton added a comment - 2017-04-25 5:42

Seamus Lee I think this needs closing - but possibly a new one opening?

Seamus Lee added a comment - 2017-04-25 5:54

Closing as an Interim PR has been merged which i believe fixes the fatal associated with this now

Seamus Lee added a comment - 2017-04-25 5:57

Have opened https://issues.civicrm.org/jira/browse/CRM-20473 as issue to sort out the permissions issue

Dave Jenkins added a comment - 2017-04-25 10:09

Great to see all the good work happening on this but on updating my 4.7.19-rc tree, clearing caches and re-testing, I still get a fatal error (as ACL'd user) on contact summary for any contact that has activities. It's now a different fatal error:

Cannot access activities. Required permission: 'view all activities''

To be clear, this happens on contact summary, /civicrm/contact/view?reset=1&cid=X, I'm not going to the activities tab: I can't get that far, as I get this fatal error. I'm updating the title of the issue to reflect this, as the original title "Fatal error on contact summary activity tab for ACL'd user" suggests the issue only happens on the activities tab: I included that bit to indicate that the source of the problem lay in the activities tab (populating its count). Changing title to "Fatal error on contact summary for ACL'd user (from activity tab count)".

I also have to disagree with the content of the error message, which suggests that activities can now only be viewed if the user has 'view all activities' permission. If that's accurate, it's a big change in behaviour for sites using ACLs and we would need to issue a warning that sites using ACLs should avoid this release unless they want this change of behaviour.

I'm a bit nervous that I'm the only one reporting these problems, have others tested the 4.7.19-rc tree tree with an ACL'd user? If others have tested and aren't seeing the problem, let's compare notes on our set-ups and figure out what's going on: I'd hate to think that some oddity of my set-up is causing spurious problems. I don't think so though, it's a recent dmaster civibuild that I switched the other day to the 4.7.19-rc branch, nothing odd that I'm aware of.

Seamus Lee added a comment - 2017-04-25 10:30

Hi Dave Jenkins the only reason that error message comes up is if no ids are passed to the activity https://github.com/civicrm/civicrm-core/blob/master/api/v3/Activity.php#L300 and we are supplying a contact id. Are you able to put some debugging in / let us know which line in getActivities is triggering that error?

Dave Jenkins added a comment - 2017-04-25 10:46

Backtrace attached.
backtrace-crm-20441-2017-04-25-11-35.txt

Seamus Lee added a comment - 2017-04-25 10:58

Thanks Dave Jenkins have opened this https://github.com/civicrm/civicrm-core/pull/10237 on my suspicion which i'm 99% sure your backtrace confirms my suspicion

Monish Deb added a comment - 2017-04-25 11:17

Seamus Lee Dave Jenkins I have submitted a alternate fix as https://github.com/civicrm/civicrm-core/pull/10239. Please have a look

Monish Deb added a comment - 2017-04-25 11:20

Issue occur when logged in user don't have permission to 'view all activities'. So better to catch the exception and return empty result.

Dave Jenkins added a comment - 2017-04-25 11:35

Both #10237 and #10239 successfully fixed the issue in my testing, see comments on PRs.

Dave Jenkins added a comment - 2017-04-26 9:28

Reverted my cherry-picking for the previous PRs, rebased to a clean 4.7.19-rc and checked that Eileen McNaughton's latest PR, #10251 was in git log. Then re-tested. Success: this fix also worked...

Fixes the fatal error. Contact summary now displays, with Activities tab showing count = 0.

As a further test, I tried adding an activity to one of the allowed contacts that didn't have any activities. I set source = one of the allowed contacts, target = another of the allowed contacts, no assignee. So that should be visible to the restricted user - and it is: count = 1 on Activities tab, can view activity.

Happy to close if everyone else is.

Dave Jenkins added a comment - 2017-04-26 9:51

Actually I created the issue so I can just close it, it's not as though anybody is going to object! Good work, everyone!

Cheers, Dave.

Eileen McNaughton added a comment - 2017-04-26 9:55

I should mention that going forwards we should aim to remove calls to

CRM_Activity_BAO_Activity::checkPermission($this->_activityId,

at least where called with VIEW with

try {
$activity = civicrm_api3('Activity', 'getsingle', array(
'id' => $this->_activityId,
'check_permissions' => 1,
));
}
catch (CiviCRM_API3_Exception $e) {
CRM_Core_Error::statusBounce(ts('You do not have permission to access this page.'));
}

or similar - then we can start to deal with the performance on the CRM_Activity_BAO_Activity::checkPermission for view by not doing the contact filter on a line-by-line basis

Eileen McNaughton added a comment - 2017-04-26 10:07

Follow on

People

Assignee:

Seamus Lee

Reporter:

Dave Jenkins

Dates

Created:

2017-04-18 12:09

Updated:

2017-04-26 10:07

Resolved:

2017-04-26 9:51