From ee50f312ee83461478a2382125b462288a05c036 Mon Sep 17 00:00:00 2001 From: Moritz Date: Sat, 24 Jan 2026 10:43:20 +0100 Subject: [PATCH] Remove NoActor module, improve Member validation, update docs --- CODE_GUIDELINES.md | 18 +++++ lib/membership/member.ex | 27 +++++-- lib/mv/authorization/checks/no_actor.ex | 70 ------------------- .../mv/authorization/checks/no_actor_test.exs | 52 -------------- 4 files changed, 40 insertions(+), 127 deletions(-) delete mode 100644 lib/mv/authorization/checks/no_actor.ex delete mode 100644 test/mv/authorization/checks/no_actor_test.exs diff --git a/CODE_GUIDELINES.md b/CODE_GUIDELINES.md index 987be42..17b03d0 100644 --- a/CODE_GUIDELINES.md +++ b/CODE_GUIDELINES.md @@ -1699,6 +1699,24 @@ end **IMPORTANT:** All tests must explicitly provide an actor for Ash operations. The NoActor bypass has been removed to prevent masking authorization bugs. +**Exception: AshAuthentication Bypass Tests** + +Tests that verify the AshAuthentication bypass mechanism are a **conscious exception**. These tests must verify that registration/login works **without an actor** via the `AshAuthenticationInteraction` check. To enable this bypass in tests, set the context explicitly: + +```elixir +# ✅ GOOD - Testing AshAuthentication bypass (conscious exception) +changeset = + Accounts.User + |> Ash.Changeset.for_create(:register_with_password, %{...}) + |> Ash.Changeset.set_context(%{private: %{ash_authentication?: true}}) + +{:ok, user} = Ash.create(changeset) # No actor - tests bypass mechanism + +# ❌ BAD - Using system_actor masks the bypass test +system_actor = Mv.Helpers.SystemActor.get_system_actor() +Ash.create(changeset, actor: system_actor) # Tests admin permissions, not bypass! +``` + **Test Fixtures:** All test fixtures use `system_actor` for authorization: diff --git a/lib/membership/member.ex b/lib/membership/member.ex index f2f27c0..1a5d805 100644 --- a/lib/membership/member.ex +++ b/lib/membership/member.ex @@ -393,11 +393,28 @@ defmodule Mv.Membership.Member do user_id = user_arg[:id] current_member_id = changeset.data.id - # This is an integrity check, not a user authorization check - # Use authorize?: false to bypass policies for this internal validation query - # This ensures the validation always works regardless of actor availability - # (consistent with MembershipFeeType destroy validations) - case Ash.get(Mv.Accounts.User, user_id, authorize?: false) do + # Get actor from changeset context (may be nil) + actor = Map.get(changeset.context || %{}, :actor) + + # Check if authorization is disabled in the parent operation's context + # Access private context where authorize? flag is stored + authorize? = + case get_in(changeset.context, [:private, :authorize?]) do + false -> false + _ -> true + end + + # Use actor for authorization when available and authorize? is true + # Fall back to authorize?: false only for bootstrap/system operations + # This ensures normal operations respect authorization while system operations work + query_opts = + if actor && authorize? do + [actor: actor] + else + [authorize?: false] + end + + case Ash.get(Mv.Accounts.User, user_id, query_opts) do # User is free to be linked {:ok, %{member_id: nil}} -> :ok diff --git a/lib/mv/authorization/checks/no_actor.ex b/lib/mv/authorization/checks/no_actor.ex deleted file mode 100644 index 1c4946f..0000000 --- a/lib/mv/authorization/checks/no_actor.ex +++ /dev/null @@ -1,70 +0,0 @@ -defmodule Mv.Authorization.Checks.NoActor do - @moduledoc """ - Custom Ash Policy Check that allows actions when no actor is present. - - **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 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 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 - - # Check permissions when actor is present - policy action_type([:read, :create, :update, :destroy]) do - authorize_if HasPermission - end - end - - ## Behavior - - - 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.SimpleCheck - - # Compile-time check: Only allow no-actor bypass in test environment - # SECURITY: This must ONLY be true in test.exs, never in prod/dev - # Using compile_env instead of Mix.env() for release-safety - @allow_no_actor_bypass Application.compile_env(:mv, :allow_no_actor_bypass, false) - - @impl true - def describe(_opts) do - if @allow_no_actor_bypass do - "allows actions when no actor is present (test environment only)" - else - "denies all actions when no actor is present (production/dev - fail-closed)" - end - end - - @impl true - def match?(nil, _context, _opts) do - # Actor is nil - # SECURITY: Only allow if compile_env flag is set (test.exs only) - # No runtime Mix.env() check - fail-closed by default (false) - @allow_no_actor_bypass - end - - def match?(_actor, _context, _opts) do - # Actor is present - don't match (let other policies decide) - false - end -end diff --git a/test/mv/authorization/checks/no_actor_test.exs b/test/mv/authorization/checks/no_actor_test.exs deleted file mode 100644 index 35205a6..0000000 --- a/test/mv/authorization/checks/no_actor_test.exs +++ /dev/null @@ -1,52 +0,0 @@ -defmodule Mv.Authorization.Checks.NoActorTest do - @moduledoc """ - Tests for the NoActor Ash Policy Check. - - This check allows actions without an actor ONLY in test environment. - In production/dev, all operations without an actor are denied. - """ - use ExUnit.Case, async: true - - alias Mv.Authorization.Checks.NoActor - - describe "match?/3" do - test "returns true when actor is nil in test environment" do - # In test environment (config :allow_no_actor_bypass = true), NoActor allows operations - result = NoActor.match?(nil, %{}, []) - assert result == true - end - - test "returns false when actor is present" do - actor = %{id: "user-123"} - result = NoActor.match?(actor, %{}, []) - assert result == false - end - - test "uses compile-time config (not runtime Mix.env)" do - # The @allow_no_actor_bypass is set via Application.compile_env at compile time - # In test.exs: config :mv, :allow_no_actor_bypass, true - # In prod/dev: not set (defaults to false) - # This ensures the check is release-safe (no runtime Mix.env dependency) - result = NoActor.match?(nil, %{}, []) - - # In test environment (as compiled), should allow - assert result == true - - # Note: We cannot test "production mode" here because the flag is compile-time. - # Production safety is guaranteed by: - # 1. Config only set in test.exs - # 2. Default is false (fail-closed) - # 3. No runtime environment checks - end - end - - describe "describe/1" do - test "returns description based on compile-time config" do - description = NoActor.describe([]) - assert is_binary(description) - - # In test environment (compiled with :allow_no_actor_bypass = true) - assert description =~ "test environment" - end - end -end