Fix registering new user account with same email as unconfirmed

Refactors get_user_by_email/2

Signed-off-by: Thomas Citharel <tcit@tcit.fr>

lib/graphql/resolvers/person.ex

@@ -235,7 +235,8 @@ defmodule Mobilizon.GraphQL.Resolvers.Person do
   This function is used to register a person afterwards the user has been created (but not activated)
   """
   def register_person(_parent, args, _resolution) do
-    with {:ok, %User{} = user} <- Users.get_user_by_email(args.email),
+    # When registering, email is assumed confirmed (unlike changing email)
+    with {:ok, %User{} = user} <- Users.get_user_by_email(args.email, unconfirmed: false),
          user_actor <- Users.get_actor_for_user(user),
          no_actor <- is_nil(user_actor),
          {:no_actor, true} <- {:no_actor, no_actor},

lib/graphql/resolvers/user.ex

@@ -7,7 +7,6 @@ defmodule Mobilizon.GraphQL.Resolvers.User do
 
   alias Mobilizon.{Actors, Admin, Config, Events, Users}
   alias Mobilizon.Actors.Actor
-  alias Mobilizon.Crypto
   alias Mobilizon.Federation.ActivityPub
   alias Mobilizon.Federation.ActivityPub.Relay
   alias Mobilizon.Service.Auth.Authenticator
@@ -19,8 +18,6 @@ defmodule Mobilizon.GraphQL.Resolvers.User do
 
   require Logger
 
-  @confirmation_token_length 30
-
   @doc """
   Find an user by its ID
   """
@@ -183,11 +180,11 @@ defmodule Mobilizon.GraphQL.Resolvers.User do
 
   @doc """
   Send the confirmation email again.
-  We only do this to accounts unconfirmed
+  We only do this to accounts not activated
   """
   def resend_confirmation_email(_parent, args, _resolution) do
     with {:ok, %User{locale: locale} = user} <-
-           Users.get_user_by_email(Map.get(args, :email), false),
+           Users.get_user_by_email(Map.get(args, :email), activated: false, unconfirmed: false),
          {:ok, email} <-
            Email.User.resend_confirmation_email(user, Map.get(args, :locale, locale)) do
       {:ok, email}
@@ -205,7 +202,8 @@ defmodule Mobilizon.GraphQL.Resolvers.User do
   """
   def send_reset_password(_parent, args, _resolution) do
     with email <- Map.get(args, :email),
-         {:ok, %User{locale: locale} = user} <- Users.get_user_by_email(email, true),
+         {:ok, %User{locale: locale} = user} <-
+           Users.get_user_by_email(email, activated: true, unconfirmed: false),
          {:can_reset_password, true} <-
            {:can_reset_password, Authenticator.can_reset_password?(user)},
          {:ok, %Bamboo.Email{} = _email_html} <-
@@ -355,14 +353,7 @@ defmodule Mobilizon.GraphQL.Resolvers.User do
            {:current_password, Authenticator.login(user.email, password)},
          {:same_email, false} <- {:same_email, new_email == old_email},
          {:email_valid, true} <- {:email_valid, Email.Checker.valid?(new_email)},
-         {:ok, %User{} = user} <-
-           user
-           |> User.changeset(%{
-             unconfirmed_email: new_email,
-             confirmation_token: Crypto.random_string(@confirmation_token_length),
-             confirmation_sent_at: DateTime.utc_now() |> DateTime.truncate(:second)
-           })
-           |> Repo.update() do
+         {:ok, %User{} = user} <- Users.update_user_email(user, new_email) do
       user
       |> Email.User.send_email_reset_old_email()
       |> Email.Mailer.deliver_later()
@@ -390,15 +381,7 @@ defmodule Mobilizon.GraphQL.Resolvers.User do
 
   def validate_email(_parent, %{token: token}, _resolution) do
     with {:get, %User{} = user} <- {:get, Users.get_user_by_activation_token(token)},
-         {:ok, %User{} = user} <-
-           user
-           |> User.changeset(%{
-             email: user.unconfirmed_email,
-             unconfirmed_email: nil,
-             confirmation_token: nil,
-             confirmation_sent_at: nil
-           })
-           |> Repo.update() do
+         {:ok, %User{} = user} <- Users.validate_email(user) do
       {:ok, user}
     else
       {:get, nil} ->

lib/mix/tasks/mobilizon/create_bot.ex

@@ -16,7 +16,7 @@ defmodule Mix.Tasks.Mobilizon.CreateBot do
   def run([email, name, summary, type, url]) do
     start_mobilizon()
 
-    with {:ok, %User{} = user} <- Users.get_user_by_email(email, true),
+    with {:ok, %User{} = user} <- Users.get_user_by_email(email, activated: true),
          actor <- Actors.register_bot(%{name: name, summary: summary}),
          {:ok, %Bot{} = bot} <-
            Actors.create_bot(%{

lib/mobilizon/users/users.ex

@@ -10,7 +10,7 @@ defmodule Mobilizon.Users do
 
   alias Ecto.Multi
   alias Mobilizon.Actors.Actor
-  alias Mobilizon.Events
+  alias Mobilizon.{Crypto, Events}
   alias Mobilizon.Events.FeedToken
   alias Mobilizon.Storage.{Page, Repo}
   alias Mobilizon.Users.{Setting, User}
@@ -19,6 +19,8 @@ defmodule Mobilizon.Users do
 
   defenum(NotificationPendingNotificationDelay, none: 0, direct: 1, one_hour: 5, one_day: 10)
 
+  @confirmation_token_length 30
+
   @doc """
   Registers an user.
   """
@@ -66,10 +68,12 @@ defmodule Mobilizon.Users do
   @doc """
   Gets an user by its email.
   """
-  @spec get_user_by_email(String.t(), boolean | nil) ::
+  @spec get_user_by_email(String.t(), Keyword.t()) ::
           {:ok, User.t()} | {:error, :user_not_found}
-  def get_user_by_email(email, activated \\ nil) do
-    query = user_by_email_query(email, activated)
+  def get_user_by_email(email, options \\ []) do
+    activated = Keyword.get(options, :activated, nil)
+    unconfirmed = Keyword.get(options, :unconfirmed, true)
+    query = user_by_email_query(email, activated, unconfirmed)
 
     case Repo.one(query) do
       nil ->
@@ -83,10 +87,13 @@ defmodule Mobilizon.Users do
   @doc """
   Gets an user by its email.
   """
-  @spec get_user_by_email!(String.t(), boolean | nil) :: User.t()
-  def get_user_by_email!(email, activated \\ nil) do
+  @spec get_user_by_email!(String.t(), Keyword.t()) :: User.t()
+  def get_user_by_email!(email, options \\ []) do
+    activated = Keyword.get(options, :activated, nil)
+    unconfirmed = Keyword.get(options, :unconfirmed, true)
+
     email
-    |> user_by_email_query(activated)
+    |> user_by_email_query(activated, unconfirmed)
     |> Repo.one!()
   end
 
@@ -123,6 +130,29 @@ defmodule Mobilizon.Users do
     end
   end
 
+  @spec update_user_email(User.t(), String.t()) :: {:ok, User.t()} | {:error, Ecto.Changeset.t()}
+  def update_user_email(%User{} = user, new_email) do
+    user
+    |> User.changeset(%{
+      unconfirmed_email: new_email,
+      confirmation_token: Crypto.random_string(@confirmation_token_length),
+      confirmation_sent_at: DateTime.utc_now() |> DateTime.truncate(:second)
+    })
+    |> Repo.update()
+  end
+
+  @spec validate_email(User.t()) :: {:ok, User.t()} | {:error, Ecto.Changeset.t()}
+  def validate_email(%User{} = user) do
+    user
+    |> User.changeset(%{
+      email: user.unconfirmed_email,
+      unconfirmed_email: nil,
+      confirmation_token: nil,
+      confirmation_sent_at: nil
+    })
+    |> Repo.update()
+  end
+
   @delete_user_default_options [reserve_email: true]
 
   @doc """
@@ -375,29 +405,26 @@ defmodule Mobilizon.Users do
     Setting.changeset(setting, %{})
   end
 
-  @spec user_by_email_query(String.t(), boolean | nil) :: Ecto.Query.t()
-  defp user_by_email_query(email, nil) do
-    from(u in User,
-      where: u.email == ^email or u.unconfirmed_email == ^email,
-      preload: :default_actor
-    )
+  @spec user_by_email_query(String.t(), boolean | nil, boolean()) :: Ecto.Query.t()
+  defp user_by_email_query(email, activated, unconfirmed) do
+    User
+    |> where([u], u.email == ^email)
+    |> include_unconfirmed(unconfirmed, email)
+    |> filter_activated(activated)
+    |> preload([:default_actor])
   end
 
-  defp user_by_email_query(email, true) do
-    from(
-      u in User,
-      where: u.email == ^email and not is_nil(u.confirmed_at) and not u.disabled,
-      preload: :default_actor
-    )
-  end
+  defp include_unconfirmed(query, false, _email), do: query
 
-  defp user_by_email_query(email, false) do
-    from(
-      u in User,
-      where: (u.email == ^email or u.unconfirmed_email == ^email) and is_nil(u.confirmed_at),
-      preload: :default_actor
-    )
-  end
+  defp include_unconfirmed(query, true, email),
+    do: or_where(query, [u], u.unconfirmed_email == ^email)
+
+  defp filter_activated(query, nil), do: query
+
+  defp filter_activated(query, true),
+    do: where(query, [u], not is_nil(u.confirmed_at) and not u.disabled)
+
+  defp filter_activated(query, false), do: where(query, [u], is_nil(u.confirmed_at))
 
   @spec user_by_activation_token_query(String.t()) :: Ecto.Query.t()
   defp user_by_activation_token_query(token) do

lib/service/auth/authenticator.ex

@@ -86,7 +86,7 @@ defmodule Mobilizon.Service.Auth.Authenticator do
   def fetch_user(nil), do: {:error, :user_not_found}
 
   def fetch_user(email) when not is_nil(email) do
-    with {:ok, %User{} = user} <- Users.get_user_by_email(email, true) do
+    with {:ok, %User{} = user} <- Users.get_user_by_email(email, activated: true) do
       user
     end
   end

test/mobilizon/users/users_test.exs

@@ -85,9 +85,9 @@ defmodule Mobilizon.UsersTest do
     test "get_user_by_email/1 finds an activated user by its email" do
       {:ok, %User{} = user} = Users.register(%{email: @email, password: @password})
 
-      {:ok, %User{id: id}} = Users.get_user_by_email(@email, false)
+      {:ok, %User{id: id}} = Users.get_user_by_email(@email, activated: false)
       assert id == user.id
-      assert {:error, :user_not_found} = Users.get_user_by_email(@email, true)
+      assert {:error, :user_not_found} = Users.get_user_by_email(@email, activated: true)
 
       Users.update_user(user, %{
         "confirmed_at" => DateTime.utc_now() |> DateTime.truncate(:second),
@@ -95,10 +95,28 @@ defmodule Mobilizon.UsersTest do
         "confirmation_token" => nil
       })
 
-      assert {:error, :user_not_found} = Users.get_user_by_email(@email, false)
-      {:ok, %User{id: id}} = Users.get_user_by_email(@email, true)
+      assert {:error, :user_not_found} = Users.get_user_by_email(@email, activated: false)
+      {:ok, %User{id: id}} = Users.get_user_by_email(@email, activated: true)
       assert id == user.id
     end
+
+    @unconfirmed_email "unconfirmed@email.com"
+    test "get_user_by_email/1 finds an user by its pending email" do
+      {:ok, %User{} = user} = Users.register(%{email: @email, password: @password})
+
+      Users.update_user(user, %{
+        "confirmed_at" => DateTime.utc_now() |> DateTime.truncate(:second),
+        "confirmation_sent_at" => nil,
+        "confirmation_token" => nil
+      })
+
+      assert {:ok, %User{}} = Users.update_user_email(user, @unconfirmed_email)
+
+      assert {:error, :user_not_found} =
+               Users.get_user_by_email(@unconfirmed_email, unconfirmed: false)
+
+      assert {:ok, %User{}} = Users.get_user_by_email(@unconfirmed_email, unconfirmed: true)
+    end
   end
 
   describe "user_settings" do