From acf9ae2a3213c3ccdac019142a34cfc004dcccff Mon Sep 17 00:00:00 2001 From: Joseph Myers Date: Sun, 12 Jan 2020 17:58:44 +0000 Subject: [PATCH] Improve handling of missing uploaded files for bulk registration. When used with a browser, the support for uploading photos as part of bulk registration resulted in errors when a ZIP file was not uploaded. This did not show up in testing because MechanicalSoup versions 0.11 and earlier do not handle missing file uploads the same way as a browser does . Similar issues apply when no CSV file is uploaded (should produce an error, did not with a browser). This change fixes the code handling uploads to handle empty filenames, as used by browsers when no file is uploaded, the same as the form field for the upload being missing, with consequent changes to handle the case of empty zip_ref following a missing ZIP upload. Tests are added for the browser-like case of missing CSV uploads, though those are only properly effective with newer MechanicalSoup. This passes testing with both older and newer MechanicalSoup. --- matholymp/roundupreg/rounduputil.py | 20 +++++++++++--------- matholymp/roundupreg/templating.py | 17 +++++++++++------ matholymp/test/test_reg_system.py | 20 ++++++++++++++++++-- 3 files changed, 40 insertions(+), 17 deletions(-) diff --git a/matholymp/roundupreg/rounduputil.py b/matholymp/roundupreg/rounduputil.py index feefd5a7..ab48537f 100644 --- a/matholymp/roundupreg/rounduputil.py +++ b/matholymp/roundupreg/rounduputil.py @@ -393,11 +393,12 @@ def bulk_csv_data(form, comma_sep_fields=()): for whether just to check the results or act on them, or a string error message. """ - # A csv_file form input means data has been uploaded for initial - # checking. A csv_contents form input means the previously - # checked data has been submitted for actual bulk registration - # (but should still be rechecked rather than trusting the client). - if 'csv_file' in form: + # A csv_file form input with nonempty filename means data has been + # uploaded for initial checking. A csv_contents form input means + # the previously checked data has been submitted for actual bulk + # registration (but should still be rechecked rather than trusting + # the client). + if 'csv_file' in form and form['csv_file'].filename != '': check_only = True file_bytes = form['csv_file'].value if not isinstance(file_bytes, bytes): @@ -498,9 +499,10 @@ def bulk_zip_data(db, form): Return a ZipFile object for an uploaded ZIP file, or None if no such file was uploaded, or a string on error. """ - # A zip_file form input means a ZIP file has been uploaded now. A - # zip_ref form input refers to such a file previously uploaded. - if 'zip_file' in form: + # A zip_file form input with nonempty filename means a ZIP file + # has been uploaded now. A nonempty zip_ref form input refers to + # such a file previously uploaded. + if 'zip_file' in form and form['zip_file'].filename != '': file_bytes = form['zip_file'].value if not isinstance(file_bytes, bytes): return 'zip_file not an uploaded file' @@ -514,7 +516,7 @@ def bulk_zip_data(db, form): except (zipfile.BadZipFile, zipfile.LargeZipFile, RuntimeError, ValueError, NotImplementedError, EOFError) as exc: return str(exc) - elif 'zip_ref' in form: + elif 'zip_ref' in form and form['zip_ref'].value != '': hashstr = form['zip_ref'].value if not isinstance(hashstr, str): return 'zip_ref an uploaded file' diff --git a/matholymp/roundupreg/templating.py b/matholymp/roundupreg/templating.py index ab835bda..c446ed28 100644 --- a/matholymp/roundupreg/templating.py +++ b/matholymp/roundupreg/templating.py @@ -472,7 +472,9 @@ def bulk_csv_contents(form): # avoid errors to the registration system admin when the template # using this function is accessed directly without such an upload, # silently return an empty string in such a case. - if 'csv_file' not in form or not isinstance(form['csv_file'].value, bytes): + if ('csv_file' not in form + or form['csv_file'].filename == '' + or not isinstance(form['csv_file'].value, bytes)): return '' return base64.b64encode(form['csv_file'].value).decode('ascii') @@ -593,11 +595,14 @@ def show_bulk_csv_person(db, form): def bulk_zip_ref(form): """Return hash of a previously uploaded ZIP file.""" - # In valid uses, zip_file is present as an uploaded file. To - # avoid errors to the registration system admin when the template - # using this function is accessed directly without such an upload, - # silently return an empty string in such a case. - if 'zip_file' not in form or not isinstance(form['zip_file'].value, bytes): + # In valid uses, zip_file is present as a possibly uploaded file + # (empty filename if no file was uploaded). To avoid errors to + # the registration system admin when the template using this + # function is accessed directly without such an upload, silently + # return an empty string in such a case. + if ('zip_file' not in form + or form['zip_file'].filename == '' + or not isinstance(form['zip_file'].value, bytes)): return '' return hashlib.sha256(form['zip_file'].value).hexdigest() diff --git a/matholymp/test/test_reg_system.py b/matholymp/test/test_reg_system.py index e2fa16a3..8672e9e2 100644 --- a/matholymp/test/test_reg_system.py +++ b/matholymp/test/test_reg_system.py @@ -3278,7 +3278,15 @@ def test_country_bulk_register_errors(self): admin_session.set({'@template': 'item'}) admin_session.check_submit_selected(error='Node id specified for bulk ' 'registration') - # Errors missing uploaded CSV data. + # Errors missing uploaded CSV data. The first case (csv_file + # present in the form but no file submitted there) is wrongly + # handled the same as the second (no csv_file in the form) by + # MechanicalSoup versions 0.11 and earlier + # . + admin_session.check_open_relative('country?@template=bulkregister') + admin_session.select_main_form() + form = admin_session.get_main_form() + admin_session.check_submit_selected(error='no CSV file uploaded') admin_session.check_open_relative('country?@template=bulkregister') admin_session.select_main_form() form = admin_session.get_main_form() @@ -12021,7 +12029,15 @@ def test_person_bulk_register_errors(self): admin_session.set({'@template': 'item'}) admin_session.check_submit_selected(error='Node id specified for bulk ' 'registration') - # Errors missing uploaded CSV data. + # Errors missing uploaded CSV data. The first case (csv_file + # present in the form but no file submitted there) is wrongly + # handled the same as the second (no csv_file in the form) by + # MechanicalSoup versions 0.11 and earlier + # . + admin_session.check_open_relative('person?@template=bulkregister') + admin_session.select_main_form() + form = admin_session.get_main_form() + admin_session.check_submit_selected(error='no CSV file uploaded') admin_session.check_open_relative('person?@template=bulkregister') admin_session.select_main_form() form = admin_session.get_main_form()