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

Don't exclude deleted contacts from ACL cache when user has permission #17379

Merged
merged 1 commit into from
May 24, 2020

Conversation

colemanw
Copy link
Member

Overview

When the user has 'access deleted contacts' permission, deleted contacts should not be excluded from the ACL cache.

Before

Deleted contacts excluded from ACL cache even when user has permission to see them.

After

Deleted contacts only excluded when user lacks permission to see them.

Comments

The code comment mentioned https://issues.civicrm.org/jira/browse/CRM-6181 which is only tangentially related, which leads me to think this was simply a coding error that never got caught.

@civibot
Copy link

civibot bot commented May 22, 2020

(Standard links)

@civibot civibot bot added the master label May 22, 2020
…mission to see them

Test failures seem to be due to assumptions of the test.
Removed via Hume's guillotine

When the user has 'access deleted contacts' permission, deleted contacts
should not be excluded from the ACL cache.
@colemanw
Copy link
Member Author

colemanw commented May 22, 2020

The only test failures were IMO due to faulty assumptions. Per Hume's guillotine, just because the SQL string does look like that, doesn't mean it ought to.

@colemanw colemanw changed the title ACL - Don't exclude deleted contacts from ACL cache when user has per… Don't exclude deleted contacts from ACL cache when user has permission May 23, 2020
@colemanw
Copy link
Member Author

@eileenmcnaughton you're probably best for reviewing this one

@eileenmcnaughton
Copy link
Contributor

This makes sense. I agree it seems like a coding error

@eileenmcnaughton eileenmcnaughton merged commit 69659ec into civicrm:master May 24, 2020
@eileenmcnaughton eileenmcnaughton deleted the del branch May 24, 2020 22:12
@demeritcowboy
Copy link
Contributor

This seems to affect basic and advanced contact search as reported at https://lab.civicrm.org/dev/core/-/issues/1864. They both always return deleted contacts now.

@colemanw
Copy link
Member Author

Ok, so we need to add the is_deleted clause back in, but in a more appropriate place.

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

Successfully merging this pull request may close these issues.

3 participants