-
Notifications
You must be signed in to change notification settings - Fork 113
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
Proof-of-concept: reCAPTCHA at sign-in #10587
Conversation
@@ -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]) |
There was a problem hiding this comment.
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:
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] } |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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?
args | |
args.merge(form_class: RecaptchaForm) |
There was a problem hiding this comment.
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:
identity-idp/app/forms/new_phone_form.rb
Lines 144 to 153 in 37afb7e
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
)
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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])) |
There was a problem hiding this comment.
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.
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. |
🛠 Summary of changes
Implements a proof-of-concept reCAPTCHA validation at sign-in.
📜 Testing Plan
👀 Screenshots