clean up code: add comments, remove logs, improve naming etc

api.planx.uk/modules/auth/controller.ts

@@ -1,27 +1,17 @@
 import { CookieOptions, RequestHandler, Response } from "express";
 import { Request } from "express-jwt";
 
-import { client } from './passport'
-
 export const failedLogin: RequestHandler = (_req, _res, next) =>
   next({
     status: 401,
     message: "User failed to authenticate",
   });
 
 export const logout: RequestHandler = (req, res) => {
-  console.log("INVOKING LOGOUT CONTROLLER")
-
-  console.log("REQ:")
-  console.log(req)
-
   req.logout(() => {
     // do nothing
   });
-  // TODO: redirect to logout URL with id_token_hint (and logout_hint ??)
-  // logout_url = client.endSessionUrl({
-  //   id_token_hint: 
-  // });
+  // TODO: implement logout for Microsoft strategy (redirect to logout URL with hint)
   res.redirect(process.env.EDITOR_URL_EXT!);
 };
 

api.planx.uk/modules/auth/middleware.ts

@@ -10,7 +10,7 @@ import { Role } from "@opensystemslab/planx-core/types";
 
 import { ServerError } from "../../errors";
 import { Template } from "../../lib/notify";
-import { passport } from "./passport";
+import { passportWithStrategies } from "./passport";
 
 export const userContext = new AsyncLocalStorage<{ user: Express.User }>();
 
