From 14497719ba6d3a9727563d15dbeacf2b537551f3 Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Sat, 2 Sep 2017 19:42:44 +0200 Subject: [PATCH 1/3] Prevent in-place editing if purge_source is false When importing files from the FTP folder the admin can choose to prevent purging of imported files. In this case the source file should not be modified in-place by galaxy. This also modifies relevant bare `except:` statements and modifies the meaning of `in_place` in upload.py from no external chown script to do not edit files in place if we keep the source file. This fixes https://github.com/galaxyproject/galaxy/issues/4527. --- lib/galaxy/tools/actions/upload_common.py | 14 +++++--- tools/data_source/upload.py | 40 ++++++++++++----------- 2 files changed, 31 insertions(+), 23 deletions(-) diff --git a/lib/galaxy/tools/actions/upload_common.py b/lib/galaxy/tools/actions/upload_common.py index 16f1a8982f6..2c46a0813af 100644 --- a/lib/galaxy/tools/actions/upload_common.py +++ b/lib/galaxy/tools/actions/upload_common.py @@ -323,20 +323,26 @@ def create_paramfile(trans, uploaded_datasets): else: try: is_binary = uploaded_dataset.datatype.is_binary - except: + except Exception: is_binary = None try: link_data_only = uploaded_dataset.link_data_only - except: + except Exception: link_data_only = 'copy_files' try: uuid_str = uploaded_dataset.uuid - except: + except Exception: uuid_str = None try: purge_source = uploaded_dataset.purge_source - except: + except Exception: purge_source = True + try: + user_ftp_dir = os.path.abspath(trans.user_ftp_dir) + except Exception: + user_ftp_dir = None + if user_ftp_dir and uploaded_dataset.path.startswith(user_ftp_dir): + uploaded_dataset.type = 'ftp_import' json = dict(file_type=uploaded_dataset.file_type, ext=uploaded_dataset.ext, name=uploaded_dataset.name, diff --git a/tools/data_source/upload.py b/tools/data_source/upload.py index b47892afd3e..7569cb9b867 100644 --- a/tools/data_source/upload.py +++ b/tools/data_source/upload.py @@ -81,6 +81,15 @@ def add_file(dataset, registry, json_file, output_path): link_data_only = dataset.get('link_data_only', 'copy_files') in_place = dataset.get('in_place', True) purge_source = dataset.get('purge_source', True) + # in_place is True if there is no external chmod in place, + # however there are other instances where modifications should not occur in_place: + # in-place unpacking or editing of line-ending when linking in data or when + # importing data from the FTP folder while purge_source is set to false + if purge_source == False and dataset.get('type') == 'ftp_import': + # If we do not purge the source we should not modify it in place. + in_place = False + if dataset.type in ('server_dir', 'path_paste'): + in_place = False check_content = dataset.get('check_content' , True) auto_decompress = dataset.get('auto_decompress', True) try: @@ -158,7 +167,7 @@ def add_file(dataset, registry, json_file, output_path): os.close(fd) gzipped_file.close() # Replace the gzipped file with the decompressed file if it's safe to do so - if dataset.type in ('server_dir', 'path_paste') or not in_place: + if not in_place: dataset.path = uncompressed else: shutil.move(uncompressed, dataset.path) @@ -191,7 +200,7 @@ def add_file(dataset, registry, json_file, output_path): os.close(fd) bzipped_file.close() # Replace the bzipped file with the decompressed file if it's safe to do so - if dataset.type in ('server_dir', 'path_paste') or not in_place: + if not in_place: dataset.path = uncompressed else: shutil.move(uncompressed, dataset.path) @@ -248,7 +257,7 @@ def add_file(dataset, registry, json_file, output_path): z.close() # Replace the zipped file with the decompressed file if it's safe to do so if uncompressed is not None: - if dataset.type in ('server_dir', 'path_paste') or not in_place: + if not in_place: dataset.path = uncompressed else: shutil.move(uncompressed, dataset.path) @@ -279,13 +288,10 @@ def add_file(dataset, registry, json_file, output_path): if check_content and check_html(dataset.path): file_err('The uploaded file contains inappropriate HTML content', dataset, json_file) return - if data_type != 'binary': + if data_type not in ('gzip', 'bz2', 'zip', 'binary'): if link_data_only == 'copy_files': - if dataset.type in ('server_dir', 'path_paste') and data_type not in ['gzip', 'bz2', 'zip']: - in_place = False - # Convert universal line endings to Posix line endings, but allow the user to turn it off, - # so that is becomes possible to upload gzip, bz2 or zip files with binary data without - # corrupting the content of those files. + # Convert universal line endings to Posix line endings if to_posix_lines is True + # and the data is not binary or gzip-, bz2- or zip-compressed. if dataset.to_posix_lines: tmpdir = output_adjacent_tmpdir(output_path) tmp_prefix = 'data_id_%s_convert_' % dataset.dataset_id @@ -313,17 +319,13 @@ def add_file(dataset, registry, json_file, output_path): 'Copy files into Galaxy instead of Link to files without copying into Galaxy so grooming can be performed.' file_err(err_msg, dataset, json_file) return - if link_data_only == 'copy_files' and dataset.type in ('server_dir', 'path_paste') and data_type not in ['gzip', 'bz2', 'zip']: + if link_data_only == 'copy_files' and converted_path: # Move the dataset to its "real" path - if converted_path is not None: - shutil.copy(converted_path, output_path) - try: - os.remove(converted_path) - except: - pass - else: - # This should not happen, but it's here just in case - shutil.copy(dataset.path, output_path) + shutil.copy(converted_path, output_path) + try: + os.remove(converted_path) + except Exception: + pass elif link_data_only == 'copy_files': if purge_source: shutil.move(dataset.path, output_path) From 62ef8b52aedb9b9b34403db83703083a0cddbaf9 Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Sun, 3 Sep 2017 10:49:23 +0200 Subject: [PATCH 2/3] Fix sniffing for non-binary files --- tools/data_source/upload.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tools/data_source/upload.py b/tools/data_source/upload.py index 7569cb9b867..159ff63c401 100644 --- a/tools/data_source/upload.py +++ b/tools/data_source/upload.py @@ -85,7 +85,7 @@ def add_file(dataset, registry, json_file, output_path): # however there are other instances where modifications should not occur in_place: # in-place unpacking or editing of line-ending when linking in data or when # importing data from the FTP folder while purge_source is set to false - if purge_source == False and dataset.get('type') == 'ftp_import': + if not purge_source and dataset.get('type') == 'ftp_import': # If we do not purge the source we should not modify it in place. in_place = False if dataset.type in ('server_dir', 'path_paste'): @@ -288,8 +288,8 @@ def add_file(dataset, registry, json_file, output_path): if check_content and check_html(dataset.path): file_err('The uploaded file contains inappropriate HTML content', dataset, json_file) return - if data_type not in ('gzip', 'bz2', 'zip', 'binary'): - if link_data_only == 'copy_files': + if data_type != 'binary': + if link_data_only == 'copy_files' and data_type not in ('gzip', 'bz2', 'zip'): # Convert universal line endings to Posix line endings if to_posix_lines is True # and the data is not binary or gzip-, bz2- or zip-compressed. if dataset.to_posix_lines: From 06db29413e838fd88a2ec84ede347f097be161e2 Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Sun, 3 Sep 2017 12:03:36 +0200 Subject: [PATCH 3/3] Move instead of copying converted datasets when possible shutil.move tries to move files by renaming them. If that fails with OSError (due to permission or cross-filesystem rename) it falls back to copying files followed by removing them (https://github.com/python/cpython/blob/2.7/Lib/shutil.py#L279). By using shutil.move and catching permission problems we avoid an unnecessary copy if source and destination are on the same filesystem. Also avoids shutil.move if the upload tool is run as real-user which should fix https://github.com/galaxyproject/galaxy/issues/4300. --- tools/data_source/upload.py | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/tools/data_source/upload.py b/tools/data_source/upload.py index 159ff63c401..8b1543d7072 100644 --- a/tools/data_source/upload.py +++ b/tools/data_source/upload.py @@ -6,6 +6,7 @@ from __future__ import print_function import codecs +import errno import gzip import os import shutil @@ -79,16 +80,12 @@ def add_file(dataset, registry, json_file, output_path): converted_path = None stdout = None link_data_only = dataset.get('link_data_only', 'copy_files') - in_place = dataset.get('in_place', True) + run_as_real_user = in_place = dataset.get('in_place', True) purge_source = dataset.get('purge_source', True) # in_place is True if there is no external chmod in place, # however there are other instances where modifications should not occur in_place: - # in-place unpacking or editing of line-ending when linking in data or when - # importing data from the FTP folder while purge_source is set to false - if not purge_source and dataset.get('type') == 'ftp_import': - # If we do not purge the source we should not modify it in place. - in_place = False - if dataset.type in ('server_dir', 'path_paste'): + # when a file is added from a directory on the local file system (ftp import folder or any other path). + if dataset.type in ('server_dir', 'path_paste', 'ftp_import'): in_place = False check_content = dataset.get('check_content' , True) auto_decompress = dataset.get('auto_decompress', True) @@ -321,13 +318,16 @@ def add_file(dataset, registry, json_file, output_path): return if link_data_only == 'copy_files' and converted_path: # Move the dataset to its "real" path - shutil.copy(converted_path, output_path) try: - os.remove(converted_path) - except Exception: - pass + shutil.move(converted_path, output_path) + except OSError as e: + # We may not have permission to remove converted_path + if e.errno != errno.EACCES: + raise elif link_data_only == 'copy_files': - if purge_source: + if purge_source and not run_as_real_user: + # if the upload tool runs as a real user the real user + # can't move dataset.path as this path is owned by galaxy. shutil.move(dataset.path, output_path) else: shutil.copy(dataset.path, output_path)