Improvements to tool test error handling.

- Add integration test for invalid admin key.
- Add tool test to ensure error message is valid JSON.
- Swap order of require_admin and expose_api - expose_api sets environment so require_admin knows it should return JSON and not HTML. This is the bug fix.

lib/galaxy/tool_util/verify/interactor.py

@@ -429,7 +429,12 @@ def publish_history(self, history_id: str) -> None:
     def test_data_path(self, tool_id, filename, tool_version=None):
         version_fragment = f"&tool_version={tool_version}" if tool_version else ""
         response = self._get(f"tools/{tool_id}/test_data_path?filename={filename}{version_fragment}", admin=True)
-        result = response.json()
+        try:
+            result = response.json()
+        except Exception as e:
+            raise Exception(
+                f"Failed to parse test_data_path from Galaxy. An admin key is required for this feature and it is probably not set or not a valid admin key. Status Code: {response.status_code}. Response: {response.text}."
+            )
         if response.status_code in [200, 404]:
             return result
         raise Exception(result["err_msg"])

lib/galaxy/webapps/galaxy/api/tools.py

@@ -249,8 +249,8 @@ def build(self, trans: GalaxyWebTransaction, id, **kwd):
         tool = self.service._get_tool(trans, id, tool_version=tool_version, user=trans.user)
         return tool.to_json(trans, kwd.get("inputs", kwd), history=history)
 
-    @web.require_admin
     @expose_api
+    @web.require_admin
     def test_data_path(self, trans: GalaxyWebTransaction, id, **kwd):
         """
         GET /api/tools/{tool_id}/test_data_path?tool_version={tool_version}
@@ -347,8 +347,8 @@ def test_data(self, trans: GalaxyWebTransaction, id, **kwd) -> List[ToolTestDesc
             test_defs.extend([t.to_dict() for t in tool.tests])
         return test_defs
 
-    @web.require_admin
     @expose_api
+    @web.require_admin
     def reload(self, trans: GalaxyWebTransaction, id, **kwd):
         """
         GET /api/tools/{tool_id}/reload
@@ -360,8 +360,8 @@ def reload(self, trans: GalaxyWebTransaction, id, **kwd):
             raise exceptions.MessageException(message)
         return {"message": message}
 
-    @web.require_admin
     @expose_api
+    @web.require_admin
     def all_requirements(self, trans: GalaxyWebTransaction, **kwds):
         """
         GET /api/tools/all_requirements
@@ -370,8 +370,8 @@ def all_requirements(self, trans: GalaxyWebTransaction, **kwds):
 
         return trans.app.toolbox.all_requirements
 
-    @web.require_admin
     @expose_api
+    @web.require_admin
     def requirements(self, trans: GalaxyWebTransaction, id, **kwds):
         """
         GET /api/tools/{tool_id}/requirements
@@ -381,8 +381,8 @@ def requirements(self, trans: GalaxyWebTransaction, id, **kwds):
         tool = self.service._get_tool(trans, id, user=trans.user)
         return tool.tool_requirements_status
 
-    @web.require_admin
     @expose_api
+    @web.require_admin
     def install_dependencies(self, trans: GalaxyWebTransaction, id, **kwds):
         """
         POST /api/tools/{tool_id}/dependencies
@@ -409,8 +409,8 @@ def install_dependencies(self, trans: GalaxyWebTransaction, id, **kwds):
         # _view.install_dependencies should return a dict with stdout, stderr and success status
         return tool.tool_requirements_status
 
-    @web.require_admin
     @expose_api
+    @web.require_admin
     def uninstall_dependencies(self, trans: GalaxyWebTransaction, id, **kwds):
         """
         DELETE /api/tools/{tool_id}/dependencies
@@ -431,8 +431,8 @@ def uninstall_dependencies(self, trans: GalaxyWebTransaction, id, **kwds):
         # TODO: rework resolver install system to log and report what has been done.
         return tool.tool_requirements_status
 
-    @web.require_admin
     @expose_api
