ashb commented on a change in pull request #11362:
URL: https://github.com/apache/airflow/pull/11362#discussion_r507633734
##########
File path: airflow/www/views.py
##########
@@ -859,8 +895,12 @@ def rendered(self):
title=title)
@expose('/get_logs_with_metadata')
- @has_dag_access(can_dag_read=True)
- @has_access
+ @auth.has_access([
+ (permissions.ACTION_CAN_READ, permissions.RESOURCE_DAGS),
+ (permissions.ACTION_CAN_READ, permissions.RESOURCE_TASK_INSTANCE),
+ (permissions.ACTION_CAN_READ, permissions.RESOURCE_LOG),
Review comment:
```suggestion
```
##########
File path: airflow/www/views.py
##########
@@ -1847,10 +1964,14 @@ class GraphForm(DateTimeWithNumRunsWithDagRunsForm):
dag_run_state=dt_nr_dr_data['dr_state'])
@expose('/duration')
- @has_dag_access(can_dag_read=True)
- @has_access
+ @auth.has_access([
+ (permissions.ACTION_CAN_READ, permissions.RESOURCE_DAGS),
+ (permissions.ACTION_CAN_READ, permissions.RESOURCE_TASK_INSTANCE),
+ (permissions.ACTION_CAN_READ, permissions.RESOURCE_TASK),
+ (permissions.ACTION_CAN_READ, permissions.RESOURCE_WEBSITE),
+ ])
@action_logging
- @provide_session
+ @provide_session # pylint: disable=too-many-locals
Review comment:
Errant pylint comment on decorator?
##########
File path: airflow/www/views.py
##########
@@ -2252,8 +2392,11 @@ def gantt(self, session=None):
)
@expose('/extra_links')
- @has_dag_access(can_dag_read=True)
- @has_access
+ @auth.has_access([
+ (permissions.ACTION_CAN_READ, permissions.RESOURCE_DAGS),
+ (permissions.ACTION_CAN_READ, permissions.RESOURCE_TASK),
Review comment:
```suggestion
(permissions.ACTION_CAN_READ, permissions.RESOURCE_TASK),
(permissions.ACTION_CAN_READ, permissions.RESOURCE_TASK_INSTANCE),
```
##########
File path: airflow/www/views.py
##########
@@ -978,8 +1024,14 @@ def log(self, session=None):
root=root, wrapped=conf.getboolean('webserver', 'default_wrap'))
@expose('/redirect_to_external_log')
- @has_dag_access(can_dag_read=True)
- @has_access
+ @auth.has_access(
+ [
+ (permissions.ACTION_CAN_READ, permissions.RESOURCE_DAGS),
+ (permissions.ACTION_CAN_READ, permissions.RESOURCE_TASK_INSTANCE),
+ (permissions.ACTION_CAN_READ, permissions.RESOURCE_LOG),
Review comment:
```suggestion
```
##########
File path: airflow/www/views.py
##########
@@ -1761,8 +1873,13 @@ def recurse_nodes(task, visited):
external_log_name=external_log_name)
@expose('/graph')
- @has_dag_access(can_dag_read=True)
- @has_access
+ @auth.has_access([
+ (permissions.ACTION_CAN_READ, permissions.RESOURCE_DAGS),
+ (permissions.ACTION_CAN_READ, permissions.RESOURCE_TASK_INSTANCE),
+ (permissions.ACTION_CAN_READ, permissions.RESOURCE_TASK),
+ (permissions.ACTION_CAN_READ, permissions.RESOURCE_LOG),
Review comment:
```suggestion
```
##########
File path: airflow/www/views.py
##########
@@ -944,8 +984,14 @@ def get_logs_with_metadata(self, session=None):
return jsonify(message=error_message, error=True,
metadata=metadata)
@expose('/log')
- @has_dag_access(can_dag_read=True)
- @has_access
+ @auth.has_access(
+ [
+ (permissions.ACTION_CAN_READ, permissions.RESOURCE_DAGS),
+ (permissions.ACTION_CAN_READ, permissions.RESOURCE_TASK_INSTANCE),
+ (permissions.ACTION_CAN_READ, permissions.RESOURCE_LOG),
Review comment:
```suggestion
```
##########
File path: airflow/www/views.py
##########
@@ -1365,8 +1450,12 @@ def clear(self):
recursive=recursive, confirmed=confirmed,
only_failed=only_failed)
@expose('/dagrun_clear', methods=['POST'])
- @has_dag_access(can_dag_edit=True)
- @has_access
+ @auth.has_access([
+ (permissions.ACTION_CAN_READ, permissions.RESOURCE_DAGS),
+ (permissions.ACTION_CAN_READ, permissions.RESOURCE_TASK),
+ (permissions.ACTION_CAN_DELETE, permissions.RESOURCE_TASK_INSTANCE),
Review comment:
```suggestion
(permissions.ACTION_CAN_DELETE, permissions.RESOURCE_DAG_RUN),
```
Possibly _also_ can_delete TI
##########
File path: airflow/www/views.py
##########
@@ -394,8 +394,10 @@ def health(self):
return wwwutils.json_response(payload)
@expose('/home')
- @has_access
- def index(self): # pylint: disable=too-many-locals,too-many-statements
+ @auth.has_access([
+ (permissions.ACTION_CAN_READ, permissions.RESOURCE_WEBSITE),
Review comment:
So other than this one case, the website permission feels entirely
needless. I wonder if this one could use `CAN_READ, RESOURCE_DAGS` here, and
entirely the website permissions.
(can read dags here, because the home page is effectively a list of the DAGs)
##########
File path: airflow/www/views.py
##########
@@ -2108,8 +2237,10 @@ def landing_times(self, session=None):
)
@expose('/paused', methods=['POST'])
- @has_dag_access(can_dag_edit=True)
- @has_access
+ @auth.has_access([
+ (permissions.ACTION_CAN_EDIT, permissions.RESOURCE_DAGS),
Review comment:
Hmmmmm - I do wonder if long term we'd want more granular permissions on
DAGs.
For example, being able to pause/unpause is a lot less "dangerous" than some
things we could do on a DAG. (I guess right now there aren't any other edit
operations on a DAG)
##########
File path: tests/www/test_views.py
##########
@@ -2058,17 +2315,42 @@ def test_run_success_for_all_dag_user(self):
self.check_content_in_response('', resp, resp_code=302)
def test_blocked_success(self):
- url = 'blocked'
self.logout()
- self.login()
+ username = 'blocked_success_user'
+ fab_utils.create_user(
+ self.app,
+ username=username,
+ role_name='blocked_success_role',
+ permissions=[
+ (permissions.ACTION_CAN_READ, permissions.RESOURCE_DAGS),
+ (permissions.ACTION_CAN_READ, permissions.RESOURCE_DAG_RUN),
+ (permissions.ACTION_CAN_READ, permissions.RESOURCE_WEBSITE),
+ ]
+ )
+ self.login(username=username,
+ password=username)
Review comment:
This block appears a lot. I wonder if it's worth making a new helper
methond on self:
```suggestion
username = 'blocked_success_user'
self.make_user_and_login
username=username,
permissions=[
(permissions.ACTION_CAN_READ, permissions.RESOURCE_DAGS),
(permissions.ACTION_CAN_READ, permissions.RESOURCE_DAG_RUN),
(permissions.ACTION_CAN_READ, permissions.RESOURCE_WEBSITE),
]
)
```
##########
File path: airflow/www/views.py
##########
@@ -1595,11 +1702,16 @@ def success(self):
future, past, State.SUCCESS)
@expose('/tree')
- @has_dag_access(can_dag_read=True)
- @has_access
+ @auth.has_access([
+ (permissions.ACTION_CAN_READ, permissions.RESOURCE_DAGS),
+ (permissions.ACTION_CAN_READ, permissions.RESOURCE_TASK),
+ (permissions.ACTION_CAN_READ, permissions.RESOURCE_TASK_INSTANCE),
+ (permissions.ACTION_CAN_READ, permissions.RESOURCE_LOG),
Review comment:
```suggestion
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
