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.
node-saml · Apr 5, 2021 · e37ee06 · e37ee06
1 parent 0798e4d
commit e37ee06
Show file tree

Hide file tree

Showing 4 changed files with 25 additions and 19 deletions.
diff --git a/src/passport-saml/index.ts b/src/passport-saml/index.ts
@@ -1,7 +1,8 @@
 import type { CacheItem, CacheProvider } from "../node-saml/inmemory-cache-provider";
 import { SAML } from "../node-saml";
-import Strategy = require("./strategy");
-import MultiSamlStrategy = require("./multiSamlStrategy");
+import { Strategy, AbstractStrategy } from "./strategy";
+import { MultiSamlStrategy } from "./multiSamlStrategy";
+
 import type {
   Profile,
   SamlConfig,
@@ -12,6 +13,7 @@ import type {
 
 export {
   SAML,
+  AbstractStrategy,
   Strategy,
   MultiSamlStrategy,
   CacheItem,

diff --git a/src/passport-saml/multiSamlStrategy.ts b/src/passport-saml/multiSamlStrategy.ts
@@ -1,6 +1,5 @@
-import * as util from "util";
 import { SAML } from "../node-saml";
-import SamlStrategy = require("./strategy");
+import { AbstractStrategy } from "./strategy";
 import type { Request } from "express";
 import {
   AuthenticateOptions,
@@ -11,9 +10,8 @@ import {
   VerifyWithRequest,
 } from "./types";
 
-class MultiSamlStrategy extends SamlStrategy {
+export class MultiSamlStrategy extends AbstractStrategy {
   static readonly newSamlProviderOnConstruct = false;
-
   _options: SamlConfig & MultiSamlConfig;
 
   constructor(options: MultiSamlConfig, verify: VerifyWithRequest);
@@ -63,7 +61,6 @@ class MultiSamlStrategy extends SamlStrategy {
     });
   }
 
-  /** @ts-expect-error typescript disallows changing method signature in a subclass */
   generateServiceProviderMetadata(
     req: Request,
     decryptionCert: string | null,
@@ -84,7 +81,7 @@ class MultiSamlStrategy extends SamlStrategy {
       Object.setPrototypeOf(strategy, this);
       return callback(
         null,
-        super.generateServiceProviderMetadata.call(strategy, decryptionCert, signingCert)
+        this._generateServiceProviderMetadata.call(strategy, decryptionCert, signingCert)
       );
     });
   }
@@ -94,5 +91,3 @@ class MultiSamlStrategy extends SamlStrategy {
     super.error(err);
   }
 }
-
-export = MultiSamlStrategy;
diff --git a/src/passport-saml/strategy.ts b/src/passport-saml/strategy.ts
@@ -10,8 +10,8 @@ import {
 } from "./types";
 import { Profile } from "./types";
 
-class Strategy extends PassportStrategy {
-  static readonly newSamlProviderOnConstruct = true;
+export abstract class AbstractStrategy extends PassportStrategy {
+  static readonly newSamlProviderOnConstruct: boolean;
 
   name: string;
   _verify: VerifyWithRequest | VerifyWithoutRequest;
@@ -178,7 +178,7 @@ class Strategy extends PassportStrategy {
       .catch((err) => callback(err));
   }
 
-  generateServiceProviderMetadata(
+  protected _generateServiceProviderMetadata(
     decryptionCert: string | null,
     signingCert?: string | null
   ): string {
@@ -195,4 +195,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/passport-saml/multiSamlStrategy.spec.ts b/test/passport-saml/multiSamlStrategy.spec.ts
@@ -2,7 +2,7 @@
 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, AbstractStrategy } from "../../src/passport-saml";
 import {
   MultiSamlConfig,
   SamlOptionsCallback,
@@ -20,7 +20,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 +33,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 +154,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 +229,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");
   });