Fix Connector from session constructor

[gaelrayot-epfl]

In some race condition, seems the session.get return the Connector instance, not a serialize version of the instance.

Fix #459

[will-moore]

Hi @gaelrayot-epfl - thanks for the bug report and fix.
As @sbesson mentioned, it looks like a result of #435 where you have a session that was created before upgrading omero-web that is still retrieved after the upgrade (so it has the Connector object itself instead of JSON).

Change looks good.
In deliberately reproducing the upgrade bug by switching omero-web versions in a development environment, this PR fixed the reported bug, but I also needed this change (that uses your fix):

diff --git a/omeroweb/webgateway/views.py b/omeroweb/webgateway/views.py
index 7cfcda1a4..1619d0793 100644
--- a/omeroweb/webgateway/views.py
+++ b/omeroweb/webgateway/views.py
@@ -1495,7 +1495,7 @@ def jsonp(f):
         try:
             server_id = kwargs.get("server_id", None)
             if server_id is None and request.session.get("connector"):
-                server_id = request.session["connector"]["server_id"]
+                server_id = Connector.from_session(request).server_id
             kwargs["server_id"] = server_id

For the GPL-licensed OME projects, I must ask you to fill out a Contributor's License Agreement as described at https://ome-contributing.readthedocs.io/en/latest/cla.html before we can accept your PR.

Thanks

[sbesson]

Interestingly, this upgrade issue was not detected on IDR where OMERO.web has been recently upgraded to OMERO.web 5.18.0. I suspect the reason is because IDR uses a blue-green deployment strategy which means it does not leave any session.

I was able to reproduce the issue by using one of the IDR pilot dev systems. After downgrading OMERO.web 5.17.0, logging in as the public user, and running

sudo /opt/omero/web/venv3/bin/pip install omero-web==5.18.0 && sudo systemctl restart omero-web

refreshing the browser suffices to instantaneously trigger the exception raised in #459. Opening the webclient in a separate browser works without issue i.e. it does not try to use the pre-upgrade session and creates a new connection.

I was able to reproduce the output described by Will in #460 (comment) after downgrading and re-upgrading OMERO.web to the HEAD of this PR

$ sudo /opt/omero/web/venv3/bin/pip install git+https://github.com/ome/omero-web@refs/pull/460/merge && sudo systemctl restart omero-web

The webclient reloaded without issue but fails quickly when trying to access a key from request.session["connector"]. The diff suggested in #460 (comment) points at the relevant line but I am concerned it will force us to review every single usage of request.session["connector"].

If the goal is to upgrade existing sessions, would it make sense to upgrade the serialized objet as well i.e. make an additional v.to_session(request) call in the new conditional block before returning the Connector?

[gaelrayot-epfl]

Hi @will-moore

I've just sent you the CLA form.

Thanks.

[will-moore]

Actually, I realise that server_id = request.session["connector"]["server_id"] happens in a dozen or more places which will all need the above fix if we want to support this?

I'm not even sure we really need the server_id in most of these cases, but let's not change that just now.

[sbesson]

As shown by the testing, one of the issue of trying to re-use an existing Connector object from a previous (pre-upgrade) session is that it leaves other objects like request.session["connector"] in a state incompatible with the assumptions of the OMERO.web code.

#435 discusses other scenarios which might lead to similar incompatible states e.g. switching between different session serializers. From a discussion with @chris-allan, we probably need to be more defensive in OMERO.web, catch these scenarios and ensure a new session get created. From the user perspective, this will either happen automatically if the public user is configured or redirect to the login page.

Specifically in that case, could we catch the scenarios where v is not a dictionary that can be used to instantiate a Connector and return None with a logging statement (warn?)

[will-moore]

Another suggestion: reset the session connector as JSON here:

if isinstance(v, Connector):
    v.to_session(request)
    return v

[imagesc-bot]

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

https://forum.image.sc/t/error-retrieving-connection/80230/2

[gaelrayot-epfl]

A less intrusive solution would be to be able to clear all the sessions during the upgrade from <5.18 to >=5.18 right?
Then simply ask the administrator to call omero web clearsessions would solve the issue I guess.

[sbesson]

@gaelrayot-epfl conceptually, fully agreed that an API to clear all sessions during an OMERO.web upgrade would be the most robust way to resolve such issue.

The initial hope was that this would happen out of the box asclearsessions is being called internally when OMERO.web starts up

omero-web/omero/plugins/web.py

Lines 528 to 531 in b3a74ac

	def start(self, args, settings):
	self.collectstatic()
	if not args.keep_sessions:
	self.clearsessions(args)

. As we found out in #435 (comment), Django's clearsessions has a much more limited scope as it only purges expired sessions using a file store backend.

[chris-allan]

In addition to what is outlined in #435 as detailed by @sbesson above, the potential problems are likely to get worse as we begin to support the 4.x series of Django which has been started in #458. Django 4.x drops support for pre-3.1 encoded sessions ¹ so if you're making a big version jump things will be really uncomfortable. Support for pickled sessions was already deprecated in 4.1 and will be removed from Django entirely when 5.0 is released.

