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 2020/11/18 18:24:28 UTC
[GitHub] [airflow] XD-DENG opened a new pull request #12458: Improve www.security.get_accessible_dags() and webserver performance
XD-DENG opened a new pull request #12458:
URL: https://github.com/apache/airflow/pull/12458
- The performance of `get_accessible_dags()` is improved simply by returning as early as possible
- All the changes made in `www.views.py` are based on the fact that the check against `permissions.RESOURCE_DAG` is already handled inside `get_accessible_dags()`, which is invoked by `get_accessible_dag_ids()` then. Comparing `permissions.RESOURCE_DAG` with the result of `get_accessible_dag_ids()` is not making sense. The lines inside the `IF`-clauses are either always executed or never executed (I believe these chunks were not cleaned-up after the relevant code changes in `get_accessible_dag_ids()` happened)
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/master/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 [UPDATING.md](https://github.com/apache/airflow/blob/master/UPDATING.md).
----------------------------------------------------------------
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] XD-DENG commented on pull request #12458: Improve www.security.get_accessible_dags() and webserver performance
Posted by GitBox <gi...@apache.org>.
XD-DENG commented on pull request #12458:
URL: https://github.com/apache/airflow/pull/12458#issuecomment-729943521
Rebased to the latest master branch, also to re-trigger test (observing transient CI 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #12458: Improve www.security.get_accessible_dags() and webserver performance
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #12458:
URL: https://github.com/apache/airflow/pull/12458#issuecomment-729939021
[The Workflow run](https://github.com/apache/airflow/actions/runs/370994908) is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.
----------------------------------------------------------------
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] github-actions[bot] commented on pull request #12458: Improve www.security.get_accessible_dags() and webserver performance
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #12458:
URL: https://github.com/apache/airflow/pull/12458#issuecomment-729884319
The PR should be OK to be merged with just subset of tests as it does not modify Core of Airflow. The committers might merge it or can add a label 'full tests needed' and re-run it to run all tests if they see it is needed!
----------------------------------------------------------------
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] XD-DENG merged pull request #12458: Improve www.security.get_accessible_dags() and webserver performance
Posted by GitBox <gi...@apache.org>.
XD-DENG merged pull request #12458:
URL: https://github.com/apache/airflow/pull/12458
----------------------------------------------------------------
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] XD-DENG commented on pull request #12458: Improve www.security.get_accessible_dags() and webserver performance
Posted by GitBox <gi...@apache.org>.
XD-DENG commented on pull request #12458:
URL: https://github.com/apache/airflow/pull/12458#issuecomment-730354308
I have made the changes that @jhtimmins suggested. Thanks again for it.
A minor change on tops of @jhtimmins 's test is to add line to invoke self.security_manager.sync_perm_for_dag(). Otherwise it doesn't make sense.
(Using personal mobile device to make these changes during lunch time at work, so maybe fat finger 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jhtimmins commented on a change in pull request #12458: Improve www.security.get_accessible_dags() and webserver performance
Posted by GitBox <gi...@apache.org>.
jhtimmins commented on a change in pull request #12458:
URL: https://github.com/apache/airflow/pull/12458#discussion_r526623519
##########
File path: airflow/www/security.py
##########
@@ -261,6 +261,8 @@ def get_accessible_dags(self, user_actions, user, session=None):
for role in user_query.roles:
for permission in role.permissions:
resource = permission.view_menu.name
+ if resource == permissions.RESOURCE_DAG:
+ return session.query(DagModel)
Review comment:
This check needs to take place after the action check on line 264. This loops over all the permissions that a user has from their various roles, and if that permission resource is either `Dags` or a dag_id, then it checks whether the action for that permission matches the `user_actions` the function is looking up.
With the ordering included in this code, if the user has either `Dags.can_read` or `Dags.can_edit`, all dags will be returned. But if `user_actions == ['can_edit']`, all dags will get returned if the user has _any_ dag resource permission. So if they can view all dags, this function will incorrectly indicate that they can also edit all dags as well.
We can fix this by first checking that the permission's action is one we care about (listed in `user_actions`). I've included a fix and a test to check for this in https://github.com/apache/airflow/pull/12473.
Feel free to incorporate those changes into this PR, then we can delete the other 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on pull request #12458: Improve www.security.get_accessible_dags() and webserver performance
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #12458:
URL: https://github.com/apache/airflow/pull/12458#issuecomment-729917303
/cc @jhtimmins
----------------------------------------------------------------
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] XD-DENG commented on pull request #12458: Improve www.security.get_accessible_dags() and webserver performance
Posted by GitBox <gi...@apache.org>.
XD-DENG commented on pull request #12458:
URL: https://github.com/apache/airflow/pull/12458#issuecomment-730168332
Thanks @jhtimmins . Makes sense to me.
I will incorporate the changes accordingly later today, and ping you guys again then
----------------------------------------------------------------
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] github-actions[bot] commented on pull request #12458: Improve www.security.get_accessible_dags() and webserver performance
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #12458:
URL: https://github.com/apache/airflow/pull/12458#issuecomment-729915804
[The Workflow run](https://github.com/apache/airflow/actions/runs/370795564) is cancelling this PR. Building images for the PR has failed. Follow the the workflow link to check the reason.
----------------------------------------------------------------
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] XD-DENG edited a comment on pull request #12458: Improve www.security.get_accessible_dags() and webserver performance
Posted by GitBox <gi...@apache.org>.
XD-DENG edited a comment on pull request #12458:
URL: https://github.com/apache/airflow/pull/12458#issuecomment-730354308
I have made the changes that @jhtimmins suggested. Thanks again for it.
A minor change on top of @jhtimmins 's test is to add line to invoke `self.security_manager.sync_perm_for_dag()`. Otherwise it may not make sense.
----------------------------------------------------------------
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