From 937645c187c0bf00d961fa119e60a23f144e8fe8 Mon Sep 17 00:00:00 2001 From: davelopez <46503462+davelopez@users.noreply.github.com> Date: Thu, 13 Feb 2025 18:40:26 +0100 Subject: [PATCH 1/4] Add integration tests for non-vault user preferences handling There is a bug in the user preferences handling when the user preferences contains fields defined as "secret" but the field is not stored in the vault. This commit adds integration tests to cover this case. --- test/integration/test_vault_extra_prefs.py | 83 +++++++++++++++++++ .../user_preferences_extra_conf.yml | 12 +++ 2 files changed, 95 insertions(+) diff --git a/test/integration/test_vault_extra_prefs.py b/test/integration/test_vault_extra_prefs.py index e3ec8fd5c7f1..34f406ec1ee0 100644 --- a/test/integration/test_vault_extra_prefs.py +++ b/test/integration/test_vault_extra_prefs.py @@ -129,6 +129,89 @@ def test_extra_prefs_vault_storage_update_secret(self): == "an_updated_secret_value" ) + def test_extra_prefs_secret_not_in_vault(self): + user = self._setup_user(TEST_USER_EMAIL) + url = self.__url("information/inputs", user) + app = cast(Any, self._test_driver.app if self._test_driver else None) + db_user = self._get_dbuser(app, user) + + # create some initial data + put( + url, + data=json.dumps( + { + "non_vault_test_section|user": "test_user", + "non_vault_test_section|pass": "test_pass", + } + ), + ) + + # retrieve saved data + response = get(url).json() + + def get_input_by_name(inputs, name): + return [input for input in inputs if input["name"] == name][0] + + inputs = [section for section in response["inputs"] if section["name"] == "non_vault_test_section"][0]["inputs"] + + # value should be what we saved + input_user = get_input_by_name(inputs, "user") + assert input_user["value"] == "test_user" + + # the secret should not be stored in the vault + assert app.vault.read_secret(f"user/{db_user.id}/preferences/non_vault_test_section/pass") is None + # it should be in the user preferences model + app.model.session.refresh(db_user) + assert db_user.extra_preferences["non_vault_test_section|pass"] == "test_pass" + + # secret type values should not be retrievable by the client + input_pass = get_input_by_name(inputs, "pass") + assert input_pass["value"] != "test_pass" + assert input_pass["value"] == "__SECRET_PLACEHOLDER__" + + # changing the text property value should not change the secret property value + put( + url, + data=json.dumps( + { + "non_vault_test_section|user": "a_new_test_user", + "non_vault_test_section|pass": "__SECRET_PLACEHOLDER__", + } + ), + ) + + response = get(url).json() + inputs = [section for section in response["inputs"] if section["name"] == "non_vault_test_section"][0]["inputs"] + input_user = get_input_by_name(inputs, "user") + assert input_user["value"] == "a_new_test_user" + # the secret value is not exposed to the client + input_pass = get_input_by_name(inputs, "pass") + assert input_pass["value"] == "__SECRET_PLACEHOLDER__" + + # The secret value should not have changed in the internal user preferences model + app.model.session.refresh(db_user) + assert db_user.extra_preferences["non_vault_test_section|pass"] == "test_pass" + + # changing the secret value should update the secret value + put( + url, + data=json.dumps( + { + "non_vault_test_section|user": "a_new_test_user", + "non_vault_test_section|pass": "a_new_test_pass", + } + ), + ) + + response = get(url).json() + inputs = [section for section in response["inputs"] if section["name"] == "non_vault_test_section"][0]["inputs"] + input_pass = get_input_by_name(inputs, "pass") + assert input_pass["value"] == "__SECRET_PLACEHOLDER__" + + # the secret value should be updated in the internal user preferences model + app.model.session.refresh(db_user) + assert db_user.extra_preferences["non_vault_test_section|pass"] == "a_new_test_pass" + def __url(self, action, user): return self._api_url(f"users/{user['id']}/{action}", params=dict(key=self.master_api_key)) diff --git a/test/integration/user_preferences_extra_conf.yml b/test/integration/user_preferences_extra_conf.yml index b678edeed82c..9fe29b8f22a7 100644 --- a/test/integration/user_preferences_extra_conf.yml +++ b/test/integration/user_preferences_extra_conf.yml @@ -21,3 +21,15 @@ preferences: type: secret store: vault required: True + + non_vault_test_section: + description: For testing Settings + inputs: + - name: user + label: User + type: text + required: True + - name: pass + label: Pass + type: secret # It's a secret but not stored in vault + required: True From c719f5ade2cc2d2bcee299a0bc893d13674a5981 Mon Sep 17 00:00:00 2001 From: davelopez <46503462+davelopez@users.noreply.github.com> Date: Thu, 13 Feb 2025 18:44:42 +0100 Subject: [PATCH 2/4] Fix user preferences secret without vault When the secret is not stored in the vault, the secret was lost when the user updated their preferences. --- lib/galaxy/webapps/galaxy/api/users.py | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/lib/galaxy/webapps/galaxy/api/users.py b/lib/galaxy/webapps/galaxy/api/users.py index f3139f93c9bb..2edc925a03f7 100644 --- a/lib/galaxy/webapps/galaxy/api/users.py +++ b/lib/galaxy/webapps/galaxy/api/users.py @@ -962,6 +962,7 @@ def set_information(self, trans, id, payload=None, **kwd): extra_user_pref_data = dict() extra_pref_keys = self._get_extra_user_preferences(trans) user_vault = UserVaultWrapper(trans.app.vault, user) + current_extra_user_pref_data = json.loads(user.preferences.get("extra_user_preferences", "{}")) if extra_pref_keys is not None: for key in extra_pref_keys: key_prefix = f"{key}|" @@ -974,8 +975,17 @@ def set_information(self, trans, id, payload=None, **kwd): input = matching_input[0] if input.get("required") and payload[item] == "": raise exceptions.ObjectAttributeMissingException("Please fill the required field") - if not (input.get("type") == "secret" and payload[item] == "__SECRET_PLACEHOLDER__"): - if input.get("store") == "vault": + input_type = input.get("type") + is_secret_value_unchanged = ( + input_type == "secret" and payload[item] == "__SECRET_PLACEHOLDER__" + ) + is_stored_in_vault = input.get("store") == "vault" + if is_secret_value_unchanged: + if not is_stored_in_vault: + # If the value is unchanged, keep the current value + extra_user_pref_data[item] = current_extra_user_pref_data.get(item, "") + else: + if is_stored_in_vault: user_vault.write_secret(f"preferences/{keys[0]}/{keys[1]}", str(payload[item])) else: extra_user_pref_data[item] = payload[item] From 18312cfc69de585002229f0484931f760a311283 Mon Sep 17 00:00:00 2001 From: Nicola Soranzo Date: Sat, 15 Feb 2025 13:34:41 +0000 Subject: [PATCH 3/4] Fix CondaSearch.get_json() to skip all header lines Fix the following `test_conda_search` failure: ``` ______________________________ test_conda_search _______________________________ @external_dependency_management @skip_unless_executable("conda") def test_conda_search(): t = CondaSearch("bioconda") search1 = t.get_json("asdfasdf") search2 = t.get_json("bioconductor-gosemsim") assert search1 == [] > assert all(r["package"] == "bioconductor-gosemsim" for r in search2) E assert False E + where False = all(. at 0x7f5be7fc34a0>) search1 = [] search2 = [{'build': 'Version', 'package': '#', 'version': 'Name'}, {'build': '0', 'package': 'bioconductor-gosemsim', 'version': '1.28.2'}, {'build': 'r3.3.1_0', 'package': 'bioconductor-gosemsim', 'version': '1.29.0'}, {'build': 'r3.3.1_0', 'package': 'bioconductor-gosemsim', 'version': '1.30.3'}, ... {'build': 'r43hf17093f_0', 'package': 'bioconductor-gosemsim', 'version': '2.28.0'}, {'build': 'r43hf17093f_1', 'package': 'bioconductor-gosemsim', 'version': '2.28.0'}, {'build': 'r44he5774e6_0', 'package': 'bioconductor-gosemsim', 'version': '2.32.0'}] ``` --- lib/galaxy/tool_util/deps/mulled/mulled_search.py | 15 +++++++++++++-- test/unit/tool_util/mulled/test_mulled_search.py | 8 ++++---- 2 files changed, 17 insertions(+), 6 deletions(-) diff --git a/lib/galaxy/tool_util/deps/mulled/mulled_search.py b/lib/galaxy/tool_util/deps/mulled/mulled_search.py index adb9cb91fc1b..eb97670b4a91 100755 --- a/lib/galaxy/tool_util/deps/mulled/mulled_search.py +++ b/lib/galaxy/tool_util/deps/mulled/mulled_search.py @@ -5,6 +5,10 @@ import logging import sys import tempfile +from typing import ( + Dict, + List, +) import requests @@ -123,7 +127,7 @@ class CondaSearch: def __init__(self, channel): self.channel = channel - def get_json(self, search_string): + def get_json(self, search_string) -> List[Dict[str, str]]: """ Function takes search_string variable and returns results from the bioconda channel in JSON format @@ -136,8 +140,15 @@ def get_json(self, search_string): except Exception as e: logging.info(f"Search failed with: {e}") return [] + header_found = False + lines_fields: List[List[str]] = [] + for line in raw_out.splitlines(): + if line.startswith("#"): + header_found = True + elif header_found: + lines_fields.append(line.split()) return [ - {"package": n.split()[0], "version": n.split()[1], "build": n.split()[2]} for n in raw_out.split("\n")[2:-1] + {"package": line_fields[0], "version": line_fields[1], "build": line_fields[2]} for line_fields in lines_fields ] diff --git a/test/unit/tool_util/mulled/test_mulled_search.py b/test/unit/tool_util/mulled/test_mulled_search.py index dd17ab79d0ee..8fc0868eda28 100644 --- a/test/unit/tool_util/mulled/test_mulled_search.py +++ b/test/unit/tool_util/mulled/test_mulled_search.py @@ -25,10 +25,10 @@ def test_quay_search(): @skip_unless_executable("conda") def test_conda_search(): t = CondaSearch("bioconda") - search1 = t.get_json("asdfasdf") - search2 = t.get_json("bioconductor-gosemsim") - assert search1 == [] - assert all(r["package"] == "bioconductor-gosemsim" for r in search2) + search = t.get_json("asdfasdf") + assert search == [] + search = t.get_json("bioconductor-gosemsim") + assert all(r["package"] == "bioconductor-gosemsim" for r in search) @external_dependency_management From 0c09d02dbb356de52a4511e4c1ec393c4687159e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Manuel=20Dom=C3=ADnguez?= Date: Wed, 5 Feb 2025 14:52:40 +0100 Subject: [PATCH 4/4] Fix `test_mulled_build.py::test_mulled_build_files_cli` with `use_mamba=True` Test `tests/tool_util/mulled/test_mulled_build.py::test_mulled_build_files_cli[True]`, where `[True]` refers to the parameter `use_mamba`, fails because if `conda install --quiet --yes 'mamba='` (the preinstall command) runs before `mamba install -p /usr/local`, then the latter expects either `/usr/local` not to exist or to be an existing environment. To work this around, create an environment in `/usr/local/env`, but still put it on the expected location `/usr/local` later. --- lib/galaxy/tool_util/deps/mulled/invfile.lua | 5 +++-- lib/galaxy/tool_util/deps/mulled/mulled_search.py | 3 ++- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/lib/galaxy/tool_util/deps/mulled/invfile.lua b/lib/galaxy/tool_util/deps/mulled/invfile.lua index f7bef84d18b5..fdacf4740ae2 100644 --- a/lib/galaxy/tool_util/deps/mulled/invfile.lua +++ b/lib/galaxy/tool_util/deps/mulled/invfile.lua @@ -88,13 +88,14 @@ inv.task('build') .using(conda_image) .withHostConfig({binds = bind_args}) .run('/bin/sh', '-c', preinstall + .. conda_bin .. ' create --quiet --yes -p /usr/local/env --copy && ' .. conda_bin .. ' install ' .. channel_args .. ' ' .. target_args - .. ' --strict-channel-priority -p /usr/local --copy --yes ' + .. ' --strict-channel-priority -p /usr/local/env --copy --yes ' .. verbose .. postinstall) - .wrap('build/dist') + .wrap('build/dist/env') .at('/usr/local') .inImage(destination_base_image) .as(repo) diff --git a/lib/galaxy/tool_util/deps/mulled/mulled_search.py b/lib/galaxy/tool_util/deps/mulled/mulled_search.py index eb97670b4a91..fe1501af2431 100755 --- a/lib/galaxy/tool_util/deps/mulled/mulled_search.py +++ b/lib/galaxy/tool_util/deps/mulled/mulled_search.py @@ -148,7 +148,8 @@ def get_json(self, search_string) -> List[Dict[str, str]]: elif header_found: lines_fields.append(line.split()) return [ - {"package": line_fields[0], "version": line_fields[1], "build": line_fields[2]} for line_fields in lines_fields + {"package": line_fields[0], "version": line_fields[1], "build": line_fields[2]} + for line_fields in lines_fields ]