Feature #35439
closedOption to require 2FA only for users with administration rights
Added by Marius BĂLTEANU about 3 years ago. Updated over 2 years ago.
0%
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?
Files
2fa_optional.png (83.9 KB) 2fa_optional.png | Marius BĂLTEANU, 2021-06-22 23:21 | ||
0001-Option-to-require-2FA-authentication-only-for-users-.patch (4.24 KB) 0001-Option-to-require-2FA-authentication-only-for-users-.patch | Marius BĂLTEANU, 2022-01-24 00:19 | ||
0001-Option-to-require-2FA-authentication-only-for-users-.patch (5.3 KB) 0001-Option-to-require-2FA-authentication-only-for-users-.patch | Marius BĂLTEANU, 2022-01-27 21:54 | ||
0001-Option-to-require-2FA-authentication-only-for-users-.patch (4.93 KB) 0001-Option-to-require-2FA-authentication-only-for-users-.patch | Marius BĂLTEANU, 2022-01-30 11:10 | ||
options.png (88 KB) options.png | Marius BĂLTEANU, 2022-01-30 11:12 | ||
hints.png (79.7 KB) hints.png | Marius BĂLTEANU, 2022-01-30 11:12 |
Related issues
Updated by Marius BĂLTEANU about 3 years ago
- Related to Feature #1237: Add support for two-factor authentication added
Updated by Bernhard Rohloff about 3 years 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.
Updated by Marius BĂLTEANU about 3 years 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.
Updated by Alex JXXX almost 3 years 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 ?
Updated by Marius BĂLTEANU over 2 years ago
- File 0001-Option-to-require-2FA-authentication-only-for-users-.patch 0001-Option-to-require-2FA-authentication-only-for-users-.patch added
Here is a patch that I would like to commit in the following days.
Updated by Marius BĂLTEANU over 2 years ago
- Related to Feature #34070: Allow setting a grace period when forcing 2FA added
Updated by Holger Just over 2 years ago
- Related to Feature #31920: Require 2FA only for certain user groups added
Updated by Holger Just over 2 years 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?
Updated by Marius BĂLTEANU over 2 years 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.
Updated by Marius BĂLTEANU over 2 years ago
- File 0001-Option-to-require-2FA-authentication-only-for-users-.patch 0001-Option-to-require-2FA-authentication-only-for-users-.patch added
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)?
Updated by Holger Just over 2 years 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.
Updated by Felix Schäfer over 2 years 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.
Updated by Marius BĂLTEANU over 2 years ago
- File 0001-Option-to-require-2FA-authentication-only-for-users-.patch 0001-Option-to-require-2FA-authentication-only-for-users-.patch added
- File hints.png hints.png added
- File options.png options.png added
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
inSetting.twofa_optional?
above. If you don't want to do that, at least the view inapp/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"?
Updated by Marius BĂLTEANU over 2 years 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.
Updated by Holger Just over 2 years 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.
Updated by Marius BĂLTEANU over 2 years ago
- Status changed from New to Closed
- Resolution set to Fixed
Feature added in r21395.
Updated by Marius BĂLTEANU over 2 years ago
- Status changed from Closed to Reopened
Updated by Marius BĂLTEANU over 2 years ago
- Status changed from Reopened to Resolved
Updated by Marius BĂLTEANU over 2 years ago
- Status changed from Resolved to Closed