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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proof-of-concept: reCAPTCHA at sign-in #10587

Closed
wants to merge 3 commits into from
Closed

Conversation

aduth
Copy link
Member

@aduth aduth commented May 10, 2024

馃洜 Summary of changes

Implements a proof-of-concept reCAPTCHA validation at sign-in.

馃摐 Testing Plan

  1. Go to http://localhost:3000
  2. Enter email and password
  3. (Optional) Customize "reCAPTCHA score" in the mock debugger tool
  4. Click "Sign in"
  5. Observe:
    • If you have already signed in on this device, you are allowed to proceed regardless of the score entered in Step 3
    • If you haven't already signed in on this device and you enter a score at or above 0.3, you are allowed to proceed
    • Otherwise, you are presented with an error message

馃憖 Screenshots

image

@@ -79,6 +80,40 @@ def process_locked_out_session
redirect_to root_url
end

def valid_captcha_result?
if cookies[:device] && (device = Device.find_by(cookie_uuid: cookies[:device]))
return true if device.user.email_addresses.lazy.map(&:email).include?(auth_params[:email])
Copy link
Contributor

Choose a reason for hiding this comment

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

oooo smart use of .lazy! Another idea:

Suggested change
return true if device.user.email_addresses.lazy.map(&:email).include?(auth_params[:email])
return true if device.user.email_addresses.any? { |e| e.email == auth_params[:email] }

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I thought it was a nice way to avoid having to use a block, but funny that the block form ends up being shorter anyways 馃槄

elsif FeatureManagement.recaptcha_enterprise?
args.merge(form_class: RecaptchaEnterpriseForm)
else
args
Copy link
Contributor

Choose a reason for hiding this comment

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

should we default to RecaptchaForm for completeness?

Suggested change
args
args.merge(form_class: RecaptchaForm)

Copy link
Member Author

Choose a reason for hiding this comment

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

It's defaulted in SignInRecaptchaForm, so not strictly necessary:

def initialize(form_class: RecaptchaForm, **form_args)

This follows from the implementation for phone setup:

def recaptcha_form_args
args = { analytics: }
if IdentityConfig.store.phone_recaptcha_mock_validator
args.merge(form_class: RecaptchaMockForm, score: recaptcha_mock_score)
elsif FeatureManagement.recaptcha_enterprise?
args.merge(form_class: RecaptchaEnterpriseForm)
else
args
end
end

But I could also change it so that form_class is a required keyword attribute and assign it here instead.

(Aside: When implementing this "proper", I'll probably plan to create a separate form class for handling the sign-in+reCAPTCHA validation, similar to what we have with NewPhoneForm)

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah seeing the default argument there is what prompted me to make the comment here

@@ -1,14 +1,15 @@
# frozen_string_literal: true

class CaptchaSubmitButtonComponent < BaseComponent
attr_reader :form, :action, :tag_options
attr_reader :form, :action, :button_options, :tag_options
Copy link
Contributor

Choose a reason for hiding this comment

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

this is the recaptcha_action not the older link-or-button action right? What if we renamed to recapcha_action tomatch the RECAPTCHA_ACTION constant it gets called with?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, it's the reCAPTCHA concept of "action", also documented with a reference link a couple lines below.

# @param [String] action https://developers.google.com/recaptcha/docs/v3#actions

But sure, I think renaming recaptcha_action could be clearer / more consistent.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also, that link should probably reference the Enterprise documentation, which is a little more complete and matches the expected production behavior:

https://cloud.google.com/recaptcha-enterprise/docs/actions-website

@@ -79,6 +80,40 @@ def process_locked_out_session
redirect_to root_url
end

def valid_captcha_result?
if cookies[:device] && (device = Device.find_by(cookie_uuid: cookies[:device]))
Copy link
Contributor

Choose a reason for hiding this comment

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

Discussed a little bit offline, but this query should probably include a subquery on email address for the user_id since cookie_uuid is not unique.

@aduth
Copy link
Member Author

aduth commented May 15, 2024

This proof-of-concept has served its purpose. I'll close this for now, but it's likely we'll use parts of this for future reference.

@aduth aduth closed this May 15, 2024
@aduth aduth deleted the aduth-poc-recaptcha-sign-in branch May 15, 2024 16:38
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