Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CRM-21104: Forcing reCaptcha on Contribution pages(with online payments) which have no Profile associated with them. #11197

Merged
merged 3 commits into from
Jul 18, 2018

Conversation

agilewarealok
Copy link
Contributor

@agilewarealok agilewarealok commented Oct 25, 2017

Overview

CiviCRM Contribution pages which have no Profile associated with them do not include a ReCaptcha and as a result are prime targets for credit card fraud.

Before

Contribution pages which have no profile or ReCaptcha disabled profile associated with them were not showing ReCaptcha.

After

We now force ReCaptcha to show on Contribution pages if payment processor is attached with it for online payment.

Other Details

We will display a warning on contribution page if ReCaptcha settings are not filled by admin, if the setting is enabled (off by default)

Agileware Ref: CIVICRM-410


@MegaphoneJon
Copy link
Contributor

I son't think we want to force ReCAPTCHA on to donation pages. This goes against the recommendation of many online fundraising experts re: the 50-70% abandonment rate of donation checkouts. It's fine to make this optional, of course.

I recognize the need for some organizations to have this, but there are many other fraud mitigation tactics available to organizations, at the application, webserver and payment processor levels. I don't think we should force this one in particular.

@jusfreeman
Copy link
Contributor

jusfreeman commented Oct 25, 2017

@MegaphoneJon thanks for the comments, appreciated.

I son't think we want to force ReCAPTCHA on to donation pages.

I think the change we need to do here is to provide a setting on /civicrm/admin/setting/misc?reset=1 with the reCAPTCHA Keys, so that you can toggle the displaying reCAPTCHA on Contribution pages by default. That way, you can choose to have it enabled or disabled on the CiviCRM site. This is still in-line with the overall goal of this PR.

Thoughts?

@jusfreeman
Copy link
Contributor

@eileenmcnaughton or @seamuslee001 would either of you mind reviewing this PR?

@agilewarealok
Copy link
Contributor Author

We've added a setting in /civicrm/admin/setting/misc from where we can turn on/off reCaptcha to display forcefully on contribution pages.

@@ -173,6 +173,7 @@ public static function getPropertyMap() {
'recaptchaOptions' => array('setting'),
'recaptchaPublicKey' => array('setting'),
'recaptchaPrivateKey' => array('setting'),
'forceRecaptcha' => array('setting'),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a big deal, but for future reference -- there's no general desire to pass all settings through $config / MagicMerge. MagicMerge is just a compatibility mechanism for legacy $config consumers. For new logic, use Civi::settings()->get('foo');

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for point this out, we will consider this for future updates.

@JKingsnorth
Copy link
Contributor

I wonder if adding an off-by-default setting is going to achieve much here.

This issue seems like something that should be made clear in the administrator documentation and guidance. It would be fine to reinforce that through warning messages in the system too.

From an 'administration' perspective, it would be unusual to have a contribution page that didn't have any profiles assigned to it? And if you've assigned a profile then you can also tick the 'add a captcha' option (though admittedly this is hidden away a bit).

If administrators are not familiar enough with the system to add a captcha in the profile, then will they be familiar enough to find and enable this setting (which is essentially an override that applies just to contribution forms)?

Will it cause more confusion if someone doesn't tick to add a captcha in the profile, but one is being added anyway when they look at the front end? (This could be solved with a message 'captchas are currently enforced for contribution pages' but I'm generally not in favour of 'more messages').

I think adding something similar to honeypot, or another 'flood control' mechanism more broadly is a good idea, and it would be great to combine this with other methods of defence (like captchas)/. I just think we need to be careful about how we implement it in core to ensure that the solution is actually meeting the 'need'.

@jusfreeman
Copy link
Contributor

@JKingsnorth thanks for reviewing and providing feedback.

I wonder if adding an off-by-default setting is going to achieve much here.

Having a setting which you can "turn on" to enable the recaptcha for ALL contribution forms by default is the overall goal here. This setting is not on by default.

Right now, you have to workaround this by adding an empty profile which has recaptcha enabled and assign that to the contribution page. This is problematic because it is not enforced by the system, so in a multi-user environment someone could forget to follow this process and create contribution pages which have no assigned profile and therefore no recaptcha.

From an 'administration' perspective, it would be unusual to have a contribution page that didn't have any profiles assigned to it?

