kaxil commented on code in PR #72909:
URL: https://github.com/apache/airflow/pull/72909#discussion_r4007948174
##########
airflow-core/src/airflow/api_fastapi/core_api/security.py:
##########
@@ -361,6 +336,21 @@ def to_orm(self, select: Select) -> Select:
return select.where(Backfill.dag_id.in_(self.value or set()))
+class PermittedDagBundleFilter(PermittedDagFilter):
+ """A parameter that filters Dag bundles to the ones holding a Dag the user
may read."""
+
+ def to_orm(self, statement: Select) -> Select:
+ # A bundle carries no per-Dag key to authorize on, so it is scoped by
the Dags inside it.
+ # Filtering in the query keeps unauthorized rows out of the count and
pagination as well.
+ # A bundle from which no Dag has ever parsed is therefore invisible
until one does, and one
+ # whose Dags have since been removed stays visible while a stale
``DagModel`` row names it.
+ return statement.where(
+ DagBundleModel.name.in_(
+
select(DagModel.bundle_name).where(DagModel.dag_id.in_(self.value or set()))
+ )
+ )
Review Comment:
Agreed, and fixed in 5c3248b.
`PermittedDagBundleFilter` now takes `include_dagless_bundles`, set from
`authorize_view(AccessView.IMPORT_ERRORS_ALL)` in a new
`readable_dag_bundles_filter_factory` modelled on
`readable_event_logs_filter_factory`. A bundle with no `DagModel` row has
nothing to authorize against, so it rides on the same admin-by-default view
that already governs import errors for a file that never registered a Dag. The
added clause is `OR dag_bundle.name NOT IN (SELECT dag.bundle_name ...)`,
additive over the readable-Dag disjunct, so it never bypasses Dag scoping for a
bundle that does have Dags, and it stays in SQL so pagination and
`total_entries` are still correct.
One piece I would like your read on. The view is authorized once without a
`team_name`, because which bundles qualify is not known until the query runs.
That is asymmetric with `_import_error_counts`, which does scope per bundle
team. It is inert under FAB and SimpleAuthManager since both treat admin as
global, but a team-aware manager granting the unscoped view would show a holder
every Dag-less bundle rather than only their own teams'. There is a comment at
the call site saying so.
If you would rather it were fully team-scoped, the shape is: resolve the
Dag-less bundle names first, filter them with `authorize_view(...,
team_name=...)`, and pass the survivors into the filter as an allow-list. That
costs one extra query per request on an endpoint that polls, which is the cost
you flag in your other comment, so I did not want to choose that unilaterally.
--
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]
