potiuk commented on code in PR #69790:
URL: https://github.com/apache/airflow/pull/69790#discussion_r3609123194
##########
airflow-core/src/airflow/api_fastapi/core_api/routes/public/import_error.py:
##########
@@ -159,6 +173,31 @@ def get_import_errors(
# Subquery for files that have any Dags
files_with_any_dags =
select(DagModel.relative_fileloc).distinct().subquery()
Review Comment:
Good catch — the same `relative_fileloc` can exist in more than one bundle,
so matching on filename alone was wrong. Fixed: the has-any-Dag subquery now
selects `bundle_name` and the join matches on both filename and bundle. Thanks!
---
Drafted-by: Claude Code (Opus 4.8); reviewed by @potiuk before posting
##########
airflow-core/src/airflow/api_fastapi/core_api/routes/public/import_error.py:
##########
@@ -159,6 +173,31 @@ def get_import_errors(
# Subquery for files that have any Dags
files_with_any_dags =
select(DagModel.relative_fileloc).distinct().subquery()
+ # Import errors for files that have **no** registered Dag have no per-Dag
+ # key to authorize on. Authorize them on the dedicated
``IMPORT_ERRORS_ALL``
+ # view (admin by default), scoped to the file's team via its bundle, and
+ # keep only the ones the caller may see. Filtering here (rather than
+ # redacting in the loop) also excludes unauthorized rows from the count and
+ # pagination, so their existence -- and, for team-scoped bundles, the
+ # cross-team file/bundle names -- does not leak.
+ unregistered_errors = session.execute(
+ select(ParseImportError.id, ParseImportError.bundle_name)
+ .outerjoin(files_with_any_dags, ParseImportError.filename ==
files_with_any_dags.c.relative_fileloc)
Review Comment:
Good catch — the same `relative_fileloc` can exist in more than one bundle,
so matching on filename alone was wrong. Fixed: the has-any-Dag subquery now
selects `bundle_name` and the join matches on both filename and bundle. Thanks!
---
Drafted-by: Claude Code (Opus 4.8); reviewed by @potiuk before posting
##########
airflow-core/src/airflow/api_fastapi/auth/managers/base_auth_manager.py:
##########
@@ -365,6 +365,28 @@ def is_authorized_view(
:param user: the user to performing the action
"""
+ def is_authorized_view_for_team(
Review Comment:
I'd rather keep the separate method here, mainly for backwards
compatibility. `is_authorized_view` is an `@abstractmethod`, and Python's ABC
machinery only checks that the *name* is overridden, not the signature — so
adding `team_name` there wouldn't force existing implementations to update, but
any out-of-tree AuthManager that keeps the current `(access_view, user)`
signature would raise `TypeError` the moment core calls it with `team_name` (a
runtime break, with no import- or instantiation-time warning). The separate
`is_authorized_view_for_team` with a default fallback avoids that — out-of-tree
managers keep working unchanged, and only multi-team-aware managers override it.
We *could* instead fold it into `is_authorized_view` with an optional
`team_name` and take it as a breaking change for out-of-band AuthManagers, but
I'd prefer to avoid that. WDYT?
---
Drafted-by: Claude Code (Opus 4.8); reviewed by @potiuk before posting
--
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]
