From f2def20fcefe67d81b25a40ded413ca0793769e1 Mon Sep 17 00:00:00 2001
From: Moritz <moritz.m@local-it.org>
Date: Thu, 22 Jan 2026 22:37:07 +0100
Subject: [PATCH] Add centralized Actor.ensure_loaded helper

Consolidate role loading logic from HasPermission and LiveHelpers.
Use Ash.Resource.Info.resource? for reliable Ash detection.
---
 lib/mv/authorization/actor.ex                 | 89 +++++++++++++++++++
 lib/mv/authorization/checks/has_permission.ex | 27 +-----
 lib/mv_web/live_helpers.ex                    | 32 ++-----
 test/mv/authorization/actor_test.exs          | 84 +++++++++++++++++
 4 files changed, 181 insertions(+), 51 deletions(-)
 create mode 100644 lib/mv/authorization/actor.ex
 create mode 100644 test/mv/authorization/actor_test.exs

diff --git a/lib/mv/authorization/actor.ex b/lib/mv/authorization/actor.ex
new file mode 100644
index 0000000..5d337e8
--- /dev/null
+++ b/lib/mv/authorization/actor.ex
@@ -0,0 +1,89 @@
+defmodule Mv.Authorization.Actor do
+  @moduledoc """
+  Helper functions for ensuring actors have required data loaded.
+
+  ## Actor Invariant
+
+  Authorization policies (especially HasPermission) require that the actor
+  has their `:role` relationship loaded. This module provides helpers to
+  ensure this invariant is maintained across all entry points:
+
+  - LiveView on_mount hooks
+  - Plug pipelines
+  - Background jobs
+  - Tests
+
+  ## Usage
+
+      # In LiveView on_mount
+      def ensure_user_role_loaded(_name, socket) do
+        user = Actor.ensure_loaded(socket.assigns[:current_user])
+        assign(socket, :current_user, user)
+      end
+
+      # In tests
+      user = Actor.ensure_loaded(user)
+
+  ## Security Note
+
+  `ensure_loaded/1` loads the role with `authorize?: true` (default).
+  The Role resource must have policies that allow an actor to read their own role.
+  See `Mv.Authorization.Checks.HasPermission` for the fallback implementation.
+  """
+
+  require Logger
+
+  @doc """
+  Ensures the actor has their `:role` relationship loaded.
+
+  - If actor is nil, returns nil
+  - If role is already loaded, returns actor as-is
+  - If role is %Ash.NotLoaded{}, loads it and returns updated actor
+
+  ## Examples
+
+      iex> Actor.ensure_loaded(nil)
+      nil
+
+      iex> Actor.ensure_loaded(%User{role: %Role{}})
+      %User{role: %Role{}}
+
+      iex> Actor.ensure_loaded(%User{role: %Ash.NotLoaded{}})
+      %User{role: %Role{}}  # role loaded
+  """
+  def ensure_loaded(nil), do: nil
+
+  def ensure_loaded(%{role: %Ash.NotLoaded{}} = actor) do
+    # Only attempt to load if actor is a valid Ash resource
+    if ash_resource?(actor) do
+      load_role(actor)
+    else
+      # Not an Ash resource (e.g., plain map in tests) - return as-is
+      actor
+    end
+  end
+
+  def ensure_loaded(actor), do: actor
+
+  # Check if actor is a valid Ash resource
+  defp ash_resource?(actor) do
+    is_struct(actor) and Ash.Resource.Info.resource?(actor.__struct__)
+  end
+
+  defp load_role(actor) do
+    # Need to specify domain for Ash.load to work
+    case Ash.load(actor, :role, domain: Mv.Accounts) do
+      {:ok, loaded_actor} ->
+        loaded_actor
+
+      {:error, error} ->
+        # Log error but don't crash - fail-closed for authorization
+        Logger.warning(
+          "Failed to load actor role: #{inspect(error)}. " <>
+            "Authorization may fail if role is required."
+        )
+
+        actor
+    end
+  end
+end
diff --git a/lib/mv/authorization/checks/has_permission.ex b/lib/mv/authorization/checks/has_permission.ex
index 865b2a9..97b74c0 100644
--- a/lib/mv/authorization/checks/has_permission.ex
+++ b/lib/mv/authorization/checks/has_permission.ex
@@ -391,29 +391,8 @@ defmodule Mv.Authorization.Checks.HasPermission do
   end
 
   # Fallback: Load role if not loaded (in case on_mount didn't run)
-  defp ensure_role_loaded(%{role: %Ash.NotLoaded{}} = actor) do
-    if ash_resource?(actor) do
-      load_role_for_actor(actor)
-    else
-      # Not an Ash resource (plain map), return as-is
-      actor
-    end
-  end
-
-  defp ensure_role_loaded(actor), do: actor
-
-  # Check if actor is a valid Ash resource
-  defp ash_resource?(actor) do
-    is_map(actor) and Map.has_key?(actor, :__struct__) and
-      function_exported?(actor.__struct__, :__ash_resource__, 0)
-  end
-
-  # Attempt to load role for Ash resource
-  defp load_role_for_actor(actor) do
-    case Ash.load(actor, :role, domain: Mv.Accounts, actor: actor) do
-      {:ok, loaded} -> loaded
-      # Return original if loading fails
-      {:error, _} -> actor
-    end
+  # Delegates to centralized Actor helper
+  defp ensure_role_loaded(actor) do
+    Mv.Authorization.Actor.ensure_loaded(actor)
   end
 end
