Text messages sometimes take a few minutes to arrive. If you do not receive the text message, you can request a new one.
+Emails sometimes take a few minutes to arrive. If you do not receive the email, you can request a new one.
<%= f.govuk_submit "Resend security code" %>We’ve sent you a text message with a security code.
+We’ve sent you an email with a security code.
<%= f.govuk_number_field :code, label: { text: "Security code", size: "m" }, @@ -24,5 +25,5 @@ <% end %>- <%= govuk_link_to "Not received a text message?", admin_two_factor_authentication_resend_path %> + <%= govuk_link_to "Not received an email?", send("#{resource_name}_two_factor_authentication_resend_path") %>
diff --git a/config/routes.rb b/config/routes.rb index 591f9cad5..4cb364878 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -14,12 +14,13 @@ Rails.application.routes.draw do sign_in: "sign-in", sign_out: "sign-out", two_factor_authentication: "two-factor-authentication", + two_factor_authentication_resend_code: "resend-code", }, sign_out_via: %i[get], } devise_scope :admin_user do - get "admin/two-factor-authentication/resend", to: "auth/two_factor_authentication#show_resend" + get "admin/two-factor-authentication/resend", to: "auth/two_factor_authentication#show_resend", as: "admin_user_two_factor_authentication_resend" end devise_for :users, { @@ -27,15 +28,20 @@ Rails.application.routes.draw do controllers: { passwords: "auth/passwords", sessions: "auth/sessions", + two_factor_authentication: "auth/two_factor_authentication", }, path_names: { sign_in: "sign-in", sign_out: "sign-out", + two_factor_authentication: "two-factor-authentication", + two_factor_authentication_resend_code: "resend-code", }, + sign_out_via: %i[get], } devise_scope :user do get "account/password/reset-confirmation", to: "auth/passwords#reset_confirmation" + get "account/two-factor-authentication/resend", to: "auth/two_factor_authentication#show_resend", as: "user_two_factor_authentication_resend" put "account", to: "users#update" end diff --git a/db/migrate/20220406093139_two_factor_authentication_add_to_user.rb b/db/migrate/20220406093139_two_factor_authentication_add_to_user.rb new file mode 100644 index 000000000..49a88ac68 --- /dev/null +++ b/db/migrate/20220406093139_two_factor_authentication_add_to_user.rb @@ -0,0 +1,15 @@ +class TwoFactorAuthenticationAddToUser < ActiveRecord::Migration[7.0] + def change + change_table :users, bulk: true do |t| + t.column :second_factor_attempts_count, :integer, default: 0 + t.column :encrypted_otp_secret_key, :string + t.column :encrypted_otp_secret_key_iv, :string + t.column :encrypted_otp_secret_key_salt, :string + t.column :direct_otp, :string + t.column :direct_otp_sent_at, :datetime + t.column :totp_timestamp, :timestamp + + t.index :encrypted_otp_secret_key, unique: true + end + end +end diff --git a/db/schema.rb b/db/schema.rb index c1ca092bb..61368691c 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -324,7 +324,15 @@ ActiveRecord::Schema[7.0].define(version: 202202071123100) do t.boolean "is_dpo", default: false t.boolean "is_key_contact", default: false t.string "phone" + t.integer "second_factor_attempts_count", default: 0 + t.string "encrypted_otp_secret_key" + t.string "encrypted_otp_secret_key_iv" + t.string "encrypted_otp_secret_key_salt" + t.string "direct_otp" + t.datetime "direct_otp_sent_at", precision: nil + t.datetime "totp_timestamp", precision: nil t.index ["email"], name: "index_users_on_email", unique: true + t.index ["encrypted_otp_secret_key"], name: "index_users_on_encrypted_otp_secret_key", unique: true t.index ["organisation_id"], name: "index_users_on_organisation_id" t.index ["reset_password_token"], name: "index_users_on_reset_password_token", unique: true t.index ["unlock_token"], name: "index_users_on_unlock_token", unique: true diff --git a/spec/factories/user.rb b/spec/factories/user.rb index 0de22af97..9b8edb23f 100644 --- a/spec/factories/user.rb +++ b/spec/factories/user.rb @@ -11,6 +11,9 @@ FactoryBot.define do trait :data_protection_officer do is_dpo { true } end + trait :support do + role { "support" } + end created_at { Time.zone.now } updated_at { Time.zone.now } end diff --git a/spec/features/admin_panel_spec.rb b/spec/features/admin_panel_spec.rb index 3536e1c08..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) @@ -42,7 +48,7 @@ RSpec.describe "Admin Panel" do admin.update!(direct_otp_sent_at: 16.minutes.ago) fill_in("code", with: otp) click_button("Submit") - expect(page).to have_content("Check your phone") + expect(page).to have_content("Check your email") expect(page).to have_http_status(:unprocessable_entity) expect(page).to have_title("Error") expect(page).to have_selector("#error-summary-title") @@ -58,7 +64,7 @@ RSpec.describe "Admin Panel" do click_button("Sign in") fill_in("code", with: otp) click_button("Submit") - expect(page).to have_content("Check your phone") + expect(page).to have_content("Check your email") expect(page).to have_http_status(:unprocessable_entity) expect(page).to have_title("Error") expect(page).to have_selector("#error-summary-title") @@ -74,12 +80,12 @@ RSpec.describe "Admin Panel" do end it "displays the resend view" do - click_link("Not received a text message?") + click_link("Not received an email?") expect(page).to have_button("Resend security code") end it "send a new OTP code and redirects back to the 2FA view" do - click_link("Not received a text message?") + click_link("Not received an email?") expect { click_button("Resend security code") }.to(change { admin.reload.direct_otp }) expect(page).to have_current_path("/admin/two-factor-authentication") end @@ -102,20 +108,15 @@ RSpec.describe "Admin Panel" do fill_in("admin_user[email]", with: admin.email) fill_in("admin_user[password]", with: admin.password) click_button("Sign in") - expect(page).to have_content("Check your phone") + expect(page).to have_content("Check your email") end end 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 d7326dd7e..5cd035278 100644 --- a/spec/features/user_spec.rb +++ b/spec/features/user_spec.rb @@ -15,7 +15,7 @@ RSpec.describe "User Features" do end context "when the user navigates to case logs" do - it " is required to log in" do + it "is required to log in" do visit("/logs") expect(page).to have_current_path("/account/sign-in") expect(page).to have_content("Sign in to your account to submit CORE data") @@ -26,7 +26,7 @@ RSpec.describe "User Features" do expect(page).to have_no_content("You need to sign in or sign up before continuing.") end - it " is redirected to case logs after signing in" do + it "is redirected to case logs after signing in" do visit("/logs") fill_in("user[email]", with: user.email) fill_in("user[password]", with: "pAssword1") @@ -34,7 +34,7 @@ RSpec.describe "User Features" do expect(page).to have_current_path("/logs") end - it " can log out again", js: true do + it "can log out again", js: true do visit("/logs") fill_in("user[email]", with: user.email) fill_in("user[password]", with: "pAssword1") @@ -44,7 +44,7 @@ RSpec.describe "User Features" do expect(page).to have_content("Start now") end - it " can log out again with js disabled" do + it "can log out again with js disabled" do visit("/logs") fill_in("user[email]", with: user.email) fill_in("user[password]", with: "pAssword1") @@ -56,13 +56,13 @@ RSpec.describe "User Features" do end context "when the user has forgotten their password" do - it " is redirected to the reset password page when they click the reset password link" do + it "is redirected to the reset password page when they click the reset password link" do visit("/logs") click_link("reset your password") expect(page).to have_current_path("/account/password/new") end - it " is shown an error message if they submit without entering an email address" do + it "is shown an error message if they submit without entering an email address" do visit("/account/password/new") click_button("Send email") expect(page).to have_selector("#error-summary-title") @@ -70,7 +70,7 @@ RSpec.describe "User Features" do expect(page).to have_title("Error") end - it " is shown an error message if they submit an invalid email address" do + it "is shown an error message if they submit an invalid email address" do visit("/account/password/new") fill_in("user[email]", with: "thisisn'tanemail") click_button("Send email") @@ -79,28 +79,28 @@ RSpec.describe "User Features" do expect(page).to have_title("Error") end - it " is redirected to check your email page after submitting an email on the reset password page" do + it "is redirected to check your email page after submitting an email on the reset password page" do visit("/account/password/new") fill_in("user[email]", with: user.email) click_button("Send email") expect(page).to have_content("Check your email") end - it " is shown their email on the password reset confirmation page" do + it "is shown their email on the password reset confirmation page" do visit("/account/password/new") fill_in("user[email]", with: user.email) click_button("Send email") expect(page).to have_content(user.email) end - it " is shown the reset password confirmation page even if their email doesn't exist in the system" do + it "is shown the reset password confirmation page even if their email doesn't exist in the system" do visit("/account/password/new") fill_in("user[email]", with: "idontexist@example.com") click_button("Send email") expect(page).to have_current_path("/account/password/reset-confirmation?email=idontexist%40example.com") end - it " is sent a reset password email via Notify" do + it "is sent a reset password email via Notify" do expect(notify_client).to receive(:send_email).with( { email_address: user.email, @@ -324,4 +324,168 @@ RSpec.describe "User Features" do end end end + + context "when the user is a customer support person" do + 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) + visit("/logs") + fill_in("user[email]", with: support_user.email) + fill_in("user[password]", with: "pAssword1") + end + + context "when they are logging in" do + before do + allow(SecureRandom).to receive(:random_number).and_return(otp) + end + + 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 + + context "with a valid 2FA code" do + before do + allow(SecureRandom).to receive(:random_number).and_return(otp) + end + + it "authenticates successfully" do + click_button("Sign in") + fill_in("code", with: otp) + click_button("Submit") + expect(page).to have_content("Logs") + expect(page).to have_content("Two factor authentication successful.") + end + + context "but it is more than 15 minutes old" do + it "does not authenticate successfully" do + click_button("Sign in") + support_user.update!(direct_otp_sent_at: 16.minutes.ago) + fill_in("code", with: otp) + click_button("Submit") + expect(page).to have_content("Check your email") + expect(page).to have_http_status(:unprocessable_entity) + expect(page).to have_title("Error") + expect(page).to have_selector("#error-summary-title") + end + end + end + + context "with an invalid 2FA code" do + it "does not authenticate successfully" do + click_button("Sign in") + fill_in("code", with: otp) + click_button("Submit") + expect(page).to have_content("Check your email") + expect(page).to have_http_status(:unprocessable_entity) + expect(page).to have_title("Error") + expect(page).to have_selector("#error-summary-title") + end + end + + context "when the 2FA code needs to be resent" do + before do + click_button("Sign in") + end + + it "displays the resend view" do + click_link("Not received an email?") + expect(page).to have_button("Resend security code") + end + + it "send a new OTP code and redirects back to the 2FA view" do + click_link("Not received an email?") + expect { click_button("Resend security code") }.to(change { support_user.reload.direct_otp }) + expect(page).to have_current_path("/account/two-factor-authentication") + end + end + + context "when signing in and out again" do + before do + allow(SecureRandom).to receive(:random_number).and_return(otp) + end + + it "requires the 2FA code on each login" do + click_button("Sign in") + fill_in("code", with: otp) + click_button("Submit") + click_link("Sign out") + visit("/logs") + fill_in("user[email]", with: support_user.email) + fill_in("user[password]", with: "pAssword1") + click_button("Sign in") + expect(page).to have_content("Check your email") + end + end + + context "when they have forgotten their password" do + let(:reset_password_token) { "MCDH5y6Km-U7CFPgAMVS" } + + before do + allow(Devise.token_generator).to receive(:generate).and_return(reset_password_token) + 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 "is redirected to the reset password page when they click the reset password link" do + visit("/account/sign-in") + click_link("reset your password") + expect(page).to have_current_path("/account/password/new") + end + + it "is shown an error message if they submit without entering an email address" do + visit("/account/password/new") + click_button("Send email") + expect(page).to have_selector("#error-summary-title") + expect(page).to have_selector("#user-email-field-error") + expect(page).to have_title("Error") + end + + it "is redirected to login page after reset email is sent" do + visit("/account/password/new") + fill_in("user[email]", with: support_user.email) + click_button("Send email") + expect(page).to have_content("Check your email") + end + + it "is sent a reset password email via Notify" do + expect(notify_client).to receive(:send_email).with( + { + email_address: support_user.email, + template_id: support_user.reset_password_notify_template, + personalisation: { + name: support_user.name, + email: support_user.email, + organisation: support_user.organisation.name, + link: "http://localhost:3000/account/password/edit?reset_password_token=#{reset_password_token}", + }, + }, + ) + visit("/account/password/new") + fill_in("user[email]", with: support_user.email) + click_button("Send email") + end + end + end end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 99d27529a..e8d0008a9 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -64,6 +64,23 @@ RSpec.describe User, type: :model do expect { user.is_data_protection_officer! } .to change { user.reload.is_data_protection_officer? }.from(false).to(true) end + + it "does not require 2FA" do + expect(user.need_two_factor_authentication?(nil)).to be false + end + + context "when the user is a Customer Support person" do + let(:user) { FactoryBot.create(:user, :support) } + let!(:other_orgs_log) { FactoryBot.create(:case_log) } + + it "has access to logs from all organisations" do + expect(user.case_logs.to_a).to eq([owned_case_log, managed_case_log, other_orgs_log]) + end + + it "requires 2FA" do + expect(user.need_two_factor_authentication?(nil)).to be true + end + end end describe "paper trail" do diff --git a/spec/requests/auth/passwords_controller_spec.rb b/spec/requests/auth/passwords_controller_spec.rb index 0243f75e2..1ba617598 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,11 +137,83 @@ 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 end end + + context "when a customer support user" do + let(:support_user) { FactoryBot.create(:user, :support) } + + describe "reset password" do + let(:new_value) { "new-password" } + + before do + 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 + _raw, enc = Devise.token_generator.generate(User, :reset_password_token) + get "/account/password/edit?reset_password_token=#{enc}" + expect(page).to have_css("h1", text: "Reset your password") + end + + context "when passwords entered don't match" do + let(:raw) { support_user.send_reset_password_instructions } + let(:params) do + { + id: support_user.id, + user: { + password: new_value, + password_confirmation: "something_else", + reset_password_token: raw, + }, + } + end + + it "shows an error" do + put "/account/password", headers: headers, params: params + expect(response).to have_http_status(:unprocessable_entity) + expect(page).to have_content("doesn't match Password") + end + end + + context "when passwords is reset" do + let(:raw) { support_user.send_reset_password_instructions } + let(:params) do + { + id: support_user.id, + user: { + password: new_value, + password_confirmation: new_value, + reset_password_token: raw, + }, + } + end + + it "updates the password" do + expect { + put "/account/password", headers: headers, params: params + support_user.reload + }.to change(support_user, :encrypted_password) + end + + it "sends you to the 2FA page and does not allow bypassing 2FA code" do + put "/account/password", headers: headers, params: params + expect(response).to redirect_to("/account/two-factor-authentication") + get "/logs", headers: headers + expect(response).to redirect_to("/account/two-factor-authentication") + end + + it "triggers an email" do + expect(notify_client).to receive(:send_email) + put "/account/password", headers: headers, params: params + end + end + end + end end diff --git a/spec/requests/case_logs_controller_spec.rb b/spec/requests/case_logs_controller_spec.rb index 7adad985b..0526ee7ed 100644 --- a/spec/requests/case_logs_controller_spec.rb +++ b/spec/requests/case_logs_controller_spec.rb @@ -159,106 +159,129 @@ RSpec.describe CaseLogsController, type: :request do context "when displaying a collection of logs" do let(:headers) { { "Accept" => "text/html" } } - before do - sign_in user - end + context "when the user is a customer support user" do + let(:user) { FactoryBot.create(:user, :support) } - context "when there are less than 20 logs" do before do - get "/logs", headers: headers, params: {} + allow(user).to receive(:need_two_factor_authentication?).and_return(false) + sign_in user end - it "shows a table of logs" do - expect(CGI.unescape_html(response.body)).to match(/