From dd4b88f0b7773d93c6090c724fe0c3510171ee0b Mon Sep 17 00:00:00 2001
From: Moritz <moritz.m@local-it.org>
Date: Thu, 8 Jan 2026 23:29:37 +0100
Subject: [PATCH] Improve: Make deny_filter robust and add regression test

- Change deny_filter from [id: {:in, []}] to expr(false)
- Add regression test to ensure deny-filter matches 0 records
---
 lib/mv/authorization/checks/has_permission.ex |  7 +--
 .../has_permission_fail_closed_test.exs       | 44 +++++++++++++++++++
 2 files changed, 48 insertions(+), 3 deletions(-)
 create mode 100644 test/mv/authorization/checks/has_permission_fail_closed_test.exs

diff --git a/lib/mv/authorization/checks/has_permission.ex b/lib/mv/authorization/checks/has_permission.ex
index 68d83d1..31c18cb 100644
--- a/lib/mv/authorization/checks/has_permission.ex
+++ b/lib/mv/authorization/checks/has_permission.ex
@@ -179,10 +179,11 @@ defmodule Mv.Authorization.Checks.HasPermission do
 
   # Helper function to return a filter that never matches (deny all records)
   # Used when authorization should be denied (fail-closed)
-  # Returns [id: {:in, []}] which means "id IN []" - never matches (correct deny-all)
-  # NOTE: [id: {:not, {:in, []}}] would be "NOT (id IN [])" = true for all IDs (allow-all) - WRONG!
+  #
+  # Using `expr(false)` avoids depending on the primary key being named `:id`.
+  # This is more robust than [id: {:in, []}] which assumes the primary key is `:id`.
   defp deny_filter do
-    [id: {:in, []}]
+    expr(false)
   end
 
   # Helper to extract action type from authorizer
diff --git a/test/mv/authorization/checks/has_permission_fail_closed_test.exs b/test/mv/authorization/checks/has_permission_fail_closed_test.exs
new file mode 100644
index 0000000..822e5aa
--- /dev/null
+++ b/test/mv/authorization/checks/has_permission_fail_closed_test.exs
@@ -0,0 +1,44 @@
+defmodule Mv.Authorization.Checks.HasPermissionFailClosedTest do
+  @moduledoc """
+  Regression tests to ensure deny-filter behavior is fail-closed (matches no records).
+
+  These tests verify that when HasPermission.auto_filter returns a deny-filter
+  (e.g., when actor is nil or no permission is found), the filter actually
+  matches zero records in the database.
+
+  This prevents regressions like the previous bug where [id: {:not, {:in, []}}]
+  was used, which logically evaluates to "NOT (id IN [])" = true for all IDs,
+  effectively allowing all records instead of denying them.
+  """
+  use Mv.DataCase, async: true
+
+  alias Mv.Authorization.Checks.HasPermission
+
+  import Mv.Fixtures
+
+  test "auto_filter deny-filter matches no records (regression for NOT IN [] allow-all bug)" do
+    # Arrange: create some members in DB
+    _m1 = member_fixture()
+    _m2 = member_fixture()
+
+    # Build a minimal authorizer with a stable action type (:read)
+    authorizer = %Ash.Policy.Authorizer{
+      resource: Mv.Membership.Member,
+      action: %{type: :read}
+    }
+
+    # Act: missing actor must yield a deny-all filter (fail-closed)
+    deny_filter = HasPermission.auto_filter(nil, authorizer, [])
+
+    # Apply the returned filter to a real DB query (no authorization involved)
+    query =
+      Mv.Membership.Member
+      |> Ash.Query.new()
+      |> Ash.Query.filter_input(deny_filter)
+
+    {:ok, results} = Ash.read(query, domain: Mv.Membership, authorize?: false)
+
+    # Assert: deny-filter must match nothing
+    assert results == []
+  end
+end