Only show access tokens once, and store hashed.

9 files changed, 46 insertions, 46 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 8f8fc5ef0..decfa2b5e 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1,6 +1,8 @@
 ## Releases
 
 * Unreleased
+    - Security:
+        - Store personal access tokens hashed, and only show once, upon generation.
     - New features:
         - Add Open Location Codes support to search box. #3047
     - Front end improvements:
diff --git a/perllib/Catalyst/Authentication/Credential/AccessToken.pm b/perllib/Catalyst/Authentication/Credential/AccessToken.pm
index 7827c936d..24398823d 100644
--- a/perllib/Catalyst/Authentication/Credential/AccessToken.pm
+++ b/perllib/Catalyst/Authentication/Credential/AccessToken.pm
@@ -4,7 +4,7 @@ use strict;
 use warnings;
 use base 'Class::Accessor::Fast';
 
-__PACKAGE__->mk_accessors(qw(token_field token_lookup));
+__PACKAGE__->mk_accessors(qw(token_field));
 
 our $VERSION = "0.01";
 
@@ -23,21 +23,23 @@ sub authenticate {
     $token ||= $c->get_param('access_token');
     return unless $token;
 
-    my $field = $self->token_field || 'access_token';
+    my $id;
+    ($id, $token) = split /-/, $token, 2;
+    return unless $id =~ /^[1-9]\d*$/;
 
-    my $value = $token;
-    if (my $lookup = $self->token_lookup) {
-        $value = {};
-        foreach (keys %$lookup) {
-            my $v = $lookup->{$_};
-            $v =~ s/TOKEN/$token/;
-            $value->{$_} = $v;
-        }
-    }
-    my $user_obj = $realm->find_user({ $field => $value }, $c);
-    if (ref $user_obj) {
+    my $user_obj = $realm->find_user({ id => $id }, $c);
+    if (ref($user_obj) && $self->check_token($user_obj, $token)) {
         return $user_obj;
     }
+    return;
+}
+
+sub check_token {
+    my ($self, $user, $token) = @_;
+
+    my $field = $self->token_field || 'access_token';
+    my $value = $user->$field;
+    return $user->_column_encoders->{password}->($token, $value) eq $value;
 }
 
 __PACKAGE__;
@@ -102,12 +104,6 @@ depending on the storage class used, but is most likely something like
 'access_token'. In fact, this is so common that if this is left out of the
 config, it defaults to 'access_token'.
 
-=item token_lookup
-
-If the token isn't a field on its own, but contained within another field, you
-can provide a custom lookup here, where the string TOKEN in a value will be
-replaced by the access token.
-
 =back
 
 =head1 USAGE
diff --git a/perllib/FixMyStreet/App.pm b/perllib/FixMyStreet/App.pm
index 4ca6f23cb..638fcc4e4 100644
--- a/perllib/FixMyStreet/App.pm
+++ b/perllib/FixMyStreet/App.pm
@@ -101,9 +101,6 @@ __PACKAGE__->config(
             use_session => 0,
             credential => {
                 class => 'AccessToken',
-                token_field => 'extra',
-                # This means the token has to be 18 characters long (as generated by AuthToken)
-                token_lookup => { like => "%access_token,T18:TOKEN,%" },
             },
             store => $store,
         },
diff --git a/perllib/FixMyStreet/App/Controller/Auth/Profile.pm b/perllib/FixMyStreet/App/Controller/Auth/Profile.pm
index a89c6f539..a5dc5d3e7 100644
--- a/perllib/FixMyStreet/App/Controller/Auth/Profile.pm
+++ b/perllib/FixMyStreet/App/Controller/Auth/Profile.pm
@@ -188,9 +188,10 @@ 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);
+            my $u = FixMyStreet::DB->resultset("User")->new({ password => $token });
+            $c->user->set_extra_metadata('access_token', $u->password);
             $c->user->update;
-            $c->stash->{token_generated} = 1;
+            $c->stash->{token_generated} = $c->user->id . '-' . $token;
         }
 
         my $action = $c->get_param('2fa_action') || '';
@@ -224,7 +225,7 @@ sub generate_token : Path('/auth/generate_token') {
     }
 
     $c->stash->{has_2fa} = $has_2fa ? 1 : 0;
-    $c->stash->{existing_token} = $c->user->get_extra_metadata('access_token');
+    $c->stash->{existing_token} = $c->user->get_extra_metadata('access_token') ? 1 : 0;
 }
 
 __PACKAGE__->meta->make_immutable;
diff --git a/perllib/FixMyStreet/DB/Result/User.pm b/perllib/FixMyStreet/DB/Result/User.pm
index 49338f245..e5be14abf 100644
--- a/perllib/FixMyStreet/DB/Result/User.pm
+++ b/perllib/FixMyStreet/DB/Result/User.pm
@@ -179,6 +179,11 @@ sub check_password {
     }
 }
 
