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

Force 2FA at login for developers, behind a waffle #10046

Closed
diox opened this issue Nov 21, 2018 · 2 comments
Closed

Force 2FA at login for developers, behind a waffle #10046

diox opened this issue Nov 21, 2018 · 2 comments

Comments

@diox
Copy link
Member

@diox diox commented Nov 21, 2018

Implementation side of mozilla/addons#732 because this issue is getting a little hard to follow with all the back & forth discussing this with 2FA.

The idea is to create a waffle that, when enabled, forces add-on developers (only developers, and not users that only have static/lightweight themes) to enable Two Factor Auth on their FxA account if they don't already. It would work by checking what FxA returns, and if we notice a developer is logging on without 2FA, we don't start the session, and instead start the FxA oauth dance again, this time asking specifically for 2 FA to be enabled. On FxA side the user would then set up their 2nd factor if needed, then proceed and come back to us all in the same flow.

The waffle should be off by default as we're waiting on some UX improvements on FxA side (https://github.com/mozilla/fxa-content-server/issues/6683 and https://github.com/mozilla/fxa-content-server/issues/6661).

Any discussion about this that are not relevant to the implementation details should go to mozilla/addons#732

@diox
Copy link
Member Author

@diox diox commented Nov 26, 2018

Something to keep in mind: regardless of what we do on our end, FxA should not allow you to log in without your second factor if you already have one set up. This issue is about forcing developers to have one in the first place.

Test scenarios when logging in:

  • If the 2fa-for-developers waffle switch is off (the default), nothing special should happen.
  • If the waffle is on:
    • If you're not a developer, or just a theme developer, nothing special should happen
    • If you are a non-theme developer, and you already had a second factor set up, nothing special should happen (FxA should already have forced you to authenticate with that second factor regardless)
    • If you are a non-theme developer, and didn't have a second factor, you should be prompted for your password again (that's an issue in FxA, linked in description above) and then shown an error message because you don't have a second factor (another issue in FxA linked above will make it possible to set it up from that screen)

Please switch the waffle off once you're done testing, it's not ready for production or daily use because of the UX issues on FxA side.

@AlexandraMoga
Copy link

@AlexandraMoga AlexandraMoga commented Nov 28, 2018

Here are my test results after verifiying this feature on AMO -dev:

2fa-for-developers waffle switch off

  • all loggins and registrations are working as expected

2fa-for-developers waffle switch on

  • developer accounts with prior 2fa set-up - user is logged in after confirming his 2fa security code
  • theme developers and regular users with prior 2fa set-up - users are logged in after confirming their 2fa security code
  • developer accounts with no prior 2fa set-up - user is asked to enable 2fa in order to log in (with the FxA flow issues mentioned in the description)
  • theme developers and regular users with no prior 2fa set-up - are logged in to AMO, no 2fa is required
  • developer accounts with no prior 2fa set-up with all their addons disabled - are logged in to AMO, no 2fa is required

There is an issue with new account registrations for witch I've filed #10105

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants