Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 7 additions & 7 deletions app/forms/new_phone_form.rb
Original file line number Diff line number Diff line change
Expand Up @@ -131,20 +131,20 @@ def validate_not_premium_rate

def validate_recaptcha_token
return if !validate_recaptcha_token?
recaptcha_validator.submit(recaptcha_token)
errors.merge!(recaptcha_validator)
recaptcha_form.submit(recaptcha_token)
errors.merge!(recaptcha_form)
end

def recaptcha_validator
@recaptcha_validator ||= PhoneRecaptchaValidator.new(parsed_phone:, **recaptcha_validator_args)
def recaptcha_form
@recaptcha_form ||= PhoneRecaptchaForm.new(parsed_phone:, **recaptcha_form_args)
end

def recaptcha_validator_args
def recaptcha_form_args
args = { analytics: }
if IdentityConfig.store.phone_recaptcha_mock_validator
args.merge(validator_class: RecaptchaMockValidator, score: recaptcha_mock_score)
args.merge(form_class: RecaptchaMockForm, score: recaptcha_mock_score)
elsif FeatureManagement.recaptcha_enterprise?
args.merge(validator_class: RecaptchaEnterpriseValidator)
args.merge(form_class: RecaptchaEnterpriseForm)
else
args
end
Expand Down
Original file line number Diff line number Diff line change
@@ -1,16 +1,16 @@
# frozen_string_literal: true

class PhoneRecaptchaValidator
class PhoneRecaptchaForm
RECAPTCHA_ACTION = 'phone_setup'

attr_reader :parsed_phone, :validator_class, :validator_args
attr_reader :parsed_phone, :form_class, :form_args

delegate :submit, :errors, to: :validator
delegate :submit, :errors, to: :form

def initialize(parsed_phone:, validator_class: RecaptchaValidator, **validator_args)
def initialize(parsed_phone:, form_class: RecaptchaForm, **form_args)
@parsed_phone = parsed_phone
@validator_class = validator_class
@validator_args = validator_args
@form_class = form_class
@form_args = form_args
end

def self.country_score_overrides
Expand All @@ -23,14 +23,14 @@ def score_threshold

private

def validator
@validator ||= validator_class.new(
def form
@form ||= form_class.new(
score_threshold:,
recaptcha_action: RECAPTCHA_ACTION,
extra_analytics_properties: {
phone_country_code: parsed_phone.country,
},
**validator_args,
**form_args,
)
end

Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
# frozen_string_literal: true

class RecaptchaEnterpriseValidator < RecaptchaValidator
class RecaptchaEnterpriseForm < RecaptchaForm
BASE_VERIFICATION_ENDPOINT = 'https://recaptchaenterprise.googleapis.com/v1/projects'

private
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
# frozen_string_literal: true

class RecaptchaValidator
class RecaptchaForm
include ActiveModel::Model
include ActionView::Helpers::TranslationHelper

Expand Down Expand Up @@ -103,7 +103,7 @@ def log_analytics(result: nil, error: nil)
score_threshold:,
evaluated_as_valid: recaptcha_result_valid?(result),
exception_class: error&.class&.name,
validator_class: self.class.name,
form_class: self.class.name,
**extra_analytics_properties,
)
end
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
# frozen_string_literal: true

class RecaptchaMockValidator < RecaptchaValidator
class RecaptchaMockForm < RecaptchaForm
attr_reader :score

def initialize(score:, **kwargs)
Expand Down
6 changes: 3 additions & 3 deletions app/services/analytics_events.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4639,14 +4639,14 @@ def reactivate_account_visit
# @param [Hash] recaptcha_result Full reCAPTCHA response body
# @param [Float] score_threshold Minimum value for considering passing result
# @param [Boolean] evaluated_as_valid Whether result was considered valid
# @param [String] validator_class Class name of validator
# @param [String] form_class Class name of form
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd consider leaving this so it's easier to track over time?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Normally I'd agree if we were already using this in dashboards or saved queries, but I think this was added primarily as a way to diagnose individual log entries, so I don't know that I'm quite as concerned about backwards compatibility?

