From 930082a87a213f607c3ab96ce604b7b7470a9237 Mon Sep 17 00:00:00 2001 From: Chris Barth Date: Tue, 13 Jun 2023 14:45:41 -0400 Subject: [PATCH] Clean up types (#813) --- src/index.ts | 2 + src/multiSamlStrategy.ts | 7 ++-- src/strategy.ts | 9 +++-- src/types.ts | 10 +++-- test/multiSamlStrategy.spec.ts | 73 +++++++++++++++++++--------------- test/strategy.spec.ts | 32 ++++++++------- test/types.ts | 2 - 7 files changed, 77 insertions(+), 58 deletions(-) diff --git a/src/index.ts b/src/index.ts index 9cb460d5..8df11b1a 100644 --- a/src/index.ts +++ b/src/index.ts @@ -6,6 +6,7 @@ import type { VerifyWithRequest, VerifyWithoutRequest, MultiStrategyConfig, + PassportSamlConfig, } from "./types"; export * from "@node-saml/node-saml"; @@ -18,4 +19,5 @@ export { VerifyWithRequest, VerifyWithoutRequest, MultiStrategyConfig, + PassportSamlConfig, }; diff --git a/src/multiSamlStrategy.ts b/src/multiSamlStrategy.ts index 9c2efe74..c2c18d2e 100644 --- a/src/multiSamlStrategy.ts +++ b/src/multiSamlStrategy.ts @@ -4,14 +4,15 @@ import { AuthenticateOptions, MultiStrategyConfig, RequestWithUser, + PassportSamlConfig, VerifyWithoutRequest, VerifyWithRequest, } from "./types"; -import { SAML, SamlConfig } from "@node-saml/node-saml"; +import { SAML } from "."; export class MultiSamlStrategy extends AbstractStrategy { static readonly newSamlProviderOnConstruct = false; - _options: SamlConfig & MultiStrategyConfig; + _options: PassportSamlConfig & MultiStrategyConfig; constructor( options: MultiStrategyConfig, @@ -33,7 +34,7 @@ export class MultiSamlStrategy extends AbstractStrategy { // and there are defaults for all `strategy`-required options. const samlConfig = { ...options, - } as SamlConfig & MultiStrategyConfig; + } as PassportSamlConfig & MultiStrategyConfig; super(samlConfig, signonVerify, logoutVerify); this._options = samlConfig; diff --git a/src/strategy.ts b/src/strategy.ts index 9fad1fa5..7ad81dad 100644 --- a/src/strategy.ts +++ b/src/strategy.ts @@ -1,7 +1,8 @@ import { Strategy as PassportStrategy } from "passport-strategy"; import { strict as assert } from "assert"; import * as url from "url"; -import { Profile, SAML, SamlConfig } from "."; +import { Profile, SAML } from "."; +import { PassportSamlConfig } from "./types"; import { AuthenticateOptions, RequestWithUser, @@ -22,16 +23,16 @@ export abstract class AbstractStrategy extends PassportStrategy { _passReqToCallback?: boolean; constructor( - options: SamlConfig, + options: PassportSamlConfig, signonVerify: VerifyWithRequest, logoutVerify: VerifyWithRequest ); constructor( - options: SamlConfig, + options: PassportSamlConfig, signonVerify: VerifyWithoutRequest, logoutVerify: VerifyWithoutRequest ); - constructor(options: SamlConfig, signonVerify: never, logoutVerify: never) { + constructor(options: PassportSamlConfig, signonVerify: never, logoutVerify: never) { super(); if (typeof options === "function") { throw new Error("Mandatory SAML options missing"); diff --git a/src/types.ts b/src/types.ts index 94455695..c66735f4 100644 --- a/src/types.ts +++ b/src/types.ts @@ -1,6 +1,6 @@ import type * as express from "express"; import * as passport from "passport"; -import { Profile, SamlConfig } from "@node-saml/node-saml"; +import { Profile, SamlConfig } from "."; export interface AuthenticateOptions extends passport.AuthenticateOptions { samlFallback?: "login-request" | "logout-request"; @@ -37,13 +37,17 @@ export type VerifyWithRequest = ( export type VerifyWithoutRequest = (profile: Profile | null, done: VerifiedCallback) => void; -export type StrategyOptionsCallback = (err: Error | null, samlOptions?: SamlConfig) => void; +export type PassportSamlConfig = SamlConfig & StrategyOptions; + +export type StrategyOptionsCallback = (err: Error | null, samlOptions?: PassportSamlConfig) => void; interface BaseMultiStrategyConfig { getSamlOptions(req: express.Request, callback: StrategyOptionsCallback): void; } -export type MultiStrategyConfig = Partial & StrategyOptions & BaseMultiStrategyConfig; +export type MultiStrategyConfig = Partial & + StrategyOptions & + BaseMultiStrategyConfig; export class ErrorWithXmlStatus extends Error { constructor(message: string, public readonly xmlStatus: string) { diff --git a/test/multiSamlStrategy.spec.ts b/test/multiSamlStrategy.spec.ts index 81ede76c..0949f655 100644 --- a/test/multiSamlStrategy.spec.ts +++ b/test/multiSamlStrategy.spec.ts @@ -3,16 +3,21 @@ import * as express from "express"; import { Strategy } from "passport-strategy"; import * as sinon from "sinon"; import { expect } from "chai"; -import { MultiSamlStrategy, SAML, AbstractStrategy, SamlConfig } from "../src"; -import { MultiStrategyConfig, RequestWithUser, StrategyOptionsCallback } from "../src/types"; -import assert = require("assert"); +import { MultiSamlStrategy, SAML, AbstractStrategy } from "../src"; +import { + MultiStrategyConfig, + RequestWithUser, + StrategyOptionsCallback, + PassportSamlConfig, +} from "../src/types"; +import * as assert from "assert"; import { FAKE_CERT } from "./types"; const noop = () => undefined; describe("MultiSamlStrategy()", function () { it("extends passport Strategy", function () { - function getSamlOptions(): SamlConfig { + function getSamlOptions(): PassportSamlConfig { return { cert: FAKE_CERT, issuer: "onesaml_login" }; } const strategy = new MultiSamlStrategy({ getSamlOptions }, noop, noop); @@ -55,7 +60,8 @@ describe("MultiSamlStrategy()", function () { noop, noop ); - strategy.authenticate("random" as any, "random" as any); + // @ts-expect-error + strategy.authenticate("random", "random"); }); it("passes options on to saml strategy", function (done) { @@ -64,7 +70,7 @@ describe("MultiSamlStrategy()", function () { getSamlOptions: function (req: express.Request, fn: StrategyOptionsCallback) { try { fn(null, { cert: FAKE_CERT, issuer: "onesaml_login" }); - expect(strategy._passReqToCallback!).to.equal(true); + expect(strategy._passReqToCallback).to.equal(true); done(); } catch (err2) { done(err2); @@ -73,12 +79,13 @@ describe("MultiSamlStrategy()", function () { }; const strategy = new MultiSamlStrategy(passportOptions, noop, noop); - strategy.authenticate("random" as any, "random" as any); + // @ts-expect-error + strategy.authenticate("random", "random"); }); it("uses given options to setup internal saml provider", function (done) { const superAuthenticateStub = this.superAuthenticateStub; - const samlOptions: SamlConfig = { + const samlOptions: PassportSamlConfig = { issuer: "http://foo.issuer", callbackUrl: "http://foo.callback", cert: "deadbeef", @@ -104,11 +111,13 @@ describe("MultiSamlStrategy()", function () { } const strategy = new MultiSamlStrategy( - { getSamlOptions, cacheProvider: "mock cache provider" as any }, + // @ts-expect-error + { getSamlOptions, cacheProvider: "mock cache provider" }, noop, noop ); - strategy.authenticate("random" as any, "random" as any); + // @ts-expect-error + strategy.authenticate("random", "random"); }); }); @@ -172,7 +181,8 @@ describe("MultiSamlStrategy()", function () { } const strategy = new MultiSamlStrategy({ getSamlOptions }, noop, noop); - strategy.logout("random" as any, "random" as any); + // @ts-expect-error + strategy.logout("random", "random"); }); it("passes options on to saml strategy", function (done) { @@ -181,7 +191,7 @@ describe("MultiSamlStrategy()", function () { getSamlOptions: function (req: express.Request, fn: StrategyOptionsCallback) { try { fn(null, { cert: FAKE_CERT, issuer: "onesaml_login" }); - expect(strategy._passReqToCallback!).to.equal(true); + expect(strategy._passReqToCallback).to.equal(true); done(); } catch (err2) { done(err2); @@ -190,12 +200,13 @@ describe("MultiSamlStrategy()", function () { }; const strategy = new MultiSamlStrategy(passportOptions, noop, noop); - strategy.logout("random" as any, "random" as any); + // @ts-expect-error + strategy.logout("random", "random"); }); it("uses given options to setup internal saml provider", function (done) { const superLogoutMock = this.superLogoutMock; - const samlOptions: SamlConfig = { + const samlOptions: PassportSamlConfig = { issuer: "http://foo.issuer", callbackUrl: "http://foo.callback", authnRequestBinding: "HTTP-POST", @@ -220,7 +231,8 @@ describe("MultiSamlStrategy()", function () { } const strategy = new MultiSamlStrategy({ getSamlOptions }, noop, noop); - strategy.logout("random" as any, sinon.spy()); + // @ts-expect-error + strategy.logout("random", sinon.spy()); }); }); @@ -250,7 +262,8 @@ describe("MultiSamlStrategy()", function () { } const strategy = new MultiSamlStrategy({ getSamlOptions }, noop, noop); - strategy.generateServiceProviderMetadata("foo" as any, "bar", "baz", noop); + // @ts-expect-error + strategy.generateServiceProviderMetadata("foo", "bar", "baz", noop); }); it("passes options on to saml strategy", function (done) { @@ -260,7 +273,7 @@ describe("MultiSamlStrategy()", function () { getSamlOptions: function (req: express.Request, fn: StrategyOptionsCallback) { try { fn(null, { cert: FAKE_CERT, issuer: "onesaml_login" }); - expect(strategy._passReqToCallback!).to.equal(true); + expect(strategy._passReqToCallback).to.equal(true); done(); } catch (err2) { done(err2); @@ -269,7 +282,8 @@ describe("MultiSamlStrategy()", function () { }; const strategy = new MultiSamlStrategy(passportOptions, noop, noop); - strategy.generateServiceProviderMetadata("foo" as any, "bar", "baz", noop); + // @ts-expect-error + strategy.generateServiceProviderMetadata("foo", "bar", "baz", noop); }); it("should pass error to callback function", function (done) { @@ -280,19 +294,15 @@ describe("MultiSamlStrategy()", function () { }; const strategy = new MultiSamlStrategy(passportOptions, noop, noop); - strategy.generateServiceProviderMetadata( - "foo" as any, - "bar", - "baz", - function (error, result) { - try { - expect(error?.message).to.equal("My error"); - done(); - } catch (err2) { - done(err2); - } + // @ts-expect-error + strategy.generateServiceProviderMetadata("foo", "bar", "baz", function (error) { + try { + expect(error?.message).to.equal("My error"); + done(); + } catch (err2) { + done(err2); } - ); + }); }); it("should pass result to callback function", function (done) { @@ -304,7 +314,8 @@ describe("MultiSamlStrategy()", function () { const strategy = new MultiSamlStrategy(passportOptions, noop, noop); strategy.generateServiceProviderMetadata( - "foo" as any, + // @ts-expect-error + "foo", "bar", "baz", function (error, result) { diff --git a/test/strategy.spec.ts b/test/strategy.spec.ts index cc834a9b..f18003e5 100644 --- a/test/strategy.spec.ts +++ b/test/strategy.spec.ts @@ -3,8 +3,13 @@ import type * as express from "express"; import { expect } from "chai"; import * as sinon from "sinon"; -import { Profile, SAML, SamlConfig, Strategy as SamlStrategy } from "../src"; -import { RequestWithUser, VerifiedCallback, VerifyWithoutRequest } from "../src/types"; +import { Profile, SAML, Strategy as SamlStrategy } from "../src"; +import { + RequestWithUser, + VerifiedCallback, + VerifyWithoutRequest, + PassportSamlConfig, +} from "../src/types"; import { FAKE_CERT } from "./types"; const noop = () => undefined; @@ -304,10 +309,13 @@ describe("Strategy()", function () { }); it("should call through to get logout URL", function () { - // @ts-ignore - new SamlStrategy({ cert: FAKE_CERT, issuer: "onesaml_login" }, noop, noop).logout({ - query: "", - }); + new SamlStrategy({ cert: FAKE_CERT, issuer: "onesaml_login" }, noop, noop).logout( + { + // @ts-expect-error + query: "", + }, + noop + ); sinon.assert.calledOnce(getLogoutUrlAsyncStub); }); }); @@ -327,18 +335,12 @@ describe("Strategy()", function () { }); it("should call through to generate metadata", function () { - const samlConfig: SamlConfig = { cert: FAKE_CERT, issuer: "onesaml_login" }; - const signonVerify: VerifyWithoutRequest = function ( - _profile: Profile | null, - cb: VerifiedCallback - ): void { + const samlConfig: PassportSamlConfig = { cert: FAKE_CERT, issuer: "onesaml_login" }; + const signonVerify: VerifyWithoutRequest = function (): void { throw Error("This shouldn't be called to generate metadata"); }; - const logoutVerify: VerifyWithoutRequest = function ( - _profile: Profile | null, - cb: VerifiedCallback - ): void { + const logoutVerify: VerifyWithoutRequest = function (): void { throw Error("This shouldn't be called to generate metadata"); }; new SamlStrategy(samlConfig, signonVerify, logoutVerify).generateServiceProviderMetadata(""); diff --git a/test/types.ts b/test/types.ts index 6941418a..633487f8 100644 --- a/test/types.ts +++ b/test/types.ts @@ -1,5 +1,3 @@ -import { SamlConfig } from "../src"; - // a certificate which is re-used by several tests export const TEST_CERT = "MIIEFzCCAv+gAwIBAgIUFJsUjPM7AmWvNtEvULSHlTTMiLQwDQYJKoZIhvcNAQEFBQAwWDELMAkGA1UEBhMCVVMxETAPBgNVBAoMCFN1YnNwYWNlMRUwEwYDVQQLDAxPbmVMb2dpbiBJZFAxHzAdBgNVBAMMFk9uZUxvZ2luIEFjY291bnQgNDIzNDkwHhcNMTQwNTEzMTgwNjEyWhcNMTkwNTE0MTgwNjEyWjBYMQswCQYDVQQGEwJVUzERMA8GA1UECgwIU3Vic3BhY2UxFTATBgNVBAsMDE9uZUxvZ2luIElkUDEfMB0GA1UEAwwWT25lTG9naW4gQWNjb3VudCA0MjM0OTCCASIwDQYJKoZIhvcNAQEBBQADggEPADCCAQoCggEBAKrAzJdY9FzFLt5blArJfPzgi87EnFGlTfcV5T1TUDwLBlDkY/0ZGKnMOpf3D7ie2C4pPFOImOogcM5kpDDL7qxTXZ1ewXVyjBdMu29NG2C6NzWeQTUMUji01EcHkC8o+Pts8ANiNOYcjxEeyhEyzJKgEizblYzMMKzdrOET6QuqWo3C83K+5+5dsjDn1ooKGRwj3HvgsYcFrQl9NojgQFjoobwsiE/7A+OJhLpBcy/nSVgnoJaMfrO+JsnukZPztbntLvOl56+Vra0N8n5NAYhaSayPiv/ayhjVgjfXd1tjMVTOiDknUOwizZuJ1Y3QH94vUtBgp0WBpBSs/xMyTs8CAwEAAaOB2DCB1TAMBgNVHRMBAf8EAjAAMB0GA1UdDgQWBBRQO4WpM5fWwxib49WTuJkfYDbxODCBlQYDVR0jBIGNMIGKgBRQO4WpM5fWwxib49WTuJkfYDbxOKFcpFowWDELMAkGA1UEBhMCVVMxETAPBgNVBAoMCFN1YnNwYWNlMRUwEwYDVQQLDAxPbmVMb2dpbiBJZFAxHzAdBgNVBAMMFk9uZUxvZ2luIEFjY291bnQgNDIzNDmCFBSbFIzzOwJlrzbRL1C0h5U0zIi0MA4GA1UdDwEB/wQEAwIHgDANBgkqhkiG9w0BAQUFAAOCAQEACdDAAoaZFCEY5pmfwbKuKrXtO5iE8lWtiCPjCZEUuT6bXRNcqrdnuV/EAfX9WQoXjalPi0eM78zKmbvRGSTUHwWw49RHjFfeJUKvHNeNnFgTXDjEPNhMvh69kHm453lFRmB+kk6yjtXRZaQEwS8Uuo2Ot+krgNbl6oTBZJ0AHH1MtZECDloms1Km7zsK8wAi5i8TVIKkVr5b2VlhrLgFMvzZ5ViAxIMGB6w47yY4QGQB/5Q8ya9hBs9vkn+wubA+yr4j14JXZ7blVKDSTYva65Ea+PqHyrp+Wnmnbw2ObS7iWexiTy1jD3G0R2avDBFjM8Fj5DbfufsE1b0U10RTtg==";