Improve error handling and sign out flow when authenticating with DfE Sign-in

.env.development

@@ -1,4 +1,5 @@
 BYPASS_DSI=true
+HOSTING_DOMAIN=http://localhost:3000
 HOSTING_ENVIRONMENT=local
 
 DFE_SIGN_IN_API_BASE_URL=https://dev-api.signin.education.gov.uk

app/components/flash_message_component.html.erb

@@ -0,0 +1,8 @@
+<%= govuk_notification_banner(
+  title_text: title,
+  classes: classes,
+  html_attributes: { role: role },
+) do |notification_banner| %>
+  <% notification_banner.with_heading(text: heading) %>
+  <%= body %>
+<% end %>

app/components/flash_message_component.rb

@@ -0,0 +1,50 @@
+# frozen_string_literal: true
+class FlashMessageComponent < ViewComponent::Base
+  ALLOWED_PRIMARY_KEYS = %i[info warning success].freeze
+  DEVISE_PRIMARY_KEYS = { alert: :warning, notice: :info }.freeze
+
+  def initialize(flash:)
+    super
+    @flash = flash.to_hash.symbolize_keys!
+  end
+
+  def message_key
+    key =
+      flash.keys.detect do |k|
+        ALLOWED_PRIMARY_KEYS.include?(k) || DEVISE_PRIMARY_KEYS.keys.include?(k)
+      end
+    DEVISE_PRIMARY_KEYS[key] || key
+  end
+
+  def title
+    I18n.t(message_key, scope: :notification_banner)
+  end
+
+  def classes
+    "govuk-notification-banner--#{message_key}"
+  end
+
+  def role
+    %i[warning success].include?(message_key) ? "alert" : "region"
+  end
+
+  def heading
+    messages.is_a?(Array) ? messages[0] : messages
+  end
+
+  def body
+    tag.p(messages[1], class: "govuk-body") if messages.is_a?(Array) && messages.count >= 2
+  end
+
+  def render?
+    !flash.empty? && message_key
+  end
+
+  private
+
+  def messages
+    flash[message_key] || flash[DEVISE_PRIMARY_KEYS.key(message_key)]
+  end
+
+  attr_reader :flash
+end

app/controllers/auth_failures_controller.rb

@@ -0,0 +1,31 @@
+class AuthFailuresController < ApplicationController
+  skip_before_action :authenticate_dsi_user!
+  skip_before_action :handle_expired_session!
+
+  class OpenIdConnectProtocolError < StandardError; end
+
+  def failure
+    return redirect_to(dsi_sign_out_path(id_token_hint: session[:id_token])) if session_expired?
+    handle_failure_then_redirect_to sign_in_path(oauth_failure: true)
+  end
+
+  private
+
+  def handle_failure_then_redirect_to(path)
+    oidc_error = OpenIdConnectProtocolError.new(error_message)
+    unless Rails.env.development?
+      Sentry.capture_exception(oidc_error)
+      redirect_to(path) and return
+    end
+
+    raise oidc_error
+  end
+
+  def error_message
+    @error_message ||= request.env["omniauth.error"]&.message
+  end
+
+  def session_expired?
+    error_message == "sessionexpired"
+  end
+end

app/controllers/omniauth_callbacks_controller.rb

@@ -5,24 +5,43 @@ class OmniauthCallbacksController < ApplicationController
   skip_before_action :authenticate_dsi_user!
   skip_before_action :handle_expired_session!
 
-  def dfe
-    auth = request.env["omniauth.auth"]
-
-    unless DfESignIn.bypass?
-      role = DfESignInApi::GetUserAccessToService.new(
-        org_id: auth.extra.raw_info.organisation.id,
-        user_id: auth.uid,
-      ).call
+  before_action :add_auth_attributes_to_session, only: :dfe
 
+  def dfe
+    if DfESignIn.bypass?
+      create_or_update_dsi_user
+    else
+      role = check_user_access_to_service
       return redirect_to not_authorised_path unless role
+
+      create_or_update_dsi_user(role)
     end
 
-    @dsi_user = DsiUser.create_or_update_from_dsi(auth, role)
+    redirect_to root_path
+  end
+  alias_method :dfe_bypass, :dfe
+
+  private
 
+  def auth
+    request.env["omniauth.auth"]
+  end
+
+  def add_auth_attributes_to_session
+    session[:id_token] = auth.credentials.id_token
+    session[:organisation_name] = auth.extra.raw_info.organisation.name
+  end
+
+  def create_or_update_dsi_user(role = nil)
+    @dsi_user = DsiUser.create_or_update_from_dsi(auth, role)
     session[:dsi_user_id] = @dsi_user.id
     session[:dsi_user_session_expiry] = 2.hours.from_now.to_i
+  end
 
-    redirect_to root_path
+  def check_user_access_to_service
+    DfESignInApi::GetUserAccessToService.new(
+      org_id: auth.extra.raw_info.organisation.id,
+      user_id: auth.uid,
+    ).call
   end
-  alias_method :dfe_bypass, :dfe
 end

app/controllers/sign_in_controller.rb

@@ -1,7 +1,15 @@
 class SignInController < ApplicationController
   skip_before_action :authenticate_dsi_user!
   skip_before_action :handle_expired_session!
+  before_action :reset_session
+  before_action :handle_failed_sign_in, if: -> { params[:oauth_failure] == "true" }
 
   def new
   end
+
+  private
+
+  def handle_failed_sign_in
+    flash.now[:warning] = I18n.t("generic_oauth_failure")
+  end
 end

app/controllers/sign_out_controller.rb

@@ -1,7 +1,7 @@
 class SignOutController < ApplicationController
   skip_before_action :handle_expired_session!
+  before_action :reset_session
 
   def new
-    session[:dsi_user_id] = nil if dsi_user_signed_in?
   end
 end

app/views/errors/not_authorised.html.erb

@@ -2,9 +2,10 @@
 
 <div class="govuk-grid-row">
   <div class="govuk-grid-column-two-thirds">
-    <h1 class="govuk-heading-l">Authorisation required</h1>
+    <h1 class="govuk-heading-l">You cannot use the DfE Sign-in account for <%= session[:organisation_name] %> to check the children's barred list.</h1>
     <p class="govuk-body">
-      You are not authorised to access this service.
+      If you have access to another organisation in DfE Sign-in, you can <%= govuk_link_to("sign out and start again", dsi_sign_out_path(id_token_hint: session[:id_token])) %>.
     </p>
   </div>
 </div>
+

app/views/layouts/base.html.erb

@@ -28,10 +28,12 @@
     <%= govuk_skip_link %>
 
     <%= govuk_header(service_name: t("service.name")) do |header|
-      if current_dsi_user
-        header.with_navigation_item(href: "/sign-out", text: "Sign out")
-      else
-        header.with_navigation_item(href: "/sign-in", text: "Sign in")
+      if request.path != not_authorised_path
+        if current_dsi_user
+          header.with_navigation_item(href: dsi_sign_out_path(id_token_hint: session[:id_token]), text: "Sign out")
+        else
+          header.with_navigation_item(href: sign_in_path, text: "Sign in")
+        end
       end
 
       if support_interface?
@@ -49,6 +51,7 @@
       <%= govuk_back_link(href: yield(:back_link_url)) if content_for?(:back_link_url) %>
       <%= yield(:breadcrumbs) if content_for?(:breadcrumbs) %>
       <main class="govuk-main-wrapper" id="main-content" role="main">
+        <%= render(FlashMessageComponent.new(flash: flash)) %>
         <%= yield :content %>
       </main>
     </div>

config/initializers/omniauth.rb

@@ -1,6 +1,9 @@
 require "dfe_sign_in"
+require "omniauth/strategies/dfe_openid_connect"
 
 OmniAuth.config.logger = Rails.logger
+OmniAuth.config.add_camelization('dfe_openid_connect', 'DfEOpenIDConnect')
+OmniAuth.config.on_failure = proc { |env| AuthFailuresController.action(:failure).call(env) }
 
 if DfESignIn.bypass?
   Rails.application.config.middleware.use OmniAuth::Builder do
@@ -12,9 +15,11 @@
   dfe_sign_in_issuer_uri = URI(ENV.fetch("DFE_SIGN_IN_ISSUER", "example"))
 
   Rails.application.config.middleware.use OmniAuth::Builder do
-    provider :openid_connect,
+    provider :dfe_openid_connect,
              name: :dfe,
              callback_path: "/auth/dfe/callback",
