Skip to content

Gate audit log rows not tied to a Dag on a dedicated AccessView - #70759

Merged
potiuk merged 3 commits into
apache:mainfrom
potiuk:scope-non-dag-audit-rows-to-authorized-readers
Aug 1, 2026
Merged

Gate audit log rows not tied to a Dag on a dedicated AccessView#70759
potiuk merged 3 commits into
apache:mainfrom
potiuk:scope-non-dag-audit-rows-to-authorized-readers

Conversation

@potiuk

@potiuk potiuk commented Jul 30, 2026

Copy link
Copy Markdown
Member

Audit log rows whose dag_id is NULL record operations that are not tied to a
Dag — Connection, Variable and Pool changes. Both read paths treated them as
Dag-level audit data:

  • PermittedEventLogFilter.to_orm appended or_(…, Log.dag_id.is_(None))
    unconditionally, so every caller that could read event logs at all received them;
  • requires_access_event_log resolved such a row to dag_id=None and passed it to
    requires_access_dag(…, DagAccessEntity.AUDIT_LOG, None), an unscoped Dag check.

There is no per-Dag key to authorize these rows on, and Dag-level audit access is
held by any viewer under the default auth manager — not the audience for a
Connection or Variable change record.

Approach

This is the same shape of problem AccessView.IMPORT_ERRORS_ALL already solves for
import errors of files with no registered Dag, so it gets the same treatment: a
dedicated, admin-by-default view.

  • AccessView.AUDIT_LOGS_ALL is added and both read paths consult it
  • the list endpoint filters the rows out in the query, so they stay out of
    total_entries and pagination too and their existence does not leak; the detail
    endpoint answers 403
  • SimpleAuthManager grants it to admins only, alongside IMPORT_ERRORS_ALL
  • the FAB auth manager maps it to a new All Audit Logs resource, added to
    ADMIN_PERMISSIONS, reached through the common.compat shim so the provider
    still imports on a core that predates the view

Rows bound to a Dag are unaffected and keep their existing per-Dag check.

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. Telling them apart lets a missing row keep the Dag-level check, so the
route still answers 404 rather than turning an unknown id into a permission error.

authorize_view no longer propagates 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 (_MAP_ACCESS_VIEW_TO_FAB_RESOURCE_TYPE[access_view])
and would raise, surfacing as a 500 on whichever endpoint consults the view — which
is how any future AccessView addition would land on an older FAB. It now denies
and warns instead: the endpoint keeps working, and it fails closed. This also covers
IMPORT_ERRORS_ALL, which has the same exposure today.

Behaviour changes

  1. GET /eventLogs no longer returns rows with a NULL dag_id to callers without
    AUDIT_LOGS_ALL, and those rows are excluded from total_entries. Under
    SimpleAuthManager that means admins only; under FAB, holders of
    All Audit Logs.can_read, which ships in the Admin role.
  2. GET /eventLogs/{event_log_id} answers 403 for such a row to the same callers.
    An id matching no row still answers 404.
  3. An auth manager that cannot map a requested AccessView now denies the view with
    a UserWarning instead of raising KeyError (previously a 500).
  4. New FAB permission resource All Audit Logs; existing custom roles do not gain it
    automatically and need it granted explicitly to keep reading these rows.

Two incidental fixes in the touched code: ReadableEventLogsFilterDep was annotated
PermittedTIFilter rather than PermittedEventLogFilter, and the two /eventLogs
rows in the FAB access-control table listed a minimum role of Viewer although
Audit Logs.can_read has only ever been in ADMIN_PERMISSIONS.

Test plan

  • test_event_logs.py — the list endpoint returns only Dag-bound rows without the
    view and all four rows with it (asserting total_entries, not just the payload);
    the detail endpoint 403s/200s on a non-Dag row; an unknown id still 404s and does
    not consult the view
  • 4 of those 5 fail against the unmodified security.py; the unknown-id case passes
    either way and is a regression guard for the 404-vs-403 distinction
  • test_simple_auth_manager.pyAUDIT_LOGS_ALL and IMPORT_ERRORS_ALL authorize
    for ADMIN and deny OP/USER/VIEWER, while every other view stays viewer-readable
  • test_base_auth_manager.py — a lookup-table manager denies an unmapped view with a
    warning and still answers a mapped one
  • test_access_view.py — the compat shim resolves both members, and both to None on
    a core without them
  • Suites run: test_event_logs.py 37 passed / 3 skipped (backend-gated, pre-existing),
    test_base_auth_manager.py + test_simple_auth_manager.py 252 passed,
    test_import_error.py + compat 49 passed, test_fab_auth_manager.py 127 passed
  • ruff check and ruff format clean on every changed file
Was generative AI tooling used to co-author this PR?
  • Yes — Claude Opus 5 (1M context)

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

`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
@potiuk potiuk added this to the Airflow 3.4.0 milestone Jul 30, 2026
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.
@potiuk potiuk added kind:feature Feature Requests and removed kind:documentation labels Jul 30, 2026

@pierrejeambrun pierrejeambrun left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks

@potiuk
potiuk merged commit dfaff48 into apache:main Aug 1, 2026
112 checks passed
@potiuk
potiuk deleted the scope-non-dag-audit-rows-to-authorized-readers branch August 1, 2026 00:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants