Wrap auth logic in AuthService

stephencdaly · stephencdaly · commit 00e05906cfe5 · 2026-04-29T15:15:41.000+01:00
This avoids having to keep track of the multiple store classes we now
have in the controllers, and removes some logic from the controllers.

Hopefully this makes it a bit easier to see what's involved in the
auth process.
diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb
@@ -119,11 +119,7 @@ def default_locale?(locale)
     false
   end
 
-  def auth_store
-    @auth_store ||= Store::AuthStore.new(session)
-  end
-
-  def return_from_one_login_store
-    @return_from_one_login_store ||= Store::ReturnFromOneLoginStore.new(session)
+  def auth_service
+    @auth_service ||= AuthService.new(session)
   end
 end
diff --git a/app/controllers/forms/check_your_answers_controller.rb b/app/controllers/forms/check_your_answers_controller.rb
@@ -43,9 +43,9 @@ def submit_answers
 
         current_context.save_submission_details(submission_reference, requested_email_confirmation)
 
-        if auth_store.logged_in?
-          return_from_one_login_store.store_return_params(form: current_context.form, mode: mode, locale: locale_param)
-          redirect_to logout_request.redirect_uri, allow_other_host: true
+        if auth_service.logged_in?
+          auth_service.store_return_params(form: current_context.form, mode: mode, locale: locale_param)
+          redirect_to auth_service.logout_redirect_uri(omniauth_logged_out_url), allow_other_host: true
         else
           redirect_to :form_submitted
         end
@@ -87,10 +87,5 @@ def back_link
         end
       end
     end
-
-    def logout_request
-      token = auth_store.get_token
-      AuthService.new.logout_request(token, omniauth_logged_out_url)
-    end
   end
 end
diff --git a/app/controllers/forms/continue_to_one_login_controller.rb b/app/controllers/forms/continue_to_one_login_controller.rb
@@ -3,7 +3,7 @@ class ContinueToOneLoginController < BaseController
     before_action :redirect_if_feature_disabled, :redirect_if_form_incomplete
 
     def show
