diff --git a/perllib/FixMyStreet/App/Controller/Auth/Social.pm b/perllib/FixMyStreet/App/Controller/Auth/Social.pm index 59d65e65257..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,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}); @@ -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 diff --git a/perllib/FixMyStreet/Cobrand/HighwaysEngland.pm b/perllib/FixMyStreet/Cobrand/HighwaysEngland.pm index c4574782eb6..475449a3885 100644 --- a/perllib/FixMyStreet/Cobrand/HighwaysEngland.pm +++ b/perllib/FixMyStreet/Cobrand/HighwaysEngland.pm @@ -1,3 +1,16 @@ +=head1 NAME + +FixMyStreet::Cobrand::HighwaysEngland - code specific to the National Highways cobrand + +=head1 SYNOPSIS + +National Highways, previously Highways England, is the national roads +authority, and responsible for motorways and major roads in England. + +=head1 DESCRIPTION + +=cut + package FixMyStreet::Cobrand::HighwaysEngland; use parent 'FixMyStreet::Cobrand::UK'; @@ -5,12 +18,14 @@ use strict; use warnings; use utf8; use DateTime; +use JSON::MaybeXS; +use LWP::UserAgent; sub council_name { 'National Highways' } -sub council_url { 'highwaysengland' } +sub council_url { 'nationalhighways' } -sub site_key { 'highwaysengland' } +sub site_key { 'nationalhighways' } sub restriction { { cobrand => shift->moniker } } @@ -22,7 +37,12 @@ sub suggest_duplicates { 1 } sub all_reports_single_body { { name => 'National Highways' } } -# Copying of functions from UKCouncils that are needed here also - factor out to a role of some sort? +=over 4 + +=item * It is not a council, so inherits from UK, not UKCouncils, but a number of functions are shared with what councils do + +=cut + sub cut_off_date { '2020-11-09' } sub problems_restriction { FixMyStreet::Cobrand::UKCouncils::problems_restriction($_[0], $_[1]) } sub problems_on_map_restriction { $_[0]->problems_restriction($_[1]) } @@ -33,7 +53,10 @@ sub base_url { FixMyStreet::Cobrand::UKCouncils::base_url($_[0]) } sub contact_name { FixMyStreet::Cobrand::UKCouncils::contact_name($_[0]) } sub contact_email { FixMyStreet::Cobrand::UKCouncils::contact_email($_[0]) } -# Make sure any reports made when site was only fully anonymous remain anonymous +=item * Any report made when the site was only fully anonymous should remain anonymous + +=cut + my $non_anon = DateTime->new( year => 2022, month => 10, day => 5 ); sub munge_problem_list { @@ -56,6 +79,23 @@ sub admin_allow_user { return $user->from_body->get_column('name') eq 'National Highways'; } +=item * We reword a few admin permissions to be clearer + +=cut + +sub available_permissions { + my $self = shift; + my $perms = $self->next::method(); + $perms->{Problems}->{default_to_body} = "Default to creating reports/updates as " . $self->council_name; + $perms->{Problems}->{contribute_as_body} = "Create reports/updates as " . $self->council_name; + $perms->{Problems}->{view_body_contribute_details} = "See user detail for reports created as " . $self->council_name; + return $perms; +} + +=item * There is an extra question asking where you heard about the site + +=cut + sub report_form_extras { ( { name => 'where_hear' } ) } @@ -67,6 +107,10 @@ sub example_places { return $self->feature('example_places') || $self->next::method(); } +=item * Provide nicer help if it looks like they're searching for a road name + +=cut + sub geocode_postcode { my ( $self, $s ) = @_; @@ -79,6 +123,10 @@ sub geocode_postcode { return $self->next::method($s); } +=item * Allow lookup by FMSid + +=cut + sub lookup_by_ref_regex { return qr/^\s*((?:FMS\s*)?\d+)\s*$/i; } @@ -93,13 +141,11 @@ sub lookup_by_ref { return 0; } -sub allow_photo_upload { 0 } +=item * No photos -sub allow_anonymous_reports { 'button' } +=cut -sub admin_user_domain { ( 'highwaysengland.co.uk', 'nationalhighways.co.uk' ) } - -sub abuse_reports_only { 1 } +sub allow_photo_upload { 0 } # Bypass photo requirement, we have none sub recent_photos { @@ -108,6 +154,28 @@ sub recent_photos { return []; } +=item * Anonymous reporting is allowed + +=cut + +sub allow_anonymous_reports { 'button' } + +=item * Two domains for admin users + +=cut + +sub admin_user_domain { ( 'highwaysengland.co.uk', 'nationalhighways.co.uk' ) } + +=item * No contact form + +=cut + +sub abuse_reports_only { 1 } + +=item * Only works in England + +=cut + sub area_check { my ( $self, $params, $context ) = @_; @@ -148,6 +216,10 @@ sub new_report_detail_field_hint { "eg ‘This road sign has been obscured for two months and…’" } +=item * New reports are possibly redacted + +=cut + sub report_new_munge_after_insert { my ($self, $report) = @_; @@ -194,6 +266,56 @@ sub _redact { return $s; } +=back + +=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, $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); +} + +=head2 Report categories + + +There is special handling of NH body/contacts, to handle the fact litter is not +NH responsibility on most, but not all, NH roads; NH categories must end "(NH)" +(this is stripped for display). + +=cut + sub munge_report_new_bodies { my ($self, $bodies) = @_; # On the cobrand there is only the HE body @@ -298,6 +420,19 @@ sub _report_new_is_on_he_road_not_litter { return scalar @$features ? 0 : 1; } +=item * Only Admin roles can access the dashboard + +=cut + +sub dashboard_permission { + my $self = shift; + my $c = $self->{c}; + + my $admin = grep { $_->name eq 'Admin' } $c->user->obj->roles->all; + return 0 unless $admin; + return undef; +} + sub dashboard_export_problems_add_columns { my ($self, $csv) = @_; diff --git a/perllib/FixMyStreet/Cobrand/TfL.pm b/perllib/FixMyStreet/Cobrand/TfL.pm index f7f5a3f0f51..61a54dfb677 100644 --- a/perllib/FixMyStreet/Cobrand/TfL.pm +++ b/perllib/FixMyStreet/Cobrand/TfL.pm @@ -375,40 +375,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 efe66b9fdcc..b148de5fd07 100644 --- a/perllib/FixMyStreet/Cobrand/UK.pm +++ b/perllib/FixMyStreet/Cobrand/UK.pm @@ -634,4 +634,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/FixMyStreet/DB/Result/Comment.pm b/perllib/FixMyStreet/DB/Result/Comment.pm index cfdf35b79e0..8eb2a582013 100644 --- a/perllib/FixMyStreet/DB/Result/Comment.pm +++ b/perllib/FixMyStreet/DB/Result/Comment.pm @@ -310,6 +310,9 @@ sub meta_line { $body = 'Island Roads'; } elsif ($body eq 'Thamesmead') { $body = 'Peabody'; + } elsif ($body eq 'National Highways') { + # Always use what was saved on the comment + $body = FixMyStreet::Template::html_filter($self->name); } } my $cobrand_always_view_body_user = $cobrand->call_hook(always_view_body_contribute_details => $contributed_as); 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 476980269c9..98a5c46dee3 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,20 @@ 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)'); +my $staffuser = $mech->create_user_ok('counciluser@example.com', name => 'Council User', from_body => $highways, password => 'password'); + +FixMyStreet::DB->resultset("Role")->create({ + body => $highways, + name => 'Inspector', + permissions => ['moderate', 'user_edit'], +}); +my $role_admin = FixMyStreet::DB->resultset("Role")->create({ + body => $highways, + name => 'Admin', + permissions => ['moderate', 'user_edit'], +}); +$staffuser->add_to_roles($role_admin); + FixMyStreet::override_config { ALLOWED_COBRANDS => [ 'highwaysengland', 'fixmystreet' ], MAPIT_URL => 'http://mapit.uk/', @@ -205,8 +221,6 @@ subtest 'Dashboard CSV extra columns' => sub { cobrand => 'fixmystreet', }); - my $staffuser = $mech->create_user_ok('counciluser@example.com', name => 'Council User', - from_body => $highways, password => 'password'); $mech->log_in_ok( $staffuser->email ); my $now = DateTime->now; @@ -225,13 +239,13 @@ subtest 'Dashboard CSV extra columns' => sub { }; $mech->content_contains('URL","Device Type","Site Used","Reported As","User Email","User Phone","Area name","Road name","Section label","How you found us","Update 1","Update 1 date","Update 1 name","Update 2","Update 2 date","Update 2 name"'); my @row1 = ( - 'http://highwaysengland.example.org/report/' . $problem1->id, + 'http://nationalhighways.example.org/report/' . $problem1->id, 'desktop', 'highwaysengland', '', $problem1->user->email, '', '"South West"', 'M5', '', '"Social media"', '"This is an update"', $comment1->confirmed->datetime, '"Council User"', '"Second update"', $comment2->confirmed->datetime, 'public', ); $mech->content_contains(join ',', @row1); - $mech->content_contains('http://highwaysengland.example.org/report/' . $problem2->id .',mobile,fixmystreet,,' . $problem2->user->email . ',,"Area 7",,M1/111,"Search engine"'); + $mech->content_contains('http://nationalhighways.example.org/report/' . $problem2->id .',mobile,fixmystreet,,' . $problem2->user->email . ',,"Area 7",,M1/111,"Search engine"'); FixMyStreet::override_config { MAPIT_URL => 'http://mapit.uk/', @@ -243,20 +257,32 @@ subtest 'Dashboard CSV extra columns' => sub { }; $mech->content_contains('URL","Device Type","Site Used","Reported As","User Email","User Phone","Area name","Road name","Section label","How you found us","Update 1","Update 1 date","Update 1 name","Update 2","Update 2 date","Update 2 name"'); @row1 = ( - 'http://highwaysengland.example.org/report/' . $problem1->id, + 'http://nationalhighways.example.org/report/' . $problem1->id, 'desktop', 'highwaysengland', '', $problem1->user->email, '', '"South West"', 'M5', '', '"Social media"', '"This is an update"', $comment1->confirmed->datetime, '"Council User"', '"Second update"', $comment2->confirmed->datetime, 'public', ); $mech->content_contains(join ',', @row1); - $mech->content_contains('http://highwaysengland.example.org/report/' . $problem2->id .',mobile,fixmystreet,,' . $problem2->user->email . ',,"Area 7",,M1/111,"Search engine"'); + $mech->content_contains('http://nationalhighways.example.org/report/' . $problem2->id .',mobile,fixmystreet,,' . $problem2->user->email . ',,"Area 7",,M1/111,"Search engine"'); + $comment1->delete; + $comment2->delete; }; FixMyStreet::override_config { ALLOWED_COBRANDS => [ 'highwaysengland' ], MAPIT_URL => 'http://mapit.uk/', }, sub { + subtest 'update message' => sub { + my $report = FixMyStreet::DB->resultset("Problem")->first; + $mech->create_comment_for_problem($report, $staffuser, 'Staff user', 'Normal update', 'f', 'confirmed', 'confirmed'); + $mech->create_comment_for_problem($report, $staffuser, 'National Highways', 'Body update', 'f', 'confirmed', 'confirmed', { extra => { contributed_as => 'body' } }); + $mech->get_ok('/report/' . $report->id); + my $metas = $mech->extract_update_metas; + like $metas->[0], qr/Posted by Staff user at/; + like $metas->[1], qr/Posted by National Highways at/; + }; + subtest 'Categories must end with (NH)' => sub { my $superuser = $mech->create_user_ok('super@example.com', name => 'Admin', from_body => $highways, password => 'password', is_superuser => 1); @@ -302,4 +328,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(); diff --git a/t/cobrand/tfl.t b/t/cobrand/tfl.t index c340aacda15..4a0df828135 100644 --- a/t/cobrand/tfl.t +++ b/t/cobrand/tfl.t @@ -983,7 +983,7 @@ for my $host ( 'www.fixmystreet.com', 'tfl.fixmystreet.com' ) { subtest 'TfL staff can access TfL admin' => sub { $mech->log_in_ok( $staffuser->email ); $mech->get_ok('/admin'); - $mech->content_contains( 'Search Reports' ); + $mech->content_contains( '

Summary

' ); }; subtest 'TLRN categories cannot be renamed' => sub { @@ -1439,7 +1439,7 @@ FixMyStreet::override_config { subtest 'Bromley staff can access Bromley admin' => sub { $mech->log_in_ok( $bromleyuser->email ); $mech->get_ok('/admin'); - $mech->content_contains( 'Search Reports' ); + $mech->content_contains( '

Summary

' ); $mech->log_out_ok; }; diff --git a/templates/web/base/admin/index.html b/templates/web/base/admin/index.html index 11031b26cc4..ef8b3f2e816 100644 --- a/templates/web/base/admin/index.html +++ b/templates/web/base/admin/index.html @@ -13,6 +13,7 @@