Merge pull request #10203 from DFE-Digital/cv/one-login-db-session

One login db session

app/controllers/candidate_interface/candidate_interface_controller.rb

@@ -1,9 +1,10 @@
 module CandidateInterface
   class CandidateInterfaceController < ApplicationController
     include BackLinks
+    include Authentication
 
     before_action :protect_with_basic_auth
-    before_action :authenticate_candidate!
+    before_action :authenticate_candidate!, unless: -> { one_login_enabled? }
     before_action :set_user_context
     before_action :check_cookie_preferences
     before_action :check_account_locked
@@ -12,6 +13,10 @@ class CandidateInterfaceController < ApplicationController
     alias audit_user current_candidate
     alias current_user current_candidate
 
+    def current_candidate
+      super || Current.session&.candidate
+    end
+
     def set_user_context(candidate_id = current_candidate&.id)
       Sentry.set_user(id: "candidate_#{candidate_id}")
 

app/controllers/candidate_interface/start_page_controller.rb

@@ -3,6 +3,7 @@ class StartPageController < CandidateInterfaceController
     before_action :redirect_to_application_if_signed_in
     before_action :redirect_to_post_offer_dashboard_if_accepted_deferred_or_recruited, if: :candidate_signed_in?
     skip_before_action :authenticate_candidate!
+    allow_unauthenticated_access only: %i[create_account_or_sign_in create_account_or_sign_in_handler]
 
     def create_account_or_sign_in
       @create_account_or_sign_in_form = CreateAccountOrSignInForm.new

app/controllers/concerns/authentication.rb

@@ -0,0 +1,63 @@
+module Authentication
+  extend ActiveSupport::Concern
+
+  included do
+    before_action :require_authentication, if: -> { one_login_enabled? }
+    helper_method :authenticated?
+  end
+
+  class_methods do
+    def allow_unauthenticated_access(**options)
+      skip_before_action :require_authentication, **options
+    end
+  end
+
+private
+
+  def authenticated?
+    resume_session
+  end
+
+  def require_authentication
+    current_candidate || resume_session || request_authentication
+  end
+
+  def resume_session
+    Current.session ||= find_session_by_cookie
+  end
+
+  def find_session_by_cookie
+    Session.find_by(id: cookies.signed[:session_id]) if cookies.signed[:session_id]
+  end
+
+  def request_authentication
+    redirect_to candidate_interface_create_account_or_sign_in_path
+  end
+
+  def start_new_session_for(candidate:, id_token_hint: nil)
+    ActiveRecord::Base.transaction do
+      unless authenticated?
+        candidate.sessions.create!(
+          user_agent: request.user_agent,
+          ip_address: request.remote_ip,
+          id_token_hint:,
+        ).tap do |session|
+          Current.session = session
+          cookies.signed.permanent[:session_id] = { value: session.id, httponly: true, same_site: :lax }
+        end
+
+        candidate.update!(last_signed_in_at: Time.zone.now)
+      end
+    end
+  end
+
+  def terminate_session
+    Current.session&.destroy
+    cookies.delete(:session_id)
+    reset_session
+  end
+
+  def one_login_enabled?
+    FeatureFlag.active?(:one_login_candidate_sign_in)
+  end
+end

app/controllers/one_login_controller.rb

@@ -1,17 +1,39 @@
 class OneLoginController < ApplicationController
+  include Authentication
+
   before_action :redirect_to_candidate_sign_in_unless_one_login_enabled
+  allow_unauthenticated_access
 
   def callback
     auth = request.env['omniauth.auth']
-    session[:one_login_id_token] = auth&.credentials&.id_token
+    id_token_hint = auth&.credentials&.id_token
     candidate = OneLoginUser.authenticate_or_create_by(auth)
 
-    sign_in_candidate(candidate)
+    start_new_session_for(
+      candidate:,
+      id_token_hint:,
+    )
 
     redirect_to candidate_interface_interstitial_path
-  rescue OneLoginUser::Error => e
-    session[:one_login_error] = e.message
-    redirect_to auth_one_login_sign_out_path
+  rescue StandardError => e
+    session_error = SessionError.create!(
+      candidate: OneLoginUser.find_candidate(auth),
+      id_token_hint:,
+      body: e.message,
+      omniauth_hash: auth&.to_h,
+    )
+
+    if e.is_a?(OneLoginUser::Error)
+      session[:session_error_id] = session_error.id
+      Sentry.capture_message(
+        "One login session error, check session_error record #{session_error.id}",
+        level: :error,
+      )
+
+      redirect_to auth_one_login_sign_out_path
+    else
+      redirect_to internal_server_error_path
+    end
   end
 
   def bypass_callback
@@ -21,7 +43,7 @@ def bypass_callback
     candidate = one_login_user_bypass.authenticate
 
     if candidate.present?
-      sign_in_candidate(candidate)
+      start_new_session_for(candidate:)
 
       redirect_to candidate_interface_interstitial_path
     else
@@ -31,32 +53,43 @@ def bypass_callback
   end
 
   def sign_out
