@tsteur opened this Pull Request on November 5th 2018 Member

fixes #13325 in a new plugin TwoFactorAuth

Also provides a new class PasswordVerifier which requires a user to first confirm their password in a new screen before continuing with a specific action (not available in a dialog currently).

@tsteur commented on November 6th 2018 Member

Todo: We need to update either this PR or https://github.com/matomo-org/matomo/pull/13472 depending on which one gets merged first to apply brute force / lock down changes to #13670

@diosmosis commented on November 28th 2018 Member

From testing locally:

  • in the setupTwoFactorAuth page, if I copy text manually, it doesn't enable the 'next' button, which confused me a bit.
  • not sure how important it is, but the 2 factor auth code input seems to save values:

image

seems a bit odd to see past values here.

  • the mailto for the "Ask super user to reset your authentication code " link isn't set properly for non-superusers. likely due to the change in UsersManager::enrichUser (the email is removed if the user isn't the current user)
@diosmosis commented on November 28th 2018 Member

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

@tsteur commented on November 29th 2018 Member

@diosmosis fyi applying all the changes you suggested.

the mailto for the "Ask super user to reset your authentication code " link isn't set properly for non-superusers. likely due to the change in UsersManager::enrichUser (the email is removed if the user isn't the current user)

this is a bug and regressed... will also look into how to fix it...

in the setupTwoFactorAuth page, if I copy text manually, it doesn't enable the 'next' button, which confused me a bit.

not sure if we can detect whether all the codes have been copied manually? sounds bit tricky maybe but will check if possible

@diosmosis commented on November 29th 2018 Member

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.

@tsteur commented on November 29th 2018 Member

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

Please backup your recovery codes using one of the above methods before continuing the two-factor authentication setup.

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.

@sgiehl commented on December 2nd 2018 Member

I will have a look now...

@tsteur commented on December 3rd 2018 Member

@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 👍

@tsteur commented on December 3rd 2018 Member

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

This Pull Request was closed on December 3rd 2018
Powered by GitHub Issue Mirror