Fix flaky auth rate limit tests and refactor auth rate limiting (#4401)

* Add safeguard against flaky auth rate limit tests

* Fix typo in a log message

* Extract and abstract rate limiting from `AuthController`

* Fix flaky rate limit test tag

* Don't leak prefix from auth rate limit

* Use more compact map syntax

* Remove special tag in favor of `eventually` test util function

lib/plausible/auth/auth.ex

@@ -1,7 +1,49 @@
 defmodule Plausible.Auth do
+  @moduledoc """
+  Functions for user authentication context.
+  """
+
   use Plausible
   use Plausible.Repo
   alias Plausible.Auth
+  alias Plausible.RateLimit
+
+  @rate_limits %{
+    login_ip: %{
+      prefix: "login:ip",
+      limit: 5,
+      interval: :timer.seconds(60)
+    },
+    login_user: %{
+      prefix: "login:user",
+      limit: 5,
+      interval: :timer.seconds(60)
+    },
+    email_change_user: %{
+      prefix: "email-change:user",
+      limit: 2,
+      interval: :timer.hours(1)
+    }
+  }
+
+  @rate_limit_types Map.keys(@rate_limits)
+
+  @type rate_limit_type() :: unquote(Enum.reduce(@rate_limit_types, &{:|, [], [&1, &2]}))
+
+  @spec rate_limits() :: map()
+  def rate_limits(), do: @rate_limits
+
+  @spec rate_limit(rate_limit_type(), Auth.User.t() | Plug.Conn.t()) ::
+          :ok | {:error, {:rate_limit, rate_limit_type()}}
+  def rate_limit(limit_type, key) when limit_type in @rate_limit_types do
+    %{prefix: prefix, limit: limit, interval: interval} = @rate_limits[limit_type]
+    full_key = "#{prefix}:#{rate_limit_key(key)}"
+
+    case RateLimit.check_rate(full_key, interval, limit) do
+      {:allow, _} -> :ok
+      {:deny, _} -> {:error, {:rate_limit, limit_type}}
+    end
+  end
 
   def create_user(name, email, pwd) do
     Auth.User.new(%{name: name, email: email, password: pwd, password_confirmation: pwd})
@@ -113,4 +155,7 @@ defmodule Plausible.Auth do
       {:error, :invalid_api_key}
     end
   end
+
+  defp rate_limit_key(%Auth.User{id: id}), do: id
+  defp rate_limit_key(%Plug.Conn{} = conn), do: PlausibleWeb.RemoteIP.get(conn)
 end

lib/plausible_web/controllers/auth_controller.ex

@@ -2,7 +2,7 @@ defmodule PlausibleWeb.AuthController do
   use PlausibleWeb, :controller
   use Plausible.Repo
 
-  alias Plausible.{Auth, RateLimit}
+  alias Plausible.Auth
   alias Plausible.Billing.Quota
   alias PlausibleWeb.TwoFactor
 
@@ -235,9 +235,9 @@ defmodule PlausibleWeb.AuthController do
   end
 
   defp login_user(conn, email, password) do
-    with :ok <- check_ip_rate_limit(conn),
+    with :ok <- Auth.rate_limit(:login_ip, conn),
          {:ok, user} <- find_user(email),
-         :ok <- check_user_rate_limit(user),
+         :ok <- Auth.rate_limit(:login_user, user),
          :ok <- check_password(user, password) do
       {:ok, user}
     else
@@ -258,8 +258,8 @@ defmodule PlausibleWeb.AuthController do
           layout: {PlausibleWeb.LayoutView, "focus.html"}
         )
 
