fix(link): revoke endpoint was disabled by its own rate-limit rule
Live QA: every call to /v1/devices/revoke returned 429 retry_after=3600. The rule was written (0, 3600) with a comment saying 'never throttle someone out of signing a lost laptop out' -- but in this limiter a limit of 0 DISABLES the endpoint outright. The comment said unlimited; the code said never. Set to 600/hour instead. Every unit test passed while the endpoint was 100% dead over HTTP, because they called svc.revoke_device() directly and never went through the handler. Added 4 tests that speak HTTP, including one that fails if ANY route in DEFAULT_RATE_RULES is configured to 0. 176 tests (was 172).
This commit is contained in:
@@ -90,9 +90,14 @@ DEFAULT_RATE_RULES = {"/v1/link": (5, 3600), "/v1/repos": (60, 3600),
|
|||||||
# Reads are cheap but still authenticated work.
|
# Reads are cheap but still authenticated work.
|
||||||
"/v1/devices": (120, 3600),
|
"/v1/devices": (120, 3600),
|
||||||
"/v1/audit": (120, 3600),
|
"/v1/audit": (120, 3600),
|
||||||
# Revocation is a safety action: never throttle
|
# Revocation is a safety action, so its limit is set
|
||||||
# someone out of signing a lost laptop out.
|
# high rather than tight. It is NOT 0: in this limiter
|
||||||
"/v1/devices/revoke": (0, 3600)}
|
# a limit of 0 DISABLES the endpoint outright (see
|
||||||
|
# RateLimiter.check), which would mean nobody could
|
||||||
|
# ever sign a lost laptop out. Caught in live QA --
|
||||||
|
# unit tests called the service method directly and so
|
||||||
|
# never went through the limiter at all.
|
||||||
|
"/v1/devices/revoke": (600, 3600)}
|
||||||
MAX_RATE_KEYS = 10000
|
MAX_RATE_KEYS = 10000
|
||||||
|
|
||||||
|
|
||||||
|
|||||||
@@ -728,6 +728,62 @@ class TestTokenNameUniqueness(ServiceTestBase):
|
|||||||
self.assertIsNone(self.svc.whoami(a["token"]))
|
self.assertIsNone(self.svc.whoami(a["token"]))
|
||||||
self.assertEqual(self.svc.whoami(b["token"]), "alice")
|
self.assertEqual(self.svc.whoami(b["token"]), "alice")
|
||||||
|
|
||||||
|
|
||||||
|
class TestDeviceEndpointsOverHttp(HandlerTestBase):
|
||||||
|
"""Through the real handler, not the service method.
|
||||||
|
|
||||||
|
The first live run of the revoke endpoint returned 429 on every call
|
||||||
|
while every unit test passed: the tests called svc.revoke_device()
|
||||||
|
directly, so nothing ever went through the rate limiter. Any route whose
|
||||||
|
limit is a policy decision needs at least one test that speaks HTTP.
|
||||||
|
"""
|
||||||
|
|
||||||
|
def setUp(self):
|
||||||
|
super().setUp()
|
||||||
|
self.enable_test_mode()
|
||||||
|
self.a = self.stub_link("s1", "alice", device="laptop",
|
||||||
|
device_id="dev-a")[1]
|
||||||
|
self.b = self.stub_link("s1", "alice", device="desktop",
|
||||||
|
device_id="dev-b")[1]
|
||||||
|
|
||||||
|
def post(self, path, obj):
|
||||||
|
body = json.dumps(obj).encode()
|
||||||
|
return self.raw_post(path, body,
|
||||||
|
{"Content-Type": "application/json",
|
||||||
|
"Content-Length": str(len(body))})
|
||||||
|
|
||||||
|
def test_revoke_is_reachable_and_not_rate_limited_away(self):
|
||||||
|
status, resp = self.post("/v1/devices/revoke",
|
||||||
|
{"token": self.b["token"],
|
||||||
|
"device_id": "dev-a"})
|
||||||
|
self.assertEqual(status, 200, resp)
|
||||||
|
self.assertIsNone(self.svc.whoami(self.a["token"]))
|
||||||
|
|
||||||
|
def test_repeated_revokes_keep_working(self):
|
||||||
|
"""A person signing several lost machines out in one sitting must not
|
||||||
|
be locked out partway through."""
|
||||||
|
for i in range(12):
|
||||||
|
self.stub_link("s1", "alice", device=f"d{i}", device_id=f"gone-{i}")
|
||||||
|
for i in range(12):
|
||||||
|
status, resp = self.post("/v1/devices/revoke",
|
||||||
|
{"token": self.b["token"],
|
||||||
|
"device_id": f"gone-{i}"})
|
||||||
|
self.assertEqual(status, 200, f"revoke {i}: {resp}")
|
||||||
|
|
||||||
|
def test_devices_and_audit_are_reachable_over_http(self):
|
||||||
|
status, resp = self.post("/v1/devices", {"token": self.a["token"]})
|
||||||
|
self.assertEqual(status, 200)
|
||||||
|
self.assertEqual(len(resp["devices"]), 2)
|
||||||
|
status, resp = self.post("/v1/audit", {"token": self.a["token"]})
|
||||||
|
self.assertEqual(status, 200)
|
||||||
|
self.assertTrue(resp["events"])
|
||||||
|
|
||||||
|
def test_no_route_a_client_uses_is_configured_to_zero(self):
|
||||||
|
"""0 means DISABLED in this limiter, so a zero on a live route is a
|
||||||
|
dead endpoint, not an unlimited one."""
|
||||||
|
for route, (limit, _window) in granthi_link.DEFAULT_RATE_RULES.items():
|
||||||
|
self.assertGreater(limit, 0, f"{route} is disabled by its rule")
|
||||||
|
|
||||||
if __name__ == "__main__":
|
if __name__ == "__main__":
|
||||||
unittest.main()
|
unittest.main()
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user