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/09/10 23:01:42 UTC
[GitHub] [airflow] jhtimmins opened a new pull request #18160: Apply parent dag permissions to subdags.
jhtimmins opened a new pull request #18160:
URL: https://github.com/apache/airflow/pull/18160
<!--
This fixes a bug where users can't access a subdag even when they have access to the parent dag.
closes: #17652
related: https://github.com/apache/airflow/pull/7752
--
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 a change in pull request #18160: Apply parent dag permissions to subdags.
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #18160:
URL: https://github.com/apache/airflow/pull/18160#discussion_r706599647
##########
File path: airflow/www/security.py
##########
@@ -485,12 +485,14 @@ def has_access(self, action_name, resource_name, user=None) -> bool:
has_access = self._has_access(user, action_name, resource_name)
# FAB built-in view access method. Won't work for AllDag access.
-
if self.is_dag_resource(resource_name):
+ root_dag_resource_name = resource_name.split(".")[0]
Review comment:
Should this logic instead go in `can_read_dag` and `can_edit_dag`?
example:
```python
def can_read_dag(self, dag_id, user=None) -> bool:
"""Determines whether a user has DAG read access."""
if not user:
user = g.user
# To account for SubDags
root_dag_id = dag_id.split(".")[0]
dag_resource_name = permissions.resource_name_for_dag(root_dag_id)
return self._has_access(
user, permissions.ACTION_CAN_READ, permissions.RESOURCE_DAG
) or self._has_access(user, permissions.ACTION_CAN_READ, dag_resource_name)
def can_edit_dag(self, dag_id, user=None) -> bool:
"""Determines whether a user has DAG edit access."""
if not user:
user = g.user
# To account for SubDags
root_dag_id = dag_id.split(".")[0]
dag_resource_name = permissions.resource_name_for_dag(root_dag_id)
return self._has_access(
user, permissions.ACTION_CAN_EDIT, permissions.RESOURCE_DAG
) or self._has_access(user, permissions.ACTION_CAN_EDIT, dag_resource_name)
```
--
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 a change in pull request #18160: Apply parent dag permissions to subdags.
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #18160:
URL: https://github.com/apache/airflow/pull/18160#discussion_r706598917
##########
File path: airflow/www/security.py
##########
@@ -485,12 +485,15 @@ def has_access(self, action_name, resource_name, user=None) -> bool:
has_access = self._has_access(user, action_name, resource_name)
# FAB built-in view access method. Won't work for AllDag access.
-
+ # breakpoint()
Review comment:
```suggestion
```
--
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] jhtimmins commented on a change in pull request #18160: Apply parent dag permissions to subdags.
Posted by GitBox <gi...@apache.org>.
jhtimmins commented on a change in pull request #18160:
URL: https://github.com/apache/airflow/pull/18160#discussion_r706646302
##########
File path: airflow/www/security.py
##########
@@ -485,12 +485,14 @@ def has_access(self, action_name, resource_name, user=None) -> bool:
has_access = self._has_access(user, action_name, resource_name)
# FAB built-in view access method. Won't work for AllDag access.
-
if self.is_dag_resource(resource_name):
+ root_dag_resource_name = resource_name.split(".")[0]
Review comment:
@kaxil Yeah you're correct, this is where it should have gone.
Can a valid dag name include a period? If so, you'll need to test for both the complete version and the root version.
--
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 a change in pull request #18160: Apply parent dag permissions to subdags.
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #18160:
URL: https://github.com/apache/airflow/pull/18160#discussion_r706599647
##########
File path: airflow/www/security.py
##########
@@ -485,12 +485,14 @@ def has_access(self, action_name, resource_name, user=None) -> bool:
has_access = self._has_access(user, action_name, resource_name)
# FAB built-in view access method. Won't work for AllDag access.
-
if self.is_dag_resource(resource_name):
+ root_dag_resource_name = resource_name.split(".")[0]
Review comment:
Should this logic instead go in `can_read_dag` and `can_edit_dag`?
example:
```python
def can_read_dag(self, dag_id, user=None) -> bool:
"""Determines whether a user has DAG read access."""
if not user:
user = g.user
# To account for SubDags
root_dag_id = dag_id.split(".")[0]
dag_resource_name = permissions.resource_name_for_dag(root_dag_id)
return self._has_access(
user, permissions.ACTION_CAN_READ, permissions.RESOURCE_DAG
) or self._has_access(user, permissions.ACTION_CAN_READ, dag_resource_name)
```
--
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 a change in pull request #18160: Apply parent dag permissions to subdags.
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #18160:
URL: https://github.com/apache/airflow/pull/18160#discussion_r706600716
##########
File path: airflow/www/security.py
##########
@@ -485,12 +485,14 @@ def has_access(self, action_name, resource_name, user=None) -> bool:
has_access = self._has_access(user, action_name, resource_name)
# FAB built-in view access method. Won't work for AllDag access.
-
if self.is_dag_resource(resource_name):
+ root_dag_resource_name = resource_name.split(".")[0]
Review comment:
I updated here: https://github.com/apache/airflow/pull/18160/commits/14682d89e4897fc2572a549a0b0e02e341cfb3dc -- do let me know if you don't agree. I want to include it in 2.1.4 -- so pushed it myself for now
--
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 merged pull request #18160: Apply parent dag permissions to subdags.
Posted by GitBox <gi...@apache.org>.
kaxil merged pull request #18160:
URL: https://github.com/apache/airflow/pull/18160
--
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 a change in pull request #18160: Apply parent dag permissions to subdags.
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #18160:
URL: https://github.com/apache/airflow/pull/18160#discussion_r706600716
##########
File path: airflow/www/security.py
##########
@@ -485,12 +485,14 @@ def has_access(self, action_name, resource_name, user=None) -> bool:
has_access = self._has_access(user, action_name, resource_name)
# FAB built-in view access method. Won't work for AllDag access.
-
if self.is_dag_resource(resource_name):
+ root_dag_resource_name = resource_name.split(".")[0]
Review comment:
I updated here: https://github.com/apache/airflow/pull/18160/commits/14682d89e4897fc2572a549a0b0e02e341cfb3dc -- do let me know if you don't agree
--
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 a change in pull request #18160: Apply parent dag permissions to subdags.
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #18160:
URL: https://github.com/apache/airflow/pull/18160#discussion_r706599647
##########
File path: airflow/www/security.py
##########
@@ -485,12 +485,14 @@ def has_access(self, action_name, resource_name, user=None) -> bool:
has_access = self._has_access(user, action_name, resource_name)
# FAB built-in view access method. Won't work for AllDag access.
-
if self.is_dag_resource(resource_name):
+ root_dag_resource_name = resource_name.split(".")[0]
Review comment:
Should this logic instead go in `can_read_dag` and `can_edit_dag`?
--
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