Fix: HasPermission auto_filter and strict_check implementation

Fixes security issue where auto_filter returned nil instead of proper
filter expressions, which could lead to incorrect authorization behavior.

lib/mv/authorization/checks/has_permission.ex

@@ -21,8 +21,8 @@ defmodule Mv.Authorization.Checks.HasPermission do
   - **:all** - Authorizes without filtering (returns all records)
   - **:own** - Filters to records where record.id == actor.id
   - **:linked** - Filters based on resource type:
-    - Member: member.user.id == actor.id (via has_one :user relationship)
-    - CustomFieldValue: custom_field_value.member.user.id == actor.id (traverses member → user relationship!)
+    - Member: `id == actor.member_id` (User.member_id → Member.id, inverse relationship)
+    - CustomFieldValue: `member_id == actor.member_id` (CustomFieldValue.member_id → Member.id → User.member_id)
 
   ## Error Handling
 
@@ -129,9 +129,18 @@ defmodule Mv.Authorization.Checks.HasPermission do
     action = get_action_from_authorizer(authorizer)
 
     cond do
-      is_nil(actor) -> nil
-      is_nil(action) -> nil
-      true -> auto_filter_with_permissions(actor, resource, action)
+      is_nil(actor) ->
+        # No actor - deny access (fail-closed)
+        # Return filter that never matches (using impossible condition)
+        # This ensures no records are returned when actor is missing
+        [id: {:not, {:in, []}}]
+
+      is_nil(action) ->
+        # Cannot determine action - deny access (fail-closed)
+        [id: {:not, {:in, []}}]
+
+      true ->
+        auto_filter_with_permissions(actor, resource, action)
     end
   end
 
@@ -148,12 +157,25 @@ defmodule Mv.Authorization.Checks.HasPermission do
              actor,
              resource_name
            ) do
-        :authorized -> nil
-        {:filter, filter_expr} -> filter_expr
-        false -> nil
+        :authorized ->
+          # :all scope - allow all records (no filter)
+          # Return empty keyword list (no filtering)
+          []
+
+        {:filter, filter_expr} ->
+          # :linked or :own scope - apply filter
+          # filter_expr is a keyword list from expr(...), return it directly
+          filter_expr
+
+        false ->
+          # No permission - deny access (fail-closed)
+          # Return filter that never matches (using impossible condition)
+          [id: {:not, {:in, []}}]
       end
     else
-      _ -> nil
+      _ ->
+        # Error case (no role, invalid permission set, etc.) - deny access (fail-closed)
+        [id: {:not, {:in, []}}]
     end
   end
 
@@ -162,17 +184,27 @@ defmodule Mv.Authorization.Checks.HasPermission do
   # Action types: :create, :read, :update, :destroy
   # Action names: :create_member, :update_member, etc.
   # PermissionSets uses action types, not action names
+  #
+  # Prefer authorizer.action.type (stable API) over authorizer.subject (varies by context)
   defp get_action_from_authorizer(authorizer) do
-    case authorizer.subject do
-      %{action_type: action_type} when action_type in [:create, :read, :update, :destroy] ->
-        action_type
-
-      # Fallback for older Ash versions or different subject shapes
-      %{action: %{type: action_type}} when action_type in [:create, :read, :update, :destroy] ->
+    # Primary: Use authorizer.action.type (stable API)
+    case Map.get(authorizer, :action) do
+      %{type: action_type} when action_type in [:create, :read, :update, :destroy] ->
         action_type
 
       _ ->
-        nil
+        # Fallback: Try authorizer.subject (for compatibility with different Ash versions/contexts)
+        case Map.get(authorizer, :subject) do
+          %{action_type: action_type} when action_type in [:create, :read, :update, :destroy] ->
+            action_type
+
+          %{action: %{type: action_type}}
+          when action_type in [:create, :read, :update, :destroy] ->
+            action_type
+
+          _ ->
+            nil
+        end
     end
   end
 

test/mv/authorization/checks/has_permission_integration_test.exs

@@ -72,14 +72,15 @@ defmodule Mv.Authorization.Checks.HasPermissionIntegrationTest do
   end
 
   describe "Filter Expression Structure - :all scope" do
-    test "Admin can read all members without filter" do
+    test "Admin can read all members without filter (returns expr(true))" do
       actor = create_actor_with_role("admin")
       authorizer = create_authorizer(Mv.Membership.Member, :read)
 
       filter = HasPermission.auto_filter(actor, authorizer, [])
 
-      # :all scope should return nil (no filter needed)
-      assert is_nil(filter)
+      # :all scope should return [] (empty keyword list = no filter = allow all records)
+      # After auto_filter fix: no longer returns nil, returns [] instead
+      assert filter == []
     end
   end
 

test/mv/membership/member_policies_test.exs

@@ -132,6 +132,8 @@ defmodule Mv.Membership.MemberPoliciesTest do
     end
 
     test "can update linked member", %{user: user, linked_member: linked_member} do
+      # Update is allowed via HasPermission check with :linked scope (not via special case)
+      # The special case policy only applies to :read actions
       {:ok, updated_member} =
         linked_member
         |> Ash.Changeset.for_update(:update_member, %{first_name: "Updated"})
@@ -367,23 +369,14 @@ defmodule Mv.Membership.MemberPoliciesTest do
     end
   end
 
-  describe "special case: user can always access linked member" do
-    test "own_data user can read linked member even without explicit permission" do
-      user = create_user_with_permission_set("own_data")
-      linked_member = create_linked_member_for_user(user)
+  describe "special case: user can always READ linked member" do
+    # Note: The special case policy only applies to :read actions.
+    # Updates are handled by HasPermission with :linked scope (if permission exists).
 
-      # Reload user to get updated member_id
-      {:ok, user} = Ash.get(Accounts.User, user.id, domain: Mv.Accounts, load: [:role])
-      {:ok, user} = Ash.load(user, :member, domain: Mv.Accounts)
-
-      # Should succeed (special case policy takes precedence)
-      {:ok, member} =
-        Ash.get(Membership.Member, linked_member.id, actor: user, domain: Mv.Membership)
-
-      assert member.id == linked_member.id
-    end
-
-    test "read_only user can read linked member (via special case)" do
+    test "read_only user can read linked member (via special case bypass)" do
+      # read_only has Member.read scope :all, but the special case ensures
+      # users can ALWAYS read their linked member, even if they had no read permission.
+      # This test verifies the special case works independently of permission sets.
       user = create_user_with_permission_set("read_only")
       linked_member = create_linked_member_for_user(user)
 
@@ -391,14 +384,16 @@ defmodule Mv.Membership.MemberPoliciesTest do
       {:ok, user} = Ash.get(Accounts.User, user.id, domain: Mv.Accounts, load: [:role])
       {:ok, user} = Ash.load(user, :member, domain: Mv.Accounts)
 
-      # Should succeed (special case policy takes precedence)
+      # Should succeed (special case bypass policy for :read takes precedence)
       {:ok, member} =
         Ash.get(Membership.Member, linked_member.id, actor: user, domain: Mv.Membership)
 
       assert member.id == linked_member.id
     end
 
-    test "own_data user can update linked member even without explicit permission" do
+    test "own_data user can read linked member (via special case bypass)" do
+      # own_data has Member.read scope :linked, but the special case ensures
+      # users can ALWAYS read their linked member regardless of permission set.
       user = create_user_with_permission_set("own_data")
       linked_member = create_linked_member_for_user(user)
 
@@ -406,7 +401,24 @@ defmodule Mv.Membership.MemberPoliciesTest do
       {:ok, user} = Ash.get(Accounts.User, user.id, domain: Mv.Accounts, load: [:role])
       {:ok, user} = Ash.load(user, :member, domain: Mv.Accounts)
 
-      # Should succeed (special case policy takes precedence)
+      # Should succeed (special case bypass policy for :read takes precedence)
+      {:ok, member} =
+        Ash.get(Membership.Member, linked_member.id, actor: user, domain: Mv.Membership)
+
+      assert member.id == linked_member.id
+    end
+
+    test "own_data user can update linked member (via HasPermission :linked scope)" do
+      # Update is NOT handled by special case - it's handled by HasPermission
+      # with :linked scope. own_data has Member.update scope :linked.
+      user = create_user_with_permission_set("own_data")
+      linked_member = create_linked_member_for_user(user)
+
+      # Reload user to get updated member_id
+      {:ok, user} = Ash.get(Accounts.User, user.id, domain: Mv.Accounts, load: [:role])
+      {:ok, user} = Ash.load(user, :member, domain: Mv.Accounts)
+
+      # Should succeed via HasPermission check (not special case)
       {:ok, updated_member} =
         linked_member
         |> Ash.Changeset.for_update(:update_member, %{first_name: "Updated"})