(core) set cookie response header more consistently

Summary:
The express-session middleware, in its regular configuration, will
only set a cookie response header at the beginninng of a session or
when the session contents have changed. It won't set the header if
only the expiration time is changed. This diff uses a dummy `alive`
field to nudge the middleware into setting the header consistently.

Test Plan: tested manually

Reviewers: dsagal

Reviewed By: dsagal

Subscribers: alexmojaki

Differential Revision: https://phab.getgrist.com/D3153
This commit is contained in:
Paul Fitzpatrick 2021-11-24 09:50:44 -05:00
parent 0b437d1544
commit 3055a11fb2
3 changed files with 19 additions and 10 deletions

View File

@ -6,7 +6,7 @@ import {canEdit, canView, getWeakestRole, Role} from 'app/common/roles';
import {Document} from 'app/gen-server/entity/Document'; import {Document} from 'app/gen-server/entity/Document';
import {User} from 'app/gen-server/entity/User'; import {User} from 'app/gen-server/entity/User';
import {DocAuthKey, DocAuthResult, HomeDBManager} from 'app/gen-server/lib/HomeDBManager'; import {DocAuthKey, DocAuthResult, HomeDBManager} from 'app/gen-server/lib/HomeDBManager';
import {getSessionProfiles, getSessionUser, getSignInStatus, linkOrgWithEmail, SessionObj, import {forceSessionChange, getSessionProfiles, getSessionUser, getSignInStatus, linkOrgWithEmail, SessionObj,
SessionUserObj, SignInStatus} from 'app/server/lib/BrowserSession'; SessionUserObj, SignInStatus} from 'app/server/lib/BrowserSession';
import {RequestWithOrg} from 'app/server/lib/extractOrg'; import {RequestWithOrg} from 'app/server/lib/extractOrg';
import {COOKIE_MAX_AGE, getAllowedOrgForSessionID, getCookieDomain, import {COOKIE_MAX_AGE, getAllowedOrgForSessionID, getCookieDomain,
@ -188,6 +188,7 @@ export async function addRequestUser(dbManager: HomeDBManager, permitStore: IPer
// If we haven't set a maxAge yet, set it now. // If we haven't set a maxAge yet, set it now.
if (session && session.cookie && !session.cookie.maxAge) { if (session && session.cookie && !session.cookie.maxAge) {
session.cookie.maxAge = COOKIE_MAX_AGE; session.cookie.maxAge = COOKIE_MAX_AGE;
forceSessionChange(session);
} }
// See if we have a profile linked with the active organization already. // See if we have a profile linked with the active organization already.
@ -262,7 +263,7 @@ export function redirectToLoginUnconditionally(
return async (req: Request, resp: Response, next: NextFunction) => { return async (req: Request, resp: Response, next: NextFunction) => {
const mreq = req as RequestWithLogin; const mreq = req as RequestWithLogin;
// Tell express-session to set our cookie: session handling post-login relies on it. // Tell express-session to set our cookie: session handling post-login relies on it.
mreq.session.alive = true; forceSessionChange(mreq.session);
// Redirect to sign up if it doesn't look like the user has ever logged in (on // Redirect to sign up if it doesn't look like the user has ever logged in (on
// this browser) After logging in, `users` will be set in the session. Even after // this browser) After logging in, `users` will be set in the session. Even after
@ -294,8 +295,9 @@ export function redirectToLogin(
getSignUpRedirectUrl); getSignUpRedirectUrl);
return async (req: Request, resp: Response, next: NextFunction) => { return async (req: Request, resp: Response, next: NextFunction) => {
const mreq = req as RequestWithLogin; const mreq = req as RequestWithLogin;
mreq.session.alive = true; // This will ensure that express-session will set our cookie // This will ensure that express-session will set our cookie if it hasn't already -
// if it hasn't already - we'll need it if we redirect. // we'll need it if we redirect.
forceSessionChange(mreq.session);
if (mreq.userIsAuthorized) { return next(); } if (mreq.userIsAuthorized) { return next(); }
try { try {

View File

@ -45,8 +45,13 @@ export interface SessionObj {
// This is optional since the session may already exist. // This is optional since the session may already exist.
orgToUser?: {[org: string]: number}; orgToUser?: {[org: string]: number};
// This gets set to encourage express-session to set a cookie. // This gets set to encourage express-session to set a cookie. Was a boolean in the past.
alive?: boolean; alive?: number;
}
// Make an artificial change to a session to encourage express-session to set a cookie.
export function forceSessionChange(session: SessionObj) {
session.alive = Number(session.alive || 0) + 1;
} }
// We expose a sign-in status in a cookie accessible to all subdomains, to assist in auto-signin. // We expose a sign-in status in a cookie accessible to all subdomains, to assist in auto-signin.

View File

@ -21,6 +21,7 @@ import {attachAppEndpoint} from 'app/server/lib/AppEndpoint';
import {addRequestUser, getUser, getUserId, isSingleUserMode, import {addRequestUser, getUser, getUserId, isSingleUserMode,
redirectToLoginUnconditionally} from 'app/server/lib/Authorizer'; redirectToLoginUnconditionally} from 'app/server/lib/Authorizer';
import {redirectToLogin, RequestWithLogin, signInStatusMiddleware} from 'app/server/lib/Authorizer'; import {redirectToLogin, RequestWithLogin, signInStatusMiddleware} from 'app/server/lib/Authorizer';
import {forceSessionChange} from 'app/server/lib/BrowserSession';
import * as Comm from 'app/server/lib/Comm'; import * as Comm from 'app/server/lib/Comm';
import {create} from 'app/server/lib/create'; import {create} from 'app/server/lib/create';
import {addDiscourseConnectEndpoints} from 'app/server/lib/DiscourseConnect'; import {addDiscourseConnectEndpoints} from 'app/server/lib/DiscourseConnect';
@ -593,7 +594,8 @@ export class FlexServer implements GristServer {
// Create an endpoint for making cookies during testing. // Create an endpoint for making cookies during testing.
this.app.get('/test/session', async (req, res) => { this.app.get('/test/session', async (req, res) => {
(req as any).session.alive = true; const mreq = req as RequestWithLogin;
forceSessionChange(mreq.session);
res.status(200).send(`Grist ${this.name} is alive and is interested in you.`); res.status(200).send(`Grist ${this.name} is alive and is interested in you.`);
}); });
@ -800,9 +802,9 @@ export class FlexServer implements GristServer {
this: FlexServer, signUp: boolean|null, req: express.Request, resp: express.Response, this: FlexServer, signUp: boolean|null, req: express.Request, resp: express.Response,
) { ) {
const mreq = req as RequestWithLogin; const mreq = req as RequestWithLogin;
mreq.session.alive = true; // This will ensure that express-session will set our cookie // This will ensure that express-session will set our cookie if it hasn't already -
// if it hasn't already - we'll need it when we come back // we'll need it when we come back from Cognito.
// from Cognito. forceSessionChange(mreq.session);
// Redirect to "/" on our requested hostname (in test env, this will redirect further) // Redirect to "/" on our requested hostname (in test env, this will redirect further)
const next = req.protocol + '://' + req.get('host') + '/'; const next = req.protocol + '://' + req.get('host') + '/';
if (signUp === null) { if (signUp === null) {