LG-260 Separate otp request tracking and throttling for verified phones

app/controllers/users/two_factor_authentication_controller.rb

@@ -191,7 +191,9 @@ def phone_to_deliver_to
     end
 
     def otp_rate_limiter
-      @_otp_rate_limited ||= OtpRateLimiter.new(phone: phone_to_deliver_to, user: current_user)
+      @_otp_rate_limited ||= OtpRateLimiter.new(phone: phone_to_deliver_to,
+                                                user: current_user,
+                                                phone_confirmed: authentication_context?)
     end
 
     def redirect_on_nothing_enabled

app/models/otp_requests_tracker.rb

@@ -1,10 +1,9 @@
 class OtpRequestsTracker < ApplicationRecord
-  def self.find_or_create_with_phone(phone)
-    tries ||= 1
-    phone ||= phone.strip
-    phone_fingerprint ||= Pii::Fingerprinter.fingerprint(phone)
+  def self.find_or_create_with_phone_and_confirmed(phone, phone_confirmed)
+    tries = 1
+    phone_fingerprint = Pii::Fingerprinter.fingerprint(phone.strip)
 
-    where(phone_fingerprint: phone_fingerprint).
+    where(phone_fingerprint: phone_fingerprint, phone_confirmed: phone_confirmed).
       first_or_create(otp_send_count: 0, otp_last_sent_at: Time.zone.now)
   rescue ActiveRecord::RecordNotUnique
     retry unless (tries -= 1).zero?

app/services/idv/send_phone_confirmation_otp.rb

@@ -48,6 +48,7 @@ def otp_rate_limiter
       @otp_rate_limiter ||= OtpRateLimiter.new(
         user: user,
         phone: phone,
+        phone_confirmed: true,
       )
     end
 

app/services/otp_rate_limiter.rb

@@ -1,7 +1,8 @@
 class OtpRateLimiter
-  def initialize(phone:, user:)
+  def initialize(phone:, user:, phone_confirmed:)
     @phone = phone
     @user = user
+    @phone_confirmed = phone_confirmed
   end
 
   def exceeded_otp_send_limit?
@@ -38,10 +39,10 @@ def increment
 
   private
 
-  attr_reader :phone, :user
+  attr_reader :phone, :user, :phone_confirmed
 
   def entry_for_current_phone
-    @entry ||= OtpRequestsTracker.find_or_create_with_phone(phone)
+    @entry ||= OtpRequestsTracker.find_or_create_with_phone_and_confirmed(phone, phone_confirmed)
   end
 
   def otp_last_sent_at

db/migrate/20180723091143_add_phone_confirmed_to_otp_requests_tracker.rb

@@ -0,0 +1,16 @@
+class AddPhoneConfirmedToOtpRequestsTracker < ActiveRecord::Migration[5.1]
+  disable_ddl_transaction!
+
+  def up
+    add_column :otp_requests_trackers, :phone_confirmed, :boolean
+    change_column_default :otp_requests_trackers, :phone_confirmed, false
+    add_index :otp_requests_trackers, ["phone_fingerprint","phone_confirmed"], name: "index_on_phone_and_confirmed", unique: true, algorithm: :concurrently
+    remove_index :otp_requests_trackers, ["phone_fingerprint"]
+  end
+
+  def down
+    remove_index :otp_requests_trackers, ["phone_fingerprint","phone_confirmed"]
+    remove_column :otp_requests_trackers, :phone_confirmed
+    add_index :otp_requests_trackers, ["phone_fingerprint"], unique: true
+  end
+end

db/schema.rb

@@ -281,7 +281,8 @@
     t.string "phone_fingerprint", default: "", null: false
     t.datetime "created_at"
     t.datetime "updated_at"
-    t.index ["phone_fingerprint"], name: "index_otp_requests_trackers_on_phone_fingerprint", unique: true
+    t.boolean "phone_confirmed", default: false
+    t.index ["phone_fingerprint", "phone_confirmed"], name: "index_on_phone_and_confirmed", unique: true
     t.index ["updated_at"], name: "index_otp_requests_trackers_on_updated_at"
   end
 

spec/controllers/users/two_factor_authentication_controller_spec.rb

