CRM-17350 Non-administrators unable to tag contacts using Tags tab

Jim Crist and Jon K Goldberg - Users with edit permissions via 'edit all contacts' OR via an ACL that grants edit permission on the contact should be able to use the Tags tab to add / remove tags.

I tested the 'edit permission via ACL' case in 4.7 and indeed the user is able to do all other contact edit tasks if they are granted edit rights via ACL (e.g. inline edit of address, phone etc and edit all properties in full contact edit form).

The immediate problem is that the Tags tab is using EntityTag api - which in term uses api permissions defined in CRM/Core/DAO/permissions.php. Commenting out line 117 prevents the api permissions:

'default' => array(
'access CiviCRM',
// 'edit all contacts',

Of course this is NOT the 'solution'. What needs to happen is replace that with a check for edit permissions on the specific contact id. Evidently this may be done somewhere else for 'address', 'phone', etc. since this same user CAN update those via inline edit which "I think" uses the api and their default permissions are set in the same place. I snooped around for that over-ride but didn't find it. Eileen McNaughton or Coleman Watts might be able to shed more light on this.

This is the error you get in 4.7 when trying to update a tag in Tags tab. Note that CRM_Contact_Page_View::checkUserPermission seems to be assigning the 'edit' permission correctly when called by CRM_Contact_Page_View_Tag - because the checkboxes are NOT set read-only (as they would be if $permission was 'view').

The inline edit on the contact summary doesn't use the api 'properly' - so this problem wasn't tackled for it. The work johanv did on the api in 4.7 lays the groundwork for dealing with this - we are now able to add joins in the api - but we still need to build in the ACLs

I see where Eileen and Johan are going with CRM-16036, and I agree that sounds like an ideal long-term solution. However, this seems a bit further out.

In the interim, what about moving the permission check from the API to BAO level? Something like:

• Remove the "Edit All Contacts" permission check from EntityTag in permissions.php;
• In CRM_Core_BAO_EntityTag::addEntitiesToTag (and CRM_Core_BAO_EntityTag::removeEntitiesFromTag) add a check in the foreach loop that's something like:
  {{
  if (!CRM_Core_Permission::check('edit all contacts'))
  Unknown macro: { $result = civicrm_api3('Contact', 'getsingle', array('id' => $entityId)); if ($result['is_error'] == 1) { continue; }}}}

I'm not 100% sure what the error you'd get would be on permission failure, so that's a question mark, but is it worth pursuing this? Or are ACL-aware joins in the API a realistic target for 4.7?

That sounds feasible specifically for that api because it is so abnormal ...(non-standard)

I agree with that approach, although instead of checking 'edit all contacts' in the loop you can actually use CRM_Contact_BAO_Contact_Permission::allow() which respects finer-grained ACLs.

I have an initial patch here: https://github.com/civicrm/civicrm-core/pull/6952

It seems that because of how the EntityTag API is non-standard, I can't really bubble up an error if someone lacks the correct permission - I just get to increment numEntitiesNotAdded. If I'm missing some better approach that lets me return an actual error but does NOT break a use case where multiple entities are receiving a tag, I'd love to hear it - but I think this is as good as it gets here.

Also - Hungarian variable notation in PHP? Sigh.

Seamus Lee pointed out a mistake I made, where I'd unintentionally loosened the permissions on more APIs than just EntityTag. I updated the PR to fix this.

The PR needs some additional work to handle tags on entities other than contacts, per comment by Coleman Watts.