feat: allow admins to set API key limit by minute (#704)

* feat: Views show API key in per-minute format regardless of rate_limiter interval

apps/api_web/lib/api_web/api_view_helpers.ex

@@ -240,27 +240,38 @@ defmodule ApiWeb.ApiViewHelpers do
     |> String.replace("/", "%2F")
   end
 
-  def limit(%ApiAccounts.Key{} = key) do
+  def default_registered_per_minute do
+    ApiWeb.RateLimiter.max_registered_per_interval() * ApiWeb.RateLimiter.intervals_per_minute()
+  end
+
+  @doc """
+  Returns the maximum number of requests a key represented as a per-minute limit
+  """
+  @spec max_requests_per_minute(ApiWeb.User.t()) :: non_neg_integer()
+  def max_requests_per_minute(%ApiWeb.User{} = user) do
+    ApiWeb.RateLimiter.max_requests(user) * ApiWeb.RateLimiter.intervals_per_minute()
+  end
+
+  def per_minute_limit(%ApiAccounts.Key{} = key) do
     key
     |> ApiWeb.User.from_key()
-    |> ApiWeb.RateLimiter.max_requests()
+    |> max_requests_per_minute()
     |> trunc()
   end
 
-  def interval_name(clear_interval \\ ApiWeb.config(:rate_limiter, :clear_interval)) do
-    case clear_interval do
-      60_000 ->
-        "Per-Minute Limit"
-
-      3_600_000 ->
-        "Hourly Limit"
+  def per_minute_limit_value(%ApiAccounts.Key{} = key) do
+    key
+    |> ApiWeb.User.from_key()
+    |> limit_or_default_of_nil()
+  end
 
-      86_400_000 ->
-        "Daily Limit"
+  defp limit_or_default_of_nil(%ApiWeb.User{limit: nil}) do
+    nil
+  end
 
-      clear_interval ->
-        second_limit = Float.round(clear_interval / 1000, 2)
-        "Requests Per #{second_limit} Seconds"
-    end
+  defp limit_or_default_of_nil(%ApiWeb.User{} = user) do
+    user
+    |> max_requests_per_minute()
+    |> trunc()
   end
 end

apps/api_web/lib/api_web/controllers/admin/accounts/key_controller.ex

@@ -29,7 +29,11 @@ defmodule ApiWeb.Admin.Accounts.KeyController do
   end
 
   def update(conn, %{"id" => key_id, "key" => key_params}) do
-    key_params = Map.put(key_params, :rate_request_pending, false)
+    key_params =
+      key_params
+      |> Map.put(:rate_request_pending, false)
+      |> handle_per_minute
+
     key = ApiAccounts.get_key!(key_id)
     user = conn.assigns.user
     # normally we would do this validation in ApiAccounts, but that
@@ -60,6 +64,35 @@ defmodule ApiWeb.Admin.Accounts.KeyController do
     end
   end
 
+  # On update, an admin can provide a per_minute_limit
+  # that is stored as the daily_limit
+  defp handle_per_minute(%{"per_minute_limit" => p_m_limit} = key_params)
+       when is_binary(p_m_limit) do
+    case Integer.parse(p_m_limit) do
+      {int_p_m_limit, ""} ->
+        key_params
+        |> per_minute_to_daily(int_p_m_limit)
+
+      _ ->
+        key_params
+    end
+  end
+
+  defp handle_per_minute(%{"per_minute_limit" => p_m_limit} = key_params)
+       when is_integer(p_m_limit) do
+    per_minute_to_daily(key_params, p_m_limit)
+  end
+
+  defp handle_per_minute(key_params) do
+    key_params
+  end
+
+  defp per_minute_to_daily(key_params, int_p_m_limit) do
+    key_params
+    |> Map.put(:daily_limit, int_p_m_limit * 60 * 24)
+    |> Map.delete(:per_minute_limit)
+  end
+
   def clone(conn, %{"id" => key_id}) do
     key = ApiAccounts.get_key!(key_id)
     user = conn.assigns.user

apps/api_web/lib/api_web/rate_limiter.ex

@@ -10,6 +10,7 @@ defmodule ApiWeb.RateLimiter do
   @clear_interval ApiWeb.config(:rate_limiter, :clear_interval)
   @wait_time_ms ApiWeb.config(:rate_limiter, :wait_time_ms)
   @intervals_per_day div(86_400_000, @clear_interval)
+  @intervals_per_minute div(60_000, @clear_interval)
   @max_anon_per_interval ApiWeb.config(:rate_limiter, :max_anon_per_interval)
   @max_registered_per_interval ApiWeb.config(:rate_limiter, :max_registered_per_interval)
 
@@ -76,6 +77,9 @@ defmodule ApiWeb.RateLimiter do
   @doc false
   def intervals_per_day, do: @intervals_per_day
 
+  @doc false
+  def intervals_per_minute, do: @intervals_per_minute
+
   if Mix.env() == :test do
     @doc "Helper function for testing, to clear the limiter state."
     def force_clear do

apps/api_web/lib/api_web/templates/admin/accounts/key/form.html.heex

@@ -6,8 +6,15 @@
   <% end %>
 
   <div class="form-group">
-    <%= label(f, :daily_limit, class: "control-label") %>
-    <%= text_input(f, :daily_limit, class: "form-control") %>
+    <%= label(
+      f,
+      "Per-Minute Limit (default #{ApiWeb.ApiViewHelpers.default_registered_per_minute()})",
+      class: "control-label"
+    ) %>
+    <%= text_input(f, :per_minute_limit,
+      value: ApiWeb.ApiViewHelpers.per_minute_limit_value(@changeset.data),
+      class: "form-control"
+    ) %>
     <%= error_tag(f, :daily_limit) %>
   </div>
 

apps/api_web/lib/api_web/templates/admin/accounts/user/show.html.heex

@@ -109,7 +109,7 @@
         <tr>
           <th>Key</th>
           <th>Version</th>
-          <th><%= ApiWeb.ApiViewHelpers.interval_name() %></th>
+          <th>Per-Minute Limit</th>
           <th>Created</th>
           <th>Requested On</th>
           <th>Approved</th>
@@ -125,7 +125,7 @@
               <div><%= key.description %></div>
             </td>
             <td><%= key.api_version %></td>
-            <td><%= ApiWeb.ApiViewHelpers.limit(key) %></td>
+            <td><%= ApiWeb.ApiViewHelpers.per_minute_limit(key) %></td>
             <td><%= key.created %></td>
             <td><%= key.requested_date %></td>
             <td>

apps/api_web/lib/api_web/templates/client_portal/portal/index.html.heex

@@ -15,7 +15,7 @@
   <thead>
     <tr>
       <th>Key</th>
-      <th><%= ApiWeb.ApiViewHelpers.interval_name() %></th>
+      <th>Per-Minute Limit</th>
       <th>Version</th>
       <th>Created</th>
       <th>Allowed Domains</th>
@@ -31,7 +31,7 @@
           <div><%= key.description %></div>
         </td>
         <td>
-          <%= ApiWeb.ApiViewHelpers.limit(key) %>
+          <%= ApiWeb.ApiViewHelpers.per_minute_limit(key) %>
           <%= unless key.rate_request_pending do %>
             <%= link("Request Increase",
               to: key_path(@conn, :request_increase, key),

apps/api_web/test/api_web/controllers/admin/accounts/key_controller_test.exs

@@ -131,6 +131,43 @@ defmodule ApiWeb.Admin.Accounts.KeyControllerTest do
     refute new_key.rate_request_pending
   end
 
+  test "can edit the daily limit by providing a per-minute limit", %{conn: base_conn, user: user} do
+    {:ok, key} = ApiAccounts.create_key(user)
+    {:ok, key} = ApiAccounts.update_key(key, %{rate_request_pending: true})
+    ApiAccounts.Keys.cache_key(key)
+
+    conn =
+      base_conn
+      |> form_header()
+      |> put(admin_key_path(base_conn, :update, user, key), key: %{per_minute_limit: "10001"})
+
+    assert redirected_to(conn) == admin_user_path(conn, :show, user)
+    assert {:ok, new_key} = ApiAccounts.Keys.fetch_valid_key(key.key)
+    assert new_key.daily_limit == 10_001 * 60 * 24
+    refute Map.has_key?(new_key, :per_minute_limit)
+    refute new_key.rate_request_pending
+  end
+
+  test "can edit the daily limit by providing a per-minute limit as an integer", %{
+    conn: base_conn,
+    user: user
+  } do
+    {:ok, key} = ApiAccounts.create_key(user)
+    {:ok, key} = ApiAccounts.update_key(key, %{rate_request_pending: true})
+    ApiAccounts.Keys.cache_key(key)
+
+    conn =
+      base_conn
+      |> form_header()
+      |> put(admin_key_path(base_conn, :update, user, key), key: %{per_minute_limit: 10_001})
+
+    assert redirected_to(conn) == admin_user_path(conn, :show, user)
+    assert {:ok, new_key} = ApiAccounts.Keys.fetch_valid_key(key.key)
+    assert new_key.daily_limit == 10_001 * 60 * 24
+    refute Map.has_key?(new_key, :per_minute_limit)
+    refute new_key.rate_request_pending
+  end
+
   test "can edit the API version", %{conn: base_conn, user: user} do
     {:ok, key} = ApiAccounts.create_key(user)
     ApiAccounts.Keys.cache_key(key)

apps/api_web/test/api_web/controllers/portal/portal_controller_test.exs

@@ -9,18 +9,24 @@ defmodule ApiWeb.Controllers.Portal.PortalControllerTest do
       assert html_response(conn, 200) =~ "Api Keys"
     end
 
-    test "index displays default key limit per interval of 100000", %{user: user, conn: conn} do
+    test "index displays default key limit per minute", %{
+      user: user,
+      conn: conn
+    } do
       {:ok, key} = ApiAccounts.create_key(user)
       {:ok, _} = ApiAccounts.update_key(key, %{approved: true})
       conn = get(conn, portal_path(conn, :index))
-      assert html_response(conn, 200) =~ "100000"
+      max = ApiWeb.config(:rate_limiter, :max_registered_per_interval)
+      interval = ApiWeb.config(:rate_limiter, :clear_interval)
+      per_interval_minute = div(max, interval) * 60_000
+      assert html_response(conn, 200) =~ "#{per_interval_minute}"
     end
 
     test "index displays dynamic key limit", %{user: user, conn: conn} do
       {:ok, key} = ApiAccounts.create_key(user)
       {:ok, _} = ApiAccounts.update_key(key, %{approved: true, daily_limit: 999_999_999_999})
       conn = get(conn, portal_path(conn, :index))
-      assert html_response(conn, 200) =~ "1157407"
+      assert html_response(conn, 200) =~ "694444200"
     end
   end
 

apps/api_web/test/api_web/views/api_view_helpers_test.exs

@@ -56,26 +56,21 @@ defmodule ApiWeb.ApiViewHelpersTest do
     assert url_safe_id(struct, %{}) == expected
   end
 
-  describe "interval_name/1" do
-    test "returns per-minute limit" do
-      assert interval_name(60_000) == "Per-Minute Limit"
-    end
-
-    test "returns hourly limit" do
-      assert interval_name(3_600_000) == "Hourly Limit"
-    end
+  test "per_minute_limit/1 returns properly formatted rate limit per key" do
+    key = %ApiAccounts.Key{key: @api_key}
 
-    test "returns daily limit" do
-      assert interval_name(86_400_000) == "Daily Limit"
-    end
+    assert per_minute_limit(key) ==
+             ApiWeb.config(:rate_limiter, :max_registered_per_interval) *
+               ApiWeb.RateLimiter.intervals_per_minute()
+  end
 
-    test "truncates to the second if doesn't match a simple case" do
-      assert interval_name(2_756) == "Requests Per 2.76 Seconds"
-    end
+  test "per_minute_limit_value/1 returns nil if daily_limit is not set" do
+    key = %ApiAccounts.Key{key: @api_key, daily_limit: nil}
+    assert per_minute_limit_value(key) == nil
   end
 
-  test "limit/1 returns proplery formated rate limit per key" do
-    key = %ApiAccounts.Key{key: @api_key}
-    assert limit(key) == ApiWeb.config(:rate_limiter, :max_registered_per_interval)
+  test "per_minute_limit_value/1 returns formatted limit value if set" do
+    key = %ApiAccounts.Key{key: @api_key, daily_limit: 100_000}
+    assert per_minute_limit_value(key) == per_minute_limit(key)
   end
 end