From 356ce57e0658cc6d9e5be165398b8f2b3193942d Mon Sep 17 00:00:00 2001
From: Mitchell Henke <mitchell.henke@gsa.gov>
Date: Wed, 20 Oct 2021 15:15:48 -0500
Subject: [PATCH] fix database race condition when tracking OTP requests

---
 app/models/otp_requests_tracker.rb       | 12 ++++--------
 spec/models/otp_requests_tracker_spec.rb | 15 +--------------
 spec/services/otp_rate_limiter_spec.rb   |  1 +
 3 files changed, 6 insertions(+), 22 deletions(-)

diff --git a/app/models/otp_requests_tracker.rb b/app/models/otp_requests_tracker.rb
index 64ae71195e0..e95ca79b878 100644
--- a/app/models/otp_requests_tracker.rb
+++ b/app/models/otp_requests_tracker.rb
@@ -1,13 +1,9 @@
 class OtpRequestsTracker < ApplicationRecord
   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, 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?
-    raise
+    create_or_find_by(
+      phone_fingerprint: Pii::Fingerprinter.fingerprint(phone.strip),
+      phone_confirmed: phone_confirmed,
+    )
   end
 
   def self.atomic_increment(id)
diff --git a/spec/models/otp_requests_tracker_spec.rb b/spec/models/otp_requests_tracker_spec.rb
index a1a1868425d..75230f999e0 100644
--- a/spec/models/otp_requests_tracker_spec.rb
+++ b/spec/models/otp_requests_tracker_spec.rb
@@ -26,26 +26,13 @@
     end
 
     context 'match not found' do
-      it 'creates new record with otp_send_count = 0 and otp_last_sent_at = current time' do
+      it 'creates new record with otp_send_count = 0' do
         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
 
         expect(existing.otp_send_count).to eq 0
-        expect(existing.otp_last_sent_at).to be_within(2.seconds).of(Time.zone.now)
-      end
-    end
-
-    context 'race condition' do
-      it 'retries once, then raises ActiveRecord::RecordNotUnique' do
-        tracker = OtpRequestsTracker.new
-        allow(OtpRequestsTracker).to receive(:where).
-          and_raise(ActiveRecord::RecordNotUnique.new(tracker))
-
-        expect(OtpRequestsTracker).to receive(:where).exactly(:once)
-        expect { OtpRequestsTracker.find_or_create_with_phone_and_confirmed(phone, true) }.
-          to raise_error ActiveRecord::RecordNotUnique
       end
     end
   end
diff --git a/spec/services/otp_rate_limiter_spec.rb b/spec/services/otp_rate_limiter_spec.rb
index 8c0fddd6835..dc1eae240ae 100644
--- a/spec/services/otp_rate_limiter_spec.rb
+++ b/spec/services/otp_rate_limiter_spec.rb
@@ -47,6 +47,7 @@
         phone,
         false,
       )
+      otp_rate_limiter.increment
       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