potiuk opened a new pull request, #70759:
URL: https://github.com/apache/airflow/pull/70759
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
- [x] `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
- [x] 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
- [x] `test_simple_auth_manager.py` — `AUDIT_LOGS_ALL` and
`IMPORT_ERRORS_ALL` authorize
for ADMIN and deny OP/USER/VIEWER, while every other view stays
viewer-readable
- [x] `test_base_auth_manager.py` — a lookup-table manager denies an
unmapped view with a
warning and still answers a mapped one
- [x] `test_access_view.py` — the compat shim resolves both members, and
both to `None` on
a core without them
- [x] 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
- [x] `ruff check` and `ruff format` clean on every changed file
##### Was generative AI tooling used to co-author this PR?
- [X] 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
