diff --git a/docs/email-sync.md b/docs/email-sync.md index c191ff4..2f765f0 100644 --- a/docs/email-sync.md +++ b/docs/email-sync.md @@ -4,6 +4,7 @@ 2. **DB constraints** - Prevent duplicates within same table (users.email, members.email) 3. **Custom validations** - Prevent cross-table conflicts only for linked entities 4. **Sync is bidirectional**: User ↔ Member (but User always wins on link) +5. **Linked member email change** - When a member is linked, only administrators or the linked user may change that member's email (Member resource validation `EmailChangePermission`). Because User.email wins on link and changes sync Member → User, allowing anyone to change a linked member's email would overwrite that user's account email; this rule keeps sync under control. --- diff --git a/lib/membership/member.ex b/lib/membership/member.ex index 7b49c86..8213ecb 100644 --- a/lib/membership/member.ex +++ b/lib/membership/member.ex @@ -25,6 +25,7 @@ defmodule Mv.Membership.Member do - Postal code format: exactly 5 digits (German format) - Date validations: join_date not in future, exit_date after join_date - Email uniqueness: prevents conflicts with unlinked users + - Linked member email change: only admins or the linked user may change a linked member's email (see `Mv.Membership.Member.Validations.EmailChangePermission`) ## Full-Text Search Members have a `search_vector` attribute (tsvector) that is automatically @@ -381,6 +382,9 @@ defmodule Mv.Membership.Member do # Validates that member email is not already used by another (unlinked) user validate Mv.Membership.Member.Validations.EmailNotUsedByOtherUser + # Only admins or the linked user may change a linked member's email (prevents breaking sync) + validate Mv.Membership.Member.Validations.EmailChangePermission, on: [:update] + # Prevent linking to a user that already has a member # This validation prevents "stealing" users from other members by checking # if the target user is already linked to a different member diff --git a/lib/mv/authorization/actor.ex b/lib/mv/authorization/actor.ex index 3482043..edc6b8b 100644 --- a/lib/mv/authorization/actor.ex +++ b/lib/mv/authorization/actor.ex @@ -1,6 +1,7 @@ defmodule Mv.Authorization.Actor do @moduledoc """ - Helper functions for ensuring User actors have required data loaded. + Helper functions for ensuring User actors have required data loaded + and for querying actor capabilities (e.g. admin, permission set). ## Actor Invariant @@ -27,8 +28,11 @@ defmodule Mv.Authorization.Actor do assign(socket, :current_user, user) end - # In tests - user = Actor.ensure_loaded(user) + # Check if actor is admin (policy checks, validations) + if Actor.admin?(actor), do: ... + + # Get permission set name (string or nil) + ps_name = Actor.permission_set_name(actor) ## Security Note @@ -47,6 +51,8 @@ defmodule Mv.Authorization.Actor do require Logger + alias Mv.Helpers.SystemActor + @doc """ Ensures the actor (User) has their `:role` relationship loaded. @@ -96,4 +102,45 @@ defmodule Mv.Authorization.Actor do actor end end + + @doc """ + Returns the actor's permission set name (string or atom) from their role, or nil. + + Ensures role is loaded (including when role is nil). Supports both atom and + string keys for session/socket assigns. Use for capability checks consistent + with `ActorIsAdmin` and `HasPermission`. + """ + @spec permission_set_name(Mv.Accounts.User.t() | map() | nil) :: String.t() | atom() | nil + def permission_set_name(nil), do: nil + + def permission_set_name(actor) do + actor = actor |> ensure_loaded() |> maybe_load_role() + + get_in(actor, [Access.key(:role), Access.key(:permission_set_name)]) || + get_in(actor, [Access.key("role"), Access.key("permission_set_name")]) + end + + @doc """ + Returns true if the actor is the system user or has the admin permission set. + + Use for validations and policy checks that require admin capability (e.g. + changing a linked member's email). Consistent with `ActorIsAdmin` policy check. + """ + @spec admin?(Mv.Accounts.User.t() | map() | nil) :: boolean() + def admin?(nil), do: false + + def admin?(actor) do + SystemActor.system_user?(actor) or permission_set_name(actor) in ["admin", :admin] + end + + # Load role only when it is nil (e.g. actor from session without role). ensure_loaded/1 + # already handles %Ash.NotLoaded{}, so we do not double-load in the normal Ash path. + defp maybe_load_role(%Mv.Accounts.User{role: nil} = user) do + case Ash.load(user, :role, domain: Mv.Accounts, authorize?: false) do + {:ok, loaded} -> loaded + _ -> user + end + end + + defp maybe_load_role(actor), do: actor end diff --git a/lib/mv/authorization/checks/actor_is_admin.ex b/lib/mv/authorization/checks/actor_is_admin.ex index 2328876..413c6c7 100644 --- a/lib/mv/authorization/checks/actor_is_admin.ex +++ b/lib/mv/authorization/checks/actor_is_admin.ex @@ -1,22 +1,18 @@ defmodule Mv.Authorization.Checks.ActorIsAdmin do @moduledoc """ - Policy check: true when the actor's role has permission_set_name "admin". + Policy check: true when the actor is the system user or has permission_set_name "admin". Used to restrict actions (e.g. User.update_user for member link/unlink) to admins only. + Delegates to `Mv.Authorization.Actor.admin?/1`, which returns true for the system actor + or for a user whose role has permission_set_name "admin". """ use Ash.Policy.SimpleCheck + alias Mv.Authorization.Actor + @impl true def describe(_opts), do: "actor has admin permission set" @impl true - def match?(nil, _context, _opts), do: false - - def match?(actor, _context, _opts) do - ps_name = - get_in(actor, [Access.key(:role), Access.key(:permission_set_name)]) || - get_in(actor, [Access.key("role"), Access.key("permission_set_name")]) - - ps_name == "admin" - end + def match?(actor, _context, _opts), do: Actor.admin?(actor) end diff --git a/lib/mv/email_sync/loader.ex b/lib/mv/email_sync/loader.ex index 98f85df..31e0468 100644 --- a/lib/mv/email_sync/loader.ex +++ b/lib/mv/email_sync/loader.ex @@ -3,13 +3,15 @@ defmodule Mv.EmailSync.Loader do Helper functions for loading linked records in email synchronization. Centralizes the logic for retrieving related User/Member entities. - ## Authorization + ## Authorization-independent link checks - This module runs systemically and uses the system actor for all operations. - This ensures that email synchronization always works, regardless of user permissions. - - All functions use `Mv.Helpers.SystemActor.get_system_actor/0` to bypass - user permission checks, as email sync is a mandatory side effect. + All functions use the **system actor** for the load. Link existence + (linked vs not linked) is therefore determined **independently of the + current request actor**. This is required so that validations (e.g. + `EmailChangePermission`, `EmailNotUsedByOtherUser`) can correctly decide + "member is linked" even when the current user would not have read permission + on the related User. Using the request actor would otherwise allow + treating a linked member as unlinked and bypass the permission rule. """ alias Mv.Helpers alias Mv.Helpers.SystemActor diff --git a/lib/mv/membership/member/validations/email_change_permission.ex b/lib/mv/membership/member/validations/email_change_permission.ex new file mode 100644 index 0000000..2b1c041 --- /dev/null +++ b/lib/mv/membership/member/validations/email_change_permission.ex @@ -0,0 +1,75 @@ +defmodule Mv.Membership.Member.Validations.EmailChangePermission do + @moduledoc """ + Validates that only admins or the linked user may change a linked member's email. + + This validation runs on member update when the email attribute is changing. + It allows the change only if: + - The member is not linked to a user, or + - The actor has the admin permission set (via `Mv.Authorization.Actor.admin?/1`), or + - The actor is the user linked to this member (actor.member_id == member.id). + + This prevents non-admins from changing another user's linked member email, + which would sync to that user's account and break email synchronization. + + Missing actor is not allowed; the system actor counts as admin (via `Actor.admin?/1`). + """ + use Ash.Resource.Validation + use Gettext, backend: MvWeb.Gettext, otp_app: :mv + + alias Mv.Authorization.Actor + alias Mv.EmailSync.Loader + + @doc """ + Validates that the actor may change the member's email when the member is linked. + + Only runs when the email attribute is changing (checked inside). Skips when + member is not linked. Allows when actor is admin or owns the linked member. + """ + @impl true + def validate(changeset, _opts, context) do + if Ash.Changeset.changing_attribute?(changeset, :email) do + validate_linked_member_email_change(changeset, context) + else + :ok + end + end + + defp validate_linked_member_email_change(changeset, context) do + linked_user = Loader.get_linked_user(changeset.data) + + if is_nil(linked_user) do + :ok + else + actor = resolve_actor(changeset, context) + member_id = changeset.data.id + + if Actor.admin?(actor) or actor_owns_member?(actor, member_id) do + :ok + else + msg = + dgettext( + "default", + "Only administrators or the linked user can change the email for members linked to users" + ) + + {:error, field: :email, message: msg} + end + end + end + + # Ash stores actor in changeset.context.private.actor; validation context has .actor; some callsites use context.actor + defp resolve_actor(changeset, context) do + ctx = changeset.context || %{} + + get_in(ctx, [:private, :actor]) || + Map.get(ctx, :actor) || + (context && Map.get(context, :actor)) + end + + defp actor_owns_member?(nil, _member_id), do: false + + defp actor_owns_member?(actor, member_id) do + actor_member_id = Map.get(actor, :member_id) || Map.get(actor, "member_id") + actor_member_id == member_id + end +end diff --git a/lib/mv/membership/member/validations/email_not_used_by_other_user.ex b/lib/mv/membership/member/validations/email_not_used_by_other_user.ex index f9fba1b..1ee8ab0 100644 --- a/lib/mv/membership/member/validations/email_not_used_by_other_user.ex +++ b/lib/mv/membership/member/validations/email_not_used_by_other_user.ex @@ -8,6 +8,8 @@ defmodule Mv.Membership.Member.Validations.EmailNotUsedByOtherUser do This allows creating members with the same email as unlinked users. """ use Ash.Resource.Validation + + alias Mv.EmailSync.Loader alias Mv.Helpers require Logger @@ -32,7 +34,8 @@ defmodule Mv.Membership.Member.Validations.EmailNotUsedByOtherUser do def validate(changeset, _opts, _context) do email_changing? = Ash.Changeset.changing_attribute?(changeset, :email) - linked_user_id = get_linked_user_id(changeset.data) + linked_user = Loader.get_linked_user(changeset.data) + linked_user_id = if linked_user, do: linked_user.id, else: nil is_linked? = not is_nil(linked_user_id) # Only validate if member is already linked AND email is changing @@ -76,16 +79,4 @@ defmodule Mv.Membership.Member.Validations.EmailNotUsedByOtherUser do defp maybe_exclude_id(query, nil), do: query defp maybe_exclude_id(query, id), do: Ash.Query.filter(query, id != ^id) - - defp get_linked_user_id(member_data) do - alias Mv.Helpers.SystemActor - - system_actor = SystemActor.get_system_actor() - opts = Helpers.ash_actor_opts(system_actor) - - case Ash.load(member_data, :user, opts) do - {:ok, %{user: %{id: id}}} -> id - _ -> nil - end - end end diff --git a/priv/gettext/de/LC_MESSAGES/default.po b/priv/gettext/de/LC_MESSAGES/default.po index 041507b..c4fd57d 100644 --- a/priv/gettext/de/LC_MESSAGES/default.po +++ b/priv/gettext/de/LC_MESSAGES/default.po @@ -2298,17 +2298,7 @@ msgstr "Verwende die Namen der Datenfelder als Spaltennamen in der CSV Datei. Da msgid "Unknown column '%{header}' will be ignored. If this is a custom field, create it in Mila before importing." msgstr "Unbekannte Spalte '%{header}' wird ignoriert. Falls dies ein Datenfeld ist, erstellen Sie es in Mila vor dem Import." -#~ #: lib/mv_web/live/global_settings_live.ex -#~ #, elixir-autogen, elixir-format, fuzzy -#~ msgid "Custom Fields in CSV Import" -#~ msgstr "Benutzerdefinierte Felder" - -#~ #: lib/mv_web/live/global_settings_live.ex -#~ #, elixir-autogen, elixir-format, fuzzy -#~ msgid "Individual data fields must be created in Mila before importing. Use the field name as the CSV column header. Unknown custom field columns will be ignored with a warning." -#~ msgstr "Individuelle Datenfelder müssen in Mila erstellt werden, bevor sie importiert werden können. Verwenden Sie den Namen des Datenfeldes als CSV-Spaltenüberschrift. Unbekannte Spaltenüberschriften werden mit einer Warnung ignoriert." - -#~ #: lib/mv_web/live/global_settings_live.ex -#~ #, elixir-autogen, elixir-format -#~ msgid "Manage Custom Fields" -#~ msgstr "Benutzerdefinierte Felder verwalten" +#: lib/mv/membership/member/validations/email_change_permission.ex +#, elixir-autogen, elixir-format, fuzzy +msgid "Only administrators or the linked user can change the email for members linked to users" +msgstr "Nur Administrator*innen oder die verknüpfte Benutzer*in können die E-Mail von Mitgliedern ändern, die mit Benutzer*innen verknüpft sind." diff --git a/priv/gettext/default.pot b/priv/gettext/default.pot index 2861f2d..0908fd8 100644 --- a/priv/gettext/default.pot +++ b/priv/gettext/default.pot @@ -2298,3 +2298,8 @@ msgstr "" #, elixir-autogen, elixir-format msgid "Unknown column '%{header}' will be ignored. If this is a custom field, create it in Mila before importing." msgstr "" + +#: lib/mv/membership/member/validations/email_change_permission.ex +#, elixir-autogen, elixir-format +msgid "Only administrators or the linked user can change the email for members linked to users" +msgstr "" diff --git a/priv/gettext/en/LC_MESSAGES/default.po b/priv/gettext/en/LC_MESSAGES/default.po index 3fe9ce3..6faa102 100644 --- a/priv/gettext/en/LC_MESSAGES/default.po +++ b/priv/gettext/en/LC_MESSAGES/default.po @@ -2299,17 +2299,7 @@ msgstr "" msgid "Unknown column '%{header}' will be ignored. If this is a custom field, create it in Mila before importing." msgstr "Unknown column '%{header}' will be ignored. If this is a custom field, create it in Mila before importing." -#~ #: lib/mv_web/live/global_settings_live.ex -#~ #, elixir-autogen, elixir-format, fuzzy -#~ msgid "Custom Fields in CSV Import" -#~ msgstr "" - -#~ #: lib/mv_web/live/global_settings_live.ex -#~ #, elixir-autogen, elixir-format, fuzzy -#~ msgid "Individual data fields must be created in Mila before importing. Use the field name as the CSV column header. Unknown custom field columns will be ignored with a warning." -#~ msgstr "" - -#~ #: lib/mv_web/live/global_settings_live.ex -#~ #, elixir-autogen, elixir-format -#~ msgid "Manage Custom Fields" -#~ msgstr "" +#: lib/mv/membership/member/validations/email_change_permission.ex +#, elixir-autogen, elixir-format, fuzzy +msgid "Only administrators or the linked user can change the email for members linked to users" +msgstr "Only administrators or the linked user can change the email for members linked to users" diff --git a/test/mv/membership/member_email_validation_test.exs b/test/mv/membership/member_email_validation_test.exs new file mode 100644 index 0000000..d1b5a10 --- /dev/null +++ b/test/mv/membership/member_email_validation_test.exs @@ -0,0 +1,236 @@ +defmodule Mv.Membership.MemberEmailValidationTest do + @moduledoc """ + Tests for Member email-change permission validation. + + When a member is linked to a user, only admins or the linked user may change + that member's email. Unlinked members and non-email updates are unaffected. + """ + use Mv.DataCase, async: false + + alias Mv.Accounts + alias Mv.Authorization + alias Mv.Helpers.SystemActor + alias Mv.Membership + + setup do + system_actor = SystemActor.get_system_actor() + %{actor: system_actor} + end + + defp create_role_with_permission_set(permission_set_name, actor) do + role_name = "Test Role #{permission_set_name} #{System.unique_integer([:positive])}" + + case Authorization.create_role( + %{ + name: role_name, + description: "Test role for #{permission_set_name}", + permission_set_name: permission_set_name + }, + actor: actor + ) do + {:ok, role} -> role + {:error, error} -> raise "Failed to create role: #{inspect(error)}" + end + end + + defp create_user_with_permission_set(permission_set_name, actor) do + role = create_role_with_permission_set(permission_set_name, actor) + + {:ok, user} = + Accounts.User + |> Ash.Changeset.for_create(:register_with_password, %{ + email: "user#{System.unique_integer([:positive])}@example.com", + password: "testpassword123" + }) + |> Ash.create(actor: actor) + + {:ok, user} = + user + |> Ash.Changeset.for_update(:update, %{}) + |> Ash.Changeset.manage_relationship(:role, role, type: :append_and_remove) + |> Ash.update(actor: actor) + + {:ok, user_with_role} = Ash.load(user, :role, domain: Mv.Accounts, actor: actor) + user_with_role + end + + defp create_admin_user(actor) do + create_user_with_permission_set("admin", actor) + end + + defp create_linked_member_for_user(user, actor) do + admin = create_admin_user(actor) + + {:ok, member} = + Membership.create_member( + %{ + first_name: "Linked", + last_name: "Member", + email: "linked#{System.unique_integer([:positive])}@example.com" + }, + actor: admin + ) + + user + |> Ash.Changeset.for_update(:update, %{}) + |> Ash.Changeset.force_change_attribute(:member_id, member.id) + |> Ash.update(actor: admin, domain: Mv.Accounts, return_notifications?: false) + + member + end + + defp create_unlinked_member(actor) do + admin = create_admin_user(actor) + + {:ok, member} = + Membership.create_member( + %{ + first_name: "Unlinked", + last_name: "Member", + email: "unlinked#{System.unique_integer([:positive])}@example.com" + }, + actor: admin + ) + + member + end + + describe "unlinked member" do + test "normal_user can update email of unlinked member", %{actor: actor} do + normal_user = create_user_with_permission_set("normal_user", actor) + unlinked_member = create_unlinked_member(actor) + + new_email = "new#{System.unique_integer([:positive])}@example.com" + + assert {:ok, updated} = + Membership.update_member(unlinked_member, %{email: new_email}, actor: normal_user) + + assert updated.email == new_email + end + + test "validation does not block when member has no linked user", %{actor: actor} do + normal_user = create_user_with_permission_set("normal_user", actor) + unlinked_member = create_unlinked_member(actor) + + new_email = "other#{System.unique_integer([:positive])}@example.com" + + assert {:ok, _} = + Membership.update_member(unlinked_member, %{email: new_email}, actor: normal_user) + end + end + + describe "linked member – another user's member" do + test "normal_user cannot update email of another user's linked member", %{actor: actor} do + user_a = create_user_with_permission_set("own_data", actor) + linked_member = create_linked_member_for_user(user_a, actor) + + normal_user_b = create_user_with_permission_set("normal_user", actor) + new_email = "other#{System.unique_integer([:positive])}@example.com" + + assert {:error, %Ash.Error.Invalid{} = error} = + Membership.update_member(linked_member, %{email: new_email}, actor: normal_user_b) + + assert Enum.any?(error.errors, &(&1.field == :email)), + "expected an error for field :email, got: #{inspect(error.errors)}" + end + + test "admin can update email of linked member", %{actor: actor} do + user_a = create_user_with_permission_set("own_data", actor) + linked_member = create_linked_member_for_user(user_a, actor) + admin = create_admin_user(actor) + + new_email = "admin_changed#{System.unique_integer([:positive])}@example.com" + + assert {:ok, updated} = + Membership.update_member(linked_member, %{email: new_email}, actor: admin) + + assert updated.email == new_email + end + end + + describe "linked member – own member" do + test "own_data user can update email of their own linked member", %{actor: actor} do + own_data_user = create_user_with_permission_set("own_data", actor) + linked_member = create_linked_member_for_user(own_data_user, actor) + + {:ok, own_data_user} = + Ash.get(Accounts.User, own_data_user.id, domain: Mv.Accounts, load: [:role], actor: actor) + + {:ok, own_data_user} = + Ash.load(own_data_user, :member, domain: Mv.Accounts, actor: actor) + + new_email = "own_updated#{System.unique_integer([:positive])}@example.com" + + assert {:ok, updated} = + Membership.update_member(linked_member, %{email: new_email}, actor: own_data_user) + + assert updated.email == new_email + end + + test "normal_user with linked member can update email of that same member", %{actor: actor} do + normal_user = create_user_with_permission_set("normal_user", actor) + linked_member = create_linked_member_for_user(normal_user, actor) + + {:ok, normal_user} = + Ash.get(Accounts.User, normal_user.id, domain: Mv.Accounts, load: [:role], actor: actor) + + {:ok, normal_user} = Ash.load(normal_user, :member, domain: Mv.Accounts, actor: actor) + + new_email = "normal_own#{System.unique_integer([:positive])}@example.com" + + assert {:ok, updated} = + Membership.update_member(linked_member, %{email: new_email}, actor: normal_user) + + assert updated.email == new_email + end + end + + describe "no-op / other fields" do + test "updating only other attributes on linked member as normal_user does not trigger validation error", + %{actor: actor} do + user_a = create_user_with_permission_set("own_data", actor) + linked_member = create_linked_member_for_user(user_a, actor) + normal_user_b = create_user_with_permission_set("normal_user", actor) + + assert {:ok, updated} = + Membership.update_member(linked_member, %{first_name: "UpdatedName"}, + actor: normal_user_b + ) + + assert updated.first_name == "UpdatedName" + assert updated.email == linked_member.email + end + + test "updating email of linked member as admin succeeds", %{actor: actor} do + user_a = create_user_with_permission_set("own_data", actor) + linked_member = create_linked_member_for_user(user_a, actor) + admin = create_admin_user(actor) + + new_email = "admin_ok#{System.unique_integer([:positive])}@example.com" + + assert {:ok, updated} = + Membership.update_member(linked_member, %{email: new_email}, actor: admin) + + assert updated.email == new_email + end + end + + describe "read_only" do + test "read_only cannot update any member (policy rejects before validation)", %{actor: actor} do + read_only_user = create_user_with_permission_set("read_only", actor) + linked_member = create_linked_member_for_user(read_only_user, actor) + + {:ok, read_only_user} = + Ash.get(Accounts.User, read_only_user.id, + domain: Mv.Accounts, + load: [:role], + actor: actor + ) + + assert {:error, %Ash.Error.Forbidden{}} = + Membership.update_member(linked_member, %{email: "changed@example.com"}, + actor: read_only_user + ) + end + end +end