dev/core#2486 - Use read-only permissions for FinancialItem API

[monishdeb]

Overview

Add permissions for financial_item entity

Before

No permission declared for financial_item

After

Financial Item got permissions, and prevent create/update action.

Comments

This is a follow-up to #20433.

ping @colemanw @eileenmcnaughton @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).

[colemanw]

So FinancialItems are never allowed to be created or updated?

[totten]

(For posterity: this PR+discussion is an offshoot from https://chat.civicrm.org/civicrm/pl/ey1myeenuj813qcxoi8hjwqhdh)

@colemanw Correct. I understand these to be special-purpose logging-tables. Here's a description of the financial entities from @JoeMurray: https://civicrm.stackexchange.com/questions/28197/what-is-a-line-item-and-what-is-a-financial-item-and-how-are-those-two-linked-t with a notable excerpt:

An important principle of accounting is that books should be auditable. One part of that is to retain records about all of the changes that have been made on items. So if a purchase is made with pay later selected, and two partial payments are made, the number of things purchased is changed, a refund is made, etc., then all of these are tracked in the financial tables in CiviCRM. Entries in civicrm_financial_item and civicrm_financial_trxn tables are intended to never be edited or deleted.

[eileenmcnaughton]

I'm fine with this - but I would note that we call the apiv3 version of this api from internal code - we have traditionally encouraged api use in internal code for readability & consistency reasons

[totten]

@eileenmcnaughton Yeah, it seems to me that (in this approach) you should be able to call APIv4 FinancialItem::create() internally as well -- just pass checkPermissions=>FALSE. A purer approach might remove the FinancialItem::create() and FinancialItem::update() API implementations completely, which would make it impossible. Here, it's more like, "This can only be called internally."

[eileenmcnaughton]

@totten OK that's great - ie we mostly use checkPermissions = FALSE once we are deep in the BAO

[eileenmcnaughton]

@totten I don't have any concerns about this based on the above - go ahead & merge if this makes sense to you

[eileenmcnaughton]

@totten I don't have any concerns about this based on the above - go ahead & merge if this makes sense to you

[totten]

r-run: I used these commands (before and after the patch):

cv ev -U admin 'civicrm_api4("FinancialItem","create",["checkPermissions"=>TRUE]);'
cv ev -U admin 'civicrm_api4("FinancialItem","create",["checkPermissions"=>FALSE]);'

As expected, the patch introduces an authorization-failure if you enable permissions -- and it still allows the call if you skip permissions.

Merging.

[monishdeb]

Thanks @totten for merging this.

@eileenmcnaughton should I make similar change to LineItem and EntityFinancialTrxn ?

[eileenmcnaughton]

@monishdeb yeah - probably - I think there might be others too