Add hard expiration to Flask session #5907

cbartz · 2023-08-25T10:48:02Z

This PR adds the ability to set a hard expiration for the Flask session. This can be used in conjunction with a Flask Multipass identity provider that specifies the session expiration. See this post for more details on use cases and implementation considerations.

indico/web/flask/session.py

indico/web/flask/session_test.py

CHANGES.rst

indico/modules/auth/auth_test.py

indico/web/flask/session.py

ThiefMaster · 2023-08-30T11:55:37Z

Just wondering... should there some kind of indicator that the session will expire soon when someone uses this feature? It'd be a bit disruptive if someone is e.g. writing an abstract or minutes for an event and does not realize that the session will expire in a few minutes!

cbartz · 2023-08-30T15:10:47Z

Just wondering... should there some kind of indicator that the session will expire soon when someone uses this feature? It'd be a bit disruptive if someone is e.g. writing an abstract or minutes for an event and does not realize that the session will expire in a few minutes!

This may make sense. Would a flash inside session.py be the right approach or is there a better way?

ThiefMaster · 2023-08-31T09:50:19Z

We do use flash messages in a few places, but IMHO it would be nicer to actually expose the session expiry to the client, and show a warning dialog client-side that the session will soon expire (with the option to refresh it (by going through the login again)) when getting close to the expiry.

SegiNyn · 2023-10-02T15:19:14Z

but IMHO it would be nicer to actually expose the session expiry to the client, and show a warning dialog client-side that the session will soon expire (with the option to refresh it (by going through the login again)) when getting close to the expiry.

Hi @ThiefMaster, we were also proposing an expiration dialog of this nature to warn users when the session is about to expire and give them the opportunity to refresh:

is this something we can coordinate with this pr?

cc @OmeGak

ThiefMaster · 2023-10-10T13:56:34Z

Maybe it's best for @SegiNyn to send a PR to @cbartz's branch used for this PR? Alterantively, @cbartz could give her access to his fork of Indico in order to push directly on top of his changes.

At least i'd prefer those options over first merging it and then having a separate PR to improve it...

SegiNyn · 2023-10-10T14:09:14Z

Sending a pr to @cbartz's branch sounds good to me.

SegiNyn · 2023-10-19T06:48:56Z

indico/modules/auth/__init__.py

@@ -30,6 +32,12 @@
 @multipass.identity_handler
 def process_identity(identity_info):
    logger.info('Received identity info: %s', identity_info)
+
+    if session_expiry := identity_info.multipass_data.get('session_expiry'):


Hi @cbartz, please does this work when logging in with a local account? I'm getting a AttributeError: 'NoneType' object has no attribute 'get' when logging in with a local account.

the multipass_data is None for local accounts. so this is currently a bug in the PR

Okay, thanks

SegiNyn · 2023-10-20T09:34:24Z

Hi @ThiefMaster, I have sent a PR to @cbartz's branch here: cbartz#1 There are still some problems with it like some resources being fetched when opening the dialog which refreshes the session. You won't see this if you run indico with the -q flag because those requests will be silenced in the logs but they still reach the app and refresh the session.

ThiefMaster · 2023-10-20T09:38:29Z

Try this to prevent those requests from refreshing the session:

diff --git a/indico/web/flask/session.py b/indico/web/flask/session.py
index 8637f295c4..1d7fc785dd 100644
--- a/indico/web/flask/session.py
+++ b/indico/web/flask/session.py
@@ -162,6 +162,9 @@ class IndicoSessionInterface(SessionInterface):
             return self.temporary_session_lifetime

     def should_refresh_session(self, app, session):
+        if (request.blueprint == 'assets' or
+                (request.blueprint.startswith('plugin_') and request.endpoint.endswith('.static'))):
+            return False
         if session.new or '_expires' not in session:
             return False
         threshold = self.get_storage_lifetime(app, session) / 2

Or replace that whole if condition with re.match(r'assets\.|plugin_.*\.static$', request.endpoint) (you may want to re.compile() that regex since it's used on every single request)

ThiefMaster · 2023-10-25T15:22:35Z

indico/web/templates/indico_new_base.html

@@ -25,4 +26,7 @@
        {{ render_announcements() }}
    {% endblock %}
    {% block body %}{% endblock %}
+    {% block session_expiration %}


@SegiNyn do you override that block on your side? or is it a leftover that's no longer needed?

Yes, sorry this is a leftover that's not being used

ThiefMaster · 2023-10-26T15:08:19Z

I forcepushed some cleanups and also getting rid of those merge commits while rebasing

ThiefMaster · 2023-10-26T15:33:41Z

@SegiNyn some feedback after testing this:

I think the RequestConfirm component is not a good choice here. It's meant for confirmation of possibly destructive actions. For example, it has no option to only show one button (OK and Cancel doing the same is bad UX) nor to not close when clicking outside the dialog.
Sending the user back to the login page only makes sense if there's a hard expiry date set for the session (and thus no possibility of refreshing it). Otherwise a request to an endpoint that simply refreshes its lifetime (as is the case during regular Indico usage) should be made which is much less intrusive (imagine you're writing minutes or an abstract and get this message - you do not expect a redirect, and in most cases it would not be necessary).

ThiefMaster · 2023-10-26T15:38:41Z

Also, considering that the dialog still needs a bit of work and is not as straightforward as I originally thought, I'm thinking about merging this PR without the dialog stuff tomorrow (3.3 is under development so having no indicator of a soon-expiring session for the time being is fine). That way we avoid the indirection of PRs to a fork, and reviewing the dialog stuff properly will be easier...