Session storage is not something I think most OMERO administrators consider when performing an upgrade. We're going to need documentation to coach those administrators on session compatibility between major versions and make them more aware of the storage and serialization realities they are exposed to. We'll also need code to help them not get themselves in trouble or have to cook up their own methods to expire or destroy sessions across the variety of backends we encourage people to use.

See also:

• https://code.djangoproject.com/ticket/31895

Footnotes

1. https://docs.djangoproject.com/en/4.2/releases/4.0/#features-removed-in-4-0 ↩

[gaelrayot-epfl]

So, let's say my OMERO.Web is configured as followed:

omero.web.caches={"default": {"BACKEND": "django_redis.cache.RedisCache", "LOCATION": "redis://127.0.0.1:6379/0"}}
omero.web.session_engine=django.contrib.sessions.backends.cache

I could simply delete all the redis entries matching sessions. Sorry this does not help you fixing all the new session management but I've an upgrade to do tomorrow and I want it to be as smooth as possible for the users.

[chris-allan]

I could simply delete all the redis entries matching sessions.

Correct.

[chris-allan]

Before you do anything I would recommend backing up what is currently in your Redis database just in case.

Based on your configuration and if you are confident you only have Django sessions in Redis database 0 you can do this by running redis-cli -n 0 and then executing FLUSHDB. Be very careful and make sure this is what you want to do as it will delete all your keys.

If you want to see all your keys you would run redis-cli -n 0 and then KEYS *.

Alternatively, you can find the prefix for all the sessions stored in Redis (it should be something like :1:django.contrib.sessions*). You would then, assuming your configuration, run:

redis-cli -n 0 KEYS ':1:django.contrib.sessions*' | xargs redis-cli -n 0 DEL

Again, be careful with this.

[gaelrayot-epfl]

Again, be careful with this.

Yes, I've just check the keys in the DB, it's only sessions for now. Thank you.

[gaelrayot-epfl]

FYI clearing the Redis sessions does not fix the issue.

[chris-allan]

FYI clearing the Redis sessions does not fix the issue.

I'm not sure how this could be the case based on how the code functions. Could you let us know what process you followed? What OMERO.web plugins do you have installed?

[gaelrayot-epfl]

FYI clearing the Redis sessions does not fix the issue.

I'm not sure how this could be the case based on how the code functions. Could you let us know what process you followed? What OMERO.web plugins do you have installed?

On a public instance, without being logged, once I upgrade from 5.16.0 to 5.19.0, then reload, I get the same message. There wasn't any session stored in redis.

The omero plugins:

omero-figure==5.1.0
omero-fpbioimage==0.4.0
omero-iviewer==0.12.0
omero-marshal==0.7.0
omero-parade==0.2.3
omero-py==5.12.0
omero-web==5.19.0
omero-webtagging-autotag==3.2.0
omero-webtagging-tagsearch==3.2.0

[chris-allan]

Did you shut OMERO.web 5.16.0 down entirely and ensure all the processes are stopped, then run the Redis database clean, perform the upgrade to 5.19.0 and then start OMERO.web back up?

[gaelrayot-epfl]

Yes

• Shutdown the OMERO.Web server (didn't check that all process were stopped though),
• Install OMERO.Web 5.19.0
• Start Omero.Web 5.19.0
• Check for any session in redis, didn't find any
• Reload the application
• Get the error message
• Downgrade to 5.17.0

[sbesson]

@gaelrayot-epfl could you paste the output of omero config get so that we can reproduce the issue in an environment as close as possible to yours?

Update: using a test instance configured with Redis cache (redis://:@127.0.0.1:6379/1"), I have been testing this OMERO.web upgrade/downgrad workflow with the following commands:

OMERO.venv/bin/pip install "omero-web<5.18" && sudo systemctl restart omero-web
OMERO.venv/bin/pip install "omero-web>5.18" && sudo systemctl restart omero-web

In both cases, trying to re-use an existing session in the browser or refreshing it leads either the exception reported in #459 for a public session or a similar one for an authenticated user session.

Introducing an intermediate step clearing all sessions in Redis fixes both errors. In the authenticated user session case, this redirects you to the login page as the session has disappeared. In the public session case, a new session gets automatically recreated in the background

OMERO.venv/bin/pip install "omero-web<5.18" &&  redis-cli -n 1 keys '*django.contrib.sessions.cache*'  | xargs redis-cli -n 1 del && sudo systemctl restart omero-web
OMERO.venv/bin/pip install "omero-web>5.18"  &&  redis-cli -n 1 keys '*django.contrib.sessions.cache*'  | xargs redis-cli -n 1 del && sudo systemctl restart omero-web

[JensWendt]

Heyo,

I upgraded today to OMERO.server 5.6.7 and also OMERO.web 5.20.0.
Ran into the described issue.
Followed @chris-allan careful steps by checking what is in the redis DB redis-cli -n 0 keys *.
Everything started with :1:django.contrib.sessions.cache*
Then copying from @sbesson redis-cli -n 0 keys '*django.contrib.sessions.cache*' | xargs redis-cli -n 0 del and omero web restart (everything was as omero-web user).
Solved the issue. Can connect now.
Just wanted to give some feedback.

PS: Maybe worth to mention we also have a Public User

[knabar]

Closing for now; will revisit if it turns out that clearing the session store does not always resolve the issue.