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

CRM-16836 - make Basic Search form group select respect ACLs #11013

Merged
merged 1 commit into from
Sep 23, 2017

Conversation

davejenx
Copy link
Contributor

Overview

See full description at CRM-16836 Basic Search form group select does not respect ACLs.
The group selector on the Basic Search form is showing all groups, for an ACL'd user who should only be able to see a restricted set of groups.
This was a regression between 4.4.14 and 4.6.4

Before

For an ACL'd user who should only be able to see a restricted set of groups, the group selector on the Basic Search form shows all groups.
screen shot 2017-09-21 at 13 23 27

After

For an ACL'd user who should only be able to see a restricted set of groups, the group selector on the Basic Search form shows only the groups they are permitted to see.
screen shot 2017-09-21 at 13 21 41

Technical Details

This is fixing a regression. The simplest way to fix was to revert the relevant part of the commit that caused the issue, but discussion on JIRA seemed to favour retaining the change from
$this->add('select', 'group', ...
to
$this->addSelect('group', ...
So I followed @eileenmcnaughton's suggestion to specify 'options' in the call to addSelect(). I populated options using the same CRM_Contact_BAO_Group::getGroupsHierarchy() call that is used in Advanced Search, which respects ACLs.

Comments

See discussion in comments on JIRA.

@seamuslee001
Copy link
Contributor

Jenkins re test this please

@seamuslee001
Copy link
Contributor

I masquaraded as an ACLed user on AUG test box. Confirmed on basic search that originally the list of groups was not limited to what the ACLed user should see. Applying the patch limited the groups correctly

This works as expected and i approve this @eileenmcnaughton @totten @monishdeb

@seamuslee001
Copy link
Contributor

Jenkins re test this please

@eileenmcnaughton
Copy link
Contributor

If jenkins & @seamuslee001 approve it I think this is good to go. Change looks good & sensible. Seamus & Dave are both known to have dealt with group performance issues so I put weight on their testing from that point of view

@seamuslee001
Copy link
Contributor

@eileenmcnaughton Jenkins has passed

@eileenmcnaughton eileenmcnaughton merged commit ba31f43 into civicrm:master Sep 23, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants