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

Implement invisible Recaptcha #18146

Closed
wants to merge 17 commits into
base: 3.9-dev
from

Conversation

@continga
Contributor

continga commented Sep 28, 2017

Pull Request for Issue #14565 and following up #14583 and #16599 et al.

Summary of Changes

This PR implements the possiblity to use the Invisible reCAPTCHA using a separate plugin. Besides implementing the basic functionality to make invisible reCAPTCHA working, there has been made some smaller architectural changes to the whole captcha process (e.g. using Exception instead of JError for errors) and the reCAPTCHA PHP library from Google has been updated, alongside with some smaller changes in the old reCAPTCHA (V1 und V2) plugin.

As this is one of my first PRs for Joomla!, please dont hesitate to make suggestions of how to improve it.

Testing Instructions

  1. Create a site- and secret-key at https://www.google.com/recaptcha/admin for an invisible reCAPTCHA (reCAPTCHA V2 keys will not work with Invisible reCAPTCHA)
  2. Configure the Invisible reCAPTCHA plugin in the Joomla! backend, entering at least the site- and secret-key
  3. Configure the Invisible reCAPTCHA plugin to be used as captcha plugin (System -> Global Configuration -> Default Captcha)
  4. Open up a form in the frontend where a captcha is enabled (e.g. by creating a menulink to a contactform)
  5. Notice the reCAPTCHA logo on the right side (see screenshot further down) which indicates that the invisible reCAPTCHA has been executed.

We also need to make sure that the reCAPTCHA plugin (V1 and V2) still is working correctly (as there has been made some bigger changes to the code by me), although I have tested this toroughly already, but I would be very glad if other people could take a look at this too, I don't want to take chances that this PR breaks something.

Additionally, as this PR changes the rendering of form fields for the registration form (this was needed as the registration form was using its own rendering of form fields, which caused issues with the invisible reCAPTCHA), we need to make sure the registration form for users is working as expected. So we need to test the registration form in resprect to custom fields of users etc, and whether they get displayed correctly now.

Expected result

image

@brianteeman

This comment has been minimized.

Show comment
Hide comment
@brianteeman

brianteeman Sep 28, 2017

Contributor

Because of the keys issue would it not make sense to create this as a new plugin?

Contributor

brianteeman commented Sep 28, 2017

Because of the keys issue would it not make sense to create this as a new plugin?

@continga

This comment has been minimized.

Show comment
Hide comment
@continga

continga Sep 28, 2017

Contributor

V1 and V2 keys also were incompatible, and we had them in the same plugin. Additionally, V2 and invisible share very much of the code (e.g. the whole server-side validation), so I don't see any advantage when using a separate plugin. Additionally, the "key issue" is pretty clear on the Google reCAPTCHA admin page, so I think it shouldn't confuse users. I just wrote it down in the testing information to make clear that new keys has to be created to test this.

Contributor

continga commented Sep 28, 2017

V1 and V2 keys also were incompatible, and we had them in the same plugin. Additionally, V2 and invisible share very much of the code (e.g. the whole server-side validation), so I don't see any advantage when using a separate plugin. Additionally, the "key issue" is pretty clear on the Google reCAPTCHA admin page, so I think it shouldn't confuse users. I just wrote it down in the testing information to make clear that new keys has to be created to test this.

@brianteeman

This comment has been minimized.

Show comment
Hide comment
@brianteeman

brianteeman Sep 28, 2017

Contributor

I thought it was a mistake last time as well

Contributor

brianteeman commented Sep 28, 2017

I thought it was a mistake last time as well

@continga

This comment has been minimized.

Show comment
Hide comment
@continga

continga Sep 28, 2017

Contributor

Well, we could do it as a separate plugin now (it is not very much work to disassemble it), but where do you see the advantage exactly?

Contributor

continga commented Sep 28, 2017

Well, we could do it as a separate plugin now (it is not very much work to disassemble it), but where do you see the advantage exactly?

@brianteeman

This comment has been minimized.

Show comment
Hide comment
@brianteeman

brianteeman Sep 28, 2017

Contributor

You would be able to use invisible!e on some forms and V2 on others.
It will also make future updates easier without having to have switches in the code for the old keys

