From 8eb4f1ac96fd85469e7c721244020df95d05ed05 Mon Sep 17 00:00:00 2001 From: jasquat Date: Tue, 10 Jan 2023 12:16:24 -0500 Subject: [PATCH] some updates to validate xml when uploading and saving w/ burnettk --- .../routes/process_models_controller.py | 109 +++++++++++------- .../services/spec_file_service.py | 35 ++++-- tests/data/dot_notation/diagram.bpmn | 6 +- .../message_receiver.bpmn | 8 +- .../message_sender.bpmn | 8 +- .../message_receiver_one.bpmn | 8 +- .../message_receiver_two.bpmn | 8 +- .../message_sender.bpmn | 16 +-- .../integration/test_process_api.py | 2 +- .../unit/test_spec_file_service.py | 20 ++++ 10 files changed, 142 insertions(+), 78 deletions(-) diff --git a/src/spiffworkflow_backend/routes/process_models_controller.py b/src/spiffworkflow_backend/routes/process_models_controller.py index 4e129deb..3b5b17f2 100644 --- a/src/spiffworkflow_backend/routes/process_models_controller.py +++ b/src/spiffworkflow_backend/routes/process_models_controller.py @@ -15,6 +15,7 @@ from flask import jsonify from flask import make_response from flask.wrappers import Response from flask_bpmn.api.api_error import ApiError +from werkzeug.datastructures import FileStorage from spiffworkflow_backend.interfaces import IdToProcessGroupMapping from spiffworkflow_backend.models.file import FileSchema @@ -38,6 +39,9 @@ from spiffworkflow_backend.services.process_instance_report_service import ( ProcessInstanceReportService, ) from spiffworkflow_backend.services.process_model_service import ProcessModelService +from spiffworkflow_backend.services.spec_file_service import ( + ProcessModelFileInvalidError, +) from spiffworkflow_backend.services.spec_file_service import SpecFileService @@ -222,26 +226,11 @@ def process_model_file_update( modified_process_model_identifier: str, file_name: str ) -> flask.wrappers.Response: """Process_model_file_update.""" - process_model_identifier = modified_process_model_identifier.replace(":", "/") - process_model = _get_process_model(process_model_identifier) - - request_file = _get_file_from_request() - request_file_contents = request_file.stream.read() - if not request_file_contents: - raise ApiError( - error_code="file_contents_empty", - message="Given request file does not have any content", - status_code=400, - ) - - SpecFileService.update_file(process_model, file_name, request_file_contents) - _commit_and_push_to_git( - f"User: {g.user.username} clicked save for" - f" {process_model_identifier}/{file_name}" + message = f"User: {g.user.username} clicked save for" + return _create_or_update_process_model_file( + modified_process_model_identifier, message, 200 ) - return Response(json.dumps({"ok": True}), status=200, mimetype="application/json") - def process_model_file_delete( modified_process_model_identifier: str, file_name: str @@ -271,28 +260,9 @@ def process_model_file_create( modified_process_model_identifier: str, ) -> flask.wrappers.Response: """Process_model_file_create.""" - process_model_identifier = modified_process_model_identifier.replace(":", "/") - process_model = _get_process_model(process_model_identifier) - request_file = _get_file_from_request() - if not request_file.filename: - raise ApiError( - error_code="could_not_get_filename", - message="Could not get filename from request", - status_code=400, - ) - - file = SpecFileService.add_file( - process_model, request_file.filename, request_file.stream.read() - ) - file_contents = SpecFileService.get_data(process_model, file.name) - file.file_contents = file_contents - file.process_model_id = process_model.id - _commit_and_push_to_git( - f"User: {g.user.username} added process model file" - f" {process_model_identifier}/{file.name}" - ) - return Response( - json.dumps(FileSchema().dump(file)), status=201, mimetype="application/json" + message = f"User: {g.user.username} added process model file" + return _create_or_update_process_model_file( + modified_process_model_identifier, message, 201 ) @@ -462,9 +432,9 @@ def process_model_create_with_natural_language( ) -def _get_file_from_request() -> Any: +def _get_file_from_request() -> FileStorage: """Get_file_from_request.""" - request_file = connexion.request.files.get("file") + request_file: FileStorage = connexion.request.files.get("file") if not request_file: raise ApiError( error_code="no_file_given", @@ -502,3 +472,58 @@ def _get_process_group_from_modified_identifier( status_code=400, ) return process_group + + +def _create_or_update_process_model_file( + modified_process_model_identifier: str, + message_for_git_commit: str, + http_status_to_return: int, +) -> flask.wrappers.Response: + """_create_or_update_process_model_file.""" + process_model_identifier = modified_process_model_identifier.replace(":", "/") + process_model = _get_process_model(process_model_identifier) + request_file = _get_file_from_request() + + # for mypy + request_file_contents = request_file.stream.read() + if not request_file_contents: + raise ApiError( + error_code="file_contents_empty", + message="Given request file does not have any content", + status_code=400, + ) + if not request_file.filename: + raise ApiError( + error_code="could_not_get_filename", + message="Could not get filename from request", + status_code=400, + ) + + file = None + try: + file = SpecFileService.update_file( + process_model, request_file.filename, request_file_contents + ) + except ProcessModelFileInvalidError as exception: + raise ( + ApiError( + error_code="process_model_file_invalid", + message=( + f"Invalid Process model file cannot be save: {request_file.name}." + f" Received error: {str(exception)}" + ), + status_code=400, + ) + ) from exception + file_contents = SpecFileService.get_data(process_model, file.name) + file.file_contents = file_contents + file.process_model_id = process_model.id + _commit_and_push_to_git( + f"{message_for_git_commit} {process_model_identifier}/{file.name}" + ) + + return Response( + json.dumps(FileSchema().dump(file)), + status=http_status_to_return, + mimetype="application/json", + ) diff --git a/src/spiffworkflow_backend/services/spec_file_service.py b/src/spiffworkflow_backend/services/spec_file_service.py index 4fdfbd6d..c9aaff89 100644 --- a/src/spiffworkflow_backend/services/spec_file_service.py +++ b/src/spiffworkflow_backend/services/spec_file_service.py @@ -6,7 +6,8 @@ from typing import List from typing import Optional from flask_bpmn.models.db import db -from SpiffWorkflow.bpmn.parser.ValidationException import ValidationException # type: ignore +from lxml import etree # type: ignore +from SpiffWorkflow.bpmn.parser.BpmnParser import BpmnValidator # type: ignore from spiffworkflow_backend.models.file import File from spiffworkflow_backend.models.file import FileType @@ -29,6 +30,10 @@ class ProcessModelFileNotFoundError(Exception): """ProcessModelFileNotFoundError.""" +class ProcessModelFileInvalidError(Exception): + """ProcessModelFileInvalidError.""" + + class SpecFileService(FileSystemService): """SpecFileService.""" @@ -44,7 +49,6 @@ class SpecFileService(FileSystemService): extension_filter: str = "", ) -> List[File]: """Return all files associated with a workflow specification.""" - # path = SpecFileService.workflow_path(process_model_info) path = os.path.join( FileSystemService.root_path(), process_model_info.id_for_file_path() ) @@ -147,12 +151,27 @@ class SpecFileService(FileSystemService): # Same as update return SpecFileService.update_file(process_model_info, file_name, binary_data) - @staticmethod + @classmethod + def validate_bpmn_xml(cls, file_name: str, binary_data: bytes) -> None: + """Validate_bpmn_xml.""" + file_type = FileSystemService.file_type(file_name) + if file_type.value == FileType.bpmn.value: + validator = BpmnValidator() + parser = MyCustomParser(validator=validator) + try: + parser.add_bpmn_xml(etree.fromstring(binary_data), filename=file_name) + except etree.XMLSyntaxError as exception: + raise ProcessModelFileInvalidError( + f"Received error trying to parse bpmn xml: {str(exception)}" + ) from exception + + @classmethod def update_file( - process_model_info: ProcessModelInfo, file_name: str, binary_data: bytes + cls, process_model_info: ProcessModelInfo, file_name: str, binary_data: bytes ) -> File: """Update_file.""" SpecFileService.assert_valid_file_name(file_name) + cls.validate_bpmn_xml(file_name, binary_data) full_file_path = SpecFileService.full_file_path(process_model_info, file_name) SpecFileService.write_file_data_to_system(full_file_path, binary_data) file = SpecFileService.to_file_object(file_name, full_file_path) @@ -282,7 +301,7 @@ class SpecFileService(FileSystemService): # if the old relative bpmn file no longer exists, then assume things were moved around # on the file system. Otherwise, assume it is a duplicate process id and error. if os.path.isfile(full_bpmn_file_path): - raise ValidationException( + raise ProcessModelFileInvalidError( f"Process id ({ref.identifier}) has already been used for " f"{process_id_lookup.relative_path}. It cannot be reused." ) @@ -314,7 +333,7 @@ class SpecFileService(FileSystemService): identifier=message_model_identifier ).first() if message_model is None: - raise ValidationException( + raise ProcessModelFileInvalidError( "Could not find message model with identifier" f" '{message_model_identifier}'Required by a Start Event in :" f" {ref.file_name}" @@ -336,7 +355,7 @@ class SpecFileService(FileSystemService): message_triggerable_process_model.process_model_identifier != ref.process_model_id ): - raise ValidationException( + raise ProcessModelFileInvalidError( "Message model is already used to start process model" f" {ref.process_model_id}" ) @@ -355,7 +374,7 @@ class SpecFileService(FileSystemService): identifier=message_model_identifier ).first() if message_model is None: - raise ValidationException( + raise ProcessModelFileInvalidError( "Could not find message model with identifier" f" '{message_model_identifier}'specified by correlation" f" property: {cpre}" diff --git a/tests/data/dot_notation/diagram.bpmn b/tests/data/dot_notation/diagram.bpmn index cb623748..96bdce89 100644 --- a/tests/data/dot_notation/diagram.bpmn +++ b/tests/data/dot_notation/diagram.bpmn @@ -2,10 +2,10 @@ - to + to - from.name + from.name @@ -20,7 +20,7 @@ - new + new diff --git a/tests/data/message_send_one_conversation/message_receiver.bpmn b/tests/data/message_send_one_conversation/message_receiver.bpmn index c82aa363..828f7d2e 100644 --- a/tests/data/message_send_one_conversation/message_receiver.bpmn +++ b/tests/data/message_send_one_conversation/message_receiver.bpmn @@ -12,18 +12,18 @@ - topica + topica - the_payload.topica + the_payload.topica - topicb + topicb - the_payload.topicb + the_payload.topicb diff --git a/tests/data/message_send_one_conversation/message_sender.bpmn b/tests/data/message_send_one_conversation/message_sender.bpmn index a735d1ad..7bda31eb 100644 --- a/tests/data/message_send_one_conversation/message_sender.bpmn +++ b/tests/data/message_send_one_conversation/message_sender.bpmn @@ -12,18 +12,18 @@ - topica + topica - the_payload.topica + the_payload.topica - topicb + topicb - the_payload.topicb + the_payload.topicb diff --git a/tests/data/message_send_two_conversations/message_receiver_one.bpmn b/tests/data/message_send_two_conversations/message_receiver_one.bpmn index 7a2356d6..62236cc4 100644 --- a/tests/data/message_send_two_conversations/message_receiver_one.bpmn +++ b/tests/data/message_send_two_conversations/message_receiver_one.bpmn @@ -12,18 +12,18 @@ - topica_one + topica_one - topica_one + topica_one - topicb_one + topicb_one - topicb_one + topicb_one diff --git a/tests/data/message_send_two_conversations/message_receiver_two.bpmn b/tests/data/message_send_two_conversations/message_receiver_two.bpmn index 354f3aad..67e856a5 100644 --- a/tests/data/message_send_two_conversations/message_receiver_two.bpmn +++ b/tests/data/message_send_two_conversations/message_receiver_two.bpmn @@ -12,18 +12,18 @@ - topica_two + topica_two - topica_two + topica_two - topicb_two + topicb_two - topicb_two + topicb_two diff --git a/tests/data/message_send_two_conversations/message_sender.bpmn b/tests/data/message_send_two_conversations/message_sender.bpmn index 38359c92..721c1780 100644 --- a/tests/data/message_send_two_conversations/message_sender.bpmn +++ b/tests/data/message_send_two_conversations/message_sender.bpmn @@ -19,18 +19,18 @@ - topica_one + topica_one - payload_var_one.topica_one + payload_var_one.topica_one - topicb_one + topicb_one - payload_var_one.topicb + payload_var_one.topicb @@ -117,18 +117,18 @@ del time - topica_two + topica_two - topica_two + topica_two - topicb_two + topicb_two - topicb_two + topicb_two diff --git a/tests/spiffworkflow_backend/integration/test_process_api.py b/tests/spiffworkflow_backend/integration/test_process_api.py index f5248050..150d033f 100644 --- a/tests/spiffworkflow_backend/integration/test_process_api.py +++ b/tests/spiffworkflow_backend/integration/test_process_api.py @@ -860,7 +860,7 @@ class TestProcessApi(BaseTest): assert response.status_code == 200 assert response.json is not None - assert response.json["ok"] + assert response.json["file_contents"] is not None response = client.get( f"/v1.0/process-models/{modified_process_model_id}/files/random_fact.svg", diff --git a/tests/spiffworkflow_backend/unit/test_spec_file_service.py b/tests/spiffworkflow_backend/unit/test_spec_file_service.py index 3cc353b5..ba2c4319 100644 --- a/tests/spiffworkflow_backend/unit/test_spec_file_service.py +++ b/tests/spiffworkflow_backend/unit/test_spec_file_service.py @@ -12,6 +12,9 @@ from tests.spiffworkflow_backend.helpers.test_data import load_test_spec from spiffworkflow_backend.models.spec_reference import SpecReferenceCache from spiffworkflow_backend.models.user import UserModel from spiffworkflow_backend.services.process_model_service import ProcessModelService +from spiffworkflow_backend.services.spec_file_service import ( + ProcessModelFileInvalidError, +) from spiffworkflow_backend.services.spec_file_service import SpecFileService @@ -206,3 +209,20 @@ class TestSpecFileService(BaseTest): assert dmn1[0].display_name == "Decision 1" assert dmn1[0].identifier == "Decision_0vrtcmk" assert dmn1[0].type == "decision" + + def test_validate_bpmn_xml_with_invalid_xml( + self, + app: Flask, + client: FlaskClient, + with_db_and_bpmn_file_cleanup: None, + ) -> None: + """Test_validate_bpmn_xml_with_invalid_xml.""" + process_model = load_test_spec( + process_model_id="group/invalid_xml", + bpmn_file_name="script_error_with_task_data.bpmn", + process_model_source_directory="error", + ) + with pytest.raises(ProcessModelFileInvalidError): + SpecFileService.update_file( + process_model, "bad_xml.bpmn", b"THIS_IS_NOT_VALID_XML" + )