Require code to be entered when activating 2FA.

6 files changed, 95 insertions, 38 deletions

diff --git a/perllib/Catalyst/Authentication/Credential/2FA.pm b/perllib/Catalyst/Authentication/Credential/2FA.pm
index 22f4b4cff..9f22fed11 100644
--- a/perllib/Catalyst/Authentication/Credential/2FA.pm
+++ b/perllib/Catalyst/Authentication/Credential/2FA.pm
@@ -26,7 +26,7 @@ sub authenticate {
 
         $c->stash->{token} = $c->get_param('token');
 
-        if ($self->check_2fa($c, $user_obj)) {
+        if ($c->check_2fa($user_obj->has_2fa)) {
             if ($c->stash->{token}) {
                 my $token = $c->forward('/tokens/load_auth_token', [ $c->stash->{token}, '2fa' ]);
                 # Will contain a detach_to and report/update data
@@ -48,18 +48,6 @@ sub authenticate {
     }
 }
 
-sub check_2fa {
-    my ($self, $c, $user) = @_;
-
-    if (my $code = $c->get_param('2fa_code')) {
-        my $auth = Auth::GoogleAuth->new;
-        my $secret32 = $user->get_extra_metadata('2fa_secret');
-        return 1 if $auth->verify($code, 2, $secret32);
-        $c->stash->{incorrect_code} = 1;
-    }
-    return 0;
-}
-
 __PACKAGE__;
 
 __END__
diff --git a/perllib/FixMyStreet/App.pm b/perllib/FixMyStreet/App.pm
index 27a5a4580..1173523bc 100644
--- a/perllib/FixMyStreet/App.pm
+++ b/perllib/FixMyStreet/App.pm
@@ -13,6 +13,7 @@ use FixMyStreet::Email::Sender;
 use FixMyStreet::PhotoStorage;
 use Utils;
 
+use Auth::GoogleAuth;
 use Path::Tiny 'path';
 use Try::Tiny;
 use Text::CSV;
@@ -516,6 +517,23 @@ sub set_param {
     $c->req->params->{$param} = $value;
 }
 
+=head2 check_2fa
+
+Given a user's secret, verifies a submitted code.
+
+=cut
+
+sub check_2fa {
+    my ($c, $secret32) = @_;
+
+    if (my $code = $c->get_param('2fa_code')) {
+        my $auth = Auth::GoogleAuth->new;
+        return 1 if $auth->verify($code, 2, $secret32);
+        $c->stash->{incorrect_code} = 1;
+    }
+    return 0;
+}
+
 =head1 SEE ALSO
 
 L<FixMyStreet::App::Controller::Root>, L<Catalyst>
diff --git a/perllib/FixMyStreet/App/Controller/Auth/Profile.pm b/perllib/FixMyStreet/App/Controller/Auth/Profile.pm
index 107720aee..8512e7562 100644
--- a/perllib/FixMyStreet/App/Controller/Auth/Profile.pm
+++ b/perllib/FixMyStreet/App/Controller/Auth/Profile.pm
@@ -188,23 +188,37 @@ sub generate_token : Path('/auth/generate_token') {
         if ($c->get_param('generate_token')) {
             my $token = mySociety::AuthToken::random_token();
             $c->user->set_extra_metadata('access_token', $token);
+            $c->user->update;
             $c->stash->{token_generated} = 1;
         }
 
-        if ($c->get_param('toggle_2fa')) {
-            if ($has_2fa) {
-                $c->user->unset_extra_metadata('2fa_secret');
-                $c->stash->{toggle_2fa_off} = 1;
+        my $action = $c->get_param('2fa_action') || '';
+        $action = 'deactivate' if $c->get_param('2fa_deactivate');
+        $action = 'activate' if $c->get_param('2fa_activate');
+
+        my $secret;
+        if ($action eq 'deactivate') {
+            $c->user->unset_extra_metadata('2fa_secret');
+            $c->user->update;
+            $c->stash->{toggle_2fa_off} = 1;
+        } elsif ($action eq 'confirm') {
+            $secret = $c->get_param('secret32');
+            if ($c->check_2fa($secret)) {
+                $c->user->set_extra_metadata('2fa_secret', $secret);
+                $c->user->update;
+                $c->stash->{stage} = 'success';
+                $has_2fa = 1;
             } else {
-                my $auth = Auth::GoogleAuth->new;
-                $c->stash->{qr_code} = $auth->qr_code(undef, $c->user->email, 'FixMyStreet');
-                $c->stash->{secret32} = $auth->secret32;
-                $c->user->set_extra_metadata('2fa_secret', $auth->secret32);
-                $c->stash->{toggle_2fa_on} = 1;
+                $action = 'activate'; # Incorrect code, reshow
             }
         }
 
-        $c->user->update();
+        if ($action eq 'activate') {
+            my $auth = Auth::GoogleAuth->new;
+            $c->stash->{qr_code} = $auth->qr_code($secret, $c->user->email, 'FixMyStreet');
+            $c->stash->{secret32} = $auth->secret32;
+            $c->stash->{stage} = 'activate';
+        }
     }
 
     $c->stash->{has_2fa} = $has_2fa ? 1 : 0;
diff --git a/t/app/controller/auth_profile.t b/t/app/controller/auth_profile.t
index 6cab1fb6c..cc567544c 100644
--- a/t/app/controller/auth_profile.t
+++ b/t/app/controller/auth_profile.t
@@ -439,19 +439,28 @@ subtest "Test two-factor authentication admin" => sub {
     $mech->get_ok('/auth/generate_token');
     ok !$user->get_extra_metadata('2fa_secret');
 
-    $mech->submit_form_ok({ button => 'toggle_2fa' }, "submit 2FA activation");
+    $mech->submit_form_ok({ button => '2fa_activate' }, "submit 2FA activation");
+    my ($token) = $mech->content =~ /name="secret32" value="([^"]*)">/;
+
+    use Auth::GoogleAuth;
+    my $auth = Auth::GoogleAuth->new({ secret32 => $token });
+    my $code = $auth->code;
+    my $wrong_code = $auth->code(undef, time() - 120);
+
+    $mech->submit_form_ok({ with_fields => { '2fa_code' => $wrong_code } }, "provide wrong 2FA code" );
+    $mech->content_contains('Try again');
+    $mech->submit_form_ok({ with_fields => { '2fa_code' => $code } }, "provide correct 2FA code" );
+
     $mech->content_contains('has been activated', "2FA activated");
 
     $user->discard_changes();
-    my $token = $user->get_extra_metadata('2fa_secret');
-    ok $token, '2FA secret set';
-
-    $mech->content_contains($token, 'secret displayed');
+    my $user_token = $user->get_extra_metadata('2fa_secret');
+    is $token, $user_token, '2FA secret set';
 
     $mech->get_ok('/auth/generate_token');
     $mech->content_lacks($token, 'secret no longer displayed');
 
-    $mech->submit_form_ok({ button => 'toggle_2fa' }, "submit 2FA deactivation");
+    $mech->submit_form_ok({ button => '2fa_deactivate' }, "submit 2FA deactivation");
     $mech->content_contains('has been deactivated', "2FA deactivated");
   }
 };
diff --git a/templates/web/base/auth/2fa/form-add.html b/templates/web/base/auth/2fa/form-add.html
new file mode 100644
index 000000000..706f1a31d
--- /dev/null
+++ b/templates/web/base/auth/2fa/form-add.html
@@ -0,0 +1,17 @@
+<p>[% loc('Please scan this image with your app, or enter the text code into your app, then generate a new one-time code and enter it below:') %]</p>
+
+<p align="center"><img src="[% qr_code %]"></p>
+<p align="center">[% secret32.replace('(....)', '$1 ') %]</p>
+
+[% IF incorrect_code %]
+    <div class="form-error">[% loc('Sorry, that wasn&rsquo;t the correct code') %].
+    [% loc('Try again') %]:</div>
+[% END %]
+
+<label for="2fa_code">[% loc('Code') %]</label>
+<div class="form-txt-submit-box">
+    <input autofocus class="form-control" type="number" id="2fa_code" name="2fa_code" value="" required>
+    <input type="submit" value="[% loc('Submit') %]" class="btn-primary">
+</div>
+<input type="hidden" name="secret32" value="[% secret32 %]">
+<input type="hidden" name="2fa_action" value="confirm">
diff --git a/templates/web/base/auth/generate_token.html b/templates/web/base/auth/generate_token.html
index 3c9dbf6aa..6b8a63ea3 100644
--- a/templates/web/base/auth/generate_token.html
+++ b/templates/web/base/auth/generate_token.html
@@ -15,25 +15,31 @@ INCLUDE 'header.html', title = loc('Security'), bodyclass = 'fullwidthpage'
     <p><a href="/my">[% loc('Your account') %]</a></p>
   </div>
 
-[% ELSIF toggle_2fa_on %]
+[% ELSIF toggle_2fa_off %]
 
   <div class="confirmation-header">
-    <h1>[% loc('Two-factor authentication has been activated') %]</h1>
-
-    <p align="center"><img src="[% qr_code %]"></p>
-    <p align="center">[% secret32.replace('(....)', '$1 ') %]</p>
+    <h1>[% loc('Two-factor authentication has been deactivated') %]</h1>
 
     <p><a href="/my">[% loc('Your account') %]</a></p>
   </div>
 
-[% ELSIF toggle_2fa_off %]
+[% ELSIF stage == 'success' %]
 
   <div class="confirmation-header">
-    <h1>[% loc('Two-factor authentication has been deactivated') %]</h1>
-
+    <h1>[% loc('Two-factor authentication has been activated') %]</h1>
+    <p>[% loc('Thanks, you have successfully enabled two-factor authentication on your account.') %]</p>
     <p><a href="/my">[% loc('Your account') %]</a></p>
   </div>
 
+[% ELSIF stage == 'activate' %]
+  <div class="confirmation-header confirmation-header--phone">
+    <h1>[% loc('Two-factor authentication') %]</h1>
+
+    <form action="[% c.uri_for_action('/auth/profile/generate_token') %]" method="post" name="generate_token">
+        <input type="hidden" name="token" value="[% csrf_token %]">
+        [% PROCESS 'auth/2fa/form-add.html' %]
+    </form>
+
 [% ELSE %]
 
 <h1>[% loc('Security') %]</h1>
@@ -51,7 +57,12 @@ INCLUDE 'header.html', title = loc('Security'), bodyclass = 'fullwidthpage'
     <p>
     <input name="generate_token" type="submit" class="btn" value="[% existing_token ? loc('Replace token') : loc('Generate token') %]">
       [% IF c.user.is_superuser || c.user.from_body %]
-        <input name="toggle_2fa" type="submit" class="btn" value="[% has_2fa ? loc('Deactivate two-factor authentication') : loc('Activate two-factor authentication') %]">
+        [% IF has_2fa %]
+        <input name="2fa_activate" type="submit" class="btn" value="[% loc('Change two-factor authentication') %]">
+        <input name="2fa_deactivate" type="submit" class="btn" value="[% loc('Deactivate two-factor authentication') %]">
+        [% ELSE %]
+        <input name="2fa_activate" type="submit" class="btn" value="[% loc('Activate two-factor authentication') %]">
+        [% END %]
       [% END %]
     </p>
 </form>