ome / omero-scripts

Core OMERO Scripts
https://pypi.org/project/omero-scripts/
12 stars 32 forks source link

Adding a script that saves data owner as key-value pair #206

Open Rdornier opened 1 year ago

Rdornier commented 1 year ago

Hello,

As suggested, here is a python script implementing ownership tracability as a key-value pair.

The script accepts as input arguments either image ids, container ids (dataset, project, well, plate, screen) or usernames. For images and containers, their ID is required and the script adds the owner-keyVal to the specified images/containers and all their children. For users, their omero-username is required and the script adds the owner-keyVal to all data owned by the users, in all groups they are member of.

If the logged-in user is admin, a sudo connection is established.

I created an excel sheet where I consigned the results of different tests I have done but, as you'll see, there is a scenario I do not really understand. I thought it was possible to add the key-value on data owned by someone in a private group with a sudo connection but, apparently, it is not the case.

Happy to answer any questions and to improve the script. Thanks and all the best,

Rémy.

Rdornier commented 1 year ago

In the end, it could be nice to integrate this script directly during the ownership changing process (i.e. when clicking on "change ownership") rather than running the script first to add the owner as KVP and then changing the ownership. But this is the next step.

joshmoore commented 1 year ago

Hey @Rdornier! Thanks for this, and very sorry for having missed it originally. We're now in a bit of a summer gap, but we'll get this in front of some folks for a review ASAP.

will-moore commented 1 year ago

Hi @Rdornier - Apologies for not looking at this sooner...

The script works well, ran as a regular user...

Screenshot 2023-07-11 at 08 48 43

