Add checkAccess API & BAO functions + hook_civicrm_checkAccess #20170
Conversation
|
(Standard links)
|
|
Yeah I am thinking about my previous approach - I guess that although we can do WHERE on the update & delete we actually call each one individually. As long as we are not having to decide about too many edit links at once then checking 1 by one is OK |
8fbdefe to
20af3ca
Compare
CRM/Core/DAO.php
Outdated
| * @param array $record | ||
| * Array of all known values for the record | ||
| * @param null $contactID | ||
| * User id |
There was a problem hiding this comment.
I think this should be clarified as the contact is whose permissions are being checked (otherwise on some api it could be the contact who we want to know about permission to)
|
This all looks good & I think we have test cover in the changes to the financialacls code I kinda wonder about performance - an extra hook call every create/update but I guess we already do the pre & post hook & it has caching but @totten do you have any thoughts on whether that should be a consideration? I'll give it a final r-run if we confirm this & you also fix the one minor code comment above |
|
On the performance front... obviously, it's hard to say authoritative things without usage-profiles/benchmarks. My gut says:
It does seem that this arrangement allows you to have one implementation of
In theory, this loses the ability to customize |
|
That's a good suggestion @totten - while we're at it, the |
264bd04 to
9a6f44a
Compare
|
@colemanw I think test failures relate here |
3749a8e to
8c39d4a
Compare
|
This is now passing tests and also increases coverage by removing the early return in |
|
@jackrabbithanna FYI this might be useful to you once merged. The immediate use-case is to determine whether to permit edit-in-place on search results |
|
@eileenmcnaughton I've been watching this. Definitely thinking about its application for access control discussion https://github.com/eileenmcnaughton/civicrm_entity/issues/296 |
|
@colemanw, a few things:
|
|
Trying to get my head around the code, I made some notes which may be useful for anyone reading this:
|
|
@colemanw @totten so status is that the unit test fails need addressing & this is more or less agreed? I have come across a couple of things that might be able to leverage this but might require more thought
|
6545797 to
b2ee53f
Compare
|
I think I fixed the test. |
|
nope |
|
Jenkins couldn't apply the patch. I squashed a couple commits let's see if that helps. |
|
jenkins, test this please |
This adds a static ::checkAccess function to all BAOs, which dispatches to a protected _checkAccess function in that BAO, as well as a new hook: hook_civicrm_checkAccess($entity, $action, $record, $contactID, &$granted)
Call checkAccess action before creating, updating or deleting
…hone, etc.) Implements the _checkAccess BAO callback for contacts and the related entities listed in _civicrm_api3_check_edit_permissions. Switch APIv4 to stop using _civicrm_api3_check_edit_permissions now that the checks are implemented in the BAO. Also fixes a couple permission check functions to respect $userID variable.
…check access The UFJoin test was failing because Get was returning no results due to an empty entity_table field. The field is optional so ACLs shouldn't be based on it. Updates conformanceTests to ensure hook permissions apply
|
Replaced by #20533 |
Overview
This combines work done by @eileenmcnaughton in #20035 with WIP from #19837 to make a checkAccess function that works at both the APIv4 and BAO layers. It standardizes on api-style entity & action names, and provides hooks the opportunity to escalate permissions or deny access on a per-record basis.