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)