Feature #35439

Option to require 2FA only for users with administration rights

Added by Marius BALTEANU over 1 year ago. Updated 8 months ago.

Status:ClosedStart date:
Priority:NormalDue date:
Assignee:Marius BALTEANU% Done:

0%

Category:Accounts / authentication
Target version:5.0.0
Resolution:Fixed

Description

#31920 adds the option to enable 2FA only for certain groups when the 2FA setting is set to optional. This is very useful, but it doesn't cover the case when you want to enable 2FA only for administrators. As a best security practice, if you cannot enforce for all users, the administrators should be top priority to secure using 2FA.

My proposal is to add a new setting to allow enforcing 2FA only for administrators:

What do you think?

2fa_optional.png (83.9 KB) Marius BALTEANU, 2021-06-22 23:21

0001-Option-to-require-2FA-authentication-only-for-users-.patch Magnifier (4.24 KB) Marius BALTEANU, 2022-01-24 00:19

0001-Option-to-require-2FA-authentication-only-for-users-.patch Magnifier (5.3 KB) Marius BALTEANU, 2022-01-27 21:54

0001-Option-to-require-2FA-authentication-only-for-users-.patch Magnifier (4.93 KB) Marius BALTEANU, 2022-01-30 11:10

options.png (88 KB) Marius BALTEANU, 2022-01-30 11:12

hints.png (79.7 KB) Marius BALTEANU, 2022-01-30 11:12


Related issues

Related to Redmine - Feature #1237: Add support for two-factor authentication Closed 2008-05-14
Related to Redmine - Feature #34070: Allow setting a grace period when forcing 2FA New
Related to Redmine - Feature #31920: Require 2FA only for certain user groups Closed

Associated revisions

Revision 21395
Added by Marius BALTEANU 8 months ago

