This is an automated email from the ASF dual-hosted git repository. vincbeck pushed a commit to branch backport-a26f9f9-v3-4-test in repository https://gitbox.apache.org/repos/asf/airflow.git
commit 5e8d4c6de0236e3e30d373c3ad9bb3619ec69852 Author: Pushkal Gupta <[email protected]> AuthorDate: Mon Oct 19 19:30:59 2026 +0530 [v3-4-test] Fix missing and incorrect endpoints in the API permission reference (#74871) (cherry picked from commit a26f9f9926c29d9126fc5ca5f33b4e724c8bc509) Co-authored-by: Pushkal Gupta <[email protected]> --- airflow-core/docs/security/api_permissions_ref.rst | 54 +++++--- scripts/ci/prek/extract_permissions.py | 35 ++++- scripts/tests/ci/prek/test_extract_permissions.py | 148 +++++++++++++++++++++ 3 files changed, 216 insertions(+), 21 deletions(-) diff --git a/airflow-core/docs/security/api_permissions_ref.rst b/airflow-core/docs/security/api_permissions_ref.rst index cf5e188ff15..9d47f6bba3b 100644 --- a/airflow-core/docs/security/api_permissions_ref.rst +++ b/airflow-core/docs/security/api_permissions_ref.rst @@ -99,15 +99,15 @@ source code so it stays up to date as endpoints are added or changed. - ``Asset`` - ``GET`` * - ``DELETE`` - - ``/api/v2/assets/{asset_id}/state-store/{key:path}`` + - ``/api/v2/assets/{asset_id}/state-store/{key}`` - ``Asset`` - ``DELETE`` * - ``GET`` - - ``/api/v2/assets/{asset_id}/state-store/{key:path}`` + - ``/api/v2/assets/{asset_id}/state-store/{key}`` - ``Asset`` - ``GET`` * - ``PUT`` - - ``/api/v2/assets/{asset_id}/state-store/{key:path}`` + - ``/api/v2/assets/{asset_id}/state-store/{key}`` - ``Asset`` - ``PUT`` * - ``GET`` @@ -126,8 +126,28 @@ source code so it stays up to date as endpoints are added or changed. - ``/api/v2/backfills`` - ``DAG.RUN`` - ``POST`` + * - ``POST`` + - ``/api/v2/backfills/dry_run`` + - ``DAG.RUN`` + - ``POST`` + * - ``GET`` + - ``/api/v2/backfills/{backfill_id}`` + - ``DAG.RUN`` + - ``GET`` * - ``PUT`` - - ``/api/v2/backfills`` + - ``/api/v2/backfills/{backfill_id}/cancel`` + - ``DAG.RUN`` + - ``PUT`` + * - ``GET`` + - ``/api/v2/backfills/{backfill_id}/dag_runs`` + - ``DAG.RUN`` + - ``GET`` + * - ``PUT`` + - ``/api/v2/backfills/{backfill_id}/pause`` + - ``DAG.RUN`` + - ``PUT`` + * - ``PUT`` + - ``/api/v2/backfills/{backfill_id}/unpause`` - ``DAG.RUN`` - ``PUT`` * - ``GET`` @@ -375,19 +395,19 @@ source code so it stays up to date as endpoints are added or changed. - ``DAG.TASK_INSTANCE`` - ``GET`` * - ``DELETE`` - - ``/api/v2/dags/{dag_id}/dagRuns/{dag_run_id}/taskInstances/{task_id}/state-store/{key:path}`` + - ``/api/v2/dags/{dag_id}/dagRuns/{dag_run_id}/taskInstances/{task_id}/state-store/{key}`` - ``DAG.TASK_INSTANCE`` - ``DELETE`` * - ``GET`` - - ``/api/v2/dags/{dag_id}/dagRuns/{dag_run_id}/taskInstances/{task_id}/state-store/{key:path}`` + - ``/api/v2/dags/{dag_id}/dagRuns/{dag_run_id}/taskInstances/{task_id}/state-store/{key}`` - ``DAG.TASK_INSTANCE`` - ``GET`` * - ``PATCH`` - - ``/api/v2/dags/{dag_id}/dagRuns/{dag_run_id}/taskInstances/{task_id}/state-store/{key:path}`` + - ``/api/v2/dags/{dag_id}/dagRuns/{dag_run_id}/taskInstances/{task_id}/state-store/{key}`` - ``DAG.TASK_INSTANCE`` - ``PUT`` * - ``PUT`` - - ``/api/v2/dags/{dag_id}/dagRuns/{dag_run_id}/taskInstances/{task_id}/state-store/{key:path}`` + - ``/api/v2/dags/{dag_id}/dagRuns/{dag_run_id}/taskInstances/{task_id}/state-store/{key}`` - ``DAG.TASK_INSTANCE`` - ``PUT`` * - ``GET`` @@ -407,15 +427,15 @@ source code so it stays up to date as endpoints are added or changed. - ``DAG.XCOM`` - ``POST`` * - ``DELETE`` - - ``/api/v2/dags/{dag_id}/dagRuns/{dag_run_id}/taskInstances/{task_id}/xcomEntries/{xcom_key:path}`` + - ``/api/v2/dags/{dag_id}/dagRuns/{dag_run_id}/taskInstances/{task_id}/xcomEntries/{xcom_key}`` - ``DAG.XCOM`` - ``DELETE`` * - ``GET`` - - ``/api/v2/dags/{dag_id}/dagRuns/{dag_run_id}/taskInstances/{task_id}/xcomEntries/{xcom_key:path}`` + - ``/api/v2/dags/{dag_id}/dagRuns/{dag_run_id}/taskInstances/{task_id}/xcomEntries/{xcom_key}`` - ``DAG.XCOM`` - ``GET`` * - ``PATCH`` - - ``/api/v2/dags/{dag_id}/dagRuns/{dag_run_id}/taskInstances/{task_id}/xcomEntries/{xcom_key:path}`` + - ``/api/v2/dags/{dag_id}/dagRuns/{dag_run_id}/taskInstances/{task_id}/xcomEntries/{xcom_key}`` - ``DAG.XCOM`` - ``PUT`` * - ``GET`` @@ -547,15 +567,15 @@ source code so it stays up to date as endpoints are added or changed. - ``Pool`` - ``POST`` * - ``DELETE`` - - ``/api/v2/pools/{pool_name:path}`` + - ``/api/v2/pools/{pool_name}`` - ``Pool`` - ``DELETE`` * - ``GET`` - - ``/api/v2/pools/{pool_name:path}`` + - ``/api/v2/pools/{pool_name}`` - ``Pool`` - ``GET`` * - ``PATCH`` - - ``/api/v2/pools/{pool_name:path}`` + - ``/api/v2/pools/{pool_name}`` - ``Pool`` - ``PUT`` * - ``GET`` @@ -575,15 +595,15 @@ source code so it stays up to date as endpoints are added or changed. - ``Variable`` - ``POST`` * - ``DELETE`` - - ``/api/v2/variables/{variable_key:path}`` + - ``/api/v2/variables/{variable_key}`` - ``Variable`` - ``DELETE`` * - ``GET`` - - ``/api/v2/variables/{variable_key:path}`` + - ``/api/v2/variables/{variable_key}`` - ``Variable`` - ``GET`` * - ``PATCH`` - - ``/api/v2/variables/{variable_key:path}`` + - ``/api/v2/variables/{variable_key}`` - ``Variable`` - ``PUT`` * - ``GET`` diff --git a/scripts/ci/prek/extract_permissions.py b/scripts/ci/prek/extract_permissions.py index b3f86e55a07..f2a14ab5879 100644 --- a/scripts/ci/prek/extract_permissions.py +++ b/scripts/ci/prek/extract_permissions.py @@ -33,6 +33,7 @@ from __future__ import annotations import ast import pathlib +import re import sys from dataclasses import dataclass @@ -95,6 +96,34 @@ def _resolve_string_node(node: ast.expr, module_consts: dict[str, str]) -> str: return f"<unresolved:{ast.unparse(node)}>" +def strip_path_converters(path: str) -> str: + """ + Drop Starlette path converters from a route path. + + A route declared as ``/{pool_name:path}`` is served, and documented in the + OpenAPI spec, as ``/{pool_name}``. The converter only tells Starlette how to + match the segment, so leaving it in prints a URL that no client ever calls. + """ + return re.sub(r"\{([^{}:]+):[^{}]+\}", r"{\1}", path) + + +def _extract_route_path(decorator: ast.Call, module_consts: dict[str, str]) -> str: + """ + Resolve the route path from a route decorator. + + FastAPI accepts the path either positionally or as ``path=``, and both forms + are in use here. Reading only the positional form silently resolves the whole + file to its router prefix, which collapses every route onto one bogus row + instead of failing. + """ + if decorator.args: + return _resolve_string_node(decorator.args[0], module_consts) + path_kwarg = next((kw for kw in decorator.keywords if kw.arg == "path"), None) + if path_kwarg is not None: + return _resolve_string_node(path_kwarg.value, module_consts) + raise RouteParseError(f"Route decorator has no path: {ast.unparse(decorator)}") + + def _extract_module_string_constants(tree: ast.Module) -> dict[str, str]: """ Walk top-level assignments and collect simple string assignments. @@ -313,9 +342,7 @@ def extract_from_file(path: pathlib.Path) -> list[PermissionEntry]: continue # Resolve the route path - route_suffix = "" - if decorator.args: - route_suffix = _resolve_string_node(decorator.args[0], module_consts) + route_suffix = _extract_route_path(decorator, module_consts) # Resolve the prefix based on the router variable used in the decorator router_prefix = "" @@ -323,7 +350,7 @@ def extract_from_file(path: pathlib.Path) -> list[PermissionEntry]: router_var = decorator.func.value.id router_prefix = routers.get(router_var, "") - full_path = API_PREFIX + router_prefix + route_suffix + full_path = strip_path_converters(API_PREFIX + router_prefix + route_suffix) # Extract tag (for grouping in the RST table) tag = _extract_tag_from_decorator(decorator) diff --git a/scripts/tests/ci/prek/test_extract_permissions.py b/scripts/tests/ci/prek/test_extract_permissions.py index 269f2989877..0ffef7b1bb6 100644 --- a/scripts/tests/ci/prek/test_extract_permissions.py +++ b/scripts/tests/ci/prek/test_extract_permissions.py @@ -32,6 +32,7 @@ Run with (no Airflow env needed — extractor is stdlib-only): from __future__ import annotations import ast +import re import sys import textwrap from pathlib import Path @@ -54,11 +55,13 @@ from extract_permissions import ( # noqa: E402 _build_resource_label, _extract_method_arg, _extract_module_string_constants, + _extract_route_path, _extract_routers, _resolve_string_node, extract_all_permissions, extract_from_file, render_rst, + strip_path_converters, ) PUBLIC_ROUTES_DIR = REPO_ROOT / "airflow-core/src/airflow/api_fastapi/core_api/routes/public" @@ -293,6 +296,59 @@ class TestBuildResourceLabel: assert _build_resource_label("requires_access_new_thing", None) == "requires_access_new_thing" +# =========================================================================== +# Unit tests: strip_path_converters +# =========================================================================== + + +class TestStripPathConverters: + @pytest.mark.parametrize( + "declared, served", + [ + ("/api/v2/pools/{pool_name:path}", "/api/v2/pools/{pool_name}"), + ("/api/v2/variables/{variable_key:path}", "/api/v2/variables/{variable_key}"), + # more than one parameter, only the converted one changes + ( + "/api/v2/assets/{asset_id}/state-store/{key:path}", + "/api/v2/assets/{asset_id}/state-store/{key}", + ), + # converters other than :path are normalised the same way + ("/api/v2/dags/{dag_id:str}/runs/{n:int}", "/api/v2/dags/{dag_id}/runs/{n}"), + # nothing to do + ("/api/v2/dags/{dag_id}", "/api/v2/dags/{dag_id}"), + ("/api/v2/backfills", "/api/v2/backfills"), + ], + ) + def test_converter_is_dropped(self, declared, served): + assert strip_path_converters(declared) == served + + +# =========================================================================== +# Unit tests: _extract_route_path +# =========================================================================== + + +class TestExtractRoutePath: + def test_positional_path(self): + deco = parse_expr('router.get("/{dag_id}", dependencies=[])') + assert _extract_route_path(deco, {}) == "/{dag_id}" + + def test_keyword_path(self): + """``backfills.py`` passes the path as ``path=``, which must resolve the same.""" + deco = parse_expr('router.get(path="/{backfill_id}/pause", dependencies=[])') + assert _extract_route_path(deco, {}) == "/{backfill_id}/pause" + + def test_keyword_path_resolves_module_constants(self): + deco = parse_expr('router.get(path=my_prefix + "/{id}")') + assert _extract_route_path(deco, {"my_prefix": "/things"}) == "/things/{id}" + + def test_no_path_at_all_raises(self): + """Falling back to an empty suffix would collapse the route onto its router prefix.""" + deco = parse_expr("router.get(dependencies=[])") + with pytest.raises(RouteParseError): + _extract_route_path(deco, {}) + + # =========================================================================== # Unit tests: extract_from_file (synthetic route files) # =========================================================================== @@ -538,6 +594,57 @@ class TestExtractFromFile: assert len(entries) == 1 assert entries[0].resource == "DAG" + def test_keyword_path_is_not_collapsed_onto_the_router_prefix(self, tmp_path): + """ + Mirrors ``backfills.py``, which declares every route as ``path=``. + + Reading only the positional argument left each of these resolving to the + bare router prefix, so eight endpoints became three wrong rows. + """ + f = _make_route_file( + tmp_path, + """ + from fastapi import Depends + backfills_router = AirflowRouter(tags=["Backfill"], prefix="/backfills") + + @backfills_router.get( + path="/{backfill_id}", + dependencies=[Depends(requires_access_backfill(method="GET"))], + ) + def get_backfill(backfill_id: int): ... + + @backfills_router.put( + path="/{backfill_id}/pause", + dependencies=[Depends(requires_access_backfill(method="PUT"))], + ) + def pause_backfill(backfill_id: int): ... + """, + ) + entries = extract_from_file(f) + assert {(e.http_method, e.full_path) for e in entries} == { + ("GET", "/api/v2/backfills/{backfill_id}"), + ("PUT", "/api/v2/backfills/{backfill_id}/pause"), + } + + def test_path_converter_is_stripped_from_the_documented_path(self, tmp_path): + """The served URL is ``/pools/{pool_name}``; the converter is matching syntax.""" + f = _make_route_file( + tmp_path, + """ + from fastapi import Depends + pools_router = AirflowRouter(tags=["Pool"], prefix="/pools") + + @pools_router.get( + "/{pool_name:path}", + dependencies=[Depends(requires_access_pool(method="GET"))], + ) + def get_pool(pool_name: str): ... + """, + ) + entries = extract_from_file(f) + assert len(entries) == 1 + assert entries[0].full_path == "/api/v2/pools/{pool_name}" + def test_syntax_error_raises_instead_of_dropping_entries(self, tmp_path): f = tmp_path / "broken.py" f.write_text("def broken(:\n") @@ -647,6 +754,47 @@ class TestExtractAllPermissions: """ assert len(all_entries) >= 100, f"Expected ≥100 entries, got {len(all_entries)}" + def test_every_declared_route_reaches_the_table(self, all_entries): + """ + Each route decorator in the source must produce its own row. + + This is the invariant that the ``path=`` bug broke: the eight routes in + ``backfills.py`` all resolved to ``/api/v2/backfills`` and deduplicated + down to three, without any warning. + """ + verbs = {"GET", "POST", "PATCH", "PUT", "DELETE", "HEAD"} + declared: dict[str, int] = {} + for route_file in sorted(PUBLIC_ROUTES_DIR.glob("*.py")): + if route_file.name == "__init__.py": + continue + count = 0 + for node in ast.walk(ast.parse(route_file.read_text(encoding="utf-8"))): + if not isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)): + continue + count += sum( + 1 + for deco in node.decorator_list + if isinstance(deco, ast.Call) + and isinstance(deco.func, ast.Attribute) + and deco.func.attr.upper() in verbs + ) + declared[route_file.name] = count + + extracted: dict[str, set[tuple[str, str]]] = {} + for entry in all_entries: + extracted.setdefault(entry.source_file, set()).add((entry.full_path, entry.http_method)) + + for name, count in declared.items(): + assert len(extracted.get(name, set())) == count, ( + f"{name} declares {count} routes but the table holds " + f"{len(extracted.get(name, set()))} distinct operations" + ) + + def test_no_documented_path_contains_a_converter(self, all_entries): + """``{key:path}`` is Starlette matching syntax, not part of the URL clients call.""" + offenders = [e.full_path for e in all_entries if re.search(r"\{[^{}]*:", e.full_path)] + assert offenders == [] + def test_output_is_sorted(self, all_entries): expected = sorted( all_entries,
