Redmine

+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.

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.

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 ?

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?

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.

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.

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.

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.

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.