(core) clean up interaction of forward auth with session

Summary:
For self-hosted Grist, forward auth has proven useful, where
some proxy wrapped around Grist manages authentication, and
passes on user information to Grist in a trusted header.
The current implementation is adequate when Grist is the
only place where the user logs in or out, but is confusing
otherwise (see https://github.com/gristlabs/grist-core/issues/207).
Here we take some steps to broaden the scenarios Grist's
forward auth support can be used with:

  * When a trusted header is present and is blank, treat
    that as the user not being logged in, and don't look
    any further for identity information. Specifically,
    don't look in Grist's session information.
  * Add a `GRIST_IGNORE_SESSION` flag to entirely prevent
    Grist from picking up identity information from a cookie,
    in order to avoid confusion between multiple login methods.
  * Add tests for common scenarios.

Test Plan: added tests

Reviewers: georgegevoian

Reviewed By: georgegevoian

Differential Revision: https://phab.getgrist.com/D3482

README.md

@@ -206,13 +206,14 @@ GRIST_HIDE_UI_ELEMENTS | comma-separated list of parts of the UI to hide. Allowe
 GRIST_HOME_INCLUDE_STATIC | if set, home server also serves static resources
 GRIST_HOST          | hostname to use when listening on a port.
 GRIST_ID_PREFIX | for subdomains of form o-*, expect or produce o-${GRIST_ID_PREFIX}*.
+GRIST_IGNORE_SESSION | if set, Grist will not use a session for authentication.
 GRIST_INST_DIR      | path to Grist instance configuration files, for Grist server.
 GRIST_MANAGED_WORKERS | if set, Grist can assume that if a url targeted at a doc worker returns a 404, that worker is gone
 GRIST_MAX_UPLOAD_ATTACHMENT_MB | max allowed size for attachments (0 or empty for unlimited).
 GRIST_MAX_UPLOAD_IMPORT_MB | max allowed size for imports (except .grist files) (0 or empty for unlimited).
 GRIST_ORG_IN_PATH | if true, encode org in path rather than domain
 GRIST_PAGE_TITLE_SUFFIX | a string to append to the end of the `<title>` in HTML documents. Defaults to `" - Grist"`. Set to `_blank` for no suffix at all.
-GRIST_PROXY_AUTH_HEADER | header which will be set by a (reverse) proxy webserver with an authorized users' email. This can be used as an alternative to a SAML service.
+GRIST_PROXY_AUTH_HEADER | header which will be set by a (reverse) proxy webserver with an authorized users' email. This can be used as an alternative to a SAML service. See also GRIST_FORWARD_AUTH_HEADER.
 GRIST_ROUTER_URL | optional url for an api that allows servers to be (un)registered with a load balancer
 GRIST_SERVE_SAME_ORIGIN | set to "true" to access home server and doc workers on the same protocol-host-port as the top-level page, same as for custom domains (careful, host header should be trustworthy)
 GRIST_SESSION_COOKIE | if set, overrides the name of Grist's cookie
@@ -237,6 +238,25 @@ GRIST_SANDBOX | a program or image name to run as the sandbox. See NSandbox.ts f
 PYTHON_VERSION | can be 2 or 3. If set, documents without an engine setting are assumed to use the specified version of python. Not all sandboxes support all versions.
 PYTHON_VERSION_ON_CREATION | can be 2 or 3. If set, newly created documents have an engine setting set to python2 or python3. Not all sandboxes support all versions.
 
+Forward authentication variables:
+
+Variable | Purpose
+-------- | -------
+GRIST_FORWARD_AUTH_HEADER | if set, trust the specified header (e.g. "x-forwarded-user") to contain authorized user emails, and enable "forward auth" logins.
+GRIST_FORWARD_AUTH_LOGIN_PATH | if GRIST_FORWARD_AUTH_HEADER is set, Grist will listen at this path for logins. Defaults to `/auth/login`.
+GRIST_FORWARD_AUTH_LOGOUT_PATH | if GRIST_FORWARD_AUTH_HEADER is set, Grist will forward to this path when user logs out.
+
+When using forward authentication, you may wish to also set the following variables:
+
+  * GRIST_FORCE_LOGIN=true to disable anonymous access.
+  * GRIST_IGNORE_SESSION=true to ignore any user identity information in a cookie.
+    Only do this if you use forward authentication on all paths.
+    You may not want to use forward authentication on all paths if it makes
+    signing in required, and you are trying to permit anonymous access.
+
+GRIST_FORWARD_AUTH_HEADER is similar to GRIST_PROXY_AUTH_HEADER, but enables
+a login system (assuming you have some forward authentication set up).
+
 Google Drive integrations:
 
 Variable | Purpose

app/common/gutil.ts

@@ -150,6 +150,17 @@ export function undef<T extends Array<any>>(...list: T): Undef<T> {
   return undefined as any;
 }
 
+/**
+ * Like undef, but each element of list is a method that is only called
+ * if needed, and promises are supported. No fancy type inference though, sorry.
+ */
+export async function firstDefined<T>(...list: Array<() => Promise<T>>): Promise<T | undefined> {
+  for(const op of list) {
+    const value = await op();
+    if (value !== undefined) { return value; }
+  }
+  return undefined;
+}
 
 /**
  * Parses json and returns the result, or returns defaultVal if parsing fails.

app/server/lib/AppSettings.ts

@@ -80,6 +80,17 @@ export class AppSettings {
     return result;
   }
 
+  /**
+   * As for read() but type (and store, and report) the result as
+   * a boolean.
+   */
+  public readBool(query: AppSettingQuery): boolean|undefined {
+    this.readString(query);
+    const result = this.getAsBool();
+    this._value = result;
+    return result;
+  }
+
   /* set this setting 'manually' */
   public set(value: JSONValue): void {
     this._value = value;

app/server/lib/Authorizer.ts

@@ -98,11 +98,14 @@ export function isSingleUserMode(): boolean {
  * Returns a profile if it can be deduced from the request. This requires a
  * header to specify the users' email address. The header to set comes from the
  * environment variable GRIST_PROXY_AUTH_HEADER, or may be passed in.
+ * A result of null means that the user should be considered known to be anonymous.
+ * A result of undefined means we should go on to consider other authentication
+ * methods (such as cookies).
  */
 export function getRequestProfile(req: Request|IncomingMessage,
-                                  header?: string): UserProfile|undefined {
+                                  header?: string): UserProfile|null|undefined {
   header = header || process.env.GRIST_PROXY_AUTH_HEADER;
-  let profile: UserProfile|undefined;
+  let profile: UserProfile|null|undefined;
 
   if (header) {
     // Careful reading headers. If we have an IncomingMessage, there is no
@@ -118,8 +121,13 @@ export function getRequestProfile(req: Request|IncomingMessage,
         };
       }
     }
+    // If no profile at this point, and header was present,
+    // treat as anonymous user, represented by null value.
+    // Don't go on to look at session.
+    if (!profile && headerContent !== undefined) {
+      profile = null;
+    }
   }
-
   return profile;
 }
 
@@ -134,6 +142,10 @@ export function getRequestProfile(req: Request|IncomingMessage,
  *   - req.users: set for org-and-session-based logins, with list of profiles in session
  */
 export async function addRequestUser(dbManager: HomeDBManager, permitStore: IPermitStore,
+                                     options: {
+                                       skipSession?: boolean,
+                                       getProfile?(req: Request|IncomingMessage): Promise<UserProfile|null|undefined>,
+                                     },
                                      req: Request, res: Response, next: NextFunction) {
   const mreq = req as RequestWithLogin;
   let profile: UserProfile|undefined;
@@ -184,10 +196,34 @@ export async function addRequestUser(dbManager: HomeDBManager, permitStore: IPer
     return res.status(401).send('Bad request (missing header)');
   }
 
+  // For some configurations, the user profile can be determined from the request.
+  // If this is the case, we won't use session information.
+  let skipSession: boolean = options.skipSession || false;
+  if (!mreq.userId) {
+    let candidate = await options.getProfile?.(mreq);
+    if (candidate === undefined) {
+      candidate = getRequestProfile(mreq);
+    }
+    if (candidate !== undefined) {
+      skipSession = true;
+    }
+    if (candidate) {
+      profile = candidate;
+      const user = await dbManager.getUserByLoginWithRetry(profile.email, {profile});
+      if (user) {
+        mreq.user = user;
+        mreq.users = [profile];
+        mreq.userId = user.id;
+        mreq.userIsAuthorized = true;
+      }
+    }
+  }
+
   // A bit of extra info we'll add to the "Auth" log message when this request passes the check
   // for custom-host-specific sessionID.
   let customHostSession = '';
 
+  if (!skipSession) {
     // If we haven't selected a user by other means, and have profiles available in the
     // session, then select a user based on those profiles.
     const session = mreq.session;
@@ -293,18 +329,6 @@ export async function addRequestUser(dbManager: HomeDBManager, permitStore: IPer
         }
       }
     }
-
-  if (!mreq.userId) {
-    profile = getRequestProfile(mreq);
-    if (profile) {
-      const user = await dbManager.getUserByLoginWithRetry(profile.email, {profile});
-      if(user) {
-        mreq.user = user;
-        mreq.users = [profile];
-        mreq.userId = user.id;
-        mreq.userIsAuthorized = true;
-      }
-    }
   }
 
   // If no userId has been found yet, fall back on anonymous.

app/server/lib/Comm.ts

@@ -23,13 +23,14 @@ import * as WebSocket from 'ws';
 
 import {CommDocEventType, CommMessage} from 'app/common/CommTypes';
 import {parseFirstUrlPart} from 'app/common/gristUrls';
-import {safeJsonParse} from 'app/common/gutil';
+import {firstDefined, safeJsonParse} from 'app/common/gutil';
 import {UserProfile} from 'app/common/LoginSessionAPI';
 import * as version from 'app/common/version';
 import {getRequestProfile} from 'app/server/lib/Authorizer';
 import {ScopedSession} from "app/server/lib/BrowserSession";
 import {Client, ClientMethod} from "app/server/lib/Client";
 import {Hosts, RequestWithOrg} from 'app/server/lib/extractOrg';
+import {GristLoginMiddleware} from 'app/server/lib/GristServer';
 import * as log from 'app/server/lib/log';
 import {localeFromRequest} from 'app/server/lib/ServerLocale';
 import {fromCallback} from 'app/server/lib/serverUtils';
@@ -39,6 +40,7 @@ export interface CommOptions {
   sessions: Sessions;                   // A collection of all sessions for this instance of Grist
   settings?: {[key: string]: unknown};  // The config object containing instance settings including features.
   hosts?: Hosts;  // If set, we use hosts.getOrgInfo(req) to extract an organization from a (possibly versioned) url.
+  loginMiddleware?: GristLoginMiddleware; // If set, use custom getProfile method if available
   httpsServer?: https.Server;   // An optional HTTPS server to listen on too.
 }
 
@@ -55,18 +57,9 @@ export interface CommOptions {
  */
 export class Comm extends EventEmitter {
   // Collection of all sessions; maps sessionIds to ScopedSession objects.
-  public readonly sessions: Sessions;
+  public readonly sessions: Sessions = this._options.sessions;
   private _wss: WebSocket.Server[]|null = null;
 
-  // The config object containing instance settings including features.
-  private _settings?: {[key: string]: unknown};
-
-  // If set, we use hosts.getOrgInfo(req) to extract an organization from a (possibly versioned) url.
-  private _hosts?: Hosts;
-
-  // An optional HTTPS server to listen on too.
-  private _httpsServer?: https.Server;
-
   private _clients = new Map<string, Client>();   // Maps clientIds to Client objects.
 
   private _methods = new Map<string, ClientMethod>();  // Maps method names to their implementation.
@@ -77,14 +70,9 @@ export class Comm extends EventEmitter {
   // for a valid server.
   private _serverVersion: string|null = null;
 
-  constructor(private _server: http.Server, options: CommOptions) {
+  constructor(private _server: http.Server, private _options: CommOptions) {
     super();
-    this._httpsServer = options.httpsServer;
     this._wss = this._startServer();
-
-    this.sessions = options.sessions;
-    this._settings = options.settings;
-    this._hosts = options.hosts;
   }
 
   /**
@@ -186,7 +174,11 @@ export class Comm extends EventEmitter {
    * Returns a profile based on the request or session.
    */
   private async _getSessionProfile(scopedSession: ScopedSession, req: http.IncomingMessage): Promise<UserProfile|null> {
-    return getRequestProfile(req) || scopedSession.getSessionProfile();
+    return await firstDefined(
+      async () => this._options.loginMiddleware?.getProfile?.(req),
+      async () => getRequestProfile(req),
+      async () => scopedSession.getSessionProfile(),
+    ) || null;
   }
 
   /**
@@ -194,12 +186,12 @@ export class Comm extends EventEmitter {
    */
   private async _onWebSocketConnection(websocket: WebSocket, req: http.IncomingMessage) {
     log.info("Comm: Got WebSocket connection: %s", req.url);
-    if (this._hosts) {
+    if (this._options.hosts) {
       // DocWorker ID (/dw/) and version tag (/v/) may be present in this request but are not
       // needed. addOrgInfo assumes req.url starts with /o/ if present.
       req.url = parseFirstUrlPart('dw', req.url!).path;
       req.url = parseFirstUrlPart('v', req.url).path;
-      await this._hosts.addOrgInfo(req);
+      await this._options.hosts.addOrgInfo(req);
     }
 
     // Parse the cookie in the request to get the sessionId.
@@ -232,7 +224,7 @@ export class Comm extends EventEmitter {
 
     client.sendConnectMessage({
       serverVersion: this._serverVersion || version.gitcommit,
-      settings: this._settings,
+      settings: this._options.settings,
     })
     .catch(err => {
       log.error(`Comm ${client}: failed to prepare or send clientConnect:`, err);
@@ -241,7 +233,7 @@ export class Comm extends EventEmitter {
 
   private _startServer() {
     const servers = [this._server];
-    if (this._httpsServer) { servers.push(this._httpsServer); }
+    if (this._options.httpsServer) { servers.push(this._options.httpsServer); }
     const wss = [];
     for (const server of servers) {
       const wssi = new WebSocket.Server({server});

app/server/lib/FlexServer.ts

@@ -178,7 +178,12 @@ export class FlexServer implements GristServer {
     this.info.push(['docsRoot', this.docsRoot]);
 
     const homeUrl = process.env.APP_HOME_URL;
+    // The "base domain" is only a thing if orgs are encoded as a subdomain.
+    if (process.env.GRIST_ORG_IN_PATH === 'true' || process.env.GRIST_SINGLE_ORG) {
+      this._defaultBaseDomain = options.baseDomain || (homeUrl && new URL(homeUrl).hostname);
+    } else {
       this._defaultBaseDomain = options.baseDomain || (homeUrl && parseSubdomain(new URL(homeUrl).hostname).base);
+    }
     this.info.push(['defaultBaseDomain', this._defaultBaseDomain]);
     this._pluginUrl = options.pluginUrl || process.env.APP_UNTRUSTED_URL;
     this.info.push(['pluginUrl', this._pluginUrl]);
@@ -488,13 +493,23 @@ export class FlexServer implements GristServer {
   // Set up the main express middleware used.  For a single user setup, without logins,
   // all this middleware is currently a no-op.
   public addAccessMiddleware() {
-    if (this._check('middleware', 'map', isSingleUserMode() ? null : 'hosts')) { return; }
+    if (this._check('middleware', 'map', 'config', isSingleUserMode() ? null : 'hosts')) { return; }
 
     if (!isSingleUserMode()) {
+      const skipSession = appSettings.section('login').flag('skipSession').readBool({
+        envVar: 'GRIST_IGNORE_SESSION',
+      });
       // Middleware to redirect landing pages to preferred host
       this._redirectToHostMiddleware = this._hosts.redirectHost;
       // Middleware to add the userId to the express request object.
-      this._userIdMiddleware = expressWrap(addRequestUser.bind(null, this._dbManager, this._internalPermitStore));
+      this._userIdMiddleware = expressWrap(addRequestUser.bind(
+        null, this._dbManager, this._internalPermitStore,
+        {
+          getProfile: this._loginMiddleware.getProfile?.bind(this._loginMiddleware),
+            // Set this to false to stop Grist using a cookie for authentication purposes.
+          skipSession,
+        }
+      ));
       this._trustOriginsMiddleware = expressWrap(trustOriginHandler);
       // middleware to authorize doc access to the app. Note that this requires the userId
       // to be set on the request by _userIdMiddleware.
@@ -722,8 +737,10 @@ export class FlexServer implements GristServer {
       baseDomain: this._defaultBaseDomain,
     });
 
-    const forcedLoginMiddleware = process.env.GRIST_FORCE_LOGIN === 'true' ?
+    const isForced = appSettings.section('login').flag('forced').readBool({
-      this._redirectToLoginWithoutExceptionsMiddleware : noop;
+      envVar: 'GRIST_FORCE_LOGIN',
+    });
+    const forcedLoginMiddleware = isForced ? this._redirectToLoginWithoutExceptionsMiddleware : noop;
 
     const welcomeNewUser: express.RequestHandler = isSingleUserMode() ?
       (req, res, next) => next() :
@@ -836,11 +853,12 @@ export class FlexServer implements GristServer {
   }
 
   public addComm() {
-    if (this._check('comm', 'start', 'homedb')) { return; }
+    if (this._check('comm', 'start', 'homedb', 'config')) { return; }
     this._comm = new Comm(this.server, {
       settings: this.settings,
       sessions: this._sessions,
       hosts: this._hosts,
+      loginMiddleware: this._loginMiddleware,
       httpsServer: this.httpsServer,
     });
   }

app/server/lib/ForwardAuthLogin.ts

@@ -1,9 +1,12 @@
 import { ApiError } from 'app/common/ApiError';
+import { UserProfile } from 'app/common/LoginSessionAPI';
+import { appSettings } from 'app/server/lib/AppSettings';
 import { getRequestProfile } from 'app/server/lib/Authorizer';
 import { expressWrap } from 'app/server/lib/expressWrap';
 import { GristLoginSystem, GristServer, setUserInSession } from 'app/server/lib/GristServer';
 import { optStringParam } from 'app/server/lib/requestUtils';
 import * as express from 'express';
+import { IncomingMessage } from 'http';
 import trimEnd = require('lodash/trimEnd');
 import trimStart = require('lodash/trimStart');
 
@@ -30,13 +33,24 @@ import trimStart = require('lodash/trimStart');
  * Redirection logic currently assumes a single-site installation.
  */
 export async function getForwardAuthLoginSystem(): Promise<GristLoginSystem|undefined> {
-  const header = process.env.GRIST_FORWARD_AUTH_HEADER;
+  const section = appSettings.section('login').section('system').section('forwardAuth');
-  const logoutPath = process.env.GRIST_FORWARD_AUTH_LOGOUT_PATH || '';
+  const header = section.flag('header').readString({
+    envVar: 'GRIST_FORWARD_AUTH_HEADER',
+  });
   if (!header) { return; }
+  section.flag('active').set(true);
+  const logoutPath = section.flag('logoutPath').readString({
+    envVar: 'GRIST_FORWARD_AUTH_LOGOUT_PATH'
+  }) || '';
+  const loginPath = section.flag('loginPath').requireString({
+    envVar: 'GRIST_FORWARD_AUTH_LOGIN_PATH',
+    defaultValue: '/auth/login',
+  }) || '';
   return {
     async getMiddleware(gristServer: GristServer) {
       async function getLoginRedirectUrl(req: express.Request, url: URL)  {
-        const target = new URL(trimEnd(gristServer.getHomeUrl(req), '/') + "/auth/login");
+        const target = new URL(trimEnd(gristServer.getHomeUrl(req), '/') +
+          '/' + trimStart(loginPath, '/'));
         // In lieu of sanatizing the next url, we include only the path
         // component. This will only work for single-domain installations.
         target.searchParams.append('next', url.pathname);
@@ -65,6 +79,9 @@ export async function getForwardAuthLoginSystem(): Promise<GristLoginSystem|unde
           }));
           return "forward-auth";
         },
+        async getProfile(req: express.Request|IncomingMessage): Promise<UserProfile|null|undefined> {
+          return getRequestProfile(req, header);
+        },
       };
     },
     async deleteUser() {

app/server/lib/GristServer.ts

@@ -15,6 +15,7 @@ import { ISendAppPageOptions } from 'app/server/lib/sendAppPage';
 import { fromCallback } from 'app/server/lib/serverUtils';
 import { Sessions } from 'app/server/lib/Sessions';
 import * as express from 'express';
+import { IncomingMessage } from 'http';
 
 /**
  * Basic information about a Grist server.  Accessible in many
@@ -59,6 +60,10 @@ export interface GristLoginMiddleware {
   getLogoutMiddleware?(): express.RequestHandler[];
   // Returns arbitrary string for log.
   addEndpoints(app: express.Express): Promise<string>;
+  // Optionally, extract profile from request. Result can be a profile,
+  // or null if anonymous (and other methods of determining profile such
+  // as a cookie should not be used), or undefined to use other methods.
+  getProfile?(req: express.Request|IncomingMessage): Promise<UserProfile|null|undefined>;
 }
 
 /**

test/nbrowser/testServer.ts

@@ -37,6 +37,7 @@ export class TestServerMerged implements IMochaServer {
   public removeLogin: HomeUtil["removeLogin"];
 
   private _serverUrl: string;
+  private _proxyUrl: string|null = null;
   private _server: ChildProcess;
   private _exitPromise: Promise<number|string>;
   private _starts: number = 0;
@@ -86,6 +87,9 @@ export class TestServerMerged implements IMochaServer {
     const stubCmd = '_build/stubs/app/server/server';
     const isCore = await fse.pathExists(stubCmd + '.js');
     const cmd = isCore ? stubCmd : '_build/core/app/server/devServerMain';
+    // If a proxy is set, use a single port - otherwise we'd need a lot of
+    // proxies.
+    const useSinglePort = this._proxyUrl !== null;
 
     // The reason we fork a process rather than start a server within the same process is mainly
     // logging. Server code uses a global logger, so it's hard to separate out (especially so if
@@ -106,7 +110,10 @@ export class TestServerMerged implements IMochaServer {
       GRIST_SERVE_SAME_ORIGIN: 'true',
       APP_UNTRUSTED_URL : "http://localhost:18096",
       // Run with HOME_PORT, STATIC_PORT, DOC_PORT, DOC_WORKER_COUNT in the environment to override.
-      ...(isCore ? {
+      ...(useSinglePort ? {
+        APP_HOME_URL: this.getHost(),
+        GRIST_SINGLE_PORT: 'true',
+      } : (isCore ? {
         HOME_PORT: '8095',
         STATIC_PORT: '8095',
         DOC_PORT: '8095',
@@ -118,7 +125,7 @@ export class TestServerMerged implements IMochaServer {
         DOC_PORT: '8100',
         DOC_WORKER_COUNT: '5',
         PORT: '0',
-      }),
+      })),
       // This skips type-checking when running server, but reduces startup time a lot.
       TS_NODE_TRANSPILE_ONLY: 'true',
       ...process.env,
@@ -186,7 +193,7 @@ export class TestServerMerged implements IMochaServer {
 
   public getHost(): string {
     if (this.isExternalServer()) { return process.env.HOME_URL!; }
-    return this._serverUrl;
+    return this._proxyUrl || this._serverUrl;
   }
 
   public getUrl(team: string, relPath: string) {
@@ -200,6 +207,12 @@ export class TestServerMerged implements IMochaServer {
     return `${url}${relPath}`;
   }
 
+  // Configure the server to be accessed via a proxy. You'll need to
+  // restart the server after changing this setting.
+  public updateProxy(proxyUrl: string|null) {
+    this._proxyUrl = proxyUrl;
+  }
+
   /**
    * Returns whether the server is up and responsive.
    */

test/server/lib/Authorizer.ts

@@ -17,6 +17,7 @@ let server: FlexServer;
 let dbManager: HomeDBManager;
 
 async function activateServer(home: FlexServer, docManager: DocManager) {
+  await home.loadConfig();
   await home.initHomeDBManager();
   home.addHosts();
   home.addDocWorkerMap();