[Refactor] Remove NoActor bypass #367
4 changed files with 40 additions and 127 deletions
|
|
@ -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:
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
@ -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
|
||||
Loading…
Add table
Add a link
Reference in a new issue