-
Notifications
You must be signed in to change notification settings - Fork 14.5k
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
Don't show grid actions if server would reject with permission denied #23332
Conversation
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) |
@@ -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) }}"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dag.can_edit
should exist I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lines 607 to 626 in 505af06
def add_user_permissions_to_dag(sender, template, context, **extra): | |
""" | |
Adds `.can_edit`, `.can_trigger`, and `.can_delete` properties | |
to DAG based on current user's permissions. | |
Located in `views.py` rather than the DAG model to keep | |
permissions logic out of the Airflow core. | |
""" | |
if 'dag' in context: | |
dag = context['dag'] | |
can_create_dag_run = current_app.appbuilder.sm.has_access( | |
permissions.ACTION_CAN_CREATE, permissions.RESOURCE_DAG_RUN | |
) | |
dag.can_edit = current_app.appbuilder.sm.can_edit_dag(dag.dag_id) | |
dag.can_trigger = dag.can_edit and can_create_dag_run | |
dag.can_delete = current_app.appbuilder.sm.can_delete_dag(dag.dag_id) | |
context['dag'] = dag | |
before_render_template.connect(add_user_permissions_to_dag) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, that's not the right thing (but still, use dag.can_edit
instead.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I can't see anything currently that makes that decision.
Will this PR now gracefully handle getting a (non-JSON?) permission denied error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
@expose('/clear', methods=['POST'])
@auth.has_access(
[
(permissions.ACTION_CAN_EDIT, permissions.RESOURCE_DAG),
(permissions.ACTION_CAN_DELETE, permissions.RESOURCE_TASK_INSTANCE),
]
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
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. |
…apache#23332) * Add edit permission check for grid actions * Remove if wrapper for meta tag * Use dag.can_edit (cherry picked from commit 67e8bdd)
The Grid view wasn't checking DAG edit permissions. Also, the permission error body response is HTML, not json.
body
inConfirmDialog
is an array to prevent the grid view from crashingcanEdit
check to disabled all grid view action buttons, and don't show mapped instance selection at allBefore:
After:
^ 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 a newsfragement file, named
{pr_number}.significant.rst
, in newsfragments.