From e02c2d2b8f6857658eff89f1bb0a160b0bf53054 Mon Sep 17 00:00:00 2001
From: Graham McIntire <graham@mcintire.me>
Date: Tue, 6 Jan 2026 13:32:17 -0600
Subject: [PATCH] Fix impersonation crash when session IDs are nil

Critical bug fix: fetch_current_scope_for_user and mount_current_scope
were calling Accounts.get_user(nil) when impersonating session flag was
true but superuser_id or target_user_id were nil.

This caused FunctionClauseError crashes for all logged-out users if they
had stale impersonation session data.

Changes:
- Check if both superuser_id and target_user_id exist before calling get_user
- Clear invalid impersonation state if IDs are missing
- Apply fix to both fetch_current_scope_for_user (controllers) and
  mount_current_scope (LiveViews)

This ensures graceful handling of corrupted/partial session state.
---
 lib/towerops_web/user_auth.ex | 44 ++++++++++++++++++++++++-----------
 1 file changed, 30 insertions(+), 14 deletions(-)

diff --git a/lib/towerops_web/user_auth.ex b/lib/towerops_web/user_auth.ex
index 81ee6a22..1e6fdbec 100644
--- a/lib/towerops_web/user_auth.ex
+++ b/lib/towerops_web/user_auth.ex
@@ -74,17 +74,27 @@ defmodule ToweropsWeb.UserAuth do
       superuser_id = get_session(conn, :superuser_id)
       target_user_id = get_session(conn, :target_user_id)
 
-      with superuser when not is_nil(superuser) <- Accounts.get_user(superuser_id),
-           target_user when not is_nil(target_user) <- Accounts.get_user(target_user_id) do
-        assign(conn, :current_scope, Scope.for_impersonation(superuser, target_user))
+      # Validate we have both IDs before attempting to fetch users
+      if superuser_id && target_user_id do
+        with superuser when not is_nil(superuser) <- Accounts.get_user(superuser_id),
+             target_user when not is_nil(target_user) <- Accounts.get_user(target_user_id) do
+          assign(conn, :current_scope, Scope.for_impersonation(superuser, target_user))
+        else
+          _ ->
+            # Impersonation invalid, clear it
+            conn
+            |> delete_session(:superuser_id)
+            |> delete_session(:target_user_id)
+            |> delete_session(:impersonating)
+            |> assign(:current_scope, Scope.for_user(nil))
+        end
       else
-        _ ->
-          # Impersonation invalid, clear it
-          conn
-          |> delete_session(:superuser_id)
-          |> delete_session(:target_user_id)
-          |> delete_session(:impersonating)
-          |> assign(:current_scope, Scope.for_user(nil))
+        # Missing IDs, clear invalid impersonation state
+        conn
+        |> delete_session(:superuser_id)
+        |> delete_session(:target_user_id)
+        |> delete_session(:impersonating)
+        |> assign(:current_scope, Scope.for_user(nil))
       end
     else
       # Normal authentication flow
@@ -411,11 +421,17 @@ defmodule ToweropsWeb.UserAuth do
         superuser_id = session["superuser_id"]
         target_user_id = session["target_user_id"]
 
-        with superuser when not is_nil(superuser) <- Accounts.get_user(superuser_id),
-             target_user when not is_nil(target_user) <- Accounts.get_user(target_user_id) do
-          Scope.for_impersonation(superuser, target_user)
+        # Validate we have both IDs before attempting to fetch users
+        if superuser_id && target_user_id do
+          with superuser when not is_nil(superuser) <- Accounts.get_user(superuser_id),
+               target_user when not is_nil(target_user) <- Accounts.get_user(target_user_id) do
+            Scope.for_impersonation(superuser, target_user)
+          else
+            _ -> Scope.for_user(nil)
+          end
         else
-          _ -> Scope.for_user(nil)
+          # Missing IDs, return nil user
+          Scope.for_user(nil)
         end
       else
         # Normal authentication flow