Require can_edit on DAG privileges to modify TaskInstances and DagRuns #16634

Jorricks · 2021-06-24T15:13:26Z

All permissions for modifying Task Instances or modifying Dag Runs as of today require dag_read permissions on the DAG and the corresponding action permission.
A full overview is shown at the Access Control page of Airflow

It feels to me as in that case the whole dag_edit base_permission is undervalued in this case and the dag_view base_permission gives too much actual permissions.

Imagine the following setup:

Everyone is able to see each others DAGs
Some people should be able to modify their own DAGs
They should not be able to modify their neighbours DAGs

This setup is currently not supported.
As a work around, on my work setup I currently implemented a SQLAlchemy listener to block update operations on TaskInstances where a user doesn't have can_edit privilege on this specific DAG.
Therefore this PR changes the following items(copied from the link above) to require DAGS.can_edit where it currently says DAGS.can_read privileges.

Currently:

Action	Permissions	Minimum Role
Clear DAG	DAGs.can_read, Task Instances.can_delete	User
Clear DAG Run	DAGs.can_read, Task Instances.can_delete	User
Mark DAG as blocked	Dags.can_read, DAG Runs.can_read	User
Mark DAG Run as failed	Dags.can_read, DAG Runs.can_edit	User
Mark DAG Run as success	Dags.can_read, DAG Runs.can_edit	User
Clear Task Instance	DAGs.can_read, DAG Runs.can_read, Task Instances.can_edit	User
Triggers Task Instance	DAGs.can_read, Task Instances.can_create	User
Mark Task as failed	DAGs.can_read, Task Instances.can_edit	User
Mark Task as success	DAGs.can_read, Task Instances.can_edit	User

Updated:

Action	Permissions	Minimum Role
Clear DAG	DAGs.can_edit, Task Instances.can_delete	User
Clear DAG Run	DAGs.can_edit, Task Instances.can_delete	User
Mark DAG as blocked	Dags.can_edit, DAG Runs.can_read	User
Mark DAG Run as failed	Dags.can_edit, DAG Runs.can_edit	User
Mark DAG Run as success	Dags.can_edit, DAG Runs.can_edit	User
Clear Task Instance	DAGs.can_edit, Task Instances.can_edit	User
Triggers Task Instance	DAGs.can_edit, Task Instances.can_create	User
Mark Task as failed	DAGs.can_edit, Task Instances.can_edit	User
Mark Task as success	DAGs.can_edit, Task Instances.can_edit	User

If there is interest in merging this PR, I will also make a corresponding PR on the docs side to update the page.

ashb · 2021-06-24T15:48:23Z

Everyone is able to see each others DAGs

Some people should be able to modify their own DAGs

They should not be able to modify their neighbours DAGs

Oh yes, this should totally be supported.

airflow/www/security.py

ashb

At the very least this is going to need some tests to ensure that the behaviour you describe works, and continues to work :)

uranusjr · 2021-06-24T15:52:10Z

It feels weird to me actions that operate on DagRun and TaskInstance need edit permission on the DAG, since they don’t change things in it.

ashb · 2021-06-24T15:53:50Z

It feels weird to me actions that operate on DagRun and TaskInstance need edit permission on the DAG, since they don’t change things in it.

I agree, which is why it probably doesn't have it right now, but the only way to express "you have edit on Dag X, but not Dag Y" and have that apply to TaskInstances/DagRuns that I can think of would be a change like this

Jorricks · 2021-06-24T16:25:43Z

At the very least this is going to need some tests to ensure that the behaviour you describe works, and continues to work :)

It already has quite some tests in there.
But I will create extra tests to show that the standard can_read permissions now fails.

Jorricks · 2021-06-26T21:02:49Z

I updated the PR today (26 June) with the following:

