Skip to content

CRM-16711 - make CRM_Contact_DAO_Relationship::makeURLClause ACL-aware. #6136

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

Closed
wants to merge 1 commit into from

Conversation

davejenx
Copy link
Contributor

@davejenx davejenx commented Jul 3, 2015

As described in CRM-9335 and CRM-16711, a contact lacking 'view all contacts' permission can view basic contact details of any contact they're related to, via the relationships tab, even if they're not permitted to view those contacts. This change applies ACL permissioning to the query used to retrieve relationships, so that a contact will only see relationships to contacts they're permitted to view.

As described in CRM-9335 and CRM-16711, a contact lacking 'view all contacts' permission can view basic contact details of any contact they're related to, via the relationships tab, even if they're not permitted to view those contacts. This change applies ACL permissioning to the query used to retrieve relationships, so that a contact will only see relationships to contacts they're permitted to view.
@davejenx
Copy link
Contributor Author

davejenx commented Jul 3, 2015

This change needs review. It has been in use on one site for over 3 years with no problems reported but I think the following points need consideration.

  • Is the change too generic: will it restrict queries in cases where they should not be restricted? Might we need a check_permissions flag?
  • Query efficiency: I used a subquery because this allowed me to use the results of CRM_ACL_API::whereClause / CRM_Contact_BAO_Query::fromClause . But subqueries aren't very efficient in MySQL.
  • Testing: Lobo suggested in forum discussion of this issue in 2011, http://forum.civicrm.org/index.php/topic,22007.msg92674.html#msg92674, that "there might be some ACL infrastructure stuff to add to tests". I'm not sure where this is up to now.

@monishdeb
Copy link
Member

@davejenx I have found another way to tackle 1 and 2 points . There is no doubt that the current PR changes will work but I found an alternative - #6182 which provides -

  1. avoids subquery usage
  2. consider ACLware permissions also with other permission checking
    Can you please verify my changes if it works for you ? :)

@monishdeb monishdeb closed this Jul 10, 2015
@monishdeb
Copy link
Member

Closed in favor of #6182

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants