From 39edd60fd69ccb6637f79d9ab7714e08ad83b007 Mon Sep 17 00:00:00 2001 From: jasquat <2487833+jasquat@users.noreply.github.com> Date: Wed, 25 Oct 2023 16:31:06 -0400 Subject: [PATCH] Feature/hide private data objects (#581) * added test and some additional support for deny permissions w/ burnettk * added support for deny through permissions-check api w/ burnettk * support DENY at the beginning of a permission target marcro * do not look up permissions using grant type, only use the uniqueness key * added support in frontend to display a nice error if user does not have access to a data object value w/ burnettk --------- Co-authored-by: jasquat --- spiffworkflow-backend/bin/get_perms | 2 +- .../src/spiffworkflow_backend/api.yml | 2 +- .../services/authorization_service.py | 48 ++++++++++++++----- .../helpers/base_test.py | 13 +++-- .../integration/test_process_api.py | 7 ++- .../unit/test_authorization_service.py | 30 ++++++++++++ spiffworkflow-frontend/src/interfaces.ts | 2 + .../src/routes/ProcessInstanceShow.tsx | 34 +++++++++++-- 8 files changed, 116 insertions(+), 22 deletions(-) diff --git a/spiffworkflow-backend/bin/get_perms b/spiffworkflow-backend/bin/get_perms index 5f561b01..3a3c2786 100755 --- a/spiffworkflow-backend/bin/get_perms +++ b/spiffworkflow-backend/bin/get_perms @@ -19,7 +19,7 @@ mysql -uroot "$database" -e ' JOIN `user_group_assignment` uga ON uga.user_id = u.id JOIN `group` g ON g.id = uga.group_id; - select pa.id, g.identifier group_identifier, pt.uri, permission from permission_assignment pa + select pa.id pa_id, g.identifier group_identifier, pt.uri, pa.grant_type, permission, p.id principal_id from permission_assignment pa JOIN principal p ON p.id = pa.principal_id JOIN `group` g ON g.id = p.group_id JOIN permission_target pt ON pt.id = pa.permission_target_id; diff --git a/spiffworkflow-backend/src/spiffworkflow_backend/api.yml b/spiffworkflow-backend/src/spiffworkflow_backend/api.yml index 553ea072..b954d5db 100755 --- a/spiffworkflow-backend/src/spiffworkflow_backend/api.yml +++ b/spiffworkflow-backend/src/spiffworkflow_backend/api.yml @@ -1803,7 +1803,7 @@ paths: schema: $ref: "#/components/schemas/OkTrue" - /process-data/{modified_process_model_identifier}/{process_instance_id}/{process_data_identifier}: + /process-data/{modified_process_model_identifier}/{process_data_identifier}/{process_instance_id}: parameters: - name: modified_process_model_identifier in: path diff --git a/spiffworkflow-backend/src/spiffworkflow_backend/services/authorization_service.py b/spiffworkflow-backend/src/spiffworkflow_backend/services/authorization_service.py index 06192821..38e12b79 100644 --- a/spiffworkflow-backend/src/spiffworkflow_backend/services/authorization_service.py +++ b/spiffworkflow-backend/src/spiffworkflow_backend/services/authorization_service.py @@ -121,15 +121,20 @@ class AuthorizationService: ) .all() ) + + if len(permission_assignments) == 0: + return False + + all_permissions_permit = True for permission_assignment in permission_assignments: if permission_assignment.grant_type == "permit": - return True + pass elif permission_assignment.grant_type == "deny": - return False + all_permissions_permit = False else: raise Exception(f"Unknown grant type: {permission_assignment.grant_type}") - return False + return all_permissions_permit @classmethod def user_has_permission(cls, user: UserModel, permission: str, target_uri: str) -> bool: @@ -151,17 +156,21 @@ class AuthorizationService: ) -> bool: uri_with_percent = re.sub(r"\*", "%", target_uri) target_uri_normalized = uri_with_percent.removeprefix(V1_API_PATH_PREFIX) + + matching_permission_assignments = [] for permission_assignment in permission_assignments: if permission_assignment.permission == permission and cls.target_uri_matches_actual_uri( permission_assignment.permission_target.uri, target_uri_normalized ): - # we might have to rethink this to actually support deny - if permission_assignment.grant_type == "permit": - return True - elif permission_assignment.grant_type == "deny": - return False - return True - return False + matching_permission_assignments.append(permission_assignment) + if len(matching_permission_assignments) == 0: + return False + + all_permissions_permit = True + for permission_assignment in matching_permission_assignments: + if permission_assignment.grant_type == "deny": + all_permissions_permit = False + return all_permissions_permit @classmethod def target_uri_matches_actual_uri(cls, target_uri: str, actual_uri: str) -> bool: @@ -214,6 +223,7 @@ class AuthorizationService: principal: PrincipalModel, permission_target: PermissionTargetModel, permission: str, + grant_type: str = "permit", ) -> PermissionAssignmentModel: permission_assignment: PermissionAssignmentModel | None = PermissionAssignmentModel.query.filter_by( principal_id=principal.id, @@ -225,10 +235,14 @@ class AuthorizationService: principal_id=principal.id, permission_target_id=permission_target.id, permission=permission, - grant_type="permit", + grant_type=grant_type, ) db.session.add(permission_assignment) db.session.commit() + elif permission_assignment.grant_type != grant_type: + permission_assignment.grant_type = grant_type + db.session.add(permission_assignment) + db.session.commit() return permission_assignment @classmethod @@ -697,13 +711,21 @@ class AuthorizationService: cls, group_identifier: str, permission: str, target: str ) -> list[PermissionAssignmentModel]: group = UserService.find_or_create_group(group_identifier) - permissions_to_assign = cls.explode_permissions(permission, target) + grant_type = "permit" + target_without_deny = target + if target.startswith("DENY:"): + target_without_deny = target.removeprefix("DENY:") + grant_type = "deny" + permissions_to_assign = cls.explode_permissions(permission, target_without_deny) permission_assignments = [] for permission_to_assign in permissions_to_assign: permission_target = cls.find_or_create_permission_target(permission_to_assign.target_uri) permission_assignments.append( cls.create_permission_for_principal( - group.principal, permission_target, permission_to_assign.permission + principal=group.principal, + permission_target=permission_target, + permission=permission_to_assign.permission, + grant_type=grant_type, ) ) return permission_assignments diff --git a/spiffworkflow-backend/tests/spiffworkflow_backend/helpers/base_test.py b/spiffworkflow-backend/tests/spiffworkflow_backend/helpers/base_test.py index b004748a..09d72468 100644 --- a/spiffworkflow-backend/tests/spiffworkflow_backend/helpers/base_test.py +++ b/spiffworkflow-backend/tests/spiffworkflow_backend/helpers/base_test.py @@ -312,9 +312,12 @@ class BaseTest: username: str, target_uri: str = PermissionTargetModel.URI_ALL, permission_names: list[str] | None = None, + grant_type: str = "permit", ) -> UserModel: user = BaseTest.find_or_create_user(username=username) - return cls.add_permissions_to_user(user, target_uri=target_uri, permission_names=permission_names) + return cls.add_permissions_to_user( + user, target_uri=target_uri, permission_names=permission_names, grant_type=grant_type + ) @classmethod def add_permissions_to_user( @@ -322,14 +325,17 @@ class BaseTest: user: UserModel, target_uri: str = PermissionTargetModel.URI_ALL, permission_names: list[str] | None = None, + grant_type: str = "permit", ) -> UserModel: principal = user.principal - cls.add_permissions_to_principal(principal, target_uri=target_uri, permission_names=permission_names) + cls.add_permissions_to_principal( + principal, target_uri=target_uri, permission_names=permission_names, grant_type=grant_type + ) return user @classmethod def add_permissions_to_principal( - cls, principal: PrincipalModel, target_uri: str, permission_names: list[str] | None + cls, principal: PrincipalModel, target_uri: str, permission_names: list[str] | None, grant_type: str = "permit" ) -> None: permission_target = AuthorizationService.find_or_create_permission_target(target_uri) @@ -341,6 +347,7 @@ class BaseTest: principal=principal, permission_target=permission_target, permission=permission, + grant_type=grant_type, ) def assert_user_has_permission( 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 9f1cbbef..08dc2617 100644 --- a/spiffworkflow-backend/tests/spiffworkflow_backend/integration/test_process_api.py +++ b/spiffworkflow-backend/tests/spiffworkflow_backend/integration/test_process_api.py @@ -106,12 +106,16 @@ class TestProcessApi(BaseTest): principal = group.principal UserService.add_user_to_group(user, group) self.add_permissions_to_principal(principal, target_uri="/v1.0/process-groups/%", permission_names=["read"]) + self.add_permissions_to_principal( + principal, target_uri="/v1.0/process-groups/deny_group:%", permission_names=["create"], grant_type="deny" + ) self.add_permissions_to_principal( principal, target_uri="/v1.0/process-groups/test_group:%", permission_names=["create"] ) request_body = { "requests_to_check": { "/v1.0/process-groups": ["GET", "POST"], + "/v1.0/process-groups/deny_group:hey": ["GET", "POST"], "/v1.0/process-groups/test_group": ["GET", "POST"], "/v1.0/process-models": ["GET"], } @@ -119,6 +123,7 @@ class TestProcessApi(BaseTest): expected_response_body = { "results": { "/v1.0/process-groups": {"GET": True, "POST": False}, + "/v1.0/process-groups/deny_group:hey": {"GET": True, "POST": False}, "/v1.0/process-groups/test_group": {"GET": True, "POST": True}, "/v1.0/process-models": {"GET": False}, } @@ -3295,7 +3300,7 @@ class TestProcessApi(BaseTest): assert process_instance_one.status == "user_input_required" response = client.get( - f"/v1.0/process-data/{self.modify_process_identifier_for_path_param(process_model.id)}/{process_instance_one.id}/the_data_object_var", + f"/v1.0/process-data/{self.modify_process_identifier_for_path_param(process_model.id)}/the_data_object_var/{process_instance_one.id}", headers=self.logged_in_headers(with_super_admin_user), ) diff --git a/spiffworkflow-backend/tests/spiffworkflow_backend/unit/test_authorization_service.py b/spiffworkflow-backend/tests/spiffworkflow_backend/unit/test_authorization_service.py index 76427754..2ba80c28 100644 --- a/spiffworkflow-backend/tests/spiffworkflow_backend/unit/test_authorization_service.py +++ b/spiffworkflow-backend/tests/spiffworkflow_backend/unit/test_authorization_service.py @@ -592,3 +592,33 @@ class TestAuthorizationService(BaseTest): waiting_assignments = UserGroupAssignmentWaitingModel.query.all() # ensure we didn't delete all of the user group assignments assert len(waiting_assignments) > 0 + + def test_can_deny_access_with_permission( + self, + app: Flask, + client: FlaskClient, + with_db_and_bpmn_file_cleanup: None, + ) -> None: + user = self.find_or_create_user(username="user_one") + user_group = UserService.find_or_create_group("group_one") + UserService.add_user_to_group(user, user_group) + AuthorizationService.add_permission_from_uri_or_macro(user_group.identifier, "read", "PG:hey") + AuthorizationService.add_permission_from_uri_or_macro(user_group.identifier, "read", "DENY:PG:hey:yo") + AuthorizationService.add_permission_from_uri_or_macro( + user_group.identifier, "read", "DENY:/process-groups/hey:new" + ) + + self.assert_user_has_permission(user, "read", "/v1.0/process-groups/hey") + + # test specific uri deny + self.assert_user_has_permission(user, "read", "/v1.0/process-groups/hey:yo", expected_result=False) + self.assert_user_has_permission(user, "read", "/v1.0/process-groups/hey:yo:me", expected_result=False) + + # test wildcard deny + self.assert_user_has_permission(user, "read", "/v1.0/process-groups/hey:new", expected_result=False) + self.assert_user_has_permission(user, "read", "/v1.0/process-groups/hey:new:group", expected_result=True) + + # test it can be permitted again + AuthorizationService.add_permission_from_uri_or_macro(user_group.identifier, "read", "PG:hey:yo") + self.assert_user_has_permission(user, "read", "/v1.0/process-groups/hey:yo", expected_result=True) + self.assert_user_has_permission(user, "read", "/v1.0/process-groups/hey:yo:me", expected_result=True) diff --git a/spiffworkflow-frontend/src/interfaces.ts b/spiffworkflow-frontend/src/interfaces.ts index 24976003..476afcb0 100644 --- a/spiffworkflow-frontend/src/interfaces.ts +++ b/spiffworkflow-frontend/src/interfaces.ts @@ -29,6 +29,8 @@ export interface Onboarding { export interface ProcessData { process_data_identifier: string; process_data_value: any; + + authorized?: boolean; } export interface RecentProcessModel { diff --git a/spiffworkflow-frontend/src/routes/ProcessInstanceShow.tsx b/spiffworkflow-frontend/src/routes/ProcessInstanceShow.tsx index b6d55fd7..7c0de680 100644 --- a/spiffworkflow-frontend/src/routes/ProcessInstanceShow.tsx +++ b/spiffworkflow-frontend/src/routes/ProcessInstanceShow.tsx @@ -619,6 +619,21 @@ export default function ProcessInstanceShow({ variant }: OwnProps) { const processDataDisplayArea = () => { if (processDataToDisplay) { + let bodyComponent = ( + <> +

Value:

+ 
{JSON.stringify(processDataToDisplay.process_data_value)}
+ + ); + if (processDataToDisplay.authorized === false) { + bodyComponent = ( + <> + {childrenForErrorObject( + errorForDisplayFromString(processDataToDisplay.process_data_value) + )} + + ); + } return (

Data Object: {processDataToDisplay.process_data_identifier}


-

Value:

- 
{JSON.stringify(processDataToDisplay.process_data_value)}
+ {bodyComponent} ); } @@ -639,6 +653,18 @@ export default function ProcessInstanceShow({ variant }: OwnProps) { setProcessDataToDisplay(processData); }; + const handleProcessDataShowReponseUnauthorized = ( + dataObjectIdentifer: string, + result: any + ) => { + const processData: ProcessData = { + process_data_identifier: dataObjectIdentifer, + process_data_value: result.message, + authorized: false, + }; + setProcessDataToDisplay(processData); + }; + const handleClickedDiagramTask = ( shapeElement: any, bpmnProcessIdentifiers: any @@ -646,9 +672,11 @@ export default function ProcessInstanceShow({ variant }: OwnProps) { if (shapeElement.type === 'bpmn:DataObjectReference') { const dataObjectIdentifer = shapeElement.businessObject.dataObjectRef.id; HttpService.makeCallToBackend({ - path: `/process-data/${params.process_model_id}/${params.process_instance_id}/${dataObjectIdentifer}`, + path: `/process-data/${params.process_model_id}/${dataObjectIdentifer}/${params.process_instance_id}`, httpMethod: 'GET', successCallback: handleProcessDataShowResponse, + onUnauthorized: (result: any) => + handleProcessDataShowReponseUnauthorized(dataObjectIdentifer, result), }); } else if (tasks) { const matchingTask: Task | undefined = tasks.find((task: Task) => {