From 5ce220862fae5e94ae940f720f08602f6819a63c Mon Sep 17 00:00:00 2001 From: Moritz Date: Thu, 6 Nov 2025 14:02:29 +0100 Subject: [PATCH] refactor and docs --- docs/oidc-account-linking.md | 207 +++++++++++++ lib/accounts/user.ex | 5 +- .../user/validations/oidc_email_collision.ex | 141 +++++++-- lib/mv_web/controllers/auth_controller.ex | 220 ++++++++------ .../live/auth/link_oidc_account_live.ex | 176 +++++++++--- lib/mv_web/locale_controller.ex | 7 +- priv/gettext/de/LC_MESSAGES/auth.po | 25 ++ priv/gettext/de/LC_MESSAGES/default.po | 10 + .../mv_web/controllers/oidc_e2e_flow_test.exs | 46 +-- .../controllers/oidc_email_update_test.exs | 271 ++++++++++++++++++ .../controllers/oidc_integration_test.exs | 17 +- .../oidc_password_linking_test.exs | 160 ++++++++++- .../oidc_passwordless_linking_test.exs | 210 ++++++++++++++ 13 files changed, 1321 insertions(+), 174 deletions(-) create mode 100644 docs/oidc-account-linking.md create mode 100644 test/mv_web/controllers/oidc_email_update_test.exs create mode 100644 test/mv_web/controllers/oidc_passwordless_linking_test.exs diff --git a/docs/oidc-account-linking.md b/docs/oidc-account-linking.md new file mode 100644 index 0000000..29c2233 --- /dev/null +++ b/docs/oidc-account-linking.md @@ -0,0 +1,207 @@ +# OIDC Account Linking Implementation + +## Overview + +This feature implements secure account linking between password-based accounts and OIDC authentication. When a user attempts to log in via OIDC with an email that already exists as a password-only account, the system requires password verification before linking the accounts. + +## Architecture + +### Key Components + +#### 1. Security Fix: `lib/accounts/user.ex` + +**Change**: The `sign_in_with_rauthy` action now filters by `oidc_id` instead of `email`. + +```elixir +read :sign_in_with_rauthy do + argument :user_info, :map, allow_nil?: false + argument :oauth_tokens, :map, allow_nil?: false + prepare AshAuthentication.Strategy.OAuth2.SignInPreparation + # SECURITY: Filter by oidc_id, NOT by email! + filter expr(oidc_id == get_path(^arg(:user_info), [:sub])) +end +``` + +**Why**: Prevents OIDC users from bypassing password authentication and taking over existing accounts. + +#### 2. Custom Error: `lib/accounts/user/errors/password_verification_required.ex` + +Custom error raised when OIDC login conflicts with existing password account. + +**Fields**: + +- `user_id`: ID of the existing user +- `oidc_user_info`: OIDC user information for account linking + +#### 3. Validation: `lib/accounts/user/validations/oidc_email_collision.ex` + +Validates email uniqueness during OIDC registration. + +**Scenarios**: + +1. **User exists with matching `oidc_id`**: Allow (upsert) +2. **User exists without `oidc_id`** (password-protected OR passwordless): Raise `PasswordVerificationRequired` + - The `LinkOidcAccountLive` will auto-link passwordless users without password prompt + - Password-protected users must verify their password +3. **User exists with different `oidc_id`**: Hard error (cannot link multiple OIDC providers) +4. **No user exists**: Allow (new user creation) + +#### 4. Account Linking Action: `lib/accounts/user.ex` + +```elixir +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 + # ... implementation +end +``` + +**Features**: + +- Links `oidc_id` to existing user +- Updates email if it differs from OIDC provider +- Syncs email changes to linked member + +#### 5. Controller: `lib/mv_web/controllers/auth_controller.ex` + +Refactored for better complexity and maintainability. + +**Key improvements**: + +- Reduced cyclomatic complexity from 11 to below 9 +- Better separation of concerns with helper functions +- Comprehensive documentation + +**Flow**: + +1. Detects `PasswordVerificationRequired` error +2. Stores OIDC info in session +3. Redirects to account linking page + +#### 6. LiveView: `lib/mv_web/live/auth/link_oidc_account_live.ex` + +Interactive UI for password verification and account linking. + +**Flow**: + +1. Retrieves OIDC info from session +2. **Auto-links passwordless users** immediately (no password prompt) +3. Displays password verification form for password-protected users +4. Verifies password using AshAuthentication +5. Links OIDC account on success +6. Redirects to complete OIDC login +7. **Logs all security-relevant events** (successful/failed linking attempts) + +### Locale Persistence + +**Problem**: Locale was lost on logout (session cleared). + +**Solution**: Store locale in persistent cookie (1 year TTL) with security flags. + +**Changes**: + +- `lib/mv_web/locale_controller.ex`: Sets locale cookie with `http_only` and `secure` flags +- `lib/mv_web/router.ex`: Reads locale from cookie if session empty + +**Security Features**: +- `http_only: true` - Cookie not accessible via JavaScript (XSS protection) +- `secure: true` - Cookie only transmitted over HTTPS in production +- `same_site: "Lax"` - CSRF protection + +## Security Considerations + +### 1. OIDC ID Matching + +- **Before**: Matched by email (vulnerable to account takeover) +- **After**: Matched by `oidc_id` (secure) + +### 2. Account Linking Flow + +- Password verification required before linking (for password-protected users) +- Passwordless users are auto-linked immediately (secure, as they have no password) +- OIDC info stored in session (not in URL/query params) +- CSRF protection on all forms +- All linking attempts logged for audit trail + +### 3. Email Updates + +- Email updates from OIDC provider are applied during linking +- Email changes sync to linked member (if exists) + +### 4. Error Handling + +- Internal errors are logged but not exposed to users (prevents information disclosure) +- User-friendly error messages shown in UI +- Security-relevant events logged with appropriate levels: + - `Logger.info` for successful operations + - `Logger.warning` for failed authentication attempts + - `Logger.error` for system errors + +## Usage Examples + +### Scenario 1: New OIDC User + +```elixir +# User signs in with OIDC for the first time +# → New user created with oidc_id +``` + +### Scenario 2: Existing OIDC User + +```elixir +# User with oidc_id signs in via OIDC +# → Matched by oidc_id, email updated if changed +``` + +### Scenario 3: Password User + OIDC Login + +```elixir +# User with password account tries OIDC login +# → PasswordVerificationRequired raised +# → Redirected to /auth/link-oidc-account +# → User enters password +# → Password verified and logged +# → oidc_id linked to account +# → Successful linking logged +# → Redirected to complete OIDC login +``` + +### Scenario 4: Passwordless User + OIDC Login + +```elixir +# User without password (invited user) tries OIDC login +# → PasswordVerificationRequired raised +# → Redirected to /auth/link-oidc-account +# → System detects passwordless user +# → oidc_id automatically linked (no password prompt) +# → Auto-linking logged +# → Redirected to complete OIDC login +``` + +## API + +### Custom Actions + +#### `link_oidc_id` + +Links an OIDC ID to existing user after password verification. + +**Arguments**: + +- `oidc_id` (required): OIDC sub/id from provider +- `oidc_user_info` (required): Full OIDC user info map + +**Returns**: Updated user with linked `oidc_id` + +**Side Effects**: + +- Updates email if different from OIDC provider +- Syncs email to linked member (if exists) + +## References + +- [AshAuthentication Documentation](https://hexdocs.pm/ash_authentication) +- [OIDC Specification](https://openid.net/specs/openid-connect-core-1_0.html) +- [Security Best Practices for Account Linking](https://cheatsheetseries.owasp.org/cheatsheets/Credential_Stuffing_Prevention_Cheat_Sheet.html) diff --git a/lib/accounts/user.ex b/lib/accounts/user.ex index ac0439c..1547ffe 100644 --- a/lib/accounts/user.ex +++ b/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) diff --git a/lib/accounts/user/validations/oidc_email_collision.ex b/lib/accounts/user/validations/oidc_email_collision.ex index bd75894..ca633ee 100644 --- a/lib/accounts/user/validations/oidc_email_collision.ex +++ b/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 diff --git a/lib/mv_web/controllers/auth_controller.ex b/lib/mv_web/controllers/auth_controller.ex index 51d44d4..9282903 100644 --- a/lib/mv_web/controllers/auth_controller.ex +++ b/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"/" diff --git a/lib/mv_web/live/auth/link_oidc_account_live.ex b/lib/mv_web/live/auth/link_oidc_account_live.ex index af91e31..2262723 100644 --- a/lib/mv_web/live/auth/link_oidc_account_live.ex +++ b/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 diff --git a/lib/mv_web/locale_controller.ex b/lib/mv_web/locale_controller.ex index 0289efa..99a200f 100644 --- a/lib/mv_web/locale_controller.ex +++ b/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 diff --git a/priv/gettext/de/LC_MESSAGES/auth.po b/priv/gettext/de/LC_MESSAGES/auth.po index ca98792..60d905e 100644 --- a/priv/gettext/de/LC_MESSAGES/auth.po +++ b/priv/gettext/de/LC_MESSAGES/auth.po @@ -69,11 +69,21 @@ msgstr "Das Passwort wurde erfolgreich zurückgesetzt" msgid "An account with email %{email} already exists. Please enter your password to link your OIDC account." msgstr "Ein Konto mit der E-Mail %{email} existiert bereits. Bitte geben Sie Ihr Passwort ein, um Ihr OIDC-Konto zu verknüpfen." +#: lib/mv_web/live/auth/link_oidc_account_live.ex:61 +#, elixir-autogen, elixir-format +msgid "Account activated! Redirecting to complete sign-in..." +msgstr "Konto aktiviert! Sie werden zur Anmeldung weitergeleitet..." + #: lib/mv_web/live/auth/link_oidc_account_live.ex:160 #, elixir-autogen, elixir-format msgid "Cancel" msgstr "Abbrechen" +#: lib/mv_web/live/auth/link_oidc_account_live.ex:67 +#, elixir-autogen, elixir-format +msgid "Failed to activate account: %{error}" +msgstr "Aktivierung des Kontos fehlgeschlagen: %{error}" + #: lib/mv_web/live/auth/link_oidc_account_live.ex:118 #, elixir-autogen, elixir-format msgid "Failed to link account: %{error}" @@ -109,6 +119,21 @@ msgstr "Verknüpfen..." msgid "Session expired. Please try again." msgstr "Sitzung abgelaufen. Bitte versuchen Sie es erneut." +#: lib/mv_web/live/auth/link_oidc_account_live.ex:79 +#, elixir-autogen, elixir-format +msgid "This OIDC account is already linked to another user. Please contact support." +msgstr "Dieses OIDC-Konto ist bereits mit einem anderen Benutzer verknüpft. Bitte kontaktieren Sie den Support." + +#: lib/mv_web/live/auth/link_oidc_account_live.ex:89 +#, elixir-autogen, elixir-format +msgid "The email address from your OIDC provider is already registered to another account. Please change your email in the identity provider or contact support." +msgstr "Die E-Mail-Adresse aus Ihrem OIDC-Provider ist bereits für ein anderes Konto registriert. Bitte ändern Sie Ihre E-Mail-Adresse im Identity-Provider oder kontaktieren Sie den Support." + +#: lib/mv_web/live/auth/link_oidc_account_live.ex:100 +#, elixir-autogen, elixir-format +msgid "Failed to link account. Please try again or contact support." +msgstr "Verknüpfung des Kontos fehlgeschlagen. Bitte versuchen Sie es erneut oder kontaktieren Sie den Support." + #: lib/mv_web/live/auth/link_oidc_account_live.ex:108 #, elixir-autogen, elixir-format msgid "Your OIDC account has been successfully linked! Redirecting to complete sign-in..." diff --git a/priv/gettext/de/LC_MESSAGES/default.po b/priv/gettext/de/LC_MESSAGES/default.po index 10a7259..a15489d 100644 --- a/priv/gettext/de/LC_MESSAGES/default.po +++ b/priv/gettext/de/LC_MESSAGES/default.po @@ -639,3 +639,13 @@ msgstr "Anmeldung mit OIDC fehlgeschlagen. Bitte versuchen Sie es erneut." #, elixir-autogen, elixir-format msgid "Unable to sign in. Please try again." msgstr "Anmeldung fehlgeschlagen. Bitte versuchen Sie es erneut." + +#: lib/mv_web/controllers/auth_controller.ex:120 +#, elixir-autogen, elixir-format +msgid "Cannot update email: This email is already registered to another account. Please change your email in the identity provider." +msgstr "E-Mail kann nicht aktualisiert werden: Diese E-Mail-Adresse ist bereits für ein anderes Konto registriert. Bitte ändern Sie Ihre E-Mail-Adresse im Identity-Provider." + +#: lib/mv_web/controllers/auth_controller.ex:126 +#, elixir-autogen, elixir-format +msgid "This email is already linked to a different OIDC account. Cannot link multiple OIDC providers to the same account." +msgstr "Diese E-Mail-Adresse ist bereits mit einem anderen OIDC-Konto verknüpft. Es können nicht mehrere OIDC-Provider mit demselben Konto verknüpft werden." diff --git a/test/mv_web/controllers/oidc_e2e_flow_test.exs b/test/mv_web/controllers/oidc_e2e_flow_test.exs index c992d2f..3b4a22f 100644 --- a/test/mv_web/controllers/oidc_e2e_flow_test.exs +++ b/test/mv_web/controllers/oidc_e2e_flow_test.exs @@ -9,7 +9,7 @@ defmodule MvWeb.OidcE2EFlowTest do require Ash.Query describe "E2E: New OIDC user registration" do - test "new user can register via OIDC", %{conn: conn} do + test "new user can register via OIDC", %{conn: _conn} do # Simulate OIDC callback for brand new user user_info = %{ "sub" => "new_oidc_user_123", @@ -40,7 +40,7 @@ defmodule MvWeb.OidcE2EFlowTest do end describe "E2E: Existing OIDC user sign-in" do - test "existing OIDC user can sign in and email updates", %{conn: conn} do + test "existing OIDC user can sign in and email updates", %{conn: _conn} do # Create OIDC user user = create_test_user(%{ @@ -70,7 +70,7 @@ defmodule MvWeb.OidcE2EFlowTest do describe "E2E: OIDC with existing password account (Email Collision)" do test "OIDC registration with password account email triggers PasswordVerificationRequired", - %{conn: conn} do + %{conn: _conn} do # Step 1: Create a password-only user password_user = create_test_user(%{ @@ -106,7 +106,7 @@ defmodule MvWeb.OidcE2EFlowTest do end test "full E2E flow: OIDC collision -> password verification -> account linked", - %{conn: conn} do + %{conn: _conn} do # Step 1: Create password user password_user = create_test_user(%{ @@ -168,7 +168,7 @@ defmodule MvWeb.OidcE2EFlowTest do end test "E2E: OIDC collision with different email at provider updates email after linking", - %{conn: conn} do + %{conn: _conn} do # Password user with old email password_user = create_test_user(%{ @@ -213,7 +213,7 @@ defmodule MvWeb.OidcE2EFlowTest do end describe "E2E: OIDC with linked member" do - test "E2E: email sync to member when linking OIDC to password account", %{conn: conn} do + test "E2E: email sync to member when linking OIDC to password account", %{conn: _conn} do # Create member member = Ash.Seed.seed!(Mv.Membership.Member, %{ @@ -270,7 +270,7 @@ defmodule MvWeb.OidcE2EFlowTest do end describe "E2E: Security scenarios" do - test "E2E: password-only user cannot be accessed via OIDC without password", %{conn: conn} do + test "E2E: password-only user cannot be accessed via OIDC without password", %{conn: _conn} do # Create password user _password_user = create_test_user(%{ @@ -315,9 +315,9 @@ defmodule MvWeb.OidcE2EFlowTest do end) end - test "E2E: user with oidc_id cannot be hijacked by different OIDC provider", %{conn: conn} do + test "E2E: user with oidc_id cannot be hijacked by different OIDC provider", %{conn: _conn} do # User linked to OIDC provider A - user = + _user = create_test_user(%{ email: "linked@example.com", oidc_id: "provider_a_123" @@ -329,25 +329,31 @@ defmodule MvWeb.OidcE2EFlowTest do "preferred_username" => "linked@example.com" } - # Should trigger password requirement (different oidc_id) + # Should trigger hard error (not PasswordVerificationRequired) {:error, %Ash.Error.Invalid{errors: errors}} = Mv.Accounts.create_register_with_rauthy(%{ user_info: user_info, oauth_tokens: %{} }) - password_error = - Enum.find(errors, fn err -> - match?(%Mv.Accounts.User.Errors.PasswordVerificationRequired{}, err) - end) + # Should have hard error about "already linked to a different OIDC account" + assert Enum.any?(errors, fn + %Ash.Error.Changes.InvalidAttribute{message: msg} -> + String.contains?(msg, "already linked to a different OIDC account") - assert password_error != nil - assert password_error.user_id == user.id + _ -> + false + end) + + # Should NOT be PasswordVerificationRequired + refute Enum.any?(errors, fn err -> + match?(%Mv.Accounts.User.Errors.PasswordVerificationRequired{}, err) + end) end - test "E2E: empty string oidc_id is treated as password-only account", %{conn: conn} do + test "E2E: empty string oidc_id is treated as password-only account", %{conn: _conn} do # User with empty oidc_id - password_user = + _password_user = create_test_user(%{ email: "empty@example.com", password: "pass123", @@ -374,7 +380,7 @@ defmodule MvWeb.OidcE2EFlowTest do end describe "E2E: Error scenarios" do - test "E2E: OIDC registration without oidc_id fails", %{conn: conn} do + test "E2E: OIDC registration without oidc_id fails", %{conn: _conn} do user_info = %{ "preferred_username" => "noid@example.com" } @@ -390,7 +396,7 @@ defmodule MvWeb.OidcE2EFlowTest do end) end - test "E2E: OIDC registration without email fails", %{conn: conn} do + test "E2E: OIDC registration without email fails", %{conn: _conn} do user_info = %{ "sub" => "noemail_123" } diff --git a/test/mv_web/controllers/oidc_email_update_test.exs b/test/mv_web/controllers/oidc_email_update_test.exs new file mode 100644 index 0000000..53a6514 --- /dev/null +++ b/test/mv_web/controllers/oidc_email_update_test.exs @@ -0,0 +1,271 @@ +defmodule MvWeb.OidcEmailUpdateTest do + @moduledoc """ + Tests for OIDC email updates - when an existing OIDC user changes their email + in the OIDC provider and logs in again. + """ + use MvWeb.ConnCase, async: true + + describe "OIDC user updates email to available email" do + test "should succeed and update email" do + # Create OIDC user + {:ok, oidc_user} = + Mv.Accounts.User + |> Ash.Changeset.for_create(:create_user, %{ + email: "original@example.com" + }) + |> Ash.Changeset.force_change_attribute(:oidc_id, "oidc_123") + |> Ash.create() + + # User logs in via OIDC with NEW email + user_info = %{ + "sub" => "oidc_123", + "preferred_username" => "newemail@example.com" + } + + result = + Mv.Accounts.create_register_with_rauthy(%{ + user_info: user_info, + oauth_tokens: %{"access_token" => "test_token"} + }) + + # Should succeed and email should be updated + assert {:ok, updated_user} = result + assert updated_user.id == oidc_user.id + assert to_string(updated_user.email) == "newemail@example.com" + assert updated_user.oidc_id == "oidc_123" + end + end + + describe "OIDC user updates email to email of passwordless user" do + test "should fail with clear error message" do + # Create OIDC user + {:ok, _oidc_user} = + Mv.Accounts.User + |> Ash.Changeset.for_create(:create_user, %{ + email: "oidcuser@example.com" + }) + |> Ash.Changeset.force_change_attribute(:oidc_id, "oidc_456") + |> Ash.create() + + # Create passwordless user with target email + {:ok, _passwordless_user} = + Mv.Accounts.User + |> Ash.Changeset.for_create(:create_user, %{ + email: "taken@example.com" + }) + |> Ash.create() + + # OIDC user tries to update email to taken email + user_info = %{ + "sub" => "oidc_456", + "preferred_username" => "taken@example.com" + } + + result = + Mv.Accounts.create_register_with_rauthy(%{ + user_info: user_info, + oauth_tokens: %{"access_token" => "test_token"} + }) + + # Should fail with email update conflict error + assert {:error, %Ash.Error.Invalid{errors: errors}} = result + + # Should contain error about email being registered to another account + assert Enum.any?(errors, fn + %Ash.Error.Changes.InvalidAttribute{field: :email, message: message} -> + String.contains?(message, "Cannot update email to") and + String.contains?(message, "already registered to another account") + + _ -> + false + end) + + # Should NOT contain PasswordVerificationRequired + refute Enum.any?(errors, fn err -> + match?(%Mv.Accounts.User.Errors.PasswordVerificationRequired{}, err) + end) + end + end + + describe "OIDC user updates email to email of password-protected user" do + test "should fail with clear error message" do + # Create OIDC user + {:ok, _oidc_user} = + Mv.Accounts.User + |> Ash.Changeset.for_create(:create_user, %{ + email: "oidcuser2@example.com" + }) + |> Ash.Changeset.force_change_attribute(:oidc_id, "oidc_789") + |> Ash.create() + + # Create password user with target email (explicitly NO oidc_id) + password_user = + create_test_user(%{ + email: "passworduser@example.com", + password: "securepass123" + }) + + # Ensure it's a password-only user + {:ok, password_user} = Ash.reload(password_user) + assert not is_nil(password_user.hashed_password) + # Force oidc_id to be nil to avoid any confusion + {:ok, password_user} = + password_user + |> Ash.Changeset.for_update(:update, %{}) + |> Ash.Changeset.force_change_attribute(:oidc_id, nil) + |> Ash.update() + + assert is_nil(password_user.oidc_id) + + # OIDC user tries to update email to password user's email + user_info = %{ + "sub" => "oidc_789", + "preferred_username" => "passworduser@example.com" + } + + result = + Mv.Accounts.create_register_with_rauthy(%{ + user_info: user_info, + oauth_tokens: %{"access_token" => "test_token"} + }) + + # Should fail with email update conflict error + assert {:error, %Ash.Error.Invalid{errors: errors}} = result + + # Should contain error about email being registered to another account + assert Enum.any?(errors, fn + %Ash.Error.Changes.InvalidAttribute{field: :email, message: message} -> + String.contains?(message, "Cannot update email to") and + String.contains?(message, "already registered to another account") + + _ -> + false + end) + + # Should NOT contain PasswordVerificationRequired + refute Enum.any?(errors, fn err -> + match?(%Mv.Accounts.User.Errors.PasswordVerificationRequired{}, err) + end) + end + end + + describe "OIDC user updates email to email of different OIDC user" do + test "should fail with clear error message about different OIDC account" do + # Create first OIDC user + {:ok, _oidc_user1} = + Mv.Accounts.User + |> Ash.Changeset.for_create(:create_user, %{ + email: "oidcuser1@example.com" + }) + |> Ash.Changeset.force_change_attribute(:oidc_id, "oidc_aaa") + |> Ash.create() + + # Create second OIDC user with target email + {:ok, _oidc_user2} = + Mv.Accounts.User + |> Ash.Changeset.for_create(:create_user, %{ + email: "oidcuser2@example.com" + }) + |> Ash.Changeset.force_change_attribute(:oidc_id, "oidc_bbb") + |> Ash.create() + + # First OIDC user tries to update email to second user's email + user_info = %{ + "sub" => "oidc_aaa", + "preferred_username" => "oidcuser2@example.com" + } + + result = + Mv.Accounts.create_register_with_rauthy(%{ + user_info: user_info, + oauth_tokens: %{"access_token" => "test_token"} + }) + + # Should fail with "already linked to different OIDC account" error + assert {:error, %Ash.Error.Invalid{errors: errors}} = result + + # Should contain error about different OIDC account + assert Enum.any?(errors, fn + %Ash.Error.Changes.InvalidAttribute{field: :email, message: message} -> + String.contains?(message, "already linked to a different OIDC account") + + _ -> + false + end) + + # Should NOT contain PasswordVerificationRequired + refute Enum.any?(errors, fn err -> + match?(%Mv.Accounts.User.Errors.PasswordVerificationRequired{}, err) + end) + end + end + + describe "New OIDC user registration scenarios (for comparison)" do + test "new OIDC user with email of passwordless user triggers linking flow" do + # Create passwordless user + {:ok, passwordless_user} = + Mv.Accounts.User + |> Ash.Changeset.for_create(:create_user, %{ + email: "passwordless@example.com" + }) + |> Ash.create() + + # New OIDC user tries to register + user_info = %{ + "sub" => "new_oidc_999", + "preferred_username" => "passwordless@example.com" + } + + result = + Mv.Accounts.create_register_with_rauthy(%{ + user_info: user_info, + oauth_tokens: %{"access_token" => "test_token"} + }) + + # Should trigger PasswordVerificationRequired (linking flow) + assert {:error, %Ash.Error.Invalid{errors: errors}} = result + + assert Enum.any?(errors, fn + %Mv.Accounts.User.Errors.PasswordVerificationRequired{user_id: user_id} -> + user_id == passwordless_user.id + + _ -> + false + end) + end + + test "new OIDC user with email of existing OIDC user shows hard error" do + # Create existing OIDC user + {:ok, _existing_oidc_user} = + Mv.Accounts.User + |> Ash.Changeset.for_create(:create_user, %{ + email: "existing@example.com" + }) + |> Ash.Changeset.force_change_attribute(:oidc_id, "oidc_existing") + |> Ash.create() + + # New OIDC user tries to register with same email + user_info = %{ + "sub" => "oidc_new", + "preferred_username" => "existing@example.com" + } + + result = + Mv.Accounts.create_register_with_rauthy(%{ + user_info: user_info, + oauth_tokens: %{"access_token" => "test_token"} + }) + + # Should fail with "already linked to different OIDC account" error + assert {:error, %Ash.Error.Invalid{errors: errors}} = result + + assert Enum.any?(errors, fn + %Ash.Error.Changes.InvalidAttribute{field: :email, message: message} -> + String.contains?(message, "already linked to a different OIDC account") + + _ -> + false + end) + end + end +end diff --git a/test/mv_web/controllers/oidc_integration_test.exs b/test/mv_web/controllers/oidc_integration_test.exs index 508ebab..bc12196 100644 --- a/test/mv_web/controllers/oidc_integration_test.exs +++ b/test/mv_web/controllers/oidc_integration_test.exs @@ -175,9 +175,9 @@ defmodule MvWeb.OidcIntegrationTest do end describe "OIDC error and edge case scenarios" do - test "OIDC registration with conflicting email and OIDC ID shows error" do + test "OIDC registration with conflicting email and OIDC ID shows hard error" do # Create user with email and OIDC ID - existing_user = + _existing_user = create_test_user(%{ email: "conflict@example.com", oidc_id: "oidc_conflict_1" @@ -195,19 +195,24 @@ defmodule MvWeb.OidcIntegrationTest do oauth_tokens: %{} }) - # Should fail with PasswordVerificationRequired (account conflict) + # Should fail with hard error (not PasswordVerificationRequired) # This prevents someone with OIDC provider B from taking over an account # that's already linked to OIDC provider A assert {:error, %Ash.Error.Invalid{errors: errors}} = result - # Should contain PasswordVerificationRequired error + # Should contain error about "already linked to a different OIDC account" assert Enum.any?(errors, fn - %Mv.Accounts.User.Errors.PasswordVerificationRequired{user_id: user_id} -> - user_id == existing_user.id + %Ash.Error.Changes.InvalidAttribute{message: msg} -> + String.contains?(msg, "already linked to a different OIDC account") _ -> false end) + + # Should NOT be PasswordVerificationRequired + refute Enum.any?(errors, fn err -> + match?(%Mv.Accounts.User.Errors.PasswordVerificationRequired{}, err) + end) end test "OIDC registration with missing sub and id should fail" do diff --git a/test/mv_web/controllers/oidc_password_linking_test.exs b/test/mv_web/controllers/oidc_password_linking_test.exs index b59633c..a898f95 100644 --- a/test/mv_web/controllers/oidc_password_linking_test.exs +++ b/test/mv_web/controllers/oidc_password_linking_test.exs @@ -322,7 +322,7 @@ defmodule MvWeb.OidcPasswordLinkingTest do |> Ash.Changeset.for_create(:create_user, %{ email: "user2@example.com" }) - |> Ash.Changeset.change_attribute(:oidc_id, "shared_oidc_333") + |> Ash.Changeset.force_change_attribute(:oidc_id, "shared_oidc_333") |> Ash.create() # Should fail due to unique constraint on oidc_id @@ -335,4 +335,162 @@ defmodule MvWeb.OidcPasswordLinkingTest do end) end end + + describe "OIDC login with passwordless user - Requires Linking Flow" do + test "user without password and without oidc_id triggers PasswordVerificationRequired" do + # Create user without password (e.g., invited user) + {:ok, existing_user} = + Mv.Accounts.User + |> Ash.Changeset.for_create(:create_user, %{ + email: "invited@example.com" + }) + |> Ash.create() + + # Verify user has no password and no oidc_id + assert is_nil(existing_user.hashed_password) + assert is_nil(existing_user.oidc_id) + + # OIDC registration should trigger linking flow (not automatic) + user_info = %{ + "sub" => "auto_link_oidc_123", + "preferred_username" => "invited@example.com" + } + + result = + Mv.Accounts.create_register_with_rauthy(%{ + user_info: user_info, + oauth_tokens: %{"access_token" => "test_token"} + }) + + # Should fail with PasswordVerificationRequired + # The LinkOidcAccountLive will auto-link without password prompt + assert {:error, %Ash.Error.Invalid{}} = result + {:error, error} = result + + assert Enum.any?(error.errors, fn err -> + match?(%Mv.Accounts.User.Errors.PasswordVerificationRequired{}, err) + end) + end + + test "user without password but WITH password later requires verification" do + # Create user without password first + {:ok, user} = + Mv.Accounts.User + |> Ash.Changeset.for_create(:create_user, %{ + email: "added-password@example.com" + }) + |> Ash.create() + + # User sets password later (using admin action) + {:ok, user_with_password} = + user + |> Ash.Changeset.for_update(:admin_set_password, %{ + password: "newpassword123" + }) + |> Ash.update() + + assert not is_nil(user_with_password.hashed_password) + + # Now OIDC login should require password verification + user_info = %{ + "sub" => "needs_verification", + "preferred_username" => "added-password@example.com" + } + + result = + Mv.Accounts.create_register_with_rauthy(%{ + user_info: user_info, + oauth_tokens: %{"access_token" => "test_token"} + }) + + # Should fail with PasswordVerificationRequired + assert {:error, %Ash.Error.Invalid{}} = result + {:error, error} = result + + assert Enum.any?(error.errors, fn err -> + match?(%Mv.Accounts.User.Errors.PasswordVerificationRequired{}, err) + end) + end + end + + describe "OIDC login with different oidc_id - Hard Error" do + test "user with different oidc_id cannot be linked (hard error)" do + # Create user with existing OIDC ID + {:ok, existing_user} = + Mv.Accounts.User + |> Ash.Changeset.for_create(:create_user, %{ + email: "already-linked@example.com" + }) + |> Ash.Changeset.force_change_attribute(:oidc_id, "original_oidc_999") + |> Ash.create() + + assert existing_user.oidc_id == "original_oidc_999" + + # Try to register with same email but different OIDC ID + user_info = %{ + "sub" => "different_oidc_888", + "preferred_username" => "already-linked@example.com" + } + + result = + Mv.Accounts.create_register_with_rauthy(%{ + user_info: user_info, + oauth_tokens: %{"access_token" => "test_token"} + }) + + # Should fail with hard error (not PasswordVerificationRequired) + assert {:error, %Ash.Error.Invalid{}} = result + {:error, error} = result + + # Should NOT be PasswordVerificationRequired + refute Enum.any?(error.errors, fn err -> + match?(%Mv.Accounts.User.Errors.PasswordVerificationRequired{}, err) + end) + + # Should be a validation error about email already linked + assert Enum.any?(error.errors, fn err -> + case err do + %Ash.Error.Changes.InvalidAttribute{message: msg} -> + String.contains?(msg, "already linked to a different OIDC account") + + _ -> + false + end + end) + end + + test "cannot link different oidc_id even with password verification" do + # Create user with password AND existing OIDC ID + existing_user = + create_test_user(%{ + email: "password-and-oidc@example.com", + password: "mypassword123", + oidc_id: "first_oidc_111" + }) + + assert existing_user.oidc_id == "first_oidc_111" + assert not is_nil(existing_user.hashed_password) + + # Try to register with different OIDC ID + user_info = %{ + "sub" => "second_oidc_222", + "preferred_username" => "password-and-oidc@example.com" + } + + result = + Mv.Accounts.create_register_with_rauthy(%{ + user_info: user_info, + oauth_tokens: %{"access_token" => "test_token"} + }) + + # Should fail - cannot link different OIDC ID + assert {:error, %Ash.Error.Invalid{}} = result + {:error, error} = result + + # Should be a hard error, not password verification + refute Enum.any?(error.errors, fn err -> + match?(%Mv.Accounts.User.Errors.PasswordVerificationRequired{}, err) + end) + end + end end diff --git a/test/mv_web/controllers/oidc_passwordless_linking_test.exs b/test/mv_web/controllers/oidc_passwordless_linking_test.exs new file mode 100644 index 0000000..9da66ac --- /dev/null +++ b/test/mv_web/controllers/oidc_passwordless_linking_test.exs @@ -0,0 +1,210 @@ +defmodule MvWeb.OidcPasswordlessLinkingTest do + @moduledoc """ + Tests for OIDC account linking with passwordless users. + + These tests verify the behavior when a passwordless user + (e.g., invited user, user created by admin) attempts to log in via OIDC. + """ + use MvWeb.ConnCase, async: true + + describe "Passwordless user - Automatic linking via special action" do + test "passwordless user can be linked via link_passwordless_oidc action" do + # Create user without password (e.g., invited user) + {:ok, existing_user} = + Mv.Accounts.User + |> Ash.Changeset.for_create(:create_user, %{ + email: "invited@example.com" + }) + |> Ash.create() + + # Verify user has no password and no oidc_id + assert is_nil(existing_user.hashed_password) + assert is_nil(existing_user.oidc_id) + + # Link via special action (simulating what happens after first OIDC attempt) + {:ok, linked_user} = + existing_user + |> Ash.Changeset.for_update(:link_oidc_id, %{ + oidc_id: "auto_link_oidc_123", + oidc_user_info: %{ + "sub" => "auto_link_oidc_123", + "preferred_username" => "invited@example.com" + } + }) + |> Ash.update() + + # User should now have oidc_id linked + assert linked_user.oidc_id == "auto_link_oidc_123" + assert linked_user.id == existing_user.id + + # Now OIDC sign-in should work + result = + Mv.Accounts.User + |> Ash.Query.for_read(:sign_in_with_rauthy, %{ + user_info: %{ + "sub" => "auto_link_oidc_123", + "preferred_username" => "invited@example.com" + }, + oauth_tokens: %{"access_token" => "test_token"} + }) + |> Ash.read_one() + + assert {:ok, signed_in_user} = result + assert signed_in_user.id == existing_user.id + end + + test "passwordless user triggers PasswordVerificationRequired for linking flow" do + # Create passwordless user + {:ok, existing_user} = + Mv.Accounts.User + |> Ash.Changeset.for_create(:create_user, %{ + email: "passwordless@example.com" + }) + |> Ash.create() + + assert is_nil(existing_user.hashed_password) + assert is_nil(existing_user.oidc_id) + + # Try OIDC registration - should trigger PasswordVerificationRequired + user_info = %{ + "sub" => "new_oidc_456", + "preferred_username" => "passwordless@example.com" + } + + result = + Mv.Accounts.create_register_with_rauthy(%{ + user_info: user_info, + oauth_tokens: %{"access_token" => "test_token"} + }) + + # Should fail with PasswordVerificationRequired + # LinkOidcAccountLive will auto-link without password prompt + assert {:error, %Ash.Error.Invalid{}} = result + {:error, error} = result + + assert Enum.any?(error.errors, fn err -> + case err do + %Mv.Accounts.User.Errors.PasswordVerificationRequired{user_id: user_id} -> + user_id == existing_user.id + + _ -> + false + end + end) + end + end + + describe "User with different OIDC ID - Hard Error" do + test "user with different oidc_id gets hard error, not password verification" do + # Create user with existing OIDC ID + {:ok, _existing_user} = + Mv.Accounts.User + |> Ash.Changeset.for_create(:create_user, %{ + email: "already-linked@example.com" + }) + |> Ash.Changeset.force_change_attribute(:oidc_id, "original_oidc_999") + |> Ash.create() + + # Try to register with same email but different OIDC ID + user_info = %{ + "sub" => "different_oidc_888", + "preferred_username" => "already-linked@example.com" + } + + result = + Mv.Accounts.create_register_with_rauthy(%{ + user_info: user_info, + oauth_tokens: %{"access_token" => "test_token"} + }) + + # Should fail with hard error + assert {:error, %Ash.Error.Invalid{}} = result + {:error, error} = result + + # Should NOT be PasswordVerificationRequired + refute Enum.any?(error.errors, fn err -> + match?(%Mv.Accounts.User.Errors.PasswordVerificationRequired{}, err) + end) + + # Should have error message about already linked + assert Enum.any?(error.errors, fn err -> + case err do + %Ash.Error.Changes.InvalidAttribute{message: msg} -> + String.contains?(msg, "already linked to a different OIDC account") + + _ -> + false + end + end) + end + + test "passwordless user with different oidc_id also gets hard error" do + # Create passwordless user with OIDC ID + {:ok, existing_user} = + Mv.Accounts.User + |> Ash.Changeset.for_create(:create_user, %{ + email: "passwordless-linked@example.com" + }) + |> Ash.Changeset.force_change_attribute(:oidc_id, "first_oidc_777") + |> Ash.create() + + assert is_nil(existing_user.hashed_password) + assert existing_user.oidc_id == "first_oidc_777" + + # Try to register with different OIDC ID + user_info = %{ + "sub" => "second_oidc_666", + "preferred_username" => "passwordless-linked@example.com" + } + + result = + Mv.Accounts.create_register_with_rauthy(%{ + user_info: user_info, + oauth_tokens: %{"access_token" => "test_token"} + }) + + # Should be hard error, not PasswordVerificationRequired + assert {:error, %Ash.Error.Invalid{}} = result + {:error, error} = result + + refute Enum.any?(error.errors, fn err -> + match?(%Mv.Accounts.User.Errors.PasswordVerificationRequired{}, err) + end) + end + end + + describe "Password user - Requires verification (existing behavior)" do + test "password user without oidc_id requires password verification" do + # Create password user + password_user = + create_test_user(%{ + email: "password@example.com", + password: "securepass123", + oidc_id: nil + }) + + assert not is_nil(password_user.hashed_password) + assert is_nil(password_user.oidc_id) + + # Try OIDC registration + user_info = %{ + "sub" => "new_oidc_999", + "preferred_username" => "password@example.com" + } + + result = + Mv.Accounts.create_register_with_rauthy(%{ + user_info: user_info, + oauth_tokens: %{"access_token" => "test_token"} + }) + + # Should require password verification + assert {:error, %Ash.Error.Invalid{}} = result + {:error, error} = result + + assert Enum.any?(error.errors, fn err -> + match?(%Mv.Accounts.User.Errors.PasswordVerificationRequired{}, err) + end) + end + end +end