Handle invalid group id in switch_active_group

[will-moore]

Fixes #381

In other usages of get_long_or_default we wrap it with try/catch as we now do here...

This avoids ValueError when group id is not a number, e.g.

/webclient/active_group/?active_group=53foo

Also, we want to avoid errors when the active_group is an invalid ID, so we test for that when changing active_group via the URL above.

To test:

• Check that /webclient/active_group/?active_group=53foo gives no error (no change in group)
• Same for e.g. /webclient/active_group/?active_group=123456 (invalid group ID)
• Also check that switching group via webclient UI works as before, and that you can still switch group using e.g. `/webclient/?show=image-123

[sbesson]

From a quick round of testing, the current handling of the active_group query parameter is causing 500 errors on invalid inputs or non-existing group IDs. The proposed changes code defensively against these situations.

Biggest inline question is how far the additional checks should go in terms of group validation in this API

[sbesson]

From a quick round of testing accessing a non-existing but valid group ID e.g. /active_group/?active_group=9999, this PR might also fix #387

[sbesson]

As this is adding a check for the group existence, have you considered whether it should also check for group membership for the current user?
Are there some redundancies/performance implications?

[will-moore]

@sbesson Thanks, yes - that's actually a good catch because without it, you can choose a group that you're not a member of. Then, when the main webclient page loads, and you try to load containers for the tree, you get :

return HttpResponseForbidden("Not a member of Group: %s" % group_id)

which causes the webclient page to refresh, which simply gets you into an infinite refresh loop!
This is a current bug before this PR.

Using isValidGroup() in 36c35ae avoids this problem and also is faster than conn.getObject("ExperimenterGroup", active_group) since it uses the eventContext.memberOfGroups which we have in hand.
Also, it allows admins to switch to groups that they are not a member of, which is an important use case.

[sbesson]

The usage of the isValidGroup API is consistent with

omero-web/omeroweb/webclient/views.py

Lines 470 to 471 in 1429a04

	if conn.isValidGroup(group_id):
	switch_active_group(request, group_id)

to validate the group permissions before making the switch.

With the latest change and using a regular user, I tested the following scenarios for the active_group query parameter:

1. non-integer valuee.g. mygroup
2. integer value not matching an existing group e.g. -2
3. integer value matching an existing group the user is not member of
4. integer value matching an existing group the user is a member of

All scenarios worked as expected with the first three first configurations resulting in no context change while scenario 4 switched the group context as expected.

Tested the same configurations with an administrative user. Scenarios 1, 3 and 4 worked as expected with the admin user being able to switch to any group independently of the membership. However, scenario 2 failed with the same error as #387. This is because the isValidGroup implementation returns True immediately if the authenticated user is an admin independently of the validity of the group ID.

Similarly, passing -1 as the value for the active_group query parameter value bypasses the membership checks and results in #387.

I suspect restoring the conn.getObject("ExperimenterGroup", active_group) is not None check before calling conn.isValidGroup(active_group) should take care of all the scenarios above.

[will-moore]

@sbesson Thanks for the testing - I've added the group check back now.

[sbesson]

Retested all scenarios above i.e. active_group query parameter with the following values:

• non integer e.g. mygroup
• integer not matching an existing group ID e.g. -2 or 9999
• -1
• existing group ID of which the user is member of
• existing group ID of which the user is not member of

With both an admin and non admin authenticated user. Everything now behaves as expected in the UI.

[knabar]

Thank you @will-moore and @sbesson - merging now