From 895e9971cb0e9c489a23e8197f868bc199443289 Mon Sep 17 00:00:00 2001 From: John Chilton Date: Wed, 13 Dec 2017 13:44:51 -0500 Subject: [PATCH 1/3] More upload configuration options testing - this time for libraries. --- test/api/test_configuration.py | 2 +- test/api/test_history_contents.py | 2 +- test/api/test_libraries.py | 2 +- test/api/test_tools.py | 2 +- test/base/populators.py | 38 ++++++++---- .../test_upload_configuration_options.py | 60 ++++++++++++++++++- 6 files changed, 87 insertions(+), 19 deletions(-) diff --git a/test/api/test_configuration.py b/test/api/test_configuration.py index 48ba5310a2c..30e45d8663f 100644 --- a/test/api/test_configuration.py +++ b/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() diff --git a/test/api/test_history_contents.py b/test/api/test_history_contents.py index 715bc36d7b3..a64278a989d 100644 --- a/test/api/test_history_contents.py +++ b/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) diff --git a/test/api/test_libraries.py b/test/api/test_libraries.py index 8050157e054..2a715f50fcb 100644 --- a/test/api/test_libraries.py +++ b/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") diff --git a/test/api/test_tools.py b/test/api/test_tools.py index 8591cc360c0..f42bb4a2f45 100644 --- a/test/api/test_tools.py +++ b/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"]] diff --git a/test/base/populators.py b/test/base/populators.py index f58974d45b2..c046a78bf2a 100644 --- a/test/base/populators.py +++ b/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 diff --git a/test/integration/test_upload_configuration_options.py b/test/integration/test_upload_configuration_options.py index cae1a693a93..d5f6789723c 100644 --- a/test/integration/test_upload_configuration_options.py +++ b/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() From ee297f0299d68c840028f283743c8e74732b8ba4 Mon Sep 17 00:00:00 2001 From: John Chilton Date: Wed, 13 Dec 2017 14:21:11 -0500 Subject: [PATCH 2/3] Fix server dir uploads - this required method was dropped in #4908. --- lib/galaxy/actions/library.py | 37 +++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/lib/galaxy/actions/library.py b/lib/galaxy/actions/library.py index bb786e727aa..7b17a9315be 100644 --- a/lib/galaxy/actions/library.py +++ b/lib/galaxy/actions/library.py @@ -119,6 +119,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 = [] From f4442ffc807285a96fa049f9a7e961b5e945bbff Mon Sep 17 00:00:00 2001 From: John Chilton Date: Tue, 19 Dec 2017 16:13:00 -0500 Subject: [PATCH 3/3] Small tweaks toward more modern library API. - Use exception-aware API decorators for library contents API endpoints. - Throw exceptions to handle permission and parameter errors for library upload paths with reusable validation functions. - Re-arrange code to avoid working with empty paths and throw better configuration not allow exceptions. --- lib/galaxy/actions/library.py | 81 ++++++++++++------- .../webapps/galaxy/api/library_contents.py | 17 ++-- 2 files changed, 56 insertions(+), 42 deletions(-) diff --git a/lib/galaxy/actions/library.py b/lib/galaxy/actions/library.py index 7b17a9315be..06b44cf3355 100644 --- a/lib/galaxy/actions/library.py +++ b/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 diff --git a/lib/galaxy/webapps/galaxy/api/library_contents.py b/lib/galaxy/webapps/galaxy/api/library_contents.py index befe4263a56..5b873f24087 100644 --- a/lib/galaxy/webapps/galaxy/api/library_contents.py +++ b/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 )