:glob ✱ Assignee
	Description • 9 years ago

2fa protection needs to also apply to:
- password changing
- api-key generation
- disabling the api_key_only preference

	:glob ✱ Assignee
	Comment 1 • 9 years ago

+ password resetting ('forgot password' workflow)

	Jesse Ruderman
	Comment 2 • 9 years ago

I guess this would go toward the end of the "forgot password" workflow, so attackers can't use it to find out whether an email address has an account.

	:glob ✱ Assignee
	Comment 3 • 9 years ago

(In reply to Jesse Ruderman from comment #2)
> I guess this would go toward the end of the "forgot password" workflow, so
> attackers can't use it to find out whether an email address has an account.

correct - it'll happen at the time you need to set a new password.  it wouldn't be required to request a forgot-password email.

	Ed Morley [:emorley]
	Comment 4 • 9 years ago

(In reply to Jesse Ruderman from comment #2)
> I guess this would go toward the end of the "forgot password" workflow, so
> attackers can't use it to find out whether an email address has an account.

Though anyone can create an account on BMO and then query https://bugzilla.mozilla.org/user_profile?login=foo@bar.com or use the CC list auto-complete feature to achieve the same.

	:glob ✱ Assignee
	Comment 5 • 9 years ago

- extends 2fa protection to:
  - setting a new password via prefs
  - setting a new email address via prefs
  - setting a new password via password-reset
  - adding a new api key
  - re-enabling a revoked api key
  - switching api_key_only pref to off
- updates login verification to use new flow, and
  - honour 'Restrict this session to this IP address' setting (bug 1201983)
  - honour 'remember login' setting (bug 1202281)
  - return you to the page that requested login (bug 1202886)
- show an icon next to actions that require 2fa
- adds enrolment specific code-failed message (bug 1202393)
- adds token attached data
  - carries state across 2ga request
- adds short-lived tokens
  - these tokens have a max lifetime of 1 hour instead of 3 days

	Dylan Hardison [:dylan] (he/him)
	Comment 6 • 9 years ago

Comment on attachment 8658583 [details] [diff] [review]
1199087_1.patch

Review of attachment 8658583 [details] [diff] [review]:
-----------------------------------------------------------------

Partial review, will keep working at it. No show-stoppers yet, but see notes.

::: Bugzilla/Constants.pm
@@ +470,5 @@
>  
>  # The maximum number of days a token will remain valid.
>  use constant MAX_TOKEN_AGE => 3;
> +# The maximum number of hours a short-lived token will remain valid.
> +use constant MAX_SHORT_TOKEN_AGE => 1;

So this is hours and the next constant is days. Perhaps MAX_SHORT_TOKEN_HOURS?

::: Bugzilla/Token.pm
@@ +490,5 @@
>      return 1;
>  }
>  
> +sub set_token_extra_data {
> +    my ($token, $data) = @_;

We should check the length of $data here. I'd rather know early if the encoded json is too big than have a hard-to-debug error later.

@@ +499,5 @@
> +
> +sub get_token_extra_data {
> +    my ($token) = @_;
> +    trick_taint($token);
> +    my $data = scalar Bugzilla->dbh->selectrow_array(

This is surprising, I would expect $data to be an integer 1 here.
my ($data) = ... may be more idiomatic.

::: js/account.js
@@ +143,5 @@
> +    $('#apikey-toggle-revoked')
> +        .click(function(event) {
> +            event.preventDefault();
> +            $('.apikey_revoked.bz_tui_hidden').removeClass('bz_tui_hidden');
> +            var $this = $(this);

$this is not used?
Also, I like either $this or that, but not both in the same file.


also, I totally missed the console.log call that's in this file. Let's remove that with this patch set. :)

	Dylan Hardison [:dylan] (he/him)
	Comment 7 • 9 years ago

Comment on attachment 8658583 [details] [diff] [review]
1199087_1.patch

Review of attachment 8658583 [details] [diff] [review]:
-----------------------------------------------------------------

r- for minor inconvenience

::: template/en/default/mfa/totp/verify.html.tmpl
@@ +17,4 @@
>    Please enter your verification code from your TOTP application:
>  </p>
>  
> +<form method="POST" action="[% postback.action FILTER none %]">

nit: elements inside the form element arn't indented more.

	:glob ✱ Assignee
	Comment 8 • 9 years ago

- MAX_SHORT_TOKEN_AGE --> MAX_SHORT_TOKEN_HOURS
- move json encoding/decoding into set_token_extra_data/get_token_extra_data
- add check for json length
- other minor changes

	Ed Morley [:emorley]
	Comment 9 • 9 years ago

Dylan, do you know when you will be able to review this? Bug 1202281 is dependant on this one - and I've just had to disable 2FA on my account since I've reached the point where I cannot face having to sign in (and unlock phone, manually type 2fa token etc) multiple times a day. Thanks :-)

	Ed Morley [:emorley]
	Comment 10 • 9 years ago

Ah mcote has just let me know you're in TRIBE this week - makes sense :-)

	Dylan Hardison [:dylan] (he/him)
	Comment 11 • 9 years ago

Comment on attachment 8661396 [details] [diff] [review]
1199087_2.patch

Review of attachment 8661396 [details] [diff] [review]:
-----------------------------------------------------------------

r=dylan

nice fix, I look forward to making everything use tokendata!

	:glob ✱ Assignee
	Comment 12 • 9 years ago

schema only:

To ssh://gitolite3@git.mozilla.org/webtools/bmo/bugzilla.git
   04b55ee..ff04d30  master -> master

	:glob ✱ Assignee
	Comment 13 • 9 years ago

To ssh://gitolite3@git.mozilla.org/webtools/bmo/bugzilla.git
   2e42540..043c752  master -> master