From b9b55768a414ba5f3cf116dfe0362e5b305b9678 Mon Sep 17 00:00:00 2001 From: Jarek Potiuk Date: Thu, 30 Jul 2026 13:56:11 +0200 Subject: [PATCH 1/3] Gate audit log rows not tied to a Dag on a dedicated AccessView `PermittedEventLogFilter` returned every `dag_id IS NULL` row unconditionally, and the detail endpoint's guard resolved such a row to `dag_id=None` and then authorized it as Dag-level `DagAccessEntity.AUDIT_LOG` access. Those rows record Connection, Variable and Pool operations, so they carry no per-Dag key to authorize on -- and Dag-level audit access is held by any viewer under the default auth manager, which is not the audience for them. Add `AccessView.AUDIT_LOGS_ALL` and gate both read paths on it, mirroring the `IMPORT_ERRORS_ALL` view that covers the same shape of problem for import errors of files with no registered Dag. The list endpoint filters the rows out in the query, so they stay out of `total_entries` and pagination too; the detail endpoint answers 403. `SimpleAuthManager` grants the view to admins only, and the FAB auth manager maps it to a new `All Audit Logs` resource that lands in `ADMIN_PERMISSIONS`. The detail guard now selects `Log.id` alongside `Log.dag_id`: a missing row and a NULL `dag_id` both read back as `None` from a bare scalar, and they authorize differently. A missing row keeps the Dag-level check so the route can still answer 404 rather than turning an unknown id into a permission error. `authorize_view` also stops propagating a `KeyError` from an auth manager that cannot map an `AccessView`. `AccessView` members are added by core, but auth managers ship as separately released providers, so a core newer than the installed manager can name a view the manager has never heard of -- the FAB manager translates the enum through a lookup table and would have raised, which surfaces as a 500 on the endpoint. It now denies and warns instead, which keeps the endpoint working and fails closed. Generated-by: Claude Opus 5 (1M context) following the guidelines at https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#gen-ai-assisted-contributions --- .../auth/managers/base_auth_manager.py | 48 ++++++++-- .../auth/managers/models/resource_details.py | 4 + .../managers/simple/simple_auth_manager.py | 7 +- .../airflow/api_fastapi/core_api/security.py | 64 +++++++++++-- .../simple/test_simple_auth_manager.py | 27 ++++++ .../auth/managers/test_base_auth_manager.py | 20 +++++ .../core_api/routes/public/test_event_logs.py | 89 ++++++++++++++++++- .../common/compat/security/access_view.py | 10 ++- .../compat/security/test_access_view.py | 28 +++--- .../fab/docs/auth-manager/access-control.rst | 6 +- .../fab/auth_manager/fab_auth_manager.py | 13 ++- .../auth_manager/security_manager/override.py | 1 + .../providers/fab/www/security/permissions.py | 1 + 13 files changed, 276 insertions(+), 42 deletions(-) diff --git a/airflow-core/src/airflow/api_fastapi/auth/managers/base_auth_manager.py b/airflow-core/src/airflow/api_fastapi/auth/managers/base_auth_manager.py index e0d5625814819..f780097cea484 100644 --- a/airflow-core/src/airflow/api_fastapi/auth/managers/base_auth_manager.py +++ b/airflow-core/src/airflow/api_fastapi/auth/managers/base_auth_manager.py @@ -383,18 +383,48 @@ def authorize_view(self, *, access_view: AccessView, user: T, team_name: str | N Core calls this instead of :meth:`is_authorized_view` on team-scoped paths: an override still on the old ``(access_view, user)`` signature would otherwise raise ``TypeError``. Removed in Airflow 4. + + A manager that does not recognise ``access_view`` at all is also tolerated, and + denied -- see :meth:`_authorize_view_unmapped_denied`. + """ + try: + if self._is_authorized_view_team_aware: + return self.is_authorized_view(access_view=access_view, user=user, team_name=team_name) + warnings.warn( + f"The '{type(self).__name__}' auth manager is not team-aware, so team-scoped views are " + "authorized across all teams and may be visible to users of other teams. Add the " + "'team_name' argument to its is_authorized_view (or upgrade the provider). Airflow 4 " + "will require team-aware auth managers.", + RemovedInAirflow4Warning, + stacklevel=2, + ) + return self.is_authorized_view(access_view=access_view, user=user) + except KeyError: + return self._authorize_view_unmapped_denied(access_view) + + def _authorize_view_unmapped_denied(self, access_view: AccessView) -> bool: + """ + Deny an ``AccessView`` the auth manager cannot map, instead of raising. + + ``AccessView`` members are added by core, but auth managers ship as separately + released providers, so a core newer than the installed auth manager can name a + view the manager has never heard of. Managers that translate the enum through a + lookup table (the FAB auth manager, for one) raise ``KeyError`` on such a member, + which would surface as a 500 on the endpoint that consults it. + + Denying keeps the endpoint working and fails closed: an unmappable view means the + manager cannot express who may see the records, and these views gate records with + no other authorization key. Upgrading the auth manager provider to a version that + maps the view restores access. """ - if self._is_authorized_view_team_aware: - return self.is_authorized_view(access_view=access_view, user=user, team_name=team_name) warnings.warn( - f"The '{type(self).__name__}' auth manager is not team-aware, so team-scoped views are " - "authorized across all teams and may be visible to users of other teams. Add the " - "'team_name' argument to its is_authorized_view (or upgrade the provider). Airflow 4 " - "will require team-aware auth managers.", - RemovedInAirflow4Warning, - stacklevel=2, + f"The '{type(self).__name__}' auth manager cannot map the '{access_view.name}' view, so " + "access to it is denied. This usually means the auth manager provider is older than " + "Airflow core; upgrade it to a version that supports this view.", + UserWarning, + stacklevel=3, ) - return self.is_authorized_view(access_view=access_view, user=user) + return False @abstractmethod def is_authorized_custom_view(self, *, method: ResourceMethod, resource_name: str, user: T) -> bool: diff --git a/airflow-core/src/airflow/api_fastapi/auth/managers/models/resource_details.py b/airflow-core/src/airflow/api_fastapi/auth/managers/models/resource_details.py index e4222603058d8..89319b9237bdf 100644 --- a/airflow-core/src/airflow/api_fastapi/auth/managers/models/resource_details.py +++ b/airflow-core/src/airflow/api_fastapi/auth/managers/models/resource_details.py @@ -98,6 +98,10 @@ class VariableDetails: class AccessView(Enum): """Enum of specific views the user tries to access.""" + # Audit log rows not tied to a Dag -- Connection, Variable, Pool, … operations: + # there is no per-Dag key to authorize on, so they get their own admin-by-default + # view rather than riding on Dag-level ``DagAccessEntity.AUDIT_LOG`` access. + AUDIT_LOGS_ALL = "AUDIT_LOGS_ALL" CLUSTER_ACTIVITY = "CLUSTER_ACTIVITY" DOCS = "DOCS" IMPORT_ERRORS = "IMPORT_ERRORS" diff --git a/airflow-core/src/airflow/api_fastapi/auth/managers/simple/simple_auth_manager.py b/airflow-core/src/airflow/api_fastapi/auth/managers/simple/simple_auth_manager.py index ae8aafdb03726..a9858081cb560 100644 --- a/airflow-core/src/airflow/api_fastapi/auth/managers/simple/simple_auth_manager.py +++ b/airflow-core/src/airflow/api_fastapi/auth/managers/simple/simple_auth_manager.py @@ -354,11 +354,12 @@ def is_authorized_variable( def is_authorized_view( self, *, access_view: AccessView, user: SimpleAuthManagerUser, team_name: str | None = None ) -> bool: - # Import errors for files with no registered Dag are admin-only; every other - # view stays readable by viewers. + # Views covering records that have no per-Dag key to authorize on are admin-only -- + # import errors for files with no registered Dag, and audit log rows not tied to a + # Dag. Every other view stays readable by viewers. allow_role = ( SimpleAuthManagerRole.ADMIN - if access_view == AccessView.IMPORT_ERRORS_ALL + if access_view in (AccessView.IMPORT_ERRORS_ALL, AccessView.AUDIT_LOGS_ALL) else SimpleAuthManagerRole.VIEWER ) return self._is_authorized(method="GET", allow_role=allow_role, user=user, team_name=team_name) diff --git a/airflow-core/src/airflow/api_fastapi/core_api/security.py b/airflow-core/src/airflow/api_fastapi/core_api/security.py index 720423797b551..96478eb5a1483 100644 --- a/airflow-core/src/airflow/api_fastapi/core_api/security.py +++ b/airflow-core/src/airflow/api_fastapi/core_api/security.py @@ -265,12 +265,22 @@ def to_orm(self, select: Select) -> Select: class PermittedEventLogFilter(PermittedDagFilter): - """A parameter that filters the permitted even logs for the user.""" + """A parameter that filters the permitted event logs for the user.""" + + def __init__(self, value: set[str] | None = None, *, include_non_dag_logs: bool = False): + super().__init__(value) + self.include_non_dag_logs = include_non_dag_logs def to_orm(self, select: Select) -> Select: - # Event Logs not related to Dags have dag_id as None and are always returned. - # return select.where(Log.dag_id.in_(self.value or set()) or Log.dag_id.is_(None)) - return select.where(or_(Log.dag_id.in_(self.value or set()), Log.dag_id.is_(None))) + permitted_dag_logs = Log.dag_id.in_(self.value or set()) + if not self.include_non_dag_logs: + return select.where(permitted_dag_logs) + # Event logs not related to a Dag have dag_id as None. They record Connection, + # Variable, Pool, … operations, so they carry no per-Dag key to authorize on and + # are gated on ``AccessView.AUDIT_LOGS_ALL`` instead of Dag-level audit access. + # Filtering here rather than after the fact keeps unauthorized rows out of the + # count and pagination too, so their existence does not leak either. + return select.where(or_(permitted_dag_logs, Log.dag_id.is_(None))) class PermittedTIFilter(PermittedDagFilter): @@ -339,9 +349,32 @@ def depends_permitted_dags_filter( ReadableTIFilterDep = Annotated[ PermittedTIFilter, Depends(permitted_dag_filter_factory("GET", PermittedTIFilter)) ] -ReadableEventLogsFilterDep = Annotated[ - PermittedTIFilter, Depends(permitted_dag_filter_factory("GET", PermittedEventLogFilter)) -] + + +def readable_event_logs_filter_factory() -> Callable[[BaseUser, BaseAuthManager], PermittedEventLogFilter]: + """ + Create a callable for Depends in FastAPI that returns the event-log filter for the user. + + Event logs need their own factory rather than ``permitted_dag_filter_factory``: besides + the readable Dag ids, the filter needs to know whether the user may read audit rows that + are not tied to a Dag, which is a separate authorization decision. + """ + + def depends_readable_event_logs_filter( + user: GetUserDep, + auth_manager: AuthManagerDep, + ) -> PermittedEventLogFilter: + return PermittedEventLogFilter( + auth_manager.get_authorized_dag_ids(user=user, method="GET"), + include_non_dag_logs=auth_manager.authorize_view( + access_view=AccessView.AUDIT_LOGS_ALL, user=user + ), + ) + + return depends_readable_event_logs_filter + + +ReadableEventLogsFilterDep = Annotated[PermittedEventLogFilter, Depends(readable_event_logs_filter_factory())] ReadableXComFilterDep = Annotated[ PermittedXComFilter, Depends(permitted_dag_filter_factory("GET", PermittedXComFilter)) ] @@ -423,7 +456,22 @@ async def inner( status_code=status.HTTP_400_BAD_REQUEST, detail="'event_log_id' must be an integer", ) - dag_id = session.scalar(select(Log.dag_id).where(Log.id == event_log_id)) + # Select the id alongside dag_id: a NULL dag_id and a missing row both come back + # as None from a bare ``Log.dag_id`` scalar, and they authorize differently. + row = session.execute(select(Log.id, Log.dag_id).where(Log.id == event_log_id)).one_or_none() + if row is not None and row.dag_id is None: + # The row records an operation that is not tied to a Dag, so there is no + # per-Dag key to authorize on: gate it on ``AccessView.AUDIT_LOGS_ALL``, + # the same view the list endpoint's filter uses for these rows. + _requires_access( + is_authorized_callback=lambda: get_auth_manager().authorize_view( + access_view=AccessView.AUDIT_LOGS_ALL, user=user + ), + ) + return + # A missing row keeps the Dag-level check so the route can answer 404 rather + # than turning an unknown id into a permission error. + dag_id = row.dag_id if row is not None else None requires_access_dag(method, DagAccessEntity.AUDIT_LOG, dag_id)( request, diff --git a/airflow-core/tests/unit/api_fastapi/auth/managers/simple/test_simple_auth_manager.py b/airflow-core/tests/unit/api_fastapi/auth/managers/simple/test_simple_auth_manager.py index c10e1273d163d..b51329006d4f1 100644 --- a/airflow-core/tests/unit/api_fastapi/auth/managers/simple/test_simple_auth_manager.py +++ b/airflow-core/tests/unit/api_fastapi/auth/managers/simple/test_simple_auth_manager.py @@ -347,6 +347,33 @@ def test_is_authorized_view_methods(self, auth_manager, api, kwargs, role, resul is result ) + @pytest.mark.parametrize( + "access_view", + [AccessView.IMPORT_ERRORS_ALL, AccessView.AUDIT_LOGS_ALL], + ) + @pytest.mark.parametrize( + ("role", "result"), + [ + ("ADMIN", True), + ("OP", False), + ("USER", False), + ("VIEWER", False), + ], + ) + def test_is_authorized_view_admin_only_views(self, auth_manager, access_view, role, result): + """The views covering records with no per-Dag key to authorize on are admin-only. + + Every other view is readable by a viewer (asserted above); these two gate records + that carry no other authorization key -- import errors for files with no registered + Dag, and audit log rows not tied to a Dag -- so they must not ride on viewer access. + """ + assert ( + auth_manager.is_authorized_view( + access_view=access_view, user=SimpleAuthManagerUser(username="test", role=role) + ) + is result + ) + @pytest.mark.parametrize( "api", [ diff --git a/airflow-core/tests/unit/api_fastapi/auth/managers/test_base_auth_manager.py b/airflow-core/tests/unit/api_fastapi/auth/managers/test_base_auth_manager.py index 5ca46908e3a69..d7f2286aa1508 100644 --- a/airflow-core/tests/unit/api_fastapi/auth/managers/test_base_auth_manager.py +++ b/airflow-core/tests/unit/api_fastapi/auth/managers/test_base_auth_manager.py @@ -190,6 +190,26 @@ def is_authorized_view(self, *, access_view, user=None): # old signature, no te assert result is True + def test_authorize_view_denies_a_view_the_manager_cannot_map(self): + # AccessView members are added by core, but auth managers ship as separately + # released providers, so a core newer than the installed manager can name a view + # the manager has never heard of. Managers that translate the enum through a lookup + # table raise KeyError on such a member, which would surface as a 500 on the + # endpoint. authorize_view denies instead -- the endpoint keeps working and the + # records these views gate stay closed. + class LookupTableAuthManager(EmptyAuthManager): + def is_authorized_view(self, *, access_view, user=None, team_name=None): + return {AccessView.WEBSITE: True}[access_view] + + manager = LookupTableAuthManager() + + with pytest.warns(UserWarning, match="cannot map the 'DOCS' view"): + result = manager.authorize_view(access_view=AccessView.DOCS, user=None) + + assert result is False + # A view the manager does map is unaffected. + assert manager.authorize_view(access_view=AccessView.WEBSITE, user=None) is True + def test_authorize_view_treats_kwargs_override_as_team_aware(self): class KwargsAuthManager(EmptyAuthManager): def is_authorized_view(self, *, access_view, user=None, **kwargs): diff --git a/airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_event_logs.py b/airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_event_logs.py index 871692f98d299..75fd6229d5d94 100644 --- a/airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_event_logs.py +++ b/airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_event_logs.py @@ -22,7 +22,11 @@ import pytest from sqlalchemy.orm import Session -from airflow.api_fastapi.auth.managers.models.resource_details import DagAccessEntity, DagDetails +from airflow.api_fastapi.auth.managers.models.resource_details import ( + AccessView, + DagAccessEntity, + DagDetails, +) from airflow.models.log import Log from airflow.utils.session import NEW_SESSION, provide_session @@ -236,6 +240,49 @@ def test_should_authorize_with_event_log_dag_id(self, test_client, setup): user=mock.ANY, ) + @pytest.mark.parametrize( + ("can_view_all_audit_logs", "expected_status_code"), + [ + pytest.param(True, 200, id="with-AUDIT_LOGS_ALL-sees-row"), + pytest.param(False, 403, id="without-AUDIT_LOGS_ALL-forbidden"), + ], + ) + def test_non_dag_row_is_gated_on_audit_logs_all( + self, test_client, setup, can_view_all_audit_logs, expected_status_code + ): + """A row with a NULL dag_id records an operation that is not tied to a Dag -- a + Connection, Variable or Pool change -- so it has no per-Dag key to authorize on. + Visibility is gated on the dedicated ``AUDIT_LOGS_ALL`` view rather than riding on + Dag-level audit log access, which every viewer holds. + """ + event_log_id = setup[EVENT_NORMAL].id + with mock.patch( + "airflow.api_fastapi.auth.managers.simple.simple_auth_manager.SimpleAuthManager.is_authorized_view", + return_value=can_view_all_audit_logs, + ) as mock_is_authorized_view: + response = test_client.get(f"/eventLogs/{event_log_id}") + + assert response.status_code == expected_status_code + mock_is_authorized_view.assert_called_once_with( + access_view=AccessView.AUDIT_LOGS_ALL, user=mock.ANY, team_name=None + ) + + def test_unknown_id_stays_404_and_does_not_consult_audit_logs_all(self, test_client, setup): + """An id that matches no row must answer 404, not 403. + + A missing row and a NULL dag_id both read back as ``None``, so the guard has to tell + them apart: turning an unknown id into a permission error would change the documented + contract of the endpoint for callers that are allowed to use it. + """ + with mock.patch( + "airflow.api_fastapi.auth.managers.simple.simple_auth_manager.SimpleAuthManager.is_authorized_view", + return_value=False, + ) as mock_is_authorized_view: + response = test_client.get(f"/eventLogs/{EVENT_NON_EXISTED_ID}") + + assert response.status_code == 404 + mock_is_authorized_view.assert_not_called() + @provide_session def test_should_return_404_for_log_without_dttm(self, test_client, *, session: Session = NEW_SESSION): # noqa: PT028 event_log = Log(event=EVENT_WITHOUT_DTTM) @@ -463,3 +510,43 @@ def test_should_raises_401_unauthenticated(self, unauthenticated_test_client): def test_should_raises_403_forbidden(self, unauthorized_test_client): response = unauthorized_test_client.get("/eventLogs") assert response.status_code == 403 + + @pytest.mark.parametrize( + ("can_view_all_audit_logs", "expected_events"), + [ + pytest.param( + True, + [EVENT_NORMAL, EVENT_WITH_OWNER, TASK_INSTANCE_EVENT, EVENT_WITH_OWNER_AND_TASK_INSTANCE], + id="with-AUDIT_LOGS_ALL-sees-non-dag-rows", + ), + pytest.param( + False, + [TASK_INSTANCE_EVENT, EVENT_WITH_OWNER_AND_TASK_INSTANCE], + id="without-AUDIT_LOGS_ALL-only-dag-rows", + ), + ], + ) + def test_non_dag_rows_are_gated_on_audit_logs_all( + self, test_client, can_view_all_audit_logs, expected_events + ): + """Rows with a NULL dag_id are returned only to callers holding ``AUDIT_LOGS_ALL``. + + Before this gate every caller that could read event logs at all received them, which + for the default auth manager is any viewer. ``EVENT_NORMAL`` and ``EVENT_WITH_OWNER`` + carry no dag_id; the other two are bound to a Dag and stay visible either way. + """ + with mock.patch( + "airflow.api_fastapi.auth.managers.simple.simple_auth_manager.SimpleAuthManager.is_authorized_view", + return_value=can_view_all_audit_logs, + ) as mock_is_authorized_view: + response = test_client.get("/eventLogs") + + assert response.status_code == 200 + resp_json = response.json() + # Filtered in the query, so the excluded rows are absent from the count and + # pagination too -- their existence does not leak through total_entries. + assert resp_json["total_entries"] == len(expected_events) + assert {event_log["event"] for event_log in resp_json["event_logs"]} == set(expected_events) + mock_is_authorized_view.assert_called_once_with( + access_view=AccessView.AUDIT_LOGS_ALL, user=mock.ANY, team_name=None + ) diff --git a/providers/common/compat/src/airflow/providers/common/compat/security/access_view.py b/providers/common/compat/src/airflow/providers/common/compat/security/access_view.py index 4c79c5a139cba..23209b68ad97c 100644 --- a/providers/common/compat/src/airflow/providers/common/compat/security/access_view.py +++ b/providers/common/compat/src/airflow/providers/common/compat/security/access_view.py @@ -16,14 +16,16 @@ # under the License. from __future__ import annotations -# ``AccessView.IMPORT_ERRORS_ALL`` was added in Airflow 3.4.0, and providers are -# released independently of core. ``None`` signals the view is unavailable on the -# running core and callers should skip mapping it. +# ``AccessView.IMPORT_ERRORS_ALL`` and ``AccessView.AUDIT_LOGS_ALL`` were added in +# Airflow 3.4.0, and providers are released independently of core. ``None`` signals the +# view is unavailable on the running core and callers should skip mapping it. try: from airflow.api_fastapi.auth.managers.models.resource_details import AccessView IMPORT_ERRORS_ALL_ACCESS_VIEW: AccessView | None = getattr(AccessView, "IMPORT_ERRORS_ALL", None) + AUDIT_LOGS_ALL_ACCESS_VIEW: AccessView | None = getattr(AccessView, "AUDIT_LOGS_ALL", None) except ImportError: IMPORT_ERRORS_ALL_ACCESS_VIEW = None + AUDIT_LOGS_ALL_ACCESS_VIEW = None -__all__ = ["IMPORT_ERRORS_ALL_ACCESS_VIEW"] +__all__ = ["AUDIT_LOGS_ALL_ACCESS_VIEW", "IMPORT_ERRORS_ALL_ACCESS_VIEW"] diff --git a/providers/common/compat/tests/unit/common/compat/security/test_access_view.py b/providers/common/compat/tests/unit/common/compat/security/test_access_view.py index 1804ff96e24a6..9798ff9330ae6 100644 --- a/providers/common/compat/tests/unit/common/compat/security/test_access_view.py +++ b/providers/common/compat/tests/unit/common/compat/security/test_access_view.py @@ -21,40 +21,44 @@ import types from unittest import mock +import pytest + RESOURCE_DETAILS_MODULE = "airflow.api_fastapi.auth.managers.models.resource_details" ACCESS_VIEW_SHIM_MODULE = "airflow.providers.common.compat.security.access_view" -def test_resolves_to_the_core_access_view_member_or_none(): - """The shim mirrors the running core: the ``AccessView.IMPORT_ERRORS_ALL`` - member on a core that defines it (>= 3.4.0), otherwise ``None``. Kept - version-agnostic so it holds across the whole provider compatibility matrix, - including cores that predate the member or lack ``api_fastapi`` entirely. +@pytest.mark.parametrize("member_name", ["IMPORT_ERRORS_ALL", "AUDIT_LOGS_ALL"]) +def test_resolves_to_the_core_access_view_member_or_none(member_name): + """The shim mirrors the running core: the ``AccessView`` member on a core that + defines it (>= 3.4.0), otherwise ``None``. Kept version-agnostic so it holds + across the whole provider compatibility matrix, including cores that predate the + member or lack ``api_fastapi`` entirely. """ - from airflow.providers.common.compat.security.access_view import IMPORT_ERRORS_ALL_ACCESS_VIEW + shim = importlib.import_module(ACCESS_VIEW_SHIM_MODULE) try: from airflow.api_fastapi.auth.managers.models.resource_details import AccessView - expected = getattr(AccessView, "IMPORT_ERRORS_ALL", None) + expected = getattr(AccessView, member_name, None) except ImportError: expected = None - assert IMPORT_ERRORS_ALL_ACCESS_VIEW is expected + assert getattr(shim, f"{member_name}_ACCESS_VIEW") is expected def test_is_none_on_older_core_without_the_member(): - """On a core that predates ``AccessView.IMPORT_ERRORS_ALL`` the shim resolves to ``None``.""" + """On a core that predates the new ``AccessView`` members the shim resolves to ``None``.""" - class _AccessViewWithoutImportErrorsAll: - """Stand-in for an older core AccessView that lacks the new member.""" + class _AccessViewWithoutNewMembers: + """Stand-in for an older core AccessView that lacks the new members.""" fake_resource_details = types.ModuleType(RESOURCE_DETAILS_MODULE) - fake_resource_details.AccessView = _AccessViewWithoutImportErrorsAll + fake_resource_details.AccessView = _AccessViewWithoutNewMembers with mock.patch.dict(sys.modules, {RESOURCE_DETAILS_MODULE: fake_resource_details}): reloaded = importlib.reload(importlib.import_module(ACCESS_VIEW_SHIM_MODULE)) assert reloaded.IMPORT_ERRORS_ALL_ACCESS_VIEW is None + assert reloaded.AUDIT_LOGS_ALL_ACCESS_VIEW is None # Restore the module against the real core so later imports see the real value. importlib.reload(importlib.import_module(ACCESS_VIEW_SHIM_MODULE)) diff --git a/providers/fab/docs/auth-manager/access-control.rst b/providers/fab/docs/auth-manager/access-control.rst index da6ad74648ed6..52658bd6e80dc 100644 --- a/providers/fab/docs/auth-manager/access-control.rst +++ b/providers/fab/docs/auth-manager/access-control.rst @@ -172,8 +172,10 @@ Endpoint /assets GET Assets.can_read Viewer /assets/{uri} GET Assets.can_read Viewer /assets/events GET Assets.can_read Viewer -/eventLogs GET Audit Logs.can_read Viewer -/eventLogs/{event_log_id} GET Audit Logs.can_read Viewer +/eventLogs GET Audit Logs.can_read Admin + All Audit Logs.can_read (for rows not tied to a Dag) +/eventLogs/{event_log_id} GET Audit Logs.can_read Admin + All Audit Logs.can_read (for rows not tied to a Dag) /importErrors GET ImportError.can_read Viewer /importErrors/{import_error_id} GET ImportError.can_read Viewer /health GET None Public diff --git a/providers/fab/src/airflow/providers/fab/auth_manager/fab_auth_manager.py b/providers/fab/src/airflow/providers/fab/auth_manager/fab_auth_manager.py index 046278a04089d..09e584243020b 100644 --- a/providers/fab/src/airflow/providers/fab/auth_manager/fab_auth_manager.py +++ b/providers/fab/src/airflow/providers/fab/auth_manager/fab_auth_manager.py @@ -56,7 +56,10 @@ from airflow.exceptions import AirflowConfigException, AirflowProviderDeprecationWarning from airflow.models import Connection, DagModel, Pool, Variable from airflow.providers.common.compat.sdk import AirflowException, conf -from airflow.providers.common.compat.security.access_view import IMPORT_ERRORS_ALL_ACCESS_VIEW +from airflow.providers.common.compat.security.access_view import ( + AUDIT_LOGS_ALL_ACCESS_VIEW, + IMPORT_ERRORS_ALL_ACCESS_VIEW, +) from airflow.providers.fab.auth_manager.models import Permission, Role, User from airflow.providers.fab.auth_manager.models.anonymous_user import AnonymousUser from airflow.providers.fab.version_compat import AIRFLOW_V_3_1_PLUS @@ -65,6 +68,7 @@ from airflow.providers.fab.www.security.permissions import ( ACTION_CAN_READ, RESOURCE_AUDIT_LOG, + RESOURCE_AUDIT_LOG_ALL, RESOURCE_BACKFILL, RESOURCE_CLUSTER_ACTIVITY, RESOURCE_CONFIG, @@ -148,10 +152,13 @@ AccessView.WEBSITE: RESOURCE_WEBSITE, } -# ``AccessView.IMPORT_ERRORS_ALL`` only exists on core >= 3.4.0; the compat shim -# yields ``None`` on older core so this provider still imports there. +# ``AccessView.IMPORT_ERRORS_ALL`` and ``AccessView.AUDIT_LOGS_ALL`` only exist on +# core >= 3.4.0; the compat shim yields ``None`` on older core so this provider still +# imports there. if IMPORT_ERRORS_ALL_ACCESS_VIEW is not None: _MAP_ACCESS_VIEW_TO_FAB_RESOURCE_TYPE[IMPORT_ERRORS_ALL_ACCESS_VIEW] = RESOURCE_IMPORT_ERROR_ALL +if AUDIT_LOGS_ALL_ACCESS_VIEW is not None: + _MAP_ACCESS_VIEW_TO_FAB_RESOURCE_TYPE[AUDIT_LOGS_ALL_ACCESS_VIEW] = RESOURCE_AUDIT_LOG_ALL _MAP_MENU_ITEM_TO_FAB_RESOURCE_TYPE = { MenuItem.ASSETS: RESOURCE_ASSET, diff --git a/providers/fab/src/airflow/providers/fab/auth_manager/security_manager/override.py b/providers/fab/src/airflow/providers/fab/auth_manager/security_manager/override.py index 05a72235f7d8e..22ba8353119f9 100644 --- a/providers/fab/src/airflow/providers/fab/auth_manager/security_manager/override.py +++ b/providers/fab/src/airflow/providers/fab/auth_manager/security_manager/override.py @@ -332,6 +332,7 @@ class FabAirflowSecurityManagerOverride(AirflowSecurityManagerV2): ADMIN_PERMISSIONS = [ (permissions.ACTION_CAN_READ, permissions.RESOURCE_AUDIT_LOG), (permissions.ACTION_CAN_ACCESS_MENU, permissions.RESOURCE_AUDIT_LOG), + (permissions.ACTION_CAN_READ, permissions.RESOURCE_AUDIT_LOG_ALL), (permissions.ACTION_CAN_READ, permissions.RESOURCE_IMPORT_ERROR_ALL), (permissions.ACTION_CAN_READ, permissions.RESOURCE_TASK_RESCHEDULE), (permissions.ACTION_CAN_ACCESS_MENU, permissions.RESOURCE_TASK_RESCHEDULE), 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 10dfd6f45f29a..7fcf1ea357a0b 100644 --- a/providers/fab/src/airflow/providers/fab/www/security/permissions.py +++ b/providers/fab/src/airflow/providers/fab/www/security/permissions.py @@ -22,6 +22,7 @@ RESOURCE_ACTION = "Permissions" RESOURCE_ADMIN_MENU = "Admin" RESOURCE_AUDIT_LOG = "Audit Logs" +RESOURCE_AUDIT_LOG_ALL = "All Audit Logs" RESOURCE_BACKFILL = "Backfills" RESOURCE_BROWSE_MENU = "Browse" RESOURCE_CONFIG = "Configurations" From 880be6870df2c136cc053fdc961b7e07814c6377 Mon Sep 17 00:00:00 2001 From: Jarek Potiuk Date: Thu, 30 Jul 2026 13:59:18 +0200 Subject: [PATCH 2/3] Add newsfragment for the audit log view gate --- .../newsfragments/70759.significant.rst | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) create mode 100644 airflow-core/newsfragments/70759.significant.rst diff --git a/airflow-core/newsfragments/70759.significant.rst b/airflow-core/newsfragments/70759.significant.rst new file mode 100644 index 0000000000000..3b3c864d53b75 --- /dev/null +++ b/airflow-core/newsfragments/70759.significant.rst @@ -0,0 +1,24 @@ +Audit log rows not tied to a Dag now require the ``AUDIT_LOGS_ALL`` view + +Rows in the audit log whose ``dag_id`` is ``NULL`` record operations that are not tied to a +Dag, such as Connection, Variable and Pool changes. They were returned to any caller that +could read event logs at all, because there is no per-Dag key to authorize them on and both +read paths fell back to a Dag-level audit log check. + +They are now gated on a dedicated ``AccessView.AUDIT_LOGS_ALL``, in the same way +``AccessView.IMPORT_ERRORS_ALL`` gates import errors for files with no registered Dag. Rows +bound to a Dag keep their existing per-Dag check. + +**Behaviour changes:** + +- ``GET /eventLogs`` no longer returns rows with a ``NULL`` ``dag_id`` to callers without the + new view, and excludes them from ``total_entries`` and pagination. + ``GET /eventLogs/{event_log_id}`` returns ``403`` for such a row. An id matching no row + still returns ``404``. +- With the simple auth manager the view is granted to the ``ADMIN`` role only. With the FAB + auth manager it maps to a new ``All Audit Logs`` resource, which ships in the ``Admin`` + role; custom roles that need to read these rows must be granted + ``All Audit Logs.can_read`` explicitly. +- An auth manager that cannot map a requested ``AccessView`` now denies that view and emits + a ``UserWarning`` instead of raising ``KeyError``. This affects auth managers older than + the core they run against, which previously failed the request with a server error. From 50af537c65148697324534dd52e5ee587926ca24 Mon Sep 17 00:00:00 2001 From: Jarek Potiuk Date: Thu, 30 Jul 2026 15:26:57 +0200 Subject: [PATCH 3/3] Update event log guard tests for the row lookup shape The guard now selects Log.id alongside Log.dag_id so a missing row and a NULL dag_id can be told apart. These tests mocked session.scalar, so with a Mock session the new call returned a truthy Mock whose dag_id was itself a Mock. Mock the execute(...).one_or_none() shape instead, and add the two cases the unit tests did not cover: a row that exists with a NULL dag_id authorizes on AUDIT_LOGS_ALL rather than the Dag check, and is Forbidden without it. --- .../api_fastapi/core_api/test_security.py | 57 ++++++++++++++++++- 1 file changed, 54 insertions(+), 3 deletions(-) diff --git a/airflow-core/tests/unit/api_fastapi/core_api/test_security.py b/airflow-core/tests/unit/api_fastapi/core_api/test_security.py index 8824969f976ee..37750e30da959 100644 --- a/airflow-core/tests/unit/api_fastapi/core_api/test_security.py +++ b/airflow-core/tests/unit/api_fastapi/core_api/test_security.py @@ -27,6 +27,7 @@ from airflow.api_fastapi.app import create_app from airflow.api_fastapi.auth.managers.base_auth_manager import COOKIE_NAME_JWT_TOKEN from airflow.api_fastapi.auth.managers.models.resource_details import ( + AccessView, ConnectionDetails, DagAccessEntity, DagDetails, @@ -468,7 +469,7 @@ async def test_requires_access_event_log_authorized_from_path( mock_get_team_name.return_value = "team1" session = Mock() - session.scalar.return_value = "event_log_dag_id" + session.execute.return_value.one_or_none.return_value = Mock(id=42, dag_id="event_log_dag_id") request = Mock() request.path_params = {"event_log_id": "42"} @@ -496,7 +497,7 @@ async def test_requires_access_event_log_unauthorized(self, mock_get_auth_manage mock_get_team_name.return_value = None session = Mock() - session.scalar.return_value = "unauthorized_dag" + session.execute.return_value.one_or_none.return_value = Mock(id=1, dag_id="unauthorized_dag") request = Mock() request.path_params = {"event_log_id": "1"} @@ -524,7 +525,7 @@ async def test_requires_access_event_log_row_not_found(self, mock_get_auth_manag mock_get_auth_manager.return_value = auth_manager session = Mock() - session.scalar.return_value = None + session.execute.return_value.one_or_none.return_value = None request = Mock() request.path_params = {"event_log_id": "999"} @@ -542,6 +543,56 @@ async def test_requires_access_event_log_row_not_found(self, mock_get_auth_manag ) mock_get_team_name.assert_not_called() + @pytest.mark.db_test + @pytest.mark.asyncio + @patch.object(DagModel, "get_team_name") + @patch("airflow.api_fastapi.core_api.security.get_auth_manager") + async def test_requires_access_event_log_non_dag_row_uses_audit_logs_all( + self, mock_get_auth_manager, mock_get_team_name + ): + """A row that exists with a NULL dag_id is gated on AUDIT_LOGS_ALL, not the Dag check. + + Such a row records an operation that is not tied to a Dag, so there is no per-Dag key + to authorize on. It must not fall through to an unscoped ``DagDetails(id=None)`` check, + which any caller holding Dag-level audit access would pass. + """ + auth_manager = Mock() + auth_manager.authorize_view.return_value = True + mock_get_auth_manager.return_value = auth_manager + + session = Mock() + session.execute.return_value.one_or_none.return_value = Mock(id=7, dag_id=None) + + request = Mock() + request.path_params = {"event_log_id": "7"} + request.query_params = {} + user = Mock() + + await requires_access_event_log("GET")(request, user, session) + + auth_manager.authorize_view.assert_called_once_with(access_view=AccessView.AUDIT_LOGS_ALL, user=user) + auth_manager.is_authorized_dag.assert_not_called() + mock_get_team_name.assert_not_called() + + @pytest.mark.db_test + @pytest.mark.asyncio + @patch("airflow.api_fastapi.core_api.security.get_auth_manager") + async def test_requires_access_event_log_non_dag_row_denied(self, mock_get_auth_manager): + """Without AUDIT_LOGS_ALL, a non-Dag row is Forbidden rather than returned.""" + auth_manager = Mock() + auth_manager.authorize_view.return_value = False + mock_get_auth_manager.return_value = auth_manager + + session = Mock() + session.execute.return_value.one_or_none.return_value = Mock(id=7, dag_id=None) + + request = Mock() + request.path_params = {"event_log_id": "7"} + request.query_params = {} + + with pytest.raises(HTTPException, match="Forbidden"): + await requires_access_event_log("GET")(request, Mock(), session) + @pytest.mark.db_test @pytest.mark.parametrize("bad_event_log_id", ["abc", "1.5", "1,2", ""]) @patch("airflow.api_fastapi.core_api.security.requires_access_dag")