From 5a679509a1cf750d3c18a7f5fa3bd9255af638fc Mon Sep 17 00:00:00 2001 From: Daniel Markstedt Date: Wed, 12 Oct 2022 12:59:01 -0700 Subject: [PATCH] More refactoring of Python code to address Sonar issues (#906) - Use Path objects for file operations - Use urllib to sanitize URLs - Some explicit type conversions - Consistent regex syntax - Add rudimentary logging when archive extraction caching fails - Fixed two cases of the property file creation not being notified in the Flash message - added doctype and html lang attribute to the web server 502 page --- python/common/src/rascsi/file_cmds.py | 39 ++++++++++++++++----------- python/common/src/util/unarchiver.py | 6 ++--- python/web/service-infra/502.html | 3 ++- python/web/src/web.py | 21 +++++++++++++-- python/web/src/web_utils.py | 1 + 5 files changed, 49 insertions(+), 21 deletions(-) diff --git a/python/common/src/rascsi/file_cmds.py b/python/common/src/rascsi/file_cmds.py index af08e01a..8985c413 100644 --- a/python/common/src/rascsi/file_cmds.py +++ b/python/common/src/rascsi/file_cmds.py @@ -12,6 +12,7 @@ from time import time from subprocess import run, CalledProcessError from json import dump, load from shutil import copyfile +from urllib.parse import quote import requests @@ -31,6 +32,7 @@ from util import unarchiver FILE_READ_ERROR = "Unhandled exception when reading file: %s" FILE_WRITE_ERROR = "Unhandled exception when writing to file: %s" +URL_SAFE = "/:?&" class FileCmds: """ @@ -103,7 +105,9 @@ class FileCmds: for file in result.image_files_info.image_files: # Add properties meta data for the image, if applicable if file.name in prop_files: - process = self.read_drive_properties(f"{CFG_DIR}/{file.name}.{PROPERTIES_SUFFIX}") + process = self.read_drive_properties( + Path(CFG_DIR) / f"{file.name}.{PROPERTIES_SUFFIX}" + ) prop = process["conf"] else: prop = False @@ -377,7 +381,7 @@ class FileCmds: tmp_full_path = tmp_dir + file_name iso_filename = f"{server_info['image_dir']}/{file_name}.iso" - req_proc = self.download_to_dir(url, tmp_dir, file_name) + req_proc = self.download_to_dir(quote(url, safe=URL_SAFE), tmp_dir, file_name) if not req_proc["status"]: return {"status": False, "msg": req_proc["msg"]} @@ -438,7 +442,11 @@ class FileCmds: logging.info("Making a request to download %s", url) try: - with requests.get(url, stream=True, headers={"User-Agent": "Mozilla/5.0"}) as req: + with requests.get( + quote(url, safe=URL_SAFE), + stream=True, + headers={"User-Agent": "Mozilla/5.0"}, + ) as req: req.raise_for_status() with open(f"{save_dir}/{file_name}", "wb") as download: for chunk in req.iter_content(chunk_size=8192): @@ -520,9 +528,9 @@ class FileCmds: Takes (str) file_name Returns (dict) with (bool) status and (str) msg """ - file_name = f"{CFG_DIR}/{file_name}" + file_path = Path(CFG_DIR) / file_name try: - with open(file_name, encoding="ISO-8859-1") as json_file: + with open(file_path, encoding="ISO-8859-1") as json_file: config = load(json_file) # If the config file format changes again in the future, # introduce more sophisticated format detection logic here. @@ -584,7 +592,7 @@ class FileCmds: logging.error(str(error)) return {"status": False, "msg": str(error)} except: - logging.error(FILE_READ_ERROR, file_name) + logging.error(FILE_READ_ERROR, str(file_path)) raise def write_drive_properties(self, file_name, conf): @@ -593,12 +601,12 @@ class FileCmds: Takes file name base (str) and (list of dicts) conf as arguments Returns (dict) with (bool) status and (str) msg """ - file_path = f"{CFG_DIR}/{file_name}" + file_path = Path(CFG_DIR) / file_name try: with open(file_path, "w") as json_file: dump(conf, json_file, indent=4) parameters = { - "target_path": file_path + "target_path": str(file_path) } return { "status": True, @@ -607,25 +615,25 @@ class FileCmds: } except (IOError, ValueError, EOFError, TypeError) as error: logging.error(str(error)) - self.delete_file(Path(file_path)) + self.delete_file(file_path) return {"status": False, "msg": str(error)} except: - logging.error(FILE_WRITE_ERROR, file_path) - self.delete_file(Path(file_path)) + logging.error(FILE_WRITE_ERROR, str(file_path)) + self.delete_file(file_path) raise # noinspection PyMethodMayBeStatic def read_drive_properties(self, file_path): """ Reads drive properties from json formatted file. - Takes (str) file_path as argument. + Takes (Path) file_path as argument. Returns (dict) with (bool) status, (str) msg, (dict) conf """ try: with open(file_path) as json_file: conf = load(json_file) parameters = { - "file_path": file_path + "file_path": str(file_path) } return { "status": True, @@ -637,7 +645,7 @@ class FileCmds: logging.error(str(error)) return {"status": False, "msg": str(error)} except: - logging.error(FILE_READ_ERROR, file_path) + logging.error(FILE_READ_ERROR, str(file_path)) raise # noinspection PyMethodMayBeStatic @@ -673,5 +681,6 @@ class FileCmds: """ try: return unarchiver.inspect_archive(file_path) - except (unarchiver.LsarCommandError, unarchiver.LsarOutputError): + except (unarchiver.LsarCommandError, unarchiver.LsarOutputError) as error: + logging.error(str(error)) raise diff --git a/python/common/src/util/unarchiver.py b/python/common/src/util/unarchiver.py index d9cfd61d..d1d54619 100644 --- a/python/common/src/util/unarchiver.py +++ b/python/common/src/util/unarchiver.py @@ -69,7 +69,7 @@ def extract_archive(file_path, **kwargs): unar_result_success = r'^Successfully extracted to "(?P.+)".$' unar_result_no_files = "No files extracted." unar_file_extracted = \ - r"^ (?P.+). \(((?P[0-9]+) B)?(?P(dir)?(, )?(rsrc)?)\)\.\.\. (?P[A-Z]+)\.$" + r"^ {2}(?P.+). \(((?P\d+) B)?(?P(dir)?(, )?(rsrc)?)\)\.\.\. (?P[A-Z]+)\.$" lines = process["stdout"].rstrip("\n").split("\n") @@ -141,7 +141,7 @@ def extract_archive(file_path, **kwargs): # The parent dir may not be specified as a member, so ensure it exists target_path.parent.mkdir(parents=True, exist_ok=True) logging.debug("Moving temp file: %s -> %s", source_path, target_path) - move(source_path, target_path) + move(str(source_path), str(target_path)) moved.append(member) return { @@ -152,7 +152,7 @@ def extract_archive(file_path, **kwargs): raise UnarUnexpectedOutputError(lines[-1]) -def inspect_archive(file_path, **kwargs): +def inspect_archive(file_path): """ Calls `lsar` to inspect the contents of an archive Takes (str) file_path diff --git a/python/web/service-infra/502.html b/python/web/service-infra/502.html index 5fdbfd80..2d131214 100644 --- a/python/web/service-infra/502.html +++ b/python/web/service-infra/502.html @@ -1,4 +1,5 @@ - + + RaSCSI-Web is Starting diff --git a/python/web/src/web.py b/python/web/src/web.py index 720950f4..b3405856 100644 --- a/python/web/src/web.py +++ b/python/web/src/web.py @@ -360,7 +360,11 @@ def drive_create(): if not process["status"]: return response(error=True, message=process["msg"]) - return response(message=_("Image file created: %(file_name)s", file_name=full_file_name)) + # TODO: Refactor the return messages into one string + return response(message=[ + (_("Image file created: %(file_name)s", file_name=full_file_name), "success"), + (process["msg"], "success"), + ]) @APP.route("/drive/cdrom", methods=["POST"]) @@ -582,6 +586,7 @@ def attach_device(): if "interface" in params.keys(): bridge_status = is_bridge_configured(params["interface"]) if not bridge_status["status"]: + # TODO: Refactor the return messages into one string return response(error=True, message=[ (bridge_status["msg"], "error"), (error_msg, "error") @@ -673,6 +678,7 @@ def attach_image(): return response(message=response_messages) + # TODO: Refactor the return messages into one string return response(error=True, message=[ (_("Failed to attach %(file_name)s to SCSI ID %(id_number)s LUN %(unit_number)s", file_name=file_name, id_number=scsi_id, unit_number=unit), "error"), @@ -706,6 +712,7 @@ def detach(): return response(message=_("Detached SCSI ID %(id_number)s LUN %(unit_number)s", id_number=scsi_id, unit_number=unit)) + # TODO: Refactor the return messages into one string return response(error=True, message=[ (_("Failed to detach SCSI ID %(id_number)s LUN %(unit_number)s", id_number=scsi_id, unit_number=unit), "error"), @@ -727,6 +734,7 @@ def eject(): return response(message=_("Ejected SCSI ID %(id_number)s LUN %(unit_number)s", id_number=scsi_id, unit_number=unit)) + # TODO: Refactor the return messages into one string return response(error=True, message=[ (_("Failed to eject SCSI ID %(id_number)s LUN %(unit_number)s", id_number=scsi_id, unit_number=unit), "error"), @@ -764,6 +772,7 @@ def reserve_id(): RESERVATIONS[int(scsi_id)] = memo return response(message=_("Reserved SCSI ID %(id_number)s", id_number=scsi_id)) + # TODO: Refactor the return messages into one string return response(error=True, message=[ (_("Failed to reserve SCSI ID %(id_number)s", id_number=scsi_id), "error"), (process["msg"], "error"), @@ -784,6 +793,7 @@ def release_id(): RESERVATIONS[int(scsi_id)] = "" return response(message=_("Released the reservation for SCSI ID %(id_number)s", id_number=scsi_id)) + # TODO: Refactor the return messages into one string return response(error=True, message=[ (_("Failed to release the reservation for SCSI ID %(id_number)s", id_number=scsi_id), "error"), (process["msg"], "error"), @@ -824,6 +834,7 @@ def download_to_iso(): process = file_cmd.download_file_to_iso(url, *iso_args) process = ReturnCodeMapper.add_msg(process) if not process["status"]: + # TODO: Refactor the return messages into one string return response(error=True, message=[ (_("Failed to create CD-ROM image from %(url)s", url=url), "error"), (process["msg"], "error"), @@ -842,6 +853,7 @@ def download_to_iso(): response_messages.append((_("Attached to SCSI ID %(id_number)s", id_number=scsi_id), "success")) return response(message=response_messages) + # TODO: Refactor the return messages into one string return response(error=True, message=[ (_("Failed to attach image to SCSI ID %(id_number)s. Try attaching it manually.", id_number=scsi_id), "error"), (process_attach["msg"], "error"), @@ -866,6 +878,7 @@ def download_file(): if process["status"]: return response(message=process["msg"]) + # TODO: Refactor the return messages into one string return response(error=True, message=[ (_("Failed to download file from %(url)s", url=url), "error"), (process["msg"], "error"), @@ -922,7 +935,11 @@ def create_file(): return response( status_code=201, - message=_("Image file created: %(file_name)s", file_name=full_file_name), + # TODO: Refactor the return messages into one string + message=[ + (_("Image file created: %(file_name)s", file_name=full_file_name), "success"), + (process["msg"], "success"), + ], image=full_file_name, ) diff --git a/python/web/src/web_utils.py b/python/web/src/web_utils.py index 8d887949..8efd3589 100644 --- a/python/web/src/web_utils.py +++ b/python/web/src/web_utils.py @@ -240,6 +240,7 @@ def is_bridge_configured(interface): Takes (str) interface of a network device being attached. Returns a (dict) with (bool) status and (str) msg """ + # TODO: Reduce the nesting of these checks, and streamline how the results are notified status = True return_msg = "" sys_cmd = SysCmds()