fix oidc security bug

lib/accounts/user.ex

@@ -171,6 +171,40 @@ defmodule Mv.Accounts.User do
       change AshAuthentication.Strategy.Password.HashPasswordChange
     end
 
+    # Action to link an OIDC account to an existing password-only user
+    # This is called after the user has verified their password
+    update :link_oidc_id do
+      description "Links an OIDC ID to an existing user after password verification"
+      accept []
+      argument :oidc_id, :string, allow_nil?: false
+      argument :oidc_user_info, :map, allow_nil?: false
+      require_atomic? false
+
+      change fn changeset, _ctx ->
+        oidc_id = Ash.Changeset.get_argument(changeset, :oidc_id)
+        oidc_user_info = Ash.Changeset.get_argument(changeset, :oidc_user_info)
+
+        # Get the new email from OIDC user_info
+        new_email = Map.get(oidc_user_info, "preferred_username")
+
+        changeset
+        |> Ash.Changeset.change_attribute(:oidc_id, oidc_id)
+        # Update email if it differs from OIDC provider
+        |> then(fn cs ->
+          if new_email && to_string(cs.data.email) != new_email do
+            Ash.Changeset.change_attribute(cs, :email, new_email)
+          else
+            cs
+          end
+        end)
+      end
+
+      # Sync email changes to member if email was updated
+      change Mv.EmailSync.Changes.SyncUserEmailToMember do
+        where [changing(:email)]
+      end
+    end
+
     read :get_by_subject do
       description "Get a user by the subject claim in a JWT"
       argument :subject, :string, allow_nil?: false
@@ -183,7 +217,11 @@ defmodule Mv.Accounts.User do
       argument :oauth_tokens, :map, allow_nil?: false
       prepare AshAuthentication.Strategy.OAuth2.SignInPreparation
 
-      filter expr(email == get_path(^arg(:user_info), [:preferred_username]))
+      # SECURITY: Filter by oidc_id, NOT by email!
+      # This ensures that OIDC sign-in only works for users who have already
+      # linked their account via OIDC. Password-only users (oidc_id = nil)
+      # cannot be accessed via OIDC login without password verification.
+      filter expr(oidc_id == get_path(^arg(:user_info), [:sub]))
     end
 
     create :register_with_rauthy do
@@ -204,6 +242,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
+      # This validation must run AFTER email and oidc_id are set above
+      validate Mv.Accounts.User.Validations.OidcEmailCollision
+
       # Sync user email to member when linking (User → Member)
       change Mv.EmailSync.Changes.SyncUserEmailToMember
     end

lib/accounts/user/errors/password_verification_required.ex

@@ -0,0 +1,33 @@
+defmodule Mv.Accounts.User.Errors.PasswordVerificationRequired do
+  @moduledoc """
+  Custom error raised when an OIDC login attempts to use an email that already exists 
+  in the system with a password-only account (no oidc_id set).
+
+  This error indicates that the user must verify their password before the OIDC account
+  can be linked to the existing password account.
+  """
+  use Splode.Error,
+    fields: [:user_id, :oidc_user_info],
+    class: :invalid
+
+  @type t :: %__MODULE__{
+          user_id: String.t(),
+          oidc_user_info: map()
+        }
+
+  @doc """
+  Returns a human-readable error message.
+
+  ## Parameters
+  - error: The error struct containing user_id and oidc_user_info
+  """
+  def message(%{user_id: user_id, oidc_user_info: user_info}) do
+    email = Map.get(user_info, "preferred_username", "unknown")
+    oidc_id = Map.get(user_info, "sub") || Map.get(user_info, "id", "unknown")
+
+    """
+    Password verification required: An account with email '#{email}' already exists (user_id: #{user_id}).
+    To link your OIDC account (oidc_id: #{oidc_id}) to this existing account, please verify your password.
+    """
+  end
+end

lib/accounts/user/validations/oidc_email_collision.ex

@@ -0,0 +1,101 @@
+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.
+
+  ## 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
+
+  3. **User exists with email AND different oidc_id**:
+     - Raise PasswordVerificationRequired error
+     - This prevents linking different OIDC providers to same account
+
+  4. **No user exists with this email**:
+     - Allow (new user will be created)
+  """
+  use Ash.Resource.Validation
+
+  alias Mv.Accounts.User.Errors.PasswordVerificationRequired
+
+  @impl true
+  def init(opts), do: {:ok, opts}
+
+  @impl true
+  def validate(changeset, _opts, _context) do
+    # Get the email and oidc_id from the changeset
+    email = Ash.Changeset.get_attribute(changeset, :email)
+    oidc_id = Ash.Changeset.get_attribute(changeset, :oidc_id)
+    user_info = Ash.Changeset.get_argument(changeset, :user_info)
+
+    # 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)
+    else
+      :ok
+    end
+  end
+
+  defp check_email_collision(email, new_oidc_id, user_info) do
+    # Find existing user with this email
+    case Mv.Accounts.User
+         |> Ash.Query.filter(email == ^to_string(email))
+         |> Ash.read_one() do
+      {:ok, nil} ->
+        # 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)
+
+      {:error, error} ->
+        # Database error
+        {:error, field: :email, message: "Could not verify email uniqueness: #{inspect(error)}"}
+    end
+  end
+
+  defp handle_existing_user(existing_user, new_oidc_id, user_info) do
+    existing_oidc_id = existing_user.oidc_id
+
+    cond do
+      # Case 1: Same oidc_id - this is an upsert, allow it
+      existing_oidc_id == new_oidc_id ->
+        :ok
+
+      # Case 2: No oidc_id set (nil or empty string) - password-only user
+      is_nil(existing_oidc_id) or existing_oidc_id == "" ->
+        {:error,
+         PasswordVerificationRequired.exception(
+           user_id: existing_user.id,
+           oidc_user_info: user_info
+         )}
+
+      # Case 3: Different oidc_id - account conflict
+      true ->
+        {:error,
+         PasswordVerificationRequired.exception(
+           user_id: existing_user.id,
+           oidc_user_info: user_info
+         )}
+    end
+  end
+
+  @impl true
+  def atomic?(), do: false
+
+  @impl true
+  def describe(_opts) do
+    [
+      message: "OIDC email collision detected",
+      vars: []
+    ]
+  end
+end