feat: reCAPTCHA implementation #Closes Implement reCAPTCHA on the Ion login page #1765
Conversation
There was a problem hiding this comment.
Hi Elijah, thanks for your PR. This implementation with the failed logins wasn't exactly what I had in mind, but it's quite a good idea. Let's keep it in. But in addition, can you add the client-side invisible reCAPTCHA that is always active? https://developers.google.com/recaptcha/docs/invisible. This lets reCAPTCHA decide based on client indicators if a login is suspicious, and if so, it automatically activates when the login button is clicked.
|
okay, check out my new commit. you will have to create 2 individual applications for invisible and checkbox recaptcha. Let me know if you find any problems. |
There was a problem hiding this comment.
Excellent work, thanks Elijah. Please see a few comments on the code. Can you make some reCAPTCHA keys on your own in your Google account to test this out, and show me in an 8th period? I finally read the reCAPTCHA documentation so I actually understand this now, sorry about my uncertainty earlier today. And you are right you will need 2 different keys.
Once we finish the testing I will ask for all the keys for production use. I guess I didn't provide the secret key either.
Also, please get the CI checks to pass using pre-commit, and squash your commits into one.
| auth_form=form, | ||
| added_context={"auth_message": "reCAPTCHA verification failed. Please try again."}, | ||
| ) | ||
| except requests.exceptions.RequestException as e: |
There was a problem hiding this comment.
If it fails to connect to reCAPTCHA, let's let the login proceed without it, just in case something weird is going on we don't want to lock out everyone. It should send an email to [email protected] though saying reCAPTCHA can't be reached, and also possibly send a warning error after logging in saying that recaptcha couldn't be reached.
| data={ | ||
| "secret": secret_key_to_use, | ||
| "response": recaptcha_response, | ||
| "remoteip": request.META.get("REMOTE_ADDR"), |
There was a problem hiding this comment.
do this:
if "HTTP_X_REAL_IP" in request.META:
ip = request.META["HTTP_X_REAL_IP"]
else:
ip = (request.META.get("REMOTE_ADDR", ""),)
if isinstance(ip, set):
ip = ip[0]
(from: https://github.com/tjcsl/ion/blob/dev/intranet/middleware/access_log.py)
if you want you could abstract this to a helper function and replace all instances in Ion accessing the IP with that helper function
this is needed because Ion is behind a reverse proxy
| @@ -32,6 +32,17 @@ | |||
|
|
|||
| {% block js %} | |||
| {{ block.super }} | |||
| <script src="https://www.google.com/recaptcha/api.js" async defer></script> | |||
| <script> | |||
There was a problem hiding this comment.
What happens if JS is disabled? does it block the user from logging in? I think that would be the best way to go
There was a problem hiding this comment.
Oh right, it does because the backend checks for g-recaptcha thing right
|
Any updates on this @ElijahFeldman7 ? |
|
I’ve been busy the past few weeks but I can start implementing the changes and testing next week. |
|
Sure, no rush |
Note : add RECAPTCHA_SECRET_KEY and RECAPTCHA_SITE_KEY to secret.py
Proposed changes
Brief description of rationale