diff --git a/server/granthi_link.py b/server/granthi_link.py index a2b4624..35342fa 100644 --- a/server/granthi_link.py +++ b/server/granthi_link.py @@ -90,9 +90,14 @@ DEFAULT_RATE_RULES = {"/v1/link": (5, 3600), "/v1/repos": (60, 3600), # Reads are cheap but still authenticated work. "/v1/devices": (120, 3600), "/v1/audit": (120, 3600), - # Revocation is a safety action: never throttle - # someone out of signing a lost laptop out. - "/v1/devices/revoke": (0, 3600)} + # Revocation is a safety action, so its limit is set + # high rather than tight. It is NOT 0: in this limiter + # 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 diff --git a/tests/test_server.py b/tests/test_server.py index bb902fb..3d27434 100644 --- a/tests/test_server.py +++ b/tests/test_server.py @@ -728,6 +728,62 @@ class TestTokenNameUniqueness(ServiceTestBase): self.assertIsNone(self.svc.whoami(a["token"])) 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__": unittest.main()