refactor and docs

lib/mv_web/controllers/auth_controller.ex

@@ -1,9 +1,21 @@
 require Logger
 
 defmodule MvWeb.AuthController do
+  @moduledoc """
+  Handles authentication callbacks for password and OIDC authentication.
+
+  This controller manages:
+  - Successful authentication (password, OIDC, password reset, email confirmation)
+  - Authentication failures with appropriate error handling
+  - OIDC account linking flow when email collision occurs
+  - Sign out functionality
+  """
+
   use MvWeb, :controller
   use AshAuthentication.Phoenix.Controller
 
+  alias Mv.Accounts.User.Errors.PasswordVerificationRequired
+
   def success(conn, activity, user, _token) do
     return_to = get_session(conn, :return_to) || ~p"/"
 
@@ -23,107 +35,149 @@ defmodule MvWeb.AuthController do
     |> redirect(to: return_to)
   end
 
+  @doc """
+  Handles authentication failures and routes to appropriate error handling.
+
+  Manages:
+  - OIDC email collisions (triggers password verification flow)
+  - Generic OIDC authentication failures
+  - Unconfirmed account errors
+  - Generic authentication failures
+  """
   def failure(conn, activity, reason) do
-    # Log the error for debugging
     Logger.warning(
       "Authentication failure - Activity: #{inspect(activity)}, Reason: #{inspect(reason)}"
     )
 
     case {activity, reason} do
-      # OIDC registration with existing email requires password verification (direct error)
-      {{:rauthy, :register}, %Ash.Error.Invalid{errors: errors}} ->
-        handle_oidc_email_collision(conn, errors)
+      {{:rauthy, _action}, reason} ->
+        handle_rauthy_failure(conn, reason)
 
-      # OIDC registration with existing email (wrapped in AuthenticationFailed)
-      {{:rauthy, :register},
-       %AshAuthentication.Errors.AuthenticationFailed{
-         caused_by: %Ash.Error.Invalid{errors: errors}
-       }} ->
-        handle_oidc_email_collision(conn, errors)
-
-      # OIDC sign-in failure (wrapped)
-      {{:rauthy, :sign_in}, %AshAuthentication.Errors.AuthenticationFailed{caused_by: caused_by}} ->
-        # Check if it's actually a registration issue
-        case caused_by do
-          %Ash.Error.Invalid{errors: errors} ->
-            handle_oidc_email_collision(conn, errors)
-
-          _ ->
-            # Real sign-in failure
-            conn
-            |> put_flash(:error, gettext("Unable to sign in with OIDC. Please try again."))
-            |> redirect(to: ~p"/sign-in")
-        end
-
-      # OIDC callback failure (can be either sign-in or registration)
-      {{:rauthy, :callback}, %AshAuthentication.Errors.AuthenticationFailed{caused_by: caused_by}} ->
-        case caused_by do
-          %Ash.Error.Invalid{errors: errors} ->
-            handle_oidc_email_collision(conn, errors)
-
-          _ ->
-            conn
-            |> put_flash(:error, gettext("Unable to authenticate with OIDC. Please try again."))
-            |> redirect(to: ~p"/sign-in")
-        end
-
-      {_,
-       %AshAuthentication.Errors.AuthenticationFailed{
-         caused_by: %Ash.Error.Forbidden{
-           errors: [%AshAuthentication.Errors.CannotConfirmUnconfirmedUser{}]
-         }
-       }} ->
-        message =
-          gettext("""
-          You have already signed in another way, but have not confirmed your account.
-          You can confirm your account using the link we sent to you, or by resetting your password.
-          """)
-
-        conn
-        |> put_flash(:error, message)
-        |> redirect(to: ~p"/sign-in")
+      {_, %AshAuthentication.Errors.AuthenticationFailed{caused_by: caused_by}} ->
+        handle_authentication_failed(conn, caused_by)
 
       _ ->
-        message = gettext("Incorrect email or password")
-
-        conn
-        |> put_flash(:error, message)
-        |> redirect(to: ~p"/sign-in")
+        redirect_with_error(conn, gettext("Incorrect email or password"))
     end
   end
 
-  # Handle OIDC email collision - user needs to verify password
-  defp handle_oidc_email_collision(conn, errors) do
-    password_verification_error =
-      Enum.find(errors, fn err ->
-        match?(%Mv.Accounts.User.Errors.PasswordVerificationRequired{}, err)
-      end)
+  # Handle all Rauthy (OIDC) authentication failures
+  defp handle_rauthy_failure(conn, %Ash.Error.Invalid{errors: errors}) do
+    handle_oidc_email_collision(conn, errors)
+  end
 
