From c1761d185b4497802e48071c46f4473ef8a11e69 Mon Sep 17 00:00:00 2001 From: baarkerlounger Date: Wed, 6 Apr 2022 16:45:44 +0100 Subject: [PATCH] Use email for OTP code --- app/controllers/auth/sessions_controller.rb | 2 +- app/models/admin_user.rb | 6 ++--- app/models/user.rb | 21 ++++++++++------ app/services/sms.rb | 15 ----------- spec/features/admin_panel_spec.rb | 21 ++++++++-------- spec/features/auth/user_lockout_spec.rb | 7 ++++-- spec/features/user_spec.rb | 25 ++++++++++++++++--- .../auth/passwords_controller_spec.rb | 8 +++--- 8 files changed, 59 insertions(+), 46 deletions(-) delete mode 100644 app/services/sms.rb diff --git a/app/controllers/auth/sessions_controller.rb b/app/controllers/auth/sessions_controller.rb index 95654e520..a2284e48b 100644 --- a/app/controllers/auth/sessions_controller.rb +++ b/app/controllers/auth/sessions_controller.rb @@ -29,7 +29,7 @@ private end def after_sign_in_path_for(resource) - if resource_class == AdminUser + if resource.need_two_factor_authentication?(request) admin_user_two_factor_authentication_path else params.dig(resource_class_name, "start").present? ? case_logs_path : super diff --git a/app/models/admin_user.rb b/app/models/admin_user.rb index dbb8fa196..7fe671e05 100644 --- a/app/models/admin_user.rb +++ b/app/models/admin_user.rb @@ -21,13 +21,13 @@ class AdminUser < ApplicationRecord validates :phone, presence: true, numericality: true - MFA_SMS_TEMPLATE_ID = "bf309d93-804e-4f95-b1f4-bd513c48ecb0".freeze + MFA_TEMPLATE_ID = "6bdf5ee1-8e01-4be1-b1f9-747061d8a24c".freeze RESET_PASSWORD_TEMPLATE_ID = "fbb2d415-b9b1-4507-ba0a-6e542fa3504d".freeze def send_two_factor_authentication_code(code) - template_id = MFA_SMS_TEMPLATE_ID + template_id = MFA_TEMPLATE_ID personalisation = { otp: code } - Sms.send(phone, template_id, personalisation) + DeviseNotifyMailer.new.send_email(email, template_id, personalisation) end def reset_password_notify_template diff --git a/app/models/user.rb b/app/models/user.rb index 5c44bf679..69d63f3ac 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -48,13 +48,6 @@ class User < ApplicationRecord case_logs.not_completed end - RESET_PASSWORD_TEMPLATE_ID = "2c410c19-80a7-481c-a531-2bcb3264f8e6".freeze - SET_PASSWORD_TEMPLATE_ID = "257460a6-6616-4640-a3f9-17c3d73d9e91".freeze - - def reset_password_notify_template - last_sign_in_at ? RESET_PASSWORD_TEMPLATE_ID : SET_PASSWORD_TEMPLATE_ID - end - def is_key_contact? is_key_contact end @@ -71,9 +64,21 @@ class User < ApplicationRecord update!(is_dpo: true) end + MFA_TEMPLATE_ID = "6bdf5ee1-8e01-4be1-b1f9-747061d8a24c".freeze + RESET_PASSWORD_TEMPLATE_ID = "2c410c19-80a7-481c-a531-2bcb3264f8e6".freeze + SET_PASSWORD_TEMPLATE_ID = "257460a6-6616-4640-a3f9-17c3d73d9e91".freeze + + def reset_password_notify_template + last_sign_in_at ? RESET_PASSWORD_TEMPLATE_ID : SET_PASSWORD_TEMPLATE_ID + end + def need_two_factor_authentication?(_request) support? end - def send_two_factor_authentication_code(code); end + def send_two_factor_authentication_code(code) + template_id = MFA_TEMPLATE_ID + personalisation = { otp: code } + DeviseNotifyMailer.new.send_email(email, template_id, personalisation) + end end diff --git a/app/services/sms.rb b/app/services/sms.rb deleted file mode 100644 index aaf223f1c..000000000 --- a/app/services/sms.rb +++ /dev/null @@ -1,15 +0,0 @@ -require "notifications/client" - -class Sms - def self.notify_client - Notifications::Client.new(ENV["GOVUK_NOTIFY_API_KEY"]) - end - - def self.send(phone_number, template_id, args) - notify_client.send_sms( - phone_number:, - template_id:, - personalisation: args, - ) - end -end diff --git a/spec/features/admin_panel_spec.rb b/spec/features/admin_panel_spec.rb index dcbd4ab80..9ade961ed 100644 --- a/spec/features/admin_panel_spec.rb +++ b/spec/features/admin_panel_spec.rb @@ -2,13 +2,15 @@ require "rails_helper" RSpec.describe "Admin Panel" do let!(:admin) { FactoryBot.create(:admin_user) } + let(:devise_notify_mailer) { DeviseNotifyMailer.new } let(:notify_client) { instance_double(Notifications::Client) } - let(:mfa_template_id) { AdminUser::MFA_SMS_TEMPLATE_ID } + let(:mfa_template_id) { AdminUser::MFA_TEMPLATE_ID } let(:otp) { "999111" } before do - allow(Sms).to receive(:notify_client).and_return(notify_client) - allow(notify_client).to receive(:send_sms).and_return(true) + allow(DeviseNotifyMailer).to receive(:new).and_return(devise_notify_mailer) + allow(devise_notify_mailer).to receive(:notify_client).and_return(notify_client) + allow(notify_client).to receive(:send_email).and_return(true) end it "shows the admin sign in page" do @@ -26,8 +28,12 @@ RSpec.describe "Admin Panel" do end it "authenticates successfully" do - expect(notify_client).to receive(:send_sms).with( - hash_including(phone_number: admin.phone, template_id: mfa_template_id), + expect(notify_client).to receive(:send_email).with( + { + email_address: admin.email, + template_id: mfa_template_id, + personalisation: { otp: }, + }, ) click_button("Sign in") fill_in("code", with: otp) @@ -108,14 +114,9 @@ RSpec.describe "Admin Panel" do context "when the admin has forgotten their password" do let!(:admin_user) { FactoryBot.create(:admin_user, last_sign_in_at: Time.zone.now) } - let(:notify_client) { instance_double(Notifications::Client) } let(:reset_password_token) { "MCDH5y6Km-U7CFPgAMVS" } - let(:devise_notify_mailer) { DeviseNotifyMailer.new } before do - allow(DeviseNotifyMailer).to receive(:new).and_return(devise_notify_mailer) - allow(devise_notify_mailer).to receive(:notify_client).and_return(notify_client) - allow(notify_client).to receive(:send_email).and_return(true) allow(Devise.token_generator).to receive(:generate).and_return(reset_password_token) end diff --git a/spec/features/auth/user_lockout_spec.rb b/spec/features/auth/user_lockout_spec.rb index a426982ba..c1ba131df 100644 --- a/spec/features/auth/user_lockout_spec.rb +++ b/spec/features/auth/user_lockout_spec.rb @@ -48,9 +48,12 @@ RSpec.describe "User Lockout" do end context "when login-in with the right admin password and incorrect 2FA token up to a maximum number of attempts" do + let(:devise_notify_mailer) { DeviseNotifyMailer.new } + before do - allow(Sms).to receive(:notify_client).and_return(notify_client) - allow(notify_client).to receive(:send_sms).and_return(true) + allow(DeviseNotifyMailer).to receive(:new).and_return(devise_notify_mailer) + allow(devise_notify_mailer).to receive(:notify_client).and_return(notify_client) + allow(notify_client).to receive(:send_email).and_return(true) visit("/admin/sign-in") fill_in("admin_user[email]", with: admin.email) diff --git a/spec/features/user_spec.rb b/spec/features/user_spec.rb index f7fb1f39b..3d20f51c0 100644 --- a/spec/features/user_spec.rb +++ b/spec/features/user_spec.rb @@ -327,19 +327,38 @@ RSpec.describe "User Features" do context "when the user is a customer support person" do context "when they are logging in" do - let!(:support_user) { FactoryBot.create(:user, :support, last_sign_in_at: Time.zone.now) } + let(:support_user) { FactoryBot.create(:user, :support, last_sign_in_at: Time.zone.now) } + let(:devise_notify_mailer) { DeviseNotifyMailer.new } + let(:notify_client) { instance_double(Notifications::Client) } + let(:mfa_template_id) { User::MFA_TEMPLATE_ID } + let(:otp) { "999111" } before do + allow(DeviseNotifyMailer).to receive(:new).and_return(devise_notify_mailer) + allow(devise_notify_mailer).to receive(:notify_client).and_return(notify_client) + allow(notify_client).to receive(:send_email).and_return(true) + allow(SecureRandom).to receive(:random_number).and_return(otp) visit("/logs") fill_in("user[email]", with: support_user.email) fill_in("user[password]", with: "pAssword1") - click_button("Sign in") end - it "asks for a 2FA code" do + it "shows the 2FA code screen" do + click_button("Sign in") expect(page).to have_content("We’ve sent you an email with a security code.") expect(page).to have_field("user[code]") end + + it "sends a 2FA code by email" do + expect(notify_client).to receive(:send_email).with( + { + email_address: support_user.email, + template_id: mfa_template_id, + personalisation: { otp: }, + }, + ) + click_button("Sign in") + end end end end diff --git a/spec/requests/auth/passwords_controller_spec.rb b/spec/requests/auth/passwords_controller_spec.rb index 0243f75e2..e10e9f2e3 100644 --- a/spec/requests/auth/passwords_controller_spec.rb +++ b/spec/requests/auth/passwords_controller_spec.rb @@ -80,8 +80,8 @@ RSpec.describe Auth::PasswordsController, type: :request do let(:new_value) { "new-password" } before do - allow(Sms).to receive(:notify_client).and_return(notify_client) - allow(notify_client).to receive(:send_sms).and_return(true) + allow(DeviseNotifyMailer).to receive(:notify_client).and_return(notify_client) + allow(notify_client).to receive(:send_email).and_return(true) end it "renders the user edit password view" do @@ -137,8 +137,8 @@ RSpec.describe Auth::PasswordsController, type: :request do expect(response).to redirect_to("/admin/two-factor-authentication") end - it "triggers an SMS" do - expect(notify_client).to receive(:send_sms) + it "triggers an email" do + expect(notify_client).to receive(:send_email) put "/admin/password", headers: headers, params: params end end