Skip to content

Update is_authorized_dag method in FabAuthManager#54926

Merged
vincbeck merged 9 commits intoapache:mainfrom
aws-mwaa:vincbeck/fab_list_perms
Sep 3, 2025
Merged

Update is_authorized_dag method in FabAuthManager#54926
vincbeck merged 9 commits intoapache:mainfrom
aws-mwaa:vincbeck/fab_list_perms

Conversation

@vincbeck
Copy link
Copy Markdown
Contributor

Resolves #53936.

Follow-up of #54197.

The way FabAuthManager checks for Dag permission is partially broken in multiple ways:

  1. To check whether user is authorized to list Dags, we are calling get_authorized_dag_ids (which returns the list of Dags the user is authorized to access) and checking whether this list is not empty. Even though this is not strictly wrong, it is not good either. A more robust solution proposed by @pierrejeambrun (see thread here for more information about the solution but also the reason why it is better) is to check whether the user has either access to all Dags or at least one dag permission with RESOURCE_DAG_PREFIX
  2. I remove RESOURCE_DAG_RUN_PREFIX because I think this is very wrong. The purpose of this constant (value is DAG Run:) is to be able to give access to Dag runs of specific Dags. Example: To give read access to user X of Dag runs of Dag test, I can add to the user's permissions (CAN_READ, DAG Run:test). But this is possible without this constant, using the same example, to grant the user access to Dag test runs, you can give them (CAN_READ, Dag:test) and (CAN_READ, RESOURCE_DAG_RUN).

^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in airflow-core/newsfragments.

Comment thread providers/fab/src/airflow/providers/fab/auth_manager/fab_auth_manager.py Outdated
@vincbeck vincbeck force-pushed the vincbeck/fab_list_perms branch from 59bcf28 to e360e2c Compare August 26, 2025 14:30
Copy link
Copy Markdown
Contributor

@o-nikolas o-nikolas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not my area of expertise, but the code seems reasonable.

@vincbeck
Copy link
Copy Markdown
Contributor Author

Please do not merge before @pierrejeambrun approval, I'd like to have it before merging :)

Copy link
Copy Markdown
Member

@pierrejeambrun pierrejeambrun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me, thanks Vincent!

Just one question for my own understanding.

Comment thread airflow-core/src/airflow/security/permissions.py
@vincbeck vincbeck merged commit 43c31e2 into apache:main Sep 3, 2025
107 checks passed
@vincbeck vincbeck deleted the vincbeck/fab_list_perms branch September 3, 2025 17:14
RoyLee1224 pushed a commit to RoyLee1224/airflow that referenced this pull request Sep 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

UI - "Runs" and "Task Instances" submenus in the "Dags" Menu only work with "Can Read on Dags" permission

4 participants