OpenImaging / miqa-phase1

A web application for medical imaging quality assurance
MIT License
20 stars 8 forks source link

Session timeout and email fixes #91

Closed scottwittenburg closed 3 years ago

scottwittenburg commented 3 years ago

Fixes for email and session issues:

scottwittenburg commented 3 years ago

This is intended to work with this girder change, though nothing prevents it from working now, just with an integer number of days for session to live (rather than float).

jeffbaumes commented 3 years ago

Fixes #90

jeffbaumes commented 3 years ago

This is looking good to me thus far. I see it is still draft, but when it is ready, could you give me a live walkthrough of the functionality/UI before merge?

scottwittenburg commented 3 years ago

This is looking good to me thus far. I see it is still draft, but when it is ready, could you give me a live walkthrough of the functionality/UI before merge?

Yes, that would be great. I'm currently working to get the web_server docker image to build with the latest release of girder, which required changing the base image because it seems to no longer work with python3.5 (moving to python 3.7 makes sense in general, and we had an issue for it).

Also, the PR says it also fixes email things, but so far I haven't got to those fixes yet. I might opt for changing the PR name/description and fixing the email issues in a subsequent PR, but let me know what you think.

This also:

Fixes #66 Fixes #79 Fixes #57 ... and hopefully Fixes #40

scottwittenburg commented 3 years ago

One question I have is what to do with the client-side session timeout functionality I added last week. If we keep it, it probably needs not to behave in a way that conflicts with or implies different results than the token-based timeout. By that, I mean specifically:

Otherwise, maybe we should get rid of the client-side timeout? But then you could avoid making http requests and get a longer session than is configured. To correct something I said last week, that would mean you could go back and forth through the timesteps of a single scan as long as you want, not look through all the scans in an experiment as long as you want. The reason is that changing scans makes a server request to fetch the metadata for the scan, and so would result in logout if you're past the session lifetime.

scottwittenburg commented 3 years ago

☝️ @jeffbaumes @aashish24 @dzenanz @curtislisle ☝️

dzenanz commented 3 years ago

I favor simplicity, and removing client-side timeout is the simplest?

scottwittenburg commented 3 years ago

I favor simplicity, and removing client-side timeout is the simplest?

Yes, I think that's the simplest, but leaves the small gap that could allow users to stay logged in longer than the session lifetime configured in girder settings.

Maybe @jimklo has thoughts/input.

aashish24 commented 3 years ago

I would probably remove the client-side timeout just to keep it simple for now. they can be logged in for longer but can only use the data from local cache which might be okay for now (based on our discussions).

jeffbaumes commented 3 years ago

I'll add a "me too" to simplifying things by just making sure you are logged out server side.

scottwittenburg commented 3 years ago

I added some email fixes here (see commit message on 7d37d0e), but I could easily remove that and add it to a subsequent PR.

Still TODO: remove the previous session management approach, and make sure the COOKIE_LIFETIME setting is derived from the previously defined MIQA_SESSION_TIMEOUT environment variable when building the web_server docker image.