diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index b32abb06dad87..e99e1224edd95 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -975,6 +975,14 @@ repos: entry: "^\\s*from urllib\\.parse import ((\\|, )(urlparse\\|urlunparse))+$" pass_filenames: true files: \.py$ + - id: check-auth-manager-mock-targets + name: Patch SimpleAuthManager, not BaseAuthManager, for methods it overrides + description: The API test client uses SimpleAuthManager, so patching an overridden BaseAuthManager method has no effect + entry: ./scripts/ci/prek/check_auth_manager_mock_targets.py + language: python + pass_filenames: true + files: ^airflow-core/tests/unit/api_fastapi/.*\.py$ + exclude: ^airflow-core/tests/unit/api_fastapi/auth/managers/simple/ - id: check-for-inclusive-language language: pygrep name: Check for language that we do not accept as community diff --git a/airflow-core/tests/unit/api_fastapi/core_api/routes/ui/test_dependencies.py b/airflow-core/tests/unit/api_fastapi/core_api/routes/ui/test_dependencies.py index 1c678857abb99..ba1179a684064 100644 --- a/airflow-core/tests/unit/api_fastapi/core_api/routes/ui/test_dependencies.py +++ b/airflow-core/tests/unit/api_fastapi/core_api/routes/ui/test_dependencies.py @@ -503,7 +503,7 @@ def test_data_dependencies_hides_asset_when_user_has_no_dag_access(self, _, test assert response.status_code == 404 @mock.patch( - "airflow.api_fastapi.auth.managers.base_auth_manager.BaseAuthManager.get_authorized_assets", + "airflow.api_fastapi.auth.managers.simple.simple_auth_manager.SimpleAuthManager.get_authorized_assets", autospec=True, return_value=set(), ) @@ -523,7 +523,7 @@ def test_scheduling_dependencies_hides_assets_the_caller_may_not_read(self, _, t assert f"asset:{asset1_id}" not in edge_endpoints @mock.patch( - "airflow.api_fastapi.auth.managers.base_auth_manager.BaseAuthManager.get_authorized_assets", + "airflow.api_fastapi.auth.managers.simple.simple_auth_manager.SimpleAuthManager.get_authorized_assets", autospec=True, ) def test_scheduling_dependencies_hides_unreadable_gate_leaf( @@ -554,7 +554,7 @@ def test_scheduling_dependencies_hides_unreadable_gate_leaf( assert (f"asset:{asset_b_id}", gate_node["id"]) not in edge_tuples @mock.patch( - "airflow.api_fastapi.auth.managers.base_auth_manager.BaseAuthManager.get_authorized_assets", + "airflow.api_fastapi.auth.managers.simple.simple_auth_manager.SimpleAuthManager.get_authorized_assets", autospec=True, return_value=set(), ) @@ -595,7 +595,7 @@ def test_scheduling_dependencies_hides_gate_leaf_without_id(self, dag_maker, tes assert {"gate_asset_a", "gate_asset_b"}.isdisjoint(node["label"] for node in result["nodes"]) @mock.patch( - "airflow.api_fastapi.auth.managers.base_auth_manager.BaseAuthManager.get_authorized_assets", + "airflow.api_fastapi.auth.managers.simple.simple_auth_manager.SimpleAuthManager.get_authorized_assets", autospec=True, return_value=set(), ) @@ -609,7 +609,7 @@ def test_data_dependencies_hides_asset_the_caller_may_not_read(self, _, test_cli assert response.status_code == 404 @mock.patch( - "airflow.api_fastapi.auth.managers.base_auth_manager.BaseAuthManager.get_authorized_assets", + "airflow.api_fastapi.auth.managers.simple.simple_auth_manager.SimpleAuthManager.get_authorized_assets", autospec=True, ) def test_data_dependencies_hides_unreadable_sibling_asset( @@ -642,7 +642,7 @@ def test_data_dependencies_hides_unreadable_sibling_asset( assert not any(node_id.startswith("task:gate_upstream_b") for node_id in node_ids) @mock.patch( - "airflow.api_fastapi.auth.managers.base_auth_manager.BaseAuthManager.get_authorized_assets", + "airflow.api_fastapi.auth.managers.simple.simple_auth_manager.SimpleAuthManager.get_authorized_assets", autospec=True, ) def test_data_dependencies_hides_gate_leaf_without_id( diff --git a/scripts/ci/prek/check_auth_manager_mock_targets.py b/scripts/ci/prek/check_auth_manager_mock_targets.py new file mode 100755 index 0000000000000..a0850ba681446 --- /dev/null +++ b/scripts/ci/prek/check_auth_manager_mock_targets.py @@ -0,0 +1,88 @@ +#!/usr/bin/env python +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +# /// script +# requires-python = ">=3.11,<3.12" +# /// +"""Check that API tests do not patch ``BaseAuthManager`` methods that ``SimpleAuthManager`` overrides. + +The API test client runs with ``SimpleAuthManager``. Patching a ``BaseAuthManager`` method that +``SimpleAuthManager`` overrides leaves the override in place, so the mock never takes effect and a +permission test passes or fails for the wrong reason. Patch ``SimpleAuthManager`` instead. + +The set of overridden methods is read from the two classes, so it follows the code. +""" + +from __future__ import annotations + +import ast +import re +import sys +from pathlib import Path + +AIRFLOW_ROOT = Path(__file__).parents[3] +BASE_AUTH_MANAGER = AIRFLOW_ROOT / "airflow-core/src/airflow/api_fastapi/auth/managers/base_auth_manager.py" +SIMPLE_AUTH_MANAGER = ( + AIRFLOW_ROOT / "airflow-core/src/airflow/api_fastapi/auth/managers/simple/simple_auth_manager.py" +) + +PATCH_TARGET = re.compile(r"BaseAuthManager\.(\w+)") + + +def get_methods(path: Path, class_name: str) -> set[str]: + tree = ast.parse(path.read_text(), filename=str(path)) + for node in tree.body: + if isinstance(node, ast.ClassDef) and node.name == class_name: + return { + item.name + for item in node.body + if isinstance(item, (ast.FunctionDef, ast.AsyncFunctionDef)) and not item.name.startswith("_") + } + raise ValueError(f"class {class_name} not found in {path}") + + +def get_overridden_methods() -> set[str]: + return get_methods(BASE_AUTH_MANAGER, "BaseAuthManager") & get_methods( + SIMPLE_AUTH_MANAGER, "SimpleAuthManager" + ) + + +def find_bad_patch_targets(text: str, overridden: set[str]) -> list[tuple[int, str]]: + return [ + (line_number, match.group(1)) + for line_number, line in enumerate(text.splitlines(), start=1) + for match in PATCH_TARGET.finditer(line) + if match.group(1) in overridden + ] + + +def main(paths: list[str]) -> int: + overridden = get_overridden_methods() + errors = [] + for path in paths: + for line_number, method in find_bad_patch_targets(Path(path).read_text(), overridden): + errors.append( + f"{path}:{line_number}: BaseAuthManager.{method} is overridden by SimpleAuthManager, " + f"so patching it has no effect in API tests; patch SimpleAuthManager.{method} instead." + ) + for error in errors: + print(error) + return 1 if errors else 0 + + +if __name__ == "__main__": + sys.exit(main(sys.argv[1:])) diff --git a/scripts/tests/ci/prek/test_check_auth_manager_mock_targets.py b/scripts/tests/ci/prek/test_check_auth_manager_mock_targets.py new file mode 100644 index 0000000000000..b73eef606ddaa --- /dev/null +++ b/scripts/tests/ci/prek/test_check_auth_manager_mock_targets.py @@ -0,0 +1,88 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +from __future__ import annotations + +import pytest +from check_auth_manager_mock_targets import ( + find_bad_patch_targets, + get_methods, + get_overridden_methods, + main, +) + +BASE = "airflow.api_fastapi.auth.managers.base_auth_manager.BaseAuthManager" +SIMPLE = "airflow.api_fastapi.auth.managers.simple.simple_auth_manager.SimpleAuthManager" + + +def test_get_methods_skips_private_methods(tmp_path): + module = tmp_path / "module.py" + module.write_text( + "class Manager:\n def public(self): ...\n async def run(self): ...\n def _private(self): ...\n" + ) + + assert get_methods(module, "Manager") == {"public", "run"} + + +def test_get_methods_raises_for_missing_class(tmp_path): + module = tmp_path / "module.py" + module.write_text("class Other: ...\n") + + with pytest.raises(ValueError, match="class Manager not found"): + get_methods(module, "Manager") + + +def test_overridden_methods_follow_the_real_classes(): + overridden = get_overridden_methods() + + assert "get_authorized_assets" in overridden + assert "get_authorized_dag_ids" not in overridden + + +@pytest.mark.parametrize( + ("text", "expected"), + [ + pytest.param( + f'@mock.patch("{BASE}.get_authorized_assets")', [(1, "get_authorized_assets")], id="base" + ), + pytest.param(f'@mock.patch("{SIMPLE}.get_authorized_assets")', [], id="simple"), + pytest.param(f'@mock.patch("{BASE}.get_authorized_dag_ids")', [], id="not-overridden"), + pytest.param( + f'x = 1\n@mock.patch(\n "{BASE}.is_authorized_dag",\n)', + [(3, "is_authorized_dag")], + id="multiline", + ), + ], +) +def test_find_bad_patch_targets(text, expected): + overridden = {"get_authorized_assets", "is_authorized_dag"} + + assert find_bad_patch_targets(text, overridden) == expected + + +def test_main_reports_bad_target_and_fails(tmp_path, capsys): + test_file = tmp_path / "test_routes.py" + test_file.write_text(f'@mock.patch("{BASE}.get_authorized_assets")\n') + + assert main([str(test_file)]) == 1 + assert "patch SimpleAuthManager.get_authorized_assets instead" in capsys.readouterr().out + + +def test_main_passes_good_target(tmp_path): + test_file = tmp_path / "test_routes.py" + test_file.write_text(f'@mock.patch("{SIMPLE}.get_authorized_assets")\n') + + assert main([str(test_file)]) == 0