Discussion / clarification on Api4 permissions
I've been trying to grok this while working on Standalone, and it's raised a few questions for me. It looks like there are several nuances/quirks/gotchas. This is my attempt to document these. I'd really appreciate anyone chipping in to improve this understanding. I'll close this issue off once I understand things, hopefully after having opened other issues/PRs including for the docs.
See: Developer docs on security
API access is split between Coarse and Fine grained levels.
-
Coarse is:
action::permissions()
,api::authorize
,AuthorizeEvent
,PermissionCheckSubscriber
... -
Fine is:
CoreUtil::checkAccessRecord
,_checkAccess
,AuthorizeRecordEvent
Coarse level control is at the entity.action level only is handled by the
API\Kernel->authorize()
method, which dispatches an AuthorizeEvent
over
civi.api.authorize
.
-
Api4/Event/Subscriber/PermissionCheckSubscriber
subscribesW_LATE
and authorizes the event: where checkPermissions is false or where the action is getLinks, or if$apiRequest->isAuthorized()
returns true. This generally is handled by code inAbstractAction
which finds the permissions needed (getPermissions()
) finds a match for the action name in the array returned by the Api Action classes’permissions()
method) and checks them with\CRM_Core_Permission::check();
Gotcha: the
save
action, if unspecified in the action'spermissions()
method gets defaults fromcreate
(if specified) rather thandefault
. -
there are other listeners too, (e.g.
AdhocProvider
(api4) andAPI/Subscriber/PermissionCheck
which is mostly api3 but it also authorizes requests for api4 wherecheckPermissions
has been set false, duplicating the same logic above.)
Fine level control (at least for DAO entites) is at an Action-and-Record level control (termed ACLs) rely on a single method on the entity's BAO:
public static function _checkAccess(string $entityName, string $action, array $record, ?int $userID): bool
-
These methods are used by
\Civi\Api4\Utils\CoreUtil::checkAccessRecord
-
This only calls the BAO's
_checkAccess()
for actions that do not extendAbstractGetAction
; intended to be write actions, but could be others. When it is called it must return TRUE for OK. A FALSE value will cause an exception. -
_checkAccess
is not called if a listener to\Civi\Api4\Event\AuthorizeRecordEvent
authorizes or explicitly declares the event unauthorized. -
_checkAccess
must only check data as follows: entity and action name and nothing else from$apiRequest
. Data in$record
(see below for what this contains). It may identify the record's primary key ID and fetch data from the database to consider, too.
-
-
checkAccessRecord
is used:-
AbstractUpdateAction::_run
calls it per row to be updated -
DAODeleteAction::_run
calls it per row. -
BasicBatchAction::_run
calls it per row. -
AbstractCreateAction::validateValues
calls it. (beforeValidateValuesEvent
)
-
$record
for Delete
This is [$idFieldName => $id]
only.
$record
for Create
validateValues calls _checkAccess
with data that has been pre processed via: formatWriteValues » fillDefaults
$record
for Update
- formatWriteValues
- some special code to convert passing a primary key value in values to a WHERE clause, and to rule out the possiblity of changing an existing primary key
- For single row updates:
- populate only the row's id field (from the where)
- calls
_checkAccess
with the id field + input (after formatWriteValues)
- For batch updates:
- getBatchRecords
- each row, we take the input (after formatWriteValues) and fill from
existing row data. Then call
_checkAccess
.
-
then call validateValues() which dispatches a ValidateValuesEvent
over
civi.api4.validate
$record
for Save
- Loops rows
- applies defaults passed in to this specific row
formatWriteValues
-
matchExisting
(presumably sets PKs?) - if a create, fills defaults
- calls
validateValues
which loops records and for each callscheckAccessDelegated
with either 'create' or 'update'-
checkAccessDelegated
creates a new API request of the same type and callsauthorize
, as a way to check permissions for the entity.action pair. (inside loop!) - it then calls
_checkAccess
for the record with this new empty api request and the record as a separate array. i.e. the$apiRequest
handled by_checkAccess
only contains the entity name and action name.
-
Questions
-
There's a couple of places where I think we could reduce work that is unnecessarily repeated in a loop.
-
AbstractSaveAction::validateValues
callscheckAccessDelegated
which in turn callsauthorize()
on a blank API call every time. The answer to this won't change during the loop; there may be one answer for 'create' and one for 'update'. -
Generic\BasicBatchAction
(minor): move if(checkPermissions) to outside the loop and cache the logged in user outside the loop too.
-
-
_checkAccess
is only called for actions that don't extend the AbstractGetAction. This could potentially be confusing for other reporting actions that don't extend this (maybe they should?). I've seen quite a few examples where specific cases return FALSE and at the end there's a default return TRUE. I'm not suggesting there's a security problem, but this doesn't feel "security-first", i.e. it's a blanket allow with some exceptions rather than a blanket deny with allow being explicitly checked. -
It feels weird that we start from an entity-action-specific class, then call a wider entity-specific method, passing in the name of the action, meaning that the logic then needs to disaggregate the action-specifics. It also feels weird that
_checkAccess
is in the BAO, though it's only called by Api4 code. Wouldn't it be better to have_checkAccess
in the action classes? -
$record
passed as part of update actions is inconsistent. It contains full original data with updated field values replaced for batch updates, but only the updated field values and primary key for single updates. Later, validateValues is called with aCRM_Utils_LazyArray
that would reload this data (in the case of a batch update). TheValidateValuesEvent->records
only contains the updated values. So three possible different sets of data. -
A comment in the
ValidateValuesEvent
notes that it's expensive to lookup the original values (because it would be one query per record), so avoid if possible.The lazy array returns only the update values as 'new', not the merged array (this is the same as in its records property). There is duplication for an update action since these are already present on the event under 'records' and are shared between all rows. However for a save action, there could be per-row specific updates.
Q. where we've loaded the data (e.g. batch update) couldn't we make that available to save the lazy function doing a one-query-per-row?