From 56144a76961f746d7f339b80d0025ea941da4fef Mon Sep 17 00:00:00 2001
From: Moritz <moritz.m@local-it.org>
Date: Thu, 22 Jan 2026 21:36:10 +0100
Subject: [PATCH] Add role loading fallback to HasPermission check

Extract ash_resource? helper to reduce nesting depth.
Add ensure_role_loaded fallback for unloaded actor roles.
---
 lib/mv/authorization/checks/has_permission.ex | 65 ++++++++++++++++++-
 .../checks/has_permission_test.exs            | 40 ++++++++++++
 2 files changed, 104 insertions(+), 1 deletion(-)

diff --git a/lib/mv/authorization/checks/has_permission.ex b/lib/mv/authorization/checks/has_permission.ex
index f3192fb..865b2a9 100644
--- a/lib/mv/authorization/checks/has_permission.ex
+++ b/lib/mv/authorization/checks/has_permission.ex
@@ -8,10 +8,37 @@ defmodule Mv.Authorization.Checks.HasPermission do
   3. Finds matching permission for current resource + action
   4. Applies scope filter (:own, :linked, :all)
 
+  ## Important: strict_check Behavior
+
+  For filter-based scopes (`:own`, `:linked`):
+  - **WITH record**: Evaluates filter against record (returns `true`/`false`)
+  - **WITHOUT record** (queries/lists): Returns `false`
+
+  **Why `false` instead of `:unknown`?**
+
+  Ash's policy evaluation doesn't reliably call `auto_filter` when `strict_check`
+  returns `:unknown`. To ensure list queries work correctly, resources **MUST** use
+  bypass policies with `expr()` for READ operations (see `docs/policy-bypass-vs-haspermission.md`).
+
+  This means `HasPermission` is **NOT** generically reusable for query authorization
+  with filter scopes - it requires companion bypass policies.
+
+  ## Usage Pattern
+
+  See `docs/policy-bypass-vs-haspermission.md` for the two-tier pattern:
+  - **READ**: `bypass` with `expr()` (handles auto_filter)
+  - **UPDATE/CREATE/DESTROY**: `HasPermission` (handles scope evaluation)
+
   ## Usage in Ash Resource
 
       policies do
-        policy action_type(:read) do
+        # READ: Bypass for list queries
+        bypass action_type(:read) do
+          authorize_if expr(id == ^actor(:id))
+        end
+
+        # UPDATE: HasPermission for scope evaluation
+        policy action_type([:update, :create, :destroy]) do
           authorize_if Mv.Authorization.Checks.HasPermission
         end
       end
@@ -34,6 +61,12 @@ defmodule Mv.Authorization.Checks.HasPermission do
 
   All errors result in Forbidden (policy fails).
 
+  ## Role Loading Fallback
+
+  If the actor's `:role` relationship is `%Ash.NotLoaded{}`, this check will
+  attempt to load it automatically. This provides a fallback if `on_mount` hooks
+  didn't run (e.g., in non-LiveView contexts).
+
   ## Examples
 
       # In a resource policy
@@ -83,6 +116,9 @@ defmodule Mv.Authorization.Checks.HasPermission do
 
   # Helper function to reduce nesting depth
   defp strict_check_with_permissions(actor, resource, action, record) do
+    # Ensure role is loaded (fallback if on_mount didn't run)
+    actor = ensure_role_loaded(actor)
+
     with %{role: %{permission_set_name: ps_name}} when not is_nil(ps_name) <- actor,
          {:ok, ps_atom} <- PermissionSets.permission_set_name_to_atom(ps_name),
          permissions <- PermissionSets.get_permissions(ps_atom),
@@ -353,4 +389,31 @@ defmodule Mv.Authorization.Checks.HasPermission do
   defp get_resource_name_for_logging(_resource) do
     "unknown"
   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
+  end
 end
diff --git a/test/mv/authorization/checks/has_permission_test.exs b/test/mv/authorization/checks/has_permission_test.exs
index 750bcc7..4e2dcea 100644
--- a/test/mv/authorization/checks/has_permission_test.exs
+++ b/test/mv/authorization/checks/has_permission_test.exs
@@ -274,4 +274,44 @@ defmodule Mv.Authorization.Checks.HasPermissionTest do
       end
     end
   end
+
+  describe "strict_check/3 - Role Loading Fallback" do
+    test "returns false if role is NotLoaded and cannot be loaded" do
+      # Create actor with NotLoaded role
+      # In real scenario, ensure_role_loaded would attempt to load via Ash.load
+      # For this test, we use a simple map to verify the pattern matching works
+      actor = %{
+        id: "user-123",
+        role: %Ash.NotLoaded{}
+      }
+
+      authorizer = create_authorizer(Mv.Accounts.User, :read)
+
+      # Should handle NotLoaded pattern and return false
+      # (In real scenario, ensure_role_loaded would attempt to load, but for this test
+      # we just verify the pattern matching works correctly)
+      {:ok, result} = HasPermission.strict_check(actor, authorizer, [])
+      assert result == false
+    end
+
+    test "returns false if role is nil" do
+      actor = %{
+        id: "user-123",
+        role: nil
+      }
+
+      authorizer = create_authorizer(Mv.Accounts.User, :read)
+
+      {:ok, result} = HasPermission.strict_check(actor, authorizer, [])
+      assert result == false
+    end
+
+    test "works correctly when role is already loaded" do
+      actor = create_actor("user-123", "admin")
+      authorizer = create_authorizer(Mv.Accounts.User, :read)
+
+      {:ok, result} = HasPermission.strict_check(actor, authorizer, [])
+      assert result == true
+    end
+  end
 end