From 9835da11e904921a55b331bfb8e9ade0f4b79f97 Mon Sep 17 00:00:00 2001
From: Quentin Barbe <quentin.barbe@dashlane.com>
Date: Mon, 15 Mar 2021 23:46:30 +0100
Subject: [PATCH] Improve the typing of the Strategy class hierarchy

The current expected ts error causes problems with some earlier version of TS.
Also, it does not make any sense to have a method overide a method with a
different signature, just because they have the same name.
---
 src/passport-saml/index.ts             |  2 +-
 src/passport-saml/multiSamlStrategy.ts |  7 +++----
 src/passport-saml/strategy.ts          | 17 ++++++++++++-----
 test/multiSamlStrategy.spec.ts         | 11 ++++++-----
 4 files changed, 22 insertions(+), 15 deletions(-)

diff --git a/src/passport-saml/index.ts b/src/passport-saml/index.ts
index 24e48ba44..f2eb2559e 100644
--- a/src/passport-saml/index.ts
+++ b/src/passport-saml/index.ts
@@ -1,6 +1,6 @@
 import type { CacheItem, CacheProvider } from "./inmemory-cache-provider";
 import { SAML } from "./saml";
-import Strategy = require("./strategy");
+import { Strategy } from "./strategy";
 import MultiSamlStrategy = require("./multiSamlStrategy");
 import type {
   Profile,
diff --git a/src/passport-saml/multiSamlStrategy.ts b/src/passport-saml/multiSamlStrategy.ts
index ef07ab6f7..29cac35f1 100644
--- a/src/passport-saml/multiSamlStrategy.ts
+++ b/src/passport-saml/multiSamlStrategy.ts
@@ -1,7 +1,7 @@
 import * as util from "util";
 import * as saml from "./saml";
 import { CacheProvider as InMemoryCacheProvider } from "./inmemory-cache-provider";
-import SamlStrategy = require("./strategy");
+import { AbstractStrategy } from "./strategy";
 import type { Request } from "express";
 import {
   AuthenticateOptions,
@@ -13,7 +13,7 @@ import {
   VerifyWithRequest,
 } from "./types";
 
-class MultiSamlStrategy extends SamlStrategy {
+class MultiSamlStrategy extends AbstractStrategy {
   static readonly newSamlProviderOnConstruct = false;
 
   _options: SamlConfig & MultiSamlConfig;
@@ -65,7 +65,6 @@ class MultiSamlStrategy extends SamlStrategy {
     });
   }
 
-  /** @ts-expect-error typescript disallows changing method signature in a subclass */
   generateServiceProviderMetadata(
     req: Request,
     decryptionCert: string | null,
@@ -86,7 +85,7 @@ class MultiSamlStrategy extends SamlStrategy {
       Object.setPrototypeOf(strategy, this);
       return callback(
         null,
-        super.generateServiceProviderMetadata.call(strategy, decryptionCert, signingCert)
+        this._generateServiceProviderMetadata.call(strategy, decryptionCert, signingCert)
       );
     });
   }
diff --git a/src/passport-saml/strategy.ts b/src/passport-saml/strategy.ts
index 8feac264e..1ef930a90 100644
--- a/src/passport-saml/strategy.ts
+++ b/src/passport-saml/strategy.ts
@@ -12,9 +12,7 @@ import {
 } from "./types";
 import { Profile } from "./types";
 
-class Strategy extends PassportStrategy {
-  static readonly newSamlProviderOnConstruct = true;
-
+export abstract class AbstractStrategy extends PassportStrategy {
   name: string;
   _verify: VerifyWithRequest | VerifyWithoutRequest;
   _saml: saml.SAML | undefined;
@@ -172,7 +170,7 @@ class Strategy extends PassportStrategy {
       .catch((err) => callback(err));
   }
 
-  generateServiceProviderMetadata(
+  protected _generateServiceProviderMetadata(
     decryptionCert: string | null,
     signingCert?: string | null
   ): string {
@@ -189,4 +187,13 @@ class Strategy extends PassportStrategy {
   }
 }
 
-export = Strategy;
+export class Strategy extends AbstractStrategy {
+  static readonly newSamlProviderOnConstruct = true;
+
+  generateServiceProviderMetadata(
+    decryptionCert: string | null,
+    signingCert?: string | null
+  ): string {
+    return this._generateServiceProviderMetadata(decryptionCert, signingCert);
+  }
+}
diff --git a/test/multiSamlStrategy.spec.ts b/test/multiSamlStrategy.spec.ts
index 4718541d4..2324f1c59 100644
--- a/test/multiSamlStrategy.spec.ts
+++ b/test/multiSamlStrategy.spec.ts
@@ -2,7 +2,8 @@
 import * as express from "express";
 import * as sinon from "sinon";
 import * as should from "should";
-import { Strategy as SamlStrategy, MultiSamlStrategy, SAML } from "../src/passport-saml";
+import { MultiSamlStrategy, SAML } from "../src/passport-saml";
+import { AbstractStrategy } from "../src/passport-saml/strategy";
 import {
   MultiSamlConfig,
   SamlOptionsCallback,
@@ -20,7 +21,7 @@ describe("MultiSamlStrategy()", function () {
       return { cert: FAKE_CERT };
     }
     const strategy = new MultiSamlStrategy({ getSamlOptions }, noop);
-    strategy.should.be.an.instanceOf(SamlStrategy);
+    strategy.should.be.an.instanceOf(AbstractStrategy);
   });
 
   it("throws if wrong finder is provided", function () {
@@ -33,7 +34,7 @@ describe("MultiSamlStrategy()", function () {
 
 describe("MultiSamlStrategy#authenticate", function () {
   beforeEach(function () {
-    this.superAuthenticateStub = sinon.stub(SamlStrategy.prototype, "authenticate");
+    this.superAuthenticateStub = sinon.stub(AbstractStrategy.prototype, "authenticate");
   });
 
   afterEach(function () {
@@ -154,7 +155,7 @@ describe("MultiSamlStrategy#authorize", function () {
 
 describe("MultiSamlStrategy#logout", function () {
   beforeEach(function () {
-    this.superLogoutMock = sinon.stub(SamlStrategy.prototype, "logout");
+    this.superLogoutMock = sinon.stub(AbstractStrategy.prototype, "logout");
   });
 
   afterEach(function () {
@@ -229,7 +230,7 @@ describe("MultiSamlStrategy#logout", function () {
 describe("MultiSamlStrategy#generateServiceProviderMetadata", function () {
   beforeEach(function () {
     this.superGenerateServiceProviderMetadata = sinon
-      .stub(SamlStrategy.prototype, "generateServiceProviderMetadata")
+      .stub(AbstractStrategy.prototype as any, "_generateServiceProviderMetadata") // force type to any to ignore TS protected property
       .returns("My Metadata Result");
   });