Add Support for 2FA for "Admin"

[jaapmarcus]

Easy to add support for "users"

[jaapmarcus]

Have used: robthree/twofactorauth "We" have used it for Hestia for the last few years without any issues. It works extremely simple...

[BelleNottelling]

This seems incomplete and not fully thought out. Seeing issues with it:

• The formatting and CS is somewhat a mess
• Why is there no way to recover access if user loses access to their 2FA? I've seen it a ton where a person will get a new phone and they don't remember to move their authentication over to the new one.
• There should be the ability to require 2FA if the organization decides that's needed.
• The controls for 2FA is only under the staff member's profile, meaning that if someone gets themselves locked out another administrator cannot reset it for them, forcing them to use the DB.
• Wording / capitalization is all over the place.
• It would be preferable to have tests for it written..

Moreover, I have it on my TODO list to build a far more complete MFA system that's for both clients and staff members which would also include more options like email-based TOTP, actually including recovery options, the ability to require it for staff / clients.

[BelleNottelling]

Does this actually work?
Your code to disable 2FA sets it to an empty string.

[jaapmarcus]

Yes it works by clearing the secret key in the user table...

When it is set and the users provide no token the login fails and the user is asked for the token.

Then verifies and continue with the login as normal

[BelleNottelling]

Ah I missed that you wrote that with a loose type comparison, which is the only reason it works.

if ('' != null) {
    echo "An empty string is not equal to null when using loose type comparisons";
} else {
    echo "An empty string is equal to null when using loose type comparisons";
}

if ('' !== null) {
    echo "An empty string is not equal to null when using strict type comparisons" ;
} else {
    echo "An empty string is equal to null when using strict type comparisons";
}

Gives:

An empty string is equal to null when using loose type comparisons
An empty string is not equal to null when using strict type comparisons

I believe our automated code refactoring will change this to be a strict comparison, but even if it doesn't I'd expect us to be using those whenever possible and I think it is incredibly likely that someone would manually change it to be a strict comparison later down the line which will then result in this check not working correctly because your code sets the token in the DB to an empty string when resetting it, not to null.

Please update the code to reset it back to null and then turn this into a strict comparison.

Or arguably even better, skip the comparison check at all and just ensure that it's not a falsely value.

if ($model->tfa_token) {
    // Code goes here
}

[jaapmarcus]

• Why is there no way to recover access if user loses access to their 2FA? I've seen it a ton where a person will get a new phone and they don't remember to move their authentication over to the new one.

All methods to reset 2FA sucks.

• Sending via email means an extra weakness if access email to lost even worse if forgot password is enabled then the attackers doesn't even need know the Fossbilling password.
• Reset tokens are never saved how ever in this case they are saved in 2FA App.

Yes need to add disable 2FA account via admin it self for other users should be relative easy...

[BelleNottelling]

• Why is there no way to recover access if user loses access to their 2FA? I've seen it a ton where a person will get a new phone and they don't remember to move their authentication over to the new one.

All methods to reset 2FA sucks.

* Sending via email means an extra weakness if access email to lost even worse if forgot password  is enabled then the attackers doesn't even need know the Fossbilling password.

* Reset tokens are never saved how ever in this case they are saved in 2FA App.

Yes need to add disable 2FA account via admin it self for other users should be relative easy...

I agree on email recovery being a poor option, but I think not including the ability to use reset tokens simply because most people won't end up keeping them is kinda a lame excuse.

It's true a lot of people will never end up using that, but I think it's fair to say that a recovery token is the best available option that doesn't require someone else performing the account recovery / the use of the database, I think the ability to do so should be there for the individuals who are responsible enough to save the recovery key

[John-S4]

• Reset tokens are never saved

I always save them

It's true a lot of people will never end up using that, but I think it's fair to say that a recovery token is the best available option that doesn't require someone else performing the account recovery / the use of the database

I'd agree with this, I think the ability to use a recovery key should be in there.

[admdly]

I echo @BelleNottelling and @John-S4 - there needs to be a recovery method, regardless of whether some (most?) people choose not to use it correctly.

Recovery code(s) is a fairly well established method, so I can't see why this wouldn't work here.