+    @web.require_admin
     def build_dependency_cache(self, trans: GalaxyWebTransaction, id, **kwds):
         """
         POST /api/tools/{tool_id}/build_dependency_cache
@@ -446,8 +446,8 @@ def build_dependency_cache(self, trans: GalaxyWebTransaction, id, **kwds):
         # TODO: Should also have a more meaningful return.
         return tool.tool_requirements_status
 
-    @web.require_admin
     @expose_api
+    @web.require_admin
     def diagnostics(self, trans: GalaxyWebTransaction, id, **kwd):
         """
         GET /api/tools/{tool_id}/diagnostics
@@ -563,8 +563,8 @@ def raw_tool_source(self, trans: GalaxyWebTransaction, id, **kwds):
         trans.response.headers["language"] = tool.tool_source.language
         return tool.tool_source.to_string()
 
-    @web.require_admin
     @expose_api
+    @web.require_admin
     def error_stack(self, trans: GalaxyWebTransaction, **kwd):
         """
         GET /api/tools/error_stack

lib/galaxy_test/api/test_tools.py

@@ -16,11 +16,13 @@
     put,
 )
 
+from galaxy.exceptions import error_codes
 from galaxy.tool_util.verify.interactor import ValidToolTestDict
 from galaxy.util import galaxy_root_path
 from galaxy.util.unittest_utils import skip_if_github_down
 from galaxy_test.base import rules_test_data
 from galaxy_test.base.api_asserts import (
+    assert_error_code_is,
     assert_has_keys,
     assert_status_code_is,
 )
@@ -388,7 +390,10 @@ def test_test_data_filepath_security(self):
     @skip_without_tool("composite_output")
     def test_test_data_admin_security(self):
         test_data_response = self._get("tools/composite_output/test_data_path?filename=../CONTRIBUTORS.md")
-        assert test_data_response.status_code == 403, test_data_response.text
+        assert_status_code_is(test_data_response, 403)
+        error_json = test_data_response.json()
+        assert_has_keys(error_json, "err_msg", "err_code")
+        assert_error_code_is(error_json, error_codes.error_codes_by_name["ADMIN_REQUIRED"].code)
 
     @skip_without_tool("dbkey_filter_multi_input")
     def test_data_table_requirement_annotated(self):

lib/galaxy_test/base/api.py

@@ -147,7 +147,7 @@ def _setup_user_get_key(self, email, password=None):
         return user, self._post(f"users/{user['id']}/api_key", admin=True).json()
 
     @contextmanager
-    def _different_user(self, email=OTHER_USER, anon=False):
+    def _different_user(self, email=OTHER_USER, anon=False, invalid_admin_key=False):
         """Use in test cases to switch get/post operations to act as new user
 
         ..code-block:: python
@@ -157,6 +157,7 @@ def _different_user(self, email=OTHER_USER, anon=False):
 
         """
         original_api_key = self.user_api_key
+        original_admin_api_key = self.master_api_key
         original_interactor_key = self.galaxy_interactor.api_key
         original_cookies = self.galaxy_interactor.cookies
         if anon:
@@ -168,10 +169,12 @@ def _different_user(self, email=OTHER_USER, anon=False):
         try:
             self.user_api_key = new_key
             self.galaxy_interactor.api_key = new_key
+            self.galaxy_interactor.master_api_key = None if not invalid_admin_key else new_key
             yield
         finally:
             self.user_api_key = original_api_key
             self.galaxy_interactor.api_key = original_interactor_key
+            self.master_api_key = original_admin_api_key
             self.galaxy_interactor.cookies = original_cookies
 
     def _get(self, *args, **kwds):

test/integration/test_galaxy_interactor.py

@@ -15,6 +15,18 @@ def test_local_test_data_download(self):
             b"chr1\t147962192\t147962580"
         )
 
+    def test_data_path_error_message(self):
+        with self._different_user(invalid_admin_key=True):  # other user is not admin, attempt to use their key anyway
+            exc = None
+            try:
+                data_path = self.galaxy_interactor.test_data_path("cat1", "1.bed")
+                print(data_path)
+                assert not data_path
+            except Exception as e:
+                exc = e
+            assert exc is not None
+            assert "You must be an administrator" in str(exc)
+
     def test_run_test_select_version(self):
         self._run_tool_test(tool_id="multiple_versions", tool_version="0.1")