Details
-
Type:
New Feature
-
Status: Done/Fixed
-
Priority:
Critical
-
Resolution: Fixed/Completed
-
Affects Version/s: 4.6
-
Fix Version/s: 4.7.5
-
Component/s: CiviContribute, CiviEvent, CiviMember, CiviReport, Core CiviCRM
-
Labels:
-
Documentation Required?:User and Admin Doc
-
Funding Source:Contributed Code
Description
Primary use case: hide some sensitive financial transactions, for example, major bequests, from staff who need to be able to see other financial transactions.
Technical objective: create role-based permissions to enable restricting the viewing and editing of contributions and the reporting of contribution income by financial types.
There can be several financial types associated with a contribution, eg different revenue accounts for each line item. For simplicity, a contribution will be viewable if all of the financial types it includes are viewable, and not viewable if any of them are not viewable. Similarly, to edit a contribution, all of the financial types associated with the contribution must be editable by the user.
The functionality needs to be implemented in core since an extension would need to override too many files or require the creation of too many new hooks.
Scope:
1. Configuration Settings:
- Enable / Disable creating Permissions for each Financial Type
- Prevent enabling permissions for financial types that have already been used for Memberships or Participants
- Prevent use of financial types that allow restrictions from being used for memberships or participants, either when managing pages or through backoffice create and update.
- Enforce for API CRUD operations on Financial Types.
2. Search
- Exclude viewing of Search options for financial types that are not viewable by user.
- Modify Find Contributions search to support FT permissions.
- Modify Advanced Search to support FT permissions.
- Modify Search Builder to support FT permissions.
3. Reports (12 contribution reports exposed in All Reports)
- Exclude viewing of Report options for financial types that are not viewable by user.
- Modify Find Contributions search to support FT permissions.
- These options result in hiding the existence of Financial Types from users who are are not permissioned to view contributions that include those financial types.
Core team agreed that automated testing for the new feature should focus testing on API and CRM levels, and not the hard to maintain web tests.
Attachments
Issue Links
- is supplemented by
-
CRM-17240 Financial Type must be required for Price Field Value
-
- Done/Fixed
-
- links to
Activity
dgg: josephmurray: are there any further details / mockups anywhere on the FT ACLs project (in particular the configuration UI)?
[12:17pm] dgg: also info on rationale behind excluding FT’s used for membership/participant
[12:20pm] dgg: josephmurray: also info on impacts on accounting batch UI / workflow
Attached file 2015-05-19_13-15-45.png shows new view, edit, create, delete permissions by Financial Type. This is similar to permissions by content type in Drupal.
Joe - I took a quick look on the test site (at civiacl.jmaconsulting...) - and I don't see where admin chooses to enable ACL on a given financial type (see screenshot of Edit Financial Type form). It looks like ALL existing financial types are injected into Permissions grid ???
Looking at the code, I'm also a bit concerned about performance hit of calling CRM_Financial_BAO_FinancialType::getAvailableFinancialTypes in so many queries, forms etc. Can we figure out some way to cache these and also to skip or return immediately if admins haven't enabled any FT ACLs. We really don't want to add a performance hit for the majority of sites that will not be using this.
1. Short answer: how about handling as illustrated in attached image with an Enable AC by FT checkbox on the Access Control page (Administer > Users and Permissions > Permissions (Access Control)), where one would expect to find something dealing with Permissions? For the few who want the functionality, they could turn this on and deal with profusion of permissions. Or (not my preference, and possibly more work for us) we could bury it a bit in editing of a Financial Type, which requires Administer CiviCRM Financial Types (see image 2).
2. Okay, we'll create a cache, return immediately if FT ACLs aren't present, and provide an appropriate string for permission C, R, U, or D operation by FT from cache. I'm not sure of the best way to create/update cache. Rather than store cache per user in user session (which might require us to deal with potentially long-lived user sessions whenever a change was made to these settings?), we could store a system wide cache of CRUD permissions per role. Suggestions here?
Stoob and client are wondering if this could be put into 4.6 release. On the expectation that is not allowed as it is new functionality, we'll provide them with a quote for putting this into a custom rolled 4.6 release.
RE: 1. I think enabling FT Access Control fits in CiviContribute Component Settings (screenshot attached). Would be good for you to craft some text for a help pop-up for that checkbox which includes some example use cases and refers folks to the Financial Types listing/editing to enable the property for specific FTs, and to the CMS permissions page to control access.
RE: 2. Sounds good. In terms of cache storage - best to check w/ Tim on this. I think we have some other similar 'config' objects that are cached.
RE: 4.6 - I agree this is too big a change for a point release.
How's this for help text, Dave:
Enabling Access Control by Financial Types is only required if you need users in one role to access some financial transactions but not ones with certain financial types. For example, Planned Giving bequests may be highly confidential and not appropriate for staff organizing events to see, even though they need to be able to administer the payments for ticket purchases. After enabling, you will be able to set create, view, edit, and delete permissions separately for each financial type by navigating to Administer > Users and Permissions > Permissions (Access Control), and clicking on the Access Control link for your CMS.
Looks good, thanks!
Can we get the PR rebased, help text added etc, so we can review for merge into master?
Submitted new PR at https://github.com/civicrm/civicrm-core/pull/6431
Patched my 4.7 local site w/ PR 6431 to do some manual testing.
1. The contribution query method is broken (regardless of whether ACL by Financial Type is TRUE or FALSE). Any page which uses that query object throws a fatal (e.g. Contributions > Dashboard; Find Contributions; Contact Contributions Tab ...). Details below.
2. Contribution Details report throws this Notice, only when ACL by Financial Type is TRUE:
Notice: Array to string conversion in HTML_QuickForm_select->toHtml() (line 502 of /Users/dgg/git/crm_v4.7/packages/HTML/QuickForm/select.php).
The fatal error breaks many screens / workflows - so will hold off additional manual tests until this is corrected.
=== Contribution Query Fatal ========
[message] => DB Error: no such field
[mode] => 16
[debug_info] =>
SELECT COUNT( conts.total_amount ) as total_count,
SUM( conts.total_amount ) as total_amount,
AVG( conts.total_amount ) as total_avg,
conts.total_amount as amount,
conts.currency as currency FROM (
SELECT civicrm_contribution.total_amount, COUNT(civicrm_contribution.total_amount) as civicrm_contribution_total_amount_count,
civicrm_contribution.currency FROM civicrm_contact contact_a LEFT JOIN civicrm_contribution ON civicrm_contribution.contact_id = contact_a.id WHERE ( civicrm_contribution.is_test = 0 ) AND (contact_a.is_deleted = 0) AND (contact_a.is_deleted = 0) AND civicrm_contribution.financial_type_id IN (3,1,4,2,5) AND li.id IS NULL AND civicrm_contribution.contribution_status_id = 1 GROUP BY civicrm_contribution.id
) as conts
GROUP BY currency [nativecode=1054 ** Unknown column 'li.id' in 'where clause']
Update PR (https://github.com/civicrm/civicrm-core/pull/6431) to include fixes suggested in above comment and also added function block for new function
There are ~7 test regressions related to this PR.
api_v3_ContributionPageTest.testSubmit
api_v3_ContributionPageTest.testSubmitRecurMultiProcessorInstantPayment
api_v3_ContributionPageTest.testSubmitPaymentProcessorFailure
api_v3_ContributionTest.testCreateContributionWithPaymentInstrument
api_v3_ContributionTest.testGetContributionByPaymentInstrument
api_v3_ContributionTest.testCreateUpdateContributionChangeTotal
api_v3_ContributionTest.testCreateUpdateWithoutChangingPendingStatus
I suspect you'll need to modify the tests to cover 'with ACLs' and 'without ACLs' conditions.
Please reassign to me for final QA once the test regressions are corrected (updated tests should include coverage for the ACLs-enabled condition).
Dave, we recall lowering an estimate as core was going to develop test, we think for this. However, I've gone ahead and asked Pradeep to do it.
Thanks Joe Murray - we are pretty swamped with 4.6 and 4.7 issues, so that is super helpful.
API regressions look to be fixed. Merged https://github.com/civicrm/civicrm-core/pull/6431
Reopening so that tests for new functionality can be added (per Joe Murray).
Changing priority to Critical since this feature is definitely part of 4.7 and not yet closed.
Joe,
Any updates on the tests
Pradeep, Please make this your priority for core work.
PR submitted at https://github.com/civicrm/civicrm-core/pull/7818
Draft PR for preliminary review but NOT for merging is submitted https://github.com/civicrm/civicrm-core/pull/5867