From d9eb131d96425a8028f996a53730027c5926c58b Mon Sep 17 00:00:00 2001 From: Moritz Date: Fri, 23 Jan 2026 20:18:28 +0100 Subject: [PATCH] Update documentation: Remove NoActor bypass references --- CODE_GUIDELINES.md | 92 ++++++++----------- docs/policy-bypass-vs-haspermission.md | 2 +- docs/roles-and-permissions-architecture.md | 88 +++++------------- ...les-and-permissions-implementation-plan.md | 15 +-- ...esource-policies-implementation-summary.md | 13 +-- 5 files changed, 73 insertions(+), 137 deletions(-) diff --git a/CODE_GUIDELINES.md b/CODE_GUIDELINES.md index 5bee497..987be42 100644 --- a/CODE_GUIDELINES.md +++ b/CODE_GUIDELINES.md @@ -690,16 +690,9 @@ end **Authorization Bootstrap Patterns:** -Three mechanisms exist for bypassing standard authorization: +Two mechanisms exist for bypassing standard authorization: -1. **NoActor** (test only) - Allows operations without actor in test environment - ```elixir - # Automatically enabled in tests via config/test.exs - # Policies use: bypass action_type(...) do authorize_if NoActor end - member = create_member(%{name: "Test"}) # Works in tests - ``` - -2. **system_actor** (systemic operations) - Admin user for operations that must always succeed +1. **system_actor** (systemic operations) - Admin user for operations that must always succeed ```elixir # Good: Systemic operation system_actor = SystemActor.get_system_actor() @@ -709,7 +702,7 @@ Three mechanisms exist for bypassing standard authorization: # Never use system_actor for user-initiated actions! ``` -3. **authorize?: false** (bootstrap only) - Skips policies for circular dependencies +2. **authorize?: false** (bootstrap only) - Skips policies for circular dependencies ```elixir # Good: Bootstrap (seeds, SystemActor loading) Accounts.create_user!(%{email: admin_email}, authorize?: false) @@ -719,10 +712,10 @@ Three mechanisms exist for bypassing standard authorization: ``` **Decision Guide:** -- Use **NoActor** for test fixtures (automatic via config) -- Use **system_actor** for email sync, cycle generation, validations +- Use **system_actor** for email sync, cycle generation, validations, and test fixtures - Use **authorize?: false** only for bootstrap (seeds, circular dependencies) - Always document why `authorize?: false` is necessary +- **Note:** NoActor bypass was removed to prevent masking authorization bugs in tests **See also:** `docs/roles-and-permissions-architecture.md` (Authorization Bootstrap Patterns section) @@ -1702,65 +1695,54 @@ case Ash.read(Mv.Membership.Member, actor: actor) do end ``` -### 5.1a NoActor Pattern - Test Environment Only +### 5.1a Authorization in Tests -**IMPORTANT:** The `Mv.Authorization.Checks.NoActor` check is **ONLY for test environment**. It must NEVER be used in production. +**IMPORTANT:** All tests must explicitly provide an actor for Ash operations. The NoActor bypass has been removed to prevent masking authorization bugs. -**What NoActor Does:** +**Test Fixtures:** -- Allows CRUD operations without an actor in **test environment only** -- Denies all operations without an actor in **production/dev** (fail-closed) -- Uses compile-time config check to prevent accidental production use (release-safe) - -**Security Guards:** +All test fixtures use `system_actor` for authorization: ```elixir -# config/test.exs -config :mv, :allow_no_actor_bypass, true - -# lib/mv/authorization/checks/no_actor.ex -# Compile-time check from config (release-safe, no Mix.env) -@allow_no_actor_bypass Application.compile_env(:mv, :allow_no_actor_bypass, false) - -# Uses compile-time flag only (no runtime Mix.env needed) -def match?(nil, _context, _opts) do - @allow_no_actor_bypass # true in test, false in prod/dev +# test/support/fixtures.ex +def member_fixture(attrs \\ %{}) do + system_actor = Mv.Helpers.SystemActor.get_system_actor() + + attrs + |> Enum.into(%{...}) + |> Mv.Membership.create_member(actor: system_actor) end ``` -**Why This Pattern Exists:** +**Why Explicit Actors in Tests:** -- Test fixtures often need to create resources without an actor -- Production operations MUST always have an actor for security -- Config-based guard (not Mix.env) ensures release-safety -- Defaults to `false` (fail-closed) if config not set +- Prevents masking authorization bugs +- Makes authorization requirements explicit +- Tests fail if authorization is broken (fail-fast) +- Consistent with production code patterns -**NEVER Use NoActor in Production:** +**Using system_actor in Tests:** ```elixir -# ❌ BAD - Don't do this in production code -Ash.create!(Member, attrs) # No actor - will fail in prod - -# ✅ GOOD - Use admin actor for system operations -admin_user = get_admin_user() -Ash.create!(Member, attrs, actor: admin_user) -``` - -**Alternative: System Actor Pattern** - -For production system operations, use the System Actor Pattern (see Section 3.3) instead of NoActor: - -```elixir -# System operations in production -system_actor = get_system_actor() +# ✅ GOOD - Explicit actor in tests +system_actor = Mv.Helpers.SystemActor.get_system_actor() Ash.create!(Member, attrs, actor: system_actor) + +# ❌ BAD - Missing actor (will fail) +Ash.create!(Member, attrs) # Forbidden error! ``` -**Testing:** +**For Bootstrap Operations:** -- NoActor tests verify the compile-time config guard -- Production safety is guaranteed by config (only set in test.exs, defaults to false) -- See `test/mv/authorization/checks/no_actor_test.exs` +Use `authorize?: false` only for bootstrap scenarios (seeds, SystemActor initialization): + +```elixir +# ✅ GOOD - Bootstrap only +Accounts.create_user!(%{email: admin_email}, authorize?: false) + +# ❌ BAD - Never use in tests for normal operations +Ash.create!(Member, attrs, authorize?: false) # Never do this! +``` ### 5.2 Password Security diff --git a/docs/policy-bypass-vs-haspermission.md b/docs/policy-bypass-vs-haspermission.md index 8a65c6f..31bb737 100644 --- a/docs/policy-bypass-vs-haspermission.md +++ b/docs/policy-bypass-vs-haspermission.md @@ -262,7 +262,7 @@ The bypass is not a design choice but a **technical necessity** due to Ash's pol - ✅ UPDATE operations via HasPermission with `scope :own` - ✅ Admin operations via HasPermission with `scope :all` - ✅ AshAuthentication bypass (registration/login) -- ✅ NoActor bypass (test environment) +- ✅ Tests use system_actor for authorization **Key Tests Proving Pattern:** diff --git a/docs/roles-and-permissions-architecture.md b/docs/roles-and-permissions-architecture.md index bc1b75c..8934688 100644 --- a/docs/roles-and-permissions-architecture.md +++ b/docs/roles-and-permissions-architecture.md @@ -946,12 +946,7 @@ defmodule Mv.Accounts.User do authorize_if always() end - # 2. NoActor Bypass (test environment only, for test fixtures) - bypass action_type([:create, :read, :update, :destroy]) do - authorize_if Mv.Authorization.Checks.NoActor - end - - # 3. SPECIAL CASE: Users can always READ their own account + # 2. SPECIAL CASE: Users can always READ their own account # Bypass needed for list queries (expr() triggers auto_filter in Ash) # UPDATE is handled by HasPermission below (scope :own works with changesets) bypass action_type(:read) do @@ -959,7 +954,7 @@ defmodule Mv.Accounts.User do authorize_if expr(id == ^actor(:id)) end - # 4. GENERAL: Check permissions from user's role + # 3. GENERAL: Check permissions from user's role # - :own_data → can UPDATE own user (scope :own via HasPermission) # - :read_only → can UPDATE own user (scope :own via HasPermission) # - :normal_user → can UPDATE own user (scope :own via HasPermission) @@ -969,7 +964,7 @@ defmodule Mv.Accounts.User do authorize_if Mv.Authorization.Checks.HasPermission end - # 5. DEFAULT: Ash implicitly forbids if no policy authorizes (fail-closed) + # 4. DEFAULT: Ash implicitly forbids if no policy authorizes (fail-closed) end # ... @@ -1007,12 +1002,7 @@ defmodule Mv.Membership.Member do use Ash.Resource, ... policies do - # 1. NoActor Bypass (test environment only, for test fixtures) - bypass action_type([:create, :read, :update, :destroy]) do - authorize_if Mv.Authorization.Checks.NoActor - end - - # 2. SPECIAL CASE: Users can always READ their linked member + # 1. SPECIAL CASE: Users can always READ their linked member # Bypass needed for list queries (expr() triggers auto_filter in Ash) # UPDATE is handled by HasPermission below (scope :linked works with changesets) bypass action_type(:read) do @@ -1020,7 +1010,7 @@ defmodule Mv.Membership.Member do authorize_if expr(id == ^actor(:member_id)) end - # 3. GENERAL: Check permissions from role + # 2. GENERAL: Check permissions from role # - :own_data → can UPDATE linked member (scope :linked via HasPermission) # - :read_only → can READ all members (scope :all), no update permission # - :normal_user → can CRUD all members (scope :all) @@ -2629,45 +2619,16 @@ This section clarifies three different mechanisms for bypassing standard authori ### Overview -The codebase uses three authorization bypass mechanisms: +The codebase uses two authorization bypass mechanisms: -1. **NoActor** - Test-only bypass (compile-time secured) -2. **system_actor** - Admin user for systemic operations -3. **authorize?: false** - Bootstrap bypass for circular dependencies +1. **system_actor** - Admin user for systemic operations +2. **authorize?: false** - Bootstrap bypass for circular dependencies -**All three are necessary and serve different purposes.** +**Both are necessary and serve different purposes.** -### 1. NoActor Check +**Note:** The NoActor bypass has been removed to prevent masking authorization bugs in tests. All tests now explicitly use `system_actor` for authorization. -**Purpose:** Allows CRUD operations without actor in test environment only. - -**Implementation:** -```elixir -# lib/mv/authorization/checks/no_actor.ex -@allow_no_actor_bypass Application.compile_env(:mv, :allow_no_actor_bypass, false) - -def match?(nil, _context, _opts) do - @allow_no_actor_bypass # true in test.exs, false elsewhere -end -``` - -**Security:** -- Compile-time flag (not runtime `Mix.env()` check) -- Default: false (fail-closed) -- Only enabled in `config/test.exs` - -**Use Case:** Test fixtures without verbose actor setup: -```elixir -# With NoActor (test environment only) -member = create_member(%{name: "Test"}) - -# Production behavior (NoActor returns false) -member = create_member(%{name: "Test"}, actor: user) -``` - -**Trade-off:** May mask tests that should fail without actor. Mitigated by explicit policy tests (e.g., `test/mv/accounts/user_policies_test.exs`). - -### 2. System Actor +### 1. System Actor **Purpose:** Admin user for systemic operations that must always succeed regardless of user permissions. @@ -2708,7 +2669,7 @@ end - Consistent authorization flow - Testable -### 3. authorize?: false +### 2. authorize?: false **Purpose:** Skip policies for bootstrap scenarios with circular dependencies. @@ -2759,21 +2720,17 @@ Mv.Authorization.Role ### Comparison -| Aspect | NoActor | system_actor | authorize?: false | -|--------|---------|--------------|-------------------| -| **Environment** | Test only | All | All | -| **Actor** | nil | Admin user | nil | -| **Policies** | Bypassed | Evaluated | Skipped | -| **Audit Trail** | No | Yes (system@mila.local) | No | -| **Use Case** | Test fixtures | Systemic operations | Bootstrap | -| **Explicit?** | Policy bypass | Function call | Query option | +| Aspect | system_actor | authorize?: false | +|--------|--------------|-------------------| +| **Environment** | All | All | +| **Actor** | Admin user | nil | +| **Policies** | Evaluated | Skipped | +| **Audit Trail** | Yes (system@mila.local) | No | +| **Use Case** | Systemic operations, test fixtures | Bootstrap | +| **Explicit?** | Function call | Query option | ### Decision Guide -**Use NoActor when:** -- ✅ Writing test fixtures -- ✅ Compile-time guard ensures test-only - **Use system_actor when:** - ✅ Systemic operation must always succeed - ✅ Email synchronization @@ -2789,7 +2746,7 @@ Mv.Authorization.Role **DON'T:** - ❌ Use `authorize?: false` for user-initiated actions - ❌ Use `authorize?: false` when `system_actor` would work -- ❌ Enable NoActor outside test environment +- ❌ Skip actor in tests (always use system_actor) ### The Circular Dependency Problem @@ -2873,7 +2830,8 @@ end - Enhanced edge case documentation **Changes from V2.0:** -- Added "Authorization Bootstrap Patterns" section explaining NoActor, system_actor, and authorize?: false +- Added "Authorization Bootstrap Patterns" section explaining system_actor and authorize?: false +- Removed NoActor bypass (all tests now use system_actor for explicit authorization) --- diff --git a/docs/roles-and-permissions-implementation-plan.md b/docs/roles-and-permissions-implementation-plan.md index 33b1702..23b045c 100644 --- a/docs/roles-and-permissions-implementation-plan.md +++ b/docs/roles-and-permissions-implementation-plan.md @@ -542,7 +542,7 @@ Following the same pattern as Member resource: 1. ✅ Open `lib/accounts/user.ex` 2. ✅ Add `policies` block 3. ✅ Add AshAuthentication bypass (registration/login without actor) -4. ✅ Add NoActor bypass (test environment only) +4. ✅ ~~Add NoActor bypass (test environment only)~~ **REMOVED** - NoActor bypass was removed to prevent masking authorization bugs. All tests now use `system_actor`. 5. ✅ Add bypass for READ: Allow user to always read their own account ```elixir bypass action_type(:read) do @@ -556,10 +556,11 @@ Following the same pattern as Member resource: **Policy Order:** 1. ✅ AshAuthentication bypass (registration/login) -2. ✅ NoActor bypass (test environment) -3. ✅ Bypass: User can READ own account (id == actor.id) -4. ✅ HasPermission: General permission check (UPDATE uses scope :own, admin uses scope :all) -5. ✅ Default: Ash implicitly forbids (fail-closed) +2. ✅ Bypass: User can READ own account (id == actor.id) +3. ✅ HasPermission: General permission check (UPDATE uses scope :own, admin uses scope :all) +4. ✅ Default: Ash implicitly forbids (fail-closed) + +**Note:** NoActor bypass was removed. All tests now use `system_actor` for authorization. **Why Bypass for READ but not UPDATE?** @@ -574,7 +575,7 @@ This ensures `scope :own` in PermissionSets is actually used (not redundant). - ✅ User can always update own credentials (via HasPermission with scope :own) - ✅ Only admin can read/update other users (scope :all) - ✅ Only admin can destroy users (scope :all) -- ✅ Policy order is correct (AshAuth → NoActor → Bypass READ → HasPermission) +- ✅ Policy order is correct (AshAuth → Bypass READ → HasPermission) - ✅ Actor preloads :role relationship - ✅ All tests pass (30/31 pass, 1 skipped) @@ -584,7 +585,7 @@ This ensures `scope :own` in PermissionSets is actually used (not redundant). - ✅ 31 tests total: 30 passing, 1 skipped (AshAuthentication edge case) - ✅ Tests for all 4 permission sets: own_data, read_only, normal_user, admin - ✅ Tests for AshAuthentication bypass (registration/login) -- ✅ Tests for NoActor bypass (test environment) +- ✅ Tests use system_actor for authorization (NoActor bypass removed) - ✅ Tests verify scope :own is used for UPDATE (not redundant) --- diff --git a/docs/user-resource-policies-implementation-summary.md b/docs/user-resource-policies-implementation-summary.md index c85d3d7..c939c6b 100644 --- a/docs/user-resource-policies-implementation-summary.md +++ b/docs/user-resource-policies-implementation-summary.md @@ -22,18 +22,13 @@ policies do authorize_if always() end - # 2. NoActor Bypass (test environment only) - bypass action_type([:create, :read, :update, :destroy]) do - authorize_if Mv.Authorization.Checks.NoActor - end - - # 3. Bypass for READ (list queries via auto_filter) + # 2. Bypass for READ (list queries via auto_filter) bypass action_type(:read) do description "Users can always read their own account" authorize_if expr(id == ^actor(:id)) end - # 4. HasPermission for all operations (uses scope from PermissionSets) + # 3. HasPermission for all operations (uses scope from PermissionSets) policy action_type([:read, :create, :update, :destroy]) do description "Check permissions from user's role and permission set" authorize_if Mv.Authorization.Checks.HasPermission @@ -51,7 +46,7 @@ end - ✅ CREATE operations (admin only) - ✅ DESTROY operations (admin only) - ✅ AshAuthentication bypass (registration/login) -- ✅ NoActor bypass (test environment) +- ✅ Tests use system_actor for authorization --- @@ -190,7 +185,7 @@ mix test test/mv/accounts/user_policies_test.exs \ **Test Environment:** - ✅ Operations without actor work in test environment -- ✅ NoActor bypass correctly detects compile-time environment +- ✅ All tests explicitly use system_actor for authorization ---