From 8d783276d0a32167020ffd8f8a63e81f0408cef3 Mon Sep 17 00:00:00 2001 From: Moritz Date: Mon, 15 Jun 2026 21:53:36 +0200 Subject: [PATCH] docs(roles): condense roles/permissions/auth docs and align with the code --- docs/admin-bootstrap-and-oidc-role-sync.md | 2 +- docs/oidc-account-linking.md | 45 +- docs/page-permission-route-coverage.md | 65 +- docs/policy-bypass-vs-haspermission.md | 287 +-- docs/roles-and-permissions-architecture.md | 1664 ++-------------- ...les-and-permissions-implementation-plan.md | 1684 +---------------- docs/roles-and-permissions-overview.md | 168 +- ...esource-policies-implementation-summary.md | 269 --- 8 files changed, 348 insertions(+), 3836 deletions(-) delete mode 100644 docs/user-resource-policies-implementation-summary.md diff --git a/docs/admin-bootstrap-and-oidc-role-sync.md b/docs/admin-bootstrap-and-oidc-role-sync.md index 5413f91..ee78069 100644 --- a/docs/admin-bootstrap-and-oidc-role-sync.md +++ b/docs/admin-bootstrap-and-oidc-role-sync.md @@ -26,7 +26,7 @@ ### Seeds (Dev/Test) -- priv/repo/seeds.exs – Uses ADMIN_PASSWORD or ADMIN_PASSWORD_FILE when set; otherwise fallback "testpassword" only in dev/test. +- priv/repo/seeds_bootstrap.exs – Uses ADMIN_PASSWORD or ADMIN_PASSWORD_FILE when set; otherwise fallback "testpassword" only in dev/test. ## OIDC Role Sync (Part B) diff --git a/docs/oidc-account-linking.md b/docs/oidc-account-linking.md index 570d4e8..48e82d8 100644 --- a/docs/oidc-account-linking.md +++ b/docs/oidc-account-linking.md @@ -102,12 +102,12 @@ Interactive UI for password verification and account linking. **Changes**: -- `lib/mv_web/locale_controller.ex`: Sets locale cookie with `http_only` and `secure` flags +- `MvWeb.LocaleController`: Sets locale cookie with `http_only` and a config-driven `secure` flag - `lib/mv_web/router.ex`: Reads locale from cookie if session empty **Security Features**: - `http_only: true` - Cookie not accessible via JavaScript (XSS protection) -- `secure: true` - Cookie only transmitted over HTTPS in production +- `secure: Application.get_env(:mv, :use_secure_cookies, false)` - the `secure` flag is config-driven (defaults to `false`; enabled in production) so the cookie is only transmitted over HTTPS in production - `same_site: "Lax"` - CSRF protection ## Security Considerations @@ -139,47 +139,6 @@ Interactive UI for password verification and account linking. - `Logger.warning` for failed authentication attempts - `Logger.error` for system errors -## Usage Examples - -### Scenario 1: New OIDC User - -```elixir -# User signs in with OIDC for the first time -# → New user created with oidc_id -``` - -### Scenario 2: Existing OIDC User - -```elixir -# User with oidc_id signs in via OIDC -# → Matched by oidc_id, email updated if changed -``` - -### Scenario 3: Password User + OIDC Login - -```elixir -# User with password account tries OIDC login -# → PasswordVerificationRequired raised -# → Redirected to /auth/link-oidc-account -# → User enters password -# → Password verified and logged -# → oidc_id linked to account -# → Successful linking logged -# → Redirected to complete OIDC login -``` - -### Scenario 4: Passwordless User + OIDC Login - -```elixir -# User without password (invited user) tries OIDC login -# → PasswordVerificationRequired raised -# → Redirected to /auth/link-oidc-account -# → System detects passwordless user -# → oidc_id automatically linked (no password prompt) -# → Auto-linking logged -# → Redirected to complete OIDC login -``` - ## API ### Custom Actions diff --git a/docs/page-permission-route-coverage.md b/docs/page-permission-route-coverage.md index 6571a39..1302594 100644 --- a/docs/page-permission-route-coverage.md +++ b/docs/page-permission-route-coverage.md @@ -19,9 +19,8 @@ This document lists all protected routes, which permission set may access them, | `/users/:id/show/edit` | ✓ (own only) | ✓ (own only) | ✓ (own only) | ✓ | | `/settings` | ✗ | ✗ | ✗ | ✓ | | `/membership_fee_settings` | ✗ | ✗ | ✗ | ✓ | -| `/membership_fee_types` | ✗ | ✗ | ✗ | ✓ | -| `/membership_fee_types/new` | ✗ | ✗ | ✗ | ✓ | -| `/membership_fee_types/:id/edit` | ✗ | ✗ | ✗ | ✓ | +| `/membership_fee_settings/new_fee_type` | ✗ | ✗ | ✗ | ✓ | +| `/membership_fee_settings/:id/edit_fee_type` | ✗ | ✗ | ✗ | ✓ | | `/groups` | ✗ | ✓ | ✓ | ✓ | | `/groups/new` | ✗ | ✗ | ✗ | ✓ | | `/groups/:slug` | ✗ | ✓ | ✓ | ✓ | @@ -31,10 +30,18 @@ This document lists all protected routes, which permission set may access them, | `/admin/roles/new` | ✗ | ✗ | ✗ | ✓ | | `/admin/roles/:id` | ✗ | ✗ | ✗ | ✓ | | `/admin/roles/:id/edit` | ✗ | ✗ | ✗ | ✓ | -| `/join_requests` (Step 2) | ✗ | ✗ | ✓ | ✓ | -| `/join_requests/:id` (Step 2) | ✗ | ✗ | ✓ | ✓ | +| `/join_requests` | ✗ | ✗ | ✓ | ✓ | +| `/join_requests/:id` | ✗ | ✗ | ✓ | ✓ | +| `/admin/datafields` | ✗ | ✗ | ✗ | ✓ | +| `/admin/import` | ✗ | ✗ | ✗ | ✓ | +| `/admin/import/template/en` | ✗ | ✗ | ✗ | ✓ | +| `/admin/import/template/de` | ✗ | ✗ | ✗ | ✓ | +| `/members/export.csv` | ✗ | ✓ | ✓ | ✓ | +| `/members/export.pdf` | ✗ | ✗ | ✗ | ✓ | -**Note:** Permission sets define `/custom_field_values` and related paths, but there are no such routes in the router; those entries are for future use. Step 2 (Approval UI) adds `/join_requests` and `/join_requests/:id` for normal_user and admin; routes and permission set entries are not yet implemented; tests exist in `check_page_permission_test.exs` (describe "join_requests routes" and integration blocks). +**Note:** Permission sets define `/custom_field_values` and related paths, but there are no such routes in the router; those entries are for future use. The Approval UI routes `/join_requests` and `/join_requests/:id` are implemented and routed: `normal_user` lists them explicitly in its permission set, and `admin` reaches them through the `*` wildcard. + +**Note on admin-only routes:** `/admin/datafields`, `/admin/import`, `/admin/import/template/en`, `/admin/import/template/de`, and `/members/export.pdf` are not listed explicitly in any permission set; only `admin` can reach them, via the `*` wildcard. `/members/export.csv` is additionally granted explicitly to `read_only` and `normal_user`. ## Public Paths (no permission check) @@ -46,50 +53,12 @@ The join confirmation route `GET /confirm_join/:token` is public (matched by `/c ## Test Coverage -**File:** `test/mv_web/plugs/check_page_permission_test.exs` +**File:** `test/mv_web/plugs/check_page_permission_test.exs` covers both unit tests (plug called directly with a mock conn) and full-router integration tests. The route→permission-set matrix above is the source of truth; each permission set (own_data/Mitglied, read_only, normal_user/Kassenwart, admin) is exercised there. Allowed routes return 200; denied routes return 302 → `/users/:id`. `GET /` redirects own_data to its profile. Unauthenticated access is denied and redirected to `/sign-in`; public paths (`/auth/sign-in`, `/register`) are allowed. Error cases (no role, invalid permission_set_name) deny. -### Unit tests (plug called directly with mock conn) +Two coverage notes: -- Static: own_data denied `/members`; read_only allowed `/members`; flash on denial. -- Dynamic: read_only allowed `/members/123`; normal_user allowed `/members/456/edit`; read_only denied `/members/123/edit`. -- read_only / normal_user: denied `/admin/roles`; read_only denied `/members/new`. -- Wildcard: admin allowed `/admin/roles`, `/members/999/edit`. -- Unauthenticated: nil user denied, redirect `/sign-in`. -- Public: unauthenticated allowed `/auth/sign-in`, `/register`. -- Error: no role, invalid permission_set_name → denied. -- **Join requests (Step 2):** normal_user and admin allowed `/join_requests`, `/join_requests/:id`; read_only and own_data denied. Tests fail (red) until routes and permission set are added. - -### Integration tests (full router, Mitglied = own_data) - -**Denied (Mitglied gets 302 → `/users/:id`):** - -- `/members`, `/members/new`, `/users`, `/users/new`, `/settings`, `/membership_fee_settings`, `/membership_fee_types`, `/membership_fee_types/new`, `/groups`, `/groups/new`, `/admin/roles`, `/admin/roles/new` -- `/members/:id/edit`, `/members/:id/show/edit`, `/users/:id` (other user), `/users/:id/edit` (other), `/users/:id/show/edit` (other), `/membership_fee_types/:id/edit`, `/groups/:slug`, `/admin/roles/:id`, `/admin/roles/:id/edit` - -**Allowed (Mitglied gets 200):** - -- `/users/:id` (own profile), `/users/:id/edit`, `/users/:id/show/edit` -- `/members/:id`, `/members/:id/edit`, `/members/:id/show/edit` for linked member (plug unit tests; full-router tests for linked member skipped: session/LiveView constraints) - -**Root:** `GET /` redirects Mitglied to profile (root not allowed for own_data). - -All protected routes above are either covered by integration “denied” tests for Mitglied or by unit tests for the relevant permission set. - -### Integration tests (full router, read_only = Vorstand/Buchhaltung) - -**Allowed (200):** `/`, `/members`, `/members/:id`, `/users/:id` (own profile), `/users/:id/edit`, `/users/:id/show/edit`, `/groups`, `/groups/:slug`. - -**Denied (302 → `/users/:id`):** `/members/new`, `/members/:id/edit`, `/members/:id/show/edit`, `/users`, `/users/new`, `/users/:id` (other user), `/settings`, `/membership_fee_settings`, `/membership_fee_types`, `/groups/new`, `/groups/:slug/edit`, `/admin/roles`, `/admin/roles/:id`. - -### Integration tests (full router, normal_user = Kassenwart) - -**Allowed (200):** `/`, `/members`, `/members/new`, `/members/:id`, `/members/:id/edit`, `/members/:id/show/edit`, `/users/:id` (own profile), `/users/:id/edit`, `/users/:id/show/edit`, `/groups`, `/groups/:slug`. - -**Denied (302 → `/users/:id`):** `/users`, `/users/new`, `/users/:id` (other user), `/settings`, `/membership_fee_settings`, `/membership_fee_types`, `/groups/new`, `/groups/:slug/edit`, `/admin/roles`, `/admin/roles/:id`. - -### Integration tests (full router, admin) - -**Allowed (200):** All protected routes (sample covered: `/`, `/members`, `/users`, `/settings`, `/membership_fee_settings`, `/admin/roles`, `/members/:id`, `/admin/roles/:id`, `/groups/:slug`). +- **Linked-member routes** (`/members/:id*` for own_data) are covered by plug unit tests; full-router integration tests for the linked member are skipped due to session/LiveView constraints. +- **Join requests:** normal_user and admin are allowed `/join_requests` and `/join_requests/:id` (normal_user via its explicit permission-set pages, admin via the `*` wildcard); read_only and own_data are denied. ## Plug behaviour: reserved segments diff --git a/docs/policy-bypass-vs-haspermission.md b/docs/policy-bypass-vs-haspermission.md index 31bb737..124d623 100644 --- a/docs/policy-bypass-vs-haspermission.md +++ b/docs/policy-bypass-vs-haspermission.md @@ -1,69 +1,56 @@ # Policy Pattern: Bypass vs. HasPermission -**Date:** 2026-01-22 -**Status:** Implemented and Tested +**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**: +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 +1. **Bypass with `expr()` for READ** — handles list queries via `auto_filter`. +2. **HasPermission for UPDATE/CREATE/DESTROY** — uses scope from PermissionSets when a record is present. -This pattern ensures that the scope concept in PermissionSets is actually used and not redundant. - ---- +This ensures the scope concept in PermissionSets is actually used and not redundant. ## The Problem -### Initial Assumption (INCORRECT) +The initial assumption was that `HasPermission` returning `{:filter, expr(...)}` would automatically trigger Ash's `auto_filter` for list queries. It does not: -> "No separate Own Credentials Bypass needed, as all permission sets already have User read/update :own. HasPermission with scope :own handles this correctly." +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. List queries fail with empty results. -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) +# Ash.read(User, actor: user) +# Expected: [user] (filtered to own record) +# Actual: [] (empty list) ``` ---- - ## The Solution -### Pattern: Bypass for READ, HasPermission for UPDATE - -**User Resource Example:** +Bypass for READ, HasPermission for everything else: ```elixir policies do - # Bypass for READ (handles list queries via auto_filter) + # AshAuthentication (registration/login) + bypass AshAuthentication.Checks.AshAuthenticationInteraction do + authorize_if always() + end + + # 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) + + # HasPermission — scope from PermissionSets, used when a record is present policy action_type([:read, :create, :update, :destroy]) do description "Check permissions from user's role and permission set" authorize_if Mv.Authorization.Checks.HasPermission @@ -71,260 +58,100 @@ policies do end ``` -**Why This Works:** +Why it 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 | +| Operation | Record? | Method | Result | +|-----------|---------|--------|--------| +| READ (list) | No | `bypass` + `expr()` | Ash compiles expr to SQL WHERE → filtered list | +| READ (single) | Yes | `bypass` + `expr()` | Ash evaluates expr → true/false | +| UPDATE / CREATE / DESTROY | Yes (changeset) | `HasPermission` + scope | `strict_check` evaluates record → authorized | -**Important: UPDATE Strategy** +### UPDATE is controlled by PermissionSets, not hardcoded -UPDATE is **NOT** a hardcoded bypass. It is controlled by **PermissionSets**: +UPDATE is **not** a hardcoded bypass. All permission sets (`:own_data`, `:read_only`, `:normal_user`, `:admin`) explicitly grant `User.update :own`; `HasPermission` evaluates `scope :own` when a changeset with a record is present. Removing `User.update :own` from a set would remove credential-update ability for that set — intentional. -- 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 +**Decision: `read_only` grants `User.update :own`** even though it is "read-only" for member data, so password changes work while member data stays read-only. -**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. +### No explicit `forbid_if always()` ---- +We do **not** add a trailing `forbid_if always()`. Ash fails closed implicitly — it forbids when no policy authorizes. An explicit terminal forbid breaks tests because it forbids valid operations that earlier policies should authorize. ## 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:** +`scope :own` is used for operations where a record is present (UPDATE/CREATE/DESTROY), even though the bypass handles READ: ```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 ✅ +%{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! -``` - ---- +Proven by `test/mv/accounts/user_policies_test.exs` ("can update own email"): the update succeeds via `HasPermission` with `scope :own` (not via bypass). ## Consistency Across Resources +Both User and Member follow the same shape — bypass for READ, HasPermission for UPDATE/CREATE/DESTROY — differing only in the actor key and scope: + ### 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 +PermissionSets: `own_data` / `read_only` / `normal_user` use `scope :own` for read/update; `admin` uses `scope :all`. ### 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 - ---- +PermissionSets: `own_data` uses `scope :linked` for read/update; `read_only` uses `scope :all` for read (no update); `normal_user` and `admin` use `scope :all`. ## Technical Deep Dive -### Why Does `expr()` in Bypass Work? +### Why `expr()` in bypass works -**Ash treats `expr()` natively in two contexts:** +Ash treats `expr()` natively in both contexts: -1. **strict_check** (single record): - - Ash evaluates the expression against the record - - Returns true/false based on match +- **strict_check** (single record): evaluates the expression against the record → true/false. +- **auto_filter** (list queries): compiles the expression to a SQL WHERE clause applied in the DB query. -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] ✅ +# Ash.read(User, actor: user) +# Compiled SQL: SELECT * FROM users WHERE id = $1 → [user] ``` -### Why Doesn't HasPermission Trigger auto_filter? - -**HasPermission.strict_check logic:** +### Why HasPermission doesn't trigger auto_filter ```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 + # No record (list query) → return false. Ash STOPS, 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) -- ✅ Tests use system_actor for authorization - -**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 - ---- +**Why return `false`, not `:unknown`?** We tested returning `:unknown`; Ash's policy evaluation still did **not** reliably call `auto_filter`. The `bypass` with `expr()` is the only consistent solution. (`has_permission_test.exs` accordingly expects `false`, not `:unknown`.) ## 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.** - ---- +If a future Ash version reliably calls `auto_filter` when `strict_check` returns `:unknown` or `{:filter, expr}`, the READ bypass could be removed and a single HasPermission policy kept for all operations (with tests updated). **This workaround was first identified under Ash 3.13.x and is still required as of the Ash version pinned in `mix.lock`; the bypass pattern remains 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` +- Ash policies: +- Implementation: see the `policies do` block in `Mv.Accounts.User` (`lib/accounts/user.ex`) +- Tests: `test/mv/accounts/user_policies_test.exs`, `test/mv/authorization/checks/has_permission_test.exs` +- Architecture: `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 216c6c9..864c328 100644 --- a/docs/roles-and-permissions-architecture.md +++ b/docs/roles-and-permissions-architecture.md @@ -101,6 +101,7 @@ Control CRUD operations on: - MemberGroup (member–group associations; own_data read :linked, read_only read :all, normal_user/admin create/destroy) - MembershipFeeType (fee type definitions; all read, admin-only create/update/destroy) - MembershipFeeCycle (fee cycles; own_data read :linked, read_only read :all, normal_user/admin read+create+update+destroy; manual "Regenerate Cycles" for normal_user and admin) +- JoinRequest (membership join requests; normal_user read+update, admin full CRUD) **4. Page-Level Permissions** @@ -123,7 +124,7 @@ Three scope levels for permissions: **6. Special Cases** - **Own Credentials:** Every user can always read/update their own credentials -- **Linked Member Email:** Only admins can edit email of member linked to user +- **Linked Member Email:** Only administrators or the linked user can change the email for members linked to users (see `Mv.Membership.Member.Validations.EmailChangePermission`) - **System Roles:** "Mitglied" role cannot be deleted (is_system_role flag) - **User-Member Linking:** Only admins can link/unlink users and members - **User Role Assignment:** Only admins can change a user's role (via `update_user` with `role_id`). Last-admin validation ensures at least one user keeps the Admin role. @@ -151,20 +152,12 @@ Role (stored in DB: "Vorstand" → "read_only") User (each user has one role) ``` -**Why This Approach?** +**Why This Approach?** Fast (2-3 weeks vs. 4-5 for DB-backed), maximum performance (< 1μs per +check, no permission queries/joins/cache), Git-tracked permission changes, deterministic +functional tests, and a well-defined Phase 3 migration path. -✅ **Fast Implementation:** 2-3 weeks vs. 4-5 weeks for DB-backed -✅ **Maximum Performance:** < 1 microsecond per check (pure function call) -✅ **Zero DB Overhead:** No permission queries, no joins, no cache needed -✅ **Git-Tracked Changes:** All permission changes in version control -✅ **Deterministic Testing:** No DB setup, purely functional tests -✅ **Clear Migration Path:** Well-defined Phase 3 for DB-backed permissions - -**Trade-offs:** - -⚠️ **Deployment Required:** Permission changes need code deployment -⚠️ **Four Fixed Sets:** Cannot add new permission sets without code change -✔️ **Acceptable for MVP:** Requirements specify 4 fixed sets, rare changes expected +**Trade-offs:** Permission changes need a code deployment and new sets cannot be added without +a code change — acceptable for the MVP, which specifies 4 fixed sets with rare changes. ### System Architecture Diagram @@ -292,11 +285,14 @@ The MVP requires **only ONE new table**: `roles` #### roles -Stores role definitions that reference permission sets by name. +Stores role definitions that reference permission sets by name. The SQL below is +**illustrative** — see `priv/repo/migrations/*_add_authorization_domain.exs` for the exact DDL +(notably the primary key uses the custom `uuid_generate_v7()` SQL function, and the unique index +is named `roles_unique_name_index`). ```sql CREATE TABLE roles ( - id UUID PRIMARY KEY DEFAULT gen_random_uuid(), + id UUID PRIMARY KEY DEFAULT uuid_generate_v7(), name VARCHAR(255) NOT NULL UNIQUE, description TEXT, permission_set_name VARCHAR(50) NOT NULL, @@ -308,7 +304,7 @@ CREATE TABLE roles ( CHECK (permission_set_name IN ('own_data', 'read_only', 'normal_user', 'admin')) ); -CREATE UNIQUE INDEX roles_name_index ON roles (name); +CREATE UNIQUE INDEX roles_unique_name_index ON roles (name); CREATE INDEX roles_permission_set_name_index ON roles (permission_set_name); ``` @@ -338,64 +334,20 @@ CREATE INDEX users_role_id_index ON users (role_id); ### Seed Data -Five predefined roles created during initial setup: +The five predefined roles are seeded in `priv/repo/seeds_bootstrap.exs` (the canonical source — +do not duplicate the data here). Each role is created idempotently via the Role resource's +`:create_role_with_system_flag` action, and the seeds map roles to permission sets as: -```elixir -# priv/repo/seeds/authorization_seeds.exs +| Role | permission_set_name | is_system_role | +|------|---------------------|----------------| +| Mitglied | own_data | true (cannot be deleted) | +| Vorstand | read_only | false | +| Kassenwart | normal_user | false | +| Buchhaltung | read_only | false | +| Admin | admin | false | -roles = [ - %{ - name: "Mitglied", - description: "Default member role with access to own data only", - permission_set_name: "own_data", - is_system_role: true # Cannot be deleted! - }, - %{ - name: "Vorstand", - description: "Board member with read access to all member data", - permission_set_name: "read_only", - is_system_role: false - }, - %{ - name: "Kassenwart", - description: "Treasurer with full member and payment management", - permission_set_name: "normal_user", - is_system_role: false - }, - %{ - name: "Buchhaltung", - description: "Accounting with read-only access for auditing", - permission_set_name: "read_only", - is_system_role: false - }, - %{ - name: "Admin", - description: "Administrator with unrestricted access", - permission_set_name: "admin", - is_system_role: false - } -] - -# Create roles with idempotent logic -Enum.each(roles, fn role_data -> - case Ash.get(Mv.Authorization.Role, name: role_data.name) do - {:ok, existing_role} -> - # Update if exists - Ash.update!(existing_role, role_data) - {:error, _} -> - # Create if not exists - Ash.create!(Mv.Authorization.Role, role_data) - end -end) - -# Assign "Mitglied" role to users without role -mitglied_role = Ash.get!(Mv.Authorization.Role, name: "Mitglied") -users_without_role = Ash.read!(Mv.Accounts.User, filter: expr(is_nil(role_id))) - -Enum.each(users_without_role, fn user -> - Ash.update!(user, %{role_id: mitglied_role.id}) -end) -``` +Assigning the default "Mitglied" role to users that have no role is handled separately by the +`assign_mitglied_role_to_existing_users` migration, not by the seed script. --- @@ -405,277 +357,41 @@ end) **Location:** `lib/mv/authorization/permission_sets.ex` -This module is the **single source of truth** for all permissions in the MVP. It defines what each permission set can do. +This module is the **single source of truth** for all permissions in the MVP. It defines what +each permission set can do, as pure compile-time functions (lookups < 1μs). -#### Module Structure +**Types & public API:** -```elixir -defmodule Mv.Authorization.PermissionSets do - @moduledoc """ - Defines the four hardcoded permission sets for the application. - - Each permission set specifies: - - Resource permissions (what CRUD operations on which resources) - - Page permissions (which LiveView pages can be accessed) - - Scopes (own, linked, all) - - ## Permission Sets - - 1. **own_data** - Default for "Mitglied" role - - Can only access own user data and linked member/custom field values - - Cannot create new members or manage system - - 2. **read_only** - For "Vorstand" and "Buchhaltung" roles - - Can read all member data - - Cannot create, update, or delete - - 3. **normal_user** - For "Kassenwart" role - - Create/Read/Update members (no delete), full CRUD on properties - - Cannot manage custom fields or users - - 4. **admin** - For "Admin" role - - Unrestricted access to all resources - - Can manage users, roles, custom fields - - ## Usage - - # Get permissions for a role's permission set - permissions = PermissionSets.get_permissions(:admin) - - # Check if a permission set name is valid - PermissionSets.valid_permission_set?("read_only") # => true - - # Convert string to atom safely - {:ok, atom} = PermissionSets.permission_set_name_to_atom("own_data") - - ## Performance - - All functions are pure and compile-time. Permission lookups are < 1 microsecond. - """ +- `@type scope :: :own | :linked | :all`, `@type action :: :read | :create | :update | :destroy` +- A `resource_permission` is `%{resource: String.t(), action:, scope:, granted: boolean}`; + a `permission_set` is `%{resources: [resource_permission], pages: [String.t()]}`. +- `all_permission_sets/0` → `[:own_data, :read_only, :normal_user, :admin]`. +- `get_permissions/1` — one function clause per set returning its `%{resources, pages}` map. + An unknown atom raises `ArgumentError` (callers always go through the conversion below). +- `valid_permission_set?/1` — accepts string or atom; the string clause delegates to the + converter; the atom clause checks membership in `all_permission_sets/0`. +- `permission_set_name_to_atom/1` — `String.to_existing_atom/1` guarded by validity, and + **rescues `ArgumentError`** (unknown string → never-created atom) returning + `{:error, :invalid_permission_set}`. This is the safe entry point used everywhere. - @type scope :: :own | :linked | :all - @type action :: :read | :create | :update | :destroy - - @type resource_permission :: %{ - resource: String.t(), - action: action(), - scope: scope(), - granted: boolean() - } - - @type permission_set :: %{ - resources: [resource_permission()], - pages: [String.t()] - } +**Resource permissions per set** are exactly the Permission Matrix below. Note `normal_user` +intentionally omits `Member :destroy` (safety); `own_data` has full CRUD on its linked +CustomFieldValues; all four sets grant `User read/update :own`. - @doc """ - Returns the list of all valid permission set names. - - ## Examples - - iex> PermissionSets.all_permission_sets() - [:own_data, :read_only, :normal_user, :admin] - """ - @spec all_permission_sets() :: [atom()] - def all_permission_sets do - [:own_data, :read_only, :normal_user, :admin] - end +**Pages per set:** the exact `pages` lists live in the `get_permissions/1` clauses of +`Mv.Authorization.PermissionSets` (single source of truth). Key facts that shape the lists: - @doc """ - Returns permissions for the given permission set. - - ## Examples - - iex> permissions = PermissionSets.get_permissions(:admin) - iex> Enum.any?(permissions.resources, fn p -> - ...> p.resource == "User" and p.action == :destroy - ...> end) - true - - iex> PermissionSets.get_permissions(:invalid) - ** (FunctionClauseError) no function clause matching - """ - @spec get_permissions(atom()) :: permission_set() - - def get_permissions(:own_data) do - %{ - resources: [ - # User: Can always read/update own credentials - %{resource: "User", action: :read, scope: :own, granted: true}, - %{resource: "User", action: :update, scope: :own, granted: true}, - - # Member: Can read/update linked member - %{resource: "Member", action: :read, scope: :linked, granted: true}, - %{resource: "Member", action: :update, scope: :linked, granted: true}, - - # CustomFieldValue: Can read/update/create/destroy custom field values of linked member - %{resource: "CustomFieldValue", action: :read, scope: :linked, granted: true}, - %{resource: "CustomFieldValue", action: :update, scope: :linked, granted: true}, - %{resource: "CustomFieldValue", action: :create, scope: :linked, granted: true}, - %{resource: "CustomFieldValue", action: :destroy, scope: :linked, granted: true}, - - # CustomField: Can read all (needed for forms) - %{resource: "CustomField", action: :read, scope: :all, granted: true} - ], - pages: [ - "/", # Home page - "/profile", # Own profile - "/members/:id" # Linked member detail (filtered by policy) - ] - } - end +- **own_data:** deliberately does **not** include `/` (Mitglied must not see the member index at + root, which has the same content as `/members`). Self-service pages are `/users/:id`, + `/users/:id/edit`, `/users/:id/show/edit`; linked-member pages are `/members/:id`, + `/members/:id/edit`, `/members/:id/show/edit` (data access filtered by policy scope `:linked`). +- **read_only / normal_user:** include `/` plus the self-service `/users/:id…` pages and their + respective member / custom-field-value / group pages; normal_user additionally has the create/edit + pages and the `/join_requests` approval pages. +- **admin:** `"*"` wildcard (all pages), with `/settings` and `/membership_fee_settings` also listed + explicitly. - def get_permissions(:read_only) do - %{ - resources: [ - # User: Can read/update own credentials only - %{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}, - - # CustomFieldValue: Can read all custom field values - %{resource: "CustomFieldValue", action: :read, scope: :all, granted: true}, - - # CustomField: Can read all - %{resource: "CustomField", action: :read, scope: :all, granted: true} - ], - pages: [ - "/", - "/members", # Member list - "/members/:id", # Member detail - "/custom_field_values" # Custom field values overview - "/profile" # Own profile - ] - } - end - - def get_permissions(:normal_user) do - %{ - resources: [ - # User: Can read/update own credentials only - %{resource: "User", action: :read, scope: :own, granted: true}, - %{resource: "User", action: :update, scope: :own, granted: true}, - - # Member: Full CRUD - %{resource: "Member", action: :read, scope: :all, granted: true}, - %{resource: "Member", action: :create, scope: :all, granted: true}, - %{resource: "Member", action: :update, scope: :all, granted: true}, - # Note: destroy intentionally omitted for safety - - # CustomFieldValue: Full CRUD - %{resource: "CustomFieldValue", action: :read, scope: :all, granted: true}, - %{resource: "CustomFieldValue", action: :create, scope: :all, granted: true}, - %{resource: "CustomFieldValue", action: :update, scope: :all, granted: true}, - %{resource: "CustomFieldValue", action: :destroy, scope: :all, granted: true}, - - # CustomField: Read only (admin manages definitions) - %{resource: "CustomField", action: :read, scope: :all, granted: true} - ], - pages: [ - "/", - "/members", - "/members/new", # Create member - "/members/:id", - "/members/:id/edit", # Edit member - "/custom_field_values", - "/custom_field_values/new", - "/custom_field_values/:id/edit", - "/profile" - ] - } - end - - def get_permissions(:admin) do - %{ - resources: [ - # User: Full management including other users - %{resource: "User", action: :read, scope: :all, granted: true}, - %{resource: "User", action: :create, scope: :all, granted: true}, - %{resource: "User", action: :update, scope: :all, granted: true}, - %{resource: "User", action: :destroy, scope: :all, granted: true}, - - # Member: Full CRUD - %{resource: "Member", action: :read, scope: :all, granted: true}, - %{resource: "Member", action: :create, scope: :all, granted: true}, - %{resource: "Member", action: :update, scope: :all, granted: true}, - %{resource: "Member", action: :destroy, scope: :all, granted: true}, - - # CustomFieldValue: Full CRUD - %{resource: "CustomFieldValue", action: :read, scope: :all, granted: true}, - %{resource: "CustomFieldValue", action: :create, scope: :all, granted: true}, - %{resource: "CustomFieldValue", action: :update, scope: :all, granted: true}, - %{resource: "CustomFieldValue", action: :destroy, scope: :all, granted: true}, - - # CustomField: Full CRUD (admin manages custom field definitions) - %{resource: "CustomField", action: :read, scope: :all, granted: true}, - %{resource: "CustomField", action: :create, scope: :all, granted: true}, - %{resource: "CustomField", action: :update, scope: :all, granted: true}, - %{resource: "CustomField", action: :destroy, scope: :all, granted: true}, - - # Role: Full CRUD (admin manages roles) - %{resource: "Role", action: :read, scope: :all, granted: true}, - %{resource: "Role", action: :create, scope: :all, granted: true}, - %{resource: "Role", action: :update, scope: :all, granted: true}, - %{resource: "Role", action: :destroy, scope: :all, granted: true} - ], - pages: [ - "*" # Wildcard: Admin can access all pages - ] - } - end - - @doc """ - Checks if a permission set name (string or atom) is valid. - - ## Examples - - iex> PermissionSets.valid_permission_set?("admin") - true - - iex> PermissionSets.valid_permission_set?(:read_only) - true - - iex> PermissionSets.valid_permission_set?("invalid") - false - """ - @spec valid_permission_set?(String.t() | atom()) :: boolean() - def valid_permission_set?(name) when is_binary(name) do - case permission_set_name_to_atom(name) do - {:ok, _atom} -> true - {:error, _} -> false - end - end - - def valid_permission_set?(name) when is_atom(name) do - name in all_permission_sets() - end - - @doc """ - Converts a permission set name string to atom safely. - - ## Examples - - iex> PermissionSets.permission_set_name_to_atom("admin") - {:ok, :admin} - - iex> PermissionSets.permission_set_name_to_atom("invalid") - {:error, :invalid_permission_set} - """ - @spec permission_set_name_to_atom(String.t()) :: {:ok, atom()} | {:error, :invalid_permission_set} - def permission_set_name_to_atom(name) when is_binary(name) do - atom = String.to_existing_atom(name) - if valid_permission_set?(atom) do - {:ok, atom} - else - {:error, :invalid_permission_set} - end - rescue - ArgumentError -> {:error, :invalid_permission_set} - end -end -``` +There is no `/profile` route; the self-service profile pages are the `/users/:id…` routes above. #### Permission Matrix @@ -697,6 +413,7 @@ Quick reference table showing what each permission set allows: | **MembershipFeeType** (all) | R | R | R | R, C, U, D | | **MembershipFeeCycle** (linked) | R | - | - | - | | **MembershipFeeCycle** (all) | - | R | R, C, U, D | R, C, U, D | +| **JoinRequest** (all) | - | - | R, U | R, C, U, D | **Legend:** R=Read, C=Create, U=Update, D=Destroy @@ -745,109 +462,27 @@ defmodule Mv.Authorization.Checks.HasPermission do """ use Ash.Policy.Check - require Ash.Query - import Ash.Expr - alias Mv.Authorization.PermissionSets - - @impl true - def describe(_opts) do - "checks if actor has permission via their role's permission set" - end - - @impl true - def match?(actor, %{resource: resource, action: %{name: action}}, _opts) do - 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), - resource_name <- get_resource_name(resource) do - check_permission(permissions.resources, resource_name, action, actor, resource_name) - else - %{role: nil} -> - log_auth_failure(actor, resource, action, "no role assigned") - {:error, :no_role} - - %{role: %{permission_set_name: nil}} -> - log_auth_failure(actor, resource, action, "role has no permission_set_name") - {:error, :no_permission_set} - - {:error, :invalid_permission_set} = error -> - log_auth_failure(actor, resource, action, "invalid permission_set_name") - error - - _ -> - log_auth_failure(actor, resource, action, "no actor or missing data") - {:error, :no_permission} - end - end - - # Extract resource name from module (e.g., Mv.Membership.Member -> "Member") - defp get_resource_name(resource) when is_atom(resource) do - resource |> Module.split() |> List.last() - end - - # Find matching permission and apply scope - defp check_permission(resource_perms, resource_name, action, actor, resource_module_name) do - case Enum.find(resource_perms, fn perm -> - perm.resource == resource_name and - perm.action == action and - perm.granted - end) do - nil -> - {:error, :no_permission} - - perm -> - apply_scope(perm.scope, actor, resource_name) - end - end - - # Scope: all - No filtering, access to all records - defp apply_scope(:all, _actor, _resource) do - :authorized - end - - # Scope: own - Filter to records where record.id == actor.id - # Used for User resource (users can access their own user record) - defp apply_scope(:own, actor, _resource) do - {:filter, expr(id == ^actor.id)} - end - - # Scope: linked - Filter based on user_id relationship (resource-specific!) - defp apply_scope(:linked, actor, resource_name) do - case resource_name do - "Member" -> - # User.member_id → Member.id (inverse relationship) - # Filter: member.id == actor.member_id - {:filter, expr(id == ^actor.member_id)} - - "CustomFieldValue" -> - # CustomFieldValue.member_id → Member.id → User.member_id - # Filter: custom_field_value.member_id == actor.member_id - {:filter, expr(member_id == ^actor.member_id)} - - _ -> - # Fallback for other resources: try direct user_id - {:filter, expr(user_id == ^actor.id)} - end - end - - # Log authorization failures for debugging - defp log_auth_failure(actor, resource, action, reason) do - require Logger - - actor_id = if is_map(actor), do: Map.get(actor, :id), else: "nil" - resource_name = get_resource_name(resource) - - Logger.debug(""" - Authorization failed: - Actor: #{actor_id} - Resource: #{resource_name} - Action: #{action} - Reason: #{reason} - """) - end + # ... end ``` +**`match?/3` logic.** With-chain: read `actor.role.permission_set_name` (non-nil) → +`PermissionSets.permission_set_name_to_atom/1` → `get_permissions/1` → resource name via +`Module.split() |> List.last()`. Find the granted permission matching resource+action; if none, +`{:error, :no_permission}`; otherwise apply the scope filter. The `else` clauses log and return a +specific reason — `{:error, :no_role}` (role nil), `{:error, :no_permission_set}` +(permission_set_name nil), `{:error, :invalid_permission_set}`, or `{:error, :no_permission}` +(no actor/missing data). Every error results in Forbidden (fail-closed). + +**Scope filters (`apply_scope/3`):** + +- `:all` → `:authorized` (no filter) +- `:own` → `{:filter, expr(id == ^actor.id)}` (User: own record) +- `:linked` → resource-specific: + - `"Member"` → `{:filter, expr(id == ^actor.member_id)}` (User.member_id → Member.id, inverse) + - `"CustomFieldValue"` → `{:filter, expr(member_id == ^actor.member_id)}` (traverses CFV.member_id → Member → User.member_id) + - fallback → `{:filter, expr(user_id == ^actor.id)}` + **Key Design Decisions:** 1. **Resource-Specific :linked Scope:** CustomFieldValue needs to traverse `member` relationship to check `user_id` @@ -888,58 +523,18 @@ end --- -## Bypass vs. HasPermission: When to Use Which? +## Bypass vs. HasPermission -**Key Finding:** For filter-based permissions (`scope :own`, `scope :linked`), we use a **two-tier approach**: +For filter-based permissions (`scope :own`, `scope :linked`) the resources use a two-tier +pattern: **bypass with `expr()` for READ** (Ash does not reliably trigger `auto_filter` +when `HasPermission`'s `strict_check` returns `{:ok, false}` on record-less list queries), +and **HasPermission for UPDATE/CREATE/DESTROY** (a changeset record is present, so scope is +evaluated correctly). The scope concept stays meaningful — bypass is only a workaround for +Ash's auto_filter limitation, not a replacement for it. -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. +The full rationale, the per-operation decision table, and why both `User` and `Member` +follow this pattern are documented in the canonical +[policy-bypass-vs-haspermission.md](./policy-bypass-vs-haspermission.md). --- @@ -1006,7 +601,7 @@ end ### Member Resource Policies -**Location:** `lib/mv/membership/member.ex` +**Location:** `lib/membership/member.ex` **Pattern:** Bypass for READ (list queries), HasPermission for UPDATE (with scope :linked). @@ -1043,11 +638,10 @@ defmodule Mv.Membership.Member do # 4. DEFAULT: Ash implicitly forbids if no policy authorizes (fail-closed) end - # Custom validation for email editing (see Special Cases section) + # Linked-member email editing is enforced by a dedicated Validations module + # (see Special Cases section) validations do - validate changing(:email), on: :update do - validate &validate_linked_member_email_change/2 - end + validate Mv.Membership.Member.Validations.EmailChangePermission, on: [:update] end # ... @@ -1252,144 +846,24 @@ Page permissions control which LiveView pages a user can access. This is enforce **Location:** `lib/mv_web/plugs/check_page_permission.ex` -This plug runs in the router pipeline and checks if the current user has permission to access the requested page. +This plug runs in the router pipeline and checks if the current user has permission to access +the requested page, **before** the LiveView mounts. -```elixir -defmodule MvWeb.Plugs.CheckPagePermission do - @moduledoc """ - Plug that checks if current user has permission to access the current page. - - ## How It Works - - 1. Extracts page path from conn (route template like "/members/:id") - 2. Gets current user from conn.assigns - 3. Gets user's permission_set_name from role - 4. Calls PermissionSets.get_permissions/1 to get allowed pages - 5. Matches requested path against allowed patterns - 6. If unauthorized: redirects to "/" with flash error - - ## Pattern Matching - - - Exact match: "/members" == "/members" - - Dynamic routes: "/members/:id" matches "/members/123" - - Wildcard: "*" matches everything (admin) - - ## Usage in Router - - pipeline :require_page_permission do - plug MvWeb.Plugs.CheckPagePermission - end - - scope "/members", MvWeb do - pipe_through [:browser, :require_authenticated_user, :require_page_permission] - - live "/", MemberLive.Index - live "/:id", MemberLive.Show - end - """ - - import Plug.Conn - import Phoenix.Controller - alias Mv.Authorization.PermissionSets - require Logger +**Behavior (`lib/mv_web/plugs/check_page_permission.ex`):** - def init(opts), do: opts - - def call(conn, _opts) do - user = conn.assigns[:current_user] - page_path = get_page_path(conn) - - if has_page_permission?(user, page_path) do - conn - else - log_page_access_denied(user, page_path) - - conn - |> put_flash(:error, "You don't have permission to access this page.") - |> redirect(to: "/") - |> halt() - end - end - - # Extract page path from conn (route template preferred, fallback to request_path) - defp get_page_path(conn) do - case conn.private[:phoenix_route] do - {_plug, _opts, _pipe, route_template, _meta} -> - route_template - - _ -> - conn.request_path - end - end - - # Check if user has permission for page - defp has_page_permission?(nil, _page_path) do - false - end - - defp has_page_permission?(user, page_path) do - with %{role: %{permission_set_name: ps_name}} <- user, - {:ok, ps_atom} <- PermissionSets.permission_set_name_to_atom(ps_name), - permissions <- PermissionSets.get_permissions(ps_atom) do - page_matches?(permissions.pages, page_path) - else - _ -> false - end - end - - # Check if requested path matches any allowed pattern - defp page_matches?(allowed_pages, requested_path) do - Enum.any?(allowed_pages, fn pattern -> - cond do - # Wildcard: admin can access all pages - pattern == "*" -> - true - - # Exact match - pattern == requested_path -> - true - - # Dynamic route match (e.g., "/members/:id" matches "/members/123") - String.contains?(pattern, ":") -> - match_dynamic_route?(pattern, requested_path) - - # No match - true -> - false - end - end) - end - - # Match dynamic route pattern against actual path - defp match_dynamic_route?(pattern, path) do - pattern_segments = String.split(pattern, "/", trim: true) - path_segments = String.split(path, "/", trim: true) - - # Must have same number of segments - if length(pattern_segments) == length(path_segments) do - Enum.zip(pattern_segments, path_segments) - |> Enum.all?(fn {pattern_seg, path_seg} -> - # Dynamic segment (starts with :) matches anything - String.starts_with?(pattern_seg, ":") or pattern_seg == path_seg - end) - else - false - end - end - - defp log_page_access_denied(user, page_path) do - user_id = if is_map(user), do: Map.get(user, :id), else: "nil" - role = if is_map(user), do: get_in(user, [:role, :name]), else: "nil" - - Logger.info(""" - Page access denied: - User: #{user_id} - Role: #{role} - Page: #{page_path} - """) - end -end -``` +1. Extracts the page path as the **route template** (e.g. `/members/:id`) via + `Phoenix.Router.route_info/4`, falling back to `conn.request_path`. Using the template, + not the concrete path, is what lets the permission `pages` lists stay parameterized. + (Public paths such as `/sign-in`, `/register`, `/auth/*` are exempt and pass through.) +2. Reads `current_user` from `conn.assigns`, resolves its `permission_set_name`, and looks up + the allowed `pages` via `PermissionSets.get_permissions/1`. +3. Matches the path against allowed patterns: `*` wildcard (admin), exact match, or + segment-wise dynamic match where a `:`-prefixed pattern segment matches any path segment + (same segment count required). +4. On no match (including nil user, no role, or invalid permission set → false): logs the + denial and **redirects to `/users/:id`** (the logged-in user's own profile) or, when there is + no user, to **`/sign-in`**, then halts. The `"You don't have permission to access this page."` + flash is set only for a logged-in user; an unauthenticated visitor is redirected without a flash. ### Router Integration @@ -1435,19 +909,19 @@ end ### Page Permission Examples **Mitglied (own_data):** -- ✅ Can access: `/`, `/profile`, `/members/123` (if 123 is their linked member) -- ❌ Cannot access: `/members`, `/members/new`, `/admin/roles` +- ✅ Can access: `/users/123` (own profile), `/members/123` (if 123 is their linked member) +- ❌ Cannot access: `/` (root member index is excluded for own_data), `/members`, `/members/new`, `/settings` **Vorstand (read_only):** -- ✅ Can access: `/`, `/members`, `/members/123`, `/custom_field_values`, `/profile` -- ❌ Cannot access: `/members/new`, `/members/123/edit`, `/admin/roles` +- ✅ Can access: `/`, `/members`, `/members/123`, `/custom_field_values`, `/users/123` (own profile) +- ❌ Cannot access: `/members/new`, `/members/123/edit`, `/settings` **Kassenwart (normal_user):** -- ✅ Can access: `/`, `/members`, `/members/new`, `/members/123/edit`, `/custom_field_values`, `/profile` -- ❌ Cannot access: `/admin/roles`, `/admin/custom_fields/new` +- ✅ Can access: `/`, `/members`, `/members/new`, `/members/123/edit`, `/custom_field_values`, `/join_requests`, `/users/123` (own profile) +- ❌ Cannot access: `/settings`, `/membership_fee_settings` **Admin:** -- ✅ Can access: `*` (all pages, including `/admin/roles`) +- ✅ Can access: `*` (all pages, including `/settings` and `/membership_fee_settings`) --- @@ -1459,316 +933,38 @@ UI-level authorization ensures that users only see buttons, links, and form fiel **Location:** `lib/mv_web/authorization.ex` -This module provides helper functions for conditional rendering in LiveView templates. +This module provides helper functions for conditional rendering in LiveView templates, +reading from the **same** `PermissionSets` module as the backend policies so UI and backend +stay consistent (pure function calls, no DB queries). Imported into `mv_web.ex` `html_helpers` +so every LiveView has it: `import MvWeb.Authorization, only: [can?: 3, can_access_page?: 2]`. -```elixir -defmodule MvWeb.Authorization do - @moduledoc """ - UI-level authorization helpers for LiveView templates. - - These functions check if the current user has permission to perform actions - or access pages. They use the same PermissionSets module as the backend policies, - ensuring UI and backend authorization are consistent. - - ## Usage in Templates - - - <%= if can?(@current_user, :create, Mv.Membership.Member) do %> - <.link patch={~p"/members/new"}>New Member - <% end %> - - - <%= if can?(@current_user, :update, @member) do %> - <.button>Edit - <% end %> - - - <%= if can_access_page?(@current_user, "/admin/roles") do %> - <.link navigate="/admin/roles">Manage Roles - <% end %> - - ## Performance - - All checks are pure function calls using the hardcoded PermissionSets module. - No database queries, < 1 microsecond per check. - """ - - alias Mv.Authorization.PermissionSets +**Public functions:** - @doc """ - Checks if user has permission for an action on a resource (atom). - - ## Examples - - iex> admin = %{role: %{permission_set_name: "admin"}} - iex> can?(admin, :create, Mv.Membership.Member) - true - - iex> mitglied = %{role: %{permission_set_name: "own_data"}} - iex> can?(mitglied, :create, Mv.Membership.Member) - false - """ - @spec can?(map() | nil, atom(), atom()) :: boolean() - def can?(nil, _action, _resource), do: false - - def can?(user, action, resource) when is_atom(action) and is_atom(resource) do - with %{role: %{permission_set_name: ps_name}} <- user, - {:ok, ps_atom} <- PermissionSets.permission_set_name_to_atom(ps_name), - permissions <- PermissionSets.get_permissions(ps_atom) do - resource_name = get_resource_name(resource) - - Enum.any?(permissions.resources, fn perm -> - perm.resource == resource_name and - perm.action == action and - perm.granted - end) - else - _ -> false - end - end +- `can?/3` (resource atom) — `can?(user, action, Mv.Membership.Member)`: true iff the user's + permission set grants `action` on that resource (any scope). +- `can?/3` (record struct) — `can?(user, action, %Member{})`: finds the matching permission, + then applies the scope check against the record: + - `:all` → always true + - `:own` → `record.id == user.id` + - `:linked` → resource-specific: Member checks `record.user_id == user.id`; CustomFieldValue + traverses `record.member.user_id == user.id` (member must be preloaded), with a `user_id` + fallback for other resources. +- `can_access_page?/2` — matches the path against the permission set's `pages` list using the + same rules as the plug: `*` wildcard, exact match, or dynamic segment match (`:id`). - @doc """ - Checks if user has permission for an action on a specific record (struct). - - Applies scope checking: - - :own - record.id == user.id - - :linked - record.user_id == user.id (or traverses relationships) - - :all - always true - - ## Examples - - iex> user = %{id: "user-123", role: %{permission_set_name: "own_data"}} - iex> member = %Member{id: "member-456", user_id: "user-123"} - iex> can?(user, :update, member) - true - - iex> other_member = %Member{id: "member-789", user_id: "other-user"} - iex> can?(user, :update, other_member) - false - """ - @spec can?(map() | nil, atom(), struct()) :: boolean() - def can?(nil, _action, _record), do: false - - def can?(user, action, %resource{} = record) when is_atom(action) do - with %{role: %{permission_set_name: ps_name}} <- user, - {:ok, ps_atom} <- PermissionSets.permission_set_name_to_atom(ps_name), - permissions <- PermissionSets.get_permissions(ps_atom) do - resource_name = get_resource_name(resource) - - # Find matching permission - matching_perm = Enum.find(permissions.resources, fn perm -> - perm.resource == resource_name and - perm.action == action and - perm.granted - end) - - case matching_perm do - nil -> false - perm -> check_scope(perm.scope, user, record, resource_name) - end - else - _ -> false - end - end +All three return **false** for a nil user, a user without a role, or an invalid +`permission_set_name` (graceful, fail-closed — no crash). The scope/page-matching logic mirrors +`HasPermission` and `CheckPagePermission` exactly; resource names come from +`Module.split() |> List.last()`. - @doc """ - Checks if user can access a specific page. - - ## Examples - - iex> admin = %{role: %{permission_set_name: "admin"}} - iex> can_access_page?(admin, "/admin/roles") - true - - iex> mitglied = %{role: %{permission_set_name: "own_data"}} - iex> can_access_page?(mitglied, "/members") - false - """ - @spec can_access_page?(map() | nil, String.t()) :: boolean() - def can_access_page?(nil, _page_path), do: false - - def can_access_page?(user, page_path) do - with %{role: %{permission_set_name: ps_name}} <- user, - {:ok, ps_atom} <- PermissionSets.permission_set_name_to_atom(ps_name), - permissions <- PermissionSets.get_permissions(ps_atom) do - page_matches?(permissions.pages, page_path) - else - _ -> false - end - end +### UI Usage Pattern - # Check if scope allows access to record - defp check_scope(:all, _user, _record, _resource_name), do: true - - defp check_scope(:own, user, record, _resource_name) do - record.id == user.id - end - - defp check_scope(:linked, user, record, resource_name) do - case resource_name do - "Member" -> - # Direct relationship: member.user_id - Map.get(record, :user_id) == user.id - - "CustomFieldValue" -> - # Need to traverse: custom_field_value.member.user_id - # Note: In UI, custom_field_value should have member preloaded - case Map.get(record, :member) do - %{user_id: member_user_id} -> member_user_id == user.id - _ -> false - end - - _ -> - # Fallback: check user_id - Map.get(record, :user_id) == user.id - end - end - - # Check if page path matches any allowed pattern - defp page_matches?(allowed_pages, requested_path) do - Enum.any?(allowed_pages, fn pattern -> - cond do - pattern == "*" -> true - pattern == requested_path -> true - String.contains?(pattern, ":") -> match_pattern?(pattern, requested_path) - true -> false - end - end) - end - - # Match dynamic route pattern - defp match_pattern?(pattern, path) do - pattern_segments = String.split(pattern, "/", trim: true) - path_segments = String.split(path, "/", trim: true) - - if length(pattern_segments) == length(path_segments) do - Enum.zip(pattern_segments, path_segments) - |> Enum.all?(fn {pattern_seg, path_seg} -> - String.starts_with?(pattern_seg, ":") or pattern_seg == path_seg - end) - else - false - end - end - - # Extract resource name from module - defp get_resource_name(resource) when is_atom(resource) do - resource |> Module.split() |> List.last() - end -end -``` - -### Import in mv_web.ex - -Make helpers available to all LiveViews: - -```elixir -defmodule MvWeb do - # ... - - def html_helpers do - quote do - # ... existing helpers ... - - # Authorization helpers - import MvWeb.Authorization, only: [can?: 3, can_access_page?: 2] - end - end - - # ... -end -``` - -### UI Examples - -**Navbar with conditional links:** - -```heex - - -``` - -**Index page with conditional "New" button:** - -```heex - -
-

