diff --git a/app/controllers/concerns/two_factor_authenticatable.rb b/app/controllers/concerns/two_factor_authenticatable.rb index 837c867cf68..e728c786c23 100644 --- a/app/controllers/concerns/two_factor_authenticatable.rb +++ b/app/controllers/concerns/two_factor_authenticatable.rb @@ -33,6 +33,16 @@ def handle_second_factor_locked_user(type) ) end + def handle_too_many_otp_sends + analytics.track_event(Analytics::MULTI_FACTOR_AUTH_MAX_SENDS) + decorator = current_user.decorate + sign_out + render( + 'two_factor_authentication/shared/max_otp_requests_reached', + locals: { decorator: decorator } + ) + end + def require_current_password redirect_to user_password_confirm_path end @@ -48,11 +58,14 @@ def check_already_authenticated end def reset_attempt_count_if_user_no_longer_locked_out - return unless decorated_user.no_longer_blocked_from_entering_2fa_code? + return unless decorated_user.no_longer_locked_out? UpdateUser.new( user: current_user, - attributes: { second_factor_attempts_count: 0, second_factor_locked_at: nil } + attributes: { + second_factor_attempts_count: 0, + second_factor_locked_at: nil, + } ).call end @@ -79,7 +92,7 @@ def handle_invalid_otp(type: 'otp') flash.now[:error] = t("devise.two_factor_authentication.invalid_#{type}") - if decorated_user.blocked_from_entering_2fa_code? + if decorated_user.locked_out? handle_second_factor_locked_user(type) else render_show_after_invalid diff --git a/app/controllers/users/sessions_controller.rb b/app/controllers/users/sessions_controller.rb index 543ce17ae9e..ae84123fdb3 100644 --- a/app/controllers/users/sessions_controller.rb +++ b/app/controllers/users/sessions_controller.rb @@ -102,7 +102,7 @@ def user_signed_in_and_not_locked_out?(user) end def user_locked_out?(user) - UserDecorator.new(user).blocked_from_entering_2fa_code? + UserDecorator.new(user).locked_out? end def store_sp_metadata_in_session diff --git a/app/controllers/users/two_factor_authentication_controller.rb b/app/controllers/users/two_factor_authentication_controller.rb index 76e13eef3f4..90c9e42a9ea 100644 --- a/app/controllers/users/two_factor_authentication_controller.rb +++ b/app/controllers/users/two_factor_authentication_controller.rb @@ -35,12 +35,21 @@ def reauthn_param end def handle_valid_otp_delivery_preference(method) + otp_rate_limiter.reset_count_and_otp_last_sent_at if decorated_user.no_longer_locked_out? + + if otp_rate_limiter.exceeded_otp_send_limit? + otp_rate_limiter.lock_out_user + + return handle_too_many_otp_sends + end + send_user_otp(method) session[:code_sent] = 'true' redirect_to login_two_factor_path(otp_delivery_preference: method, reauthn: reauthn?) end def send_user_otp(method) + otp_rate_limiter.increment current_user.create_direct_otp job = "#{method.capitalize}OtpSenderJob".constantize @@ -70,5 +79,9 @@ def phone_to_deliver_to user_session[:unconfirmed_phone] end + + def otp_rate_limiter + @_otp_rate_limited ||= OtpRateLimiter.new(phone: phone_to_deliver_to, user: current_user) + end end end diff --git a/app/decorators/user_decorator.rb b/app/decorators/user_decorator.rb index 4f3a7bad068..e1731e1d855 100644 --- a/app/decorators/user_decorator.rb +++ b/app/decorators/user_decorator.rb @@ -19,7 +19,7 @@ def lockout_time_remaining_in_words end def lockout_time_remaining - (Devise.direct_otp_valid_for - (Time.zone.now - user.second_factor_locked_at)).to_i + (lockout_period - (Time.zone.now - user.second_factor_locked_at)).to_i end def confirmation_period_expired_error @@ -98,12 +98,12 @@ def qrcode(otp_secret_key) qrcode.as_png(size: 280).to_data_url end - def blocked_from_entering_2fa_code? - user.second_factor_locked_at.present? && !blocked_from_2fa_period_expired? + def locked_out? + user.second_factor_locked_at.present? && !lockout_period_expired? end - def no_longer_blocked_from_entering_2fa_code? - user.second_factor_locked_at.present? && blocked_from_2fa_period_expired? + def no_longer_locked_out? + user.second_factor_locked_at.present? && lockout_period_expired? end def should_acknowledge_personal_key?(session) @@ -136,7 +136,11 @@ def masked_number(number) "***-***-#{number[-4..-1]}" end - def blocked_from_2fa_period_expired? - (Time.zone.now - user.second_factor_locked_at) > Devise.direct_otp_valid_for + def lockout_period + Figaro.env.lockout_period_in_minutes.to_i.minutes + end + + def lockout_period_expired? + (Time.zone.now - user.second_factor_locked_at) > lockout_period end end diff --git a/app/models/otp_requests_tracker.rb b/app/models/otp_requests_tracker.rb new file mode 100644 index 00000000000..206783f1bc6 --- /dev/null +++ b/app/models/otp_requests_tracker.rb @@ -0,0 +1,22 @@ +class OtpRequestsTracker < ActiveRecord::Base + include EncryptableAttribute + + encrypted_attribute_without_setter(name: :phone) + + def self.find_or_create_with_phone(phone) + tries ||= 1 + phone ||= phone.strip + phone_fingerprint ||= Pii::Fingerprinter.fingerprint(phone) + + where(phone_fingerprint: phone_fingerprint). + first_or_create(phone: phone, otp_send_count: 0, otp_last_sent_at: Time.zone.now) + rescue ActiveRecord::RecordNotUnique + retry unless (tries -= 1).zero? + raise + end + + def phone=(phone) + set_encrypted_attribute(name: :phone, value: phone) + self.phone_fingerprint = phone.present? ? encrypted_attributes[:phone].fingerprint : '' + end +end diff --git a/app/services/analytics.rb b/app/services/analytics.rb index 577e68c2b86..96811367afd 100644 --- a/app/services/analytics.rb +++ b/app/services/analytics.rb @@ -55,6 +55,7 @@ def uuid MULTI_FACTOR_AUTH_ENTER_OTP_VISIT = 'Multi-Factor Authentication: enter OTP visited'.freeze MULTI_FACTOR_AUTH_MAX_ATTEMPTS = 'Multi-Factor Authentication: max attempts reached'.freeze MULTI_FACTOR_AUTH_PHONE_SETUP = 'Multi-Factor Authentication: phone setup'.freeze + MULTI_FACTOR_AUTH_MAX_SENDS = 'Multi-Factor Authentication: max otp sends reached'.freeze OPENID_CONNECT_BEARER_TOKEN = 'OpenID Connect: bearer token authentication'.freeze OPENID_CONNECT_LOGOUT = 'OpenID Connect: logout'.freeze OPENID_CONNECT_REQUEST_AUTHORIZATION = 'OpenID Connect: authorization request'.freeze diff --git a/app/services/otp_rate_limiter.rb b/app/services/otp_rate_limiter.rb new file mode 100644 index 00000000000..1fb7bc29dcf --- /dev/null +++ b/app/services/otp_rate_limiter.rb @@ -0,0 +1,61 @@ +class OtpRateLimiter + def initialize(phone:, user:) + @phone = phone + @user = user + end + + def exceeded_otp_send_limit? + return false if entry_for_current_phone.blank? + + if rate_limit_period_expired? + reset_count_and_otp_last_sent_at + return false + end + + max_requests_reached? + end + + def max_requests_reached? + entry_for_current_phone.otp_send_count >= otp_maxretry_times + end + + def rate_limit_period_expired? + otp_last_sent_at.present? && (otp_last_sent_at + otp_findtime) < Time.zone.now + end + + def reset_count_and_otp_last_sent_at + entry_for_current_phone.update(otp_last_sent_at: Time.zone.now, otp_send_count: 0) + end + + def lock_out_user + UpdateUser.new(user: user, attributes: { second_factor_locked_at: Time.zone.now }).call + end + + def increment + now = Time.zone.now + + entry_for_current_phone.otp_send_count += 1 + entry_for_current_phone.otp_last_sent_at = now + entry_for_current_phone.save! + end + + private + + attr_reader :phone, :user + + def entry_for_current_phone + @entry ||= OtpRequestsTracker.find_or_create_with_phone(phone) + end + + def otp_last_sent_at + entry_for_current_phone.otp_last_sent_at + end + + def otp_findtime + Figaro.env.otp_delivery_blocklist_findtime.to_i.minutes + end + + def otp_maxretry_times + Figaro.env.otp_delivery_blocklist_maxretry.to_i + end +end diff --git a/app/views/two_factor_authentication/shared/max_otp_requests_reached.html.erb b/app/views/two_factor_authentication/shared/max_otp_requests_reached.html.erb new file mode 100644 index 00000000000..c5db7d9aabe --- /dev/null +++ b/app/views/two_factor_authentication/shared/max_otp_requests_reached.html.erb @@ -0,0 +1,19 @@ +<% lockout_time_in_words = decorator.lockout_time_remaining_in_words %> +<% lockout_time_remaining = decorator.lockout_time_remaining %> +<% title t('titles.account_locked') %> + +
+<%= t("devise.two_factor_authentication.max_otp_requests_reached") %> +
++ <%= t('devise.two_factor_authentication.please_try_again_html', + time_remaining: content_tag(:span, lockout_time_in_words, id: 'countdown')) %> +
+ +<%= nonced_javascript_tag do %> + var test = <%= lockout_time_remaining %> * 1000; + window.LoginGov.countdownTimer(document.getElementById('countdown'), test); +<% end %> diff --git a/config/application.yml.example b/config/application.yml.example index e033efa27e3..3d9d481d90c 100644 --- a/config/application.yml.example +++ b/config/application.yml.example @@ -73,14 +73,14 @@ development: hmac_fingerprinter_key: 'a2c813d4dca919340866ba58063e4072adc459b767a74cf2666d5c1eef3861db26708e7437abde1755eb24f4034386b0fea1850a1cb7e56bff8fae3cc6ade96c' hmac_fingerprinter_key_queue: '["old-key-one", "old-key-two"]' idp_sso_target_url: 'https://upaya-mockidp.18f.gov/saml/auth' + lockout_period_in_minutes: '10' logins_per_ip_limit: '20' logins_per_ip_period: '8' newrelic_license_key: 'xxx' newrelic_browser_key: '' newrelic_browser_app_id: '' - otp_delivery_blocklist_bantime: '30' - otp_delivery_blocklist_findtime: '15' - otp_delivery_blocklist_maxretry: '3' + otp_delivery_blocklist_findtime: '5' + otp_delivery_blocklist_maxretry: '10' otp_valid_for: '5' password_pepper: 'f22d4b2cafac9066fe2f4416f5b7a32c6942d82f7e00740c7594a095fa8de8db17c05314be7b18a5d6dd5683e73eadf6cc95aa633e5ad9a701edb95192a6a105' password_strength_enabled: 'true' @@ -129,14 +129,14 @@ production: hmac_fingerprinter_key: 'generate via `rake secret`' hmac_fingerprinter_key_queue: '["old-key-one", "old-key-two"]' idp_sso_target_url: 'example.com/saml/auth' + lockout_period_in_minutes: '10' logins_per_ip_limit: '20' logins_per_ip_period: '8' newrelic_license_key: 'xxx' newrelic_browser_key: '' newrelic_browser_app_id: '' - otp_delivery_blocklist_bantime: '30' - otp_delivery_blocklist_findtime: '15' - otp_delivery_blocklist_maxretry: '3' + otp_delivery_blocklist_findtime: '5' + otp_delivery_blocklist_maxretry: '10' otp_valid_for: '5' participate_in_dap: 'false' password_pepper: 'generate via `rake secret`' @@ -184,11 +184,11 @@ test: hmac_fingerprinter_key: 'a2c813d4dca919340866ba58063e4072adc459b767a74cf2666d5c1eef3861db26708e7437abde1755eb24f4034386b0fea1850a1cb7e56bff8fae3cc6ade96c' hmac_fingerprinter_key_queue: '["old-key-one", "old-key-two"]' idp_sso_target_url: 'http://identityprovider.example.com/saml/auth' + lockout_period_in_minutes: '5' logins_per_ip_limit: '2' logins_per_ip_period: '60' max_mail_events: '2' newrelic_license_key: 'xxx' - otp_delivery_blocklist_bantime: '1' otp_delivery_blocklist_findtime: '1' otp_delivery_blocklist_maxretry: '2' otp_valid_for: '5' diff --git a/config/initializers/figaro.rb b/config/initializers/figaro.rb index 3404e34e1b9..414d367bcec 100644 --- a/config/initializers/figaro.rb +++ b/config/initializers/figaro.rb @@ -10,12 +10,12 @@ 'equifax_ssh_passphrase', 'hmac_fingerprinter_key', 'idp_sso_target_url', + 'lockout_period_in_minutes', 'logins_per_ip_limit', 'logins_per_ip_period', 'max_mail_events', 'max_mail_events_window_in_days', 'min_password_score', - 'otp_delivery_blocklist_bantime', 'otp_delivery_blocklist_findtime', 'otp_delivery_blocklist_maxretry', 'otp_valid_for', diff --git a/config/initializers/rack_attack.rb b/config/initializers/rack_attack.rb index ebb7b9bb3c2..f75a491a419 100644 --- a/config/initializers/rack_attack.rb +++ b/config/initializers/rack_attack.rb @@ -83,24 +83,6 @@ def remote_ip end end - # After maxretry OTP requests in findtime minutes, - # block all requests from that user for bantime minutes. - blocklist('OTP delivery') do |req| - # `filter` returns truthy value if request fails, or if it's to a - # previously banned phone_number so the request is blocked - phone_number = req.env['warden'].user&.phone - - Allow2Ban.filter( - "otp-#{phone_number}", - maxretry: Figaro.env.otp_delivery_blocklist_maxretry.to_i, - findtime: Figaro.env.otp_delivery_blocklist_findtime.to_i.minutes, - bantime: Figaro.env.otp_delivery_blocklist_bantime.to_i.minutes - ) do - # The count for the phone_number is incremented if the return value is truthy - req.get? && req.path == '/otp/send' - end - end - ### Custom Throttle Response ### # By default, Rack::Attack returns an HTTP 429 for throttled responses, diff --git a/config/locales/devise/en.yml b/config/locales/devise/en.yml index 60d8a4c63e6..94019d25c3f 100644 --- a/config/locales/devise/en.yml +++ b/config/locales/devise/en.yml @@ -120,6 +120,9 @@ en: max_otp_login_attempts_reached: > Your account is temporarily locked because you have entered the one-time security code incorrectly too many times. + max_otp_requests_reached: > + Your account is temporarily locked because you have requested a + security code too many times. max_personal_key_login_attempts_reached: > Your account is temporarily locked because you have entered the personal key incorrectly too many times. diff --git a/config/locales/devise/es.yml b/config/locales/devise/es.yml index 6d96b683960..acbd5a27c26 100644 --- a/config/locales/devise/es.yml +++ b/config/locales/devise/es.yml @@ -108,6 +108,7 @@ es: max_otp_login_attempts_reached: Su cuenta ha sido bloqueada temporalmente porque ha ingresado el código de acceso único de forma incorrecta demasiadas veces. + max_otp_requests_reached: NOT TRANSLATED YET max_personal_key_login_attempts_reached: Su cuenta ha sido bloqueada temporalmente porque ha ingresado el código de acceso único de forma incorrecta demasiadas veces. diff --git a/db/migrate/20170621202836_create_otp_requests_tracker.rb b/db/migrate/20170621202836_create_otp_requests_tracker.rb new file mode 100644 index 00000000000..22c4a3b0d41 --- /dev/null +++ b/db/migrate/20170621202836_create_otp_requests_tracker.rb @@ -0,0 +1,16 @@ +class CreateOtpRequestsTracker < ActiveRecord::Migration + def change + create_table :otp_requests_trackers do |t| + t.text :encrypted_phone + t.timestamp :otp_last_sent_at + t.integer :otp_send_count, default: 0 + t.string :attribute_cost + t.string :phone_fingerprint, default: '', null: false + + t.timestamps + + t.index :phone_fingerprint, unique: true + t.index :updated_at + end + end +end diff --git a/db/schema.rb b/db/schema.rb index 4895d88e652..71f4961a843 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20170531204549) do +ActiveRecord::Schema.define(version: 20170621202836) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -68,6 +68,19 @@ add_index "identities", ["user_id"], name: "index_identities_on_user_id", using: :btree add_index "identities", ["uuid"], name: "index_identities_on_uuid", unique: true, using: :btree + create_table "otp_requests_trackers", force: :cascade do |t| + t.text "encrypted_phone" + t.datetime "otp_last_sent_at" + t.integer "otp_send_count", default: 0 + t.string "attribute_cost" + t.string "phone_fingerprint", default: "", null: false + t.datetime "created_at" + t.datetime "updated_at" + end + + add_index "otp_requests_trackers", ["phone_fingerprint"], name: "index_otp_requests_trackers_on_phone_fingerprint", unique: true, using: :btree + add_index "otp_requests_trackers", ["updated_at"], name: "index_otp_requests_trackers_on_updated_at", using: :btree + create_table "profiles", force: :cascade do |t| t.integer "user_id", null: false t.boolean "active", default: false, null: false diff --git a/spec/controllers/two_factor_authentication/otp_verification_controller_spec.rb b/spec/controllers/two_factor_authentication/otp_verification_controller_spec.rb index 7a1a427e8b1..f7c3b1afcf2 100644 --- a/spec/controllers/two_factor_authentication/otp_verification_controller_spec.rb +++ b/spec/controllers/two_factor_authentication/otp_verification_controller_spec.rb @@ -158,8 +158,9 @@ context 'when the user lockout period expires' do before do sign_in_before_2fa + lockout_period = Figaro.env.lockout_period_in_minutes.to_i.minutes subject.current_user.update( - second_factor_locked_at: Time.zone.now - Devise.direct_otp_valid_for - 1.second, + second_factor_locked_at: Time.zone.now - lockout_period - 1.second, second_factor_attempts_count: 3 ) end diff --git a/spec/controllers/two_factor_authentication/totp_verification_controller_spec.rb b/spec/controllers/two_factor_authentication/totp_verification_controller_spec.rb index 66152e2f37e..d55bb7d82bf 100644 --- a/spec/controllers/two_factor_authentication/totp_verification_controller_spec.rb +++ b/spec/controllers/two_factor_authentication/totp_verification_controller_spec.rb @@ -87,10 +87,11 @@ context 'when the user lockout period expires' do before do + lockout_period = Figaro.env.lockout_period_in_minutes.to_i.minutes user = create( :user, :signed_up, - second_factor_locked_at: Time.zone.now - Devise.direct_otp_valid_for - 1.second, + second_factor_locked_at: Time.zone.now - lockout_period - 1.second, second_factor_attempts_count: 3 ) sign_in_before_2fa(user) diff --git a/spec/controllers/users/two_factor_authentication_controller_spec.rb b/spec/controllers/users/two_factor_authentication_controller_spec.rb index 02214bcae8c..26b9618fa99 100644 --- a/spec/controllers/users/two_factor_authentication_controller_spec.rb +++ b/spec/controllers/users/two_factor_authentication_controller_spec.rb @@ -111,7 +111,8 @@ def index describe '#send_code' do context 'when selecting SMS OTP delivery' do before do - sign_in_before_2fa + @user = create(:user, :with_phone) + sign_in_before_2fa(@user) @old_otp = subject.current_user.direct_otp allow(SmsOtpSenderJob).to receive(:perform_later) end @@ -147,6 +148,27 @@ def index get :send_code, otp_delivery_selection_form: { otp_delivery_preference: 'sms' } end + + it 'calls OtpRateLimiter#exceeded_otp_send_limit? and #increment' do + otp_rate_limiter = instance_double(OtpRateLimiter) + allow(OtpRateLimiter).to receive(:new).with(phone: @user.phone, user: @user). + and_return(otp_rate_limiter) + + expect(otp_rate_limiter).to receive(:exceeded_otp_send_limit?) + expect(otp_rate_limiter).to receive(:increment) + + get :send_code, otp_delivery_selection_form: { otp_delivery_preference: 'sms' } + end + + it 'marks the user as locked out after too many attempts' do + expect(@user.second_factor_locked_at).to be_nil + + (Figaro.env.otp_delivery_blocklist_maxretry.to_i + 1).times do + get :send_code, otp_delivery_selection_form: { otp_delivery_preference: 'sms' } + end + + expect(@user.reload.second_factor_locked_at.to_f).to be_within(0.1).of(Time.zone.now.to_f) + end end context 'when selecting voice OTP delivery' do diff --git a/spec/decorators/user_decorator_spec.rb b/spec/decorators/user_decorator_spec.rb index 994e203fd49..5cea4336e82 100644 --- a/spec/decorators/user_decorator_spec.rb +++ b/spec/decorators/user_decorator_spec.rb @@ -6,9 +6,9 @@ Timecop.freeze(Time.zone.now) do user = build_stubbed(:user, second_factor_locked_at: Time.zone.now - 180) user_decorator = UserDecorator.new(user) - allow(Devise).to receive(:direct_otp_valid_for).and_return(535) + allow(Figaro.env).to receive(:lockout_period_in_minutes).and_return('8') - expect(user_decorator.lockout_time_remaining).to eq 355 + expect(user_decorator.lockout_time_remaining).to eq 300 end end end @@ -18,10 +18,10 @@ Timecop.freeze(Time.zone.now) do user = build_stubbed(:user, second_factor_locked_at: Time.zone.now - 180) user_decorator = UserDecorator.new(user) - allow(Devise).to receive(:direct_otp_valid_for).and_return(535) + allow(Figaro.env).to receive(:lockout_period_in_minutes).and_return('8') expect(user_decorator.lockout_time_remaining_in_words). - to eq '5 minutes and 55 seconds' + to eq '5 minutes' end end end diff --git a/spec/features/idv/flow_spec.rb b/spec/features/idv/flow_spec.rb index 3530bb75ce9..4ebe49ed1dd 100644 --- a/spec/features/idv/flow_spec.rb +++ b/spec/features/idv/flow_spec.rb @@ -384,6 +384,8 @@ end scenario 'continue phone OTP verification after cancel' do + allow(Figaro.env).to receive(:otp_delivery_blocklist_maxretry).and_return('4') + different_phone = '555-555-9876' user = sign_in_live_with_2fa visit verify_session_path diff --git a/spec/features/two_factor_authentication/change_factor_spec.rb b/spec/features/two_factor_authentication/change_factor_spec.rb index 76dbb1a4c8f..49880e3891c 100644 --- a/spec/features/two_factor_authentication/change_factor_spec.rb +++ b/spec/features/two_factor_authentication/change_factor_spec.rb @@ -15,6 +15,8 @@ end scenario 'editing phone number' do + allow(Figaro.env).to receive(:otp_delivery_blocklist_maxretry).and_return('4') + mailer = instance_double(ActionMailer::MessageDelivery, deliver_later: true) allow(UserMailer).to receive(:phone_changed).with(user).and_return(mailer) diff --git a/spec/features/two_factor_authentication/sign_in_spec.rb b/spec/features/two_factor_authentication/sign_in_spec.rb index 80d33721990..b3e116f5427 100644 --- a/spec/features/two_factor_authentication/sign_in_spec.rb +++ b/spec/features/two_factor_authentication/sign_in_spec.rb @@ -115,6 +115,7 @@ def submit_prefilled_otp_code it 'locks the user out and leaves user on the page during entire lockout period' do allow(Figaro.env).to receive(:session_check_frequency).and_return('0') allow(Figaro.env).to receive(:session_check_delay).and_return('0') + lockout_period = Figaro.env.lockout_period_in_minutes.to_i.minutes five_minute_countdown_regex = /4:5\d/ user = create(:user, :signed_up) @@ -132,7 +133,7 @@ def submit_prefilled_otp_code UpdateUser.new( user: user, attributes: { - second_factor_locked_at: Time.zone.now - (Devise.direct_otp_valid_for + 1.second), + second_factor_locked_at: Time.zone.now - (lockout_period + 1.second), } ).call @@ -143,6 +144,158 @@ def submit_prefilled_otp_code end end + context 'user requests an OTP too many times within `findtime` minutes', js: true do + it 'locks the user out and leaves user on the page during entire lockout period' do + allow(Figaro.env).to receive(:session_check_frequency).and_return('0') + allow(Figaro.env).to receive(:session_check_delay).and_return('0') + lockout_period = Figaro.env.lockout_period_in_minutes.to_i.minutes + five_minute_countdown_regex = /4:5\d/ + + user = create(:user, :signed_up) + sign_in_before_2fa(user) + + Figaro.env.otp_delivery_blocklist_maxretry.to_i.times do + click_link t('links.two_factor_authentication.resend_code.sms') + end + + expect(page).to have_content t('titles.account_locked') + expect(page).to have_content(five_minute_countdown_regex) + expect(page).to have_content t('devise.two_factor_authentication.max_otp_requests_reached') + + visit root_path + signin(user.email, user.password) + + expect(page).to have_content t('titles.account_locked') + expect(page).to have_content(five_minute_countdown_regex) + expect(page). + to have_content t('devise.two_factor_authentication.max_generic_login_attempts_reached') + + # let lockout period expire + Timecop.travel(lockout_period) do + signin(user.email, user.password) + + expect(current_path).to eq login_two_factor_path(otp_delivery_preference: 'sms') + end + end + end + + context 'findtime period is greater than lockout period' do + it 'does not lock the user' do + allow(Figaro.env).to receive(:otp_delivery_blocklist_findtime).and_return('10') + lockout_period = Figaro.env.lockout_period_in_minutes.to_i.minutes + user = create(:user, :signed_up) + + sign_in_before_2fa(user) + + Figaro.env.otp_delivery_blocklist_maxretry.to_i.times do + click_link t('links.two_factor_authentication.resend_code.sms') + end + + expect(page).to have_content t('titles.account_locked') + + # let lockout period expire + Timecop.travel(lockout_period) do + signin(user.email, user.password) + + expect(current_path).to eq login_two_factor_path(otp_delivery_preference: 'sms') + end + end + end + + context 'user requests OTP several times but spaced out far apart' do + it 'does not lock the user out' do + max_attempts = Figaro.env.otp_delivery_blocklist_maxretry.to_i + findtime = Figaro.env.otp_delivery_blocklist_findtime.to_i.minutes + user = create(:user, :signed_up) + + sign_in_before_2fa(user) + (max_attempts - 1).times do + click_link t('links.two_factor_authentication.resend_code.sms') + end + click_submit_default + + expect(current_path).to eq account_path + + phone_fingerprint = Pii::Fingerprinter.fingerprint(user.phone) + rate_limited_phone = OtpRequestsTracker.find_by(phone_fingerprint: phone_fingerprint) + + # let findtime period expire + rate_limited_phone.update(otp_last_sent_at: Time.zone.now - (findtime + 1)) + + visit destroy_user_session_url + sign_in_before_2fa(user) + + expect(rate_limited_phone.reload.otp_send_count).to eq 1 + + click_link t('links.two_factor_authentication.resend_code.sms') + + expect(current_path).to eq login_two_factor_path(otp_delivery_preference: 'sms') + + click_submit_default + + expect(current_path).to eq account_path + end + end + + context '2 users with same phone number request OTP too many times within findtime' do + it 'locks both users out' do + allow(Figaro.env).to receive(:otp_delivery_blocklist_maxretry).and_return('3') + first_user = create(:user, :signed_up, phone: '+1 703-555-1212') + second_user = create(:user, :signed_up, phone: '+1 703-555-1212') + max_attempts = Figaro.env.otp_delivery_blocklist_maxretry.to_i + + sign_in_before_2fa(first_user) + click_link t('links.two_factor_authentication.resend_code.sms') + + expect(current_path).to eq login_two_factor_path(otp_delivery_preference: 'sms') + + visit destroy_user_session_url + + sign_in_before_2fa(second_user) + click_link t('links.two_factor_authentication.resend_code.sms') + phone_fingerprint = Pii::Fingerprinter.fingerprint(first_user.phone) + rate_limited_phone = OtpRequestsTracker.find_by(phone_fingerprint: phone_fingerprint) + + expect(current_path).to eq otp_send_path + expect(rate_limited_phone.otp_send_count).to eq max_attempts + + visit account_path + + expect(current_path).to eq root_path + + visit destroy_user_session_url + + signin(first_user.email, first_user.password) + + expect(page).to have_content t('devise.two_factor_authentication.max_otp_requests_reached') + + visit account_path + expect(current_path).to eq root_path + end + end + + context 'When setting up 2FA for the first time' do + it 'enforces rate limiting only for current phone' do + second_user = create(:user, :signed_up, phone: '+1 202-555-1212') + + sign_in_before_2fa + max_attempts = Figaro.env.otp_delivery_blocklist_maxretry.to_i + + submit_2fa_setup_form_with_valid_phone_and_choose_phone_call_delivery + + max_attempts.times do + click_link t('links.two_factor_authentication.resend_code.voice') + end + + expect(page).to have_content t('titles.account_locked') + + visit root_path + signin(second_user.email, second_user.password) + + expect(current_path).to eq login_two_factor_path(otp_delivery_preference: 'sms') + end + end + context 'user signs in while locked out' do it 'signs the user out and lets them know they are locked out' do user = create(:user, :signed_up, second_factor_locked_at: Time.zone.now - 1.minute) diff --git a/spec/services/otp_rate_limiter_spec.rb b/spec/services/otp_rate_limiter_spec.rb new file mode 100644 index 00000000000..bdf838dcac8 --- /dev/null +++ b/spec/services/otp_rate_limiter_spec.rb @@ -0,0 +1,56 @@ +require 'rails_helper' + +RSpec.describe OtpRateLimiter do + let(:current_user) { build(:user, :with_phone) } + subject(:otp_rate_limiter) { OtpRateLimiter.new(phone: current_user.phone, user: current_user) } + let(:phone_fingerprint) { Pii::Fingerprinter.fingerprint(current_user.phone) } + let(:rate_limited_phone) { OtpRequestsTracker.find_by(phone_fingerprint: phone_fingerprint) } + + describe '#exceeded_otp_send_limit?' do + it 'is false by default' do + expect(otp_rate_limiter.exceeded_otp_send_limit?).to eq(false) + end + + it 'is true after maxretry_times attemps in findtime minutes' do + expect(otp_rate_limiter.exceeded_otp_send_limit?).to eq(false) + + Figaro.env.otp_delivery_blocklist_maxretry.to_i.times do + otp_rate_limiter.increment + end + + expect(otp_rate_limiter.exceeded_otp_send_limit?).to eq(true) + end + end + + describe '#increment' do + it 'sets the otp_last_sent_at' do + now = Time.zone.now + otp_rate_limiter.increment + + expect(rate_limited_phone.otp_last_sent_at.to_i).to eq(now.to_i) + end + + it 'increments the otp_send_count' do + otp_rate_limiter.increment + + expect { otp_rate_limiter.increment }. + to change { rate_limited_phone.reload.otp_send_count }.from(1).to(2) + end + end + + describe '#lock_out_user' do + before do + otp_rate_limiter.increment + rate_limited_phone.otp_last_sent_at = 5.minutes.ago + rate_limited_phone.otp_send_count = 0 + end + + it 'sets the second_factor_locked_at' do + expect(current_user.second_factor_locked_at).to be_nil + + otp_rate_limiter.lock_out_user + + expect(current_user.second_factor_locked_at.to_i).to eq(Time.zone.now.to_i) + end + end +end