diff --git a/app/forms/new_phone_form.rb b/app/forms/new_phone_form.rb index a12f2c73afa..62c6aa03f10 100644 --- a/app/forms/new_phone_form.rb +++ b/app/forms/new_phone_form.rb @@ -130,12 +130,9 @@ def validate_not_premium_rate end def validate_recaptcha_token - return if !validate_recaptcha_token? || recaptcha_validator.valid?(recaptcha_token) - errors.add( - :recaptcha_token, - I18n.t('errors.messages.invalid_recaptcha_token'), - type: :invalid_recaptcha_token, - ) + return if !validate_recaptcha_token? + recaptcha_validator.submit(recaptcha_token) + errors.merge!(recaptcha_validator) end def recaptcha_validator diff --git a/app/services/phone_recaptcha_validator.rb b/app/services/phone_recaptcha_validator.rb index 65dbe978826..2878b1265d6 100644 --- a/app/services/phone_recaptcha_validator.rb +++ b/app/services/phone_recaptcha_validator.rb @@ -5,7 +5,7 @@ class PhoneRecaptchaValidator attr_reader :parsed_phone, :validator_class, :validator_args - delegate :valid?, :exempt?, to: :validator + delegate :submit, :errors, to: :validator def initialize(parsed_phone:, validator_class: RecaptchaValidator, **validator_args) @parsed_phone = parsed_phone diff --git a/app/services/recaptcha_enterprise_validator.rb b/app/services/recaptcha_enterprise_validator.rb index 2430751c450..5ab494efb94 100644 --- a/app/services/recaptcha_enterprise_validator.rb +++ b/app/services/recaptcha_enterprise_validator.rb @@ -16,7 +16,7 @@ def assessment_url ) end - def recaptcha_result(recaptcha_token) + def recaptcha_result response = faraday.post( assessment_url, { diff --git a/app/services/recaptcha_mock_validator.rb b/app/services/recaptcha_mock_validator.rb index a06d4e2ffd2..eedc7c156c7 100644 --- a/app/services/recaptcha_mock_validator.rb +++ b/app/services/recaptcha_mock_validator.rb @@ -10,7 +10,7 @@ def initialize(score:, **kwargs) private - def recaptcha_result(_recaptcha_token) + def recaptcha_result RecaptchaResult.new(success: true, score:) end end diff --git a/app/services/recaptcha_validator.rb b/app/services/recaptcha_validator.rb index ceee4414ca9..6f1a0c8a91e 100644 --- a/app/services/recaptcha_validator.rb +++ b/app/services/recaptcha_validator.rb @@ -1,14 +1,21 @@ # frozen_string_literal: true class RecaptchaValidator + include ActiveModel::Model + include ActionView::Helpers::TranslationHelper + VERIFICATION_ENDPOINT = 'https://www.google.com/recaptcha/api/siteverify' RESULT_ERRORS = ['missing-input-secret', 'invalid-input-secret'].freeze attr_reader :recaptcha_action, + :recaptcha_token, :score_threshold, :analytics, :extra_analytics_properties + validate :validate_token_exists + validate :validate_recaptcha_result + RecaptchaResult = Struct.new(:success, :score, :errors, :reasons, keyword_init: true) do alias_method :success?, :success @@ -33,20 +40,30 @@ def exempt? !score_threshold.positive? end - def valid?(recaptcha_token) - return true if exempt? - return false if recaptcha_token.blank? - result = recaptcha_result(recaptcha_token) - log_analytics(result:) - recaptcha_result_valid?(result) + def submit(recaptcha_token) + @recaptcha_token = recaptcha_token + @recaptcha_result = recaptcha_result if !exempt? && recaptcha_token.present? + + log_analytics(result: @recaptcha_result) if @recaptcha_result + FormResponse.new(success: valid?, errors:, serialize_error_details_only: true) rescue Faraday::Error => error log_analytics(error:) - true + FormResponse.new(success: true, serialize_error_details_only: true) end private - def recaptcha_result(recaptcha_token) + def validate_token_exists + return if exempt? || recaptcha_token.present? + errors.add(:recaptcha_token, :blank, message: t('errors.messages.invalid_recaptcha_token')) + end + + def validate_recaptcha_result + return if @recaptcha_result.blank? || recaptcha_result_valid?(@recaptcha_result) + errors.add(:recaptcha_token, :invalid, message: t('errors.messages.invalid_recaptcha_token')) + end + + def recaptcha_result response = faraday.post( VERIFICATION_ENDPOINT, URI.encode_www_form(secret: recaptcha_secret_key, response: recaptcha_token), diff --git a/spec/forms/new_phone_form_spec.rb b/spec/forms/new_phone_form_spec.rb index b15a95b3cee..e6fdbde9596 100644 --- a/spec/forms/new_phone_form_spec.rb +++ b/spec/forms/new_phone_form_spec.rb @@ -374,7 +374,7 @@ end context 'with recaptcha enabled' do - let(:valid) { nil } + let(:errors) { ActiveModel::Errors.new(RecaptchaValidator.new) } let(:validator) { PhoneRecaptchaValidator.new(parsed_phone: nil) } let(:recaptcha_token) { 'token' } let(:phone) { '3065550100' } @@ -385,13 +385,12 @@ before do allow(FeatureManagement).to receive(:phone_recaptcha_enabled?).and_return(true) - allow(validator).to receive(:valid?).with(recaptcha_token).and_return(valid) + 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) end context 'with valid recaptcha result' do - let(:valid) { true } - it 'is valid' do expect(result.success?).to eq(true) expect(result.errors).to be_blank @@ -417,7 +416,9 @@ end context 'with invalid recaptcha result' do - let(:valid) { false } + before do + errors.add(:recaptcha_token, :fail, message: 'fail') + end it 'is invalid' do expect(result.success?).to eq(false) diff --git a/spec/services/phone_recaptcha_validator_spec.rb b/spec/services/phone_recaptcha_validator_spec.rb index ec62fc93a50..01b34ef3b2b 100644 --- a/spec/services/phone_recaptcha_validator_spec.rb +++ b/spec/services/phone_recaptcha_validator_spec.rb @@ -14,7 +14,10 @@ end it 'passes instance variables to validator' do - recaptcha_validator = instance_double(RecaptchaValidator, valid?: true) + recaptcha_validator = instance_double( + RecaptchaValidator, + submit: FormResponse.new(success: true), + ) expect(RecaptchaValidator).to receive(:new). with( score_threshold: score_threshold_config, @@ -26,7 +29,7 @@ ). and_return(recaptcha_validator) - validator.valid?('token') + validator.submit('token') end context 'with custom recaptcha validator class' do @@ -39,31 +42,37 @@ end it 'delegates to validator instance of the given class' do - recaptcha_validator = instance_double(RecaptchaMockValidator, valid?: true) + recaptcha_validator = instance_double( + RecaptchaValidator, + submit: FormResponse.new(success: true), + ) expect(RecaptchaMockValidator).to receive(:new).and_return(recaptcha_validator) - expect(recaptcha_validator).to receive(:valid?) + expect(recaptcha_validator).to receive(:submit) - validator.valid?('token') + validator.submit('token') end end - describe '#valid?' do + describe '#submit' do it 'is delegated to recaptcha validator' do - recaptcha_validator = instance_double(RecaptchaValidator, valid?: true) + recaptcha_validator = instance_double( + RecaptchaValidator, + submit: FormResponse.new(success: true), + ) expect(validator).to receive(:validator).and_return(recaptcha_validator) - expect(recaptcha_validator).to receive(:valid?) + expect(recaptcha_validator).to receive(:submit) - validator.valid?('token') + validator.submit('token') end end - describe '#exempt?' do + describe '#errors' do it 'is delegated to recaptcha validator' do - recaptcha_validator = instance_double(RecaptchaValidator, exempt?: true) + recaptcha_validator = instance_double(RecaptchaValidator, errors: ActiveModel::Errors.new({})) expect(validator).to receive(:validator).and_return(recaptcha_validator) - expect(recaptcha_validator).to receive(:exempt?) + expect(recaptcha_validator).to receive(:errors) - validator.exempt? + validator.errors end end diff --git a/spec/services/recaptcha_enterprise_validator_spec.rb b/spec/services/recaptcha_enterprise_validator_spec.rb index f2e191866d2..2bcd247e5b6 100644 --- a/spec/services/recaptcha_enterprise_validator_spec.rb +++ b/spec/services/recaptcha_enterprise_validator_spec.rb @@ -51,19 +51,21 @@ end end - describe '#valid?' do + describe '#submit' do let(:token) { nil } - subject(:valid) { validator.valid?(token) } + subject(:response) { validator.submit(token) } context 'with exemption' do before do allow(validator).to receive(:exempt?).and_return(true) end - it { expect(valid).to eq(true) } + it 'is successful' do + expect(response.to_h).to eq(success: true) + end it 'does not log analytics' do - valid + response expect(analytics).not_to have_logged_event('reCAPTCHA verify result received') end @@ -72,10 +74,15 @@ context 'with missing token' do let(:token) { nil } - it { expect(valid).to eq(false) } + it 'is unsuccessful with error for blank token' do + expect(response.to_h).to eq( + success: false, + error_details: { recaptcha_token: { blank: true } }, + ) + end it 'does not log analytics' do - valid + response expect(analytics).not_to have_logged_event('reCAPTCHA verify result received') end @@ -84,10 +91,15 @@ context 'with blank token' do let(:token) { '' } - it { expect(valid).to eq(false) } + it 'is unsuccessful with error for blank token' do + expect(response.to_h).to eq( + success: false, + error_details: { recaptcha_token: { blank: true } }, + ) + end it 'does not log analytics' do - valid + response expect(analytics).not_to have_logged_event('reCAPTCHA verify result received') end @@ -107,10 +119,15 @@ ) end - it { expect(valid).to eq(false) } + it 'is unsuccessful with error for invalid token' do + expect(response.to_h).to eq( + success: false, + error_details: { recaptcha_token: { invalid: true } }, + ) + end it 'logs analytics of the body' do - valid + response expect(analytics).to have_logged_event( 'reCAPTCHA verify result received', @@ -140,10 +157,12 @@ ) end - it { expect(valid).to eq(true) } + it 'is successful' do + expect(response.to_h).to eq(success: true) + end it 'logs analytics of the body' do - valid + response expect(analytics).to have_logged_event( 'reCAPTCHA verify result received', @@ -167,10 +186,12 @@ stub_request(:post, assessment_url).to_timeout end - it { expect(valid).to eq(true) } + it 'is successful' do + expect(response.to_h).to eq(success: true) + end it 'logs analytics of the body' do - valid + response expect(analytics).to have_logged_event( 'reCAPTCHA verify result received', @@ -198,10 +219,15 @@ ) end - it { expect(valid).to eq(false) } + it 'is unsuccessful with error for invalid token' do + expect(response.to_h).to eq( + success: false, + error_details: { recaptcha_token: { invalid: true } }, + ) + end it 'logs analytics of the body' do - valid + response expect(analytics).to have_logged_event( 'reCAPTCHA verify result received', @@ -222,8 +248,8 @@ let(:token) { 'token' } let(:score) { score_threshold + 0.1 } - before do - stub_recaptcha_response( + around do |example| + stubbed_request = stub_recaptcha_response( body: { tokenProperties: { valid: true, action: }, riskAnalysis: { score:, reasons: ['LOW_CONFIDENCE'] }, @@ -232,12 +258,16 @@ action:, token:, ) + example.run + expect(stubbed_request).to have_been_made.once end - it { expect(valid).to eq(true) } + it 'is successful' do + expect(response.to_h).to eq(success: true) + end it 'logs analytics of the body' do - valid + response expect(analytics).to have_logged_event( 'reCAPTCHA verify result received', @@ -266,14 +296,19 @@ ) end - it { expect(valid).to eq(false) } + it 'is unsuccessful with error for invalid token' do + expect(response.to_h).to eq( + success: false, + error_details: { recaptcha_token: { invalid: true } }, + ) + end end context 'with extra analytics properties', allowed_extra_analytics: [:extra] do let(:extra_analytics_properties) { { extra: true } } it 'logs analytics of the body' do - valid + response expect(analytics).to have_logged_event( 'reCAPTCHA verify result received', @@ -295,7 +330,7 @@ let(:analytics) { nil } it 'validates gracefully without analytics logging' do - valid + response end end end diff --git a/spec/services/recaptcha_mock_validator_spec.rb b/spec/services/recaptcha_mock_validator_spec.rb index 84b461f0a47..7e81436972e 100644 --- a/spec/services/recaptcha_mock_validator_spec.rb +++ b/spec/services/recaptcha_mock_validator_spec.rb @@ -12,17 +12,22 @@ freeze_time { example.run } end - describe '#valid?' do + describe '#submit' do let(:token) { 'token' } - subject(:valid) { validator.valid?(token) } + subject(:response) { validator.submit(token) } context 'with failing score from validation service' do let(:score) { score_threshold - 0.1 } - it { expect(valid).to eq(false) } + it 'is unsuccessful with error for invalid token' do + expect(response.to_h).to eq( + success: false, + error_details: { recaptcha_token: { invalid: true } }, + ) + end it 'logs analytics of the body' do - valid + response expect(analytics).to have_logged_event( 'reCAPTCHA verify result received', @@ -43,10 +48,12 @@ let(:token) { 'token' } let(:score) { score_threshold + 0.1 } - it { expect(valid).to eq(true) } + it 'is successful' do + expect(response.to_h).to eq(success: true) + end it 'logs analytics of the body' do - valid + response expect(analytics).to have_logged_event( 'reCAPTCHA verify result received', @@ -66,7 +73,7 @@ let(:analytics) { nil } it 'validates gracefully without analytics logging' do - valid + response end end end diff --git a/spec/services/recaptcha_validator_spec.rb b/spec/services/recaptcha_validator_spec.rb index d475410e00e..2b433756c00 100644 --- a/spec/services/recaptcha_validator_spec.rb +++ b/spec/services/recaptcha_validator_spec.rb @@ -31,19 +31,21 @@ end end - describe '#valid?' do + describe '#submit' do let(:token) { nil } - subject(:valid) { validator.valid?(token) } + subject(:response) { validator.submit(token) } context 'with exemption' do before do allow(validator).to receive(:exempt?).and_return(true) end - it { expect(valid).to eq(true) } + it 'is successful' do + expect(response.to_h).to eq(success: true) + end it 'does not log analytics' do - valid + response expect(analytics).not_to have_logged_event('reCAPTCHA verify result received') end @@ -52,10 +54,15 @@ context 'with missing token' do let(:token) { nil } - it { expect(valid).to eq(false) } + it 'is unsuccessful with error for blank token' do + expect(response.to_h).to eq( + success: false, + error_details: { recaptcha_token: { blank: true } }, + ) + end it 'does not log analytics' do - valid + response expect(analytics).not_to have_logged_event('reCAPTCHA verify result received') end @@ -64,10 +71,15 @@ context 'with blank token' do let(:token) { '' } - it { expect(valid).to eq(false) } + it 'is unsuccessful with error for blank token' do + expect(response.to_h).to eq( + success: false, + error_details: { recaptcha_token: { blank: true } }, + ) + end it 'does not log analytics' do - valid + response expect(analytics).not_to have_logged_event('reCAPTCHA verify result received') end @@ -83,10 +95,15 @@ ) end - it { expect(valid).to eq(false) } + it 'is unsuccessful with error for invalid token' do + expect(response.to_h).to eq( + success: false, + error_details: { recaptcha_token: { invalid: true } }, + ) + end it 'logs analytics of the body' do - valid + response expect(analytics).to have_logged_event( 'reCAPTCHA verify result received', @@ -111,10 +128,12 @@ ) end - it { expect(valid).to eq(true) } + it 'is successful' do + expect(response.to_h).to eq(success: true) + end it 'logs analytics of the body' do - valid + response expect(analytics).to have_logged_event( 'reCAPTCHA verify result received', @@ -139,10 +158,12 @@ ) end - it { expect(valid).to eq(true) } + it 'is successful' do + expect(response.to_h).to eq(success: true) + end it 'logs analytics of the body' do - valid + response expect(analytics).to have_logged_event( 'reCAPTCHA verify result received', @@ -168,10 +189,12 @@ stub_request(:post, RecaptchaValidator::VERIFICATION_ENDPOINT).to_timeout end - it { expect(valid).to eq(true) } + it 'is successful' do + expect(response.to_h).to eq(success: true) + end it 'logs analytics of the body' do - valid + response expect(analytics).to have_logged_event( 'reCAPTCHA verify result received', @@ -191,10 +214,15 @@ stub_recaptcha_response(body: { success: true, score: }, token:) end - it { expect(valid).to eq(false) } + it 'is unsuccessful with error for invalid token' do + expect(response.to_h).to eq( + success: false, + error_details: { recaptcha_token: { invalid: true } }, + ) + end it 'logs analytics of the body' do - valid + response expect(analytics).to have_logged_event( 'reCAPTCHA verify result received', @@ -215,14 +243,18 @@ let(:token) { 'token' } let(:score) { score_threshold + 0.1 } - before do - stub_recaptcha_response(body: { success: true, score: }, token:) + around do |example| + stubbed_request = stub_recaptcha_response(body: { success: true, score: }, token:) + example.run + expect(stubbed_request).to have_been_made.once end - it { expect(valid).to eq(true) } + it 'is successful' do + expect(response.to_h).to eq(success: true) + end it 'logs analytics of the body' do - valid + response expect(analytics).to have_logged_event( 'reCAPTCHA verify result received', @@ -242,7 +274,7 @@ let(:extra_analytics_properties) { { extra: true } } it 'logs analytics of the body' do - valid + response expect(analytics).to have_logged_event( 'reCAPTCHA verify result received', @@ -264,7 +296,7 @@ let(:analytics) { nil } it 'validates gracefully without analytics logging' do - valid + response end end end