rjgoyln commented on PR #71113:
URL: https://github.com/apache/airflow/pull/71113#issuecomment-5282718085
> This is a clean fix and it meets all three acceptance criteria from #71080
— the path id is the only authorization subject on those five routes, unknown
and unreadable are indistinguishable, and list/create/dry-run keep authorizing
off the request. Nice touches: all five routes already declared `404` in
`create_openapi_http_exception_doc`, so nothing needs regenerating, and the
lookup narrowed from a full `Backfill` row to `select(Backfill.dag_id)`.
>
> Approving. One thing worth a follow-up commit here, and two nits you can
take or leave.
>
> ### The 404 detail string changed on four routes too — worth a
newsfragment line
> See the inline comment on `security.py`. The newsfragment covers 403 → 404
for unreadable Dags, but for an _authorized_ caller the message for an unknown
id also changed, because the dependency now answers before the handler runs:
>
> Route Before After
> `GET /backfills/{id}` `Backfill not found` unchanged
> `GET /backfills/{id}/dag_runs` `Backfill with id 999 not found`
`Backfill not found`
> `PUT .../pause`, `.../unpause`, `.../cancel` `Could not find
backfill with id 999` `Backfill not found`
> Nothing caught it because `test_list_backfill_dag_runs_not_found` asserts
only the status code and the three `PUT` routes have no unknown-id detail
assertion.
>
> ### Smaller observations
> * See the inline comment on `security.py` about the second
`is_authorized_dag` call.
> * See the inline comment on the new `dag_reader_test_client` fixture.
>
> > _This review was drafted by an AI-assisted tool and
> > confirmed by an Airflow maintainer. The maintainer
> > approving this PR has read the findings and signed off. If
> > something feels off, please reply on the PR and a maintainer
> > will follow up._
> > _More on how Airflow handles maintainer review:_
> >
[contributing-docs/05_pull_requests.rst](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst).
>
> Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
Thanks — the detail string was the part I missed, and it is a real break for
anyone matching on it.
Follow-up pushed: all five routes now raise `security.BACKFILL_NOT_FOUND`
(the constant lost its underscore so the routes can share it), so the one
condition has one message whether the dependency or the handler answers, and
the newsfragment records the change with the old strings. I also added the
missing unknown-id detail assertions on `/dag_runs`, `unpause` and `cancel`.
On the second `is_authorized_dag` call: I left it as two calls because the
decision is three-way rather than a single gate — allow, `403` when the caller
may read the Dag but not write it, `404` otherwise. `batch_is_authorized_dag`
collapses the set to one boolean, so a `False` still would not say which of the
two failed, and asking for `method` and `GET` together would turn a
can-edit-without-can-read role from allowed into refused, which the allow-first
ordering deliberately permits. Happy to revisit if you would rather the pair
were required.
On the fixture: agreed, and I have kept it local here. A `role_test_client`
factory in `conftest.py` folding all three is a tidy separate change — I can
open it if that is useful.
--
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]