@@ -191,10 +191,10 @@ def index
 
       it 'calls OtpRateLimiter#exceeded_otp_send_limit? and #increment' do
         otp_rate_limiter = instance_double(OtpRateLimiter)
-        allow(OtpRateLimiter).to receive(:new).with(
-          phone: MfaContext.new(@user).phone_configurations.first.phone,
-          user: @user,
-        ).and_return(otp_rate_limiter)
+        allow(OtpRateLimiter).to receive(:new).
+          with(phone: MfaContext.new(@user).phone_configurations.first.phone,
+               user: @user, phone_confirmed: true).
+          and_return(otp_rate_limiter)
 
         expect(otp_rate_limiter).to receive(:exceeded_otp_send_limit?).twice
         expect(otp_rate_limiter).to receive(:increment)

spec/features/phone/rate_limitting_spec.rb

@@ -17,21 +17,6 @@ def visit_otp_confirmation(delivery_method)
     end
   end
 
-  context 'on sign in' do
-    let(:user) { create(:user, :signed_up) }
-
-    it_behaves_like 'phone rate limitting', :sms
-    it_behaves_like 'phone rate limitting', :voice
-
-    def visit_otp_confirmation(delivery_method)
-      user.phone_configurations.first.update!(
-        phone: PhoneFormatter.format(phone),
-        delivery_preference: delivery_method,
-      )
-      sign_in_user(user)
-    end
-  end
-
   context 'on add phone' do
     let(:user) { create(:user, :signed_up) }
 

spec/features/two_factor_authentication/sign_in_spec.rb

@@ -263,7 +263,9 @@ def attempt_to_bypass_2fa
                otp_delivery_preference: 'voice',
                with: { phone: '+12255551000', delivery_preference: 'voice' })
       end
-      let(:otp_rate_limiter) { OtpRateLimiter.new(user: user, phone: '+12255551000') }
+      let(:otp_rate_limiter) do
+        OtpRateLimiter.new(user: user, phone_confirmed: true, phone: '+12255551000')
+      end
 
       it 'does not change their OTP delivery preference' do
         allow(OtpRateLimiter).to receive(:new).and_return(otp_rate_limiter)

spec/models/otp_requests_tracker_spec.rb

@@ -4,29 +4,30 @@
   let(:phone) { '+1 703 555 1212' }
   let(:phone_fingerprint) { Pii::Fingerprinter.fingerprint(phone) }
 
-  describe '.find_or_create_with_phone' do
+  describe '.find_or_create_with_phone_and_confirmed' do
     context 'match found' do
       it 'returns the existing record and does not change it' do
         OtpRequestsTracker.create(
           phone_fingerprint: phone_fingerprint,
+          phone_confirmed: true,
           otp_send_count: 3,
           otp_last_sent_at: Time.zone.now - 1.hour,
         )
 
         existing = OtpRequestsTracker.where(phone_fingerprint: phone_fingerprint).first
 
-        expect { OtpRequestsTracker.find_or_create_with_phone(phone) }.
+        expect { OtpRequestsTracker.find_or_create_with_phone_and_confirmed(phone, true) }.
           to_not change(OtpRequestsTracker, :count)
-        expect { OtpRequestsTracker.find_or_create_with_phone(phone) }.
+        expect { OtpRequestsTracker.find_or_create_with_phone_and_confirmed(phone, true) }.
           to_not change(existing, :otp_send_count)
-        expect { OtpRequestsTracker.find_or_create_with_phone(phone) }.
+        expect { OtpRequestsTracker.find_or_create_with_phone_and_confirmed(phone, true) }.
           to_not change(existing, :otp_last_sent_at)
       end
     end
 
     context 'match not found' do
       it 'creates new record with otp_send_count = 0 and otp_last_sent_at = current time' do
-        expect { OtpRequestsTracker.find_or_create_with_phone(phone) }.
+        expect { OtpRequestsTracker.find_or_create_with_phone_and_confirmed(phone, true) }.
           to change(OtpRequestsTracker, :count).by(1)
 
         existing = OtpRequestsTracker.where(phone_fingerprint: phone_fingerprint).first
@@ -43,7 +44,7 @@
           and_raise(ActiveRecord::RecordNotUnique.new(tracker))
 
         expect(OtpRequestsTracker).to receive(:where).exactly(:once)
-        expect { OtpRequestsTracker.find_or_create_with_phone(phone) }.
+        expect { OtpRequestsTracker.find_or_create_with_phone_and_confirmed(phone, true) }.
           to raise_error ActiveRecord::RecordNotUnique
       end
     end
@@ -54,6 +55,7 @@
       old_ort = OtpRequestsTracker.create(
         phone_fingerprint: phone_fingerprint,
         otp_send_count: 3,
+        phone_confirmed: true,
         otp_last_sent_at: Time.zone.now - 1.hour,
       )
       new_ort = OtpRequestsTracker.atomic_increment(old_ort.id)
@@ -64,6 +66,7 @@
       old_ort = OtpRequestsTracker.create(
         phone_fingerprint: phone_fingerprint,
         otp_send_count: 3,
+        phone_confirmed: true,
         otp_last_sent_at: Time.zone.now,
       )
       new_ort = OtpRequestsTracker.atomic_increment(old_ort.id)

spec/services/idv/send_phone_confirmation_otp_spec.rb

@@ -17,7 +17,7 @@
   let(:user) { create(:user, :signed_up) }
 
   let(:exceeded_otp_send_limit) { false }
-  let(:otp_rate_limiter) { OtpRateLimiter.new(user: user, phone: phone) }
+  let(:otp_rate_limiter) { OtpRateLimiter.new(user: user, phone: phone, phone_confirmed: true) }
 
   before do
     # Setup Idv::Session
@@ -28,7 +28,7 @@
       and_return(otp_code)
 
     # Mock OtpRateLimiter
-    allow(OtpRateLimiter).to receive(:new).with(user: user, phone: phone).
+    allow(OtpRateLimiter).to receive(:new).with(user: user, phone: phone, phone_confirmed: true).
       and_return(otp_rate_limiter)
     allow(otp_rate_limiter).to receive(:exceeded_otp_send_limit?).
       and_return(exceeded_otp_send_limit)

spec/services/otp_rate_limiter_spec.rb

@@ -3,15 +3,16 @@
 RSpec.describe OtpRateLimiter do
   let(:current_user) { build(:user, :with_phone) }
   let(:phone) { MfaContext.new(current_user).phone_configurations.first.phone }
-
   subject(:otp_rate_limiter) do
-    OtpRateLimiter.new(phone: phone, user: current_user)
+    OtpRateLimiter.new(phone: phone, user: current_user, phone_confirmed: false)
   end
-
-  let(:phone_fingerprint) do
-    Pii::Fingerprinter.fingerprint(phone)
+  subject(:otp_rate_limiter_confirmed) do
+    OtpRateLimiter.new(phone: phone, user: current_user, phone_confirmed: true)
+  end
+  let(:phone_fingerprint) { Pii::Fingerprinter.fingerprint(phone) }
+  let(:rate_limited_phone) do
+    OtpRequestsTracker.find_by(phone_fingerprint: phone_fingerprint, phone_confirmed: false)
   end
-  let(:rate_limited_phone) { OtpRequestsTracker.find_by(phone_fingerprint: phone_fingerprint) }
 
   describe '#exceeded_otp_send_limit?' do
     it 'is false by default' do
@@ -27,11 +28,23 @@
 
       expect(otp_rate_limiter.exceeded_otp_send_limit?).to eq(true)
     end
+
+    it 'tracks verified phones separately. limiting one does not limit the other' do
+      expect(otp_rate_limiter.exceeded_otp_send_limit?).to eq(false)
+
+      (Figaro.env.otp_delivery_blocklist_maxretry.to_i + 1).times do
+        otp_rate_limiter.increment
+      end
+
+      expect(otp_rate_limiter.exceeded_otp_send_limit?).to eq(true)
+      expect(otp_rate_limiter_confirmed.exceeded_otp_send_limit?).to eq(false)
+    end
   end
 
   describe '#increment' do
     it 'updates otp_last_sent_at' do
-      tracker = OtpRequestsTracker.find_or_create_with_phone(phone)
+      tracker = OtpRequestsTracker.find_or_create_with_phone_and_confirmed(phone,
+                                                                           false)
       old_otp_last_sent_at = tracker.reload.otp_last_sent_at
       otp_rate_limiter.increment
       new_otp_last_sent_at = tracker.reload.otp_last_sent_at