From 4455428888e3d9b08f0ddb15aef485e6e353186f Mon Sep 17 00:00:00 2001 From: Danilo Spinelli Date: Wed, 19 Feb 2020 09:52:32 +0100 Subject: [PATCH] chore: refactoring (#14) --- .gitignore | 2 +- src/example.ts | 25 ++++++++++++----- src/index.ts | 42 +++++++++++++++------------- src/strategy/redis_cache_provider.ts | 2 +- src/strategy/spid.ts | 18 +++++------- 5 files changed, 49 insertions(+), 40 deletions(-) diff --git a/.gitignore b/.gitignore index db5b374e..8b528f46 100644 --- a/.gitignore +++ b/.gitignore @@ -6,4 +6,4 @@ coverage .vscode .idea/ *.pem - +.npmrc diff --git a/src/example.ts b/src/example.ts index f63e4a97..1d46f7a6 100644 --- a/src/example.ts +++ b/src/example.ts @@ -17,7 +17,6 @@ import { LogoutT, withSpid } from "."; -import { overrideCacheProvider } from "./strategy/redis_cache_provider"; import { logger } from "./utils/logger"; import { IServiceProviderConfig } from "./utils/middleware"; @@ -85,7 +84,6 @@ const samlConfig: SamlConfig = { acceptedClockSkewMs: 0, attributeConsumingServiceIndex: "0", authnContext: "https://www.spid.gov.it/SpidL1", - cacheProvider: overrideCacheProvider(), callbackUrl: "http://localhost:3000" + appConfig.assertionConsumerServicePath, // decryptionPvk: fs.readFileSync("./certs/key.pem", "utf-8"), identifierFormat: "urn:oasis:names:tc:SAML:2.0:nameid-format:transient", @@ -112,16 +110,26 @@ app.use(passport.initialize()); withSpid( appConfig, samlConfig, - redisClient, serviceProviderConfig, + redisClient, app, acs, logout ) - .map(withSpidApp => { - withSpidApp.get("/success", (_, res) => res.json({ success: "success" })); + .map(({ app: withSpidApp, startIdpMetadataRefreshTimer }) => { + const idpMetadataRefreshTimer = startIdpMetadataRefreshTimer(); + withSpidApp.on("server:stop", () => clearInterval(idpMetadataRefreshTimer)); + withSpidApp.get("/success", (_, res) => + res.json({ + success: "success" + }) + ); withSpidApp.get("/error", (_, res) => - res.json({ error: "error" }).status(400) + res + .json({ + error: "error" + }) + .status(400) ); withSpidApp.use( ( @@ -129,7 +137,10 @@ withSpid( _: express.Request, res: express.Response, ___: express.NextFunction - ) => res.status(505).send({ error: error.message }) + ) => + res.status(505).send({ + error: error.message + }) ); withSpidApp.listen(3000); }) diff --git a/src/index.ts b/src/index.ts index 4cee3da6..3bdf7108 100644 --- a/src/index.ts +++ b/src/index.ts @@ -20,7 +20,7 @@ import * as passport from "passport"; import { SamlConfig } from "passport-saml"; import { RedisClient } from "redis"; import { Builder } from "xml2js"; -import { overrideCacheProvider } from "./strategy/redis_cache_provider"; +import { noopCacheProvider } from "./strategy/redis_cache_provider"; import { logger } from "./utils/logger"; import { getSpidStrategyOptionsUpdater, @@ -59,9 +59,7 @@ export interface IApplicationConfig { } // re-export -export { overrideCacheProvider }; -export { IServiceProviderConfig }; -export { SamlConfig }; +export { noopCacheProvider, IServiceProviderConfig, SamlConfig }; /** * Wraps assertion consumer service handler @@ -118,12 +116,15 @@ const withSpidAuthMiddleware = ( export function withSpid( appConfig: IApplicationConfig, samlConfig: SamlConfig, - redisClient: RedisClient, serviceProviderConfig: IServiceProviderConfig, + redisClient: RedisClient, app: express.Express, acs: AssertionConsumerServiceT, logout: LogoutT -): TaskEither { +): TaskEither< + Error, + { app: express.Express; startIdpMetadataRefreshTimer: () => NodeJS.Timeout } +> { const loadSpidStrategyOptions = getSpidStrategyOptionsUpdater( samlConfig, serviceProviderConfig @@ -156,19 +157,20 @@ export function withSpid( .map(spidStrategy => { // Schedule get and refresh // SPID passport strategy options - const idpMetadataRefreshTimer = setInterval( - () => - loadSpidStrategyOptions() - .map(opts => setSpidStrategyOption(app, opts)) - .run() - .catch(e => { - logger.error("loadSpidStrategyOptions|error:%s", e); - }), - serviceProviderConfig.idpMetadataRefreshIntervalMillis - ); - - // Avoid hanging when express server exits - app.on("server:stop", () => clearInterval(idpMetadataRefreshTimer)); + const startIdpMetadataRefreshTimer = () => + // Remember to call + // app.on("server:stop", () => clearInterval(idpMetadataRefreshTimer)); + // to avoid hanging when express server exits + setInterval( + () => + loadSpidStrategyOptions() + .map(opts => setSpidStrategyOption(app, opts)) + .run() + .catch(e => { + logger.error("loadSpidStrategyOptions|error:%s", e); + }), + serviceProviderConfig.idpMetadataRefreshIntervalMillis + ); // Initializes SpidStrategy for passport passport.use("spid", spidStrategy); @@ -225,6 +227,6 @@ export function withSpid( // Setup logout handler app.post(appConfig.sloPath, toExpressHandler(logout)); - return app; + return { app, startIdpMetadataRefreshTimer }; }); } diff --git a/src/strategy/redis_cache_provider.ts b/src/strategy/redis_cache_provider.ts index b7392933..1b3300b9 100644 --- a/src/strategy/redis_cache_provider.ts +++ b/src/strategy/redis_cache_provider.ts @@ -27,7 +27,7 @@ export interface IExtendedCacheProvider { // those methods must never fail since there's // practically no error handling in passport-saml // (a very bad lot of spaghetti code) -export const overrideCacheProvider = (): CacheProvider => { +export const noopCacheProvider = (): CacheProvider => { return { // saves the key with the optional value // invokes the callback with the value saved diff --git a/src/strategy/spid.ts b/src/strategy/spid.ts index 34d1fef1..6d9bb1cb 100644 --- a/src/strategy/spid.ts +++ b/src/strategy/spid.ts @@ -14,7 +14,8 @@ import { RedisClient } from "redis"; import { MultiSamlConfig } from "passport-saml/multiSamlStrategy"; import { getExtendedRedisCacheProvider, - IExtendedCacheProvider + IExtendedCacheProvider, + noopCacheProvider } from "./redis_cache_provider"; import { CustomSamlClient } from "./saml_client"; @@ -56,20 +57,15 @@ export class SpidStrategy extends SamlStrategy { // 8 hours options.requestIdExpirationPeriodMs = 28800000; } + + // use our custom cache provider this.extendedRedisCacheProvider = getExtendedRedisCacheProvider( this.redisClient, options.requestIdExpirationPeriodMs ); - if (!options.cacheProvider) { - // WARNING: you cannot use this one if you have - // multiple instances of the express app running - // (ie. multiple pods on Kubernetes). - // Use a RedisCacheProvider instead which can be - // safely shared between instances. - options.cacheProvider = new InMemoryCacheProvider({ - keyExpirationPeriodMs: options.requestIdExpirationPeriodMs - }); - } + + // bypass passport-saml cache provider + options.cacheProvider = noopCacheProvider(); } public authenticate(