Merge pull request 'Member Email Validation for Linked Members closes #397' (#399) from feature/397_emailsync_permission into main

Reviewed-on: #399

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.
 
 ---
 

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

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

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

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

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

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

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."

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 ""

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"

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