diff --git a/airflow-core/src/airflow/security/permissions.py b/airflow-core/src/airflow/security/permissions.py index 867ea7b6bdf7c..b5a39182d2fdc 100644 --- a/airflow-core/src/airflow/security/permissions.py +++ b/airflow-core/src/airflow/security/permissions.py @@ -106,8 +106,10 @@ class ResourceDetails(TypedDict): def resource_name(dag_id: str, resource: str) -> str: """Return the resource name for a DAG id.""" - if dag_id in RESOURCE_DETAILS_MAP.keys(): - return dag_id + # NB: do not short-circuit when ``dag_id`` equals a reserved resource name + # (e.g. "DAGs"). A real ``dag_id`` can collide with one, and returning it + # unchanged would map the per-DAG resource onto the global resource. Only an + # already-prefixed value (e.g. "DAG:foo") is returned as-is. if dag_id.startswith(tuple(PREFIX_RESOURCES_MAP.keys())): return dag_id return f"{RESOURCE_DETAILS_MAP[resource]['prefix']}{dag_id}" diff --git a/airflow-core/tests/unit/security/test_permissions.py b/airflow-core/tests/unit/security/test_permissions.py new file mode 100644 index 0000000000000..c267d8b55e20e --- /dev/null +++ b/airflow-core/tests/unit/security/test_permissions.py @@ -0,0 +1,35 @@ +# +# 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 + +from airflow.security import permissions + + +def test_resource_name_does_not_collide_with_reserved_resource_names(): + """A dag_id equal to a reserved resource name must be prefixed, not returned as-is. + + Regression for the case where a DAG literally named ``DAGs`` resolved to the + global all-DAGs resource instead of its own ``DAG:DAGs`` resource. + """ + # "DAGs" is the global resource name *and* a valid dag_id — it must resolve to + # the per-DAG resource, never be returned unchanged. + assert permissions.resource_name(permissions.RESOURCE_DAG, permissions.RESOURCE_DAG) == "DAG:DAGs" + # Ordinary dag_ids are prefixed as before. + assert permissions.resource_name("my_dag", permissions.RESOURCE_DAG) == "DAG:my_dag" + # Already-prefixed names stay idempotent (the surviving short-circuit branch). + assert permissions.resource_name("DAG:my_dag", permissions.RESOURCE_DAG) == "DAG:my_dag" diff --git a/providers/fab/src/airflow/providers/fab/www/security/permissions.py b/providers/fab/src/airflow/providers/fab/www/security/permissions.py index 37ce89ba18c86..5d72503deea0a 100644 --- a/providers/fab/src/airflow/providers/fab/www/security/permissions.py +++ b/providers/fab/src/airflow/providers/fab/www/security/permissions.py @@ -96,8 +96,10 @@ class ResourceDetails(TypedDict): def resource_name(dag_id: str, resource: str) -> str: """Return the resource name for a DAG id.""" - if dag_id in RESOURCE_DETAILS_MAP.keys(): - return dag_id + # NB: do not short-circuit when ``dag_id`` equals a reserved resource name + # (e.g. "DAGs"). A real ``dag_id`` can collide with one, and returning it + # unchanged would map the per-DAG resource onto the global resource. Only an + # already-prefixed value (e.g. "DAG:foo") is returned as-is. if dag_id.startswith(tuple(PREFIX_RESOURCES_MAP.keys())): return dag_id return f"{RESOURCE_DETAILS_MAP[resource]['prefix']}{dag_id}" diff --git a/providers/fab/tests/unit/fab/auth_manager/test_security.py b/providers/fab/tests/unit/fab/auth_manager/test_security.py index e80e5f3b07729..24094a978995e 100644 --- a/providers/fab/tests/unit/fab/auth_manager/test_security.py +++ b/providers/fab/tests/unit/fab/auth_manager/test_security.py @@ -1273,3 +1273,22 @@ def test_add_user_uses_configured_hash_method(mock_hash, app, security_manager): mock_hash.assert_called_with("plaintext", method="pbkdf2:sha256") finally: delete_user(app, "hash_method_add_test") + + +def test_resource_name_does_not_collide_with_reserved_resource_names(): + """A dag_id equal to a reserved resource name must be prefixed, not returned as-is. + + Regression for the case where a DAG literally named ``DAGs`` resolved to the + global all-DAGs resource instead of its own ``DAG:DAGs`` resource. + """ + # "DAGs" is the global resource name *and* a valid dag_id — it must resolve to + # the per-DAG resource, never be returned unchanged. + assert permissions.resource_name(permissions.RESOURCE_DAG, permissions.RESOURCE_DAG) == "DAG:DAGs" + # Ordinary dag_ids are prefixed as before. + assert permissions.resource_name("my_dag", permissions.RESOURCE_DAG) == "DAG:my_dag" + # Already-prefixed names stay idempotent (the surviving short-circuit branch). + assert permissions.resource_name("DAG:my_dag", permissions.RESOURCE_DAG) == "DAG:my_dag" + # Sanity: an access_control sync resolves a DAG named "DAGs" to its per-DAG + # resource, so the permission is granted on "DAG:DAGs", never the global "DAGs". + # The grant in _sync_dag_view_permissions goes to exactly resource_name(dag_id, ...), + # so the assertion above is what guards the privilege-escalation regression.