Add integration test for joining session

[dominikl]

Adds an integration test for reproducing issue https://github.com/ome/omero-insight/issues/293 , join via session id and browse different groups.

Currently fails, only works for the Read-Annotate group, fails for other groups.

/cc @jburel

[jburel]

So we can rule out that is an issue with insight itself.

[dominikl]

Yes, I think the fact that it depends on the group permission if the test fails, suggest that it's neither Insight nor Java gateway related. Server side issue?

[dominikl]

No, the problem is actually in the Gateway. I don't know why it works for the read-annotate group, but for the others it fails when line https://github.com/ome/omero-gateway-java/blob/master/src/main/java/omero/gateway/Gateway.java#L1773 is hit. Because for the session id login the id is only set as username and not as password it fails. If you set it for both the test works for all groups. Hopefully shouldn't be hard to fix. Edit: Now I know why it works for the read-annotate group, because it's the user's default group, no group switch is necessary in that case.

[jburel]

Tested using today's insight and it still fails from insight insight has gateway 5.6.10-snapshot

[chris-allan]

I've spent quite a bit of time looking at this and I think I've come to the conclusion that as currently architected it's not possible for the Java gateway to function correctly across multiple groups when a session ID is provided as a means to log in.

Fundamentally, this is rooted in the Java gateway's strategy of handling access to multiple groups by creating a new session for each group and following it up with a call to setSecurityContext(). This is not strictly at odds with the current documentation ^1 but is certainly not adhering to the spirit and best practices we have been suggesting the community follow since late OMERO 4 when cross group querying was introduced. As discussed with @joshmoore, users may not create new sessions for themselves via ISession.createSessionWithTimeout() unless they have been granted group sudo privileges.

This strategy has performance and scalability implications beyond the problems outlined in ome/omero-insight#293 especially if a user's group membership is high.

omero-py has a few instances in the Python gateway where setSecurityContext() is present but none of these are used by any of our clients. There are also some instances in omero-web in dead code paths. I'd recommend that we deprecate these for removal ASAP.

Finally, there seems to be some confusion about the differences between createSession(username, password) and joinSession(session) in the Java gateway. I'm not sure where the idea from ome/omero-gateway-java#62 came from but what joinSession(session) does is call createSession(session, session) behind the scenes ^2. It's possible these things got muddled up when ome/openmicroscopy#5513 was added. I'm not sure why we started doing checks to find out whether something appears to be a session ID or not. As far as client is concerned there is no difference in outcome when using a session ID with either of these two calls. It's likely in our interest to resolve these inconsistencies, at best they are confusing to a user. Use of Connector.getConnector() should also be reviewed and the documentation for setSecuritySession() expanded.

So that OMERO.insight does not behave erratically we should probably prevent cross group use when a session ID is in use.

/cc @joshmoore, @sbesson

[dominikl]

So maybe this multi-group feature should be removed generally from the gateway (and thus from Insight), like it is for python/web? Would have made things much easier from the start.

users may not create new sessions for themselves via ISession.createSessionWithTimeout() unless they have been granted group sudo privileges.

Afaik createSessionWithTimeout() is already only used in the context of sudo.

I'm not sure where the idea from https://github.com/ome/omero-gateway-java/pull/62 came from but what joinSession(session) does is call createSession(session, session) ...

The idea came from the fact that there are these two seemingly different methods in the client in the first place (I didn't check that they're basically the same thing), and the exception itself says Glacier2.PermissionDeniedException reason = "username and password must be equal; use joinSession"

[chris-allan]

So maybe this multi-group feature should be removed generally from the gateway (and thus from Insight), like it is for python/web? Would have made things much easier from the start.

The current design certainly precludes use with a session ID. Whether that was even considered when those decisions were made and would have made things much easier from the start is likely lost to the annals of time. It has been this way for at least 11 years, certainly before the Java gateway existed as a separate thing.

Should we have changed the design when the cross group querying features of OMERO 4.4. were added? Possibly, but again that rationale and the specifics of those discussions is likely lost to the annals of time. Now? Definitely, but I'm pretty sure that means a near complete rewrite; every method call needs to know the group context in which it is operating with the Python gateway style.

Afaik createSessionWithTimeout() is already only used in the context of sudo.

As Gateway.getConnector() is a public method I think we should be careful with that assumption. At the very least it's probably important that we update the documentation. Having the methods that call Gateway.getConnector() enforce this criteria is not a good idea going forward in my opinion.

and the exception itself says Glacier2.PermissionDeniedException reason = "username and password must be equal; use joinSession"

Under what conditions were you getting that exception? At the very least we should make that message clearer.

[jburel]

Insight was initially not meant to support login using session ID. This was added afterwards to allow its usage for place like ome-ssbd. One option (which does not solve the gateway) for insight could be to only load the group the session is associated to but this will require a review of insight. We should be careful before starting such effort.

As for the exception mentioned above, it happens when insight connects using a session ID.

[chris-allan]

Insight was initially not meant to support login using session ID.

Certainly based on the architecture I guess that's now very obvious since we're not testing it and it's been like this for over a decade. There's also no way to prevent this without performing more interrogation of the session than we are.

As for the exception mentioned above, it happens when insight connects using a session ID.

Under what exact conditions? joinSession() is just calling createSession().

[dominikl]

I think it must happen when createSession is called with a session id as username and an empty or null password.

[jburel]

insight connect using cli arguments

List<String> res = new ArrayList<>();
res.add("--omero.user="+escape(getUser().getUsername()));
res.add("--omero.pass="+escape(getUser().getPassword()));
res.add("--omero.host="+getServer().getHost());
res.add("--omero.port="+getServer().getPort());

then createSession() is invoked. In that case it fails with the error above if a session key is used. if joinSession(sessionKey) is used, it potentially failed with another error. Note that the password is not set when a sessionKey is used

[chris-allan]

I think it must happen when createSession is called with a session id as username and an empty or null password.

That would make sense.

I don't want to be pedantic but is that then not telling a user exactly what they need to do? Either they need to call createSession(session, session) or joinSession(session) (which is really doing the first thing under the hood). What language would you suggest we use?

then createSession() is invoked. In that case it fails with the error above if a session key is used.

Then we're not calling createSession(session, session) which is obviously incorrect and what the exception says.

[dominikl]

--exclude this for now

[dominikl]

I'm totally lost now on this one... shall I add the tests again or remove the PR?