kaxil commented on code in PR #76329:
URL: https://github.com/apache/airflow/pull/76329#discussion_r4449597978
##########
providers/keycloak/src/airflow/providers/keycloak/auth_manager/keycloak_auth_manager.py:
##########
@@ -91,9 +91,10 @@
RESOURCE_ID_ATTRIBUTE_NAME = "resource_id"
-# ``AccessView`` values covering records that carry no per-Dag or per-team key
to authorize on. They
-# are admin-by-default in core. Compared by value because older Airflow
versions lack some members.
-ADMIN_ACCESS_VIEW_NAMES = frozenset({"AUDIT_LOGS_ALL", "IMPORT_ERRORS_ALL",
"REPARSE_ALL"})
+# ``AccessView`` values showing records beyond the Dags a caller can read. In
multi-team mode they
+# are checked against ``AdminView``, which only ``SuperAdmin`` holds. Compared
by value because
+# older Airflow versions lack some members.
+ADMIN_ACCESS_VIEW_NAMES = frozenset({"AUDIT_LOGS_ALL", "DAG_BUNDLES",
"IMPORT_ERRORS_ALL", "REPARSE_ALL"})
Review Comment:
If someone upgrades core but stays on Keycloak 0.11.2, `DAG_BUNDLES` isn't
in this set there, so in multi-team mode it falls through to `View`, which
every team role can read. Team Viewers would see the bundle pages, not just
SuperAdmin. I think the Keycloak changelog needs a note that multi-team
deployments have to upgrade the provider with core 3.4.0, and
`docs/auth-manager/manage/permissions.rst` should add Dag bundles to its
`AdminView` list.
##########
airflow-core/src/airflow/api_fastapi/core_api/routes/ui/dag_bundles.py:
##########
@@ -0,0 +1,44 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+from __future__ import annotations
+
+from fastapi import Depends
+from sqlalchemy import func, select
+
+from airflow.api_fastapi.common.db.common import SessionDep
+from airflow.api_fastapi.common.router import AirflowRouter
+from airflow.api_fastapi.core_api.datamodels.ui.dag_bundles import
DagBundleCountResponse
+from airflow.api_fastapi.core_api.security import ReadableDagsFilterDep,
requires_access_dag
+from airflow.models import DagModel
+
+dag_bundles_router = AirflowRouter(tags=["Dag Bundle"], prefix="/dagBundles")
+
+
+@dag_bundles_router.get("/count",
dependencies=[Depends(requires_access_dag(method="GET"))])
+def get_dag_bundle_count(
+ readable_dags_filter: ReadableDagsFilterDep,
+ session: SessionDep,
+) -> DagBundleCountResponse:
+ """
+ Count the Dag bundles holding a Dag the caller may read.
+
+ Readable by anyone who can read Dags, unlike ``GET /dagBundles``, so the
UI can tell whether a
+ bundle name is worth showing next to an import error. A bundle holding
only stale Dags is
+ counted too: that is a bundle whose every file fails to import, where the
name matters most.
+ """
+ count =
session.scalar(readable_dags_filter.to_orm(select(func.count(DagModel.bundle_name.distinct()))))
Review Comment:
This only counts bundles that have a `DagModel` row, so a bundle whose only
file fails before defining a Dag (the case the description leads with) never
shows up here. `GET /dagBundles` on main also counted those dagless bundles for
callers holding `IMPORT_ERRORS_ALL` on the bundle's team. With a healthy bundle
A and a dagless, broken bundle B, an Admin now gets 1, so the Dashboard
import-errors modal shows B's error without its bundle name.
Could this also count bundles with an import error the caller can read,
using the same per-team `IMPORT_ERRORS_ALL` check `get_import_errors` uses? Or
just always show the bundle name. A test with a bundle that has an import error
and no Dag row would pin it.
##########
airflow-core/src/airflow/ui/src/pages/DagBundle/FileImportError.tsx:
##########
@@ -40,25 +47,55 @@ type Props = {
* error. A dialog rather than a page of its own, because a single stack trace
is not worth an
* addressable route.
*/
-export const FileImportError = ({ bundleName, onClose, relativeFileloc }:
Props) => {
+export const FileImportError = ({
+ bundleName,
+ hasImportError,
+ onClose,
+ recheckImportError,
+ relativeFileloc,
+}: Props) => {
const { t: translate } = useTranslation("browse");
+ const [recheckedFileloc, setRecheckedFileloc] = useState<string |
undefined>(undefined);
- const { data, isLoading } = useImportErrorServiceGetImportErrors(
+ const { data, error, isLoading } = useImportErrorServiceGetImportErrors(
{ bundleName, filename: relativeFileloc ?? "" },
undefined,
{ enabled: relativeFileloc !== undefined },
);
const [importError] = data?.import_errors ?? [];
+ // ``GET /importErrors`` leaves out errors the caller may not read, so an
empty result is told
+ // apart by the file list, whose count is not filtered by caller. The badge
was clicked on a
+ // count that may predate a fix, so the list is refetched once before the
count is trusted.
+ const needsRecheck =
+ relativeFileloc !== undefined &&
+ data !== undefined &&
+ importError === undefined &&
+ hasImportError &&
+ recheckedFileloc !== relativeFileloc;
+
+ useEffect(() => {
+ if (needsRecheck) {
+ void recheckImportError().finally(() => {
+ setRecheckedFileloc(relativeFileloc);
+ });
+ }
+ }, [needsRecheck, recheckImportError, relativeFileloc]);
+
+ const close = () => {
+ setRecheckedFileloc(undefined);
+ onClose();
+ };
+
if (importError !== undefined) {
return (
- <DagImportErrorModal importError={importError} onClose={onClose}
open={relativeFileloc !== undefined} />
+ <DagImportErrorModal importError={importError} onClose={close}
open={relativeFileloc !== undefined} />
);
}
- // The table polls, so the error can be gone by the time its badge is
clicked. Say so rather
- // than leaving the click with no visible effect.
+ const isForbidden = getErrorStatus(error) === 403 || (data !== undefined &&
hasImportError);
Review Comment:
The recheck only refetches the file list (`recheckImportError={refetch}` in
`BundleFiles`), not this import-error query, which stays fresh for 5 minutes
under the default `staleTime`. If a user opens a file after its error was fixed
(caching an empty result), and the error then comes back, reopening within 5
minutes gives an empty cached result plus a positive count, so an Admin is told
they lack permission. Refetching both in the recheck, or `staleTime: 0` on this
query, would avoid it.
##########
scripts/tests/ci/prek/test_fab_permissions_doc.py:
##########
@@ -0,0 +1,44 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+from __future__ import annotations
+
+import sys
+import textwrap
+from pathlib import Path
+from unittest import mock
+
+REPO_ROOT = Path(__file__).resolve().parents[4]
+sys.path.insert(0, str(REPO_ROOT / "scripts" / "ci" / "prek"))
Review Comment:
`scripts/pyproject.toml` already puts `ci/prek` on `pythonpath`, so `import
fab_permissions_doc` works without the `sys.path.insert` and the `noqa: E402`.
##########
airflow-core/docs/administration-and-deployment/dag-bundles.rst:
##########
@@ -263,17 +260,15 @@ Some caveats:
Dags are parsed.
* The version and the import-error count together cannot confirm that a
particular commit parsed,
so do not treat the pair as a deployment-health check. Import errors are not
recorded against a
- version, a file whose parse exceeds
+ version, and a file whose parse exceeds
:ref:`config:dag_processor__dag_file_processor_timeout` is killed without
writing an import-error
- row at all, and the count only covers errors you are allowed to read. A
count of zero therefore
- means "nothing I can show you", not "this commit is healthy". To tie a
running Dag to a commit,
- read the Dag's version in ``/api/v2/dags/{dag_id}/dagVersions``.
+ row at all. A count of zero therefore does not mean "this commit is
healthy". To tie a running Dag
+ to a commit, read the Dag's version in ``/api/v2/dags/{dag_id}/dagVersions``.
* With several Dag processors the row reflects whichever refreshed last, as
they share one row per
bundle.
-* A bundle is listed for users who can read at least one Dag recorded against
it. A bundle holding
- no registered Dag at all -- a first deploy whose only file fails to import,
for instance -- has no
- Dag to authorize against, so it is listed only for users who may read import
errors for files with
- no registered Dag (``Admin`` by default under the FAB auth manager).
+* Bundles have their own permission rather than following Dag access, so a
user who holds it sees
+ every file and count in a bundle, whatever Dags they can read. Which users
hold it by default is
+ up to the auth manager.
Review Comment:
Could this list the defaults rather than leaving it to the reader? Simple
gives op and admin, FAB gives Op and Admin once the provider has the resource,
and Keycloak in multi-team mode gives SuperAdmin only (with the upgraded
provider). The FAB note above covers one of the three.
##########
providers/keycloak/tests/unit/keycloak/auth_manager/test_keycloak_auth_manager.py:
##########
@@ -1013,6 +1013,11 @@ def test_is_authorized_view(
("AUDIT_LOGS_ALL", "False", "View#GET"),
("IMPORT_ERRORS", "True", "View#GET"),
("PLUGINS", "True", "View#GET"),
+ *(
+ [("DAG_BUNDLES", "True", "AdminView#GET"), ("DAG_BUNDLES",
"False", "View#GET")]
+ if hasattr(AccessView, "DAG_BUNDLES")
Review Comment:
The test body already skips when `getattr(AccessView, view_name, None)` is
`None`, so these two cases can be listed plainly like their siblings.
##########
airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_dag_bundles.py:
##########
@@ -821,64 +561,45 @@ def test_should_raise_401_unauthenticated(self,
unauthenticated_test_client):
def test_should_raise_403_unauthorized(self, unauthorized_test_client):
assert
unauthorized_test_client.get(f"/dagBundles/{GIT_BUNDLE}/files").status_code ==
403
- def test_lists_files_ordered_by_path(self, admin_client):
- response = admin_client.get(f"/dagBundles/{GIT_BUNDLE}/files")
+ def test_should_raise_403_without_the_bundle_view(self,
no_bundle_view_client):
+ assert
no_bundle_view_client.get(f"/dagBundles/{GIT_BUNDLE}/files").status_code == 403
+
+ def test_lists_files_ordered_by_path(self, test_client):
+ response = test_client.get(f"/dagBundles/{GIT_BUNDLE}/files")
assert response.status_code == 200
body = response.json()
- assert [file["relative_fileloc"] for file in body["dag_bundle_files"]]
== [
- UNREGISTERED_FILE,
- REGISTERED_FILE,
- ]
- assert body["total_entries"] == 2
+ assert [file["relative_fileloc"] for file in body["dag_bundle_files"]]
== GIT_BUNDLE_FILES
+ assert body["total_entries"] == len(GIT_BUNDLE_FILES)
- def test_reports_parse_time_and_duration(self, dag_scoped_client):
- body = dag_scoped_client.get(f"/dagBundles/{GIT_BUNDLE}/files").json()
+ def test_lists_every_file_to_a_caller_who_may_read_no_dag(self,
no_dag_read_client):
+ """The list is the bundle's, so it is not narrowed to the files of
readable Dags."""
+ body = no_dag_read_client.get(f"/dagBundles/{GIT_BUNDLE}/files").json()
by_path = {file["relative_fileloc"]: file for file in
body["dag_bundle_files"]}
- assert by_path[REGISTERED_FILE]["dag_count"] == 1
- assert by_path[REGISTERED_FILE]["last_parsed_time"] ==
"2026-09-10T12:01:00Z"
- assert by_path[REGISTERED_FILE]["last_parse_duration"] ==
PARSE_DURATION
+ assert list(by_path) == GIT_BUNDLE_FILES
+ assert by_path[UNREADABLE_FILE]["dag_count"] == 1
Review Comment:
`UNREADABLE_FILE`, `unreadable_in_git_bundle` and the "not yours" stacktrace
now describe a Dag no test restricts, so `by_path[UNREADABLE_FILE]["dag_count"]
== 1` reads like a leak assertion. Worth renaming them to something neutral?
##########
airflow-core/src/airflow/api_fastapi/core_api/security.py:
##########
@@ -1284,6 +1270,26 @@ def inner(
return inner
+def requires_access_dag_bundles() -> Callable[[BaseUser, BaseAuthManager],
None]:
+ """
+ Require ``AccessView.DAG_BUNDLES``, without a team.
+
+ In a multi-team deployment this only admits the caller to the route, and
+ ``ReadableDagBundlesFilterDep`` then narrows the bundles to the caller's
teams. Goes through
+ ``authorize_view`` rather than ``requires_access_view`` so that an auth
manager that cannot map
+ the view (raises ``KeyError``) denies it instead of failing the request.
+ """
+
+ def inner(user: GetUserDep, auth_manager: AuthManagerDep) -> None:
+ _requires_access(
+ is_authorized_callback=lambda: auth_manager.authorize_view(
+ access_view=AccessView.DAG_BUNDLES, user=user
Review Comment:
In multi-team mode, should this admit anyone who holds the view for at least
one of their teams? The filter's per-team check can only narrow what this
unscoped check lets in. An auth manager that grants `DAG_BUNDLES` for team_a
but not globally gets a 403 here, while the `IMPORT_ERRORS_ALL` path in
`get_import_errors` authorizes per team with no global check. Nothing in-tree
grants it that way today, but a custom auth manager with per-team policies
would.
##########
airflow-core/src/airflow/api_fastapi/core_api/datamodels/dag_bundles.py:
##########
@@ -44,17 +44,13 @@ class DagBundleResponse(BaseModel):
)
)
bundle_url: str | None = Field(
- description=(
- "A link to view the bundle at ``version``, when one is configured
and the caller may "
- "read Dag versions."
- )
+ description=("A link to view the bundle at ``version``, when the
bundle is configured with one.")
Review Comment:
`render_url` also returns `None` when the template uses `{version}` and no
Dag processor has refreshed the bundle yet, and when the template is malformed
(it logs a warning and degrades to no link). Maybe "Null when the bundle has no
URL template, or when the template needs a version and the bundle hasn't been
refreshed yet."
--
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]