+sub access_token {
+    my $self = shift;
+    return $self->get_extra_metadata('access_token');
+}
+
 around password => sub {
     my ($orig, $self) = (shift, shift);
     if (@_) {
diff --git a/t/app/controller/auth.t b/t/app/controller/auth.t
index 24deb8cab..8b4b772fc 100644
--- a/t/app/controller/auth.t
+++ b/t/app/controller/auth.t
@@ -245,19 +245,20 @@ subtest "check logging in with token" => sub {
 
     my $user =  FixMyStreet::DB->resultset('User')->find( { email => $test_email } );
     # token needs to be 18 characters
-    $user->set_extra_metadata('access_token', '1234567890abcdefgh');
+    my $u = FixMyStreet::DB->resultset("User")->new({ password => '1234567890abcdefgh' });
+    $user->set_extra_metadata('access_token', $u->password);
     $user->update();
 
-    $mech->add_header('Authorization', 'Bearer 1234567890abcdefgh');
+    $mech->add_header('Authorization', 'Bearer ' . $user->id . '-1234567890abcdefgh');
     $mech->logged_in_ok;
 
     $mech->delete_header('Authorization');
     $mech->not_logged_in_ok;
 
-    $mech->get_ok('/auth/check_auth?access_token=1234567890abcdefgh');
+    $mech->get_ok('/auth/check_auth?access_token=' . $user->id . '-1234567890abcdefgh');
 
-    $mech->add_header('Authorization', 'Bearer 1234567890abcdefgh');
-    $user->set_extra_metadata('access_token', 'XXXXXXXXXXXXXXXXXX');
+    $mech->add_header('Authorization', 'Bearer ' . $user->id . '-1234567890abcdefgh');
+    $user->set_extra_metadata('access_token', '$2a$08$HNslSx7Uic7q6Ti5WYT5JOT6npYPwrwLnDMJMJoD22LIqG5TfDIKf');
     $user->update();
     $mech->not_logged_in_ok;
 
diff --git a/t/app/controller/auth_profile.t b/t/app/controller/auth_profile.t
index e5dfe2764..230e02d2b 100644
--- a/t/app/controller/auth_profile.t
+++ b/t/app/controller/auth_profile.t
@@ -417,16 +417,16 @@ subtest "Test generate token page" => sub {
         "submit generate token form"
     );
     $mech->content_contains( 'Your token has been generated', "token generated" );
+    my ($token) = $mech->content =~ /<span>(.*?)<\/span>/;
+    my @parts = split /-/, $token, 2;
+    is $parts[0], $user->id, 'token has user ID at start';
 
     $user->discard_changes();
-    my $token = $user->get_extra_metadata('access_token');
-    ok $token, 'access token set';
-
-    $mech->content_contains($token, 'access token displayed');
+    $user->password($user->get_extra_metadata('access_token'), 1);
+    ok $user->check_password($parts[1]), 'access token set';
 
     $mech->get_ok('/auth/generate_token');
-    $mech->content_contains('Current token:');
-    $mech->content_contains($token, 'access token displayed');
+    $mech->content_lacks($parts[1], 'access token not displayed');
     $mech->content_contains('If you generate a new token');
 
     $mech->log_out_ok;
diff --git a/t/app/controller/dashboard.t b/t/app/controller/dashboard.t
index c62ada89a..0f07bcae0 100644
--- a/t/app/controller/dashboard.t
+++ b/t/app/controller/dashboard.t
@@ -236,14 +236,15 @@ FixMyStreet::override_config {
     subtest 'export as csv using token' => sub {
         $mech->log_out_ok;
 
-        $counciluser->set_extra_metadata('access_token', '1234567890abcdefgh');
+        my $u = FixMyStreet::DB->resultset("User")->new({ password => '1234567890abcdefgh' });
+        $counciluser->set_extra_metadata('access_token', $u->password);
         $counciluser->update();
 
         $mech->get_ok('/dashboard?export=1');
         like $mech->res->header('Content-type'), qr'text/html';
         $mech->content_lacks('Report ID');
 
-        $mech->add_header('Authorization', 'Bearer 1234567890abcdefgh');
+        $mech->add_header('Authorization', 'Bearer ' . $counciluser->id . '-1234567890abcdefgh');
         $mech->get_ok('/dashboard?export=1');
         like $mech->res->header('Content-type'), qr'text/csv';
         $mech->content_contains('Report ID');
diff --git a/templates/web/base/auth/generate_token.html b/templates/web/base/auth/generate_token.html
index 9152d0cb3..7654e11eb 100644
--- a/templates/web/base/auth/generate_token.html
+++ b/templates/web/base/auth/generate_token.html
@@ -9,7 +9,11 @@ INCLUDE 'header.html', title = loc('Security'), bodyclass = 'fullwidthpage'
 
     <p>
         <strong>[% loc('Token') %]:</strong>
-        <span>[% existing_token | html %]</span>
+        <span>[% token_generated %]</span>
+    </p>
+
+    <p>
+        [% loc('This will be the only time this token is visible, so please make a note of it now.') %]
     </p>
 
     <p><a href="/my">[% loc('Your account') %]</a></p>
@@ -67,13 +71,6 @@ INCLUDE 'header.html', title = loc('Security'), bodyclass = 'fullwidthpage'
 
     <input type="hidden" name="token" value="[% csrf_token %]">
 
-    [% IF existing_token %]
-        <p>
-            <strong>[% loc('Current token:') %]</strong>
-            <span>[% existing_token | html %]</span>
-        </p>
-    [% END %]
-
     <p>
     <input name="generate_token" type="submit" class="btn" value="[% existing_token ? loc('Replace token') : loc('Generate token') %]">
     </p>