Financial ACLs shouldn't restrict financial accounts by relationship type #30643
Conversation
|
🤖 Thank you for contributing to CiviCRM! ❤️ We will need to test and review this PR. 👷 Introduction for new contributors...
Quick links for reviewers...
|
|
Civi\Financialacls\FinancialAccountTest::testGetFinancialAccount /home/homer/buildkit/build/build-2/web/sites/all/modules/civicrm/ext/financialacls/tests/phpunit/Civi/Financialacls/FinancialAccountTest.php:25 |
MegaphoneJon
force-pushed
the
all-account-relationships
branch
from
b1636b1
to
9b1b087
Compare
|
@eileenmcnaughton Thanks for extracting that failure from the results. I just updated the test to match the new behavior. |
|
TL;DR this is not necessary and can lead to data leakage. I agree there is at least one problem here regarding how CheckPermissions and super-admin user overrides of ACLs/permissioning should work (see B and C below). A) Correction to overview: The current logic when Financial ALCs are enabled is that all financial accounts that are in any EntityFinancialAccount relationship with a viewable financial type are viewable (not just 'Income Account Is'). Note that there are more of these types of relationships than appear in the default tarball data: The justification for this is that enabling Financial ACLs means that there is a desire to hide certain financial information from some users. The paradigm is to only show what is explicitly enabled for viewing. If an org configures separate A/R and expense and banking expense financial accounts for each Financial Type, this allows all transactions for each Financial Type to be hidden properly without leakage. Potential leakage in terms of people viewing information that they shouldn't can occur in many ways. Avoiding leakage requires that all of the financial accounts that are in an EntityFinancialAccount relationship with an unviewable financial type should not be viewable. Bad configuration in the form of shared financial accounts between viewable and unviewable financial types can lead to leakage. For example, sharing a single Accounts Receivable Account across all financial types when creating A/R entries for all contributions would allow the A/R entries to show contribution information to people who are not permissioned to view the revenue account/contribution data. One way to avoid this is a Chart of Accounts with subaccounts for the A/R account for each of the revenue accounts, with separate CiviCRM A/R financial accounts for each. A convenient workaround if these A/R subaccounts don't exist in the external accounting system is just to configure separate A/R Financial Accounts in CiviCRM for each Financial Type, and map them all to the same A/R in the accounting system. Similarly, leakage could occur based on expense or cost of sales accounts, and can be avoided through configuration. Multiplying the A/R or expense accounts in this way leads to inconvenience and some confusion when using the core reports. But it should be possible to coalesce the data appropriately using SearchKit. I don't have a strong view about the viewability of accounts that are not in an EntityFinancialAccount relationship with any financial type. I tend to think it is safer to not have them viewable, but I won't insist. But letting financial accounts be visible so long as they are not Income Account Is accounts for unviewable financial types can lead to leakage. So I'm not in favour of this PR being merged. B) When Check Permissions is false, shouldn't all data be returned? Isn't that how CheckPermissions is interpreted in other API contexts? So \Civi\Api4\FinancialAccount::get(FALSE)->execute(); should return all Financial Accounts no matter which user is calling and no matter what permissions have been set when Financial ACLs extension is enabled. This is not subject of current PR, and so it's probably best to let sleeping dogs lie. C) When user is super-admin, shouldn't all data be returned even if Check Permissions is true? Isn't that how super-admin is interpreted in other API contexts? So when executed by super-admin user, \Civi\Api4\FinancialAccount::get(TRUE)->execute(); should return all Financial Accounts. This is not subject of current PR, and so it's probably best to let sleeping dogs lie. Does this make sense, @MegaphoneJon and @eileenmcnaughton ? |
|
@JoeMurray I'm happy to submit an alternate PR that implements items B and C. But with regard to the rest - couldn't two financial types share the same income account? It seems the potential for data leakage would be no different. Also - what is the exact data leakage that can occur? I don't think that actual contribution data can be revealed because that's checked separately. Likewise the existence of other financial types. So the only leakage I can see is the existence of certain financial accounts? But if two Financial Types share a financial account, and I have access to one type, I should know of the existence of its financial account. |
|
@JoeMurray Do you have thoughts on my last message? I can just do the other PR you suggest but I want to make sure I'm thinking about the security correctly. |
|
Sorry for the delay. Here's response on the easy parts B and C: yes please go ahead. I will come back later to respond to your questions about A when I have a few hours to test and answer. |
Overview
Financial ACLs restrict viewing financial accounts to those with a relationship of "Income Account is". This is true even for super-admins.
So if you want to make an API call to get all, say, Expense accounts, you can't.
To replicate:
Enable Financial ACLs and run again, still as super-admin.
Before
Get 4 results.
After
Get 9 results.
Comments
Advanced mathematicians might notice that 9 != 14. There are some accounts that, by default, aren't tied to any financial type. IMO those should also be returned, but I'm not sure if that's as clear-cut. Would appreciate feedback from those with deep knowledge (e.g. @JoeMurray) to see if there's any reason why, e.g., "Premiums Inventory" or "Discounts" account types shouldn't appear in an API call when Financial ACLs are enabled.