dev/core#2486 - Use read-only permissions for LineItem, EntityFinancialTrxn and FinancialTrxn APIv4

[monishdeb]

Overview

Added read-only permissions for LineItem, EntityFinancialTrxn and FinancialTrxn API

Before

CRUD actions are available for financial entities in APIv4

After

Added read-only permissions for LineItem, EntityFinancialTrxn and FinancialTrxn APIv4

Technical Details

The changes only affect APIv4 not APIv3 in order to ensure no breakage in internal code/extensions

Comments

This is a followup to #20499

ping @colemanw @eileenmcnaughton @JoeMurray @totten

[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]

Hmm When we have a payment api (which we don't yet in v4) we will be permitting create.payment fairly widely - it's the financial_trxn we will limit ability to do a create on

[monishdeb]

yeah, and payment API would be one of those abstract entity which got its own set of CRUD and in terms of permission it has to rely on one of the financial entity (here FinancialTrxn). Recently I am working on another abstract entity - Attachment (PR #20494) which could be useful for implementing (financial) abstract entities like Payment, Order etc.

[eileenmcnaughton]

@monishdeb yeah so if we add a Payment.create api it will probably have quite open permissions (make online contributions, access CiviContribute) whereas the others will be quite narrow - does that align with what you are doing here?

[monishdeb]

This will affect the api3 and API4(check_permissions = FALSE) and currently the declared payment's permission is declared as:

// Payment permissions
    $permissions['payment'] = [
      'get' => [
        'access CiviCRM',
        'access CiviContribute',
      ],
      'delete' => [
        'access CiviCRM',
        'access CiviContribute',
        'delete in CiviContribute',
      ],
      'cancel' => [
        'access CiviCRM',
        'access CiviContribute',
        'edit contributions',
      ],
      'create' => [
        'access CiviCRM',
        'access CiviContribute',
        'edit contributions',
      ],
      'default' => [
        'access CiviCRM',
        'access CiviContribute',
        'edit contributions',
      ],
    ];
    ```
 And this set of permissions are copied over to contribution_recur and now to entity_financial_trxn 

[eileenmcnaughton]

@colemanw we should ideally wrap this up pre fork too - but I'm struggling to get my head around what is right. This seems to be INCREASING who can write directly to financial entities by giving the the same permissions as 'payment' ?

[eileenmcnaughton]

@monishdeb on re-reading - the ONLY line in this PR I have an issue with is

 $permissions['entity_financial_trxn'] = $permissions['payment'];

as it is the only line that is loosening rather than tightening - do tests fail if we remove it?

[monishdeb]

nope, tests (on api3 and api4) won't be affected but APIv4 class need to updated to

class EntityFinancialTrxn extends Generic\DAOEntity {  
use Generic\Traits\EntityBridge;
...
public static function permissions() {    
- $permissions = \CRM_Core_Permission::getEntityActionPermissions()['entity_financial_trxn'] ?? []; 
+ $permissions = \CRM_Core_Permission::getEntityActionPermissions()['default'] ?? [];     
...

[colemanw]

If the goal is to remove create, update, delete, save and replace as api actions on these entities, why not do that?
The entity class can extend AbstractEntity instead of DAOEntity and only implement get and getFields.
Then we don't need to do anything with permissions.

[JoeMurray]

If the goal is to remove create, update, delete, save and replace as api actions on these entities, why not do that?
The entity class can extend AbstractEntity instead of DAOEntity and only implement get and getFields.
Then we don't need to do anything with permissions.

That sounds promising.

[colemanw]

An example of another read-only entity that does this is RelationshipCache.

[eileenmcnaughton]

@colemanw @JoeMurray my underlying preference is to have these apis but have the higher permission level on them ie

Payment.create = api that should be readily usable
FinancialTrxn.create etc = really handy to have the api for internal BAO use but would want to make it as invisible as possible outside the BAO layer. For v3 we 'just have these apis'

[JoeMurray]

My main concern is to not expose pieces of internal financial API methods that change the data. They should be internal methods because they are intimately enmeshed with other ones in ways that are easy to mess up and that have significant bad consequences. I don't have strong views on how this is done.

[eileenmcnaughton]

Note that they already ARE exposed via v3 api - we would have to make the entities create methods protected if we wanted to block people using them - at the end of the day developers do what they will do - we need to document what is supported & not let the risk of them doing something be a blocker (their responsibility)

[JoeMurray]

I would be happier to see them protected, putting a bit of an inconvenience between extension developers and their use. But I'm not going to insist. Is there a way to flag them as not recommended in unit test documentation and even APIv4 explorer??

[colemanw]

I think #20609 will resolve all the points we covered here in the discussion