diff --git a/.env.example b/.env.example index c86fc0949..2163f2802 100644 --- a/.env.example +++ b/.env.example @@ -1,3 +1,5 @@ DB_USERNAME=postgres-user DB_PASSWORD=postgres-password -GOVUK_NOTIFY_API_KEY= + +GOVUK_NOTIFY_API_KEY= +OTP_SECRET_ENCRYPTION_KEY="" diff --git a/Gemfile b/Gemfile index ffb456be4..b96ea93e1 100644 --- a/Gemfile +++ b/Gemfile @@ -37,6 +37,9 @@ gem "json-schema" # Authentication # Point at branch until devise is compatible with Turbo, see https://github.com/heartcombo/devise/pull/5340 gem "devise", github: "baarkerlounger/devise", branch: "dluhc-fixes" +# Two-factor Authentication for devise models. Pointing at fork until this is merged for Rails 6 compatibility +# https://github.com/Houdini/two_factor_authentication/pull/204 +gem "two_factor_authentication", github: "baarkerlounger/two_factor_authentication" # UK postcode parsing and validation gem "uk_postcode" # Get rich data from postcode lookups. Wraps postcodes.io diff --git a/Gemfile.lock b/Gemfile.lock index 87a9f4a7d..b48605158 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -18,6 +18,17 @@ GIT responders warden (~> 1.2.3) +GIT + remote: https://github.com/baarkerlounger/two_factor_authentication.git + revision: a7522becd7222f1aa4ddf73d7caf19f05bdb4dac + specs: + two_factor_authentication (2.2.0) + devise + encryptor + rails (>= 3.1.1) + randexp + rotp (>= 4.0.0) + GIT remote: https://github.com/tagliala/activeadmin.git revision: d1492c54e76871d95f3a7ff20e445b48f455d4cb @@ -157,6 +168,7 @@ GEM dotenv-rails (2.7.6) dotenv (= 2.7.6) railties (>= 3.2) + encryptor (3.0.0) erubi (1.10.0) excon (0.90.0) factory_bot (6.2.0) @@ -310,6 +322,7 @@ GEM zeitwerk (~> 2.5) rainbow (3.1.1) rake (13.0.6) + randexp (0.1.7) ransack (2.5.0) activerecord (>= 5.2.4) activesupport (>= 5.2.4) @@ -325,6 +338,7 @@ GEM roo (2.8.3) nokogiri (~> 1) rubyzip (>= 1.3.0, < 3.0.0) + rotp (6.2.0) rspec-core (3.10.2) rspec-support (~> 3.10.0) rspec-expectations (3.10.2) @@ -472,6 +486,7 @@ DEPENDENCIES scss_lint-govuk selenium-webdriver simplecov + two_factor_authentication! tzinfo-data uk_postcode view_component diff --git a/app/admin/admin_users.rb b/app/admin/admin_users.rb index fd15aa33d..e161e0202 100644 --- a/app/admin/admin_users.rb +++ b/app/admin/admin_users.rb @@ -1,5 +1,5 @@ ActiveAdmin.register AdminUser do - permit_params :email, :password, :password_confirmation + permit_params :email, :phone, :password, :password_confirmation controller do def update_resource(object, attributes) @@ -12,6 +12,7 @@ ActiveAdmin.register AdminUser do selectable_column id_column column :email + column "Phone Number", :phone column :current_sign_in_at column :sign_in_count column :created_at @@ -19,6 +20,7 @@ ActiveAdmin.register AdminUser do end filter :email + filter :phone filter :current_sign_in_at filter :sign_in_count filter :created_at @@ -26,6 +28,7 @@ ActiveAdmin.register AdminUser do form do |f| f.inputs do f.input :email + f.input :phone f.input :password f.input :password_confirmation end diff --git a/app/controllers/auth/two_factor_authentication_controller.rb b/app/controllers/auth/two_factor_authentication_controller.rb new file mode 100644 index 000000000..d107ef15f --- /dev/null +++ b/app/controllers/auth/two_factor_authentication_controller.rb @@ -0,0 +1,5 @@ +class Auth::TwoFactorAuthenticationController < Devise::TwoFactorAuthenticationController + def show_resend + render "devise/two_factor_authentication/resend" + end +end diff --git a/app/models/admin_user.rb b/app/models/admin_user.rb index 14ea71789..7fc666a7e 100644 --- a/app/models/admin_user.rb +++ b/app/models/admin_user.rb @@ -1,5 +1,18 @@ class AdminUser < ApplicationRecord # Include default devise modules. Others available are: # :confirmable, :lockable, :timeoutable, :trackable and :omniauthable - devise :database_authenticatable, :recoverable, :rememberable, :validatable + devise :two_factor_authenticatable, :database_authenticatable, :recoverable, + :rememberable, :validatable + + has_one_time_password(encrypted: true) + + validates :phone, presence: true, numericality: true + + MFA_SMS_TEMPLATE_ID = "bf309d93-804e-4f95-b1f4-bd513c48ecb0".freeze + + def send_two_factor_authentication_code(code) + template_id = MFA_SMS_TEMPLATE_ID + personalisation = { otp: code } + Sms.send(phone, template_id, personalisation) + end end diff --git a/app/services/sms.rb b/app/services/sms.rb new file mode 100644 index 000000000..c5a4f8ffd --- /dev/null +++ b/app/services/sms.rb @@ -0,0 +1,15 @@ +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: phone_number, + template_id: template_id, + personalisation: args, + ) + end +end diff --git a/app/views/devise/two_factor_authentication/resend.html.erb b/app/views/devise/two_factor_authentication/resend.html.erb new file mode 100644 index 000000000..38f1ec222 --- /dev/null +++ b/app/views/devise/two_factor_authentication/resend.html.erb @@ -0,0 +1,23 @@ +<% content_for :title, "Resend security code" %> + +<% content_for :before_content do %> + <%= govuk_back_link( + text: 'Back', + href: 'javascript:history.back()', + ) %> +<% end %> + +<%= form_with(url: resend_code_admin_user_two_factor_authentication_path, html: { method: :get }) do |f| %> +
+
+ +

+ <%= content_for(:title) %> +

+ +

Text messages sometimes take a few minutes to arrive. If you do not receive the text message, you can request a new one.

+ + <%= f.govuk_submit "Resend security code" %> +
+
+<% end %> diff --git a/app/views/devise/two_factor_authentication/show.html.erb b/app/views/devise/two_factor_authentication/show.html.erb new file mode 100644 index 000000000..a742056fe --- /dev/null +++ b/app/views/devise/two_factor_authentication/show.html.erb @@ -0,0 +1,27 @@ +<% content_for :title, "Check your phone" %> + +<%= form_with(url: "/admin/two-factor-authentication", html: { method: :put }) do |f| %> +
+
+ +

+ <%= content_for(:title) %> +

+ +

We’ve sent you a text message with a security code.

+ + <%= f.govuk_number_field :code, + label: { text: "Security code" }, + width: 5, + autocomplete: 'one-time-code', + autofocus: true + %> + + <%= f.govuk_submit "Submit" %> +
+
+<% end %> + +

+ <%= govuk_link_to "Not received a text message?", admin_two_factor_authentication_resend_path %> +

diff --git a/config/initializers/devise.rb b/config/initializers/devise.rb index d5196b163..0d9dcbaf7 100644 --- a/config/initializers/devise.rb +++ b/config/initializers/devise.rb @@ -309,4 +309,15 @@ Devise.setup do |config| # When set to false, does not sign a user in automatically after their password is # changed. Defaults to true, so a user is signed in automatically after changing a password. # config.sign_in_after_change_password = true + + # 2FA + config.max_login_attempts = 3 # Maximum second factor attempts count. + config.allowed_otp_drift_seconds = 30 # Allowed TOTP time drift between client and server. + config.otp_length = 6 # TOTP code length + config.direct_otp_valid_for = 5.minutes # Time before direct OTP becomes invalid + config.direct_otp_length = 6 # Direct OTP code length + config.remember_otp_session_for_seconds = 1.day # Time before browser has to perform 2fA again. Default is 0. + config.otp_secret_encryption_key = ENV["OTP_SECRET_ENCRYPTION_KEY"] + config.second_factor_resource_id = "id" # Field or method name used to set value for 2fA remember cookie + config.delete_cookie_on_logout = false # Delete cookie when user signs out, to force 2fA again on login end diff --git a/config/locales/en.yml b/config/locales/en.yml index b8fe96c35..9b0789828 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -122,3 +122,10 @@ en: message: "Net income is lower than expected based on the main tenant’s working situation. Are you sure this is correct?" in_soft_max_range: message: "Net income is higher than expected based on the main tenant’s working situation. Are you sure this is correct?" + devise: + two_factor_authentication: + success: "Two factor authentication successful." + attempt_failed: "Attempt failed." + max_login_attempts_reached: "Access completely denied as you have reached your attempts limit" + contact_administrator: "Please contact your system administrator." + code_has_been_sent: "Your security code has been sent." diff --git a/config/routes.rb b/config/routes.rb index 40ad98333..02b3a1e40 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -1,5 +1,22 @@ Rails.application.routes.draw do - devise_for :admin_users, ActiveAdmin::Devise.config + devise_for :admin_users, { + path: :admin, + controllers: { + sessions: "active_admin/devise/sessions", + passwords: "active_admin/devise/passwords", + unlocks: "active_admin/devise/unlocks", + registrations: "active_admin/devise/registrations", + confirmations: "active_admin/devise/confirmations", + two_factor_authentication: "auth/two_factor_authentication", + }, + path_names: { sign_in: "login", sign_out: "logout", two_factor_authentication: "two-factor-authentication" }, + sign_out_via: %i[delete get], + } + + devise_scope :admin_user do + get "admin/two-factor-authentication/resend", to: "auth/two_factor_authentication#show_resend" + end + devise_for :users, controllers: { passwords: "auth/passwords", sessions: "auth/sessions", diff --git a/db/migrate/20211203135623_two_factor_authentication_add_to_admin_users.rb b/db/migrate/20211203135623_two_factor_authentication_add_to_admin_users.rb new file mode 100644 index 000000000..556023f3c --- /dev/null +++ b/db/migrate/20211203135623_two_factor_authentication_add_to_admin_users.rb @@ -0,0 +1,16 @@ +class TwoFactorAuthenticationAddToAdminUsers < ActiveRecord::Migration[6.1] + def change + change_table :admin_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.column :phone, :string + + t.index :encrypted_otp_secret_key, unique: true + end + end +end diff --git a/db/schema.rb b/db/schema.rb index a56d45399..d93c8efed 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -23,6 +23,15 @@ ActiveRecord::Schema.define(version: 2022_01_31_123638) do t.datetime "remember_created_at" t.datetime "created_at", precision: 6, null: false t.datetime "updated_at", precision: 6, null: false + 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" + t.datetime "totp_timestamp" + t.string "phone" + t.index ["encrypted_otp_secret_key"], name: "index_admin_users_on_encrypted_otp_secret_key", unique: true end create_table "case_logs", force: :cascade do |t| diff --git a/db/seeds.rb b/db/seeds.rb index a59808694..14b5ea456 100644 --- a/db/seeds.rb +++ b/db/seeds.rb @@ -30,4 +30,4 @@ User.create!( role: "data_coordinator", ) -AdminUser.create!(email: "admin@example.com", password: "password") +AdminUser.create!(email: "admin@example.com", password: "password", phone: "000000000") diff --git a/spec/controllers/admin/admin_users_controller_spec.rb b/spec/controllers/admin/admin_users_controller_spec.rb index 17eb7fd6e..e5977d830 100644 --- a/spec/controllers/admin/admin_users_controller_spec.rb +++ b/spec/controllers/admin/admin_users_controller_spec.rb @@ -22,7 +22,7 @@ describe Admin::AdminUsersController, type: :controller do end describe "Create admin users" do - let(:params) { { admin_user: { email: "test2@example.com", password: "pAssword1" } } } + let(:params) { { admin_user: { email: "test2@example.com", password: "pAssword1", phone: "07566126368" } } } it "creates a new admin user" do expect { post :create, session: valid_session, params: params }.to change(AdminUser, :count).by(1) diff --git a/spec/factories/admin_user.rb b/spec/factories/admin_user.rb index 29a5b079b..74f9a96b5 100644 --- a/spec/factories/admin_user.rb +++ b/spec/factories/admin_user.rb @@ -2,6 +2,7 @@ FactoryBot.define do factory :admin_user do sequence(:email) { |i| "admin#{i}@example.com" } password { "pAssword1" } + phone { "07563867654" } 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 new file mode 100644 index 000000000..45998ac0c --- /dev/null +++ b/spec/features/admin_panel_spec.rb @@ -0,0 +1,75 @@ +require "rails_helper" + +RSpec.describe "Admin Panel" do + let!(:admin) { FactoryBot.create(:admin_user) } + let(:notify_client) { instance_double(Notifications::Client) } + let(:mfa_template_id) { AdminUser::MFA_SMS_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) + end + + context "with a valid 2FA code" do + before do + allow(SecureRandom).to receive(:random_number).and_return(otp) + visit("/admin") + fill_in("admin_user[email]", with: admin.email) + fill_in("admin_user[password]", with: admin.password) + end + + it "authenticates successfully" do + expect(notify_client).to receive(:send_sms).with( + hash_including(phone_number: admin.phone, template_id: mfa_template_id), + ) + click_button("Login") + fill_in("code", with: otp) + click_button("Submit") + expect(page).to have_content("Dashboard") + expect(page).to have_content("Two factor authentication successful.") + end + + context "but it is more than 5 minutes old" do + it "does not authenticate successfully" do + click_button("Login") + admin.update!(direct_otp_sent_at: 10.minutes.ago) + fill_in("code", with: otp) + click_button("Submit") + expect(page).to have_content("Check your phone") + end + end + end + + context "with an invalid 2FA code" do + it "does not authenticate successfully" do + visit("/admin") + fill_in("admin_user[email]", with: admin.email) + fill_in("admin_user[password]", with: admin.password) + click_button("Login") + fill_in("code", with: otp) + click_button("Submit") + expect(page).to have_content("Check your phone") + end + end + + context "when the 2FA code needs to be resent" do + before do + visit("/admin") + fill_in("admin_user[email]", with: admin.email) + fill_in("admin_user[password]", with: admin.password) + click_button("Login") + end + + it "displays the resend view" do + click_link("Not received a text message?") + 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?") + expect { click_button("Resend security code") }.to(change { admin.reload.direct_otp }) + expect(page).to have_current_path("/admin/two-factor-authentication") + end + end +end diff --git a/spec/models/admin_user_spec.rb b/spec/models/admin_user_spec.rb new file mode 100644 index 000000000..a112bfb5b --- /dev/null +++ b/spec/models/admin_user_spec.rb @@ -0,0 +1,52 @@ +require "rails_helper" + +RSpec.describe AdminUser, type: :model do + describe "#new" do + it "requires a phone number" do + expect { + described_class.create!( + email: "admin_test@example.com", + password: "password123", + ) + }.to raise_error(ActiveRecord::RecordInvalid) + end + + it "requires a numerical phone number" do + expect { + described_class.create!( + email: "admin_test@example.com", + password: "password123", + phone: "string", + ) + }.to raise_error(ActiveRecord::RecordInvalid) + end + end + + it "requires an email" do + expect { + described_class.create!( + password: "password123", + phone: "075752137", + ) + }.to raise_error(ActiveRecord::RecordInvalid) + end + + it "requires a password" do + expect { + described_class.create!( + email: "admin_test@example.com", + phone: "075752137", + ) + }.to raise_error(ActiveRecord::RecordInvalid) + end + + it "can be created" do + expect { + described_class.create!( + email: "admin_test@example.com", + password: "password123", + phone: "075752137", + ) + }.to change(described_class, :count).by(1) + end +end