Members

- - - <%= if can?(@current_user, :create, Mv.Membership.Member) do %> - <.link patch={~p"/members/new"} class="btn-primary"> - New Member - - <% end %> -
- - - - <%= for member <- @members do %> - - - - - <% end %> -
<%= member.name %> - - <%= if can?(@current_user, :update, member) do %> - <.link patch={~p"/members/#{member.id}/edit"}>Edit - <% end %> - - - <%= if can?(@current_user, :destroy, member) do %> - <.button phx-click="delete" phx-value-id={member.id}>Delete - <% end %> -
-``` - -**Show page with conditional edit button:** - -```heex - -
-

<%= @member.name %>

- -
-
Email
-
<%= @member.email %>
- -
Address
-
<%= @member.address %>
-
- - - <%= if can?(@current_user, :update, @member) do %> - <.link patch={~p"/members/#{@member.id}/edit"} class="btn-primary"> - Edit Member - - <% end %> -
-``` +LiveView templates gate elements with the helpers: page-level links use +`can_access_page?(@current_user, path)` (e.g. the `/members` link and the admin +dropdown), resource-level buttons use `can?(@current_user, :create, Resource)` +(e.g. "New Member"), and per-record buttons use `can?(@current_user, action, record)` +(e.g. Edit/Delete in a member row, or the edit button on a show page). The navbar has +since been replaced by the sidebar (`lib/mv_web/components/layouts/sidebar.ex`). --- @@ -1807,7 +1003,7 @@ end - 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. +**Important:** UPDATE is NOT an immovable special case (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 @@ -1832,209 +1028,33 @@ end - **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 -] -``` +**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. Every set's `get_permissions/...` therefore carries both `%{resource: "User", action: :read, scope: :own}` and `%{... action: :update, scope: :own}`; the "read_only" label applies to member data (no `Member :update`), not credentials. ### 2. Linked Member Email Editing -**Requirement:** Only administrators can edit the email of a member that is linked to a user (has `user_id` set). This prevents breaking email synchronization. +**Requirement:** For a member linked to a user account (has a linked user), only administrators **or the linked user themselves** can change the email. This prevents breaking the Member↔User email synchronization while still letting a user update their own email. -**Implementation:** - -Custom validation in `Member` resource: - -```elixir -defmodule Mv.Membership.Member do - use Ash.Resource, ... - - validations do - # Only run when email is being changed - validate changing(:email), on: :update do - validate &validate_linked_member_email_change/2 - end - end - - defp validate_linked_member_email_change(changeset, _context) do - member = changeset.data - actor = changeset.context[:actor] - - # If member is not linked to user, allow change - if is_nil(member.user_id) do - :ok - else - # Member is linked - check if actor is admin - if has_admin_permission?(actor) do - :ok - else - {:error, "Only administrators can change email for members linked to user accounts"} - end - end - end - - defp has_admin_permission?(nil), do: false - - defp has_admin_permission?(actor) do - with %{role: %{permission_set_name: ps_name}} <- actor, - {:ok, ps_atom} <- PermissionSets.permission_set_name_to_atom(ps_name), - permissions <- PermissionSets.get_permissions(ps_atom) do - # Check if actor has User.update permission with scope :all (admin privilege) - Enum.any?(permissions.resources, fn perm -> - perm.resource == "User" and - perm.action == :update and - perm.scope == :all and - perm.granted - end) - else - _ -> false - end - end -end -``` - -**Why this is needed:** -- Member email and User email are kept in sync -- If a non-admin changes linked member email, it could create inconsistency -- Validation runs AFTER policy check, so normal_user can update member -- But validation blocks email field specifically if member is linked +**Implementation:** The `Mv.Membership.Member.Validations.EmailChangePermission` module (registered as `validate Mv.Membership.Member.Validations.EmailChangePermission, on: [:update]`) runs **after** the policy check (so a `normal_user` may update the member but is still blocked on the email field). It only acts when the email is changing: if the member has no linked user it allows the change; otherwise it allows the change when the actor is admin (`Mv.Authorization.Actor.admin?/1`, which also treats the system actor as admin) **or** owns the linked member (`actor.member_id == member.id`), and otherwise returns `{:error, "Only administrators or the linked user can change the email for members linked to users"}`. A missing actor is not allowed. ### 3. System Role Protection -**Requirement:** The "Mitglied" role cannot be deleted because it's the default role for all users. +**Requirement:** The "Mitglied" role cannot be deleted (it's the default role for all users). -**Implementation:** - -Flag + validation in `Role` resource: - -```elixir -defmodule Mv.Authorization.Role do - use Ash.Resource, ... - - attributes do - # ... - attribute :is_system_role, :boolean, default: false - end - - validations do - validate action(:destroy) do - validate fn _changeset, %{data: role} -> - if role.is_system_role do - {:error, "Cannot delete system role. System roles are required for the application to function."} - else - :ok - end - end - end - end -end -``` - -**Seeds set the flag:** - -```elixir -%{ - name: "Mitglied", - permission_set_name: "own_data", - is_system_role: true # <-- Protected! -} -``` - -**UI hides delete button:** - -```heex -<%= if can?(@current_user, :destroy, role) and not role.is_system_role do %> - <.button phx-click="delete">Delete -<% end %> -``` +**Implementation:** The `Role` resource has an `is_system_role` boolean (default false); a destroy validation returns `{:error, "Cannot delete system role. ..."}` when `role.is_system_role` is true. Seeds set `is_system_role: true` only on "Mitglied". The UI also hides the delete button: `can?(@current_user, :destroy, role) and not role.is_system_role`. ### 4. User Without Role (Edge Case) -**Requirement:** Users without a role should be denied all access (except logout). +**Requirement:** Users without a role are denied all access (except logout). -**Implementation:** +**Implementation:** Seeds assign "Mitglied" to all users where `role_id` is nil. At runtime every check handles a missing role gracefully — `HasPermission` returns `{:error, :no_role}` (and the UI helpers/plug return false) rather than crashing. -**Default Assignment:** Seeds assign "Mitglied" role to all existing users - -```elixir -# In authorization_seeds.exs -mitglied_role = Ash.get!(Role, name: "Mitglied") -users_without_role = Ash.read!(User, filter: expr(is_nil(role_id))) - -Enum.each(users_without_role, fn user -> - Ash.update!(user, %{role_id: mitglied_role.id}) -end) -``` - -**Runtime Handling:** All authorization checks handle missing role gracefully - -```elixir -# In HasPermission check -def match?(actor, %{resource: resource, action: action}, _opts) do - with %{role: %{permission_set_name: ps_name}} when not is_nil(ps_name) <- actor, - # ... - else - %{role: nil} -> - {:error, :no_role} # User has no role -> forbidden - - _ -> - {:error, :no_permission} - end -end -``` - -**Result:** User with no role sees empty UI, cannot access pages, gets forbidden on all actions. +**Result:** A user with no role sees an empty UI, cannot access pages, and is forbidden on all actions. ### 5. Invalid permission_set_name (Edge Case) **Requirement:** If a role has an invalid `permission_set_name`, fail gracefully without crashing. -**Implementation:** - -**Prevention:** Validation on Role resource - -```elixir -validations do - validate attribute(:permission_set_name) do - validate fn _changeset, value -> - if PermissionSets.valid_permission_set?(value) do - :ok - else - {:error, "Invalid permission set name. Must be one of: #{Enum.join(PermissionSets.all_permission_sets(), ", ")}"} - end - end - end -end -``` - -**Runtime Handling:** All lookups check validity - -```elixir -# In PermissionSets module -def permission_set_name_to_atom(name) when is_binary(name) do - atom = String.to_existing_atom(name) - if valid_permission_set?(atom) do - {:ok, atom} - else - {:error, :invalid_permission_set} - end -rescue - ArgumentError -> {:error, :invalid_permission_set} -end -``` +**Implementation:** Prevented up front by a `Role` attribute validation that rejects any value not in `PermissionSets.all_permission_sets/0` (`"Invalid permission set name. Must be one of: ..."`). At runtime, every lookup goes through `permission_set_name_to_atom/1`, which rescues the `ArgumentError` from `String.to_existing_atom/1` (see PermissionSets above), so an invalid name yields `{:error, :invalid_permission_set}`. **Result:** Invalid `permission_set_name` → authorization fails → forbidden (safe default). @@ -2054,158 +1074,41 @@ Users and Members are separate entities that can be linked. Special rules: - **User side:** The User resource restricts the `update_user` action (which accepts the `member` argument for link/unlink) to admins only via `Mv.Authorization.Checks.ActorIsAdmin`. The UserLive.Form shows the Member-Linking UI and runs member link/unlink on save only when the current user is admin; non-admins use the `:update` action (email only) for profile edit. - **Member side:** Only admins may set or change the user–member link on **Member** create or update. When creating or updating a member, the `:user` argument (which links the member to a user account) is forbidden for non-admins. This is enforced by `Mv.Authorization.Checks.ForbidMemberUserLinkUnlessAdmin` in the Member resource policies (`forbid_if` before `authorize_if HasPermission`). Non-admins can still create and update members as long as they do **not** pass the `:user` argument. The Member resource uses **`on_missing: :ignore`** for the `:user` relationship on update_member, so **omitting** `:user` from params does **not** change the link (no "unlink by omission"); unlink is only possible by explicitly passing `:user` (e.g. `user: nil`), which is admin-only. -### Approach: Separate Ash Actions +### Approach: One Pair of Actions Plus an Admin-Only `:user` Argument -We use **different Ash actions** to enforce different policies: - -1. **`create_member_for_self`** - User creates member and links to themselves -2. **`create_member`** - Admin creates member for any user (or unlinked) -3. **`link_member_to_user`** - Admin links existing member to user -4. **`unlink_member_from_user`** - Admin removes user link -5. **`update`** - Standard update (cannot change `user_id`) +Linking is **not** modelled as separate per-operation actions. The `Mv.Membership.Member` +resource (`lib/membership/member.ex`) exposes the actions `create_member`, `update_member`, +`set_vereinfacht_contact_id`, `search`, and `available_for_linking` (plus the default +`:read`/`:destroy`). Linking and unlinking happen through the optional **`:user` argument** on +`create_member` / `update_member`, not through dedicated `link_*`/`unlink_*` actions. (`user_id` +is deliberately **not** in the accept list, so the foreign key cannot be set directly.) ### Implementation -```elixir -defmodule Mv.Membership.Member do - use Ash.Resource, ... - - actions do - # SELF-SERVICE: User creates member and links to self - create :create_member_for_self do - description "User creates a new member and links it to their own account" - - accept [:name, :email, :address, ...] # All fields except user_id - - # Automatically set user_id to actor - change set_attribute(:user_id, actor(:id)) - - # Prevent creating multiple members for same user (optional business rule) - validate fn changeset, _context -> - actor_id = get_change(changeset, :user_id) - - case Ash.read(Member, filter: expr(user_id == ^actor_id)) do - {:ok, []} -> :ok # No existing member, allow - {:ok, [_member | _]} -> {:error, "You already have a member profile"} - {:error, _} -> :ok - end - end - end +The user–member link is governed by two facts about `create_member` / `update_member`: - # ADMIN: Create member with optional user link - create :create_member do - description "Admin creates a new member, optionally linked to a user" - - accept [:name, :email, :address, ..., :user_id] # Admin can set user_id - end +- The `:user` argument drives the relationship via `manage_relationship(:user, ...)` with + `on_lookup: :relate`, `on_no_match: :error`, `on_match: :error`, and **`on_missing: :ignore`**. + Because of `on_missing: :ignore`, **omitting** `:user` leaves the link unchanged (no "unlink by + omission"); unlink is explicit (`user: nil`/`user: %{}`), handled on update via the + `UnrelateUserWhenArgumentNil` change. +- Whether the `:user` argument may be used at all is gated by the policy check + `Mv.Authorization.Checks.ForbidMemberUserLinkUnlessAdmin` (`forbid_if` before + `authorize_if HasPermission` on `action_type([:create, :update])`). It forbids the action for a + non-admin whenever the `:user` argument **key is present** (any value), so only admins may set or + change the link. Non-admins can still create/update members as long as they do not pass `:user`. - # ADMIN: Link existing member to user - update :link_member_to_user do - description "Admin links an existing member to a user account" - - accept [:user_id] - - validate fn changeset, _context -> - member = changeset.data - - # Cannot link if already linked - if is_nil(member.user_id) do - :ok - else - {:error, "Member is already linked to a user"} - end - end - end +Self-service ("a user creates a member and is linked to it") is handled on the **User** side, not +by a special Member action: the admin-only `update_user` action takes a `:member` argument for +link/unlink (see Enforcement above), and the UI gates the linking controls on admin status. - # ADMIN: Remove user link from member - update :unlink_member_from_user do - description "Admin removes user link from member" - - change set_attribute(:user_id, nil) - end +### Why This Design? - # STANDARD UPDATE: Cannot change user_id - update :update do - description "Update member data (cannot change user link)" - - accept [:name, :email, :address, ...] # user_id NOT in accept list - end - end - - policies do - # Self-service member creation - policy action(:create_member_for_self) do - description "Any authenticated user can create member for themselves" - authorize_if actor_present() - end - - # Admin-only actions - policy action([:create_member, :link_member_to_user, :unlink_member_from_user]) do - description "Only admin can manage user-member links" - authorize_if Mv.Authorization.Checks.HasPermission - end - - # Standard actions (regular permission check) - policy action([:read, :update, :destroy]) do - authorize_if Mv.Authorization.Checks.HasPermission - end - end -end -``` - -### UI Examples - -**User Self-Service:** - -```heex - -<%= if is_nil(@current_user.member_id) do %> - <.link navigate="/members/new_for_self"> - Create My Member Profile - -<% end %> - - -<.simple_form for={@form} phx-submit="create_for_self"> - <.input field={@form[:name]} label="Name" /> - <.input field={@form[:email]} label="Email" /> - <.input field={@form[:address]} label="Address" /> - - - - <:actions> - <.button>Create My Profile - - -``` - -**Admin Interface:** - -```heex - -<%= if can?(@current_user, :link_member_to_user, @member) do %> - <%= if is_nil(@member.user_id) do %> - - <.form for={@link_form} phx-submit="link_to_user"> - <.input field={@link_form[:user_id]} type="select" label="Link to User" options={@users} /> - <.button>Link to User - - <% else %> - - <.button phx-click="unlink_from_user" phx-value-id={@member.id}> - Unlink from User (<%= @member.user.email %>) - - <% end %> -<% end %> -``` - -### Why Separate Actions? - -✅ **Clear Intent:** Action name communicates what's happening -✅ **Precise Policies:** Different policies for different operations -✅ **Better UX:** Separate UI flows for self-service vs. admin -✅ **Testable:** Each action can be tested independently -✅ **Idiomatic Ash:** Uses Ash's action system as designed +Keeping the link on a single `:user` argument (rather than a fan-out of `link_*`/`unlink_*` +actions) means there is exactly one create and one update path to reason about, the +admin-only rule lives in one reusable policy check (`ForbidMemberUserLinkUnlessAdmin`) instead of +being duplicated per action, and `user_id` can never be mass-assigned because it is not accepted — +only the argument-driven relationship management can change it. --- @@ -2244,58 +1147,12 @@ def get_permissions(:read_only) do end ``` -**Read Filtering via Ash Calculations:** +**Read filtering** via an Ash calculation that takes `allowed_fields` from PermissionSets and +`Map.take/2`s each record to those fields. **Write protection** via an update validation that +diffs `Map.keys(changeset.attributes)` against the allowed write fields and returns +`{:error, "You do not have permission to modify: ..."}` for any forbidden field. -```elixir -defmodule Mv.Membership.Member do - calculations do - calculate :filtered_fields, :map do - calculate fn members, context -> - actor = context[:actor] - - # Get allowed fields from PermissionSets - allowed_fields = get_allowed_read_fields(actor, "Member") - - # Filter fields - Enum.map(members, fn member -> - Map.take(member, allowed_fields) - end) - end - end - end -end -``` - -**Write Protection via Custom Validations:** - -```elixir -validations do - validate on: :update do - validate fn changeset, context -> - actor = context[:actor] - changed_fields = Map.keys(changeset.attributes) - - # Get allowed fields from PermissionSets - allowed_fields = get_allowed_write_fields(actor, "Member") - - # Check if any forbidden field is being changed - forbidden = Enum.reject(changed_fields, &(&1 in allowed_fields)) - - if Enum.empty?(forbidden) do - :ok - else - {:error, "You do not have permission to modify: #{Enum.join(forbidden, ", ")}"} - end - end - end -end -``` - -**Benefits:** -- ✅ No database schema changes -- ✅ Still uses hardcoded PermissionSets -- ✅ Granular control over sensitive fields -- ✅ Clear error messages +**Benefits:** No database schema changes, still uses hardcoded PermissionSets, granular control over sensitive fields, clear error messages. **Estimated Effort:** 2-3 weeks @@ -2368,15 +1225,9 @@ defmodule Mv.Authorization.PermissionCache do end ``` -**Benefits:** -- ✅ Runtime permission configuration -- ✅ More flexible than hardcoded -- ✅ Can add new permission sets without code changes +**Benefits:** Runtime permission configuration, more flexible than hardcoded, can add new permission sets without code changes. -**Trade-offs:** -- ⚠️ More complex (DB queries, cache, invalidation) -- ⚠️ Slightly slower (mitigated by cache) -- ⚠️ More testing needed +**Trade-offs:** More complex (DB queries, cache, invalidation), slightly slower (mitigated by cache), more testing needed. **Estimated Effort:** 3-4 weeks @@ -2393,12 +1244,24 @@ See [Migration Strategy](#migration-strategy) for detailed migration plan. ### Three-Phase Approach -**Phase 1: MVP (2-3 weeks) - CURRENT** +**Phase 1: MVP (2-3 weeks) - CURRENT** (shipped 2026-01-08, PR #346, closes #345) - Hardcoded PermissionSets module - `HasPermission` check reads from module - Role table with `permission_set_name` string - Zero DB queries for permission checks +**What's NOT in MVP (deferred to Phase 3):** +- `PermissionSetResource` database table +- `PermissionSetPage` database table +- ETS Permission Cache +- Database-backed dynamic permissions / runtime permission editing + +**MVP DB migration & rollback.** Issue #1 adds a single migration: create the `roles` table (`name` unique, `permission_set_name`, `is_system_role`, timestamps; indexes on `name` and `permission_set_name`) and add nullable `users.role_id` FK (`ON DELETE RESTRICT`) with its index. The migration is additive only — no existing table is modified destructively. The 5 roles are created by `priv/repo/seeds_bootstrap.exs`, and the `assign_mitglied_role_to_existing_users` migration assigns "Mitglied" to users without a role. + +Rollback options, in order of escalation: +1. **DB rollback:** the `down` migration drops the `users.role_id` index, removes the `role_id` column, and drops the `roles` table — `mix ecto.rollback --step 1`. Existing tables are untouched. +2. **Code rollback:** revert the commit and redeploy the previous version. + **Phase 2: Field-Level (2-3 weeks) - FUTURE** - Extend PermissionSets with `:fields` key - Ash Calculations for read filtering @@ -2432,60 +1295,13 @@ See [Migration Strategy](#migration-strategy) for detailed migration plan. ### Migration from MVP to Phase 3 -**Step-by-step:** - -1. **Create DB Tables** (1 day) - - Run migrations for `permission_sets`, `permission_set_resources`, `permission_set_pages` - - Add indexes - -2. **Seed from PermissionSets Module** (1 day) - - Script that reads from `PermissionSets.get_permissions/1` - - Inserts into new tables - - Verify data integrity - -3. **Create HasResourcePermission Check** (2 days) - - New check that queries DB - - Same logic as `HasPermission` but different data source - - Comprehensive tests - -4. **Implement ETS Cache** (2 days) - - Cache module - - Cache invalidation on updates - - Performance tests - -5. **Update Policies** (3 days) - - Replace `HasPermission` with `HasResourcePermission` in all resources - - Test each resource thoroughly - -6. **Update UI Helpers** (1 day) - - Modify `MvWeb.Authorization` to query DB - - Use cache for performance - -7. **Update Page Plug** (1 day) - - Modify `CheckPagePermission` to query DB - - Use cache - -8. **Integration Testing** (3 days) - - Full user journey tests - - Performance testing - - Load testing - -9. **Deploy to Staging** (1 day) - - Feature flag approach - - Run both systems in parallel - - Compare results - -10. **Deploy to Production** (1 day) - - Gradual rollout - - Monitor performance - - Rollback plan ready - -11. **Cleanup** (1 day) - - Remove old `HasPermission` check - - Remove `PermissionSets` module - - Update documentation - -**Total:** ~3-4 weeks +Sequence (~3-4 weeks): create the three permission tables + indexes; seed them from +`PermissionSets.get_permissions/1`; add a `HasResourcePermission` check that queries the DB +(same logic as `HasPermission`, different data source) backed by the ETS cache with +invalidation on update; swap `HasPermission` → `HasResourcePermission` in all resources and +point the UI helper + page plug at the DB/cache; integration + performance/load test; deploy +behind the feature flag (run both systems in parallel to compare) then gradually to production; +finally remove the old `HasPermission` check and `PermissionSets` module. --- @@ -2547,28 +1363,9 @@ See [Migration Strategy](#migration-strategy) for detailed migration plan. ### Audit Logging (Future) -**Not in MVP, but planned:** - -```elixir -defmodule Mv.Authorization.AuditLog do - def log_authorization_failure(actor, resource, action, reason) do - Ash.create!(AuditLog, %{ - user_id: actor.id, - resource: inspect(resource), - action: action, - outcome: "forbidden", - reason: reason, - ip_address: get_ip_address(), - timestamp: DateTime.utc_now() - }) - end -end -``` - -**Benefits:** -- Track suspicious authorization attempts -- Compliance (GDPR requires access logs) -- Debugging production issues +Not in MVP, but planned: persist authorization failures (user id, resource, action, outcome, +reason, IP, timestamp) to an `AuditLog` resource — for tracking suspicious attempts, GDPR access +logs, and production debugging. Currently failures are only `Logger`-logged. --- @@ -2577,11 +1374,9 @@ end ### Glossary - **Permission Set:** Named collection of permissions (e.g., "admin", "read_only") -- **Role:** Database entity linking users to permission sets -- **Scope:** Range of records permission applies to (:own, :linked, :all) +- **Role:** Database entity linking users to a permission set; **system role** cannot be deleted (`is_system_role=true`) +- **Scope:** Range of records a permission applies to (`:own`, `:linked`, `:all`) - **Actor:** Currently authenticated user in Ash context -- **Policy:** Ash authorization rule on a resource -- **System Role:** Role that cannot be deleted (is_system_role=true) - **Special Case:** Authorization rule that takes precedence over general permissions ### Resource Name Mapping @@ -2614,54 +1409,9 @@ These strings must match exactly in `PermissionSets` module. | User without role | Access denied everywhere | Seeds assign default role, runtime checks handle gracefully | | Invalid permission_set_name | Access denied | Validation on Role, runtime safety checks | | System role deletion | Forbidden | Validation prevents deletion if `is_system_role=true` | -| Linked member email | Admin-only edit | Custom validation in Member resource | +| Linked member email | Admin or linked user may edit | `Member.Validations.EmailChangePermission` | | Own credentials | Always accessible | Special policy before general check | -### Testing Checklist - -**For Each Resource:** -- [ ] All 5 roles tested (Mitglied, Vorstand, Kassenwart, Buchhaltung, Admin) -- [ ] All actions tested (read, create, update, destroy) -- [ ] All scopes tested (own, linked, all) -- [ ] Special cases tested -- [ ] Edge cases tested (nil role, invalid permission_set_name) - -**For UI:** -- [ ] Buttons/links show/hide correctly per role -- [ ] Page access controlled per role -- [ ] No broken links (all visible links are accessible) - -**Integration:** -- [ ] One complete user journey per role -- [ ] Cross-resource scenarios (e.g., Member -> CustomFieldValue) -- [ ] Special cases in context (e.g., linked member email during full edit flow) - -### Useful Commands - -```bash -# Run all authorization tests -mix test test/mv/authorization - -# Run integration tests -mix test test/integration - -# Run with coverage -mix test --cover - -# Generate migrations -mix ash.codegen - -# Run seeds -mix run priv/repo/seeds/authorization_seeds.exs - -# Check permission for user in IEx -iex> user = Mv.Accounts.get_user!("user-id") -iex> MvWeb.Authorization.can?(user, :create, Mv.Membership.Member) - -# Check page access in IEx -iex> MvWeb.Authorization.can_access_page?(user, "/members/new") -``` - --- ## Authorization Bootstrap Patterns diff --git a/docs/roles-and-permissions-implementation-plan.md b/docs/roles-and-permissions-implementation-plan.md index 95db031..74b8705 100644 --- a/docs/roles-and-permissions-implementation-plan.md +++ b/docs/roles-and-permissions-implementation-plan.md @@ -1,1663 +1,39 @@ -# Roles and Permissions - Implementation Plan (MVP) +# Roles and Permissions - Implementation Record (MVP) -**Version:** 2.0 (Clean Rewrite) -**Date:** 2025-01-13 -**Last Updated:** 2026-01-13 -**Status:** ✅ Implemented (2026-01-08, PR #346, closes #345) -**Related Documents:** -- [Overview](./roles-and-permissions-overview.md) - High-level concepts -- [Architecture](./roles-and-permissions-architecture.md) - Technical specification +**Status:** ✅ Implemented (2026-01-08, PR #346, closes #345) +**Related:** [Overview](./roles-and-permissions-overview.md) · [Architecture](./roles-and-permissions-architecture.md) ---- +> Historical record of how the MVP (Phase 1) of the hardcoded Roles & Permissions +> system was built. The architecture document is the canonical design reference; +> the DB migration/rollback steps and the "What's NOT in MVP" boundary now live in +> its [Migration Strategy](./roles-and-permissions-architecture.md#migration-strategy) +> section. -## Table of Contents +## How the MVP was built -- [Executive Summary](#executive-summary) -- [MVP Scope](#mvp-scope) -- [Implementation Strategy](#implementation-strategy) -- [Issue Breakdown](#issue-breakdown) - - [Sprint 1: Foundation](#sprint-1-foundation-week-1) - - [Sprint 2: Policies](#sprint-2-policies-week-2) - - [Sprint 3: Special Cases & Seeds](#sprint-3-special-cases--seeds-week-3) - - [Sprint 4: UI & Integration](#sprint-4-ui--integration-week-4) -- [Dependencies & Parallelization](#dependencies--parallelization) -- [Testing Strategy](#testing-strategy) -- [Migration & Rollback](#migration--rollback) -- [Risk Management](#risk-management) +The MVP shipped as **PR #346 (closes #345)** across four week-sized sprints, built +test-first (TDD) with the work split into 15 issues: ---- +- **Sprint 1 — Foundation:** `Role` Ash resource + `users.role_id` FK (#1); hardcoded + `PermissionSets` module with the 4 sets `own_data`/`read_only`/`normal_user`/`admin` (#2); + Role CRUD admin LiveViews (#3). +- **Sprint 2 — Policies:** `HasPermission` custom Ash policy check (#6); resource policies + for Member (#7), User (#8), CustomFieldValue (#9), CustomField (#10); page-permission + router plug (#11). Issues #7–#11 ran in parallel after #6. +- **Sprint 3 — Special cases & seeds:** linked-member email validation (#12); role seed + data + default-role assignment (#13). +- **Sprint 4 — UI & integration:** `MvWeb.Authorization` UI helper (`can?/3`, + `can_access_page?/2`) (#14); admin role-management UI (#15); applying UI authorization + to existing LiveViews + navbar (#16); per-role integration journey tests (#17). -## Executive Summary +The 5 seeded roles map to permission sets as: Mitglied → own_data (system role), +Vorstand → read_only, Kassenwart → normal_user, Buchhaltung → read_only, Admin → admin. -### Overview +Issues #4, #5, #18 (DB-backed permission tables and ETS cache) were intentionally +**not** built — see "What's NOT in MVP" in the architecture document. -This document defines the implementation plan for the **MVP (Phase 1)** of the Roles and Permissions system using **hardcoded Permission Sets** in an Elixir module. - -**Key Characteristics:** -- **15 issues total** (Issues #1-3, #6-17) -- **2-3 weeks duration** -- **180+ tests** -- **Test-Driven Development (TDD)** throughout -- **No database tables for permissions** - only `roles` table -- **Zero performance concerns** - all permission checks are in-memory function calls - -### What's NOT in MVP - -**Deferred to Phase 3 (Future):** -- Issue #4: `PermissionSetResource` database table -- Issue #5: `PermissionSetPage` database table -- Issue #18: ETS Permission Cache -- Database-backed dynamic permissions - -### The Four Permission Sets - -Hardcoded in `Mv.Authorization.PermissionSets` module: - -1. **own_data** - User can only access their own data (default for "Mitglied") -2. **read_only** - Read access to all members/custom field values (for "Vorstand", "Buchhaltung") -3. **normal_user** - Create/Read/Update members (no delete), full CRUD on properties (for "Kassenwart") -4. **admin** - Unrestricted access including user/role management (for "Admin") - -### The Five Roles - -Stored in database `roles` table, each referencing a `permission_set_name`: - -1. **Mitglied** → "own_data" (is_system_role=true, default) -2. **Vorstand** → "read_only" -3. **Kassenwart** → "normal_user" -4. **Buchhaltung** → "read_only" -5. **Admin** → "admin" - ---- - -## MVP Scope - -### What We're Building - -**Core Authorization System:** -- ✅ Hardcoded PermissionSets module with 4 permission sets -- ✅ Role database table and CRUD interface -- ✅ Custom Ash Policy Check (`HasPermission`) that reads from PermissionSets -- ✅ Policies on all resources (Member, User, CustomFieldValue, CustomField, Role, Group, MemberGroup, MembershipFeeType, MembershipFeeCycle) -- ✅ Page-level permissions via Phoenix Plug (including admin-only `/settings` and `/membership_fee_settings`) -- ✅ UI authorization helpers for conditional rendering -- ✅ Special case: Member email validation for linked users -- ✅ User role assignment: admin-only `role_id` in update_user; Last-Admin validation; role dropdown in User form when `can?(actor, :update, Role)` -- ✅ Seed data for 5 roles - -**Benefits of Hardcoded Approach:** -- **Speed:** 2-3 weeks vs. 4-5 weeks for DB-backed -- **Performance:** < 1 microsecond per check (pure function call) -- **Simplicity:** No cache, no DB queries, easy to reason about -- **Version Control:** All permission changes tracked in Git -- **Testing:** Deterministic, no DB setup needed - -**Clear Migration Path to Phase 3:** -- Architecture document defines exact DB schema for future -- HasPermission check can be swapped for DB-querying version -- Role->PermissionSet link remains unchanged - ---- - -## Implementation Strategy - -### Test-Driven Development - -**Every issue follows TDD:** -1. Write failing tests first -2. Implement minimum code to pass tests -3. Refactor if needed -4. All tests must pass before moving on - -**Test Types:** -- **Unit Tests:** Individual modules (PermissionSets, Policy checks, Helpers) -- **Integration Tests:** Cross-resource authorization, special cases -- **LiveView Tests:** UI rendering, page permissions -- **E2E Tests:** Complete user journeys (one per role) - -### Incremental Rollout - -**Feature Flag Approach:** -- Implement behind environment variable `ENABLE_RBAC` -- Default: `false` (existing auth remains active) -- Test thoroughly in staging -- Flip flag in production after validation -- Allows instant rollback if needed - -### Definition of Done (All Issues) - -- [ ] All acceptance criteria met -- [ ] All tests written and passing -- [ ] Code reviewed and approved -- [ ] Documentation updated -- [ ] No linter errors -- [ ] Manual testing completed -- [ ] Feature flag tested (on/off states) - ---- - -## Issue Breakdown - -### Sprint 1: Foundation (Week 1) - -#### Issue #1: Create Authorization Domain and Role Resource - -**Size:** M (2 days) -**Dependencies:** None -**Assignable to:** Backend Developer - -**Description:** - -Create the authorization domain in Ash with the `Role` resource. This establishes the foundation for all authorization logic. - -**Tasks:** - -1. Create `lib/mv/authorization/` directory -2. Create `lib/mv/authorization/role.ex` Ash resource with: - - `id` (UUIDv7, primary key) - - `name` (String, unique, required) - e.g., "Vorstand", "Admin" - - `description` (String, optional) - - `permission_set_name` (String, required) - must be one of: "own_data", "read_only", "normal_user", "admin" - - `is_system_role` (Boolean, default false) - prevents deletion - - timestamps -3. Add validation: `permission_set_name` must exist in `PermissionSets.all_permission_sets/0` -4. Add `role_id` (UUID, nullable, foreign key) to `users` table -5. Add `belongs_to :role` relationship in User resource -6. Run `mix ash.codegen` to generate migrations -7. Review and apply migrations - -**Acceptance Criteria:** - -- [ ] Role resource created with all fields -- [ ] Migration applied successfully -- [ ] User.role relationship works -- [ ] Validation prevents invalid `permission_set_name` -- [ ] `is_system_role` flag present - -**Test Strategy:** - -**Smoke Tests Only** (detailed behavior tests in later issues): - -- Role resource can be loaded via `Code.ensure_loaded?(Mv.Authorization.Role)` -- Migration created valid table (manually verify with `psql`) -- User resource can be loaded and has `:role` in `relationships()` - -**No extensive behavior tests** - those come in Issue #3 (Role CRUD). - -**Test File:** `test/mv/authorization/role_test.exs` (minimal smoke tests) - ---- - -#### Issue #2: PermissionSets Elixir Module (Hardcoded Permissions) - -**Size:** M (2 days) -**Dependencies:** None -**Can work in parallel:** Yes (parallel with #1) -**Assignable to:** Backend Developer - -**Description:** - -Create the core `PermissionSets` module that defines all four permission sets with their resource and page permissions. This is the heart of the MVP's authorization logic. - -**Tasks:** - -1. Create `lib/mv/authorization/permission_sets.ex` -2. Define module with `@moduledoc` explaining the 4 permission sets -3. Define types: - ```elixir - @type scope :: :own | :linked | :all - @type action :: :read | :create | :update | :destroy - @type resource_permission :: %{ - resource: String.t(), - action: action(), - scope: scope(), - granted: boolean() - } - @type permission_set :: %{ - resources: [resource_permission()], - pages: [String.t()] - } - ``` -4. Implement `get_permissions/1` for each of the 4 permission sets -5. Implement `all_permission_sets/0` returning `[:own_data, :read_only, :normal_user, :admin]` -6. Implement `valid_permission_set?/1` checking if name is in the list -7. Implement `permission_set_name_to_atom/1` with error handling -8. Add comprehensive `@doc` examples for each function - -**Permission Set Details:** - -**1. own_data (Mitglied):** -- Resources: - - User: read/update :own - - Member: read/update :linked - - CustomFieldValue: read/update :linked - - CustomField: read :all -- Pages: `["/", "/profile", "/members/:id"]` - -**2. read_only (Vorstand, Buchhaltung):** -- Resources: - - User: read :own, update :own - - Member: read :all - - CustomFieldValue: read :all - - CustomField: read :all -- Pages: `["/", "/members", "/members/:id", "/custom_field_values"]` - -**3. normal_user (Kassenwart):** -- Resources: - - User: read/update :own - - Member: read/create/update :all (no destroy for safety) - - CustomFieldValue: read/create/update/destroy :all - - CustomField: read :all -- Pages: `["/", "/members", "/members/new", "/members/:id", "/members/:id/edit", "/custom_field_values", "/custom_field_values/new", "/custom_field_values/:id/edit"]` - -**4. admin:** -- Resources: - - User: read/update/destroy :all - - Member: read/create/update/destroy :all - - CustomFieldValue: read/create/update/destroy :all - - CustomField: read/create/update/destroy :all - - Role: read/create/update/destroy :all -- Pages: `["*"]` (wildcard = all pages) - -**Acceptance Criteria:** - -- [ ] Module created with all 4 permission sets -- [ ] `get_permissions/1` returns correct structure for each set -- [ ] `valid_permission_set?/1` works for atoms and strings -- [ ] `permission_set_name_to_atom/1` handles errors gracefully -- [ ] All functions have `@doc` and `@spec` -- [ ] Code is readable and well-commented - -**Test Strategy (TDD):** - -**Structure Tests:** -- `get_permissions(:own_data)` returns map with `:resources` and `:pages` keys -- Each permission set returns list of resource permissions -- Each resource permission has required keys: `:resource`, `:action`, `:scope`, `:granted` -- Pages lists are non-empty (except potentially for restricted roles) - -**Permission Content Tests:** -- `:own_data` allows User read/update with scope :own -- `:own_data` allows Member/CustomFieldValue read/update with scope :linked -- `:read_only` allows Member/CustomFieldValue read with scope :all -- `:read_only` does NOT allow Member/CustomFieldValue create/update/destroy -- `:normal_user` allows Member/CustomFieldValue full CRUD with scope :all -- `:admin` allows everything with scope :all -- `:admin` has wildcard page permission "*" - -**Validation Tests:** -- `valid_permission_set?("own_data")` returns true -- `valid_permission_set?(:admin)` returns true -- `valid_permission_set?("invalid")` returns false -- `permission_set_name_to_atom("own_data")` returns `{:ok, :own_data}` -- `permission_set_name_to_atom("invalid")` returns `{:error, :invalid_permission_set}` - -**Edge Cases:** -- All 4 sets defined in `all_permission_sets/0` -- Function doesn't crash on nil input (returns false/error tuple) - -**Test File:** `test/mv/authorization/permission_sets_test.exs` - ---- - -#### Issue #3: Role CRUD LiveViews - -**Size:** M (3 days) -**Dependencies:** #1 (Role resource) -**Assignable to:** Backend Developer + Frontend Developer - -**Description:** - -Create LiveView interface for administrators to manage roles. Only admins should be able to access this. - -**Tasks:** - -1. Create `lib/mv_web/live/role_live/` directory -2. Implement `index.ex` - List all roles -3. Implement `show.ex` - View role details -4. Implement `form.ex` - Create/Edit role form component -5. Add routes in `router.ex` under `/admin` scope -6. Create table component showing: name, description, permission_set_name, is_system_role -7. Add form validation for `permission_set_name` (dropdown with 4 options) -8. Prevent deletion of system roles (UI + backend) -9. Add flash messages for success/error -10. Style with existing DaisyUI theme - -**Acceptance Criteria:** - -- [ ] Index page lists all roles -- [ ] Show page displays role details -- [ ] Form allows creating new roles -- [ ] Form allows editing non-system roles -- [ ] `permission_set_name` is dropdown (not free text) -- [ ] Cannot delete system roles (grayed out button + backend check) -- [ ] All CRUD operations work -- [ ] Routes are under `/admin/roles` - -**Test Strategy (TDD):** - -**LiveView Mount Tests:** -- Index page mounts successfully -- Index page loads all roles from database -- Show page mounts with valid role ID -- Show page returns 404 for invalid role ID - -**CRUD Operation Tests:** -- Create new role with valid data succeeds -- Create new role with invalid `permission_set_name` shows error -- Update role name succeeds -- Update system role's `permission_set_name` succeeds -- Delete non-system role succeeds -- Delete system role fails with error message - -**UI Rendering Tests:** -- Index page shows table with role names -- System roles have badge/indicator -- Delete button disabled for system roles -- Form dropdown shows all 4 permission sets -- Flash messages appear after actions - -**Test File:** `test/mv_web/live/role_live_test.exs` - ---- - -### Sprint 2: Policies (Week 2) - -#### Issue #6: Custom Policy Check - HasPermission - -**Size:** L (3-4 days) -**Dependencies:** #2 (PermissionSets), #3 (Role resource exists) -**Assignable to:** Senior Backend Developer - -**Description:** - -Create the core custom Ash Policy Check that reads permissions from the `PermissionSets` module and applies them to Ash queries. This is the bridge between hardcoded permissions and Ash's authorization system. - -**Tasks:** - -1. Create `lib/mv/authorization/checks/has_permission.ex` -2. Implement `use Ash.Policy.Check` -3. Implement `describe/1` - returns human-readable description -4. Implement `match?/3` - the core authorization logic: - - Extract `actor.role.permission_set_name` - - Convert to atom via `PermissionSets.permission_set_name_to_atom/1` - - Call `PermissionSets.get_permissions/1` - - Find matching permission for current resource + action - - Apply scope filter -5. Implement `apply_scope/3` helper: - - `:all` → `:authorized` (no filter) - - `:own` → `{:filter, expr(id == ^actor.id)}` - - `:linked` → resource-specific logic: - - Member: `{:filter, expr(user_id == ^actor.id)}` - - CustomFieldValue: `{:filter, expr(member.user_id == ^actor.id)}` (traverse relationship!) -6. Handle errors gracefully: - - No actor → `{:error, :no_actor}` - - No role → `{:error, :no_role}` - - Invalid permission_set_name → `{:error, :invalid_permission_set}` - - No matching permission → `{:error, :no_permission}` -7. Add logging for authorization failures (debug level) -8. Add comprehensive `@doc` with examples - -**Acceptance Criteria:** - -- [ ] Check module implements `Ash.Policy.Check` behavior -- [ ] `match?/3` correctly evaluates permissions from PermissionSets -- [ ] Scope filters work correctly (:all, :own, :linked) -- [ ] `:linked` scope handles Member and CustomFieldValue differently -- [ ] Errors are handled gracefully (no crashes) -- [ ] Authorization failures are logged -- [ ] Module is well-documented - -**Test Strategy (TDD):** - -**Permission Lookup Tests:** -- Actor with :admin permission_set has permission for all resources/actions -- Actor with :read_only permission_set has read permission for Member -- Actor with :read_only permission_set does NOT have create permission for Member -- Actor with :own_data permission_set has update permission for User with scope :own - -**Scope Application Tests - :all:** -- Actor with scope :all can access any record -- Query returns all records in database - -**Scope Application Tests - :own:** -- Actor with scope :own can access record where record.id == actor.id -- Actor with scope :own cannot access record where record.id != actor.id -- Query filters to only actor's own record - -**Scope Application Tests - :linked:** -- Actor with scope :linked can access Member where member.user_id == actor.id -- Actor with scope :linked can access CustomFieldValue where custom_field_value.member.user_id == actor.id (relationship traversal!) -- Actor with scope :linked cannot access unlinked member -- Query correctly filters based on user_id relationship - -**Error Handling Tests:** -- `match?` with nil actor returns `{:error, :no_actor}` -- `match?` with actor missing role returns `{:error, :no_role}` -- `match?` with invalid permission_set_name returns `{:error, :invalid_permission_set}` -- `match?` with no matching permission returns `{:error, :no_permission}` -- No crashes on edge cases - -**Logging Tests:** -- Authorization failure logs at debug level -- Log includes actor ID, resource, action, reason - -**Test Files:** -- `test/mv/authorization/checks/has_permission_test.exs` - ---- - -#### Issue #7: Member Resource Policies - -**Size:** M (2 days) -**Dependencies:** #6 (HasPermission check) -**Can work in parallel:** Yes (parallel with #8, #9, #10) -**Assignable to:** Backend Developer - -**Description:** - -Add authorization policies to the Member resource using the new `HasPermission` check. - -**Tasks:** - -1. Open `lib/mv/membership/member.ex` -2. Add `policies` block at top of resource (before actions) -3. Configure policy to `Mv.Authorization.Checks.HasPermission` -4. Add policy for each action: - - `:read` → check HasPermission for :read - - `:create` → check HasPermission for :create - - `:update` → check HasPermission for :update - - `:destroy` → check HasPermission for :destroy -5. Add special policy: Allow user to read/update their linked member (before general policy) - ```elixir - policy action_type(:read) do - authorize_if expr(user_id == ^actor(:id)) - end - ``` -6. Ensure policies load actor with `:role` relationship preloaded -7. Test policies with different actors - -**Policy Order (Critical!):** -1. Allow user to access their own linked member (most specific) -2. Check HasPermission (general authorization) -3. Default: Forbid - -**Acceptance Criteria:** - -- [ ] Policies block added to Member resource -- [ ] All CRUD actions protected by HasPermission -- [ ] Special case: User can always access linked member -- [ ] Policy order is correct (specific before general) -- [ ] Actor preloads :role relationship -- [ ] All policies tested - -**Test Strategy (TDD):** - -**Policy Tests for :own_data (Mitglied):** -- User can read their linked member (user_id matches) -- User can update their linked member -- User cannot read unlinked member (returns empty list or forbidden) -- User cannot create member -- Verify scope :linked works - -**Policy Tests for :read_only (Vorstand):** -- User can read all members (returns all records) -- User cannot create member (returns Forbidden) -- User cannot update any member (returns Forbidden) -- User cannot destroy any member (returns Forbidden) - -**Policy Tests for :normal_user (Kassenwart):** -- User can read all members -- User can create new member -- User can update any member -- User cannot destroy member (not in permission set) - -**Policy Tests for :admin:** -- User can perform all CRUD operations on any member -- No restrictions - -**Test File:** `test/mv/membership/member_policies_test.exs` - ---- - -#### Issue #8: User Resource Policies - -**Size:** M (2 days) -**Dependencies:** #6 (HasPermission check) -**Can work in parallel:** Yes (parallel with #7, #9, #10) -**Assignable to:** Backend Developer -**Status:** ✅ **COMPLETED** - -**Description:** - -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/accounts/user.ex` -2. ✅ Add `policies` block -3. ✅ Add AshAuthentication bypass (registration/login without actor) -4. ✅ ~~Add NoActor bypass (test environment only)~~ **REMOVED** - NoActor bypass was removed to prevent masking authorization bugs. All tests now use `system_actor`. -5. ✅ Add bypass for READ: Allow user to always read their own account - ```elixir - bypass action_type(:read) do - description "Users can always read their own account" - authorize_if expr(id == ^actor(:id)) - end - ``` -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. ✅ AshAuthentication bypass (registration/login) -2. ✅ Bypass: User can READ own account (id == actor.id) -3. ✅ HasPermission: General permission check (UPDATE uses scope :own, admin uses scope :all) -4. ✅ Default: Ash implicitly forbids (fail-closed) - -**Note:** NoActor bypass was removed. All tests now use `system_actor` for authorization. - -**Why Bypass for READ but not UPDATE?** - -- **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 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 → Bypass READ → HasPermission) -- ✅ Actor preloads :role relationship -- ✅ All tests pass (30/31 pass, 1 skipped) - -**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 use system_actor for authorization (NoActor bypass removed) -- ✅ Tests verify scope :own is used for UPDATE (not redundant) - ---- - -#### Issue #9: CustomFieldValue Resource Policies - -**Size:** M (2 days) -**Dependencies:** #6 (HasPermission check) -**Can work in parallel:** Yes (parallel with #7, #8, #10) -**Assignable to:** Backend Developer - -**Description:** - -Add authorization policies to the CustomFieldValue resource. CustomFieldValues are linked to members, which are linked to users. - -**Tasks:** - -1. Open `lib/mv/membership/custom_field_value.ex` -2. Add `policies` block -3. Add special policy: Allow user to read/update custom field values of their linked member - ```elixir - policy action_type([:read, :update]) do - authorize_if expr(member.user_id == ^actor(:id)) - end - ``` -4. Add general policy: Check HasPermission -5. Ensure CustomFieldValue preloads :member relationship for scope checks -6. Preload :role relationship for actor - -**Policy Order:** -1. Allow user to read/update properties of linked member -2. Check HasPermission -3. Default: Forbid - -**Acceptance Criteria:** - -- [ ] User can access properties of their linked member -- [ ] Policy traverses Member -> User relationship correctly -- [ ] HasPermission check works for other scopes -- [ ] Actor preloads :role relationship - -**Test Strategy (TDD):** - -**Linked CustomFieldValues Tests (:own_data):** -- User can read custom field values of their linked member -- User can update custom field values of their linked member -- User cannot read custom field values of unlinked members -- Verify relationship traversal works (custom_field_value.member.user_id) - -**Read-Only Tests:** -- User with :read_only can read all custom field values -- User with :read_only cannot create/update custom field values - -**Normal User Tests:** -- User with :normal_user can CRUD custom field values - -**Admin Tests:** -- Admin can perform all operations - -**Test File:** `test/mv/membership/custom_field_value_policies_test.exs` - ---- - -#### Issue #10: CustomField Resource Policies - -**Size:** S (1 day) -**Dependencies:** #6 (HasPermission check) -**Can work in parallel:** Yes (parallel with #7, #8, #9) -**Assignable to:** Backend Developer - -**Description:** - -Add authorization policies to the CustomField resource. CustomFields are admin-managed, but readable by all. - -**Tasks:** - -1. Open `lib/mv/membership/custom_field.ex` -2. Add `policies` block -3. Add read policy: All authenticated users can read (scope :all) -4. Add write policies: Only admin can create/update/destroy -5. Use HasPermission check - -**Acceptance Criteria:** - -- [ ] All users can read custom fields -- [ ] Only admin can create/update/destroy custom fields -- [ ] Policies tested - -**Test Strategy (TDD):** - -**Read Access (All Roles):** -- User with :own_data can read all custom fields -- User with :read_only can read all custom fields -- User with :normal_user can read all custom fields -- User with :admin can read all custom fields - -**Write Access (Admin Only):** -- Non-admin cannot create custom field (Forbidden) -- Non-admin cannot update custom field (Forbidden) -- Non-admin cannot destroy custom field (Forbidden) -- Admin can create custom field -- Admin can update custom field -- Admin can destroy custom field - -**Test File:** `test/mv/membership/custom_field_policies_test.exs` - ---- - -#### Issue #11: Page Permission Router Plug - -**Size:** S (1 day) -**Dependencies:** #2 (PermissionSets), #6 (HasPermission) -**Can work in parallel:** Yes (after #2 and #6) -**Assignable to:** Backend Developer - -**Description:** - -Create a Phoenix plug that checks if the current user has permission to access the requested page/route. This runs before LiveView mounts. - -**Tasks:** - -1. Create `lib/mv_web/plugs/check_page_permission.ex` -2. Implement `init/1` and `call/2` -3. Extract page path from `conn.private[:phoenix_route]` (route template like "/members/:id") -4. Get user from `conn.assigns[:current_user]` -5. Get user's role and permission_set_name -6. Call `PermissionSets.get_permissions/1` to get allowed pages list -7. Match requested path against allowed patterns: - - Exact match: "/members" == "/members" - - Dynamic match: "/members/:id" matches "/members/123" - - Wildcard: "*" matches everything (admin) -8. If unauthorized: redirect to "/" with flash error "You don't have permission to access this page." -9. If authorized: continue (conn not halted) -10. Add plug to router pipelines (`:browser`, `:require_authenticated_user`) - -**Acceptance Criteria:** - -- [ ] Plug checks page permissions from PermissionSets -- [ ] Static routes work ("/members") -- [ ] Dynamic routes work ("/members/:id" matches "/members/123") -- [ ] Wildcard works for admin ("*") -- [ ] Unauthorized users redirected with flash message -- [ ] Plug added to appropriate router pipelines - -**Test Strategy (TDD):** - -**Static Route Tests:** -- User with permission for "/members" can access (conn not halted) -- User without permission for "/members" is denied (conn halted, redirected to "/") -- Flash error message present after denial - -**Dynamic Route Tests:** -- User with "/members/:id" permission can access "/members/123" -- User with "/members/:id/edit" permission can access "/members/456/edit" -- User with only "/members/:id" cannot access "/members/123/edit" -- Pattern matching works correctly - -**Wildcard Tests:** -- Admin with "*" permission can access any page -- Wildcard overrides all other checks - -**Unauthenticated User Tests:** -- Nil current_user is redirected to login -- Login redirect preserves attempted path (optional feature) - -**Error Handling Tests:** -- User with invalid permission_set_name is denied -- User with no role is denied -- Error is logged but user sees generic message - -**Test File:** `test/mv_web/plugs/check_page_permission_test.exs` - ---- - -### Sprint 3: Special Cases & Seeds (Week 3) - -#### Issue #12: Member Email Validation for Linked Members - -**Size:** M (2 days) -**Dependencies:** #7 (Member policies), #8 (User policies) -**Assignable to:** Backend Developer - -**Description:** - -Implement special validation: Only admins can edit a member's email if that member is linked to a user. This prevents breaking email synchronization. - -**Tasks:** - -1. Open `lib/mv/membership/member.ex` -2. Add custom validation in `validations` block: - ```elixir - validate changing(:email), on: :update do - validate &validate_email_change_permission/2 - end - ``` -3. Implement `validate_email_change_permission/2`: - - Check if member has `user_id` (is linked) - - If linked: Check if actor has User.update permission with scope :all (admin) - - If not admin: Return error "Only administrators can change email for members linked to users" - - If not linked: Allow change -4. Use `PermissionSets.get_permissions/1` to check admin status -5. Add tests for all cases - -**Acceptance Criteria:** - -- [ ] Non-admin can edit email of unlinked member -- [ ] Non-admin cannot edit email of linked member -- [ ] Admin can edit email of linked member -- [ ] Validation only runs when email changes -- [ ] Error message is clear and helpful - -**Test Strategy (TDD):** - -**Unlinked Member Tests:** -- User with :normal_user can update email of unlinked member -- User with :read_only cannot update email (caught by policy, not validation) -- Validation doesn't block if member.user_id is nil - -**Linked Member Tests:** -- User with :normal_user cannot update email of linked member (validation error) -- Error message mentions "administrators" and "linked to users" -- User with :admin can update email of linked member (validation passes) - -**No-Op Tests:** -- Validation doesn't run if email didn't change -- Updating other fields (name, address) works normally - -**Test File:** `test/mv/membership/member_email_validation_test.exs` - ---- - -#### Issue #13: Seed Data - Roles and Default Assignment - -**Size:** S (1 day) -**Dependencies:** #2 (PermissionSets), #3 (Role resource) -**Can work in parallel:** Yes (parallel with #12 after #2 and #3 complete) -**Assignable to:** Backend Developer - -**Description:** - -Create seed data for 5 roles and assign default "Mitglied" role to existing users. Optionally designate one admin via environment variable. - -**Tasks:** - -1. Create `priv/repo/seeds/authorization_seeds.exs` -2. Seed 5 roles using `Ash.Seed.seed!/2` or create actions: - - **Mitglied:** name="Mitglied", description="Default member role", permission_set_name="own_data", is_system_role=true - - **Vorstand:** name="Vorstand", description="Board member with read access", permission_set_name="read_only", is_system_role=false - - **Kassenwart:** name="Kassenwart", description="Treasurer with full member management", permission_set_name="normal_user", is_system_role=false - - **Buchhaltung:** name="Buchhaltung", description="Accounting with read access", permission_set_name="read_only", is_system_role=false - - **Admin:** name="Admin", description="Administrator with full access", permission_set_name="admin", is_system_role=false -3. Make idempotent: Use upsert logic (get by name, update if exists, create if not) -4. Assign "Mitglied" role to all users without role_id: - ```elixir - mitglied_role = Ash.get!(Role, name: "Mitglied") - users_without_role = Ash.read!(User, filter: expr(is_nil(role_id))) - Enum.each(users_without_role, fn user -> - Ash.update!(user, %{role_id: mitglied_role.id}) - end) - ``` -5. (Optional) Check for `ADMIN_EMAIL` env var, assign Admin role to that user -6. Add error handling with clear error messages -7. Add `IO.puts` statements to show progress - -**Acceptance Criteria:** - -- [ ] All 5 roles created with correct permission_set_name -- [ ] "Mitglied" has is_system_role=true -- [ ] Existing users without role get "Mitglied" role -- [ ] Optional: ADMIN_EMAIL user gets Admin role -- [ ] Seeds are idempotent (can run multiple times) -- [ ] Error messages are clear -- [ ] Progress is logged to console - -**Test Strategy (TDD):** - -**Role Creation Tests:** -- After running seeds, 5 roles exist -- Each role has correct permission_set_name: - - Mitglied → "own_data" - - Vorstand → "read_only" - - Kassenwart → "normal_user" - - Buchhaltung → "read_only" - - Admin → "admin" -- "Mitglied" role has is_system_role=true -- Other roles have is_system_role=false -- All permission_set_names are valid (exist in PermissionSets.all_permission_sets/0) - -**User Assignment Tests:** -- Users without role_id are assigned "Mitglied" role -- Users who already have role_id are not changed -- Count of users with "Mitglied" role increases by number of previously unassigned users - -**Idempotency Tests:** -- Running seeds twice doesn't create duplicate roles -- Each role name appears exactly once -- Running seeds twice doesn't reassign users who already have roles - -**Optional Admin Tests:** -- If ADMIN_EMAIL set, user with that email gets Admin role -- If ADMIN_EMAIL not set, no error occurs -- If email doesn't exist, error is logged but seeds continue - -**Error Handling Tests:** -- Seeds fail gracefully if invalid permission_set_name provided -- Error message indicates which permission_set_name is invalid - -**Test File:** `test/seeds/authorization_seeds_test.exs` - ---- - -### Sprint 4: UI & Integration (Week 4) - -#### Issue #14: UI Authorization Helper Module - -**Size:** M (2-3 days) -**Dependencies:** #2 (PermissionSets), #6 (HasPermission), #13 (Seeds - for testing) -**Assignable to:** Backend Developer + Frontend Developer - -**Description:** - -Create helper functions for UI-level authorization checks. These will be used in LiveView templates to conditionally render buttons, links, and sections based on user permissions. - -**Tasks:** - -1. Create `lib/mv_web/authorization.ex` -2. Implement `can?/3` for resource-level checks: - ```elixir - def can?(user, action, resource) when is_atom(resource) - # Returns true if user has permission for action on resource - # e.g., can?(current_user, :create, Mv.Membership.Member) - ``` -3. Implement `can?/3` for record-level checks: - ```elixir - def can?(user, action, %resource{} = record) - # Returns true if user has permission for action on specific record - # Applies scope checking (own, linked, all) - # e.g., can?(current_user, :update, member) - ``` -4. Implement `can_access_page?/2`: - ```elixir - def can_access_page?(user, page_path) - # Returns true if user's permission set includes page - # e.g., can_access_page?(current_user, "/members/new") - ``` -5. All functions use `PermissionSets.get_permissions/1` (same logic as HasPermission) -6. All functions handle nil user gracefully (return false) -7. Implement resource-specific scope checking (Member vs CustomFieldValue for :linked) -8. Add comprehensive `@doc` with template examples -9. Import helper in `mv_web.ex` `html_helpers` section - -**Acceptance Criteria:** - -- [ ] `can?/3` works for resource atoms -- [ ] `can?/3` works for record structs with scope checking -- [ ] `can_access_page?/2` matches page patterns correctly -- [ ] Nil user always returns false -- [ ] Invalid permission_set_name returns false (not crash) -- [ ] Helper imported in `mv_web.ex` -- [ ] Comprehensive documentation with examples - -**Test Strategy (TDD):** - -**can?/3 with Resource Atom:** -- Returns true when user has permission for resource+action -- Admin can create Member (returns true) -- Read-only cannot create Member (returns false) -- Nil user returns false - -**can?/3 with Record Struct - Scope :all:** -- Admin can update any member (returns true for any record) -- Normal user can update any member (scope :all) - -**can?/3 with Record Struct - Scope :own:** -- User can update own User record (record.id == user.id) -- User cannot update other User record (record.id != user.id) - -**can?/3 with Record Struct - Scope :linked:** -- User can update linked Member (member.user_id == user.id) -- User cannot update unlinked Member -- User can update CustomFieldValue of linked Member (custom_field_value.member.user_id == user.id) -- User cannot update CustomFieldValue of unlinked Member -- Scope checking is resource-specific (Member vs CustomFieldValue) - -**can_access_page?/2:** -- User with page in list can access (returns true) -- User without page in list cannot access (returns false) -- Dynamic routes match correctly ("/members/:id" matches "/members/123") -- Admin wildcard "*" matches any page -- Nil user returns false - -**Error Handling:** -- User without role returns false -- User with invalid permission_set_name returns false (no crash) -- Handles missing fields gracefully - -**Test File:** `test/mv_web/authorization_test.exs` - ---- - -#### Issue #15: Admin UI for Role Management - -**Size:** M (2 days) -**Dependencies:** #14 (UI Authorization Helper) -**Assignable to:** Frontend Developer - -**Description:** - -Update Role management LiveViews to use authorization helpers for conditional rendering. Add UI polish. - -**Tasks:** - -1. Open `lib/mv_web/live/role_live/index.ex` -2. Add authorization checks for "New Role" button: - ```heex - <%= if can?(@current_user, :create, Mv.Authorization.Role) do %> - <.link patch={~p"/admin/roles/new"}>New Role - <% end %> - ``` -3. Add authorization checks for "Edit" and "Delete" buttons in table -4. Gray out/hide "Delete" for system roles -5. Update `show.ex` to hide edit button if user can't update -6. Add role badge/pill for system roles -7. Add permission_set_name badge with color coding: - - own_data → gray - - read_only → blue - - normal_user → green - - admin → red -8. Test UI with different user roles - -**Acceptance Criteria:** - -- [ ] Only admin sees "New Role" button -- [ ] Only admin sees "Edit" and "Delete" buttons -- [ ] System roles have visual indicator -- [ ] Delete button hidden/disabled for system roles -- [ ] Permission set badges are color-coded -- [ ] UI tested with all role types - -**Test Strategy (TDD):** - -**Admin View:** -- Admin sees "New Role" button -- Admin sees "Edit" buttons for all roles -- Admin sees "Delete" buttons for non-system roles -- Admin does not see "Delete" button for system roles - -**Non-Admin View:** -- Non-admin does not see "New Role" button (redirected by page permission plug anyway) -- Non-admin cannot access /admin/roles (caught by plug) - -**Visual Tests:** -- System roles have badge -- Permission set names are color-coded -- UI renders correctly - -**Test File:** `test/mv_web/live/role_live_authorization_test.exs` - ---- - -#### Issue #16: Apply UI Authorization to Existing LiveViews - -**Size:** L (3 days) -**Dependencies:** #14 (UI Authorization Helper) -**Can work in parallel:** Yes (parallel with #15) -**Assignable to:** Frontend Developer - -**Description:** - -Update all existing LiveViews (Member, User, CustomFieldValue, CustomField) to use authorization helpers for conditional rendering. - -**Tasks:** - -1. **Member LiveViews:** - - Index: Hide "New Member" if can't create - - Index: Hide "Edit" and "Delete" buttons per record if can't update/destroy - - Show: Hide "Edit" button if can't update record - - Form: Should not be accessible (caught by page permission plug) - -2. **User LiveViews:** - - Index: Only show if user is admin - - Show: Only show other users if admin, always show own profile - - Edit: Only allow editing own profile or admin editing anyone - -3. **CustomFieldValue LiveViews:** - - Similar to Member (hide create/edit/delete based on permissions) - -4. **CustomField LiveViews:** - - All users can view - - Only admin can create/edit/delete - -5. **Navbar:** - - Only show "Admin" dropdown if user has admin permission set - - Only show "Roles" link if can access /admin/roles - - Only show "Members" link if can access /members - - Always show "Profile" link - -6. Test all views with all 5 role types - -**Acceptance Criteria:** - -- [ ] All LiveViews use `can?/3` for conditional rendering -- [ ] Buttons/links hidden when user lacks permission -- [ ] Navbar shows appropriate links per role -- [ ] Tested with all 5 roles (Mitglied, Vorstand, Kassenwart, Buchhaltung, Admin) -- [ ] UI is clean (no awkward empty spaces from hidden buttons) - -**Test Strategy (TDD):** - -**Member Index - Mitglied (own_data):** -- Does not see "New Member" button -- Does not see list of members (empty or filtered) -- Can only see own linked member if navigated directly - -**Member Index - Vorstand (read_only):** -- Sees full member list -- Does not see "New Member" button -- Does not see "Edit" or "Delete" buttons - -**Member Index - Kassenwart (normal_user):** -- Sees full member list -- Sees "New Member" button -- Sees "Edit" button for all members -- Does not see "Delete" button (not in permission set) - -**Member Index - Admin:** -- Sees everything (New, Edit, Delete) - -**Navbar Tests (all roles):** -- Mitglied: Sees only "Home" and "Profile" -- Vorstand: Sees "Home", "Members" (read-only), "Profile" -- Kassenwart: Sees "Home", "Members", "Properties", "Profile" -- Buchhaltung: Sees "Home", "Members" (read-only), "Profile" -- Admin: Sees "Home", "Members", "Custom Field Values", "Custom Fields", "Admin", "Profile" - -**Test Files:** -- `test/mv_web/live/member_live_authorization_test.exs` -- `test/mv_web/live/user_live_authorization_test.exs` -- `test/mv_web/live/custom_field_value_live_authorization_test.exs` -- `test/mv_web/live/custom_field_live_authorization_test.exs` -- `test/mv_web/components/navbar_authorization_test.exs` - ---- - -#### Issue #17: Integration Tests - Complete User Journeys - -**Size:** L (3 days) -**Dependencies:** All above (full system must be functional) -**Assignable to:** Backend Developer - -**Description:** - -Write comprehensive integration tests that follow complete user journeys for each role. These tests verify that policies, UI helpers, and page permissions all work together correctly. - -**Tasks:** - -1. Create test file for each role: - - `test/integration/mitglied_journey_test.exs` - - `test/integration/vorstand_journey_test.exs` - - `test/integration/kassenwart_journey_test.exs` - - `test/integration/buchhaltung_journey_test.exs` - - `test/integration/admin_journey_test.exs` - -2. Each test follows a complete user flow: - - Login as user with role - - Navigate to allowed pages - - Attempt to access forbidden pages - - Perform allowed actions - - Attempt forbidden actions - - Verify UI shows/hides appropriate elements - -3. Test cross-cutting concerns: - - Email synchronization (Member <-> User) - - User-Member linking (admin only) - - System role protection - -**Acceptance Criteria:** - -- [ ] One integration test per role (5 total) -- [ ] Tests cover complete user journeys -- [ ] Tests verify both backend (policies) and frontend (UI helpers) -- [ ] Tests verify page permissions -- [ ] Tests verify special cases (email, linking, system roles) -- [ ] All tests pass - -**Test Strategy:** - -**Mitglied Journey:** -1. Login as Mitglied user -2. Can access home page and profile -3. Cannot access /members (redirected) -4. Cannot access /admin/roles (redirected) -5. Can view own linked member via direct URL -6. Can update own member data -7. Cannot update unlinked member -8. Can update own user credentials -9. Cannot view other users - -**Vorstand Journey:** -1. Login as Vorstand user -2. Can access /members (reads all members) -3. Cannot create member (no button in UI, backend forbids) -4. Cannot edit member (no button in UI, backend forbids) -5. Can access /members/:id (read-only view) -6. Cannot access /members/:id/edit (page permission denies) -7. Can update own credentials -8. Cannot access /admin/roles - -**Kassenwart Journey:** -1. Login as Kassenwart user -2. Can access /members -3. Can create new member -4. Can edit any member (except email if linked - see special case) -5. Cannot delete member -6. Can manage properties -7. Cannot manage custom fields (read-only) -8. Cannot access /admin/roles - -**Buchhaltung Journey:** -1. Login as Buchhaltung user -2. Can access /members (read-only) -3. Cannot create/edit members -4. Can view properties (read-only) -5. Same restrictions as Vorstand - -**Admin Journey:** -1. Login as Admin user -2. Can access all pages (wildcard permission) -3. Can CRUD all resources -4. Can edit member email even if linked -5. Can manage roles -6. Cannot delete system roles (backend prevents) -7. Can link/unlink users and members -8. Can edit any user's credentials - -**Special Cases Tests:** -- Member email editing (admin vs non-admin for linked member) -- System role deletion (always fails) -- User without role (access denied everywhere) -- User with invalid permission_set_name (access denied) - -**Test Files:** -- `test/integration/mitglied_journey_test.exs` -- `test/integration/vorstand_journey_test.exs` -- `test/integration/kassenwart_journey_test.exs` -- `test/integration/buchhaltung_journey_test.exs` -- `test/integration/admin_journey_test.exs` -- `test/integration/special_cases_test.exs` - ---- - -## Dependencies & Parallelization - -### Dependency Graph - -``` - ┌──────────────────┐ - │ Issue #1 │ - │ Auth Domain │ - │ + Role Res │ - └────────┬─────────┘ - │ - ┌────────────┴────────────┐ - │ │ - ┌───────▼────────┐ ┌───────▼────────┐ - │ Issue #2 │ │ Issue #3 │ - │ PermissionSets│ │ Role CRUD │ - │ Module │ │ LiveViews │ - └───────┬────────┘ └────────────────┘ - │ - │ - └────────────┬────────────┘ - │ - ┌────────▼─────────┐ - │ Issue #6 │ - │ HasPermission │ - │ Policy Check │ - └────────┬─────────┘ - │ - ┌────────────────────┼─────────────────────┐ - │ │ │ - ┌────▼─────┐ ┌──────▼──────┐ ┌──────▼──────┐ - │ Issue #7 │ │ Issue #8 │ │ Issue #11 │ - │ Member │ │ User │ │ Page Plug │ - │ Policies │ │ Policies │ └──────┬──────┘ - └────┬─────┘ └──────┬──────┘ │ - │ │ │ - ┌────▼─────┐ ┌──────▼──────┐ │ - │ Issue #9 │ │ Issue #10 │ │ - │ CustomFieldValue │ │ CustomField │ │ - │ Policies │ │ Policies │ │ - └────┬─────┘ └──────┬──────┘ │ - │ │ │ - └────────────────────┴─────────────────────┘ - │ - ┌────────────┴────────────┐ - │ │ - ┌───────▼────────┐ ┌───────▼────────┐ - │ Issue #12 │ │ Issue #13 │ - │ Email Valid │ │ Seeds │ - └───────┬────────┘ └───────┬────────┘ - │ │ - └────────────┬────────────┘ - │ - ┌────────▼─────────┐ - │ Issue #14 │ - │ UI Helper │ - └────────┬─────────┘ - │ - ┌────────────┴────────────┐ - │ │ - ┌───────▼────────┐ ┌───────▼────────┐ - │ Issue #15 │ │ Issue #16 │ - │ Admin UI │ │ Apply UI Auth│ - └───────┬────────┘ └───────┬────────┘ - │ │ - └────────────┬────────────┘ - │ - ┌────────▼─────────┐ - │ Issue #17 │ - │ Integration │ - │ Tests │ - └──────────────────┘ -``` - -### Parallelization Opportunities - -**After Issue #1:** -- Issues #2 and #3 can run in parallel - -**After Issue #6:** -- Issues #7, #8, #9, #10, #11 can ALL run in parallel (5 issues!) -- This is the main parallelization opportunity - -**After Issues #7-#11:** -- Issues #12 and #13 can run in parallel - -**After Issue #14:** -- Issues #15 and #16 can run in parallel - -### Sprint Breakdown - -| Sprint | Issues | Duration | Can Parallelize | -|--------|--------|----------|-----------------| -| Sprint 1 | #1, #2, #3 | Week 1 | #2 and #3 after #1 | -| Sprint 2 | #6, #7, #8, #9, #10, #11 | Week 2 | #7-#11 after #6 (5 parallel!) | -| Sprint 3 | #12, #13 | Week 3 | Yes (2 parallel) | -| Sprint 4 | #14, #15, #16, #17 | Week 4 | #15 & #16 after #14 | - ---- - -## Testing Strategy - -### Test-Driven Development Process - -**For Every Issue:** -1. Read acceptance criteria -2. Write failing tests covering all criteria -3. Verify tests fail (red) -4. Implement minimum code to pass -5. Verify tests pass (green) -6. Refactor if needed -7. All tests still pass - -### Test Coverage Goals - -**Total Estimated Tests: 180+** - -| Test Type | Count | Coverage | -|-----------|-------|----------| -| Unit Tests | ~80 | PermissionSets module, Policy checks, Scope logic, UI helpers | -| Integration Tests | ~70 | Cross-resource authorization, Special cases, Email validation | -| LiveView Tests | ~25 | UI rendering, Page permissions, Conditional elements | -| E2E Journey Tests | ~5 | Complete user flows (one per role) | - -### What to Test (Focus on Behavior) - -**DO Test:** -- Permission lookups return correct results -- Policies allow/deny actions correctly -- Scope filters work (own, linked, all) -- UI elements show/hide based on permissions -- Page access is controlled -- Special cases work (email, system roles) -- Error handling (no crashes) - -**DON'T Test:** -- Database schema existence -- Table columns (Ash generates these) -- Implementation details -- Private functions (test through public API) - -### Test Files Structure - -``` -test/ -├── mv/ -│ └── authorization/ -│ ├── permission_sets_test.exs # Issue #2 -│ ├── role_test.exs # Issue #1 (smoke) -│ └── checks/ -│ └── has_permission_test.exs # Issue #6 -├── mv/accounts/ -│ └── user_policies_test.exs # Issue #8 -├── mv/membership/ -│ ├── member_policies_test.exs # Issue #7 -│ ├── member_email_validation_test.exs # Issue #12 -│ ├── custom_field_value_policies_test.exs # Issue #9 -│ └── custom_field_policies_test.exs # Issue #10 -├── mv_web/ -│ ├── authorization_test.exs # Issue #14 -│ ├── plugs/ -│ │ └── check_page_permission_test.exs # Issue #11 -│ └── live/ -│ ├── role_live_test.exs # Issue #3 -│ ├── role_live_authorization_test.exs # Issue #15 -│ ├── member_live_authorization_test.exs # Issue #16 -│ ├── user_live_authorization_test.exs # Issue #16 -│ ├── custom_field_value_live_authorization_test.exs # Issue #16 -│ └── custom_field_live_authorization_test.exs # Issue #16 -├── integration/ -│ ├── mitglied_journey_test.exs # Issue #17 -│ ├── vorstand_journey_test.exs # Issue #17 -│ ├── kassenwart_journey_test.exs # Issue #17 -│ ├── buchhaltung_journey_test.exs # Issue #17 -│ ├── admin_journey_test.exs # Issue #17 -│ └── special_cases_test.exs # Issue #17 -└── seeds/ - └── authorization_seeds_test.exs # Issue #13 -``` - ---- - -## Migration & Rollback - -### Database Migrations - -**Issue #1 creates one migration:** - -```elixir -# priv/repo/migrations/TIMESTAMP_add_authorization.exs -defmodule Mv.Repo.Migrations.AddAuthorization do - use Ecto.Migration - - def up do - # Create roles table - create table(:roles, primary_key: false) do - add :id, :binary_id, primary_key: true, default: fragment("gen_random_uuid()") - add :name, :string, null: false - add :description, :text - add :permission_set_name, :string, null: false - add :is_system_role, :boolean, default: false, null: false - - timestamps() - end - - create unique_index(:roles, [:name]) - create index(:roles, [:permission_set_name]) - - # Add role_id to users table - alter table(:users) do - add :role_id, references(:roles, type: :binary_id, on_delete: :restrict) - end - - create index(:users, [:role_id]) - end - - def down do - drop index(:users, [:role_id]) - - alter table(:users) do - remove :role_id - end - - drop table(:roles) - end -end -``` - -### Data Migration (Seeds) - -**After migration applied:** - -Run seeds to create roles and assign defaults: - -```bash -mix run priv/repo/seeds/authorization_seeds.exs -``` - -### Rollback Plan - -**If issues discovered in production:** - -1. **Immediate Rollback:** - - Set `ENABLE_RBAC=false` environment variable - - Restart application - - Old authorization system takes over instantly - -2. **Database Rollback (if needed):** - ```bash - mix ecto.rollback --step 1 - ``` - - Removes `role_id` from users - - Removes `roles` table - - Existing auth untouched - -3. **Code Rollback:** - - Revert Git commit - - Redeploy previous version - -**Rollback Safety:** -- No existing tables modified (only additions) -- Feature flag allows instant disable -- Old auth code remains in place until RBAC proven stable - ---- - -## Risk Management - -### Identified Risks - -| Risk | Probability | Impact | Mitigation | -|------|-------------|--------|------------| -| **Policy order issues** | Medium | High | Clear documentation, strict order enforcement, integration tests verify policies work together | -| **Scope filter errors** | Medium | High | TDD approach, extensive scope tests (own/linked/all), test with all resource types | -| **UI/Policy divergence** | Low | Medium | UI helpers use same PermissionSets module as policies, shared logic, integration tests verify consistency | -| **Breaking existing auth** | Low | High | Feature flag allows instant rollback, parallel systems until proven, gradual rollout | -| **User without role edge case** | Low | Medium | Default "Mitglied" role assigned in seeds, validation on User.create, tests cover nil role | -| **Invalid permission_set_name** | Low | Low | Validation on Role resource, tests cover invalid names, error handling throughout | -| **Performance (not a concern)** | Very Low | Low | Hardcoded permissions are < 1 microsecond, no DB queries, no cache needed | - -### Edge Cases Handled - -**User without role:** -- Default: Access denied (no permissions) -- Seeds assign "Mitglied" to all existing users -- New users must be assigned role on creation - -**Invalid permission_set_name:** -- Role validation prevents creation -- Runtime checks handle gracefully (return false/error, no crash) -- Error logged for debugging - -**System role protection:** -- Cannot delete role with `is_system_role=true` -- UI hides delete button -- Backend validation prevents deletion -- "Mitglied" is system role by default - -**Linked member email:** -- Custom validation on Member resource -- Only admins can edit if member.user_id present -- Prevents breaking email synchronization - -**Missing actor context:** -- All policies check for actor presence -- Missing actor = access denied -- No crashes, graceful error handling - -### Performance Considerations - -**No concerns for MVP:** -- Hardcoded permissions are pure function calls -- No database queries for permission checks -- Pattern matching on small lists (< 50 items total) -- Typical check: < 1 microsecond -- Can handle 10,000+ requests/second easily - -**Future considerations (Phase 3):** -- If migrating to database-backed: add ETS cache -- Cache invalidation on role/permission changes -- Database indexes on permission tables - ---- - -## Success Criteria - -**MVP is successful when:** - -- [ ] All 15 issues completed -- [ ] All 180+ tests passing -- [ ] Zero linter errors -- [ ] Manual testing completed for all 5 roles -- [ ] Integration tests verify complete user journeys -- [ ] Feature flag tested (on/off states) -- [ ] Documentation complete -- [ ] Code review approved -- [ ] Deployed to staging and verified -- [ ] Performance verified (< 100ms per page load) -- [ ] No authorization bypasses found in security review - -**Ready for Production when:** - -- [ ] 1 week in staging with no critical issues -- [ ] All stakeholders have tested their role types -- [ ] Rollback plan tested -- [ ] Monitoring/alerting configured -- [ ] Runbook created for common issues - ---- - -## Next Steps After MVP - -**Phase 2: Field-Level Permissions (Future - 2-3 weeks)** - -- Extend PermissionSets with `:fields` key -- Implement Ash Calculations to filter readable fields -- Implement Custom Validations for writable fields -- No database changes needed -- See [Architecture Document](./roles-and-permissions-architecture.md) for details - -**Phase 3: Database-Backed Permissions (Future - 3-4 weeks)** - -- Create `permission_sets`, `permission_set_resources`, `permission_set_pages` tables -- Replace hardcoded PermissionSets module with DB queries -- Implement ETS cache for performance -- Allow runtime permission configuration -- See [Architecture Document](./roles-and-permissions-architecture.md) for migration strategy - ---- - -## Document History - -| Version | Date | Author | Changes | -|---------|------|--------|---------| -| 1.0 | 2025-01-12 | AI Assistant | Initial version with DB-backed permissions | -| 2.0 | 2025-01-13 | AI Assistant | Complete rewrite for hardcoded MVP, removed all V1 references, fixed Buchhaltung inconsistency | - ---- - -## Appendix - -### Glossary - -- **Permission Set:** A named collection of resource and page permissions (e.g., "admin", "read_only") -- **Role:** A database entity that links users to a permission set -- **Scope:** The range of records a permission applies to (:own, :linked, :all) -- **Actor:** The currently authenticated user in Ash authorization context -- **System Role:** A role that cannot be deleted (is_system_role=true) - -### Key Files - -- `lib/mv/authorization/permission_sets.ex` - Core permissions logic -- `lib/mv/authorization/checks/has_permission.ex` - Ash policy check -- `lib/mv_web/authorization.ex` - UI helper functions -- `lib/mv_web/plugs/check_page_permission.ex` - Page access control -- `priv/repo/seeds/authorization_seeds.exs` - Role seed data - -### Useful Commands - -```bash -# Run all authorization tests -mix test test/mv/authorization - -# Run integration tests only -mix test test/integration - -# Run with coverage -mix test --cover - -# Generate migrations after Ash resource changes -mix ash.codegen - -# Run seeds -mix run priv/repo/seeds/authorization_seeds.exs - -# Check for linter errors -mix credo --strict -``` - ---- - -**End of Implementation Plan** +## Scope, migration & rollback +For the MVP scope boundary, the DB migration (create `roles`, add `users.role_id`), +the seed step, and the two-tier rollback plan (`mix ecto.rollback` → code revert), see +[Architecture › Migration Strategy](./roles-and-permissions-architecture.md#migration-strategy). diff --git a/docs/roles-and-permissions-overview.md b/docs/roles-and-permissions-overview.md index 13bf7cf..6331682 100644 --- a/docs/roles-and-permissions-overview.md +++ b/docs/roles-and-permissions-overview.md @@ -63,20 +63,7 @@ During the design phase, we evaluated multiple implementation approaches to find ### Approach 1: JSONB in Roles Table -Store all permissions as a single JSONB column directly in the roles table. - -**Advantages:** -- Simplest database schema (single table) -- Very flexible structure -- No additional tables needed -- Fast to implement - -**Disadvantages:** -- Poor queryability (can't efficiently filter by specific permissions) -- No referential integrity -- Difficult to validate structure -- Hard to audit permission changes -- Can't leverage database indexes effectively +Store all permissions as a single JSONB column directly in the roles table. Simplest schema (single table), flexible, fast to implement — but poor queryability (can't filter by specific permissions), no referential integrity, hard to validate/audit, can't use indexes. **Verdict:** Rejected - Poor queryability makes it unsuitable for complex permission logic. @@ -84,22 +71,7 @@ Store all permissions as a single JSONB column directly in the roles table. ### Approach 2: Normalized Database Tables -Separate tables for `permission_sets`, `permission_set_resources`, `permission_set_pages` with full normalization. - -**Advantages:** -- Fully queryable with SQL -- Runtime configurable permissions -- Strong referential integrity -- Easy to audit changes -- Can index for performance - -**Disadvantages:** -- Complex database schema (4+ tables) -- DB queries required for every permission check -- Requires ETS cache for performance -- Needs admin UI for permission management -- Longer implementation time (4-5 weeks) -- Overkill for fixed set of 4 permission sets +Separate tables for `permission_sets`, `permission_set_resources`, `permission_set_pages` with full normalization. Fully queryable, runtime-configurable, strong referential integrity, auditable, indexable — but complex schema (4+ tables), a DB query per check, needs ETS cache + admin UI, 4-5 weeks, overkill for 4 fixed sets. **Verdict:** Deferred to Phase 3 - Excellent for runtime configuration but too complex for MVP. @@ -107,20 +79,7 @@ Separate tables for `permission_sets`, `permission_set_resources`, `permission_s ### Approach 3: Custom Authorizer -Implement a custom Ash Authorizer from scratch instead of using Ash Policies. - -**Advantages:** -- Complete control over authorization logic -- Can implement any custom behavior -- Not constrained by Ash Policy DSL - -**Disadvantages:** -- Significantly more code to write and maintain -- Loses benefits of Ash's declarative policies -- Harder to test than built-in policy system -- Mixes declarative and imperative approaches -- Must reimplement filter generation for queries -- Higher bug risk +Implement a custom Ash Authorizer from scratch instead of using Ash Policies. Full control over logic — but significantly more code, loses Ash's declarative policies (must reimplement query filter generation), harder to test, mixes declarative/imperative, higher bug risk. **Verdict:** Rejected - Too much custom code, reduces maintainability and loses Ash ecosystem benefits. @@ -128,21 +87,7 @@ Implement a custom Ash Authorizer from scratch instead of using Ash Policies. ### Approach 4: Simple Role Enum -Add a simple `:role` enum field directly on User resource with hardcoded checks in each policy. - -**Advantages:** -- Very simple to implement (< 1 week) -- No extra tables needed -- Fast performance -- Easy to understand - -**Disadvantages:** -- No separation between roles and permissions -- Can't add new roles without code changes -- No dynamic permission configuration -- Not extensible to field-level permissions -- Violates separation of concerns (role = job function, not permission set) -- Difficult to maintain as requirements grow +Add a `:role` enum field directly on User with hardcoded checks in each policy. Very simple (< 1 week), no extra tables, fast — but no separation of role (job function) from permission set, can't add roles without code changes, no dynamic config, not extensible to field-level, hard to maintain as requirements grow. **Verdict:** Rejected - Too inflexible, doesn't meet requirement for configurable permissions and role separation. @@ -150,33 +95,11 @@ Add a simple `:role` enum field directly on User resource with hardcoded checks ### Approach 5: Hardcoded Permissions with Migration Path (SELECTED for MVP) -Permission Sets hardcoded in Elixir module, only Roles table in database. +Permission Sets hardcoded in Elixir module, only Roles table in database. Fast (2-3 weeks vs 4-5), maximum performance (zero DB queries, < 1μs), pure-function testing, Git-reviewable permissions, no data migration, keeps role/permission-set separation, clear Phase 3 upgrade path. Trade-offs: permissions not editable at runtime (only role assignment), new permissions need a code deploy, unsuitable if permissions change > 1x/week, limited to the 4 predefined sets. -**Advantages:** -- Fast implementation (2-3 weeks vs 4-5 weeks) -- Maximum performance (zero DB queries, < 1 microsecond) -- Simple to test (pure functions) -- Code-reviewable permissions (visible in Git) -- No migration needed for existing data -- Clearly defined 4 permission sets as required -- Clear migration path to database-backed solution (Phase 3) -- Maintains separation of roles and permission sets +**Why Selected:** MVP requires 4 fixed sets (not custom ones), no stated need for runtime permission editing, performance is critical, fast time-to-market, and a clear upgrade path exists when runtime config becomes necessary. -**Disadvantages:** -- Permissions not editable at runtime (only role assignment possible) -- New permissions require code deployment -- Not suitable if permissions change frequently (> 1x/week) -- Limited to the 4 predefined permission sets - -**Why Selected:** -- MVP requirement is for 4 fixed permission sets (not custom ones) -- No stated requirement for runtime permission editing -- Performance is critical for authorization checks -- Fast time-to-market (2-3 weeks) -- Clear upgrade path when runtime configuration becomes necessary - -**Migration Path:** -When runtime permission editing becomes a business requirement, migrate to Approach 2 (normalized DB tables) without changing the public API of the PermissionSets module. +**Migration Path:** When runtime permission editing becomes a business requirement, migrate to Approach 2 (normalized DB tables) without changing the public API of the PermissionSets module. --- @@ -201,7 +124,7 @@ When runtime permission editing becomes a business requirement, migrate to Appro **Resource Level (MVP):** - Controls create, read, update, destroy actions on resources -- Resources: Member, User, CustomFieldValue, CustomField, Role +- Resources: Member, User, CustomFieldValue, CustomField, Role, Group, MemberGroup, MembershipFeeType, MembershipFeeCycle, JoinRequest **Page Level (MVP):** - Controls access to LiveView pages @@ -214,7 +137,7 @@ When runtime permission editing becomes a business requirement, migrate to Appro ### Special Cases 1. **Own Credentials:** Users can always edit their own email and password -2. **Linked Member Email:** Only admins can edit email of members linked to users +2. **Linked Member Email:** Only administrators or the linked user themselves can change the email of a member linked to a user 3. **User-Member Linking:** Only admins can link/unlink users to members (except self-service creation) --- @@ -331,46 +254,39 @@ Users need to create member profiles for themselves (self-service), but only adm - Unlink members from users - Create members pre-linked to arbitrary users -### Selected Approach: Separate Ash Actions +### Selected Approach: Admin-Only `:user` Argument -Instead of complex field-level validation, we use action-based authorization. +Linking is **not** modelled as separate per-operation actions. The Member resource has a single +`create_member` and a single `update_member` action; linking and unlinking happen through an +optional **`:user` argument** on those actions. `user_id` is deliberately not accepted, so the +foreign key cannot be set directly. -### Actions on Member Resource +### How Linking Works on the Member Resource -**1. create_member_for_self** (All authenticated users) -- Automatically sets user_id = actor.id -- User cannot specify different user_id -- UI: "Create My Profile" button +**`create_member` / `update_member`** (the only Member write actions) +- The optional `:user` argument drives the relationship via `manage_relationship`. +- On update, `on_missing: :ignore` means omitting `:user` leaves the link unchanged + (no "unlink by omission"); unlink is explicit (`user: nil`). +- The policy check `ForbidMemberUserLinkUnlessAdmin` forbids the action for non-admins whenever the + `:user` argument is present (any value), so only admins may set or change the link. +- Non-admins can still create/update members as long as they do not pass `:user`. -**2. create_member** (Admin only) -- Can set user_id to any user or leave unlinked -- Full flexibility for admin -- UI: Admin member management form +**Self-service** ("a user creates a member linked to themselves") is handled on the **User** side: +the admin-only `update_user` action takes a `:member` argument for link/unlink, and the UI exposes +the linking controls only to admins. -**3. link_member_to_user** (Admin only) -- Updates existing member to set user_id -- Connects unlinked member to user account +### Why This Design? -**4. unlink_member_from_user** (Admin only) -- Sets user_id to nil -- Disconnects member from user account +**Single write path:** one create and one update action to reason about, instead of a fan-out of +`link_*`/`unlink_*` actions. -**5. update** (Permission-based) -- Normal updates (name, address, etc.) -- user_id NOT in accept list (prevents manipulation) -- Available to users with Member.update permission +**Centralized rule:** the admin-only constraint lives in one reusable policy check +(`ForbidMemberUserLinkUnlessAdmin`). -### Why Separate Actions? +**Server-Side Security:** `user_id` is never accepted directly, so it cannot be mass-assigned — +only argument-driven relationship management can change it. -**Explicit Semantics:** Each action has clear, single purpose - -**Server-Side Security:** user_id set by server, not client input - -**Better UX:** Different UI flows for different use cases - -**Simple Policies:** Authorization at action level, not field level - -**Easy Testing:** Each action independently testable +**Better UX:** distinct UI flows for self-service vs. admin linking. --- @@ -486,23 +402,7 @@ Use Custom Validations **[roles-and-permissions-architecture.md](./roles-and-permissions-architecture.md):** Complete technical specification with code examples -**[roles-and-permissions-implementation-plan.md](./roles-and-permissions-implementation-plan.md):** Detailed implementation plan with TDD approach +**[roles-and-permissions-implementation-plan.md](./roles-and-permissions-implementation-plan.md):** Historical record of how the MVP was built (PR #346/#345) **[CODE_GUIDELINES.md](../CODE_GUIDELINES.md):** Project coding standards ---- - -## Summary - -The selected architecture uses **hardcoded Permission Sets in Elixir** for the MVP, providing: -- **Speed:** 2-3 weeks implementation vs 4-5 weeks -- **Performance:** Zero database queries for authorization -- **Clarity:** Permissions in Git, reviewable and testable -- **Flexibility:** Clear migration path to database-backed system - -**User-Member linking** uses **separate Ash Actions** for clarity and security. - -**Field-level permissions** have a **defined strategy** (Calculations + Validations) for Phase 2 implementation. - -The approach balances pragmatism for MVP delivery with extensibility for future requirements. - diff --git a/docs/user-resource-policies-implementation-summary.md b/docs/user-resource-policies-implementation-summary.md deleted file mode 100644 index c939c6b..0000000 --- a/docs/user-resource-policies-implementation-summary.md +++ /dev/null @@ -1,269 +0,0 @@ -# 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. 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 - - # 3. HasPermission for all operations (uses scope from PermissionSets) - policy action_type([:read, :create, :update, :destroy]) do - description "Check permissions from user's role and permission set" - authorize_if Mv.Authorization.Checks.HasPermission - 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) -- ✅ Tests use system_actor for authorization - ---- - -## 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 -- ✅ All tests explicitly use system_actor for authorization - ---- - -## 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** 🎉