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/08/24 06:21:39 UTC
[GitHub] [airflow] Jorricks opened a new pull request #16634: Require can_edit on DAG privileges to modify TaskInstances and DagRuns
Jorricks opened a new pull request #16634:
URL: https://github.com/apache/airflow/pull/16634
All permissions for modifying Task Instances or modifying Dag Runs as of today require `dag_read` permissions on the DAG and the corresponding action permission.
A full overview is shown [at the Access Control page of Airflow](https://airflow.apache.org/docs/apache-airflow/stable/security/access-control.html#dag-level-permissions)
It feels to me as in that case the whole `dag_edit` base_permission is undervalued in this case and the `dag_view` base_permission gives too much actual permissions.
Imagine the following setup:
- Everyone is able to see each others DAGs
- Some people should be able to modify their own DAGs
- They should not be able to modify their neighbours DAGs
This setup is currently not supported.
As a work around, on my work setup I currently implemented a SQLAlchemy listener to block update operations on TaskInstances where a user doesn't have `can_edit` privilege on this specific DAG.
Therefore this PR changes the following items(copied from the link above) to require `DAGS.can_edit` where it currently says `DAGS.can_read` privileges.
**Currently:**
Action | Permissions | Minimum Role
-- | -- | --
Clear DAG | DAGs.can_read, Task Instances.can_delete | User
Clear DAG Run | DAGs.can_read, Task Instances.can_delete | User
Mark DAG as blocked | Dags.can_read, DAG Runs.can_read | User
Mark DAG Run as failed | Dags.can_read, DAG Runs.can_edit | User
Mark DAG Run as success | Dags.can_read, DAG Runs.can_edit | User
Clear Task Instance | DAGs.can_read, DAG Runs.can_read, Task Instances.can_edit | User
Triggers Task Instance | DAGs.can_read, Task Instances.can_create | User
Mark Task as failed | DAGs.can_read, Task Instances.can_edit | User
Mark Task as success | DAGs.can_read, Task Instances.can_edit | User
**Updated:**
Action | Permissions | Minimum Role
-- | -- | --
Clear DAG | DAGs.can_edit, Task Instances.can_delete | User
Clear DAG Run | DAGs.can_edit, Task Instances.can_delete | User
Mark DAG as blocked | Dags.can_edit, DAG Runs.can_read | User
Mark DAG Run as failed | Dags.can_edit, DAG Runs.can_edit | User
Mark DAG Run as success | Dags.can_edit, DAG Runs.can_edit | User
Clear Task Instance | DAGs.can_edit, Task Instances.can_edit | User
Triggers Task Instance | DAGs.can_edit, Task Instances.can_create | User
Mark Task as failed | DAGs.can_edit, Task Instances.can_edit | User
Mark Task as success | DAGs.can_edit, Task Instances.can_edit | User
If there is interest in merging this PR, I will also make a corresponding PR on the docs side to update the page.
--
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] Jorricks commented on a change in pull request #16634: Change TaskInstances and DagRun modify permissions to require can_edit on DAG resource.
Posted by GitBox <gi...@apache.org>.
Jorricks commented on a change in pull request #16634:
URL: https://github.com/apache/airflow/pull/16634#discussion_r658102684
##########
File path: airflow/www/security.py
##########
@@ -520,6 +520,17 @@ def _has_perm(self, action_name, resource_name):
self._get_and_cache_perms()
return (action_name, resource_name) in self.perms
+ def has_all_dags_edit_access(self):
+ """
+ Has all the dag access in any of the 3 cases:
+ 1. Role needs to be in (Admin, Viewer, User, Op).
+ 2. Has can_read action on dags resource.
+ 3. Has can_edit action on dags resource.
+ """
+ return self._has_role(['Admin', 'Op', 'User']) or self._has_perm(
Review comment:
I agree. I didn't like the `_has_role` item in there either.
I think it's done for performance reasons.
Do you want me to remove it?
--
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] Jorricks edited a comment on pull request #16634: Change TaskInstances and DagRun modify permissions to require can_edit on DAG resource.
Posted by GitBox <gi...@apache.org>.
Jorricks edited a comment on pull request #16634:
URL: https://github.com/apache/airflow/pull/16634#issuecomment-869061373
I updated the PR today (26 June) with the following:
1. Inside the views, we already had the DagEditFilter. However, FlaskAppbuilder doesn't do the extra checks to verify that the items you are modifying/adding/deleting are inside the BaseFilter. Therefore, I created another abstraction for the view which checks for add/update/delete options that the user has `can_edit` access on the DAG the item he is accessing belongs to. If there was someone who still tried to cheat by changing primary keys in the front end with an HTML editor (add/update/delete a DAG he doesn't have edit access on), he will get an error message :)
2. For the actions, it is the same thing. However, there is no way within Flask Appbuilder to check the items you are working with before entering the function (without using wrapper). Hence, of course I created wrapper. For all items provided/selected, it checks that the user had `can_edit` access on the DAG it is a part of.
3. I implemented tests. For both DagRun and TaskInstance I created tests for `deleting`. For DagRun also for `adding`. Furthermore I also created the tests for all actions of both views to verify the behaviour of the actions.
PS: Currently waiting for the commits to become visible here... Is there some way we can force this?
--
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] Jorricks commented on a change in pull request #16634: Require can_edit on DAG privileges to modify TaskInstances and DagRuns
Posted by GitBox <gi...@apache.org>.
Jorricks commented on a change in pull request #16634:
URL: https://github.com/apache/airflow/pull/16634#discussion_r711916494
##########
File path: airflow/www/views.py
##########
@@ -3145,6 +3156,71 @@ class AirflowModelView(ModelView):
CustomSQLAInterface = wwwutils.CustomSQLAInterface
+class AirflowPrivilegeVerifierModelView(AirflowModelView):
+ """
+ This ModelView prevents ability to pass primary keys of objects relating to DAGs you shouldn't be able to
+ edit. This only holds for the add, update and delete operations.
+ You will still need to use the `action_has_dag_edit_access()` for actions.
+ """
+
+ @staticmethod
+ def validate_dag_edit_access(item: Union[DagRun, TaskInstance]):
+ """Validates whether the user has 'can_edit' access for this specific DAG."""
+ if not current_app.appbuilder.sm.can_edit_dag(item.dag_id):
+ raise AirflowException(f"Access denied for dag_id {item.dag_id}")
+
+ def pre_add(self, item: Union[DagRun, TaskInstance]):
+ self.validate_dag_edit_access(item)
+
+ def pre_update(self, item: Union[DagRun, TaskInstance]):
+ self.validate_dag_edit_access(item)
+
+ def pre_delete(self, item: Union[DagRun, TaskInstance]):
+ self.validate_dag_edit_access(item)
+
+ def post_add_redirect(self): # Required to prevent redirect loop
+ return redirect(self.get_default_url())
+
+ def post_edit_redirect(self): # Required to prevent redirect loop
+ return redirect(self.get_default_url())
+
+ def post_delete_redirect(self): # Required to prevent redirect loop
+ return redirect(self.get_default_url())
+
+
+def action_has_dag_edit_access(action_func: Callable) -> Callable:
+ """Decorator for actions which verifies you have DAG edit access on the given tis/drs."""
+
+ @wraps(action_func)
+ def check_dag_edit_acl_for_actions(
+ self,
+ items: Optional[Union[Iterable[TaskInstance], Iterable[DagRun], TaskInstance, DagRun]],
+ *args,
+ **kwargs,
+ ):
+ if items is None:
+ dag_ids: Set[str] = set()
+ elif isinstance(items, list):
Review comment:
Support all sort of Iterables.
```suggestion
- elif isinstance(items, list):
+ elif isinstance(items, Iterable):
```
--
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 pull request #16634: Require can_edit on DAG privileges to modify TaskInstances and DagRuns
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #16634:
URL: https://github.com/apache/airflow/pull/16634#issuecomment-920802362
That’s what happens when you have a file containing ~5k lines of source code 😞 One day I’m going to sit down and split `www/views.py` into a directory with like 20 files.
--
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 pull request #16634: Require can_edit on DAG privileges to modify TaskInstances and DagRuns
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #16634:
URL: https://github.com/apache/airflow/pull/16634#issuecomment-919848919
Still mailing 😞
--
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 merged pull request #16634: Require can_edit on DAG privileges to modify TaskInstances and DagRuns
Posted by GitBox <gi...@apache.org>.
jedcunningham merged pull request #16634:
URL: https://github.com/apache/airflow/pull/16634
--
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] Jorricks commented on a change in pull request #16634: Require can_edit on DAG privileges to modify TaskInstances and DagRuns
Posted by GitBox <gi...@apache.org>.
Jorricks commented on a change in pull request #16634:
URL: https://github.com/apache/airflow/pull/16634#discussion_r711362982
##########
File path: airflow/www/views.py
##########
@@ -3145,6 +3156,65 @@ class AirflowModelView(ModelView):
CustomSQLAInterface = wwwutils.CustomSQLAInterface
+class AirflowPrivilegeVerifierModelView(AirflowModelView):
+ """
+ This ModelView prevents ability to pass primary keys of objects relating to DAGs you shouldn't be able to
+ edit. This only holds for the add, update and delete operations.
+ You will still need to use the `action_has_dag_edit_access()` for actions.
+ """
+
+ @staticmethod
+ def validate_dag_edit_access(item: Union[DagRun, TaskInstance]):
+ """Validates whether the user has 'can_edit' access for this specific DAG."""
+ if not current_app.appbuilder.sm.can_edit_dag(item.dag_id):
+ raise AirflowException(f"Access denied for dag_id {item.dag_id}")
+
+ def pre_add(self, item: Union[DagRun, TaskInstance]):
+ self.validate_dag_edit_access(item)
+
+ def pre_update(self, item: Union[DagRun, TaskInstance]):
+ self.validate_dag_edit_access(item)
+
+ def pre_delete(self, item: Union[DagRun, TaskInstance]):
+ self.validate_dag_edit_access(item)
+
+ def post_add_redirect(self): # Required to prevent redirect loop
+ return redirect(self.get_default_url())
+
+ def post_edit_redirect(self): # Required to prevent redirect loop
+ return redirect(self.get_default_url())
+
+ def post_delete_redirect(self): # Required to prevent redirect loop
+ return redirect(self.get_default_url())
+
+
+def action_has_dag_edit_access(action_func: Callable) -> Callable:
+ """Decorator for actions which verifies you have DAG edit access on the given tis/drs."""
+
+ @wraps(action_func)
+ def check_dag_edit_acl_for_actions(
+ self,
+ items: Optional[Union[List[TaskInstance], List[DagRun], TaskInstance, DagRun, None]],
+ *args,
+ **kwargs,
+ ) -> None:
+ if items is None:
+ dag_ids: Set[str] = set()
+ elif isinstance(items, list):
+ dag_ids = {item.dag_id for item in items if item is not None}
+ else:
+ dag_ids = items.dag_id
+
+ for dag_id in dag_ids:
Review comment:
Good catch! The last else should have been a set instead of a singular item.
--
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] potiuk commented on pull request #16634: Require can_edit on DAG privileges to modify TaskInstances and DagRuns
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #16634:
URL: https://github.com/apache/airflow/pull/16634#issuecomment-890591321
I am not as confident on permissions as @jhtimmins -maybe you can review that James?
--
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] Jorricks edited a comment on pull request #16634: Change TaskInstances and DagRun modify permissions to require can_edit on DAG resource.
Posted by GitBox <gi...@apache.org>.
Jorricks edited a comment on pull request #16634:
URL: https://github.com/apache/airflow/pull/16634#issuecomment-869061373
I updated the PR today (26 June) with the following:
1. Inside the views, we already had the DagEditFilter. However, FlaskAppbuilder doesn't do the extra checks to verify that the items you are modifying/adding/deleting are inside the BaseFilter. Therefore, I created another abstraction for the view which checks for add/update/delete options that the user has `can_edit` access on the DAG the item he is accessing belongs to. If there was someone who still tried to cheat by changing primary keys in the front end with an HTML editor (add/update/delete a DAG he doesn't have edit access on), he will get an error message :)
2. For the actions, it is the same thing. However, there is no way within Flask Appbuilder to check the items you are working with before entering the function (without using wrapper). Hence, of course I created wrapper. For all items provided/selected, it checks that the user had `can_edit` access on the DAG it is a part of.
3. I implemented tests. For both DagRun and TaskInstance I created tests for `deleting`. For DagRun also for `adding`. Furthermore I also created the tests for all actions of both views to verify the behaviour of the actions.
--
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] Jorricks commented on a change in pull request #16634: Require can_edit on DAG privileges to modify TaskInstances and DagRuns
Posted by GitBox <gi...@apache.org>.
Jorricks commented on a change in pull request #16634:
URL: https://github.com/apache/airflow/pull/16634#discussion_r713392948
##########
File path: airflow/www/views.py
##########
@@ -3145,6 +3156,65 @@ class AirflowModelView(ModelView):
CustomSQLAInterface = wwwutils.CustomSQLAInterface
+class AirflowPrivilegeVerifierModelView(AirflowModelView):
+ """
+ This ModelView prevents ability to pass primary keys of objects relating to DAGs you shouldn't be able to
+ edit. This only holds for the add, update and delete operations.
+ You will still need to use the `action_has_dag_edit_access()` for actions.
+ """
+
+ @staticmethod
+ def validate_dag_edit_access(item: Union[DagRun, TaskInstance]):
+ """Validates whether the user has 'can_edit' access for this specific DAG."""
+ if not current_app.appbuilder.sm.can_edit_dag(item.dag_id):
+ raise AirflowException(f"Access denied for dag_id {item.dag_id}")
+
+ def pre_add(self, item: Union[DagRun, TaskInstance]):
+ self.validate_dag_edit_access(item)
+
+ def pre_update(self, item: Union[DagRun, TaskInstance]):
+ self.validate_dag_edit_access(item)
+
+ def pre_delete(self, item: Union[DagRun, TaskInstance]):
+ self.validate_dag_edit_access(item)
+
+ def post_add_redirect(self): # Required to prevent redirect loop
+ return redirect(self.get_default_url())
+
+ def post_edit_redirect(self): # Required to prevent redirect loop
+ return redirect(self.get_default_url())
+
+ def post_delete_redirect(self): # Required to prevent redirect loop
+ return redirect(self.get_default_url())
+
+
+def action_has_dag_edit_access(action_func: Callable) -> Callable:
+ """Decorator for actions which verifies you have DAG edit access on the given tis/drs."""
+
+ @wraps(action_func)
+ def check_dag_edit_acl_for_actions(
+ self,
+ items: Optional[Union[List[TaskInstance], List[DagRun], TaskInstance, DagRun, None]],
+ *args,
+ **kwargs,
+ ) -> None:
+ if items is None:
+ dag_ids: Set[str] = set()
+ elif isinstance(items, list):
+ dag_ids = {item.dag_id for item in items if item is not None}
+ else:
+ dag_ids = items.dag_id
+
+ for dag_id in dag_ids:
Review comment:
Closing this as this is fixed and also tested explicitly 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] jedcunningham commented on a change in pull request #16634: Require can_edit on DAG privileges to modify TaskInstances and DagRuns
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on a change in pull request #16634:
URL: https://github.com/apache/airflow/pull/16634#discussion_r713257794
##########
File path: airflow/www/security.py
##########
@@ -520,6 +520,17 @@ def _has_perm(self, action_name, resource_name):
self._get_and_cache_perms()
return (action_name, resource_name) in self.perms
+ def has_all_dags_edit_access(self):
+ """
+ Has all the dag access in any of the 3 cases:
+ 1. Role needs to be in (Admin, Viewer, User, Op).
+ 2. Has can_read action on dags resource.
+ 3. Has can_edit action on dags resource.
+ """
+ return self._has_role(['Admin', 'Op', 'User']) or self._has_perm(
Review comment:
@Jorricks, I also think we should remove the role check, and as @jhtimmins mentioned probably this helper too?
--
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] Jorricks edited a comment on pull request #16634: Require can_edit on DAG privileges to modify TaskInstances and DagRuns
Posted by GitBox <gi...@apache.org>.
Jorricks edited a comment on pull request #16634:
URL: https://github.com/apache/airflow/pull/16634#issuecomment-919453822
The `create_task_instance_of_operator` decorator in `conftest.py` made my life a lot easier :v: Tests should be fixed 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] uranusjr commented on pull request #16634: Require can_edit on DAG privileges to modify TaskInstances and DagRuns
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #16634:
URL: https://github.com/apache/airflow/pull/16634#issuecomment-890292297
CI seems to work 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] ashb commented on a change in pull request #16634: Change TaskInstances and DagRun modify permissions to require can_edit on DAG resource.
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #16634:
URL: https://github.com/apache/airflow/pull/16634#discussion_r658072491
##########
File path: airflow/www/security.py
##########
@@ -520,6 +520,17 @@ def _has_perm(self, action_name, resource_name):
self._get_and_cache_perms()
return (action_name, resource_name) in self.perms
+ def has_all_dags_edit_access(self):
+ """
+ Has all the dag access in any of the 3 cases:
+ 1. Role needs to be in (Admin, Viewer, User, Op).
+ 2. Has can_read action on dags resource.
+ 3. Has can_edit action on dags resource.
+ """
+ return self._has_role(['Admin', 'Op', 'User']) or self._has_perm(
Review comment:
Why do we need the rule check? Don't the roles already have this permission? (And by adding the Role is "x" it means the permissions on the build in roles don't have any effect)
##########
File path: airflow/www/security.py
##########
@@ -520,6 +520,17 @@ def _has_perm(self, action_name, resource_name):
self._get_and_cache_perms()
return (action_name, resource_name) in self.perms
+ def has_all_dags_edit_access(self):
+ """
+ Has all the dag access in any of the 3 cases:
+ 1. Role needs to be in (Admin, Viewer, User, Op).
+ 2. Has can_read action on dags resource.
+ 3. Has can_edit action on dags resource.
+ """
+ return self._has_role(['Admin', 'Op', 'User']) or self._has_perm(
Review comment:
Why do we need the role check? Don't the roles already have this permission? (And by adding the Role is "x" it means the permissions on the build in roles don't have any effect)
--
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] Jorricks commented on pull request #16634: Require can_edit on DAG privileges to modify TaskInstances and DagRuns
Posted by GitBox <gi...@apache.org>.
Jorricks commented on pull request #16634:
URL: https://github.com/apache/airflow/pull/16634#issuecomment-919730941
Hmm I expected this run to succeed as it ran successfully locally. Anyone has any idea what the problem is? My assumption is that the TaskInstances aren't directly created when the Dag Run is created. Is that correct?
--
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 pull request #16634: Require can_edit on DAG privileges to modify TaskInstances and DagRuns
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #16634:
URL: https://github.com/apache/airflow/pull/16634#issuecomment-920610364
Quarantined tests are quarantined because they misbehave (sometimes), so don’t worry about that.
--
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] Jorricks commented on a change in pull request #16634: Require can_edit on DAG privileges to modify TaskInstances and DagRuns
Posted by GitBox <gi...@apache.org>.
Jorricks commented on a change in pull request #16634:
URL: https://github.com/apache/airflow/pull/16634#discussion_r711913945
##########
File path: airflow/www/views.py
##########
@@ -3194,16 +3194,22 @@ def action_has_dag_edit_access(action_func: Callable) -> Callable:
@wraps(action_func)
def check_dag_edit_acl_for_actions(
self,
- items: Optional[Union[List[TaskInstance], List[DagRun], TaskInstance, DagRun, None]],
+ items: Optional[Union[Iterable[TaskInstance], Iterable[DagRun], TaskInstance, DagRun]],
*args,
**kwargs,
- ) -> None:
+ ):
Review comment:
When rewriting the function I must have accidentally removed it. Adding it back 👍
--
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 #16634: Require can_edit on DAG privileges to modify TaskInstances and DagRuns
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #16634:
URL: https://github.com/apache/airflow/pull/16634#discussion_r711902425
##########
File path: airflow/www/views.py
##########
@@ -3194,16 +3194,22 @@ def action_has_dag_edit_access(action_func: Callable) -> Callable:
@wraps(action_func)
def check_dag_edit_acl_for_actions(
self,
- items: Optional[Union[List[TaskInstance], List[DagRun], TaskInstance, DagRun, None]],
+ items: Optional[Union[Iterable[TaskInstance], Iterable[DagRun], TaskInstance, DagRun]],
Review comment:
The implementation currently can only take `list` (aside from one single TaskInstance and DagRun), if you change this, you need to rewrite the implementation as well.
##########
File path: airflow/www/views.py
##########
@@ -3194,16 +3194,22 @@ def action_has_dag_edit_access(action_func: Callable) -> Callable:
@wraps(action_func)
def check_dag_edit_acl_for_actions(
self,
- items: Optional[Union[List[TaskInstance], List[DagRun], TaskInstance, DagRun, None]],
+ items: Optional[Union[Iterable[TaskInstance], Iterable[DagRun], TaskInstance, DagRun]],
*args,
**kwargs,
- ) -> None:
+ ):
Review comment:
Why removing this?
--
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] Jorricks commented on pull request #16634: Require can_edit on DAG privileges to modify TaskInstances and DagRuns
Posted by GitBox <gi...@apache.org>.
Jorricks commented on pull request #16634:
URL: https://github.com/apache/airflow/pull/16634#issuecomment-914223620
I think this was my 5th time rebasing to the latest main. I hope someone can make the time to review my efforts 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] Jorricks commented on pull request #16634: Require can_edit on DAG privileges to modify TaskInstances and DagRuns
Posted by GitBox <gi...@apache.org>.
Jorricks commented on pull request #16634:
URL: https://github.com/apache/airflow/pull/16634#issuecomment-904079791
Let me know if you need me to rebase to the latest main 👍
--
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 closed pull request #16634: Require can_edit on DAG privileges to modify TaskInstances and DagRuns
Posted by GitBox <gi...@apache.org>.
uranusjr closed pull request #16634:
URL: https://github.com/apache/airflow/pull/16634
--
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] Jorricks edited a comment on pull request #16634: Require can_edit on DAG privileges to modify TaskInstances and DagRuns
Posted by GitBox <gi...@apache.org>.
Jorricks edited a comment on pull request #16634:
URL: https://github.com/apache/airflow/pull/16634#issuecomment-904079791
~~Let me know if you need me to rebase to the latest main 👍 ~~
Nvm. Saw a merge conflict so rebased to the latest main.
--
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 pull request #16634: Require can_edit on DAG privileges to modify TaskInstances and DagRuns
Posted by GitBox <gi...@apache.org>.
jhtimmins commented on pull request #16634:
URL: https://github.com/apache/airflow/pull/16634#issuecomment-919399312
@Jorricks sorry this got lost in the inbox. I'll take a look today
--
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 #16634: Require can_edit on DAG privileges to modify TaskInstances and DagRuns
Posted by GitBox <gi...@apache.org>.
jhtimmins commented on a change in pull request #16634:
URL: https://github.com/apache/airflow/pull/16634#discussion_r710607640
##########
File path: airflow/www/views.py
##########
@@ -3145,6 +3156,65 @@ class AirflowModelView(ModelView):
CustomSQLAInterface = wwwutils.CustomSQLAInterface
+class AirflowPrivilegeVerifierModelView(AirflowModelView):
+ """
+ This ModelView prevents ability to pass primary keys of objects relating to DAGs you shouldn't be able to
+ edit. This only holds for the add, update and delete operations.
+ You will still need to use the `action_has_dag_edit_access()` for actions.
+ """
+
+ @staticmethod
+ def validate_dag_edit_access(item: Union[DagRun, TaskInstance]):
+ """Validates whether the user has 'can_edit' access for this specific DAG."""
+ if not current_app.appbuilder.sm.can_edit_dag(item.dag_id):
+ raise AirflowException(f"Access denied for dag_id {item.dag_id}")
+
+ def pre_add(self, item: Union[DagRun, TaskInstance]):
+ self.validate_dag_edit_access(item)
+
+ def pre_update(self, item: Union[DagRun, TaskInstance]):
+ self.validate_dag_edit_access(item)
+
+ def pre_delete(self, item: Union[DagRun, TaskInstance]):
+ self.validate_dag_edit_access(item)
+
+ def post_add_redirect(self): # Required to prevent redirect loop
+ return redirect(self.get_default_url())
+
+ def post_edit_redirect(self): # Required to prevent redirect loop
+ return redirect(self.get_default_url())
+
+ def post_delete_redirect(self): # Required to prevent redirect loop
+ return redirect(self.get_default_url())
+
+
+def action_has_dag_edit_access(action_func: Callable) -> Callable:
+ """Decorator for actions which verifies you have DAG edit access on the given tis/drs."""
+
+ @wraps(action_func)
+ def check_dag_edit_acl_for_actions(
+ self,
+ items: Optional[Union[List[TaskInstance], List[DagRun], TaskInstance, DagRun, None]],
+ *args,
+ **kwargs,
+ ) -> None:
+ if items is None:
+ dag_ids: Set[str] = set()
+ elif isinstance(items, list):
+ dag_ids = {item.dag_id for item in items if item is not None}
+ else:
+ dag_ids = items.dag_id
+
+ for dag_id in dag_ids:
Review comment:
If `items` is a single object not in a list, won't `dag_ids` be a single ID, and not an Iterable? If so, this will cause an error.
If there isn't a scenario where the `items_id` is a single object, we should be able to remove `TaskInstance, DagRun` from the typing.
##########
File path: airflow/www/security.py
##########
@@ -520,6 +520,17 @@ def _has_perm(self, action_name, resource_name):
self._get_and_cache_perms()
return (action_name, resource_name) in self.perms
+ def has_all_dags_edit_access(self):
+ """
+ Has all the dag access in any of the 3 cases:
+ 1. Role needs to be in (Admin, Viewer, User, Op).
+ 2. Has can_read action on dags resource.
+ 3. Has can_edit action on dags resource.
+ """
+ return self._has_role(['Admin', 'Op', 'User']) or self._has_perm(
Review comment:
I think that security.py already has too many helper functions, and _ideally_ we wouldn't be adding new ones.
That said, this is fine for the moment and shouldn't hold up the PR from getting merged.
--
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] Jorricks commented on a change in pull request #16634: Require can_edit on DAG privileges to modify TaskInstances and DagRuns
Posted by GitBox <gi...@apache.org>.
Jorricks commented on a change in pull request #16634:
URL: https://github.com/apache/airflow/pull/16634#discussion_r711574274
##########
File path: airflow/www/views.py
##########
@@ -3145,6 +3156,65 @@ class AirflowModelView(ModelView):
CustomSQLAInterface = wwwutils.CustomSQLAInterface
+class AirflowPrivilegeVerifierModelView(AirflowModelView):
+ """
+ This ModelView prevents ability to pass primary keys of objects relating to DAGs you shouldn't be able to
+ edit. This only holds for the add, update and delete operations.
+ You will still need to use the `action_has_dag_edit_access()` for actions.
+ """
+
+ @staticmethod
+ def validate_dag_edit_access(item: Union[DagRun, TaskInstance]):
+ """Validates whether the user has 'can_edit' access for this specific DAG."""
+ if not current_app.appbuilder.sm.can_edit_dag(item.dag_id):
+ raise AirflowException(f"Access denied for dag_id {item.dag_id}")
+
+ def pre_add(self, item: Union[DagRun, TaskInstance]):
+ self.validate_dag_edit_access(item)
+
+ def pre_update(self, item: Union[DagRun, TaskInstance]):
+ self.validate_dag_edit_access(item)
+
+ def pre_delete(self, item: Union[DagRun, TaskInstance]):
+ self.validate_dag_edit_access(item)
+
+ def post_add_redirect(self): # Required to prevent redirect loop
+ return redirect(self.get_default_url())
+
+ def post_edit_redirect(self): # Required to prevent redirect loop
+ return redirect(self.get_default_url())
+
+ def post_delete_redirect(self): # Required to prevent redirect loop
+ return redirect(self.get_default_url())
+
+
+def action_has_dag_edit_access(action_func: Callable) -> Callable:
+ """Decorator for actions which verifies you have DAG edit access on the given tis/drs."""
+
+ @wraps(action_func)
+ def check_dag_edit_acl_for_actions(
+ self,
+ items: Optional[Union[List[TaskInstance], List[DagRun], TaskInstance, DagRun, None]],
+ *args,
+ **kwargs,
+ ) -> None:
+ if items is None:
+ dag_ids: Set[str] = set()
+ elif isinstance(items, list):
+ dag_ids = {item.dag_id for item in items if item is not None}
+ else:
+ dag_ids = items.dag_id
+
+ for dag_id in dag_ids:
Review comment:
I fixed the issue and created a test to just test this specific decorator.
Interested to hear what you think.
##########
File path: airflow/www/views.py
##########
@@ -3145,6 +3156,65 @@ class AirflowModelView(ModelView):
CustomSQLAInterface = wwwutils.CustomSQLAInterface
+class AirflowPrivilegeVerifierModelView(AirflowModelView):
+ """
+ This ModelView prevents ability to pass primary keys of objects relating to DAGs you shouldn't be able to
+ edit. This only holds for the add, update and delete operations.
+ You will still need to use the `action_has_dag_edit_access()` for actions.
+ """
+
+ @staticmethod
+ def validate_dag_edit_access(item: Union[DagRun, TaskInstance]):
+ """Validates whether the user has 'can_edit' access for this specific DAG."""
+ if not current_app.appbuilder.sm.can_edit_dag(item.dag_id):
+ raise AirflowException(f"Access denied for dag_id {item.dag_id}")
+
+ def pre_add(self, item: Union[DagRun, TaskInstance]):
+ self.validate_dag_edit_access(item)
+
+ def pre_update(self, item: Union[DagRun, TaskInstance]):
+ self.validate_dag_edit_access(item)
+
+ def pre_delete(self, item: Union[DagRun, TaskInstance]):
+ self.validate_dag_edit_access(item)
+
+ def post_add_redirect(self): # Required to prevent redirect loop
+ return redirect(self.get_default_url())
+
+ def post_edit_redirect(self): # Required to prevent redirect loop
+ return redirect(self.get_default_url())
+
+ def post_delete_redirect(self): # Required to prevent redirect loop
+ return redirect(self.get_default_url())
+
+
+def action_has_dag_edit_access(action_func: Callable) -> Callable:
+ """Decorator for actions which verifies you have DAG edit access on the given tis/drs."""
+
+ @wraps(action_func)
+ def check_dag_edit_acl_for_actions(
+ self,
+ items: Optional[Union[List[TaskInstance], List[DagRun], TaskInstance, DagRun, None]],
+ *args,
+ **kwargs,
+ ) -> None:
+ if items is None:
+ dag_ids: Set[str] = set()
+ elif isinstance(items, list):
+ dag_ids = {item.dag_id for item in items if item is not None}
+ else:
+ dag_ids = items.dag_id
+
+ for dag_id in dag_ids:
Review comment:
I fixed the issue and you mentioned created a test to just test this specific decorator.
Interested to hear what you think.
##########
File path: airflow/www/views.py
##########
@@ -3145,6 +3156,65 @@ class AirflowModelView(ModelView):
CustomSQLAInterface = wwwutils.CustomSQLAInterface
+class AirflowPrivilegeVerifierModelView(AirflowModelView):
+ """
+ This ModelView prevents ability to pass primary keys of objects relating to DAGs you shouldn't be able to
+ edit. This only holds for the add, update and delete operations.
+ You will still need to use the `action_has_dag_edit_access()` for actions.
+ """
+
+ @staticmethod
+ def validate_dag_edit_access(item: Union[DagRun, TaskInstance]):
+ """Validates whether the user has 'can_edit' access for this specific DAG."""
+ if not current_app.appbuilder.sm.can_edit_dag(item.dag_id):
+ raise AirflowException(f"Access denied for dag_id {item.dag_id}")
+
+ def pre_add(self, item: Union[DagRun, TaskInstance]):
+ self.validate_dag_edit_access(item)
+
+ def pre_update(self, item: Union[DagRun, TaskInstance]):
+ self.validate_dag_edit_access(item)
+
+ def pre_delete(self, item: Union[DagRun, TaskInstance]):
+ self.validate_dag_edit_access(item)
+
+ def post_add_redirect(self): # Required to prevent redirect loop
+ return redirect(self.get_default_url())
+
+ def post_edit_redirect(self): # Required to prevent redirect loop
+ return redirect(self.get_default_url())
+
+ def post_delete_redirect(self): # Required to prevent redirect loop
+ return redirect(self.get_default_url())
+
+
+def action_has_dag_edit_access(action_func: Callable) -> Callable:
+ """Decorator for actions which verifies you have DAG edit access on the given tis/drs."""
+
+ @wraps(action_func)
+ def check_dag_edit_acl_for_actions(
+ self,
+ items: Optional[Union[List[TaskInstance], List[DagRun], TaskInstance, DagRun, None]],
+ *args,
+ **kwargs,
+ ) -> None:
+ if items is None:
+ dag_ids: Set[str] = set()
+ elif isinstance(items, list):
+ dag_ids = {item.dag_id for item in items if item is not None}
+ else:
+ dag_ids = items.dag_id
+
+ for dag_id in dag_ids:
Review comment:
I fixed the issue and you mentioned created a test for this specific decorator.
Interested to hear what you 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] Jorricks commented on pull request #16634: Require can_edit on DAG privileges to modify TaskInstances and DagRuns
Posted by GitBox <gi...@apache.org>.
Jorricks commented on pull request #16634:
URL: https://github.com/apache/airflow/pull/16634#issuecomment-890525713
Rebased on latest main.
--
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 change in pull request #16634: Require can_edit on DAG privileges to modify TaskInstances and DagRuns
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #16634:
URL: https://github.com/apache/airflow/pull/16634#discussion_r708302970
##########
File path: tests/www/views/test_views_tasks.py
##########
@@ -594,6 +622,38 @@ def _get_appbuilder_pk_string(model_view_cls, instance) -> str:
return model_view_cls._serialize_pk_if_composite(model_view_cls, pk_value)
+@pytest.fixture(scope="function")
+def task_instance_to_delete(session):
+ dag = DagBag().get_dag("example_bash_operator")
+ task0 = dag.get_task("runme_0")
+ task0.task_id = "test_task_instance_to_delete"
+ execution_date = timezone.datetime(2021, 6, 26)
+ ti = TaskInstance(task0, execution_date, state="success")
Review comment:
@Jorricks This is the cause of your failure now -- a recently merged change means that you can no longer create a TI in the database without a matching DagRun row.
--
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 #16634: Require can_edit on DAG privileges to modify TaskInstances and DagRuns
Posted by GitBox <gi...@apache.org>.
kaxil commented on pull request #16634:
URL: https://github.com/apache/airflow/pull/16634#issuecomment-923350460
Can you resolve the conflicts please @Jorricks
--
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 closed pull request #16634: Require can_edit on DAG privileges to modify TaskInstances and DagRuns
Posted by GitBox <gi...@apache.org>.
uranusjr closed pull request #16634:
URL: https://github.com/apache/airflow/pull/16634
--
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] Jorricks commented on pull request #16634: Require can_edit on DAG privileges to modify TaskInstances and DagRuns
Posted by GitBox <gi...@apache.org>.
Jorricks commented on pull request #16634:
URL: https://github.com/apache/airflow/pull/16634#issuecomment-901127445
> I am not as confident on permissions as @jhtimmins -maybe you can review that James?
Bump thread.
@jhtimmins @potiuk :)
--
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] Jorricks commented on pull request #16634: Require can_edit on DAG privileges to modify TaskInstances and DagRuns
Posted by GitBox <gi...@apache.org>.
Jorricks commented on pull request #16634:
URL: https://github.com/apache/airflow/pull/16634#issuecomment-885637512
Given the SQLite CI/CD pipeline was successful, I think it's safe to say I fixed the tests that were failing.
The rest of the failed pipelines I can't find an error message related to my changes.
--
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] Jorricks commented on pull request #16634: Require can_edit on DAG privileges to modify TaskInstances and DagRuns
Posted by GitBox <gi...@apache.org>.
Jorricks commented on pull request #16634:
URL: https://github.com/apache/airflow/pull/16634#issuecomment-919178184
I'm seeing loads of errors here.
I guess a lot of logic changed in the API and on the way we authenticate users.
Either I remove a bunch of tests, or I need to modify them.
--
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] Jorricks commented on pull request #16634: Require can_edit on DAG privileges to modify TaskInstances and DagRuns
Posted by GitBox <gi...@apache.org>.
Jorricks commented on pull request #16634:
URL: https://github.com/apache/airflow/pull/16634#issuecomment-919453822
The `create_task_instance_of_operator` made my life a lot easier :v: Tests should be fixed 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] Jorricks commented on a change in pull request #16634: Require can_edit on DAG privileges to modify TaskInstances and DagRuns
Posted by GitBox <gi...@apache.org>.
Jorricks commented on a change in pull request #16634:
URL: https://github.com/apache/airflow/pull/16634#discussion_r711916494
##########
File path: airflow/www/views.py
##########
@@ -3145,6 +3156,71 @@ class AirflowModelView(ModelView):
CustomSQLAInterface = wwwutils.CustomSQLAInterface
+class AirflowPrivilegeVerifierModelView(AirflowModelView):
+ """
+ This ModelView prevents ability to pass primary keys of objects relating to DAGs you shouldn't be able to
+ edit. This only holds for the add, update and delete operations.
+ You will still need to use the `action_has_dag_edit_access()` for actions.
+ """
+
+ @staticmethod
+ def validate_dag_edit_access(item: Union[DagRun, TaskInstance]):
+ """Validates whether the user has 'can_edit' access for this specific DAG."""
+ if not current_app.appbuilder.sm.can_edit_dag(item.dag_id):
+ raise AirflowException(f"Access denied for dag_id {item.dag_id}")
+
+ def pre_add(self, item: Union[DagRun, TaskInstance]):
+ self.validate_dag_edit_access(item)
+
+ def pre_update(self, item: Union[DagRun, TaskInstance]):
+ self.validate_dag_edit_access(item)
+
+ def pre_delete(self, item: Union[DagRun, TaskInstance]):
+ self.validate_dag_edit_access(item)
+
+ def post_add_redirect(self): # Required to prevent redirect loop
+ return redirect(self.get_default_url())
+
+ def post_edit_redirect(self): # Required to prevent redirect loop
+ return redirect(self.get_default_url())
+
+ def post_delete_redirect(self): # Required to prevent redirect loop
+ return redirect(self.get_default_url())
+
+
+def action_has_dag_edit_access(action_func: Callable) -> Callable:
+ """Decorator for actions which verifies you have DAG edit access on the given tis/drs."""
+
+ @wraps(action_func)
+ def check_dag_edit_acl_for_actions(
+ self,
+ items: Optional[Union[Iterable[TaskInstance], Iterable[DagRun], TaskInstance, DagRun]],
+ *args,
+ **kwargs,
+ ):
+ if items is None:
+ dag_ids: Set[str] = set()
+ elif isinstance(items, list):
Review comment:
Support all sort of Iterables.
```suggestion
elif isinstance(items, Iterable):
```
--
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] Jorricks edited a comment on pull request #16634: Require can_edit on DAG privileges to modify TaskInstances and DagRuns
Posted by GitBox <gi...@apache.org>.
Jorricks edited a comment on pull request #16634:
URL: https://github.com/apache/airflow/pull/16634#issuecomment-904079791
~~Let me know if you need me to rebase to the latest main 👍~~
Nvm. Saw a merge conflict so rebased to the latest main.
--
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] Jorricks commented on pull request #16634: Require can_edit on DAG privileges to modify TaskInstances and DagRuns
Posted by GitBox <gi...@apache.org>.
Jorricks commented on pull request #16634:
URL: https://github.com/apache/airflow/pull/16634#issuecomment-904439972
@uranusjr can we restart the CI/CD one more time :)?
Seems it didn't run anything last run.
--
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] Jorricks commented on a change in pull request #16634: Require can_edit on DAG privileges to modify TaskInstances and DagRuns
Posted by GitBox <gi...@apache.org>.
Jorricks commented on a change in pull request #16634:
URL: https://github.com/apache/airflow/pull/16634#discussion_r711916009
##########
File path: airflow/www/views.py
##########
@@ -3145,6 +3156,71 @@ class AirflowModelView(ModelView):
CustomSQLAInterface = wwwutils.CustomSQLAInterface
+class AirflowPrivilegeVerifierModelView(AirflowModelView):
+ """
+ This ModelView prevents ability to pass primary keys of objects relating to DAGs you shouldn't be able to
+ edit. This only holds for the add, update and delete operations.
+ You will still need to use the `action_has_dag_edit_access()` for actions.
+ """
+
+ @staticmethod
+ def validate_dag_edit_access(item: Union[DagRun, TaskInstance]):
+ """Validates whether the user has 'can_edit' access for this specific DAG."""
+ if not current_app.appbuilder.sm.can_edit_dag(item.dag_id):
+ raise AirflowException(f"Access denied for dag_id {item.dag_id}")
+
+ def pre_add(self, item: Union[DagRun, TaskInstance]):
+ self.validate_dag_edit_access(item)
+
+ def pre_update(self, item: Union[DagRun, TaskInstance]):
+ self.validate_dag_edit_access(item)
+
+ def pre_delete(self, item: Union[DagRun, TaskInstance]):
+ self.validate_dag_edit_access(item)
+
+ def post_add_redirect(self): # Required to prevent redirect loop
+ return redirect(self.get_default_url())
+
+ def post_edit_redirect(self): # Required to prevent redirect loop
+ return redirect(self.get_default_url())
+
+ def post_delete_redirect(self): # Required to prevent redirect loop
+ return redirect(self.get_default_url())
+
+
+def action_has_dag_edit_access(action_func: Callable) -> Callable:
+ """Decorator for actions which verifies you have DAG edit access on the given tis/drs."""
+
+ @wraps(action_func)
+ def check_dag_edit_acl_for_actions(
+ self,
+ items: Optional[Union[Iterable[TaskInstance], Iterable[DagRun], TaskInstance, DagRun]],
+ *args,
+ **kwargs,
+ ):
Review comment:
Change typing information back
```suggestion
) -> None:
```
--
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] Jorricks commented on pull request #16634: Require can_edit on DAG privileges to modify TaskInstances and DagRuns
Posted by GitBox <gi...@apache.org>.
Jorricks commented on pull request #16634:
URL: https://github.com/apache/airflow/pull/16634#issuecomment-920307196
Alright. I rewrote the last 3 failing tests. Hopefully they are more successful 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] Jorricks edited a comment on pull request #16634: Require can_edit on DAG privileges to modify TaskInstances and DagRuns
Posted by GitBox <gi...@apache.org>.
Jorricks edited a comment on pull request #16634:
URL: https://github.com/apache/airflow/pull/16634#issuecomment-914223620
Rebased to the latest main.
--
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] Jorricks commented on pull request #16634: Require can_edit on DAG privileges to modify TaskInstances and DagRuns
Posted by GitBox <gi...@apache.org>.
Jorricks commented on pull request #16634:
URL: https://github.com/apache/airflow/pull/16634#issuecomment-885081841
Rebased PR to latest main again.
Does anyone have any tips on how I can give this PR to have more exposure :)?
--
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] Jorricks edited a comment on pull request #16634: Require can_edit on DAG privileges to modify TaskInstances and DagRuns
Posted by GitBox <gi...@apache.org>.
Jorricks edited a comment on pull request #16634:
URL: https://github.com/apache/airflow/pull/16634#issuecomment-920325961
All tests passed :v: 💯
Only the quarantined step failed. Not sure what that is about?
--
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] Jorricks commented on pull request #16634: Change TaskInstances and DagRun modify permissions to require can_edit on DAG resource.
Posted by GitBox <gi...@apache.org>.
Jorricks commented on pull request #16634:
URL: https://github.com/apache/airflow/pull/16634#issuecomment-867778158
> At the very least this is going to need some tests to ensure that the behaviour you describe works, and continues to work :)
It already has quite some tests in there.
But I will create extra tests to show that the standard `can_read` permissions now fails.
--
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] Jorricks commented on pull request #16634: Require can_edit on DAG privileges to modify TaskInstances and DagRuns
Posted by GitBox <gi...@apache.org>.
Jorricks commented on pull request #16634:
URL: https://github.com/apache/airflow/pull/16634#issuecomment-908970506
Hey hey,
In on holiday atm. But will do that ones I get back :thumsup:.
On Tue, 31 Aug 2021, 08:32 Tzu-ping Chung, ***@***.***> wrote:
> @Jorricks <https://github.com/Jorricks> I just re-triggered CI, but if it
> fails again, you probably need to rebase the PR to latest main.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/airflow/pull/16634#issuecomment-908940476>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AE2RU4ESEOYOY6JJT24ZWUDT7RZOPANCNFSM47IDBQEA>
> .
> Triage notifications on the go with GitHub Mobile for iOS
> <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
> or Android
> <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
>
>
--
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 #16634: Change TaskInstances and DagRun modify permissions to require can_edit on DAG resource.
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #16634:
URL: https://github.com/apache/airflow/pull/16634#issuecomment-867747699
> - Everyone is able to see each others DAGs
> - Some people should be able to modify their own DAGs
> - They should not be able to modify their neighbours DAGs
Oh yes, this should totally be supported.
--
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] ephraimbuddy closed pull request #16634: Require can_edit on DAG privileges to modify TaskInstances and DagRuns
Posted by GitBox <gi...@apache.org>.
ephraimbuddy closed pull request #16634:
URL: https://github.com/apache/airflow/pull/16634
--
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 pull request #16634: Require can_edit on DAG privileges to modify TaskInstances and DagRuns
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #16634:
URL: https://github.com/apache/airflow/pull/16634#issuecomment-920610547
There are merge conflicts though.
--
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] Jorricks commented on pull request #16634: Require can_edit on DAG privileges to modify TaskInstances and DagRuns
Posted by GitBox <gi...@apache.org>.
Jorricks commented on pull request #16634:
URL: https://github.com/apache/airflow/pull/16634#issuecomment-923621986
> Can you resolve the conflicts please @Jorricks
On it 👍
--
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] potiuk closed pull request #16634: Require can_edit on DAG privileges to modify TaskInstances and DagRuns
Posted by GitBox <gi...@apache.org>.
potiuk closed pull request #16634:
URL: https://github.com/apache/airflow/pull/16634
--
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] Jorricks commented on pull request #16634: Require can_edit on DAG privileges to modify TaskInstances and DagRuns
Posted by GitBox <gi...@apache.org>.
Jorricks commented on pull request #16634:
URL: https://github.com/apache/airflow/pull/16634#issuecomment-906967141
I think we should really be aiming at Airflow 2.2 with this change.
It's truly a standard feature we are missing here and I'd like to get rid of the horrible code that prevents our users from modifying DAGs they don't have `edit` access too.
--
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 #16634: Require can_edit on DAG privileges to modify TaskInstances and DagRuns
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #16634:
URL: https://github.com/apache/airflow/pull/16634#issuecomment-920805365
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] Jorricks commented on pull request #16634: Require can_edit on DAG privileges to modify TaskInstances and DagRuns
Posted by GitBox <gi...@apache.org>.
Jorricks commented on pull request #16634:
URL: https://github.com/apache/airflow/pull/16634#issuecomment-901127445
> I am not as confident on permissions as @jhtimmins -maybe you can review that James?
Bump thread.
@jhtimmins @potiuk :)
--
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 pull request #16634: Require can_edit on DAG privileges to modify TaskInstances and DagRuns
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #16634:
URL: https://github.com/apache/airflow/pull/16634#issuecomment-919735714
I think you need to pass `session` into `create_task_instance_of_operator`
--
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] Jorricks closed pull request #16634: Require can_edit on DAG privileges to modify TaskInstances and DagRuns
Posted by GitBox <gi...@apache.org>.
Jorricks closed pull request #16634:
URL: https://github.com/apache/airflow/pull/16634
--
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] Jorricks commented on pull request #16634: Require can_edit on DAG privileges to modify TaskInstances and DagRuns
Posted by GitBox <gi...@apache.org>.
Jorricks commented on pull request #16634:
URL: https://github.com/apache/airflow/pull/16634#issuecomment-919778174
> I think you need to pass `session` into `create_task_instance_of_operator`
Thanks for the quick reply. Let's hope it passes now :v:
--
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] Jorricks edited a comment on pull request #16634: Require can_edit on DAG privileges to modify TaskInstances and DagRuns
Posted by GitBox <gi...@apache.org>.
Jorricks edited a comment on pull request #16634:
URL: https://github.com/apache/airflow/pull/16634#issuecomment-920678577
> There are merge conflicts though.
That wasn't the case yesterday, man I keep on rebasing :P.
Anyways, rebased on latest main again :v:
--
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 closed pull request #16634: Require can_edit on DAG privileges to modify TaskInstances and DagRuns
Posted by GitBox <gi...@apache.org>.
uranusjr closed pull request #16634:
URL: https://github.com/apache/airflow/pull/16634
--
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] Jorricks commented on pull request #16634: Require can_edit on DAG privileges to modify TaskInstances and DagRuns
Posted by GitBox <gi...@apache.org>.
Jorricks commented on pull request #16634:
URL: https://github.com/apache/airflow/pull/16634#issuecomment-908970506
Hey hey,
In on holiday atm. But will do that ones I get back :thumsup:.
On Tue, 31 Aug 2021, 08:32 Tzu-ping Chung, ***@***.***> wrote:
> @Jorricks <https://github.com/Jorricks> I just re-triggered CI, but if it
> fails again, you probably need to rebase the PR to latest main.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/airflow/pull/16634#issuecomment-908940476>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AE2RU4ESEOYOY6JJT24ZWUDT7RZOPANCNFSM47IDBQEA>
> .
> Triage notifications on the go with GitHub Mobile for iOS
> <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
> or Android
> <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
>
>
--
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] Jorricks edited a comment on pull request #16634: Require can_edit on DAG privileges to modify TaskInstances and DagRuns
Posted by GitBox <gi...@apache.org>.
Jorricks edited a comment on pull request #16634:
URL: https://github.com/apache/airflow/pull/16634#issuecomment-908970506
Hey hey,
In on holiday atm. But will do that once I get back :thumsup:.
On Tue, 31 Aug 2021, 08:32 Tzu-ping Chung, ***@***.***> wrote:
> @Jorricks <https://github.com/Jorricks> I just re-triggered CI, but if it
> fails again, you probably need to rebase the PR to latest main.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/airflow/pull/16634#issuecomment-908940476>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AE2RU4ESEOYOY6JJT24ZWUDT7RZOPANCNFSM47IDBQEA>
> .
> Triage notifications on the go with GitHub Mobile for iOS
> <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
> or Android
> <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
>
>
--
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] Jorricks commented on pull request #16634: Require can_edit on DAG privileges to modify TaskInstances and DagRuns
Posted by GitBox <gi...@apache.org>.
Jorricks commented on pull request #16634:
URL: https://github.com/apache/airflow/pull/16634#issuecomment-872074620
Rebased PR to latest main and squashed commits.
--
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 pull request #16634: Require can_edit on DAG privileges to modify TaskInstances and DagRuns
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #16634:
URL: https://github.com/apache/airflow/pull/16634#issuecomment-908940476
@Jorricks I just re-triggered CI, but if it fails again, you probably need to rebase the PR to latest main.
--
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] potiuk commented on pull request #16634: Require can_edit on DAG privileges to modify TaskInstances and DagRuns
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #16634:
URL: https://github.com/apache/airflow/pull/16634#issuecomment-886225139
Closed/reopened to re-trigger.
--
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] Jorricks commented on pull request #16634: Require can_edit on DAG privileges to modify TaskInstances and DagRuns
Posted by GitBox <gi...@apache.org>.
Jorricks commented on pull request #16634:
URL: https://github.com/apache/airflow/pull/16634#issuecomment-905710216
CI pipeline still not running..
--
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] Jorricks commented on pull request #16634: Require can_edit on DAG privileges to modify TaskInstances and DagRuns
Posted by GitBox <gi...@apache.org>.
Jorricks commented on pull request #16634:
URL: https://github.com/apache/airflow/pull/16634#issuecomment-922095213
> > Can I just create a PR to do it or does it need to be done at a specific time?
>
> No specific timeline, this is just something that I want to do. So feel free to do a PR anytime you feel fit.
>
> The easy part would be to split helpers into a `views/utils.py` file, and some small view classes into their own files. The most difficult part would be to split up a giant `Airflow` class that spans >2k lines itself. It has a ton of not-really-related views, but we can’t just split it since the class name tied to URL back-references. There are a number of ways to work around that (mixins, composition, Flask URL registration hacks, or just changing the backrefs, etc.) and I’m not sure what’s the best technique.
Ah good one. Didn't think about the large Airflow class. I'll see if I can come up with something reasonable for that.
--
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] Jorricks commented on a change in pull request #16634: Require can_edit on DAG privileges to modify TaskInstances and DagRuns
Posted by GitBox <gi...@apache.org>.
Jorricks commented on a change in pull request #16634:
URL: https://github.com/apache/airflow/pull/16634#discussion_r713391890
##########
File path: airflow/www/security.py
##########
@@ -520,6 +520,17 @@ def _has_perm(self, action_name, resource_name):
self._get_and_cache_perms()
return (action_name, resource_name) in self.perms
+ def has_all_dags_edit_access(self):
+ """
+ Has all the dag access in any of the 3 cases:
+ 1. Role needs to be in (Admin, Viewer, User, Op).
+ 2. Has can_read action on dags resource.
+ 3. Has can_edit action on dags resource.
+ """
+ return self._has_role(['Admin', 'Op', 'User']) or self._has_perm(
Review comment:
Alright. Pushing an update that removes this unnecessary helper function.
I originally implemented it for performance reasons, as many of these helpers seem to be, but I agree that the performance improvement is negligible and not worth the extra maintenance.
--
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 pull request #16634: Require can_edit on DAG privileges to modify TaskInstances and DagRuns
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #16634:
URL: https://github.com/apache/airflow/pull/16634#issuecomment-908940476
@Jorricks I just re-triggered CI, but if it fails again, you probably need to rebase the PR to latest main.
--
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] Jorricks commented on pull request #16634: Require can_edit on DAG privileges to modify TaskInstances and DagRuns
Posted by GitBox <gi...@apache.org>.
Jorricks commented on pull request #16634:
URL: https://github.com/apache/airflow/pull/16634#issuecomment-920325961
Results look promising.
Not sure why the Quarantined tests failed?
--
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 #16634: Change TaskInstances and DagRun modify permissions to require can_edit on DAG resource.
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #16634:
URL: https://github.com/apache/airflow/pull/16634#issuecomment-867753999
> It feels weird to me actions that operate on DagRun and TaskInstance need edit permission on the DAG, since they don’t change things in it.
I agree, which is why it probably doesn't have it right now, but the only way to express "you have edit on Dag X, but not Dag Y" and have that apply to TaskInstances/DagRuns that I can think of would be a change like this
--
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] Jorricks edited a comment on pull request #16634: Require can_edit on DAG privileges to modify TaskInstances and DagRuns
Posted by GitBox <gi...@apache.org>.
Jorricks edited a comment on pull request #16634:
URL: https://github.com/apache/airflow/pull/16634#issuecomment-908970506
Hey hey,
In on holiday atm. But will do that once I get back :thumsup:.
On Tue, 31 Aug 2021, 08:32 Tzu-ping Chung, ***@***.***> wrote:
> @Jorricks <https://github.com/Jorricks> I just re-triggered CI, but if it
> fails again, you probably need to rebase the PR to latest main.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/airflow/pull/16634#issuecomment-908940476>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AE2RU4ESEOYOY6JJT24ZWUDT7RZOPANCNFSM47IDBQEA>
> .
> Triage notifications on the go with GitHub Mobile for iOS
> <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
> or Android
> <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
>
>
--
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] Jorricks commented on a change in pull request #16634: Require can_edit on DAG privileges to modify TaskInstances and DagRuns
Posted by GitBox <gi...@apache.org>.
Jorricks commented on a change in pull request #16634:
URL: https://github.com/apache/airflow/pull/16634#discussion_r708922539
##########
File path: tests/www/views/test_views_tasks.py
##########
@@ -594,6 +624,36 @@ def _get_appbuilder_pk_string(model_view_cls, instance) -> str:
return model_view_cls._serialize_pk_if_composite(model_view_cls, pk_value)
+@pytest.fixture(scope="function")
+def task_instance_to_delete(session, create_task_instance_of_operator):
+ return create_task_instance_of_operator(
+ BashOperator,
+ dag_id="unit_test_task_instance_delete",
+ task_id="test_bash",
+ bash_command="pwd",
+ execution_date=datetime.datetime(2021, 1, 1),
Review comment:
Proposed change from @uranusjr
```suggestion
execution_date=datetime.datetime(2021, 1, 1),
session=session,
```
--
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] Jorricks commented on pull request #16634: Require can_edit on DAG privileges to modify TaskInstances and DagRuns
Posted by GitBox <gi...@apache.org>.
Jorricks commented on pull request #16634:
URL: https://github.com/apache/airflow/pull/16634#issuecomment-921142840
> That’s what happens when you have a file containing ~5k lines of source code 😞 One day I’m going to sit down and split `www/views.py` into a directory with like 20 files.
I would love that challenge though 😁
Can I just create a PR to do it or does it need to be done at a specific time?
--
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 pull request #16634: Require can_edit on DAG privileges to modify TaskInstances and DagRuns
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #16634:
URL: https://github.com/apache/airflow/pull/16634#issuecomment-921556087
> Can I just create a PR to do it or does it need to be done at a specific time?
No specific timeline, this is just something that I want to do. So feel free to do a PR anytime you feel fit.
The easy part would be to split helpers into a `views/utils.py` file, and some small view classes into their own files. The most difficult part would be to split up a giant `Airflow` class that spans >2k lines itself. It has a ton of not-really-related views, but we can’t just split it since the class name tied to URL back-references. There are a number of ways to work around that (mixins, composition, Flask URL registration hacks, or just changing the backrefs, etc.) and I’m not sure what’s the best technique.
--
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] ephraimbuddy closed pull request #16634: Require can_edit on DAG privileges to modify TaskInstances and DagRuns
Posted by GitBox <gi...@apache.org>.
ephraimbuddy closed pull request #16634:
URL: https://github.com/apache/airflow/pull/16634
--
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 pull request #16634: Change TaskInstances and DagRun modify permissions to require can_edit on DAG resource.
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #16634:
URL: https://github.com/apache/airflow/pull/16634#issuecomment-867752737
It feels weird to me actions that operate on DagRun and TaskInstance need edit permission on the DAG, since they don’t change things in it.
--
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] ephraimbuddy closed pull request #16634: Require can_edit on DAG privileges to modify TaskInstances and DagRuns
Posted by GitBox <gi...@apache.org>.
ephraimbuddy closed pull request #16634:
URL: https://github.com/apache/airflow/pull/16634
--
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 #16634: Require can_edit on DAG privileges to modify TaskInstances and DagRuns
Posted by GitBox <gi...@apache.org>.
kaxil commented on pull request #16634:
URL: https://github.com/apache/airflow/pull/16634#issuecomment-885255983
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Jorricks commented on pull request #16634: Change TaskInstances and DagRun modify permissions to require can_edit on DAG resource.
Posted by GitBox <gi...@apache.org>.
Jorricks commented on pull request #16634:
URL: https://github.com/apache/airflow/pull/16634#issuecomment-869061373
I updated the PR with the following:
1. Inside the views, we already had the DagEditFilter. However, FlaskAppbuilder doesn't do the extra checks to verify that the items you are modifying/adding/deleting are inside the BaseFilter. Therefore, I created another abstraction for the view which checks for add/update/delete options that the user has `can_edit` access on the DAG the item he is accessing belongs to. If there was someone who still tried to cheat by changing primary keys in the front end with an HTML editor (add/update/delete a DAG he doesn't have edit access on), he will get an error message :)
2. For the actions, it is the same thing. However, there is no way within Flask Appbuilder to check the items you are working with before entering the function (without using wrapper). Hence, of course I created wrapper. For all items provided/selected, it checks that the user had `can_edit` access on the DAG it is a part of.
3. I implemented tests. For both DagRun and TaskInstance I created tests for `deleting`. For DagRun also for `adding`. Furthermore I also created the tests for all actions of both views to verify the behaviour of the actions.
--
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 edited a comment on pull request #16634: Require can_edit on DAG privileges to modify TaskInstances and DagRuns
Posted by GitBox <gi...@apache.org>.
kaxil edited a comment on pull request #16634:
URL: https://github.com/apache/airflow/pull/16634#issuecomment-885255983
cc @jhtimmins Can you take a look at this please
--
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] Jorricks commented on a change in pull request #16634: Require can_edit on DAG privileges to modify TaskInstances and DagRuns
Posted by GitBox <gi...@apache.org>.
Jorricks commented on a change in pull request #16634:
URL: https://github.com/apache/airflow/pull/16634#discussion_r708347767
##########
File path: tests/www/views/test_views_tasks.py
##########
@@ -594,6 +622,38 @@ def _get_appbuilder_pk_string(model_view_cls, instance) -> str:
return model_view_cls._serialize_pk_if_composite(model_view_cls, pk_value)
+@pytest.fixture(scope="function")
+def task_instance_to_delete(session):
+ dag = DagBag().get_dag("example_bash_operator")
+ task0 = dag.get_task("runme_0")
+ task0.task_id = "test_task_instance_to_delete"
+ execution_date = timezone.datetime(2021, 6, 26)
+ ti = TaskInstance(task0, execution_date, state="success")
Review comment:
Thanks a lot for pinpointing that!!!! I will see if I can spend some time to fix the tests this week.
--
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 pull request #16634: Require can_edit on DAG privileges to modify TaskInstances and DagRuns
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #16634:
URL: https://github.com/apache/airflow/pull/16634#issuecomment-919851208
Hmm, the only difference I can make out is I’ve only ever used `create_task_instance_of_operator` in a test function, not fixture. Maybe there’s some kind of pytest thing that causes things not fully populated before a test starts? Not sure at all...
--
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] Jorricks commented on pull request #16634: Require can_edit on DAG privileges to modify TaskInstances and DagRuns
Posted by GitBox <gi...@apache.org>.
Jorricks commented on pull request #16634:
URL: https://github.com/apache/airflow/pull/16634#issuecomment-921531599
Btw: corresponding with this change, we will need to update the airflow documentation.
I will create a PR for this but I would like to list the Airflow version for which the permissions changed.
Therefore, if someone merges this into a release branch, could they please give me a heads up :v:
--
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] Jorricks commented on a change in pull request #16634: Require can_edit on DAG privileges to modify TaskInstances and DagRuns
Posted by GitBox <gi...@apache.org>.
Jorricks commented on a change in pull request #16634:
URL: https://github.com/apache/airflow/pull/16634#discussion_r711916009
##########
File path: airflow/www/views.py
##########
@@ -3145,6 +3156,71 @@ class AirflowModelView(ModelView):
CustomSQLAInterface = wwwutils.CustomSQLAInterface
+class AirflowPrivilegeVerifierModelView(AirflowModelView):
+ """
+ This ModelView prevents ability to pass primary keys of objects relating to DAGs you shouldn't be able to
+ edit. This only holds for the add, update and delete operations.
+ You will still need to use the `action_has_dag_edit_access()` for actions.
+ """
+
+ @staticmethod
+ def validate_dag_edit_access(item: Union[DagRun, TaskInstance]):
+ """Validates whether the user has 'can_edit' access for this specific DAG."""
+ if not current_app.appbuilder.sm.can_edit_dag(item.dag_id):
+ raise AirflowException(f"Access denied for dag_id {item.dag_id}")
+
+ def pre_add(self, item: Union[DagRun, TaskInstance]):
+ self.validate_dag_edit_access(item)
+
+ def pre_update(self, item: Union[DagRun, TaskInstance]):
+ self.validate_dag_edit_access(item)
+
+ def pre_delete(self, item: Union[DagRun, TaskInstance]):
+ self.validate_dag_edit_access(item)
+
+ def post_add_redirect(self): # Required to prevent redirect loop
+ return redirect(self.get_default_url())
+
+ def post_edit_redirect(self): # Required to prevent redirect loop
+ return redirect(self.get_default_url())
+
+ def post_delete_redirect(self): # Required to prevent redirect loop
+ return redirect(self.get_default_url())
+
+
+def action_has_dag_edit_access(action_func: Callable) -> Callable:
+ """Decorator for actions which verifies you have DAG edit access on the given tis/drs."""
+
+ @wraps(action_func)
+ def check_dag_edit_acl_for_actions(
+ self,
+ items: Optional[Union[Iterable[TaskInstance], Iterable[DagRun], TaskInstance, DagRun]],
+ *args,
+ **kwargs,
+ ):
Review comment:
Change typing information back
```suggestion
- ):
+ ) -> None:
```
##########
File path: airflow/www/views.py
##########
@@ -3145,6 +3156,71 @@ class AirflowModelView(ModelView):
CustomSQLAInterface = wwwutils.CustomSQLAInterface
+class AirflowPrivilegeVerifierModelView(AirflowModelView):
+ """
+ This ModelView prevents ability to pass primary keys of objects relating to DAGs you shouldn't be able to
+ edit. This only holds for the add, update and delete operations.
+ You will still need to use the `action_has_dag_edit_access()` for actions.
+ """
+
+ @staticmethod
+ def validate_dag_edit_access(item: Union[DagRun, TaskInstance]):
+ """Validates whether the user has 'can_edit' access for this specific DAG."""
+ if not current_app.appbuilder.sm.can_edit_dag(item.dag_id):
+ raise AirflowException(f"Access denied for dag_id {item.dag_id}")
+
+ def pre_add(self, item: Union[DagRun, TaskInstance]):
+ self.validate_dag_edit_access(item)
+
+ def pre_update(self, item: Union[DagRun, TaskInstance]):
+ self.validate_dag_edit_access(item)
+
+ def pre_delete(self, item: Union[DagRun, TaskInstance]):
+ self.validate_dag_edit_access(item)
+
+ def post_add_redirect(self): # Required to prevent redirect loop
+ return redirect(self.get_default_url())
+
+ def post_edit_redirect(self): # Required to prevent redirect loop
+ return redirect(self.get_default_url())
+
+ def post_delete_redirect(self): # Required to prevent redirect loop
+ return redirect(self.get_default_url())
+
+
+def action_has_dag_edit_access(action_func: Callable) -> Callable:
+ """Decorator for actions which verifies you have DAG edit access on the given tis/drs."""
+
+ @wraps(action_func)
+ def check_dag_edit_acl_for_actions(
+ self,
+ items: Optional[Union[Iterable[TaskInstance], Iterable[DagRun], TaskInstance, DagRun]],
+ *args,
+ **kwargs,
+ ):
+ if items is None:
+ dag_ids: Set[str] = set()
+ elif isinstance(items, list):
Review comment:
Support all sort of Iterables.
```suggestion
elif isinstance(items, list):
elif isinstance(items, Iterable):
```
--
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] Jorricks commented on pull request #16634: Require can_edit on DAG privileges to modify TaskInstances and DagRuns
Posted by GitBox <gi...@apache.org>.
Jorricks commented on pull request #16634:
URL: https://github.com/apache/airflow/pull/16634#issuecomment-920678577
> There are merge conflicts though.
That wasn't the case yesterday, man I keep on rebasing :P.
Anyways, rebased on latest main and fixed the merge :v:
--
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