-    id_token = session[:one_login_id_token]
-    one_login_error = session[:one_login_error]
-    reset_session
+    session_error = SessionError.find_by(id: session[:session_error_id])
+    id_token_hint = if authenticated?
+                      Current.session&.id_token_hint
+                    else
+                      session_error&.id_token_hint
+                    end
+
+    terminate_session
 
-    session[:one_login_error] = one_login_error
-    if OneLogin.bypass? || id_token.nil?
+    session[:session_error_id] = session_error.id if session_error.present?
+    if OneLogin.bypass? || id_token_hint.nil?
       redirect_to candidate_interface_create_account_or_sign_in_path
     else
       # Go back to one login to sign out the user on their end as well
-      redirect_to logout_one_login(id_token), allow_other_host: true
+      redirect_to logout_one_login(id_token_hint), allow_other_host: true
     end
   end
 
   def sign_out_complete
-    if session[:one_login_error].present?
-      Sentry.capture_message(session[:one_login_error], level: :error)
+    if session[:session_error_id].present?
+      reset_session
       redirect_to internal_server_error_path
     else
       redirect_to candidate_interface_create_account_or_sign_in_path
     end
   end
 
   def failure
-    session[:one_login_error] = "One login failure with #{params[:message]} " \
-                                "for one_login_id_token: #{session[:one_login_id_token]}"
+    session_error = SessionError.create!(
+      body: "One login failure with #{params[:message]}",
+    )
+    Sentry.capture_message(
+      "#{session_error.body}, check session_error record #{session_error.id}",
+      level: :error,
+    )
 
+    session[:session_error_id] = session_error.id if session_error.present?
     redirect_to auth_one_login_sign_out_path
   end
 
@@ -68,11 +101,6 @@ def redirect_to_candidate_sign_in_unless_one_login_enabled
     end
   end
 
-  def sign_in_candidate(candidate)
-    sign_in(candidate, scope: :candidate)
-    candidate.update!(last_signed_in_at: Time.zone.now)
-  end
-
   def logout_one_login(id_token_hint)
     params = {
       post_logout_redirect_uri: URI(auth_one_login_sign_out_complete_url),

app/models/current.rb

@@ -0,0 +1,4 @@
+class Current < ActiveSupport::CurrentAttributes
+  attribute :session
+  delegate :candidate, to: :session, allow_nil: true
+end

app/models/one_login_user.rb

@@ -21,6 +21,14 @@ def authenticate_or_create_by
     create_candidate!
   end
 
+  def self.find_candidate(omniauth_auth)
+    new(omniauth_auth).find_candidate
+  end
+
+  def find_candidate
+    OneLoginAuth.find_by(token:)&.candidate || Candidate.find_by(email_address:)
+  end
+
 private
 
   def candidate_with_one_login(one_login_auth)

config/brakeman.ignore

@@ -115,6 +115,29 @@
       ],
       "note": ""
     },
+    {
+      "warning_type": "Redirect",
+      "warning_code": 18,
+      "fingerprint": "783f66b9e6119c7b65c794a57f29b291e256d49267c511bff375d031ef83ea96",
+      "check_name": "Redirect",
+      "message": "Possible unprotected redirect",
+      "file": "app/controllers/one_login_controller.rb",
+      "line": 71,
+      "link": "https://brakemanscanner.org/docs/warning_types/redirect/",
+      "code": "redirect_to(logout_one_login((Current.session.id_token_hint or SessionError.find_by(:id => session[:session_error_id]).id_token_hint)), :allow_other_host => true)",
+      "render_path": null,
+      "location": {
+        "type": "method",
+        "class": "OneLoginController",
+        "method": "sign_out"
+      },
+      "user_input": "Current.session.id_token_hint",
+      "confidence": "Weak",
+      "cwe_id": [
+        601
+      ],
+      "note": ""
+    },
     {
       "warning_type": "SQL Injection",
       "warning_code": 0,
@@ -276,6 +299,29 @@
       ],
       "note": ""
     },
+    {
+      "warning_type": "Unscoped Find",
+      "warning_code": 82,
+      "fingerprint": "bb99c47602bf4e5e8c658f10645d3d0af3f1ac7e78e590db5e66d8986ba1642a",
+      "check_name": "UnscopedFind",
+      "message": "Unscoped call to `Session#find_by`",
+      "file": "app/controllers/concerns/authentication.rb",
+      "line": 32,
+      "link": "https://brakemanscanner.org/docs/warning_types/unscoped_find/",
+      "code": "Session.find_by(:id => cookies.signed[:session_id])",
+      "render_path": null,
+      "location": {
+        "type": "method",
+        "class": "Authentication",
+        "method": "find_session_by_cookie"
+      },
+      "user_input": "cookies.signed[:session_id]",
+      "confidence": "Weak",
+      "cwe_id": [
+        285
+      ],
+      "note": ""
+    },
     {
       "warning_type": "SQL Injection",
       "warning_code": 0,
@@ -346,6 +392,6 @@
       "note": ""
     }
   ],
-  "updated": "2024-08-27 15:35:00 +0100",
-  "brakeman_version": "6.1.2"
+  "updated": "2025-01-02 12:10:42 +0000",
+  "brakeman_version": "6.2.2"
 }