From 692749d1cfa7ad7b028213ce830e88611a429345 Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Thu, 23 Dec 2021 01:45:38 -0500 Subject: [PATCH] express-session: Extend session lifetime if user is active --- CHANGELOG.md | 5 ++-- settings.json.template | 36 +++++++++++++++++++++----- src/node/handler/PadMessageHandler.js | 1 + src/node/hooks/express.js | 4 ++- src/node/hooks/express/socketio.js | 13 ++++++++++ src/node/hooks/express/specialpages.js | 7 +++++ src/node/utils/Settings.js | 1 + src/static/js/pad.js | 5 ++++ src/static/js/timeslider.js | 6 +++++ 9 files changed, 68 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5f78af33d..9ce591326 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,7 +6,8 @@ * `express_sid` cookies and `sessionstorage:*` database records are no longer created unless `requireAuthentication` is `true` (or a plugin causes them to be created). - * Login sessions now have a finite lifetime by default (10 days). + * Login sessions now have a finite lifetime by default (10 days after + leaving). * `sessionstorage:*` database records are automatically deleted when the login session expires (with some exceptions that will be fixed in the future). * Requests for static content (e.g., `/robots.txt`) and special pages (e.g., @@ -47,7 +48,7 @@ ### Compatibility changes * The default login session expiration (applicable if `requireAuthentication` is - `true`) changed from never to 10 days. + `true`) changed from never to 10 days after the user leaves. #### For plugin authors diff --git a/settings.json.template b/settings.json.template index 54372c59d..c9825264a 100644 --- a/settings.json.template +++ b/settings.json.template @@ -378,24 +378,46 @@ "sameSite": "Lax", /* - * How long (in milliseconds) a session lasts before the user is required to - * log in again. (The express_sid cookie is set to expire at time now + - * sessionLifetime when first created.) If requireAuthentication is false - * then this value does not really matter. + * How long (in milliseconds) after navigating away from Etherpad before the + * user is required to log in again. (The express_sid cookie is set to + * expire at time now + sessionLifetime when first created, and its + * expiration time is periodically refreshed to a new now + sessionLifetime + * value.) If requireAuthentication is false then this value does not really + * matter. * * The "best" value depends on your users' usage patterns and the amount of * convenience you desire. A long lifetime is more convenient (users won't * have to log back in as often) but has some drawbacks: * - It increases the amount of state kept in the database. - * - It might weaken security somewhat: Once a user has accessed a pad, - * the user can continue to use the pad until the session expires. + * - It might weaken security somewhat: The cookie expiration is refreshed + * indefinitely without consulting authentication or authorization + * hooks, so once a user has accessed a pad, the user can continue to + * use the pad until the user leaves for longer than sessionLifetime. * * Session lifetime can be set to infinity (not recommended) by setting this * to null or 0. Note that if the session does not expire, most browsers * will delete the cookie when the browser exits, but a session record is * kept in the database forever. */ - "sessionLifetime": 864000000 // = 10d * 24h/d * 60m/h * 60s/m * 1000ms/s + "sessionLifetime": 864000000, // = 10d * 24h/d * 60m/h * 60s/m * 1000ms/s + + /* + * How long (in milliseconds) before the expiration time of an active user's + * session is refreshed (to now + sessionLifetime). This setting affects the + * following: + * - How often a new session expiration time will be written to the + * database. + * - How often each user's browser will ping the Etherpad server to + * refresh the expiration time of the session cookie. + * + * High values reduce the load on the database and the load from browsers, + * but can shorten the effective session lifetime if Etherpad is restarted + * or the user navigates away. + * + * Automatic session refreshes can be disabled (not recommended) by setting + * this to null. + */ + "sessionRefreshInterval": 86400000 // = 1d * 24h/d * 60m/h * 60s/m * 1000ms/s }, /* diff --git a/src/node/handler/PadMessageHandler.js b/src/node/handler/PadMessageHandler.js index a358807e9..2ab904d46 100644 --- a/src/node/handler/PadMessageHandler.js +++ b/src/node/handler/PadMessageHandler.js @@ -998,6 +998,7 @@ const handleClientReady = async (socket, message) => { readOnlyId: sessionInfo.readOnlyPadId, readonly: sessionInfo.readonly, serverTimestamp: Date.now(), + sessionRefreshInterval: settings.cookie.sessionRefreshInterval, userId: sessionInfo.author, abiwordAvailable: settings.abiwordAvailable(), sofficeAvailable: settings.sofficeAvailable(), diff --git a/src/node/hooks/express.js b/src/node/hooks/express.js index 3e231bdde..9c42fd6d8 100644 --- a/src/node/hooks/express.js +++ b/src/node/hooks/express.js @@ -176,8 +176,10 @@ exports.restartServer = async () => { app.use(cookieParser(settings.sessionKey, {})); - sessionStore = new SessionStore(); + sessionStore = new SessionStore(settings.cookie.sessionRefreshInterval); exports.sessionMiddleware = expressSession({ + propagateTouch: true, + rolling: true, secret: settings.sessionKey, store: sessionStore, resave: false, diff --git a/src/node/hooks/express/socketio.js b/src/node/hooks/express/socketio.js index 47a657747..edb679940 100644 --- a/src/node/hooks/express/socketio.js +++ b/src/node/hooks/express/socketio.js @@ -105,6 +105,19 @@ exports.expressCreateServer = (hookName, args, cb) => { express.sessionMiddleware(req, {}, next); }); + io.use((socket, next) => { + socket.conn.on('packet', (packet) => { + // Tell express-session that the session is still active. The session store can use these + // touch events to defer automatic session cleanup, and if express-session is configured with + // rolling=true the cookie's expiration time will be renewed. (Note that WebSockets does not + // have a standard mechanism for periodically updating the browser's cookies, so the browser + // will not see the new cookie expiration time unless it makes a new HTTP request or the new + // cookie value is sent to the client in a custom socket.io message.) + if (socket.request.session != null) socket.request.session.touch(); + }); + next(); + }); + // var socketIOLogger = log4js.getLogger("socket.io"); // Debug logging now has to be set at an environment level, this is stupid. // https://github.com/Automattic/socket.io/wiki/Migrating-to-1.0 diff --git a/src/node/hooks/express/specialpages.js b/src/node/hooks/express/specialpages.js index bf23487c2..e0a2e681b 100644 --- a/src/node/hooks/express/specialpages.js +++ b/src/node/hooks/express/specialpages.js @@ -106,5 +106,12 @@ exports.expressCreateServer = (hookName, args, cb) => { })); }); + // The client occasionally polls this endpoint to get an updated expiration for the express_sid + // cookie. This handler must be installed after the express-session middleware. + args.app.put('/_extendExpressSessionLifetime', (req, res) => { + // express-session automatically calls req.session.touch() so we don't need to do it here. + res.json({status: 'ok'}); + }); + return cb(); }; diff --git a/src/node/utils/Settings.js b/src/node/utils/Settings.js index cd4e5c8fe..51f48237a 100644 --- a/src/node/utils/Settings.js +++ b/src/node/utils/Settings.js @@ -323,6 +323,7 @@ exports.cookie = { */ sameSite: 'Lax', sessionLifetime: 10 * 24 * 60 * 60 * 1000, + sessionRefreshInterval: 1 * 24 * 60 * 60 * 1000, }; /* diff --git a/src/static/js/pad.js b/src/static/js/pad.js index 1696acc56..e831454de 100644 --- a/src/static/js/pad.js +++ b/src/static/js/pad.js @@ -293,6 +293,11 @@ const handshake = async () => { } else if (!receivedClientVars && obj.type === 'CLIENT_VARS') { receivedClientVars = true; window.clientVars = obj.data; + if (window.clientVars.sessionRefreshInterval) { + const ping = + () => $.ajax('../_extendExpressSessionLifetime', {method: 'PUT'}).catch(() => {}); + setInterval(ping, window.clientVars.sessionRefreshInterval); + } } else if (obj.disconnect) { padconnectionstatus.disconnected(obj.disconnect); socket.disconnect(); diff --git a/src/static/js/timeslider.js b/src/static/js/timeslider.js index 7268f95f0..246872061 100644 --- a/src/static/js/timeslider.js +++ b/src/static/js/timeslider.js @@ -111,6 +111,12 @@ const handleClientVars = (message) => { // save the client Vars window.clientVars = message.data; + if (window.clientVars.sessionRefreshInterval) { + const ping = + () => $.ajax('../../_extendExpressSessionLifetime', {method: 'PUT'}).catch(() => {}); + setInterval(ping, window.clientVars.sessionRefreshInterval); + } + // load all script that doesn't work without the clientVars BroadcastSlider = require('./broadcast_slider') .loadBroadcastSliderJS(fireWhenAllScriptsAreLoaded);