diff --git a/docs/roles-and-permissions-architecture.md b/docs/roles-and-permissions-architecture.md index 0035a1e..216c6c9 100644 --- a/docs/roles-and-permissions-architecture.md +++ b/docs/roles-and-permissions-architecture.md @@ -2052,7 +2052,7 @@ Users and Members are separate entities that can be linked. Special rules: **Enforcement:** - **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 (e.g. normal_user / Kassenwart) can still create and update members as long as they do not pass the `:user` argument. +- **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 diff --git a/lib/membership/member.ex b/lib/membership/member.ex index fc007ac..06dbf57 100644 --- a/lib/membership/member.ex +++ b/lib/membership/member.ex @@ -154,15 +154,13 @@ defmodule Mv.Membership.Member do change manage_relationship(:custom_field_values, on_match: :update, on_no_match: :create) # Manage the user relationship during member update + # on_missing: :ignore so that omitting :user does NOT unlink (security: only admins may + # change the link; unlink is explicit via user: nil, forbidden for non-admins by policy). change manage_relationship(:user, :user, - # Look up existing user and relate to it on_lookup: :relate, - # Error if user doesn't exist in database on_no_match: :error, - # Error if user is already linked to another member (prevents "stealing") on_match: :error, - # If no user provided, remove existing relationship (allows user removal) - on_missing: :unrelate + on_missing: :ignore ) # Sync member email to user when email changes (Member → User) diff --git a/lib/mv/authorization/checks/forbid_member_user_link_unless_admin.ex b/lib/mv/authorization/checks/forbid_member_user_link_unless_admin.ex index ab4af9d..1e7cb77 100644 --- a/lib/mv/authorization/checks/forbid_member_user_link_unless_admin.ex +++ b/lib/mv/authorization/checks/forbid_member_user_link_unless_admin.ex @@ -6,16 +6,16 @@ defmodule Mv.Authorization.Checks.ForbidMemberUserLinkUnlessAdmin do **is present** (key in arguments, regardless of value), only admins may perform the action. This covers: - **Linking:** `user: %{id: user_id}` → only admin - - **Unlinking:** `user: nil` or `user: %{}` on update_member triggers `on_missing: :unrelate` → only admin - Non-admin users (e.g. normal_user / Kassenwart) can create and update members only when - they do **not** pass the `:user` argument at all. + - **Unlinking:** explicit `user: nil` or `user: %{}` on update_member → only admin + Non-admin users can create and update members only when they do **not** pass the + `:user` argument; omitting `:user` leaves the relationship unchanged. - ## Unlink via Member actions + ## Unlink semantics (update_member) - Unlink is intended via Member update_member: when `:user` is not provided in params, - manage_relationship uses `on_missing: :unrelate` and removes the link. Passing `user: nil` - or `user: %{}` explicitly is still "changing the link" and is forbidden for non-admins - (argument presence is checked, not value). + The Member resource uses `on_missing: :ignore` for the `:user` relationship on update. + 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 this + check forbids for non-admins. Admins may link or unlink via the `:user` argument. ## Usage @@ -30,7 +30,7 @@ defmodule Mv.Authorization.Checks.ForbidMemberUserLinkUnlessAdmin do - If the `:user` argument **key is not present** → does not forbid. - If `:user` is present (any value, including nil or %{}) and actor is not admin → forbids. - - If actor is nil → treated as non-admin (forbid when :user present); no crash. + - If actor is nil → treated as non-admin (forbid when :user present). `Actor.admin?(nil)` is defined and returns false. - If actor is admin (or system actor) → does not forbid. """ use Ash.Policy.Check @@ -42,7 +42,7 @@ defmodule Mv.Authorization.Checks.ForbidMemberUserLinkUnlessAdmin do @impl true def strict_check(actor, authorizer, _opts) do - # Defensive: nil actor → treat as non-admin (Actor.ensure_loaded(nil) and admin?(nil) are safe) + # Nil actor: treat as non-admin (Actor.admin?(nil) returns false; no crash) actor = if is_nil(actor), do: nil, else: Actor.ensure_loaded(actor) if user_argument_present?(authorizer) and not Actor.admin?(actor) do @@ -53,10 +53,10 @@ defmodule Mv.Authorization.Checks.ForbidMemberUserLinkUnlessAdmin do end # Forbid when :user was passed at all (link, unlink via nil/empty, or invalid value). - # Check argument key presence, not value, to avoid bypass via user: nil or user: %{}. + # Check argument key presence (atom or string) for defense-in-depth. defp user_argument_present?(authorizer) do - args = get_arguments(authorizer) - Map.has_key?(args || %{}, :user) + args = get_arguments(authorizer) || %{} + Map.has_key?(args, :user) or Map.has_key?(args, "user") end defp get_arguments(authorizer) do diff --git a/test/mv/membership/member_policies_test.exs b/test/mv/membership/member_policies_test.exs index d9ab95c..f2d3084 100644 --- a/test/mv/membership/member_policies_test.exs +++ b/test/mv/membership/member_policies_test.exs @@ -505,6 +505,35 @@ defmodule Mv.Membership.MemberPoliciesTest do Membership.update_member(linked_member, %{user: nil}, actor: normal_user) end + test "normal_user update linked member without :user keeps link", %{ + normal_user: normal_user, + admin: admin, + unlinked_member: unlinked_member + } do + # Admin links member to a user + link_target = + Mv.Fixtures.user_with_role_fixture("own_data") + |> Mv.Authorization.Actor.ensure_loaded() + + {:ok, linked_member} = + Membership.update_member( + unlinked_member, + %{user: %{id: link_target.id}}, + actor: admin + ) + + # normal_user updates only first_name (no :user) – link must remain (on_missing: :ignore) + {:ok, updated} = + Membership.update_member(linked_member, %{first_name: "Updated"}, actor: normal_user) + + assert updated.first_name == "Updated" + + {:ok, user} = + Ash.get(Mv.Accounts.User, link_target.id, domain: Mv.Accounts, actor: admin) + + assert user.member_id == updated.id + end + test "admin can create member with :user argument", %{admin: admin} do link_target = Mv.Fixtures.user_with_role_fixture("own_data")