No, this is not unusual. You can set up a donation page without having to add a profile. Any contribution page can be made available, without assigning a profile. Since you only need a profile to record additional information, if you do not need additional information - no profile.

And if you've assigned a profile then you can also tick the 'add a captcha' option (though admittedly this is hidden away a bit).

You mean the single check box that is hidden in the Advanced Settings at the bottom of the Profile page, which is collapsed by default. Fairly obvious, right? Myself, I have to search for it every time.

If administrators are not familiar enough with the system to add a captcha in the profile, then will they be familiar enough to find and enable this setting (which is essentially an override that applies just to contribution forms)?

Which is a manual process, requiring the person to remember to follow the process. People and manual processes are fallible.

Just as likely to read the CiviCRM documentation, IMHO.

Will it cause more confusion if someone doesn't tick to add a captcha in the profile, but one is being added anyway when they look at the front end? (This could be solved with a message 'captchas are currently enforced for contribution pages' but I'm generally not in favour of 'more messages').

Noted

I think adding something similar to honeypot, or another 'flood control' mechanism more broadly is a good idea, and it would be great to combine this with other methods of defence (like captchas)/. I just think we need to be careful about how we implement it in core to ensure that the solution is actually meeting the 'need'.

Good points, but that would be a different PR.

@JKingsnorth
Copy link
Contributor

Thanks for the response @jusfreeman.

I agree that the current 'use captcha' checkbox is rather buried away; and that having to use an empty profile with 'use reCAPTCHA' is a messy workaround for the problem.

I agree that the general anti-flooding approach is out of scope for this - but I think it's important to consider the bigger picture, in case we make like harder for ourselves in the long run by implementing a new feature to tackle a specific part of a problem, which could be resolved more broadly.

I agree with the principle, but I still have reservations about the approach.

  1. The setting is being added to 'misc' presumably because that's where the reCAPTCHA key settings are. But the setting only affects the 'CiviContribute' component, so shouldn't it be in the contribute settings?

  2. What about events? You could set up an event registration page with a very limited profile and a payment processor form, but this setting would not apply to it? Do we also need a setting for CiviEvents to be able to force the reCAPTCHA?

  3. The disconnect between the new setting and the existing 'use captcha' box on profile forms could be resolved through altering the wording on the profile form. ie: if the global setting is applied, display the profile form checkbox as 'ticked and disabled' with a note like 'reCAPTCHA is forced 'on' all contribution pages'.

@jusfreeman
Copy link
Contributor

@JKingsnorth

The setting is being added to 'misc' presumably because that's where the reCAPTCHA key settings are. But the setting only affects the 'CiviContribute' component, so shouldn't it be in the contribute settings?

Possibly, we were just following the existing pattern.

What about events? You could set up an event registration page with a very limited profile and a payment processor form, but this setting would not apply to it? Do we also need a setting for CiviEvents to be able to force the reCAPTCHA?

The plan was to get this PR merged first. Then submit a new PR to provide same feature to Event payment pages.

The disconnect between the new setting and the existing 'use captcha' box on profile forms could be resolved through altering the wording on the profile form. ie: if the global setting is applied, display the profile form checkbox as 'ticked and disabled' with a note like 'reCAPTCHA is forced 'on' all contribution pages'.

Agree, something like this would make sense. Although, not all Profiles are used on Contribution pages, so disabling the reCAPTCHA option for all Profiles may not be the best approach - but I get what you intend.

@jusfreeman
Copy link
Contributor

Thanks for the great discussion everyone. Was there any other feedback for this PR?

@jusfreeman
Copy link
Contributor

@eileenmcnaughton or @seamuslee001 would either of you mind reviewing this PR?

@jusfreeman
Copy link
Contributor

Anyone available to review this PR so we can get it merged?

@colemanw
Copy link
Member

colemanw commented Feb 28, 2018

I appreciate the work and discussion that has gone into this PR. I'm ambivalent about it though. It solves a certain problem in a specific way, but the solution leaves some room for improvement in terms of usability and intuitiveness. Not really the fault of the author of this PR, the whole captcha UI was unintuitive to begin with. Having to include it in a blank profile is confusing and cludgy, but I think this hard-to-find and easy-to-forget system setting is equally confusing and cludgy.

