From 6fc213842151b495aa3467025b86deda5f213f9f Mon Sep 17 00:00:00 2001 From: baarkerlounger Date: Thu, 3 Feb 2022 15:39:27 +0000 Subject: [PATCH 1/6] 2FA code is valid for 15 minutes --- config/initializers/devise.rb | 1 + spec/features/admin_panel_spec.rb | 4 ++-- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/config/initializers/devise.rb b/config/initializers/devise.rb index 0d9dcbaf7..d03f9692b 100644 --- a/config/initializers/devise.rb +++ b/config/initializers/devise.rb @@ -316,6 +316,7 @@ Devise.setup do |config| 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.direct_otp_valid_for = 15.minutes # Time before direct OTP becomes invalid 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 diff --git a/spec/features/admin_panel_spec.rb b/spec/features/admin_panel_spec.rb index 45998ac0c..d5732ca6f 100644 --- a/spec/features/admin_panel_spec.rb +++ b/spec/features/admin_panel_spec.rb @@ -30,10 +30,10 @@ 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") From 731f689cf07c7744365bdd6daf79ab473af8af6c Mon Sep 17 00:00:00 2001 From: baarkerlounger Date: Thu, 3 Feb 2022 15:39:55 +0000 Subject: [PATCH 2/6] 2FA code required on every sign in --- config/initializers/devise.rb | 2 +- spec/features/admin_panel_spec.rb | 20 ++++++++++++++++++++ 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/config/initializers/devise.rb b/config/initializers/devise.rb index d03f9692b..7b48556a7 100644 --- a/config/initializers/devise.rb +++ b/config/initializers/devise.rb @@ -320,5 +320,5 @@ Devise.setup do |config| 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 diff --git a/spec/features/admin_panel_spec.rb b/spec/features/admin_panel_spec.rb index d5732ca6f..70226286a 100644 --- a/spec/features/admin_panel_spec.rb +++ b/spec/features/admin_panel_spec.rb @@ -72,4 +72,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 From eceba7a9113639e7d274af55f8adacca0779bcf7 Mon Sep 17 00:00:00 2001 From: baarkerlounger Date: Thu, 3 Feb 2022 15:40:21 +0000 Subject: [PATCH 3/6] OTP code 5 digits consistent with notify gateway --- config/initializers/devise.rb | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/config/initializers/devise.rb b/config/initializers/devise.rb index 7b48556a7..de41a7b75 100644 --- a/config/initializers/devise.rb +++ b/config/initializers/devise.rb @@ -313,10 +313,9 @@ 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 From cf2ac1c964e3bda6c4905f40436ca20368f097d4 Mon Sep 17 00:00:00 2001 From: baarkerlounger Date: Thu, 3 Feb 2022 17:53:10 +0000 Subject: [PATCH 4/6] Return 422 if OTP auth is unsuccessful --- Gemfile.lock | 4 ++-- app/views/devise/two_factor_authentication/show.html.erb | 8 ++++---- spec/features/admin_panel_spec.rb | 2 ++ 3 files changed, 8 insertions(+), 6 deletions(-) diff --git a/Gemfile.lock b/Gemfile.lock index b48605158..51a6d8e23 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -20,7 +20,7 @@ GIT GIT remote: https://github.com/baarkerlounger/two_factor_authentication.git - revision: a7522becd7222f1aa4ddf73d7caf19f05bdb4dac + revision: 025d0a39ae971798402a7cefbd677cb15aa4983c specs: two_factor_authentication (2.2.0) devise @@ -117,7 +117,7 @@ GEM ast (2.4.2) aws-eventstream (1.2.0) aws-partitions (1.551.0) - aws-sdk-core (3.125.5) + aws-sdk-core (3.125.6) aws-eventstream (~> 1, >= 1.0.2) aws-partitions (~> 1, >= 1.525.0) aws-sigv4 (~> 1.1) diff --git a/app/views/devise/two_factor_authentication/show.html.erb b/app/views/devise/two_factor_authentication/show.html.erb index a742056fe..cc4738e12 100644 --- a/app/views/devise/two_factor_authentication/show.html.erb +++ b/app/views/devise/two_factor_authentication/show.html.erb @@ -11,10 +11,10 @@

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 + label: { text: "Security code" }, + width: 5, + autocomplete: 'one-time-code', + autofocus: true %> <%= f.govuk_submit "Submit" %> diff --git a/spec/features/admin_panel_spec.rb b/spec/features/admin_panel_spec.rb index 70226286a..838bc1b1e 100644 --- a/spec/features/admin_panel_spec.rb +++ b/spec/features/admin_panel_spec.rb @@ -37,6 +37,7 @@ 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) end end end @@ -50,6 +51,7 @@ 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) end end From 07adb67e22b06ea01fede4aa634d43ce80108e76 Mon Sep 17 00:00:00 2001 From: baarkerlounger Date: Thu, 3 Feb 2022 18:24:41 +0000 Subject: [PATCH 5/6] Add gov uk error summary if OTP code is invalid --- Gemfile.lock | 2 +- app/views/devise/two_factor_authentication/show.html.erb | 3 ++- app/views/layouts/application.html.erb | 2 +- spec/features/admin_panel_spec.rb | 4 ++++ 4 files changed, 8 insertions(+), 3 deletions(-) diff --git a/Gemfile.lock b/Gemfile.lock index 51a6d8e23..f34c578ab 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -20,7 +20,7 @@ GIT GIT remote: https://github.com/baarkerlounger/two_factor_authentication.git - revision: 025d0a39ae971798402a7cefbd677cb15aa4983c + revision: 276a896d4f8f4186b199588f8b8900abb8b24a9a specs: two_factor_authentication (2.2.0) devise diff --git a/app/views/devise/two_factor_authentication/show.html.erb b/app/views/devise/two_factor_authentication/show.html.erb index cc4738e12..1eea10171 100644 --- 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| %>
+ <%= f.govuk_error_summary %>

<%= content_for(:title) %> diff --git a/app/views/layouts/application.html.erb b/app/views/layouts/application.html.erb index 4055cda6c..5b2994020 100644 --- a/app/views/layouts/application.html.erb +++ b/app/views/layouts/application.html.erb @@ -1,7 +1,7 @@ - <%= browser_title(yield(:title), @user, @organisation, @case_log, @resource) %> + <%= browser_title(yield(:title), @admin_user, @user, @organisation, @case_log, @resource) %> <%= csrf_meta_tags %> <%= csp_meta_tag %> <%= tag :meta, name: 'viewport', content: 'width=device-width, initial-scale=1' %> diff --git a/spec/features/admin_panel_spec.rb b/spec/features/admin_panel_spec.rb index 838bc1b1e..93a0e0a1f 100644 --- a/spec/features/admin_panel_spec.rb +++ b/spec/features/admin_panel_spec.rb @@ -38,6 +38,8 @@ RSpec.describe "Admin Panel" do 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 @@ -52,6 +54,8 @@ RSpec.describe "Admin Panel" do 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 From 9ceade98333518d7e43077b172d2b2e067440529 Mon Sep 17 00:00:00 2001 From: baarkerlounger Date: Fri, 4 Feb 2022 10:26:47 +0000 Subject: [PATCH 6/6] Less patching upstream --- Gemfile.lock | 16 +++++++-------- .../two_factor_authentication_controller.rb | 20 +++++++++++++++++++ 2 files changed, 28 insertions(+), 8 deletions(-) diff --git a/Gemfile.lock b/Gemfile.lock index f34c578ab..b985f123e 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -20,7 +20,7 @@ GIT GIT remote: https://github.com/baarkerlounger/two_factor_authentication.git - revision: 276a896d4f8f4186b199588f8b8900abb8b24a9a + 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.6) + 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) diff --git a/app/controllers/auth/two_factor_authentication_controller.rb b/app/controllers/auth/two_factor_authentication_controller.rb index d107ef15f..7d820f9af 100644 --- 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