diff --git a/lib/mv_web/live_helpers.ex b/lib/mv_web/live_helpers.ex
index 95d8235..b8f070c 100644
--- a/lib/mv_web/live_helpers.ex
+++ b/lib/mv_web/live_helpers.ex
@@ -27,39 +27,17 @@ defmodule MvWeb.LiveHelpers do
   end
 
   defp ensure_user_role_loaded(socket) do
-    if socket.assigns[:current_user] do
-      user = socket.assigns.current_user
-      user_with_role = load_user_role(user)
+    user = socket.assigns[:current_user]
+
+    if user do
+      # Use centralized Actor helper to ensure role is loaded
+      user_with_role = Mv.Authorization.Actor.ensure_loaded(user)
       assign(socket, :current_user, user_with_role)
     else
       socket
     end
   end
 
-  defp load_user_role(user) do
-    case Map.get(user, :role) do
-      %Ash.NotLoaded{} -> load_role_safely(user)
-      nil -> load_role_safely(user)
-      _role -> user
-    end
-  end
-
-  defp load_role_safely(user) do
-    # Use self as actor for loading own role relationship
-    opts = [domain: Mv.Accounts, actor: user]
-
-    case Ash.load(user, :role, opts) do
-      {:ok, loaded_user} ->
-        loaded_user
-
-      {:error, error} ->
-        # Log warning if role loading fails - this can cause authorization issues
-        require Logger
-        Logger.warning("Failed to load role for user #{user.id}: #{inspect(error)}")
-        user
-    end
-  end
-
   @doc """
   Helper function to get the current actor (user) from socket assigns.
 
diff --git a/test/mv/authorization/actor_test.exs b/test/mv/authorization/actor_test.exs
new file mode 100644
index 0000000..9c83bd3
--- /dev/null
+++ b/test/mv/authorization/actor_test.exs
@@ -0,0 +1,84 @@
+defmodule Mv.Authorization.ActorTest do
+  @moduledoc """
+  Tests for the Actor helper module.
+  """
+  use Mv.DataCase, async: false
+
+  alias Mv.Accounts
+  alias Mv.Authorization.Actor
+
+  describe "ensure_loaded/1" do
+    test "returns nil when actor is nil" do
+      assert Actor.ensure_loaded(nil) == nil
+    end
+
+    test "returns actor as-is when role is already loaded" do
+      # Create user with role
+      {:ok, user} =
+        Accounts.User
+        |> Ash.Changeset.for_create(:register_with_password, %{
+          email: "test#{System.unique_integer([:positive])}@example.com",
+          password: "testpassword123"
+        })
+        |> Ash.create()
+
+      # Load role
+      {:ok, user_with_role} = Ash.load(user, :role, domain: Mv.Accounts)
+
+      # Should return as-is (no additional load)
+      result = Actor.ensure_loaded(user_with_role)
+      assert result.id == user.id
+      assert result.role != %Ash.NotLoaded{}
+    end
+
+    test "loads role when it's NotLoaded" do
+      # Create a role first
+      {:ok, role} =
+        Mv.Authorization.Role
+        |> Ash.Changeset.for_create(:create_role, %{
+          name: "Test Role #{System.unique_integer([:positive])}",
+          description: "Test role",
+          permission_set_name: "own_data"
+        })
+        |> Ash.create()
+
+      # Create user with role
+      {:ok, user} =
+        Accounts.User
+        |> Ash.Changeset.for_create(:register_with_password, %{
+          email: "test#{System.unique_integer([:positive])}@example.com",
+          password: "testpassword123"
+        })
+        |> Ash.create()
+
+      # Assign role to user
+      {:ok, user_with_role} =
+        user
+        |> Ash.Changeset.for_update(:update, %{})
+        |> Ash.Changeset.manage_relationship(:role, role, type: :append_and_remove)
+        |> Ash.update()
+
+      # Fetch user again WITHOUT loading role (simulates "role not preloaded" scenario)
+      {:ok, user_without_role_loaded} =
+        Ash.get(Accounts.User, user_with_role.id, domain: Mv.Accounts)
+
+      # User has role as NotLoaded (relationship not preloaded)
+      assert match?(%Ash.NotLoaded{}, user_without_role_loaded.role)
+
+      # ensure_loaded should load it
+      result = Actor.ensure_loaded(user_without_role_loaded)
+      assert result.id == user.id
+      refute match?(%Ash.NotLoaded{}, result.role)
+      assert result.role.id == role.id
+    end
+
+    test "handles load errors gracefully (returns original actor)" do
+      # Create a plain map (not a real Ash resource)
+      fake_actor = %{id: "fake", role: %Ash.NotLoaded{field: :role}}
+
+      # Should not crash, returns original
+      result = Actor.ensure_loaded(fake_actor)
+      assert result == fake_actor
+    end
+  end
+end