Merge pull request #18246 from jmchilton/backport_user_data_1

[24.1] Small bug fixes for user data plugins

client/src/api/schema/schema.ts

@@ -329,7 +329,7 @@ export interface paths {
         post: operations["file_sources__test_new_instance_configuration"];
     };
     "/api/file_source_instances/{user_file_source_id}": {
-        /** Get a list of persisted file source instances defined by the requesting user. */
+        /** Get a persisted user file source instance. */
         get: operations["file_sources__instances_get"];
         /** Update or upgrade user file source instance. */
         put: operations["file_sources__instances_update"];
@@ -1280,7 +1280,7 @@ export interface paths {
         post: operations["object_stores__test_new_instance_configuration"];
     };
     "/api/object_store_instances/{user_object_store_id}": {
-        /** Get a persisted object store instances owned by the requesting user. */
+        /** Get a persisted user object store instance. */
         get: operations["object_stores__instances_get"];
         /** Update or upgrade user object store instance. */
         put: operations["object_stores__instances_update"];
@@ -15189,7 +15189,7 @@ export interface operations {
         };
     };
     file_sources__instances_get: {
-        /** Get a list of persisted file source instances defined by the requesting user. */
+        /** Get a persisted user file source instance. */
         parameters: {
             /** @description The user ID that will be used to effectively make this API call. Only admins and designated users can make API calls on behalf of other users. */
             header?: {
@@ -21113,7 +21113,7 @@ export interface operations {
         };
     };
     object_stores__instances_get: {
-        /** Get a persisted object store instances owned by the requesting user. */
+        /** Get a persisted user object store instance. */
         parameters: {
             /** @description The user ID that will be used to effectively make this API call. Only admins and designated users can make API calls on behalf of other users. */
             header?: {

client/src/components/ConfigTemplates/InstanceDropdown.vue

@@ -42,7 +42,7 @@ const emit = defineEmits<{
                 :href="routeUpgrade"
                 @keypress="router.push(routeUpgrade)"
                 @click.prevent="router.push(routeUpgrade)">
-                <FontAwesomeIcon icon="arrowUp" />
+                <FontAwesomeIcon :icon="faArrowUp" />
                 <span v-localize>Upgrade</span>
             </button>
             <button

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 }}'

lib/galaxy/webapps/galaxy/api/file_sources.py

@@ -86,7 +86,7 @@ class FastAPIFileSources:
 
     @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(

lib/galaxy/webapps/galaxy/api/object_store.py

@@ -124,7 +124,7 @@ class FastAPIObjectStore:
 
     @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(

test/unit/app/managers/test_user_file_sources.py

@@ -3,15 +3,25 @@ from typing import (
     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 @@ SIMPLE_VAULT_CREATE_PAYLOAD = CreateInstancePayload(
     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 @@ class TestFileSourcesTestCase(BaseTestCase):
         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 @@ class TestFileSourcesTestCase(BaseTestCase):
         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 @@ class TestFileSourcesTestCase(BaseTestCase):
         )
 
         # 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 @@ class TestFileSourcesTestCase(BaseTestCase):
         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 @@ class TestFileSourcesTestCase(BaseTestCase):
         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
@@ -341,22 +362,96 @@ class TestFileSourcesTestCase(BaseTestCase):
         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._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()
@@ -438,6 +533,14 @@ class TestFileSourcesTestCase(BaseTestCase):
         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()
@@ -454,6 +557,32 @@ class TestFileSourcesTestCase(BaseTestCase):
         )
         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 _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
@@ -467,7 +596,10 @@ class TestFileSourcesTestCase(BaseTestCase):
         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,

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):