Then I logged-in as an Admin, and changed the owner of an Image to another member of the group (Admin wasn't a member of this group) and I tried running the script again as an Admin.

This attempted to remove the previous annotation, which failed (see error below), so the annotations didn't get updated at-all. I'm not sure why the Admin didn't have permission to remove the annotation, but probably the new owner also wouldn't have permission to remove the old annotation (unless in a read-write group).

Stack trace ``` Traceback (most recent call last): File "/opt/omero/server/omero/tmp/omero_omero-server/814/processsn_azv25.dir/./script", line 58, in remove_map_annotations conn.c.waitOnCmd(handle, loops=10, ms=500, failonerror=True, File "/opt/omero/server/venv3/lib/python3.10/site-packages/omero/clients.py", line 1020, in waitOnCmd raise omero.CmdError(rsp) omero.CmdError: object #0 (::omero::cmd::GraphException) { category = ::omero::cmd::Delete2 name = graph-fail parameters = { key = stacktrace value = ome.services.graphs.GraphException(message=not permitted to delete ome.model.annotations.MapAnnotation[2516]) at ome.services.graphs.GraphTraversal.assertPermissions(GraphTraversal.java:1434) at ome.services.graphs.GraphTraversal.assertMayBeDeleted(GraphTraversal.java:1407) at ome.services.graphs.GraphTraversal.processTargets(GraphTraversal.java:1661) at omero.cmd.graphs.Delete2I.step(Delete2I.java:164) at omero.cmd.HandleI.steps(HandleI.java:448) at omero.cmd.HandleI$RunSteps.innerWork(HandleI.java:509) at omero.cmd.HandleI$2.doWork(HandleI.java:383) at omero.cmd.HandleI$2.doWork(HandleI.java:380) at jdk.internal.reflect.GeneratedMethodAccessor292.invoke(Unknown Source) at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) at java.base/java.lang.reflect.Method.invoke(Method.java:566) at org.springframework.aop.support.AopUtils.invokeJoinpointUsingReflection(AopUtils.java:333) at org.springframework.aop.framework.ReflectiveMethodInvocation.invokeJoinpoint(ReflectiveMethodInvocation.java:190) at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:157) at ome.services.util.Executor$Impl$Interceptor.invoke(Executor.java:568) at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:179) at ome.security.basic.EventHandler.invoke(EventHandler.java:154) at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:179) at org.springframework.orm.hibernate3.HibernateInterceptor.invoke(HibernateInterceptor.java:119) at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:179) at org.springframework.transaction.interceptor.TransactionInterceptor$1.proceedWithInvocation(TransactionInterceptor.java:99) at org.springframework.transaction.interceptor.TransactionAspectSupport.invokeWithinTransaction(TransactionAspectSupport.java:282) at org.springframework.transaction.interceptor.TransactionInterceptor.invoke(TransactionInterceptor.java:96) at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:179) at ome.tools.hibernate.ProxyCleanupFilter$Interceptor.invoke(ProxyCleanupFilter.java:249) at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:179) at ome.services.util.ServiceHandler.invoke(ServiceHandler.java:121) at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:179) at org.springframework.aop.framework.JdkDynamicAopProxy.invoke(JdkDynamicAopProxy.java:213) at com.sun.proxy.$Proxy81.doWork(Unknown Source) at ome.services.util.Executor$Impl.execute(Executor.java:447) at omero.cmd.HandleI.run(HandleI.java:379) at java.base/java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:515) at ome.services.util.Executor$Impl$1.call(Executor.java:488) at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264) at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128) at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628) at java.base/java.lang.Thread.run(Thread.java:829) key = message value = } message = not permitted to delete ome.model.annotations.MapAnnotation[2516] } During handling of the above exception, another exception occurred: Traceback (most recent call last): File "/opt/omero/server/omero/tmp/omero_omero-server/814/processsn_azv25.dir/./script", line 406, in run_script() File "/opt/omero/server/omero/tmp/omero_omero-server/814/processsn_azv25.dir/./script", line 398, in run_script message = add_owner_as_keyval(conn, script_params) File "/opt/omero/server/omero/tmp/omero_omero-server/814/processsn_azv25.dir/./script", line 321, in add_owner_as_keyval nImage += process_image(user_conn, omero_object, key, owner) File "/opt/omero/server/omero/tmp/omero_omero-server/814/processsn_azv25.dir/./script", line 115, in process_image return (1 if annotate_object(conn, image, key, owner)== True else 0) File "/opt/omero/server/omero/tmp/omero_omero-server/814/processsn_azv25.dir/./script", line 89, in annotate_object remove_map_annotations(conn, obj, namespace) File "/opt/omero/server/omero/tmp/omero_omero-server/814/processsn_azv25.dir/./script", line 62, in remove_map_annotations print("Failed to delete links: {}".format(ex.message)) AttributeError: 'CmdError' object has no attribute 'message' !! 07/11/23 07:55:22.847 error: communicator not destroyed during global destruction. ```

If I commented-out the # remove_map_annotations(conn, obj, namespace) line and ran the script again (as Admin) it combines the ownership history nicely, but we are still left with the previous annotation:

Screenshot 2023-07-11 at 09 07 34

I don't know how the permissions will work in your typical use case, but at the very least you should try/catch the removal of annotation (or test if you can delete) so that the script doesn't fail in this case. Maybe safer to assume that you can never remove previous annotations and always just add a single annotation.

Rdornier commented 1 year ago

Hello @will-moore

Thanks for having a look to my code and to figure this issue. I actually struggled a bit with this deletion issue and I thought it was fixed.

Anyway, I pushed some modifications and key-values are not deleted anymore. I only add them in the namespace. So this issue should be corrected now.

I don't know how the permissions will work in your typical use case, but at the very least you should try/catch the removal of annotation (or test if you can delete) so that the script doesn't fail in this case.

Yes, I did it only on SecurityViolation, not on CmdError. I should had done it on Exception

Maybe safer to assume that you can never remove previous annotations and always just add a single annotation.

Done

but we are still left with the previous annotation:

This is corrected now. I only link the MapAnnotationWrapper if it does not exist yet (i.e. link it once to the object)