New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New Role #62

Open
pwalczysko opened this Issue Nov 23, 2016 · 62 comments

Comments

Projects
None yet
4 participants
@pwalczysko
Member

pwalczysko commented Nov 23, 2016

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

This comment has been minimized.

Show comment
Hide comment
@pwalczysko

pwalczysko Nov 23, 2016

Member

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

Member

pwalczysko commented Nov 23, 2016

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

@pwalczysko

This comment has been minimized.

Show comment
Hide comment
@pwalczysko

pwalczysko Nov 25, 2016

Member
            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.

Member

pwalczysko commented Nov 25, 2016

            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

This comment has been minimized.

Show comment
Hide comment
@pwalczysko

pwalczysko Nov 30, 2016

Member

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
Member

pwalczysko commented Nov 30, 2016

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

This comment has been minimized.

Show comment
Hide comment
@mtbc

mtbc Nov 30, 2016

Member

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

Member

mtbc commented Nov 30, 2016

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

@pwalczysko

This comment has been minimized.

Show comment
Hide comment
@pwalczysko

pwalczysko Nov 30, 2016

Member

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).

Member

pwalczysko commented Nov 30, 2016

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

This comment has been minimized.

Show comment
Hide comment
@pwalczysko

pwalczysko Dec 1, 2016

Member

@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).

Member

pwalczysko commented Dec 1, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@mtbc

mtbc Dec 1, 2016

Member

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.

Member

mtbc commented Dec 1, 2016

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

This comment has been minimized.

Show comment
Hide comment
@mtbc

mtbc Dec 2, 2016

Member

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

Member

mtbc commented Dec 2, 2016

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

@mtbc

This comment has been minimized.

Show comment
Hide comment
@mtbc

mtbc Dec 5, 2016

Member

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.

Member

mtbc commented Dec 5, 2016

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

This comment has been minimized.

Show comment
Hide comment
@pwalczysko

pwalczysko Dec 6, 2016

Member

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.

Member

pwalczysko commented Dec 6, 2016

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

This comment has been minimized.

Show comment
Hide comment
@pwalczysko

pwalczysko Dec 7, 2016

Member

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.

Member

pwalczysko commented Dec 7, 2016

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

This comment has been minimized.

Show comment
Hide comment
@pwalczysko

pwalczysko Dec 8, 2016

Member

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]$ 
Member

pwalczysko commented Dec 8, 2016

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

This comment has been minimized.

Show comment
Hide comment
@mtbc

mtbc Dec 8, 2016

Member

Yeah, probably will need WriteOwned to create a container.

Member

mtbc commented Dec 8, 2016

Yeah, probably will need WriteOwned to create a container.

@pwalczysko

This comment has been minimized.

Show comment
Hide comment
@pwalczysko

pwalczysko Dec 8, 2016

Member

