Lock users out temporarily after too many OTPs

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

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

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

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

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

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

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

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') %>
+
+<h1 class="h3 my0">
+  <%= t('titles.account_locked') %>
+</h1>
+<p>
+<%= t("devise.two_factor_authentication.max_otp_requests_reached") %>
+</p>
+<p>
+  <%= t('devise.two_factor_authentication.please_try_again_html',
+  time_remaining: content_tag(:span, lockout_time_in_words, id: 'countdown')) %>
+</p>
+
+<%= nonced_javascript_tag do %>
+  var test = <%= lockout_time_remaining %> * 1000;
+  window.LoginGov.countdownTimer(document.getElementById('countdown'), test);
+<% end %>

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'

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',

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,

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.

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.

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

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

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