[CAPT-1781] Revisit OTP (#3333)

* move one time password lib => models * otp issuer now keyword argument * email verification now uses non shared secret * mobile verification now uses non shared secret * remove OTP shared SECRET --------- Co-authored-by: Steven Lorek <[email protected]>
DFE-Digital · Oct 23, 2024 · cd54613 · cd54613
1 parent 7eb9c54
commit cd54613
Show file tree

Hide file tree

Showing 20 changed files with 62 additions and 40 deletions.
diff --git a/app/controllers/claims_controller.rb b/app/controllers/claims_controller.rb
@@ -120,7 +120,7 @@ def correct_journey_for_claim_in_progress?
   def handle_magic_link
     return unless params[:code] && params[:email]
 
-    otp = OneTimePassword::Validator.new(params[:code], secret: ENV.fetch("EY_MAGIC_LINK_SECRET") + params[:email])
+    otp = OneTimePassword::Validator.new(params[:code], secret: ROTP::Base32.encode(ENV.fetch("EY_MAGIC_LINK_SECRET") + params[:email]))
     if otp.valid?
       journey_session.answers.assign_attributes(email_address: params[:email])
       journey_session.answers.assign_attributes(email_verified: true)

diff --git a/app/controllers/journeys/additional_payments_for_teaching/reminders_form_callbacks.rb b/app/controllers/journeys/additional_payments_for_teaching/reminders_form_callbacks.rb
@@ -15,6 +15,7 @@ def email_verification_before_update
 
       def personal_details_after_form_save_success
         answers.reminder_id = current_reminder.to_param
+        answers.email_verification_secret = ROTP::Base32.random
         journey_session.save!
         try_mailer { send_verification_email } || return
         redirect_to_next_slug
@@ -42,7 +43,7 @@ def inject_sent_one_time_password_at_into_the_form
       end
 
       def send_verification_email
-        otp = OneTimePassword::Generator.new
+        otp = OneTimePassword::Generator.new(secret: answers.email_verification_secret)
         ReminderMailer.email_verification(current_reminder, otp.code).deliver_now
         session[:sent_one_time_password_at] = Time.now
       end

diff --git a/app/forms/email_address_form.rb b/app/forms/email_address_form.rb
@@ -24,6 +24,7 @@ def save
     journey_session.answers.assign_attributes(
       email_address: email_address,
       email_verified: email_verified,
+      email_verification_secret: otp_secret,
       sent_one_time_password_at: Time.now
     )
 
@@ -43,7 +44,11 @@ def email_verified
     answers.email_verified
   end
 
+  def otp_secret
+    @otp_secret ||= ROTP::Base32.random
+  end
+
   def otp_code
-    @otp_code ||= OneTimePassword::Generator.new.code
+    @otp_code ||= OneTimePassword::Generator.new(secret: otp_secret).code
   end
 end
diff --git a/app/forms/email_verification_form.rb b/app/forms/email_verification_form.rb
@@ -33,7 +33,8 @@ def sent_one_time_password_must_be_valid
   def otp_must_be_valid
     otp = OneTimePassword::Validator.new(
       one_time_password,
-      sent_one_time_password_at
+      sent_one_time_password_at,
+      secret: journey_session.answers.email_verification_secret
     )
 
     errors.add(:one_time_password, otp.warning) unless otp.valid?

diff --git a/app/forms/journeys/additional_payments_for_teaching/reminders/email_verification_form.rb b/app/forms/journeys/additional_payments_for_teaching/reminders/email_verification_form.rb
@@ -45,7 +45,8 @@ def sent_one_time_password_must_be_valid
         def otp_must_be_valid
           otp = OneTimePassword::Validator.new(
             one_time_password,
-            sent_one_time_password_at
+            sent_one_time_password_at,
+            secret: journey_session.answers.email_verification_secret
           )
 
           errors.add(:one_time_password, otp.warning) unless otp.valid?

diff --git a/app/forms/journeys/early_years_payment/provider/start/email_address_form.rb b/app/forms/journeys/early_years_payment/provider/start/email_address_form.rb
@@ -31,7 +31,7 @@ def email_verified
           end
 
           def otp_code(email_address)
-            @otp_code ||= OneTimePassword::Generator.new(secret: ENV.fetch("EY_MAGIC_LINK_SECRET") + email_address).code
+            @otp_code ||= OneTimePassword::Generator.new(secret: ROTP::Base32.encode(ENV.fetch("EY_MAGIC_LINK_SECRET") + email_address)).code
           end
         end
       end

diff --git a/app/forms/mobile_number_form.rb b/app/forms/mobile_number_form.rb
@@ -25,6 +25,7 @@ def save
     journey_session.answers.assign_attributes(
       mobile_number: mobile_number,
       mobile_verified: nil,
+      mobile_verification_secret: otp_secret,
       sent_one_time_password_at: sent_one_time_password_at
     )
 
@@ -38,17 +39,21 @@ def save
 
   def send_sms_message
     if Rails.env.development?
-      Rails.logger.info("\n\nSMS CODE: #{OneTimePassword::Generator.new.code}\n")
+      Rails.logger.info("\n\nSMS CODE: #{OneTimePassword::Generator.new(secret: otp_secret).code}\n")
       return true
     end
 
     NotifySmsMessage.new(
       phone_number: mobile_number,
       template_id: NotifySmsMessage::OTP_PROMPT_TEMPLATE_ID,
-      personalisation: {otp: OneTimePassword::Generator.new.code}
+      personalisation: {otp: OneTimePassword::Generator.new(secret: otp_secret).code}
     ).deliver!
   end
 
+  def otp_secret
+    @otp_secret ||= ROTP::Base32.random
+  end
+
   def mobile_number_changed?
     mobile_number != answers.mobile_number
   end

diff --git a/app/forms/mobile_verification_form.rb b/app/forms/mobile_verification_form.rb
@@ -23,7 +23,8 @@ def save
   def otp_validate
     otp = OneTimePassword::Validator.new(
       one_time_password,
-      answers.sent_one_time_password_at
+      answers.sent_one_time_password_at,
+      secret: journey_session.answers.mobile_verification_secret
     )
 
     errors.add(:one_time_password, otp.warning) unless otp.valid?

diff --git a/app/mailers/claim_mailer.rb b/app/mailers/claim_mailer.rb
@@ -69,6 +69,10 @@ def email_verification(claim, one_time_password)
       validity_duration: one_time_password_validity_duration
     }
 
+    if Rails.env.development?
+      Rails.logger.info("\n\nEmail verification code: #{@one_time_password}\n")
+    end
+
     send_mail(OTP_EMAIL_NOTIFY_TEMPLATE_ID, personalisation)
   end
 

diff --git a/app/models/journeys/session_answers.rb b/app/models/journeys/session_answers.rb
@@ -28,7 +28,9 @@ class SessionAnswers
     attribute :provide_mobile_number, :boolean
     attribute :mobile_number, :string
     attribute :email_verified, :boolean
+    attribute :email_verification_secret, :string
     attribute :mobile_verified, :boolean
+    attribute :mobile_verification_secret, :string
     attribute :hmrc_bank_validation_succeeded, :boolean
     attribute :hmrc_bank_validation_responses, default: []
     attribute :logged_in_with_tid, :boolean

diff --git a/lib/one_time_password/base.rb → app/models/one_time_password/base.rb b/lib/one_time_password/base.rb → app/models/one_time_password/base.rb
@@ -1,10 +1,10 @@
 require "rotp"
+
 module OneTimePassword
   class Base
     DRIFT = 900
     LENGTH = 6
     ISSUER = "Claim Additional Payments for Teaching"
-    SECRET = ROTP::Base32.random.freeze
 
     def rotp
       @rotp ||= ROTP::TOTP
@@ -13,9 +13,5 @@ def rotp
     private
 
     attr_reader :issuer, :secret
-
-    def encode_secret(secret)
-      ROTP::Base32.encode(secret) if secret
-    end
   end
 end
diff --git a/lib/one_time_password/generator.rb → app/models/one_time_password/generator.rb b/lib/one_time_password/generator.rb → app/models/one_time_password/generator.rb
@@ -1,8 +1,8 @@
 module OneTimePassword
   class Generator < Base
-    def initialize(issuer = nil, secret: nil)
+    def initialize(secret:, issuer: nil)
       @issuer = issuer || ISSUER
-      @secret = encode_secret(secret) || SECRET
+      @secret = secret
     end
 
     def code

