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/12/16 13:54:58 UTC
[GitHub] [airflow] kaxil opened a new pull request #20346: Fix inconsitencies in checking edit permissions for a DAG
kaxil opened a new pull request #20346:
URL: https://github.com/apache/airflow/pull/20346
We were short-circuting permission in the views instead of letting the security manager handle that. A user will find it inconsistent as the Graph and other views check "per-dag" permissions via https://github.com/apache/airflow/blob/174681911f96f17d41a4f560ca08d5e200944f7f/airflow/www/views.py#L579
so if someone uses Custom Security Manager that will end up with user not being able to "pause" DAG from individual dag page but would be able to do so from Homepage. This PR fixes this inconsistency and gives back this responsibility of permissions to security manager instead if Views.
<!--
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/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 [UPDATING.md](https://github.com/apache/airflow/blob/main/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.
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 pull request #20346: Fix inconsitencies in checking edit permissions for a DAG
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on pull request #20346:
URL: https://github.com/apache/airflow/pull/20346#issuecomment-997073751
I'm just saying that, regardless of what security manager is in use, global `can_edit` should be expected to give edit on all DAGs. We shouldn't assume all things will use `get_editable_dags_ids`. Maybe we add a new `can_edit_all_dags` as a consistent way to check for that (and that assumption-breaking security managers can always return as false)?
It'll eventually short-circuit, but in the worst case scenario what impact do we have by not using the more efficient short circuit?
--
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] uranusjr commented on a change in pull request #20346: Fix inconsitencies in checking edit permissions for a DAG
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #20346:
URL: https://github.com/apache/airflow/pull/20346#discussion_r782669453
##########
File path: airflow/www/security.py
##########
@@ -298,6 +298,11 @@ def get_accessible_dag_ids(self, user, user_actions=None, session=None) -> Set[s
if user.is_anonymous:
roles = self.get_user_roles(user)
else:
+ if (permissions.ACTION_CAN_EDIT in user_actions and self.can_edit_all_dags()) or (
+ permissions.ACTION_CAN_READ in user_actions and self.can_read_all_dags()
+ ):
+ return {dag.dag_id for dag in session.query(DagModel.dag_id)}
Review comment:
This is causing tests to fail with
```
E RuntimeError: Working outside of application context.
E
E This typically means that you attempted to use functionality that needed
E to interface with the current application object in some way. To solve
E this, set up an application context with app.app_context(). See the
E documentation for more information.
```
because these `can` methods calls `_get_and_cache_perms` under the hood, which relies on `g.user`.
I think this should be fixable by setting up an application context in tests. This would make those tests _a lot_ slower, but unfortunately I don’t see a straightforward alternative.
--
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] kaxil commented on pull request #20346: Fix inconsitencies in checking edit permissions for a DAG
Posted by GitBox <gi...@apache.org>.
kaxil commented on pull request #20346:
URL: https://github.com/apache/airflow/pull/20346#issuecomment-997098689
> Add generic all read and all edit function
What do you think about - https://github.com/apache/airflow/pull/20346/commits/66a70332ee9d2ad5d4189b3d774aae0f25062de0?
--
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 pull request #20346: Fix inconsitencies in checking edit permissions for a DAG
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on pull request #20346:
URL: https://github.com/apache/airflow/pull/20346#issuecomment-997055109
Fwiw, this isn't how permissions are documented:
https://airflow.apache.org/docs/apache-airflow/stable/security/access-control.html#dag-level-permissions
Have we tried this in an instance with a large number of DAGs?
--
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 #20346: Fix inconsitencies in checking edit permissions for a DAG
Posted by GitBox <gi...@apache.org>.
bbovenzi commented on pull request #20346:
URL: https://github.com/apache/airflow/pull/20346#issuecomment-996997559
This looks right to me but I had issues with my local setup to test nor is python my strong suit.
--
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 #20346: Fix inconsitencies in checking edit permissions for a DAG
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #20346:
URL: https://github.com/apache/airflow/pull/20346#issuecomment-997102082
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] joshuayeung commented on pull request #20346: Fix inconsitencies in checking edit permissions for a DAG
Posted by GitBox <gi...@apache.org>.
joshuayeung commented on pull request #20346:
URL: https://github.com/apache/airflow/pull/20346#issuecomment-1072123265
Should we make the `can_create_dag_run` also handled by the security manager?
--
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] joshuayeung commented on pull request #20346: Fix inconsitencies in checking edit permissions for a DAG
Posted by GitBox <gi...@apache.org>.
joshuayeung commented on pull request #20346:
URL: https://github.com/apache/airflow/pull/20346#issuecomment-1072123265
Should we make the `can_create_dag_run` also handled by the security manager?
--
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] kaxil commented on pull request #20346: Fix inconsitencies in checking edit permissions for a DAG
Posted by GitBox <gi...@apache.org>.
kaxil commented on pull request #20346:
URL: https://github.com/apache/airflow/pull/20346#issuecomment-997060038
> Fwiw, this isn't how permissions are documented: https://airflow.apache.org/docs/apache-airflow/stable/security/access-control.html#dag-level-permissions
>
> Have we tried this in an instance with a large number of DAGs?
`current_app.appbuilder.sm.get_editable_dag_ids(g.user)` logically does the same thing what existed before the PR change too -- however instead of checking the permission in the view, it hands of the permissions of doing it to the Security Manager. So if you have "can_edit" permission on the "DAG" resource, a user will by default still be able to edit/pause all the dags.
re: Optimization -- we have a short-circuit too, i.e. as soon as we find "can_edit" on "DAG" we return, check the following source:
Source Code:
https://github.com/apache/airflow/blob/fc0fb22c120a7c12426ecaf6254159e7daf2de9e/airflow/www/security.py#L288-L290
https://github.com/apache/airflow/blob/fc0fb22c120a7c12426ecaf6254159e7daf2de9e/airflow/www/security.py#L320-L322
--
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 pull request #20346: Fix inconsitencies in checking edit permissions for a DAG
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on pull request #20346:
URL: https://github.com/apache/airflow/pull/20346#issuecomment-997101087
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org