Allow display of ex-members data in group

[will-moore]

Currently, if you set the user context (for filtering data in the tree) to a user who is not in the current group, the filtering will automatically switch to "All Members". However, this prevents filtering by an Experimenter who has left a Group: see https://forum.image.sc/t/visibility-of-data-from-a-user-that-left-the-group/117309

This PR removes that restriction, allowing you to filter by a user who is not a member of the current Group.

With these changes you can: use ?experimenter=123 or ?image-123 (image owned by ex-member) to switch the filtering of a group to the Ex member (without changing the group context or switching to "All Members").

E.g. this workflow...

• Filter a group by "All Members" to allow you to find an image owned by an Ex Member
• Copy the Link to that image
• Switch back to only showing your own images (to switch off "All Members")
• Then go to the image link. This will switch to filtering to by Ex Member - (and will show the chosen image)

A follow-up PR could add options to the Group-User dropdown menu to check for the existence of any data that belongs to non-group members (see discussion on image.sc above).

[imagesc-bot]

This pull request has been mentioned on Image.sc Forum. There might be relevant details there:

https://forum.image.sc/t/visibility-of-data-from-a-user-that-left-the-group/117309/6

[JensWendt]

I wanted to try this out and on my sandbox I ran /opt/omero/web/venv3/bin/pip install git+https://github.com/will-moore/omero-web.git@ex_members_group_data which installed me a version 5.29.3.dev0.
Unfortunately, after a omero web restart I could not get it to start properly with this error message:

Traceback (most recent call last):

  File "/opt/omero/web/venv3/lib/python3.10/site-packages/django/core/handlers/exception.py", line 55, in inner
    response = get_response(request)

  File "/opt/omero/web/venv3/lib/python3.10/site-packages/django/core/handlers/base.py", line 197, in _get_response
    response = wrapped_callback(request, *callback_args, **callback_kwargs)

  File "/opt/omero/web/venv3/lib/python3.10/site-packages/omeroweb/decorators.py", line 538, in wrapped
    retval = f(request, *args, **kwargs)

  File "/opt/omero/web/venv3/lib/python3.10/site-packages/omeroweb/decorators.py", line 597, in wrapper
    context = f(request, *args, **kwargs)

  File "/opt/omero/web/venv3/lib/python3.10/site-packages/omeroweb/webclient/views.py", line 578, in load_template
    return _load_template(request=request, menu=menu, conn=conn, url=url, **kwargs)

  File "/opt/omero/web/venv3/lib/python3.10/site-packages/omeroweb/webclient/views.py", line 558, in _load_template
    context["active_user"] = conn.getObject("Experimenter", int(user_id))

TypeError: int() argument must be a string, a bytes-like object or a real number, not 'NoneType'

<WSGIRequest: GET '/webclient/'>

Any other way to test this?

[will-moore]

Apologies - haven't tested much! Hopefully that commit should fix that.

[will-moore]

@JensWendt - Did you manage to try this workflow above?

[will-moore]

This is causing a failing test https://merge-ci.openmicroscopy.org/jenkins/job/OMERO-test-integration/544/testReport/OmeroWeb.test.integration.test_tree/TestTree/test_marshal_experimenter/

The test fails as it's expected that def marshal_experimenter() shouldn't allow a regular user to access an experimenter who is not in any of your groups.
However, the server already provides the security needed here by returning '' fields.
So the test fails with:

AssertionError: assert {'firstName': '<hidden>', 'id': 4562, 'lastName': '<hidden>', 'omeName': '<hidden>'} == None

I don't think that marshal_experimenter() experimenter should try to restrict access further, so I propose we update the test to the new behaviour.

[will-moore]

Excluding to avoid failing test till I have time to discuss/fix the test

--exclude

[JensWendt]

Hey Will,

sorry for the delay, I was prepping/holding a workshop and then I was sick.
I tested the described workaround and can confirm it does work on my sandbox.
This is better than nothing, but for everyday users this will not really be usable.
The amount of stuff in one group is too much to keep searching for stuff of the user that left.
One would have to keep a separate "left user cheat sheet" with example URLs of stuff for every user that left.
BUT, I do not want to sound ungrateful, thank you for working on this!
I hope we can have nice GUI based solution in the future at some point, but I understand you guys are as swamped with work/feature requests as everyone else.