Contributor

brianteeman commented Sep 28, 2017

You would be able to use invisible!e on some forms and V2 on others.
It will also make future updates easier without having to have switches in the code for the old keys

@continga

This comment has been minimized.

Show comment
Hide comment
@continga

continga Sep 28, 2017

Contributor

Okay, true. Especially the first point is very interesting. I think we should wait for some more feedback, but if the broad agreement is to put it into a separate plugin, I will be very glad to do so.

Contributor

continga commented Sep 28, 2017

Okay, true. Especially the first point is very interesting. I think we should wait for some more feedback, but if the broad agreement is to put it into a separate plugin, I will be very glad to do so.

@810

This comment has been minimized.

Show comment
Hide comment
@810

810 Sep 28, 2017

Contributor

i vote for same plugin

Contributor

810 commented Sep 28, 2017

i vote for same plugin

@Fedik

This comment has been minimized.

Show comment
Hide comment
@Fedik

Fedik Sep 29, 2017

Contributor

it should be a plugin, there even a pull request for it, already, somewhere here

Contributor

Fedik commented Sep 29, 2017

it should be a plugin, there even a pull request for it, already, somewhere here

@paulus103

This comment has been minimized.

Show comment
Hide comment
@paulus103

paulus103 Oct 20, 2017

Hi This looks a nice feature to add :)

paulus103 commented Oct 20, 2017

Hi This looks a nice feature to add :)

@franz-wohlkoenig

This comment has been minimized.

Show comment
Hide comment
@franz-wohlkoenig

franz-wohlkoenig commented Oct 28, 2017

@paulus103 can you please test?


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/18146.

@paulus103

This comment has been minimized.

Show comment
Hide comment
@paulus103

paulus103 Oct 28, 2017

@franz-wohlkoenig

I have tested this on a live server and it works :)


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/18146.

paulus103 commented Oct 28, 2017

@franz-wohlkoenig

I have tested this on a live server and it works :)


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/18146.

@franz-wohlkoenig

This comment has been minimized.

Show comment
Hide comment
@franz-wohlkoenig

franz-wohlkoenig Oct 29, 2017

@paulus103 thanks for Info. Please mark your Test as successfully:

  • open Issue Tracker
  • Login with your github-Account
  • Click on blue "Test this"-Button above Authors-Picture
  • mark your Test as successfully
  • hit "submit test result"

franz-wohlkoenig commented Oct 29, 2017

@paulus103 thanks for Info. Please mark your Test as successfully:

  • open Issue Tracker
  • Login with your github-Account
  • Click on blue "Test this"-Button above Authors-Picture
  • mark your Test as successfully
  • hit "submit test result"
@paulus103

This comment has been minimized.

Show comment
Hide comment
@paulus103

paulus103 Oct 31, 2017

I have tested this item successfully on 259a657

tested successfully on an 3.8.1 old development site :)


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/18146.

paulus103 commented Oct 31, 2017

I have tested this item successfully on 259a657

tested successfully on an 3.8.1 old development site :)


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/18146.

@pmleconte

This comment has been minimized.

Show comment
Hide comment
@pmleconte

pmleconte Nov 12, 2017

What can be done, so this pr will be included in 3.8.3. I was disappointed not seeing it in 3.8.2.

If I can help, let me know,

Pascal

pmleconte commented Nov 12, 2017

What can be done, so this pr will be included in 3.8.3. I was disappointed not seeing it in 3.8.2.

If I can help, let me know,

Pascal

@franz-wohlkoenig

This comment has been minimized.

Show comment
Hide comment
@franz-wohlkoenig

franz-wohlkoenig Nov 12, 2017

@pmleconte Pull Requests needs 2 successfully Tests. If you can test this PR, next Step is done.

franz-wohlkoenig commented Nov 12, 2017

@pmleconte Pull Requests needs 2 successfully Tests. If you can test this PR, next Step is done.

@franz-wohlkoenig

This comment has been minimized.

Show comment
Hide comment
@franz-wohlkoenig

franz-wohlkoenig Nov 12, 2017

@continga can you please resolve conflicting Files so PR can be tested?

franz-wohlkoenig commented Nov 12, 2017

@continga can you please resolve conflicting Files so PR can be tested?

@Fedik

This comment has been minimized.

Show comment
Hide comment
@Fedik

Fedik Nov 12, 2017

Contributor

I would really suggest to make/test a plugin (there even already one #16599)
and invest your time in to a plugin

Contributor

Fedik commented Nov 12, 2017

I would really suggest to make/test a plugin (there even already one #16599)
and invest your time in to a plugin

@franz-wohlkoenig

This comment has been minimized.

Show comment
Hide comment
@franz-wohlkoenig

franz-wohlkoenig Nov 12, 2017

Status is set on "Needs Review".

franz-wohlkoenig commented Nov 12, 2017

Status is set on "Needs Review".

@pmleconte

This comment has been minimized.

Show comment
Hide comment
@pmleconte

pmleconte Nov 12, 2017

I have tested this item successfully on 259a657

Test ok on 3.8.1 site. Both V2 and invisible captcha are OK.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/18146.

pmleconte commented Nov 12, 2017

I have tested this item successfully on 259a657

Test ok on 3.8.1 site. Both V2 and invisible captcha are OK.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/18146.

@continga

This comment has been minimized.

Show comment
Hide comment
@continga

continga Jul 22, 2018

Contributor

I totally agree with you @mbabker and that is also why I want to change this. Good to know you are the same meaning. Let us wait for some more feedback, but you just verified to me that'd be the way to go 🙂

Contributor

continga commented Jul 22, 2018

I totally agree with you @mbabker and that is also why I want to change this. Good to know you are the same meaning. Let us wait for some more feedback, but you just verified to me that'd be the way to go 🙂

@roland-d

This comment has been minimized.

Show comment
Hide comment
@roland-d

roland-d Jul 23, 2018

Contributor

@continga I also agree with your findings and those of @mbabker . Just like com_contacts we would need a layout override file to account for this code:

<?php if (!$field->required && $field->type !== 'Spacer') : ?>
										<span class="optional"><?php echo JText::_('COM_USERS_OPTIONAL'); ?></span>
<?php endif; ?>

This is also done for com_contacts. That would clean up nicely.

Contributor

roland-d commented Jul 23, 2018

@continga I also agree with your findings and those of @mbabker . Just like com_contacts we would need a layout override file to account for this code:

<?php if (!$field->required && $field->type !== 'Spacer') : ?>
										<span class="optional"><?php echo JText::_('COM_USERS_OPTIONAL'); ?></span>
<?php endif; ?>

This is also done for com_contacts. That would clean up nicely.

Rene Pasing
@continga

This comment has been minimized.

Show comment
Hide comment
@continga

continga Jul 24, 2018

Contributor

I now fixed the label issue, see commit f75d99c6.

Now from my point of view every feedback and review-change-request has been applied. Can you guys maybe take a look again and check whether this is mergable? This PR is pretty old and I am crossing fingers we finally get this thing merged 🙂 Thank you in advance!

Contributor

continga commented Jul 24, 2018

I now fixed the label issue, see commit f75d99c6.

Now from my point of view every feedback and review-change-request has been applied. Can you guys maybe take a look again and check whether this is mergable? This PR is pretty old and I am crossing fingers we finally get this thing merged 🙂 Thank you in advance!

@roland-d

This comment has been minimized.

Show comment
Hide comment
@roland-d

roland-d Jul 27, 2018

Contributor

@continga I am sure we will get it merged once all issues are solved :)

When I do a password reset, I see this form:
image

Same when I do forget username I see this form:
image

So I think for these 2 pages you still need to add the layout fix.

Looking good:

  • New user registration
  • Contact form
  • Edit article

Those are the only places I can think of and find where the captcha could be shown.

Contributor

roland-d commented Jul 27, 2018

@continga I am sure we will get it merged once all issues are solved :)

When I do a password reset, I see this form:
image

Same when I do forget username I see this form:
image

So I think for these 2 pages you still need to add the layout fix.

Looking good:

  • New user registration
  • Contact form
  • Edit article

Those are the only places I can think of and find where the captcha could be shown.

Rene Pasing
Use the Form renderField(set) methods to display the com_users fronte…
…nd login/remind/reset forms. This fixes some problems with formfields not being rendered correctly (e.g. the invisible recaptcha field)
@continga

This comment has been minimized.

Show comment
Hide comment
@continga

continga Jul 28, 2018

Contributor

Thanks @roland-d I fixed the cases you found.

Additionally I checked the sourcecode for other cases where form fields are being rendered manually and found some additional ones in the com_users login, password reset confirmation and success views. There shouldn't be a captcha there, but just to be sure I replaced the manual rendering with the Form renderer.

As far as I see, the only other places where forms are being rendered manually now are in several com_config views, and in the profile edit view of com_users. Both cases are not easily straight-forward replaceable by the Form renderer, so I left them in their current state, also because they don't display a captcha at all.

Can you check again? ☺️ Thank you very much in advance!

Contributor

continga commented Jul 28, 2018

Thanks @roland-d I fixed the cases you found.

Additionally I checked the sourcecode for other cases where form fields are being rendered manually and found some additional ones in the com_users login, password reset confirmation and success views. There shouldn't be a captcha there, but just to be sure I replaced the manual rendering with the Form renderer.

As far as I see, the only other places where forms are being rendered manually now are in several com_config views, and in the profile edit view of com_users. Both cases are not easily straight-forward replaceable by the Form renderer, so I left them in their current state, also because they don't display a captcha at all.

Can you check again? ☺️ Thank you very much in advance!

@roland-d

This comment has been minimized.

Show comment
Hide comment
@roland-d

roland-d Jul 29, 2018

Contributor

I have tested this item successfully on 67ac41a

After applying the patch I setup the ReCaptcha invisible settings in the plugin and after that I tested the following view:

  1. User Registration (http://joomla-cms-live.test/index.php?option=com_users&view=registration&Itemid=405)
  2. Forgot Password (http://joomla-cms-live.test/index.php?option=com_users&view=reset&Itemid=409)
  3. Forgot Username (http://joomla-cms-live.test/index.php?option=com_users&view=remind&Itemid=406)
  4. Edit an article (http://joomla-cms-live.test/index.php?option=com_content&view=form&layout=edit&a_id=6&Itemid=257&catid=26)
  5. Contact Form (http://joomla-cms-live.test/index.php?option=com_contact&view=contact&id=1&Itemid=229)

All the views show the Captcha logo and allowed me to process the form successfully.

I had a look at the JS callbacks as well they work fine for me.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/18146.

Contributor

roland-d commented Jul 29, 2018

I have tested this item successfully on 67ac41a

After applying the patch I setup the ReCaptcha invisible settings in the plugin and after that I tested the following view:

  1. User Registration (http://joomla-cms-live.test/index.php?option=com_users&view=registration&Itemid=405)
  2. Forgot Password (http://joomla-cms-live.test/index.php?option=com_users&view=reset&Itemid=409)
  3. Forgot Username (http://joomla-cms-live.test/index.php?option=com_users&view=remind&Itemid=406)
  4. Edit an article (http://joomla-cms-live.test/index.php?option=com_content&view=form&layout=edit&a_id=6&Itemid=257&catid=26)
  5. Contact Form (http://joomla-cms-live.test/index.php?option=com_contact&view=contact&id=1&Itemid=229)

All the views show the Captcha logo and allowed me to process the form successfully.

I had a look at the JS callbacks as well they work fine for me.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/18146.

@MastersOfMedia

This comment has been minimized.

Show comment
Hide comment
@MastersOfMedia

MastersOfMedia Aug 12, 2018

I have tested this item successfully on 67ac41a

I have tested this item succesfully on a fresh install of Joomla! 3.8.11. On all 5 forms the invisible ReCaptcha badge show correctly and I was able to enter and process the form data correctly. By altering the keys in the plugin settings I was able to determine the validation actually works correctly.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/18146.

MastersOfMedia commented Aug 12, 2018

I have tested this item successfully on 67ac41a

I have tested this item succesfully on a fresh install of Joomla! 3.8.11. On all 5 forms the invisible ReCaptcha badge show correctly and I was able to enter and process the form data correctly. By altering the keys in the plugin settings I was able to determine the validation actually works correctly.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/18146.

@franz-wohlkoenig

This comment has been minimized.

Show comment
Hide comment
@franz-wohlkoenig

franz-wohlkoenig Aug 13, 2018

Ready to Commit after two successful tests.

franz-wohlkoenig commented Aug 13, 2018

Ready to Commit after two successful tests.

@joomla-cms-bot joomla-cms-bot added the RTC label Aug 13, 2018

@mbabker mbabker added this to the Joomla 3.9.0 milestone Aug 21, 2018

Outdated

@mbabker mbabker changed the base branch from staging to 3.9-dev Aug 28, 2018

mbabker added a commit that referenced this pull request Aug 28, 2018

@mbabker

This comment has been minimized.

Show comment
Hide comment
@mbabker

mbabker Aug 28, 2018

Member

Merged to 3.9-dev via 9a12f28

Member

mbabker commented Aug 28, 2018

Merged to 3.9-dev via 9a12f28

PLG_RECAPTCHA_CALLBACK_DESC="(Optional) JavaScript callback, executed after successful reCAPTCHA response"
PLG_RECAPTCHA_EXPIRED_CALLBACK_LABEL="Expired callback"
PLG_RECAPTCHA_EXPIRED_CALLBACK_DESC="(Optional) JavaScript callback, executed when the reCAPTCHA expired"
PLG_RECAPTCHA_EXPIRED_CALLBACK_LABEL="Error callback"

This comment has been minimized.

@tecpromotion

tecpromotion Aug 30, 2018

Contributor

has to be PLG_RECAPTCHA_ERROR_CALLBACK_LABEL

@tecpromotion

tecpromotion Aug 30, 2018

Contributor

has to be PLG_RECAPTCHA_ERROR_CALLBACK_LABEL

PLG_RECAPTCHA_EXPIRED_CALLBACK_LABEL="Expired callback"
PLG_RECAPTCHA_EXPIRED_CALLBACK_DESC="(Optional) JavaScript callback, executed when the reCAPTCHA expired"
PLG_RECAPTCHA_EXPIRED_CALLBACK_LABEL="Error callback"
PLG_RECAPTCHA_EXPIRED_CALLBACK_DESC="(Optional) JavaScript callback, executed when the reCAPTCHA encounters an error"

This comment has been minimized.

@tecpromotion

tecpromotion Aug 30, 2018

Contributor

hast to be PLG_RECAPTCHA_ERROR_CALLBACK_DESC

@tecpromotion

tecpromotion Aug 30, 2018

Contributor

hast to be PLG_RECAPTCHA_ERROR_CALLBACK_DESC

@tecpromotion

two strings are duplicated in the en-GB.plg_captcha_recaptcha.ini file and must be corrected.

@brianteeman

This comment has been minimized.

Show comment
Hide comment
@brianteeman

brianteeman Aug 30, 2018

Contributor

@tecpromotion
Good spot

as this has been merged already can you do a PR to fix it - or shall I ?

Contributor

brianteeman commented Aug 30, 2018

@tecpromotion
Good spot

as this has been merged already can you do a PR to fix it - or shall I ?

@tecpromotion

This comment has been minimized.

Show comment
Hide comment
@tecpromotion

tecpromotion Aug 30, 2018

Contributor

@brianteeman
It would be an honor for me to write the PR. I found the mistake because I'm doing the german translation :)

Contributor

tecpromotion commented Aug 30, 2018

@brianteeman
It would be an honor for me to write the PR. I found the mistake because I'm doing the german translation :)

tecpromotion added a commit to tecpromotion/joomla-cms that referenced this pull request Aug 30, 2018

Implement invisible Recaptcha joomla#18146
There were two double strings

tecpromotion added a commit to tecpromotion/joomla-cms that referenced this pull request Aug 30, 2018

mbabker added a commit that referenced this pull request Aug 30, 2018

Fix language strings in recaptcha plugin (#21918)
* Implement invisible Recaptcha #18146

There were two double strings

* Implement invisible Recaptcha #18146

thanks @brianteeman
fix order
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment