From a3c648a637c5d126a403c83bf269c5687e678682 Mon Sep 17 00:00:00 2001 From: John Chilton Date: Wed, 22 May 2024 14:33:22 -0400 Subject: [PATCH 1/6] Abstractions to cleanup test_user_file_sources test case a bit. --- .../app/managers/test_user_file_sources.py | 26 ++++++++++++------- 1 file changed, 17 insertions(+), 9 deletions(-) diff --git a/test/unit/app/managers/test_user_file_sources.py b/test/unit/app/managers/test_user_file_sources.py index 07c9ec75c68b..abde7960f7d1 100644 --- a/test/unit/app/managers/test_user_file_sources.py +++ b/test/unit/app/managers/test_user_file_sources.py @@ -341,22 +341,17 @@ def test_purge(self, tmp_path): self._init_managers(tmp_path, simple_vault_template(tmp_path)) user_file_source = self._create_instance(SIMPLE_VAULT_CREATE_PAYLOAD) assert "sec1" in user_file_source.secrets - user_vault = self.trans.user_vault - config_secret_key = f"file_source_config/{user_file_source.uuid}/sec1" - assert user_vault.read_secret(config_secret_key) == "foosec" + self._assert_secret_is(user_file_source, "sec1", "foosec") self.manager.purge_instance(self.trans, user_file_source.uuid) - # delete resets it to '' - assert user_vault.read_secret(config_secret_key) == "" + self._assert_secret_absent(user_file_source, "sec1") def test_update_secret(self, tmp_path): self._init_managers(tmp_path, simple_vault_template(tmp_path)) user_file_source = self._create_instance(SIMPLE_VAULT_CREATE_PAYLOAD) - user_vault = self.trans.user_vault - config_secret_key = f"file_source_config/{user_file_source.uuid}/sec1" - assert user_vault.read_secret(config_secret_key) == "foosec" + self._assert_secret_is(user_file_source, "sec1", "foosec") update = UpdateInstanceSecretPayload(secret_name="sec1", secret_value="newvalue") self.manager.modify_instance(self.trans, user_file_source.uuid, update) - assert user_vault.read_secret(config_secret_key) == "newvalue" + self._assert_secret_is(user_file_source, "sec1", "newvalue") def test_status_valid(self, tmp_path): self.init_user_in_database() @@ -454,6 +449,19 @@ def _create_user_file_source(self, template_id="home_directory") -> UserFileSour ) return self._create_instance(create_payload) + def _read_secret(self, user_file_source: UserFileSourceModel, secret_name: str) -> str: + user_vault = self.trans.user_vault + config_secret_key = f"file_source_config/{user_file_source.uuid}/{secret_name}" + return user_vault.read_secret(config_secret_key) + + def _assert_secret_is(self, user_file_source: UserFileSourceModel, secret_name: str, expected_value: str): + assert self._read_secret(user_file_source, secret_name) == expected_value + + def _assert_secret_absent(self, user_file_source: UserFileSourceModel, secret_name: str): + sec_val = self._read_secret(user_file_source, secret_name) + # deleting vs never inserted... + assert sec_val in ["", None] + def _create_instance(self, create_payload: CreateInstancePayload) -> UserFileSourceModel: user_file_source = self.manager.create_instance(self.trans, create_payload) return user_file_source From 77e6df43035993569d0fb3d12bd784ff3e67436c Mon Sep 17 00:00:00 2001 From: John Chilton Date: Wed, 22 May 2024 14:49:47 -0400 Subject: [PATCH 2/6] Fill out missing file source management tests. --- .../testing_multi_version_with_secrets.yml | 34 +++++ .../app/managers/test_user_file_sources.py | 142 ++++++++++++++++-- test/unit/files/test_template_models.py | 1 + 3 files changed, 168 insertions(+), 9 deletions(-) create mode 100644 lib/galaxy/files/templates/examples/testing_multi_version_with_secrets.yml diff --git a/lib/galaxy/files/templates/examples/testing_multi_version_with_secrets.yml b/lib/galaxy/files/templates/examples/testing_multi_version_with_secrets.yml new file mode 100644 index 000000000000..a36482d4fc02 --- /dev/null +++ b/lib/galaxy/files/templates/examples/testing_multi_version_with_secrets.yml @@ -0,0 +1,34 @@ +# a template with multiple versions that could be coherent for unit/integration +# testing upgrading object stores templates +- id: secure_disk + version: 0 + name: Secure Disk + description: Secure Disk Bound to You + secrets: + sec1: + help: This is my test secret. + configuration: + type: posix + root: '/data/secure/{{ user.username }}/{{ secrets.sec1 }}/aftersec' +- id: secure_disk + version: 1 + name: Secure Disk + description: Secure Disk Bound to You + secrets: + sec1: + help: This is my test secret. + sec2: + help: This is my test secret 2. + configuration: + type: posix + root: '/data/secure/{{ user.username }}/{{ secrets.sec1 }}/{{ secrets.sec2 }}' +- id: secure_disk + version: 2 + name: Secure Disk + description: Secure Disk Bound to You + secrets: + sec2: + help: This is my test secret 2. + configuration: + type: posix + root: '/data/secure/{{ user.username }}/newbar/{{ secrets.sec2 }}' diff --git a/test/unit/app/managers/test_user_file_sources.py b/test/unit/app/managers/test_user_file_sources.py index abde7960f7d1..7020921f99f3 100644 --- a/test/unit/app/managers/test_user_file_sources.py +++ b/test/unit/app/managers/test_user_file_sources.py @@ -3,15 +3,25 @@ cast, List, Optional, + Type, ) +from yaml import safe_load + +from galaxy.exceptions import ( + RequestParameterInvalidException, + RequestParameterMissingException, +) from galaxy.files import FileSourcesUserContext from galaxy.files.templates import ConfiguredFileSourceTemplates +from galaxy.files.templates.examples import get_example from galaxy.managers.file_source_instances import ( CreateInstancePayload, FileSourceInstancesManager, + ModifyInstancePayload, UpdateInstancePayload, UpdateInstanceSecretPayload, + UpgradeInstancePayload, USER_FILE_SOURCES_SCHEME, UserDefinedFileSourcesConfig, UserDefinedFileSourcesImpl, @@ -165,6 +175,16 @@ def simple_vault_template(tmp_path): variables={}, secrets={"sec1": "foosec"}, ) +UPGRADE_EXAMPLE = get_example("testing_multi_version_with_secrets.yml") +UPGRADE_INITIAL_PAYLOAD = CreateInstancePayload( + name="My Upgradable Disk", + template_id="secure_disk", + template_version=0, + secrets={ + "sec1": "moocow", + }, + variables={}, +) class TestFileSourcesTestCase(BaseTestCase): @@ -206,14 +226,15 @@ def test_to_dict_filters_hidden(self, tmp_path): user_file_source = self._create_user_file_source() assert user_file_source.uri_root in self._uri_roots() hide = UpdateInstancePayload(hidden=True) - self.manager.modify_instance(self.trans, user_file_source.uuid, hide) + self._modify(user_file_source, hide) + assert user_file_source.uri_root not in self._uri_roots() def test_find_best_match_not_filters_hidden(self, tmp_path): self.init_user_in_database() self._init_managers(tmp_path) user_file_source = self._create_user_file_source() hide = UpdateInstancePayload(hidden=True) - self.manager.modify_instance(self.trans, user_file_source.uuid, hide) + self._modify(user_file_source, hide) match = self.file_sources.find_best_match(user_file_source.uri_root) assert match @@ -221,8 +242,8 @@ def test_find_best_match_filters_deactivated(self, tmp_path): self.init_user_in_database() self._init_managers(tmp_path) user_file_source = self._create_user_file_source() - hide = UpdateInstancePayload(active=False) - self.manager.modify_instance(self.trans, user_file_source.uuid, hide) + deactivate = UpdateInstancePayload(active=False) + self._modify(user_file_source, deactivate) match = self.file_sources.find_best_match(user_file_source.uri_root) assert not match @@ -301,7 +322,7 @@ def test_simple_update(self, tmp_path): ) # assert response includes updated variable as well as a fresh show() - response = self.manager.modify_instance(self.trans, user_file_source.uuid, update) + response = self._modify(user_file_source, update) assert response.variables assert response.variables["var1"] == "newval" @@ -316,7 +337,7 @@ def test_hide(self, tmp_path): assert not user_file_source_showed.hidden hide = UpdateInstancePayload(hidden=True) - self.manager.modify_instance(self.trans, user_file_source.uuid, hide) + self._modify(user_file_source, hide) user_file_source_showed = self.manager.show(self.trans, user_file_source.uuid) assert user_file_source_showed.hidden @@ -330,7 +351,7 @@ def test_deactivate(self, tmp_path): assert not user_file_source_showed.purged deactivate = UpdateInstancePayload(active=False) - self.manager.modify_instance(self.trans, user_file_source.uuid, deactivate) + self._modify(user_file_source, deactivate) user_file_source_showed = self.manager.show(self.trans, user_file_source.uuid) assert user_file_source_showed.hidden @@ -350,9 +371,88 @@ def test_update_secret(self, tmp_path): user_file_source = self._create_instance(SIMPLE_VAULT_CREATE_PAYLOAD) self._assert_secret_is(user_file_source, "sec1", "foosec") update = UpdateInstanceSecretPayload(secret_name="sec1", secret_value="newvalue") - self.manager.modify_instance(self.trans, user_file_source.uuid, update) + self._modify(user_file_source, update) self._assert_secret_is(user_file_source, "sec1", "newvalue") + def test_cannot_update_invalid_secret(self, tmp_path): + self._init_managers(tmp_path, simple_vault_template(tmp_path)) + user_file_source = self._create_instance(SIMPLE_VAULT_CREATE_PAYLOAD) + update = UpdateInstanceSecretPayload(secret_name="undefinedsec", secret_value="newvalue") + self._assert_modify_throws_exception(user_file_source, update, RequestParameterInvalidException) + + def test_upgrade(self, tmp_path): + user_file_source = self._init_upgrade_test_case(tmp_path) + assert "sec1" in user_file_source.secrets + assert "sec2" not in user_file_source.secrets + self._assert_secret_is(user_file_source, "sec1", "moocow") + self._assert_secret_absent(user_file_source, "foobarxyz") + self._assert_secret_absent(user_file_source, "sec2") + upgrade_to_1 = UpgradeInstancePayload( + template_version=1, + secrets={ + "sec1": "moocow", + "sec2": "aftersec2", + }, + variables={}, + ) + user_file_source = self._modify(user_file_source, upgrade_to_1) + assert "sec1" in user_file_source.secrets + assert "sec2" in user_file_source.secrets + self._assert_secret_is(user_file_source, "sec1", "moocow") + self._assert_secret_is(user_file_source, "sec2", "aftersec2") + upgrade_to_2 = UpgradeInstancePayload( + template_version=2, + secrets={}, + variables={}, + ) + + user_file_source = self._modify(user_file_source, upgrade_to_2) + assert "sec1" not in user_file_source.secrets + assert "sec2" in user_file_source.secrets + self._assert_secret_absent(user_file_source, "sec1") + self._assert_secret_is(user_file_source, "sec2", "aftersec2") + + def test_upgrade_does_not_allow_extra_variables(self, tmp_path): + user_object_store = self._init_upgrade_test_case(tmp_path) + upgrade_to_1 = UpgradeInstancePayload( + template_version=1, + variables={ + "extra_variable": "moocow", + }, + secrets={ + "sec1": "moocow", + "sec2": "aftersec2", + }, + ) + + self._assert_modify_throws_exception(user_object_store, upgrade_to_1, RequestParameterInvalidException) + + def test_upgrade_does_not_allow_extra_secrets(self, tmp_path): + user_object_store = self._init_upgrade_test_case(tmp_path) + upgrade_to_1 = UpgradeInstancePayload( + template_version=1, + variables={}, + secrets={ + "sec1": "moocow", + "sec2": "aftersec2", + "extrasec": "moo345", + }, + ) + + self._assert_modify_throws_exception(user_object_store, upgrade_to_1, RequestParameterInvalidException) + + def test_upgrade_fails_if_new_secrets_absent(self, tmp_path): + user_object_store = self._init_upgrade_test_case(tmp_path) + upgrade_to_1 = UpgradeInstancePayload( + template_version=1, + variables={}, + secrets={ + "sec1": "moocow", + }, + ) + + self._assert_modify_throws_exception(user_object_store, upgrade_to_1, RequestParameterMissingException) + def test_status_valid(self, tmp_path): self.init_user_in_database() self._init_managers(tmp_path) @@ -433,6 +533,14 @@ def test_status_invalid_settings_configuration_validation(self, tmp_path): assert "Input should be a valid boolean" in status.template_settings.message assert status.connection is None + def _init_upgrade_test_case(self, tmp_path) -> UserFileSourceModel: + example_yaml_str = UPGRADE_EXAMPLE + example_yaml_str.replace("/data", str(tmp_path)) + config = safe_load(example_yaml_str) + self._init_managers(tmp_path, config) + user_file_source = self._create_instance(UPGRADE_INITIAL_PAYLOAD) + return user_file_source + def _init_and_create_simple(self, tmp_path) -> UserFileSourceModel: self._init_managers(tmp_path) user_file_source = self._create_user_file_source() @@ -462,6 +570,19 @@ def _assert_secret_absent(self, user_file_source: UserFileSourceModel, secret_na # deleting vs never inserted... assert sec_val in ["", None] + def _assert_modify_throws_exception( + self, user_file_source: UserFileSourceModel, modify: ModifyInstancePayload, exception_type: Type + ): + exception_thrown = False + try: + self._modify(user_file_source, modify) + except exception_type: + exception_thrown = True + assert exception_thrown + + def _modify(self, user_file_source: UserFileSourceModel, modify: ModifyInstancePayload) -> UserFileSourceModel: + return self.manager.modify_instance(self.trans, user_file_source.uuid, modify) + def _create_instance(self, create_payload: CreateInstancePayload) -> UserFileSourceModel: user_file_source = self.manager.create_instance(self.trans, create_payload) return user_file_source @@ -475,7 +596,10 @@ def _init_managers(self, tmp_path, config_dict=None): self.app.setup_test_vault() if config_dict is None: config_dict = home_directory_template(tmp_path) - templates_config = Config([config_dict]) + if isinstance(config_dict, dict): + templates_config = Config([config_dict]) + else: + templates_config = Config(config_dict) templates = ConfiguredFileSourceTemplates.from_app_config( templates_config, vault_configured=True, diff --git a/test/unit/files/test_template_models.py b/test/unit/files/test_template_models.py index 7fced1b7f4bb..9d11069fc01d 100644 --- a/test/unit/files/test_template_models.py +++ b/test/unit/files/test_template_models.py @@ -299,6 +299,7 @@ def test_examples_parse(): _assert_example_parses("templating_override.yml") _assert_example_parses("admin_secrets.yml") _assert_example_parses("admin_secrets_with_defaults.yml") + _assert_example_parses("testing_multi_version_with_secrets.yml") def _assert_example_parses(filename: str): From 39f126bfc388063181dc2fd5f687b7741c1095fc Mon Sep 17 00:00:00 2001 From: John Chilton Date: Wed, 22 May 2024 09:46:02 -0400 Subject: [PATCH 3/6] Fix API strings. --- lib/galaxy/webapps/galaxy/api/file_sources.py | 2 +- lib/galaxy/webapps/galaxy/api/object_store.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/galaxy/webapps/galaxy/api/file_sources.py b/lib/galaxy/webapps/galaxy/api/file_sources.py index 94e111242a05..b72571373d7b 100644 --- a/lib/galaxy/webapps/galaxy/api/file_sources.py +++ b/lib/galaxy/webapps/galaxy/api/file_sources.py @@ -86,7 +86,7 @@ def instance_index( @router.get( "/api/file_source_instances/{user_file_source_id}", - summary="Get a list of persisted file source instances defined by the requesting user.", + summary="Get a persisted user file source instance.", operation_id="file_sources__instances_get", ) def instances_show( diff --git a/lib/galaxy/webapps/galaxy/api/object_store.py b/lib/galaxy/webapps/galaxy/api/object_store.py index deb3e79b3dc7..f028c963018e 100644 --- a/lib/galaxy/webapps/galaxy/api/object_store.py +++ b/lib/galaxy/webapps/galaxy/api/object_store.py @@ -124,7 +124,7 @@ def instance_index( @router.get( "/api/object_store_instances/{user_object_store_id}", - summary="Get a persisted object store instances owned by the requesting user.", + summary="Get a persisted user object store instance.", operation_id="object_stores__instances_get", ) def instances_show( From 8d303f41ab0fbb00ea7143e343754cd37e6d5e06 Mon Sep 17 00:00:00 2001 From: John Chilton Date: Thu, 23 May 2024 10:39:53 -0400 Subject: [PATCH 4/6] Bug fix for arrow. --- client/src/components/ConfigTemplates/InstanceDropdown.vue | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client/src/components/ConfigTemplates/InstanceDropdown.vue b/client/src/components/ConfigTemplates/InstanceDropdown.vue index 3314f58300e8..da85c8ee60ff 100644 --- a/client/src/components/ConfigTemplates/InstanceDropdown.vue +++ b/client/src/components/ConfigTemplates/InstanceDropdown.vue @@ -42,7 +42,7 @@ const emit = defineEmits<{ :href="routeUpgrade" @keypress="router.push(routeUpgrade)" @click.prevent="router.push(routeUpgrade)"> - + Upgrade