Introduce civi.api4.authorizeRecord and civi.api4.validate #20533
Conversation
The original form (`getBatchRecords()`) still returns an array of items. The alternate form (`getBatchAction()`) returns an API call for fetching the batchs - but this API call may be further refined (e.g. selecting different fields or data-pages).
…l pas Context: There were three separate, concurrent PRs - two added more tests and events to APIv4, and the third added a new entity (FinancialItem). FinancialItem got merged first. I'm working reconciling the other two... and discovered that `FinancialItem` isn't passing. Problem: When the `ConformanceTest` creates a `FinancialItem`, it doesn't fill in valid values for `entity_table,entity_id`. These values are important to the access-control criteria used in reading-back data.
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.
The primary purpose of this is to provide a trait (`AuthorizedTrait`) to describe the common semantics of of coarse-grained authorization check and the upcoming fine-grained authorization check. The extracted trait makes a few small changes: * Change the default value from `FALSE` to `NULL`. In grepping universe for consumers of `isAuthorized(0`, I could only find consumers that used bool-ish values. So this should be the same for them. However, for future cases, it will allow some distinction between NULL/FALSE. * Use more type-hints. The type should be nullable-boolean. * Mutators should be amenable to fluent style (e.g. `$event->authorize()->stopPropagation()`).
|
(Standard links)
|
|
188 test fails eg |
a9e0712 to
eccc78b
Compare
|
@eileenmcnaughton Yeah, I think that failure (which recurred in several places) was due to the (lack of) fallback behavior in scenarios where there was no BAO class. Several other failures were due to a copy-paste-typo that I made a few minutes before pushing up. Preliminary local test-run look good for a dozen relevant test-classes. Jenkins is now doing a new full run. 🤞 |
|
hmm closer - but still 3 fails - this test should be handled in but it seems the code has not yet been moved to the extension & is sitting here civicrm-core/api/v3/Contribution.php Line 210 in 5f31e5f api_v3_FinancialTypeACLTest::testDeleteACLContribution ) Failed asserting that 0 matches expected 1. /home/jenkins/bknix-dfl/build/core-20533-3e0yp/web/sites/all/modules/civicrm/Civi/Test/Api3TestTrait.php:86 |
eccc78b to
3cb1873
Compare
Yeah, Coleman had already put code for this in the extension. I changed it from a hook-listener to Symfony-listener, but the way I registered it was a little flaky. (It would only work during a solo test-run - not during a full test run.) Switching the registration mechanism has fixed this locally. (It now does |
|
@totten so in terms of moving that code out of v3 api into the extension nothing is changed by this ? |
|
@eileenmcnaughton The incantations are changed, but the structure is the same as Coleman's prior PR. So if (!empty($params['check_permissions']) && !\Civi\Api4\Utils\CoreUtil::checkAccessDelegated('Contribution', 'delete', ['id' => $contributionID], CRM_Core_Session::getLoggedInContactID())) {which lands in I suppose one might call it a quirk that an APIv3 implementation calls a utility from APIv4-land, though I don't think anything is lost in doing so. If it seems problematic for some reason, then we just need to rename |
|
@totten yeah - I guess my feeling is that line really shouldn't be in the v3 api - ie financialacls are supposed to being transformed into 'just another hook-implemented acl' - the change you cite makes it MORE generic - but it's still tied to contribution.api as opposed to 'any entity might do this' (This discussion is non-blocking on the main effort) |
|
On acls - I think the financial permissions acls work the opposite way to the rest - ie they reduce other-wise given permissions - ug |
Technically, there is an inheritable contract-change here - modifying `isAuthorized()` to accept the current user ID. However, I grepped universe for references: ``` [bknix-min:~/bknix/build/universe] grep -ri isAuthorized $( find -name Civi ) ``` And all references were internal to `civicrm-core.git`. This makes some sense, given the available alternative extension-points (`Civi\Api4\$ENTITY::permissions()` and `civi.api.authorize`).
…sDelegate. Code paths: * Before: There are many callers to `$bao::checkAccess()`. * After: There is only one caller to `$bao::checkAccess()` (ie `CoreUtil`). Delegation mechanics: * Before: When delegating access-control to another entity, various things invoke `$bao::checkAccess()`. * After: When delegating access-control to another entity, various things invoke `CoreUtil::checkAccessDelegated()`
1. This removes the special-case where `CustomValue::checkAccess()` needs an extra parameter to identify the target entity. 2. This lines things up to do the swap from `_checkAccess()` to a hook/event listener
This change invovles a few things: 1. Pass the `AbstractAction $apiRequest` instead of the tuple `string $entity, string $action`. 2. There are a couple cases where we don't actually want to re-use the current `$apiRequest`. Switch these using `checkAccessDelegated()`. 3. Always resolve the userID before calling `checkAccessRecord()`. `$userID===null` can mean two different things (ie "active user" vs "anonymous user"). By resolving this once before we do any work with `checkAccess()`, we ensure that it will consistently mean "anonymous user" (even if there are multiple rounds of delegation). 3. Change the name from `checkAccess()` to `checkAccessRecord`. There are a few flavors of `...checkAccess...`, and this makes it easier to differentiate when skimming.
…e `$granted=NULL`.
Regarding invocations:
* Before: There are three different ways `Hook::checkAccess()` may be invoked, e.g.
* `CRM_Core_DAO::checkAccess()`, which sprinkles in a call to `static::_checkAccess()` before `Hook::checkAccess()`
* `CRM_Core_BAO_CustomValue::checkAccess()`, which sprinkles in a call to `checkAccessDelegated()` after `Hook::checkAccess()`
* `CoreUtil::checkAccessRecord()`, which delegates to one of the above (if appropriate) or else calls `Hook::checkAccess()`
* `CoreUtil::checkAccessRecord()` is the most general entry-point
* After: There is one way to invoke `Hook::checkAccess()`, and it incorporates some qausi/unofficial listeners.
* `CoreUtil::checkAccessRecord()` is still the most general entry-point.
* `CoreUtil::checkAccessRecord()` fires `Hook::checkAccess()` unconditionally
* `CoreUtil::checkAccessRecord()` calls `CRM_Core_DAO::checkAccess()` and/or `CRM_Core_BAO_CustomValue::_checkAccess()`,
which are now quasi/unofficial listeners for the hook
Regarding initialization and passing of `$granted`:
* Before: The value of `$granted` defaults to `TRUE`. Listeners may flip between `TRUE`/`FALSE`. The value of `$granted` is passed to each listener.
* After: The value of `$granted` defaults to `NULL`. Listeners may flip to `TRUE`/`FALSE`. If it remains `NULL` until the end, then it's treated as `TRUE`.
The value of `$granted` is not passed to each listener.
* Comment: IMHO, this is an overall simplification. If you pass in `$granted`, then each listener has to decide
whether/how to mix the inputted value with its own decision. (Ex: Should it be `return $grantedInput &&
$myGrantedDecision` or `return $grantedInput || $myGrantedDecision` or `return $myGrantedDecision`? That choice appears to be
carefully informed by the context of what steps ran before.) In the updated protocol, each `_checkAccess()` a smaller scope.
3cb1873 to
af4cccf
Compare
|
a) No kidding 🤕 b) My only fear is that it's not so simple -- more pre-existing/unarticulated issues may intervene and prompt us to pivot to a different approach. (An alternative approach - which has been sketched by Coleman before - is to temporarily override the global contact ID - and then set it back.) c) Right. It's that interplay between things like (i)
Well... it passes any existing tests... FWIW, this revision can distinguish between 'explicitly authorized' ( -- I was curious to see: When, if ever, does the system currently do permission-checks on behalf of other users (who are not presently interacting with the app)? Such use-cases ought to use
Maybe the ACL cache example is salient? Suppose you had a background-script proactively populating the ACL cache for staff-users - and the ACLs depended on smart-groups. The smart-groups are built from APIv4 queries. It would be important for all the permissions to be resolved. OTOH, for ACL cache, you'd probably want to just do the -- Speaking of ACLs - there are ACLs for CustomGroups. Is |
|
Re customGroups - I think the acls have traditionally done this really weirdly - ie our favourity function 'getTree' sets permissions to true (often inappropriately) and returns metadata without acl-limited custom fields - I suspect that is behind some of the test fails in this PR #20531 My head still hurts - but I'm not sure if any of the edge cases invalidate the approach here |
|
Eileen raises a good point about gatekeeper vs fine-grained checks - for most entities the latter overrides the former, which means we should not be doing gatekeeer checks in the api at all for any entity which supports acls. |
…ture update. Context: AuthorizeEvent did not allow tracking userID. AuthorizeRecordEvent is spec'd to track userID. This is a step toward supporting checks when the target user is non-present (ie not the user in the browser/session). However, this step is not *sufficient* - additional work is also needed to support non-present users. Original: AuthorizeEvent and AbstractAction::isAuthorized did not report current userID. However, the wiring for AuthorizeRecordEvent is spec'd to allow userID. Previous: Made a breaking change in the signature of AuthorizeEvent/AbstractAction::isAuthorized() to report userID. However, even with the break, it's not clear if this is the best approach. Revised: * Both AuthorizeEvent and AuthorizeRecordEvent report `userID`. This allows consumers to start using this information -- laying the groundwork for future changes. * If an existing event-consumer ignores the `userID`, it will still work as correctly as before. This is because we guarantee that the userID matches the session-user. * The signature of `AbstractAction::isAuthorized()` matches its original. No BC break. However, the method is flagged `@internal` to warn about the prospect of future changes. * In the future, after we do more legwork on to ensure that the overall system makes sense, we may flip this and start doing non-present users.
45f7c0f to
70da392
Compare
|
@colemanw I've pushed up both changes. I'll work on the description tomorrow - but it should be testable now. |
6de9c39 to
b87406e
Compare
Before: Reports that access is available based one delegated-check for `Contact.update` After: Reports that access is available based on multiple checks: 1. The user must have access to the relevant CustomGroup (by way of ACL or perms) 2. The user must have acces to the underlying entity (by way of checkAccessDelgated) Comments: I did a bit of testing with `Custom_*.get`, and it does seem to give access to single-value CustomGroups. So I removed a comment about multi-value CustomGroups and expanded to a larger list of entities.
|
This is looking good and is passing tests. Feels safe to merge at this point & we can hammer on it during the RC. |
|
@totten it is merged - let the great branching commence |
Overview
Combines prior PRs. Refactor so that events use more consistent style (ie
civi.api4.{$TASK}with aliascivi.api4.{$TASK}::{$ENTITY}). Description TODO.A brief description of the pull request. Keep technical jargon to a minimum. Hyperlink relevant discussions.
Before
What is the old user-interface or technical-contract (as appropriate)?
For optimal clarity, include a concrete example such as a screenshot, GIF (LICEcap, SilentCast), or code-snippet.
After
What changed? What is new old user-interface or technical-contract?
For optimal clarity, include a concrete example such as a screenshot, GIF (LICEcap, SilentCast), or code-snippet.
Technical Details
If the PR involves technical details/changes/considerations which would not be manifest to a casual developer skimming the above sections, please describe the details here.
Comments
Anything else you would like the reviewer to note