So I'm a -1 on this PR. It does not help usability but it increases code complexity. This feature would IMO belong better in an extension.

@jusfreeman
Copy link
Contributor

Just to re-iterate the non-theoretical problem trying to be solved here: CiviCRM Contribution pages which have no Profile associated with them do not include a ReCaptcha and as a result are prime targets for credit card fraud.

For anyone else Googl'ing / searching for a solution to this problem. The kludgy workaround remains:

Right now, you have to workaround this by adding an empty profile which has reCaptcha enabled and assign that to the contribution page. This is problematic because it is not enforced by the system, so in a multi-user environment someone could forget to follow this process and create contribution pages which have no assigned profile and therefore no reCaptcha.

Additionally, you can install https://github.com/elisseck/com.elisseck.civihoneypot - which funnily enough recommends reCaptcha as the first option. So use kludge above.

This feature would IMO belong better in an extension.

Funny, we actually asked that exact question on https://issues.civicrm.org/jira/browse/CRM-21104 back in Oct. 2017.

If someone could please close this PR, that would be great. We'll work on other solutions.

@colemanw
Copy link
Member

colemanw commented Mar 6, 2018

@jusfreeman I've discussed this with @totten and he changed my mind. Yes this solution is incomplete but it's still important.

@jusfreeman
Copy link
Contributor

@colemanw thanks, appreciated.

@colemanw
Copy link
Member

colemanw commented Mar 7, 2018

I tried running this and unfortunately it did not work as expected.
My setup:

  • Latest CiviCRM 4.7.32
  • No ReCaptcha keys configured
  • Enabled the new setting
  • Visited the default "Support CiviCRM" sample contribution page

Expected result: Error message "To display reCAPTCHA on form you must get an API key from Google"

Actual result: No error message.

It appears that the function CRM_Contribute_Form_ContributionBase::buildCustom is being called with a null $id param. Probably because there are no custom fields on the form. But this setting is supposed to display a captcha regardless of profiles, so that seems off.

@jusfreeman
Copy link
Contributor

Thanks @colemanw we'll test against 4.7.32

@agilewarealok
Copy link
Contributor Author

Hi @colemanw

We've tested this against master and 4.7.31 and in both of them we're able to see following warning message

To display reCAPTCHA on form you must get an API key from

It is not displaying for logged in user. Isn't it the expected behaviour ?

Thanks!

@colemanw
Copy link
Member

I would expect it to display for logged in admins and not for anonymous users. Are you saying it does the opposite?

@eileenmcnaughton
Copy link
Contributor

eileenmcnaughton commented Mar 21, 2018

Perhaps it should be a system check instead? You could also do a check on the setting & encourage it

@agilewarealok
Copy link
Contributor Author

Hi @colemanw

We've updated the PR to display warning for logged in admins only.

Thanks!

@agilewarealok
Copy link
Contributor Author

@eileenmcnaughton
Can we retest this please ? I think the test fail is not due to the changes I pushed.

Thanks!

@eileenmcnaughton
Copy link
Contributor

Tests are running now. (They will run when you push so a minor tweak to your commit & a force push and they will run again)

@eileenmcnaughton
Copy link
Contributor

@colemanw it looks like the status here is that your review has been responded to

@seamuslee001
Copy link
Contributor

@eileenmcnaughton @colemanw We have deployed this PR on AUG systems and appears to be to be working fine @andrew-cormick-dockery can probably comment more on what testing he performed for us

@andrew-cormick-dockery
Copy link
Contributor

This PR works within the limited scope of what it does. It does not automatically ensure that all forms attached to a payment processor will have a CAPTCHA attached to it. So the main limitation here will be to manage expectations. This is not a cure-all. We have deployed this as a partial fix to the serious problem of fraudsters using our payment system. We had to do many other things to complete the task, but this patch did simplify that whole task quite substantially. We did not discover any errors with the patch itself.

'type' => 'Boolean',
'quick_form_type' => 'YesNo',
'html_type' => '',
'default' => '0',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if this is the right default for new installs - perhaps it should default to 1 (& maybe upgrade script to change over existing). Non blocking comment

@colemanw
Copy link
Member

@colemanw colemanw merged commit 8b58a98 into civicrm:master Jul 18, 2018
@eileenmcnaughton
Copy link
Contributor

woot!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants