dev/financial#50 - Fix contributions and participants getting overwritten

[pfigel]

Overview

This fixes an issue where contributions or participants may be overwritten when users open an existing contribution or participant after opening the creation form (but before actually submitting it).

Before

An existing contribution or participant is overwritten if it's been opened after the creation form was loaded.

After

A new contribution or participant is recorded.

Technical Details

The form passes a reference to itself when calling CRM_Utils_Request::retrieve, which eventually leads to the ID being loaded from $_SESSION (via CRM_Core_Controller::get()).

Some forms bypass this behaviour by not loading the ID for CRM_Core_Action::ADD (or only for CRM_Core_Action::UPDATE), but I could not find any issues with this approach (i.e. not passing $this).

https://lab.civicrm.org/dev/financial/issues/50

[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).

[pradpnayak]

I get fatal error when trying to save contribution opened in new tab

[pfigel]

@pradpnayak Thanks, nice catch! Digging a bit deeper, it seems that unlike the AJAX form, this one does not pass the contribution ID as a GET/POST parameter, thus triggering the fallback to session. Naturally, that also means the standalone edit form suffers from the same problem - if you open more than one, values from the form submission will be applied to the one you opened last.

I'll check if I can make the standalone form behave like the AJAX form.

[pfigel]

Jenkins test this please

[pfigel]

8b92924 seems to have addressed the issue @pradpnayak found.

The approach used here (passing the record ID explicitly):

• Is what most web frameworks would use (in some variation)
• Is not used by Civi consistently, but is used in many places, e.g. in the AJAX form for contributions

With that in mind, there's some remaining uncertainty on whether this would break in unexpected places, and test coverage is not great for forms, to say the least. I wasn't able to uncover further issues during r-run, FWIW.

[eileenmcnaughton]

@mattwire @colemanw @seamuslee001 @xurizaemon this looks promising & addresses an ongoing annoyance - can anyone see any reason for caution here?

[mattwire]

I've not tested but the principal here is good. I've come up against issues when writing custom stuff and the ID of what you're trying to work with can sometimes be nearly impossible to retrieve! If the ID changes the form would be reloaded so I can't see any issues with this.

[pfigel]

Jenkins test this please

[eileenmcnaughton]

test this please

[eileenmcnaughton]

@pfigel just sitting with @colemanw now & discussing this & this seems to make sense but there could be security implications - ie could someone alter id in firebug & access a contribution that they would otherwise be blocked from by the financial acl check - ie

    if (CRM_Financial_BAO_FinancialType::isACLFinancialTypeStatus()
      && $this->_action & CRM_Core_Action::UPDATE
      && CRM_Utils_Array::value('financial_type_id', $this->_values)
    ) {
      $financialTypeID = CRM_Contribute_PseudoConstant::financialType($this->_values['financial_type_id']);
      CRM_Financial_BAO_FinancialType::checkPermissionedLineItems($this->_id, 'edit');
      if (!CRM_Core_Permission::check('edit contributions of type ' . $financialTypeID)) {
        CRM_Core_Error::fatal(ts('You do not have permission to access this page.'));
      }
    }

[pfigel]

@eileenmcnaughton @colemanw so I tried to reproduce this and I think the check in CRM_Contribute_Form_Contribution::buildQuickForm still works as intended. I tried the following:

• Opening the "View" link of a contribution I shouldn't have access to: no access
• Opening the "Edit" link of a contribution I shouldn't have access to: no access
• Opening the "Edit" link of a contribution I have access to and then changing the "id" input value to a contribution I shouldn't have access to: no access

On the bright side, this got me to do some testing with deleting contributions, and that seems to have regressed with this change. I'll continue tomorrow.

[pfigel]

@eileenmcnaughton @colemanw issue with deletes was resolved. If someone could double-check my findings for financial ACLs (haven't really used them before), I think this would be ready.

[JoeMurray]

@monishdeb please review.

[eileenmcnaughton]

I also had a go at circumventing the acl by going into the edit screen of a contribution I had access to & setting cj('#id').val(15) in firefox and that seemed to be ignored in post processing

@xurizaemon opening the hacking floor to you :-) , also @monishdeb will you have a chance to try? @pradpnayak

Note the test config is a bit weird - you need to enable financial acls on the contribution settings screen and then the limited user needs to have edit contributions + edit contributions of a particular financial type in the drupal screen

[eileenmcnaughton]

In absence of further input (I've tried a few places) I'm going to merge this in time for the rc

[eileenmcnaughton]

@pfigel ok I've found a regression from this - it's pretty kooky https://lab.civicrm.org/dev/report/issues/16 - question is should we revert this out of 5.16 rc & bring back into master to give us more time given the crazy ugliness of it

[eileenmcnaughton]

& a PR for the regression - #14732 (but I'm running scared now)

[JKingsnorth]

Hiah, I'm just trying to follow along the trail with this - it looks like the participant element of this has NOT been fixed yet then? Is there an open issue/PR for this bit of it?

[eileenmcnaughton]

@JKingsnorth you are right - we found a regression on the participant side & backed it out - I'd say it's a fixable but probably requires a bit of retrofitting sanity onto the code to get past the are which regressed

[JKingsnorth]

Created a follow up issue on gitlab for the event registration part then: https://lab.civicrm.org/dev/core/issues/1164