SegiNyn · 2023-10-26T15:47:43Z

hi @ThiefMaster, Yes I did notice the issue with the buttons which basically do the same thing but I had not thought of how disruptive it would be to refresh the page while the user is filling in an abstract. So I can add a custom dialog for this part. It's ok for me if you merge this pr with the hard expiry and then I create a different one for the dialog. I'm not going to be in tomorrow so I cannot work on it until Monday anyway.

Make sure the expiry is set as the function may return before the end, where the logic was placed before.

Co-authored-by: Adrian <adrian@planetcoding.net>

ThiefMaster · 2023-10-27T10:46:48Z

@SegiNyn I pushed a branch containing your changes, rebased on top of the latest master, to session-expiry-with-dialog in my indico fork. It contains some fixes I did on top of your code so it might be good for you to reset your local branch to that one.

cbartz · 2023-10-27T10:51:42Z

Thanks for merging the PR @ThiefMaster

ThiefMaster reviewed Aug 25, 2023

View reviewed changes

cbartz marked this pull request as ready for review August 28, 2023 07:57

ThiefMaster reviewed Aug 30, 2023

View reviewed changes

indico/modules/auth/auth_test.py Outdated Show resolved Hide resolved

indico/modules/auth/auth_test.py Outdated Show resolved Hide resolved

indico/web/flask/session.py Outdated Show resolved Hide resolved

cbartz force-pushed the multipass-session-expiry branch from 74a845a to 30f8cec Compare August 30, 2023 11:51

SegiNyn reviewed Oct 19, 2023

View reviewed changes

ThiefMaster reviewed Oct 25, 2023

View reviewed changes

cbartz added 2 commits October 26, 2023 16:40

Add hard expiry to flask session

0db8413

Use session_expiry from multipass_data

8b319d7

ThiefMaster force-pushed the multipass-session-expiry branch from d8018b1 to 8ee74d9 Compare October 26, 2023 15:08

ThiefMaster force-pushed the multipass-session-expiry branch from d213be1 to 8e9778d Compare October 27, 2023 09:39

cbartz and others added 8 commits October 27, 2023 12:06

Add hard session expiry to CHANGES

484da28

Fix missing/wrong headers

74cc04c

Remove hard_expiry check in should_refresh_session

ee4223d

Improve session_test

8873609

Use min of lifetimes in get_storage_lifetime

4be3812

Set sess expiry at the start of process_identity

cf4ee41

Make sure the expiry is set as the function may return before the end, where the logic was placed before.

Add test for process_identity

3a16d3a

Use db instead of database fixture

a0ecf3c

Co-authored-by: Adrian <adrian@planetcoding.net>

cbartz and others added 2 commits October 27, 2023 12:06

Re-add session sid refresh

ce58555

Fix error when there's no multipass data

6d27d38

ThiefMaster force-pushed the multipass-session-expiry branch from df4cdde to 6d27d38 Compare October 27, 2023 10:07

ThiefMaster approved these changes Oct 27, 2023

View reviewed changes

ThiefMaster merged commit 3689c09 into indico:master Oct 27, 2023
12 checks passed

cbartz deleted the multipass-session-expiry branch October 27, 2023 13:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add hard expiration to Flask session #5907

Add hard expiration to Flask session #5907

cbartz commented Aug 25, 2023 •

edited

ThiefMaster commented Aug 30, 2023

cbartz commented Aug 30, 2023

ThiefMaster commented Aug 31, 2023

SegiNyn commented Oct 2, 2023

ThiefMaster commented Oct 10, 2023

SegiNyn commented Oct 10, 2023

SegiNyn Oct 19, 2023

ThiefMaster Oct 19, 2023

SegiNyn Oct 19, 2023

SegiNyn commented Oct 20, 2023

ThiefMaster commented Oct 20, 2023 •

edited

ThiefMaster Oct 25, 2023

SegiNyn Oct 25, 2023

ThiefMaster commented Oct 26, 2023

ThiefMaster commented Oct 26, 2023

ThiefMaster commented Oct 26, 2023

SegiNyn commented Oct 26, 2023

ThiefMaster commented Oct 27, 2023

cbartz commented Oct 27, 2023

Add hard expiration to Flask session #5907

Add hard expiration to Flask session #5907

Conversation

cbartz commented Aug 25, 2023 • edited

ThiefMaster commented Aug 30, 2023

cbartz commented Aug 30, 2023

ThiefMaster commented Aug 31, 2023

SegiNyn commented Oct 2, 2023

ThiefMaster commented Oct 10, 2023

SegiNyn commented Oct 10, 2023

SegiNyn Oct 19, 2023

Choose a reason for hiding this comment

ThiefMaster Oct 19, 2023

Choose a reason for hiding this comment

SegiNyn Oct 19, 2023

Choose a reason for hiding this comment

SegiNyn commented Oct 20, 2023

ThiefMaster commented Oct 20, 2023 • edited

ThiefMaster Oct 25, 2023

Choose a reason for hiding this comment

SegiNyn Oct 25, 2023

Choose a reason for hiding this comment

ThiefMaster commented Oct 26, 2023

ThiefMaster commented Oct 26, 2023

ThiefMaster commented Oct 26, 2023

SegiNyn commented Oct 26, 2023

ThiefMaster commented Oct 27, 2023

cbartz commented Oct 27, 2023

cbartz commented Aug 25, 2023 •

edited

ThiefMaster commented Oct 20, 2023 •

edited