Skip to content

Commit 3342565

Browse files
committed
fix(authz): adapt plugin route derivation to fastapi 0.138 lazy include_router
fastapi 0.138 / starlette 1.3.1 made include_router(prefix=...) lazy: rebased routes are stashed behind a _IncludedRouter proxy instead of being materialized as APIRoute objects, so the derivation's walk of composed.routes found nothing and every plugin route was treated as unruled. Add _iter_composed_routes() to descend each proxy's effective_route_contexts(), yielding composed-path APIRoute copies (original .endpoint preserved so get_path_rules still resolves the @path_rule metadata) and the composed starlette_route for WebSocket/Mount leaves (so the fail-closed branches still fire and no route is silently dropped). Downstream derivation logic is unchanged; the helper passes concrete routes through, so it also works if eager-include behavior returns. Signed-off-by: Max Dubrinsky <mdubrinsky@nvidia.com>
1 parent e0a2506 commit 3342565

3 files changed

Lines changed: 91 additions & 18 deletions

File tree

packages/nemo_platform_plugin/src/nemo_platform_plugin/authz_discovery.py

Lines changed: 56 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,9 @@
2525

2626
from __future__ import annotations
2727

28+
import copy
2829
import logging
30+
from collections.abc import Iterator
2931
from dataclasses import dataclass, field
3032
from typing import Any
3133

@@ -41,6 +43,7 @@
4143
)
4244
from nemo_platform_plugin.authz_format import is_valid_permission_id
4345
from nemo_platform_plugin.service import NemoService
46+
from starlette.routing import BaseRoute
4447

4548
logger = logging.getLogger(__name__)
4649

