From 2b616588649e7ccdc158ded026470fad11fceb56 Mon Sep 17 00:00:00 2001 From: Richard Lynch Date: Fri, 11 Oct 2024 14:01:12 +0100 Subject: [PATCH 1/2] Show email as fallback There's an issue where the name fields are not being populated in the DSI oauth payload, as a temporary fix display the email address of the DFE sign in user who verified the claim --- .../further_education_payments/admin_tasks_presenter.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/policies/further_education_payments/admin_tasks_presenter.rb b/app/models/policies/further_education_payments/admin_tasks_presenter.rb index a6101ff37b..85d322c3db 100644 --- a/app/models/policies/further_education_payments/admin_tasks_presenter.rb +++ b/app/models/policies/further_education_payments/admin_tasks_presenter.rb @@ -14,7 +14,7 @@ def provider_verification end def provider_name - [verifier.fetch("first_name"), verifier.fetch("last_name")].join(" ") + [verifier.fetch("first_name"), verifier.fetch("last_name")].join(" ").presence || verifier.fetch("email") end def provider_verification_submitted? From dec1fcf8a842bc6e6428cb1f3b1041b995e6171e Mon Sep 17 00:00:00 2001 From: Richard Lynch Date: Fri, 11 Oct 2024 15:02:12 +0100 Subject: [PATCH 2/2] Handle missing info details The omniauth hash from DfE sign in is not populating the first name and last name fields. We require these fields when displaying who verified a task. This commit adds a fallback to request this information from the DSI api, if it's not present in the oauth payload. --- .../provider/omniauth_callback_form.rb | 35 ++- .../provider_verifying_claims_spec.rb | 8 + .../provider/omniauth_callback_form_spec.rb | 269 +++++++++++++++--- 3 files changed, 272 insertions(+), 40 deletions(-) diff --git a/app/forms/journeys/further_education_payments/provider/omniauth_callback_form.rb b/app/forms/journeys/further_education_payments/provider/omniauth_callback_form.rb index 550a4eb544..35a3000a29 100644 --- a/app/forms/journeys/further_education_payments/provider/omniauth_callback_form.rb +++ b/app/forms/journeys/further_education_payments/provider/omniauth_callback_form.rb @@ -2,6 +2,8 @@ module Journeys module FurtherEducationPayments module Provider class OmniauthCallbackForm + include DfeSignIn::Utils + def initialize(journey_session:, auth:) @journey_session = journey_session @auth = auth @@ -65,17 +67,46 @@ def dfe_sign_in_role_codes end def dfe_sign_in_first_name - auth.dig("info", "first_name") + auth.dig("info", "first_name") || dfe_sign_in_api_user&.first_name end def dfe_sign_in_last_name - auth.dig("info", "last_name") + auth.dig("info", "last_name") || dfe_sign_in_api_user&.last_name end def dfe_sign_in_email auth.dig("info", "email") end + class ApiUser < Struct.new(:first_name, :last_name, keyword_init: true); end + + def dfe_sign_in_api_user + return @dfe_sign_in_api_user if @dfe_sign_in_api_user + + ukprn = journey_session.answers.claim.school.ukprn + + uri = URI(DfeSignIn.configuration.base_url) + uri.path = "/organisations/#{ukprn}/users" + uri.query = {email: dfe_sign_in_email}.to_query + + response = dfe_sign_in_request(uri) + + return unless response.code == "200" + + data = JSON.parse(response.body) + users = data.fetch("users") + user = users.detect { |user| user["email"] == dfe_sign_in_email } + + return unless user.present? + + @dfe_sign_in_api_user = ApiUser.new( + first_name: user.fetch("firstName"), + last_name: user.fetch("lastName") + ) + rescue JSON::ParserError, KeyError => e + raise e if Rails.env.development? + end + class StubApiUser def initialize(params) @params = params diff --git a/spec/features/further_education_payments/provider/provider_verifying_claims_spec.rb b/spec/features/further_education_payments/provider/provider_verifying_claims_spec.rb index 1f89c32976..32f325efc5 100644 --- a/spec/features/further_education_payments/provider/provider_verifying_claims_spec.rb +++ b/spec/features/further_education_payments/provider/provider_verifying_claims_spec.rb @@ -3,6 +3,14 @@ RSpec.feature "Provider verifying claims" do before do create(:journey_configuration, :further_education_payments_provider) + # Stub fetching name from DSI, not required for these tests + stub_request( + :get, + %r{https://example.com/organisations/.*} + ).to_return( + status: 404, + body: nil + ) end scenario "provider visits a claim without service access" do diff --git a/spec/forms/journeys/further_education_payments/provider/omniauth_callback_form_spec.rb b/spec/forms/journeys/further_education_payments/provider/omniauth_callback_form_spec.rb index 3ad78ae658..4f16c03c9d 100644 --- a/spec/forms/journeys/further_education_payments/provider/omniauth_callback_form_spec.rb +++ b/spec/forms/journeys/further_education_payments/provider/omniauth_callback_form_spec.rb @@ -1,8 +1,25 @@ require "rails_helper" RSpec.describe Journeys::FurtherEducationPayments::Provider::OmniauthCallbackForm do + let(:school) { create(:school, ukprn: "123456") } + + let(:claim) do + create( + :claim, + policy: Policies::FurtherEducationPayments, + eligibility_attributes: { + school: school + } + ) + end + let(:journey_session) do - create(:further_education_payments_provider_session) + create( + :further_education_payments_provider_session, + answers: { + claim_id: claim.id + } + ) end let(:form) do @@ -30,8 +47,8 @@ "uid" => "11111", "info" => { "email" => "seymore.skinner@springfield-elementary.edu", - "first_name" => "Seymoure", - "last_name" => "Skinner" + "first_name" => info_first_name, + "last_name" => info_last_name }, "extra" => { "raw_info" => { @@ -48,47 +65,223 @@ context "with access to the service" do let(:service_access) { true } - it "updates the session with the auth details from dfe signin" do - expect { form.save! }.to( - change(journey_session.answers, :dfe_sign_in_uid).from(nil).to("11111").and( - change(journey_session.answers, :dfe_sign_in_organisation_ukprn) - .from(nil) - .to("12345678") - ).and( - change(journey_session.answers, :dfe_sign_in_organisation_id) - .from(nil) - .to("22222") - ).and( - change(journey_session.answers, :dfe_sign_in_organisation_name) - .from(nil) - .to("Springfield Elementary") - ).and( - change(journey_session.answers, :dfe_sign_in_service_access?) - .from(false) - .to(true) - ).and( - change(journey_session.answers, :dfe_sign_in_role_codes) - .from([]) - .to(["teacher_payments_claim_verifier"]) - ).and( - change(journey_session.answers, :dfe_sign_in_first_name) - .from(nil) - .to("Seymoure") - ).and( - change(journey_session.answers, :dfe_sign_in_last_name) - .from(nil) - .to("Skinner") - ).and( - change(journey_session.answers, :dfe_sign_in_email) - .from(nil) - .to("seymore.skinner@springfield-elementary.edu") + context "when the info payload contains the user's name" do + let(:info_first_name) { "Seymoure" } + let(:info_last_name) { "Skinner" } + + it "updates the session with the auth details from dfe signin" do + expect { form.save! }.to( + change(journey_session.answers, :dfe_sign_in_uid).from(nil).to("11111").and( + change(journey_session.answers, :dfe_sign_in_organisation_ukprn) + .from(nil) + .to("12345678") + ).and( + change(journey_session.answers, :dfe_sign_in_organisation_id) + .from(nil) + .to("22222") + ).and( + change(journey_session.answers, :dfe_sign_in_organisation_name) + .from(nil) + .to("Springfield Elementary") + ).and( + change(journey_session.answers, :dfe_sign_in_service_access?) + .from(false) + .to(true) + ).and( + change(journey_session.answers, :dfe_sign_in_role_codes) + .from([]) + .to(["teacher_payments_claim_verifier"]) + ).and( + change(journey_session.answers, :dfe_sign_in_first_name) + .from(nil) + .to("Seymoure") + ).and( + change(journey_session.answers, :dfe_sign_in_last_name) + .from(nil) + .to("Skinner") + ).and( + change(journey_session.answers, :dfe_sign_in_email) + .from(nil) + .to("seymore.skinner@springfield-elementary.edu") + ) ) - ) + end + end + + context "when the info payload does not contain the user's name" do + let(:info_first_name) { nil } + let(:info_last_name) { nil } + + before do + stub_request( + :get, + "https://example.com/organisations/123456/users?email=seymore.skinner%40springfield-elementary.edu" + ).to_return( + status: status, + body: body.to_json + ) + end + + context "when the dfe api request isn't successful" do + let(:status) { 404 } + let(:body) do + {} + end + + it "updates the session with the auth details from dfe signin" do + expect { form.save! }.to( + change(journey_session.answers, :dfe_sign_in_uid).from(nil).to("11111").and( + change(journey_session.answers, :dfe_sign_in_organisation_ukprn) + .from(nil) + .to("12345678") + ).and( + change(journey_session.answers, :dfe_sign_in_organisation_id) + .from(nil) + .to("22222") + ).and( + change(journey_session.answers, :dfe_sign_in_organisation_name) + .from(nil) + .to("Springfield Elementary") + ).and( + change(journey_session.answers, :dfe_sign_in_service_access?) + .from(false) + .to(true) + ).and( + change(journey_session.answers, :dfe_sign_in_role_codes) + .from([]) + .to(["teacher_payments_claim_verifier"]) + ).and( + not_change(journey_session.answers, :dfe_sign_in_first_name) + .from(nil) + ).and( + not_change(journey_session.answers, :dfe_sign_in_last_name) + .from(nil) + ).and( + change(journey_session.answers, :dfe_sign_in_email) + .from(nil) + .to("seymore.skinner@springfield-elementary.edu") + ) + ) + end + end + + # Edge case: I don't think this can happen IRL as the user has just signed into that org! + context "when the dfe api request doesn't include the user" do + let(:status) { 200 } + let(:body) do + { + "ukprn" => "12345678", + "users" => [ + { + "email" => "someone-else@springfield-elementary.edu", + "firstName" => "Seymoure", + "lastName" => "Skinner", + "userStatus" => 1, + "roles" => ["teacher_payments_claim_verifier"] + } + ] + } + end + + it "updates the session with the auth details from dfe signin" do + expect { form.save! }.to( + change(journey_session.answers, :dfe_sign_in_uid).from(nil).to("11111").and( + change(journey_session.answers, :dfe_sign_in_organisation_ukprn) + .from(nil) + .to("12345678") + ).and( + change(journey_session.answers, :dfe_sign_in_organisation_id) + .from(nil) + .to("22222") + ).and( + change(journey_session.answers, :dfe_sign_in_organisation_name) + .from(nil) + .to("Springfield Elementary") + ).and( + change(journey_session.answers, :dfe_sign_in_service_access?) + .from(false) + .to(true) + ).and( + change(journey_session.answers, :dfe_sign_in_role_codes) + .from([]) + .to(["teacher_payments_claim_verifier"]) + ).and( + not_change(journey_session.answers, :dfe_sign_in_first_name) + .from(nil) + ).and( + not_change(journey_session.answers, :dfe_sign_in_last_name) + .from(nil) + ).and( + change(journey_session.answers, :dfe_sign_in_email) + .from(nil) + .to("seymore.skinner@springfield-elementary.edu") + ) + ) + end + end + + context "when the dfe api request is successful" do + let(:status) { 200 } + let(:body) do + { + "ukprn" => "12345678", + "users" => [ + { + "email" => "seymore.skinner@springfield-elementary.edu", + "firstName" => "Seymoure", + "lastName" => "Skinner", + "userStatus" => 1, + "roles" => ["teacher_payments_claim_verifier"] + } + ] + } + end + + it "updates the session with the auth details from dfe signin" do + expect { form.save! }.to( + change(journey_session.answers, :dfe_sign_in_uid).from(nil).to("11111").and( + change(journey_session.answers, :dfe_sign_in_organisation_ukprn) + .from(nil) + .to("12345678") + ).and( + change(journey_session.answers, :dfe_sign_in_organisation_id) + .from(nil) + .to("22222") + ).and( + change(journey_session.answers, :dfe_sign_in_organisation_name) + .from(nil) + .to("Springfield Elementary") + ).and( + change(journey_session.answers, :dfe_sign_in_service_access?) + .from(false) + .to(true) + ).and( + change(journey_session.answers, :dfe_sign_in_role_codes) + .from([]) + .to(["teacher_payments_claim_verifier"]) + ).and( + change(journey_session.answers, :dfe_sign_in_first_name) + .from(nil) + .to("Seymoure") + ).and( + change(journey_session.answers, :dfe_sign_in_last_name) + .from(nil) + .to("Skinner") + ).and( + change(journey_session.answers, :dfe_sign_in_email) + .from(nil) + .to("seymore.skinner@springfield-elementary.edu") + ) + ) + end + end end end context "without access to the service" do let(:service_access) { false } + let(:info_first_name) { "Seymoure" } + let(:info_last_name) { "Skinner" } it "sets the service access flag to false" do expect { form.save! }.to(