Inside the views, we already had the DagEditFilter. However, FlaskAppbuilder doesn't do the extra checks to verify that the items you are modifying/adding/deleting are inside the BaseFilter. Therefore, I created another abstraction for the view which checks for add/update/delete options that the user has can_edit access on the DAG the item he is accessing belongs to. If there was someone who still tried to cheat by changing primary keys in the front end with an HTML editor (add/update/delete a DAG he doesn't have edit access on), he will get an error message :)
For the actions, it is the same thing. However, there is no way within Flask Appbuilder to check the items you are working with before entering the function (without using wrapper). Hence, of course I created wrapper. For all items provided/selected, it checks that the user had can_edit access on the DAG it is a part of.
I implemented tests. For both DagRun and TaskInstance I created tests for deleting. For DagRun also for adding. Furthermore I also created the tests for all actions of both views to verify the behaviour of the actions.

Jorricks · 2021-07-01T09:14:36Z

Rebased PR to latest main and squashed commits.

Jorricks · 2021-07-22T17:21:14Z

Rebased PR to latest main again.
Does anyone have any tips on how I can give this PR to have more exposure :)?

kaxil · 2021-07-22T21:45:38Z

cc @jhtimmins Can you take a look at this please

Jorricks · 2021-07-23T13:25:34Z

Given the SQLite CI/CD pipeline was successful, I think it's safe to say I fixed the tests that were failing.
The rest of the failed pipelines I can't find an error message related to my changes.

(cherry picked from commit eb2c7af)

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.

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.

apache#16634) (cherry picked from commit f74d0ab)

apache#16634) (cherry picked from commit f74d0ab) (cherry picked from commit e9f0f90)

apache#16634) (cherry picked from commit f74d0ab) (cherry picked from commit e9f0f90) (cherry picked from commit 0a4e99a)

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)

This was updated in Airflow 2.2.0 via apache/airflow#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 05b9f3db5471e49e894e4148d8d1deb4361a9b53) GitOrigin-RevId: fab8b121c6906f62e28ea6485d8d8e493cb8b769

This was updated in Airflow 2.2.0 via apache/airflow#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. GitOrigin-RevId: 05b9f3db5471e49e894e4148d8d1deb4361a9b53

twang90 · 2022-08-08T17:46:03Z

@Jorricks Thanks for putting together this PR. Just want to make sure I understand it correctly. With your PR merged, do I expect users without dag edit permission but with task instance delete permission can not delete a task instance, right? i.e. the behavior matches the documentation? Do I need any workaround like the SQLAlchemy listener you mentioned in the original post? Thanks!

Jorricks · 2022-08-09T05:36:49Z

Hi @twang90,

First of all, this has been merged a long time ago just before 2.1.0 if I remember correctly.
Furthermore, your assumption is correct and you will not need the sqlalchemy listener either because that behavior is now inside the UI already.

This was updated in Airflow 2.2.0 via apache/airflow#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. GitOrigin-RevId: 05b9f3db5471e49e894e4148d8d1deb4361a9b53

Jorricks requested review from ashb, ephraimbuddy, jhtimmins, kaxil, mik-laj and ryanahamilton as code owners June 24, 2021 15:13

boring-cyborg bot added area:API Airflow's REST/HTTP API area:webserver Webserver related Issues labels Jun 24, 2021

Jorricks marked this pull request as draft June 24, 2021 15:13

Jorricks marked this pull request as ready for review June 24, 2021 15:26

Jorricks mentioned this pull request Jun 24, 2021

DAG action views should only show DAGs a user can edit #12301

Closed

ashb reviewed Jun 24, 2021

View reviewed changes

airflow/www/security.py Outdated Show resolved Hide resolved

ashb reviewed Jun 24, 2021

View reviewed changes

Jorricks requested a review from ashb June 27, 2021 08:00

Jorricks changed the title ~~Change TaskInstances and DagRun modify permissions to require can_edit on DAG resource.~~ Require can_edit on DAG privileges to modify TaskInstances and DagRuns Jun 27, 2021

Jorricks force-pushed the optimize-dag-edit-privileges branch from f794196 to 39c8c3a Compare July 1, 2021 09:14

uranusjr mentioned this pull request Jul 2, 2021

DAG-Level Access Control - Ability to set more permissions #16772

Closed

Jorricks force-pushed the optimize-dag-edit-privileges branch from 39c8c3a to 179bf42 Compare July 22, 2021 17:19

kaxil removed their request for review July 22, 2021 21:45

potiuk closed this Jul 25, 2021

potiuk reopened this Jul 25, 2021

Jorricks added a commit to Jorricks/airflow that referenced this pull request Sep 30, 2021

Changes related to PR apache#16634

6bd3ace

Jorricks mentioned this pull request Sep 30, 2021

Document changes related to PR #16634 #18644

Merged

eladkal mentioned this pull request Oct 6, 2021

#16037 Templated requirements.txt in Python operators #17349

Merged

3 tasks

potiuk pushed a commit that referenced this pull request Oct 14, 2021

Changes related to PR #16634 (#18644)

eb2c7af

jedcunningham pushed a commit that referenced this pull request Oct 14, 2021

Changes related to PR #16634 (#18644)

f3dd5b5

(cherry picked from commit eb2c7af)

kaxil mentioned this pull request Jan 5, 2022

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

Merged

jedcunningham pushed a commit to astronomer/airflow that referenced this pull request Jan 13, 2022

Require can_edit on DAG privileges to modify TaskInstances and DagRuns (

e9f0f90

apache#16634) (cherry picked from commit f74d0ab)

jedcunningham pushed a commit to astronomer/airflow that referenced this pull request Jan 13, 2022

Require can_edit on DAG privileges to modify TaskInstances and DagRuns (

1609049

apache#16634) (cherry picked from commit f74d0ab) (cherry picked from commit e9f0f90)

jedcunningham pushed a commit to astronomer/airflow that referenced this pull request Jan 13, 2022

Require can_edit on DAG privileges to modify TaskInstances and DagRuns (

0a4e99a

apache#16634) (cherry picked from commit f74d0ab) (cherry picked from commit e9f0f90)

twang90 mentioned this pull request Aug 8, 2022

Failed to manage per-DAG permission in clearing DAG run #25560

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Require can_edit on DAG privileges to modify TaskInstances and DagRuns #16634

Require can_edit on DAG privileges to modify TaskInstances and DagRuns #16634

Jorricks commented Jun 24, 2021 •

edited

Loading

ashb commented Jun 24, 2021

ashb left a comment

uranusjr commented Jun 24, 2021

ashb commented Jun 24, 2021

Jorricks commented Jun 24, 2021

Jorricks commented Jun 26, 2021 •

edited

Loading

Jorricks commented Jul 1, 2021

Jorricks commented Jul 22, 2021

kaxil commented Jul 22, 2021 •

edited

Loading

Jorricks commented Jul 23, 2021

twang90 commented Aug 8, 2022

Jorricks commented Aug 9, 2022

Require can_edit on DAG privileges to modify TaskInstances and DagRuns #16634

Require can_edit on DAG privileges to modify TaskInstances and DagRuns #16634

Conversation

Jorricks commented Jun 24, 2021 • edited Loading

ashb commented Jun 24, 2021

ashb left a comment

Choose a reason for hiding this comment

uranusjr commented Jun 24, 2021

ashb commented Jun 24, 2021

Jorricks commented Jun 24, 2021

Jorricks commented Jun 26, 2021 • edited Loading

Jorricks commented Jul 1, 2021

Jorricks commented Jul 22, 2021

kaxil commented Jul 22, 2021 • edited Loading

Jorricks commented Jul 23, 2021

twang90 commented Aug 8, 2022

Jorricks commented Aug 9, 2022

Jorricks commented Jun 24, 2021 •

edited

Loading

Jorricks commented Jun 26, 2021 •

edited

Loading

kaxil commented Jul 22, 2021 •

edited

Loading