From ab5c1d66399ab0ee1e979a65e095598920cc712b Mon Sep 17 00:00:00 2001 From: Moritz Date: Fri, 17 Oct 2025 14:21:23 +0200 Subject: [PATCH] feat: email uniqueness constraint between user and member --- lib/accounts/user.ex | 19 +- lib/membership/member.ex | 4 + .../email_not_used_by_other_member.ex | 52 +++++ .../email_not_used_by_other_user.ex | 59 +++++ test/accounts/email_uniqueness_test.exs | 201 ++++++++++++++++++ 5 files changed, 329 insertions(+), 6 deletions(-) create mode 100644 lib/mv/accounts/user/validations/email_not_used_by_other_member.ex create mode 100644 lib/mv/membership/member/validations/email_not_used_by_other_user.ex create mode 100644 test/accounts/email_uniqueness_test.exs diff --git a/lib/accounts/user.ex b/lib/accounts/user.ex index 7101e16..278e71a 100644 --- a/lib/accounts/user.ex +++ b/lib/accounts/user.ex @@ -66,14 +66,17 @@ defmodule Mv.Accounts.User do end actions do - # Default actions kept for framework/tooling integration: - # - :create -> Used by AshAdmin's generated "Create" UI and by generic - # AshPhoenix helpers that assume a default create action. - # It does NOT manage the :member relationship. For admin - # flows that may link an existing member, use :create_user. + # Default actions for framework/tooling integration: # - :read -> Standard read used across the app and by admin tooling. # - :destroy-> Standard delete used by admin tooling and maintenance tasks. - defaults [:read, :create, :destroy] + # + # NOTE: :create is INTENTIONALLY excluded from defaults! + # Using a default :create would bypass email-synchronization logic. + # Always use one of these explicit create actions instead: + # - :create_user (for manual user creation with optional member link) + # - :register_with_password (for password-based registration) + # - :register_with_rauthy (for OIDC-based registration) + defaults [:read, :destroy] # Primary generic update action: # - Selected by AshAdmin's generated "Edit" UI and generic AshPhoenix @@ -209,6 +212,10 @@ defmodule Mv.Accounts.User do where: [action_is([:register_with_password, :admin_set_password])], message: "must have length of at least 8" + # Email uniqueness check for all actions that change the email attribute + # Validates that user email is not already used by another (unlinked) member + validate Mv.Accounts.User.Validations.EmailNotUsedByOtherMember + # Email validation with EctoCommons.EmailValidator (same as Member) # This ensures consistency between User and Member email validation validate fn changeset, _ -> diff --git a/lib/membership/member.ex b/lib/membership/member.ex index 9330922..a0799fd 100644 --- a/lib/membership/member.ex +++ b/lib/membership/member.ex @@ -108,6 +108,10 @@ defmodule Mv.Membership.Member do validate present(:last_name) validate present(:email) + # Email uniqueness check for all actions that change the email attribute + # Validates that member email is not already used by another (unlinked) user + validate Mv.Membership.Member.Validations.EmailNotUsedByOtherUser + # 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/accounts/user/validations/email_not_used_by_other_member.ex b/lib/mv/accounts/user/validations/email_not_used_by_other_member.ex new file mode 100644 index 0000000..cf3c624 --- /dev/null +++ b/lib/mv/accounts/user/validations/email_not_used_by_other_member.ex @@ -0,0 +1,52 @@ +defmodule Mv.Accounts.User.Validations.EmailNotUsedByOtherMember do + @moduledoc """ + Validates that the user's email is not already used by another member + (unless that member is linked to this user). + + This prevents email conflicts when syncing between users and members. + """ + use Ash.Resource.Validation + + @impl true + def validate(changeset, _opts, _context) do + case Ash.Changeset.fetch_change(changeset, :email) do + {:ok, new_email} -> + check_email_not_used_by_other_member(changeset, new_email) + + :error -> + # Email not being changed + :ok + end + end + + defp check_email_not_used_by_other_member(changeset, new_email) do + member_id = Ash.Changeset.get_attribute(changeset, :member_id) + + # Check if any member has this email + # Exclude the member linked to this user (if any) + query = + Mv.Membership.Member + |> Ash.Query.filter(email == ^to_string(new_email)) + |> then(fn q -> + if member_id do + Ash.Query.filter(q, id != ^member_id) + else + q + end + end) + + case Ash.read(query) do + {:ok, []} -> + # No conflicting member found + :ok + + {:ok, members} when is_list(members) and length(members) > 0 -> + # Email is already used by another member + {:error, field: :email, message: "is already used by another member", value: new_email} + + {:error, _} -> + # Error reading members - be safe and allow + :ok + end + 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 new file mode 100644 index 0000000..f48613b --- /dev/null +++ b/lib/mv/membership/member/validations/email_not_used_by_other_user.ex @@ -0,0 +1,59 @@ +defmodule Mv.Membership.Member.Validations.EmailNotUsedByOtherUser do + @moduledoc """ + Validates that the member's email is not already used by another user + (unless that user is linked to this member). + + This prevents email conflicts when syncing between users and members. + """ + use Ash.Resource.Validation + + @impl true + def validate(changeset, _opts, _context) do + case Ash.Changeset.fetch_change(changeset, :email) do + {:ok, new_email} -> + check_email_not_used_by_other_user(changeset, new_email) + + :error -> + # Email not being changed + :ok + end + end + + defp check_email_not_used_by_other_user(changeset, new_email) do + # Load the user relationship to check if this member is linked to a user + member_with_user = + case Ash.load(changeset.data, :user) do + {:ok, loaded} -> loaded + {:error, _} -> changeset.data + end + + linked_user_id = if member_with_user.user, do: member_with_user.user.id, else: nil + + # Check if any user has this email (case-insensitive) + # Exclude the user linked to this member (if any) + query = + Mv.Accounts.User + |> Ash.Query.filter(email == ^new_email) + |> then(fn q -> + if linked_user_id do + Ash.Query.filter(q, id != ^linked_user_id) + else + q + end + end) + + case Ash.read(query) do + {:ok, []} -> + # No conflicting user found + :ok + + {:ok, users} when is_list(users) and length(users) > 0 -> + # Email is already used by another user + {:error, field: :email, message: "is already used by another user", value: new_email} + + {:error, _} -> + # Error reading users - be safe and allow + :ok + end + end +end diff --git a/test/accounts/email_uniqueness_test.exs b/test/accounts/email_uniqueness_test.exs new file mode 100644 index 0000000..8665c48 --- /dev/null +++ b/test/accounts/email_uniqueness_test.exs @@ -0,0 +1,201 @@ +defmodule Mv.Accounts.EmailUniquenessTest do + use Mv.DataCase, async: false + + alias Mv.Accounts + alias Mv.Membership + + describe "Email uniqueness validation" do + test "cannot create member with existing unlinked user email" do + # Create a user with email + {:ok, _user} = + Accounts.create_user(%{ + email: "existing@example.com" + }) + + # Try to create member with same email + result = + Membership.create_member(%{ + first_name: "John", + last_name: "Doe", + email: "existing@example.com" + }) + + assert {:error, %Ash.Error.Invalid{} = error} = result + + assert error.errors + |> Enum.any?(fn e -> + e.field == :email and + (String.contains?(e.message, "already") or String.contains?(e.message, "used")) + end) + end + + test "cannot create user with existing unlinked member email" do + # Create a member with email + {:ok, _member} = + Membership.create_member(%{ + first_name: "John", + last_name: "Doe", + email: "existing@example.com" + }) + + # Try to create user with same email + result = + Accounts.create_user(%{ + email: "existing@example.com" + }) + + assert {:error, %Ash.Error.Invalid{} = error} = result + + assert error.errors + |> Enum.any?(fn e -> + e.field == :email and + (String.contains?(e.message, "already") or String.contains?(e.message, "used")) + end) + end + + test "member email cannot be changed to an existing unlinked user email" do + # Create a user with email + {:ok, user} = + Accounts.create_user(%{ + email: "existing_user@example.com" + }) + + # Create a member with different email + {:ok, member} = + Membership.create_member(%{ + first_name: "John", + last_name: "Doe", + email: "member@example.com" + }) + + # Try to change member email to existing user email + result = + Membership.update_member(member, %{ + email: "existing_user@example.com" + }) + + assert {:error, %Ash.Error.Invalid{} = error} = result + + assert error.errors + |> Enum.any?(fn e -> + e.field == :email and + (String.contains?(e.message, "already") or String.contains?(e.message, "used")) + end) + end + + test "user email cannot be changed to an existing unlinked member email" do + # Create a member with email + {:ok, member} = + Membership.create_member(%{ + first_name: "John", + last_name: "Doe", + email: "existing_member@example.com" + }) + + # Create a user with different email + {:ok, user} = + Accounts.create_user(%{ + email: "user@example.com" + }) + + # Try to change user email to existing member email + result = + Accounts.update_user(user, %{ + email: "existing_member@example.com" + }) + + assert {:error, %Ash.Error.Invalid{} = error} = result + + assert error.errors + |> Enum.any?(fn e -> + e.field == :email and + (String.contains?(e.message, "already") or String.contains?(e.message, "used")) + end) + end + + test "member email syncs to linked user email without validation error" do + # Create a user + {:ok, user} = + Accounts.create_user(%{ + email: "user@example.com" + }) + + # Create a member linked to this user + # The override change will set member.email = user.email automatically + {:ok, member} = + Membership.create_member(%{ + first_name: "John", + last_name: "Doe", + email: "member@example.com", + user: %{id: user.id} + }) + + # Member email should have been overridden to user email + # This happens through our sync mechanism, which should NOT trigger + # the "email already used" validation because it's the same user + {:ok, member_after_link} = Ash.get(Mv.Membership.Member, member.id) + assert member_after_link.email == "user@example.com" + end + + test "user email syncs to linked member without validation error" do + # Create a member + {:ok, member} = + Membership.create_member(%{ + first_name: "John", + last_name: "Doe", + email: "member@example.com" + }) + + # Create a user linked to this member + # The override change will set member.email = user.email automatically + {:ok, user} = + Accounts.create_user(%{ + email: "user@example.com", + member: %{id: member.id} + }) + + # Member email should have been overridden to user email + # This happens through our sync mechanism, which should NOT trigger + # the "email already used" validation because it's the same member + {:ok, member_after_link} = Ash.get(Mv.Membership.Member, member.id) + assert member_after_link.email == "user@example.com" + end + + test "two unlinked users cannot have the same email" do + # Create first user + {:ok, _user1} = + Accounts.create_user(%{ + email: "duplicate@example.com" + }) + + # Try to create second user with same email + result = + Accounts.create_user(%{ + email: "duplicate@example.com" + }) + + assert {:error, %Ash.Error.Invalid{}} = result + end + + test "two unlinked members cannot have the same email (members have unique constraint)" do + # Create first member + {:ok, _member1} = + Membership.create_member(%{ + first_name: "John", + last_name: "Doe", + email: "duplicate@example.com" + }) + + # Try to create second member with same email - should fail + result = + Membership.create_member(%{ + first_name: "Jane", + last_name: "Smith", + email: "duplicate@example.com" + }) + + assert {:error, %Ash.Error.Invalid{}} = result + # Members DO have a unique email constraint at database level + end + end +end