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 <jasquat@users.noreply.github.com>

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;

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

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
+                matching_permission_assignments.append(permission_assignment)
-                if permission_assignment.grant_type == "permit":
+        if len(matching_permission_assignments) == 0:
-                    return True
+            return False
-                elif permission_assignment.grant_type == "deny":
+
-                    return False
+        all_permissions_permit = True
-                return True
+        for permission_assignment in matching_permission_assignments:
-        return False
+            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

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(

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),
         )
 

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)

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 {

spiffworkflow-frontend/src/routes/ProcessInstanceShow.tsx

@@ -619,6 +619,21 @@ export default function ProcessInstanceShow({ variant }: OwnProps) {
 
   const processDataDisplayArea = () => {
     if (processDataToDisplay) {
+      let bodyComponent = (
+        <>
+          <p>Value:</p>
+          <pre>{JSON.stringify(processDataToDisplay.process_data_value)}</pre>
+        </>
+      );
+      if (processDataToDisplay.authorized === false) {
+        bodyComponent = (
+          <>
+            {childrenForErrorObject(
+              errorForDisplayFromString(processDataToDisplay.process_data_value)
+            )}
+          </>
+        );
+      }
       return (
         <Modal
           open={!!processDataToDisplay}
@@ -627,8 +642,7 @@ export default function ProcessInstanceShow({ variant }: OwnProps) {
         >
           <h2>Data Object: {processDataToDisplay.process_data_identifier}</h2>
           <br />
-          <p>Value:</p>
+          {bodyComponent}
-          <pre>{JSON.stringify(processDataToDisplay.process_data_value)}</pre>
         </Modal>
       );
     }
@@ -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) => {