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.

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

Fix CRM_ACL_API::whereClause to respect $contactId param #12576

Merged
merged 3 commits into from Aug 11, 2018

Conversation

colemanw
Copy link
Member

@colemanw colemanw commented Jul 26, 2018

Overview

Fixes an edge-case bug in contact permission checks and adds more robust support to the CMS-based permission checking.

Before

The CRM_ACL_API::whereClause() function did not correctly handle the $contactID parameter. It would always check user permissions based on the current user, ignoring that param.

After

It correctly checks permissions against the specified user.

Technical Details

This fix required a patch to all 6 CMS integration points (D6, D7, D8, BD, J!, WP). They all handle checking permissions slightly differently and none of it is covered by unit tests (nor do I think it could be with our current test infrastructure). So this will require a fair amount of manual testing.

Notes

I discovered this bug while working on this issue: Load case to webform with contactID, checksum and caseID in the URL

@civibot
Copy link

civibot bot commented Jul 26, 2018

(Standard links)

@monishdeb
Copy link
Member

monishdeb commented Aug 11, 2018

This PR introduces a nice enhancement where you can check given permission of a $contactID which can be passed to CRM_Core_Permission::check(<permission string>, $contactID), unlike earlier which checks only for logged-in user. In order to test it in all CMSs I have executed a small script

civicrm_initialize();
$contactID = N;
$permission = 'edit all contacts';
if (CRM_Core_Permission::check($perm, $contactID)) {
  echo "Contact ID - $contactID has $permission permission"
}

after applying this patch. And it is working as expected. Doesn't cause any unintended regression.

Hence merging this PR.

@monishdeb monishdeb merged commit 08cf6bd into civicrm:master Aug 11, 2018
@colemanw
Copy link
Member Author

Thanks @monishdeb!

@colemanw colemanw deleted the userPerm branch August 11, 2018 14:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants