diff --git a/lib/accounts/user.ex b/lib/accounts/user.ex index 0fc5ab0..ac0439c 100644 --- a/lib/accounts/user.ex +++ b/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 diff --git a/lib/accounts/user/errors/password_verification_required.ex b/lib/accounts/user/errors/password_verification_required.ex new file mode 100644 index 0000000..ffcc260 --- /dev/null +++ b/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 diff --git a/lib/accounts/user/validations/oidc_email_collision.ex b/lib/accounts/user/validations/oidc_email_collision.ex new file mode 100644 index 0000000..bd75894 --- /dev/null +++ b/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