-      {:rate_limit, _} ->
-        maybe_log_failed_login_attempts("too many logging attempts for #{email}")
+      {:error, {:rate_limit, _}} ->
+        maybe_log_failed_login_attempts("too many login attempts for #{email}")
 
         render_error(
           conn,
@@ -298,27 +298,6 @@ defmodule PlausibleWeb.AuthController do
     end
   end
 
-  @login_interval 60_000
-  @login_limit 5
-  @email_change_limit 2
-  @email_change_interval :timer.hours(1)
-
-  defp check_ip_rate_limit(conn) do
-    ip_address = PlausibleWeb.RemoteIP.get(conn)
-
-    case RateLimit.check_rate("login:ip:#{ip_address}", @login_interval, @login_limit) do
-      {:allow, _} -> :ok
-      {:deny, _} -> {:rate_limit, :ip_address}
-    end
-  end
-
-  defp check_user_rate_limit(user) do
-    case RateLimit.check_rate("login:user:#{user.id}", @login_interval, @login_limit) do
-      {:allow, _} -> :ok
-      {:deny, _} -> {:rate_limit, :user}
-    end
-  end
-
   defp find_user(email) do
     user =
       Repo.one(
@@ -509,12 +488,12 @@ defmodule PlausibleWeb.AuthController do
   defp get_2fa_user_limited(conn) do
     case TwoFactor.Session.get_2fa_user(conn) do
       {:ok, user} ->
-        with :ok <- check_ip_rate_limit(conn),
-             :ok <- check_user_rate_limit(user) do
+        with :ok <- Auth.rate_limit(:login_ip, conn),
+             :ok <- Auth.rate_limit(:login_user, user) do
           {:ok, user}
         else
-          {:rate_limit, _} ->
-            maybe_log_failed_login_attempts("too many logging attempts for #{user.email}")
+          {:error, {:rate_limit, _}} ->
+            maybe_log_failed_login_attempts("too many login attempts for #{user.email}")
 
             conn
             |> TwoFactor.Session.clear_2fa_user()
@@ -553,33 +532,25 @@ defmodule PlausibleWeb.AuthController do
   def update_email(conn, %{"user" => user_params}) do
     user = conn.assigns.current_user
 
-    case RateLimit.check_rate(
-           "email-change:user:#{user.id}",
-           @email_change_interval,
-           @email_change_limit
-         ) do
-      {:allow, _} ->
-        changes = Auth.User.email_changeset(user, user_params)
+    with :ok <- Auth.rate_limit(:email_change_user, user),
+         changes = Auth.User.email_changeset(user, user_params),
+         {:ok, user} <- Repo.update(changes) do
+      if user.email_verified do
+        handle_email_updated(conn)
+      else
+        Auth.EmailVerification.issue_code(user)
+        redirect(conn, to: Routes.auth_path(conn, :activate_form))
+      end
+    else
+      {:error, %Ecto.Changeset{} = changeset} ->
+        settings_changeset = Auth.User.settings_changeset(user)
 
-        case Repo.update(changes) do
-          {:ok, user} ->
-            if user.email_verified do
-              handle_email_updated(conn)
-            else
-              Auth.EmailVerification.issue_code(user)
-              redirect(conn, to: Routes.auth_path(conn, :activate_form))
-            end
+        render_settings(conn,
+          settings_changeset: settings_changeset,
+          email_changeset: changeset
+        )
 
-          {:error, changeset} ->
-            settings_changeset = Auth.User.settings_changeset(user)
-
-            render_settings(conn,
-              settings_changeset: settings_changeset,
-              email_changeset: changeset
-            )
-        end
-
-      {:deny, _} ->
+      {:error, {:rate_limit, _}} ->
         settings_changeset = Auth.User.settings_changeset(user)
 
         {:error, changeset} =

test/plausible_web/controllers/auth_controller/logs_test.exs

@@ -29,25 +29,33 @@ defmodule PlausibleWeb.AuthController.LogsTest do
       assert logs =~ "[warning] [login] wrong password for #{user.email}"
     end
 
-    test "logs on too many login attempts", %{conn: conn} do
+    test "logs on too many login attempts" do
       user = insert(:user, password: "password")
 
-      capture_log(fn ->
-        for _ <- 1..5 do
-          build_conn()
-          |> put_req_header("x-forwarded-for", "1.1.1.1")
-          |> post("/login", email: user.email, password: "wrong")
-        end
-      end)
+      conn =
+        build_conn()
+        |> put_req_header("x-forwarded-for", "1.1.1.1")
 
       logs =
-        capture_log(fn ->
-          conn
-          |> put_req_header("x-forwarded-for", "1.1.1.1")
-          |> post("/login", email: user.email, password: "wrong")
-        end)
+        eventually(
+          fn ->
+            capture_log(fn ->
+              Enum.each(1..5, fn _ ->
+                post(conn, "/login", email: user.email, password: "wrong")
+              end)
+            end)
 
-      assert logs =~ "[warning] [login] too many logging attempts for #{user.email}"
+            {conn, logs} =
+              with_log(fn ->
+                post(conn, "/login", email: user.email, password: "wrong")
+              end)
+
+            {conn.status == 429, logs}
+          end,
+          500
+        )
+
+      assert logs =~ "[warning] [login] too many login attempts for #{user.email}"
     end
   end
 end

test/plausible_web/controllers/auth_controller_test.exs

@@ -420,33 +420,23 @@ defmodule PlausibleWeb.AuthControllerTest do
     test "limits login attempts to 5 per minute" do
       user = insert(:user, password: "password")
 
-      build_conn()
-      |> put_req_header("x-forwarded-for", "1.2.3.5")
-      |> post("/login", email: user.email, password: "wrong")
+      conn = put_req_header(build_conn(), "x-forwarded-for", "1.2.3.5")
 
-      build_conn()
-      |> put_req_header("x-forwarded-for", "1.2.3.5")
-      |> post("/login", email: user.email, password: "wrong")
+      response =
+        eventually(
+          fn ->
+            Enum.each(1..5, fn _ ->
+              post(conn, "/login", email: user.email, password: "wrong")
+            end)
 
-      build_conn()
-      |> put_req_header("x-forwarded-for", "1.2.3.5")
-      |> post("/login", email: user.email, password: "wrong")
+            conn = post(conn, "/login", email: user.email, password: "wrong")
 
-      build_conn()
-      |> put_req_header("x-forwarded-for", "1.2.3.5")
-      |> post("/login", email: user.email, password: "wrong")
+            {conn.status == 429, conn}
+          end,
+          500
+        )
 
-      build_conn()
-      |> put_req_header("x-forwarded-for", "1.2.3.5")
-      |> post("/login", email: user.email, password: "wrong")
-
-      conn =
-        build_conn()
-        |> put_req_header("x-forwarded-for", "1.2.3.5")
-        |> post("/login", email: user.email, password: "wrong")
-
-      assert get_session(conn, :current_user_id) == nil
-      assert html_response(conn, 429) =~ "Too many login attempts"
+      assert html_response(response, 429) =~ "Too many login attempts"
     end
   end
 
@@ -1833,37 +1823,29 @@ defmodule PlausibleWeb.AuthControllerTest do
       {:ok, user, _} = Auth.TOTP.initiate(user)
       {:ok, user, _} = Auth.TOTP.enable(user, :skip_verify)
 
-      conn = login_with_cookie(conn, user.email, "password")
-
-      conn
-      |> put_req_header("x-forwarded-for", "1.1.1.1")
-      |> post(Routes.auth_path(conn, :verify_2fa), %{code: "invalid"})
-
-      conn
-      |> put_req_header("x-forwarded-for", "1.1.1.1")
-      |> post(Routes.auth_path(conn, :verify_2fa), %{code: "invalid"})
-
-      conn
-      |> put_req_header("x-forwarded-for", "1.1.1.1")
-      |> post(Routes.auth_path(conn, :verify_2fa), %{code: "invalid"})
-
-      conn
-      |> put_req_header("x-forwarded-for", "1.1.1.1")
-      |> post(Routes.auth_path(conn, :verify_2fa), %{code: "invalid"})
-
-      conn
-      |> put_req_header("x-forwarded-for", "1.1.1.1")
-      |> post(Routes.auth_path(conn, :verify_2fa), %{code: "invalid"})
-
       conn =
         conn
+        |> login_with_cookie(user.email, "password")
         |> put_req_header("x-forwarded-for", "1.1.1.1")
-        |> post(Routes.auth_path(conn, :verify_2fa), %{code: "invalid"})
 
-      assert get_session(conn, :current_user_id) == nil
+      response =
+        eventually(
+          fn ->
+            Enum.each(1..5, fn _ ->
+              post(conn, Routes.auth_path(conn, :verify_2fa), %{code: "invalid"})
+            end)
+
+            conn = post(conn, Routes.auth_path(conn, :verify_2fa), %{code: "invalid"})
+
+            {conn.status == 429, conn}
+          end,
+          500
+        )
+
+      assert get_session(response, :current_user_id) == nil
       # 2FA session terminated
-      assert conn.resp_cookies["session_2fa"].max_age == 0
-      assert html_response(conn, 429) =~ "Too many login attempts"
+      assert response.resp_cookies["session_2fa"].max_age == 0
+      assert html_response(response, 429) =~ "Too many login attempts"
     end
   end
 
@@ -2004,37 +1986,34 @@ defmodule PlausibleWeb.AuthControllerTest do
       {:ok, user, _} = Auth.TOTP.initiate(user)
       {:ok, user, _} = Auth.TOTP.enable(user, :skip_verify)
 
-      conn = login_with_cookie(conn, user.email, "password")
-
-      conn
-      |> put_req_header("x-forwarded-for", "1.2.3.4")
-      |> post(Routes.auth_path(conn, :verify_2fa_recovery_code), %{recovery_code: "invalid"})
-
-      conn
-      |> put_req_header("x-forwarded-for", "1.2.3.4")
-      |> post(Routes.auth_path(conn, :verify_2fa_recovery_code), %{recovery_code: "invalid"})
-
-      conn
-      |> put_req_header("x-forwarded-for", "1.2.3.4")
-      |> post(Routes.auth_path(conn, :verify_2fa_recovery_code), %{recovery_code: "invalid"})
-
-      conn
-      |> put_req_header("x-forwarded-for", "1.2.3.4")
-      |> post(Routes.auth_path(conn, :verify_2fa_recovery_code), %{recovery_code: "invalid"})
-
-      conn
-      |> put_req_header("x-forwarded-for", "1.2.3.4")
-      |> post(Routes.auth_path(conn, :verify_2fa_recovery_code), %{recovery_code: "invalid"})
-
       conn =
         conn
+        |> login_with_cookie(user.email, "password")
         |> put_req_header("x-forwarded-for", "1.2.3.4")
-        |> post(Routes.auth_path(conn, :verify_2fa_recovery_code), %{recovery_code: "invalid"})
 
-      assert get_session(conn, :current_user_id) == nil
+      response =
+        eventually(
+          fn ->
+            Enum.each(1..5, fn _ ->
+              post(conn, Routes.auth_path(conn, :verify_2fa_recovery_code), %{
+                recovery_code: "invalid"
+              })
+            end)
+
+            conn =
+              post(conn, Routes.auth_path(conn, :verify_2fa_recovery_code), %{
+                recovery_code: "invalid"
+              })
+
+            {conn.status == 429, conn}
+          end,
+          500
+        )
+
+      assert get_session(response, :current_user_id) == nil
       # 2FA session terminated
-      assert conn.resp_cookies["session_2fa"].max_age == 0
-      assert html_response(conn, 429) =~ "Too many login attempts"
+      assert response.resp_cookies["session_2fa"].max_age == 0
+      assert html_response(response, 429) =~ "Too many login attempts"
     end
   end