User tests: Successful: Unsuccessful:
Pull Request for Issue #14565 and following up #14583 and #16599 et al.
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.
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.
Category | ⇒ | Administration Language & Strings External Library Composer Change Libraries Repository Unit Tests |
Status | New | ⇒ | Pending |
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.
I thought it was a mistake last time as well
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?
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
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.
i vote for same plugin
it should be a plugin, there even a pull request for it, already, somewhere here
Hi This looks a nice feature to add :)
@paulus103 can you please test?
I have tested this on a live server and it works :)
@paulus103 thanks for Info. Please mark your Test as successfully:
I have tested this item
tested successfully on an 3.8.1 old development site :)
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 Pull Requests needs 2 successfully Tests. If you can test this PR, next Step is done.
Status | Pending | ⇒ | Needs Review |
Status is set on "Needs Review".
I have tested this item
Test ok on 3.8.1 site. Both V2 and invisible captcha are OK.
Category | Administration Language & Strings External Library Composer Change Libraries Repository Unit Tests | ⇒ | Administration Language & Strings External Library Composer Change SQL Installation Postgresql MS SQL Libraries Repository Unit Tests |
After the feedback here, I decided tocompletely rework my implementation and to swap out the Invisible reCAPTCHA implementation into its own plugin.
I additionally removed the support for reCAPTCHA V1, as this has been dropped by Google[1]. Users should switch to V2 or Invisible.
I would be very glad to receive additional feedback, especially if you see anything that can (and should) be done better. I would be very glad to enhance this to a point where we can get this merged.
[1] = https://developers.google.com/recaptcha/docs/versions#v1
Status | Needs Review | ⇒ | Pending |
Status is set back on "Pending".
The recaptcha v1 will continue to work until march 2018. It has already been removed from j4
Nvm, I reverted removing V1 functionality.
Labels |
Added:
?
?
?
?
|
Category | Administration Language & Strings External Library Composer Change Libraries Repository Unit Tests SQL Installation Postgresql MS SQL | ⇒ | SQL Administration com_admin Postgresql MS SQL Language & Strings External Library Composer Change Installation Libraries Repository Unit Tests |
@paulus103 @pmleconte Can you maybe re-test this PR, after I switched the functionality to a separate plugin? Thanks in advance!
Hi,
I'm trying to install your new plugin as an update on an 3.8.2 website.
I tried different update channels (Testing, Next Joomla, Default) without success. Am I missing something ? is your zip file supposed to work in update mode ?
On a new install, V2 and Invisible captcha are OK.
So, should I set test as OK ?
Pascal
I have tested this item
V2 and invisible captcha Ok on a new Joomla installation.
Is there tutorial on how to add this to Joomla for testing purposes? I've got a multitude of joomla environments and wouldn't mind testing this in dev and small live sites.
Go here for testing instructions/tutorial: https://docs.joomla.org/Testing_Joomla!_patches
@pmleconte There are database update scripts inside of administrator/components/com_admin/sql/updates
which are not executed when you manually appy the changes made by this PR to your Joomla! installation. One possibility would be to execute the SQL scripts manually, after having applied the patch.
These SQL update scripts should be executed automatically once this PR gets merged and released within the 3.8.2 release - can someone else confirm this, or do I need to add some additional stuff for it to work?
Could you update the pr.
Then we can do the final tests.
When I go on google invisible recaptcha the registration site is down. Is that method "taken down" ?
I just signed up to it
Whenever this PR will be updated, I'll be ready to test it again.
Pascal
https://www.google.com/recaptcha/intro/comingsoon/invisible.html
That's the only address I can find for signing up. When I click the link the form is not available?
Just click on "continue" instead of "sign up" and you should be able to register for "invisible captcha".
Pascal
I will do this at some time next week, sorry for the delay, didn't have time for this yet
@roland-d sorry, you are absolutely right, I should have worked on this much sooner. Thank you for the reminder, and sorry again for the huge delay!
I resolved all conflicts now. I needed to squash-rebase this whole thing, as the conflicts were CRAAAZY³ when doing a normal rebase due to having reworked this thing several times, and a lot of stuff happened in Joomla since - we lost a bit of history due to this (the main thing is now inside of 1 commit), but I think this should be OK.
I hope we are good now. Please do some tests and let's try to bring this into one of the next minor releases. I promise to react on fast feedback very fast this time ;-) I hope we can finally get this off the pipe!
I don't know if something like google recaptcha should be in core since it's something that is also discussed due to gdpr and it's an external service... Wouldn't it be better to have something "own" ?
Sorry for disturbing the party...
Wouldn't it be better to have something "own" ?
I disagree with this because first of all I don't think we should invent everything ourselves just because of GDPR. GDPR doesn't forbid using external services. We will not build such an intelligent recaptcha ourselves.
We don't force users to use this, it is optional when they want to use this.
It does not forbid it, but the user should be aware he has to sign a contract with google to be legal to use it - well at least in germany with the DSGVO...
It is not joomla's job to ensure that a web site is compliant to every law in every country of the world nor is it joomla's job to educate users in those countries of their own laws. We don't force or even tell people about impressum do we? We dont force or tell people about a11y laws
If someone is adding a 3rd party to Joomla, the argument is always that we don't do that.
I was trying to consider that fact that we usually try to avoid that.
Personally I would find it good to have something in core that is independant of Google. But if you don't agree, it's ok.
briantreeman - that's a stupid response. When 1/5 of the world has to be GDPR compliant why should Joomla not be...
That being said, this tool is GDRP compliant. You just need to mention it in your privacy policy so I don't know what people are moaning about.
@brianteeman @cymode
It's OK -> we can take into account that we report that to the new com_privacy then and all is fine.
I was asking about that also because of above stated reasons (Independence of 3rd party) - but if you all decide that its ok, I am fine...
that's a stupid response.
i don't like that kind of "Argument".
Can someone explain me what the test instruction are, to move this forward?
Wouldn't it be better to have something "own" ?
If someone in the Joomla community really thinks they can create and maintain a captcha system that is arguably as smart and secure as existing third party solutions, feel free to do so. Fact is there are a lot of bits and pieces of Joomla that we really need to offload onto the shoulders of third party solutions (be it PHP or UI related libraries or services like captcha) because we don't have the resources to build and maintain every single thing ourselves.
The sql import etc suggests that this is a NEW plugin but the xml file is the xml for the original recaptcha plugin
it looks like you got a bit confused when updating the pr and you have changed files from the existing recaptcha plugin which should have been new files for this invisible-recaptcha plugin
What do you mean exactly? As far as I see, this is not the case, i.e. the XML file is completely new ( https://github.com/joomla/joomla-cms/pull/18146/files#diff-1e6b80a6f1929ba0e37a095f7c2fb985 ). Can you show me what you are referring to?
Yeah, part of this PR was to remove the JReCaptcha class, so there was some stuff that needed to be done in the old plugin - I took that possibility to pass all per documentation possible parameters to the reCAPTCHA v2 javascript instance (see also https://github.com/joomla/joomla-cms/pull/18146/files#diff-b934bb49d0402bc42e4577e744d93a95 ). Additionally I did some general code cleanup in the old plugin.
Are you suggesting this should be removed?
Its always confusing when there are more than one issue being addressed in a single pr - especially when there is no mention of it in the original post and testing instructions. Obviously as you have changed the existing plugin as well in this pr then that needs to be tested to ensure it still works
uuuh, I mentioned that in my OP:
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.
I will take a look at that bug with the label tonight or tomorrow, that should have been fixed already, but I will why its popping up again. Thanks for testing this!
I made a PR for that that is not merged yet...
sorry I missed that part of the post
Do we really need to expose the tabindex and the callback options ?
also i see that you expose the expired callback option in the recaptcha v2 plugin but not in the invisible plugin
@coolcat-creations I will take a look at that, thank you.
@brianteeman You are right. Not sure how this happened, I am pretty sure I got all options covered when I wrote the plugin. I think Google added this option at a later date. I will double-check this once again and add all missing options (will also check for the old plugin).
And yes, I think it makes sense to expose those to the user. Imho they can be useful in some situations, and we shouldn't force the user to hack the core to could use them
Category | Administration Language & Strings External Library Composer Change Libraries Repository Unit Tests SQL Installation Postgresql MS SQL com_admin | ⇒ | Repository SQL Administration com_admin Postgresql MS SQL Language & Strings External Library Composer Change Installation Libraries |
I just updated this PR and should've accounted all feedback so far. Can you guys take a look again?
@brianteeman I could not reproduce the bug with the label. Can you check whether it still happens, or give me some more information about your environment?
Labels |
Removed:
?
|
@continga I have been testing and running into a few issues.
The captcha label should not be shown as now the form looks broken.
After I submit the form I am greeted by this error:
So I deleted the autoload_psr4.php file but the error remains.
There is no language entry for these labels
label="PLG_RECAPTCHA_INVISIBLE_ERROR_CALLBACK_LABEL"
description="PLG_RECAPTCHA_INVISIBLE_ERROR_CALLBACK_DESC"
It would be nice to have larger input boxes
This way you can see your keys better.
Okay, I found the reason for the "label problem". The problem is, that the com_users registration view, components/com_users/views/registration/tmpl/default.php, between lines 34 and 49 renders the input fields completely by himself, and does not take into account the hiddenLabel
attribute of the field XML, see for reference components/com_contact/layouts/joomla/form/renderfield.php#L48 or layouts/joomla/form/renderfield.php#L32
If we would replace those 16 lines, with only this single 1 line:
<?php echo $field->renderField(); ?>
Then the field renderer of Joomla! is being used to render that form, and the problem is solved for the registration form. I did some quick tests, and I did not see any differences in the generated form when using $field->renderField()
instead of the current implementation, but I am unsure whether this would be the way to fix this.
So my question to you guys: What do you think about this, should the fix be to use the renderField
method of each field there, and to replace the current manual-rendering of those? Or do you see any problems with that?
edit: All other points should be fixed now (so the Call to undefined method, missing translation, larger input boxes things)
Do Custom fields still work with your change?
Custom Fields in the registration form? Yes, I think they will still work. To be honest, I think they will work even better than before, as they will be rendered completely using their own mechanics - and not using the "hardcoded" way in the registration view like before.
But, as I said before, I'd like to get some more input to that point
Then the field renderer of Joomla! is being used to render that form, and the problem is solved for the registration form. I did some quick tests, and I did not see any differences in the generated form when using $field->renderField() instead of the current implementation, but I am unsure whether this would be the way to fix this.
In general I would suggest that unless there is some major need for styling that contradicts what the form fields and their layouts render that $field->renderField()
should always be used (or, if you have all the fields enclosed into a fieldset, $form->renderFieldset($fieldset->name);
). Not doing so actually makes it harder to use plugins to dynamically alter forms and I'd almost suggest adding a static analysis tool raising warnings about manual rendering of field elements (echo $field->label;
or echo $field->input;
).
@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.
Category | Administration Language & Strings External Library Composer Change Libraries Repository SQL Installation Postgresql MS SQL com_admin | ⇒ | Repository SQL Administration com_admin Postgresql MS SQL Language & Strings Front End com_users External Library Composer Change Installation Libraries |
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
@continga I am sure we will get it merged once all issues are solved :)
When I do a password reset
, I see this form:
Same when I do forget username
I see this form:
So I think for these 2 pages you still need to add the layout fix.
Looking good:
Those are the only places I can think of and find where the captcha could be shown.
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?
I have tested this item
After applying the patch I setup the ReCaptcha invisible settings in the plugin and after that I tested the following view:
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.
I have tested this item
Status | Pending | ⇒ | Ready to Commit |
Ready to Commit after two successful tests.
Labels |
Added:
?
|
Status | Ready to Commit | ⇒ | Closed |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2018-08-28 23:30:03 |
Closed_By | ⇒ | mbabker | |
Labels |
Added:
?
|
@tecpromotion
Good spot
as this has been merged already can you do a PR to fix it - or shall I ?
@brianteeman
It would be an honor for me to write the PR. I found the mistake because I'm doing the german translation :)
Because of the keys issue would it not make sense to create this as a new plugin?