Merge pull request #270 from communitiesuk/2fa_fixes

2fa fixes
3 years ago · 1fccaa77e2
6 changed files with 67 additions and 20 deletions
--- a/Gemfile.lock
+++ b/Gemfile.lock
@ -20,7 +20,7 @@ GIT

 GIT
  remote: https://github.com/baarkerlounger/two_factor_authentication.git
-  revision: a7522becd7222f1aa4ddf73d7caf19f05bdb4dac
+  revision: 1fa214d18d311e019a343f836f2c591c0fa3d308
  specs:
    two_factor_authentication (2.2.0)
      devise
@ -116,17 +116,17 @@ GEM
      public_suffix (>= 2.0.2, < 5.0)
    ast (2.4.2)
    aws-eventstream (1.2.0)
-    aws-partitions (1.551.0)
-    aws-sdk-core (3.125.5)
+    aws-partitions (1.552.0)
+    aws-sdk-core (3.126.0)
      aws-eventstream (~> 1, >= 1.0.2)
      aws-partitions (~> 1, >= 1.525.0)
      aws-sigv4 (~> 1.1)
      jmespath (~> 1.0)
-    aws-sdk-kms (1.53.0)
-      aws-sdk-core (~> 3, >= 3.125.0)
+    aws-sdk-kms (1.54.0)
+      aws-sdk-core (~> 3, >= 3.126.0)
      aws-sigv4 (~> 1.1)
-    aws-sdk-s3 (1.111.3)
-      aws-sdk-core (~> 3, >= 3.125.0)
+    aws-sdk-s3 (1.112.0)
+      aws-sdk-core (~> 3, >= 3.126.0)
      aws-sdk-kms (~> 1)
      aws-sigv4 (~> 1.4)
    aws-sigv4 (1.4.0)
@ -170,7 +170,7 @@ GEM
      railties (>= 3.2)
    encryptor (3.0.0)
    erubi (1.10.0)
-    excon (0.90.0)
+    excon (0.91.0)
    factory_bot (6.2.0)
      activesupport (>= 5.0.0)
    factory_bot_rails (6.2.0)
--- a/app/controllers/auth/two_factor_authentication_controller.rb
+++ b/app/controllers/auth/two_factor_authentication_controller.rb
@ -2,4 +2,24 @@ class Auth::TwoFactorAuthenticationController < Devise::TwoFactorAuthenticationC
  def show_resend
    render "devise/two_factor_authentication/resend"
  end
+
+  def update
+    resource.errors.add :base, I18n.t("devise.two_factor_authentication.code_required") if resource && params_code.empty?
+    super
+  end
+
+private
+
+  def after_two_factor_fail_for(resource)
+    resource.second_factor_attempts_count += 1
+    resource.save!
+
+    if resource.max_login_attempts?
+      sign_out(resource)
+      render :max_login_attempts_reached, status: :unprocessable_entity
+    else
+      resource.errors.add :base, I18n.t("devise.two_factor_authentication.code_incorrect") if resource
+      render :show, status: :unprocessable_entity
+    end
+  end
 end
--- a/app/views/devise/two_factor_authentication/show.html.erb
+++ b/app/views/devise/two_factor_authentication/show.html.erb
@ -1,8 +1,9 @@
 <% content_for :title, "Check your phone" %>

-<%= form_with(url: "/admin/two-factor-authentication", html: { method: :put }) do |f| %>
+<%= form_with(model: resource, url: "/admin/two-factor-authentication", html: { method: :put }) do |f| %>
  <div class="govuk-grid-row">
    <div class="govuk-grid-column-two-thirds">
+      <%= f.govuk_error_summary %>

      <h1 class="govuk-heading-l">
        <%= content_for(:title) %>
@ -11,10 +12,10 @@
      <p class="govuk-body">We’ve sent you a text message with a security code.</p>

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

      <%= f.govuk_submit "Submit" %>
--- a/app/views/layouts/application.html.erb
+++ b/app/views/layouts/application.html.erb
@ -1,7 +1,7 @@
 <!DOCTYPE html>
 <html lang="en" class="govuk-template">
  <head>
-    <title><%= browser_title(yield(:title), @user, @organisation, @case_log, @resource) %></title>
+    <title><%= browser_title(yield(:title), @admin_user, @user, @organisation, @case_log, @resource) %></title>
    <%= csrf_meta_tags %>
    <%= csp_meta_tag %>
    <%= tag :meta, name: 'viewport', content: 'width=device-width, initial-scale=1' %>
--- a/config/initializers/devise.rb
+++ b/config/initializers/devise.rb
@ -313,11 +313,11 @@ Devise.setup do |config|
  # 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.otp_length = 5 # TOTP code length
+  config.direct_otp_valid_for = 15.minutes # Time before direct OTP becomes invalid
+  config.direct_otp_length = 5 # 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
+  config.delete_cookie_on_logout = true # Delete cookie when user signs out, to force 2fA again on login
 end
--- a/spec/features/admin_panel_spec.rb
+++ b/spec/features/admin_panel_spec.rb
@ -30,13 +30,16 @@ RSpec.describe "Admin Panel" do
      expect(page).to have_content("Two factor authentication successful.")
    end

-    context "but it is more than 5 minutes old" do
+    context "but it is more than 15 minutes old" do
      it "does not authenticate successfully" do
        click_button("Login")
-        admin.update!(direct_otp_sent_at: 10.minutes.ago)
+        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_http_status(:unprocessable_entity)
+        expect(page).to have_title("Error")
+        expect(page).to have_selector("#error-summary-title")
      end
    end
  end
@ -50,6 +53,9 @@ RSpec.describe "Admin Panel" do
      fill_in("code", with: otp)
      click_button("Submit")
      expect(page).to have_content("Check your phone")
+      expect(page).to have_http_status(:unprocessable_entity)
+      expect(page).to have_title("Error")
+      expect(page).to have_selector("#error-summary-title")
    end
  end

@ -72,4 +78,24 @@ RSpec.describe "Admin Panel" do
      expect(page).to have_current_path("/admin/two-factor-authentication")
    end
  end
+
+  context "when logging out and in again" do
+    before do
+      allow(SecureRandom).to receive(:random_number).and_return(otp)
+    end
+
+    it "requires the 2FA code on each login" 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")
+      click_link("Logout")
+      fill_in("admin_user[email]", with: admin.email)
+      fill_in("admin_user[password]", with: admin.password)
+      click_button("Login")
+      expect(page).to have_content("Check your phone")
+    end
+  end
 end