Implement Two Factor Authentication #13670
Conversation
…not return secret in user api etc
tsteur
added
the
Pull Request WIP
|
From testing locally:
seems a bit odd to see past values here.
|
|
Looked through the code, seems ok but there's a lot to review, so could have missed something. @sgiehl might want to review as well |
|
@diosmosis fyi applying all the changes you suggested.
this is a bug and regressed... will also look into how to fix it...
not sure if we can detect whether all the codes have been copied manually? sounds bit tricky maybe but will check if possible |
Could just allow users to click next w/o copying/etc. Would only be important if lots of people tried to copy manually. |
platforms like github force you to press one of the buttons basically to increase likelihood that they have created a backup of the recovery codes. I just checked and it does show a notification
So I reckon it should be good for now... also fixed couple screenshot tests but a few more need to be updated after next run. Also added support for activity log... @sgiehl let me know whether you want to have a look or whether it can be merged. |
|
I will have a look now... |
There was a problem hiding this comment.
Some Feedback:
-
Left some comments
-
When 2FA is already setup the UI imho does not make it clear enough that
Setup a different devicewill also remove the currently configured 2FA and also the recovery codes. Github for example displays this warning when changing the 2FA device:
-
Migration from GoogleAuthenticator plugin seems to work fine. Shall I release a new version that marks the plugin not compatible with Matomo 3.8?
| /** | ||
| * | ||
| */ | ||
| class FormTwoFactorAuthCode extends QuickForm2 |
There was a problem hiding this comment.
We should maybe try to avoid using QuickForm2 in the future, as that library should better be removed. It's unmaintained since 2014
There was a problem hiding this comment.
It still works and we're using it in other places. There's not much point of replacing or not using it right now IMO. at some point we can replace all the usages at once but as long as it works I think there are other parts that are maybe more important. (for example replacing email lib etc)
| @@ -1,5 +1,10 @@ | |||
| { | |||
| "UsersManager": { | |||
| "2FA": "2FA", | |||
There was a problem hiding this comment.
Actually that string is also available in the 2FA plugin. Maybe we should only use one even if transifex will autofill one of the translations as soon as the other was translated. Unfortunately that doesn't happen when the translations are later changed, so in some cases their might be different translations for both...
There was a problem hiding this comment.
I left it there separately in case TwoFactorAuth plugin is disabled. I don't think we load the translation then? We're not checking in the UI whether 2FA plugin is disabled in UsersManager as it's edge case and bit tricky. This way it still shows correct translation there.
| 'twofactor_recovery_code' => "CREATE TABLE {$prefixTables}twofactor_recovery_code ( | ||
| idrecoverycode BIGINT UNSIGNED NOT NULL AUTO_INCREMENT, | ||
| login VARCHAR(100) NOT NULL, | ||
| recovery_code VARCHAR(40) NOT NULL, |
There was a problem hiding this comment.
That one differs to the update script, it only creates a VARCHAR(20)
|
@sgiehl now showing a warning re changing the device. Also fixed the update schema Yes sounds good to mark Google authenticator as invalid with 3.8 👍 |
|
merging now to avoid even more merge conflicts... getting conflicts all the time with other PRs that were merged in the meantime. let me know if there was anything else, happy to change it... |
fixes #13325 in a new plugin TwoFactorAuth
Also provides a new class
PasswordVerifierwhich requires a user to first confirm their password in a new screen before continuing with a specific action (not available in a dialog currently).