From eb85622c2e1a0bc21bf80dfee5791a02b1f790ee Mon Sep 17 00:00:00 2001
From: Moray Jones
Date: Thu, 15 Aug 2024 09:32:13 +0100
Subject: [PATCH 1/5] [Highways England] Initialise single-sign on
Initial setup for single sign on
---
.../FixMyStreet/App/Controller/Auth/Social.pm | 6 +++-
.../FixMyStreet/Cobrand/HighwaysEngland.pm | 29 +++++++++++++++++++
2 files changed, 34 insertions(+), 1 deletion(-)
diff --git a/perllib/FixMyStreet/App/Controller/Auth/Social.pm b/perllib/FixMyStreet/App/Controller/Auth/Social.pm
index 59d65e65257..a93386bae9d 100644
--- a/perllib/FixMyStreet/App/Controller/Auth/Social.pm
+++ b/perllib/FixMyStreet/App/Controller/Auth/Social.pm
@@ -308,7 +308,11 @@ sub oidc_callback: Path('/auth/OIDC') : Args(0) {
$c->log->info("Social::oidc_callback no id_token: " . $oidc->{last_response}->{_content});
$c->detach('oauth_failure');
}
-
+ my $message = '';
+ for my $key (sort keys %{$id_token->payload}) {
+ $message .= $key . " : " . $id_token->payload->{$key} . "\n" if $id_token->payload->{$key};
+ }
+ $c->log->debug($message) if $message;
# sanity check the token audience is us...
unless ($id_token->payload->{aud} eq $c->forward('oidc_config')->{client_id}) {
$c->log->info("Social::oidc_callback invalid id_token: expected aud to be " . $c->forward('oidc_config')->{client_id} . " but it was " . $id_token->payload->{aud});
diff --git a/perllib/FixMyStreet/Cobrand/HighwaysEngland.pm b/perllib/FixMyStreet/Cobrand/HighwaysEngland.pm
index 95b855b7bda..d4e9b44673a 100644
--- a/perllib/FixMyStreet/Cobrand/HighwaysEngland.pm
+++ b/perllib/FixMyStreet/Cobrand/HighwaysEngland.pm
@@ -194,6 +194,35 @@ sub _redact {
return $s;
}
+=head1 OIDC single sign on
+
+Noational Highways has a single-sign on option
+
+=over 4
+
+=item * Single sign on is enabled if the configuration is set up
+
+=cut
+
+sub social_auth_enabled {
+ my $self = shift;
+
+ return $self->feature('oidc_login') ? 1 : 0;
+}
+
+=item * Different single sign-ons send user details differently, user_from_oidc extracts the relevant parts
+
+=cut
+
+sub user_from_oidc {
+ my ($self, $payload) = @_;
+
+ my $name = join(" ", $payload->{given_name}, $payload->{family_name});
+ my $email = $payload->{email} ? lc($payload->{email}) : '';
+
+ return ($name, $email);
+}
+
sub munge_report_new_bodies {
my ($self, $bodies) = @_;
# On the cobrand there is only the HE body
From 8722785b83388d8ab3b4caa4c6e6ba2a38694f58 Mon Sep 17 00:00:00 2001
From: Moray Jones
Date: Fri, 16 Aug 2024 12:03:50 +0100
Subject: [PATCH 2/5] fixup! [Highways England] Initialise single-sign on
---
perllib/FixMyStreet/App/Controller/Auth/Social.pm | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/perllib/FixMyStreet/App/Controller/Auth/Social.pm b/perllib/FixMyStreet/App/Controller/Auth/Social.pm
index a93386bae9d..b03a8217930 100644
--- a/perllib/FixMyStreet/App/Controller/Auth/Social.pm
+++ b/perllib/FixMyStreet/App/Controller/Auth/Social.pm
@@ -312,7 +312,7 @@ sub oidc_callback: Path('/auth/OIDC') : Args(0) {
for my $key (sort keys %{$id_token->payload}) {
$message .= $key . " : " . $id_token->payload->{$key} . "\n" if $id_token->payload->{$key};
}
- $c->log->debug($message) if $message;
+ $c->log->info($message) if $message;
# sanity check the token audience is us...
unless ($id_token->payload->{aud} eq $c->forward('oidc_config')->{client_id}) {
$c->log->info("Social::oidc_callback invalid id_token: expected aud to be " . $c->forward('oidc_config')->{client_id} . " but it was " . $id_token->payload->{aud});
From aaea3a8e378a2298ee588a90f027733f08645076 Mon Sep 17 00:00:00 2001
From: Moray Jones
Date: Fri, 16 Aug 2024 15:24:23 +0100
Subject: [PATCH 3/5] fixup! [Highways England] Initialise single-sign on
---
perllib/FixMyStreet/Cobrand/HighwaysEngland.pm | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/perllib/FixMyStreet/Cobrand/HighwaysEngland.pm b/perllib/FixMyStreet/Cobrand/HighwaysEngland.pm
index d4e9b44673a..8d125135291 100644
--- a/perllib/FixMyStreet/Cobrand/HighwaysEngland.pm
+++ b/perllib/FixMyStreet/Cobrand/HighwaysEngland.pm
@@ -217,7 +217,7 @@ sub social_auth_enabled {
sub user_from_oidc {
my ($self, $payload) = @_;
- my $name = join(" ", $payload->{given_name}, $payload->{family_name});
+ my $name = $payload->{name} ? $payload->{name} : '';
my $email = $payload->{email} ? lc($payload->{email}) : '';
return ($name, $email);
From 9e00579237e9fd423fa6c0d281329eaf030e6091 Mon Sep 17 00:00:00 2001
From: Matthew Somerville
Date: Fri, 13 Sep 2024 15:23:18 +0100
Subject: [PATCH 4/5] [NH] Look up department in Entra.
---
.../FixMyStreet/App/Controller/Auth/Social.pm | 21 ++++---
.../FixMyStreet/Cobrand/HighwaysEngland.pm | 14 ++++-
perllib/FixMyStreet/Cobrand/TfL.pm | 34 -----------
perllib/FixMyStreet/Cobrand/UK.pm | 34 +++++++++++
.../OIDC/Lite/Client/IDTokenResponseParser.pm | 5 +-
t/Mock/MicrosoftGraph.pm | 18 ++++++
t/Mock/OpenIDConnect.pm | 6 ++
t/cobrand/highwaysengland.t | 58 +++++++++++++++++++
8 files changed, 146 insertions(+), 44 deletions(-)
create mode 100644 t/Mock/MicrosoftGraph.pm
diff --git a/perllib/FixMyStreet/App/Controller/Auth/Social.pm b/perllib/FixMyStreet/App/Controller/Auth/Social.pm
index b03a8217930..7e5694b7dc9 100644
--- a/perllib/FixMyStreet/App/Controller/Auth/Social.pm
+++ b/perllib/FixMyStreet/App/Controller/Auth/Social.pm
@@ -292,9 +292,8 @@ sub oidc_callback: Path('/auth/OIDC') : Args(0) {
# The only other valid state param is 'login' at this point.
$c->detach('/page_error_400_bad_request', []) unless $c->get_param('state') eq 'login';
- my $id_token;
- eval {
- $id_token = $oidc->get_access_token(
+ my $token = eval {
+ $oidc->get_access_token(
code => $c->get_param('code'),
redirect_uri => $c->uri_for('/auth/OIDC')
);
@@ -303,16 +302,22 @@ sub oidc_callback: Path('/auth/OIDC') : Args(0) {
(my $message = $@) =~ s/at [^ ]*Auth.pm.*//;
$c->detach('/page_error_500_internal_error', [ $message ]);
}
+ my $id_token = $token->{id_token};
+ my $access_token = $token->{access_token};
if (!$id_token) {
$c->log->info("Social::oidc_callback no id_token: " . $oidc->{last_response}->{_content});
$c->detach('oauth_failure');
}
- my $message = '';
- for my $key (sort keys %{$id_token->payload}) {
- $message .= $key . " : " . $id_token->payload->{$key} . "\n" if $id_token->payload->{$key};
+
+ if (FixMyStreet->config('STAGING_SITE')) {
+ my $message = '';
+ for my $key (sort keys %{$id_token->payload}) {
+ $message .= $key . " : " . $id_token->payload->{$key} . "\n" if $id_token->payload->{$key};
+ }
+ $c->log->info($message) if $message;
}
- $c->log->info($message) if $message;
+
# sanity check the token audience is us...
unless ($id_token->payload->{aud} eq $c->forward('oidc_config')->{client_id}) {
$c->log->info("Social::oidc_callback invalid id_token: expected aud to be " . $c->forward('oidc_config')->{client_id} . " but it was " . $id_token->payload->{aud});
@@ -337,7 +342,7 @@ sub oidc_callback: Path('/auth/OIDC') : Args(0) {
$c->session->{oauth}{id_token} = $id_token->token_string;
# Cobrands can use different fields for name and email
- my ($name, $email) = $c->cobrand->call_hook(user_from_oidc => $id_token->payload);
+ my ($name, $email) = $c->cobrand->call_hook(user_from_oidc => $id_token->payload, $access_token);
$name = '' if $name && $name !~ /\w/;
# There's a chance that a user may have multiple OIDC logins, so build a namespaced uid to prevent collisions
diff --git a/perllib/FixMyStreet/Cobrand/HighwaysEngland.pm b/perllib/FixMyStreet/Cobrand/HighwaysEngland.pm
index 8d125135291..617e5a4fd91 100644
--- a/perllib/FixMyStreet/Cobrand/HighwaysEngland.pm
+++ b/perllib/FixMyStreet/Cobrand/HighwaysEngland.pm
@@ -5,6 +5,8 @@ use strict;
use warnings;
use utf8;
use DateTime;
+use JSON::MaybeXS;
+use LWP::UserAgent;
sub council_name { 'National Highways' }
@@ -215,11 +217,21 @@ sub social_auth_enabled {
=cut
sub user_from_oidc {
- my ($self, $payload) = @_;
+ my ($self, $payload, $access_token) = @_;
my $name = $payload->{name} ? $payload->{name} : '';
my $email = $payload->{email} ? lc($payload->{email}) : '';
+ if ($payload->{oid} && $access_token) {
+ my $ua = LWP::UserAgent->new;
+ my $response = $ua->get(
+ 'https://graph.microsoft.com/v1.0/users/' . $payload->{oid} . '?$select=displayName,department',
+ Authorization => 'Bearer ' . $access_token,
+ );
+ my $user = decode_json($response->decoded_content);
+ $payload->{roles} = [ $user->{department} ] if $user->{department};
+ }
+
return ($name, $email);
}
diff --git a/perllib/FixMyStreet/Cobrand/TfL.pm b/perllib/FixMyStreet/Cobrand/TfL.pm
index ba3a45b3d0f..dd8041e3466 100644
--- a/perllib/FixMyStreet/Cobrand/TfL.pm
+++ b/perllib/FixMyStreet/Cobrand/TfL.pm
@@ -366,40 +366,6 @@ sub user_from_oidc {
return ($name, $email);
}
-=item * TfL sends the user role in the single sign-on payload, which we use to set the FMS role
-
-=cut
-
-sub roles_from_oidc {
- my ($self, $user, $roles) = @_;
-
- return unless $roles && @$roles;
-
- $user->user_roles->delete;
- $user->from_body($self->body->id);
-
- my $cfg = $self->feature('oidc_login') || {};
- my $role_map = $cfg->{role_map} || {};
-
- my @body_roles;
- for ($user->from_body->roles->order_by('name')->all) {
- push @body_roles, {
- id => $_->id,
- name => $_->name,
- }
- }
-
- for my $assign_role (@$roles) {
- my ($body_role) = grep { $role_map->{$assign_role} && $_->{name} eq $role_map->{$assign_role} } @body_roles;
-
- if ($body_role) {
- $user->user_roles->find_or_create({
- role_id => $body_role->{id},
- });
- }
- }
-}
-
sub state_groups_inspect {
my $rs = FixMyStreet::DB->resultset("State");
my @open = grep { $_ !~ /^(planned|investigating|for triage)$/ } FixMyStreet::DB::Result::Problem->open_states;
diff --git a/perllib/FixMyStreet/Cobrand/UK.pm b/perllib/FixMyStreet/Cobrand/UK.pm
index 72bdf8a5d29..a034506fee2 100644
--- a/perllib/FixMyStreet/Cobrand/UK.pm
+++ b/perllib/FixMyStreet/Cobrand/UK.pm
@@ -647,4 +647,38 @@ sub _email_to_body {
return $body;
}
+=item * Some OIDC users send the user role in the single sign-on payload, which we use to set the FMS role
+
+=cut
+
+sub roles_from_oidc {
+ my ($self, $user, $roles) = @_;
+
+ return unless $roles && @$roles;
+
+ $user->user_roles->delete;
+ $user->from_body($self->body->id);
+
+ my $cfg = $self->feature('oidc_login') || {};
+ my $role_map = $cfg->{role_map} || {};
+
+ my @body_roles;
+ for ($user->from_body->roles->order_by('name')->all) {
+ push @body_roles, {
+ id => $_->id,
+ name => $_->name,
+ }
+ }
+
+ for my $assign_role (@$roles) {
+ my ($body_role) = grep { $role_map->{$assign_role} && $_->{name} eq $role_map->{$assign_role} } @body_roles;
+
+ if ($body_role) {
+ $user->user_roles->find_or_create({
+ role_id => $body_role->{id},
+ });
+ }
+ }
+}
+
1;
diff --git a/perllib/OIDC/Lite/Client/IDTokenResponseParser.pm b/perllib/OIDC/Lite/Client/IDTokenResponseParser.pm
index 41db1bbdbde..6216239f889 100644
--- a/perllib/OIDC/Lite/Client/IDTokenResponseParser.pm
+++ b/perllib/OIDC/Lite/Client/IDTokenResponseParser.pm
@@ -50,7 +50,10 @@ sub parse {
message => sprintf("Response doesn't include 'id_token'")
) unless exists $result->{id_token};
- $token = OIDC::Lite::Model::IDToken->load($result->{id_token});
+ $token = {
+ id_token => OIDC::Lite::Model::IDToken->load($result->{id_token}),
+ access_token => $result->{access_token},
+ };
} else {
diff --git a/t/Mock/MicrosoftGraph.pm b/t/Mock/MicrosoftGraph.pm
new file mode 100644
index 00000000000..513c300050a
--- /dev/null
+++ b/t/Mock/MicrosoftGraph.pm
@@ -0,0 +1,18 @@
+package t::Mock::MicrosoftGraph;
+
+use Web::Simple;
+
+sub dispatch_request {
+ my $self = shift;
+
+ sub (GET + /v1.0/users/*) {
+ my ($self, $oid) = @_;
+ my $json = '{"department":"Department","displayName":"Name"}';
+ return [ 200, [ 'Content-Type' => 'application/json' ], [ $json ] ];
+ },
+
+}
+
+LWP::Protocol::PSGI->register(t::Mock::MicrosoftGraph->to_psgi_app, host => 'graph.microsoft.com');
+
+__PACKAGE__->run_if_script;
diff --git a/t/Mock/OpenIDConnect.pm b/t/Mock/OpenIDConnect.pm
index 41ea9598a5e..f7409eaca06 100644
--- a/t/Mock/OpenIDConnect.pm
+++ b/t/Mock/OpenIDConnect.pm
@@ -89,6 +89,11 @@ sub dispatch_request {
$payload->{family_name} = "Dwyer";
$payload->{email} = 'Pkg-tappcontrollerauth_socialt-oidc@TFL.gov.uk' if $self->returns_email;
$payload->{roles} = $self->roles;
+ } elsif ($self->cobrand eq 'highwaysengland') {
+ $payload->{given_name} = "Andy";
+ $payload->{family_name} = "Dwyer";
+ $payload->{email} = 'pkg-tcobrandhighwaysenglandt-oidc@nationalhighways.example.org' if $self->returns_email;
+ $payload->{oid} = 'OID-OID-OID';
}
my $signature = "dummy";
my $id_token = join(".", (
@@ -97,6 +102,7 @@ sub dispatch_request {
encode_base64($signature, '')
));
my $data = {
+ access_token => 'AccessToken',
id_token => $id_token,
token_type => "Bearer",
not_before => $now,
diff --git a/t/cobrand/highwaysengland.t b/t/cobrand/highwaysengland.t
index 8fea5a3025e..9673170d4d5 100644
--- a/t/cobrand/highwaysengland.t
+++ b/t/cobrand/highwaysengland.t
@@ -7,6 +7,8 @@ use HighwaysEngland;
use DateTime;
use File::Temp 'tempdir';
use Test::MockModule;
+use t::Mock::OpenIDConnect;
+use t::Mock::MicrosoftGraph;
my $he_mock = Test::MockModule->new('HighwaysEngland');
$he_mock->mock('database_file', sub { FixMyStreet->path_to('t/geocode/roads.sqlite'); });
@@ -47,6 +49,12 @@ my $highways = $mech->create_body_ok(164186, 'National Highways', { send_method
$mech->create_contact_ok(email => 'highways@example.com', body_id => $highways->id, category => 'Pothole (NH)');
+FixMyStreet::DB->resultset("Role")->create({
+ body => $highways,
+ name => 'Inspector',
+ permissions => ['moderate', 'user_edit'],
+});
+
FixMyStreet::override_config {
ALLOWED_COBRANDS => [ 'highwaysengland', 'fixmystreet' ],
MAPIT_URL => 'http://mapit.uk/',
@@ -302,4 +310,54 @@ FixMyStreet::override_config {
};
};
+FixMyStreet::override_config {
+ ALLOWED_COBRANDS => 'highwaysengland',
+ MAPIT_URL => 'http://mapit.uk/',
+ COBRAND_FEATURES => {
+ oidc_login => {
+ highwaysengland => {
+ client_id => 'example_client_id',
+ secret => 'example_secret_key',
+ auth_uri => 'http://oidc.example.org/oauth2/v2.0/authorize',
+ token_uri => 'http://oidc.example.org/oauth2/v2.0/token',
+ logout_uri => 'http://oidc.example.org/oauth2/v2.0/logout',
+ display_name => 'MyAccount',
+ role_map => {
+ 'Department' => 'Inspector',
+ },
+ }
+ }
+ }
+}, sub {
+ subtest 'National Highways department lookup' => sub {
+ my $email = $mech->uniquify_email('oidc@nationalhighways.example.org');
+ my $redirect_pattern = qr{oidc\.example\.org/oauth2/v2\.0/authorize};
+
+ $mech->log_out_ok;
+ $mech->cookie_jar({});
+ $mech->host('oidc.example.org');
+
+ my $resolver = Test::MockModule->new('Email::Valid');
+ $resolver->mock('address', sub { $email });
+ my $social = Test::MockModule->new('FixMyStreet::App::Controller::Auth::Social');
+ $social->mock('generate_nonce', sub { 'MyAwesomeRandomValue' });
+
+ my $mock_api = t::Mock::OpenIDConnect->new( host => 'oidc.example.org' );
+ LWP::Protocol::PSGI->register($mock_api->to_psgi_app, host => 'oidc.example.org');
+ $mock_api->cobrand('highwaysengland');
+
+ $mech->get_ok('/my');
+ $mech->submit_form(button => 'social_sign_in');
+ is $mech->res->previous->code, 302, "button redirected";
+ like $mech->res->previous->header('Location'), $redirect_pattern, "redirect to oauth URL";
+
+ $mech->get_ok('/auth/OIDC?code=response-code&state=login');
+ is $mech->uri->path, '/my', 'Successfully on /my page';
+
+ my $user = FixMyStreet::DB->resultset( 'User' )->find( { email => $email } );
+ my @roles = sort map { $_->name } $user->roles->all;
+ is_deeply ['Inspector'], \@roles, 'Correct roles assigned to user';
+ };
+};
+
done_testing();
From 5c5a381d9b9974c96f57edbe176fc91e8d1c1eca Mon Sep 17 00:00:00 2001
From: Matthew Somerville
Date: Thu, 19 Sep 2024 11:26:42 +0100
Subject: [PATCH 5/5] Make SSO button use separate form.
This reduces confusion if a browser auto-fills the email form but
someone still clicks the SSO button, or similar.
---
perllib/FixMyStreet/App/Controller/Auth.pm | 5 +-
t/app/controller/auth_social.t | 1 +
templates/web/base/auth/general.html | 53 ++++++++++------------
templates/web/brent/errors/generic.html | 2 +-
templates/web/camden/auth/general.html | 18 ++++----
templates/web/hackney/auth/general.html | 18 ++++----
templates/web/tfl/errors/generic.html | 2 +-
7 files changed, 47 insertions(+), 52 deletions(-)
diff --git a/perllib/FixMyStreet/App/Controller/Auth.pm b/perllib/FixMyStreet/App/Controller/Auth.pm
index 79eb29aba4c..052e97630ba 100644
--- a/perllib/FixMyStreet/App/Controller/Auth.pm
+++ b/perllib/FixMyStreet/App/Controller/Auth.pm
@@ -44,10 +44,7 @@ sub general : Path : Args(0) {
# decide which action to take
$c->detach('code_sign_in') if $clicked_sign_in_by_code || ($data_email && !$data_password);
- if (!$data_username && !$data_password && !$data_email && $c->get_param('social_sign_in')) {
- $c->forward('social/handle_sign_in');
- }
-
+ $c->detach('social/handle_sign_in') if $c->get_param('social_sign_in');
$c->forward( 'sign_in', [ $data_username ] )
&& $c->detach( 'redirect_on_signin', [ $c->get_param('r') ] );
diff --git a/t/app/controller/auth_social.t b/t/app/controller/auth_social.t
index 4211e560548..b40ceca29b9 100644
--- a/t/app/controller/auth_social.t
+++ b/t/app/controller/auth_social.t
@@ -143,6 +143,7 @@ for my $state ( 'refused', 'no email', 'existing UID', 'okay' ) {
update => 'Test update',
};
}
+ $mech->form_with_fields('social_sign_in');
$mech->submit_form(with_fields => $fields, button => 'social_sign_in');
# As well as the cookie issue above, caused by this external
diff --git a/templates/web/base/auth/general.html b/templates/web/base/auth/general.html
index 89d2c3d427c..c32b386e091 100644
--- a/templates/web/base/auth/general.html
+++ b/templates/web/base/auth/general.html
@@ -18,14 +18,12 @@
[% loc('Sorry, we could not log you in. Please fill in the form below.') %]
[% END %]
-
+[% IF c.cobrand.feature('oidc_login') AND NOT oauth_need_email %]
+
+
+
+
+[% END %]
+
[% INCLUDE 'footer.html' %]
[% BLOCK form_sign_in_yes %]
@@ -76,11 +84,3 @@
[%~ END ~%]
">
[% END %]
-
-[% BLOCK form_sign_in_camden_staff %]
- [% IF c.cobrand.feature('oidc_login') %]
-
- [% END %]
-[% END %]
diff --git a/templates/web/hackney/auth/general.html b/templates/web/hackney/auth/general.html
index 791bf47569e..344005a38b7 100644
--- a/templates/web/hackney/auth/general.html
+++ b/templates/web/hackney/auth/general.html
@@ -44,11 +44,19 @@
[% ELSE %]
[% INCLUDE form_sign_in_yes %]
[% INCLUDE form_sign_in_no %]
- [% INCLUDE form_sign_in_hackney_staff %]
[% END %]
+[% IF c.cobrand.feature('oidc_login') AND NOT oauth_need_email %]
+
+
+
+
+[% END %]
+
[% INCLUDE 'footer.html' %]
[% BLOCK form_sign_in_yes %]
@@ -78,11 +86,3 @@
[%~ END ~%]
">
[% END %]
-
-[% BLOCK form_sign_in_hackney_staff %]
- [% IF c.cobrand.feature('oidc_login') %]
-
- [% END %]
-[% END %]
diff --git a/templates/web/tfl/errors/generic.html b/templates/web/tfl/errors/generic.html
index f1931d05fd7..23961ebbc30 100755
--- a/templates/web/tfl/errors/generic.html
+++ b/templates/web/tfl/errors/generic.html
@@ -25,7 +25,7 @@