@@ -162,6 +165,54 @@ def _register_permission(catalog: dict[str, Permission], perm: Permission, warni
162165
catalog.setdefault(perm.id, perm)
163166

164167

168+
def _iter_composed_routes(service: NemoService) -> Iterator[BaseRoute]:
169+
"""Yield the fully-composed leaf routes of *service*, one ``BaseRoute`` per mounted route.
170+
171+
This re-creates the runtime mount (``/apis/<name>`` + ``RouterSpec.prefix`` + route path)
172+
and flattens it to leaves so the derivation can read each route's final ``.path``,
173+
``.methods``, and (for ``APIRoute``) original ``.endpoint``.
174+
175+
Lazy-include workaround (fastapi 0.138.0 / starlette 1.3.1): ``include_router(prefix=...)``
176+
no longer materializes rebased ``APIRoute`` objects into ``.routes`` — it stores a
177+
``fastapi.routing._IncludedRouter`` proxy, so walking ``.routes`` for ``APIRoute`` finds
178+
nothing. We descend each proxy via ``effective_route_contexts()`` (which also recurses
179+
through nested includes) and reconstruct the composed leaves:
180+
181+
- For an ``APIRoute`` we shallow-copy the original and overwrite ``.path``/``.methods`` with
182+
the context's composed values. The copy keeps ``isinstance(route, APIRoute)`` true and
183+
preserves the original ``.endpoint`` object so ``get_path_rules(route.endpoint)`` still
184+
finds the function-attached rules; copying (rather than mutating) avoids corrupting the
185+
shared original route.
186+
- For non-``APIRoute`` leaves (WebSocketRoute / Mount / plain Route) we yield the context's
187+
``starlette_route``, which already carries the composed path — so the existing
188+
fail-closed / warning branches still fire and no route is silently dropped.
189+
190+
A concrete route appearing directly in ``.routes`` (e.g. a future eager-include path) is
191+
yielded as-is, so this keeps working if the proxy behavior changes again.
192+
"""
193+
composed = APIRouter()
194+
for spec in service.get_routers():
195+
composed.include_router(spec.router, prefix=f"/apis/{service.name}{spec.prefix}")
196+
197+
for route in composed.routes:
198+
contexts = getattr(route, "effective_route_contexts", None)
199+
if contexts is None:
200+
# Already a concrete leaf (no lazy-include proxy) — pass it through unchanged.
201+
yield route
202+
continue
203+
for ctx in contexts():
204+
original = ctx.original_route
205+
if isinstance(original, APIRoute):
206+
# Rebased APIRoute: copy + composed path/methods, original endpoint preserved.
207+
rebased = copy.copy(original)
208+
rebased.path = ctx.path
209+
rebased.methods = ctx.methods
210+
yield rebased
211+
else:
212+
# WS / Mount / plain Route: the composed-path route is on the context.
213+
yield ctx.starlette_route or original
214+
215+
165216
def _derive_service_contribution(service: NemoService) -> tuple[AuthzContribution, list[str], list[str]]:
166217
"""Derive one plugin's wire contribution, split into deny-worthy errors and warnings.
167218
@@ -180,16 +231,12 @@ def _derive_service_contribution(service: NemoService) -> tuple[AuthzContributio
180231
warnings: list[str] = []
181232
catalog: dict[str, Permission] = {}
182233

183-
# Re-create the runtime mount: /apis/<name> + RouterSpec.prefix + route path.
184-
composed = APIRouter()
185-
for spec in service.get_routers():
186-
composed.include_router(spec.router, prefix=f"/apis/{service.name}{spec.prefix}")
187-
188-
# Pass 1: walk routes, collapse OR'd rules, and collect referenced permissions.
189-
# ``bindings`` holds the tentative allow binding per (path, method); unruled / invalid
190-
# routes are recorded as None and become DENY regardless of namespace validity.
234+
# Pass 1: walk the fully-composed leaf routes (/apis/<name> + RouterSpec.prefix + route
235+
# path), collapse OR'd rules, and collect referenced permissions. ``bindings`` holds the
236+
# tentative allow binding per (path, method); unruled / invalid routes are recorded as None
237+
# and become DENY regardless of namespace validity.
191238
bindings: dict[str, dict[str, AuthzEndpointMethod | None]] = {}
192-
for route in composed.routes:
239+
for route in _iter_composed_routes(service):
193240
if not isinstance(route, APIRoute):
194241
# Mount / plain Starlette Route / WebSocket route — not an HTTP API route the PDP
195242
# binds by (path, method). Never silently skip it (that lets it fall through the

packages/nemo_platform_plugin/tests/test_factory_authz.py

Lines changed: 16 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -39,13 +39,25 @@ async def _compiler(*args: object, **kwargs: object) -> object: # never called
3939

4040

4141
def _rules_by_path_method(router: APIRouter) -> dict[tuple[str, str], list]:
42-
"""Map (path, lower-method) -> attached PathRules for every APIRoute in *router*."""
42+
"""Map (composed-path, lower-method) -> attached PathRules for every APIRoute in *router*.
43+
44+
fastapi 0.138.0 makes ``include_router(prefix=...)`` lazy: rebased ``APIRoute``\\ s live
45+
behind a ``_IncludedRouter`` proxy rather than in ``.routes``. Descend the proxy via
46+
``effective_route_contexts()`` to read each route's composed path/methods and original
47+
endpoint (which still carries the ``@path_rule`` metadata).
48+
"""
4349
out: dict[tuple[str, str], list] = {}
4450
for route in router.routes:
45-
if not isinstance(route, APIRoute):
51+
contexts = getattr(route, "effective_route_contexts", None)
52+
if contexts is None:
53+
if isinstance(route, APIRoute):
54+
for method in route.methods or set():
55+
out[(route.path, method.lower())] = get_path_rules(route.endpoint)
4656
continue
47-
for method in route.methods or set():
48-
out[(route.path, method.lower())] = get_path_rules(route.endpoint)
57+
for ctx in contexts():
58+
if isinstance(ctx.original_route, APIRoute):
59+
for method in ctx.methods or set():
60+
out[(ctx.path, method.lower())] = get_path_rules(ctx.original_route.endpoint)
4961
return out
5062

5163

packages/nemo_platform_plugin/tests/test_path_rule.py

Lines changed: 19 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@
2222
perm,
2323
validate_caller_strings,
2424
)
25-
from nemo_platform_plugin.authz_discovery import _method_from_dict
25+
from nemo_platform_plugin.authz_discovery import _iter_composed_routes, _method_from_dict
2626
from nemo_platform_plugin.service import NemoService, RouterSpec
2727

2828
_READ = Permission("x.read", "Read x")
@@ -121,7 +121,12 @@ async def handler() -> None: ...
121121

122122

123123
def test_path_rule_survives_router_prefix_rebasing() -> None:
124-
"""D5: function-attached metadata must survive include_router(prefix=...) rebasing."""
124+
"""D5: function-attached metadata must survive include_router(prefix=...) rebasing.
125+
126+
fastapi 0.138.0 makes ``include_router(prefix=...)`` lazy (rebased routes live behind a
127+
``_IncludedRouter`` proxy, not in ``.routes``), so discoverability is asserted via the
128+
derivation's composed-route enumeration rather than by scanning raw ``.routes``.
129+
"""
125130
router = APIRouter()
126131
items_read = Permission("items.read", "Read items")
127132

@@ -131,16 +136,25 @@ async def get_item(name: str) -> dict[str, str]:
131136
return {"name": name}
132137

133138
# Two prefix hops, as a real plugin mount does (/apis/<plugin> then workspace prefix).
139+
# _iter_composed_routes re-creates the /apis/<name> mount, so the spec supplies only the
140+
# inner workspace prefix; the helper prepends /apis/example.
134141
inner = APIRouter()
135142
inner.include_router(router, prefix="/v2/workspaces/{workspace}")
136-
app_router = APIRouter()
137-
app_router.include_router(inner, prefix="/apis/example")
138143

139-
matching = [r for r in app_router.routes if isinstance(r, APIRoute) and r.path.endswith("/items/{name}")]
144+
class _Svc(NemoService):
145+
name = "example"
146+
147+
def get_routers(self) -> list[RouterSpec]:
148+
return [RouterSpec(inner)]
149+
150+
matching = [
151+
r for r in _iter_composed_routes(_Svc()) if isinstance(r, APIRoute) and r.path.endswith("/items/{name}")
152+
]
140153
assert len(matching) == 1
141154
final_route = matching[0]
142155
assert final_route.path == "/apis/example/v2/workspaces/{workspace}/items/{name}"
143156

157+
# Metadata survived the rebase: the rule is still readable off the (identity-preserved) endpoint.
144158
rules = get_path_rules(final_route.endpoint)
145159
assert len(rules) == 1
146160
assert rules[0].callers == [CallerKind.PRINCIPAL]

0 commit comments

Comments
 (0)