From 6846363132f33d8b30a4f1df10b01dcdbb3999a6 Mon Sep 17 00:00:00 2001
From: Moritz <moritz.m@local-it.org>
Date: Thu, 8 Jan 2026 22:54:49 +0100
Subject: [PATCH] Refactor: NoActor to SimpleCheck with compile-time
 environment check

This prevents security issues where :create/:read without actor would
be allowed in production. Now all operations require an actor in production.
---
 lib/membership/member.ex                |  9 ++--
 lib/mv/authorization/checks/no_actor.ex | 66 +++++++++++++++----------
 2 files changed, 45 insertions(+), 30 deletions(-)

diff --git a/lib/membership/member.ex b/lib/membership/member.ex
index 0cd7174..b26e3b4 100644
--- a/lib/membership/member.ex
+++ b/lib/membership/member.ex
@@ -300,11 +300,12 @@ defmodule Mv.Membership.Member do
   # Authorization Policies
   # Order matters: Most specific policies first, then general permission check
   policies do
-    # SYSTEM OPERATIONS: Allow operations without actor (seeds, tests, system jobs)
-    # This must come first to allow database seeding and test fixtures
-    # IMPORTANT: Use bypass so this short-circuits and doesn't require other policies
+    # SYSTEM OPERATIONS: Allow CRUD operations without actor
+    # In test: All operations allowed (for test fixtures)
+    # In production: Only :create and :read allowed (enforced by NoActor.check)
+    # :read is needed for internal Ash lookups (e.g., relationship validation during user creation).
     bypass action_type([:create, :read, :update, :destroy]) do
-      description "Allow system operations without actor (seeds, tests)"
+      description "Allow system operations without actor (seeds, tests, internal lookups)"
       authorize_if Mv.Authorization.Checks.NoActor
     end
 
diff --git a/lib/mv/authorization/checks/no_actor.ex b/lib/mv/authorization/checks/no_actor.ex
index c80ea9b..f5eebb7 100644
--- a/lib/mv/authorization/checks/no_actor.ex
+++ b/lib/mv/authorization/checks/no_actor.ex
@@ -2,22 +2,27 @@ defmodule Mv.Authorization.Checks.NoActor do
   @moduledoc """
   Custom Ash Policy Check that allows actions when no actor is present.
 
-  This is primarily used for:
-  - Database seeding (priv/repo/seeds.exs)
-  - Test fixtures that create data without authentication
-  - Background jobs that operate on behalf of the system
+  **IMPORTANT:** This check ONLY works in test environment for security reasons.
+  In production/dev, ALL operations without an actor are denied.
 
   ## Security Note
 
-  This check should only be used for specific actions where system-level
-  access is appropriate. It should always be combined with other policy
-  checks that validate actor-based permissions when an actor IS present.
+  This check uses compile-time environment detection to prevent accidental
+  security issues in production. In production, ALL operations (including :create
+  and :read) will be denied if no actor is present.
+
+  For seeds and system operations in production, use an admin actor instead:
+
+      admin_user = get_admin_user()
+      Ash.create!(resource, attrs, actor: admin_user)
 
   ## Usage in Policies
 
       policies do
-        # Allow seeding and system operations
-        policy action_type(:create) do
+        # Allow system operations without actor (TEST ENVIRONMENT ONLY)
+        # In test: All operations allowed
+        # In production: ALL operations denied (fail-closed)
+        bypass action_type([:create, :read, :update, :destroy]) do
           authorize_if NoActor
         end
         
@@ -29,32 +34,41 @@ defmodule Mv.Authorization.Checks.NoActor do
 
   ## Behavior
 
-  - Returns `{:ok, true}` when actor is nil (allows action)
-  - Returns `{:ok, :unknown}` when actor is present (delegates to other policies)
-  - `auto_filter` returns nil (no filtering needed)
+  - In test environment: Returns `true` when actor is nil (allows all operations)
+  - In production/dev: Returns `false` when actor is nil (denies all operations - fail-closed)
+  - Returns `false` when actor is present (delegates to other policies)
   """
 
-  use Ash.Policy.Check
+  use Ash.Policy.SimpleCheck
+
+  # Compile-time check: Only allow no-actor bypass in test environment
+  @allow_no_actor_bypass Mix.env() == :test
+  # Alternative (if you want to control via config):
+  # @allow_no_actor_bypass Application.compile_env(:mv, :allow_no_actor_bypass, false)
 
   @impl true
   def describe(_opts) do
-    "allows actions when no actor is present (for seeds and system operations)"
-  end
-
-  @impl true
-  def strict_check(actor, _authorizer, _opts) do
-    if is_nil(actor) do
-      # No actor present - allow (for seeds, tests, system operations)
-      {:ok, true}
+    if @allow_no_actor_bypass do
+      "allows actions when no actor is present (test environment only)"
     else
-      # Actor present - let other policies decide
-      {:ok, :unknown}
+      "denies all actions when no actor is present (production/dev - fail-closed)"
     end
   end
 
   @impl true
-  def auto_filter(_actor, _authorizer, _opts) do
-    # No filtering needed - this check only validates presence/absence of actor
-    nil
+  def match?(nil, _context, _opts) do
+    # Actor is nil
+    if @allow_no_actor_bypass do
+      # Test environment: Allow all operations
+      true
+    else
+      # Production/dev: Deny all operations (fail-closed for security)
+      false
+    end
+  end
+
+  def match?(_actor, _context, _opts) do
+    # Actor is present - don't match (let other policies decide)
+    false
   end
 end