Fix unlink-by-omission: on_missing :ignore, test, doc, string-key
Some checks failed
continuous-integration/drone/push Build is failing
Some checks failed
continuous-integration/drone/push Build is failing
- Member update_member: on_missing :unrelate → :ignore (no unlink when :user omitted) - Test: normal_user update linked member without :user keeps link - Doc: unlink only explicit (user: nil), admin-only; Actor.admin?(nil) note - Check: defense-in-depth for "user" string key
This commit is contained in:
parent
543fded102
commit
5194b20b5c
4 changed files with 46 additions and 19 deletions
|
|
@ -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)
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue