This is an automated email from the ASF dual-hosted git repository. ephraimanierobi pushed a commit to branch v2-6-test in repository https://gitbox.apache.org/repos/asf/airflow.git
commit 191daab7a4f9e632e95e506724d053a3a7cf4606 Author: Jarek Potiuk <[email protected]> AuthorDate: Wed Jul 5 21:39:27 2023 +0200 Hide sensitive values from extra in connection edit form (#32309) The fields that are sensitive (i.e password field is used to show them in Connection edit view) should also be hidden when they are stored as "extra" in the form extra field. This PR handles both - replacing such values with a placeholder as well as not updating the value if the placeholder has not been modified. (cherry picked from commit d01248382fe45a5f5a7fdeed4082a80c5f814ad8) --- airflow/providers_manager.py | 8 +++- airflow/www/views.py | 57 ++++++++++++++++++++----- tests/www/views/test_views_connection.py | 71 ++++++++++++++++++++++++++++---- 3 files changed, 117 insertions(+), 19 deletions(-) diff --git a/airflow/providers_manager.py b/airflow/providers_manager.py index dd214c755c..cbba074750 100644 --- a/airflow/providers_manager.py +++ b/airflow/providers_manager.py @@ -215,6 +215,7 @@ class ConnectionFormWidgetInfo(NamedTuple): package_name: str field: Any field_name: str + is_sensitive: bool T = TypeVar("T", bound=Callable) @@ -873,7 +874,12 @@ class ProvidersManager(LoggingMixin, metaclass=Singleton): # In case of inherited hooks this might be happening several times continue self._connection_form_widgets[prefixed_field_name] = ConnectionFormWidgetInfo( - hook_class.__name__, package_name, field, field_identifier + hook_class.__name__, + package_name, + field, + field_identifier, + hasattr(field.field_class.widget, "input_type") + and field.field_class.widget.input_type == "password", ) def _add_customized_fields(self, package_name: str, hook_class: type, customized_fields: dict): diff --git a/airflow/www/views.py b/airflow/www/views.py index 18188d3de5..0e06a1ff29 100644 --- a/airflow/www/views.py +++ b/airflow/www/views.py @@ -87,8 +87,14 @@ from airflow.api.common.mark_tasks import ( from airflow.compat.functools import cached_property from airflow.configuration import AIRFLOW_CONFIG, conf from airflow.datasets import Dataset -from airflow.exceptions import AirflowException, ParamValidationError, RemovedInAirflow3Warning +from airflow.exceptions import ( + AirflowException, + AirflowNotFoundException, + ParamValidationError, + RemovedInAirflow3Warning, +) from airflow.executors.executor_loader import ExecutorLoader +from airflow.hooks.base import BaseHook from airflow.jobs.job import Job from airflow.jobs.scheduler_job_runner import SchedulerJobRunner from airflow.jobs.triggerer_job_runner import TriggererJobRunner @@ -144,6 +150,8 @@ LINECHART_X_AXIS_TICKFORMAT = ( "} else {xLabel = d3.time.format('%H:%M, %d %b')(new Date(parseInt(d)));} return xLabel;}" ) +SENSITIVE_FIELD_PLACEHOLDER = "RATHER_LONG_SENSITIVE_FIELD_PLACEHOLDER" + def truncate_task_duration(task_duration): """ @@ -4420,14 +4428,22 @@ class ConnectionModelView(AirflowModelView): base_order = ("conn_id", "asc") - def _iter_extra_field_names(self) -> Iterator[tuple[str, str]]: + def _iter_extra_field_names_and_sensitivity(self) -> Iterator[tuple[str, str, bool]]: """Iterate through provider-backed connection fields. Note that this cannot be a property (including a cached property) because Flask-Appbuilder attempts to access all members on startup, and using a property would initialize the providers manager too eagerly. + + Returns tuple of: + + * key + * field_name + * whether the field is sensitive """ - return ((k, v.field_name) for k, v in ProvidersManager().connection_form_widgets.items()) + return ( + (k, v.field_name, v.is_sensitive) for k, v in ProvidersManager().connection_form_widgets.items() + ) @property def add_columns(self) -> list[str]: @@ -4440,7 +4456,10 @@ class ConnectionModelView(AirflowModelView): superfuluous checks done by Flask-Appbuilder on startup). """ if self._add_columns is type(self)._add_columns and has_request_context(): - self._add_columns = [*self._add_columns, *(k for k, _ in self._iter_extra_field_names())] + self._add_columns = [ + *self._add_columns, + *(k for k, _, _ in self._iter_extra_field_names_and_sensitivity()), + ] return self._add_columns @property @@ -4454,7 +4473,10 @@ class ConnectionModelView(AirflowModelView): superfuluous checks done by Flask-Appbuilder on startup). """ if self._edit_columns is type(self)._edit_columns and has_request_context(): - self._edit_columns = [*self._edit_columns, *(k for k, _ in self._iter_extra_field_names())] + self._edit_columns = [ + *self._edit_columns, + *(k for k, _, _ in self._iter_extra_field_names_and_sensitivity()), + ] return self._edit_columns @action("muldelete", "Delete", "Are you sure you want to delete selected records?", single=False) @@ -4543,7 +4565,6 @@ class ConnectionModelView(AirflowModelView): """Process form data.""" conn_id = form.data["conn_id"] conn_type = form.data["conn_type"] - # The extra value is the combination of custom fields for this conn_type and the Extra field. # The extra form field with all extra values (including custom fields) is in the form being processed # so we start with those values, and override them with anything in the custom fields. @@ -4568,8 +4589,7 @@ class ConnectionModelView(AirflowModelView): ) del form.extra del extra_json - - for key, field_name in self._iter_extra_field_names(): + for key, field_name, is_sensitive in self._iter_extra_field_names_and_sensitivity(): if key in form.data and key.startswith("extra__"): conn_type_from_extra_field = key.split("__")[1] if conn_type_from_extra_field == conn_type: @@ -4578,8 +4598,21 @@ class ConnectionModelView(AirflowModelView): # value isn't an empty string. if value != "": extra[field_name] = value - if extra.keys(): + sensitive_unchanged_keys = set() + for key, value in extra.items(): + if value == SENSITIVE_FIELD_PLACEHOLDER: + sensitive_unchanged_keys.add(key) + if sensitive_unchanged_keys: + try: + conn = BaseHook.get_connection(conn_id) + except AirflowNotFoundException: + conn = None + for key in sensitive_unchanged_keys: + if conn and conn.extra_dejson.get(key): + extra[key] = conn.extra_dejson.get(key) + else: + del extra[key] form.extra.data = json.dumps(extra) def prefill_form(self, form, pk): @@ -4597,7 +4630,7 @@ class ConnectionModelView(AirflowModelView): logging.warning("extra field for %s is not a dictionary", form.data.get("conn_id", "<unknown>")) return - for field_key, field_name in self._iter_extra_field_names(): + for field_key, field_name, is_sensitive in self._iter_extra_field_names_and_sensitivity(): value = extra_dictionary.get(field_name, "") if not value: @@ -4607,6 +4640,10 @@ class ConnectionModelView(AirflowModelView): if value: field = getattr(form, field_key) field.data = value + if is_sensitive and field_name in extra_dictionary: + extra_dictionary[field_name] = SENSITIVE_FIELD_PLACEHOLDER + # form.data is a property that builds the dictionary from fields so we have to modify the fields + form.extra.data = json.dumps(extra_dictionary) class PluginView(AirflowBaseView): diff --git a/tests/www/views/test_views_connection.py b/tests/www/views/test_views_connection.py index fb6566d936..e57a8d484c 100644 --- a/tests/www/views/test_views_connection.py +++ b/tests/www/views/test_views_connection.py @@ -78,10 +78,29 @@ def test_prefill_form_null_extra(): mock_form.data = {"conn_id": "test", "extra": None, "conn_type": "test"} cmv = ConnectionModelView() - cmv._iter_extra_field_names = mock.Mock(return_value=()) + cmv._iter_extra_field_names_and_sensitivity = mock.Mock(return_value=()) cmv.prefill_form(form=mock_form, pk=1) +def test_prefill_form_sensitive_fields_extra(): + mock_form = mock.Mock() + mock_form.data = { + "conn_id": "test", + "extra": json.dumps({"sensitive_extra": "TEST1", "non_sensitive_extra": "TEST2"}), + "conn_type": "test", + } + + cmv = ConnectionModelView() + cmv._iter_extra_field_names_and_sensitivity = mock.Mock( + return_value=[("sensitive_extra_key", "sensitive_extra", True)] + ) + cmv.prefill_form(form=mock_form, pk=1) + assert json.loads(mock_form.extra.data) == { + "sensitive_extra": "RATHER_LONG_SENSITIVE_FIELD_PLACEHOLDER", + "non_sensitive_extra": "TEST2", + } + + @pytest.mark.parametrize( "extras, expected", [ @@ -105,7 +124,9 @@ def test_prefill_form_backcompat(extras, expected): mock_form.data = {"conn_id": "test", "extra": json.dumps(extras), "conn_type": "test"} cmv = ConnectionModelView() - cmv._iter_extra_field_names = mock.Mock(return_value=[("extra__test__my_param", "my_param")]) + cmv._iter_extra_field_names_and_sensitivity = mock.Mock( + return_value=[("extra__test__my_param", "my_param", False)] + ) cmv.prefill_form(form=mock_form, pk=1) assert mock_form.extra__test__my_param.data == expected @@ -133,7 +154,9 @@ def test_process_form_extras_both(mock_pm_hooks, mock_import_str, field_name): } cmv = ConnectionModelView() - cmv._iter_extra_field_names = mock.Mock(return_value=[("extra__test__custom_field", field_name)]) + cmv._iter_extra_field_names_and_sensitivity = mock.Mock( + return_value=[("extra__test__custom_field", field_name, False)] + ) cmv.process_form(form=mock_form, is_created=True) assert json.loads(mock_form.extra.data) == { field_name: "custom_field_val", @@ -159,7 +182,7 @@ def test_process_form_extras_extra_only(mock_pm_hooks, mock_import_str): } cmv = ConnectionModelView() - cmv._iter_extra_field_names = mock.Mock(return_value=()) + cmv._iter_extra_field_names_and_sensitivity = mock.Mock(return_value=()) cmv.process_form(form=mock_form, is_created=True) assert json.loads(mock_form.extra.data) == {"param2": "param2_val"} @@ -185,10 +208,10 @@ def test_process_form_extras_custom_only(mock_pm_hooks, mock_import_str, field_n } cmv = ConnectionModelView() - cmv._iter_extra_field_names = mock.Mock( + cmv._iter_extra_field_names_and_sensitivity = mock.Mock( return_value=[ - ("extra__test3__custom_field", field_name), - ("extra__test3__custom_bool_field", False), + ("extra__test3__custom_field", field_name, False), + ("extra__test3__custom_bool_field", False, False), ], ) cmv.process_form(form=mock_form, is_created=True) @@ -216,7 +239,9 @@ def test_process_form_extras_updates(mock_pm_hooks, mock_import_str, field_name) } cmv = ConnectionModelView() - cmv._iter_extra_field_names = mock.Mock(return_value=[("extra__test4__custom_field", field_name)]) + cmv._iter_extra_field_names_and_sensitivity = mock.Mock( + return_value=[("extra__test4__custom_field", field_name, False)] + ) cmv.process_form(form=mock_form, is_created=True) if field_name == "custom_field": @@ -228,6 +253,36 @@ def test_process_form_extras_updates(mock_pm_hooks, mock_import_str, field_name) assert json.loads(mock_form.extra.data) == {"extra__test4__custom_field": "custom_field_val4"} [email protected]("airflow.utils.module_loading.import_string") [email protected]("airflow.providers_manager.ProvidersManager.hooks", new_callable=PropertyMock) [email protected]("airflow.www.views.BaseHook") +def test_process_form_extras_updates_sensitive_placeholder_unchanged( + mock_base_hook, mock_pm_hooks, mock_import_str +): + """ + Test the handling of sensitive unchanged field (where placeholder has not been modified). + """ + + # Testing parameters set in both extra and custom fields (connection updates). + mock_form = mock.Mock() + mock_form.data = { + "conn_type": "test4", + "conn_id": "extras_test4", + "extra": '{"sensitive_extra": "RATHER_LONG_SENSITIVE_FIELD_PLACEHOLDER", "extra__custom": "value"}', + } + mock_base_hook.get_connection.return_value = Connection(extra='{"sensitive_extra": "old_value"}') + cmv = ConnectionModelView() + cmv._iter_extra_field_names_and_sensitivity = mock.Mock( + return_value=[("sensitive_extra_key", "sensitive_extra", True)] + ) + cmv.process_form(form=mock_form, is_created=True) + + assert json.loads(mock_form.extra.data) == { + "extra__custom": "value", + "sensitive_extra": "old_value", + } + + def test_duplicate_connection(admin_client): """Test Duplicate multiple connection with suffix""" conn1 = Connection(
