diff --git a/app/controllers/users/phone_setup_controller.rb b/app/controllers/users/phone_setup_controller.rb index 41f9ff5508f..46860bbbb26 100644 --- a/app/controllers/users/phone_setup_controller.rb +++ b/app/controllers/users/phone_setup_controller.rb @@ -36,7 +36,10 @@ def create if result.success? handle_create_success(@new_phone_form.phone) else - flash.now[:error] = result.first_error_message(:recaptcha_token) + flash.now[:error] = result.first_error_message( + :recaptcha_token, + :phone_fingerprint, + ) render :index end end diff --git a/app/forms/new_phone_form.rb b/app/forms/new_phone_form.rb index a12f2c73afa..a69e7817364 100644 --- a/app/forms/new_phone_form.rb +++ b/app/forms/new_phone_form.rb @@ -4,6 +4,7 @@ class NewPhoneForm include ActiveModel::Model include FormPhoneValidator include OtpDeliveryPreferenceValidator + include ActionView::Helpers::DateHelper BLOCKED_PHONE_TYPES = [ :premium_rate, @@ -17,6 +18,7 @@ class NewPhoneForm validate :validate_not_premium_rate validate :validate_recaptcha_token validate :validate_allowed_carrier + validate :validate_phone_submission_limit attr_reader :phone, :international_code, @@ -177,4 +179,27 @@ def ingest_submitted_params(params) def confirmed_phone? false end + + def validate_phone_submission_limit + fingerprint = Pii::Fingerprinter.fingerprint(Phonelib.parse(phone).e164.to_s) + submission_rate_limiter ||= RateLimiter.new( + target: fingerprint, + rate_limit_type: :phone_fingerprint_confirmation, + ) + submission_rate_limiter.increment! + if submission_rate_limiter.maxed? + errors.add( + :phone_fingerprint, + I18n.t( + 'errors.messages.phone_confirmation_limited', + timeout: distance_of_time_in_words( + Time.zone.now, + [submission_rate_limiter.expires_at, Time.zone.now].compact.max, + except: :seconds, + ), + ), + type: :locked_phone_fingerprint, + ) + end + end end diff --git a/app/services/form_response.rb b/app/services/form_response.rb index e62f67c9613..36d2c7d8daf 100644 --- a/app/services/form_response.rb +++ b/app/services/form_response.rb @@ -39,10 +39,16 @@ def merge(other) end end - def first_error_message(key = nil) + def first_error_message(*keys) return if errors.blank? - key ||= errors.keys.first - errors[key].first + if keys.blank? + errors.values.first.first + else + keys.each do |key| + return errors[key].first if errors.key?(key) + end + nil + end end def ==(other) diff --git a/app/services/rate_limiter.rb b/app/services/rate_limiter.rb index f25751f9246..eaba8fd0dbc 100644 --- a/app/services/rate_limiter.rb +++ b/app/services/rate_limiter.rb @@ -214,6 +214,11 @@ def self.load_rate_limit_config max_attempts: IdentityConfig.store.phone_confirmation_max_attempts, attempt_window: IdentityConfig.store.phone_confirmation_max_attempt_window_in_minutes, }, + phone_fingerprint_confirmation: { + max_attempts: IdentityConfig.store.phone_submissions_per_fingerprint_limit, + attempt_window: IdentityConfig.store. + phone_submissions_per_fingerprint_max_attempts_window_in_minutes, + }, phone_otp: { max_attempts: IdentityConfig.store.otp_delivery_blocklist_maxretry + 1, attempt_window: IdentityConfig.store.otp_delivery_blocklist_findtime, diff --git a/config/application.yml.default b/config/application.yml.default index 417085b30f6..d33d41cc030 100644 --- a/config/application.yml.default +++ b/config/application.yml.default @@ -245,6 +245,8 @@ phone_recaptcha_country_score_overrides: '{"AS":0.0,"GU":0.0,"MP":0.0,"PR":0.0," phone_setups_per_ip_limit: 25 phone_setups_per_ip_period: 300 phone_setups_per_ip_track_only_mode: false +phone_submissions_per_fingerprint_limit: 20 +phone_submissions_per_fingerprint_max_attempts_window_in_minutes: 10 pii_lock_timeout_in_minutes: 30 pinpoint_sms_sender_id: 'aaa' pinpoint_sms_configs: '[]' diff --git a/lib/identity_config.rb b/lib/identity_config.rb index 5dee8f89ef1..672db43bc92 100644 --- a/lib/identity_config.rb +++ b/lib/identity_config.rb @@ -299,6 +299,8 @@ def self.store config.add(:phone_setups_per_ip_limit, type: :integer) config.add(:phone_setups_per_ip_period, type: :integer) config.add(:phone_setups_per_ip_track_only_mode, type: :boolean) + config.add(:phone_submissions_per_fingerprint_limit, type: :integer) + config.add(:phone_submissions_per_fingerprint_max_attempts_window_in_minutes, type: :integer) config.add(:pii_lock_timeout_in_minutes, type: :integer) config.add(:pinpoint_sms_configs, type: :json) config.add(:pinpoint_sms_sender_id, type: :string, allow_nil: true) diff --git a/spec/controllers/users/phone_setup_controller_spec.rb b/spec/controllers/users/phone_setup_controller_spec.rb index 20dc76dd601..cec2c45c52e 100644 --- a/spec/controllers/users/phone_setup_controller_spec.rb +++ b/spec/controllers/users/phone_setup_controller_spec.rb @@ -234,6 +234,38 @@ expect(subject.user_session[:context]).to eq 'confirmation' end end + + context 'with rate phone fingerprint rate limit' do + before do + @user = create(:user) + @user2 = create(:user) + @unconfirmed_phone = '+1 (202) 555-1213' + @unconfirmed_phone2 = '+1 (202) 555-1215' + end + it 'displays an error banner' do + sign_in_before_2fa(@user) + allow(IdentityConfig.store).to receive( + :phone_submissions_per_fingerprint_max_attempts_window_in_minutes, + ).and_return(2) + IdentityConfig.store.phone_submissions_per_fingerprint_limit.times do + post(:create, params: { new_phone_form: { phone: @unconfirmed_phone } }) + end + + expect(flash[:error]).to eq( + t('errors.messages.phone_confirmation_limited', timeout: '1 minute'), + ) + + sign_in_before_2fa(@user2) + post(:create, params: { new_phone_form: { phone: @unconfirmed_phone } }) + expect(flash[:error]).to eq( + t('errors.messages.phone_confirmation_limited', timeout: '1 minute'), + ) + + sign_in_before_2fa(@user) + post(:create, params: { new_phone_form: { phone: @unconfirmed_phone2 } }) + expect(flash[:error]).to eq(nil) + end + end end describe 'before_actions' do diff --git a/spec/forms/new_phone_form_spec.rb b/spec/forms/new_phone_form_spec.rb index b15a95b3cee..fbdfef0574d 100644 --- a/spec/forms/new_phone_form_spec.rb +++ b/spec/forms/new_phone_form_spec.rb @@ -2,6 +2,7 @@ RSpec.describe NewPhoneForm do include Shoulda::Matchers::ActiveModel + include ActionView::Helpers::DateHelper let(:user) { build(:user, :fully_registered) } let(:phone) { '703-555-5000' } @@ -164,6 +165,38 @@ expect(result.success?).to eq(true) end + context 'validating phone fingerprint submission rate limit' do + let(:params) do + { + phone: '703-555-1212', + international_code: 'US', + } + end + it 'enforces phone fingerprint rate limit' do + allow(IdentityConfig.store).to receive(:otp_delivery_blocklist_maxretry).and_return(999) + + freeze_time do + IdentityConfig.store.phone_submissions_per_fingerprint_limit.times do + subject.submit(params) + end + + result = subject.submit(params) + expect(result).to be_kind_of(FormResponse) + expect(result.success?).to eq(false) + expect(result.errors[:phone_fingerprint]).to eq( + [ + I18n.t( + 'errors.messages.phone_confirmation_limited', + timeout: distance_of_time_in_words( + RateLimiter.attempt_window_in_minutes(:phone_fingerprint_confirmation).minutes, + ), + ), + ], + ) + end + end + end + context 'when the user has already added the number' do it 'is invalid' do phone = PhoneFormatter.format('+1 (954) 555-0100', country_code: 'US') diff --git a/spec/services/form_response_spec.rb b/spec/services/form_response_spec.rb index 5efcefb17ac..8f7ef4b6d1a 100644 --- a/spec/services/form_response_spec.rb +++ b/spec/services/form_response_spec.rb @@ -112,10 +112,8 @@ end describe '#first_error_message' do - let(:key) { nil } - subject(:first_error_message) { form_response.first_error_message(*[key].compact) } - context 'with no errors' do + subject(:first_error_message) { form_response.first_error_message } let(:errors) { {} } it { expect(first_error_message).to be_nil } @@ -125,7 +123,7 @@ let(:errors) { { email: ['invalid', 'too_short'], language: ['blank'] } } context 'without specified key' do - let(:key) { nil } + subject(:first_error_message) { form_response.first_error_message } it 'returns the first error of the first field' do expect(first_error_message).to eq('invalid') @@ -133,11 +131,27 @@ end context 'with specified key' do - let(:key) { :language } + subject(:first_error_message) { form_response.first_error_message(:language) } it 'returns the first error of the specified field' do expect(first_error_message).to eq('blank') end + + context 'with key that does not exist in set of errors' do + subject(:first_error_message) { form_response.first_error_message(:foo) } + + it 'returns nil' do + expect(first_error_message).to be_nil + end + end + end + + context 'with multiple specified keys' do + subject(:first_error_message) { form_response.first_error_message(:foo, :language, :email) } + + it 'returns the first error of the key which exists as an error' do + expect(first_error_message).to eq('blank') + end end end end