-
Notifications
You must be signed in to change notification settings - Fork 167
Proof-of-concept: reCAPTCHA at sign-in #10587
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -31,6 +31,7 @@ def create | |||||||||||||||||||||||||||
| session[:sign_in_flow] = :sign_in | ||||||||||||||||||||||||||||
| return process_locked_out_session if session_bad_password_count_max_exceeded? | ||||||||||||||||||||||||||||
| return process_locked_out_user if current_user && user_locked_out?(current_user) | ||||||||||||||||||||||||||||
| return process_failed_captcha if !valid_captcha_result? | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| rate_limit_password_failure = true | ||||||||||||||||||||||||||||
| self.resource = warden.authenticate!(auth_options) | ||||||||||||||||||||||||||||
|
|
@@ -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])) | ||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||||||||||||||||||||||||||||
| return true if device.user.email_addresses.lazy.map(&:email).include?(auth_params[:email]) | ||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oooo smart use of
Suggested change
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 😅 |
||||||||||||||||||||||||||||
| end | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| response, _assessment_id = recaptcha_form.submit(params.require(:user)[:recaptcha_token]) | ||||||||||||||||||||||||||||
| flash[:error] = response.first_error_message if !response.success? | ||||||||||||||||||||||||||||
| response.success? | ||||||||||||||||||||||||||||
| end | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| def process_failed_captcha | ||||||||||||||||||||||||||||
| warden.logout(:user) | ||||||||||||||||||||||||||||
| warden.lock! | ||||||||||||||||||||||||||||
| redirect_to root_url | ||||||||||||||||||||||||||||
| end | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| def recaptcha_form | ||||||||||||||||||||||||||||
| @recaptcha_form ||= SignInRecaptchaForm.new(**recaptcha_form_args) | ||||||||||||||||||||||||||||
| end | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| def recaptcha_form_args | ||||||||||||||||||||||||||||
| args = { analytics: } | ||||||||||||||||||||||||||||
| if IdentityConfig.store.phone_recaptcha_mock_validator | ||||||||||||||||||||||||||||
| args.merge( | ||||||||||||||||||||||||||||
| form_class: RecaptchaMockForm, | ||||||||||||||||||||||||||||
| score: params.require(:user)[:recaptcha_mock_score].to_f, | ||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||
| elsif FeatureManagement.recaptcha_enterprise? | ||||||||||||||||||||||||||||
| args.merge(form_class: RecaptchaEnterpriseForm) | ||||||||||||||||||||||||||||
| else | ||||||||||||||||||||||||||||
| args | ||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should we default to
Suggested change
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's defaulted in
This follows from the implementation for phone setup: identity-idp/app/forms/new_phone_form.rb Lines 144 to 153 in 37afb7e
But I could also change it so that (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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||||||||||||||||||||||||||||
| end | ||||||||||||||||||||||||||||
| end | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| def redirect_to_signin | ||||||||||||||||||||||||||||
| controller_info = 'users/sessions#create' | ||||||||||||||||||||||||||||
| analytics.invalid_authenticity_token(controller: controller_info) | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| # frozen_string_literal: true | ||
|
|
||
| class SignInRecaptchaForm | ||
| RECAPTCHA_ACTION = 'sign_in' | ||
|
|
||
| attr_reader :form_class, :form_args | ||
|
|
||
| delegate :submit, :errors, to: :form | ||
|
|
||
| def initialize(form_class: RecaptchaForm, **form_args) | ||
| @form_class = form_class | ||
| @form_args = form_args | ||
| end | ||
|
|
||
| private | ||
|
|
||
| def form | ||
| @form ||= form_class.new( | ||
| score_threshold: IdentityConfig.store.sign_in_recaptcha_score_threshold, | ||
| recaptcha_action: RECAPTCHA_ACTION, | ||
| **form_args, | ||
| ) | ||
| end | ||
| end |
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_actionnot the older link-or-button action right? What if we renamed torecapcha_actiontomatch theRECAPTCHA_ACTIONconstant 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.
identity-idp/app/components/captcha_submit_button_component.rb
Line 8 in 2dd7c9c
But sure, I think renaming
recaptcha_actioncould 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