diff --git a/reboot/aio/http.py b/reboot/aio/http.py index e4d91d2ec..13fc538ac 100644 --- a/reboot/aio/http.py +++ b/reboot/aio/http.py @@ -66,6 +66,11 @@ class APIRoute: path: str kwargs: dict endpoint: Callable[..., Any] + # Whether the endpoint is handed an *app-internal* + # `ExternalContext` (one carrying the application's + # `caller_id`) instead of the external one. See the DANGER note + # in `HTTP._api_route`. + app_internal: bool = False @dataclass(kw_only=True, frozen=True) class Mount: @@ -88,11 +93,6 @@ class HTTP: def __init__(self): self._api_routes: list[PythonWebFramework.APIRoute] = [] self._mounts: list[PythonWebFramework.Mount] = [] - # Exact request paths whose handlers receive an *app-internal* - # context (one that can call app-internal-only servicers) - # instead of the usual external one, because they opted in via - # `app_internal=True`. See the DANGER note in `_api_route`. - self._app_internal_paths: set[str] = set() def _api_route(self, path: str, **kwargs): # `app_internal` is our own kwarg, not one of FastAPI's, so we @@ -100,6 +100,10 @@ def _api_route(self, path: str, **kwargs): # handler is given an *app-internal* `ExternalContext` (one # carrying the application's `caller_id`, able to call # app-internal-only servicers) instead of the external one. + # The grant is bound to the endpoint: it reaches exactly the + # requests that Starlette dispatches to this handler, however + # its path is spelled (a template such as `/things/{id}` + # included). # # DANGER: an app-internal context bypasses authorizers, so a # route that gets one can make trusted in-app calls on behalf @@ -109,8 +113,7 @@ def _api_route(self, path: str, **kwargs): # only after the authorization code has been exchanged and # validated. Never set it on a route that acts on unvalidated # request input. - if kwargs.pop("app_internal", False): - self._app_internal_paths.add(path) + app_internal: bool = kwargs.pop("app_internal", False) # TODO: add type annotations for `endpoint` so that what # we take in is exactly what we return. @@ -127,6 +130,7 @@ def decorator(endpoint): path=path, endpoint=endpoint, kwargs=kwargs, + app_internal=app_internal, ) ) return endpoint @@ -271,26 +275,34 @@ def app_internal_external_context_from_request( caller_id=CallerID(application_id=application_id), ) + def app_internal_external_context_dependency(request: Request): + # A route-level dependency that hands its endpoint an + # app-internal context. FastAPI resolves it after Starlette + # has dispatched the request to that endpoint and before the + # endpoint runs, so the grant reaches exactly the requests + # the endpoint handles. + request.state.reboot_external_context = ( + app_internal_external_context_from_request(request) + ) + fastapi = FastAPI() @fastapi.middleware("http") async def external_context_middleware(request: Request, call_next): - # Most routes get an *external* context (no `caller_id`): an - # HTTP handler serves untrusted external traffic, so handing it - # a caller that bypasses authorizers would let external - # requests escalate to trusted in-app calls. Those routes must - # do their own end-user auth. Only routes that opted in via - # `app_internal=True` get an *app-internal* context instead — - # see the DANGER note on `HTTP._api_route`. We namespace this - # on `request.state` so other middleware doesn't clash. - if request.url.path in self._http._app_internal_paths: - request.state.reboot_external_context = ( - app_internal_external_context_from_request(request) - ) - else: - request.state.reboot_external_context = ( - external_context_from_request(request) - ) + # Every request gets an *external* context (no `caller_id`): + # an HTTP handler serves untrusted external traffic, so + # handing it a caller that bypasses authorizers would let + # external requests escalate to trusted in-app calls. Routes + # must do their own end-user auth. Only routes that opted in + # via `app_internal=True` get an *app-internal* context + # instead, which `app_internal_external_context_dependency` + # puts in place once the request has been routed to such an + # endpoint — see the DANGER note on `HTTP._api_route`. We + # namespace this on `request.state` so other middleware + # doesn't clash. + request.state.reboot_external_context = ( + external_context_from_request(request) + ) return await call_next(request) @@ -304,10 +316,18 @@ async def external_context_middleware(request: Request, call_next): ) for api_route in self._http._api_routes: + kwargs = dict(api_route.kwargs) + if api_route.app_internal: + # Ahead of any dependencies the route declared itself, + # so those already see the app-internal context. + kwargs["dependencies"] = [ + Depends(app_internal_external_context_dependency), + *(kwargs.get("dependencies") or []), + ] fastapi.add_api_route( api_route.path, api_route.endpoint, - **api_route.kwargs, + **kwargs, ) config = uvicorn.Config( diff --git a/tests/reboot/aio/BUILD.bazel b/tests/reboot/aio/BUILD.bazel index 069fd4dec..75322339b 100644 --- a/tests/reboot/aio/BUILD.bazel +++ b/tests/reboot/aio/BUILD.bazel @@ -1,3 +1,4 @@ +load("@rbt_pypi//:requirements.bzl", "requirement") load("@rules_python//python:defs.bzl", "py_test") py_test( @@ -61,6 +62,21 @@ py_test( ], ) +py_test( + name = "http_app_internal_test_py", + timeout = "short", + srcs = [":http_app_internal_test.py"], + main = "http_app_internal_test.py", + deps = [ + "//reboot/aio:applications_py", + "//reboot/aio:external_py", + "//reboot/aio:http_py", + "//reboot/aio:tests_py", + "//tests/reboot:greeter_servicers_py", + requirement("httpx"), + ], +) + py_test( name = "caller_id_test_py", timeout = "short", diff --git a/tests/reboot/aio/http_app_internal_test.py b/tests/reboot/aio/http_app_internal_test.py new file mode 100644 index 000000000..62b62b77c --- /dev/null +++ b/tests/reboot/aio/http_app_internal_test.py @@ -0,0 +1,93 @@ +""" +Tests which `ExternalContext` a custom HTTP route is handed: a route +registered with `app_internal=True` gets an app-internal one (carrying +the application's `caller_id`), whatever the shape of its path, while a +route registered without it gets an external one even when it lives +under the same path prefix as an app-internal route. +""" + +import httpx +import unittest +from reboot.aio.applications import Application +from reboot.aio.external import ExternalContext +from reboot.aio.http import InjectExternalContext +from reboot.aio.tests import Reboot +from tests.reboot.greeter_servicers import MyGreeterServicer + +# Generous per-request HTTP timeout: each request crosses a full Reboot +# cluster plus a local Envoy, which can take a while on a loaded CI +# runner, and Bazel's test timeout remains the backstop against a hang. +_HTTP_TIMEOUT_SECONDS = 30.0 + + +def _describe(context: ExternalContext) -> dict[str, bool]: + """The kind of context a handler was given: an app-internal context + carries the application's `caller_id`, an external one carries + none.""" + return {"app_internal": context.caller_id is not None} + + +class HTTPAppInternalTest(unittest.IsolatedAsyncioTestCase): + + async def asyncSetUp(self) -> None: + self.rbt = Reboot() + await self.rbt.start() + + application = Application(servicers=[MyGreeterServicer]) + + # Starlette dispatches to the first route whose path matches, + # so the static routes are registered ahead of the + # parameterized one that would otherwise capture them. + @application.http.get("/__/test/static", app_internal=True) + def static(context: ExternalContext = InjectExternalContext): + return _describe(context) + + @application.http.get("/__/test/plain") + def plain(context: ExternalContext = InjectExternalContext): + return _describe(context) + + @application.http.get("/__/test/{item}", app_internal=True) + def parameterized( + item: str, + context: ExternalContext = InjectExternalContext, + ): + return _describe(context) + + await self.rbt.up(application) + + async def asyncTearDown(self) -> None: + await self.rbt.stop() + + async def _get(self, path: str) -> dict[str, bool]: + async with httpx.AsyncClient(timeout=_HTTP_TIMEOUT_SECONDS) as client: + response = await client.get(self.rbt.http_localhost_url(path)) + self.assertEqual(200, response.status_code, response.text) + return response.json() + + async def test_static_app_internal_route(self) -> None: + self.assertEqual( + {"app_internal": True}, + await self._get("/__/test/static"), + ) + + async def test_parameterized_app_internal_route(self) -> None: + # The route's path is a template (`/__/test/{item}`); the + # request's path is concrete, so an exact path lookup would + # never match it. + self.assertEqual( + {"app_internal": True}, + await self._get("/__/test/some-item"), + ) + + async def test_plain_route_under_app_internal_prefix(self) -> None: + # `/__/test/plain` also matches the `/__/test/{item}` pattern, + # so an app-internal grant keyed on a path pattern rather than + # on the dispatched endpoint would leak to this route. + self.assertEqual( + {"app_internal": False}, + await self._get("/__/test/plain"), + ) + + +if __name__ == "__main__": + unittest.main()