-    case password_verification_error do
-      %Mv.Accounts.User.Errors.PasswordVerificationRequired{
-        user_id: user_id,
-        oidc_user_info: oidc_user_info
-      } ->
-        # Store the OIDC info in session for the linking flow
-        conn
-        |> put_session(:oidc_linking_user_id, user_id)
-        |> put_session(:oidc_linking_user_info, oidc_user_info)
-        |> put_flash(
-          :info,
-          gettext(
-            "An account with this email already exists. Please verify your password to link your OIDC account."
-          )
-        )
-        |> redirect(to: ~p"/auth/link-oidc-account")
+  defp handle_rauthy_failure(conn, %AshAuthentication.Errors.AuthenticationFailed{
+         caused_by: caused_by
+       }) do
+    case caused_by do
+      %Ash.Error.Invalid{errors: errors} ->
+        handle_oidc_email_collision(conn, errors)
 
       _ ->
-        # Other validation errors - show generic error
-        conn
-        |> put_flash(:error, gettext("Unable to sign in. Please try again."))
-        |> redirect(to: ~p"/sign-in")
+        redirect_with_error(conn, gettext("Unable to authenticate with OIDC. Please try again."))
     end
   end
 
+  # Handle generic AuthenticationFailed errors
+  defp handle_authentication_failed(conn, %Ash.Error.Forbidden{errors: errors}) do
+    if Enum.any?(errors, &match?(%AshAuthentication.Errors.CannotConfirmUnconfirmedUser{}, &1)) do
+      message =
+        gettext("""
+        You have already signed in another way, but have not confirmed your account.
+        You can confirm your account using the link we sent to you, or by resetting your password.
+        """)
+
+      redirect_with_error(conn, message)
+    else
+      redirect_with_error(conn, gettext("Authentication failed. Please try again."))
+    end
+  end
+
+  defp handle_authentication_failed(conn, _other) do
+    redirect_with_error(conn, gettext("Authentication failed. Please try again."))
+  end
+
+  # Handle OIDC email collision - user needs to verify password to link accounts
+  defp handle_oidc_email_collision(conn, errors) do
+    case find_password_verification_error(errors) do
+      %PasswordVerificationRequired{user_id: user_id, oidc_user_info: oidc_user_info} ->
+        redirect_to_account_linking(conn, user_id, oidc_user_info)
+
+      nil ->
+        # Check if it's a "different OIDC account" error or email uniqueness error
+        error_message = extract_meaningful_error_message(errors)
+        redirect_with_error(conn, error_message)
+    end
+  end
+
+  # Extract meaningful error message from Ash errors
+  defp extract_meaningful_error_message(errors) do
+    # Look for specific error messages in InvalidAttribute errors
+    meaningful_error =
+      Enum.find_value(errors, fn
+        %Ash.Error.Changes.InvalidAttribute{message: message, field: :email}
+        when is_binary(message) ->
+          cond do
+            # Email update conflict during OIDC login
+            String.contains?(message, "Cannot update email to") and
+                String.contains?(message, "already registered to another account") ->
+              gettext(
+                "Cannot update email: This email is already registered to another account. Please change your email in the identity provider."
+              )
+
+            # Different OIDC account error
+            String.contains?(message, "already linked to a different OIDC account") ->
+              gettext(
+                "This email is already linked to a different OIDC account. Cannot link multiple OIDC providers to the same account."
+              )
+
+            true ->
+              nil
+          end
+
+        %Ash.Error.Changes.InvalidAttribute{message: message}
+        when is_binary(message) ->
+          # Return any other meaningful message
+          if String.length(message) > 20 and
+               not String.contains?(message, "has already been taken") do
+            message
+          else
+            nil
+          end
+
+        _ ->
+          nil
+      end)
+
+    meaningful_error || gettext("Unable to sign in. Please try again.")
+  end
+
+  # Find PasswordVerificationRequired error in error list
+  defp find_password_verification_error(errors) do
+    Enum.find(errors, &match?(%PasswordVerificationRequired{}, &1))
+  end
+
+  # Redirect to account linking page with OIDC info stored in session
+  defp redirect_to_account_linking(conn, user_id, oidc_user_info) do
+    conn
+    |> put_session(:oidc_linking_user_id, user_id)
+    |> put_session(:oidc_linking_user_info, oidc_user_info)
+    |> put_flash(
+      :info,
+      gettext(
+        "An account with this email already exists. Please verify your password to link your OIDC account."
+      )
+    )
+    |> redirect(to: ~p"/auth/link-oidc-account")
+  end
+
+  # Generic error redirect helper
+  defp redirect_with_error(conn, message) do
+    conn
+    |> put_flash(:error, message)
+    |> redirect(to: ~p"/sign-in")
+  end
+
   def sign_out(conn, _params) do
     return_to = get_session(conn, :return_to) || ~p"/"
 

