Fix password reset token reuse race condition

lib/recognizer/accounts.ex

@@ -520,6 +520,15 @@ defmodule Recognizer.Accounts do
   @doc """
   Resets the user password.
 
+  Deletes the user's tokens first and only proceeds if a token was actually
+  deleted. Under InnoDB's default REPEATABLE READ isolation, a `DELETE`
+  takes a lock on matching rows and a concurrent `DELETE` for the same rows
+  blocks until the first transaction commits, then re-checks against the
+  now-committed (deleted) data. So if two requests race on the same reset
+  token, only the one whose `DELETE` commits first will have deleted any
+  rows; the other finds nothing left to delete and gets `:token_already_used`
+  instead of also updating the password.
+
   ## Examples
 
       iex> reset_user_password(user, %{password: "new long password", password_confirmation: "new long password"})
@@ -528,16 +537,23 @@ defmodule Recognizer.Accounts do
       iex> reset_user_password(user, %{password: "valid", password_confirmation: "not the same"})
       {:error, %Ecto.Changeset{}}
 
+      iex> reset_user_password(user, %{password: "new long password", password_confirmation: "new long password"})
+      {:error, :token_already_used}
+
   """
   def reset_user_password(user, attrs) do
     Ecto.Multi.new()
+    |> Ecto.Multi.delete_all(:tokens, user_and_contexts_query(user, :all))
+    |> Ecto.Multi.run(:ensure_token_not_reused, fn _repo, %{tokens: {count, _}} ->
+      if count > 0, do: {:ok, count}, else: {:error, :token_already_used}
+    end)
     |> Ecto.Multi.update(:user, User.password_changeset(user, attrs))
     |> Ecto.Multi.delete_all(:oauth, user_and_oauth_access_query(user))
-    |> Ecto.Multi.delete_all(:tokens, user_and_contexts_query(user, :all))
     |> Repo.transaction()
     |> case do
       {:ok, %{user: user}} -> {:ok, user}
       {:error, :user, changeset, _} -> {:error, changeset}
+      {:error, :ensure_token_not_reused, :token_already_used, _} -> {:error, :token_already_used}
     end
   end
 

lib/recognizer_web/controllers/accounts/user_reset_password_controller.ex

@@ -57,6 +57,11 @@ defmodule RecognizerWeb.Accounts.UserResetPasswordController do
         |> put_flash(:info, "Password reset successfully.")
         |> redirect(to: Routes.user_session_path(conn, :new))
 
+      {:error, :token_already_used} ->
+        conn
+        |> put_flash(:error, "Reset password link is invalid or it has expired.")
+        |> redirect(to: Routes.user_reset_password_path(conn, :new))
+
       {:error, changeset} ->
         render(conn, "edit.html", changeset: changeset)
     end

test/recognizer/accounts_test.exs

@@ -353,7 +353,14 @@ defmodule Recognizer.AccountsTest do
 
   describe "reset_user_password/2" do
     setup do
-      %{user: insert(:user)}
+      user = insert(:user)
+
+      _token =
+        extract_user_token(fn url ->
+          Accounts.deliver_user_reset_password_instructions(user, url)
+        end)
+
+      %{user: user}
     end
 
     test "validates password", %{user: user} do
@@ -388,6 +395,33 @@ defmodule Recognizer.AccountsTest do
       {:ok, _} =
         Accounts.reset_user_password(user, %{password: @new_valid_password, password_confirmation: @new_valid_password})
     end
+
+    test "rejects reuse of an already-consumed token", %{user: user} do
+      attrs = %{password: @new_valid_password, password_confirmation: @new_valid_password}
+
+      {:ok, _} = Accounts.reset_user_password(user, attrs)
+
+      assert Accounts.reset_user_password(user, attrs) == {:error, :token_already_used}
+    end
+
+    test "only one of two concurrent resets on the same token succeeds", %{user: user} do
+      attrs_a = %{password: "ConcurrentPassA1!", password_confirmation: "ConcurrentPassA1!"}
+      attrs_b = %{password: "ConcurrentPassB1!", password_confirmation: "ConcurrentPassB1!"}
+      parent = self()
+
+      results =
+        [attrs_a, attrs_b]
+        |> Enum.map(fn attrs ->
+          Task.async(fn ->
+            Ecto.Adapters.SQL.Sandbox.allow(Repo, parent, self())
+            Accounts.reset_user_password(user, attrs)
+          end)
+        end)
+        |> Enum.map(&Task.await/1)
+
+      assert Enum.count(results, &match?({:ok, _}, &1)) == 1
+      assert Enum.count(results, &(&1 == {:error, :token_already_used})) == 1
+    end
   end
 
   describe "two-factor" do

test/recognizer_web/controllers/accounts/user_reset_password_controller_test.exs

@@ -138,5 +138,21 @@ defmodule RecognizerWeb.Accounts.UserResetPasswordControllerTest do
       assert redirected_to(conn) == Routes.user_reset_password_path(conn, :create)
       assert Flash.get(conn.assigns.flash, :error) =~ "Reset password link is invalid or it has expired"
     end
+
+    test "does not allow the same token to reset the password twice", %{conn: conn, token: token} do
+      params = %{
+        "user" => %{
+          "password" => "n@wvAli4dPassw!d",
+          "password_confirmation" => "n@wvAli4dPassw!d"
+        }
+      }
+
+      conn1 = put(conn, Routes.user_reset_password_path(conn, :update, token), params)
+      assert redirected_to(conn1) == Routes.user_session_path(conn1, :new)
+
+      conn2 = put(conn, Routes.user_reset_password_path(conn, :update, token), params)
+      assert redirected_to(conn2) == Routes.user_reset_password_path(conn2, :create)
+      assert Flash.get(conn2.assigns.flash, :error) =~ "Reset password link is invalid or it has expired"
+    end
   end
 end