Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Don't allow enforcing 2FA unless the superuser has set it up already #17352

Closed
Findus23 opened this issue Mar 17, 2021 · 6 comments
Closed

Don't allow enforcing 2FA unless the superuser has set it up already #17352

Findus23 opened this issue Mar 17, 2021 · 6 comments
Labels
c: Usability For issues that let users achieve a defined goal more effectively or efficiently. Help wanted Beginner friendly issues or issues where we'd highly appreciate community's help and involvement.
Milestone
4.5.0

Comments

@Findus23
Copy link
Member

reported in https://forum.matomo.org/t/problem-with-the-two-factor-authentication-setting/41128

If a user who is unable to set up 2FA accidentally enables Require two-factor authentication for everyone, they are unable to disable it until they set up 2FA.
(or they update the setting in the db:

update matomo_plugin_setting
set setting_value=0
where setting_name = 'twoFactorAuthRequired'

)

I think this setting should only be allowed if at least one superuser has 2FA already set up (or maybe only if the current superuser has 2FA already set up)
@Findus23 Findus23 added the c: Usability For issues that let users achieve a defined goal more effectively or efficiently. label Mar 17, 2021
@Findus23 Findus23 changed the title Don't allow enforcing 2FA unless at least one superuser has set it up already Don't allow enforcing 2FA unless the superuser has set it up already Mar 17, 2021
@tsteur
Copy link
Member

tsteur commented Mar 18, 2021

Ideally we would get someone to activate 2FA first and then allow enabling it. That might be a bit more work though so we could maybe rather add a message Enforcing 2FA will require all users to set up 2FA using an authenticator app. This means they need to have a device like a smartphone or computer where they can install an app..

We could have otherwise checked if the current visitor has 2FA set up and then try if we can disable a setting in the UI super easily if 2FA is disabled for the current account and adjust the message saying please set up 2FA first but it's not needed if it takes say more than 10 minutes.

@tsteur tsteur added this to the 4.5.0 milestone Mar 18, 2021
@tsteur tsteur added the Help wanted Beginner friendly issues or issues where we'd highly appreciate community's help and involvement. label Mar 18, 2021
@andyjdavis andyjdavis mentioned this issue Mar 28, 2021
Merged
10 tasks
@andyjdavis
Copy link
Contributor

andyjdavis commented Mar 28, 2021 •
edited

I have put up a PR that just expands the text on the setting UI to provide more information about the consequences of turning on require 2fa. #17400

I have some questions about how to determine if the current user has 2fa enabled themselves.

The directory containing plugins/TwoFactorAuth/SystemSettings.php contains Controller.php, Validator.php, and TwoFactorAuthentication.php. The controller contains an instance of validator which contains an instance of twoFa. There are functions available on each level of that structure to determine if the user has 2fa set up. For example
Validator::check2FaEnabled()
Validator::checkVerified2FA()

Is there a preferred approach for accessing these methods (or similar) from the context of the system settings? I haven't been able to find an example of a plugin SystemSettings.php loading an instance of an object to allow it to selectively disable a setting on a per-user basis.

@diosmosis
Copy link
Member

Hi @andyjdavis, you'd want to use the TwoFactorAuthentication::isUserUsingTwoFactorAuthentication method. The TwoFactorAuthentication class is injected via DI, which means how you get it varies based on how you're coding. The easiest way to do this is via StaticContainer: StaticContainer::get(TwoFactorAuthentication::class), however using StaticContainer is discouraged if there is a simple alternative available (this would be brought up in the review if it were the case). Here are our docs on DI in Matomo: https://developer.matomo.org/guides/dependency-injection.

@andyjdavis
Copy link
Contributor

#17400 has now merged. I believe this issue can be closed.

@diosmosis
Copy link
Member

Fixed by #17400

@mattab mattab modified the milestones: 4.7.0, 4.5.0 Aug 25, 2021
@MatomoForumNotifications
Copy link

This issue has been mentioned on Matomo forums. There might be relevant details there:

https://forum.matomo.org/t/unable-to-enforce-2fa-for-everyone-greyed-out/44900/2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
c: Usability For issues that let users achieve a defined goal more effectively or efficiently. Help wanted Beginner friendly issues or issues where we'd highly appreciate community's help and involvement.
Projects
None yet
Milestone
4.5.0
Development

No branches or pull requests
6 participants
@diosmosis @tsteur @andyjdavis @mattab @Findus23 @MatomoForumNotifications