diff --git a/lib/recognizer/accounts.ex b/lib/recognizer/accounts.ex index 158d279..64d3f9a 100644 --- a/lib/recognizer/accounts.ex +++ b/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 diff --git a/lib/recognizer_web/controllers/accounts/user_reset_password_controller.ex b/lib/recognizer_web/controllers/accounts/user_reset_password_controller.ex index 5579bac..34ebe3c 100644 --- a/lib/recognizer_web/controllers/accounts/user_reset_password_controller.ex +++ b/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 diff --git a/test/recognizer/accounts_test.exs b/test/recognizer/accounts_test.exs index 232d4c7..a8f0905 100644 --- a/test/recognizer/accounts_test.exs +++ b/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 diff --git a/test/recognizer_web/controllers/accounts/user_reset_password_controller_test.exs b/test/recognizer_web/controllers/accounts/user_reset_password_controller_test.exs index d344e60..b50dc20 100644 --- a/test/recognizer_web/controllers/accounts/user_reset_password_controller_test.exs +++ b/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