Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow Viewing DagRuns and TIs if a user has DAG "read" perms #20663

Merged
merged 2 commits into from Jan 5, 2022

Conversation

kaxil
Copy link
Member

@kaxil kaxil commented Jan 5, 2022

This was updated in Airflow 2.2.0 via #16634 which restricts a user to even views the DagRuns and TI records if they don't have "edit" permissions on DAG even though it has "read" permissions.

The Behaviour seems inconsistent as a User can still view those from the Graph and Tree View of the DAG.

And since we have got @action_has_dag_edit_access on all the Actions like Delete/Clear etc the approach in this PR is better as when a user will try to perform any actions from the List Dag Run view like deleting the record it will give a Access Denied error.

cc @Jorricks

image


^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code change, 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 UPDATING.md.

@kaxil kaxil requested a review from ashb January 5, 2022 04:35
@boring-cyborg boring-cyborg bot added the area:webserver Webserver related Issues label Jan 5, 2022
@kaxil
Copy link
Member Author

kaxil commented Jan 5, 2022

@Jorricks Can you take a look at it please? This PR still respects the table you had in the PR description for #16634 but allow viewing records where users have read-only perms

@kaxil kaxil added this to the Airflow 2.2.4 milestone Jan 5, 2022
@github-actions github-actions bot added the okay to merge It's ok to merge this PR as it does not require more tests label Jan 5, 2022
@github-actions
Copy link

github-actions bot commented Jan 5, 2022

The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest main or amend the last commit of the PR, and push it with --force-with-lease.

@kaxil kaxil marked this pull request as draft January 5, 2022 05:51
@kaxil kaxil marked this pull request as ready for review January 5, 2022 08:06
@kaxil kaxil closed this Jan 5, 2022
@kaxil kaxil reopened this Jan 5, 2022
@Jorricks
Copy link
Contributor

Jorricks commented Jan 5, 2022

Hey @kaxil,
Very nice addition. I agree that this improves the user experience.
Code looks good to me.
Thanks for the heads up!

@kaxil kaxil closed this Jan 5, 2022
@kaxil kaxil reopened this Jan 5, 2022
@kaxil kaxil closed this Jan 5, 2022
@kaxil kaxil reopened this Jan 5, 2022
This was updated in Airflow 2.2.0 via apache#16634 which restricts a user to even views the DagRuns and TI records if they don't have "edit" permissions on DAG even though it has "read" permissions.

The Behaviour seems inconsistent as a User can still view those from the Graph and Tree View of the DAG.

And since we have got `@action_has_dag_edit_access` on all the Actions like Delete/Clear etc the approach in this PR is better as when a user will try to perform any actions from the List Dag Run view like deleting the record it will give a Access Denied error.
@kaxil kaxil merged commit 05b9f3d into apache:main Jan 5, 2022
@kaxil kaxil deleted the dag-read-perms branch January 5, 2022 19:56
@jedcunningham jedcunningham added the type:bug-fix Changelog: Bug Fixes label Jan 26, 2022
jedcunningham pushed a commit that referenced this pull request Jan 26, 2022
This was updated in Airflow 2.2.0 via #16634 which restricts a user to even views the DagRuns and TI records if they don't have "edit" permissions on DAG even though it has "read" permissions.

The Behaviour seems inconsistent as a User can still view those from the Graph and Tree View of the DAG.

And since we have got `@action_has_dag_edit_access` on all the Actions like Delete/Clear etc the approach in this PR is better as when a user will try to perform any actions from the List Dag Run view like deleting the record it will give an Access Denied error.

(cherry picked from commit 05b9f3d)
jedcunningham pushed a commit that referenced this pull request Jan 27, 2022
This was updated in Airflow 2.2.0 via #16634 which restricts a user to even views the DagRuns and TI records if they don't have "edit" permissions on DAG even though it has "read" permissions.

The Behaviour seems inconsistent as a User can still view those from the Graph and Tree View of the DAG.

And since we have got `@action_has_dag_edit_access` on all the Actions like Delete/Clear etc the approach in this PR is better as when a user will try to perform any actions from the List Dag Run view like deleting the record it will give an Access Denied error.

(cherry picked from commit 05b9f3d)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:webserver Webserver related Issues okay to merge It's ok to merge this PR as it does not require more tests type:bug-fix Changelog: Bug Fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants