From cbc85f8bb8653d3946efce103250516d011829f4 Mon Sep 17 00:00:00 2001 From: Moritz Date: Thu, 22 Jan 2026 19:19:27 +0100 Subject: [PATCH] docs(auth): document User policies and bypass pattern Add bypass vs HasPermission pattern documentation Update architecture and implementation plan docs --- docs/policy-bypass-vs-haspermission.md | 319 ++++++++++++++++++ docs/roles-and-permissions-architecture.md | 148 ++++++-- ...les-and-permissions-implementation-plan.md | 81 +++-- ...esource-policies-implementation-summary.md | 274 +++++++++++++++ 4 files changed, 757 insertions(+), 65 deletions(-) create mode 100644 docs/policy-bypass-vs-haspermission.md create mode 100644 docs/user-resource-policies-implementation-summary.md diff --git a/docs/policy-bypass-vs-haspermission.md b/docs/policy-bypass-vs-haspermission.md new file mode 100644 index 0000000..cc66150 --- /dev/null +++ b/docs/policy-bypass-vs-haspermission.md @@ -0,0 +1,319 @@ +# Policy Pattern: Bypass vs. HasPermission + +**Date:** 2026-01-22 +**Status:** Implemented and Tested +**Applies to:** User Resource, Member Resource + +--- + +## Summary + +For filter-based permissions (`scope :own`, `scope :linked`), we use a **two-tier authorization pattern**: + +1. **Bypass with `expr()` for READ operations** - Handles list queries via auto_filter +2. **HasPermission for UPDATE/CREATE/DESTROY** - Uses scope from PermissionSets when record is present + +This pattern ensures that the scope concept in PermissionSets is actually used and not redundant. + +--- + +## The Problem + +### Initial Assumption (INCORRECT) + +> "No separate Own Credentials Bypass needed, as all permission sets already have User read/update :own. HasPermission with scope :own handles this correctly." + +This assumption was based on the idea that `HasPermission` returning `{:filter, expr(...)}` would automatically trigger Ash's `auto_filter` for list queries. + +### Reality + +**When HasPermission returns `{:filter, expr(...)}`:** + +1. `strict_check` is called first +2. For list queries (no record yet), `strict_check` returns `{:ok, false}` +3. Ash **STOPS** evaluation and does **NOT** call `auto_filter` +4. Result: List queries fail with empty results ❌ + +**Example:** +```elixir +# This FAILS for list queries: +policy action_type([:read, :update]) do + authorize_if Mv.Authorization.Checks.HasPermission +end + +# User tries to list all users: +Ash.read(User, actor: user) +# Expected: Returns [user] (filtered to own record) +# Actual: Returns [] (empty list) +``` + +--- + +## The Solution + +### Pattern: Bypass for READ, HasPermission for UPDATE + +**User Resource Example:** + +```elixir +policies do + # Bypass for READ (handles 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 + + # HasPermission for UPDATE (scope :own works with changesets) + policy action_type([:read, :create, :update, :destroy]) do + description "Check permissions from user's role and permission set" + authorize_if Mv.Authorization.Checks.HasPermission + end +end +``` + +**Why This Works:** + +| Operation | Record Available? | Method | Result | +|-----------|-------------------|--------|--------| +| **READ (list)** | ❌ No | `bypass` with `expr()` | Ash applies expr as SQL WHERE → ✅ Filtered list | +| **READ (single)** | ✅ Yes | `bypass` with `expr()` | Ash evaluates expr → ✅ true/false | +| **UPDATE** | ✅ Yes (changeset) | `HasPermission` with `scope :own` | strict_check evaluates record → ✅ Authorized | +| **CREATE** | ✅ Yes (changeset) | `HasPermission` with `scope :own` | strict_check evaluates record → ✅ Authorized | +| **DESTROY** | ✅ Yes | `HasPermission` with `scope :own` | strict_check evaluates record → ✅ Authorized | + +--- + +## Why `scope :own` Is NOT Redundant + +### The Question + +> "If we use a bypass with `expr(id == ^actor(:id))` for READ, isn't `scope :own` in PermissionSets redundant?" + +### The Answer: NO! ✅ + +**`scope :own` is ONLY used for operations where a record is present:** + +```elixir +# PermissionSets.ex +%{resource: "User", action: :read, scope: :own, granted: true}, # Not used (bypass handles it) +%{resource: "User", action: :update, scope: :own, granted: true}, # USED by HasPermission ✅ +``` + +**Test Proof:** + +```elixir +# test/mv/accounts/user_policies_test.exs:82 +test "can update own email", %{user: user} do + new_email = "updated@example.com" + + # This works via HasPermission with scope :own (NOT via bypass) + {:ok, updated_user} = + user + |> Ash.Changeset.for_update(:update_user, %{email: new_email}) + |> Ash.update(actor: user) + + assert updated_user.email == Ash.CiString.new(new_email) +end +# ✅ Test passes - proves scope :own is used! +``` + +--- + +## Consistency Across Resources + +### User Resource + +```elixir +# Bypass for READ list queries +bypass action_type(:read) do + authorize_if expr(id == ^actor(:id)) +end + +# HasPermission for UPDATE (uses scope :own from PermissionSets) +policy action_type([:read, :create, :update, :destroy]) do + authorize_if Mv.Authorization.Checks.HasPermission +end +``` + +**PermissionSets:** +- `own_data`, `read_only`, `normal_user`: `scope :own` for read/update +- `admin`: `scope :all` for all operations + +### Member Resource + +```elixir +# Bypass for READ list queries +bypass action_type(:read) do + authorize_if expr(id == ^actor(:member_id)) +end + +# HasPermission for UPDATE (uses scope :linked from PermissionSets) +policy action_type([:read, :create, :update, :destroy]) do + authorize_if Mv.Authorization.Checks.HasPermission +end +``` + +**PermissionSets:** +- `own_data`: `scope :linked` for read/update +- `read_only`: `scope :all` for read (no update permission) +- `normal_user`, `admin`: `scope :all` for all operations + +--- + +## Technical Deep Dive + +### Why Does `expr()` in Bypass Work? + +**Ash treats `expr()` natively in two contexts:** + +1. **strict_check** (single record): + - Ash evaluates the expression against the record + - Returns true/false based on match + +2. **auto_filter** (list queries): + - Ash compiles the expression to SQL WHERE clause + - Applies filter directly in database query + +**Example:** +```elixir +bypass action_type(:read) do + authorize_if expr(id == ^actor(:id)) +end + +# For list query: Ash.read(User, actor: user) +# Compiled SQL: SELECT * FROM users WHERE id = $1 (user.id) +# Result: [user] ✅ +``` + +### Why Doesn't HasPermission Trigger auto_filter? + +**HasPermission.strict_check logic:** + +```elixir +def strict_check(actor, authorizer, _opts) do + # ... + case check_permission(...) do + {:filter, filter_expr} -> + if record do + # Evaluate filter against record + evaluate_filter_for_strict_check(filter_expr, actor, record, resource_name) + else + # No record (list query) - return false + # Ash STOPS here, does NOT call auto_filter + {:ok, false} + end + end +end +``` + +**Why return false instead of :unknown?** + +We tested returning `:unknown`, but Ash's policy evaluation still didn't reliably call `auto_filter`. The `bypass` with `expr()` is the only consistent solution. + +--- + +## Design Principles + +### 1. Consistency + +Both User and Member follow the same pattern: +- Bypass for READ (list queries) +- HasPermission for UPDATE/CREATE/DESTROY (with scope) + +### 2. Scope Concept Is Essential + +PermissionSets define scopes for all operations: +- `:own` - User can access their own records +- `:linked` - User can access linked records (e.g., their member) +- `:all` - User can access all records (admin) + +**These scopes are NOT redundant** - they are used for UPDATE/CREATE/DESTROY. + +### 3. Bypass Is a Technical Workaround + +The bypass is not a design choice but a **technical necessity** due to Ash's policy evaluation behavior: +- Ash doesn't call `auto_filter` when `strict_check` returns `false` +- `expr()` in bypass is handled natively by Ash for both contexts +- This is consistent with Ash's documentation and best practices + +--- + +## Test Coverage + +### User Resource Tests + +**File:** `test/mv/accounts/user_policies_test.exs` + +**Coverage:** +- ✅ 31 tests: 30 passing, 1 skipped +- ✅ All 4 permission sets: `own_data`, `read_only`, `normal_user`, `admin` +- ✅ READ operations (list and single) via bypass +- ✅ UPDATE operations via HasPermission with `scope :own` +- ✅ Admin operations via HasPermission with `scope :all` +- ✅ AshAuthentication bypass (registration/login) +- ✅ NoActor bypass (test environment) + +**Key Tests Proving Pattern:** + +```elixir +# Test 1: READ list uses bypass (returns filtered list) +test "list users returns only own user", %{user: user} do + {:ok, users} = Ash.read(Accounts.User, actor: user, domain: Mv.Accounts) + assert length(users) == 1 # Filtered to own user ✅ + assert hd(users).id == user.id +end + +# Test 2: UPDATE uses HasPermission with scope :own +test "can update own email", %{user: user} do + {:ok, updated_user} = + user + |> Ash.Changeset.for_update(:update_user, %{email: "new@example.com"}) + |> Ash.update(actor: user) + + assert updated_user.email # Uses scope :own from PermissionSets ✅ +end + +# Test 3: Admin uses HasPermission with scope :all +test "admin can update other users", %{admin: admin, other_user: other_user} do + {:ok, updated_user} = + other_user + |> Ash.Changeset.for_update(:update_user, %{email: "admin-changed@example.com"}) + |> Ash.update(actor: admin) + + assert updated_user.email # Uses scope :all from PermissionSets ✅ +end +``` + +--- + +## Lessons Learned + +1. **Don't assume** that returning a filter from `strict_check` will trigger `auto_filter` - test it! +2. **Bypass with `expr()` is necessary** for list queries with filter-based permissions +3. **Scope concept is NOT redundant** - it's used for operations with records (UPDATE/CREATE/DESTROY) +4. **Consistency matters** - following the same pattern across resources improves maintainability +5. **Documentation is key** - explaining WHY the pattern exists prevents future confusion + +--- + +## Future Considerations + +### If Ash Changes Policy Evaluation + +If a future version of Ash reliably calls `auto_filter` when `strict_check` returns `:unknown` or `{:filter, expr}`: + +1. We could **remove** the bypass for READ +2. Keep only the HasPermission policy for all operations +3. Update tests to verify the new behavior + +**However, for now (Ash 3.13.1), the bypass pattern is necessary and correct.** + +--- + +## References + +- **Ash Policy Documentation**: [https://hexdocs.pm/ash/policies.html](https://hexdocs.pm/ash/policies.html) +- **Implementation**: `lib/accounts/user.ex` (lines 271-315) +- **Tests**: `test/mv/accounts/user_policies_test.exs` +- **Architecture Doc**: `docs/roles-and-permissions-architecture.md` +- **Permission Sets**: `lib/mv/authorization/permission_sets.ex` diff --git a/docs/roles-and-permissions-architecture.md b/docs/roles-and-permissions-architecture.md index 6c21907..d3db975 100644 --- a/docs/roles-and-permissions-architecture.md +++ b/docs/roles-and-permissions-architecture.md @@ -871,79 +871,166 @@ end **Policy Order Matters!** Ash evaluates policies top-to-bottom, first match wins. +--- + +## Bypass vs. HasPermission: When to Use Which? + +**Key Finding:** For filter-based permissions (`scope :own`, `scope :linked`), we use a **two-tier approach**: + +1. **Bypass with `expr()` for READ** - Handles list queries (auto_filter) +2. **HasPermission for UPDATE/CREATE/DESTROY** - Handles operations with records + +### Why This Pattern? + +**The Problem with HasPermission for List Queries:** + +When `HasPermission` returns `{:filter, expr(...)}` for `scope :own` or `scope :linked`: +- `strict_check` returns `{:ok, false}` for queries without a record +- Ash does **NOT** reliably call `auto_filter` when `strict_check` returns `false` +- Result: List queries fail ❌ + +**The Solution:** + +Use `bypass` with `expr()` directly for READ operations: +- Ash handles `expr()` natively for both `strict_check` and `auto_filter` +- List queries work correctly ✅ +- Single-record reads work correctly ✅ + +### Pattern Summary + +| Operation | Has Record? | Use | Why | +|-----------|-------------|-----|-----| +| **READ (list)** | ❌ No | `bypass` with `expr()` | Triggers auto_filter | +| **READ (single)** | ✅ Yes | `bypass` with `expr()` | expr() evaluates to true/false | +| **UPDATE** | ✅ Yes (changeset) | `HasPermission` | strict_check can evaluate record | +| **CREATE** | ✅ Yes (changeset) | `HasPermission` | strict_check can evaluate record | +| **DESTROY** | ✅ Yes | `HasPermission` | strict_check can evaluate record | + +### Is scope :own/:linked Still Useful? + +**YES! ✅** The scope concept is essential: + +1. **Documentation** - Clearly expresses intent in PermissionSets +2. **UPDATE/CREATE/DESTROY** - Works perfectly via HasPermission when record is present +3. **Consistency** - All permissions are centralized in PermissionSets +4. **Maintainability** - Easy to see what each role can do + +The bypass is a **technical workaround** for Ash's auto_filter limitation, not a replacement for the scope concept. + +### Consistency Across Resources + +Both `User` and `Member` follow this pattern: + +- **User**: Bypass for READ (`id == ^actor(:id)`), HasPermission for UPDATE (`scope :own`) +- **Member**: Bypass for READ (`id == ^actor(:member_id)`), HasPermission for UPDATE (`scope :linked`) + +This ensures consistent behavior and predictable authorization logic throughout the application. + +--- + ### User Resource Policies **Location:** `lib/mv/accounts/user.ex` -**Special Case:** Users can ALWAYS read/update their own credentials, regardless of role. +**Pattern:** Bypass for READ (list queries), HasPermission for UPDATE (with scope :own). + +**Key Insight:** Bypass with `expr()` is needed ONLY for READ list queries because HasPermission's strict_check cannot properly trigger auto_filter. UPDATE operations work correctly via HasPermission because a changeset with record is available. ```elixir defmodule Mv.Accounts.User do use Ash.Resource, ... policies do - # SPECIAL CASE: Users can always access their own account - # This takes precedence over permission checks - policy action_type([:read, :update]) do - description "Users can always read and update their own account" + # 1. AshAuthentication Bypass (registration/login without actor) + bypass AshAuthentication.Checks.AshAuthenticationInteraction 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 + # 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 + description "Users can always read their own account" authorize_if expr(id == ^actor(:id)) end - # GENERAL: Other operations require permission - # (e.g., admin reading/updating other users, admin destroying users) + # 4. 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) + # - :admin → can read/create/update/destroy all users (scope :all) policy action_type([:read, :create, :update, :destroy]) do - description "Check permissions from user's role" + description "Check permissions from user's role and permission set" authorize_if Mv.Authorization.Checks.HasPermission end - # DEFAULT: Forbid if no policy matched - policy action_type([:read, :create, :update, :destroy]) do - forbid_if always() - end + # 5. DEFAULT: Ash implicitly forbids if no policy authorizes (fail-closed) end # ... end ``` +**Why Bypass for READ but not UPDATE?** + +- **READ list queries** (`Ash.read(User, actor: user)`): No record at strict_check time → HasPermission returns `{:ok, false}` → auto_filter not called → bypass with `expr()` needed ✅ +- **UPDATE operations** (`Ash.update(changeset, actor: user)`): Changeset contains record → HasPermission can evaluate `scope :own` correctly → works via HasPermission ✅ + **Permission Matrix:** | Action | Mitglied | Vorstand | Kassenwart | Buchhaltung | Admin | |--------|----------|----------|------------|-------------|-------| -| Read own | ✅ (special) | ✅ (special) | ✅ (special) | ✅ (special) | ✅ | -| Update own | ✅ (special) | ✅ (special) | ✅ (special) | ✅ (special) | ✅ | -| Read others | ❌ | ❌ | ❌ | ❌ | ✅ | -| Update others | ❌ | ❌ | ❌ | ❌ | ✅ | -| Create | ❌ | ❌ | ❌ | ❌ | ✅ | -| Destroy | ❌ | ❌ | ❌ | ❌ | ✅ | +| Read own | ✅ (bypass) | ✅ (bypass) | ✅ (bypass) | ✅ (bypass) | ✅ (scope :all) | +| Update own | ✅ (scope :own) | ✅ (scope :own) | ✅ (scope :own) | ✅ (scope :own) | ✅ (scope :all) | +| Read others | ❌ | ❌ | ❌ | ❌ | ✅ (scope :all) | +| Update others | ❌ | ❌ | ❌ | ❌ | ✅ (scope :all) | +| Create | ❌ | ❌ | ❌ | ❌ | ✅ (scope :all) | +| Destroy | ❌ | ❌ | ❌ | ❌ | ✅ (scope :all) | + +**Note:** This pattern is consistent with Member resource policies (bypass for READ, HasPermission for UPDATE). ### Member Resource Policies **Location:** `lib/mv/membership/member.ex` -**Special Case:** Users can always READ their linked member (where `id == user.member_id`). +**Pattern:** Bypass for READ (list queries), HasPermission for UPDATE (with scope :linked). + +**Key Insight:** Same pattern as User - bypass with `expr()` is needed ONLY for READ list queries. UPDATE operations work correctly via HasPermission because a changeset with record is available. ```elixir defmodule Mv.Membership.Member do use Ash.Resource, ... policies do - # SPECIAL CASE: Users can always access their linked member - policy action_type([:read, :update]) do - description "Users can access member linked to their account" - authorize_if expr(user_id == ^actor(:id)) + # 1. NoActor Bypass (test environment only, for test fixtures) + bypass action_type([:create, :read, :update, :destroy]) do + authorize_if Mv.Authorization.Checks.NoActor end - # GENERAL: Check permissions from role + # 2. 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 + description "Users can always read member linked to their account" + authorize_if expr(id == ^actor(:member_id)) + end + + # 3. 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) + # - :admin → can CRUD all members (scope :all) policy action_type([:read, :create, :update, :destroy]) do description "Check permissions from user's role" authorize_if Mv.Authorization.Checks.HasPermission end - # DEFAULT: Forbid - policy action_type([:read, :create, :update, :destroy]) do - forbid_if always() - end + # 4. DEFAULT: Ash implicitly forbids if no policy authorizes (fail-closed) end # Custom validation for email editing (see Special Cases section) @@ -957,6 +1044,11 @@ defmodule Mv.Membership.Member do end ``` +**Why Bypass for READ but not UPDATE?** + +- **READ list queries**: No record at strict_check time → bypass with `expr(id == ^actor(:member_id))` needed for auto_filter ✅ +- **UPDATE operations**: Changeset contains record → HasPermission evaluates `scope :linked` correctly ✅ + **Permission Matrix:** | Action | Mitglied | Vorstand | Kassenwart | Buchhaltung | Admin | diff --git a/docs/roles-and-permissions-implementation-plan.md b/docs/roles-and-permissions-implementation-plan.md index eab8414..2f8ad4b 100644 --- a/docs/roles-and-permissions-implementation-plan.md +++ b/docs/roles-and-permissions-implementation-plan.md @@ -524,61 +524,68 @@ Add authorization policies to the Member resource using the new `HasPermission` **Size:** M (2 days) **Dependencies:** #6 (HasPermission check) **Can work in parallel:** Yes (parallel with #7, #9, #10) -**Assignable to:** Backend Developer +**Assignable to:** Backend Developer +**Status:** ✅ **COMPLETED** **Description:** -Add authorization policies to the User resource. Special case: Users can always read/update their own credentials. +Add authorization policies to the User resource. Users can always read their own credentials (via bypass), and update their own credentials (via HasPermission with scope :own). + +**Implementation Pattern:** + +Following the same pattern as Member resource: +- **Bypass for READ** - Handles list queries (auto_filter) +- **HasPermission for UPDATE** - Handles updates with scope :own **Tasks:** -1. Open `lib/mv/accounts/user.ex` -2. Add `policies` block -3. Add special policy: Allow user to always access their own account (before general policy) +1. ✅ Open `lib/mv/accounts/user.ex` +2. ✅ Add `policies` block +3. ✅ Add AshAuthentication bypass (registration/login without actor) +4. ✅ Add NoActor bypass (test environment only) +5. ✅ Add bypass for READ: Allow user to always read their own account ```elixir - policy action_type([:read, :update]) do + bypass action_type(:read) do + description "Users can always read their own account" authorize_if expr(id == ^actor(:id)) end ``` -4. Add general policy: Check HasPermission for all actions -5. Ensure :destroy is admin-only (via HasPermission) -6. Preload :role relationship for actor +6. ✅ Add general policy: Check HasPermission for all actions (including UPDATE with scope :own) +7. ✅ Ensure :destroy is admin-only (via HasPermission) +8. ✅ Preload :role relationship for actor in tests **Policy Order:** -1. Allow user to read/update own account (id == actor.id) -2. Check HasPermission (for admin operations) -3. Default: Forbid +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) + +**Why Bypass for READ but not UPDATE?** + +- **READ list queries**: No record at strict_check time → bypass with `expr()` needed for auto_filter ✅ +- **UPDATE operations**: Changeset contains record → HasPermission evaluates `scope :own` correctly ✅ + +This ensures `scope :own` in PermissionSets is actually used (not redundant). **Acceptance Criteria:** -- [ ] User can always read/update own credentials -- [ ] Only admin can read/update other users -- [ ] Only admin can destroy users -- [ ] Policy order is correct -- [ ] Actor preloads :role relationship +- ✅ User can always read own credentials (via bypass) +- ✅ 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) +- ✅ Actor preloads :role relationship +- ✅ All tests pass (30/31 pass, 1 skipped) -**Test Strategy (TDD):** - -**Own Data Tests (All Roles):** -- User with :own_data can read own user record -- User with :own_data can update own email/password -- User with :own_data cannot read other users -- User with :read_only can read own data -- User with :normal_user can read own data -- Verify special policy takes precedence - -**Admin Tests:** -- Admin can read all users -- Admin can update any user's credentials -- Admin can destroy users -- Admin has unrestricted access - -**Forbidden Tests:** -- Non-admin cannot read other users -- Non-admin cannot update other users -- Non-admin cannot destroy users +**Test Results:** **Test File:** `test/mv/accounts/user_policies_test.exs` +- ✅ 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 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 new file mode 100644 index 0000000..c85d3d7 --- /dev/null +++ b/docs/user-resource-policies-implementation-summary.md @@ -0,0 +1,274 @@ +# User Resource Authorization Policies - Implementation Summary + +**Date:** 2026-01-22 +**Status:** ✅ COMPLETED + +--- + +## Overview + +Successfully implemented authorization policies for the User resource following the Bypass + HasPermission pattern, ensuring consistency with Member resource policies and proper use of the scope concept from PermissionSets. + +--- + +## What Was Implemented + +### 1. Policy Structure in `lib/accounts/user.ex` + +```elixir +policies do + # 1. AshAuthentication Bypass + bypass AshAuthentication.Checks.AshAuthenticationInteraction 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) + 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) + policy action_type([:read, :create, :update, :destroy]) do + description "Check permissions from user's role and permission set" + authorize_if Mv.Authorization.Checks.HasPermission + end +end +``` + +### 2. Test Suite in `test/mv/accounts/user_policies_test.exs` + +**Coverage:** +- ✅ 31 tests total: 30 passing, 1 skipped +- ✅ All 4 permission sets tested: `own_data`, `read_only`, `normal_user`, `admin` +- ✅ READ operations (list and single record) +- ✅ UPDATE operations (own and other users) +- ✅ CREATE operations (admin only) +- ✅ DESTROY operations (admin only) +- ✅ AshAuthentication bypass (registration/login) +- ✅ NoActor bypass (test environment) + +--- + +## Key Design Decisions + +### Decision 1: Bypass for READ, HasPermission for UPDATE + +**Rationale:** +- READ list queries have no record at `strict_check` time +- `HasPermission` returns `{:ok, false}` for queries without record +- Ash doesn't call `auto_filter` when `strict_check` returns `false` +- `expr()` in bypass is handled natively by Ash for `auto_filter` + +**Result:** +- Bypass handles READ list queries ✅ +- HasPermission handles UPDATE with `scope :own` ✅ +- No redundancy - both are necessary ✅ + +### Decision 2: No Explicit `forbid_if always()` + +**Rationale:** +- Ash implicitly forbids if no policy authorizes (fail-closed by default) +- Explicit `forbid_if always()` at the end breaks tests +- It would forbid valid operations that should be authorized by previous policies + +**Result:** +- Policies rely on Ash's implicit forbid ✅ +- Tests pass with this approach ✅ + +### Decision 3: Consistency with Member Resource + +**Rationale:** +- Member resource uses same pattern: Bypass for READ, HasPermission for UPDATE +- Consistent patterns improve maintainability and predictability +- Developers can understand authorization logic across resources + +**Result:** +- User and Member follow identical pattern ✅ +- Authorization logic is consistent throughout the app ✅ + +--- + +## The Scope Concept Is NOT Redundant + +### Initial Concern + +> "If we use a bypass with `expr(id == ^actor(:id))` for READ, isn't `scope :own` in PermissionSets redundant?" + +### Resolution + +**NO! The scope concept is essential:** + +1. **Documentation** - `scope :own` clearly expresses intent in PermissionSets +2. **UPDATE operations** - `scope :own` is USED by HasPermission when changeset contains record +3. **Admin operations** - `scope :all` allows admins full access +4. **Maintainability** - All permissions centralized in one place + +**Test Proof:** + +```elixir +test "can update own email", %{user: user} do + # This works via HasPermission with scope :own (NOT bypass) + {:ok, updated_user} = + user + |> Ash.Changeset.for_update(:update_user, %{email: "new@example.com"}) + |> Ash.update(actor: user) + + assert updated_user.email # ✅ Proves scope :own is used +end +``` + +--- + +## Documentation Updates + +### 1. Created `docs/policy-bypass-vs-haspermission.md` + +Comprehensive documentation explaining: +- Why bypass is needed for READ +- Why HasPermission works for UPDATE +- Technical deep dive into Ash policy evaluation +- Test coverage proving the pattern +- Lessons learned + +### 2. Updated `docs/roles-and-permissions-architecture.md` + +- Added "Bypass vs. HasPermission: When to Use Which?" section +- Updated User Resource Policies section with correct implementation +- Updated Member Resource Policies section for consistency +- Added pattern comparison table + +### 3. Updated `docs/roles-and-permissions-implementation-plan.md` + +- Marked Issue #8 as COMPLETED ✅ +- Added implementation details +- Documented why bypass is needed +- Added test results + +--- + +## Test Results + +### All Relevant Tests Pass + +```bash +mix test test/mv/accounts/user_policies_test.exs \ + test/mv/authorization/checks/has_permission_test.exs \ + test/mv/membership/member_policies_test.exs + +# Results: +# 75 tests: 74 passing, 1 skipped +# ✅ User policies: 30/31 (1 skipped) +# ✅ HasPermission check: 21/21 +# ✅ Member policies: 23/23 +``` + +### Specific Test Coverage + +**Own Data Access (All Roles):** +- ✅ Can read own user record (via bypass) +- ✅ Can update own email (via HasPermission with scope :own) +- ✅ Cannot read other users (filtered by bypass) +- ✅ Cannot update other users (forbidden by HasPermission) +- ✅ List returns only own user (auto_filter via bypass) + +**Admin Access:** +- ✅ Can read all users (HasPermission with scope :all) +- ✅ Can update other users (HasPermission with scope :all) +- ✅ Can create users (HasPermission with scope :all) +- ✅ Can destroy users (HasPermission with scope :all) + +**AshAuthentication:** +- ✅ Registration works without actor +- ✅ OIDC registration works +- ✅ OIDC sign-in works + +**Test Environment:** +- ✅ Operations without actor work in test environment +- ✅ NoActor bypass correctly detects compile-time environment + +--- + +## Files Changed + +### Implementation +1. ✅ `lib/accounts/user.ex` - Added policies block (lines 271-315) +2. ✅ `lib/mv/authorization/checks/has_permission.ex` - Added User resource support in `evaluate_filter_for_strict_check` + +### Tests +3. ✅ `test/mv/accounts/user_policies_test.exs` - Created comprehensive test suite (435 lines) +4. ✅ `test/mv/authorization/checks/has_permission_test.exs` - Updated to expect `false` instead of `:unknown` + +### Documentation +5. ✅ `docs/policy-bypass-vs-haspermission.md` - New comprehensive guide (created) +6. ✅ `docs/roles-and-permissions-architecture.md` - Updated User and Member sections +7. ✅ `docs/roles-and-permissions-implementation-plan.md` - Marked Issue #8 as completed +8. ✅ `docs/user-resource-policies-implementation-summary.md` - This file (created) + +--- + +## Lessons Learned + +### 1. Test Before Assuming + +The initial plan assumed HasPermission with `scope :own` would be sufficient. Testing revealed that Ash's policy evaluation doesn't reliably call `auto_filter` when `strict_check` returns `false` or `:unknown`. + +### 2. Bypass Is Not a Workaround, It's a Pattern + +The bypass with `expr()` is not a hack or workaround - it's the **correct pattern** for filter-based authorization in Ash when dealing with list queries. + +### 3. Scope Concept Remains Essential + +Even with bypass for READ, the scope concept in PermissionSets is essential for: +- UPDATE/CREATE/DESTROY operations +- Documentation and maintainability +- Centralized permission management + +### 4. Consistency Across Resources + +Following the same pattern (Bypass for READ, HasPermission for UPDATE) across User and Member resources makes the codebase more maintainable and predictable. + +### 5. Documentation Is Key + +Thorough documentation explaining **WHY** the pattern exists prevents future confusion and ensures the pattern is applied correctly in future resources. + +--- + +## Future Considerations + +### If Adding New Resources with Filter-Based Permissions + +Follow the same pattern: +1. Bypass with `expr()` for READ (list queries) +2. HasPermission for UPDATE/CREATE/DESTROY (uses scope from PermissionSets) +3. Define appropriate scopes in PermissionSets (`:own`, `:linked`, `:all`) + +### If Ash Framework Changes + +If a future version of Ash reliably calls `auto_filter` when `strict_check` returns `:unknown`: +1. Consider removing bypass for READ +2. Keep only HasPermission policy +3. Update tests to verify new behavior +4. Update documentation + +**For now (Ash 3.13.1), the current pattern is correct and necessary.** + +--- + +## Conclusion + +✅ **User Resource Authorization Policies are fully implemented, tested, and documented.** + +The implementation: +- Follows best practices for Ash policies +- Is consistent with Member resource pattern +- Uses the scope concept from PermissionSets effectively +- Has comprehensive test coverage +- Is thoroughly documented for future developers + +**Status: PRODUCTION READY** 🎉