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

Add support for invisible reCAPTCHAs #8258

Merged

Conversation

jimchamp
Copy link
Collaborator

@jimchamp jimchamp commented Sep 1, 2023

Closes #7674

Replaces our image-based reCAPTCHA with an invisible reCAPTCHA. Not yet tested.

Edit: I was able to create an account on testing.

Technical

Testing

Screenshot

Stakeholders

@jimchamp jimchamp added State: Blocked Work has stopped, waiting for something (Info, Dependent fix, etc. See comments). [managed] Needs: Testing labels Sep 1, 2023
@codecov-commenter
Copy link

codecov-commenter commented Sep 1, 2023

Codecov Report

Merging #8258 (dcc72e7) into master (159fd3e) will increase coverage by 0.02%.
The diff coverage is 14.28%.

@@            Coverage Diff             @@
##           master    #8258      +/-   ##
==========================================
+ Coverage   15.93%   15.95%   +0.02%     
==========================================
  Files          86       86              
  Lines        4707     4707              
  Branches      826      827       +1     
==========================================
+ Hits          750      751       +1     
+ Misses       3431     3430       -1     
  Partials      526      526              
Files Coverage Δ
...gins/openlibrary/js/realtime_account_validation.js 34.84% <14.28%> (+1.51%) ⬆️

@mekarpeles mekarpeles self-assigned this Sep 5, 2023
@mekarpeles mekarpeles added On testing.openlibrary.org Priority: 2 Important, as time permits. [managed] and removed State: Blocked Work has stopped, waiting for something (Info, Dependent fix, etc. See comments). [managed] labels Sep 18, 2023
@cdrini
Copy link
Collaborator

cdrini commented Sep 25, 2023

Tried to create an account on testing ; the invisible captcha floating button thing appeared, but clicking "sign up" caused the button to become disabled for a bit, and then it became re-enabled? Didn't receive any email.

@cdrini cdrini added the Needs: Submitter Input Waiting on input from the creator of the issue/pr [managed] label Sep 25, 2023
@cdrini cdrini marked this pull request as draft November 6, 2023 21:19
@jimchamp jimchamp force-pushed the 7674/feature/invisible-recaptcha branch from 5bf5357 to dcc72e7 Compare November 7, 2023 21:43
@jimchamp jimchamp marked this pull request as ready for review November 7, 2023 21:46
@jimchamp jimchamp added On testing.openlibrary.org and removed Needs: Submitter Input Waiting on input from the creator of the issue/pr [managed] labels Nov 7, 2023
@mekarpeles
Copy link
Member

This code looks like it work! One weird edge case with aliases which I think we fixed:

diff --git a/openlibrary/accounts/model.py b/openlibrary/accounts/model.py
index 7f0126942..c3862e244 100644
--- a/openlibrary/accounts/model.py
+++ b/openlibrary/accounts/model.py
@@ -657,20 +657,19 @@ class InternetArchiveAccount(web.storage):
                 service='openlibrary',
             )
 
+            vs = response.get('values', {})
+            vsj = ' '.join(vs.values())
             if response.get('success'):
                 ia_account = cls.get(email=email)
                 if test:
                     ia_account.test = True
                 return ia_account
-
-            elif 'screenname' not in response.get('values', {}):
-                errors = '_'.join(response.get('values', {}))
-                raise ValueError(errors)
-
+            # If we have errors besides taken screenname, abort
+            elif list(vs) != ['screenname']:
+                raise ValueError(vsj)
+            # If we're out of retries
             elif attempt >= retries:
-                ve = ValueError('username_registered')
-                ve.value = _screenname
-                raise ve
+                raise ValueError(vsj)
 
             _screenname = append_random_suffix(screenname)
             attempt += 1
diff --git a/openlibrary/plugins/upstream/account.py b/openlibrary/plugins/upstream/account.py
index c8956cde2..2bd25f049 100644
--- a/openlibrary/plugins/upstream/account.py
+++ b/openlibrary/plugins/upstream/account.py
@@ -283,7 +283,9 @@ class account_create(delegate.page):
                 """  # nopep8
                 mls = ['ml_best_of', 'ml_updates']
                 notifications = mls if f.ia_newsletter.checked else []
-                InternetArchiveAccount.create(
+                email = f.email
+                email_val = email.value
+                r = InternetArchiveAccount.create(
                     screenname=f.username.value,
                     email=f.email.value,
                     password=f.password.value,
@@ -294,8 +296,8 @@ class account_create(delegate.page):
                 return render['account/verify'](
                     username=f.username.value, email=f.email.value
                 )
-            except ValueError:
-                f.note = LOGIN_ERRORS['max_retries_exceeded']
+            except ValueError as ve:
+                f.note = str(ve)
 
         return render['account/create'](f)

@mekarpeles mekarpeles added the Needs: Submitter Input Waiting on input from the creator of the issue/pr [managed] label Nov 13, 2023
@mekarpeles
Copy link
Member

Code tested and works, there are some challenges with alias accounts i.e. mek+blah@archive.org but I believe that can be handled in a separate PR which addresses #8258 (comment)

@mekarpeles mekarpeles merged commit 090a96b into internetarchive:master Nov 13, 2023
3 checks passed
@jimchamp jimchamp deleted the 7674/feature/invisible-recaptcha branch November 16, 2023 17:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs: Submitter Input Waiting on input from the creator of the issue/pr [managed] Needs: Testing Priority: 2 Important, as time permits. [managed]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Invisible reCaptcha
4 participants