Add "required for administrators" option to Two-factor authentication settings that behaves like optional, but will require all users with administration rights to set up two-factor authentication at their next login (#35439).

Revision 21396
Added by Marius BALTEANU 8 months ago

Add locales (#35439).

History

#1 Updated by Marius BALTEANU over 1 year ago

  • Related to Feature #1237: Add support for two-factor authentication added

#2 Updated by Bernhard Rohloff over 1 year ago

+1 I like the idea. It sounds very reasonable.
One thing I would like to mention is that it doesn't take much to remove the tick from the checkbox as an administrator without the other admins taking notice of the change.
Wouldn't it be better to have this setting in the configuration.yml to have more control on who can change it? Another option could be that every administrator gets notified if this option gets disabled.

#3 Updated by Marius BALTEANU over 1 year ago

Bernhard Rohloff wrote:

Wouldn't it be better to have this setting in the configuration.yml to have more control on who can change it? Another option could be that every administrator gets notified if this option gets disabled.

A notification sounds better to me.

#4 Updated by Alex JXXX 11 months ago

I think also it's the better way to add the option to force for admins.
If only one admin was set-up and he lost this phone. Any option exist to reset this OTP with SSH access ?

#5 Updated by Marius BALTEANU 8 months ago

  • Target version set to 5.0.0

#6 Updated by Marius BALTEANU 8 months ago

Here is a patch that I would like to commit in the following days.

#7 Updated by Marius BALTEANU 8 months ago

  • Related to Feature #34070: Allow setting a grace period when forcing 2FA added

#8 Updated by Holger Just 8 months ago

  • Related to Feature #31920: Require 2FA only for certain user groups added

#9 Updated by Holger Just 8 months ago

I think this general idea of enforcing 2FA for admins only, might a good idea to allow people to simplify this transition.

Right now, a workaround for that would be to create a group with all admin users and force 2fa for this group. With this setting, people would be relieved from having to maintain a separate group for that while still protecting the most important users.

However, I'm thinking whether it wouldn't be more sensible to introduce an additional possible value for Setting.twofa instead of this separate setting (and thus just an additional option in the dropdown)? This could look like this (as a rough sketch):

class Setting
  #...
  def self.twofa_required?
    twofa == '2'
  end

  def self.twofa_optional?
    %w[1 3].include? twofa
  end

  def self.twofa_required_for_administrators?
    twofa == '3'
  end
end

class User
  # ...

  def must_activate_twofa?
    return false if twofa_active?

    return true if Setting.twofa_required?
    return true if Setting.twofa_required_for_administrators? && admin?
    return true if Setting.twofa_optional? && groups.any?(&:twofa_required?)

    false
  end
end

What do you think?

#10 Updated by Marius BALTEANU 8 months ago

Thank you, Holger!

Holger Just wrote:

I think this general idea of enforcing 2FA for admins only, might a good idea to allow people to simplify this transition.

Right now, a workaround for that would be to create a group with all admin users and force 2fa for this group. With this setting, people would be relieved from having to maintain a separate group for that while still protecting the most important users.

I totally agree with you, this is way I created this ticket.

However, I'm thinking whether it wouldn't be more sensible to introduce an additional possible value for Setting.twofa instead of this separate setting (and thus just an additional option in the dropdown)? This could look like this (as a rough sketch):

[...]

What do you think?

Great ideea, thank you! I'm going to update the patch.

#11 Updated by Marius BALTEANU 8 months ago

Here is the updated version, all tests pass. Thanks again Holger for your feedback!

On a related topic, do you know why was added a new local (label_required_lower) instead of using downcase on the existing one (label_required)?

#12 Updated by Holger Just 8 months ago

Thank you Marius for taking care of this!

I think the new option should work kind of like "optional, but required for admins". The possibility of to also enforce 2FA for members of specific groups should still be possible with this setting.

Because of that, I proposed to return true in Setting.twofa_optional? above. If you don't want to do that, at least the view in app/views/groups/_form.html.erb would have to be adapted for the new option (or it may have to in any case, I not entirely sure now)

As for User#must_activate_twofa?, I personally like my proposed option better stylisticly because I think the rules are much easier to understand rather than having to parse nested boolean algebra. The result should be exactly the same. The original method was borderline okay with just a few rules. But now with the additional ones, I think we should refactor the method.

#13 Updated by Felix Schäfer 8 months ago

Marius BALTEANU wrote:

On a related topic, do you know why was added a new local (label_required_lower) instead of using downcase on the existing one (label_required)?

downcase might work somewhat OK for languages with latin scripts (and even then, in German nouns start with a capital no matter where they are in a sentence, which would not be a problem in this case but might be in others), but that's not something that will work reliably for arbitrary locales.

I agree that the choice of the name for the locale key is a little unfortunate, but it was chosen to resemble the _plural keys for example.

#14 Updated by Marius BALTEANU 8 months ago

Holger Just wrote:

Thank you Marius for taking care of this!

I think the new option should work kind of like "optional, but required for admins". The possibility of to also enforce 2FA for members of specific groups should still be possible with this setting.

I totally miss that from the updated version of the patch, thanks for pointing this out.

Because of that, I proposed to return true in Setting.twofa_optional? above. If you don't want to do that, at least the view in app/views/groups/_form.html.erb would have to be adapted for the new option (or it may have to in any case, I not entirely sure now)

First option sounds good to me as well.

As for User#must_activate_twofa?, I personally like my proposed option better stylisticly because I think the rules are much easier to understand rather than having to parse nested boolean algebra. The result should be exactly the same. The original method was borderline okay with just a few rules. But now with the additional ones, I think we should refactor the method.

I've already committed this change as part of #31920.

I'm attaching a new version which should work as expected. The patch adds the option "required for administrators" in the Two-factor authentication dropdown, between optional and required with the following hint: "Setting required for administrators behaves like optional, but will require all users with administration rights to set up two-factor authentication at their next login."

Some screenshots:

Are the translations clear enough? Or is it better "optional, but required for administrators"?

#15 Updated by Marius BALTEANU 8 months ago

Felix Schäfer wrote:

Marius BALTEANU wrote:

On a related topic, do you know why was added a new local (label_required_lower) instead of using downcase on the existing one (label_required)?

downcase might work somewhat OK for languages with latin scripts (and even then, in German nouns start with a capital no matter where they are in a sentence, which would not be a problem in this case but might be in others), but that's not something that will work reliably for arbitrary locales.

I agree that the choice of the name for the locale key is a little unfortunate, but it was chosen to resemble the _plural keys for example.

Thanks Felix for your response, it's clear now.

#16 Updated by Holger Just 8 months ago

Thank you for taking care of the changes!

Marius BALTEANU wrote:

Are the translations clear enough? Or is it better "optional, but required for administrators"?

I think the option name is clear along with the explanatory text below.

#17 Updated by Marius BALTEANU 8 months ago

  • Status changed from New to Closed
  • Resolution set to Fixed

Feature added in r21395.

#18 Updated by Marius BALTEANU 8 months ago

  • Status changed from Closed to Reopened

#19 Updated by Marius BALTEANU 8 months ago

  • Status changed from Reopened to Resolved

#20 Updated by Marius BALTEANU 8 months ago

  • Status changed from Resolved to Closed

Also available in: Atom PDF