aboutsummaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
Diffstat
-rw-r--r--CHANGELOG.md2
-rw-r--r--perllib/Catalyst/Authentication/Credential/2FA.pm47
-rw-r--r--perllib/FixMyStreet/App/Controller/Auth.pm5
-rw-r--r--perllib/FixMyStreet/App/Controller/Auth/Profile.pm1
-rw-r--r--t/app/controller/auth.t48
-rw-r--r--t/app/controller/auth_profile.t49
-rw-r--r--t/app/controller/report_new.t29
-rw-r--r--templates/web/base/auth/2fa/intro.html32
-rw-r--r--templates/web/base/auth/generate_token.html4
9 files changed, 198 insertions, 19 deletions
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 49188e212..084a819ff 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -21,7 +21,7 @@
- Allow categories/Open311 questions to disable the reporting form. #2599
- Improve category edit form. #2469
- Allow editing of category name. #1398
- - Allow non-superusers to store 2FA secrets.
+ - Allow non-superuser staff to use 2FA, and optional enforcement of 2FA.
- New features:
- Categories can be listed under more than one group #2475
- OpenID Connect login support. #2523
diff --git a/perllib/Catalyst/Authentication/Credential/2FA.pm b/perllib/Catalyst/Authentication/Credential/2FA.pm
index 9f22fed11..8b6771037 100644
--- a/perllib/Catalyst/Authentication/Credential/2FA.pm
+++ b/perllib/Catalyst/Authentication/Credential/2FA.pm
@@ -21,11 +21,54 @@ sub authenticate {
my $user_obj = $realm->find_user($userfindauthinfo, $c);
if (ref($user_obj)) {
- # We don't care unless user has a 2FA secret
- return $user_obj unless $user_obj->get_extra_metadata('2fa_secret');
+
+ # We don't care unless user has a 2FA secret, or the cobrand mandates it
+ return $user_obj unless $user_obj->has_2fa || $c->cobrand->call_hook('must_have_2fa', $user_obj);
$c->stash->{token} = $c->get_param('token');
+ if (!$user_obj->has_2fa) {
+ $c->stash->{template} = 'auth/2fa/intro.html';
+ my $action = $c->get_param('2fa_action') || '';
+
+ my $secret;
+ if ($action eq 'confirm') {
+ $secret = $c->get_param('secret32');
+ if ($c->check_2fa($secret)) {
+ $user_obj->set_extra_metadata('2fa_secret' => $secret);
+ $user_obj->update;
+ 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
+ $c->stash($token->data);
+ } else {
+ $c->stash->{stage} = 'success';
+ $c->stash->{detach_to} = '/auth/two_factor_setup_success';
+ }
+ return $user_obj;
+ } else {
+ $action = 'activate'; # Incorrect code, reshow
+ }
+ }
+
+ if ($action eq 'activate') {
+ my $auth = Auth::GoogleAuth->new;
+ $c->stash->{qr_code} = $auth->qr_code($secret, $user_obj->email, 'FixMyStreet');
+ $c->stash->{secret32} = $auth->secret32;
+ $c->stash->{stage} = 'activate';
+ }
+
+ if ($c->stash->{tfa_data}) {
+ my $token = $c->model("DB::Token")->create( {
+ scope => '2fa',
+ data => $c->stash->{tfa_data},
+ });
+ $c->stash->{token} = $token->token;
+ }
+
+ $c->detach;
+ }
+
if ($c->check_2fa($user_obj->has_2fa)) {
if ($c->stash->{token}) {
my $token = $c->forward('/tokens/load_auth_token', [ $c->stash->{token}, '2fa' ]);
diff --git a/perllib/FixMyStreet/App/Controller/Auth.pm b/perllib/FixMyStreet/App/Controller/Auth.pm
index 9680a5624..2aa1144a0 100644
--- a/perllib/FixMyStreet/App/Controller/Auth.pm
+++ b/perllib/FixMyStreet/App/Controller/Auth.pm
@@ -540,6 +540,11 @@ sub check_auth : Local {
return;
}
+sub two_factor_setup_success : Private {
+ my ($self, $c) = @_;
+ # Only here to be detached to after setup success
+}
+
__PACKAGE__->meta->make_immutable;
1;
diff --git a/perllib/FixMyStreet/App/Controller/Auth/Profile.pm b/perllib/FixMyStreet/App/Controller/Auth/Profile.pm
index 8512e7562..91ffac205 100644
--- a/perllib/FixMyStreet/App/Controller/Auth/Profile.pm
+++ b/perllib/FixMyStreet/App/Controller/Auth/Profile.pm
@@ -195,6 +195,7 @@ sub generate_token : Path('/auth/generate_token') {
my $action = $c->get_param('2fa_action') || '';
$action = 'deactivate' if $c->get_param('2fa_deactivate');
$action = 'activate' if $c->get_param('2fa_activate');
+ $action = 'activate' if $action eq 'deactivate' && $has_2fa && $c->cobrand->call_hook('must_have_2fa', $c->user);
my $secret;
if ($action eq 'deactivate') {
diff --git a/t/app/controller/auth.t b/t/app/controller/auth.t
index fc1966b17..cc40bd2b0 100644
--- a/t/app/controller/auth.t
+++ b/t/app/controller/auth.t
@@ -1,3 +1,10 @@
+package FixMyStreet::Cobrand::Dummy;
+use parent 'FixMyStreet::Cobrand::Default';
+
+sub must_have_2fa { 1 }
+
+package main;
+
use Test::MockModule;
use FixMyStreet::TestMech;
@@ -7,10 +14,6 @@ my $test_email = 'test@example.com';
my $test_email3 = 'newuser@example.org';
my $test_password = 'foobar123';
-END {
- done_testing();
-}
-
$mech->get_ok('/auth');
# check that we can't reach a page that is only available to authenticated users
@@ -304,3 +307,40 @@ subtest "Test two-factor authentication login" => sub {
$mech->submit_form_ok({ with_fields => { '2fa_code' => $code } }, "provide correct 2FA code" );
$mech->logged_in_ok;
};
+
+subtest "Test enforced two-factor authentication" => sub {
+ FixMyStreet::override_config {
+ ALLOWED_COBRANDS => 'dummy',
+ }, sub {
+ my $user = FixMyStreet::App->model('DB::User')->find( { email => $test_email } );
+ $user->unset_extra_metadata('2fa_secret');
+ $user->update;
+
+ $mech->get_ok('/auth');
+ $mech->submit_form_ok(
+ { with_fields => { username => $test_email, password_sign_in => 'password' } },
+ "sign in using form" );
+
+ $mech->content_contains('requires two-factor');
+ $mech->submit_form_ok({ with_fields => { '2fa_action' => '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('successfully enabled two-factor authentication', "2FA activated");
+
+ $user->discard_changes();
+ my $user_token = $user->get_extra_metadata('2fa_secret');
+ is $token, $user_token, '2FA secret set';
+
+ $mech->logged_in_ok;
+ };
+};
+
+done_testing();
diff --git a/t/app/controller/auth_profile.t b/t/app/controller/auth_profile.t
index cc567544c..f68b64835 100644
--- a/t/app/controller/auth_profile.t
+++ b/t/app/controller/auth_profile.t
@@ -1,3 +1,10 @@
+package FixMyStreet::Cobrand::Dummy;
+use parent 'FixMyStreet::Cobrand::Default';
+
+sub must_have_2fa { 1 }
+
+package main;
+
use FixMyStreet::TestMech;
my $mech = FixMyStreet::TestMech->new;
@@ -10,10 +17,6 @@ my $test_email = 'test@example.com';
my $test_email2 = 'test@example.net';
my $test_password = 'foobar123';
-END {
- done_testing();
-}
-
# get a sign in email and change password
subtest "Test change password page" => sub {
$mech->clear_emails_ok;
@@ -425,6 +428,7 @@ subtest "Test generate token page" => sub {
$mech->log_out_ok;
$mech->add_header('Authorization', "Bearer $token");
$mech->logged_in_ok;
+ $mech->delete_header('Authorization');
};
subtest "Test two-factor authentication admin" => sub {
@@ -464,3 +468,40 @@ subtest "Test two-factor authentication admin" => sub {
$mech->content_contains('has been deactivated', "2FA deactivated");
}
};
+
+subtest "Test enforced two-factor authentication" => sub {
+ FixMyStreet::override_config {
+ ALLOWED_COBRANDS => 'dummy',
+ }, sub {
+ use Auth::GoogleAuth;
+ my $auth = Auth::GoogleAuth->new;
+ my $code = $auth->code;
+
+ # Sign in with 2FA
+ my $user = FixMyStreet::App->model('DB::User')->find( { email => $test_email } );
+ $user->password('password');
+ $user->set_extra_metadata('2fa_secret', $auth->secret32);
+ $user->update;
+
+ $mech->get_ok('/auth');
+ $mech->submit_form_ok(
+ { with_fields => { username => $test_email, password_sign_in => 'password' } },
+ "sign in using form" );
+ $mech->content_contains('Please generate a two-factor code');
+ $mech->submit_form_ok({ with_fields => { '2fa_code' => $code } }, "provide correct 2FA code" );
+
+ $mech->get_ok('/auth/generate_token');
+ $mech->content_contains('Change two-factor');
+ $mech->content_lacks('Deactivate two-factor');
+
+ my ($csrf) = $mech->content =~ /meta content="([^"]*)" name="csrf-token"/;
+ $mech->post_ok('/auth/generate_token', {
+ '2fa_deactivate' => 1,
+ 'token' => $csrf,
+ });
+ $mech->content_lacks('has been deactivated', "2FA not deactivated");
+ $mech->content_contains('Please scan this image', 'Change 2FA page shown instead');
+ };
+};
+
+done_testing();
diff --git a/t/app/controller/report_new.t b/t/app/controller/report_new.t
index 20eecb50e..48169c148 100644
--- a/t/app/controller/report_new.t
+++ b/t/app/controller/report_new.t
@@ -914,8 +914,9 @@ subtest "test password errors for a user who is signing in as they report" => su
};
foreach my $test (
- { two_factor => 0, desc => '', },
- { two_factor => 1, desc => ' with two-factor', },
+ { two_factor => '', desc => '', },
+ { two_factor => 'yes', desc => ' with two-factor', },
+ { two_factor => 'new', desc => ' with mandated two-factor, not yet set up', },
) {
subtest "test report creation for a user who is signing in as they report$test->{desc}" => sub {
$mech->log_out_ok;
@@ -932,21 +933,25 @@ foreach my $test (
name => 'Joe Bloggs',
phone => '01234 567 890',
password => 'secret2',
+ $test->{two_factor} ? (is_superuser => 1) : (),
} ), "set user details";
my $auth;
- if ($test->{two_factor}) {
+ my $mock;
+ if ($test->{two_factor} eq 'yes') {
use Auth::GoogleAuth;
$auth = Auth::GoogleAuth->new;
- $user->is_superuser(1);
$user->set_extra_metadata('2fa_secret', $auth->generate_secret32);
$user->update;
+ } elsif ($test->{two_factor} eq 'new') {
+ $mock = Test::MockModule->new('FixMyStreet::Cobrand::FixMyStreet');
+ $mock->mock(must_have_2fa => sub { 1 });
}
# submit initial pc form
$mech->get_ok('/around');
FixMyStreet::override_config {
- ALLOWED_COBRANDS => [ { fixmystreet => '.' } ],
+ ALLOWED_COBRANDS => 'fixmystreet',
MAPIT_URL => 'http://mapit.uk/',
}, sub {
$mech->submit_form_ok( { with_fields => { pc => 'EH1 1BB', } },
@@ -971,13 +976,23 @@ foreach my $test (
"submit good details"
);
- if ($test->{two_factor}) {
+ if ($test->{two_factor} eq 'yes') {
my $code = $auth->code;
my $wrong_code = $auth->code(undef, time() - 120);
$mech->content_contains('Please generate a two-factor code');
$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" );
+ } elsif ($test->{two_factor} eq 'new') {
+ $mech->content_contains('requires two-factor');
+ $mech->submit_form_ok({ with_fields => { '2fa_action' => '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;
+ print $mech->encoded_content;
+ $mech->submit_form_ok({ with_fields => { '2fa_code' => $code } }, "provide correct 2FA code" );
}
# check that we got the message expected
@@ -998,7 +1013,7 @@ foreach my $test (
my $report = $user->problems->first;
ok $report, "Found the report";
- if (!$test->{two_factor}) {
+ if ($test->{two_factor} eq '') {
# The superuser account will be immediately redirected
$mech->content_contains('Thank you for reporting this issue');
}
diff --git a/templates/web/base/auth/2fa/intro.html b/templates/web/base/auth/2fa/intro.html
new file mode 100644
index 000000000..85e623e7f
--- /dev/null
+++ b/templates/web/base/auth/2fa/intro.html
@@ -0,0 +1,32 @@
+[%
+INCLUDE 'header.html', title = loc('Two-factor authentication'), bodyclass = 'fullwidthpage'
+%]
+
+<div class="confirmation-header confirmation-header--phone">
+ <h1>[% loc('Two-factor authentication') %]</h1>
+
+ <form action="/auth" method="post">
+
+ [% IF stage == 'success' %]
+ <p>[% loc('Thanks, you have successfully enabled two-factor authentication on your account.') %]</p>
+ <p><a href="/my">[% loc('Your account') %]</a></p>
+
+ [% ELSIF stage == 'activate' %]
+ [% PROCESS 'auth/2fa/form-add.html' %]
+
+ [% ELSE # stage is intro %]
+ <p align="center">[% loc('Your account requires two-factor authentication to be set up.') %]</p>
+ <p align="center">
+ <input class="btn-primary" type="submit" value="[% loc('Activate two-factor authentication') %]">
+ </p>
+ <input type="hidden" name="2fa_action" value="activate">
+ [% END %]
+
+ <input type="hidden" name="username" value="[% c.get_param('username') | html %]">
+ <input type="hidden" name="password_sign_in" value="[% c.get_param('password_sign_in') | html %]">
+ <input type="hidden" name="r" value="[% c.get_param('r') | html %]">
+ <input type="hidden" name="token" value="[% token | html %]">
+ </form>
+</div>
+
+[% INCLUDE 'footer.html' %]
diff --git a/templates/web/base/auth/generate_token.html b/templates/web/base/auth/generate_token.html
index 6b8a63ea3..a24c7ccd2 100644
--- a/templates/web/base/auth/generate_token.html
+++ b/templates/web/base/auth/generate_token.html
@@ -59,7 +59,9 @@ INCLUDE 'header.html', title = loc('Security'), bodyclass = 'fullwidthpage'
[% IF c.user.is_superuser || c.user.from_body %]
[% 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') %]">
+ [% IF !c.cobrand.call_hook('must_have_2fa', c.user) %]
+ <input name="2fa_deactivate" type="submit" class="btn" value="[% loc('Deactivate two-factor authentication') %]">
+ [% END %]
[% ELSE %]
<input name="2fa_activate" type="submit" class="btn" value="[% loc('Activate two-factor authentication') %]">
[% END %]
generated by cgit v1.2.3 (git 2.25.1) at 2025-07-19 15:47:22 +0000