CRM-20166: Making CVV always required for front-end pages.

[agilewarealok]

Overview

Setting "CVV required for backoffice" to NO results in it setting all front facing forms to 'not require' CVV site wide, ie on all public facing contribution pages.

Before

A single setting was there to disable/enable CVV on front office and back office pages.

After

Now we don't consider back-office setting for front-end contribution pages by making CVV always required for front-end pages.

Agileware Ref: CIVICRM-689

---

• CRM-20166: Setting CVV is 'not required for backend' affects all front facing forms

[mlutfy]

Is there a legitimate use-case for allowing the front-end forms to not validate cvv2? Seems dangerous. Folks might disable it thinking that it's more convenient, not realizing how interesting this will become for people testing stolen card numbers.

(I agree with the fix, and realize that not having a setting might cause regressions.. but we could also consider it a security issue).

[KarinG]

@mlufty - agreed - giving admins the option to
allow users to not enter CVV comes with high risk; not to mention that many payment processors will not allow no CVV via webservices; In my opinion CVV should always be required for backoffice too;

[jusfreeman]

@mlutfy @KarinG - thanks for your comments.

The situation currently in CiviCRM is that there is one setting: "CVV required for backoffice" which determines if CVV is required on:

• backoffice data entry AND
• front end processing

This does not make sense and is counter-intuitive. Why impact on front-end at all? It's not even what the field label states!

This PR separates the functionality so that:

• Backoffice - CVV can be enabled/disabled, OR
• Frontend - CVV can be enabled/disabled

There are very legitimate situations where backoffice processing requires CVV to be DISABLED. But you want the frontend CVV to still be ENABLED.

This PR provides the flexibility to determine when CVV should be required.

[KarinG]

If CiviCRM currently disables CVV on front end contribution forms when people check a box that says disable CVV on backoffice forms, then that’s a bug - and should be fixed urgently. CVV on front end should -always- be enabled as non-CVV public contribution pages are all but certainly going to be subjected to card tumbling and the org is going to risk having their merchant account closed;

[jusfreeman]

Is there a legitimate use-case for allowing the front-end forms to not validate cvv2? Seems dangerous.

@mlutfy Personally, I don't object to that idea - frontend CVV being ALWAYS enabled.

Would people in the CiviCRM community would object to that change? Possibly, because it is a change to the existing behaviour.

This is why we are suggesting making it a separate setting for frontend, so CiviCRM sites using the current CVV frontend disabled approach can still do so - for whatever reasons they have.

[jusfreeman]

If CiviCRM allows CVV on front end contribution forms to be disabled

@KarinG that's the current situation. As described in the PR overview and linked JIRA issue.

[KarinG]

Exactly - that’s the bug; let’s fix the bug & fix the security issue

[KarinG]

Here’s a reference: https://www.thebalance.com/keep-donors-safe-preventing-online-credit-card-fraud-4148333

[jusfreeman]

@KarinG We will revise this PR and re-submit to remove the setting for front-end. Making it always required. Since this is also the behaviour in CiviCRM 4.6 now, we will do a related PR for that too.

[petednz]

@jitendrapurohit can you take a look at this PR thx

[davejenx]

Agree with KarinG.

[agilewarealok]

We've updated the PR to make CVV always required for front end pages.

[jitendrapurohit]

Just tested these changes by disabling the setting on backend form and found a minor issue. To replicate -

• Disable the setting and click on Submit credit card contribution on contribution tab of any contact summary page.
• The required field marker(*) was not shown on CVV code input(correct).
• Don't enter any value for cvv and click on Save.
• Formrule error displays which need a value for cvv.

Maybe, we also need to handle the setting assigned at https://github.com/civicrm/civicrm-core/blob/master/CRM/Core/Payment.php#L689

[mattwire]

@jitendrapurohit Which payment processor? Payment processors can override validatePaymentInstrument() and I think this can make fields required even if the UI doesn't show them as required.

[jitendrapurohit]

I tested this on my local environment with a test processor.

Seems to be because getAllFields() also calls getPaymentFormFieldsMetadata() without any value for $backOffice and hence cvv is always set to required.

[jusfreeman]

@jitendrapurohit thanks for testing. Confirmed, we'll rework the PR.

[agilewarealok]

@jitendrapurohit We've updated the validatePaymentInstrument method to distinguish backoffice pages and front facing pages.

[jusfreeman]

Ping @jitendrapurohit @mattwire @KarinG

Would someone mind verifying our PR?

[mattwire]

@jusfreeman @agilewarealok On first glance I'm a little bit worried about adding $isBackOffice to all those functions. A lot of those functions can and are overridden by payments processors (particularly validatePaymentInstrument) - changing the method signatures could cause errors. Would it be possible to pass isBackOffice via one of the existing parameters - $values? @eileenmcnaughton may have a view..

[eileenmcnaughton]

At least with the functions on CRM_Core_Payment class we should not pass it in - we should use these functions

  /**
   * Set back office property.
   *
   * @param bool $isBackOffice
   */
  public function setBackOffice($isBackOffice) {
    $this->backOffice = $isBackOffice;
  }

  /**
   * Is this a back office transaction.
   *
   * @var bool
   */
  protected $backOffice = FALSE;

  /**
   * @return bool
   */
  public function isBackOffice() {
    return $this->backOffice;
  }

[agilewarealok]

@eileenmcnaughton Thanks for the hint. We can use isBackOffice directly without passing $isBackOffice as a separate parameter in functions.
We've updated the PR accordingly.

@jitendrapurohit @mattwire @KarinG Can you guys please verify this ?

[jitendrapurohit]

The use-case I mentioned works fine and the code changes looks good to me. @agilewarealok Any chances of adding a simple unit test which asserts this fix? For eg-

• Set 0/1 for cvv_backoffice_required .
• Call getPaymentFormFieldsMetadata() function
• Assert the value returned is as expected.

[mattwire]

@agilewarealok Code looks clean. +1 for a unit test if possible as this is payment related.

[eileenmcnaughton]

I've given this the merge-ready label. I think the fix is right per previous comments. I would be preferable to get a unit test, so I'm hanging off merging now in hope...

[agilewarealok]

@jitendrapurohit @mattwire @eileenmcnaughton
Thanks for the review guys, We will write UT for this. :)

[agilewarealok]

@jitendrapurohit @mattwire @eileenmcnaughton
We've written Unit test for this. Can you please verify ?

[mattwire]

@agilewarealok Unit test looks good. Just to be really picky, can you extend the unit test to check for the setting the other way too - add:
+ Civi::settings()->set('cvv_backoffice_required', 1);
which should return required for both front and backend.

[agilewarealok]

@mattwire We've updated the UnitTest to test it the other way too.

[eileenmcnaughton]

Ok I think this has met reviewer requirements and it has had solid & engaged review. I also think it's a good change, clean code, nice test. Gold star