From 2916e5f1c5f2dca41ab9d8097ea506f48423a2fc Mon Sep 17 00:00:00 2001 From: jasquat Date: Wed, 25 Oct 2023 14:25:16 -0400 Subject: [PATCH 1/5] added test and some additional support for deny permissions w/ burnettk --- spiffworkflow-backend/bin/get_perms | 2 +- .../src/spiffworkflow_backend/api.yml | 2 +- .../services/authorization_service.py | 25 ++++++++++++++----- .../integration/test_process_api.py | 2 +- .../unit/test_authorization_service.py | 25 +++++++++++++++++++ 5 files changed, 47 insertions(+), 9 deletions(-) diff --git a/spiffworkflow-backend/bin/get_perms b/spiffworkflow-backend/bin/get_perms index 5f561b014..3a3c27860 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 553ea0726..b954d5db2 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 061928210..cb6761463 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 @@ def has_permission(cls, principals: list[PrincipalModel], permission: str, targe ) .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: @@ -214,18 +219,23 @@ def create_permission_for_principal( principal: PrincipalModel, permission_target: PermissionTargetModel, permission: str, + grant_type: str | None = None, ) -> PermissionAssignmentModel: + if grant_type is None: + grant_type = "permit" + permission_assignment: PermissionAssignmentModel | None = PermissionAssignmentModel.query.filter_by( principal_id=principal.id, permission_target_id=permission_target.id, permission=permission, + grant_type=grant_type, ).first() if permission_assignment is None: permission_assignment = PermissionAssignmentModel( 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() @@ -694,7 +704,7 @@ def explode_permissions(cls, permission_set: str, target: str) -> list[Permissio @classmethod def add_permission_from_uri_or_macro( - cls, group_identifier: str, permission: str, target: str + cls, group_identifier: str, permission: str, target: str, grant_type: str | None = None ) -> list[PermissionAssignmentModel]: group = UserService.find_or_create_group(group_identifier) permissions_to_assign = cls.explode_permissions(permission, target) @@ -703,7 +713,10 @@ def add_permission_from_uri_or_macro( 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/integration/test_process_api.py b/spiffworkflow-backend/tests/spiffworkflow_backend/integration/test_process_api.py index 9f1cbbef1..c3a6775c5 100644 --- a/spiffworkflow-backend/tests/spiffworkflow_backend/integration/test_process_api.py +++ b/spiffworkflow-backend/tests/spiffworkflow_backend/integration/test_process_api.py @@ -3295,7 +3295,7 @@ def test_process_data_show( 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 76427754e..72162e3ae 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,28 @@ def test_can_refresh_permissions_with_regexes( 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", "PG:hey:yo", "deny") + AuthorizationService.add_permission_from_uri_or_macro( + user_group.identifier, "read", "/process-groups/hey:new", "deny" + ) + + 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) From 9bf2ce0e19caeb01fdc46d748525e9a3e494208c Mon Sep 17 00:00:00 2001 From: jasquat Date: Wed, 25 Oct 2023 15:01:50 -0400 Subject: [PATCH 2/5] added support for deny through permissions-check api w/ burnettk --- .../services/authorization_service.py | 25 ++++++++++--------- .../helpers/base_test.py | 13 +++++++--- .../integration/test_process_api.py | 5 ++++ .../src/routes/ProcessInstanceShow.tsx | 2 +- 4 files changed, 29 insertions(+), 16 deletions(-) diff --git a/spiffworkflow-backend/src/spiffworkflow_backend/services/authorization_service.py b/spiffworkflow-backend/src/spiffworkflow_backend/services/authorization_service.py index cb6761463..4d9c55a8c 100644 --- a/spiffworkflow-backend/src/spiffworkflow_backend/services/authorization_service.py +++ b/spiffworkflow-backend/src/spiffworkflow_backend/services/authorization_service.py @@ -156,17 +156,21 @@ def permission_assignments_include( ) -> 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: @@ -219,11 +223,8 @@ def create_permission_for_principal( principal: PrincipalModel, permission_target: PermissionTargetModel, permission: str, - grant_type: str | None = None, + grant_type: str = "permit", ) -> PermissionAssignmentModel: - if grant_type is None: - grant_type = "permit" - permission_assignment: PermissionAssignmentModel | None = PermissionAssignmentModel.query.filter_by( principal_id=principal.id, permission_target_id=permission_target.id, @@ -704,7 +705,7 @@ def explode_permissions(cls, permission_set: str, target: str) -> list[Permissio @classmethod def add_permission_from_uri_or_macro( - cls, group_identifier: str, permission: str, target: str, grant_type: str | None = None + cls, group_identifier: str, permission: str, target: str, grant_type: str = "permit" ) -> list[PermissionAssignmentModel]: group = UserService.find_or_create_group(group_identifier) permissions_to_assign = cls.explode_permissions(permission, target) diff --git a/spiffworkflow-backend/tests/spiffworkflow_backend/helpers/base_test.py b/spiffworkflow-backend/tests/spiffworkflow_backend/helpers/base_test.py index b004748a7..09d724688 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 @@ def create_user_with_permission( 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 @@ def add_permissions_to_user( 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 @@ def add_permissions_to_principal( 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 c3a6775c5..08dc26175 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 @@ def test_permissions_check_with_wildcard_permissions_through_group( 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 @@ def test_permissions_check_with_wildcard_permissions_through_group( 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}, } diff --git a/spiffworkflow-frontend/src/routes/ProcessInstanceShow.tsx b/spiffworkflow-frontend/src/routes/ProcessInstanceShow.tsx index b6d55fd7d..38b66b259 100644 --- a/spiffworkflow-frontend/src/routes/ProcessInstanceShow.tsx +++ b/spiffworkflow-frontend/src/routes/ProcessInstanceShow.tsx @@ -646,7 +646,7 @@ 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, }); From 15df778a6061f48f997b7ee7a9292eaf03f5b197 Mon Sep 17 00:00:00 2001 From: jasquat Date: Wed, 25 Oct 2023 15:21:10 -0400 Subject: [PATCH 3/5] support DENY at the beginning of a permission target marcro --- .../services/authorization_service.py | 9 +++++++-- .../unit/test_authorization_service.py | 4 ++-- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/spiffworkflow-backend/src/spiffworkflow_backend/services/authorization_service.py b/spiffworkflow-backend/src/spiffworkflow_backend/services/authorization_service.py index 4d9c55a8c..691931608 100644 --- a/spiffworkflow-backend/src/spiffworkflow_backend/services/authorization_service.py +++ b/spiffworkflow-backend/src/spiffworkflow_backend/services/authorization_service.py @@ -705,10 +705,15 @@ def explode_permissions(cls, permission_set: str, target: str) -> list[Permissio @classmethod def add_permission_from_uri_or_macro( - cls, group_identifier: str, permission: str, target: str, grant_type: str = "permit" + 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) 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 72162e3ae..7577ba5e1 100644 --- a/spiffworkflow-backend/tests/spiffworkflow_backend/unit/test_authorization_service.py +++ b/spiffworkflow-backend/tests/spiffworkflow_backend/unit/test_authorization_service.py @@ -603,9 +603,9 @@ def test_can_deny_access_with_permission( 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", "PG:hey:yo", "deny") + 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", "/process-groups/hey:new", "deny" + user_group.identifier, "read", "DENY:/process-groups/hey:new" ) self.assert_user_has_permission(user, "read", "/v1.0/process-groups/hey") From 7eb4d1b0c56d570ca358edffacb946fd6be86b24 Mon Sep 17 00:00:00 2001 From: jasquat Date: Wed, 25 Oct 2023 15:37:32 -0400 Subject: [PATCH 4/5] do not look up permissions using grant type, only use the uniqueness key --- .../spiffworkflow_backend/services/authorization_service.py | 5 ++++- .../spiffworkflow_backend/unit/test_authorization_service.py | 5 +++++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/spiffworkflow-backend/src/spiffworkflow_backend/services/authorization_service.py b/spiffworkflow-backend/src/spiffworkflow_backend/services/authorization_service.py index 691931608..38e12b794 100644 --- a/spiffworkflow-backend/src/spiffworkflow_backend/services/authorization_service.py +++ b/spiffworkflow-backend/src/spiffworkflow_backend/services/authorization_service.py @@ -229,7 +229,6 @@ def create_permission_for_principal( principal_id=principal.id, permission_target_id=permission_target.id, permission=permission, - grant_type=grant_type, ).first() if permission_assignment is None: permission_assignment = PermissionAssignmentModel( @@ -240,6 +239,10 @@ def create_permission_for_principal( ) 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 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 7577ba5e1..2ba80c283 100644 --- a/spiffworkflow-backend/tests/spiffworkflow_backend/unit/test_authorization_service.py +++ b/spiffworkflow-backend/tests/spiffworkflow_backend/unit/test_authorization_service.py @@ -617,3 +617,8 @@ def test_can_deny_access_with_permission( # 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) From 08fb74bd9f07eae073d15fb1491fddd3bc47e890 Mon Sep 17 00:00:00 2001 From: jasquat Date: Wed, 25 Oct 2023 16:18:51 -0400 Subject: [PATCH 5/5] added support in frontend to display a nice error if user does not have access to a data object value w/ burnettk --- spiffworkflow-frontend/src/interfaces.ts | 2 ++ .../src/routes/ProcessInstanceShow.tsx | 32 +++++++++++++++++-- 2 files changed, 32 insertions(+), 2 deletions(-) diff --git a/spiffworkflow-frontend/src/interfaces.ts b/spiffworkflow-frontend/src/interfaces.ts index 24976003c..476afcb0e 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 38b66b259..7c0de6803 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 @@ -649,6 +675,8 @@ export default function ProcessInstanceShow({ variant }: OwnProps) { 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) => {