lib/mv_web/live/auth/link_oidc_account_live.ex

@@ -5,41 +5,139 @@ defmodule MvWeb.LinkOidcAccountLive do
   This page is shown when a user tries to log in via OIDC using an email
   that already exists with a password-only account. The user must verify
   their password before the OIDC account can be linked.
+
+  ## Flow
+  1. User attempts OIDC login with email that has existing password account
+  2. System raises `PasswordVerificationRequired` error
+  3. AuthController redirects here with user_id and oidc_user_info in session
+  4. User enters password to verify identity
+  5. On success, oidc_id is linked to user account
+  6. User is redirected to complete OIDC login
   """
   use MvWeb, :live_view
   require Ash.Query
+  require Logger
 
   @impl true
   def mount(_params, session, socket) do
-    user_id = Map.get(session, "oidc_linking_user_id")
-    oidc_user_info = Map.get(session, "oidc_linking_user_info")
-
-    if user_id && oidc_user_info do
-      # Load the user
-      case Ash.get(Mv.Accounts.User, user_id) do
-        {:ok, user} ->
-          {:ok,
-           socket
-           |> assign(:user, user)
-           |> assign(:oidc_user_info, oidc_user_info)
-           |> assign(:password, "")
-           |> assign(:error, nil)
-           |> assign(:form, to_form(%{"password" => ""}))}
-
-        {:error, _} ->
-          {:ok,
-           socket
-           |> put_flash(:error, dgettext("auth", "Session expired. Please try again."))
-           |> redirect(to: ~p"/sign-in")}
+    with user_id when not is_nil(user_id) <- Map.get(session, "oidc_linking_user_id"),
+         oidc_user_info when not is_nil(oidc_user_info) <-
+           Map.get(session, "oidc_linking_user_info"),
+         {:ok, user} <- Ash.get(Mv.Accounts.User, user_id) do
+      # Check if user is passwordless
+      if passwordless?(user) do
+        # Auto-link passwordless user immediately
+        {:ok, auto_link_passwordless_user(socket, user, oidc_user_info)}
+      else
+        # Show password form for password-protected user
+        {:ok, initialize_socket(socket, user, oidc_user_info)}
       end
     else
-      {:ok,
-       socket
-       |> put_flash(:error, dgettext("auth", "Invalid session. Please try again."))
-       |> redirect(to: ~p"/sign-in")}
+      nil ->
+        {:ok, redirect_with_error(socket, dgettext("auth", "Invalid session. Please try again."))}
+
+      {:error, _} ->
+        {:ok, redirect_with_error(socket, dgettext("auth", "Session expired. Please try again."))}
     end
   end
 
+  defp passwordless?(user) do
+    is_nil(user.hashed_password)
+  end
+
+  defp reload_user!(user_id) do
+    Mv.Accounts.User
+    |> Ash.Query.filter(id == ^user_id)
+    |> Ash.read_one!()
+  end
+
+  defp reset_password_form(socket) do
+    assign(socket, :form, to_form(%{"password" => ""}))
+  end
+
+  defp auto_link_passwordless_user(socket, user, oidc_user_info) do
+    oidc_id = Map.get(oidc_user_info, "sub") || Map.get(oidc_user_info, "id")
+
+    case user.id
+         |> reload_user!()
+         |> Ash.Changeset.for_update(:link_oidc_id, %{
+           oidc_id: oidc_id,
+           oidc_user_info: oidc_user_info
+         })
+         |> Ash.update() do
+      {:ok, updated_user} ->
+        Logger.info(
+          "Passwordless account auto-linked to OIDC: user_id=#{updated_user.id}, oidc_id=#{oidc_id}"
+        )
+
+        socket
+        |> put_flash(
+          :info,
+          dgettext("auth", "Account activated! Redirecting to complete sign-in...")
+        )
+        |> Phoenix.LiveView.redirect(to: ~p"/auth/user/rauthy")
+
+      {:error, error} ->
+        Logger.warning(
+          "Failed to auto-link passwordless account: user_id=#{user.id}, error=#{inspect(error)}"
+        )
+
+        error_message = extract_user_friendly_error(error)
+
+        socket
+        |> put_flash(:error, error_message)
+        |> redirect(to: ~p"/sign-in")
+    end
+  end
+
+  defp extract_user_friendly_error(%Ash.Error.Invalid{errors: errors}) do
+    # Check for specific error types
+    Enum.find_value(errors, fn
+      %Ash.Error.Changes.InvalidAttribute{field: :oidc_id, message: message} ->
+        if String.contains?(message, "already been taken") do
+          dgettext(
+            "auth",
+            "This OIDC account is already linked to another user. Please contact support."
+          )
+        else
+          nil
+        end
+
+      %Ash.Error.Changes.InvalidAttribute{field: :email, message: message} ->
+        if String.contains?(message, "already been taken") do
+          dgettext(
+            "auth",
+            "The email address from your OIDC provider is already registered to another account. Please change your email in the identity provider or contact support."
+          )
+        else
+          nil
+        end
+
+      _ ->
+        nil
+    end) ||
+      dgettext("auth", "Failed to link account. Please try again or contact support.")
+  end
+
+  defp extract_user_friendly_error(_error) do
+    dgettext("auth", "Failed to link account. Please try again or contact support.")
+  end
+
+  defp initialize_socket(socket, user, oidc_user_info) do
+    socket
+    |> assign(:user, user)
+    |> assign(:oidc_user_info, oidc_user_info)
+    |> assign(:password, "")
+    |> assign(:error, nil)
+    |> reset_password_form()
+  end
+
+  defp redirect_with_error(socket, message) do
+    socket
+    |> put_flash(:error, message)
+    |> redirect(to: ~p"/sign-in")
+  end
+
   @impl true
   def handle_event("validate", %{"password" => password}, socket) do
     {:noreply, assign(socket, :password, password)}
@@ -57,11 +155,13 @@ defmodule MvWeb.LinkOidcAccountLive do
         link_oidc_account(socket, verified_user, oidc_user_info)
 
       {:error, _reason} ->
-        # Password incorrect
+        # Password incorrect - log security event
+        Logger.warning("Failed password verification for OIDC linking: user_email=#{user.email}")
+
         {:noreply,
          socket
          |> assign(:error, dgettext("auth", "Incorrect password. Please try again."))
-         |> assign(:form, to_form(%{"password" => ""}))}
+         |> reset_password_form()}
     end
   end
 
@@ -88,17 +188,20 @@ defmodule MvWeb.LinkOidcAccountLive do
     oidc_id = Map.get(oidc_user_info, "sub") || Map.get(oidc_user_info, "id")
 
     # Update the user with the OIDC ID
-    case Mv.Accounts.User
-         |> Ash.Query.filter(id == ^user.id)
-         |> Ash.read_one!()
+    case user.id
+         |> reload_user!()
          |> Ash.Changeset.for_update(:link_oidc_id, %{
            oidc_id: oidc_id,
            oidc_user_info: oidc_user_info
          })
          |> Ash.update() do
-      {:ok, _updated_user} ->
+      {:ok, updated_user} ->
         # After successful linking, redirect to OIDC login
         # Since the user now has an oidc_id, the next OIDC login will succeed
+        Logger.info(
+          "OIDC account successfully linked after password verification: user_id=#{updated_user.id}, oidc_id=#{oidc_id}"
+        )
+
         {:noreply,
          socket
          |> put_flash(
@@ -111,13 +214,16 @@ defmodule MvWeb.LinkOidcAccountLive do
          |> Phoenix.LiveView.redirect(to: ~p"/auth/user/rauthy")}
 
       {:error, error} ->
+        Logger.warning(
+          "Failed to link OIDC account after password verification: user_id=#{user.id}, error=#{inspect(error)}"
+        )
+
+        error_message = extract_user_friendly_error(error)
+
         {:noreply,
          socket
-         |> assign(
-           :error,
-           dgettext("auth", "Failed to link account: %{error}", error: inspect(error))
-         )
-         |> assign(:form, to_form(%{"password" => ""}))}
+         |> assign(:error, error_message)
+         |> reset_password_form()}
     end
   end
 

lib/mv_web/locale_controller.ex

@@ -5,7 +5,12 @@ defmodule MvWeb.LocaleController do
     conn
     |> put_session(:locale, locale)
     # Store locale in a cookie that persists beyond the session
-    |> put_resp_cookie("locale", locale, max_age: 365 * 24 * 60 * 60, same_site: "Lax")
+    |> put_resp_cookie("locale", locale,
+      max_age: 365 * 24 * 60 * 60,
+      same_site: "Lax",
+      http_only: true,
+      secure: Application.get_env(:mv, :use_secure_cookies, false)
+    )
     |> redirect(to: get_referer(conn) || "/")
   end