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

[wip] Use hook for checkAccess #20035

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 21 additions & 2 deletions CRM/Core/DAO.php
Original file line number Diff line number Diff line change
Expand Up @@ -2960,7 +2960,7 @@ public function addSelectWhereClause() {
$clauses[$fieldName] = CRM_Utils_SQL::mergeSubquery('Contact');
}
// Clause for an entity_table/entity_id combo
if ($fieldName == 'entity_id' && isset($fields['entity_table'])) {
if ($fieldName === 'entity_id' && isset($fields['entity_table'])) {
$relatedClauses = [];
$relatedEntities = $this->buildOptions('entity_table', 'get');
foreach ((array) $relatedEntities as $table => $ent) {
Expand Down Expand Up @@ -3007,9 +3007,28 @@ public static function getSelectWhereClause($tableAlias = NULL) {
return $clauses;
}

/**
* Check whether the given action can be performed on these values.
*
* The return is the input values array with inaccessible items filtered out.
*
* @param string $action
* @param array $values
* @param null $contactID
*
* @return array
*/
public function checkAccess(string $action, array $values, $contactID = NULL): array {
if (is_null($contactID)) {
$contactID = CRM_Core_Session::getLoggedInContactID();
}
CRM_Utils_Hook::checkAccess(CRM_Core_DAO_AllCoreTables::getBriefName(get_class($this)), $action, $contactID, $values);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eileenmcnaughton I'm guessing we may want to call core methods that already exist before we get to the hook invocation and would we want extensions to potentially override core functions e.g. result of CRM_Mailing_BAO_Mailing::acl or whatever is that function?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@seamuslee001 yeah - this is the function on the DAO class which would be overridden by various BAO classes

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eileenmcnaughton I guess in my mind at least wouldn't that mean we end up with a lot of duplicated code around the place because you would want to copy n paste a fair chunk of this function on the other DAOs?

return $values;
}

/**
* ensure database name is 'safe', i.e. only contains word characters (includes underscores)
* and dashes, and contains at least one [a-z] case insenstive.
* and dashes, and contains at least one [a-z] case insensitive.
*
* @param $database
*
Expand Down
19 changes: 19 additions & 0 deletions CRM/Utils/Hook.php
Original file line number Diff line number Diff line change
Expand Up @@ -2086,6 +2086,25 @@ public static function permission_check($permission, &$granted, $contactId) {
);
}

/**
* Check whether the given contact has access to perform the given action.
*
* If the contact cannot perform the action the
*
* @param string $entity
* @param string $action
* @param int|null $contactID
* @param array $values
*
* @return mixed
*/
public static function checkAccess(string $entity, string $action, ?int $contactID, array &$values) {
return self::singleton()->invoke(['entity', 'action', 'contactID', 'values'], $entity, $action, $contactID,
$values, self::$_nullObject, self::$_nullObject,
'civicrm_checkAccess'
);
}

/**
* Rotate the cryptographic key used in the database.
*
Expand Down
22 changes: 8 additions & 14 deletions api/v3/Contribution.php
Original file line number Diff line number Diff line change
Expand Up @@ -204,23 +204,17 @@ function _civicrm_api3_contribution_create_legacy_support_45(&$params) {
function civicrm_api3_contribution_delete($params) {

$contributionID = !empty($params['contribution_id']) ? $params['contribution_id'] : $params['id'];
// First check contribution financial type
$financialType = CRM_Core_DAO::getFieldValue('CRM_Contribute_DAO_Contribution', $contributionID, 'financial_type_id');
// Now check permissioned lineitems & permissioned contribution
if (!empty($params['check_permissions']) && CRM_Financial_BAO_FinancialType::isACLFinancialTypeStatus() &&
(
!CRM_Core_Permission::check('delete contributions of type ' . CRM_Contribute_PseudoConstant::financialType($financialType))
|| !CRM_Financial_BAO_FinancialType::checkPermissionedLineItems($contributionID, 'delete', FALSE)
)
) {
throw new API_Exception('You do not have permission to delete this contribution');
if (!empty($params['check_permissions'])) {
$contribution = new CRM_Contribute_BAO_Contribution();
$values = $contribution->checkAccess('delete', [$contributionID => ['id' => $contributionID]]);
if (empty($values)) {
throw new API_Exception('You do not have permission to delete this contribution');
}
}
if (CRM_Contribute_BAO_Contribution::deleteContribution($contributionID)) {
return civicrm_api3_create_success([$contributionID => 1]);
}
else {
throw new API_Exception('Could not delete contribution');
}
throw new API_Exception('Could not delete contribution');
}

/**
Expand Down Expand Up @@ -670,7 +664,7 @@ function _ipn_process_transaction($params, $contribution, $input, $ids) {
static $domainFromName;
static $domainFromEmail;
if (empty($domainFromEmail) && (empty($params['receipt_from_name']) || empty($params['receipt_from_email']))) {
list($domainFromName, $domainFromEmail) = CRM_Core_BAO_Domain::getNameAndEmail(TRUE);
[$domainFromName, $domainFromEmail] = CRM_Core_BAO_Domain::getNameAndEmail(TRUE);
}
$input['receipt_from_name'] = CRM_Utils_Array::value('receipt_from_name', $params, $domainFromName);
$input['receipt_from_email'] = CRM_Utils_Array::value('receipt_from_email', $params, $domainFromEmail);
Expand Down
29 changes: 29 additions & 0 deletions ext/financialacls/financialacls.php
Original file line number Diff line number Diff line change
Expand Up @@ -286,6 +286,35 @@ function financialacls_civicrm_permission(&$permissions) {
];
}

/**
* @param string $entity
* @param string $action
* @param int|null $contactID
* @param array $values
*
* @throws \CRM_Core_Exception
*/
function financialacls_civicrm_checkAccess(string $entity, string $action, ?int $contactID, array &$values) {
if (!financialacls_is_acl_limiting_enabled()) {
return;
}
if ($action === 'delete' && $entity === 'Contribution') {
foreach ($values as $index => $details) {
$contributionID = $details['id'];
// First check contribution financial type
$financialType = CRM_Core_PseudoConstant::getName('CRM_Contribute_DAO_Contribution', 'financial_type_id', CRM_Core_DAO::getFieldValue('CRM_Contribute_DAO_Contribution', $contributionID, 'financial_type_id'));
// Now check permissioned line items & permissioned contribution
if (!CRM_Core_Permission::check('delete contributions of type ' . $financialType) ||
!CRM_Financial_BAO_FinancialType::checkPermissionedLineItems($contributionID, 'delete', FALSE)
) {
unset($values[$index]);
continue;
}
}
}

}

/**
* Remove unpermitted financial types from field Options in search context.
*
Expand Down
6 changes: 4 additions & 2 deletions tests/phpunit/api/v3/FinancialTypeACLTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -291,8 +291,10 @@ public function testEditACLContribution() {

/**
* Test that acl contributions can be deleted.
*
* @throws \CRM_Core_Exception
*/
public function testDeleteACLContribution() {
public function testDeleteACLContribution(): void {
$this->enableFinancialACLs();

$this->setPermissions([
Expand All @@ -313,7 +315,7 @@ public function testDeleteACLContribution() {
$this->addFinancialAclPermissions([['delete', 'Donation']]);
$contribution = $this->callAPISuccess('Contribution', 'delete', $params);

$this->assertEquals($contribution['count'], 1);
$this->assertEquals(1, $contribution['count']);
}

public function testMembershipTypeACLFinancialTypeACL() {
Expand Down