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. 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/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") 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"