Copy link
Contributor

Choose a reason for hiding this comment

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

makes sense then

# @param [String, nil] exception_class Class name of exception, if error occurred
# @param [String, nil] phone_country_code Country code associated with reCAPTCHA phone result
def recaptcha_verify_result_received(
recaptcha_result:,
score_threshold:,
evaluated_as_valid:,
validator_class:,
form_class:,
exception_class:,
phone_country_code: nil,
**extra
Expand All @@ -4657,7 +4657,7 @@ def recaptcha_verify_result_received(
recaptcha_result:,
score_threshold:,
evaluated_as_valid:,
validator_class:,
form_class:,
exception_class:,
phone_country_code:,
**extra,
Expand Down
2 changes: 1 addition & 1 deletion app/views/users/phone_setup/index.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@

<%= render CaptchaSubmitButtonComponent.new(
form: f,
action: PhoneRecaptchaValidator::RECAPTCHA_ACTION,
action: PhoneRecaptchaForm::RECAPTCHA_ACTION,
).with_content(t('forms.buttons.send_one_time_code')) %>
<% end %>

Expand Down
4 changes: 2 additions & 2 deletions knapsack_rspec_report.json
Original file line number Diff line number Diff line change
Expand Up @@ -783,7 +783,7 @@
"spec/services/personal_key_generator_spec.rb": 0.320518645,
"spec/services/phone_formatter_spec.rb": 0.030768014,
"spec/services/phone_number_capabilities_spec.rb": 0.104795072,
"spec/services/phone_recaptcha_validator_spec.rb": 0.065012571,
"spec/services/phone_recaptcha_form_spec.rb": 0.065012571,
"spec/services/pii/attributes_spec.rb": 0.026663207,
"spec/services/pii/cacher_spec.rb": 0.328829647,
"spec/services/pii/fingerprinter_spec.rb": 0.026187948,
Expand Down Expand Up @@ -840,7 +840,7 @@
"spec/services/reactivate_account_session_spec.rb": 0.486191592,
"spec/services/recaptcha_enterprise_validator_spec.rb": 0.184840574,
"spec/services/recaptcha_mock_validator_spec.rb": 0.031384338,
"spec/services/recaptcha_validator_spec.rb": 0.207950607,
"spec/services/recaptcha_form_spec.rb": 0.207950607,
"spec/services/redis_rate_limiter_spec.rb": 0.045844488,
"spec/services/remember_device_cookie_spec.rb": 0.144756036,
"spec/services/reporting/account_deletion_rate_report_spec.rb": 0.287856172,
Expand Down
16 changes: 8 additions & 8 deletions spec/forms/new_phone_form_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -374,8 +374,8 @@
end

context 'with recaptcha enabled' do
let(:errors) { ActiveModel::Errors.new(RecaptchaValidator.new) }
let(:validator) { PhoneRecaptchaValidator.new(parsed_phone: nil) }
let(:errors) { ActiveModel::Errors.new(RecaptchaForm.new) }
let(:recaptcha_form) { PhoneRecaptchaForm.new(parsed_phone: nil) }
let(:recaptcha_token) { 'token' }
let(:phone) { '3065550100' }
let(:international_code) { 'CA' }
Expand All @@ -385,9 +385,9 @@

before do
allow(FeatureManagement).to receive(:phone_recaptcha_enabled?).and_return(true)
allow(validator).to receive(:submit).with(recaptcha_token)
allow(validator).to receive(:errors).and_return(errors)
allow(form).to receive(:recaptcha_validator).and_return(validator)
allow(recaptcha_form).to receive(:submit).with(recaptcha_token)
allow(recaptcha_form).to receive(:errors).and_return(errors)
allow(form).to receive(:recaptcha_form).and_return(recaptcha_form)
end

context 'with valid recaptcha result' do
Expand All @@ -397,9 +397,9 @@
end

context 'with recaptcha enterprise' do
let(:validator) do
PhoneRecaptchaValidator.new(
validator_class: RecaptchaEnterpriseValidator,
let(:recaptcha_form) do
PhoneRecaptchaForm.new(
form_class: RecaptchaEnterpriseForm,
parsed_phone: nil,
)
end
Expand Down
Original file line number Diff line number Diff line change
@@ -1,24 +1,24 @@
require 'rails_helper'

RSpec.describe PhoneRecaptchaValidator do
RSpec.describe PhoneRecaptchaForm do
let(:country_score_overrides_config) { {} }
let(:score_threshold_config) { 0.2 }
let(:parsed_phone) { Phonelib.parse('+15135551234') }
let(:analytics) { FakeAnalytics.new }
subject(:validator) { described_class.new(parsed_phone:, analytics:) }
subject(:form) { described_class.new(parsed_phone:, analytics:) }
before do
allow(IdentityConfig.store).to receive(:phone_recaptcha_country_score_overrides).
and_return(country_score_overrides_config)
allow(IdentityConfig.store).to receive(:phone_recaptcha_score_threshold).
and_return(score_threshold_config)
end

it 'passes instance variables to validator' do
recaptcha_validator = instance_double(
RecaptchaValidator,
it 'passes instance variables to form' do
recaptcha_form = instance_double(
RecaptchaForm,
submit: FormResponse.new(success: true),
)
expect(RecaptchaValidator).to receive(:new).
expect(RecaptchaForm).to receive(:new).
with(
score_threshold: score_threshold_config,
analytics:,
Expand All @@ -27,52 +27,52 @@
phone_country_code: parsed_phone.country,
},
).
and_return(recaptcha_validator)
and_return(recaptcha_form)

validator.submit('token')
form.submit('token')
end

context 'with custom recaptcha validator class' do
subject(:validator) do
context 'with custom recaptcha form class' do
subject(:form) do
described_class.new(
parsed_phone:,
analytics:,
validator_class: RecaptchaMockValidator,
form_class: RecaptchaMockForm,
)
end

it 'delegates to validator instance of the given class' do
recaptcha_validator = instance_double(
RecaptchaValidator,
it 'delegates to form instance of the given class' do
recaptcha_form = instance_double(
RecaptchaForm,
submit: FormResponse.new(success: true),
)
expect(RecaptchaMockValidator).to receive(:new).and_return(recaptcha_validator)
expect(recaptcha_validator).to receive(:submit)
expect(RecaptchaMockForm).to receive(:new).and_return(recaptcha_form)
expect(recaptcha_form).to receive(:submit)

validator.submit('token')
form.submit('token')
end
end

describe '#submit' do
it 'is delegated to recaptcha validator' do
recaptcha_validator = instance_double(
RecaptchaValidator,
it 'is delegated to recaptcha form' do
recaptcha_form = instance_double(
RecaptchaForm,
submit: FormResponse.new(success: true),
)
expect(validator).to receive(:validator).and_return(recaptcha_validator)
expect(recaptcha_validator).to receive(:submit)
expect(form).to receive(:form).and_return(recaptcha_form)
expect(recaptcha_form).to receive(:submit)

validator.submit('token')
form.submit('token')
end
end

describe '#errors' do
it 'is delegated to recaptcha validator' do
recaptcha_validator = instance_double(RecaptchaValidator, errors: ActiveModel::Errors.new({}))
expect(validator).to receive(:validator).and_return(recaptcha_validator)
expect(recaptcha_validator).to receive(:errors)
it 'is delegated to recaptcha form' do
recaptcha_form = instance_double(RecaptchaForm, errors: ActiveModel::Errors.new({}))
expect(form).to receive(:form).and_return(recaptcha_form)
expect(recaptcha_form).to receive(:errors)

validator.errors
form.errors
end
end

Expand All @@ -85,7 +85,7 @@
end

describe '#score_threshold' do
subject(:score_threshold) { validator.score_threshold }
subject(:score_threshold) { form.score_threshold }

context 'without country override' do
it 'returns default score threshold configuration value' do
Expand Down
Loading