From e196a560c9831971e1ef43daf8cc1fd32adeeaa8 Mon Sep 17 00:00:00 2001 From: Marius BALTEANU Date: Sun, 23 Jan 2022 23:28:05 +0200 Subject: [PATCH] Allow setting a grace periond before requiring two factor (#34070). --- app/controllers/account_controller.rb | 8 +++- app/models/setting.rb | 4 ++ app/models/user.rb | 10 ++++- app/views/settings/_authentication.html.erb | 23 ++++++++-- config/locales/en.yml | 4 ++ config/settings.yml | 4 ++ test/integration/twofa_test.rb | 49 +++++++++++++++++++++ test/unit/setting_test.rb | 9 ++++ test/unit/user_test.rb | 32 ++++++++++++++ 9 files changed, 138 insertions(+), 5 deletions(-) diff --git a/app/controllers/account_controller.rb b/app/controllers/account_controller.rb index 749fc8f64..d49f67608 100644 --- a/app/controllers/account_controller.rb +++ b/app/controllers/account_controller.rb @@ -336,7 +336,13 @@ class AccountController < ApplicationController set_autologin_cookie(user) end call_hook(:controller_account_success_authentication_after, {:user => user}) - redirect_back_or_default my_page_path + + if Setting.twofa_required? && !user.twofa_grace_period_expired? + flash[:warning] = l(:twofa_warning_require) + " " + l(:twofa_text_grace_period, :datetime => format_time(user.twofa_grace_period_expiration_date)) + redirect_to controller: 'twofa', action: 'select_scheme' + else + redirect_back_or_default my_page_path + end end def set_autologin_cookie(user) diff --git a/app/models/setting.rb b/app/models/setting.rb index f4bdbaadf..32555ecde 100644 --- a/app/models/setting.rb +++ b/app/models/setting.rb @@ -247,6 +247,10 @@ class Setting < ActiveRecord::Base twofa == '1' end + def self.twofa_grace_period_expiration_date + Setting.where(name: :twofa).pick(:updated_on) + Setting.twofa_grace_period.to_i * 3600 + end + # Helper that returns an array based on per_page_options setting def self.per_page_options_array per_page_options.split(%r{[\s,]}).collect(&:to_i).select {|n| n > 0}.sort diff --git a/app/models/user.rb b/app/models/user.rb index 08e949d93..16ebfdfbf 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -385,7 +385,7 @@ class User < Principal def must_activate_twofa? ( - Setting.twofa_required? || + (Setting.twofa_required? && self.twofa_grace_period_expired?) || ( Setting.twofa_optional? && ( groups.any?(&:twofa_required?) || @@ -395,6 +395,14 @@ class User < Principal ) && !twofa_active? end + def twofa_grace_period_expiration_date + convert_time_to_user_timezone(Setting.twofa_grace_period_expiration_date) + end + + def twofa_grace_period_expired? + Setting.twofa_required? && (twofa_grace_period_expiration_date < convert_time_to_user_timezone(DateTime.current)) + end + def pref self.preference ||= UserPreference.new(:user => self) end diff --git a/app/views/settings/_authentication.html.erb b/app/views/settings/_authentication.html.erb index 3d2a35135..14d5d171b 100644 --- a/app/views/settings/_authentication.html.erb +++ b/app/views/settings/_authentication.html.erb @@ -43,6 +43,17 @@ <%= l(:setting_twofa_required_for_administrators) %> + + "> + + + <%= t(:twofa_hint_grace_period_setting) -%> + +

@@ -64,12 +75,18 @@ <%= javascript_tag do %> $('#settings_twofa').on('change', function(e){ const twofa = e.target.value; - const parent_block = document.getElementById("twofa_optional"); + const optional = document.getElementById("twofa_optional"); + const required = document.getElementById("twofa_required"); if (twofa == "1") { - parent_block.classList.remove('hidden'); + optional.classList.remove('hidden'); + required.classList.add('hidden'); + } else if (twofa == "2") { + optional.classList.add('hidden'); + required.classList.remove('hidden'); } else { - parent_block.classList.add('hidden'); + optional.classList.add('hidden'); + required.classList.add('hidden'); } }); <% end %> diff --git a/config/locales/en.yml b/config/locales/en.yml index a925e76a9..a5691157c 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -509,6 +509,7 @@ en: setting_project_list_defaults: Projects list defaults setting_twofa: Two-factor authentication setting_twofa_required_for_administrators: Require two-factor authentication for administrators + setting_twofa_grace_period: Grace period permission_add_project: Create project permission_add_subprojects: Create subprojects @@ -1375,5 +1376,8 @@ en: twofa_backup_codes_already_shown: Backup codes cannot be shown again, please generate new backup codes if required. twofa_text_group_required: "This setting is only effective when the global two factor authentication setting is set to 'optional'. Currently, two factor authentication is required for all users." twofa_text_group_disabled: "This setting is only effective when the global two factor authentication setting is set to 'optional'. Currently, two factor authentication is disabled." + twofa_text_grace_period: "You need to do it before %{datetime}." + twofa_hint_grace_period_setting: Maximum time (in hours) that users are allowed to skip enablig two-factor authentication. Set to 0 (zero) to require at next sign in. + text_user_destroy_confirmation: "Are you sure you want to delete this user and remove all references to them? This cannot be undone. Often, locking a user instead of deleting them is the better solution. To confirm, please enter their login (%{login}) below." text_project_destroy_enter_identifier: "To confirm, please enter the project's identifier (%{identifier}) below." diff --git a/config/settings.yml b/config/settings.yml index c76509235..9aef941f0 100644 --- a/config/settings.yml +++ b/config/settings.yml @@ -40,6 +40,10 @@ twofa: twofa_required_for_administrators: default: 0 security_notifications: 1 +twofa_grace_period: + format: int + default: 0 + security_notifications: 1 unsubscribe: default: 1 password_required_char_classes: diff --git a/test/integration/twofa_test.rb b/test/integration/twofa_test.rb index fd284d3a6..07c243014 100644 --- a/test/integration/twofa_test.rb +++ b/test/integration/twofa_test.rb @@ -20,6 +20,7 @@ require File.expand_path('../../test_helper', __FILE__) class TwofaTest < Redmine::IntegrationTest + include Redmine::I18n fixtures :projects, :users, :email_addresses test "should require twofa setup when configured" do @@ -76,6 +77,54 @@ class TwofaTest < Redmine::IntegrationTest end end + test "should require twofa when grace period expired" do + user = User.find_by_login 'jsmith' + assert_not user.must_activate_twofa? + + with_settings twofa: "1", twofa_grace_period: "0" do + assert Setting.twofa_optional? + assert_not Setting.twofa_required? + assert_not user.must_activate_twofa? + end + + with_settings twofa: "2", twofa_grace_period: "1" do + # Override last updated on value + datetime = Time.gm(2021, 01, 20, 16, 30).utc # 2021-01-20 16:30 UTC + Setting.where(name: :twofa).first.update(updated_on: datetime) + + assert user.must_activate_twofa? + log_user('jsmith', 'jsmith') + follow_redirect! + assert_redirected_to "/my/twofa/totp/activate/confirm" + end + end + + test "should not require twofa when grace period is not expired" do + user = User.find_by_login 'jsmith' + assert_not user.must_activate_twofa? + + time = Time.current + with_settings twofa: "2", twofa_grace_period: "1" do + Setting.where(name: :twofa).first.update(updated_on: time) + assert_not user.must_activate_twofa? + log_user('jsmith', 'jsmith') + + # User should be redirected to two-factor authentication setup page + assert_response :redirect + assert_redirected_to "/my/twofa/select_scheme" + + follow_redirect! + # Assert flash message that informs the user about the grace period + assert_select "div.flash.warning", text: "The administrator requires you to enable two-factor authentication. You need to do it before #{format_time(time + 3600)}." + + # Assert user can leave the two-fatctor authentication setup page + get "/my/page" + assert_response :success + assert_select "h2", "My page" + end + end + + test 'should require to change password first when must_change_passwd is true' do User.find_by(login: 'jsmith').update_attribute(:must_change_passwd, true) with_settings twofa: '2' do diff --git a/test/unit/setting_test.rb b/test/unit/setting_test.rb index f8d35a6a6..ab5e6d42d 100644 --- a/test/unit/setting_test.rb +++ b/test/unit/setting_test.rb @@ -145,4 +145,13 @@ class SettingTest < ActiveSupport::TestCase end end end + + def test_twofa_grace_period_expiration_date_should_take_into_account_twofa_grace_period_in_hours + datetime = Time.gm(2021, 01, 20, 16, 30).utc # 2021-01-20 16:30 UTC + Setting.where(name: :twofa).first.update(updated_on: datetime) + + with_settings twofa_grace_period: "4" do + assert_equal datetime + (4 * 3600) , Setting.twofa_grace_period_expiration_date + end + end end diff --git a/test/unit/user_test.rb b/test/unit/user_test.rb index 6590ce2f7..36486482b 100644 --- a/test/unit/user_test.rb +++ b/test/unit/user_test.rb @@ -1348,4 +1348,36 @@ class UserTest < ActiveSupport::TestCase cv2a.reload assert_equal @dlopper.id.to_s, cv2a.value end + + def test_user_twofa_grace_period_expiration_date_should_return_the_expiration_datetime_according_to_user_time_zone + datetime = Time.gm(2021, 01, 20, 16, 30).utc # 2021-01-20 16:30 UTC + Setting.where(name: :twofa).first.update(updated_on: datetime) + + preference = User.find(1).pref + preference.update_attribute :time_zone, 'Bucharest' # UTC+2 + + with_settings twofa_grace_period: "4" do + # time + 4 hours (grace period) + 2 hours (time zone difference) + assert_equal Time.new(2021, 01, 20, 22, 30).to_s, User.find(1).twofa_grace_period_expiration_date.to_s + end + end + + def test_twofa_grace_period_expired_should_retun_true + datetime = Time.gm(2021, 01, 20, 16, 30).utc # 2021-01-20 16:30 UTC + Setting.where(name: :twofa).first.update(updated_on: datetime) + + with_settings twofa_grace_period: "0", twofa: "2" do + assert User.find(1).twofa_grace_period_expired? + end + end + + + def test_twofa_grace_period_expired_should_retun_false_if_twofa_is_not_required + datetime = Time.gm(2021, 01, 20, 16, 30).utc # 2021-01-20 16:30 UTC + Setting.where(name: :twofa).first.update(updated_on: datetime) + + with_settings twofa_grace_period: "0" do + assert_not User.find(1).twofa_grace_period_expired? + end + end end -- 2.32.0 (Apple Git-132)