Ome user id fix

[will-moore]

This fixes a bug from #278.
See #278 (comment)

It was caused because I used the ome.user_id template context assuming it referred to the currently logged-in user (similar to ome.user.id) but it actually referred to the current browsing context, so for "All Members" it was -1 which caused an error generating URLs for manage_avatar URLs (upload, crop & delete).

The fix is to always use ome.user_id to refer to the logged-in user, and use ome.active_user to refer to the current viewing context, as we already do with ome.active_group. This consistency will help avoid similar errors in future.

To test:

• Show "All Members" (webclient/?experimenter=-1) then browse to User Settings page.
• Confirm that Change and Delete photo still working as normal

Since this bug is affecting users, it would be nice to get it released soon. cc @erindiel

[snoopycrimecop]

Conflicting PR. Removed from build OMERO-python-superbuild-push#686. See the console output for more details.
Possible conflicts:

• PR Update jquery #180 will-moore 'Update jquery'
  • omeroweb/webadmin/templates/webadmin/myaccount.html
• PR fix editphoto #274 will-moore 'fix editphoto'
  • omeroweb/webadmin/templates/webadmin/avatar.html
  • omeroweb/webadmin/templates/webadmin/myaccount.html

--conflicts Conflict resolved in build OMERO-python-superbuild-push#690. See the console output for more details.

[sbesson]

Can the integration tests introduced in ome/openmicroscopy#6269 be easily amended to test both scenarios (single-group context + group -1 context) so that we can flag regressions in the future?

[dominikl]

Tested the functionality. The PR fixes the issue. Workflow gives 500 on latest-ci, but works fine on merge-ci 👍

[will-moore]

@sbesson Added a test at ome/openmicroscopy@27c9670 to check User Settings page when experimenter context is -1.

[sbesson]

The biggest downside of this approach is that it will break all calls of type https://myserver/webadmin/change_avatar/xxx/{upload,deletephoto}/ in favor of https://myserver/webadmin/change_avatar/{upload,deletephoto}/.

From my understanding of the associated code, the passed ID was silently ignored anyways and the current user ID used under the hood. Was the ability to set/delete other's avatar ever functional?

Trying to make the minimal assumptions about consumers, is it possible to update this to keep supporting both forms of URLs but use the same view?

[jburel]

This is a breaking change, so we should try to handle both url formats if possible

[will-moore]

I can certainly allow change_avatar/xxx/{upload,deletephoto}/ but for how long do we need to support it and how would we mark it as deprecated?
I don't think there was ever the ability to upload or delete another user's photo and I think it very unlikely that URL would be used by any external application (delete was even unusable in webclient till just now (#274).
So the sooner we can remove it the better.

[joshmoore]

Was the ID previously always ignored even if for another user?

[will-moore]

Yes. As you can see in master (and not changed in this PR) manageavatar() view function makes no use of any eid:

omero-web/omeroweb/webadmin/views.py

Line 1089 in 688acfe

def manage_avatar(request, action=None, conn=None, **kwargs):

Same in v5.9.1 etc.

[joshmoore]

👍 I was wondering if that means we should disallow now eid != current user.

[sbesson]

Tested against the CI server using a logged user:

• using the UI, both the upload workflow User settings -> Change -> Browse -> Upload as well as the deletion workflow User settings -> Delete Photo -> Confirm Delete
• in terms of endpoint, selecting Change now redirects you to /webadmin/change_avatar/ endpoint. On a production deployment with OMERO.web 5.9.1, this opens /webadmin/change_avatar/<id>/. Confirmed that /webadmin/change_avatar/<id> is still functional with this PR included. In both cases (with this PR and with 5.9.1), any value of <id> redirects you to the current user ID view.
• With this PR included, any GET call to /webadmin/change_avatar/foo/ or any /webadmin/change_avatar/<id>/foo/ work and redirect you to the webadmin/change_avatar view independently of the ID.

Generally, this PR seems to be deprecating the change_avatar/<id>/[<action>/] URLs in favor of change_avatar/[<action>/] which I think is completely fine and we can drop the deprecated form in the next breaking release. I do not know whether there is a canonical way to mark this deprecation in the code.

On the handling of IDs different than the current user, I am not strongly opinionated. I could see some feedback/error indicating this path is disallowed. On the other side, it might be overkill if this is effectively deprecated and we are moving towards the ID-less form.

[will-moore]

I think it has been broken like this for a long time, but likely never used outside of the webadmin UI. I don't think we need to add any validation of the ignored eid value.

[will-moore]

Just saw this error reported on nightshade:

File "/opt/omero/web/venv3/lib64/python3.6/site-packages/django/urls/base.py" in reverse
  91.     return force_text(iri_to_uri(resolver._reverse_with_prefix(view, prefix, *args, **kwargs)))

File "/opt/omero/web/venv3/lib64/python3.6/site-packages/django/urls/resolvers.py" in _reverse_with_prefix
  497.         raise NoReverseMatch(msg)

Exception Type: NoReverseMatch at /webadmin/myaccount/edit/
Exception Value: Reverse for 'wamanageavatar' with arguments '(-1,)' not found. 1 pattern(s) tried: ['(?i)webadmin/change_avatar/(?P<eid>[0-9]+)/(?:(?P<action>[a-z]+)/)?$']
Request information:
USER: [unable to retrieve the current user]