lukefitzwolfgang / icatproject

Automatically exported from code.google.com/p/icatproject
0 stars 0 forks source link

Delete rules should also consider cascaded related objects #150

Open GoogleCodeExporter opened 9 years ago

GoogleCodeExporter commented 9 years ago
If a user tries to delete an object, ICAT checks whether he got delete 
permission on that object.  But by cascading, the delete call might in fact 
also delete other related objects.  ICAT does not check whether the user also 
has delete permission on these related objects.

What steps will reproduce the problem?
1. Consider a user that does not have delete permission on a dataset, but does 
have delete permission on the sample related to this dataset.
2. Verify that the user does not have delete perms on the dataset:
>>> client.delete(dataset)
ERROR:suds.client:<suds.sax.document.Document instance at 0x7fa7d2a87c68>
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/lib/python2.7/site-packages/icat/client.py", line 367, in delete
    raise translateError(e)
icat.exception.ICATPrivilegesError: DELETE access to this Dataset is not 
allowed.
3. Delete the related sample:
>>> client.delete(dataset.sample)
4. Confirm that the dataset has also been deleted:
>>> client.search("Dataset [id=%d]" % dataset.id)
[]

What is the expected output? What do you see instead?
The actual result was that the user could delete the dataset along with the 
sample although he did not have delete permission on the dataset.  Expected 
that deleting the sample should fail with a ICATPrivilegesError.

Please use labels and text to provide additional information.
I acknowledge that the documentation does not state anything on whether 
permissions are also checked for cascaded objects.  So one may argue that this 
is not a bug.  But there is a use case for this setup of permissions, it would 
be desirable that they work as expected:
I manage access rules for investigations as a whole.  E.g. people with write 
access to an investigation get CRUD permission on all datafiles, datasets, and 
samples related to this investigation.  But I also want to be able to set 
individual datasets as read-only so that they cannot be modified anymore once 
they are complete.  So I add a condition "dataset.complete = False" to the CRUD 
rule for Dataset and Datafile.  This works as expected - aside from the fact 
that this can be circumvented by deleting the related Sample.
Of course I could remove the "D" in the "CRUD" rule for Sample.  But then the 
user cannot do housekeeping in their investigation, for instance deleting 
excess samples that might have been created accidently.  As far as I know there 
is currently no way to spell a rule giving "delete permission to Sample, unless 
there exists a related dataset having complete = True".

Original issue reported on code.google.com by rolf.kr...@helmholtz-berlin.de on 9 Jan 2015 at 1:06

GoogleCodeExporter commented 9 years ago
This is "by design", however I appreciate that it is not really the expected 
behaviour so I will fix it. I don't think it requires many lines of code to 
give the desired behaviour - and to produce a clear error message when a 
cascaded object prevents deletion.

Original comment by dr.s.m.f...@gmail.com on 9 Jan 2015 at 1:28

GoogleCodeExporter commented 9 years ago
Thinking more about this I now feel that the fact that all one to many 
relationships are cascaded is wrong. In the case of the dataset - sample 
relationship it makes no sense to delete a dataset when the sample is deleted. 
The cascade should not be present when the item on the many side does does not 
have a constraint which includes the item on the one side.As far as I can see 
this only affects this one relationship. With this  change I don't think there 
is any need for checking to many relationships for the right to delete. 
Removing the cascade will of course also affect creation.

Original comment by dr.s.m.f...@gmail.com on 22 Jan 2015 at 4:33