Skip to content

Commit

Permalink
Refactor unneeded 'continue' jumps in www (#33838)
Browse files Browse the repository at this point in the history
  • Loading branch information
eumiro authored Sep 1, 2023
1 parent 3b58a38 commit f923bd3
Show file tree
Hide file tree
Showing 4 changed files with 44 additions and 62 deletions.
42 changes: 17 additions & 25 deletions airflow/www/security.py
Original file line number Diff line number Diff line change
Expand Up @@ -235,12 +235,10 @@ def __init__(self, appbuilder) -> None:
# This is needed to support the "hack" where we had to edit
# FieldConverter.conversion_table in place in airflow.www.utils
for attr in dir(self):
if not attr.endswith("view"):
continue
view = getattr(self, attr, None)
if not view or not getattr(view, "datamodel", None):
continue
view.datamodel = CustomSQLAInterface(view.datamodel.obj)
if attr.endswith("view"):
view = getattr(self, attr, None)
if view and getattr(view, "datamodel", None):
view.datamodel = CustomSQLAInterface(view.datamodel.obj)
self.perms = None

def _get_root_dag_id(self, dag_id: str) -> str:
Expand Down Expand Up @@ -374,17 +372,15 @@ def get_accessible_dag_ids(
for role in roles:
for permission in role.permissions:
action = permission.action.name
if action not in user_actions:
continue

resource = permission.resource.name
if resource == permissions.RESOURCE_DAG:
return {dag.dag_id for dag in session.execute(select(DagModel.dag_id))}

if resource.startswith(permissions.RESOURCE_DAG_PREFIX):
resources.add(resource[len(permissions.RESOURCE_DAG_PREFIX) :])
else:
resources.add(resource)
if action in user_actions:
resource = permission.resource.name
if resource == permissions.RESOURCE_DAG:
return {dag.dag_id for dag in session.execute(select(DagModel.dag_id))}

if resource.startswith(permissions.RESOURCE_DAG_PREFIX):
resources.add(resource[len(permissions.RESOURCE_DAG_PREFIX) :])
else:
resources.add(resource)
return {
dag.dag_id
for dag in session.execute(select(DagModel.dag_id).where(DagModel.dag_id.in_(resources)))
Expand Down Expand Up @@ -775,14 +771,10 @@ def check_authorization(
(permissions.ACTION_CAN_DELETE, permissions.RESOURCE_DAG),
):
can_access_all_dags = self.has_access(*perm)
if can_access_all_dags:
continue

action = perm[0]
if self.can_access_some_dags(action, dag_id):
continue
return False

if not can_access_all_dags:
action = perm[0]
if not self.can_access_some_dags(action, dag_id):
return False
elif not self.has_access(*perm):
return False

Expand Down
11 changes: 5 additions & 6 deletions airflow/www/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -806,12 +806,11 @@ def clean_column_names():
clean_column_names()
# Support for AssociationProxy in search and list columns
for obj_attr, desc in self.obj.__mapper__.all_orm_descriptors.items():
if not isinstance(desc, AssociationProxy):
continue
proxy_instance = getattr(self.obj, obj_attr)
if hasattr(proxy_instance.remote_attr.prop, "columns"):
self.list_columns[obj_attr] = proxy_instance.remote_attr.prop.columns[0]
self.list_properties[obj_attr] = proxy_instance.remote_attr.prop
if isinstance(desc, AssociationProxy):
proxy_instance = getattr(self.obj, obj_attr)
if hasattr(proxy_instance.remote_attr.prop, "columns"):
self.list_columns[obj_attr] = proxy_instance.remote_attr.prop.columns[0]
self.list_properties[obj_attr] = proxy_instance.remote_attr.prop

def is_utcdatetime(self, col_name):
"""Check if the datetime is a UTC one."""
Expand Down
36 changes: 16 additions & 20 deletions airflow/www/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -971,12 +971,10 @@ def index(self):
def _iter_parsed_moved_data_table_names():
for table_name in inspect(session.get_bind()).get_table_names():
segments = table_name.split("__", 3)
if len(segments) < 3:
continue
if segments[0] != settings.AIRFLOW_MOVED_TABLE_PREFIX:
continue
# Second segment is a version marker that we don't need to show.
yield segments[-1], table_name
if len(segments) >= 3:
if segments[0] == settings.AIRFLOW_MOVED_TABLE_PREFIX:
# Second segment is a version marker that we don't need to show.
yield segments[-1], table_name

if (
permissions.ACTION_CAN_ACCESS_MENU,
Expand Down Expand Up @@ -3379,12 +3377,11 @@ def tries(self, dag_id: str, session: Session = NEW_SESSION):
y_points = []
x_points = []
for ti in tis:
if ti.task_id != task.task_id:
continue
dttm = wwwutils.epoch(ti.execution_date)
x_points.append(dttm)
# y value should reflect completed tries to have a 0 baseline.
y_points.append(ti.prev_attempted_tries)
if ti.task_id == task.task_id:
dttm = wwwutils.epoch(ti.execution_date)
x_points.append(dttm)
# y value should reflect completed tries to have a 0 baseline.
y_points.append(ti.prev_attempted_tries)
if x_points:
chart.add_serie(name=task.task_id, x=x_points, y=y_points)

Expand Down Expand Up @@ -3476,14 +3473,13 @@ def landing_times(self, dag_id: str, session: Session = NEW_SESSION):
for task in dag.tasks:
task_id = task.task_id
for ti in tis:
if ti.task_id != task.task_id:
continue
ts = dag.get_run_data_interval(ti.dag_run).end
if ti.end_date:
dttm = wwwutils.epoch(ti.execution_date)
secs = (ti.end_date - ts).total_seconds()
x_points[task_id].append(dttm)
y_points[task_id].append(secs)
if ti.task_id == task.task_id:
ts = dag.get_run_data_interval(ti.dag_run).end
if ti.end_date:
dttm = wwwutils.epoch(ti.execution_date)
secs = (ti.end_date - ts).total_seconds()
x_points[task_id].append(dttm)
y_points[task_id].append(secs)

# determine the most relevant time unit for the set of landing times
# for the DAG
Expand Down
17 changes: 6 additions & 11 deletions tests/www/views/test_views_configuration.py
Original file line number Diff line number Diff line change
Expand Up @@ -38,31 +38,26 @@ def test_user_can_view_configuration(admin_client):
resp = admin_client.get("configuration", follow_redirects=True)
for section, key in conf.sensitive_config_values:
value = conf.get(section, key, fallback="")
if not value:
continue
check_content_in_response(html.escape(value), resp)
if value:
check_content_in_response(html.escape(value), resp)


@conf_vars({("webserver", "expose_config"): "non-sensitive-only"})
def test_configuration_redacted(admin_client):
resp = admin_client.get("configuration", follow_redirects=True)
for section, key in conf.sensitive_config_values:
value = conf.get(section, key, fallback="")
if not value or value == "airflow":
continue
if value.startswith("db+postgresql"): # this is in configuration comment
continue
check_content_not_in_response(value, resp)
if value and value != "airflow" and not value.startswith("db+postgresql"):
check_content_not_in_response(value, resp)


@conf_vars({("webserver", "expose_config"): "non-sensitive-only"})
def test_configuration_redacted_in_running_configuration(admin_client):
resp = admin_client.get("configuration", follow_redirects=True)
for section, key in conf.sensitive_config_values:
value = conf.get(section, key, fallback="")
if not value or value == "airflow":
continue
check_content_not_in_response("<td class='code'>" + html.escape(value) + "</td", resp)
if value and value != "airflow":
check_content_not_in_response("<td class='code'>" + html.escape(value) + "</td", resp)


@conf_vars({("webserver", "expose_config"): "non-sensitive-only"})
Expand Down

0 comments on commit f923bd3

Please sign in to comment.