Clarify that DAG.access_control is only read by the FAB auth manager - #71260
Open
1fanwang wants to merge 3 commits into
Open
Clarify that DAG.access_control is only read by the FAB auth manager#712601fanwang wants to merge 3 commits into
1fanwang wants to merge 3 commits into
Conversation
The migration list mapped DAG.access_control to filter_authorized_dag_ids, which is a batching helper over is_authorized_dag rather than the place a dag-level decision is made, and which the FAB auth manager itself does not override. It also read as a relocation, when access_control has no successor. Core has only synced access_control when the auth manager is FAB since apache#48070, so under any other auth manager the argument is parsed and serialized but never consulted. A deployment migrating off FAB silently stops enforcing those grants. Signed-off-by: 1fanwang <1fannnw@gmail.com>
…ement The auth manager guide already states that access_control is FAB-only and covers how a custom manager should express dag-level access. Re-deriving the method stack here would put a third answer in a second doc, which is how the two pages drifted apart in the first place. Signed-off-by: 1fanwang <1fannnw@gmail.com>
Signed-off-by: 1fanwang <1fannnw@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
DAG.access_controlwas documented as if it moved tofilter_authorized_dag_ids. That row is wrong in two ways.First, it names the wrong method. FAB is the only auth manager whose Dag permissions come from
access_control, and it overridesget_authorized_dag_ids, notfilter_authorized_dag_ids, which has a default implementation whose docstring treats overriding it as a performance choice.Second, the row reads like an import swap. It is not.
access_controlwas an input to FAB's permission tables, not a generic authorization mechanism. Since #48070, core has only synced it when the auth manager is FAB (collection.py). With any other auth manager, the argument is still accepted and still serialized, but nothing reads it. A deployment that moves off FAB and leavesaccess_controlon its Dags silently stops enforcing those grants.The auth manager guide already says FAB is the only reader and names the right method. The migration list is where the deprecation warning sends readers, so it should say the same thing.
The warning emitted when
access_controlis set also reused the deprecation message from theairflow.security.permissionsmodule, even when callers never imported that module. It now namesDAG.access_controland warns that non-FAB auth managers ignore it and grant no access.related: #53716