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

Added an easy way to add captcha to any form #5

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

roadster31
Copy link
Contributor

@roadster31 roadster31 commented Dec 11, 2019

This PR adds a specific form validator to the module , which extends the base TheliaFormValidator: ReCaptchaFormValidator.
This form validator dispatches the CHECK_CAPTCHA_EVENT event if the g-recaptcha-response form field is defined in the request, and throw an exception if a suspicious action is detected.

Thus, adding {hook name="recaptcha.check"} to any form will add a ReCaptcha check to this form without the need to update the form validation code.

@@ -25,6 +25,12 @@
<tag name="kernel.event_subscriber" />
<argument type="service" id="request"/>
</service>

<service id="thelia.form_validator" class="ReCaptcha\FormValidator\ReCaptchaFormValidator">
Copy link
Member

Choose a reason for hiding this comment

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

I think this is dangerous to do this because this will not work if another module override the thelia form validator

Copy link
Contributor Author

@roadster31 roadster31 Dec 16, 2019

Choose a reason for hiding this comment

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

True, but it makes the module usable almost out of the box (inserting a hook déclaration in a template is enough), instead of having to code something.
Additionally, the module could not be used easily for base Thelia forms. This change make it usable in these forms too.

Copy link
Member

Choose a reason for hiding this comment

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

Yes I understand your PR and this is very great but like I said it's dangerous because you can think you are protected but you are not because another module override the form validator.
Maybe we can add a warning message in this case ? (I don't know if we can check if another module override this service)

Choose a reason for hiding this comment

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

Nowadays, ReCaptcha is a must-have Thelia module (it should be included directly in Thelia) so any solution for making module's integration easier would be appreciated 👍

Copy link
Member

Choose a reason for hiding this comment

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

Sorry we are not going to include a module who is connected to a proprietary service in Thelia.
I agreed we need a more simple way to add this module to a form but like I explain this PR is not a good solution,
the service can be override by another module, so it will be inefficient and if a bot remove the field g-recaptcha-response the module will not check the captcha and will be inefficient too.

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

Successfully merging this pull request may close these issues.

3 participants