-      return_from_one_login_store.store_return_params(
+      auth_service.store_return_params(
         form: current_context.form,
         mode: mode,
         locale: locale_param,
diff --git a/app/controllers/users/omniauth_controller.rb b/app/controllers/users/omniauth_controller.rb
@@ -1,25 +1,12 @@
 module Users
   class OmniauthController < ApplicationController
-    class OmniAuthLoggedInDataMissingError < StandardError; end
-
     class OmniAuthFailure < StandardError; end
 
     def callback
       auth_hash = request.env["omniauth.auth"]
-      raise OmniAuthLoggedInDataMissingError, "Auth hash is missing on request" if auth_hash.blank?
-
-      email = auth_hash.dig("info", "email")
-      raise OmniAuthLoggedInDataMissingError, "Email is missing in OmniAuth auth hash" if email.blank?
-
-      token = auth_hash.dig("credentials", "id_token")
-      raise OmniAuthLoggedInDataMissingError, "Token is missing in OmniAuth auth hash" if token.blank?
-
-      auth_store.store_token(token)
-
-      form_id = return_from_one_login_store.form_id
-      path_params = return_from_one_login_store.get_path_params
+      auth_service.store_auth_details(auth_hash)
 
-      Store::ConfirmationDetailsStore.new(session, form_id).save_copy_of_answers_email_address(email)
+      path_params = auth_service.form_path_params
 
       redirect_to check_your_answers_path(**path_params)
     rescue Store::ReturnFromOneLoginStore::MissingReturnParamsError
@@ -33,9 +20,9 @@ def failure
     end
 
     def logged_out
-      auth_store.clear
+      auth_service.clear_auth_session
 
-      path_params = return_from_one_login_store.get_path_params
+      path_params = auth_service.form_path_params
       redirect_to form_submitted_path(**path_params)
     end
   end
diff --git a/app/lib/store/return_from_one_login_store.rb b/app/lib/store/return_from_one_login_store.rb
@@ -20,7 +20,7 @@ def store_return_params(form:, mode:, locale:)
       @store[RETURN_FROM_ONE_LOGIN_KEY][LAST_LOCALE_KEY] = locale
     end
 
-    def get_path_params
+    def form_path_params
       raise MissingReturnParamsError if @store[RETURN_FROM_ONE_LOGIN_KEY].blank?
 
       {
diff --git a/app/services/auth_service.rb b/app/services/auth_service.rb
@@ -1,9 +1,43 @@
 class AuthService
-  def logout_request(token, post_logout_redirect_uri)
-    logout_utility.build_request(
+  class DataMissingError < StandardError; end
+
+  attr_reader :auth_store, :return_from_one_login_store
+
+  def initialize(store)
+    @store = store
+    @return_from_one_login_store = Store::ReturnFromOneLoginStore.new(store)
+    @auth_store = Store::AuthStore.new(store)
+  end
+
+  delegate :logged_in?, to: :auth_store
+  delegate :store_return_params, :form_path_params, to: :return_from_one_login_store
+
+  def store_auth_details(auth_hash)
+    form_id = @return_from_one_login_store.form_id
+
+    raise DataMissingError, "Auth hash is missing on request" if auth_hash.blank?
+
+    email = auth_hash.dig("info", "email")
+    raise DataMissingError, "Email is missing in OmniAuth auth hash" if email.blank?
+
+    token = auth_hash.dig("credentials", "id_token")
+    raise DataMissingError, "Token is missing in OmniAuth auth hash" if token.blank?
+
+    @auth_store.store_token(token)
+    Store::ConfirmationDetailsStore.new(@store, form_id).save_copy_of_answers_email_address(email)
+  end
+
+  def logout_redirect_uri(post_logout_redirect_uri)
+    token = @auth_store.get_token
+    logout_request = logout_utility.build_request(
       id_token_hint: token,
       post_logout_redirect_uri: url_without_params(post_logout_redirect_uri),
     )
+    logout_request.redirect_uri
+  end
+
+  def clear_auth_session
+    @auth_store.clear
   end
 
 private
diff --git a/spec/lib/store/return_from_one_login_store_spec.rb b/spec/lib/store/return_from_one_login_store_spec.rb
@@ -12,7 +12,7 @@
     it "stores and return the path params" do
       return_from_one_login_store.store_return_params(form:, mode:, locale:)
 
-      expect(return_from_one_login_store.get_path_params).to eq({
+      expect(return_from_one_login_store.form_path_params).to eq({
         mode: mode.to_s,
         form_id: form.id,
         form_slug: form.form_slug,
@@ -21,7 +21,7 @@
     end
 
     it "raises an error if the return params have not been stored" do
-      expect { return_from_one_login_store.get_path_params }.to raise_error(Store::ReturnFromOneLoginStore::MissingReturnParamsError)
+      expect { return_from_one_login_store.form_path_params }.to raise_error(Store::ReturnFromOneLoginStore::MissingReturnParamsError)
     end
   end
 
diff --git a/spec/requests/forms/check_your_answers_controller_spec.rb b/spec/requests/forms/check_your_answers_controller_spec.rb
@@ -327,7 +327,7 @@
       let(:end_session_endpoint) { "http://example.com/one-login-mock/logout" }
 
       before do
-        allow(Store::ReturnFromOneLoginStore).to receive(:new).and_wrap_original do |original_method, *_args|
+        allow(AuthService).to receive(:new).and_wrap_original do |original_method, *_args|
           original_method.call(store)
         end
 
diff --git a/spec/requests/forms/continue_to_one_login_controller_spec.rb b/spec/requests/forms/continue_to_one_login_controller_spec.rb
@@ -37,7 +37,7 @@
     allow(Flow::Context).to receive(:new).and_wrap_original do |original_method, *args|
       original_method.call(form: args[0][:form], form_document: args[0][:form_document], store:)
     end
-    allow(Store::ReturnFromOneLoginStore).to receive(:new).and_wrap_original do |original_method, *_args|
+    allow(AuthService).to receive(:new).and_wrap_original do |original_method, *_args|
       original_method.call(store)
     end
   end
diff --git a/spec/requests/users/omniauth_controller_spec.rb b/spec/requests/users/omniauth_controller_spec.rb
@@ -40,13 +40,7 @@
       OmniAuth.config.test_mode = true
       OmniAuth.config.mock_auth[:default] = auth_hash
 
-      allow(Store::ReturnFromOneLoginStore).to receive(:new).and_wrap_original do |original_method, *_args|
-        original_method.call(store)
-      end
-      allow(Store::ConfirmationDetailsStore).to receive(:new).and_wrap_original do |original_method, *args|
-        original_method.call(store, args[1])
-      end
-      allow(Store::AuthStore).to receive(:new).and_wrap_original do |original_method, *_args|
+      allow(AuthService).to receive(:new).and_wrap_original do |original_method, *_args|
         original_method.call(store)
       end
     end
@@ -69,19 +63,11 @@
       end
     end
 
-    context "when the auth details are not present on the request" do
+    context "when data is missing on the auth details on the request" do
       let(:auth_hash) { {} }
 
       it "raises an OmniAuthLoggedInDataMissingError" do
-        expect { get omniauth_callback_path }.to raise_error(Users::OmniauthController::OmniAuthLoggedInDataMissingError)
-      end
-    end
-
-    context "when the email on the auth hash is blank" do
-      let(:email) { "" }
-
-      it "raises a OmniAuthLoggedInDataMissingError" do
-        expect { get omniauth_callback_path }.to raise_error(Users::OmniauthController::OmniAuthLoggedInDataMissingError)
+        expect { get omniauth_callback_path }.to raise_error(AuthService::DataMissingError)
       end
     end
 
@@ -107,10 +93,7 @@
     let(:token) { Faker::Alphanumeric.alphanumeric }
 
     before do
-      allow(Store::ReturnFromOneLoginStore).to receive(:new).and_wrap_original do |original_method, *_args|
-        original_method.call(store)
-      end
-      allow(Store::AuthStore).to receive(:new).and_wrap_original do |original_method, *_args|
+      allow(AuthService).to receive(:new).and_wrap_original do |original_method, *_args|
         original_method.call(store)
       end
     end
diff --git a/spec/services/auth_service_spec.rb b/spec/services/auth_service_spec.rb
@@ -1,30 +1,117 @@
 require "rails_helper"
 
 RSpec.describe AuthService do
-  subject(:auth_service) { described_class.new }
+  subject(:auth_service) { described_class.new(store) }
 
   let(:end_session_endpoint) { "http://example.com/one-login-mock/logout" }
+  let(:store) { {}.with_indifferent_access }
+  let(:token) { Faker::Alphanumeric.alphanumeric }
 
   before do
     idp_configuration = instance_double(OmniAuth::GovukOneLogin::IdpConfiguration, end_session_endpoint:)
     allow(Rails.application.config.x).to receive(:one_login).and_return(double(idp_configuration:))
   end
 
-  describe "#logout_request" do
-    let(:token) { Faker::Alphanumeric.alphanumeric }
+  describe "#store_auth_details" do
+    let(:email) { "test@example.com" }
+    let(:id_token) { Faker::Alphanumeric.alphanumeric }
+    let(:auth_hash) do
+      {
+        info: { email: },
+        credentials: { id_token: },
+      }.with_indifferent_access
+    end
+
+    context "when the return from one login params are not set on the session" do
+      it "raises a MissingReturnParamsError" do
+        expect { auth_service.store_auth_details({}) }
+          .to raise_error(Store::ReturnFromOneLoginStore::MissingReturnParamsError)
+      end
+    end
+
+    context "when the return from one login params are set on the session" do
+      let(:form_id) { 42 }
+      let(:store) do
+        {
+          "return_from_one_login" => {
+            "last_form_id" => form_id,
+          },
+        }.with_indifferent_access
+      end
+
+      context "when the auth hash is empty" do
+        let(:auth_hash) { {}.with_indifferent_access }
+
+        it "raises a DataMissingError" do
+          expect { auth_service.store_auth_details(auth_hash) }
+            .to raise_error(AuthService::DataMissingError, "Auth hash is missing on request")
+        end
+      end
+
+      context "when the email is missing from the auth hash" do
+        let(:auth_hash) { { credentials: { id_token: } }.with_indifferent_access }
+
+        it "raises a DataMissingError" do
+          expect { auth_service.store_auth_details(auth_hash) }
+            .to raise_error(AuthService::DataMissingError, "Email is missing in OmniAuth auth hash")
+        end
+      end
+
+      context "when the token is missing from the auth hash" do
+        let(:auth_hash) { { info: { email: } }.with_indifferent_access }
+
+        it "raises a DataMissingError" do
+          expect { auth_service.store_auth_details(auth_hash) }
+            .to raise_error(AuthService::DataMissingError, "Token is missing in OmniAuth auth hash")
+        end
+      end
+
+      context "when the auth hash is valid" do
+        it "stores the email on the session" do
+          auth_service.store_auth_details(auth_hash)
+          expect(store.dig("confirmation_details", form_id.to_s, "copy_of_answers_email_address")).to eq email
+        end
+
+        it "stores the token on the session" do
+          auth_service.store_auth_details(auth_hash)
+          expect(store["auth"]["token"]).to eq id_token
+        end
+      end
+    end
+  end
+
+  describe "#logout_redirect_uri" do
+    let(:store) do
+      {
+        auth: { token: },
+      }.with_indifferent_access
+    end
     let(:post_logout_redirect_uri) { "https://example.com/some-path?with=params" }
 
     it "returns a logout request with the expected One Login URI" do
-      logout_request = auth_service.logout_request(token, post_logout_redirect_uri)
-      expect(logout_request.redirect_uri).to start_with(end_session_endpoint)
+      logout_redirect_uri = auth_service.logout_redirect_uri(post_logout_redirect_uri)
+      expect(logout_redirect_uri).to start_with(end_session_endpoint)
     end
 
     it "strips query params from the post_logout_redirect_uri" do
-      logout_request = auth_service.logout_request(token, post_logout_redirect_uri)
-      redirect_uri = URI.parse(logout_request.redirect_uri)
-      query = Rack::Utils.parse_query(redirect_uri.query)
+      logout_redirect_uri = auth_service.logout_redirect_uri(post_logout_redirect_uri)
+      parsed_uri = URI.parse(logout_redirect_uri)
+      query = Rack::Utils.parse_query(parsed_uri.query)
       expect(query).to include("post_logout_redirect_uri" => "https://example.com/some-path",
                                "id_token_hint" => token)
     end
   end
+
+  describe "#clear_auth_session" do
+    let(:store) do
+      {
+        auth: { token: },
+      }.with_indifferent_access
+    end
+
+    it "clears the session" do
+      auth_service.clear_auth_session
+      expect(store["auth"]).to be_nil
+    end
+  end
 end

Original file line number	Diff line number	Diff line change
`@@ -20,7 +20,7 @@ def store_return_params(form:, mode:, locale:)`
`20`	`20`	`@store[RETURN_FROM_ONE_LOGIN_KEY][LAST_LOCALE_KEY] = locale`
`21`	`21`	`end`
`22`	`22`
`23`		`- def get_path_params`
	`23`	`+ def form_path_params`
`24`	`24`	`raise MissingReturnParamsError if @store[RETURN_FROM_ONE_LOGIN_KEY].blank?`
`25`	`25`
`26`	`26`	`{`