+             logout_path: "/sign-out",
+             post_logout_redirect_uri: "#{ENV['HOSTING_DOMAIN']}/sign-out",
              client_options: {
                host: dfe_sign_in_issuer_uri&.host,
                identifier: ENV["DFE_SIGN_IN_CLIENT_ID"],

config/locales/en.yml

@@ -3,6 +3,7 @@ en:
     name: Check the Children’s Barred List
     email: [email protected]
     dbs_email: [email protected]
+  generic_oauth_failure: There was a problem signing you in. Please try again.
   activemodel:
     errors:
       models:

config/routes.rb

@@ -6,6 +6,7 @@
 
   get "/sign-in", to: "sign_in#new"
   get "/sign-out", to: "sign_out#new"
+  get "/auth/dfe/sign-out", to: "sign_out#new", as: :dsi_sign_out
 
   get "/auth/dfe/callback", to: "omniauth_callbacks#dfe"
   post "/auth/developer/callback", to: "omniauth_callbacks#dfe_bypass"

lib/omniauth/strategies/dfe_openid_connect.rb

@@ -0,0 +1,27 @@
+# This strategy ensures that the id_token_hint param is included in the post_logout_redirect_uri.
+# The node-oidc-provider library requires this param to be present in order for the redirect to work.
+# See: https://github.com/panva/node-oidc-provider/blob/03c9bc513860e68ee7be84f99bfc9dc930b224e8/lib/actions/end_session.js#L27
+# See: https://github.com/omniauth/omniauth_openid_connect/blob/34370d655d39fe7980f89f55715888e0ebd7270e/lib/omniauth/strategies/openid_connect.rb#L423
+#
+module OmniAuth
+  module Strategies
+    class DfEOpenIDConnect < OmniAuth::Strategies::OpenIDConnect
+      TOKEN_KEY = "id_token_hint".freeze
+
+      def encoded_post_logout_redirect_uri
+        return unless options.post_logout_redirect_uri
+
+        logout_uri_params = {
+          "post_logout_redirect_uri" => options.post_logout_redirect_uri
+        }
+
+        if query_string.present?
+          query_params = CGI.parse(query_string[1..])
+          logout_uri_params[TOKEN_KEY] = query_params[TOKEN_KEY].first if query_params.key?(TOKEN_KEY)
+        end
+
+        URI.encode_www_form(logout_uri_params)
+      end
+    end
+  end
+end

spec/support/system/authentication_steps.rb

@@ -11,6 +11,9 @@ def given_dsi_auth_is_mocked(authorised:)
       {
         provider: "dfe",
         uid: "123456",
+        credentials: {
+          id_token: "abc123",
+        },
         info: {
           email: "[email protected]",
           first_name: "Test",
@@ -20,6 +23,7 @@ def given_dsi_auth_is_mocked(authorised:)
           raw_info: {
             organisation: {
               id: org_id,
+              name: "Test School",
             }
           }
         }
@@ -34,6 +38,21 @@ def given_dsi_auth_is_mocked(authorised:)
     )
   end
 
+  def given_dsi_auth_is_mocked_with_a_failure(message)
+    allow(Sentry).to receive(:capture_exception)
+    OmniAuth.config.mock_auth[:dfe] = message.to_sym
+    global_failure_handler = OmniAuth.config.on_failure
+    local_failure_handler = proc do |env|
+      env["omniauth.error"] = OmniAuth::Strategies::OpenIDConnect::CallbackError.new(error: message)
+      env
+    end
+    OmniAuth.config.on_failure = global_failure_handler << local_failure_handler
+
+    yield if block_given?
+
+    OmniAuth.config.on_failure = global_failure_handler
+  end
+
   def user_roles_endpoint
     "#{ENV.fetch("DFE_SIGN_IN_API_BASE_URL")}/services/checkchildrensbarredlist/organisations/#{org_id}/users/123456"
   end

spec/system/unauthorised_user_signs_in_spec.rb

@@ -16,6 +16,14 @@
 
   def then_i_am_not_authorised
     expect(page.status_code).to eq 401
-    expect(page).to have_content("You are not authorised to access this service")
+    expect(page).to have_content(
+      "You cannot use the DfE Sign-in account for Test School to check the children's barred list"
+    )
+    expect(page).to have_link("sign out and start again", href: "/auth/dfe/sign-out?id_token_hint=abc123")
+
+    within(".govuk-header__content") do
+      expect(page).not_to have_link("Sign in")
+      expect(page).not_to have_link("Sign out")
+    end
   end
 end

spec/system/user_has_oauth_error_signing_in_spec.rb

@@ -0,0 +1,39 @@
+# frozen_string_literal: true
+
+require "rails_helper"
+
+RSpec.describe "DSI authentication", type: :system do
+  include ActivateFeaturesSteps
+  include AuthenticationSteps
+
+  before do
+    given_the_service_is_open
+    allow(Sentry).to receive(:capture_exception)
+  end
+
+  scenario "User has oauth error when signing in", test: :with_stubbed_auth do
+    given_dsi_auth_is_mocked_with_a_failure("invalid_credentials") do
+      when_i_visit_the_sign_in_page
+      and_click_the_dsi_sign_in_button
+      then_i_see_a_sign_in_error
+    end
+  end
+
+  scenario "User has sessionexpiry oauth error", test: :with_stubbed_auth do
+    given_dsi_auth_is_mocked_with_a_failure("sessionexpired") do
+      when_i_visit_the_sign_in_page
+      and_click_the_dsi_sign_in_button
+      then_i_am_redirected_to_sign_in
+    end
+  end
+
+  private
+
+  def then_i_see_a_sign_in_error
+    expect(page).to have_content "There was a problem signing you in. Please try again."
+  end
+
+  def then_i_am_redirected_to_sign_in
+    expect(page).to have_current_path(sign_in_path)
+  end
+end

spec/system/user_signs_in_spec.rb

@@ -15,7 +15,12 @@
   private
 
   def then_i_am_signed_in
-    within("header") { expect(page).to have_content "Sign out" }
+    within("header") do
+      expect(page).to have_link("Sign out")
+      sign_out_link = find_link("Sign out")
+      # Expect the token from mocked auth to be in the sign out link
+      expect(sign_out_link[:href]).to include "id_token_hint=abc123"
+    end
     expect(DsiUser.count).to eq 1
     expect(DsiUserSession.count).to eq 1
   end