From f4442ffc807285a96fa049f9a7e961b5e945bbff Mon Sep 17 00:00:00 2001 From: John Chilton Date: Tue, 19 Dec 2017 16:13:00 -0500 Subject: [PATCH] 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 7b17a9315be3..06b44cf3355f 100644 --- a/lib/galaxy/actions/library.py +++ b/lib/galaxy/actions/library.py @@ -8,6 +8,11 @@ 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 @@ 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 @@ def _upload_dataset(self, trans, library_id, folder_id, replace_dataset=None, ** 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 befe4263a56d..5b873f240878 100644 --- a/lib/galaxy/webapps/galaxy/api/library_contents.py +++ b/lib/galaxy/webapps/galaxy/api/library_contents.py @@ -12,9 +12,8 @@ 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 @@ def show(self, trans, id, library_id, **kwd): 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 @@ def _upload_library_dataset(self, trans, library_id, folder_id, **kwd): 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 @@ def _scan_json_block(self, meta, prefix=""): # 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 @@ def _decode_library_content_id(self, content_id): 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 )