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

GH-3546 Recaptcha on login form #4626

Merged
merged 51 commits into from
Apr 25, 2024

Conversation

i-am-chitti
Copy link
Contributor

@i-am-chitti i-am-chitti commented Mar 22, 2024

Description

This PR adds recaptcha on login form. One can add any one of three recaptcha vendor -

  1. Google Recaptcha - https://developers.google.com/recaptcha/docs/v3#programmatically_invoke_the_challenge
  2. HCaptcha - https://docs.hcaptcha.com/invisible#programmatically-invoke-the-challenge
  3. Turnstile - https://developers.cloudflare.com/turnstile/get-started/client-side-rendering/#execution-modes

Issue

Environment variables -

  1. CAPTCHA_DRIVER - google-recaptcha | hcaptcha | turnstile
  2. CAPTCHA_SITE_KEY - site key
  3. CAPTCHA_SECRET_KEY - secret key

Engineering choices

  1. If some of the above env variable provided, then, backend generates an error -
    image
    Please note that login/signup form will keep working as expected.
  2. I'm using a Captcha guard that intercepts the request. If "captchaToken" is present in the body and all env is set, then, the captcha token is verified by backend through the service.
  3. One can use this guard on any resolver to protect it by the captcha.
  4. On frontend, two hooks useGenerateCaptchaToken and useInsertCaptchaScript is created. useInsertCaptchaScript adds the respective captcha JS script on frontend. useGenerateCaptchaToken returns a function that one can use to trigger captcha token generation programatically. This allows one to generate token keeping recaptcha invisible.

Note

This PR contains some changes in unrelated files like indentation, spacing, inverted comma etc. I ran "yarn nx fmt:fix twenty-front" and "yarn nx lint twenty-front -- --fix".

Screenshots

image

Copy link

github-actions bot commented Mar 22, 2024

Warnings
⚠️ Changes were made to the environment variables, but not to the documentation - Please review your changes and check if a change needs to be documented!

Generated by 🚫 dangerJS against 81e57dd

@i-am-chitti i-am-chitti marked this pull request as ready for review March 22, 2024 22:55
@FelixMalfait
Copy link
Member

Wow big one! Thanks!!!

Fyi server tests not passing because of missing import probably:

● AuthResolver › should be defined
    Nest can't resolve dependencies of the CaptchaGuard (?). Please make sure that the argument CaptchaService at index [0] is available in the RootTestModule context.
    Potential solutions:
    ```
    
    Frontend tests failing but that doesn't seem related to your changes 

@i-am-chitti
Copy link
Contributor Author

@FelixMalfait, added CaptchaGuard in authresolver test file. Now, server test is passing.

@i-am-chitti
Copy link
Contributor Author

@FelixMalfait Please have a re-review.

@FelixMalfait
Copy link
Member

@i-am-chitti sorry about the docker-compose, we got rid of it yes - we thought it'd be simpler to focus on 1 recommended setup only. I'll update the makefile you're right. And review your PR again! Thanks 🙏

@charlesBochet
Copy link
Member

@i-am-chitti this is great work! I have refactored the frontend to write in a simpler manner and to use effects in a way to avoid re-renders

I could not make Hcaptcha work easily so I have removed it to merge this PR before it gets conflicts with other PRs but we would be very happy if you want to re-apply your hcaptcha implementation (the driver on backend + update the new hooks on frontend)

@charlesBochet charlesBochet merged commit dc576d0 into twentyhq:main Apr 25, 2024
4 checks passed
@i-am-chitti
Copy link
Contributor Author

@charlesBochet Sure, I'll integrate the HCaptcha in another PR 🚧 .
Thanks.

@FelixMalfait
Copy link
Member

@i-am-chitti thanks again for your really great work!
To be honest I don't think re-integrating HCaptcha would be the most useful task at this point (even if it's fast). We're good enough with 2 / we'll start learning from it soon. Thanks to your work we know it will be possible later to add more, but if you have time I'd rather spend it on other issues.

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.

None yet

3 participants