When 2FA is set to optional, allow to require 2FA only for certain user groups (#31920).

Patch by Jens Krämer.

git-svn-id: http://svn.redmine.org/redmine/trunk@21060 e93f8b46-1217-0410-a6f0-8f06a7374b81

8 files changed, 58 insertions, 2 deletions

diff --git a/app/models/group.rb b/app/models/group.rb
index a84f8650f..a1fd35602 100644
--- a/app/models/group.rb
+++ b/app/models/group.rb
@@ -41,6 +41,7 @@ class Group < Principal
 
   safe_attributes(
     'name',
+    'twofa_required',
     'user_ids',
     'custom_field_values',
     'custom_fields',
diff --git a/app/models/setting.rb b/app/models/setting.rb
index e5c547665..dfa054028 100644
--- a/app/models/setting.rb
+++ b/app/models/setting.rb
@@ -236,6 +236,14 @@ class Setting < ActiveRecord::Base
     params
   end
 
+  def self.twofa_required?
+    twofa == '2'
+  end
+
+  def self.twofa_optional?
+    twofa == '1'
+  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 cc1841a61..2591f56be 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -407,7 +407,10 @@ class User < Principal
   end
 
   def must_activate_twofa?
-    Setting.twofa == '2' && !twofa_active?
+    (
+      Setting.twofa_required? ||
+      (Setting.twofa_optional? && groups.any?(&:twofa_required?))
+    ) && !twofa_active?
   end
 
   def pref
diff --git a/app/views/groups/_form.html.erb b/app/views/groups/_form.html.erb
index 9d5b087e1..723b9f203 100644
--- a/app/views/groups/_form.html.erb
+++ b/app/views/groups/_form.html.erb
@@ -3,6 +3,15 @@
 <div class="box tabular">
   <p><%= f.text_field :name, :required => true, :size => 60,
            :disabled => !@group.safe_attribute?('name')  %></p>
+  <% unless @group.builtin? %>
+    <p><%= f.check_box :twofa_required, disabled: !Setting.twofa_optional? %>
+      <% if Setting.twofa_required? %>
+        <em class="info"><%= l 'twofa_text_group_required' %></em>
+      <% elsif !Setting.twofa_optional? %>
+        <em class="info"><%= l 'twofa_text_group_disabled' %></em>
+      <% end %>
+    </p>
+  <% end %>
 
   <% @group.custom_field_values.each do |value| %>
     <p><%= custom_field_tag_with_label :group, value %></p>
diff --git a/app/views/settings/_authentication.html.erb b/app/views/settings/_authentication.html.erb
index 5522ff5cf..9fd0ef646 100644
--- a/app/views/settings/_authentication.html.erb
+++ b/app/views/settings/_authentication.html.erb
@@ -34,6 +34,7 @@
                               [l(:label_required_lower), "2"]] -%>
   <em class="info">
     <%= t 'twofa_hint_disabled_html', label: t(:label_disabled) -%><br/>
+    <%= t 'twofa_hint_optional_html', label: t(:label_optional) -%><br/>
     <%= t 'twofa_hint_required_html', label: t(:label_required_lower) -%>
   </em>
 </p>
diff --git a/config/locales/en.yml b/config/locales/en.yml
index 0aeafe516..488a9b4ee 100644
--- a/config/locales/en.yml
+++ b/config/locales/en.yml
@@ -408,6 +408,7 @@ en:
   field_history_default_tab: Issue's history default tab
   field_unique_id: Unique ID
   field_toolbar_language_options: Code highlighting toolbar languages
+  field_twofa_required: Require two factor authentication
 
   setting_app_title: Application title
   setting_welcome_text: Welcome text
@@ -1335,6 +1336,7 @@ en:
   twofa_not_active: "Not activated"
   twofa_label_code: Code
   twofa_hint_disabled_html: Setting <strong>%{label}</strong> will deactivate and unpair two-factor authentication devices for all users.
+  twofa_hint_optional_html: Setting <strong>%{label}</strong> will let users set up two-factor authentication at will, unless it is required by one of their groups.
   twofa_hint_required_html: Setting <strong>%{label}</strong> will require all users to set up two-factor authentication at their next login.
   twofa_label_setup: Enable two-factor authentication
   twofa_label_deactivation_confirmation: Disable two-factor authentication
@@ -1359,6 +1361,7 @@ en:
   twofa_text_backup_codes_hint: Use these codes instead of a one-time password should you not have access to your second factor. Each code can only be used once. It is recommended to print and store them in a safe place.
   twofa_text_backup_codes_created_at: Backup codes generated %{datetime}.
   twofa_backup_codes_already_shown: Backup codes cannot be shown again, please <a data-method="post" href="%{bc_path}">generate new backup codes</a> 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."
   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/db/migrate/20210704125704_add_twofa_required_to_groups.rb b/db/migrate/20210704125704_add_twofa_required_to_groups.rb
new file mode 100644
index 000000000..91361bc46
--- /dev/null
+++ b/db/migrate/20210704125704_add_twofa_required_to_groups.rb
@@ -0,0 +1,5 @@
+class AddTwofaRequiredToGroups < ActiveRecord::Migration[6.1]
+  def change
+    add_column :users, :twofa_required, :boolean, default: false
+  end
+end
diff --git a/test/integration/twofa_test.rb b/test/integration/twofa_test.rb
index a787e2770..a25fa2895 100644
--- a/test/integration/twofa_test.rb
+++ b/test/integration/twofa_test.rb
@@ -24,6 +24,32 @@ class TwofaTest < Redmine::IntegrationTest
 
   test "should require twofa setup when configured" do
     with_settings twofa: "2" do
+      assert Setting.twofa_required?
+      log_user('jsmith', 'jsmith')
+      follow_redirect!
+      assert_redirected_to "/my/twofa/totp/activate/confirm"
+    end
+  end
+
+  test "should require twofa setup when required by group" do
+    user = User.find_by_login 'jsmith'
+    assert_not user.must_activate_twofa?
+
+    group = Group.all.first
+    group.update_column :twofa_required, true
+    group.users << user
+    user.reload
+
+    with_settings twofa: "0" do
+      assert_not Setting.twofa_optional?
+      assert_not Setting.twofa_required?
+      assert_not user.must_activate_twofa?
+    end
+
+    with_settings twofa: "1" do
+      assert Setting.twofa_optional?
+      assert_not Setting.twofa_required?
+      assert user.must_activate_twofa?
       log_user('jsmith', 'jsmith')
       follow_redirect!
       assert_redirected_to "/my/twofa/totp/activate/confirm"