@@ -113,41 +113,34 @@ export const useJWT = expressjwt({
 
 export const useGoogleAuth: RequestHandler = (req, res, next) => {
   req.session!.returnTo = req.get("Referrer");
-  return passport.authenticate("google", {
+  return passportWithStrategies.authenticate("google", {
     scope: ["profile", "email"],
   })(req, res, next);
 };
 
 export const useGoogleCallbackAuth: RequestHandler = (req, res, next) => {
-  return passport.authenticate("google", {
+  return passportWithStrategies.authenticate("google", {
     failureRedirect: "/auth/login/failed",
   })(req, res, next);
 };
 
 export const useMicrosoftAuth: RequestHandler = (req, res, next) => {
-  console.log("INVOKING MICROSOFT MIDDLEWARE")
   req.session!.returnTo = req.get("Referrer");
-  console.log("REFERRER:")
-  console.log(req.get("Referrer"))
+
+  // generate a nonce to enable us to validate the response from OP
   const nonce = generators.nonce();
-  console.log(`Generated a nonce: ${nonce}`);
+  console.debug(`Generated a nonce: %s`, nonce);
   req.session!.nonce = nonce
-  // @ts-expect-error - method not typed to accept nonce (but it does deliver it to the strategy)
-  // DOES IT ??
-  return passport.authenticate("microsoft-oidc", {
+
+  // @ts-expect-error (method not typed to accept nonce, but it does pass it to the strategy)
+  return passportWithStrategies.authenticate("microsoft-oidc", {
     prompt: "select_account",
-    nonce: nonce,
+    nonce,
   })(req, res, next);
 };
 
 export const useMicrosoftCallbackAuth: RequestHandler = (req, res, next) => {
-  console.log("INVOKING MICROSOFT CALLBACK MIDDLEWARE")
-  console.log("THIS ALMOST WORKS, BUT THEN REDIRECTS BACK TO MICROSOFT AFTER HITTING /")
-
-  console.log("REQ:")
-  console.log(req)
-
-  return passport.authenticate("microsoft-oidc", {
+  return passportWithStrategies.authenticate("microsoft-oidc", {
     failureRedirect: "/auth/login/failed",
   })(req, res, next);
 };

api.planx.uk/modules/auth/passport.ts

@@ -2,55 +2,39 @@ import { custom, Issuer } from 'openid-client';
 import passport from 'passport';
 
 import { googleStrategy } from './strategy/google';
-import { getMicrosoftOidcStrategy } from './strategy/microsoft-oidc';
-
-const MICROSOFT_OPENID_CONFIG_URL = "https://login.microsoftonline.com/common/v2.0/.well-known/openid-configuration";
-
-// we need the client in various files, so it needs to be instantiated early on, and exported
-let client;
-
-// TODO: reset timeout (extended for local testing with poor connection)
-custom.setHttpOptionsDefaults({
-  timeout: 10000,
-})
-console.log("ATTEMPTING TO DISCOVER METADATA DOC")
-Issuer.discover(MICROSOFT_OPENID_CONFIG_URL).then(microsoftIssuer => {
-  console.log(
-    "Discovered issuer %s %O",
-    microsoftIssuer.issuer,
-    microsoftIssuer.metadata,
-  );
-
-  const client_id = process.env.MICROSOFT_CLIENT_ID!;
-  if (typeof client_id !== 'string') {
-    throw new Error('No MICROSOFT_CLIENT_ID in the environment');
-  }
-
-  client = new microsoftIssuer.Client({
-    client_id: client_id,
-    client_secret: process.env.MICROSOFT_CLIENT_SECRET!,
-    redirect_uris: [`${process.env.API_URL_EXT}/auth/microsoft/callback`],
-    post_logout_redirect_uris: [process.env.EDITOR_URL_EXT!],
-    response_types: ["id_token"],
+import { getMicrosoftOidcStrategy, getMicrosoftClientConfig, MICROSOFT_OPENID_CONFIG_URL } from './strategy/microsoft-oidc';
+
+const setupPassport = () => {
+  // TODO: remove below config (timeout extended for local testing with poor connection)
+  custom.setHttpOptionsDefaults({
+    timeout: 10000,
+  })
+
+  // explicitly instantiate new passport for clarity
+  let passportWithStrategies = new passport.Passport()
+
+  // build Microsoft OIDC client, and use it to build the related strategy
+  let microsoftOidcClient;
+  Issuer.discover(MICROSOFT_OPENID_CONFIG_URL).then(microsoftIssuer => {
+    console.debug("Discovered issuer %s", microsoftIssuer.issuer);
+    const microsoftClientConfig = getMicrosoftClientConfig();
+    microsoftOidcClient = new microsoftIssuer.Client(microsoftClientConfig);
+    console.debug("Built Microsoft client: %O", microsoftOidcClient);
+    passportWithStrategies.use('microsoft-oidc', getMicrosoftOidcStrategy(microsoftOidcClient));
   });
 
-  console.log("Built Microsoft client:");
-  console.log(client);
-
-  passport.use('google', googleStrategy);
-  passport.use('microsoft-oidc', getMicrosoftOidcStrategy(client));
-
-  passport.serializeUser((user: any, done) => {
-    console.log("SERIALIZING USER")
-    console.log(user)
+  // do any other aspects of passport setup which can be handled here
+  passportWithStrategies.use('google', googleStrategy);
+  passportWithStrategies.serializeUser((user: any, done) => {
     done(null, user);
   });
-
-  passport.deserializeUser((obj: any, done) => {
-    console.log("DESERIALIZING USER")
-    console.log(obj)
+  passportWithStrategies.deserializeUser((obj: any, done) => {
     done(null, obj);
   });
-});
+
+  return { passportWithStrategies, microsoftOidcClient }
+}
 
-export { passport, client };
+// instantiate and export the new passport class and Microsoft client as early as possible
+let { passportWithStrategies, microsoftOidcClient } = setupPassport();
+export { passportWithStrategies, microsoftOidcClient };

api.planx.uk/modules/auth/service.ts

@@ -3,16 +3,11 @@ import { $api } from "../../client";
 import { User, Role } from "@opensystemslab/planx-core/types";
 
 export const buildJWT = async (email: string): Promise<string | undefined> => {
-  console.log("FETCHING USER FROM HASURA BY EMAIL")
   await checkUserCanAccessEnv(email, process.env.NODE_ENV);
   const user = await $api.user.getByEmail(email);
-  console.log("USER:")
-  console.log(user)
   if (!user) return;
 
   const claims = generateHasuraClaimsForUser(user);
-  console.log("CLAIMS:")
-  console.log(claims)
 
   const data = {
     sub: user.id.toString(),

api.planx.uk/modules/auth/strategy/microsoft-oidc.ts

@@ -1,41 +1,44 @@
-import { Strategy, TokenSet, Client } from "openid-client";
+import { Strategy, TokenSet, Client, ClientMetadata } from "openid-client";
 import { buildJWT } from "../service";
 
+export const MICROSOFT_OPENID_CONFIG_URL = "https://login.microsoftonline.com/common/v2.0/.well-known/openid-configuration";
+
+export const getMicrosoftClientConfig = (): ClientMetadata => {
+  const client_id = process.env.MICROSOFT_CLIENT_ID!;
+  if (typeof client_id !== 'string') {
+    throw new Error('No MICROSOFT_CLIENT_ID in the environment');
+  }
+  return {
+    client_id,
+    client_secret: process.env.MICROSOFT_CLIENT_SECRET!,
+    redirect_uris: [`${process.env.API_URL_EXT}/auth/microsoft/callback`],
+    post_logout_redirect_uris: [process.env.EDITOR_URL_EXT!],
+    response_types: ["id_token"],
+  }
+}
+
+// oidc = OpenID Connect, an auth standard built on top of OAuth 2.0
 export const getMicrosoftOidcStrategy = (
   client: Client,
 ): Strategy<Client> => {
-  console.log("redirect uri domain:");
-  console.log(process.env.API_URL_EXT);
-
-  // oidc = OpenID Connect
   return new Strategy({
       client: client,
       params: {
         scope: "openid email profile",
-        response_mode: "form_post", // could also be 'query' or 'fragment'
+        response_mode: "form_post",
       },
+      // need the request in the verify callback to validate the returned nonce
       passReqToCallback: true,
-      // usePKCE: false, // whether to use PKCE - defaults to true, according to docs
     },
     async (req: any, tokenSet: TokenSet, done: any): Promise<void> => {
-      console.log("INVOKING STRATEGY CALLBACK!")
-
-      console.log("TOKEN SET:");
-      console.log(tokenSet);
-
-      console.log("CLAIMS:")
-      console.log(tokenSet.claims())
-
-      const id_token = tokenSet.id_token;
+      // TODO: use state to pass the redirectTo query param through the auth flow
       const state = tokenSet.state;
-      // TODO: do something with state??
 
       const claims = tokenSet.claims();
-      const email = claims.email
-      const returned_nonce = claims.nonce
-
-      console.log("SESSION:")
-      console.log(req.session)
+      const email = claims.email;
+      const returned_nonce = claims.nonce;
+      // we grab login_hint to provide to the logout endpoint later (as per OpenID spec)
+      const login_hint = claims.login_hint;
 
       if (returned_nonce != req.session.nonce) {
         return done(new Error("Returned nonce does not match session nonce"), null)
@@ -55,8 +58,6 @@ export const getMicrosoftOidcStrategy = (
       }
 
       return done(null, { jwt });
-
-      // TODO: handle error case i.e. done(err)
     },
   );
 };

api.planx.uk/server.ts

@@ -13,7 +13,7 @@ import helmet from "helmet";
 import { ServerError } from "./errors";
 import airbrake from "./airbrake";
 import { apiLimiter } from "./rateLimit";
-import { passport } from "./modules/auth/passport";
+import { passportWithStrategies } from "./modules/auth/passport";
 import authRoutes from "./modules/auth/routes";
 import teamRoutes from "./modules/team/routes";
 import miscRoutes from "./modules/misc/routes";
@@ -115,8 +115,8 @@ app.use(
   }),
 );
 
-app.use(passport.initialize());
-app.use(passport.session());
+app.use(passportWithStrategies.initialize());
+app.use(passportWithStrategies.session());
 
 app.use(urlencoded({ extended: true }));
 

editor.planx.uk/src/index.tsx

@@ -37,16 +37,12 @@ if (!window.customElements.get("my-map")) {
 const hasJWT = (): boolean | void => {
   // This cookie indicates the presence of the secure httpOnly "jwt" cookie
   const authCookie = getCookie("auth");
-  console.log("AUTH COOKIE:");
-  console.log(authCookie);
   if (authCookie) return true;
 
   // If JWT not set via cookie, check search params
-  console.log("LOOKING FOR JWT IN QUERY PARAMS");
   const jwtSearchParams = new URLSearchParams(window.location.search).get(
     "jwt",
   );
-  console.log(jwtSearchParams);
   if (!jwtSearchParams) return false;
 
   // Remove JWT from URL, and re-run this function