sartography / spiff-arena

SpiffWorkflow is a software development platform for building, running, and monitoring executable diagrams
https://www.spiffworkflow.org/
GNU Lesser General Public License v2.1
48 stars 36 forks source link

Private fields need to be hidden from admins/support (data object) #574

Open calexh-sar opened 7 months ago

calexh-sar commented 7 months ago

We will have the need to create processes which include sensitive data.

We want the option to have 2 types of processes:

  1. Integration process which copy data from 1 system to another, using service tasks which hold the sensitive data in-memory and clear memory on completion
  2. Processes where some fields are sensitive and so will be stored in a data object and data store which is limited to access by process participants only. Even admins will not have access to these.

When defining a field in BPMN, we should have the option to a) store field in memory only (out-of-scope for this ticket) b) mark field as sensitive, so only process participants can see the field.

probably store the setting for the data object in an extension in the xml. probably set it via a checkbox in the properties panel.

right now the URL looks like this, where the data object name is salary:

/process-data/compensation-model/salary/34

we could set up deny rules for this in the permission system, like DENY for:

/process-data/compensation-model/salary/*

this is already implemented and would work, but people have expressed concern that if data object names change, permissions will need to be updated.

to resolve this, we could make a convention for data object names, like a "sensitive" prefix, where the part after the "sensitive" would be free to change:

/process-data/compensation-model/sensitive-salary/34

but i'm not certain this sort of permission wildcarding works:

/process-data/compensation-model/sensitive-*

perhaps BPMN architects will not want the forced naming convention. there could be a separate user-facing name field, but this might add confusion, since the name is very important and is used programatically.

another way to make it less common to edit permission rules: we could introduce another property on the data object. if it were a checkbox for "sensitive," perhaps we would always add this identifier to the beginning of the url, like so:

/process-data/sensitive/compensation-model/salary/34

or like this if the data object was not sensitive:

/process-data/default/compensation-model/salary/34

this way, we could establish permission rules that would allow access to:

/process-data/default/compensation-model/*

if we were to add this sort of property, we might instead decide to make it more flexible, and call it a "category" and let the user type a url-safe category in a text box when defining the data object, like "finance" or "compensation" and then it would be placed into the same place in the url:

/process-data/finance/compensation-model/* /process-data/compensation/compensation-model/*

jasquat commented 6 months ago

Deny supported in PR #581.

madhurrya commented 6 months ago

Tested Deny permission. image

madhurrya commented 6 months ago

Moving this to In progress based on Harmeet's comment here. https://www.notion.so/Hide-private-data-objects-from-admins-04150f4e82284e96b9c43c0f4dd3775c?pvs=4

harmeet-status commented 6 months ago

Copying that comment here:

@madhurrya the permissions to the Data Object should be handled as such:

A data object should have a property, where you can define if it contains sensitive information. Data Objects should have read permissions, explicitly defined. By default, if data object == sensitive, then no one gets access to this data objects, not even admins.

In order to get read permissions to data object, we should explicitly define this in the permissions table.

harmeet-status commented 6 months ago

@mrtrosen this is what we talked about private data in the Data Object.

harmeet-status commented 6 months ago

From now all, all discussion regarding this ticket is being moved to GitHub (here).

Had a chat with @burnettk about how we would implement the permissions on the Data Object. The original plan was to create a property called sensitive on the Data Object. When the system reads the data object as sensitive, then it will auto deny all permissions, unless permission is explicitly given.

The problem with this approach is that permissions are applied on URL basis, so we wouldnt be able to determine if a data object should be auto allow or deny permissions. In order to make the management of this easy, we did want that if a user is given the read permissions to a process model, then any data objects in that process model also be given read permissions.

Here are some options discussed:

  1. We could use a naming convention in the name of the data object, e.g. sensitive-invoice-data, when the system reads sensitive, it auto assumes that permission will be denied unless explicitly stated in the permissions file. The problem with this approach is that it enforced a naming convention, which BPMN architects may not like
  2. The second option is similar to the first option, but we introduce a user-facing name field, which does not need to have sensitive as part of the name. The system-name field will still have sensitive field, so the permissions model using URL will still work as the data object will be referenced using it's system name

Example of URL to reference the data object: /process-data/[process model name]/[data object name]/[process instance id]

e.g. /process-data/compensation/salary/34

sashayar13 commented 6 months ago

@harmeet-status I'm not sure if I got this rightIn order to make the management of this easy, we did want that if a user is given the read permissions to a process model, then any data objects in that process model also be given read permissions. But if a user granted "read" permission to a process model <> user can see the Data Object's data. Note, that only users who have access to the Task with which the Data Object is associated can "read" the data (through the human tasks UI).

sashayar13 commented 6 months ago

@burnettk @harmeet-status The requirement from my side would be to configure a solution that will not depend on the name hard-coded by a process architect. It should be an attribute that then creates the URL for permission or something.

Another food for thought is that if our process models explicitly show in BPMN/DMN files stored in GitHub that there is smth "sensitive", it can attract bad guys to hack the system, and target their activities on users who can access the sensitive data.

harmeet-status commented 6 months ago

@burnettk I'm ok with setting up groups (which Daniel suggested), and assigning data objects to groups for setting permissions. Do you see any issues with this approach?

E.g.

burnettk commented 6 months ago

seems good to me. maybe we decide to file a new ticket for this, since we've already burned this one down, but no big deal either way.

harmeet-status commented 6 months ago

Nah let's just update the estimate on this ticket and keep it all together, so that when we come back in the future, it becomes easier to track.

jasquat commented 5 months ago

Category is implemented with #796 .

dinithihj commented 4 months ago

@burnettk @jasquat Need help on this ticket. I have created a sample process model https://dev.app.spiff.status.im/process-models/misc:qa:data-object-permissions

On Data Object Properties - Given the Data Object Category as restricted. image

On the group_permissions_dev.dmn Added a new entry as below image

I'm not sure if the steps mentioned above are correct, and even if they are, I'm not sure what to do next for testing.

jasquat commented 4 months ago

@dinithihj The steps are mostly correct. Instead of setting the group to "restricted" though, it may be better to set it to "internal". Then you can make sure that admins can see the data from the data object after running the process model, and a normal user like "core" cannot.

burnettk commented 4 months ago

@usama9500 i'm not sure if the video you recorded the other day (that included a full setup of this feature) is something you could easily send to dinithi.

usama9500 commented 4 months ago

Sent @burnettk

dinithihj commented 4 months ago

Thank you @KB, @jasquat and @usama9500.

dinithihj commented 4 months ago

Tested and verified on dev.app (Version 320/322) and feature working as expected. Tested using model https://dev.app.spiff.status.im/process-models/misc:qa:data-object-permissions

Set Data Object Category as below on properties panel image

Added new entry to the permission group_permissions as below - everyone does not have permission to read data object image

The administrator see below message when tries to access data object image Normal user also sees the same message image

When support group does not have read permissions image Admin can access the data object image Support group member cannot read data object and sees below message image

usama9500 commented 4 months ago

Documented https://github.com/sartography/spiff-arena/pull/899

dinithihj commented 3 months ago

Released to prod on 2024 February 12