From 9e00579237e9fd423fa6c0d281329eaf030e6091 Mon Sep 17 00:00:00 2001 From: Matthew Somerville Date: Fri, 13 Sep 2024 15:23:18 +0100 Subject: [PATCH] [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();