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.

Author: forty

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,

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)
       );
     });
   }

src/passport-saml/strategy.ts

@@ -12,8 +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;
@@ -172,7 +171,7 @@ class Strategy extends PassportStrategy {
       .catch((err) => callback(err));
   }
 
-  generateServiceProviderMetadata(
+  protected _generateServiceProviderMetadata(
     decryptionCert: string | null,
     signingCert?: string | null
   ): string {
@@ -189,4 +188,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);
+  }
+}

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");
   });