From b4778c090fcc450017b3e9434f00cf3c8fe63fe1 Mon Sep 17 00:00:00 2001 From: Dan Goss Date: Fri, 12 Jul 2024 19:15:50 +0100 Subject: [PATCH] clean up code: add comments, remove logs, improve naming etc --- api.planx.uk/modules/auth/controller.ts | 12 +-- api.planx.uk/modules/auth/middleware.ts | 29 +++----- api.planx.uk/modules/auth/passport.ts | 74 ++++++++----------- api.planx.uk/modules/auth/service.ts | 5 -- .../modules/auth/strategy/microsoft-oidc.ts | 49 ++++++------ api.planx.uk/server.ts | 6 +- editor.planx.uk/src/index.tsx | 4 - 7 files changed, 69 insertions(+), 110 deletions(-) diff --git a/api.planx.uk/modules/auth/controller.ts b/api.planx.uk/modules/auth/controller.ts index 9d5c22c324..744a0e927f 100644 --- a/api.planx.uk/modules/auth/controller.ts +++ b/api.planx.uk/modules/auth/controller.ts @@ -1,8 +1,6 @@ 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, @@ -10,18 +8,10 @@ export const failedLogin: RequestHandler = (_req, _res, next) => }); 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!); }; diff --git a/api.planx.uk/modules/auth/middleware.ts b/api.planx.uk/modules/auth/middleware.ts index 276f9f8cf7..31e5639f9a 100644 --- a/api.planx.uk/modules/auth/middleware.ts +++ b/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); }; diff --git a/api.planx.uk/modules/auth/passport.ts b/api.planx.uk/modules/auth/passport.ts index 58a2cbe0a8..295c69b2ae 100644 --- a/api.planx.uk/modules/auth/passport.ts +++ b/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 }; \ No newline at end of file +// instantiate and export the new passport class and Microsoft client as early as possible +let { passportWithStrategies, microsoftOidcClient } = setupPassport(); +export { passportWithStrategies, microsoftOidcClient }; \ No newline at end of file diff --git a/api.planx.uk/modules/auth/service.ts b/api.planx.uk/modules/auth/service.ts index 7665ffe92e..114a1553b2 100644 --- a/api.planx.uk/modules/auth/service.ts +++ b/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 => { - 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(), diff --git a/api.planx.uk/modules/auth/strategy/microsoft-oidc.ts b/api.planx.uk/modules/auth/strategy/microsoft-oidc.ts index f947867564..3cf5958576 100644 --- a/api.planx.uk/modules/auth/strategy/microsoft-oidc.ts +++ b/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 => { - 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 => { - 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) }, ); }; diff --git a/api.planx.uk/server.ts b/api.planx.uk/server.ts index 62a5857b38..965ee9c645 100644 --- a/api.planx.uk/server.ts +++ b/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 })); diff --git a/editor.planx.uk/src/index.tsx b/editor.planx.uk/src/index.tsx index 040ed4aae6..50e58d2691 100644 --- a/editor.planx.uk/src/index.tsx +++ b/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