ome / design

OME Design proposals
http://ome.github.io/design/
1 stars 15 forks source link

New Role #62

Open pwalczysko opened 7 years ago

pwalczysko commented 7 years ago

see https://trello.com/c/t0nT7KYa/133-new-role#comment-57f4f24eac9fdd7102327437. Accompanying tabular explanation Accompanying power point presentation

This is an attempt to design the New Role (LightAdmin, people like Importer, Analyst, User Organizer and similar) from the user perspective. The trello card above encompasses the technical back-end part of the problematics and defines some restrictions of and Admin in OMERO (these restrictions make him then a LightAdmin, without the right to do everything in OMERO, but leaving him some extra, server-wide rights).

The extra rights (properties) up till now (depending on the progress of @mtbc who is creating further such rights atm, noted as "envisaged" in the table below) can be defined as:

  1. Chgrp
  2. Chown
  3. ModifyUser
  4. ReadSession
  5. Sudo (needed for ImportAs)
  6. WriteFile (necessary for ImportAs, right to write files into ManagedRepo). Nevertheless, removal of this right does not completely protect deletion of original files. Also, workflows of ImporterAs not using Sudo tend to be clanky. Suggestion is to use this permission only hand-in-hand with WriteOwned.
  7. WriteOwned (right to create other people's database items, needed for organizing other peoples data)
  8. ModifyGroup (envisaged)
  9. Upload Official Script (envisaged)

Based on these rights I would suggest to construct following "New Roles":

  1. Analayst. Needs to access, analyse and store the results (including new images) in any group. He might need to store the results in such a manner that they belong to the owner of the original image. As the results might be images which need the import to OMERO, the Sudo feature is necessary here too. Optionally we can offer curtailing the right to delete other peoples database items like P/D and curtailing the right to delete other peoples original data for this role. Rights of Analyst: WriteFile (6.), WriteOwned (7.), Sudo (5.), Upload (and Run) Official Script (9.)

  2. Importer. Basically imports data of anyone to any group which he is not a member of. Needs WriteFile and Sudo because of the import, but also the WriteOwned in order to create P/Ds for the imported images. Optionally we can offer curtailing the right to delete other peoples database items like P/D and curtailing the right to delete other peoples original data for this role. Chown and Chgrp can come handy for this person in case he/she has no right to delete already erroneously imported data. Chown and Chgrp in right can be optional for this type of person (user can create an Importer role both With and Without the right to Chown and Chgrp) Rights of Importer: WriteFile (6.), WriteOwned (7.), Sudo (5.), Chown (2.), Chgrp (1.)

  3. User organizer: Can create and modify users (names, loginnames, email addresses etc.) and also add/remove users to from groups. Can also create new groups. Optionally, this role can acquire the power to Chown and Chgrp, if the users-to-be-shuffled have data-to-be-moved or data-to-be-chowned Rights of User organizer: ModifyUser (3.), ModifyGroup (8.), Chown (2.), Chgrp (1.)

  4. Group organizer: Can create and modify groups (putting existing users into existing and new groups). This is a "User organizer" role above, just without the right to create and modify the users themselves. Optionally, this role can acquire the power to Chown and Chgrp, if the users-to-be-shuffled have data-to-be-moved or data-to-be-chowned Rights of Group organizer: ModifyGroup (8.), Chown (2.), Chgrp (1.)

  5. Any other combination of above and more: The 9. categories/rights/permissions in the first list in this table can be reorganized into any other fitting New Role, such as "Data organizer", "Data and user organizer" etc. as needed by the particular institutions.

@mtbc @joshmoore @jburel @gusferguson @kennethgillen

pwalczysko commented 7 years ago

cc @will-moore @dominikl -sorry, forgot to cc you in - please read above

pwalczysko commented 7 years ago
            config.add(new NamedValue(permission, "true"));
            config.add(new NamedValue(AdminPrivilegeChgrp.value, "false"));
            config.add(new NamedValue(AdminPrivilegeChown.value, "false"));
            config.add(new NamedValue(AdminPrivilegeModifyUser.value, "false"));
            config.add(new NamedValue(AdminPrivilegeReadSession.value, "false"));
            config.add(new NamedValue(AdminPrivilegeWriteFile.value, "false"));
            config.add(new NamedValue(AdminPrivilegeWriteOwned.value, "false"));

I have done this above and then the only restriction which is true is Sudo, which is sufficient for ImportAs, but not for any deletions whether files or omero objects, no creation of user, no chgrp of other peoples data and no chown permitted as experimentally tried using insight and CLI. The creation of new containers at import stage for the user to import for is not a problem. The creation of new containers on behalf of another user after import is not possible in Insight in any case. On the CLI you would This is more like it, I think I am starting to understand...

Interestingly, this almost "powerless" LightAdmin can still annotate other peoples images with tags and FileAttachments being in Read-only group. This is actually quite nice from user perspective and understandable as a "remnant" of the full sysadmins rights. Strange is the behaviour of Copy Cut Paste Link in the tree on other peoples data, it allows Cut and Paste but interprets it as Copy and Paste. Again, in Insigth it would probably mean though that the link will belong to the LightAdmin, so not ideal if you want to do a post-hoc cleaning. Editing of P/D names not allowed in this config.

pwalczysko commented 7 years ago

Discussing with @dominikl , the way forward for ImporterAs in Importer/Insight would be to offer (in much later stages) a similar workflow as on the CLI: first sudo, then do everything as the other user (with warning at the top of the central pane "logged in as, sudoed as" or somesuch. Also the group context dropdown would have to be adjusted. The sudoer should not see the groups he is a member of, but rather the groups which the user which he sudoed as is a member of. This would allow the ImporterAs to:

  1. import data As another user
  2. clean and rectify mistakes post import using insight
  3. we would not have to worry about giving the ImporterAs the rights WriteOwned and WriteFile, all could be done by sudo only
mtbc commented 7 years ago

If it would help with retiring Insight but allowing post-import cleanup then perhaps OMERO.web could offer a sudo mode.

pwalczysko commented 7 years ago

Empirically found out that the LightAdmin with only Sudo permission (=prospective ImporterAs) can upload a script which can be executed by everybody using Insight. This is probably not exactly what we want, but possibly not a total priority to sort out either (just security check, not a workflow blocker).

pwalczysko commented 7 years ago

@mtbc : So no, even with permissions set as below, an attempt by light admin to chgrp on behalf of the user fails with java.lang.AssertionError: Found ERR when pass==true: ::omero::cmd::Chgrp2 (not-in-group) params={stacktrace=java.lang.IllegalArgumentException: not a member of the chgrp destination group screen shot 2016-12-01 at 17 33 31

So chgrp to a group the owner of the data is not a member of is principally forbidden (needless to say, graphical clients will not allow you anything like this).

mtbc commented 7 years ago

Can a regular full-admin user (other than root) do the Chgrp? If so, there may be a bug there. (My testing has focused more on making sure things can't be done rather than that they can be.) I can take a look.

mtbc commented 7 years ago

I am trying to understand why my testChgrpPrivilegeViaRequest is passing when the light admin is not a member of the destination group.

mtbc commented 7 years ago

Ah, it turns out that "on behalf of the user" is about sudo, sorry I didn't initially realize the context: yes, @pwalczysko, I agree this makes sense: after sudo'ing to a normal user one loses one's special admin powers to push data into arbitrary groups.

pwalczysko commented 7 years ago

Note that during the work on ImporterAs workflow, an additional (sub)-role is emerging, which would be probably called Data organizer. This should encompass the ability to chgrp, chown, create a container on behalf of somebody (P/D...) and link the chgrp-ed/chown-ed images to this container. Note that this role is partially mapped in client code in webclient (the Move workflow of sysadmins), see https://github.com/openmicroscopy/openmicroscopy/blob/develop/components/tools/OmeroWeb/omeroweb/webclient/views.py#L4106 and https://github.com/openmicroscopy/openmicroscopy/blob/develop/components/tools/OmeroPy/src/omero/gateway/__init__.py#L4016 where the link is being owned by the owner of the objects (not by the (light ) admin creating the link, which is the right way to do it), thanks @will-moore.

The Data organizer workflows (see above) will be important for ImporterAs who mistargeted the import (both user- as well as group-wise) and wants to rectify this without deleting and re-importing.

Interestingly, the Chown permission is completely useless without accompanying WriteOwned and WriteFile permissions, which means that Data organizer would be able to delete anybody's data with impunity.

Unfortunately, Data organizer would be completely helpless in Insight, atm there are no tools available for him there, unless he is also the member of all the groups in question. Even so, no logic about how to create a container for somebody else is present in Insight.

pwalczysko commented 7 years ago

See https://github.com/pwalczysko/openmicroscopy/commits/new-role - the importerAs tests should be done. I can try tomorrow to go manually through this workflow using Importer/Insight or Importer/Web. Importer definitely manages pure sudo-equipped light admin just fine.

pwalczysko commented 7 years ago

Note: Webclient throws a server error when Light Admin equipped only with Sudo (which he does not use at that moment) and Chgrp permissions tries to Move an image of normal user to another group and in the process tries to create a container for this user. Light Admin is a member of none of the two groups. This is probably expected, but something to keep in mind.

SecurityViolation at /webclient/chgrp/
exception ::omero::SecurityViolation
{
    serverStackTrace = ome.conditions.SecurityViolation: You are not authorized to create ome.model.containers.Project:Id_210
    at ome.security.basic.OmeroInterceptor.newTransientDetails(OmeroInterceptor.java:715)
    at ome.security.basic.OmeroInterceptor.onSave(OmeroInterceptor.java:167)
    at org.hibernate.event.def.AbstractSaveEventListener.substituteValuesIfNecessary(AbstractSaveEventListener.java:414)
    at org.hibernate.event.def.AbstractSaveEventListener.performSaveOrReplicate(AbstractSaveEventListener.java:293)
    at org.hibernate.event.def.AbstractSaveEventListener.performSave(AbstractSaveEventListener.java:204)
    at org.hibernate.event.def.AbstractSaveEventListener.saveWithGeneratedId(AbstractSaveEventListener.java:144)
...

The error is fully recoverable, just cancel the error window and continue, no harm done.

Edit: Simple move of the same kind as above (no creation of project, moving dataset and containing image) goes just fine in webclient.

Edit: If the light admin is given additionally a WriteOwned privilege, the creation of Datasets and Projects on the Move stage in Webclient goes smoothly as well as the move itself. it makes a good sense to couple the Chgrp privilege with the WriteOwned for the reason of creating of new containers.

Edit: On the CLI, the workflow above is not possible due to a workflow bug already discussed with Colin. The CLI insists the admin (any admin, light or root will fare the same) to be a member of the target group for a successful chgrp operation.

[pwalczysko@ls31619 ~/Work/openmicroscopy/dist]$ bin/omero chgrp 3469 Image:611
Using session 0c3fc1ff-3a8a-4b31-8979-73ab115edd29 (importeras@localhost:4064). Idle timeout: 10 min. Current group: system
Current user is not member of group: 3469

Edit2: For the workflow bug with not being a member of target group created https://trello.com/c/WOR1HYsB/736-cli-chgrp-for-admin-not-fully-working

Most nasty bug on these workflows is still the impossiblity to target the ImportAs into a non-default group. Bug present on eel latest too.

[pwalczysko@ls31619 ~/Work/openmicroscopy/dist]$ bin/omero --sudo importeras -u user1 import -g 3471  ~/Desktop/CMPO2.png 
Previously logged in to localhost:4064 as user1
Server: [localhost:4064]
Skipped session f898da93-4c5a-4a96-a64f-64c495a2676e due to property conflicts: omero.group: None!=3471
Password for importeras:
InternalException: Failed to connect: exception ::Glacier2::CannotCreateSessionException
{
    reason = No info in database for importeras
}
[pwalczysko@ls31619 ~/Work/openmicroscopy/dist]$ 
mtbc commented 7 years ago

Yeah, probably will need WriteOwned to create a container.

pwalczysko commented 7 years ago

Regarding Insight and ImporterAs workflows using Sudo:

pwalczysko commented 7 years ago

Tomorrow I will test chown for light admin on the CLI.

jburel commented 7 years ago

@pwalczysko: Point 1: this is the same of admin.

pwalczysko commented 7 years ago

@jburel : Ta, edited the text.

pwalczysko commented 7 years ago

On my branch https://github.com/pwalczysko/openmicroscopy/commits/new-role we have now:

All tests behave as expected. Chown needs WriteFile and WriteOwned, Chgrp does not need anything additonally. Also captured in the tests. With all permutations this gives around 100 new tests.

Next steps:

pwalczysko commented 7 years ago

We cannot automatically assume that Chown needs all three permissions (Chown, WriteFile and WriteOwned). Obviously, for light Admin equipped with Chown only it is possible to chown a dataset, image and link as shown in my last test, probably because he is an owner of these data before the chown. See https://github.com/pwalczysko/openmicroscopy/commit/477f9290f281002af4011c6624b36aae24aa9e70#diff-d91232ef1e245a87e311c4a79a2a5f77R682

pwalczysko commented 7 years ago

Apparently, deleting an image (fully, with the original file too it seems) is allowed when WriteOwned is true but WriteFile is false. Note https://github.com/pwalczysko/openmicroscopy/commit/737c1872f7caf9a3387fa91bbe3899418a296d58#diff-d91232ef1e245a87e311c4a79a2a5f77R389 - this line shoud be failing if WriteFile is of importance for image deletion. But the opposite is true, and also the assert below https://github.com/pwalczysko/openmicroscopy/commit/737c1872f7caf9a3387fa91bbe3899418a296d58#diff-d91232ef1e245a87e311c4a79a2a5f77R431 showed clearly that the image and original file were indeed deleted. Consulted this with @joshmoore briefly and he was as suprised as me. Will continue discussing in the new year.

pwalczysko commented 7 years ago

As the Delete behaviour is now tested, see https://github.com/openmicroscopy/design/issues/62#issuecomment-267686884, I will continue with:

mtbc commented 7 years ago

https://github.com/openmicroscopy/design/issues/62#issuecomment-267686884 is a good find that surprised me too. From blitz-delete-rules.xml it looks as though if an OriginalFile instance is orphaned by model graph deletions then it is deleted regardless of permissions on it. Why this is so, I don't recall: it looks intentional and I don't see likely ways for it to cause harm but we can certainly discuss it. I'm hoping it's okay as-is as adjusting bits of graph policy like this tends to cause unwanted side effects and, barring some good luck, the 5.3.0 schedule affords little slack.

mtbc commented 7 years ago

The above may become more of an issue if in privileges "bundles" we plan to give users DeleteOwned without DeleteFile.

pwalczysko commented 7 years ago

The above may become more of an issue if in privileges "bundles" we plan to give users DeleteOwned without DeleteFile.

I was toying with that idea, but clearly this is not going to work because of the abovementioned behaviour. Thus, we will offer the LightAdmin(Importer, Data Organizer, Analyst...) which either can or cannot Create & Delete everything on behalf of the other user, no granulatiry offered in the sense of what can be deleted. Later, when @mtbc will create the DeleteFile and DeleteOwned privileges and this will splity the Create from Delete part in the previous sentence, offering someone who can Create (and Link) but not Delete anything.

pwalczysko commented 7 years ago

@mtbc : Spent quite a bit of time trashing this https://github.com/pwalczysko/openmicroscopy/commit/dd839554630c4f2d8ed1f8bee1226776da9a252f#diff-d91232ef1e245a87e311c4a79a2a5f77R872 out. Is this a fair conclusion ? I.e. if the Project has a link to a dataset and this in its turn to an image, a chown of the project is expected to transfer the whole bunch to the target user, right ? But this did not work in this case. I have the only explanation that the Project (as well as the Dat and Image) were already owned by the target user in the moment of the transfer and thus some shortcut implemented in Chown2 logic did not bother to check the whole tree under the Project ?

pwalczysko commented 7 years ago

Bug:

So I think that there is a clear (blocking) bug here specific for light Admins (as opposed to full Admins). Interestingly, the bug does not appear in case light Admin owns the data which he is chowning. But this mitigation is not rendering the bug harmless, as in most cases of Data Organizer, the data will not be owned by Data Organizer.

pwalczysko commented 7 years ago

Note that no amount of additional permissions will render the light Admin to be able to chown the whole hierarchy (https://github.com/openmicroscopy/design/issues/62#issuecomment-270924297). Even light Admin who has screen shot 2017-01-06 at 15 37 47 will still fail as described in the bug report

pwalczysko commented 7 years ago

Interestingly (but expectedly I suppose, @mtbc ?) we cannot link (images to datasets) in private group without sudoing. This adds another drawback to non-sudo ImporterAs workflows. Will push the corresponding test after we sort out the bug. This has some implications on "Data organizer" workflows too. On the flip side, all the complex tests are passing for Importer As in Sudo situation in all group types.

jburel commented 7 years ago

The fact that you cannot link in a private group is a known issue that was raised a while back when we first started to work on the table explaining the permissions system. You can delete but you cannot link!

mtbc commented 7 years ago

Yeah, need to chown before linking image to dataset in private group. I'm not sure how safe it is to assume being able to do it outside read-write to be honest: the server currently allows it but I don't know if we promise that anywhere.

mtbc commented 7 years ago

Bug of https://github.com/openmicroscopy/design/issues/62#issuecomment-270930221 confirmed: unrestricted light admin is behaving worse than full admin.

mtbc commented 7 years ago

@pwalczysko: I pushed to my roles branch a couple of commits that should fix the chown hierarchy issue for you; I am not yet sure they're good so I'll run some more tests tomorrow. There is a broader issue noted at https://trello.com/c/4KesMDI6/737-light-admins-and-o-on-objects that I think can wait for later.

mtbc commented 7 years ago

I can't presently do the testing I'd planned but, looking again at my commits after a night's sleep, I'm not yet happy with them so I've removed them from the branch. I'll probably figure it out quite soon; thanks for reporting the bug so clearly!

mtbc commented 7 years ago

It's a bit of a hack but https://github.com/mtbc/openmicroscopy/commit/2c32865e on my branch should fix the chown hierarchy issue for light admins.

pwalczysko commented 7 years ago

@mtbc : Further to our; yesterday's discussion with @jburel . Did you say that a light admin can chown whilst being sudoed as normalUser as long as he has Chown, WriteOwned and WriteFile permissions ? I thought I did not test such eventuality, but I did, see https://github.com/pwalczysko/openmicroscopy/commit/c5326b6a3f009884d046a6f43d294febe5cea8a6#diff-d91232ef1e245a87e311c4a79a2a5f77R491. This test would indicate that your assumption does not hold ?

mtbc commented 7 years ago

Yes, you are correct: the user they sudo to would also need to be an administrator. But, per our conversation, I'll take a look at relaxing that requirement.

pwalczysko commented 7 years ago

Can confirm that the bug about the chowning of hierarchy as light admin iis now fixed, see https://github.com/openmicroscopy/openmicroscopy/commit/fa9e2da53aa8aba55d16d652d12549a0a360a3f8#diff-d91232ef1e245a87e311c4a79a2a5f77R880 test (chowning the whole Project chowns also contained Dataset, Image and the two links).

pwalczysko commented 7 years ago

PR opened https://github.com/openmicroscopy/openmicroscopy/pull/5048 mainly to make sure the tests and features will pass on the CI system.

pwalczysko commented 7 years ago

Chgrp does not seem to need WriteOwned or WriteFile in any situation, see https://github.com/openmicroscopy/openmicroscopy/pull/5048/files#diff-d91232ef1e245a87e311c4a79a2a5f77R596 and https://github.com/openmicroscopy/openmicroscopy/pull/5048/files#diff-d91232ef1e245a87e311c4a79a2a5f77R615

pwalczysko commented 7 years ago

ImporterAs (Sudo privilege ONLY) works with CLI client in the command used by @bramalingam in his video, bin/omero --sudo lightadmin -u user1 import -T Dataset:name:importForHim ~/path/to/image -> this was tested for newly created dataset as well as pre-existing dataset

pwalczysko commented 7 years ago

https://github.com/openmicroscopy/openmicroscopy/pull/5048/commits/8a904e9d25df13cb765dbe7e5c340351b9c1c947 shows that Create (of Project and Dataset) needs either Sudo or WriteOwned (when not sudoing). Did not try to create OriginalFile or somesuch (like file attachment for another user) as these are non-mainstream workflows (actually cannot imagine a workflow like this atm, maybe original file attached by an analyst and then ... but this is a chown workflow for me, so I think Create is covered.)

mtbc commented 7 years ago

@pwalczysko: Could you give me a minimal test that shows this interesting issue you saw where the light admin does not need DeleteFile or DeleteManagedRepo to delete an image's files?

pwalczysko commented 7 years ago

@mtbc : the test is the delete test. I hope it is not too big, you can see that other than DeleteOwned permissions are not given at all in any combination of data provider. this line https://github.com/openmicroscopy/openmicroscopy/pull/5048/files#diff-d91232ef1e245a87e311c4a79a2a5f77R434 should show that the Original File has been deleted as well. I do not know how to test automatically the true original file absence in the Managed Repo - but I checked this manually, the file is not there.

will-moore commented 7 years ago

Just chatted with @jburel about UI for roles. I think that this might get confusing for users if we present checkboxes with the 4 roles:

This provides Admins with more flexibility than the 4 Roles above, (E.g. Import As without Chown, as suggested by Petr and doesn't have the UI nightmare of trying to enable "Importer" role when someone selects "Analyst" and "Group Organiser" etc. Question: How does Insight / CLI decide if someone should be able to Import As?

mtbc commented 7 years ago

@pwalczysko: Aha, thank you. From https://github.com/openmicroscopy/openmicroscopy/blob/0ebf62ad/components/blitz/resources/ome/services/graph-rules/blitz-delete-rules.xml#L335 it looks like there is a permissions override (the /n) on orphaned original files that comes into effect. (I don't now recall why.) So at least there isn't a deeper bug here.

pwalczysko commented 7 years ago

Discussed with @will-moore Re

How does Insight / CLI decide if someone should be able to Import As?

In general, Insight and CLI are adjusted in such a way that you Sudo immediately and then ImportAs. In other words, the ImporterAs needs in fact only Sudo permissions to do 90% of the work (or say 100% if he does not want/need to correct mistakes afterwards). Insight(Importer) sudoes you in in the moment when you select another user to import for from the dropdown. On the CLI you have to sudo (=login) yourself using --sudo flag, then import as the user you are sudoed in. Necessary permissions: isAdmin, Sudo.

Note that the permissions are now wider than listed in the header of this issue, mainly the new Delete... permissions were added as well as some group/user concerning permissions. The full list is at https://trello.com/c/t0nT7KYa/133-new-role#comment-588b5c2eed8b537bfd9af0a9. The most authoritative and up-to-date for me is my .ppt team/pwalczysko/new-role-test-explanation/Explanation-of-test-new-role.ppt

As discussed with @will-moore , fully agree with trying to keep the choices down to the list he suggests at https://github.com/openmicroscopy/design/issues/62#issuecomment-277666749. The interesting more involved questions:

Will try to create a table from my .ppt to get better overview.

pwalczysko commented 7 years ago

@mtbc @jburel @will-moore @joshmoore : So the promised table is at google.docs.

Edit: also the Power Point presentation is now as a g.doc better accessible.

will-moore commented 7 years ago

Discussed the gdoc spreadsheet matrix with @pwalczysko and suggested list of Roles would be (with summary of use-cases as I understand it)

Also discussed the creation of Light-Admins with NONE of the above allowed: In this case the user can browse all data (in webclient). Anything else they can do (E.g. annotate in private/read-only groups etc)?

NB: "workarounds" exist. E.g. If a user only has Chown, they could change someone else's data to be their own, then edit the data and chown back to someone else (without having Write Owned rights). Presumably this would also remove annotations etc?

jburel commented 7 years ago

Anything else they can do (E.g. annotate in private/read-only groups etc)? They won't be able to annotate in private since an admin cannot do it They can only view (not delete like admin can),

mtbc commented 7 years ago

The roles are implemented very much as restrictions on full admins rather than extra powers on normal users: there will be some extra things that light admins can do even with none of the above allowed. Reading (nearly) all data is indeed one. There will be others: e.g., maybe some of the LDAP service stuff?