Improve oauth error handling

There are cases where the auth flow between this service and DSI can get
into an inconsistent state, resulting in CSRF and state errors from the
auth provider. For example:

- authenticate with an unauthorised organisation
- open a new tab to attempt a new sign in
- submit the form and get a state error from DSI

To resolve these cases, and to make the auth flow more robust, we do the
following:

- Reset the session on the sign in page instead of clearing specific
  session attributes in the omniauth callback controller. This gives us
  a simpler baseline state from which to attempt authentication,
  regardless of how the user arrives at the page (whether through direct
  navigation, or a redirect following an error).
- Add an oauth_failure param when redirecting after an oauth error, so
  that we can continue to display a flash message on the sign in page
  despite having reset the session.
- Add system specs for this behaviour.

app/controllers/auth_failures_controller.rb

@@ -1,9 +1,12 @@
 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
+    handle_failure_then_redirect_to sign_in_path(oauth_failure: true)
   end
 
   private
@@ -12,7 +15,6 @@ def handle_failure_then_redirect_to(path)
     oidc_error = OpenIdConnectProtocolError.new(error_message)
     unless Rails.env.development?
       Sentry.capture_exception(oidc_error)
-      flash[:warning] = I18n.t("generic_oauth_failure")
       redirect_to(path) and return
     end
 

app/controllers/omniauth_callbacks_controller.rb

@@ -5,7 +5,6 @@ class OmniauthCallbacksController < ApplicationController
   skip_before_action :authenticate_dsi_user!
   skip_before_action :handle_expired_session!
 
-  before_action :clear_session_attributes
   before_action :add_auth_attributes_to_session, only: :dfe
 
   def dfe
@@ -28,13 +27,6 @@ def auth
     request.env["omniauth.auth"]
   end
 
-  def clear_session_attributes
-    session[:organisation_name] = nil
-    session[:id_token] = nil
-    session[:dsi_user_id] = nil
-    session[:dsi_user_session_expiry] = nil
-  end
-
   def add_auth_attributes_to_session
     session[:id_token] = auth.credentials.id_token
     session[:organisation_name] = auth.extra.raw_info.organisation.name

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

spec/support/system/authentication_steps.rb

@@ -38,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/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