From cd54613020ae0cb2c5676900040f7e10cb860b03 Mon Sep 17 00:00:00 2001 From: Phil Lee Date: Wed, 23 Oct 2024 19:01:49 +0100 Subject: [PATCH] [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 --- app/controllers/claims_controller.rb | 2 +- .../reminders_form_callbacks.rb | 3 ++- app/forms/email_address_form.rb | 7 ++++++- app/forms/email_verification_form.rb | 3 ++- .../reminders/email_verification_form.rb | 3 ++- .../provider/start/email_address_form.rb | 2 +- app/forms/mobile_number_form.rb | 9 +++++++-- app/forms/mobile_verification_form.rb | 3 ++- app/mailers/claim_mailer.rb | 4 ++++ app/models/journeys/session_answers.rb | 2 ++ {lib => app/models}/one_time_password/base.rb | 6 +----- {lib => app/models}/one_time_password/generator.rb | 4 ++-- {lib => app/models}/one_time_password/validator.rb | 4 ++-- config/application.rb | 1 - lib/one_time_password.rb | 3 --- spec/forms/email_verification_form_spec.rb | 11 +++++++---- .../reminders/email_verification_form_spec.rb | 10 +++++++--- spec/forms/mobile_verification_form_spec.rb | 9 ++++++--- spec/lib/one_time_password/generator_spec.rb | 7 +++---- spec/lib/one_time_password/validator_spec.rb | 9 +++++---- 20 files changed, 62 insertions(+), 40 deletions(-) rename {lib => app/models}/one_time_password/base.rb (66%) rename {lib => app/models}/one_time_password/generator.rb (64%) rename {lib => app/models}/one_time_password/validator.rb (91%) delete mode 100644 lib/one_time_password.rb diff --git a/app/controllers/claims_controller.rb b/app/controllers/claims_controller.rb index b7ed3bf5aa..673afea846 100644 --- 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 index d2b6549973..fe5c115279 100644 --- 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 index 8e8b53704b..1eb2beeb21 100644 --- 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 index d89ce82fe5..d32db085ab 100644 --- 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 index 1016240971..098d921de8 100644 --- 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 index 89e21d5baf..0c30e62af1 100644 --- 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 index 3e5d85dc18..3e3af3cc6e 100644 --- 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 index 6dc62f5d81..34808cc1cf 100644 --- 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 index 7da5c336b2..7a07b4e888 100644 --- 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 index b28c219a47..8d5afb0228 100644 --- 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 b/app/models/one_time_password/base.rb similarity index 66% rename from lib/one_time_password/base.rb rename to app/models/one_time_password/base.rb index 5a71c1ed83..7b7dbecef6 100644 --- a/lib/one_time_password/base.rb +++ b/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 b/app/models/one_time_password/generator.rb similarity index 64% rename from lib/one_time_password/generator.rb rename to app/models/one_time_password/generator.rb index b73b647551..1bee642a22 100644 --- a/lib/one_time_password/generator.rb +++ b/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 b/app/models/one_time_password/validator.rb similarity index 91% rename from lib/one_time_password/validator.rb rename to app/models/one_time_password/validator.rb index c17fecb448..bb49be2f87 100644 --- a/lib/one_time_password/validator.rb +++ b/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 index 016ad05e2b..04b8d65fe8 100644 --- 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 deleted file mode 100644 index 3003d8ad03..0000000000 --- a/lib/one_time_password.rb +++ /dev/null @@ -1,3 +0,0 @@ -require_relative "one_time_password/base" -require_relative "one_time_password/validator" -require_relative "one_time_password/generator" diff --git a/spec/forms/email_verification_form_spec.rb b/spec/forms/email_verification_form_spec.rb index 9c6724b3e0..0a465fdbc4 100644 --- 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,19 +64,19 @@ 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 @@ -81,7 +84,7 @@ 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/forms/journeys/additional_payments_for_teaching/reminders/email_verification_form_spec.rb b/spec/forms/journeys/additional_payments_for_teaching/reminders/email_verification_form_spec.rb index 5f4233803f..7b42e4c5cf 100644 --- a/spec/forms/journeys/additional_payments_for_teaching/reminders/email_verification_form_spec.rb +++ b/spec/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 index b68c5b1e9c..5738e3b68c 100644 --- 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,13 +64,13 @@ 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 @@ -75,7 +78,7 @@ 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 index 10fd60f5f1..c93d962533 100644 --- 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 index f3335c9fcb..5bd18969d5 100644 --- 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 }