From 6f2efb1c450daa75d848dab8e3729ced81ed3904 Mon Sep 17 00:00:00 2001 From: Ivan Tashkinov Date: Thu, 27 Feb 2020 18:46:05 +0300 Subject: Runtime configurability of RateLimiter. Refactoring. Disabled default rate limits in tests. --- test/plugs/rate_limiter_test.exs | 155 +++++++++++---------- .../controllers/account_controller_test.exs | 75 +++++----- 2 files changed, 125 insertions(+), 105 deletions(-) (limited to 'test') diff --git a/test/plugs/rate_limiter_test.exs b/test/plugs/rate_limiter_test.exs index 06ffa7b70..c0630c039 100644 --- a/test/plugs/rate_limiter_test.exs +++ b/test/plugs/rate_limiter_test.exs @@ -6,69 +6,79 @@ defmodule Pleroma.Plugs.RateLimiterTest do use ExUnit.Case, async: true use Plug.Test + alias Pleroma.Config alias Pleroma.Plugs.RateLimiter import Pleroma.Factory + import Pleroma.Tests.Helpers, only: [clear_config: 1, clear_config: 2] # Note: each example must work with separate buckets in order to prevent concurrency issues + clear_config([Pleroma.Web.Endpoint, :http, :ip]) + clear_config(:rate_limit) + describe "config" do + @limiter_name :test_init + + clear_config([Pleroma.Plugs.RemoteIp, :enabled]) + test "config is required for plug to work" do - limiter_name = :test_init - Pleroma.Config.put([:rate_limit, limiter_name], {1, 1}) - Pleroma.Config.put([Pleroma.Web.Endpoint, :http, :ip], {8, 8, 8, 8}) + Config.put([:rate_limit, @limiter_name], {1, 1}) + Config.put([Pleroma.Web.Endpoint, :http, :ip], {8, 8, 8, 8}) assert %{limits: {1, 1}, name: :test_init, opts: [name: :test_init]} == - RateLimiter.init(name: limiter_name) - - assert nil == RateLimiter.init(name: :foo) + [name: @limiter_name] + |> RateLimiter.init() + |> RateLimiter.action_settings() + + assert nil == + [name: :nonexisting_limiter] + |> RateLimiter.init() + |> RateLimiter.action_settings() end test "it is disabled for localhost" do - limiter_name = :test_init - Pleroma.Config.put([:rate_limit, limiter_name], {1, 1}) - Pleroma.Config.put([Pleroma.Web.Endpoint, :http, :ip], {127, 0, 0, 1}) - Pleroma.Config.put([Pleroma.Plugs.RemoteIp, :enabled], false) + Config.put([:rate_limit, @limiter_name], {1, 1}) + Config.put([Pleroma.Web.Endpoint, :http, :ip], {127, 0, 0, 1}) + Config.put([Pleroma.Plugs.RemoteIp, :enabled], false) assert RateLimiter.disabled?() == true end test "it is disabled for socket" do - limiter_name = :test_init - Pleroma.Config.put([:rate_limit, limiter_name], {1, 1}) - Pleroma.Config.put([Pleroma.Web.Endpoint, :http, :ip], {:local, "/path/to/pleroma.sock"}) - Pleroma.Config.put([Pleroma.Plugs.RemoteIp, :enabled], false) + Config.put([:rate_limit, @limiter_name], {1, 1}) + Config.put([Pleroma.Web.Endpoint, :http, :ip], {:local, "/path/to/pleroma.sock"}) + Config.put([Pleroma.Plugs.RemoteIp, :enabled], false) assert RateLimiter.disabled?() == true end test "it is enabled for socket when remote ip is enabled" do - limiter_name = :test_init - Pleroma.Config.put([:rate_limit, limiter_name], {1, 1}) - Pleroma.Config.put([Pleroma.Web.Endpoint, :http, :ip], {:local, "/path/to/pleroma.sock"}) - Pleroma.Config.put([Pleroma.Plugs.RemoteIp, :enabled], true) + Config.put([:rate_limit, @limiter_name], {1, 1}) + Config.put([Pleroma.Web.Endpoint, :http, :ip], {:local, "/path/to/pleroma.sock"}) + Config.put([Pleroma.Plugs.RemoteIp, :enabled], true) assert RateLimiter.disabled?() == false end test "it restricts based on config values" do - limiter_name = :test_opts + limiter_name = :test_plug_opts scale = 80 limit = 5 - Pleroma.Config.put([Pleroma.Web.Endpoint, :http, :ip], {8, 8, 8, 8}) - Pleroma.Config.put([:rate_limit, limiter_name], {scale, limit}) + Config.put([Pleroma.Web.Endpoint, :http, :ip], {8, 8, 8, 8}) + Config.put([:rate_limit, limiter_name], {scale, limit}) - opts = RateLimiter.init(name: limiter_name) + plug_opts = RateLimiter.init(name: limiter_name) conn = conn(:get, "/") for i <- 1..5 do - conn = RateLimiter.call(conn, opts) - assert {^i, _} = RateLimiter.inspect_bucket(conn, limiter_name, opts) + conn = RateLimiter.call(conn, plug_opts) + assert {^i, _} = RateLimiter.inspect_bucket(conn, limiter_name, plug_opts) Process.sleep(10) end - conn = RateLimiter.call(conn, opts) + conn = RateLimiter.call(conn, plug_opts) assert %{"error" => "Throttled"} = Phoenix.ConnTest.json_response(conn, :too_many_requests) assert conn.halted @@ -76,8 +86,8 @@ defmodule Pleroma.Plugs.RateLimiterTest do conn = conn(:get, "/") - conn = RateLimiter.call(conn, opts) - assert {1, 4} = RateLimiter.inspect_bucket(conn, limiter_name, opts) + conn = RateLimiter.call(conn, plug_opts) + assert {1, 4} = RateLimiter.inspect_bucket(conn, limiter_name, plug_opts) refute conn.status == Plug.Conn.Status.code(:too_many_requests) refute conn.resp_body @@ -89,78 +99,81 @@ defmodule Pleroma.Plugs.RateLimiterTest do test "`bucket_name` option overrides default bucket name" do limiter_name = :test_bucket_name - Pleroma.Config.put([:rate_limit, limiter_name], {1000, 5}) - Pleroma.Config.put([Pleroma.Web.Endpoint, :http, :ip], {8, 8, 8, 8}) + Config.put([:rate_limit, limiter_name], {1000, 5}) + Config.put([Pleroma.Web.Endpoint, :http, :ip], {8, 8, 8, 8}) base_bucket_name = "#{limiter_name}:group1" - opts = RateLimiter.init(name: limiter_name, bucket_name: base_bucket_name) + plug_opts = RateLimiter.init(name: limiter_name, bucket_name: base_bucket_name) conn = conn(:get, "/") - RateLimiter.call(conn, opts) - assert {1, 4} = RateLimiter.inspect_bucket(conn, base_bucket_name, opts) - assert {:err, :not_found} = RateLimiter.inspect_bucket(conn, limiter_name, opts) + RateLimiter.call(conn, plug_opts) + assert {1, 4} = RateLimiter.inspect_bucket(conn, base_bucket_name, plug_opts) + assert {:err, :not_found} = RateLimiter.inspect_bucket(conn, limiter_name, plug_opts) end test "`params` option allows different queries to be tracked independently" do limiter_name = :test_params - Pleroma.Config.put([:rate_limit, limiter_name], {1000, 5}) - Pleroma.Config.put([Pleroma.Web.Endpoint, :http, :ip], {8, 8, 8, 8}) + Config.put([:rate_limit, limiter_name], {1000, 5}) + Config.put([Pleroma.Web.Endpoint, :http, :ip], {8, 8, 8, 8}) - opts = RateLimiter.init(name: limiter_name, params: ["id"]) + plug_opts = RateLimiter.init(name: limiter_name, params: ["id"]) conn = conn(:get, "/?id=1") conn = Plug.Conn.fetch_query_params(conn) conn_2 = conn(:get, "/?id=2") - RateLimiter.call(conn, opts) - assert {1, 4} = RateLimiter.inspect_bucket(conn, limiter_name, opts) - assert {0, 5} = RateLimiter.inspect_bucket(conn_2, limiter_name, opts) + RateLimiter.call(conn, plug_opts) + assert {1, 4} = RateLimiter.inspect_bucket(conn, limiter_name, plug_opts) + assert {0, 5} = RateLimiter.inspect_bucket(conn_2, limiter_name, plug_opts) end test "it supports combination of options modifying bucket name" do limiter_name = :test_options_combo - Pleroma.Config.put([:rate_limit, limiter_name], {1000, 5}) - Pleroma.Config.put([Pleroma.Web.Endpoint, :http, :ip], {8, 8, 8, 8}) + Config.put([:rate_limit, limiter_name], {1000, 5}) + Config.put([Pleroma.Web.Endpoint, :http, :ip], {8, 8, 8, 8}) base_bucket_name = "#{limiter_name}:group1" - opts = RateLimiter.init(name: limiter_name, bucket_name: base_bucket_name, params: ["id"]) + + plug_opts = + RateLimiter.init(name: limiter_name, bucket_name: base_bucket_name, params: ["id"]) + id = "100" conn = conn(:get, "/?id=#{id}") conn = Plug.Conn.fetch_query_params(conn) conn_2 = conn(:get, "/?id=#{101}") - RateLimiter.call(conn, opts) - assert {1, 4} = RateLimiter.inspect_bucket(conn, base_bucket_name, opts) - assert {0, 5} = RateLimiter.inspect_bucket(conn_2, base_bucket_name, opts) + RateLimiter.call(conn, plug_opts) + assert {1, 4} = RateLimiter.inspect_bucket(conn, base_bucket_name, plug_opts) + assert {0, 5} = RateLimiter.inspect_bucket(conn_2, base_bucket_name, plug_opts) end end describe "unauthenticated users" do test "are restricted based on remote IP" do limiter_name = :test_unauthenticated - Pleroma.Config.put([:rate_limit, limiter_name], [{1000, 5}, {1, 10}]) - Pleroma.Config.put([Pleroma.Web.Endpoint, :http, :ip], {8, 8, 8, 8}) + Config.put([:rate_limit, limiter_name], [{1000, 5}, {1, 10}]) + Config.put([Pleroma.Web.Endpoint, :http, :ip], {8, 8, 8, 8}) - opts = RateLimiter.init(name: limiter_name) + plug_opts = RateLimiter.init(name: limiter_name) conn = %{conn(:get, "/") | remote_ip: {127, 0, 0, 2}} conn_2 = %{conn(:get, "/") | remote_ip: {127, 0, 0, 3}} for i <- 1..5 do - conn = RateLimiter.call(conn, opts) - assert {^i, _} = RateLimiter.inspect_bucket(conn, limiter_name, opts) + conn = RateLimiter.call(conn, plug_opts) + assert {^i, _} = RateLimiter.inspect_bucket(conn, limiter_name, plug_opts) refute conn.halted end - conn = RateLimiter.call(conn, opts) + conn = RateLimiter.call(conn, plug_opts) assert %{"error" => "Throttled"} = Phoenix.ConnTest.json_response(conn, :too_many_requests) assert conn.halted - conn_2 = RateLimiter.call(conn_2, opts) - assert {1, 4} = RateLimiter.inspect_bucket(conn_2, limiter_name, opts) + conn_2 = RateLimiter.call(conn_2, plug_opts) + assert {1, 4} = RateLimiter.inspect_bucket(conn_2, limiter_name, plug_opts) refute conn_2.status == Plug.Conn.Status.code(:too_many_requests) refute conn_2.resp_body @@ -175,37 +188,37 @@ defmodule Pleroma.Plugs.RateLimiterTest do :ok end - test "can have limits seperate from unauthenticated connections" do - limiter_name = :test_authenticated + test "can have limits separate from unauthenticated connections" do + limiter_name = :test_authenticated1 scale = 50 limit = 5 - Pleroma.Config.put([Pleroma.Web.Endpoint, :http, :ip], {8, 8, 8, 8}) - Pleroma.Config.put([:rate_limit, limiter_name], [{1000, 1}, {scale, limit}]) + Config.put([Pleroma.Web.Endpoint, :http, :ip], {8, 8, 8, 8}) + Config.put([:rate_limit, limiter_name], [{1000, 1}, {scale, limit}]) - opts = RateLimiter.init(name: limiter_name) + plug_opts = RateLimiter.init(name: limiter_name) user = insert(:user) conn = conn(:get, "/") |> assign(:user, user) for i <- 1..5 do - conn = RateLimiter.call(conn, opts) - assert {^i, _} = RateLimiter.inspect_bucket(conn, limiter_name, opts) + conn = RateLimiter.call(conn, plug_opts) + assert {^i, _} = RateLimiter.inspect_bucket(conn, limiter_name, plug_opts) refute conn.halted end - conn = RateLimiter.call(conn, opts) + conn = RateLimiter.call(conn, plug_opts) assert %{"error" => "Throttled"} = Phoenix.ConnTest.json_response(conn, :too_many_requests) assert conn.halted end - test "diffrerent users are counted independently" do - limiter_name = :test_authenticated - Pleroma.Config.put([:rate_limit, limiter_name], [{1, 10}, {1000, 5}]) - Pleroma.Config.put([Pleroma.Web.Endpoint, :http, :ip], {8, 8, 8, 8}) + test "different users are counted independently" do + limiter_name = :test_authenticated2 + Config.put([:rate_limit, limiter_name], [{1, 10}, {1000, 5}]) + Config.put([Pleroma.Web.Endpoint, :http, :ip], {8, 8, 8, 8}) - opts = RateLimiter.init(name: limiter_name) + plug_opts = RateLimiter.init(name: limiter_name) user = insert(:user) conn = conn(:get, "/") |> assign(:user, user) @@ -214,16 +227,16 @@ defmodule Pleroma.Plugs.RateLimiterTest do conn_2 = conn(:get, "/") |> assign(:user, user_2) for i <- 1..5 do - conn = RateLimiter.call(conn, opts) - assert {^i, _} = RateLimiter.inspect_bucket(conn, limiter_name, opts) + conn = RateLimiter.call(conn, plug_opts) + assert {^i, _} = RateLimiter.inspect_bucket(conn, limiter_name, plug_opts) end - conn = RateLimiter.call(conn, opts) + conn = RateLimiter.call(conn, plug_opts) assert %{"error" => "Throttled"} = Phoenix.ConnTest.json_response(conn, :too_many_requests) assert conn.halted - conn_2 = RateLimiter.call(conn_2, opts) - assert {1, 4} = RateLimiter.inspect_bucket(conn_2, limiter_name, opts) + conn_2 = RateLimiter.call(conn_2, plug_opts) + assert {1, 4} = RateLimiter.inspect_bucket(conn_2, limiter_name, plug_opts) refute conn_2.status == Plug.Conn.Status.code(:too_many_requests) refute conn_2.resp_body refute conn_2.halted diff --git a/test/web/mastodon_api/controllers/account_controller_test.exs b/test/web/mastodon_api/controllers/account_controller_test.exs index 8625bb9cf..b3e796d37 100644 --- a/test/web/mastodon_api/controllers/account_controller_test.exs +++ b/test/web/mastodon_api/controllers/account_controller_test.exs @@ -673,10 +673,48 @@ defmodule Pleroma.Web.MastodonAPI.AccountControllerTest do assert json_response(res, 400) == %{"error" => "{\"email\":[\"has already been taken\"]}"} end - clear_config([Pleroma.Plugs.RemoteIp, :enabled]) + test "returns bad_request if missing required params", %{ + conn: conn, + valid_params: valid_params + } do + app_token = insert(:oauth_token, user: nil) - test "rate limit", %{conn: conn} do + conn = put_req_header(conn, "authorization", "Bearer " <> app_token.token) + + res = post(conn, "/api/v1/accounts", valid_params) + assert json_response(res, 200) + + [{127, 0, 0, 1}, {127, 0, 0, 2}, {127, 0, 0, 3}, {127, 0, 0, 4}] + |> Stream.zip(valid_params) + |> Enum.each(fn {ip, {attr, _}} -> + res = + conn + |> Map.put(:remote_ip, ip) + |> post("/api/v1/accounts", Map.delete(valid_params, attr)) + |> json_response(400) + + assert res == %{"error" => "Missing parameters"} + end) + end + + test "returns forbidden if token is invalid", %{conn: conn, valid_params: valid_params} do + conn = put_req_header(conn, "authorization", "Bearer " <> "invalid-token") + + res = post(conn, "/api/v1/accounts", valid_params) + assert json_response(res, 403) == %{"error" => "Invalid credentials"} + end + end + + describe "create account by app / rate limit" do + clear_config([Pleroma.Plugs.RemoteIp, :enabled]) do Pleroma.Config.put([Pleroma.Plugs.RemoteIp, :enabled], true) + end + + clear_config([:rate_limit, :app_account_creation]) do + Pleroma.Config.put([:rate_limit, :app_account_creation], {10_000, 2}) + end + + test "respects rate limit setting", %{conn: conn} do app_token = insert(:oauth_token, user: nil) conn = @@ -684,7 +722,7 @@ defmodule Pleroma.Web.MastodonAPI.AccountControllerTest do |> put_req_header("authorization", "Bearer " <> app_token.token) |> Map.put(:remote_ip, {15, 15, 15, 15}) - for i <- 1..5 do + for i <- 1..2 do conn = post(conn, "/api/v1/accounts", %{ username: "#{i}lain", @@ -718,37 +756,6 @@ defmodule Pleroma.Web.MastodonAPI.AccountControllerTest do assert json_response(conn, :too_many_requests) == %{"error" => "Throttled"} end - - test "returns bad_request if missing required params", %{ - conn: conn, - valid_params: valid_params - } do - app_token = insert(:oauth_token, user: nil) - - conn = put_req_header(conn, "authorization", "Bearer " <> app_token.token) - - res = post(conn, "/api/v1/accounts", valid_params) - assert json_response(res, 200) - - [{127, 0, 0, 1}, {127, 0, 0, 2}, {127, 0, 0, 3}, {127, 0, 0, 4}] - |> Stream.zip(valid_params) - |> Enum.each(fn {ip, {attr, _}} -> - res = - conn - |> Map.put(:remote_ip, ip) - |> post("/api/v1/accounts", Map.delete(valid_params, attr)) - |> json_response(400) - - assert res == %{"error" => "Missing parameters"} - end) - end - - test "returns forbidden if token is invalid", %{conn: conn, valid_params: valid_params} do - conn = put_req_header(conn, "authorization", "Bearer " <> "invalid-token") - - res = post(conn, "/api/v1/accounts", valid_params) - assert json_response(res, 403) == %{"error" => "Invalid credentials"} - end end describe "GET /api/v1/accounts/:id/lists - account_lists" do -- cgit v1.2.3 From c747260989fdba32a8f319f88f0840c811ff8b50 Mon Sep 17 00:00:00 2001 From: Ivan Tashkinov Date: Sat, 29 Feb 2020 22:04:09 +0300 Subject: [#2250] Tiny refactoring per merge request review. --- test/plugs/rate_limiter_test.exs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'test') diff --git a/test/plugs/rate_limiter_test.exs b/test/plugs/rate_limiter_test.exs index c0630c039..104d67611 100644 --- a/test/plugs/rate_limiter_test.exs +++ b/test/plugs/rate_limiter_test.exs @@ -109,7 +109,7 @@ defmodule Pleroma.Plugs.RateLimiterTest do RateLimiter.call(conn, plug_opts) assert {1, 4} = RateLimiter.inspect_bucket(conn, base_bucket_name, plug_opts) - assert {:err, :not_found} = RateLimiter.inspect_bucket(conn, limiter_name, plug_opts) + assert {:error, :not_found} = RateLimiter.inspect_bucket(conn, limiter_name, plug_opts) end test "`params` option allows different queries to be tracked independently" do -- cgit v1.2.3 From 4d416343fae4a9e0b1654b12bd476017be63a7e9 Mon Sep 17 00:00:00 2001 From: rinpatch Date: Fri, 28 Feb 2020 17:35:01 +0300 Subject: rate limiter: Fix a race condition When multiple requests are processed by rate limiter plug at the same time and the bucket is not yet initialized, both would try to initialize the bucket resulting in an internal server error. --- test/plugs/rate_limiter_test.exs | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) (limited to 'test') diff --git a/test/plugs/rate_limiter_test.exs b/test/plugs/rate_limiter_test.exs index 104d67611..8cdc8d1a2 100644 --- a/test/plugs/rate_limiter_test.exs +++ b/test/plugs/rate_limiter_test.exs @@ -242,4 +242,35 @@ defmodule Pleroma.Plugs.RateLimiterTest do refute conn_2.halted end end + + test "doesn't crash due to a race condition when multiple requests are made at the same time and the bucket is not yet initialized" do + limiter_name = :test_race_condition + Pleroma.Config.put([:rate_limit, limiter_name], {1000, 5}) + Pleroma.Config.put([Pleroma.Web.Endpoint, :http, :ip], {8, 8, 8, 8}) + + opts = RateLimiter.init(name: limiter_name) + + conn = conn(:get, "/") + conn_2 = conn(:get, "/") + + %Task{pid: pid1} = + task1 = + Task.async(fn -> + receive do + :process2_up -> + RateLimiter.call(conn, opts) + end + end) + + task2 = + Task.async(fn -> + send(pid1, :process2_up) + RateLimiter.call(conn_2, opts) + end) + + Task.await(task1) + Task.await(task2) + + refute {:err, :not_found} == RateLimiter.inspect_bucket(conn, limiter_name, opts) + end end -- cgit v1.2.3