diff --git a/spiffworkflow-backend/src/spiffworkflow_backend/exceptions/api_error.py b/spiffworkflow-backend/src/spiffworkflow_backend/exceptions/api_error.py index ee71e453..0e590c0e 100644 --- a/spiffworkflow-backend/src/spiffworkflow_backend/exceptions/api_error.py +++ b/spiffworkflow-backend/src/spiffworkflow_backend/exceptions/api_error.py @@ -302,7 +302,7 @@ def handle_exception(exception: Exception) -> flask.wrappers.Response: else: api_exception = ApiError( error_code=error_code, - message=f"{exception.__class__.__name__} {str(exception)}", + message=f"{exception.__class__.__name__}: {str(exception)}", sentry_link=sentry_link, status_code=status_code, ) diff --git a/spiffworkflow-backend/src/spiffworkflow_backend/models/process_model.py b/spiffworkflow-backend/src/spiffworkflow_backend/models/process_model.py index 2c2b2cca..47bd952b 100644 --- a/spiffworkflow-backend/src/spiffworkflow_backend/models/process_model.py +++ b/spiffworkflow-backend/src/spiffworkflow_backend/models/process_model.py @@ -70,6 +70,9 @@ class ProcessModelInfo: def id_for_file_path(self) -> str: return self.id.replace("/", os.sep) + def modified_process_model_identifier(self) -> str: + return self.modify_process_identifier_for_path_param(self.id) + @classmethod def modify_process_identifier_for_path_param(cls, identifier: str) -> str: if "\\" in identifier: diff --git a/spiffworkflow-backend/src/spiffworkflow_backend/routes/process_api_blueprint.py b/spiffworkflow-backend/src/spiffworkflow_backend/routes/process_api_blueprint.py index a55c915e..0f3dd58d 100644 --- a/spiffworkflow-backend/src/spiffworkflow_backend/routes/process_api_blueprint.py +++ b/spiffworkflow-backend/src/spiffworkflow_backend/routes/process_api_blueprint.py @@ -64,7 +64,18 @@ def process_list() -> Any: primary process - helpful for finding possible call activities. """ references = SpecReferenceCache.query.filter_by(type="process").all() - return SpecReferenceSchema(many=True).dump(references) + process_model_identifiers = [r.process_model_id for r in references] + permitted_process_model_identifiers = ProcessModelService.process_model_identifiers_with_permission_for_user( + user=g.user, + permission_to_check="create", + permission_base_uri="/v1.0/process-instances", + process_model_identifiers=process_model_identifiers, + ) + permitted_references = [] + for spec_reference in references: + if spec_reference.process_model_id in permitted_process_model_identifiers: + permitted_references.append(spec_reference) + return SpecReferenceSchema(many=True).dump(permitted_references) def process_caller_list(bpmn_process_identifiers: list[str]) -> Any: diff --git a/spiffworkflow-backend/src/spiffworkflow_backend/routes/process_models_controller.py b/spiffworkflow-backend/src/spiffworkflow_backend/routes/process_models_controller.py index c3c7b92b..be85ad14 100644 --- a/spiffworkflow-backend/src/spiffworkflow_backend/routes/process_models_controller.py +++ b/spiffworkflow-backend/src/spiffworkflow_backend/routes/process_models_controller.py @@ -517,7 +517,7 @@ def _create_or_update_process_model_file( file = None try: - file = SpecFileService.update_file(process_model, request_file.filename, request_file_contents) + file = SpecFileService.update_file(process_model, request_file.filename, request_file_contents, user=g.user) except ProcessModelFileInvalidError as exception: raise ( ApiError( diff --git a/spiffworkflow-backend/src/spiffworkflow_backend/services/data_setup_service.py b/spiffworkflow-backend/src/spiffworkflow_backend/services/data_setup_service.py index 6b32497f..55ebd82a 100644 --- a/spiffworkflow-backend/src/spiffworkflow_backend/services/data_setup_service.py +++ b/spiffworkflow-backend/src/spiffworkflow_backend/services/data_setup_service.py @@ -30,6 +30,7 @@ class DataSetupService: for ref in refs: try: SpecFileService.update_caches(ref) + db.session.commit() except Exception as ex: failing_process_models.append( ( diff --git a/spiffworkflow-backend/src/spiffworkflow_backend/services/process_caller_service.py b/spiffworkflow-backend/src/spiffworkflow_backend/services/process_caller_service.py index bdbbb06b..73a8c2a0 100644 --- a/spiffworkflow-backend/src/spiffworkflow_backend/services/process_caller_service.py +++ b/spiffworkflow-backend/src/spiffworkflow_backend/services/process_caller_service.py @@ -27,7 +27,6 @@ class ProcessCallerService: db.session.add( ProcessCallerCacheModel(process_identifier=called_process_id, calling_process_identifier=process_id) ) - db.session.commit() @staticmethod def callers(process_ids: list[str]) -> list[str]: diff --git a/spiffworkflow-backend/src/spiffworkflow_backend/services/process_model_service.py b/spiffworkflow-backend/src/spiffworkflow_backend/services/process_model_service.py index 432eef52..9700249f 100644 --- a/spiffworkflow-backend/src/spiffworkflow_backend/services/process_model_service.py +++ b/spiffworkflow-backend/src/spiffworkflow_backend/services/process_model_service.py @@ -18,6 +18,7 @@ from spiffworkflow_backend.models.process_instance import ProcessInstanceModel from spiffworkflow_backend.models.process_model import PROCESS_MODEL_SUPPORTED_KEYS_FOR_DISK_SERIALIZATION from spiffworkflow_backend.models.process_model import ProcessModelInfo from spiffworkflow_backend.models.process_model import ProcessModelInfoSchema +from spiffworkflow_backend.models.user import UserModel from spiffworkflow_backend.services.authorization_service import AuthorizationService from spiffworkflow_backend.services.file_system_service import FileSystemService from spiffworkflow_backend.services.user_service import UserService @@ -213,6 +214,7 @@ class ProcessModelService(FileSystemService): process_models = cls.get_process_models( process_group_id=process_group_id, recursive=recursive, include_files=include_files ) + process_model_identifiers = [p.id for p in process_models] permission_to_check = "read" permission_base_uri = "/v1.0/process-models" @@ -224,6 +226,22 @@ class ProcessModelService(FileSystemService): permission_to_check = "create" permission_base_uri = "/v1.0/extensions" + permitted_process_model_identifiers = cls.process_model_identifiers_with_permission_for_user( + user=user, + permission_to_check=permission_to_check, + permission_base_uri=permission_base_uri, + process_model_identifiers=process_model_identifiers, + ) + permitted_process_models = [] + for process_model in process_models: + if process_model.id in permitted_process_model_identifiers: + permitted_process_models.append(process_model) + return permitted_process_models + + @classmethod + def process_model_identifiers_with_permission_for_user( + cls, user: UserModel, permission_to_check: str, permission_base_uri: str, process_model_identifiers: list[str] + ) -> list[str]: # if user has access to uri/* with that permission then there's no reason to check each one individually guid_of_non_existent_item_to_check_perms_against = str(uuid.uuid4()) has_permission = AuthorizationService.user_has_permission( @@ -232,18 +250,21 @@ class ProcessModelService(FileSystemService): target_uri=f"{permission_base_uri}/{guid_of_non_existent_item_to_check_perms_against}", ) if has_permission: - return process_models + return process_model_identifiers - new_process_model_list = [] - for process_model in process_models: - modified_process_model_id = ProcessModelInfo.modify_process_identifier_for_path_param(process_model.id) + permitted_process_model_identifiers = [] + for process_model_identifier in process_model_identifiers: + modified_process_model_id = ProcessModelInfo.modify_process_identifier_for_path_param( + process_model_identifier + ) uri = f"{permission_base_uri}/{modified_process_model_id}" has_permission = AuthorizationService.user_has_permission( user=user, permission=permission_to_check, target_uri=uri ) if has_permission: - new_process_model_list.append(process_model) - return new_process_model_list + permitted_process_model_identifiers.append(process_model_identifier) + + return permitted_process_model_identifiers @classmethod def get_parent_group_array_and_cache_it( diff --git a/spiffworkflow-backend/src/spiffworkflow_backend/services/spec_file_service.py b/spiffworkflow-backend/src/spiffworkflow_backend/services/spec_file_service.py index 61b35fc1..a8006340 100644 --- a/spiffworkflow-backend/src/spiffworkflow_backend/services/spec_file_service.py +++ b/spiffworkflow-backend/src/spiffworkflow_backend/services/spec_file_service.py @@ -12,6 +12,8 @@ from spiffworkflow_backend.models.file import SpecReference from spiffworkflow_backend.models.message_triggerable_process_model import MessageTriggerableProcessModel from spiffworkflow_backend.models.process_model import ProcessModelInfo from spiffworkflow_backend.models.spec_reference import SpecReferenceCache +from spiffworkflow_backend.models.user import UserModel +from spiffworkflow_backend.services.authentication_service import NotAuthorizedError from spiffworkflow_backend.services.custom_parser import MyCustomParser from spiffworkflow_backend.services.file_system_service import FileSystemService from spiffworkflow_backend.services.process_caller_service import ProcessCallerService @@ -97,6 +99,8 @@ class SpecFileService(FileSystemService): sub_parsers = list(parser.process_parsers.values()) messages = parser.messages correlations = parser.correlations + # to check permissions for call activities + parser.get_process_dependencies() elif file_type.value == FileType.dmn.value: parser.add_dmn_xml(cls.get_etree_from_xml_bytes(binary_data)) sub_parsers = list(parser.dmn_parsers.values()) @@ -149,7 +153,9 @@ class SpecFileService(FileSystemService): ) from exception @classmethod - def update_file(cls, process_model_info: ProcessModelInfo, file_name: str, binary_data: bytes) -> File: + def update_file( + cls, process_model_info: ProcessModelInfo, file_name: str, binary_data: bytes, user: UserModel | None = None + ) -> File: SpecFileService.assert_valid_file_name(file_name) cls.validate_bpmn_xml(file_name, binary_data) @@ -157,6 +163,7 @@ class SpecFileService(FileSystemService): primary_process_ref = next((ref for ref in references if ref.is_primary and ref.is_executable), None) SpecFileService.clear_caches_for_file(file_name, process_model_info) + all_called_element_ids: set[str] = set() for ref in references: # If no valid primary process is defined, default to the first process in the # updated file. @@ -176,8 +183,35 @@ class SpecFileService(FileSystemService): process_model_info, update_hash, ) + + all_called_element_ids = all_called_element_ids | set(ref.called_element_ids) SpecFileService.update_caches(ref) + if user is not None: + called_element_refs = SpecReferenceCache.query.filter( + SpecReferenceCache.identifier.in_(all_called_element_ids) + ).all() + if len(called_element_refs) > 0: + process_model_identifiers: list[str] = [r.process_model_id for r in called_element_refs] + permitted_process_model_identifiers = ( + ProcessModelService.process_model_identifiers_with_permission_for_user( + user=user, + permission_to_check="create", + permission_base_uri="/v1.0/process-instances", + process_model_identifiers=process_model_identifiers, + ) + ) + unpermitted_process_model_identifiers = set(process_model_identifiers) - set( + permitted_process_model_identifiers + ) + if len(unpermitted_process_model_identifiers): + raise NotAuthorizedError( + "You are not authorized to use one or more processes as a called element:" + f" {','.join(unpermitted_process_model_identifiers)}" + ) + + db.session.commit() + # make sure we save the file as the last thing we do to ensure validations have run full_file_path = SpecFileService.full_file_path(process_model_info, file_name) SpecFileService.write_file_data_to_system(full_file_path, binary_data) @@ -251,7 +285,6 @@ class SpecFileService(FileSystemService): if process_id_lookup is None: process_id_lookup = SpecReferenceCache.from_spec_reference(ref) db.session.add(process_id_lookup) - db.session.commit() else: if ref.relative_path != process_id_lookup.relative_path: full_bpmn_file_path = SpecFileService.full_path_from_relative_path(process_id_lookup.relative_path) @@ -265,7 +298,6 @@ class SpecFileService(FileSystemService): else: process_id_lookup.relative_path = ref.relative_path db.session.add(process_id_lookup) - db.session.commit() @staticmethod def update_process_caller_cache(ref: SpecReference) -> None: @@ -284,7 +316,6 @@ class SpecFileService(FileSystemService): process_model_identifier=ref.process_model_id, ) db.session.add(message_triggerable_process_model) - db.session.commit() else: if message_triggerable_process_model.process_model_identifier != ref.process_model_id: raise ProcessModelFileInvalidError( @@ -315,4 +346,3 @@ class SpecFileService(FileSystemService): retrieval_expression=retrieval_expression, ) db.session.add(new_cache) - db.session.commit() diff --git a/spiffworkflow-backend/tests/spiffworkflow_backend/integration/test_process_api.py b/spiffworkflow-backend/tests/spiffworkflow_backend/integration/test_process_api.py index 43c545da..8c1d3907 100644 --- a/spiffworkflow-backend/tests/spiffworkflow_backend/integration/test_process_api.py +++ b/spiffworkflow-backend/tests/spiffworkflow_backend/integration/test_process_api.py @@ -521,7 +521,9 @@ class TestProcessApi(BaseTest): "/v1.0/processes", headers=self.logged_in_headers(with_super_admin_user), ) + assert response.status_code == 200 assert response.json is not None + assert isinstance(response.json, list) # We should get 5 back, as one of the items in the cache is a decision. assert len(response.json) == 5 simple_form = next(p for p in response.json if p["identifier"] == "Process_WithForm") @@ -531,6 +533,57 @@ class TestProcessApi(BaseTest): assert simple_form["is_executable"] is True assert simple_form["is_primary"] is True + def test_process_list_with_restricted_access( + self, + app: Flask, + client: FlaskClient, + with_db_and_bpmn_file_cleanup: None, + with_super_admin_user: UserModel, + ) -> None: + load_test_spec( + "test_group_one/simple_form", + process_model_source_directory="simple_form", + bpmn_file_name="simple_form", + ) + # When adding a process model with one Process, no decisions, and some json files, only one process is recorded. + assert len(SpecReferenceCache.query.all()) == 1 + + self.create_group_and_model_with_bpmn( + client=client, + user=with_super_admin_user, + process_group_id="test_group_two", + process_model_id="call_activity_nested", + bpmn_file_location="call_activity_nested", + ) + # When adding a process model with 4 processes and a decision, 5 new records will be in the Cache + assert len(SpecReferenceCache.query.all()) == 6 + + user_one = self.create_user_with_permission( + username="user_one", target_uri="/v1.0/process-groups/test_group_one:*" + ) + self.add_permissions_to_user(user=user_one, target_uri="/v1.0/processes", permission_names=["read"]) + self.add_permissions_to_user( + user=user_one, target_uri="/v1.0/process-instances/test_group_one:*", permission_names=["create"] + ) + + # get the results + response = client.get( + "/v1.0/processes", + headers=self.logged_in_headers(user_one), + ) + assert response.status_code == 200 + assert response.json is not None + + # This user should only have access to one process + assert isinstance(response.json, list) + assert len(response.json) == 1 + simple_form = next(p for p in response.json if p["identifier"] == "Process_WithForm") + assert simple_form["display_name"] == "Process With Form" + assert simple_form["process_model_id"] == "test_group_one/simple_form" + assert simple_form["has_lanes"] is False + assert simple_form["is_executable"] is True + assert simple_form["is_primary"] is True + def test_process_callers( self, app: Flask, diff --git a/spiffworkflow-backend/tests/spiffworkflow_backend/integration/test_process_models_controller.py b/spiffworkflow-backend/tests/spiffworkflow_backend/integration/test_process_models_controller.py new file mode 100644 index 00000000..7d936770 --- /dev/null +++ b/spiffworkflow-backend/tests/spiffworkflow_backend/integration/test_process_models_controller.py @@ -0,0 +1,62 @@ +import io +from hashlib import sha256 + +from flask.app import Flask +from flask.testing import FlaskClient +from spiffworkflow_backend.models.user import UserModel +from spiffworkflow_backend.services.spec_file_service import SpecFileService + +from tests.spiffworkflow_backend.helpers.base_test import BaseTest + + +class TestProcessModelsController(BaseTest): + def test_cannot_save_process_model_file_with_called_elements_user_does_not_have_access_to( + self, + app: Flask, + client: FlaskClient, + with_db_and_bpmn_file_cleanup: None, + with_super_admin_user: UserModel, + ) -> None: + process_model = self.create_group_and_model_with_bpmn( + client=client, + user=with_super_admin_user, + process_group_id="caller", + process_model_id="caller", + bpmn_file_location="call_activity_same_directory", + bpmn_file_name="call_activity_test.bpmn", + ) + self.create_group_and_model_with_bpmn( + client=client, + user=with_super_admin_user, + process_group_id="callee", + process_model_id="callee", + bpmn_file_location="call_activity_same_directory", + bpmn_file_name="callable_process.bpmn", + ) + + user_one = self.create_user_with_permission(username="user_one", target_uri="/v1.0/process-groups/caller:*") + self.add_permissions_to_user( + user=user_one, target_uri="/v1.0/process-models/caller:*", permission_names=["create", "read", "update"] + ) + assert process_model.primary_file_name is not None + bpmn_file_data_bytes = SpecFileService.get_data(process_model, process_model.primary_file_name) + file_contents_hash = sha256(bpmn_file_data_bytes).hexdigest() + + data = {"file": (io.BytesIO(bpmn_file_data_bytes), process_model.primary_file_name)} + url = ( + f"/v1.0/process-models/{process_model.modified_process_model_identifier()}/files/" + f"{process_model.primary_file_name}?file_contents_hash={file_contents_hash}" + ) + response = client.put( + url, + data=data, + follow_redirects=True, + content_type="multipart/form-data", + headers=self.logged_in_headers(user_one), + ) + + assert response.status_code == 403 + assert response.json is not None + assert response.json["message"].startswith( + "NotAuthorizedError: You are not authorized to use one or more processes as a called element" + )