diff --git a/CODE_GUIDELINES.md b/CODE_GUIDELINES.md index fe2d816..5bee497 100644 --- a/CODE_GUIDELINES.md +++ b/CODE_GUIDELINES.md @@ -641,7 +641,92 @@ def card(assigns) do end ``` -### 3.3 Ash Framework +### 3.3 System Actor Pattern + +**When to Use System Actor:** + +Some operations must always run regardless of user permissions. These are **systemic operations** that are mandatory side effects: + +- **Email synchronization** (Member ↔ User) +- **Email uniqueness validation** (data integrity requirement) +- **Cycle generation** (if defined as mandatory side effect) +- **Background jobs** +- **Seeds** + +**Implementation:** + +Use `Mv.Helpers.SystemActor.get_system_actor/0` for all systemic operations: + +```elixir +# Good - Email sync uses system actor +def get_linked_member(user) do + system_actor = SystemActor.get_system_actor() + opts = Helpers.ash_actor_opts(system_actor) + + case Ash.get(Mv.Membership.Member, id, opts) do + {:ok, member} -> member + {:error, _} -> nil + end +end + +# Bad - Using user actor for systemic operation +def get_linked_member(user, actor) do + opts = Helpers.ash_actor_opts(actor) # May fail if user lacks permissions! + # ... +end +``` + +**System Actor Details:** + +- System actor is a user with admin role (email: "system@mila.local") +- Cached in Agent for performance +- Falls back to admin user from seeds if system user doesn't exist +- Should NEVER be used for user-initiated actions (only systemic operations) + +**User Mode vs System Mode:** + +- **User Mode**: User-initiated actions use the actual user actor, policies are enforced +- **System Mode**: Systemic operations use system actor, bypass user permissions + +**Authorization Bootstrap Patterns:** + +Three 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 + ```elixir + # Good: Systemic operation + system_actor = SystemActor.get_system_actor() + Ash.read(Member, actor: system_actor) + + # Bad: User-initiated action + # Never use system_actor for user-initiated actions! + ``` + +3. **authorize?: false** (bootstrap only) - Skips policies for circular dependencies + ```elixir + # Good: Bootstrap (seeds, SystemActor loading) + Accounts.create_user!(%{email: admin_email}, authorize?: false) + + # Bad: User-initiated action + Ash.destroy(member, authorize?: false) # Never do this! + ``` + +**Decision Guide:** +- Use **NoActor** for test fixtures (automatic via config) +- Use **system_actor** for email sync, cycle generation, validations +- Use **authorize?: false** only for bootstrap (seeds, circular dependencies) +- Always document why `authorize?: false` is necessary + +**See also:** `docs/roles-and-permissions-architecture.md` (Authorization Bootstrap Patterns section) + +### 3.4 Ash Framework **Resource Definition Best Practices:** @@ -1617,6 +1702,66 @@ case Ash.read(Mv.Membership.Member, actor: actor) do end ``` +### 5.1a NoActor Pattern - Test Environment Only + +**IMPORTANT:** The `Mv.Authorization.Checks.NoActor` check is **ONLY for test environment**. It must NEVER be used in production. + +**What NoActor Does:** + +- 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:** + +```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 +end +``` + +**Why This Pattern Exists:** + +- 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 + +**NEVER Use NoActor in Production:** + +```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() +Ash.create!(Member, attrs, actor: system_actor) +``` + +**Testing:** + +- 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` + ### 5.2 Password Security **Use bcrypt for Password Hashing:** diff --git a/config/test.exs b/config/test.exs index 45acaa4..b48c408 100644 --- a/config/test.exs +++ b/config/test.exs @@ -49,3 +49,7 @@ config :mv, :require_token_presence_for_authentication, false # Enable SQL Sandbox for async LiveView tests # This flag controls sync vs async behavior in CycleGenerator after_action hooks config :mv, :sql_sandbox, true + +# Allow operations without actor in test environment (NoActor check) +# SECURITY: This must ONLY be true in test.exs, never in prod/dev +config :mv, :allow_no_actor_bypass, true diff --git a/docs/policy-bypass-vs-haspermission.md b/docs/policy-bypass-vs-haspermission.md new file mode 100644 index 0000000..8a65c6f --- /dev/null +++ b/docs/policy-bypass-vs-haspermission.md @@ -0,0 +1,330 @@ +# 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 | + +**Important: UPDATE Strategy** + +UPDATE is **NOT** a hardcoded bypass. It is controlled by **PermissionSets**: + +- All permission sets (`:own_data`, `:read_only`, `:normal_user`, `:admin`) explicitly grant `User.update :own` +- `HasPermission` evaluates `scope :own` when a changeset with record is present +- If a permission set is changed to remove `User.update :own`, users with that set will lose the ability to update their credentials +- This is intentional - UPDATE is controlled by PermissionSets, not hardcoded + +**Example:** The `read_only` permission set grants `User.update :own` even though it's "read-only" for member data. This allows password changes while keeping member data read-only. + +--- + +## 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..bc1b75c 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` +**Location:** `lib/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 | @@ -1652,17 +1744,21 @@ end **Implementation:** -Policy in `User` resource places this check BEFORE the general `HasPermission` check: +Policy in `User` resource uses a two-tier approach: +- **READ**: Bypass with `expr()` for list queries (auto_filter) +- **UPDATE**: HasPermission with `scope :own` (evaluates PermissionSets) ```elixir policies do - # SPECIAL CASE: Takes precedence over role permissions - policy action_type([:read, :update]) do - description "Users can always read and update their own account" + # SPECIAL CASE: Users can always READ their own account + # Bypass needed for list queries (expr() triggers auto_filter in Ash) + bypass action_type(:read) do + description "Users can always read their own account" authorize_if expr(id == ^actor(:id)) end - # GENERAL: For other operations (e.g., admin reading other users) + # GENERAL: Check permissions from user's role + # UPDATE uses scope :own from PermissionSets (all sets grant User.update :own) policy action_type([:read, :create, :update, :destroy]) do authorize_if Mv.Authorization.Checks.HasPermission end @@ -1670,10 +1766,53 @@ end ``` **Why this works:** -- Ash evaluates policies top-to-bottom -- First matching policy wins -- Special case catches own-account access before checking permissions -- Even a user with `own_data` (no admin permissions) can update their credentials +- READ bypass handles list queries correctly (auto_filter) +- UPDATE is handled by HasPermission with `scope :own` from PermissionSets +- All permission sets (`:own_data`, `:read_only`, `:normal_user`, `:admin`) grant `User.update :own` +- Even a user with `read_only` (read-only for member data) can update their own credentials + +**Important:** UPDATE is NOT an unverrückbarer Spezialfall (hardcoded bypass). It is controlled by PermissionSets. If a permission set is changed to remove `User.update :own`, users with that set will lose the ability to update their credentials. See "User Credentials: Why read_only Can Still Update" below for details. + +### 1a. User Credentials: Why read_only Can Still Update + +**Question:** If `read_only` means "read-only", why can users with this permission set still update their own credentials? + +**Answer:** The `read_only` permission set refers to **member data**, NOT user credentials. All permission sets grant `User.update :own` to allow password changes and profile updates. + +**Implementation Details:** + +1. **UPDATE is controlled by PermissionSets**, not a hardcoded bypass +2. **All 4 permission sets** (`:own_data`, `:read_only`, `:normal_user`, `:admin`) explicitly grant: + ```elixir + %{resource: "User", action: :update, scope: :own, granted: true} + ``` +3. **HasPermission** evaluates `scope :own` for UPDATE operations (when a changeset with record is present) +4. **No special bypass** is needed for UPDATE - it works correctly via HasPermission + +**Why This Design?** + +- **Flexibility:** Permission sets can be modified to change UPDATE behavior +- **Consistency:** All permissions are centralized in PermissionSets +- **Clarity:** The name "read_only" refers to member data, not user credentials +- **Maintainability:** Easy to see what each role can do in PermissionSets module + +**Warning:** If a permission set is changed to remove `User.update :own`, users with that set will **lose the ability to update their credentials**. This is intentional - UPDATE is controlled by PermissionSets, not hardcoded. + +**Example:** +```elixir +# In PermissionSets.get_permissions(:read_only) +resources: [ + # User: Can read/update own credentials only + # IMPORTANT: "read_only" refers to member data, NOT user credentials. + # All permission sets grant User.update :own to allow password changes. + %{resource: "User", action: :read, scope: :own, granted: true}, + %{resource: "User", action: :update, scope: :own, granted: true}, + + # Member: Can read all members, no modifications + %{resource: "Member", action: :read, scope: :all, granted: true}, + # Note: No Member.update permission - this is the "read_only" part +] +``` ### 2. Linked Member Email Editing @@ -2484,8 +2623,241 @@ iex> MvWeb.Authorization.can_access_page?(user, "/members/new") --- +## Authorization Bootstrap Patterns + +This section clarifies three different mechanisms for bypassing standard authorization, their purposes, and when to use each. + +### Overview + +The codebase uses three 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 + +**All three are necessary and serve different purposes.** + +### 1. NoActor Check + +**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 + +**Purpose:** Admin user for systemic operations that must always succeed regardless of user permissions. + +**Implementation:** +```elixir +system_actor = Mv.Helpers.SystemActor.get_system_actor() +# => %User{email: "system@mila.local", role: %{permission_set_name: "admin"}} +``` + +**Security:** +- No password (hashed_password = nil) → cannot login +- No OIDC ID (oidc_id = nil) → cannot authenticate +- Cached in Agent for performance +- Created automatically in test environment if missing + +**Use Cases:** +- **Email synchronization** (User ↔ Member email sync) +- **Email uniqueness validation** (cross-resource checks) +- **Cycle generation** (mandatory side effect) +- **OIDC account linking** (user not yet logged in) +- **Cross-resource validations** (must work regardless of actor) + +**Example:** +```elixir +def get_linked_member(%{member_id: id}) do + system_actor = SystemActor.get_system_actor() + opts = Helpers.ash_actor_opts(system_actor) + + # Email sync must work regardless of user permissions + Ash.get(Mv.Membership.Member, id, opts) +end +``` + +**Why not `authorize?: false`?** +- System actor is explicit (clear intent: "systemic operation") +- Policies are evaluated (with admin rights) +- Audit trail (actor.email = "system@mila.local") +- Consistent authorization flow +- Testable + +### 3. authorize?: false + +**Purpose:** Skip policies for bootstrap scenarios with circular dependencies. + +**Use Cases:** + +**1. Seeds** - No admin exists yet to use as actor: +```elixir +# priv/repo/seeds.exs +Accounts.create_user!(%{email: admin_email}, + authorize?: false # Bootstrap: no admin exists yet +) +``` + +**2. SystemActor Bootstrap** - Chicken-and-egg problem: +```elixir +# lib/mv/helpers/system_actor.ex +defp find_user_by_email(email) do + # Need to find system actor, but loading requires system actor! + Mv.Accounts.User + |> Ash.Query.filter(email == ^email) + |> Ash.read_one(authorize?: false) # Bootstrap only +end +``` + +**3. Actor.ensure_loaded** - Circular dependency: +```elixir +# lib/mv/authorization/actor.ex +defp load_role(actor) do + # Actor needs role for authorization, + # but loading role requires authorization! + Ash.load(actor, :role, authorize?: false) # Bootstrap only +end +``` + +**4. assign_default_role** - User creation: +```elixir +# User doesn't have actor during creation +Mv.Authorization.Role +|> Ash.Query.filter(name == "Mitglied") +|> Ash.read_one(authorize?: false) # Bootstrap only +``` + +**Security:** +- Very powerful - skips ALL policies +- Use sparingly and document every usage +- Only for bootstrap scenarios +- All current usages are legitimate + +### 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 | + +### 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 +- ✅ Cycle generation +- ✅ Cross-resource validations +- ✅ OIDC flows (user not logged in) + +**Use authorize?: false when:** +- ✅ Bootstrap scenario (seeds) +- ✅ Circular dependency (SystemActor bootstrap, Actor.ensure_loaded) +- ⚠️ Document with comment explaining why + +**DON'T:** +- ❌ Use `authorize?: false` for user-initiated actions +- ❌ Use `authorize?: false` when `system_actor` would work +- ❌ Enable NoActor outside test environment + +### The Circular Dependency Problem + +**SystemActor Bootstrap:** +``` +SystemActor.get_system_actor() + ↓ calls find_user_by_email() + ↓ needs to query User + ↓ User policies require actor + ↓ but we're loading the actor! + +Solution: authorize?: false for bootstrap query +``` + +**Actor.ensure_loaded:** +``` +Authorization check (HasPermission) + ↓ needs actor.role.permission_set_name + ↓ but role is %Ash.NotLoaded{} + ↓ load role with Ash.load(actor, :role) + ↓ but loading requires authorization + ↓ which needs actor.role! + +Solution: authorize?: false for role load +``` + +**Why this is safe:** +- Actor is loading their OWN data (role relationship) +- Actor already passed authentication boundary +- Role contains no sensitive data (just permission_set reference) +- Alternative (denormalize permission_set_name) adds complexity + +### Examples + +**Good - system_actor for systemic operation:** +```elixir +defp check_if_email_used(email) do + system_actor = SystemActor.get_system_actor() + opts = Helpers.ash_actor_opts(system_actor) + + # Validation must work regardless of current actor + Ash.read(User, opts) +end +``` + +**Good - authorize?: false for bootstrap:** +```elixir +# Seeds - no admin exists yet +Accounts.create_user!(%{email: admin_email}, authorize?: false) +``` + +**Bad - authorize?: false for user action:** +```elixir +# WRONG: Bypasses all policies for user-initiated action +def delete_member(member) do + Ash.destroy(member, authorize?: false) # ❌ Don't do this! +end + +# CORRECT: Use actor +def delete_member(member, actor) do + Ash.destroy(member, actor: actor) # ✅ Policies enforced +end +``` + +--- + **Document Version:** 2.0 (Clean Rewrite) -**Last Updated:** 2026-01-13 +**Last Updated:** 2026-01-23 **Implementation Status:** ✅ Complete (2026-01-08) **Status:** Ready for Implementation @@ -2500,6 +2872,9 @@ iex> MvWeb.Authorization.can_access_page?(user, "/members/new") - Added comprehensive security section - Enhanced edge case documentation +**Changes from V2.0:** +- Added "Authorization Bootstrap Patterns" section explaining NoActor, system_actor, and authorize?: false + --- **End of Architecture Document** diff --git a/docs/roles-and-permissions-implementation-plan.md b/docs/roles-and-permissions-implementation-plan.md index eab8414..33b1702 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/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** 🎉 diff --git a/lib/accounts/user.ex b/lib/accounts/user.ex index 9598b76..08d1130 100644 --- a/lib/accounts/user.ex +++ b/lib/accounts/user.ex @@ -5,9 +5,8 @@ defmodule Mv.Accounts.User do use Ash.Resource, domain: Mv.Accounts, data_layer: AshPostgres.DataLayer, - extensions: [AshAuthentication] - - # authorizers: [Ash.Policy.Authorizer] + extensions: [AshAuthentication], + authorizers: [Ash.Policy.Authorizer] postgres do table "users" @@ -267,6 +266,36 @@ defmodule Mv.Accounts.User do end end + # Authorization Policies + # Order matters: Most specific policies first, then general permission check + policies do + # AshAuthentication bypass (registration/login without actor) + bypass AshAuthentication.Checks.AshAuthenticationInteraction do + description "Allow AshAuthentication internal operations (registration, login)" + authorize_if always() + end + + # NoActor bypass (test fixtures only, see no_actor.ex) + bypass action_type([:create, :read, :update, :destroy]) do + description "Allow system operations without actor (test environment only)" + authorize_if Mv.Authorization.Checks.NoActor + end + + # READ bypass for list queries (scope :own via expr) + bypass action_type(:read) do + description "Users can always read their own account" + authorize_if expr(id == ^actor(:id)) + end + + # UPDATE/DESTROY via HasPermission (evaluates PermissionSets scope) + 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 + + # Default: Ash implicitly forbids if no policy authorizes (fail-closed) + end + # Global validations - applied to all relevant actions validations do # Password strength policy: minimum 8 characters for all password-related actions diff --git a/lib/accounts/user/validations/oidc_email_collision.ex b/lib/accounts/user/validations/oidc_email_collision.ex index 041647a..08a8911 100644 --- a/lib/accounts/user/validations/oidc_email_collision.ex +++ b/lib/accounts/user/validations/oidc_email_collision.ex @@ -42,25 +42,29 @@ defmodule Mv.Accounts.User.Validations.OidcEmailCollision do if email && oidc_id && user_info do # Check if a user with this oidc_id already exists # If yes, this will be an upsert (email update), not a new registration + # Use SystemActor for authorization during OIDC registration (no logged-in actor) + system_actor = Mv.Helpers.SystemActor.get_system_actor() + existing_oidc_user = case Mv.Accounts.User |> Ash.Query.filter(oidc_id == ^to_string(oidc_id)) - |> Ash.read_one() do + |> Ash.read_one(actor: system_actor) do {:ok, user} -> user _ -> nil end - check_email_collision(email, oidc_id, user_info, existing_oidc_user) + check_email_collision(email, oidc_id, user_info, existing_oidc_user, system_actor) else :ok end end - defp check_email_collision(email, new_oidc_id, user_info, existing_oidc_user) do + defp check_email_collision(email, new_oidc_id, user_info, existing_oidc_user, system_actor) do # Find existing user with this email + # Use SystemActor for authorization during OIDC registration (no logged-in actor) case Mv.Accounts.User |> Ash.Query.filter(email == ^to_string(email)) - |> Ash.read_one() do + |> Ash.read_one(actor: system_actor) do {:ok, nil} -> # No user exists with this email - OK to create new user :ok diff --git a/lib/membership/member.ex b/lib/membership/member.ex index 5a4d01c..650cf43 100644 --- a/lib/membership/member.ex +++ b/lib/membership/member.ex @@ -124,8 +124,9 @@ defmodule Mv.Membership.Member do case result do {:ok, member} -> if member.membership_fee_type_id && member.join_date do - actor = Map.get(changeset.context, :actor) - handle_cycle_generation(member, actor: actor) + # Capture initiator for audit trail (if available) + initiator = Map.get(changeset.context, :actor) + handle_cycle_generation(member, initiator: initiator) end {:error, _} -> @@ -196,16 +197,12 @@ defmodule Mv.Membership.Member do Ash.Changeset.changing_attribute?(changeset, :membership_fee_type_id) if fee_type_changed && member.membership_fee_type_id && member.join_date do - actor = Map.get(changeset.context, :actor) - - case regenerate_cycles_on_type_change(member, actor: actor) do + case regenerate_cycles_on_type_change(member) do {:ok, notifications} -> # Return notifications to Ash - they will be sent automatically after commit {:ok, member, notifications} {:error, reason} -> - require Logger - Logger.warning( "Failed to regenerate cycles for member #{member.id}: #{inspect(reason)}" ) @@ -230,8 +227,9 @@ defmodule Mv.Membership.Member do exit_date_changed = Ash.Changeset.changing_attribute?(changeset, :exit_date) if (join_date_changed || exit_date_changed) && member.membership_fee_type_id do - actor = Map.get(changeset.context, :actor) - handle_cycle_generation(member, actor: actor) + # Capture initiator for audit trail (if available) + initiator = Map.get(changeset.context, :actor) + handle_cycle_generation(member, initiator: initiator) end {:error, _} -> @@ -409,8 +407,16 @@ defmodule Mv.Membership.Member do actor = Map.get(changeset.context || %{}, :actor) # Check the current state of the user in the database - # Pass actor to ensure proper authorization (User might have policies in future) - case Ash.get(Mv.Accounts.User, user_id, actor: actor) do + # 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 + + # Pass actor and authorize? to ensure proper authorization (User might have policies in future) + case Ash.get(Mv.Accounts.User, user_id, actor: actor, authorize?: authorize?) do # User is free to be linked {:ok, %{member_id: nil}} -> :ok @@ -790,37 +796,37 @@ defmodule Mv.Membership.Member do # Returns {:ok, notifications} or {:error, reason} where notifications are collected # to be sent after transaction commits @doc false - def regenerate_cycles_on_type_change(member, opts \\ []) do + # Uses system actor for cycle regeneration (mandatory side effect) + def regenerate_cycles_on_type_change(member, _opts \\ []) do + alias Mv.Helpers + alias Mv.Helpers.SystemActor + today = Date.utc_today() lock_key = :erlang.phash2(member.id) - actor = Keyword.get(opts, :actor) # Use advisory lock to prevent concurrent deletion and regeneration # This ensures atomicity when multiple updates happen simultaneously if Mv.Repo.in_transaction?() do - regenerate_cycles_in_transaction(member, today, lock_key, actor: actor) + regenerate_cycles_in_transaction(member, today, lock_key) else - regenerate_cycles_new_transaction(member, today, lock_key, actor: actor) + regenerate_cycles_new_transaction(member, today, lock_key) end end # Already in transaction: use advisory lock directly # Returns {:ok, notifications} - notifications should be returned to after_action hook - defp regenerate_cycles_in_transaction(member, today, lock_key, opts) do - actor = Keyword.get(opts, :actor) + defp regenerate_cycles_in_transaction(member, today, lock_key) do Ecto.Adapters.SQL.query!(Mv.Repo, "SELECT pg_advisory_xact_lock($1)", [lock_key]) - do_regenerate_cycles_on_type_change(member, today, skip_lock?: true, actor: actor) + do_regenerate_cycles_on_type_change(member, today, skip_lock?: true) end # Not in transaction: start new transaction with advisory lock # Returns {:ok, notifications} - notifications should be sent by caller (e.g., via after_action) - defp regenerate_cycles_new_transaction(member, today, lock_key, opts) do - actor = Keyword.get(opts, :actor) - + defp regenerate_cycles_new_transaction(member, today, lock_key) do Mv.Repo.transaction(fn -> Ecto.Adapters.SQL.query!(Mv.Repo, "SELECT pg_advisory_xact_lock($1)", [lock_key]) - case do_regenerate_cycles_on_type_change(member, today, skip_lock?: true, actor: actor) do + case do_regenerate_cycles_on_type_change(member, today, skip_lock?: true) do {:ok, notifications} -> # Return notifications - they will be sent by the caller notifications @@ -838,11 +844,16 @@ defmodule Mv.Membership.Member do # Performs the actual cycle deletion and regeneration # Returns {:ok, notifications} or {:error, reason} # notifications are collected to be sent after transaction commits + # Uses system actor for all operations defp do_regenerate_cycles_on_type_change(member, today, opts) do + alias Mv.Helpers + alias Mv.Helpers.SystemActor + require Ash.Query skip_lock? = Keyword.get(opts, :skip_lock?, false) - actor = Keyword.get(opts, :actor) + system_actor = SystemActor.get_system_actor() + actor_opts = Helpers.ash_actor_opts(system_actor) # Find all unpaid cycles for this member # We need to check cycle_end for each cycle using its own interval @@ -852,20 +863,16 @@ defmodule Mv.Membership.Member do |> Ash.Query.filter(status == :unpaid) |> Ash.Query.load([:membership_fee_type]) - result = - if actor do - Ash.read(all_unpaid_cycles_query, actor: actor) - else - Ash.read(all_unpaid_cycles_query) - end - - case result do + case Ash.read(all_unpaid_cycles_query, actor_opts) do {:ok, all_unpaid_cycles} -> cycles_to_delete = filter_future_cycles(all_unpaid_cycles, today) - delete_and_regenerate_cycles(cycles_to_delete, member.id, today, - skip_lock?: skip_lock?, - actor: actor + delete_and_regenerate_cycles( + cycles_to_delete, + member.id, + today, + actor_opts, + skip_lock?: skip_lock? ) {:error, reason} -> @@ -893,26 +900,27 @@ defmodule Mv.Membership.Member do # Deletes future cycles and regenerates them with the new type/amount # Passes today to ensure consistent date across deletion and regeneration # Returns {:ok, notifications} or {:error, reason} - defp delete_and_regenerate_cycles(cycles_to_delete, member_id, today, opts) do + # Uses system actor for cycle generation and deletion + defp delete_and_regenerate_cycles(cycles_to_delete, member_id, today, actor_opts, opts) do skip_lock? = Keyword.get(opts, :skip_lock?, false) - actor = Keyword.get(opts, :actor) if Enum.empty?(cycles_to_delete) do # No cycles to delete, just regenerate - regenerate_cycles(member_id, today, skip_lock?: skip_lock?, actor: actor) + regenerate_cycles(member_id, today, skip_lock?: skip_lock?) else - case delete_cycles(cycles_to_delete) do - :ok -> regenerate_cycles(member_id, today, skip_lock?: skip_lock?, actor: actor) + case delete_cycles(cycles_to_delete, actor_opts) do + :ok -> regenerate_cycles(member_id, today, skip_lock?: skip_lock?) {:error, reason} -> {:error, reason} end end end # Deletes cycles and returns :ok if all succeeded, {:error, reason} otherwise - defp delete_cycles(cycles_to_delete) do + # Uses system actor for authorization to ensure deletion always works + defp delete_cycles(cycles_to_delete, actor_opts) do delete_results = Enum.map(cycles_to_delete, fn cycle -> - Ash.destroy(cycle) + Ash.destroy(cycle, actor_opts) end) if Enum.any?(delete_results, &match?({:error, _}, &1)) do @@ -928,13 +936,11 @@ defmodule Mv.Membership.Member do # Returns {:ok, notifications} - notifications should be returned to after_action hook defp regenerate_cycles(member_id, today, opts) do skip_lock? = Keyword.get(opts, :skip_lock?, false) - actor = Keyword.get(opts, :actor) case Mv.MembershipFees.CycleGenerator.generate_cycles_for_member( member_id, today: today, - skip_lock?: skip_lock?, - actor: actor + skip_lock?: skip_lock? ) do {:ok, _cycles, notifications} when is_list(notifications) -> {:ok, notifications} @@ -948,49 +954,57 @@ defmodule Mv.Membership.Member do # based on environment (test vs production) # This function encapsulates the common logic for cycle generation # to avoid code duplication across different hooks + # Uses system actor for cycle generation (mandatory side effect) + # Captures initiator for audit trail (if available in opts) defp handle_cycle_generation(member, opts) do - actor = Keyword.get(opts, :actor) + initiator = Keyword.get(opts, :initiator) if Mv.Config.sql_sandbox?() do - handle_cycle_generation_sync(member, actor: actor) + handle_cycle_generation_sync(member, initiator) else - handle_cycle_generation_async(member, actor: actor) + handle_cycle_generation_async(member, initiator) end end # Runs cycle generation synchronously (for test environment) - defp handle_cycle_generation_sync(member, opts) do - require Logger - actor = Keyword.get(opts, :actor) - + defp handle_cycle_generation_sync(member, initiator) do case Mv.MembershipFees.CycleGenerator.generate_cycles_for_member( member.id, today: Date.utc_today(), - actor: actor + initiator: initiator ) do {:ok, cycles, notifications} -> send_notifications_if_any(notifications) - log_cycle_generation_success(member, cycles, notifications, sync: true) + + log_cycle_generation_success(member, cycles, notifications, + sync: true, + initiator: initiator + ) {:error, reason} -> - log_cycle_generation_error(member, reason, sync: true) + log_cycle_generation_error(member, reason, sync: true, initiator: initiator) end end # Runs cycle generation asynchronously (for production environment) - defp handle_cycle_generation_async(member, opts) do - actor = Keyword.get(opts, :actor) - + defp handle_cycle_generation_async(member, initiator) do Task.Supervisor.async_nolink(Mv.TaskSupervisor, fn -> - case Mv.MembershipFees.CycleGenerator.generate_cycles_for_member(member.id, actor: actor) do + case Mv.MembershipFees.CycleGenerator.generate_cycles_for_member(member.id, + initiator: initiator + ) do {:ok, cycles, notifications} -> send_notifications_if_any(notifications) - log_cycle_generation_success(member, cycles, notifications, sync: false) + + log_cycle_generation_success(member, cycles, notifications, + sync: false, + initiator: initiator + ) {:error, reason} -> - log_cycle_generation_error(member, reason, sync: false) + log_cycle_generation_error(member, reason, sync: false, initiator: initiator) end end) + |> Task.await(:infinity) end # Sends notifications if any are present @@ -1001,13 +1015,15 @@ defmodule Mv.Membership.Member do end # Logs successful cycle generation - defp log_cycle_generation_success(member, cycles, notifications, sync: sync?) do - require Logger - + defp log_cycle_generation_success(member, cycles, notifications, + sync: sync?, + initiator: initiator + ) do sync_label = if sync?, do: "", else: " (async)" + initiator_info = get_initiator_info(initiator) Logger.debug( - "Successfully generated cycles for member#{sync_label}", + "Successfully generated cycles for member#{sync_label} (initiator: #{initiator_info})", member_id: member.id, cycles_count: length(cycles), notifications_count: length(notifications) @@ -1015,13 +1031,12 @@ defmodule Mv.Membership.Member do end # Logs cycle generation errors - defp log_cycle_generation_error(member, reason, sync: sync?) do - require Logger - + defp log_cycle_generation_error(member, reason, sync: sync?, initiator: initiator) do sync_label = if sync?, do: "", else: " (async)" + initiator_info = get_initiator_info(initiator) Logger.error( - "Failed to generate cycles for member#{sync_label}", + "Failed to generate cycles for member#{sync_label} (initiator: #{initiator_info})", member_id: member.id, member_email: member.email, error: inspect(reason), @@ -1029,6 +1044,11 @@ defmodule Mv.Membership.Member do ) end + # Extracts initiator information for audit trail + defp get_initiator_info(nil), do: "system" + defp get_initiator_info(%{email: email}), do: email + defp get_initiator_info(_), do: "unknown" + # Helper to extract error type for structured logging defp error_type(%{__struct__: struct_name}), do: struct_name defp error_type(error) when is_atom(error), do: error diff --git a/lib/mv/accounts/user/validations/email_not_used_by_other_member.ex b/lib/mv/accounts/user/validations/email_not_used_by_other_member.ex index af68f96..0e693e1 100644 --- a/lib/mv/accounts/user/validations/email_not_used_by_other_member.ex +++ b/lib/mv/accounts/user/validations/email_not_used_by_other_member.ex @@ -9,6 +9,8 @@ defmodule Mv.Accounts.User.Validations.EmailNotUsedByOtherMember do """ use Ash.Resource.Validation + require Logger + @doc """ Validates email uniqueness across linked User-Member pairs. @@ -73,19 +75,29 @@ defmodule Mv.Accounts.User.Validations.EmailNotUsedByOtherMember do end defp check_email_uniqueness(email, exclude_member_id) do + alias Mv.Helpers + alias Mv.Helpers.SystemActor + query = Mv.Membership.Member |> Ash.Query.filter(email == ^to_string(email)) |> maybe_exclude_id(exclude_member_id) - case Ash.read(query) do + system_actor = SystemActor.get_system_actor() + opts = Helpers.ash_actor_opts(system_actor) + + case Ash.read(query, opts) do {:ok, []} -> :ok {:ok, _} -> {:error, field: :email, message: "is already used by another member", value: email} - {:error, _} -> + {:error, reason} -> + Logger.warning( + "Email uniqueness validation query failed for user email '#{email}': #{inspect(reason)}. Allowing operation to proceed (fail-open)." + ) + :ok end end diff --git a/lib/mv/application.ex b/lib/mv/application.ex index 09eefce..ea0c78e 100644 --- a/lib/mv/application.ex +++ b/lib/mv/application.ex @@ -14,6 +14,7 @@ defmodule Mv.Application do {DNSCluster, query: Application.get_env(:mv, :dns_cluster_query) || :ignore}, {Phoenix.PubSub, name: Mv.PubSub}, {AshAuthentication.Supervisor, otp_app: :my}, + Mv.Helpers.SystemActor, # Start a worker by calling: Mv.Worker.start_link(arg) # {Mv.Worker, arg}, # Start to serve requests, typically the last entry diff --git a/lib/mv/authorization/actor.ex b/lib/mv/authorization/actor.ex new file mode 100644 index 0000000..3482043 --- /dev/null +++ b/lib/mv/authorization/actor.ex @@ -0,0 +1,99 @@ +defmodule Mv.Authorization.Actor do + @moduledoc """ + Helper functions for ensuring User actors have required data loaded. + + ## Actor Invariant + + Authorization policies (especially HasPermission) require that the User actor + has their `:role` relationship loaded. This module provides helpers to + ensure this invariant is maintained across all entry points: + + - LiveView on_mount hooks + - Plug pipelines + - Background jobs + - Tests + + ## Scope + + This module ONLY handles `Mv.Accounts.User` resources. Other resources with + a `:role` field are ignored (returned as-is). This prevents accidental + authorization bypasses and keeps the logic focused. + + ## Usage + + # In LiveView on_mount + def ensure_user_role_loaded(_name, socket) do + user = Actor.ensure_loaded(socket.assigns[:current_user]) + assign(socket, :current_user, user) + end + + # In tests + user = Actor.ensure_loaded(user) + + ## Security Note + + `ensure_loaded/1` loads the role with `authorize?: false` to avoid circular + dependency (actor needs role loaded to be authorized, but loading role requires + authorization). This is safe because: + + - The actor (User) is loading their OWN role (user.role relationship) + - This load is needed FOR authorization checks to work + - The role itself contains no sensitive data (just permission_set reference) + - The actor is already authenticated (passed auth boundary) + + Alternative would be to denormalize permission_set_name on User, but that + adds complexity and potential for inconsistency. + """ + + require Logger + + @doc """ + Ensures the actor (User) has their `:role` relationship loaded. + + - If actor is nil, returns nil + - If role is already loaded, returns actor as-is + - If role is %Ash.NotLoaded{}, loads it and returns updated actor + - If actor is not a User, returns as-is (no-op) + + ## Examples + + iex> Actor.ensure_loaded(nil) + nil + + iex> Actor.ensure_loaded(%User{role: %Role{}}) + %User{role: %Role{}} + + iex> Actor.ensure_loaded(%User{role: %Ash.NotLoaded{}}) + %User{role: %Role{}} # role loaded + """ + def ensure_loaded(nil), do: nil + + # Only handle Mv.Accounts.User - clear intention, no accidental other resources + def ensure_loaded(%Mv.Accounts.User{role: %Ash.NotLoaded{}} = user) do + load_role(user) + end + + def ensure_loaded(actor), do: actor + + defp load_role(actor) do + # SECURITY: We skip authorization here because this is a bootstrap scenario: + # - The actor is loading their OWN role (actor.role relationship) + # - This load is needed FOR authorization checks to work (circular dependency) + # - The role itself contains no sensitive data (just permission_set reference) + # - The actor is already authenticated (passed auth boundary) + # Alternative would be to denormalize permission_set_name on User. + case Ash.load(actor, :role, domain: Mv.Accounts, authorize?: false) do + {:ok, loaded_actor} -> + loaded_actor + + {:error, error} -> + # Log error but don't crash - fail-closed for authorization + Logger.warning( + "Failed to load actor role: #{inspect(error)}. " <> + "Authorization may fail if role is required." + ) + + actor + end + end +end diff --git a/lib/mv/authorization/checks/has_permission.ex b/lib/mv/authorization/checks/has_permission.ex index 18ffe5b..97b74c0 100644 --- a/lib/mv/authorization/checks/has_permission.ex +++ b/lib/mv/authorization/checks/has_permission.ex @@ -8,10 +8,37 @@ defmodule Mv.Authorization.Checks.HasPermission do 3. Finds matching permission for current resource + action 4. Applies scope filter (:own, :linked, :all) + ## Important: strict_check Behavior + + For filter-based scopes (`:own`, `:linked`): + - **WITH record**: Evaluates filter against record (returns `true`/`false`) + - **WITHOUT record** (queries/lists): Returns `false` + + **Why `false` instead of `:unknown`?** + + Ash's policy evaluation doesn't reliably call `auto_filter` when `strict_check` + returns `:unknown`. To ensure list queries work correctly, resources **MUST** use + bypass policies with `expr()` for READ operations (see `docs/policy-bypass-vs-haspermission.md`). + + This means `HasPermission` is **NOT** generically reusable for query authorization + with filter scopes - it requires companion bypass policies. + + ## Usage Pattern + + See `docs/policy-bypass-vs-haspermission.md` for the two-tier pattern: + - **READ**: `bypass` with `expr()` (handles auto_filter) + - **UPDATE/CREATE/DESTROY**: `HasPermission` (handles scope evaluation) + ## Usage in Ash Resource policies do - policy action_type(:read) do + # READ: Bypass for list queries + bypass action_type(:read) do + authorize_if expr(id == ^actor(:id)) + end + + # UPDATE: HasPermission for scope evaluation + policy action_type([:update, :create, :destroy]) do authorize_if Mv.Authorization.Checks.HasPermission end end @@ -34,6 +61,12 @@ defmodule Mv.Authorization.Checks.HasPermission do All errors result in Forbidden (policy fails). + ## Role Loading Fallback + + If the actor's `:role` relationship is `%Ash.NotLoaded{}`, this check will + attempt to load it automatically. This provides a fallback if `on_mount` hooks + didn't run (e.g., in non-LiveView contexts). + ## Examples # In a resource policy @@ -83,6 +116,9 @@ defmodule Mv.Authorization.Checks.HasPermission do # Helper function to reduce nesting depth defp strict_check_with_permissions(actor, resource, action, record) do + # Ensure role is loaded (fallback if on_mount didn't run) + actor = ensure_role_loaded(actor) + with %{role: %{permission_set_name: ps_name}} when not is_nil(ps_name) <- actor, {:ok, ps_atom} <- PermissionSets.permission_set_name_to_atom(ps_name), permissions <- PermissionSets.get_permissions(ps_atom), @@ -95,11 +131,25 @@ defmodule Mv.Authorization.Checks.HasPermission do resource_name ) do :authorized -> + # For :all scope, authorize directly {:ok, true} {:filter, filter_expr} -> - # For strict_check on single records, evaluate the filter against the record - evaluate_filter_for_strict_check(filter_expr, actor, record, resource_name) + # For :own/:linked scope: + # - With a record, evaluate filter against record for strict authorization + # - Without a record (queries/lists), return false + # + # NOTE: Returning false here forces the use of expr-based bypass policies. + # This is necessary because Ash's policy evaluation doesn't reliably call auto_filter + # when strict_check returns :unknown. Instead, resources should use bypass policies + # with expr() directly for filter-based authorization (see User resource). + if record do + evaluate_filter_for_strict_check(filter_expr, actor, record, resource_name) + else + # No record yet (e.g., read/list queries) - deny at strict_check level + # Resources must use expr-based bypass policies for list filtering + {:ok, false} + end false -> {:ok, false} @@ -224,9 +274,18 @@ defmodule Mv.Authorization.Checks.HasPermission do end # Evaluate filter expression for strict_check on single records + # For :own scope with User resource: id == actor.id # For :linked scope with Member resource: id == actor.member_id defp evaluate_filter_for_strict_check(_filter_expr, actor, record, resource_name) do case {resource_name, record} do + {"User", %{id: user_id}} when not is_nil(user_id) -> + # Check if this user's ID matches the actor's ID (scope :own) + if user_id == actor.id do + {:ok, true} + else + {:ok, false} + end + {"Member", %{id: member_id}} when not is_nil(member_id) -> # Check if this member's ID matches the actor's member_id if member_id == actor.member_id do @@ -330,4 +389,10 @@ defmodule Mv.Authorization.Checks.HasPermission do defp get_resource_name_for_logging(_resource) do "unknown" end + + # Fallback: Load role if not loaded (in case on_mount didn't run) + # Delegates to centralized Actor helper + defp ensure_role_loaded(actor) do + Mv.Authorization.Actor.ensure_loaded(actor) + end end diff --git a/lib/mv/authorization/checks/no_actor.ex b/lib/mv/authorization/checks/no_actor.ex index f5eebb7..1c4946f 100644 --- a/lib/mv/authorization/checks/no_actor.ex +++ b/lib/mv/authorization/checks/no_actor.ex @@ -42,9 +42,9 @@ defmodule Mv.Authorization.Checks.NoActor do 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) + # 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 @@ -58,13 +58,9 @@ defmodule Mv.Authorization.Checks.NoActor do @impl true 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 + # 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 diff --git a/lib/mv/authorization/permission_sets.ex b/lib/mv/authorization/permission_sets.ex index 11ddb5a..22132cb 100644 --- a/lib/mv/authorization/permission_sets.ex +++ b/lib/mv/authorization/permission_sets.ex @@ -95,7 +95,9 @@ defmodule Mv.Authorization.PermissionSets do def get_permissions(:own_data) do %{ resources: [ - # User: Can always read/update own credentials + # User: Can read/update own credentials only + # IMPORTANT: "read_only" refers to member data, NOT user credentials. + # All permission sets grant User.update :own to allow password changes. %{resource: "User", action: :read, scope: :own, granted: true}, %{resource: "User", action: :update, scope: :own, granted: true}, @@ -125,6 +127,8 @@ defmodule Mv.Authorization.PermissionSets do %{ resources: [ # User: Can read/update own credentials only + # IMPORTANT: "read_only" refers to member data, NOT user credentials. + # All permission sets grant User.update :own to allow password changes. %{resource: "User", action: :read, scope: :own, granted: true}, %{resource: "User", action: :update, scope: :own, granted: true}, @@ -157,6 +161,8 @@ defmodule Mv.Authorization.PermissionSets do %{ resources: [ # User: Can read/update own credentials only + # IMPORTANT: "read_only" refers to member data, NOT user credentials. + # All permission sets grant User.update :own to allow password changes. %{resource: "User", action: :read, scope: :own, granted: true}, %{resource: "User", action: :update, scope: :own, granted: true}, diff --git a/lib/mv/email_sync/changes/sync_member_email_to_user.ex b/lib/mv/email_sync/changes/sync_member_email_to_user.ex index 0c0d8f7..48c7955 100644 --- a/lib/mv/email_sync/changes/sync_member_email_to_user.ex +++ b/lib/mv/email_sync/changes/sync_member_email_to_user.ex @@ -41,10 +41,8 @@ defmodule Mv.EmailSync.Changes.SyncMemberEmailToUser do Ash.Changeset.around_transaction(changeset, fn cs, callback -> result = callback.(cs) - actor = Map.get(changeset.context, :actor) - with {:ok, member} <- Helpers.extract_record(result), - linked_user <- Loader.get_linked_user(member, actor) do + linked_user <- Loader.get_linked_user(member) do Helpers.sync_email_to_linked_record(result, linked_user, new_email) else _ -> result diff --git a/lib/mv/email_sync/changes/sync_user_email_to_member.ex b/lib/mv/email_sync/changes/sync_user_email_to_member.ex index 54829a4..eb6770c 100644 --- a/lib/mv/email_sync/changes/sync_user_email_to_member.ex +++ b/lib/mv/email_sync/changes/sync_user_email_to_member.ex @@ -33,17 +33,7 @@ defmodule Mv.EmailSync.Changes.SyncUserEmailToMember do if Map.get(context, :syncing_email, false) do changeset else - # Ensure actor is in changeset context - get it from context if available - actor = Map.get(changeset.context, :actor) || Map.get(context, :actor) - - changeset_with_actor = - if actor && !Map.has_key?(changeset.context, :actor) do - Ash.Changeset.put_context(changeset, :actor, actor) - else - changeset - end - - sync_email(changeset_with_actor) + sync_email(changeset) end end @@ -52,7 +42,7 @@ defmodule Mv.EmailSync.Changes.SyncUserEmailToMember do result = callback.(cs) with {:ok, record} <- Helpers.extract_record(result), - {:ok, user, member} <- get_user_and_member(record, cs) do + {:ok, user, member} <- get_user_and_member(record) do # When called from Member-side, we need to update the member in the result # When called from User-side, we update the linked member in DB only case record do @@ -71,19 +61,16 @@ defmodule Mv.EmailSync.Changes.SyncUserEmailToMember do end # Retrieves user and member - works for both resource types - defp get_user_and_member(%Mv.Accounts.User{} = user, changeset) do - actor = Map.get(changeset.context, :actor) - - case Loader.get_linked_member(user, actor) do + # Uses system actor via Loader functions + defp get_user_and_member(%Mv.Accounts.User{} = user) do + case Loader.get_linked_member(user) do nil -> {:error, :no_member} member -> {:ok, user, member} end end - defp get_user_and_member(%Mv.Membership.Member{} = member, changeset) do - actor = Map.get(changeset.context, :actor) - - case Loader.load_linked_user!(member, actor) do + defp get_user_and_member(%Mv.Membership.Member{} = member) do + case Loader.load_linked_user!(member) do {:ok, user} -> {:ok, user, member} error -> error end diff --git a/lib/mv/email_sync/loader.ex b/lib/mv/email_sync/loader.ex index 91927fb..98f85df 100644 --- a/lib/mv/email_sync/loader.ex +++ b/lib/mv/email_sync/loader.ex @@ -5,25 +5,26 @@ defmodule Mv.EmailSync.Loader do ## Authorization - This module runs systemically and accepts optional actor parameters. - When called from hooks/changes, actor is extracted from changeset context. - When called directly, actor should be provided for proper authorization. + This module runs systemically and uses the system actor for all operations. + This ensures that email synchronization always works, regardless of user permissions. - All functions accept an optional `actor` parameter that is passed to Ash operations - to ensure proper authorization checks are performed. + All functions use `Mv.Helpers.SystemActor.get_system_actor/0` to bypass + user permission checks, as email sync is a mandatory side effect. """ alias Mv.Helpers + alias Mv.Helpers.SystemActor @doc """ Loads the member linked to a user, returns nil if not linked or on error. - Accepts optional actor for authorization. + Uses system actor for authorization to ensure email sync always works. """ - def get_linked_member(user, actor \\ nil) - def get_linked_member(%{member_id: nil}, _actor), do: nil + def get_linked_member(user) + def get_linked_member(%{member_id: nil}), do: nil - def get_linked_member(%{member_id: id}, actor) do - opts = Helpers.ash_actor_opts(actor) + def get_linked_member(%{member_id: id}) do + system_actor = SystemActor.get_system_actor() + opts = Helpers.ash_actor_opts(system_actor) case Ash.get(Mv.Membership.Member, id, opts) do {:ok, member} -> member @@ -34,10 +35,11 @@ defmodule Mv.EmailSync.Loader do @doc """ Loads the user linked to a member, returns nil if not linked or on error. - Accepts optional actor for authorization. + Uses system actor for authorization to ensure email sync always works. """ - def get_linked_user(member, actor \\ nil) do - opts = Helpers.ash_actor_opts(actor) + def get_linked_user(member) do + system_actor = SystemActor.get_system_actor() + opts = Helpers.ash_actor_opts(system_actor) case Ash.load(member, :user, opts) do {:ok, %{user: user}} -> user @@ -49,10 +51,11 @@ defmodule Mv.EmailSync.Loader do Loads the user linked to a member, returning an error tuple if not linked. Useful when a link is required for the operation. - Accepts optional actor for authorization. + Uses system actor for authorization to ensure email sync always works. """ - def load_linked_user!(member, actor \\ nil) do - opts = Helpers.ash_actor_opts(actor) + def load_linked_user!(member) do + system_actor = SystemActor.get_system_actor() + opts = Helpers.ash_actor_opts(system_actor) case Ash.load(member, :user, opts) do {:ok, %{user: user}} when not is_nil(user) -> {:ok, user} diff --git a/lib/mv/helpers/system_actor.ex b/lib/mv/helpers/system_actor.ex new file mode 100644 index 0000000..7a8ab8b --- /dev/null +++ b/lib/mv/helpers/system_actor.ex @@ -0,0 +1,436 @@ +defmodule Mv.Helpers.SystemActor do + @moduledoc """ + Provides access to the system actor for systemic operations. + + The system actor is a user with admin permissions that is used + for operations that must always run regardless of user permissions: + - Email synchronization + - Email uniqueness validation + - Cycle generation (if mandatory) + - Background jobs + - Seeds + + ## Usage + + # Get system actor for systemic operations + system_actor = Mv.Helpers.SystemActor.get_system_actor() + Ash.read(query, actor: system_actor) + + ## Implementation + + The system actor is cached in an Agent for performance. On first access, + it attempts to load a user with email "system@mila.local" and admin role. + If that user doesn't exist, it falls back to the admin user from seeds + (identified by ADMIN_EMAIL environment variable or "admin@localhost"). + + ## Caching + + The system actor is cached in an Agent to avoid repeated database queries. + The cache is invalidated on application restart. For long-running applications, + consider implementing cache invalidation on role changes. + + ## Race Conditions + + The system actor creation uses `upsert?: true` with `upsert_identity: :unique_email` + to prevent race conditions when multiple processes try to create the system user + simultaneously. This ensures idempotent creation and prevents database constraint errors. + + ## Security + + The system actor should NEVER be used for user-initiated actions. It is + only for systemic operations that must bypass user permissions. + + The system user is created without a password (`hashed_password = nil`) and + without an OIDC ID (`oidc_id = nil`) to prevent login. This ensures the + system user cannot be used for authentication, even if credentials are + somehow obtained. + """ + + use Agent + + require Ash.Query + + alias Mv.Config + + @doc """ + Starts the SystemActor Agent. + + This is called automatically by the application supervisor. + The agent starts with nil state and loads the system actor lazily on first access. + """ + def start_link(_opts) do + # Start with nil - lazy initialization on first get_system_actor call + # This prevents database access during application startup (important for tests) + Agent.start_link(fn -> nil end, name: __MODULE__) + end + + @doc """ + Returns the system actor (user with admin role). + + The system actor is cached in an Agent for performance. On first access, + it loads the system user from the database or falls back to the admin user. + + ## Returns + + - `%Mv.Accounts.User{}` - User with admin role loaded + - Raises if system actor cannot be found or loaded + + ## Examples + + iex> system_actor = Mv.Helpers.SystemActor.get_system_actor() + iex> system_actor.role.permission_set_name + "admin" + + """ + @spec get_system_actor() :: Mv.Accounts.User.t() + def get_system_actor do + case get_system_actor_result() do + {:ok, actor} -> actor + {:error, reason} -> raise "Failed to load system actor: #{inspect(reason)}" + end + end + + @doc """ + Returns the system actor as a result tuple. + + This variant returns `{:ok, actor}` or `{:error, reason}` instead of raising, + which is useful for error handling in pipes or when you want to handle errors explicitly. + + ## Returns + + - `{:ok, %Mv.Accounts.User{}}` - Successfully loaded system actor + - `{:error, term()}` - Error loading system actor + + ## Examples + + case SystemActor.get_system_actor_result() do + {:ok, actor} -> use_actor(actor) + {:error, reason} -> handle_error(reason) + end + + """ + @spec get_system_actor_result() :: {:ok, Mv.Accounts.User.t()} | {:error, term()} + def get_system_actor_result do + # In test environment (SQL sandbox), always load directly to avoid Agent/Sandbox issues + if Config.sql_sandbox?() do + try do + {:ok, load_system_actor()} + rescue + e -> {:error, e} + end + else + try do + result = + Agent.get_and_update(__MODULE__, fn + nil -> + # Cache miss - load system actor + try do + actor = load_system_actor() + {actor, actor} + rescue + e -> {{:error, e}, nil} + end + + cached_actor -> + # Cache hit - return cached actor + {cached_actor, cached_actor} + end) + + case result do + {:error, reason} -> {:error, reason} + actor -> {:ok, actor} + end + catch + :exit, {:noproc, _} -> + # Agent not started - load directly without caching + try do + {:ok, load_system_actor()} + rescue + e -> {:error, e} + end + end + end + end + + @doc """ + Invalidates the system actor cache. + + This forces a reload of the system actor on the next call to `get_system_actor/0`. + Useful when the system user's role might have changed. + + ## Examples + + iex> Mv.Helpers.SystemActor.invalidate_cache() + :ok + + """ + @spec invalidate_cache() :: :ok + def invalidate_cache do + case Process.whereis(__MODULE__) do + nil -> :ok + _pid -> Agent.update(__MODULE__, fn _state -> nil end) + end + end + + @doc """ + Returns the email address of the system user. + + This is useful for other modules that need to reference the system user + without loading the full user record. + + ## Returns + + - `String.t()` - The system user email address ("system@mila.local") + + ## Examples + + iex> Mv.Helpers.SystemActor.system_user_email() + "system@mila.local" + + """ + @spec system_user_email() :: String.t() + def system_user_email, do: system_user_email_config() + + # Returns the system user email from environment variable or default + # This allows configuration via SYSTEM_ACTOR_EMAIL env var + @spec system_user_email_config() :: String.t() + defp system_user_email_config do + System.get_env("SYSTEM_ACTOR_EMAIL") || "system@mila.local" + end + + # Loads the system actor from the database + # First tries to find system@mila.local, then falls back to admin user + @spec load_system_actor() :: Mv.Accounts.User.t() | no_return() + defp load_system_actor do + case find_user_by_email(system_user_email_config()) do + {:ok, user} when not is_nil(user) -> + load_user_with_role(user) + + {:ok, nil} -> + handle_system_user_not_found("no system user or admin user found") + + {:error, _reason} = error -> + handle_system_user_error(error) + end + end + + # Handles case when system user doesn't exist + @spec handle_system_user_not_found(String.t()) :: Mv.Accounts.User.t() | no_return() + defp handle_system_user_not_found(message) do + case load_admin_user_fallback() do + {:ok, admin_user} -> + admin_user + + {:error, _} -> + handle_fallback_error(message) + end + end + + # Handles database error when loading system user + @spec handle_system_user_error(term()) :: Mv.Accounts.User.t() | no_return() + defp handle_system_user_error(error) do + case load_admin_user_fallback() do + {:ok, admin_user} -> + admin_user + + {:error, _} -> + handle_fallback_error("Failed to load system actor: #{inspect(error)}") + end + end + + # Handles fallback error - creates test actor or raises + @spec handle_fallback_error(String.t()) :: Mv.Accounts.User.t() | no_return() + defp handle_fallback_error(message) do + if Config.sql_sandbox?() do + create_test_system_actor() + else + raise "Failed to load system actor: #{message}" + end + end + + # Creates a temporary admin user for tests when no system/admin user exists + @spec create_test_system_actor() :: Mv.Accounts.User.t() | no_return() + defp create_test_system_actor do + alias Mv.Accounts + alias Mv.Authorization + + admin_role = ensure_admin_role_exists() + create_system_user_with_role(admin_role) + end + + # Ensures admin role exists - finds or creates it + @spec ensure_admin_role_exists() :: Mv.Authorization.Role.t() | no_return() + defp ensure_admin_role_exists do + case find_admin_role() do + {:ok, role} -> + role + + {:error, :not_found} -> + create_admin_role_with_retry() + end + end + + # Finds admin role in existing roles + @spec find_admin_role() :: {:ok, Mv.Authorization.Role.t()} | {:error, :not_found} + defp find_admin_role do + alias Mv.Authorization + + case Authorization.list_roles() do + {:ok, roles} -> + case Enum.find(roles, &(&1.permission_set_name == "admin")) do + nil -> {:error, :not_found} + role -> {:ok, role} + end + + _ -> + {:error, :not_found} + end + end + + # Creates admin role, handling race conditions + @spec create_admin_role_with_retry() :: Mv.Authorization.Role.t() | no_return() + defp create_admin_role_with_retry do + alias Mv.Authorization + + case create_admin_role() do + {:ok, role} -> + role + + {:error, :already_exists} -> + find_existing_admin_role() + + {:error, error} -> + raise "Failed to create admin role: #{inspect(error)}" + end + end + + # Attempts to create admin role + @spec create_admin_role() :: + {:ok, Mv.Authorization.Role.t()} | {:error, :already_exists | term()} + defp create_admin_role do + alias Mv.Authorization + + case Authorization.create_role(%{ + name: "Admin", + description: "Administrator with full access", + permission_set_name: "admin" + }) do + {:ok, role} -> + {:ok, role} + + {:error, %Ash.Error.Invalid{errors: [%{field: :name, message: "has already been taken"}]}} -> + {:error, :already_exists} + + {:error, error} -> + {:error, error} + end + end + + # Finds existing admin role after creation attempt failed due to race condition + @spec find_existing_admin_role() :: Mv.Authorization.Role.t() | no_return() + defp find_existing_admin_role do + alias Mv.Authorization + + case Authorization.list_roles() do + {:ok, roles} -> + Enum.find(roles, &(&1.permission_set_name == "admin")) || + raise "Admin role should exist but was not found" + + _ -> + raise "Failed to find admin role after creation attempt" + end + end + + # Creates system user with admin role assigned + # SECURITY: System user is created without password (hashed_password = nil) and + # without OIDC ID (oidc_id = nil) to prevent login. This user is ONLY for + # internal system operations via SystemActor and should never be used for authentication. + @spec create_system_user_with_role(Mv.Authorization.Role.t()) :: + Mv.Accounts.User.t() | no_return() + defp create_system_user_with_role(admin_role) do + alias Mv.Accounts + + Accounts.create_user!(%{email: system_user_email_config()}, + upsert?: true, + upsert_identity: :unique_email + ) + |> Ash.Changeset.for_update(:update, %{}) + |> Ash.Changeset.manage_relationship(:role, admin_role, type: :append_and_remove) + |> Ash.update!() + |> Ash.load!(:role, domain: Mv.Accounts) + end + + # Finds a user by email address + # SECURITY: Uses authorize?: false for bootstrap lookup only. + # This is necessary because we need to find the system/admin user before + # we can load the system actor. If User policies require an actor, this + # would create a chicken-and-egg problem. This is safe because: + # 1. We only query by email (no sensitive data exposed) + # 2. This is only used during system actor initialization (bootstrap phase) + # 3. Once system actor is loaded, all subsequent operations use proper authorization + @spec find_user_by_email(String.t()) :: {:ok, Mv.Accounts.User.t() | nil} | {:error, term()} + defp find_user_by_email(email) do + Mv.Accounts.User + |> Ash.Query.filter(email == ^email) + |> Ash.read_one(domain: Mv.Accounts, authorize?: false) + end + + # Loads a user with their role preloaded (required for authorization) + @spec load_user_with_role(Mv.Accounts.User.t()) :: Mv.Accounts.User.t() | no_return() + defp load_user_with_role(user) do + case Ash.load(user, :role, domain: Mv.Accounts) do + {:ok, user_with_role} -> + validate_admin_role(user_with_role) + + {:error, reason} -> + raise "Failed to load role for system actor: #{inspect(reason)}" + end + end + + # Validates that the user has an admin role + @spec validate_admin_role(Mv.Accounts.User.t()) :: Mv.Accounts.User.t() | no_return() + defp validate_admin_role(%{role: %{permission_set_name: "admin"}} = user) do + user + end + + @spec validate_admin_role(Mv.Accounts.User.t()) :: no_return() + defp validate_admin_role(%{role: %{permission_set_name: permission_set}}) do + raise """ + System actor must have admin role, but has permission_set_name: #{permission_set} + Please assign the "Admin" role to the system user. + """ + end + + @spec validate_admin_role(Mv.Accounts.User.t()) :: no_return() + defp validate_admin_role(%{role: nil}) do + raise """ + System actor must have a role assigned, but role is nil. + Please assign the "Admin" role to the system user. + """ + end + + @spec validate_admin_role(term()) :: no_return() + defp validate_admin_role(_user) do + raise """ + System actor must have a role with admin permissions. + Please assign the "Admin" role to the system user. + """ + end + + # Fallback: Loads admin user from seeds (ADMIN_EMAIL env var or default) + @spec load_admin_user_fallback() :: {:ok, Mv.Accounts.User.t()} | {:error, term()} + defp load_admin_user_fallback do + admin_email = System.get_env("ADMIN_EMAIL") || "admin@localhost" + + case find_user_by_email(admin_email) do + {:ok, user} when not is_nil(user) -> + {:ok, load_user_with_role(user)} + + {:ok, nil} -> + {:error, :admin_user_not_found} + + {:error, _reason} = error -> + error + end + end +end diff --git a/lib/mv/membership/member/validations/email_not_used_by_other_user.ex b/lib/mv/membership/member/validations/email_not_used_by_other_user.ex index e1bbc4e..f9fba1b 100644 --- a/lib/mv/membership/member/validations/email_not_used_by_other_user.ex +++ b/lib/mv/membership/member/validations/email_not_used_by_other_user.ex @@ -10,6 +10,8 @@ defmodule Mv.Membership.Member.Validations.EmailNotUsedByOtherUser do use Ash.Resource.Validation alias Mv.Helpers + require Logger + @doc """ Validates email uniqueness across linked Member-User pairs. @@ -30,8 +32,7 @@ defmodule Mv.Membership.Member.Validations.EmailNotUsedByOtherUser do def validate(changeset, _opts, _context) do email_changing? = Ash.Changeset.changing_attribute?(changeset, :email) - actor = Map.get(changeset.context || %{}, :actor) - linked_user_id = get_linked_user_id(changeset.data, actor) + linked_user_id = get_linked_user_id(changeset.data) is_linked? = not is_nil(linked_user_id) # Only validate if member is already linked AND email is changing @@ -40,19 +41,22 @@ defmodule Mv.Membership.Member.Validations.EmailNotUsedByOtherUser do if should_validate? do new_email = Ash.Changeset.get_attribute(changeset, :email) - check_email_uniqueness(new_email, linked_user_id, actor) + check_email_uniqueness(new_email, linked_user_id) else :ok end end - defp check_email_uniqueness(email, exclude_user_id, actor) do + defp check_email_uniqueness(email, exclude_user_id) do + alias Mv.Helpers.SystemActor + query = Mv.Accounts.User |> Ash.Query.filter(email == ^email) |> maybe_exclude_id(exclude_user_id) - opts = Helpers.ash_actor_opts(actor) + system_actor = SystemActor.get_system_actor() + opts = Helpers.ash_actor_opts(system_actor) case Ash.read(query, opts) do {:ok, []} -> @@ -61,7 +65,11 @@ defmodule Mv.Membership.Member.Validations.EmailNotUsedByOtherUser do {:ok, _} -> {:error, field: :email, message: "is already used by another user", value: email} - {:error, _} -> + {:error, reason} -> + Logger.warning( + "Email uniqueness validation query failed for member email '#{email}': #{inspect(reason)}. Allowing operation to proceed (fail-open)." + ) + :ok end end @@ -69,8 +77,11 @@ defmodule Mv.Membership.Member.Validations.EmailNotUsedByOtherUser do defp maybe_exclude_id(query, nil), do: query defp maybe_exclude_id(query, id), do: Ash.Query.filter(query, id != ^id) - defp get_linked_user_id(member_data, actor) do - opts = Helpers.ash_actor_opts(actor) + defp get_linked_user_id(member_data) do + alias Mv.Helpers.SystemActor + + system_actor = SystemActor.get_system_actor() + opts = Helpers.ash_actor_opts(system_actor) case Ash.load(member_data, :user, opts) do {:ok, %{user: %{id: id}}} -> id diff --git a/lib/mv/membership_fees/cycle_generator.ex b/lib/mv/membership_fees/cycle_generator.ex index 0d17dd3..ec33914 100644 --- a/lib/mv/membership_fees/cycle_generator.ex +++ b/lib/mv/membership_fees/cycle_generator.ex @@ -30,12 +30,11 @@ defmodule Mv.MembershipFees.CycleGenerator do ## Authorization - This module runs systemically and accepts optional actor parameters. - When called from hooks/changes, actor is extracted from changeset context. - When called directly, actor should be provided for proper authorization. + This module runs systemically and uses the system actor for all operations. + This ensures that cycle generation always works, regardless of user permissions. - All functions accept an optional `actor` parameter in the `opts` keyword list - that is passed to Ash operations to ensure proper authorization checks are performed. + All functions use `Mv.Helpers.SystemActor.get_system_actor/0` to bypass + user permission checks, as cycle generation is a mandatory side effect. ## Examples @@ -47,6 +46,8 @@ defmodule Mv.MembershipFees.CycleGenerator do """ + alias Mv.Helpers + alias Mv.Helpers.SystemActor alias Mv.Membership.Member alias Mv.MembershipFees.CalendarCycles alias Mv.MembershipFees.Changes.SetMembershipFeeStartDate @@ -86,9 +87,7 @@ defmodule Mv.MembershipFees.CycleGenerator do def generate_cycles_for_member(member_or_id, opts \\ []) def generate_cycles_for_member(member_id, opts) when is_binary(member_id) do - actor = Keyword.get(opts, :actor) - - case load_member(member_id, actor: actor) do + case load_member(member_id) do {:ok, member} -> generate_cycles_for_member(member, opts) {:error, reason} -> {:error, reason} end @@ -98,27 +97,25 @@ defmodule Mv.MembershipFees.CycleGenerator do today = Keyword.get(opts, :today, Date.utc_today()) skip_lock? = Keyword.get(opts, :skip_lock?, false) - do_generate_cycles_with_lock(member, today, skip_lock?, opts) + do_generate_cycles_with_lock(member, today, skip_lock?) end # Generate cycles with lock handling # Returns {:ok, cycles, notifications} - notifications are never sent here, # they should be returned to the caller (e.g., via after_action hook) - defp do_generate_cycles_with_lock(member, today, true = _skip_lock?, opts) do + defp do_generate_cycles_with_lock(member, today, true = _skip_lock?) do # Lock already set by caller (e.g., regenerate_cycles_on_type_change) # Just generate cycles without additional locking - actor = Keyword.get(opts, :actor) - do_generate_cycles(member, today, actor: actor) + do_generate_cycles(member, today) end - defp do_generate_cycles_with_lock(member, today, false, opts) do + defp do_generate_cycles_with_lock(member, today, false) do lock_key = :erlang.phash2(member.id) - actor = Keyword.get(opts, :actor) Repo.transaction(fn -> Ecto.Adapters.SQL.query!(Repo, "SELECT pg_advisory_xact_lock($1)", [lock_key]) - case do_generate_cycles(member, today, actor: actor) do + case do_generate_cycles(member, today) do {:ok, cycles, notifications} -> # Return cycles and notifications - do NOT send notifications here # They will be sent by the caller (e.g., via after_action hook) @@ -168,12 +165,15 @@ defmodule Mv.MembershipFees.CycleGenerator do # Query ALL members with fee type assigned (including inactive/left members) # The exit_date boundary is applied during cycle generation, not here. # This allows catch-up generation for members who left but are missing cycles. + system_actor = SystemActor.get_system_actor() + opts = Helpers.ash_actor_opts(system_actor) + query = Member |> Ash.Query.filter(not is_nil(membership_fee_type_id)) |> Ash.Query.filter(not is_nil(join_date)) - case Ash.read(query) do + case Ash.read(query, opts) do {:ok, members} -> results = process_members_in_batches(members, batch_size, today) {:ok, build_results_summary(results)} @@ -235,33 +235,25 @@ defmodule Mv.MembershipFees.CycleGenerator do # Private functions - defp load_member(member_id, opts) do - actor = Keyword.get(opts, :actor) + defp load_member(member_id) do + system_actor = SystemActor.get_system_actor() + opts = Helpers.ash_actor_opts(system_actor) query = Member |> Ash.Query.filter(id == ^member_id) |> Ash.Query.load([:membership_fee_type, :membership_fee_cycles]) - result = - if actor do - Ash.read_one(query, actor: actor) - else - Ash.read_one(query) - end - - case result do + case Ash.read_one(query, opts) do {:ok, nil} -> {:error, :member_not_found} {:ok, member} -> {:ok, member} {:error, reason} -> {:error, reason} end end - defp do_generate_cycles(member, today, opts) do - actor = Keyword.get(opts, :actor) - + defp do_generate_cycles(member, today) do # Reload member with relationships to ensure fresh data - case load_member(member.id, actor: actor) do + case load_member(member.id) do {:ok, member} -> cond do is_nil(member.membership_fee_type_id) -> @@ -271,7 +263,7 @@ defmodule Mv.MembershipFees.CycleGenerator do {:error, :no_join_date} true -> - generate_missing_cycles(member, today, actor: actor) + generate_missing_cycles(member, today) end {:error, reason} -> @@ -279,8 +271,7 @@ defmodule Mv.MembershipFees.CycleGenerator do end end - defp generate_missing_cycles(member, today, opts) do - actor = Keyword.get(opts, :actor) + defp generate_missing_cycles(member, today) do fee_type = member.membership_fee_type interval = fee_type.interval amount = fee_type.amount @@ -296,7 +287,7 @@ defmodule Mv.MembershipFees.CycleGenerator do # Only generate if start_date <= end_date if start_date && Date.compare(start_date, end_date) != :gt do cycle_starts = generate_cycle_starts(start_date, end_date, interval) - create_cycles(cycle_starts, member.id, fee_type.id, amount, actor: actor) + create_cycles(cycle_starts, member.id, fee_type.id, amount) else {:ok, [], []} end @@ -391,8 +382,10 @@ defmodule Mv.MembershipFees.CycleGenerator do end end - defp create_cycles(cycle_starts, member_id, fee_type_id, amount, opts) do - actor = Keyword.get(opts, :actor) + defp create_cycles(cycle_starts, member_id, fee_type_id, amount) do + system_actor = SystemActor.get_system_actor() + opts = Helpers.ash_actor_opts(system_actor) + # Always use return_notifications?: true to collect notifications # Notifications will be returned to the caller, who is responsible for # sending them (e.g., via after_action hook returning {:ok, result, notifications}) @@ -407,7 +400,7 @@ defmodule Mv.MembershipFees.CycleGenerator do } handle_cycle_creation_result( - Ash.create(MembershipFeeCycle, attrs, return_notifications?: true, actor: actor), + Ash.create(MembershipFeeCycle, attrs, [return_notifications?: true] ++ opts), cycle_start ) end) diff --git a/lib/mv_web.ex b/lib/mv_web.ex index 08a3c23..2b1ade6 100644 --- a/lib/mv_web.ex +++ b/lib/mv_web.ex @@ -52,7 +52,8 @@ defmodule MvWeb do quote do use Phoenix.LiveView - on_mount MvWeb.LiveHelpers + on_mount {MvWeb.LiveHelpers, :default} + on_mount {MvWeb.LiveHelpers, :ensure_user_role_loaded} unquote(html_helpers()) end diff --git a/lib/mv_web/live/auth/link_oidc_account_live.ex b/lib/mv_web/live/auth/link_oidc_account_live.ex index 05faf67..325a2fd 100644 --- a/lib/mv_web/live/auth/link_oidc_account_live.ex +++ b/lib/mv_web/live/auth/link_oidc_account_live.ex @@ -20,10 +20,13 @@ defmodule MvWeb.LinkOidcAccountLive do @impl true def mount(_params, session, socket) do + # Use SystemActor for authorization during OIDC linking (user is not yet logged in) + system_actor = Mv.Helpers.SystemActor.get_system_actor() + with user_id when not is_nil(user_id) <- Map.get(session, "oidc_linking_user_id"), oidc_user_info when not is_nil(oidc_user_info) <- Map.get(session, "oidc_linking_user_info"), - {:ok, user} <- Ash.get(Mv.Accounts.User, user_id) do + {:ok, user} <- Ash.get(Mv.Accounts.User, user_id, actor: system_actor) do # Check if user is passwordless if passwordless?(user) do # Auto-link passwordless user immediately @@ -46,9 +49,12 @@ defmodule MvWeb.LinkOidcAccountLive do end defp reload_user!(user_id) do + # Use SystemActor for authorization during OIDC linking (user is not yet logged in) + system_actor = Mv.Helpers.SystemActor.get_system_actor() + Mv.Accounts.User |> Ash.Query.filter(id == ^user_id) - |> Ash.read_one!() + |> Ash.read_one!(actor: system_actor) end defp reset_password_form(socket) do @@ -58,13 +64,16 @@ defmodule MvWeb.LinkOidcAccountLive do defp auto_link_passwordless_user(socket, user, oidc_user_info) do oidc_id = Map.get(oidc_user_info, "sub") || Map.get(oidc_user_info, "id") + # Use SystemActor for authorization (passwordless user auto-linking) + system_actor = Mv.Helpers.SystemActor.get_system_actor() + case user.id |> reload_user!() |> Ash.Changeset.for_update(:link_oidc_id, %{ oidc_id: oidc_id, oidc_user_info: oidc_user_info }) - |> Ash.update() do + |> Ash.update(actor: system_actor) do {:ok, updated_user} -> Logger.info( "Passwordless account auto-linked to OIDC: user_id=#{updated_user.id}, oidc_id=#{oidc_id}" @@ -187,6 +196,9 @@ defmodule MvWeb.LinkOidcAccountLive do defp link_oidc_account(socket, user, oidc_user_info) do oidc_id = Map.get(oidc_user_info, "sub") || Map.get(oidc_user_info, "id") + # Use SystemActor for authorization (user just verified password but is not yet logged in) + system_actor = Mv.Helpers.SystemActor.get_system_actor() + # Update the user with the OIDC ID case user.id |> reload_user!() @@ -194,7 +206,7 @@ defmodule MvWeb.LinkOidcAccountLive do oidc_id: oidc_id, oidc_user_info: oidc_user_info }) - |> Ash.update() do + |> Ash.update(actor: system_actor) do {:ok, updated_user} -> # After successful linking, redirect to OIDC login # Since the user now has an oidc_id, the next OIDC login will succeed diff --git a/lib/mv_web/live/member_live/form.ex b/lib/mv_web/live/member_live/form.ex index 395198a..2f3a0ff 100644 --- a/lib/mv_web/live/member_live/form.ex +++ b/lib/mv_web/live/member_live/form.ex @@ -21,8 +21,6 @@ defmodule MvWeb.MemberLive.Form do """ use MvWeb, :live_view - on_mount {MvWeb.LiveHelpers, :ensure_user_role_loaded} - import MvWeb.LiveHelpers, only: [current_actor: 1, submit_form: 3] alias Mv.MembershipFees diff --git a/lib/mv_web/live/member_live/index.ex b/lib/mv_web/live/member_live/index.ex index f8d7323..3083d65 100644 --- a/lib/mv_web/live/member_live/index.ex +++ b/lib/mv_web/live/member_live/index.ex @@ -27,8 +27,6 @@ defmodule MvWeb.MemberLive.Index do """ use MvWeb, :live_view - on_mount {MvWeb.LiveHelpers, :ensure_user_role_loaded} - require Ash.Query require Logger import Ash.Expr diff --git a/lib/mv_web/live/member_live/show.ex b/lib/mv_web/live/member_live/show.ex index 359a9b2..d484672 100644 --- a/lib/mv_web/live/member_live/show.ex +++ b/lib/mv_web/live/member_live/show.ex @@ -22,8 +22,6 @@ defmodule MvWeb.MemberLive.Show do import Ash.Query import MvWeb.LiveHelpers, only: [current_actor: 1] - on_mount {MvWeb.LiveHelpers, :ensure_user_role_loaded} - alias MvWeb.Helpers.MembershipFeeHelpers @impl true diff --git a/lib/mv_web/live/member_live/show/membership_fees_component.ex b/lib/mv_web/live/member_live/show/membership_fees_component.ex index e85baab..5350e9f 100644 --- a/lib/mv_web/live/member_live/show/membership_fees_component.ex +++ b/lib/mv_web/live/member_live/show/membership_fees_component.ex @@ -554,46 +554,55 @@ defmodule MvWeb.MemberLive.Show.MembershipFeesComponent do end def handle_event("regenerate_cycles", _params, socket) do - socket = assign(socket, :regenerating, true) - member = socket.assigns.member actor = current_actor(socket) - case CycleGenerator.generate_cycles_for_member(member.id, actor: actor) do - {:ok, _new_cycles, _notifications} -> - # Reload member with cycles - actor = current_actor(socket) + # SECURITY: Only admins can manually regenerate cycles via UI + # Cycle generation itself uses system actor, but UI access should be restricted + if actor.role && actor.role.permission_set_name == "admin" do + socket = assign(socket, :regenerating, true) + member = socket.assigns.member - updated_member = - member - |> Ash.load!( - [ - :membership_fee_type, - membership_fee_cycles: [:membership_fee_type] - ], - actor: actor - ) + case CycleGenerator.generate_cycles_for_member(member.id) do + {:ok, _new_cycles, _notifications} -> + # Reload member with cycles + actor = current_actor(socket) - cycles = - Enum.sort_by( - updated_member.membership_fee_cycles || [], - & &1.cycle_start, - {:desc, Date} - ) + updated_member = + member + |> Ash.load!( + [ + :membership_fee_type, + membership_fee_cycles: [:membership_fee_type] + ], + actor: actor + ) - send(self(), {:member_updated, updated_member}) + cycles = + Enum.sort_by( + updated_member.membership_fee_cycles || [], + & &1.cycle_start, + {:desc, Date} + ) - {:noreply, - socket - |> assign(:member, updated_member) - |> assign(:cycles, cycles) - |> assign(:regenerating, false) - |> put_flash(:info, gettext("Cycles regenerated successfully"))} + send(self(), {:member_updated, updated_member}) - {:error, error} -> - {:noreply, - socket - |> assign(:regenerating, false) - |> put_flash(:error, format_error(error))} + {:noreply, + socket + |> assign(:member, updated_member) + |> assign(:cycles, cycles) + |> assign(:regenerating, false) + |> put_flash(:info, gettext("Cycles regenerated successfully"))} + + {:error, error} -> + {:noreply, + socket + |> assign(:regenerating, false) + |> put_flash(:error, format_error(error))} + end + else + {:noreply, + socket + |> put_flash(:error, gettext("Only administrators can regenerate cycles"))} end end diff --git a/lib/mv_web/live/membership_fee_type_live/form.ex b/lib/mv_web/live/membership_fee_type_live/form.ex index 76d3bfa..fc9ee65 100644 --- a/lib/mv_web/live/membership_fee_type_live/form.ex +++ b/lib/mv_web/live/membership_fee_type_live/form.ex @@ -13,7 +13,6 @@ defmodule MvWeb.MembershipFeeTypeLive.Form do """ use MvWeb, :live_view - on_mount {MvWeb.LiveHelpers, :ensure_user_role_loaded} import MvWeb.LiveHelpers, only: [current_actor: 1, submit_form: 3] require Ash.Query diff --git a/lib/mv_web/live/membership_fee_type_live/index.ex b/lib/mv_web/live/membership_fee_type_live/index.ex index b3eecc0..84cd26d 100644 --- a/lib/mv_web/live/membership_fee_type_live/index.ex +++ b/lib/mv_web/live/membership_fee_type_live/index.ex @@ -14,7 +14,6 @@ defmodule MvWeb.MembershipFeeTypeLive.Index do """ use MvWeb, :live_view - on_mount {MvWeb.LiveHelpers, :ensure_user_role_loaded} import MvWeb.LiveHelpers, only: [current_actor: 1] require Ash.Query diff --git a/lib/mv_web/live/role_live/form.ex b/lib/mv_web/live/role_live/form.ex index 0ad0fdd..ea76fe8 100644 --- a/lib/mv_web/live/role_live/form.ex +++ b/lib/mv_web/live/role_live/form.ex @@ -17,8 +17,6 @@ defmodule MvWeb.RoleLive.Form do import MvWeb.RoleLive.Helpers, only: [format_error: 1] - on_mount {MvWeb.LiveHelpers, :ensure_user_role_loaded} - @impl true def render(assigns) do ~H""" diff --git a/lib/mv_web/live/role_live/index.ex b/lib/mv_web/live/role_live/index.ex index 4f8e45d..091b191 100644 --- a/lib/mv_web/live/role_live/index.ex +++ b/lib/mv_web/live/role_live/index.ex @@ -24,8 +24,6 @@ defmodule MvWeb.RoleLive.Index do import MvWeb.RoleLive.Helpers, only: [format_error: 1, permission_set_badge_class: 1, opts_with_actor: 3] - on_mount {MvWeb.LiveHelpers, :ensure_user_role_loaded} - @impl true def mount(_params, _session, socket) do actor = socket.assigns[:current_user] diff --git a/lib/mv_web/live/role_live/show.ex b/lib/mv_web/live/role_live/show.ex index 7184b68..0e1c7ca 100644 --- a/lib/mv_web/live/role_live/show.ex +++ b/lib/mv_web/live/role_live/show.ex @@ -19,8 +19,6 @@ defmodule MvWeb.RoleLive.Show do import MvWeb.RoleLive.Helpers, only: [format_error: 1, permission_set_badge_class: 1, opts_with_actor: 3] - on_mount {MvWeb.LiveHelpers, :ensure_user_role_loaded} - @impl true def mount(%{"id" => id}, _session, socket) do try do diff --git a/lib/mv_web/live/user_live/form.ex b/lib/mv_web/live/user_live/form.ex index 7b02053..3e773cb 100644 --- a/lib/mv_web/live/user_live/form.ex +++ b/lib/mv_web/live/user_live/form.ex @@ -33,7 +33,6 @@ defmodule MvWeb.UserLive.Form do """ use MvWeb, :live_view - on_mount {MvWeb.LiveHelpers, :ensure_user_role_loaded} import MvWeb.LiveHelpers, only: [current_actor: 1, submit_form: 3] @impl true diff --git a/lib/mv_web/live/user_live/index.ex b/lib/mv_web/live/user_live/index.ex index 5e5949e..2062ae6 100644 --- a/lib/mv_web/live/user_live/index.ex +++ b/lib/mv_web/live/user_live/index.ex @@ -23,7 +23,6 @@ defmodule MvWeb.UserLive.Index do use MvWeb, :live_view import MvWeb.TableComponents - on_mount {MvWeb.LiveHelpers, :ensure_user_role_loaded} import MvWeb.LiveHelpers, only: [current_actor: 1] @impl true diff --git a/lib/mv_web/live/user_live/show.ex b/lib/mv_web/live/user_live/show.ex index 4a5b5d1..641e091 100644 --- a/lib/mv_web/live/user_live/show.ex +++ b/lib/mv_web/live/user_live/show.ex @@ -26,7 +26,6 @@ defmodule MvWeb.UserLive.Show do """ use MvWeb, :live_view - on_mount {MvWeb.LiveHelpers, :ensure_user_role_loaded} import MvWeb.LiveHelpers, only: [current_actor: 1] @impl true diff --git a/lib/mv_web/live_helpers.ex b/lib/mv_web/live_helpers.ex index 95d8235..b8f070c 100644 --- a/lib/mv_web/live_helpers.ex +++ b/lib/mv_web/live_helpers.ex @@ -27,39 +27,17 @@ defmodule MvWeb.LiveHelpers do end defp ensure_user_role_loaded(socket) do - if socket.assigns[:current_user] do - user = socket.assigns.current_user - user_with_role = load_user_role(user) + user = socket.assigns[:current_user] + + if user do + # Use centralized Actor helper to ensure role is loaded + user_with_role = Mv.Authorization.Actor.ensure_loaded(user) assign(socket, :current_user, user_with_role) else socket end end - defp load_user_role(user) do - case Map.get(user, :role) do - %Ash.NotLoaded{} -> load_role_safely(user) - nil -> load_role_safely(user) - _role -> user - end - end - - defp load_role_safely(user) do - # Use self as actor for loading own role relationship - opts = [domain: Mv.Accounts, actor: user] - - case Ash.load(user, :role, opts) do - {:ok, loaded_user} -> - loaded_user - - {:error, error} -> - # Log warning if role loading fails - this can cause authorization issues - require Logger - Logger.warning("Failed to load role for user #{user.id}: #{inspect(error)}") - user - end - end - @doc """ Helper function to get the current actor (user) from socket assigns. diff --git a/priv/gettext/de/LC_MESSAGES/default.po b/priv/gettext/de/LC_MESSAGES/default.po index 3369548..3463f17 100644 --- a/priv/gettext/de/LC_MESSAGES/default.po +++ b/priv/gettext/de/LC_MESSAGES/default.po @@ -1945,12 +1945,7 @@ msgstr "Bezahlstatus" msgid "Reset" msgstr "Zurücksetzen" -#~ #: lib/mv_web/live/components/member_filter_component.ex -#~ #, elixir-autogen, elixir-format -#~ msgid "Filter by %{name}" -#~ msgstr "Filtern nach %{name}" - -#~ #: lib/mv_web/live/components/member_filter_component.ex -#~ #, elixir-autogen, elixir-format, fuzzy -#~ msgid "Payment status filter" -#~ msgstr "Bezahlstatusfilter" +#: lib/mv_web/live/member_live/show/membership_fees_component.ex +#, elixir-autogen, elixir-format +msgid "Only administrators can regenerate cycles" +msgstr "Nur Administrator*innen können Zyklen regenerieren" diff --git a/priv/gettext/default.pot b/priv/gettext/default.pot index fb50446..8a0a91a 100644 --- a/priv/gettext/default.pot +++ b/priv/gettext/default.pot @@ -1945,3 +1945,8 @@ msgstr "" #, elixir-autogen, elixir-format msgid "Reset" msgstr "" + +#: lib/mv_web/live/member_live/show/membership_fees_component.ex +#, elixir-autogen, elixir-format +msgid "Only administrators can regenerate cycles" +msgstr "" diff --git a/priv/gettext/en/LC_MESSAGES/default.po b/priv/gettext/en/LC_MESSAGES/default.po index 37a7a0d..421bab3 100644 --- a/priv/gettext/en/LC_MESSAGES/default.po +++ b/priv/gettext/en/LC_MESSAGES/default.po @@ -1946,6 +1946,11 @@ msgstr "" msgid "Reset" msgstr "" +#: lib/mv_web/live/member_live/show/membership_fees_component.ex +#, elixir-autogen, elixir-format +msgid "Only administrators can regenerate cycles" +msgstr "" + #~ #: lib/mv_web/live/custom_field_value_live/form.ex #~ #, elixir-autogen, elixir-format, fuzzy #~ msgid "Use this form to manage Custom Field Value records in your database." diff --git a/priv/repo/seeds.exs b/priv/repo/seeds.exs index 2e7543a..91b6fa3 100644 --- a/priv/repo/seeds.exs +++ b/priv/repo/seeds.exs @@ -161,24 +161,30 @@ end # This handles both existing users (e.g., from OIDC) and newly created users case Accounts.User |> Ash.Query.filter(email == ^admin_email) - |> Ash.read_one(domain: Mv.Accounts) do + |> Ash.read_one(domain: Mv.Accounts, authorize?: false) do {:ok, existing_admin_user} when not is_nil(existing_admin_user) -> # User already exists (e.g., via OIDC) - assign admin role + # Use authorize?: false for bootstrap - this is initial setup existing_admin_user |> Ash.Changeset.for_update(:update, %{}) |> Ash.Changeset.manage_relationship(:role, admin_role, type: :append_and_remove) - |> Ash.update!() + |> Ash.update!(authorize?: false) {:ok, nil} -> # User doesn't exist - create admin user with password - Accounts.create_user!(%{email: admin_email}, upsert?: true, upsert_identity: :unique_email) + # Use authorize?: false for bootstrap - no admin user exists yet to use as actor + Accounts.create_user!(%{email: admin_email}, + upsert?: true, + upsert_identity: :unique_email, + authorize?: false + ) |> Ash.Changeset.for_update(:admin_set_password, %{password: "testpassword"}) - |> Ash.update!() + |> Ash.update!(authorize?: false) |> then(fn user -> user |> Ash.Changeset.for_update(:update, %{}) |> Ash.Changeset.manage_relationship(:role, admin_role, type: :append_and_remove) - |> Ash.update!() + |> Ash.update!(authorize?: false) end) {:error, error} -> @@ -190,10 +196,10 @@ end admin_user_with_role = case Accounts.User |> Ash.Query.filter(email == ^admin_email) - |> Ash.read_one(domain: Mv.Accounts) do + |> Ash.read_one(domain: Mv.Accounts, authorize?: false) do {:ok, user} when not is_nil(user) -> user - |> Ash.load!(:role) + |> Ash.load!(:role, authorize?: false) {:ok, nil} -> raise "Admin user not found after creation/assignment" @@ -202,6 +208,45 @@ admin_user_with_role = raise "Failed to load admin user: #{inspect(error)}" end +# Create system user for systemic operations (email sync, validations, cycle generation) +# This user is used by Mv.Helpers.SystemActor for operations that must always run +# Email is configurable via SYSTEM_ACTOR_EMAIL environment variable +system_user_email = Mv.Helpers.SystemActor.system_user_email() + +case Accounts.User + |> Ash.Query.filter(email == ^system_user_email) + |> Ash.read_one(domain: Mv.Accounts, authorize?: false) do + {:ok, existing_system_user} when not is_nil(existing_system_user) -> + # System user already exists - ensure it has admin role + # Use authorize?: false for bootstrap + existing_system_user + |> Ash.Changeset.for_update(:update, %{}) + |> Ash.Changeset.manage_relationship(:role, admin_role, type: :append_and_remove) + |> Ash.update!(authorize?: false) + + {:ok, nil} -> + # System user doesn't exist - create it with admin role + # SECURITY: System user must NOT be able to log in: + # - No password (hashed_password = nil) - prevents password login + # - No OIDC ID (oidc_id = nil) - prevents OIDC login + # - This user is ONLY for internal system operations via SystemActor + # If either hashed_password or oidc_id is set, the user could potentially log in + # Use authorize?: false for bootstrap - system user creation happens before system actor exists + Accounts.create_user!(%{email: system_user_email}, + upsert?: true, + upsert_identity: :unique_email, + authorize?: false + ) + |> Ash.Changeset.for_update(:update, %{}) + |> Ash.Changeset.manage_relationship(:role, admin_role, type: :append_and_remove) + |> Ash.update!(authorize?: false) + + {:error, error} -> + # Log error but don't fail seeds - SystemActor will fall back to admin user + IO.puts("Warning: Failed to create system user: #{inspect(error)}") + IO.puts("SystemActor will fall back to admin user (#{admin_email})") +end + # Load all membership fee types for assignment # Sort by name to ensure deterministic order all_fee_types = @@ -361,9 +406,20 @@ additional_users = [ created_users = Enum.map(additional_users, fn user_attrs -> - Accounts.create_user!(user_attrs, upsert?: true, upsert_identity: :unique_email) - |> Ash.Changeset.for_update(:admin_set_password, %{password: "testpassword"}) - |> Ash.update!() + # Use admin user as actor for additional user creation (not bootstrap) + user = + Accounts.create_user!(user_attrs, + upsert?: true, + upsert_identity: :unique_email, + actor: admin_user_with_role + ) + |> Ash.Changeset.for_update(:admin_set_password, %{password: "testpassword"}) + |> Ash.update!(actor: admin_user_with_role) + + # Reload user to ensure all fields (including member_id) are loaded + Accounts.User + |> Ash.Query.filter(id == ^user.id) + |> Ash.read_one!(domain: Mv.Accounts, actor: admin_user_with_role) end) # Create members with linked users to demonstrate the 1:1 relationship @@ -413,11 +469,13 @@ Enum.with_index(linked_members) member = if user.member_id == nil do # User is free, create member and link - use upsert to prevent duplicates + # Use authorize?: false for User lookup during relationship management (bootstrap phase) Membership.create_member!( Map.put(member_attrs_without_fee_type, :user, %{id: user.id}), upsert?: true, upsert_identity: :unique_email, - actor: admin_user_with_role + actor: admin_user_with_role, + authorize?: false ) else # User already has a member, just create the member without linking - use upsert to prevent duplicates diff --git a/test/mv/accounts/user_policies_test.exs b/test/mv/accounts/user_policies_test.exs new file mode 100644 index 0000000..bacb19d --- /dev/null +++ b/test/mv/accounts/user_policies_test.exs @@ -0,0 +1,424 @@ +defmodule Mv.Accounts.UserPoliciesTest do + @moduledoc """ + Tests for User resource authorization policies. + + Tests all 4 permission sets (own_data, read_only, normal_user, admin) + and verifies that policies correctly enforce access control based on + user roles and permission sets. + """ + # async: false because we need database commits to be visible across queries + use Mv.DataCase, async: false + + alias Mv.Accounts + alias Mv.Authorization + + require Ash.Query + + # Helper to create a role with a specific permission set + defp create_role_with_permission_set(permission_set_name) do + role_name = "Test Role #{permission_set_name} #{System.unique_integer([:positive])}" + + case Authorization.create_role(%{ + name: role_name, + description: "Test role for #{permission_set_name}", + permission_set_name: permission_set_name + }) do + {:ok, role} -> role + {:error, error} -> raise "Failed to create role: #{inspect(error)}" + end + end + + # Helper to create a user with a specific permission set + # Returns user with role preloaded (required for authorization) + defp create_user_with_permission_set(permission_set_name) do + # Create role with permission set + role = create_role_with_permission_set(permission_set_name) + + # Create user + {:ok, user} = + Accounts.User + |> Ash.Changeset.for_create(:register_with_password, %{ + email: "user#{System.unique_integer([:positive])}@example.com", + password: "testpassword123" + }) + |> Ash.create() + + # Assign role to user + {:ok, user} = + user + |> Ash.Changeset.for_update(:update, %{}) + |> Ash.Changeset.manage_relationship(:role, role, type: :append_and_remove) + |> Ash.update() + + # Reload user with role preloaded (critical for authorization!) + {:ok, user_with_role} = Ash.load(user, :role, domain: Mv.Accounts) + user_with_role + end + + # Helper to create another user (for testing access to other users) + defp create_other_user do + create_user_with_permission_set("own_data") + end + + # Shared test setup for permission sets with scope :own access + defp setup_user_with_own_access(permission_set) do + user = create_user_with_permission_set(permission_set) + other_user = create_other_user() + + # Reload user to ensure role is preloaded + {:ok, user} = Ash.get(Accounts.User, user.id, domain: Mv.Accounts, load: [:role]) + + %{user: user, other_user: other_user} + end + + describe "own_data permission set (Mitglied)" do + setup do + setup_user_with_own_access("own_data") + end + + test "can read own user record", %{user: user} do + {:ok, fetched_user} = + Ash.get(Accounts.User, user.id, actor: user, domain: Mv.Accounts) + + assert fetched_user.id == user.id + end + + test "can update own email", %{user: user} do + new_email = "updated#{System.unique_integer([:positive])}@example.com" + + {: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 "cannot read other users (returns not found due to auto_filter)", %{ + user: user, + other_user: other_user + } do + # Note: With auto_filter policies, when a user tries to read a user that doesn't + # match the filter (id == actor.id), Ash returns NotFound, not Forbidden. + # This is the expected behavior - the filter makes the record "invisible" to the user. + assert_raise Ash.Error.Invalid, fn -> + Ash.get!(Accounts.User, other_user.id, actor: user, domain: Mv.Accounts) + end + end + + test "cannot update other users (returns forbidden)", %{user: user, other_user: other_user} do + assert_raise Ash.Error.Forbidden, fn -> + other_user + |> Ash.Changeset.for_update(:update_user, %{email: "hacked@example.com"}) + |> Ash.update!(actor: user) + end + end + + test "list users returns only own user", %{user: user} do + {:ok, users} = Ash.read(Accounts.User, actor: user, domain: Mv.Accounts) + + # Should only return the own user (scope :own filters) + assert length(users) == 1 + assert hd(users).id == user.id + end + + test "cannot create user (returns forbidden)", %{user: user} do + assert_raise Ash.Error.Forbidden, fn -> + Accounts.User + |> Ash.Changeset.for_create(:create_user, %{ + email: "new#{System.unique_integer([:positive])}@example.com" + }) + |> Ash.create!(actor: user) + end + end + + test "cannot destroy user (returns forbidden)", %{user: user} do + assert_raise Ash.Error.Forbidden, fn -> + Ash.destroy!(user, actor: user) + end + end + end + + describe "read_only permission set (Vorstand/Buchhaltung)" do + setup do + setup_user_with_own_access("read_only") + end + + test "can read own user record", %{user: user} do + {:ok, fetched_user} = + Ash.get(Accounts.User, user.id, actor: user, domain: Mv.Accounts) + + assert fetched_user.id == user.id + end + + test "can update own email", %{user: user} do + new_email = "updated#{System.unique_integer([:positive])}@example.com" + + {: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 "cannot read other users (returns not found due to auto_filter)", %{ + user: user, + other_user: other_user + } do + # Note: With auto_filter policies, when a user tries to read a user that doesn't + # match the filter (id == actor.id), Ash returns NotFound, not Forbidden. + # This is the expected behavior - the filter makes the record "invisible" to the user. + assert_raise Ash.Error.Invalid, fn -> + Ash.get!(Accounts.User, other_user.id, actor: user, domain: Mv.Accounts) + end + end + + test "cannot update other users (returns forbidden)", %{user: user, other_user: other_user} do + assert_raise Ash.Error.Forbidden, fn -> + other_user + |> Ash.Changeset.for_update(:update_user, %{email: "hacked@example.com"}) + |> Ash.update!(actor: user) + end + end + + test "list users returns only own user", %{user: user} do + {:ok, users} = Ash.read(Accounts.User, actor: user, domain: Mv.Accounts) + + # Should only return the own user (scope :own filters) + assert length(users) == 1 + assert hd(users).id == user.id + end + + test "cannot create user (returns forbidden)", %{user: user} do + assert_raise Ash.Error.Forbidden, fn -> + Accounts.User + |> Ash.Changeset.for_create(:create_user, %{ + email: "new#{System.unique_integer([:positive])}@example.com" + }) + |> Ash.create!(actor: user) + end + end + + test "cannot destroy user (returns forbidden)", %{user: user} do + assert_raise Ash.Error.Forbidden, fn -> + Ash.destroy!(user, actor: user) + end + end + end + + describe "normal_user permission set (Kassenwart)" do + setup do + setup_user_with_own_access("normal_user") + end + + test "can read own user record", %{user: user} do + {:ok, fetched_user} = + Ash.get(Accounts.User, user.id, actor: user, domain: Mv.Accounts) + + assert fetched_user.id == user.id + end + + test "can update own email", %{user: user} do + new_email = "updated#{System.unique_integer([:positive])}@example.com" + + {: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 "cannot read other users (returns not found due to auto_filter)", %{ + user: user, + other_user: other_user + } do + # Note: With auto_filter policies, when a user tries to read a user that doesn't + # match the filter (id == actor.id), Ash returns NotFound, not Forbidden. + # This is the expected behavior - the filter makes the record "invisible" to the user. + assert_raise Ash.Error.Invalid, fn -> + Ash.get!(Accounts.User, other_user.id, actor: user, domain: Mv.Accounts) + end + end + + test "cannot update other users (returns forbidden)", %{user: user, other_user: other_user} do + assert_raise Ash.Error.Forbidden, fn -> + other_user + |> Ash.Changeset.for_update(:update_user, %{email: "hacked@example.com"}) + |> Ash.update!(actor: user) + end + end + + test "list users returns only own user", %{user: user} do + {:ok, users} = Ash.read(Accounts.User, actor: user, domain: Mv.Accounts) + + # Should only return the own user (scope :own filters) + assert length(users) == 1 + assert hd(users).id == user.id + end + + test "cannot create user (returns forbidden)", %{user: user} do + assert_raise Ash.Error.Forbidden, fn -> + Accounts.User + |> Ash.Changeset.for_create(:create_user, %{ + email: "new#{System.unique_integer([:positive])}@example.com" + }) + |> Ash.create!(actor: user) + end + end + + test "cannot destroy user (returns forbidden)", %{user: user} do + assert_raise Ash.Error.Forbidden, fn -> + Ash.destroy!(user, actor: user) + end + end + end + + describe "admin permission set" do + setup do + user = create_user_with_permission_set("admin") + other_user = create_other_user() + + # Reload user to ensure role is preloaded + {:ok, user} = Ash.get(Accounts.User, user.id, domain: Mv.Accounts, load: [:role]) + + %{user: user, other_user: other_user} + end + + test "can read all users", %{user: user, other_user: other_user} do + {:ok, users} = Ash.read(Accounts.User, actor: user, domain: Mv.Accounts) + + # Should return all users (scope :all) + user_ids = Enum.map(users, & &1.id) + assert user.id in user_ids + assert other_user.id in user_ids + end + + test "can read other users", %{user: user, other_user: other_user} do + {:ok, fetched_user} = + Ash.get(Accounts.User, other_user.id, actor: user, domain: Mv.Accounts) + + assert fetched_user.id == other_user.id + end + + test "can update other users", %{user: user, other_user: other_user} do + new_email = "adminupdated#{System.unique_integer([:positive])}@example.com" + + {:ok, updated_user} = + other_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 "can create user", %{user: user} do + {:ok, new_user} = + Accounts.User + |> Ash.Changeset.for_create(:create_user, %{ + email: "new#{System.unique_integer([:positive])}@example.com" + }) + |> Ash.create(actor: user) + + assert new_user.email + end + + test "can destroy user", %{user: user, other_user: other_user} do + :ok = Ash.destroy(other_user, actor: user) + + # Verify user is deleted + assert {:error, _} = Ash.get(Accounts.User, other_user.id, domain: Mv.Accounts) + end + end + + describe "AshAuthentication bypass" do + test "register_with_password works without actor" do + # Registration should work without actor (AshAuthentication bypass) + {:ok, user} = + Accounts.User + |> Ash.Changeset.for_create(:register_with_password, %{ + email: "register#{System.unique_integer([:positive])}@example.com", + password: "testpassword123" + }) + |> Ash.create() + + assert user.email + end + + test "register_with_rauthy works with OIDC user_info" do + # OIDC registration should work (AshAuthentication bypass) + user_info = %{ + "sub" => "oidc_sub_#{System.unique_integer([:positive])}", + "email" => "oidc#{System.unique_integer([:positive])}@example.com" + } + + oauth_tokens = %{access_token: "token", refresh_token: "refresh"} + + {:ok, user} = + Accounts.User + |> Ash.Changeset.for_create(:register_with_rauthy, %{ + user_info: user_info, + oauth_tokens: oauth_tokens + }) + |> Ash.create() + + assert user.email + assert user.oidc_id == user_info["sub"] + end + + test "sign_in_with_rauthy works with OIDC user_info" do + # First create a user with OIDC ID + user_info_create = %{ + "sub" => "oidc_sub_#{System.unique_integer([:positive])}", + "email" => "oidc#{System.unique_integer([:positive])}@example.com" + } + + oauth_tokens = %{access_token: "token", refresh_token: "refresh"} + + {:ok, user} = + Accounts.User + |> Ash.Changeset.for_create(:register_with_rauthy, %{ + user_info: user_info_create, + oauth_tokens: oauth_tokens + }) + |> Ash.create() + + # Now test sign_in_with_rauthy (should work via AshAuthentication bypass) + {:ok, signed_in_user} = + Accounts.User + |> Ash.Query.for_read(:sign_in_with_rauthy, %{ + user_info: user_info_create, + oauth_tokens: oauth_tokens + }) + |> Ash.read_one() + + assert signed_in_user.id == user.id + end + + # NOTE: get_by_subject is tested implicitly via AshAuthentication's JWT flow. + # Direct testing via Ash.Query.for_read(:get_by_subject) doesn't properly + # simulate the AshAuthentication context and would require mocking JWT tokens. + # The AshAuthentication bypass policy ensures this action works correctly + # when called through the proper authentication flow (sign_in, token refresh, etc.). + # Integration tests that use actual JWT tokens cover this functionality. + end + + describe "test environment bypass (NoActor)" do + test "operations without actor are allowed in test environment" do + # In test environment, NoActor check should allow operations + {:ok, user} = + Accounts.User + |> Ash.Changeset.for_create(:create_user, %{ + email: "noactor#{System.unique_integer([:positive])}@example.com" + }) + |> Ash.create() + + assert user.email + + # Read should also work + {:ok, fetched_user} = Ash.get(Accounts.User, user.id, domain: Mv.Accounts) + assert fetched_user.id == user.id + end + end +end diff --git a/test/mv/authorization/actor_test.exs b/test/mv/authorization/actor_test.exs new file mode 100644 index 0000000..e542301 --- /dev/null +++ b/test/mv/authorization/actor_test.exs @@ -0,0 +1,84 @@ +defmodule Mv.Authorization.ActorTest do + @moduledoc """ + Tests for the Actor helper module. + """ + use Mv.DataCase, async: false + + alias Mv.Accounts + alias Mv.Authorization.Actor + + describe "ensure_loaded/1" do + test "returns nil when actor is nil" do + assert Actor.ensure_loaded(nil) == nil + end + + test "returns actor as-is when role is already loaded" do + # Create user with role + {:ok, user} = + Accounts.User + |> Ash.Changeset.for_create(:register_with_password, %{ + email: "test#{System.unique_integer([:positive])}@example.com", + password: "testpassword123" + }) + |> Ash.create() + + # Load role + {:ok, user_with_role} = Ash.load(user, :role, domain: Mv.Accounts) + + # Should return as-is (no additional load) + result = Actor.ensure_loaded(user_with_role) + assert result.id == user.id + assert result.role != %Ash.NotLoaded{} + end + + test "loads role when it's NotLoaded" do + # Create a role first + {:ok, role} = + Mv.Authorization.Role + |> Ash.Changeset.for_create(:create_role, %{ + name: "Test Role #{System.unique_integer([:positive])}", + description: "Test role", + permission_set_name: "own_data" + }) + |> Ash.create() + + # Create user with role + {:ok, user} = + Accounts.User + |> Ash.Changeset.for_create(:register_with_password, %{ + email: "test#{System.unique_integer([:positive])}@example.com", + password: "testpassword123" + }) + |> Ash.create() + + # Assign role to user + {:ok, user_with_role} = + user + |> Ash.Changeset.for_update(:update, %{}) + |> Ash.Changeset.manage_relationship(:role, role, type: :append_and_remove) + |> Ash.update() + + # Fetch user again WITHOUT loading role (simulates "role not preloaded" scenario) + {:ok, user_without_role_loaded} = + Ash.get(Accounts.User, user_with_role.id, domain: Mv.Accounts) + + # User has role as NotLoaded (relationship not preloaded) + assert match?(%Ash.NotLoaded{}, user_without_role_loaded.role) + + # ensure_loaded should load it + result = Actor.ensure_loaded(user_without_role_loaded) + assert result.id == user.id + refute match?(%Ash.NotLoaded{}, result.role) + assert result.role.id == role.id + end + + test "returns non-User actors as-is (no-op)" do + # Create a plain map (not Mv.Accounts.User) + other_actor = %{id: "fake", role: %Ash.NotLoaded{field: :role}} + + # Should return as-is (pattern match doesn't apply to non-User) + result = Actor.ensure_loaded(other_actor) + assert result == other_actor + end + end +end diff --git a/test/mv/authorization/checks/has_permission_test.exs b/test/mv/authorization/checks/has_permission_test.exs index f577d03..4e2dcea 100644 --- a/test/mv/authorization/checks/has_permission_test.exs +++ b/test/mv/authorization/checks/has_permission_test.exs @@ -76,8 +76,10 @@ defmodule Mv.Authorization.Checks.HasPermissionTest do {:ok, result} = HasPermission.strict_check(own_data_user, authorizer, []) - # Should return :unknown for :own scope (needs filter) - assert result == :unknown + # Should return false for :own scope without record + # This prevents bypassing expr-based filters in bypass policies + # The actual filtering is done via bypass policies with expr(id == ^actor(:id)) + assert result == false end end @@ -104,14 +106,16 @@ defmodule Mv.Authorization.Checks.HasPermissionTest do end describe "strict_check/3 - Scope :own" do - test "actor with scope :own returns :unknown (needs filter)" do + test "actor with scope :own returns false (needs bypass policy with expr filter)" do user = create_actor("user-123", "own_data") authorizer = create_authorizer(Mv.Accounts.User, :read) {:ok, result} = HasPermission.strict_check(user, authorizer, []) - # Should return :unknown for :own scope (needs filter via auto_filter) - assert result == :unknown + # Should return false for :own scope without record + # This prevents bypassing expr-based filters in bypass policies + # The actual filtering is done via bypass policies with expr(id == ^actor(:id)) + assert result == false end end @@ -270,4 +274,44 @@ defmodule Mv.Authorization.Checks.HasPermissionTest do end end end + + describe "strict_check/3 - Role Loading Fallback" do + test "returns false if role is NotLoaded and cannot be loaded" do + # Create actor with NotLoaded role + # In real scenario, ensure_role_loaded would attempt to load via Ash.load + # For this test, we use a simple map to verify the pattern matching works + actor = %{ + id: "user-123", + role: %Ash.NotLoaded{} + } + + authorizer = create_authorizer(Mv.Accounts.User, :read) + + # Should handle NotLoaded pattern and return false + # (In real scenario, ensure_role_loaded would attempt to load, but for this test + # we just verify the pattern matching works correctly) + {:ok, result} = HasPermission.strict_check(actor, authorizer, []) + assert result == false + end + + test "returns false if role is nil" do + actor = %{ + id: "user-123", + role: nil + } + + authorizer = create_authorizer(Mv.Accounts.User, :read) + + {:ok, result} = HasPermission.strict_check(actor, authorizer, []) + assert result == false + end + + test "works correctly when role is already loaded" do + actor = create_actor("user-123", "admin") + authorizer = create_authorizer(Mv.Accounts.User, :read) + + {:ok, result} = HasPermission.strict_check(actor, authorizer, []) + assert result == true + end + end end diff --git a/test/mv/authorization/checks/no_actor_test.exs b/test/mv/authorization/checks/no_actor_test.exs new file mode 100644 index 0000000..35205a6 --- /dev/null +++ b/test/mv/authorization/checks/no_actor_test.exs @@ -0,0 +1,52 @@ +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 diff --git a/test/mv/helpers/system_actor_test.exs b/test/mv/helpers/system_actor_test.exs new file mode 100644 index 0000000..751f5c5 --- /dev/null +++ b/test/mv/helpers/system_actor_test.exs @@ -0,0 +1,362 @@ +defmodule Mv.Helpers.SystemActorTest do + @moduledoc """ + Tests for the SystemActor helper module. + """ + use Mv.DataCase, async: false + + alias Mv.Helpers.SystemActor + alias Mv.Authorization + alias Mv.Accounts + + require Ash.Query + + # Helper function to ensure admin role exists + defp ensure_admin_role do + case Authorization.list_roles() do + {:ok, roles} -> + case Enum.find(roles, &(&1.permission_set_name == "admin")) do + nil -> + {:ok, role} = + Authorization.create_role(%{ + name: "Admin", + description: "Administrator with full access", + permission_set_name: "admin" + }) + + role + + role -> + role + end + + _ -> + {:ok, role} = + Authorization.create_role(%{ + name: "Admin", + description: "Administrator with full access", + permission_set_name: "admin" + }) + + role + end + end + + # Helper function to ensure system user exists with admin role + defp ensure_system_user(admin_role) do + case Accounts.User + |> Ash.Query.filter(email == ^"system@mila.local") + |> Ash.read_one(domain: Mv.Accounts) do + {:ok, user} when not is_nil(user) -> + user + |> Ash.Changeset.for_update(:update, %{}) + |> Ash.Changeset.manage_relationship(:role, admin_role, type: :append_and_remove) + |> Ash.update!() + |> Ash.load!(:role, domain: Mv.Accounts) + + _ -> + Accounts.create_user!(%{email: "system@mila.local"}, + upsert?: true, + upsert_identity: :unique_email + ) + |> Ash.Changeset.for_update(:update, %{}) + |> Ash.Changeset.manage_relationship(:role, admin_role, type: :append_and_remove) + |> Ash.update!() + |> Ash.load!(:role, domain: Mv.Accounts) + end + end + + # Helper function to ensure admin user exists with admin role + defp ensure_admin_user(admin_role) do + admin_email = System.get_env("ADMIN_EMAIL") || "admin@localhost" + + case Accounts.User + |> Ash.Query.filter(email == ^admin_email) + |> Ash.read_one(domain: Mv.Accounts) do + {:ok, user} when not is_nil(user) -> + user + |> Ash.Changeset.for_update(:update, %{}) + |> Ash.Changeset.manage_relationship(:role, admin_role, type: :append_and_remove) + |> Ash.update!() + |> Ash.load!(:role, domain: Mv.Accounts) + + _ -> + Accounts.create_user!(%{email: admin_email}, + upsert?: true, + upsert_identity: :unique_email + ) + |> Ash.Changeset.for_update(:update, %{}) + |> Ash.Changeset.manage_relationship(:role, admin_role, type: :append_and_remove) + |> Ash.update!() + |> Ash.load!(:role, domain: Mv.Accounts) + end + end + + setup do + admin_role = ensure_admin_role() + system_user = ensure_system_user(admin_role) + admin_user = ensure_admin_user(admin_role) + + # Invalidate cache to ensure fresh load + SystemActor.invalidate_cache() + + %{admin_role: admin_role, system_user: system_user, admin_user: admin_user} + end + + describe "get_system_actor/0" do + test "returns system user with admin role", %{system_user: _system_user} do + system_actor = SystemActor.get_system_actor() + + assert %Mv.Accounts.User{} = system_actor + assert to_string(system_actor.email) == "system@mila.local" + assert system_actor.role != nil + assert system_actor.role.permission_set_name == "admin" + end + + test "falls back to admin user if system user doesn't exist", %{admin_user: _admin_user} do + # Delete system user if it exists + case Accounts.User + |> Ash.Query.filter(email == ^"system@mila.local") + |> Ash.read_one(domain: Mv.Accounts) do + {:ok, user} when not is_nil(user) -> + Ash.destroy!(user, domain: Mv.Accounts) + + _ -> + :ok + end + + # Invalidate cache to force reload + SystemActor.invalidate_cache() + + # Should fall back to admin user + system_actor = SystemActor.get_system_actor() + + assert %Mv.Accounts.User{} = system_actor + assert system_actor.role != nil + assert system_actor.role.permission_set_name == "admin" + # Should be admin user, not system user + assert to_string(system_actor.email) != "system@mila.local" + end + + test "caches system actor for performance" do + # First call + actor1 = SystemActor.get_system_actor() + + # Second call should return cached actor (same struct) + actor2 = SystemActor.get_system_actor() + + # Should be the same struct (cached) + assert actor1.id == actor2.id + end + + test "creates system user in test environment if none exists", %{admin_role: _admin_role} do + # In test environment, system actor should auto-create if missing + # Delete all users to test auto-creation + case Accounts.User + |> Ash.Query.filter(email == ^"system@mila.local") + |> Ash.read_one(domain: Mv.Accounts) do + {:ok, user} when not is_nil(user) -> + Ash.destroy!(user, domain: Mv.Accounts) + + _ -> + :ok + end + + admin_email = System.get_env("ADMIN_EMAIL") || "admin@localhost" + + case Accounts.User + |> Ash.Query.filter(email == ^admin_email) + |> Ash.read_one(domain: Mv.Accounts) do + {:ok, user} when not is_nil(user) -> + Ash.destroy!(user, domain: Mv.Accounts) + + _ -> + :ok + end + + # Invalidate cache + SystemActor.invalidate_cache() + + # Should auto-create system user in test environment + system_actor = SystemActor.get_system_actor() + + assert %Mv.Accounts.User{} = system_actor + assert to_string(system_actor.email) == "system@mila.local" + assert system_actor.role != nil + assert system_actor.role.permission_set_name == "admin" + end + end + + describe "invalidate_cache/0" do + test "forces reload of system actor on next call" do + # Get initial actor + actor1 = SystemActor.get_system_actor() + + # Invalidate cache + :ok = SystemActor.invalidate_cache() + + # Next call should reload (but should be same user) + actor2 = SystemActor.get_system_actor() + + # Should be same user (same ID) + assert actor1.id == actor2.id + end + end + + describe "get_system_actor_result/0" do + test "returns ok tuple with system actor" do + assert {:ok, actor} = SystemActor.get_system_actor_result() + assert %Mv.Accounts.User{} = actor + assert actor.role.permission_set_name == "admin" + end + + test "returns error tuple when system actor cannot be loaded" do + # Delete all users to force error + case Accounts.User + |> Ash.Query.filter(email == ^"system@mila.local") + |> Ash.read_one(domain: Mv.Accounts) do + {:ok, user} when not is_nil(user) -> + Ash.destroy!(user, domain: Mv.Accounts) + + _ -> + :ok + end + + admin_email = System.get_env("ADMIN_EMAIL") || "admin@localhost" + + case Accounts.User + |> Ash.Query.filter(email == ^admin_email) + |> Ash.read_one(domain: Mv.Accounts) do + {:ok, user} when not is_nil(user) -> + Ash.destroy!(user, domain: Mv.Accounts) + + _ -> + :ok + end + + SystemActor.invalidate_cache() + + # In test environment, it should auto-create, so this should succeed + # But if it fails, we should get an error tuple + result = SystemActor.get_system_actor_result() + + # Should either succeed (auto-created) or return error + assert match?({:ok, _}, result) or match?({:error, _}, result) + end + end + + describe "system_user_email/0" do + test "returns the system user email address" do + assert SystemActor.system_user_email() == "system@mila.local" + end + end + + describe "edge cases" do + test "raises error if admin user has no role", %{admin_user: admin_user} do + # Remove role from admin user + admin_user + |> Ash.Changeset.for_update(:update, %{}) + |> Ash.Changeset.manage_relationship(:role, nil, type: :append_and_remove) + |> Ash.update!() + + # Delete system user to force fallback + case Accounts.User + |> Ash.Query.filter(email == ^"system@mila.local") + |> Ash.read_one(domain: Mv.Accounts) do + {:ok, user} when not is_nil(user) -> + Ash.destroy!(user, domain: Mv.Accounts) + + _ -> + :ok + end + + SystemActor.invalidate_cache() + + # Should raise error because admin user has no role + assert_raise RuntimeError, ~r/System actor must have a role assigned/, fn -> + SystemActor.get_system_actor() + end + end + + test "handles concurrent calls without race conditions" do + # Delete system user and admin user to force creation + case Accounts.User + |> Ash.Query.filter(email == ^"system@mila.local") + |> Ash.read_one(domain: Mv.Accounts) do + {:ok, user} when not is_nil(user) -> + Ash.destroy!(user, domain: Mv.Accounts) + + _ -> + :ok + end + + admin_email = System.get_env("ADMIN_EMAIL") || "admin@localhost" + + case Accounts.User + |> Ash.Query.filter(email == ^admin_email) + |> Ash.read_one(domain: Mv.Accounts) do + {:ok, user} when not is_nil(user) -> + Ash.destroy!(user, domain: Mv.Accounts) + + _ -> + :ok + end + + SystemActor.invalidate_cache() + + # Call get_system_actor concurrently from multiple processes + tasks = + for _ <- 1..10 do + Task.async(fn -> SystemActor.get_system_actor() end) + end + + results = Task.await_many(tasks, :infinity) + + # All should succeed and return the same actor + assert length(results) == 10 + assert Enum.all?(results, &(&1.role.permission_set_name == "admin")) + assert Enum.all?(results, fn actor -> to_string(actor.email) == "system@mila.local" end) + + # All should have the same ID (same user) + ids = Enum.map(results, & &1.id) + assert Enum.uniq(ids) |> length() == 1 + end + + test "raises error if system user has wrong role", %{system_user: system_user} do + # Create a non-admin role (using read_only as it's a valid permission set) + {:ok, read_only_role} = + Authorization.create_role(%{ + name: "Read Only Role", + description: "Read-only access", + permission_set_name: "read_only" + }) + + # Assign wrong role to system user + system_user + |> Ash.Changeset.for_update(:update, %{}) + |> Ash.Changeset.manage_relationship(:role, read_only_role, type: :append_and_remove) + |> Ash.update!() + + SystemActor.invalidate_cache() + + # Should raise error because system user doesn't have admin role + assert_raise RuntimeError, ~r/System actor must have admin role/, fn -> + SystemActor.get_system_actor() + end + end + + test "raises error if system user has no role", %{system_user: system_user} do + # Remove role from system user + system_user + |> Ash.Changeset.for_update(:update, %{}) + |> Ash.Changeset.manage_relationship(:role, nil, type: :append_and_remove) + |> Ash.update!() + + SystemActor.invalidate_cache() + + # Should raise error because system user has no role + assert_raise RuntimeError, ~r/System actor must have a role assigned/, fn -> + SystemActor.get_system_actor() + end + end + end +end