Dmitri's remarks

app/client/ui/errorPages.ts

@@ -104,10 +104,10 @@ export function createNotFoundPage(appModel: AppModel, message?: string) {
 }
 
 export function createSigninFailedPage(appModel: AppModel, message?: string) {
-  document.title = t("Signin failed{{suffix}}", {suffix: getPageTitleSuffix(getGristConfig())});
-  return pagePanelsError(appModel, t("Signin failed{{suffix}}", {suffix: ''}), [
+  document.title = t("Sign-in failed{{suffix}}", {suffix: getPageTitleSuffix(getGristConfig())});
+  return pagePanelsError(appModel, t("Sign-in failed{{suffix}}", {suffix: ''}), [
     cssErrorText(message ??
-      t("Failed to login.{{separator}}Please try again or contact support.", {
+      t("Failed to log in.{{separator}}Please try again or contact support.", {
         separator: dom('br')
     })),
     signInAgainButton(),

app/server/lib/BrowserSession.ts

@@ -69,19 +69,21 @@ export interface SessionObj {
                           // something they just added, without allowing the suer
                           // to edit other people's contributions).
 
-  oidc?: {
-    // more details on protections are available here: https://danielfett.de/2020/05/16/pkce-vs-nonce-equivalent-or-not/#special-case-error-responses
-    // code_verifier is used during OIDC authentication for PKCE protection, to protect against attacks like CSRF.
-    // PKCE + state are currently the best combination to protect against CSRF and code injection attacks.
-    code_verifier?: string;
-    // much like code_verifier, for OIDC providers that do not support PKCE.
-    nonce?: string;
-    // state is used to protect against Error Responses spoofs.
-    state?: string;
-    targetUrl?: string;
-    // Stores user claims signed by the issuer, store it to allow loging out.
-    idToken?: string;
-  }
+  oidc?: SessionOIDCInfo
+}
+
+export interface SessionOIDCInfo {
+  // more details on protections are available here: https://danielfett.de/2020/05/16/pkce-vs-nonce-equivalent-or-not/#special-case-error-responses
+  // code_verifier is used during OIDC authentication for PKCE protection, to protect against attacks like CSRF.
+  // PKCE + state are currently the best combination to protect against CSRF and code injection attacks.
+  code_verifier?: string;
+  // much like code_verifier, for OIDC providers that do not support PKCE.
+  nonce?: string;
+  // state is used to protect against Error Responses spoofs.
+  state?: string;
+  targetUrl?: string;
+  // Stores user claims signed by the issuer, store it to allow loging out.
+  idToken?: string;
 }
 
 // Make an artificial change to a session to encourage express-session to set a cookie.

app/server/lib/OIDCConfig.ts

@@ -75,6 +75,7 @@ import { UserProfile } from 'app/common/LoginSessionAPI';
 import { SendAppPage } from 'app/server/lib/sendAppPage';
 import { StringUnionError } from 'app/common/StringUnion';
 import { EnabledProtection, EnabledProtectionString, ProtectionsManager } from './oidc/Protections';
+import { SessionObj } from './BrowserSession';
 
 const CALLBACK_URL = '/oauth2/callback';
 
@@ -191,7 +192,7 @@ export class OIDCConfig {
     try {
       mreq = this._getRequestWithSession(req);
     } catch(err) {
-      log.warn(err.message);
+      log.warn("OIDCConfig callback:", err.message);
       return this._sendErrorPage(req, res);
     }
 
@@ -224,8 +225,11 @@ export class OIDCConfig {
         profile,
       }));
 
+      // We clear the previous session info, like the states, nonce or the code verifier, which
+      // now that we are authenticated.
+      // We store the idToken for later, especially for the logout
       mreq.session.oidc = {
-        idToken: tokenSet.id_token, // keep idToken for logout
+        idToken: tokenSet.id_token,
       };
       res.redirect(targetUrl ?? '/');
     } catch (err) {
@@ -260,7 +264,7 @@ export class OIDCConfig {
   }
 
   public async getLogoutRedirectUrl(req: express.Request, redirectUrl: URL): Promise<string> {
-    const mreq = this._getRequestWithSession(req, { throwIfMissing: false });
+    const session: SessionObj|undefined = (req as RequestWithLogin).session;
     // For IdPs that don't have end_session_endpoint, we just redirect to the logout page.
     if (this._skipEndSessionEndpoint) {
       return redirectUrl.href;
@@ -271,7 +275,7 @@ export class OIDCConfig {
     }
     return this._client.endSessionUrl({
       post_logout_redirect_uri: redirectUrl.href,
-      id_token_hint: mreq.session?.oidc?.idToken,
+      id_token_hint: session?.oidc?.idToken,
     });
   }
 
@@ -303,9 +307,9 @@ export class OIDCConfig {
     });
   }
 
-  private _getRequestWithSession(req: express.Request, {throwIfMissing} = {throwIfMissing: true}) {
+  private _getRequestWithSession(req: express.Request) {
     const mreq = req as RequestWithLogin;
-    if (!mreq.session && throwIfMissing) { throw new Error('no session available'); }
+    if (!mreq.session) { throw new Error('no session available'); }
 
     return mreq;
   }

app/server/lib/oidc/Protections.ts

@@ -1,5 +1,5 @@
 import { StringUnion } from 'app/common/StringUnion';
-import { SessionObj } from 'app/server/lib/BrowserSession';
+import { SessionOIDCInfo } from 'app/server/lib/BrowserSession';
 import { AuthorizationParameters, generators, OpenIDCallbackChecks } from 'openid-client';
 
 export const EnabledProtection = StringUnion(
@@ -9,8 +9,6 @@ export const EnabledProtection = StringUnion(
 );
 export type EnabledProtectionString = typeof EnabledProtection.type;
 
-type SessionOIDCInfo = Exclude<SessionObj['oidc'], undefined>;
-
 interface Protection {
   generate(): SessionOIDCInfo;
   forgeAuthUrlParams(sessionInfo: SessionOIDCInfo): AuthorizationParameters;