Skip to content
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

LG-12712 Use phone fingerprint as rate limit key for phone confirmation #10459

Closed
wants to merge 27 commits into from
Closed
Show file tree
Hide file tree
Changes from 26 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
6c9f694
changelog: Internal, Fraud Prevention, Use phone fingerprint as rate …
kevinsmaster5 Apr 15, 2024
16ebd44
add submission limit check to phone setup. config values
kevinsmaster5 Apr 18, 2024
97b0596
move check invocation to before_action
kevinsmaster5 Apr 18, 2024
d1a73e9
fingerprint submissions limit to 20
kevinsmaster5 Apr 18, 2024
7802e47
check for nil phone value before running limit check
kevinsmaster5 Apr 18, 2024
4f3a121
explicitly pass parsed phone as string
kevinsmaster5 Apr 19, 2024
bda810a
added check for blank entry and fix for nil string errors that spec s…
kevinsmaster5 Apr 19, 2024
546ca7c
add test to lock out user exceeding phone submissions
kevinsmaster5 Apr 22, 2024
c013074
expand on phone submission test with additional user and locked attri…
kevinsmaster5 Apr 22, 2024
500e098
refactor setup controller to remove logout. update lockout time in ap…
kevinsmaster5 Apr 23, 2024
3730833
redirect failed phone setup to account
kevinsmaster5 Apr 24, 2024
57dbca5
leverages OtpRateLimiter and modifies it to accept another limiter
kevinsmaster5 Apr 25, 2024
a37aae9
add limit_type to otp rate limiter spec
kevinsmaster5 Apr 26, 2024
f2ef5b9
add limit_type to sign in and send phone confirmation spec
kevinsmaster5 Apr 26, 2024
839626d
retreat changes after commit 0550b52
kevinsmaster5 Apr 26, 2024
deb8d9a
rename fingerprint rate limit type
kevinsmaster5 Apr 26, 2024
f8e8474
corrects test for user2 2F lockout
kevinsmaster5 Apr 26, 2024
48e60c3
standardize rate limiter name, remove no-longer needed returns, redir…
kevinsmaster5 Apr 29, 2024
7307e18
move validation to NewPhoneForm
kevinsmaster5 Apr 29, 2024
c16519f
rename error to match actual form symbol
kevinsmaster5 Apr 29, 2024
934a6cd
rebase to incorporate phone_setup changes. add to error handling from…
kevinsmaster5 Apr 30, 2024
53b187c
revise spec to account for phone setup controller ability to show errors
kevinsmaster5 Apr 30, 2024
d1c9be8
revises error message handling and respective test
kevinsmaster5 May 1, 2024
5233d20
revert change to phone setup controller spec
kevinsmaster5 May 1, 2024
dcfb36a
fix error messaging to flash. add testing for error display
kevinsmaster5 May 2, 2024
650baf8
remove unneeded symbol from first_error_message
kevinsmaster5 May 2, 2024
b67eadd
cleanup and optimize phone setup spec, downgrade instance variable in…
kevinsmaster5 May 2, 2024
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
5 changes: 4 additions & 1 deletion app/controllers/users/phone_setup_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
25 changes: 25 additions & 0 deletions app/forms/new_phone_form.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ class NewPhoneForm
include ActiveModel::Model
include FormPhoneValidator
include OtpDeliveryPreferenceValidator
include ActionView::Helpers::DateHelper

BLOCKED_PHONE_TYPES = [
:premium_rate,
Expand All @@ -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,
Expand Down Expand Up @@ -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(
Copy link
Member

Choose a reason for hiding this comment

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

If we're not using this anywhere outside the scope of this method, I don't think we need to assign it as an instance variable.

Suggested change
@submission_rate_limiter ||= RateLimiter.new(
submission_rate_limiter ||= RateLimiter.new(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense. 👍

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
12 changes: 9 additions & 3 deletions app/services/form_response.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
5 changes: 5 additions & 0 deletions app/services/rate_limiter.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
2 changes: 2 additions & 0 deletions config/application.yml.default
Original file line number Diff line number Diff line change
Expand Up @@ -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: '[]'
Expand Down
2 changes: 2 additions & 0 deletions lib/identity_config.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
35 changes: 35 additions & 0 deletions spec/controllers/users/phone_setup_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -234,6 +234,41 @@
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(:otp_delivery_blocklist_maxretry).and_return(999)
Copy link
Member

Choose a reason for hiding this comment

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

Is this necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not at all, apparently. It would be useful for hammering at the phone submissions limiter and not wanting to hit that other limit it seems. Isn't a problem here. Removed.

allow(IdentityConfig.store).to receive(
:phone_submissions_per_fingerprint_max_attempts_window_in_minutes,
).and_return(10)
Copy link
Member

Choose a reason for hiding this comment

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

We could make the test run a little faster if we reduced this a bit so it's not looping as many times in the loop that follows.

Suggested change
).and_return(10)
).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: '9 minutes'),
)

travel_to(5.minutes.from_now) do
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: '5 minutes'),
)
end

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
Expand Down
33 changes: 33 additions & 0 deletions spec/forms/new_phone_form_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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' }
Expand Down Expand Up @@ -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')
Expand Down
24 changes: 19 additions & 5 deletions spec/services/form_response_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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 }
Expand All @@ -125,19 +123,35 @@
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')
end
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
Expand Down