OIDC handling and linking closes #171 #192

Merged
moritz merged 9 commits from feature/oidc_handling into main 2025-11-13 16:36:04 +01:00
3 changed files with 177 additions and 1 deletions
Showing only changes of commit 293e85334f - Show all commits

View file

@ -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)

Does this do anything? it looks like oidc_id is the same value we're getting out of the changeset a few lines above.

Does this do anything? it looks like `oidc_id` is the same value we're getting out of the changeset a few lines above.

On line 184, oidc_id is taken out of the changeset as an argument, but on line 191 it is set as an attribute. These are two different things in Ash:
Arguments are temporary values that are passed to the action
Attributes are the persisted fields in the data model So the code transforms the argument into an attribute that is stored in the database.

On line 184, oidc_id is taken out of the changeset as an argument, but on line 191 it is set as an attribute. These are two different things in Ash: Arguments are temporary values that are passed to the action Attributes are the persisted fields in the data model So the code transforms the argument into an attribute that is stored in the database.
# Update email if it differs from OIDC provider
|> then(fn cs ->
if new_email && to_string(cs.data.email) != new_email do
moritz marked this conversation as resolved Outdated

Is the equality check needed? From the ash docs:

Adds a change to the changeset, unless the value matches the existing value.

Is the equality check needed? From the [ash docs](https://hexdocs.pm/ash/Ash.Changeset.html#change_attribute/3): > Adds a change to the changeset, **unless the value matches the existing value.**
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

View file

@ -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

View file

@ -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
)}
moritz marked this conversation as resolved Outdated

Removing this function and inlining its code instead would be shorter and could be more readable, even at the cost of an extra level of indentation above.

Removing this function and inlining its code instead would be shorter and could be more readable, even at the cost of an extra level of indentation above.
end
end
@impl true
def atomic?(), do: false
@impl true
def describe(_opts) do
[
message: "OIDC email collision detected",
vars: []
]
end
end