You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2021/01/27 05:20:12 UTC
[GitHub] [airflow] jhtimmins opened a new pull request #13922: Fix read allows dag trigger bug
jhtimmins opened a new pull request #13922:
URL: https://github.com/apache/airflow/pull/13922
From 1.10.x -> 2.0, the required permissions to trigger a dag have changed from DAG.can_edit to DAG.can_read + DagRun.can_create. Since the Viewer role has `DAG.can_read` by default, it isn't possible to give a Viewer trigger access to a single DAG without giving access to all DAGs.
This fixes that discrepancy by making the trigger requirement DAG.can_edit + DagRun.can_create. Now, to trigger a DAG, a viewer will need to be given both permissions, as neither is with the Viewer role by default.
This PR also hides the Trigger/Refresh buttons on the home page if the user doesn't have permission to perform those actions.
closes: #13891
related: #13891
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jhtimmins commented on pull request #13922: Fix read allows dag trigger bug
Posted by GitBox <gi...@apache.org>.
jhtimmins commented on pull request #13922:
URL: https://github.com/apache/airflow/pull/13922#issuecomment-769558500
@kaxil added and updated!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jhtimmins commented on pull request #13922: Fix read allows dag trigger bug
Posted by GitBox <gi...@apache.org>.
jhtimmins commented on pull request #13922:
URL: https://github.com/apache/airflow/pull/13922#issuecomment-772161758
@kaxil mind merging if this looks good?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #13922: Fix read allows dag trigger bug
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #13922:
URL: https://github.com/apache/airflow/pull/13922#discussion_r567296006
##########
File path: airflow/migrations/versions/2c6edca13270_resource_based_permissions.py
##########
@@ -161,7 +161,7 @@
(permissions.ACTION_CAN_READ, permissions.RESOURCE_TASK_INSTANCE),
],
("Airflow", "can_trigger"): [
- (permissions.ACTION_CAN_READ, permissions.RESOURCE_DAG),
+ (permissions.ACTION_CAN_EDIT, permissions.RESOURCE_DAG),
Review comment:
Changing the migration won't affect anyone who has already upgrade to 2.0.0 - do we need a new migration to fix this up?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jhtimmins commented on a change in pull request #13922: Fix read allows dag trigger bug
Posted by GitBox <gi...@apache.org>.
jhtimmins commented on a change in pull request #13922:
URL: https://github.com/apache/airflow/pull/13922#discussion_r568226639
##########
File path: airflow/migrations/versions/2c6edca13270_resource_based_permissions.py
##########
@@ -161,7 +161,7 @@
(permissions.ACTION_CAN_READ, permissions.RESOURCE_TASK_INSTANCE),
],
("Airflow", "can_trigger"): [
- (permissions.ACTION_CAN_READ, permissions.RESOURCE_DAG),
+ (permissions.ACTION_CAN_EDIT, permissions.RESOURCE_DAG),
Review comment:
This will will help anyone who goes straight from 1.X to 2.0.1, which is probably most of the users.
We shouldn't need a migration, since triggering also requires `Dag Run.can_create`, which is only included in `>= User`. `Dag.can_edit` is also included in `>= User`, so either way, someone with default roles won't be able to trigger dags unless they have `User` or higher.
This might change the behavior of users with custom roles. If someone has `Viewer` + a custom role with `Dag Run.can_create`, or they manually added `Dag Run.can_create` to `Viewer`, then they will lose access until they add `Dag.can_edit` to the custom role. I suspect this is a small group of users.
##########
File path: airflow/migrations/versions/2c6edca13270_resource_based_permissions.py
##########
@@ -161,7 +161,7 @@
(permissions.ACTION_CAN_READ, permissions.RESOURCE_TASK_INSTANCE),
],
("Airflow", "can_trigger"): [
- (permissions.ACTION_CAN_READ, permissions.RESOURCE_DAG),
+ (permissions.ACTION_CAN_EDIT, permissions.RESOURCE_DAG),
Review comment:
@ashb This will will help anyone who goes straight from 1.X to 2.0.1, which is probably most of the users.
We shouldn't need a migration, since triggering also requires `Dag Run.can_create`, which is only included in `>= User`. `Dag.can_edit` is also included in `>= User`, so either way, someone with default roles won't be able to trigger dags unless they have `User` or higher.
This might change the behavior of users with custom roles. If someone has `Viewer` + a custom role with `Dag Run.can_create`, or they manually added `Dag Run.can_create` to `Viewer`, then they will lose access until they add `Dag.can_edit` to the custom role. I suspect this is a small group of users.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] bbovenzi commented on a change in pull request #13922: Fix read allows dag trigger bug
Posted by GitBox <gi...@apache.org>.
bbovenzi commented on a change in pull request #13922:
URL: https://github.com/apache/airflow/pull/13922#discussion_r567985340
##########
File path: airflow/www/templates/airflow/dags.html
##########
@@ -167,15 +167,15 @@ <h2>DAGs</h2>
<td class="text-center">
<div class="btn-group">
{% if dag %}
- <a href="{{ url_for('Airflow.trigger', dag_id=dag.dag_id) }}" title="Trigger DAG" aria-label="Trigger DAG" class="btn btn-sm btn-default btn-icon-only">
- <span class="material-icons" aria-hidden="true">play_arrow</span>
- </a>
- <a href="{{ url_for('Airflow.refresh', dag_id=dag.dag_id) }}" onclick="postAsForm(this.href); return false" title="Refresh DAG" aria-label="Refresh DAG" class="btn btn-sm btn-default btn-icon-only">
- <span class="material-icons" aria-hidden="true">refresh</span>
- </a>
+ <a href="{{ url_for('Airflow.trigger', dag_id=dag.dag_id) }}" title="Trigger DAG" aria-label="Trigger DAG" class="btn btn-sm btn-default btn-icon-only {{ ' disabled' if not dag.can_trigger }}">
Review comment:
It seems like there's an unnecessary indentation here.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #13922: Fix read allows dag trigger bug
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #13922:
URL: https://github.com/apache/airflow/pull/13922#discussion_r567030007
##########
File path: airflow/www/templates/airflow/dags.html
##########
@@ -167,15 +167,15 @@ <h2>DAGs</h2>
<td class="text-center">
<div class="btn-group">
{% if dag %}
- <a href="{{ url_for('Airflow.trigger', dag_id=dag.dag_id) }}" title="Trigger DAG" aria-label="Trigger DAG" class="btn btn-sm btn-default btn-icon-only">
- <span class="material-icons" aria-hidden="true">play_arrow</span>
- </a>
- <a href="{{ url_for('Airflow.refresh', dag_id=dag.dag_id) }}" onclick="postAsForm(this.href); return false" title="Refresh DAG" aria-label="Refresh DAG" class="btn btn-sm btn-default btn-icon-only">
- <span class="material-icons" aria-hidden="true">refresh</span>
- </a>
+ <a href="{{ url_for('Airflow.trigger', dag_id=dag.dag_id) }}" title="Trigger DAG" aria-label="Trigger DAG" class="btn btn-sm btn-default btn-icon-only {{ ' disabled' if not dag.can_trigger else '' }}">
+ <span class="material-icons" aria-hidden="true">play_arrow</span>
+ </a>
+ <a href="{{ url_for('Airflow.refresh', dag_id=dag.dag_id) }}" onclick="postAsForm(this.href); return false" title="Refresh DAG" aria-label="Refresh DAG" class="btn btn-sm btn-default btn-icon-only {{ ' disabled' if not dag.can_edit else '' }}">
+ <span class="material-icons" aria-hidden="true">refresh</span>
+ </a>
{% endif %}
{# Use dag_id instead of dag.dag_id, because the DAG might not exist in the webserver's DagBag #}
- <a href="{{ url_for('Airflow.delete', dag_id=dag.dag_id) }}" onclick="return confirmDeleteDag(this, '{{ dag.dag_id }}')" title="Delete DAG" aria-label="Delete DAG" class="btn btn-sm btn-default btn-icon-only">
+ <a href="{{ url_for('Airflow.delete', dag_id=dag.dag_id) }}" onclick="return confirmDeleteDag(this, '{{ dag.dag_id }}')" title="Delete DAG" aria-label="Delete DAG" class="btn btn-sm btn-default btn-icon-only {{ ' disabled' if not dag.can_delete else '' }}">
Review comment:
```suggestion
<a href="{{ url_for('Airflow.delete', dag_id=dag.dag_id) }}" onclick="return confirmDeleteDag(this, '{{ dag.dag_id }}')" title="Delete DAG" aria-label="Delete DAG" class="btn btn-sm btn-default btn-icon-only {{ ' disabled' if not dag.can_delete }}">
```
WDYT?
cc @ryanahamilton
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil merged pull request #13922: Fix read allows dag trigger bug
Posted by GitBox <gi...@apache.org>.
kaxil merged pull request #13922:
URL: https://github.com/apache/airflow/pull/13922
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jhtimmins commented on a change in pull request #13922: Fix read allows dag trigger bug
Posted by GitBox <gi...@apache.org>.
jhtimmins commented on a change in pull request #13922:
URL: https://github.com/apache/airflow/pull/13922#discussion_r568226639
##########
File path: airflow/migrations/versions/2c6edca13270_resource_based_permissions.py
##########
@@ -161,7 +161,7 @@
(permissions.ACTION_CAN_READ, permissions.RESOURCE_TASK_INSTANCE),
],
("Airflow", "can_trigger"): [
- (permissions.ACTION_CAN_READ, permissions.RESOURCE_DAG),
+ (permissions.ACTION_CAN_EDIT, permissions.RESOURCE_DAG),
Review comment:
This will will help anyone who goes straight from 1.X to 2.0.1, which is probably most of the users.
We shouldn't need a migration, since triggering also requires `Dag Run.can_create`, which is only included in `>= User`. `Dag.can_edit` is also included in `>= User`, so either way, someone with default roles won't be able to trigger dags unless they have `User` or higher.
This might change the behavior of users with custom roles. If someone has `Viewer` + a custom role with `Dag Run.can_create`, or they manually added `Dag Run.can_create` to `Viewer`, then they will lose access until they add `Dag.can_edit` to the custom role. I suspect this is a small group of users.
##########
File path: airflow/migrations/versions/2c6edca13270_resource_based_permissions.py
##########
@@ -161,7 +161,7 @@
(permissions.ACTION_CAN_READ, permissions.RESOURCE_TASK_INSTANCE),
],
("Airflow", "can_trigger"): [
- (permissions.ACTION_CAN_READ, permissions.RESOURCE_DAG),
+ (permissions.ACTION_CAN_EDIT, permissions.RESOURCE_DAG),
Review comment:
@ashb This will will help anyone who goes straight from 1.X to 2.0.1, which is probably most of the users.
We shouldn't need a migration, since triggering also requires `Dag Run.can_create`, which is only included in `>= User`. `Dag.can_edit` is also included in `>= User`, so either way, someone with default roles won't be able to trigger dags unless they have `User` or higher.
This might change the behavior of users with custom roles. If someone has `Viewer` + a custom role with `Dag Run.can_create`, or they manually added `Dag Run.can_create` to `Viewer`, then they will lose access until they add `Dag.can_edit` to the custom role. I suspect this is a small group of users.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org