refactor and docs

lib/accounts/user.ex

@@ -228,6 +228,7 @@ defmodule Mv.Accounts.User do
       argument :user_info, :map, allow_nil?: false
       argument :oauth_tokens, :map, allow_nil?: false
       upsert? true
+      # Upsert based on oidc_id (primary match for existing OIDC users)
       upsert_identity :unique_oidc_id
 
       validate &__MODULE__.validate_oidc_id_present/2
@@ -242,8 +243,10 @@ defmodule Mv.Accounts.User do
         |> Ash.Changeset.change_attribute(:oidc_id, user_info["sub"] || user_info["id"])
       end
 
-      # Check for email collisions with existing password-only accounts
+      # Check for email collisions with existing accounts
       # This validation must run AFTER email and oidc_id are set above
+      # - Raises PasswordVerificationRequired for password-protected OR passwordless users
+      # - The LinkOidcAccountLive will auto-link passwordless users without password prompt
       validate Mv.Accounts.User.Validations.OidcEmailCollision
 
       # Sync user email to member when linking (User → Member)

lib/accounts/user/validations/oidc_email_collision.ex

@@ -2,26 +2,29 @@ defmodule Mv.Accounts.User.Validations.OidcEmailCollision do
   @moduledoc """
   Validation that checks for email collisions during OIDC registration.
 
-  This validation prevents OIDC accounts from automatically taking over existing
-  password-only accounts. Instead, it requires password verification.
+  This validation prevents unauthorized account takeovers and enforces proper
+  account linking flows based on user state.
 
   ## Scenarios:
 
   1. **User exists with matching oidc_id**: 
      - Allow (upsert will update the existing user)
 
-  2. **User exists with email but NO oidc_id (or empty string)**:
-     - Raise PasswordVerificationRequired error
-     - User must verify password before linking
+  2. **User exists with different oidc_id**:
+     - Hard error: Cannot link multiple OIDC providers to same account
+     - No linking possible - user must use original OIDC provider
 
-  3. **User exists with email AND different oidc_id**:
+  3. **User exists without oidc_id** (password-protected OR passwordless):
      - Raise PasswordVerificationRequired error
-     - This prevents linking different OIDC providers to same account
+     - User is redirected to LinkOidcAccountLive which will:
+       - Show password form if user has password
+       - Auto-link immediately if user is passwordless
 
   4. **No user exists with this email**:
      - Allow (new user will be created)
   """
   use Ash.Resource.Validation
+  require Logger
 
   alias Mv.Accounts.User.Errors.PasswordVerificationRequired
 
@@ -37,13 +40,23 @@ defmodule Mv.Accounts.User.Validations.OidcEmailCollision do
 
     # Only validate if we have both email and oidc_id (from OIDC registration)
     if email && oidc_id && user_info do
-      check_email_collision(email, oidc_id, user_info)
+      # Check if a user with this oidc_id already exists
+      # If yes, this will be an upsert (email update), not a new registration
+      existing_oidc_user =
+        case Mv.Accounts.User
+             |> Ash.Query.filter(oidc_id == ^to_string(oidc_id))
+             |> Ash.read_one() do
+          {:ok, user} -> user
+          _ -> nil
+        end
+
+      check_email_collision(email, oidc_id, user_info, existing_oidc_user)
     else
       :ok
     end
   end
 
-  defp check_email_collision(email, new_oidc_id, user_info) do
+  defp check_email_collision(email, new_oidc_id, user_info, existing_oidc_user) do
     # Find existing user with this email
     case Mv.Accounts.User
          |> Ash.Query.filter(email == ^to_string(email))
@@ -52,42 +65,116 @@ defmodule Mv.Accounts.User.Validations.OidcEmailCollision do
         # No user exists with this email - OK to create new user
         :ok
 
-      {:ok, existing_user} ->
-        # User exists - check oidc_id
-        handle_existing_user(existing_user, new_oidc_id, user_info)
+      {:ok, user_with_email} ->
+        # User exists with this email - check if it's an upsert or registration
+        is_upsert = not is_nil(existing_oidc_user)
+
+        handle_existing_user(
+          user_with_email,
+          new_oidc_id,
+          user_info,
+          is_upsert,
+          existing_oidc_user
+        )
 
       {:error, error} ->
