CiviCRM Community Forums (archive)

Hi,
I posted this as an issue when we were testing 3.4.alpha and 3.4.beta but at that time, I wasn't sure if it was related to our multi-site installation or to a core issue, but now I am experiencing the same issue in a 3.4.1 non-multi-site installation of civi.

Permissions for the role "staff" are as follows: access civicrm, access contacts, add contacts, profile listings, access content and change user name.

I log in as a user with the "staff" role. Do a search for all contacts. Select a contact. I am able to select "delete contact" from the "More" menu that appears to the right of the selected contact. I am then able to delete the selected contact, usually without error, but the contact continues to appear on the screen even though the message indicates that the contact was deleted. Sometimes though, when I click on delete contact I receive the message "sorry, a non-recoverable error has occurred".

Further... if I log out and then log back in as an administrator with full permissions, the record that was deleted by "staff" does not appear when I do a search/find contacts but it does appear when I do an advanced search/search in "trash".

I can't test this on the demo because i can't get to the Drupal permissions.

Any help would be much appreciated.

Thanks,
Cynthia

Hi,

I've had a bit of a look through the relevant code. In our case I found that "edit all contacts" permission was allowing users to delete contacts.

Cynthia, you didn't mention "edit all contacts" permission but you mentioned "access contacts", which isn't a permission that I can see. Can you confirm the exact Civi permissions that your staff role has? Do you have any ACLs defined for the relevant users?

Here's the code path that seems to be responsible:

CRM_Contact_Selector::getRows:
line 567:
        $links =& self::links( $this->_context, $this->_contextMenu, $this->_key );
then line 723:
                // FIXME: guard with permission check
                } else {
                    $row['action']   = CRM_Core_Action::formLink( $links, $mapMask, array( 'id' => $result->contact_id ) );
                }
...where $this->_contextMenu was passed as a parameter to CRM_Contact_Selector::_construct, which was called in CRM_Contact_Form_Search::postProcess, passing contextMenu originally set in preProcess from CRM_Contact_BAO_Contact::contextMenu().

CRM_Contact_BAO_Contact::contextMenu() checks for 'access deleted contacts' and 'delete contacts' - but it then does this:
line 2543:
                 // if still user does not have required permissions, check acl.
                 if ( !$hasAllPermissions ) {
                     if ( in_array( $values['ref'], $aclPermissionedTasks ) &&
                          $corePermission == CRM_Core_Permission::EDIT ) {
                         $hasAllPermissions = true;
                     } else...
                 }
where $values['ref'] is 'delete-contact', which is in $aclPermissionedTasks.
So it checks $corePermission,
which comes from CRM_Core_Permission::getPermission(),
which calls CRM_Core_Permission_Drupal::getPermission(),
which checks self::$_editPermission,
which is set to true in self::group() if the user has 'edit all contacts' permission (or, IIUC, if they have ACL edit rights for any group).

So CRM_Core_Permission_Drupal::getPermission() returns CRM_Core_Permission::EDIT.

So CRM_Contact_BAO_Contact::contextMenu() sets $hasAllPermissions = true and so includes the Delete Contact link.

I'm not sure what's the best way to sort this out. I wonder if there's an underlying issue that needs thinking about: the way there's a grand CRM_Core_Permission::EDIT, conferred by 'edit all contacts', which bypasses more fine-grained permission checking. Is this a hangover from days when the permissioning was simpler, and does it need re-thinking? It would be helpful if one of the core team could offer their thoughts.

Short of such a re-think, it seems to me the place to fix it is in CRM_Contact_BAO_Contact::contextMenu(). Could someone who understands these things explain the intention behind the above code snippet from that function, where it looks at $aclPermissionedTasks? It seems to be saying if the task is one that can be ACL'd, then allow access if the user has CRM_Core_Permission::EDIT.

Ah, just had a look at why the Delete Contact form doesn't refuse the deletion and found this in CRM_Contact_Form_Task_Delete, line 94:
            if ( !CRM_Contact_BAO_Contact_Permission::allow( $cid, CRM_Core_Permission::EDIT ) ) {
                CRM_Core_Error::fatal( ts( 'You do not have permission to delete this contact. Note: you can delete contacts if you can edit them.' ) );
            }

Is that the currently intended behaviour or an oversight from before delete permission was introduced?

Dave J

Hi,

I've created issue http://issues.civicrm.org/jira/browse/CRM-8602 . Any thoughts from the core team about the above musings on how to fix?

Regards,

Dave J

Hi Lobo,

yes most / all of the permissioning code was done before the "delete permission" was added. so basically edit permission meant that the user could do everything

That's useful to know.

i'm not sure about "delete", but i think u can only "delete" if you have edit permissions and the delete permission.

In my testing, 'access CiviCRM', 'add contacts', 'edit all contacts' and 'view all contacts' are sufficient to allow you to delete contacts - see description in CRM-8602.

For now, do u want to look at contextMenu and make a short term fix to it? at some point, we will need to refactor and clean the permissioning system

I'll try to find time - we're short-staffed this week and then I'm off-grid for ten days!

Thanks,

Dave J

lobo, dave and kurund,
thank you very much for helping to get this fixed. it is much appreciated.

cynthia