dev/core#2752 Use acl, not blanket permissions on FinancialAccount, FinancialType, EntityFinancialAccount

[eileenmcnaughton]

Overview

Use acl, not blanket permissions on FinancialAccount, FinancialType, EntityFinancialAccount

https://lab.civicrm.org/dev/core/-/issues/2752

Before

API get calls for the FinancialAccount, FinancialType and EntityFinancialAccount entities fail due to the minimum permission being 'Administer CiviCRM'

After

If the contact has access CiviContribute permission that is enough for 'get'. However, the api results will be filtered by their permitted financial types. In the case of financial type this is a direct filter but in the case of financial account I have interpretted this as a filter on the associated income account

Technical Details

@seamuslee001 this is part of the issue I was pinging you about. I think this part is probably straight forward - it's the payments (the important part) that isn't so I'll leave out of scope for this PR

Comments

[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).

[eileenmcnaughton]

Initial test runs suggest that maybe we want the permissions of contribution.get OR 'Administer CiviCRM' (or 'Administer CiviCRM Data' which is implicit in Administer CiviCRM)

• since I guess lines like this suggest that we need to be careful not to reduce access

  civicrm-core/CRM/Event/BAO/Event.php

  Lines 2364 to 2366 in 0fba5cf

  	if (array_key_exists('CiviContribute', CRM_Core_Component::getEnabledComponents())) {
  	return CRM_Financial_BAO_FinancialType::getIncomeFinancialType();
  	}

I just added error handling - but not 100% sure if that is enough forgiveness

[eileenmcnaughton]

note I updated the v3 api to always set $props['check_permissions'] - the ?? will default to true if NULL but not if set to false

[eileenmcnaughton]

@seamuslee001 @colemanw @monishdeb this is passing now but I'm still trying to get my head around it.

I guess before this change

• only someone with administer civicrm could do a get on FinancialType, FinancialAccount, EntityFinancialAccess with checkPermissions = TRUE
• even with checkPermissions = TRUE not all financial types were returned for those users if financial acls are enabled

That is not consistent with our model - our model is that checkPermissions = FALSE means 'don't check permissions at all' and checkPermissions = true means 'apply both entity level and acl level permissions'

So with this change the requirement for the get is reduced to 'access CiviContribute' in order to make data available in search kit (eg. FinancialType.is_deductible) but it means that when calls to the altered entities are made with checkPermissions = FALSE then all rows will be returned (regardless of permission to that financial type).

I implemented the permissions for CRM_Financial_BAO_FinancialType::getIncomeFinancialType - largely in order to add tests & put the logic through it's paces. Most of the places the function is called from are configuration screens and I think it is reasonable to assume people on those screens have 'access CiviContribute' and that it is appropriate to call with checkPermissions = FALSE. In 2 places it is called in buildOptions and I altered that to pass permissions through - but it assumes 'unset' is TRUE. I think for apiv3 we can rely on it being set - but beyond that? I'm not sure. I'm now starting to lean towards thinking it would be safer to assume unset is FALSE

I don't have to convert getIncomeFinancialType but I feel like doing that actually helps us consolidate and flush out the questions.....

So my 2 biggest doubts right now are

1. should checkPermissions 'assume' FALSE if not set in buildOptions (Event and contribution) and
2. should the default permission be access CiviContribute or should it be ('access CiviContribute' || 'adminster CiviCRM')

[eileenmcnaughton]

@seamuslee001 did you get another chance to think about this ? I'm hoping to get this merged before the rc is cut

[colemanw]

I'm going to merge this based on code review, solid test coverage, and the fact that it's been used in production for 6 months by WMF.