Regarding Insight and ImporterAs workflows using Sudo:

  • biggest drawback is insistence of Insight and graphical Importer that the Light Admin must be a member of the group he is importing into (as @jburel remarks, no difference there from the behaviour for full admins)
  • but ImportAs work from there just fine, as well as a subsequent Move of the data into another group (even though the light admin is not a member of the target group of the move)
  • for the 2 above points it is necessary only to equip the light admin with Sudo, Chgrp and WriteOwned powers note that in Insight you cannot target anything anyway (creating new P/Ds at Move stage if you are an admin moving for somebody else
    Edit: From the last sentence comes the conclusion that for Importer/Insight workflows, Sudo and Chgrp permissions only will give the light admin everything they could ever find useful in those two graphical clients.
Member

pwalczysko commented Dec 8, 2016

Regarding Insight and ImporterAs workflows using Sudo:

  • biggest drawback is insistence of Insight and graphical Importer that the Light Admin must be a member of the group he is importing into (as @jburel remarks, no difference there from the behaviour for full admins)
  • but ImportAs work from there just fine, as well as a subsequent Move of the data into another group (even though the light admin is not a member of the target group of the move)
  • for the 2 above points it is necessary only to equip the light admin with Sudo, Chgrp and WriteOwned powers note that in Insight you cannot target anything anyway (creating new P/Ds at Move stage if you are an admin moving for somebody else
    Edit: From the last sentence comes the conclusion that for Importer/Insight workflows, Sudo and Chgrp permissions only will give the light admin everything they could ever find useful in those two graphical clients.
@pwalczysko

This comment has been minimized.

Show comment
Hide comment
@pwalczysko

pwalczysko Dec 8, 2016

Member

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

Member

pwalczysko commented Dec 8, 2016

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

@jburel

This comment has been minimized.

Show comment
Hide comment
@jburel

jburel Dec 8, 2016

Member

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

Member

jburel commented Dec 8, 2016

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

@pwalczysko

This comment has been minimized.

Show comment
Hide comment
@pwalczysko

pwalczysko Dec 9, 2016

Member

@jburel : Ta, edited the text.

Member

pwalczysko commented Dec 9, 2016

@jburel : Ta, edited the text.

@pwalczysko

This comment has been minimized.

Show comment
Hide comment
@pwalczysko

pwalczysko Dec 9, 2016

Member

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

    1. tests for ImporterAs with Sudo workflows with subsequent sudo-based cleanup (chgrp, new containers creation etc.)
    1. tests for ImporterAs with Sudo workflows with subsequent non-sudo cleanup
    1. tests for ImporterAs with No Sudo workflows (importing as light admin, then chown or chown and chgrp)

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:

  • widen the tests for non-sudo workflows for creation and subsequent Chgrp and Chown actions on P/D containers (in point 3. above)
  • explore better the Delete behaviour in case of sudo login (put it together with the non-sudo login cleanup delete tests under 1. above and put it as a separate test). This should capture the possible work of @mtbc on adding new permissions DeleteFile and DeleteOwned.
  • try Chown workflows on CLI
  • try the workflows in point 3. above on CLI (should be possible afaik)
Member

pwalczysko commented Dec 9, 2016

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

    1. tests for ImporterAs with Sudo workflows with subsequent sudo-based cleanup (chgrp, new containers creation etc.)
    1. tests for ImporterAs with Sudo workflows with subsequent non-sudo cleanup
    1. tests for ImporterAs with No Sudo workflows (importing as light admin, then chown or chown and chgrp)

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:

  • widen the tests for non-sudo workflows for creation and subsequent Chgrp and Chown actions on P/D containers (in point 3. above)
  • explore better the Delete behaviour in case of sudo login (put it together with the non-sudo login cleanup delete tests under 1. above and put it as a separate test). This should capture the possible work of @mtbc on adding new permissions DeleteFile and DeleteOwned.
  • try Chown workflows on CLI
  • try the workflows in point 3. above on CLI (should be possible afaik)
@pwalczysko

This comment has been minimized.

Show comment
Hide comment
@pwalczysko

pwalczysko Dec 13, 2016

Member

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 pwalczysko/openmicroscopy@477f929#diff-d91232ef1e245a87e311c4a79a2a5f77R682

Member

pwalczysko commented Dec 13, 2016

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 pwalczysko/openmicroscopy@477f929#diff-d91232ef1e245a87e311c4a79a2a5f77R682

@pwalczysko

This comment has been minimized.

Show comment
Hide comment
@pwalczysko

pwalczysko Dec 16, 2016

Member

Apparently, deleting an image (fully, with the original file too it seems) is allowed when WriteOwned is true but WriteFile is false.
Note pwalczysko/openmicroscopy@737c187#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 pwalczysko/openmicroscopy@737c187#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.

Member

pwalczysko commented Dec 16, 2016

Apparently, deleting an image (fully, with the original file too it seems) is allowed when WriteOwned is true but WriteFile is false.
Note pwalczysko/openmicroscopy@737c187#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 pwalczysko/openmicroscopy@737c187#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

This comment has been minimized.

Show comment
Hide comment
@pwalczysko

pwalczysko Dec 16, 2016

Member

As the Delete behaviour is now tested, see #62 (comment), I will continue with:

  • exploring the CLI for using of all the workflows in the tests
  • writing tests for Group Owner who became a light admin
  • witing tests for uploading of official script behaviour
Member

pwalczysko commented Dec 16, 2016

As the Delete behaviour is now tested, see #62 (comment), I will continue with:

  • exploring the CLI for using of all the workflows in the tests
  • writing tests for Group Owner who became a light admin
  • witing tests for uploading of official script behaviour
@mtbc

This comment has been minimized.

Show comment
Hide comment
@mtbc

mtbc Jan 4, 2017

Member

#62 (comment) 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.

Member

mtbc commented Jan 4, 2017

#62 (comment) 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

This comment has been minimized.

Show comment
Hide comment
@mtbc

mtbc Jan 4, 2017

Member

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

Member

mtbc commented Jan 4, 2017

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

@pwalczysko

This comment has been minimized.

Show comment
Hide comment
@pwalczysko

pwalczysko Jan 4, 2017

Member

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.

Member

pwalczysko commented Jan 4, 2017

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

This comment has been minimized.

Show comment
Hide comment
@pwalczysko

pwalczysko Jan 4, 2017

Member

@mtbc : Spent quite a bit of time trashing this pwalczysko/openmicroscopy@dd83955#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 ?

Member

pwalczysko commented Jan 4, 2017

@mtbc : Spent quite a bit of time trashing this pwalczysko/openmicroscopy@dd83955#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

This comment has been minimized.

Show comment
Hide comment
@pwalczysko

pwalczysko Jan 6, 2017

Member

Bug:

  • Lets have hierarchy (Dataset - Link - Image). The whole hierarchy is owned by one user.

  • Then chown (Dataset) as light Admin with permissions Chown, WriteOwned and WriteFile -> observe that only the Dataset was transferred to recipient, not Link and Image

  • This is contrary to expectations, as spelled out for example in this @mtbc 's test which is passing

  • The bug comes down purely to discrepancy between having full Admin as in Mark's test and having light Admin as in my test.

  • In my test, which is currently passing because the full Admin is doing the chown, it is only necessary to comment out

    • the line 1158 in order to let the light Admin do the chown -> the test will fail because the Image and Link won't be chowned
    • the line [1115 in order to let the light Admin own the whole hierarchy -> this will render the test passing again, even as light Admin is doing the chown

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.

Member

pwalczysko commented Jan 6, 2017

Bug:

  • Lets have hierarchy (Dataset - Link - Image). The whole hierarchy is owned by one user.

  • Then chown (Dataset) as light Admin with permissions Chown, WriteOwned and WriteFile -> observe that only the Dataset was transferred to recipient, not Link and Image

  • This is contrary to expectations, as spelled out for example in this @mtbc 's test which is passing

  • The bug comes down purely to discrepancy between having full Admin as in Mark's test and having light Admin as in my test.

  • In my test, which is currently passing because the full Admin is doing the chown, it is only necessary to comment out

    • the line 1158 in order to let the light Admin do the chown -> the test will fail because the Image and Link won't be chowned
    • the line [1115 in order to let the light Admin own the whole hierarchy -> this will render the test passing again, even as light Admin is doing the chown

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

This comment has been minimized.

Show comment
Hide comment
@pwalczysko

pwalczysko Jan 6, 2017

Member

Note that no amount of additional permissions will render the light Admin to be able to chown the whole hierarchy (#62 (comment)). Even light Admin who has
screen shot 2017-01-06 at 15 37 47
will still fail as described in the bug report

Member

pwalczysko commented Jan 6, 2017

Note that no amount of additional permissions will render the light Admin to be able to chown the whole hierarchy (#62 (comment)). Even light Admin who has
screen shot 2017-01-06 at 15 37 47
will still fail as described in the bug report

@pwalczysko

This comment has been minimized.

Show comment
Hide comment
@pwalczysko

pwalczysko Jan 6, 2017

Member

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.

Member

pwalczysko commented Jan 6, 2017

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

This comment has been minimized.

Show comment
Hide comment
@jburel

jburel Jan 6, 2017

Member

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!

Member

jburel commented Jan 6, 2017

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

This comment has been minimized.

Show comment
Hide comment
@mtbc

mtbc Jan 9, 2017

Member

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.

Member

mtbc commented Jan 9, 2017

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.

@pwalczysko pwalczysko referenced this issue Jan 18, 2017

Closed

New roles #5048

@pwalczysko

This comment has been minimized.

Show comment
Hide comment
@pwalczysko

pwalczysko Jan 18, 2017

Member

PR opened openmicroscopy/openmicroscopy#5048 mainly to make sure the tests and features will pass on the CI system.

Member

pwalczysko commented Jan 18, 2017

PR opened openmicroscopy/openmicroscopy#5048 mainly to make sure the tests and features will pass on the CI system.

@pwalczysko

This comment has been minimized.

Show comment
Hide comment
@pwalczysko

pwalczysko Jan 20, 2017

Member

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

Member

pwalczysko commented Jan 20, 2017

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

This comment has been minimized.

Show comment
Hide comment
@pwalczysko

pwalczysko Jan 20, 2017

Member

openmicroscopy/openmicroscopy@8a904e9 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.)

Member

pwalczysko commented Jan 20, 2017

openmicroscopy/openmicroscopy@8a904e9 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

This comment has been minimized.

Show comment
Hide comment
@mtbc

mtbc Feb 6, 2017

Member

@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?

Member

mtbc commented Feb 6, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@pwalczysko

pwalczysko Feb 6, 2017

Member

@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.

Member

pwalczysko commented Feb 6, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@will-moore

will-moore Feb 6, 2017

Member

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:

  • Analyst
  • Importer
  • User Organiser
  • Group Organiser
  • Other combinations
    Because the rights are shared between them. E.g. If I assign someone as Analyst AND User Organiser, they will also have all the rights of Importer and Group Organiser.
    I think we should maybe allow the user to pick the rights themselves, but not allow combinations that make no sense. E.g. Write Owned (always includes Write File).
    Then the list might look something like:
  • Create data As (Write File, Write Owned, Sudo)
  • Upload Official Script
  • Chgrp
  • Chown
  • Modify User
  • Modify Group

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?

Member

will-moore commented Feb 6, 2017

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:

  • Analyst
  • Importer
  • User Organiser
  • Group Organiser
  • Other combinations
    Because the rights are shared between them. E.g. If I assign someone as Analyst AND User Organiser, they will also have all the rights of Importer and Group Organiser.
    I think we should maybe allow the user to pick the rights themselves, but not allow combinations that make no sense. E.g. Write Owned (always includes Write File).
    Then the list might look something like:
  • Create data As (Write File, Write Owned, Sudo)
  • Upload Official Script
  • Chgrp
  • Chown
  • Modify User
  • Modify Group

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

This comment has been minimized.

Show comment
Hide comment
@mtbc

mtbc Feb 6, 2017

Member

@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.

Member

mtbc commented Feb 6, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@pwalczysko

pwalczysko Feb 6, 2017

Member

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 #62 (comment). The interesting more involved questions:

  • Possibly we should always give WriteOwned (needed for creation of new containers in a group you are not a member of) together with Chown and Chgrp ?
  • How to fiddle out the cases where the Sudo is shunned, means no deletion authority is forced onto the light admin, meaning more clanky non-Sudo import workflows are needed ?
  • Similar to previous point: How to deal out the Delete.. powers (most important of which is DeleteOwned) in general ? Possibly additional check-boxes after each creation of a new light admin (can this new guy delete as well or not ?) - of course we have to marry that with cases where he has a Sudo already

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

Member

pwalczysko commented Feb 6, 2017

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 #62 (comment). The interesting more involved questions:

  • Possibly we should always give WriteOwned (needed for creation of new containers in a group you are not a member of) together with Chown and Chgrp ?
  • How to fiddle out the cases where the Sudo is shunned, means no deletion authority is forced onto the light admin, meaning more clanky non-Sudo import workflows are needed ?
  • Similar to previous point: How to deal out the Delete.. powers (most important of which is DeleteOwned) in general ? Possibly additional check-boxes after each creation of a new light admin (can this new guy delete as well or not ?) - of course we have to marry that with cases where he has a Sudo already

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

@pwalczysko

This comment has been minimized.

Show comment
Hide comment
@pwalczysko

pwalczysko Feb 6, 2017

Member

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

  • Red fields are "you must give this perm for this workflow",
  • Green fields "keep calm, this perm is not necessary".
  • Orange fields are "you do not have to, but you better give this perm for this workflow, as it is not totally expected behaviour that this perm is not needed, this behaviour might change in future". Or maybe in another case "it makes a good workflowy sense to give this permission too".
  • Magenta is a quirky workflow which will probably not be ready for 5.3.0 - on Marks ToDo list, but not priority

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

Member

pwalczysko commented Feb 6, 2017

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

  • Red fields are "you must give this perm for this workflow",
  • Green fields "keep calm, this perm is not necessary".
  • Orange fields are "you do not have to, but you better give this perm for this workflow, as it is not totally expected behaviour that this perm is not needed, this behaviour might change in future". Or maybe in another case "it makes a good workflowy sense to give this permission too".
  • Magenta is a quirky workflow which will probably not be ready for 5.3.0 - on Marks ToDo list, but not priority

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

@will-moore

This comment has been minimized.

Show comment
Hide comment
@will-moore

will-moore Feb 8, 2017

Member

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

  • Sudo [You are the user. Can import, edit, delete etc as the user]
  • Write Owned [You can create, edit & link other users' data]
  • Chgrp
  • Chown
  • Delete
  • Edit Group
  • Edit User
  • Edit Group-User links
  • Upload official Script

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?

Member

will-moore commented Feb 8, 2017

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

  • Sudo [You are the user. Can import, edit, delete etc as the user]
  • Write Owned [You can create, edit & link other users' data]
  • Chgrp
  • Chown
  • Delete
  • Edit Group
  • Edit User
  • Edit Group-User links
  • Upload official Script

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

This comment has been minimized.

Show comment
Hide comment
@jburel

jburel Feb 8, 2017

Member

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),

Member

jburel commented Feb 8, 2017

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

This comment has been minimized.

Show comment
Hide comment
@mtbc

mtbc Feb 8, 2017

Member

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?

Member

mtbc commented Feb 8, 2017

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?

@pwalczysko

This comment has been minimized.

Show comment
Hide comment
@pwalczysko

pwalczysko Feb 8, 2017

Member

Discussed with @mtbc about the annotation powers of light admins. Basically, there should be no difference between the linking of Project-Dataset and linking Image-Annoation. This means that when they canLink they canAnnotate (the WriteOwned permission is necessary for this). This is possible in RW, RA and RO groups but not private groups. Will investigate the annotation behaviour further (can light admin create new Tags, File Annotations (in a group he is not a member of, which is the default for light admins).

Member

pwalczysko commented Feb 8, 2017

Discussed with @mtbc about the annotation powers of light admins. Basically, there should be no difference between the linking of Project-Dataset and linking Image-Annoation. This means that when they canLink they canAnnotate (the WriteOwned permission is necessary for this). This is possible in RW, RA and RO groups but not private groups. Will investigate the annotation behaviour further (can light admin create new Tags, File Annotations (in a group he is not a member of, which is the default for light admins).

@pwalczysko

This comment has been minimized.

Show comment
Hide comment
@pwalczysko

pwalczysko Feb 8, 2017

Member

@will-moore :

Presumably this would also remove annotations etc?

As @mtbc expected, and my new two tests for Chgrp and Chown show, the Chown and Chgrp permissions are fully equipped and self-sufficient to do whatever it is they need to do. I.e. when the Chgrp or Chown action needs for its execution to sever some links, it will do so, no additional permissions needed (e.g. move an image included in a dataset to another group, or Chown an image included in a dataset to somebody else in private group).

Member

pwalczysko commented Feb 8, 2017

@will-moore :

Presumably this would also remove annotations etc?

As @mtbc expected, and my new two tests for Chgrp and Chown show, the Chown and Chgrp permissions are fully equipped and self-sufficient to do whatever it is they need to do. I.e. when the Chgrp or Chown action needs for its execution to sever some links, it will do so, no additional permissions needed (e.g. move an image included in a dataset to another group, or Chown an image included in a dataset to somebody else in private group).

@pwalczysko

This comment has been minimized.

Show comment
Hide comment
@pwalczysko

pwalczysko Feb 10, 2017

Member

@will-moore : The last test shows that for adding a File Attachment (wich Original File) to an image, the light admin needs WriteOwned and WriteFile permissions. This workflow is very important for any image analyst (the results will be delivered by him/her and uploaded to OMERO as file attachments. Also, the analyst will need sometimes to be able to import new images for them (for which he/she needs WriteManagedRepo as well). He will be unlikely to use Sudo though, as these processes might be run as part of his scripts. I would suggest we need to add another layter in your bullet point list just after - Write Owned [You can create, edit & link other users' data], namely

  • WriteFile & WriteManagedRepo (you can upload (assuming you have WriteOwned as well) File Attachment (results of analysis) and import an image not using Sudo)
Member

pwalczysko commented Feb 10, 2017

@will-moore : The last test shows that for adding a File Attachment (wich Original File) to an image, the light admin needs WriteOwned and WriteFile permissions. This workflow is very important for any image analyst (the results will be delivered by him/her and uploaded to OMERO as file attachments. Also, the analyst will need sometimes to be able to import new images for them (for which he/she needs WriteManagedRepo as well). He will be unlikely to use Sudo though, as these processes might be run as part of his scripts. I would suggest we need to add another layter in your bullet point list just after - Write Owned [You can create, edit & link other users' data], namely

  • WriteFile & WriteManagedRepo (you can upload (assuming you have WriteOwned as well) File Attachment (results of analysis) and import an image not using Sudo)
@pwalczysko

This comment has been minimized.

Show comment
Hide comment
@pwalczysko

pwalczysko Feb 10, 2017

Member

@will-moore : Note that your second bullet point Write Owned [You can create, edit & link other users' data] should be probably now put more precise as Write Owned (You can create, edit & link and annotate other user's data except file annotations (but including Tags, MapAnnotations..., ROIs and Rnd settings). Nevertheless, linking and annotating (which is principally linking the annotations) CANNOT be done in private groups.

Member

pwalczysko commented Feb 10, 2017

@will-moore : Note that your second bullet point Write Owned [You can create, edit & link other users' data] should be probably now put more precise as Write Owned (You can create, edit & link and annotate other user's data except file annotations (but including Tags, MapAnnotations..., ROIs and Rnd settings). Nevertheless, linking and annotating (which is principally linking the annotations) CANNOT be done in private groups.

@will-moore

This comment has been minimized.

Show comment
Hide comment
@will-moore

will-moore Feb 13, 2017

Member

As discussed with @pwalczysko, it probably makes sense to group WriteOwned, WriteFile and WriteManagedRepo into a single checkbox in the UI, since there isn't really a case for needing these roles in isolation.
Similarly for DeleteOwned, DeleteFile and DeleteManagedRepo.

Member

will-moore commented Feb 13, 2017

As discussed with @pwalczysko, it probably makes sense to group WriteOwned, WriteFile and WriteManagedRepo into a single checkbox in the UI, since there isn't really a case for needing these roles in isolation.
Similarly for DeleteOwned, DeleteFile and DeleteManagedRepo.

@mtbc

This comment has been minimized.

Show comment
Hide comment
@mtbc

mtbc Feb 13, 2017

Member

@pwalczysko points out that as ModifyGroupMembership allows light admins to make normal users into group owners then those are possibly more powerful than the light admin. For example, they may be able to delete data that the light admin cannot. If or how best to address this is not yet clear to me.

Member

mtbc commented Feb 13, 2017

@pwalczysko points out that as ModifyGroupMembership allows light admins to make normal users into group owners then those are possibly more powerful than the light admin. For example, they may be able to delete data that the light admin cannot. If or how best to address this is not yet clear to me.

@jburel

This comment has been minimized.

Show comment
Hide comment
@jburel

jburel Feb 13, 2017

Member

@mtbc @pwalczysko is that really an issue?
I can see a light admin, making a PI a group owner so he/she can manage the users in the facility without having to go back to the administrator of the system

Member

jburel commented Feb 13, 2017

@mtbc @pwalczysko is that really an issue?
I can see a light admin, making a PI a group owner so he/she can manage the users in the facility without having to go back to the administrator of the system

@mtbc

This comment has been minimized.

Show comment
Hide comment
@mtbc

mtbc Feb 13, 2017

Member

Mmm, hence my "if". 😃

Member

mtbc commented Feb 13, 2017

Mmm, hence my "if". 😃

@pwalczysko

This comment has been minimized.

Show comment
Hide comment
@pwalczysko

pwalczysko Feb 13, 2017

Member

@mtbc @jburel : Yes, agree, in the first instance, we should just let it be as it is, and explain the feature. There is nothing wrong per se with the workflow, it was just a "gotcha" for me I did not think about before.

Member

pwalczysko commented Feb 13, 2017

@mtbc @jburel : Yes, agree, in the first instance, we should just let it be as it is, and explain the feature. There is nothing wrong per se with the workflow, it was just a "gotcha" for me I did not think about before.

@will-moore will-moore referenced this issue Feb 13, 2017

Merged

New role webadmin #5101

3 of 3 tasks complete
@pwalczysko

This comment has been minimized.

Show comment
Hide comment
@pwalczysko

pwalczysko Feb 14, 2017

Member

@will-moore @mtbc @jburel : Note on ModifyGroupMembership: When this is the only permission which the light admin has (all the rest is forbidden), then the Web UI cannot be used for adding/removing of the users from groups, because it is probably trying to update the user/group, which is throwing securityViolation, as the light admin does not have the ModifyUser and ModifyGroup permissions.

The CLI is in this case absolutely fine though. The ModifyGroupMembership role in isolation (without the ModifyUser and ModifyGroup) has sense when using the CLI, but not Web. For Web, we would have to give the ModifyUser as well as ModifyGroup permissions additionaly.

Member

pwalczysko commented Feb 14, 2017

@will-moore @mtbc @jburel : Note on ModifyGroupMembership: When this is the only permission which the light admin has (all the rest is forbidden), then the Web UI cannot be used for adding/removing of the users from groups, because it is probably trying to update the user/group, which is throwing securityViolation, as the light admin does not have the ModifyUser and ModifyGroup permissions.

The CLI is in this case absolutely fine though. The ModifyGroupMembership role in isolation (without the ModifyUser and ModifyGroup) has sense when using the CLI, but not Web. For Web, we would have to give the ModifyUser as well as ModifyGroup permissions additionaly.

@pwalczysko

This comment has been minimized.

Show comment
Hide comment
@pwalczysko

pwalczysko Feb 15, 2017

Member

Further note on ModifyUser, ModifyGroup and ModifyGroupMembership. It turned out again when testing the ModifyGroup privilege in isolation that the user has a poor experience in Web (some red message saying "you are not an admin or owner of the group" but then the action proceeds !). So I might try to formulate in the doc something like :

If your newly created Admin with restricted privileges plans to use Web UI for user/group administration, give always all three ModifyUser, ModifyGroup and ModifyGroupMembership, when you create this Admin. 
Member

pwalczysko commented Feb 15, 2017

Further note on ModifyUser, ModifyGroup and ModifyGroupMembership. It turned out again when testing the ModifyGroup privilege in isolation that the user has a poor experience in Web (some red message saying "you are not an admin or owner of the group" but then the action proceeds !). So I might try to formulate in the doc something like :

If your newly created Admin with restricted privileges plans to use Web UI for user/group administration, give always all three ModifyUser, ModifyGroup and ModifyGroupMembership, when you create this Admin. 
@mtbc

This comment has been minimized.

Show comment
Hide comment
@mtbc

mtbc Feb 16, 2017

Member

For making privilege elevation prohibition more obvious client-side (when I get the enforcement working!) might want to gray out any privileges that rely on anything not included in the admin service's getCurrentAdminPrivileges().

Member

mtbc commented Feb 16, 2017

For making privilege elevation prohibition more obvious client-side (when I get the enforcement working!) might want to gray out any privileges that rely on anything not included in the admin service's getCurrentAdminPrivileges().

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment