-
Notifications
You must be signed in to change notification settings - Fork 166
LG-8860: Add reCAPTCHA fallback error screen #7826
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
1997092
c5eafb8
39dd204
b0e8096
7a67be7
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 |
|---|---|---|
|
|
@@ -21,7 +21,8 @@ class NewPhoneForm | |
| :otp_delivery_preference, | ||
| :otp_make_default_number, | ||
| :setup_voice_preference, | ||
| :recaptcha_token | ||
| :recaptcha_token, | ||
| :recaptcha_version | ||
|
|
||
| alias_method :setup_voice_preference?, :setup_voice_preference | ||
|
|
||
|
|
@@ -31,6 +32,7 @@ def initialize(user:, analytics: nil, setup_voice_preference: false) | |
| @otp_delivery_preference = user.otp_delivery_preference | ||
| @otp_make_default_number = false | ||
| @setup_voice_preference = setup_voice_preference | ||
| @recaptcha_version = 3 | ||
| end | ||
|
|
||
| def submit(params) | ||
|
|
@@ -129,7 +131,7 @@ def validate_not_premium_rate | |
| end | ||
|
|
||
| def validate_recaptcha_token | ||
| return if !FeatureManagement.phone_recaptcha_enabled? | ||
| return if !phone_recaptcha_enabled? | ||
| return if recaptcha_validator.valid?(recaptcha_token) | ||
| errors.add( | ||
| :recaptcha_token, | ||
|
|
@@ -139,7 +141,15 @@ def validate_recaptcha_token | |
| end | ||
|
|
||
| def recaptcha_validator | ||
| @recaptcha_validator ||= PhoneRecaptchaValidator.new(parsed_phone:, analytics:) | ||
| @recaptcha_validator ||= PhoneRecaptchaValidator.new( | ||
| parsed_phone:, | ||
| recaptcha_version:, | ||
| analytics:, | ||
| ) | ||
| end | ||
|
|
||
| def phone_recaptcha_enabled? | ||
| FeatureManagement.phone_recaptcha_enabled? | ||
| end | ||
|
|
||
| def parsed_phone | ||
|
|
@@ -155,6 +165,7 @@ def ingest_submitted_params(params) | |
| @otp_delivery_preference = delivery_prefs if delivery_prefs | ||
| @otp_make_default_number = true if default_prefs | ||
| @recaptcha_token = params[:recaptcha_token] | ||
| @recaptcha_version = 2 if params[:recaptcha_version].to_i == 2 | ||
|
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. is there a reason we have to hardcode it like this and not let recaptcha version be passed in?
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. The main idea was to limit the set of potential values we'd allow for constructing the
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. I think I'd like to keep this in the controller, but I also added some extra checks to the validator class in e4f0e1ac6 to add extra assurances that it's initialized correctly. |
||
| end | ||
|
|
||
| def confirmed_phone? | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,13 +1,19 @@ | ||
| class RecaptchaValidator | ||
| VERIFICATION_ENDPOINT = 'https://www.google.com/recaptcha/api/siteverify'.freeze | ||
|
|
||
| EXEMPT_ERROR_CODES = ['missing-input-secret', 'invalid-input-secret'] | ||
| VALID_RECAPTCHA_VERSIONS = [2, 3] | ||
|
|
||
| attr_reader :recaptcha_version, :score_threshold, :analytics | ||
|
|
||
| attr_reader :score_threshold, :analytics | ||
| def initialize(recaptcha_version: 3, score_threshold: 0.0, analytics: nil) | ||
| if !VALID_RECAPTCHA_VERSIONS.include?(recaptcha_version) | ||
| raise ArgumentError, "Invalid reCAPTCHA version #{recaptcha_version}, expected one of " \ | ||
| "#{VALID_RECAPTCHA_VERSIONS}" | ||
| end | ||
|
|
||
| def initialize(score_threshold: 0.0, analytics: nil) | ||
| @score_threshold = score_threshold | ||
| @analytics = analytics | ||
| @recaptcha_version = recaptcha_version | ||
| end | ||
|
|
||
| def exempt? | ||
|
|
@@ -20,10 +26,7 @@ def valid?(recaptcha_token) | |
|
|
||
| response = faraday.post( | ||
| VERIFICATION_ENDPOINT, | ||
| URI.encode_www_form( | ||
| secret: IdentityConfig.store.recaptcha_secret_key, | ||
| response: recaptcha_token, | ||
| ), | ||
| URI.encode_www_form(secret: recaptcha_secret_key, response: recaptcha_token), | ||
| ) do |request| | ||
| request.options.context = { service_name: 'recaptcha' } | ||
| end | ||
|
|
@@ -45,21 +48,45 @@ def faraday | |
| end | ||
|
|
||
| def recaptcha_result_valid?(recaptcha_result) | ||
| if recaptcha_result.blank? | ||
| true | ||
| elsif recaptcha_result['success'] | ||
| recaptcha_result['score'] >= score_threshold | ||
| return true if recaptcha_result.blank? | ||
|
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. I refactored this a bit in a7018b44a to extract a few methods for readability, based on team review discussion yesterday that the implied |
||
|
|
||
| success, score, error_codes = recaptcha_result.values_at('success', 'score', 'error-codes') | ||
| if success | ||
| recaptcha_score_meets_threshold?(score) | ||
| else | ||
| (recaptcha_result['error-codes'] - EXEMPT_ERROR_CODES).blank? | ||
| recaptcha_errors_exempt?(error_codes) | ||
| end | ||
| end | ||
|
|
||
| def recaptcha_score_meets_threshold?(score) | ||
| case recaptcha_version | ||
| when 2 | ||
| true | ||
| when 3 | ||
| score >= score_threshold | ||
| end | ||
| end | ||
|
|
||
| def recaptcha_errors_exempt?(error_codes) | ||
| (error_codes - EXEMPT_ERROR_CODES).blank? | ||
| end | ||
|
|
||
| def log_analytics(recaptcha_result: nil, error: nil) | ||
| analytics&.recaptcha_verify_result_received( | ||
| recaptcha_result:, | ||
| score_threshold:, | ||
| recaptcha_version:, | ||
| evaluated_as_valid: recaptcha_result_valid?(recaptcha_result), | ||
| exception_class: error&.class&.name, | ||
| ) | ||
| end | ||
|
|
||
| def recaptcha_secret_key | ||
| case recaptcha_version | ||
| when 2 | ||
| IdentityConfig.store.recaptcha_secret_key_v2 | ||
| when 3 | ||
| IdentityConfig.store.recaptcha_secret_key_v3 | ||
| end | ||
| end | ||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,43 @@ | ||
| <% title t('titles.spam_protection') %> | ||
|
|
||
| <%= render PageHeadingComponent.new.with_content(t('titles.spam_protection')) %> | ||
|
|
||
| <p><%= t('forms.spam_protection.description') %></p> | ||
|
|
||
| <%= simple_form_for(@new_phone_form, url: request.url, method: :post) do |f| %> | ||
| <%= f.input :phone, as: :hidden %> | ||
| <%= f.input :international_code, as: :hidden %> | ||
| <%= f.input :otp_delivery_preference, as: :hidden %> | ||
| <%= f.input :otp_make_default_number, as: :hidden %> | ||
| <%= f.input :recaptcha_version, as: :hidden, input_html: { value: 2 } %> | ||
|
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. is this fallback page always version 2? is there a way to set this fro a controller or local variable from the form? just to minimize the number of places we hardcode things?
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. Yes it is. Although interestingly, Google enforces that the reCAPTCHA keys matches the configured version for the frontend display of the checkbox, but it appears that any reCAPTCHA key can be used for the verification endpoint. There might be some option to configure this in the controller. One interesting thing is that all of these values will carry over from the previous submission, including
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. I think that alternative ends up looking like adding a line before this: ...as: @new_phone_form.recaptcha_version = 2...but it feels a little awkward / unpredictable to modify the form like that ahead of rendering the view? I guess contrasted with how it's implemented here, I'm a bit more comfortable with forcefully overriding the value in the view. |
||
| <%= f.input :recaptcha_token, as: :hidden, input_html: { id: :recaptcha_token } %> | ||
| <%= javascript_tag(nonce: true) do %> | ||
| function onCaptchaResponse(token) { | ||
| const input = document.getElementById('recaptcha_token'); | ||
| input.value = token; | ||
| input.closest('form').submit(); | ||
| } | ||
| <% end %> | ||
| <div | ||
| class="g-recaptcha" | ||
| data-sitekey="<%= IdentityConfig.store.recaptcha_site_key_v2 %>" | ||
| data-callback="onCaptchaResponse" | ||
| ></div> | ||
| <script src="https://www.google.com/recaptcha/api.js" async></script> | ||
| <% end %> | ||
|
|
||
| <%= render TroubleshootingOptionsComponent.new do |c| %> | ||
| <% c.header { t('components.troubleshooting_options.default_heading') } %> | ||
| <% if local_assigns[:two_factor_options_path].present? %> | ||
| <% c.option( | ||
| url: two_factor_options_path, | ||
| ).with_content(t('two_factor_authentication.login_options_link_text')) %> | ||
| <% end %> | ||
| <% c.option( | ||
| url: MarketingSite.help_center_article_url( | ||
| category: 'get-started', | ||
| article: 'authentication-options', | ||
| ), | ||
| new_tab: true, | ||
| ).with_content(t('two_factor_authentication.phone_verification.troubleshooting.learn_more')) %> | ||
| <% 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.
just thinking out loud, if this new template is so different from existing stuff, should we add a new controller for it? so we redirect to
users/phone_setup/spam_production#indexor something?counterpoint, would we be able to easily link back to the phone setup from that new controller if we did?
Uh oh!
There was an error while loading. Please reload this page.
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 noodled on a few different approaches here. One key thing that is hard to do with other approaches is maintaining form values between the initial submission and the subsequent checkbox submission. As implemented here without a redirect, those form values carry over automatically. With a redirect, we'd have to find some other way to save those values (e.g. storing them in session).