Merge pull request #5209 from jmchilton/library_upload_testing

Library Upload Refactoring, Testing, and Fixes

lib/galaxy/actions/library.py

@@ -8,6 +8,11 @@ import os.path
 from markupsafe import escape
 
 from galaxy import util
+from galaxy.exceptions import (
+    AdminRequiredException,
+    ConfigDoesNotAllowException,
+    RequestParameterInvalidException,
+)
 from galaxy.tools.actions import upload_common
 from galaxy.tools.parameters import populate_state
 from galaxy.util.path import (
@@ -19,6 +24,47 @@ from galaxy.util.path import (
 log = logging.getLogger(__name__)
 
 
+def validate_server_directory_upload(trans, server_dir):
+    if server_dir in [None, 'None', '']:
+        raise RequestParameterInvalidException("Invalid or unspecified server_dir parameter")
+
+    if trans.user_is_admin():
+        import_dir = trans.app.config.library_import_dir
+        import_dir_desc = 'library_import_dir'
+        if not import_dir:
+            raise ConfigDoesNotAllowException('"library_import_dir" is not set in the Galaxy configuration')
+    else:
+        import_dir = trans.app.config.user_library_import_dir
+        if not import_dir:
+            raise ConfigDoesNotAllowException('"user_library_import_dir" is not set in the Galaxy configuration')
+        if server_dir != trans.user.email:
+            import_dir = os.path.join(import_dir, trans.user.email)
+        import_dir_desc = 'user_library_import_dir'
+
+    full_dir = os.path.join(import_dir, server_dir)
+    unsafe = None
+    if safe_relpath(server_dir):
+        username = trans.user.username if trans.app.config.user_library_import_check_permissions else None
+        if import_dir_desc == 'user_library_import_dir' and safe_contains(import_dir, full_dir, whitelist=trans.app.config.user_library_import_symlink_whitelist, username=username):
+            for unsafe in unsafe_walk(full_dir, whitelist=[import_dir] + trans.app.config.user_library_import_symlink_whitelist):
+                log.error('User attempted to import a path that resolves to a path outside of their import dir: %s -> %s', unsafe, os.path.realpath(unsafe))
+    else:
+        log.error('User attempted to import a directory path that resolves to a path outside of their import dir: %s -> %s', server_dir, os.path.realpath(full_dir))
+        unsafe = True
+    if unsafe:
+        raise RequestParameterInvalidException("Invalid server_dir specified")
+
+    return full_dir, import_dir_desc
+
+
+def validate_path_upload(trans):
+    if not trans.app.config.allow_library_path_paste:
+        raise ConfigDoesNotAllowException('"allow_path_paste" is not set to True in the Galaxy configuration file')
+
+    if not trans.user_is_admin():
+        raise AdminRequiredException('Uploading files via filesystem paths can only be performed by administrators')
+
+
 class LibraryActions(object):
     """
     Mixin for controllers that provide library functionality.
@@ -42,38 +88,11 @@ class LibraryActions(object):
         upload_option = kwd.get('upload_option', 'upload_file')
         response_code = 200
         if upload_option == 'upload_directory':
-            if server_dir in [None, 'None', '']:
-                response_code = 400
-            if trans.user_is_admin():
-                import_dir = trans.app.config.library_import_dir
-                import_dir_desc = 'library_import_dir'
-            else:
-                import_dir = trans.app.config.user_library_import_dir
-                if server_dir != trans.user.email:
-                    import_dir = os.path.join(import_dir, trans.user.email)
-                import_dir_desc = 'user_library_import_dir'
-            full_dir = os.path.join(import_dir, server_dir)
-            unsafe = None
-            if safe_relpath(server_dir):
-                username = trans.user.username if trans.app.config.user_library_import_check_permissions else None
-                if import_dir_desc == 'user_library_import_dir' and safe_contains(import_dir, full_dir, whitelist=trans.app.config.user_library_import_symlink_whitelist):
-                    for unsafe in unsafe_walk(full_dir, whitelist=[import_dir] + trans.app.config.user_library_import_symlink_whitelist, username=username):
-                        log.error('User attempted to import a path that resolves to a path outside of their import dir: %s -> %s', unsafe, os.path.realpath(unsafe))
-            else:
-                log.error('User attempted to import a directory path that resolves to a path outside of their import dir: %s -> %s', server_dir, os.path.realpath(full_dir))
-                unsafe = True
-            if unsafe:
-                response_code = 403
-                message = 'Invalid server_dir'
-            if import_dir:
-                message = 'Select a directory'
-            else:
-                response_code = 403
-                message = '"%s" is not defined in the Galaxy configuration file' % import_dir_desc
+            full_dir, import_dir_desc = validate_server_directory_upload(trans, server_dir)
+            message = 'Select a directory'
         elif upload_option == 'upload_paths':
-            if not trans.app.config.allow_library_path_paste:
-                response_code = 403
-                message = '"allow_library_path_paste" is not defined in the Galaxy configuration file'
+            # Library API already checked this - following check isn't actually needed.
+            validate_path_upload(trans)
         # Some error handling should be added to this method.
         try:
             # FIXME: instead of passing params here ( which have been processed by util.Params(), the original kwd
@@ -119,6 +138,43 @@ class LibraryActions(object):
                 uploaded_datasets.append(self._make_library_uploaded_dataset(trans, params, name, file, 'server_dir', library_bunch))
             return uploaded_datasets, 200, None
 
+    def _get_server_dir_files(self, params, full_dir, import_dir_desc):
+        files = []
+        try:
+            for entry in os.listdir(full_dir):
+                # Only import regular files
+                path = os.path.join(full_dir, entry)
+                link_data_only = params.get('link_data_only', 'copy_files')
+                if os.path.islink(full_dir) and link_data_only == 'link_to_files':
+                    # If we're linking instead of copying and the
+                    # sub-"directory" in the import dir is actually a symlink,
+                    # dereference the symlink, but not any of its contents.
+                    link_path = os.readlink(full_dir)
+                    if os.path.isabs(link_path):
+                        path = os.path.join(link_path, entry)
+                    else:
+                        path = os.path.abspath(os.path.join(link_path, entry))
+                elif os.path.islink(path) and os.path.isfile(path) and link_data_only == 'link_to_files':
+                    # If we're linking instead of copying and the "file" in the
+                    # sub-directory of the import dir is actually a symlink,
+                    # dereference the symlink (one dereference only, Vasili).
+                    link_path = os.readlink(path)
+                    if os.path.isabs(link_path):
+                        path = link_path
+                    else:
+                        path = os.path.abspath(os.path.join(os.path.dirname(path), link_path))
+                if os.path.isfile(path):
+                    files.append(path)
+        except Exception as e:
+            message = "Unable to get file list for configured %s, error: %s" % (import_dir_desc, str(e))
+            response_code = 500
+            return None, response_code, message
+        if not files:
+            message = "The directory '%s' contains no valid files" % full_dir
+            response_code = 400
+            return None, response_code, message
+        return files, None, None
+
     def _get_path_paste_uploaded_datasets(self, trans, params, library_bunch, response_code, message):
         preserve_dirs = util.string_as_bool(params.get('preserve_dirs', False))
         uploaded_datasets = []

lib/galaxy/webapps/galaxy/api/library_contents.py

@@ -12,9 +12,8 @@ from galaxy import (
     exceptions,
     managers,
     util,
-    web
 )
-from galaxy.actions.library import LibraryActions
+from galaxy.actions.library import LibraryActions, validate_path_upload
 from galaxy.managers.collections_util import (
     api_payload_to_create_params,
     dictify_dataset_collection_instance
@@ -158,7 +157,7 @@ class LibraryContentsController(BaseAPIController, UsesLibraryMixin, UsesLibrary
             rval['parent_library_id'] = trans.security.encode_id(rval['parent_library_id'])
         return rval
 
-    @web.expose_api
+    @expose_api
     def create(self, trans, library_id, payload, **kwd):
         """
         create( self, trans, library_id, payload, **kwd )
@@ -314,12 +313,8 @@ class LibraryContentsController(BaseAPIController, UsesLibraryMixin, UsesLibrary
         if folder and last_used_build in ['None', None, '?']:
             last_used_build = folder.genome_build
         error = False
-        if upload_option == 'upload_paths' and not trans.app.config.allow_library_path_paste:
-            error = True
-            message = '"allow_library_path_paste" is not defined in the Galaxy configuration file'
-        elif upload_option == 'upload_paths' and not is_admin:
-            error = True
-            message = 'Uploading files via filesystem paths can only be performed by administrators'
+        if upload_option == 'upload_paths':
+            validate_path_upload(trans)  # Duplicate check made in _upload_dataset.
         elif upload_option not in ('upload_file', 'upload_directory', 'upload_paths'):
             error = True
             message = 'Invalid upload_option'
@@ -373,7 +368,7 @@ class LibraryContentsController(BaseAPIController, UsesLibraryMixin, UsesLibrary
             # for cross type comparisions, ie "True" == True
             yield prefix, ("%s" % (meta)).encode("utf8", errors='replace')
 
-    @web.expose_api
+    @expose_api
     def update(self, trans, id, library_id, payload, **kwd):
         """
         update( self, trans, id, library_id, payload, **kwd )
@@ -411,7 +406,7 @@ class LibraryContentsController(BaseAPIController, UsesLibraryMixin, UsesLibrary
         else:
             raise HTTPBadRequest('Malformed library content id ( %s ) specified, unable to decode.' % str(content_id))
 
-    @web.expose_api
+    @expose_api
     def delete(self, trans, library_id, id, **kwd):
         """
         delete( self, trans, library_id, id, **kwd )

test/api/test_configuration.py

@@ -29,7 +29,7 @@ class ConfigurationApiTestCase(api.ApiTestCase):
 
     def setUp(self):
         super(ConfigurationApiTestCase, self).setUp()
-        self.library_populator = LibraryPopulator(self)
+        self.library_populator = LibraryPopulator(self.galaxy_interactor)
 
     def test_normal_user_configuration(self):
         config = self._get_configuration()

test/api/test_history_contents.py

@@ -22,7 +22,7 @@ class HistoryContentsApiTestCase(api.ApiTestCase, TestsDatasets):
         self.history_id = self._new_history()
         self.dataset_populator = DatasetPopulator(self.galaxy_interactor)
         self.dataset_collection_populator = DatasetCollectionPopulator(self.galaxy_interactor)
-        self.library_populator = LibraryPopulator(self)
+        self.library_populator = LibraryPopulator(self.galaxy_interactor)
 
     def test_index_hda_summary(self):
         hda1 = self._new_dataset(self.history_id)

test/api/test_libraries.py

@@ -13,7 +13,7 @@ class LibrariesApiTestCase(api.ApiTestCase, TestsDatasets):
         super(LibrariesApiTestCase, self).setUp()
         self.dataset_populator = DatasetPopulator(self.galaxy_interactor)
         self.dataset_collection_populator = DatasetCollectionPopulator(self.galaxy_interactor)
-        self.library_populator = LibraryPopulator(self)
+        self.library_populator = LibraryPopulator(self.galaxy_interactor)
 
     def test_create(self):
         data = dict(name="CreateTestLibrary")

test/api/test_tools.py

@@ -259,7 +259,7 @@ class ToolsTestCase(api.ApiTestCase):
     @skip_without_tool("library_data")
     def test_library_data_param(self):
         with self.dataset_populator.test_history() as history_id:
-            ld = LibraryPopulator(self).new_library_dataset("lda_test_library")
+            ld = LibraryPopulator(self.galaxy_interactor).new_library_dataset("lda_test_library")
             inputs = {
                 "library_dataset": ld["ldda_id"],
                 "library_dataset_multiple": [ld["ldda_id"], ld["ldda_id"]]

test/base/populators.py

@@ -461,9 +461,8 @@ class WorkflowPopulator(BaseWorkflowPopulator, ImporterGalaxyInterface):
 
 class LibraryPopulator(object):
 
-    def __init__(self, api_test_case):
-        self.api_test_case = api_test_case
-        self.galaxy_interactor = api_test_case.galaxy_interactor
+    def __init__(self, galaxy_interactor):
+        self.galaxy_interactor = galaxy_interactor
 
     def new_private_library(self, name):
         library = self.new_library(name)
@@ -500,44 +499,57 @@ class LibraryPopulator(object):
 
     def user_private_role_id(self):
         user_email = self.user_email()
-        roles_response = self.api_test_case.galaxy_interactor.get("roles", admin=True)
+        roles_response = self.galaxy_interactor.get("roles", admin=True)
         users_roles = [r for r in roles_response.json() if r["name"] == user_email]
         assert len(users_roles) == 1
         return users_roles[0]["id"]
 
     def create_dataset_request(self, library, **kwds):
+        upload_option = kwds.get("upload_option", "upload_file")
         create_data = {
             "folder_id": kwds.get("folder_id", library["root_folder_id"]),
             "create_type": "file",
             "files_0|NAME": kwds.get("name", "NewFile"),
-            "upload_option": kwds.get("upload_option", "upload_file"),
+            "upload_option": upload_option,
             "file_type": kwds.get("file_type", "auto"),
             "db_key": kwds.get("db_key", "?"),
         }
-        files = {
-            "files_0|file_data": kwds.get("file", StringIO(kwds.get("contents", "TestData"))),
-        }
+
+        if upload_option == "upload_file":
+            files = {
+                "files_0|file_data": kwds.get("file", StringIO(kwds.get("contents", "TestData"))),
+            }
+        elif upload_option == "upload_paths":
+            create_data["filesystem_paths"] = kwds["paths"]
+            files = {}
+        elif upload_option == "upload_directory":
+            create_data["server_dir"] = kwds["server_dir"]
+            files = {}
+
         return create_data, files
 
     def new_library_dataset(self, name, **create_dataset_kwds):
         library = self.new_private_library(name)
         payload, files = self.create_dataset_request(library, **create_dataset_kwds)
-        url_rel = "libraries/%s/contents" % (library["id"])
-        dataset = self.api_test_case.galaxy_interactor.post(url_rel, payload, files=files).json()[0]
+        dataset = self.raw_library_contents_create(library["id"], payload, files=files).json()[0]
 
         def show():
-            return self.api_test_case.galaxy_interactor.get("libraries/%s/contents/%s" % (library["id"], dataset["id"]))
+            return self.galaxy_interactor.get("libraries/%s/contents/%s" % (library["id"], dataset["id"]))
 
         wait_on_state(show, timeout=DEFAULT_TIMEOUT)
         return show().json()
 
+    def raw_library_contents_create(self, library_id, payload, files={}):
+        url_rel = "libraries/%s/contents" % library_id
+        return self.galaxy_interactor.post(url_rel, payload, files=files)
+
     def show_ldda(self, library_id, library_dataset_id):
-        return self.api_test_case.galaxy_interactor.get("libraries/%s/contents/%s" % (library_id, library_dataset_id))
+        return self.galaxy_interactor.get("libraries/%s/contents/%s" % (library_id, library_dataset_id))
 
     def new_library_dataset_in_private_library(self, library_name="private_dataset", wait=True):
         library = self.new_private_library(library_name)
         payload, files = self.create_dataset_request(library, file_type="txt", contents="create_test")
-        create_response = self.api_test_case.galaxy_interactor.post("libraries/%s/contents" % library["id"], payload, files=files)
+        create_response = self.galaxy_interactor.post("libraries/%s/contents" % library["id"], payload, files=files)
         api_asserts.assert_status_code_is(create_response, 200)
         library_datasets = create_response.json()
         assert len(library_datasets) == 1

test/integration/test_upload_configuration_options.py

@@ -34,6 +34,7 @@ from base.constants import (
 )
 from base.populators import (
     DatasetPopulator,
+    LibraryPopulator,
     skip_without_datatype,
 )
 
@@ -49,6 +50,7 @@ class BaseUploadContentConfigurationTestCase(integration_util.IntegrationTestCas
     def setUp(self):
         super(BaseUploadContentConfigurationTestCase, self).setUp()
         self.dataset_populator = DatasetPopulator(self.galaxy_interactor)
+        self.library_populator = LibraryPopulator(self.galaxy_interactor)
         self.history_id = self.dataset_populator.new_history()
 
 
@@ -85,6 +87,12 @@ class NonAdminsCannotPasteFilePathTestCase(BaseUploadContentConfigurationTestCas
         # the newer API decorator that handles those details.
         assert create_response.status_code >= 400
 
+    def test_disallowed_for_libraries(self):
+        library = self.library_populator.new_private_library("pathpastedisallowedlibraries")
+        payload, files = self.library_populator.create_dataset_request(library, upload_option="upload_paths", paths="%s/1.txt" % TEST_DATA_DIRECTORY)
+        response = self.library_populator.raw_library_contents_create(library["id"], payload, files=files)
+        assert response.status_code == 403, response.json()
+
 
 class AdminsCanPasteFilePathsTestCase(BaseUploadContentConfigurationTestCase):
 
@@ -99,10 +107,16 @@ class AdminsCanPasteFilePathsTestCase(BaseUploadContentConfigurationTestCase):
             self.history_id, 'file://%s/random-file' % TEST_DATA_DIRECTORY,
         )
         create_response = self._post("tools", data=payload)
-        # Ideally this would be 403 but the tool API endpoint isn't using
-        # the newer API decorator that handles those details.
+        # Is admin - so this should work fine!
         assert create_response.status_code == 200
 
+    def test_admin_path_paste_libraries(self):
+        library = self.library_populator.new_private_library("pathpasteallowedlibraries")
+        payload, files = self.library_populator.create_dataset_request(library, upload_option="upload_paths", paths="%s/1.txt" % TEST_DATA_DIRECTORY)
+        response = self.library_populator.raw_library_contents_create(library["id"], payload, files=files)
+        # Was 403 for non-admin above.
+        assert response.status_code == 200
+
 
 class DefaultBinaryContentFiltersTestCase(BaseUploadContentConfigurationTestCase):
 
@@ -414,3 +428,45 @@ class UploadOptionsFtpUploadConfigurationTestCase(BaseFtpUploadConfigurationTest
 
     def _write_user_ftp_file(self, path, content):
         return self._write_ftp_file(os.path.join(self.ftp_dir(), TEST_USER), content, filename=path)
+
+
+class ServerDirectoryOffByDefaultTestCase(BaseUploadContentConfigurationTestCase):
+
+    require_admin_user = True
+
+    @classmethod
+    def handle_galaxy_config_kwds(cls, config):
+        config["library_import_dir"] = None
+
+    def test_server_dir_uploads_403_if_dir_not_set(self):
+        library = self.library_populator.new_private_library("serverdiroffbydefault")
+        payload, files = self.library_populator.create_dataset_request(library, upload_option="upload_directory", server_dir="foobar")
+        response = self.library_populator.raw_library_contents_create(library["id"], payload, files=files)
+        assert response.status_code == 403, response.json()
+        assert '"library_import_dir" is not set' in response.json()["err_msg"]
+
+
+class ServerDirectoryValidUsageTestCase(BaseUploadContentConfigurationTestCase):
+
+    require_admin_user = True
+
+    def test_valid_server_dir_uploads_okay(self):
+        library = self.library_populator.new_private_library("serverdirupload")
+        # upload $GALAXY_ROOT/test-data/library
+        payload, files = self.library_populator.create_dataset_request(library, upload_option="upload_directory", server_dir="library")
+        response = self.library_populator.raw_library_contents_create(library["id"], payload, files=files)
+        assert response.status_code == 200, response.json()
+
+
+class ServerDirectoryRestrictedToAdminsUsageTestCase(BaseUploadContentConfigurationTestCase):
+
+    @classmethod
+    def handle_galaxy_config_kwds(cls, config):
+        config["user_library_import_dir"] = None
+
+    def test_library_import_dir_not_available_to_non_admins(self):
+        # same test case above works for admins
+        library = self.library_populator.new_private_library("serverdirupload")
+        payload, files = self.library_populator.create_dataset_request(library, upload_option="upload_directory", server_dir="library")
+        response = self.library_populator.raw_library_contents_create(library["id"], payload, files=files)
+        assert response.status_code == 403, response.json()