[knabar]

@will-moore since removing the membership check happens in methods used for different templates, are there any other features impacted by this change? Will it be possible to access data in other places now that was prohibited before? I am trying to determine what the general security implications are.

[will-moore]

In general, I don't think omero-web should be trying to apply any security / permissions that's different from the server.
If the server allows it then the web should too.

There is just the one failing test above with this change, so I don't think it has a big impact.
I will update the test if we agree that this PR is otherwise good to go?

[knabar]

In general, I don't think omero-web should be trying to apply any security / permissions that's different from the server. If the server allows it then the web should too.

Totally agreed, security is not quite the right term to use here. I'm more talking about impact on other features, and I guess why this restriction was there in the first place?

[will-moore]

This restriction was added because we thought that if you are trying to set the context to show "user-1's" data in "group A" then you should first ensure that user-1 is a member of group A. We never considered the case where user-1 could have left group A. So, if you are in group A, trying to switch context to user-1 and they're not a member, then you abort the context switch.

So if you are in group A (or a different group), looking at someone else's data, then you follow a link to an image belonging to user-1 in that group,
e.g. https://merge-ci.openmicroscopy.org/web/webclient/?show=image-2503
then you won't see that image because you won't switch to user-1. This is really a bug.

But if you're already looking at "All Members" then that same link will work OK.

[knabar]

So if you are in group A (or a different group), looking at someone else's data, then you follow a link to an image belonging to user-1 in that group, e.g. https://merge-ci.openmicroscopy.org/web/webclient/?show=image-2503 then you won't see that image because you won't switch to user-1. This is really a bug.

But if you're already looking at "All Members" then that same link will work OK.

Understood, yet I still think that we should address and support this in a different way.

Some of these changes were introduced in b5fa7b7 in response to a security vulnerability and released in a combined OMERO.server 5.6.1 and OMERO.web 5.6.3 release (see https://www.openmicroscopy.org/2020/03/25/omero-5-6-1.html), so at this point I'm not convinced that removing the restriction is a good idea.

If we want to support filtering by experimenters who are no longer in the current group, it might be better to add targeted exceptions or a new feature that allows users to do that.

[will-moore]

I think the change in b5fa7b7 was introduced along with server changes that also obscured details of Experimenters who you don't share a group with.
https://www.openmicroscopy.org/security/advisories/2019-SV3/

I think we made the changes in web as a "belt and braces" approach, before we had the fix in the server (even though both were released at the same time).
Now, the server hides this automatically and instead gives you <hidden> <hidden> instead of user names (see the tree and right-panel with this PR):

[will-moore]

I guess there are 2 parts to this PR:

• In tree.py, we previously add a filter to the sql so that we only find users that are in our own groups. So, that on merge-ci, logged-in as user-3, /webclient/api/experimenters/52/ gives me 404 whereas the JSON api allows me to access that user at /api/v0/m/experimenters/52/ showing

...
"FirstName": "<hidden>",
"LastName": "<hidden>",
"UserName": "<hidden>",

Removing the sql filter means that I can access that user via /webclient/api/experimenters/52/ in the same way.

• In views.py, we previously check that either the experimenter is -1 (which allows us to see data belonging to all users, including ex-members) or if the experimenter is in the group. This prevents us switching the experimenter context to filter by ex-members. That might seem like a good thing if it was impossible for non-members to have data in the group (likely the original understanding). However, we now know differently.

By removing the check, we allow you to filter a group by any user. So, without changing the group context, you can manually go to /webclient/?experimenter=2 and you'll filter the data by Experimenter:2. This may show you no data (if that experimenter has no data in the current group), but that's not really a bug - and you'd only see this if you manually entered the URL.
If you followed a link /webclient/?show=image-123 as above, then you would also be able to switch context to the owner of the image (even if they're not a member of the group that the image is in), and you wouldn't find no data.