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 2022/04/28 15:22:34 UTC
[GitHub] [airflow] bbovenzi opened a new pull request, #23332: Add edit permission check for grid actions
bbovenzi opened a new pull request, #23332:
URL: https://github.com/apache/airflow/pull/23332
The Grid view wasn't checking DAG edit permissions. Also, the permission error body response is HTML, not json.
- Check if `body` in `ConfirmDIalog` is an array to prevent the grid view from crashing
- Add `canEdit` check to disabled all grid view action buttons, and don't show mapped instance selection at all
<img width="996" alt="Screen Shot 2022-04-28 at 11 19 20 AM" src="https://user-images.githubusercontent.com/4600967/165787286-b25c1c2a-35d8-4c80-950d-789d8d966910.png">
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in a newsfragement file, named `{pr_number}.significant.rst`, in [newsfragments](https://github.com/apache/airflow/tree/main/newsfragments).
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a diff in pull request #23332: Don't show grid actions if server would reject with permission denied
Posted by GitBox <gi...@apache.org>.
ashb commented on code in PR #23332:
URL: https://github.com/apache/airflow/pull/23332#discussion_r861063974
##########
airflow/www/templates/airflow/dag.html:
##########
@@ -82,6 +82,9 @@
{% endif %}
{% if external_log_name is defined %}
<meta name="external_log_name" content="{{ external_log_name }}">
+ {% if appbuilder.sm.can_edit_dag(dag.dag_id) %}
+ <meta name="can_edit" content="{{ appbuilder.sm.can_edit_dag(dag.dag_id) }}">
Review Comment:
No, I can't see anything currently that makes that decision.
Will this PR now gracefully handle getting a (non-JSON?) permission denied error.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] bbovenzi commented on a diff in pull request #23332: Don't show grid actions if server would reject with permission denied
Posted by GitBox <gi...@apache.org>.
bbovenzi commented on code in PR #23332:
URL: https://github.com/apache/airflow/pull/23332#discussion_r861065004
##########
airflow/www/templates/airflow/dag.html:
##########
@@ -82,6 +82,9 @@
{% endif %}
{% if external_log_name is defined %}
<meta name="external_log_name" content="{{ external_log_name }}">
+ {% if appbuilder.sm.can_edit_dag(dag.dag_id) %}
+ <meta name="can_edit" content="{{ appbuilder.sm.can_edit_dag(dag.dag_id) }}">
Review Comment:
Should I use that instead of the current meta `content`?
Also, what I meant is that a lot of these endpoints are checking multiple permissions, not just dag.can_edit.
(ie: [/clear](https://github.com/apache/airflow/blob/main/airflow/www/views.py#L2014)
```
@expose('/clear', methods=['POST'])
@auth.has_access(
[
(permissions.ACTION_CAN_EDIT, permissions.RESOURCE_DAG),
(permissions.ACTION_CAN_DELETE, permissions.RESOURCE_TASK_INSTANCE),
]
)
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on pull request #23332: Add edit permission check for grid actions
Posted by GitBox <gi...@apache.org>.
ashb commented on PR #23332:
URL: https://github.com/apache/airflow/pull/23332#issuecomment-1112347167
What's the current behaviour? Permissions are still respected on the API/server-side right?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a diff in pull request #23332: Don't show grid actions if server would reject with permission denied
Posted by GitBox <gi...@apache.org>.
ashb commented on code in PR #23332:
URL: https://github.com/apache/airflow/pull/23332#discussion_r861060423
##########
airflow/www/templates/airflow/dag.html:
##########
@@ -82,6 +82,9 @@
{% endif %}
{% if external_log_name is defined %}
<meta name="external_log_name" content="{{ external_log_name }}">
+ {% if appbuilder.sm.can_edit_dag(dag.dag_id) %}
+ <meta name="can_edit" content="{{ appbuilder.sm.can_edit_dag(dag.dag_id) }}">
Review Comment:
https://github.com/apache/airflow/blob/505af06303d8160c71f6a7abe4792746f640083d/airflow/www/views.py#L607-L626
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] bbovenzi commented on a diff in pull request #23332: Don't show grid actions if server would reject with permission denied
Posted by GitBox <gi...@apache.org>.
bbovenzi commented on code in PR #23332:
URL: https://github.com/apache/airflow/pull/23332#discussion_r861069996
##########
airflow/www/templates/airflow/dag.html:
##########
@@ -82,6 +82,9 @@
{% endif %}
{% if external_log_name is defined %}
<meta name="external_log_name" content="{{ external_log_name }}">
+ {% if appbuilder.sm.can_edit_dag(dag.dag_id) %}
+ <meta name="can_edit" content="{{ appbuilder.sm.can_edit_dag(dag.dag_id) }}">
Review Comment:
_More gracfully_, yes. The error response is still an html redirect. But the UI won't crash.
I feel like fixing the error response from the webserver should be in another PR?
##########
airflow/www/templates/airflow/dag.html:
##########
@@ -82,6 +82,9 @@
{% endif %}
{% if external_log_name is defined %}
<meta name="external_log_name" content="{{ external_log_name }}">
+ {% if appbuilder.sm.can_edit_dag(dag.dag_id) %}
+ <meta name="can_edit" content="{{ appbuilder.sm.can_edit_dag(dag.dag_id) }}">
Review Comment:
_More gracefully_, yes. The error response is still an html redirect. But the UI won't crash.
I feel like fixing the error response from the webserver should be in another PR?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] bbovenzi commented on a diff in pull request #23332: Add edit permission check for grid actions
Posted by GitBox <gi...@apache.org>.
bbovenzi commented on code in PR #23332:
URL: https://github.com/apache/airflow/pull/23332#discussion_r861047045
##########
airflow/www/templates/airflow/dag.html:
##########
@@ -82,6 +82,9 @@
{% endif %}
{% if external_log_name is defined %}
<meta name="external_log_name" content="{{ external_log_name }}">
+ {% if appbuilder.sm.can_edit_dag(dag.dag_id) %}
+ <meta name="can_edit" content="{{ appbuilder.sm.can_edit_dag(dag.dag_id) }}">
Review Comment:
This is only checking edit permissions on the DAG. Is there an easy way to check Task Instance and DAG Run edit perrmissions via FAB?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a diff in pull request #23332: Don't show grid actions if server would reject with permission denied
Posted by GitBox <gi...@apache.org>.
ashb commented on code in PR #23332:
URL: https://github.com/apache/airflow/pull/23332#discussion_r861060931
##########
airflow/www/templates/airflow/dag.html:
##########
@@ -82,6 +82,9 @@
{% endif %}
{% if external_log_name is defined %}
<meta name="external_log_name" content="{{ external_log_name }}">
+ {% if appbuilder.sm.can_edit_dag(dag.dag_id) %}
+ <meta name="can_edit" content="{{ appbuilder.sm.can_edit_dag(dag.dag_id) }}">
Review Comment:
Oh, that's not the right thing (but still, use `dag.can_edit` instead.)
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a diff in pull request #23332: Don't show grid actions if server would reject with permission denied
Posted by GitBox <gi...@apache.org>.
ashb commented on code in PR #23332:
URL: https://github.com/apache/airflow/pull/23332#discussion_r861060037
##########
airflow/www/templates/airflow/dag.html:
##########
@@ -82,6 +82,9 @@
{% endif %}
{% if external_log_name is defined %}
<meta name="external_log_name" content="{{ external_log_name }}">
+ {% if appbuilder.sm.can_edit_dag(dag.dag_id) %}
+ <meta name="can_edit" content="{{ appbuilder.sm.can_edit_dag(dag.dag_id) }}">
Review Comment:
`dag.can_edit` should exist I think.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on a diff in pull request #23332: Don't show grid actions if server would reject with permission denied
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on code in PR #23332:
URL: https://github.com/apache/airflow/pull/23332#discussion_r861061775
##########
airflow/www/templates/airflow/dag.html:
##########
@@ -82,6 +82,9 @@
{% endif %}
{% if external_log_name is defined %}
<meta name="external_log_name" content="{{ external_log_name }}">
+ {% if appbuilder.sm.can_edit_dag(dag.dag_id) %}
Review Comment:
Oops, old diff 🍺
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] bbovenzi merged pull request #23332: Don't show grid actions if server would reject with permission denied
Posted by GitBox <gi...@apache.org>.
bbovenzi merged PR #23332:
URL: https://github.com/apache/airflow/pull/23332
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #23332: Don't show grid actions if server would reject with permission denied
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #23332:
URL: https://github.com/apache/airflow/pull/23332#issuecomment-1112375368
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.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on a diff in pull request #23332: Don't show grid actions if server would reject with permission denied
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on code in PR #23332:
URL: https://github.com/apache/airflow/pull/23332#discussion_r861060713
##########
airflow/www/templates/airflow/dag.html:
##########
@@ -82,6 +82,9 @@
{% endif %}
{% if external_log_name is defined %}
<meta name="external_log_name" content="{{ external_log_name }}">
+ {% if appbuilder.sm.can_edit_dag(dag.dag_id) %}
Review Comment:
Should we always add this to meta?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] bbovenzi commented on a diff in pull request #23332: Don't show grid actions if server would reject with permission denied
Posted by GitBox <gi...@apache.org>.
bbovenzi commented on code in PR #23332:
URL: https://github.com/apache/airflow/pull/23332#discussion_r861065004
##########
airflow/www/templates/airflow/dag.html:
##########
@@ -82,6 +82,9 @@
{% endif %}
{% if external_log_name is defined %}
<meta name="external_log_name" content="{{ external_log_name }}">
+ {% if appbuilder.sm.can_edit_dag(dag.dag_id) %}
+ <meta name="can_edit" content="{{ appbuilder.sm.can_edit_dag(dag.dag_id) }}">
Review Comment:
Should I use that instead of the current meta `content`?
Also, what I meant is that a lot of these endpoints are checking multiple permissions, not just dag.can_edit.
(ie: [/clear](https://github.com/apache/airflow/blob/main/airflow/www/views.py#L2014))
```
@expose('/clear', methods=['POST'])
@auth.has_access(
[
(permissions.ACTION_CAN_EDIT, permissions.RESOURCE_DAG),
(permissions.ACTION_CAN_DELETE, permissions.RESOURCE_TASK_INSTANCE),
]
)
##########
airflow/www/templates/airflow/dag.html:
##########
@@ -82,6 +82,9 @@
{% endif %}
{% if external_log_name is defined %}
<meta name="external_log_name" content="{{ external_log_name }}">
+ {% if appbuilder.sm.can_edit_dag(dag.dag_id) %}
+ <meta name="can_edit" content="{{ appbuilder.sm.can_edit_dag(dag.dag_id) }}">
Review Comment:
Should I use that instead of the current meta `content`?
Also, what I meant is that a lot of these endpoints are checking multiple permissions, not just dag.can_edit. (ie: [/clear](https://github.com/apache/airflow/blob/main/airflow/www/views.py#L2014))
```
@expose('/clear', methods=['POST'])
@auth.has_access(
[
(permissions.ACTION_CAN_EDIT, permissions.RESOURCE_DAG),
(permissions.ACTION_CAN_DELETE, permissions.RESOURCE_TASK_INSTANCE),
]
)
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] bbovenzi commented on pull request #23332: Add edit permission check for grid actions
Posted by GitBox <gi...@apache.org>.
bbovenzi commented on PR #23332:
URL: https://github.com/apache/airflow/pull/23332#issuecomment-1112353170
> What's the current behaviour? Permissions are still respected on the API/server-side right?
Yes, the server still doesn't perform any actions. But the UI was also not handling the non-JSON error correctly and the grid view went blank. (I added a gif to the PR description)
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org