Improve and simplify email verification codes generation (#3407)

* Refactor email verification codes generation to avoid predictability

* Improve `Site.Memberships.any?` slightly

* Update tests

* Fix seeds

* Use `expired?` predicate for checking verification code validity in tests

* Store verification code as string in database to avoid unnecessary int casting

config/runtime.exs

@@ -440,8 +440,6 @@ base_cron = [
   {"0 12 * * *", Plausible.Workers.SendCheckStatsEmails},
   # Every 15 minutes
   {"*/15 * * * *", Plausible.Workers.SpikeNotifier},
-  # Every day at midnight
-  {"0 0 * * *", Plausible.Workers.CleanEmailVerificationCodes},
   # Every day at 1am
   {"0 1 * * *", Plausible.Workers.CleanInvitations},
   # Every 2 hours
@@ -468,7 +466,6 @@ base_queues = [
   spike_notifications: 1,
   check_stats_emails: 1,
   site_setup_emails: 1,
-  clean_email_verification_codes: 1,
   clean_invitations: 1,
   google_analytics_imports: 1,
   domain_change_transition: 1

lib/plausible/auth/auth.ex

@@ -2,61 +2,6 @@ defmodule Plausible.Auth do
   use Plausible.Repo
   alias Plausible.Auth
 
-  def issue_email_verification(user) do
-    Repo.update_all(from(c in "email_verification_codes", where: c.user_id == ^user.id),
-      set: [user_id: nil]
-    )
-
-    code =
-      Repo.one(
-        from(c in "email_verification_codes", where: is_nil(c.user_id), select: c.code, limit: 1)
-      )
-
-    Repo.update_all(from(c in "email_verification_codes", where: c.code == ^code),
-      set: [user_id: user.id, issued_at: Timex.now()]
-    )
-
-    code
-  end
-
-  defp is_expired?(activation_code_issued) do
-    Timex.before?(activation_code_issued, Timex.shift(Timex.now(), hours: -4))
-  end
-
-  def verify_email(user, code) do
-    found_code =
-      Repo.one(
-        from c in "email_verification_codes",
-          where: c.user_id == ^user.id,
-          where: c.code == ^code,
-          select: %{code: c.code, issued: c.issued_at}
-      )
-
-    cond do
-      is_nil(found_code) ->
-        {:error, :incorrect}
-
-      is_expired?(found_code[:issued]) ->
-        {:error, :expired}
-
-      true ->
-        {:ok, _} =
-          Ecto.Multi.new()
-          |> Ecto.Multi.update(
-            :user,
-            Ecto.Changeset.change(user, email_verified: true)
-          )
-          |> Ecto.Multi.update_all(
-            :codes,
-            from(c in "email_verification_codes", where: c.user_id == ^user.id),
-            set: [user_id: nil]
-          )
-          |> Repo.transaction()
-
-        :ok
-    end
-  end
-
   def create_user(name, email, pwd) do
     Auth.User.new(%{name: name, email: email, password: pwd, password_confirmation: pwd})
     |> Repo.insert()

lib/plausible/auth/email_activation_code.ex

@@ -0,0 +1,35 @@
+defmodule Plausible.Auth.EmailActivationCode do
+  @moduledoc """
+  Schema for email activation codes.
+  """
+  use Ecto.Schema
+
+  import Ecto.Changeset
+
+  alias Plausible.Auth.User
+
+  @type t() :: %__MODULE__{}
+
+  schema "email_activation_codes" do
+    field :code, :string
+    field :issued_at, :naive_datetime
+
+    belongs_to :user, User
+  end
+
+  @spec new(User.t(), NaiveDateTime.t()) :: Ecto.Changeset.t()
+  def new(user, now) do
+    now = NaiveDateTime.truncate(now, :second)
+
+    %__MODULE__{}
+    |> change(code: generate_code(), issued_at: now)
+    |> put_assoc(:user, user)
+  end
+
+  @spec generate_code() :: String.t()
+  def generate_code do
+    1000..9999
+    |> Enum.random()
+    |> Integer.to_string()
+  end
+end

lib/plausible/auth/email_verification.ex

@@ -0,0 +1,91 @@
+defmodule Plausible.Auth.EmailVerification do
+  @moduledoc """
+  API for verifying emails.
+  """
+
+  import Ecto.Query, only: [from: 2]
+
+  alias Plausible.Auth
+  alias Plausible.Auth.EmailActivationCode
+  alias Plausible.Repo
+
+  require Logger
+
+  @expiration_hours 4
+
+  @spec any?(Auth.User.t()) :: boolean()
+  def any?(user) do
+    Repo.exists?(from(v in EmailActivationCode, where: v.user_id == ^user.id))
+  end
+
+  @spec issue_code(Auth.User.t(), NaiveDateTime.t()) ::
+          {:ok, EmailActivationCode.t()} | {:error, :hard_bounce | :unknown_error}
+  def issue_code(user, now \\ NaiveDateTime.utc_now()) do
+    now = NaiveDateTime.truncate(now, :second)
+
+    verification =
+      user
+      |> EmailActivationCode.new(now)
+      |> Repo.insert!(
+        on_conflict: [
+          set: [
+            issued_at: now,
+            code: EmailActivationCode.generate_code()
+          ]
+        ],
+        conflict_target: :user_id,
+        returning: true
+      )
+
+    email_template = PlausibleWeb.Email.activation_email(user, verification.code)
+
+    case Plausible.Mailer.send(email_template) do
+      :ok ->
+        Logger.debug(
+          "E-mail verification e-mail sent. In dev environment GET /sent-emails for details."
+        )
+
+        {:ok, verification}
+
+      error ->
+        error
+    end
+  end
+
+  @spec verify_code(Auth.User.t(), String.t() | non_neg_integer()) ::
+          :ok | {:error, :incorrect | :expired}
+  def verify_code(user, code) do
+    with {:ok, verification} <- get_verification(user, code) do
+      Repo.transaction(fn ->
+        user
+        |> Ecto.Changeset.change(email_verified: true)
+        |> Repo.update!()
+
+        Repo.delete_all(from(c in EmailActivationCode, where: c.id == ^verification.id))
+      end)
+
+      :ok
+    end
+  end
+
+  defp get_verification(user, code) do
+    verification = Repo.get_by(EmailActivationCode, user_id: user.id, code: code)
+
+    cond do
+      is_nil(verification) ->
+        {:error, :incorrect}
+
+      expired?(verification) ->
+        {:error, :expired}
+
+      true ->
+        {:ok, verification}
+    end
+  end
+
+  @spec expired?(EmailActivationCode.t()) :: boolean()
+  def expired?(verification) do
+    expiration_time = Timex.shift(NaiveDateTime.utc_now(), hours: -1 * @expiration_hours)
+    Timex.before?(verification.issued_at, expiration_time)
+  end
+end

lib/plausible/site/memberships.ex

@@ -5,6 +5,7 @@ defmodule Plausible.Site.Memberships do
 
   import Ecto.Query, only: [from: 2]
 
+  alias Plausible.Auth
   alias Plausible.Repo
   alias Plausible.Site.Memberships
 
@@ -13,13 +14,11 @@ defmodule Plausible.Site.Memberships do
   defdelegate reject_invitation(invitation_id, user), to: Memberships.RejectInvitation
   defdelegate remove_invitation(invitation_id, site), to: Memberships.RemoveInvitation
 
-  @spec any?(String.t()) :: boolean()
-  def any?(user_id) do
-    Repo.exists?(
-      from(m in Plausible.Site.Membership,
-        where: m.user_id == ^user_id
-      )
-    )
+  @spec any?(Auth.User.t()) :: boolean()
+  def any?(user) do
+    user
+    |> Ecto.assoc(:site_memberships)
+    |> Repo.exists?()
   end
 
   @spec has_any_invitations?(String.t()) :: boolean()

lib/plausible/users.ex

@@ -5,17 +5,17 @@ defmodule Plausible.Users do
 
   import Ecto.Query
 
-  alias Plausible.Auth.User
+  alias Plausible.Auth
   alias Plausible.Billing.Subscription
   alias Plausible.Repo
 
-  def with_subscription(%User{id: user_id} = user) do
+  def with_subscription(%Auth.User{id: user_id} = user) do
     Repo.preload(user, subscription: last_subscription_query(user_id))
   end
 
   def with_subscription(user_id) when is_integer(user_id) do
     Repo.one(
-      from(user in User,
+      from(user in Auth.User,
         left_join: last_subscription in subquery(last_subscription_query(user_id)),
         on: last_subscription.user_id == user.id,
         left_join: subscription in Subscription,
@@ -26,13 +26,9 @@ defmodule Plausible.Users do
     )
   end
 
-  @spec has_email_code?(String.t()) :: boolean()
-  def has_email_code?(user_id) do
-    Repo.exists?(
-      from(c in "email_verification_codes",
-        where: c.user_id == ^user_id
-      )
-    )
+  @spec has_email_code?(Auth.User.t()) :: boolean()
+  def has_email_code?(user) do
+    Auth.EmailVerification.any?(user)
   end
 
   defp last_subscription_query(user_id) do

lib/plausible_web/controllers/auth_controller.ex

@@ -45,7 +45,7 @@ defmodule PlausibleWeb.AuthController do
       if user.email_verified do
         redirect(conn, to: Routes.site_path(conn, :new))
       else
-        send_email_verification(user)
+        Auth.EmailVerification.issue_code(user)
         redirect(conn, to: Routes.auth_path(conn, :activate_form))
       end
     end
@@ -58,31 +58,19 @@ defmodule PlausibleWeb.AuthController do
       if user.email_verified do
         redirect(conn, to: Routes.site_path(conn, :index))
       else
-        send_email_verification(user)
+        Auth.EmailVerification.issue_code(user)
         redirect(conn, to: Routes.auth_path(conn, :activate_form))
       end
     end
   end
 
-  defp send_email_verification(user) do
-    code = Auth.issue_email_verification(user)
-    email_template = PlausibleWeb.Email.activation_email(user, code)
-    result = Plausible.Mailer.send(email_template)
-
-    Logger.debug(
-      "E-mail verification e-mail sent. In dev environment GET /sent-emails for details."
-    )
-
-    result
-  end
-
   def activate_form(conn, _params) do
-    user = conn.assigns[:current_user]
+    user = conn.assigns.current_user
 
     render(conn, "activate.html",
-      has_email_code?: Plausible.Users.has_email_code?(user.id),
+      has_email_code?: Plausible.Users.has_email_code?(user),
       has_any_invitations?: Plausible.Site.Memberships.has_any_invitations?(user.email),
-      has_any_memberships?: Plausible.Site.Memberships.any?(user.id),
+      has_any_memberships?: Plausible.Site.Memberships.any?(user),
       layout: {PlausibleWeb.LayoutView, "focus.html"}
     )
   end
@@ -91,11 +79,9 @@ defmodule PlausibleWeb.AuthController do
     user = conn.assigns[:current_user]
 
     has_any_invitations? = Plausible.Site.Memberships.has_any_invitations?(user.email)
-    has_any_memberships? = Plausible.Site.Memberships.any?(user.id)
+    has_any_memberships? = Plausible.Site.Memberships.any?(user)
 
-    {code, ""} = Integer.parse(code)
-
-    case Auth.verify_email(user, code) do
+    case Auth.EmailVerification.verify_code(user, code) do
       :ok ->
         cond do
           has_any_memberships? ->
@@ -129,11 +115,8 @@ defmodule PlausibleWeb.AuthController do
   end
 
   def request_activation_code(conn, _params) do
-    user = conn.assigns[:current_user]
-    code = Auth.issue_email_verification(user)
-
-    email_template = PlausibleWeb.Email.activation_email(user, code)
-    Plausible.Mailer.send(email_template)
+    user = conn.assigns.current_user
+    Auth.EmailVerification.issue_code(user)
 
     conn
     |> put_flash(:success, "Activation code was sent to #{user.email}")
@@ -352,7 +335,7 @@ defmodule PlausibleWeb.AuthController do
         if user.email_verified do
           handle_email_updated(conn)
         else
-          send_email_verification(user)
+          Auth.EmailVerification.issue_code(user)
           redirect(conn, to: Routes.auth_path(conn, :activate_form))
         end
 

lib/workers/clean_email_verification_codes.ex

@@ -1,17 +0,0 @@
-defmodule Plausible.Workers.CleanEmailVerificationCodes do
-  use Plausible.Repo
-  use Oban.Worker, queue: :clean_email_verification_codes
-
-  @impl Oban.Worker
-  def perform(_job) do
-    Repo.update_all(
-      from(c in "email_verification_codes",
-        where: not is_nil(c.user_id),
-        where: c.issued_at < fragment("now() - INTERVAL '4 hours'")
-      ),
-      set: [user_id: nil]
-    )
-
-    :ok
-  end
-end

priv/repo/seeds.exs

@@ -39,7 +39,14 @@ site =
   Plausible.Factory.insert(:site,
     domain: "dummy.site",
     native_stats_start_at: NaiveDateTime.new!(native_stats_range.first, ~T[00:00:00]),
-    stats_start_date: NaiveDateTime.new!(imported_stats_range.first, ~T[00:00:00])
+    stats_start_date: NaiveDateTime.new!(imported_stats_range.first, ~T[00:00:00]),
+    memberships: [
+      Plausible.Factory.build(:site_membership, user: user, role: :owner),
+      Plausible.Factory.build(:site_membership,
+        user: Plausible.Factory.build(:user, name: "Arnold Wallaby"),
+        role: :viewer
+      )
+    ]
   )
 
 # Plugins API: on dev environment, use "plausible-plugin-dev-seed-token" for "dummy.site" to authenticate
@@ -62,8 +69,6 @@ seeded_token = Plausible.Plugins.API.Token.generate("seed-token")
     %{"goal_id" => goal3.id}
   ])
 
-_membership = Plausible.Factory.insert(:site_membership, user: user, site: site, role: :owner)
-
 put_random_time = fn
   date, 0 ->
     current_hour = Time.utc_now().hour

test/plausible_web/controllers/auth_controller_test.exs

@@ -6,8 +6,10 @@ defmodule PlausibleWeb.AuthControllerTest do
   import Plausible.Test.Support.HTML
   import Mox
 
+  require Logger
   require Plausible.Billing.Subscription.Status
 
+  alias Plausible.Auth
   alias Plausible.Auth.User
   alias Plausible.Billing.Subscription
 
@@ -203,18 +205,40 @@ defmodule PlausibleWeb.AuthControllerTest do
   describe "POST /activate/request-code" do
     setup [:create_user, :log_in]
 
-    test "associates an activation pin with the user account", %{conn: conn, user: user} do
+    test "generates an activation pin for user account", %{conn: conn, user: user} do
       post(conn, "/activate/request-code")
 
-      code =
-        Repo.one(
-          from c in "email_verification_codes",
-            where: c.user_id == ^user.id,
-            select: %{user_id: c.user_id, issued_at: c.issued_at}
-        )
+      assert code = Repo.get_by(Auth.EmailActivationCode, user_id: user.id)
 
-      assert code[:user_id] == user.id
-      assert Timex.after?(code[:issued_at], Timex.now() |> Timex.shift(seconds: -10))
+      assert code.user_id == user.id
+      refute Plausible.Auth.EmailVerification.expired?(code)
+    end
+
+    test "regenerates an activation pin even if there's one already", %{conn: conn, user: user} do
+      five_minutes_ago =
+        NaiveDateTime.utc_now()
+        |> Timex.shift(minutes: -5)
+        |> NaiveDateTime.truncate(:second)
+
+      {:ok, verification} = Auth.EmailVerification.issue_code(user, five_minutes_ago)
+
+      post(conn, "/activate/request-code")
+
+      assert new_verification = Repo.get_by(Auth.EmailActivationCode, user_id: user.id)
+
+      assert verification.id == new_verification.id
+      assert verification.user_id == new_verification.user_id
+      # this actually has a chance to fail 1 in 8999 runs
+      # but at the same time it's good to have a confirmation
+      # that it indeed generates a new code
+      if verification.code == new_verification.code do
+        Logger.warn(
+          "Congratulations! You you have hit 1 in 8999 chance of the same " <>
+            "email verification code repeating twice in a row!"
+        )
+      end
+
+      assert NaiveDateTime.compare(verification.issued_at, new_verification.issued_at) == :lt
     end
 
     test "sends activation email to user", %{conn: conn, user: user} do
@@ -225,7 +249,7 @@ defmodule PlausibleWeb.AuthControllerTest do
       assert subject =~ "is your Plausible email verification code"
     end
 
-    test "redirets user to /activate", %{conn: conn} do
+    test "redirects user to /activate", %{conn: conn} do
       conn = post(conn, "/activate/request-code")
 
       assert redirected_to(conn, 302) == "/activate"
@@ -242,15 +266,14 @@ defmodule PlausibleWeb.AuthControllerTest do
     end
 
     test "with expired pin - reloads the form with error", %{conn: conn, user: user} do
-      Repo.insert_all("email_verification_codes", [
-        %{
-          code: 1234,
-          user_id: user.id,
-          issued_at: Timex.shift(Timex.now(), days: -1)
-        }
-      ])
+      one_day_ago =
+        NaiveDateTime.utc_now()
+        |> Timex.shift(days: -1)
+        |> NaiveDateTime.truncate(:second)
 
-      conn = post(conn, "/activate", %{code: "1234"})
+      {:ok, verification} = Auth.EmailVerification.issue_code(user, one_day_ago)
+
+      conn = post(conn, "/activate", %{code: verification.code})
 
       assert html_response(conn, 200) =~ "Code is expired, please request another one"
     end
@@ -259,13 +282,9 @@ defmodule PlausibleWeb.AuthControllerTest do
       Repo.update!(Plausible.Auth.User.changeset(user, %{email_verified: false}))
       post(conn, "/activate/request-code")
 
-      code =
-        Repo.one(
-          from c in "email_verification_codes", where: c.user_id == ^user.id, select: c.code
-        )
-        |> Integer.to_string()
+      verification = Repo.get_by!(Auth.EmailActivationCode, user_id: user.id)
 
-      conn = post(conn, "/activate", %{code: code})
+      conn = post(conn, "/activate", %{code: verification.code})
       user = Repo.get_by(Plausible.Auth.User, id: user.id)
 
       assert user.email_verified
@@ -278,30 +297,22 @@ defmodule PlausibleWeb.AuthControllerTest do
       Repo.update!(Plausible.Auth.User.changeset(user, %{email_verified: false}))
       post(conn, "/activate/request-code")
 
-      code =
-        Repo.one(
-          from c in "email_verification_codes", where: c.user_id == ^user.id, select: c.code
-        )
-        |> Integer.to_string()
+      verification = Repo.get_by!(Auth.EmailActivationCode, user_id: user.id)
 
-      conn = post(conn, "/activate", %{code: code})
+      conn = post(conn, "/activate", %{code: verification.code})
 
       assert redirected_to(conn) == "/sites"
     end
 
-    test "removes the user association from the verification code", %{conn: conn, user: user} do
+    test "removes used up verification code", %{conn: conn, user: user} do
       Repo.update!(Plausible.Auth.User.changeset(user, %{email_verified: false}))
       post(conn, "/activate/request-code")
 
-      code =
-        Repo.one(
-          from c in "email_verification_codes", where: c.user_id == ^user.id, select: c.code
-        )
-        |> Integer.to_string()
+      verification = Repo.get_by!(Auth.EmailActivationCode, user_id: user.id)
 
-      post(conn, "/activate", %{code: code})
+      post(conn, "/activate", %{code: verification.code})
 
-      refute Repo.exists?(from c in "email_verification_codes", where: c.user_id == ^user.id)
+      refute Repo.get_by(Auth.EmailActivationCode, user_id: user.id)
     end
   end
 

test/workers/clean_email_verification_codes_test.exs

@@ -1,34 +0,0 @@
-defmodule Plausible.Workers.CleanEmailVerificationCodesTest do
-  use Plausible.DataCase
-  alias Plausible.Workers.CleanEmailVerificationCodes
-
-  defp issue_code(user, issued_at) do
-    code =
-      Repo.one(
-        from(c in "email_verification_codes", where: is_nil(c.user_id), select: c.code, limit: 1)
-      )
-
-    Repo.update_all(from(c in "email_verification_codes", where: c.code == ^code),
-      set: [user_id: user.id, issued_at: issued_at]
-    )
-  end
-
-  test "cleans codes that are more than 4 hours old" do
-    user = insert(:user)
-    issue_code(user, Timex.now() |> Timex.shift(hours: -5))
-    issue_code(user, Timex.now() |> Timex.shift(days: -5))
-
-    CleanEmailVerificationCodes.perform(nil)
-
-    refute Repo.exists?(from c in "email_verification_codes", where: c.user_id == ^user.id)
-  end
-
-  test "does not clean code from 2 hours ago" do
-    user = insert(:user)
-    issue_code(user, Timex.now() |> Timex.shift(hours: -2))
-
-    CleanEmailVerificationCodes.perform(nil)
-
-    assert Repo.exists?(from c in "email_verification_codes", where: c.user_id == ^user.id)
-  end
-end