-        # Database error
-        {:error, field: :email, message: "Could not verify email uniqueness: #{inspect(error)}"}
+        # Database error - log for debugging but don't expose internals to user
+        Logger.error("Email uniqueness check failed during OIDC registration: #{inspect(error)}")
+        {:error, field: :email, message: "Could not verify email uniqueness. Please try again."}
     end
   end
 
-  defp handle_existing_user(existing_user, new_oidc_id, user_info) do
-    existing_oidc_id = existing_user.oidc_id
+  defp handle_existing_user(
+         user_with_email,
+         new_oidc_id,
+         user_info,
+         is_upsert,
+         existing_oidc_user
+       ) do
+    if is_upsert do
+      handle_upsert_scenario(user_with_email, user_info, existing_oidc_user)
+    else
+      handle_create_scenario(user_with_email, new_oidc_id, user_info)
+    end
+  end
 
+  # Handle email update for existing OIDC user
+  defp handle_upsert_scenario(user_with_email, user_info, existing_oidc_user) do
     cond do
-      # Case 1: Same oidc_id - this is an upsert, allow it
-      existing_oidc_id == new_oidc_id ->
+      # Same user updating their own record
+      not is_nil(existing_oidc_user) and user_with_email.id == existing_oidc_user.id ->
         :ok
 
-      # Case 2: No oidc_id set (nil or empty string) - password-only user
-      is_nil(existing_oidc_id) or existing_oidc_id == "" ->
+      # Different user exists with target email
+      not is_nil(existing_oidc_user) and user_with_email.id != existing_oidc_user.id ->
+        handle_email_conflict(user_with_email, user_info)
+
+      # Should not reach here
+      true ->
+        {:error, field: :email, message: "Unexpected error during email update"}
+    end
+  end
+
+  # Handle email conflict during upsert
+  defp handle_email_conflict(user_with_email, user_info) do
+    email = Map.get(user_info, "preferred_username", "unknown")
+    email_user_oidc_id = user_with_email.oidc_id
+
+    # Check if target email belongs to another OIDC user
+    if not is_nil(email_user_oidc_id) and email_user_oidc_id != "" do
+      different_oidc_error(email)
+    else
+      email_taken_error(email)
+    end
+  end
+
+  # Handle new OIDC user registration scenarios
+  defp handle_create_scenario(user_with_email, new_oidc_id, user_info) do
+    email_user_oidc_id = user_with_email.oidc_id
+
+    cond do
+      # Same oidc_id (should not happen in practice, but allow for safety)
+      email_user_oidc_id == new_oidc_id ->
+        :ok
+
+      # Different oidc_id exists (hard error)
+      not is_nil(email_user_oidc_id) and email_user_oidc_id != "" and
+          email_user_oidc_id != new_oidc_id ->
+        email = Map.get(user_info, "preferred_username", "unknown")
+        different_oidc_error(email)
+
+      # No oidc_id (require account linking)
+      is_nil(email_user_oidc_id) or email_user_oidc_id == "" ->
         {:error,
          PasswordVerificationRequired.exception(
-           user_id: existing_user.id,
+           user_id: user_with_email.id,
            oidc_user_info: user_info
          )}
 
-      # Case 3: Different oidc_id - account conflict
+      # Should not reach here
       true ->
-        {:error,
-         PasswordVerificationRequired.exception(
-           user_id: existing_user.id,
-           oidc_user_info: user_info
-         )}
+        {:error, field: :email, message: "Unexpected error during OIDC registration"}
     end
   end
 
+  # Generate error for different OIDC account conflict
+  defp different_oidc_error(email) do
+    {:error,
+     field: :email,
+     message:
+       "Email '#{email}' is already linked to a different OIDC account. " <>
+         "Cannot link multiple OIDC providers to the same account."}
+  end
+
+  # Generate error for email already taken
+  defp email_taken_error(email) do
+    {:error,
+     field: :email,
+     message:
+       "Cannot update email to '#{email}': This email is already registered to another account. " <>
+         "Please change your email in the identity provider."}
+  end
+
   @impl true
   def atomic?(), do: false