Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[National Highways] Initialise single-sign on #5114

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 13 additions & 4 deletions perllib/FixMyStreet/App/Controller/Auth/Social.pm
Original file line number Diff line number Diff line change
Expand Up @@ -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')
);
Expand All @@ -303,12 +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');
}

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;
}

# 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});
Expand All @@ -333,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
Expand Down
41 changes: 41 additions & 0 deletions perllib/FixMyStreet/Cobrand/HighwaysEngland.pm
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@
use warnings;
use utf8;
use DateTime;
use JSON::MaybeXS;
use LWP::UserAgent;

sub council_name { 'National Highways' }

Expand Down Expand Up @@ -194,6 +196,45 @@
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;

Check warning on line 210 in perllib/FixMyStreet/Cobrand/HighwaysEngland.pm

View check run for this annotation

Codecov / codecov/patch

perllib/FixMyStreet/Cobrand/HighwaysEngland.pm#L210

Added line #L210 was not covered by tests

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, $access_token) = @_;

Check warning on line 220 in perllib/FixMyStreet/Cobrand/HighwaysEngland.pm

View check run for this annotation

Codecov / codecov/patch

perllib/FixMyStreet/Cobrand/HighwaysEngland.pm#L220

Added line #L220 was not covered by tests

my $name = $payload->{name} ? $payload->{name} : '';
my $email = $payload->{email} ? lc($payload->{email}) : '';

if ($payload->{oid} && $access_token) {
my $ua = LWP::UserAgent->new;

Check warning on line 226 in perllib/FixMyStreet/Cobrand/HighwaysEngland.pm

View check run for this annotation

Codecov / codecov/patch

perllib/FixMyStreet/Cobrand/HighwaysEngland.pm#L226

Added line #L226 was not covered by tests
my $response = $ua->get(
'https://graph.microsoft.com/v1.0/users/' . $payload->{oid} . '?$select=displayName,department',

Check warning on line 228 in perllib/FixMyStreet/Cobrand/HighwaysEngland.pm

View check run for this annotation

Codecov / codecov/patch

perllib/FixMyStreet/Cobrand/HighwaysEngland.pm#L228

Added line #L228 was not covered by tests
Authorization => 'Bearer ' . $access_token,
);
my $user = decode_json($response->decoded_content);

Check warning on line 231 in perllib/FixMyStreet/Cobrand/HighwaysEngland.pm

View check run for this annotation

Codecov / codecov/patch

perllib/FixMyStreet/Cobrand/HighwaysEngland.pm#L231

Added line #L231 was not covered by tests
$payload->{roles} = [ $user->{department} ] if $user->{department};
}

return ($name, $email);

Check warning on line 235 in perllib/FixMyStreet/Cobrand/HighwaysEngland.pm

View check run for this annotation

Codecov / codecov/patch

perllib/FixMyStreet/Cobrand/HighwaysEngland.pm#L235

Added line #L235 was not covered by tests
}

sub munge_report_new_bodies {
my ($self, $bodies) = @_;
# On the cobrand there is only the HE body
Expand Down
34 changes: 0 additions & 34 deletions perllib/FixMyStreet/Cobrand/TfL.pm
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
34 changes: 34 additions & 0 deletions perllib/FixMyStreet/Cobrand/UK.pm
Original file line number Diff line number Diff line change
Expand Up @@ -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;
5 changes: 4 additions & 1 deletion perllib/OIDC/Lite/Client/IDTokenResponseParser.pm
Original file line number Diff line number Diff line change
Expand Up @@ -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 {

Expand Down
18 changes: 18 additions & 0 deletions t/Mock/MicrosoftGraph.pm
Original file line number Diff line number Diff line change
@@ -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;
6 changes: 6 additions & 0 deletions t/Mock/OpenIDConnect.pm
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,11 @@ sub dispatch_request {
$payload->{family_name} = "Dwyer";
$payload->{email} = '[email protected]' if $self->returns_email;
$payload->{roles} = $self->roles;
} elsif ($self->cobrand eq 'highwaysengland') {
$payload->{given_name} = "Andy";
$payload->{family_name} = "Dwyer";
$payload->{email} = '[email protected]' if $self->returns_email;
$payload->{oid} = 'OID-OID-OID';
}
my $signature = "dummy";
my $id_token = join(".", (
Expand All @@ -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,
Expand Down
58 changes: 58 additions & 0 deletions t/cobrand/highwaysengland.t
Original file line number Diff line number Diff line change
Expand Up @@ -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'); });
Expand Down Expand Up @@ -47,6 +49,12 @@ my $highways = $mech->create_body_ok(164186, 'National Highways', { send_method

$mech->create_contact_ok(email => '[email protected]', 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/',
Expand Down Expand Up @@ -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('[email protected]');
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();
Loading