diff --git a/lib/one_time_password/validator.rb → app/models/one_time_password/validator.rb b/lib/one_time_password/validator.rb → app/models/one_time_password/validator.rb
@@ -1,9 +1,9 @@
 module OneTimePassword
   class Validator < Base
-    def initialize(code, generated_at = nil, secret: nil)
+    def initialize(code, generated_at = nil, secret:)
       @code = code
       @generated_at = generated_at
-      @secret = encode_secret(secret) || SECRET
+      @secret = secret
     end
 
     def valid?

diff --git a/config/application.rb b/config/application.rb
@@ -20,7 +20,6 @@
 require_relative "../lib/dfe_sign_in"
 require_relative "../lib/hmrc"
 require_relative "../lib/ordnance_survey"
-require_relative "../lib/one_time_password"
 require_relative "../lib/dqt"
 require_relative "../lib/notify_sms_message"
 

diff --git a/lib/one_time_password.rb b/lib/one_time_password.rb
diff --git a/spec/forms/email_verification_form_spec.rb b/spec/forms/email_verification_form_spec.rb
@@ -2,6 +2,8 @@
 
 RSpec.describe EmailVerificationForm do
   shared_examples "email_verification" do |journey|
+    let(:secret) { ROTP::Base32.random }
+
     let(:params) do
       ActionController::Parameters.new(
         claim: {
@@ -14,6 +16,7 @@
       create(
         :"#{journey::I18N_NAMESPACE}_session",
         answers: {
+          email_verification_secret: secret,
           sent_one_time_password_at: sent_one_time_password_at
         }
       )
@@ -61,27 +64,27 @@
         end
 
         context "when the code has expired" do
-          let(:one_time_password) { OneTimePassword::Generator.new.code }
+          let(:one_time_password) { OneTimePassword::Generator.new(secret:).code }
           let(:sent_one_time_password_at) { 30.minutes.ago }
           it { is_expected.not_to be_valid }
         end
 
         context "when the code generation timestamp is missing" do
-          let(:one_time_password) { OneTimePassword::Generator.new.code }
+          let(:one_time_password) { OneTimePassword::Generator.new(secret:).code }
           let(:sent_one_time_password_at) { nil }
           it { is_expected.not_to be_valid }
         end
 
         context "when correct code" do
-          let(:one_time_password) { OneTimePassword::Generator.new.code }
+          let(:one_time_password) { OneTimePassword::Generator.new(secret:).code }
           let(:sent_one_time_password_at) { Time.now }
           it { is_expected.to be_valid }
         end
       end
     end
 
     describe "#save" do
-      let(:one_time_password) { OneTimePassword::Generator.new.code }
+      let(:one_time_password) { OneTimePassword::Generator.new(secret:).code }
       let(:sent_one_time_password_at) { Time.now }
 
       before { form.save }

diff --git a/...forms/journeys/additional_payments_for_teaching/reminders/email_verification_form_spec.rb b/...forms/journeys/additional_payments_for_teaching/reminders/email_verification_form_spec.rb
@@ -5,8 +5,12 @@
     described_class.new(reminder: reminder, journey:, journey_session:, params:)
   end
 
+  let(:secret) { ROTP::Base32.random }
   let(:journey) { Journeys::AdditionalPaymentsForTeaching }
-  let(:journey_session) { build(:additional_payments_session) }
+  let(:answers) do
+    {email_verification_secret: secret}
+  end
+  let(:journey_session) { build(:additional_payments_session, answers:) }
   let(:reminder) { Reminder.create! }
   let(:slug) { "email-verification" }
   let(:params) { ActionController::Parameters.new({slug:, form: form_params}) }
@@ -22,7 +26,7 @@
     context "valid params" do
       let(:form_params) do
         {
-          "one_time_password" => OneTimePassword::Generator.new.code,
+          "one_time_password" => OneTimePassword::Generator.new(secret:).code,
           "sent_one_time_password_at" => Time.now
         }
       end
@@ -36,7 +40,7 @@
     context "invalid params" do
       let(:form_params) do
         {
-          "one_time_password" => OneTimePassword::Generator.new.code,
+          "one_time_password" => OneTimePassword::Generator.new(secret:).code,
           "sent_one_time_password_at" => ""
         }
       end

diff --git a/spec/forms/mobile_verification_form_spec.rb b/spec/forms/mobile_verification_form_spec.rb
@@ -2,10 +2,13 @@
 
 RSpec.describe MobileVerificationForm do
   shared_examples "mobile_verification" do |journey|
+    let(:secret) { ROTP::Base32.random }
+
     let(:journey_session) do
       create(
         :"#{journey::I18N_NAMESPACE}_session",
         answers: {
+          mobile_verification_secret: secret,
           sent_one_time_password_at: sent_one_time_password_at
         }
       )
@@ -61,21 +64,21 @@
         end
 
         context "when the code has expired" do
-          let(:one_time_password) { OneTimePassword::Generator.new.code }
+          let(:one_time_password) { OneTimePassword::Generator.new(secret:).code }
           let(:sent_one_time_password_at) { 30.minutes.ago }
           it { is_expected.not_to be_valid }
         end
 
         context "when correct code" do
-          let(:one_time_password) { OneTimePassword::Generator.new.code }
+          let(:one_time_password) { OneTimePassword::Generator.new(secret:).code }
           let(:sent_one_time_password_at) { Time.now }
           it { is_expected.to be_valid }
         end
       end
     end
 
     describe "#save" do
-      let(:one_time_password) { OneTimePassword::Generator.new.code }
+      let(:one_time_password) { OneTimePassword::Generator.new(secret:).code }
       let(:sent_one_time_password_at) { Time.now }
 
       before { form.save }

diff --git a/spec/lib/one_time_password/generator_spec.rb b/spec/lib/one_time_password/generator_spec.rb
@@ -3,13 +3,14 @@
 RSpec.describe OneTimePassword::Generator do
   let(:totp) { instance_double ROTP::TOTP, now: one_time_passcode }
   let(:one_time_passcode) { 123456 }
+  let(:secret) { ROTP::Base32.random }
 
   before do
     allow(ROTP::TOTP).to receive(:new).and_return(totp)
   end
 
   describe "#code" do
-    subject { described_class.new.code }
+    subject { described_class.new(secret:).code }
 
     it "generates a new code" do
       expect(subject).to eq one_time_passcode
@@ -19,10 +20,8 @@
   context "specifying a secret" do
     subject { described_class.new(secret: secret).code }
 
-    let(:secret) { "somesecret" }
-
     it "uses the secret" do
-      expect(ROTP::TOTP).to receive(:new).with(ROTP::Base32.encode(secret), anything).and_return(totp)
+      expect(ROTP::TOTP).to receive(:new).with(secret, anything).and_return(totp)
       subject
     end
 

diff --git a/spec/lib/one_time_password/validator_spec.rb b/spec/lib/one_time_password/validator_spec.rb
@@ -1,9 +1,10 @@
 require "rails_helper"
 
 RSpec.describe OneTimePassword::Validator do
-  subject { described_class.new(one_time_passcode, generated_at) }
+  subject { described_class.new(one_time_passcode, generated_at, secret:) }
 
-  let!(:one_time_passcode) { OneTimePassword::Generator.new.code }
+  let(:secret) { ROTP::Base32.random }
+  let!(:one_time_passcode) { OneTimePassword::Generator.new(secret:).code }
   let!(:generated_at) { Time.now }
 
   context "with a valid code" do
@@ -78,7 +79,7 @@
     end
 
     context "when generated_at is not specified" do
-      subject { described_class.new(one_time_passcode) }
+      subject { described_class.new(one_time_passcode, secret:) }
 
       it { is_expected.to_not be_valid }
 
@@ -89,7 +90,7 @@
   end
 
   context "not specifying generated_at" do
-    subject { described_class.new(one_time_passcode) }
+    subject { described_class.new(one_time_passcode, secret:) }
 
     context "with a valid code" do
       it { is_expected.to be_valid }
-Original file line number
+Diff line change
@@ Expand Up / @@ -31,7 +31,7 @@ def email_verified @@
               end
               def otp_code(email_address)
-                @otp_code ||= OneTimePassword::Generator.new(secret: ENV.fetch("EY_MAGIC_LINK_SECRET") + email_address).code
+                @otp_code ||= OneTimePassword::Generator.new(secret: ROTP::Base32.encode(ENV.fetch("EY_MAGIC_LINK_SECRET") + email_address)).code
               end
             end
           end
@@ Expand Down @@