search

Health-Informatics-UoN / Carrot-Mapper

Convenient And Reusable Rapid Omop Transformer.
https://carrot.ac.uk
MIT License
14 stars 3 forks source link

Possible permissions regression - non-critical #797

Open spco opened 2 months ago

spco commented 2 months ago

Is there an existing issue for this?

Current Behavior

Take a Dataset with Restricted viewership. Inside sits a Scan Report. If a user is not explicitly given permissions to the parent Dataset, but is explicitly designated as a Viewer of the Scan Report, they still cannot see the Scan Report.

Expected Behavior

I would expect the user to be able to View the Scan Report, regardless of their status within the Dataset. See the logic as presented at https://carrot4omop.ac.uk/Carrot-Mapper/projects-datasets-and-scanreports/ . The rationale for this is it allows Dataset owners to grant view/edit permissions on a per-Scan Report basis, without having to grant any permissions to individuals across the full Dataset.

Steps To Reproduce

Create a Dataset with Restricted viewership. Add a child Scan Report. Add another user, who is not a viewer/editor/admin of the Dataset, as a Viewer/Editor on the Scan Report. That user tries to view the Scan Report and cannot. The Scan Report does not show up on their Scan Report List page. (I haven't checked whether they can access the SR directly by URL)

Environment

- OS: NA
- Other environment details: NA

I'm part of a Project Team

No response

Anything else?

No response

Are you willing to contribute to resolve this issue?

None

AndyRae commented 2 months ago

Yes that makes sense, thanks Sam. Introduced in #779 - we will investigate.

AndyRae commented 2 months ago

So looking at this more closely, it wasn't introduced in #779, but I think was pre-existing. I'd note that the Access Controls are slightly ambiguous - it initially indicates to me that the rules are combined, but it makes much more sense that a user would want to override it at a SR level.

Currently a Scan Report will not be shown in the SR list if the user does not have access to the dataset, but is a viewer/editor on the SR. (this filtering has not changed). See: https://github.com/Health-Informatics-UoN/CaRROT-Mapper/blob/fix/761/pagination-ordering/app/api/api/views.py#L348

The desired behaviour is slightly more tricky to implement safely. As granting access to a Scan Report is fine in itself (it is just a matter of changing that filtering), but they then need some access to the parent dataset. The current behaviour on the dataset/ and datasets_data_partners/ (list datasets) requires the user to have privileges on a dataset to access it.

For example: A dataset is restricted, but a user is a viewer/editor on a child Scan Report within it. The user accesses the /details page of a Scan Report (the edit SR form), this will access the Scan Report Dataset, and the list of datasets to populate in the form (see above endpoints). However, that user can't access the parent dataset detail, and it will not be in the list either. Therefore the form is broken and will error - we can't add logic to these endpoints that would check if the user is a member of any child SR.

The fix here is actually to stop using the datasets/ API for this kind of information, but to serialise the desired information about the parent dataset on the Scan Report model. This would further simplify the form also.

spco commented 2 months ago

Thanks Andy. I was careful to word it as a "possible" regression as I couldn't be sure :)

Agreed on all points. I don't know the priority of this bug - users can just assign viewership to the Dataset in the interim, I don't think it will cause any issues for users to do so. If they're really in need, they can create a fresh Dataset with the required permissions for a single SR.

AndyRae commented 2 months ago

Just to note this is fixed in a temporary way in #813 / #817

As in - users can now have access to a specific Scan Report, even if they do not have access to the dataset. This is only in the Next app.

However, the same user will not be able access the Scan Report details (edit) page, regardless of there role on the Scan Report, even if they have been made an author. As I mentioned above - they don't have sufficient permission for this form to function properly, yet.

We will fix this in the future so they can - as we want to serialise that data sooner also to enable linking to a dataset from a Scan Report. But it's fairly low priority right now.