Avoid permission checking on getOrganizationNames

[francescbassas]

Overview

see related issue Bookkeeping Transactions Report insufficient permissions

Before

Users with contribution permissions can't visualize the Bookeeping Transactions Report

After

Users with contribution permissions can visualize the Bookeeping Transactions Report

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

Yep - per previous discussion I agree this is the right fix

[JoeMurray]

I think this change breaks the usability for organizations that have multiple sets of books for which different staff are permissioned to view data.

[eileenmcnaughton]

@JoeMurray I can't see any evidence that any nuance of organisations in the changed call...

[JoeMurray]

Perhaps I'm wrong.

I haven't traced through the permissions involved in https://github.com/civicrm/civicrm-core/blob/master/CRM/Financial/BAO/FinancialAccount.php#L468. My guess was that people could have made a contact in org1 not able to view org2 record, and then they wouldn't be able to see the accounts for org2. I guess alternatively they could use ACLs on the financial accounts directly. But even in that case I wouldn't want to override the permissions and let people see the accounts for other orgs, and doing it in this report would let them see all of the related bookkeeping entries which is really what should be hidden between the orgs. The main points of associating financial accounts with organizations is to allow multiple organizations to have separate books in civi, and that often involves a desire to keep things from being viewable by all the other orgs.

[eileenmcnaughton]

@JoeMurray I think the functionality you want is not in play from what I can see - but the issue is that people who have the ability to view contributions should be able to view this report. They shouldn't need to have some greater permission for the report to load

It's not clear to me that for those who DO have administer civicrm any filtering is going on

However, we can hang off on it for a few days so you can get @monishdeb to dig in if you want & write any tests to lock in any required behaviour

[francescbassas]

Thanks for reviewing this. For your comments I undersand that if there is no affectation for this PR the solutions is enough. Otherwise we need to return to the first PR #13159 to define FinancialAccount permissions.

[eileenmcnaughton]

Yeah - let's see if @JoeMurray is able to get @monishdeb to dig into it to check

[monishdeb]

@francescbassas @JoeMurray @eileenmcnaughton

I did some investigation and found that getOrganizationNames() was introduced in this PR #10112. Back then, this was implemented in core to limit FA owners in these two BK (Bookeeping report) filters, based on ACL permission. In core, this function is only used for BK. Other then that, it is used in easybatch extension here

I have confirmed that ACL permissions are the only ones which have an effect on filtering FA organizations in that option list, as I went deep into the code where the permission clause is built https://github.com/civicrm/civicrm-core/blob/master/CRM/ACL/BAO/ACL.php#L732 So without ACL, the rest of the user who has to administer/view contribution permission can see all the FA orgs w/o this patch.

Now as per this patch, ACL permssions won't have any affect on those options and thus show all FA owners instead of showing only those orgs, the logged in user is meant to see, which is incorrect.

[JoeMurray]

Reviewing the Financial ACL Report extension, it will properly filter ACLs for financial accounts for the bookkeeping report if it is installed. No change is required.

Although I had a concern that the PR will cause sites with ACLs to show unpermissioned objects, I've since realized that if they don't have the Financial ACL Report extension installed and they leave the affected filters blank they would get unpermissioned content, eg staff of one org would be able to see sensitive financial information about another that the ACLs had been setup to prevent. Allowing them to filter on content just for the unpermissioned org won't change that. So I'm no longer opposed to the PR. Let's merge.

[JoeMurray]

@monishdeb can you make any needed updates to Easy Batch extension and make a new release before next Wednesday when this change will come out in the RC? Thanks.

[eileenmcnaughton]

thanks for following through @JoeMurray

[JoeMurray]

:)

[francescbassas]

Thanks all!