Revert "Add permission metadata to contact is_deleted field"

[colemanw]

Overview

Fixes https://lab.civicrm.org/dev/core/-/issues/2507 by reverting the commit from #17721.

Technical Details

This reverts commit d2ff128.

ACLs for accessing deleted contacts are already enforced in the query, so this field was being hidden for UI purposes not ACL purposes.
It had the unfortunate side-effect of crashing any API call using is_deleted in the WHERE clause for non-permissioned users.
It would be nice to hide it from SearchKit for non-permissioned users, but not mission-critical, and it's far more critical for us to prevent the crashes documented in https://lab.civicrm.org/dev/core/-/issues/2507.

[civibot]

(Standard links)

• If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[totten]

@jmcclelland Are you able to review this?

[jmcclelland]

@totten - yes, I can review. Can you confirm what commands I need to run after applying the fix? I tried:

cd xml && php GenCode.php schema/Schema.xml
cv flush

But my tests are still failing in the same way as before the fix. I wonder if I'm missing a step? Or if the fix is not working?

[totten]

@jmcclelland @colemanw Those steps sound more than sufficient to me... (Since DAOs are committed to git, I would expect a simple git checkout or git cherry-pick to be enough; but cv flush seems prudent too. )

[colemanw]

Yea the cache flush is critical because APIv4 metadata is stored in the cache.

[totten]

So then cv flush should've been enough, right? There's still some unidentified issue with why the patch didn't work?

[jmcclelland]

Yes - I'm still working on it. I'm somewhat befuddled by the mixture of permissions needed to make it fail in the first place. I plan to have either a report or a semi-intelligent question later today.

[jmcclelland]

@totten and @colemanw - I figured out my befuddlement. There is a difference in behavior between 5.39 lts and master.

WIth 5.39 lts granting or restricting the 'access deleted contacts' permission controls whether or not the error is triggered.

In master, as long as the user has 'access CiviCRM backend and API' permission, the error will not be triggered. But if the user doesn't have that permission, then the error will be triggered (regardless of whether or not they have 'access deleted contacts').

After applying the patch, I'm not seeing any difference. I'm not sure what's going on but will work on a test to try to demonstrate the behavior (or uncover something dumb I may be doing).

[jmcclelland]

Hi all,

I've finally finished the review and it's surprisingly messy for what seems like such a simple change.

r-eplain: issue: See below. Wah, the explanation is no longer true - maybe it could be updated with a preface "In previous versions" and a final sentence "Also, this change makes querying on is_deleted consitent regardless of whether you have access to deleted contacts or not.
r-user: this change will alter behavior for some users - I'm not sure if anyone is counting on the old behavior (which I think is broken), but it does change things.
r-doc pass
r-run I tested both via the UI using both search results and phone bank results to test the generation of the overlay profile. In addition, I tested with a php script containing an APIv4 called to get a Contact and running cv --user=<user> php:script /path/to/script and substituting the admin user and a user without access to deleted contacts to compare the results.
r-tech: pass
r-maint: pass
r-test: fail (but I don't think it's related)

There is some subtle change to the Api4 in master compared with version 5.39 making the original issue less urgent. Now, as long as you have "access CiviCRM" permissions, you won't get the error when you call Api4 with is_deleted, even if you do not have permission to access deleted contacts.

However, I still think this change is an important improvement.

Consider this scenario:

1. You do not have access to deleted contacts
2. There is one contact in the database that has is_deleted set to 1.
3. Your run an Api4 query with a where clause specifying is_deleted = 1 (with the intention of getting deleted contacts)
4. You pop off the first record

Before the change: You get the first record in the database that is not deleted (unexpected). That's because the is_deleted where clause seems to be silently ignored, while your restriction against seeing deleted contacts is still enforced.

After the change: You get no results (expected)

And lastly, I was a bit surprised that I could access the phone bank without "access CiviCRM" permissions simply by knowing the right URL, but I don't think this is a security concern since:

1. Access to the page is controlled by the "interview campaign contacts" permission
2. It's often granted to volunteers, so is conceivably considered more of a front end page then a back end page

[colemanw]

Thanks for the review @jmcclelland - based on your review this ought to get merged, and IMO should be merged into 5.45 so people stop getting tripped up by the unexpected behavior.