Support 202203 fixes 2 (#1158)

* fix internal error when adjusting rate limits downwards were the new limit would result in negative available requests for already tracked clients (#1126)

* remove debug output and unused variable

* expose CACHE_MAX_ENTRIES to be set via env, also implement lower limit sanity check for it (#1151)

* auth-id changes

* fix test data failure

Co-authored-by: Stefan Pratter <stefan@20c.com>

mainsite/settings/__init__.py

@@ -432,13 +432,21 @@ MEDIA_URL = f"/m/{PEERINGDB_VERSION}/"
 STATIC_ROOT = os.path.abspath(os.path.join(BASE_DIR, "static"))
 STATIC_URL = f"/s/{PEERINGDB_VERSION}/"
 
+# maximum number of entries in the cache
+set_option("CACHE_MAX_ENTRIES", 5000)
+
+# dont allow going below 5000 (#1151)
+if CACHE_MAX_ENTRIES < 5000:
+    raise ValueError("CACHE_MAX_ENTRIES needs to be >= 5000 (#1151)")
+
+
 CACHES = {
     "default": {
         "BACKEND": "django.core.cache.backends.db.DatabaseCache",
         "LOCATION": "django_cache",
         "OPTIONS": {
             # maximum number of entries in the cache
-            "MAX_ENTRIES": 5000,
+            "MAX_ENTRIES": CACHE_MAX_ENTRIES,
             # once max entries are reach delete 500 of the oldest entries
             "CULL_FREQUENCY": 10,
         },

peeringdb_server/exceptions.py

@@ -6,6 +6,7 @@ def format_wait_time(wait_time):
     """
     Format wait time in seconds to human readable format
     """
+
     if wait_time < 60:
         return f"{wait_time} seconds"
     elif wait_time < 3600 and wait_time > 60:

peeringdb_server/middleware.py

@@ -106,7 +106,7 @@ class PDBPermissionMiddleware(MiddlewareMixin):
         # session auth already exists, set x-auth-id value and return
 
         if request.user.is_authenticated:
-            request.auth_id = request.user.username
+            request.auth_id = f"u{request.user.id}"
 
             # request attempting to provide separate authentication while
             # already authenticated through session cookie, fail with
@@ -131,14 +131,17 @@ class PDBPermissionMiddleware(MiddlewareMixin):
             user = authenticate(username=username, password=password)
 
             # return username input in x-auth-id header
-            request.auth_id = username
+            if user:
+                request.auth_id = f"u{user.id}"
 
             # if user is not authenticated return 401 Unauthorized
-            if not user:
+            else:
 
                 # truncate the username if needed.
                 if len(username) > 255:
                     request.auth_id = username[:255]
+                else:
+                    request.auth_id = username
 
                 return self.response_unauthorized(
                     request, message="Invalid username or password", status=401
@@ -168,8 +171,14 @@ class PDBPermissionMiddleware(MiddlewareMixin):
                 )
 
             # If API key is provided, check if the user has an active session
-            if api_key:
-                request.auth_id = f"apikey_{api_key.prefix}"
+            else:
+
+                if isinstance(api_key, OrganizationAPIKey):
+                    prefix = f"o{api_key.org_id}"
+                else:
+                    prefix = f"u{api_key.user_id}"
+
+                request.auth_id = f"{prefix}_apikey_{api_key.prefix}"
                 if request.session.get("_auth_user_id") and request.user.id:
                     if int(request.user.id) == int(
                         request.session.get("_auth_user_id")

peeringdb_server/rest_throttles.py

@@ -55,6 +55,42 @@ class TargetedRateThrottle(throttling.SimpleRateThrottle):
             msg_setting
         )
 
+    def wait(self):
+        """
+        Returns the recommended next request time in seconds.
+
+        This is a custom implmentation of the original wait() logic that can
+        also handle dynamic downward adjustments of rate limits (through
+        changing EnvironmentSetting variables for example)
+        """
+        if self.history:
+            remaining_duration = self.duration - (self.now - self.history[-1])
+        else:
+            remaining_duration = self.duration
+
+        available_requests = self.num_requests - len(self.history) + 1
+        if available_requests < 1:
+
+            # a negative/zero value only occurs when rate limit has been adjusted
+            # while already being tracked for the requesting client
+
+            remaining_duration = self.duration
+            new_history = []
+            for entry in self.history[: self.num_requests]:
+                diff = self.now - entry
+                if diff < self.duration:
+                    new_history.append(entry)
+                    remaining_duration = self.duration - diff
+
+            self.history = new_history
+            self.cache.set(self.key, self.history, self.duration)
+            available_requests = self.num_requests - len(self.history) + 1
+
+        if available_requests < 1:
+            return None
+
+        return remaining_duration / float(available_requests)
+
     def _allow_request_user_auth(self, request, view, ident_prefix=""):
         self.ident = f"{ident_prefix}user:{self.user.pk}"
         self.scope = self.scope_user

tests/test_api_throttle.py

@@ -66,11 +66,11 @@ class APIThrottleTests(TestCase):
         cache.clear()
 
         self.factory = APIRequestFactory()
-        env = models.EnvironmentSetting(
+        self.rate_anon = env = models.EnvironmentSetting(
             setting="API_THROTTLE_RATE_ANON", value_str="10/minute"
         )
         env.save()
-        env = models.EnvironmentSetting(
+        self.rate_user = env = models.EnvironmentSetting(
             setting="API_THROTTLE_RATE_USER", value_str="10/minute"
         )
         env.save()
@@ -139,6 +139,64 @@ class APIThrottleTests(TestCase):
         assert response.status_code == 429
         assert "Rate limit exceeded (anon)" in response.data["message"]
 
+    def test_anon_requests_above_throttle_rate_dynamic_changes(self):
+        """
+        Ensure request rate is limited for anonymous users while
+        changing the rate between requests
+        """
+
+        request = self.factory.get("/")
+        for dummy in range(11):
+            response = MockView.as_view({"get": "get"})(request)
+        assert response.status_code == 429
+        assert "Rate limit exceeded (anon)" in response.data["message"]
+
+        # adjust rate limit downwards
+
+        self.rate_anon.value_str = "1/minute"
+        self.rate_anon.save()
+
+        # still rate limited, no error
+
+        response = MockView.as_view({"get": "get"})(request)
+        assert response.status_code == 429
+
+        # adjust rate limit upwards
+
+        self.rate_anon.value_str = "100/minute"
+        self.rate_anon.save()
+
+        # no longer rate limited, no error
+
+        response = MockView.as_view({"get": "get"})(request)
+        assert response.status_code == 200
+
+        # adjust rate limit downwards (change duration)
+
+        self.rate_anon.value_str = "1/hour"
+        self.rate_anon.save()
+
+        # rate limited again, no error
+
+        response = MockView.as_view({"get": "get"})(request)
+        assert response.status_code == 429
+
+        # adjust rate limit upwards (change duration)
+
+        self.rate_anon.value_str = "20/hour"
+        self.rate_anon.save()
+
+        # no longer rate limited (19 attempts), no error
+
+        for idx in range(19):
+            response = MockView.as_view({"get": "get"})(request)
+            assert response.status_code == 200
+
+        # rate limited again, no error
+
+        response = MockView.as_view({"get": "get"})(request)
+        assert response.status_code == 429
+
     def test_authenticated_requests_above_throttle_rate(self):
         """
         Ensure request rate is not limited for authenticated users
@@ -153,6 +211,66 @@ class APIThrottleTests(TestCase):
         assert response.status_code == 429
         assert "Rate limit exceeded (user)" in response.data["message"]
 
+    def test_authenticated_requests_above_throttle_rate_dynamic_changes(self):
+        """
+        Ensure request rate is not limited for authenticated users
+        """
+
+        user = models.User(username="test")
+        user.save()
+        request = self.factory.get("/")
+        request.user = user
+        for dummy in range(11):
+            response = MockView.as_view({"get": "get"})(request)
+        assert response.status_code == 429
+        assert "Rate limit exceeded (user)" in response.data["message"]
+
+        # adjust rate limit downwards
+
+        self.rate_user.value_str = "1/minute"
+        self.rate_user.save()
+
+        # still rate limited, no error
+
+        response = MockView.as_view({"get": "get"})(request)
+        assert response.status_code == 429
+
+        # adjust rate limit upwards
+
+        self.rate_user.value_str = "100/minute"
+        self.rate_user.save()
+
+        # no longer rate limited, no error
+
+        response = MockView.as_view({"get": "get"})(request)
+        assert response.status_code == 200
+
+        # adjust rate limit downwards (change duration)
+
+        self.rate_user.value_str = "1/hour"
+        self.rate_user.save()
+
+        # rate limited again, no error
+
+        response = MockView.as_view({"get": "get"})(request)
+        assert response.status_code == 429
+
+        # adjust rate limit upwards (change duration)
+
+        self.rate_user.value_str = "20/hour"
+        self.rate_user.save()
+
+        # no longer rate limited (19 attempts), no error
+
+        for idx in range(19):
+            response = MockView.as_view({"get": "get"})(request)
+            assert response.status_code == 200
+
+        # rate limited again, no error
+
+        response = MockView.as_view({"get": "get"})(request)
+        assert response.status_code == 429
+
     def test_response_size_ip_block(self):
         """
         Ensure request rate is limited based on response size

tests/test_middleware.py

@@ -10,7 +10,8 @@ from django.test import (
 from rest_framework.test import APIClient, APITestCase
 
 from peeringdb_server.middleware import PDBCommonMiddleware
-from peeringdb_server.models import User, UserAPIKey
+from peeringdb_server.models import Organization, OrganizationAPIKey, User, UserAPIKey
+from .util import reset_group_ids
 
 
 def get_response_empty(request):
@@ -67,7 +68,31 @@ class PDBPermissionMiddlewareTest(APITestCase):
         self.client.credentials(HTTP_AUTHORIZATION=f"Api-Key {key}")
         response = self.client.get("/api/fac")
         self.assertEqual(response.status_code, 200)
-        assert response.headers.get("X-Auth-ID").startswith("apikey_")
+        assert (
+            response.headers.get("X-Auth-ID") == f"u{user.id}_apikey_{api_key.prefix}"
+        )
+
+        # test that header gets cleared between requests
+        other_client = APIClient()
+        response = other_client.get("/api/fac")
+        self.assertEqual(response.status_code, 200)
+        assert response.headers.get("X-Auth-ID") is None
+
+    def test_auth_id_org_api_key(self):
+        reset_group_ids()
+
+        org = Organization.objects.create(name="Test org", status="ok")
+
+        # Create an API key for the user
+        api_key, key = OrganizationAPIKey.objects.create_key(
+            name="test",
+            org=org,
+        )
+
+        self.client.credentials(HTTP_AUTHORIZATION=f"Api-Key {key}")
+        response = self.client.get("/api/fac")
+        self.assertEqual(response.status_code, 200)
+        assert response.headers.get("X-Auth-ID") == f"o{org.id}_apikey_{api_key.prefix}"
 
         # test that header gets cleared between requests
         other_client = APIClient()
@@ -83,7 +108,7 @@ class PDBPermissionMiddlewareTest(APITestCase):
         self.client.force_login(user)
         response = self.client.get("/api/fac")
         self.assertEqual(response.status_code, 200)
-        assert response.headers.get("X-Auth-ID") == user.username
+        assert response.headers.get("X-Auth-ID") == f"u{user.id}"
 
         # test that header gets cleared between requests
         other_client = APIClient()
@@ -101,7 +126,7 @@ class PDBPermissionMiddlewareTest(APITestCase):
 
         response = self.client.get("/api/fac")
         self.assertEqual(response.status_code, 200)
-        assert response.headers.get("X-Auth-ID") == user.username
+        assert response.headers.get("X-Auth-ID") == f"u{user.id}"
 
         # test that header gets cleared between requests
         other_client = APIClient()