henry3260 commented on code in PR #73007:
URL: https://github.com/apache/airflow/pull/73007#discussion_r4006591958
##########
airflow-core/src/airflow/api_fastapi/core_api/routes/public/assets.py:
##########
@@ -407,6 +411,18 @@ def create_asset_event(
asset_model = session.scalar(select(AssetModel).where(AssetModel.id ==
body.asset_id).limit(1))
if not asset_model:
raise HTTPException(status.HTTP_404_NOT_FOUND, f"Asset with ID:
`{body.asset_id}` was not found")
+ # The asset is named in the body, which the route dependency cannot read,
so it only checked the
+ # generic asset POST permission. Authorize on the resolved asset here so
an auth manager can scope
+ # by id, name, or uri.
+ if not get_auth_manager().is_authorized_asset(
Review Comment:
> It is safe to remove `Depends(requires_access_asset(method="POST"))` then.
One covers the other
Good point, the per-asset check does cover it. However, the permission
reference docs `(api_permissions_ref.rst and the FAB access-control.rst) `are
generated from the `requires_access_*` route dependencies, so removing it would
`list POST /assets/events `as Public / No Airflow permission required, even
though the route still enforces the asset POST permission.
/connections/enqueue-test already shows up that way for the same reason.
Would you be OK with keeping the dependency in this PR and handling it in a
follow-up? The follow-up would remove it and teach extract_permissions.py to
recognize in-route authorization checks, so the docs stay accurate.
--
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]
