From 42f38997587046a1ad55f6be0f1dbd5229b1c0d7 Mon Sep 17 00:00:00 2001 From: Mike Hesler Date: Mon, 1 Feb 2021 11:44:10 -0500 Subject: [PATCH 1/6] allow for authnRequestBinding in SAML options --- src/passport-saml/saml.ts | 2 ++ src/passport-saml/strategy.ts | 4 +--- src/passport-saml/types.ts | 2 +- test/multiSamlStrategy.js | 6 ------ 4 files changed, 4 insertions(+), 10 deletions(-) diff --git a/src/passport-saml/saml.ts b/src/passport-saml/saml.ts index 267bea7b..e2a143dd 100644 --- a/src/passport-saml/saml.ts +++ b/src/passport-saml/saml.ts @@ -189,6 +189,8 @@ class SAML { options.RACComparison = 'exact'; } + options.authnRequestBinding = options.authnRequestBinding || 'HTTP-Redirect'; + return options as SAMLOptions; } diff --git a/src/passport-saml/strategy.ts b/src/passport-saml/strategy.ts index f8cb12ba..6b0bbd5e 100644 --- a/src/passport-saml/strategy.ts +++ b/src/passport-saml/strategy.ts @@ -9,7 +9,6 @@ class Strategy extends PassportStrategy { _verify: VerifyWithRequest | VerifyWithoutRequest; _saml: saml.SAML; _passReqToCallback?: boolean; - _authnRequestBinding?: string; constructor(options: SamlConfig, verify: VerifyWithRequest | VerifyWithoutRequest) { super(); @@ -34,7 +33,6 @@ class Strategy extends PassportStrategy { this._verify = verify; this._saml = new saml.SAML(options); this._passReqToCallback = !!options.passReqToCallback; - this._authnRequestBinding = options.authnRequestBinding || 'HTTP-Redirect'; } authenticate(req: RequestWithUser, options: AuthenticateOptions & AuthorizeOptions): void { @@ -92,7 +90,7 @@ class Strategy extends PassportStrategy { } else { const requestHandler = { 'login-request': () => { - if (this._authnRequestBinding === 'HTTP-POST') { + if (this._saml.options.authnRequestBinding === 'HTTP-POST') { this._saml.getAuthorizeForm(req, (err: Error | null, data?: any) => { if (err) { this.error(err); diff --git a/src/passport-saml/types.ts b/src/passport-saml/types.ts index ffe1f4ea..889ed604 100644 --- a/src/passport-saml/types.ts +++ b/src/passport-saml/types.ts @@ -37,6 +37,7 @@ export interface SAMLOptions { authnContext: string | string[]; forceAuthn: boolean; skipRequestCompression: boolean; + authnRequestBinding?: string; RACComparison: 'exact' | 'minimum' | 'maximum' | 'better'; providerName: string; passive: boolean; @@ -65,7 +66,6 @@ export type SamlConfig = Partial & StrategyOptions interface StrategyOptions { name?: string; passReqToCallback?: boolean; - authnRequestBinding?: string; } export interface SamlScopingConfig { diff --git a/test/multiSamlStrategy.js b/test/multiSamlStrategy.js index 95419eee..5a7b23a0 100644 --- a/test/multiSamlStrategy.js +++ b/test/multiSamlStrategy.js @@ -50,12 +50,10 @@ describe('strategy#authenticate', function() { it('passes options on to saml strategy', function(done) { var passportOptions = { passReqToCallback: true, - authnRequestBinding: 'HTTP-POST', getSamlOptions: function (req, fn) { try { fn(); strategy._passReqToCallback.should.eql(true); - strategy._authnRequestBinding.should.eql('HTTP-POST'); done(); } catch (err2) { done(err2); @@ -133,12 +131,10 @@ describe('strategy#logout', function() { it('passes options on to saml strategy', function(done) { var passportOptions = { passReqToCallback: true, - authnRequestBinding: 'HTTP-POST', getSamlOptions: function (req, fn) { try { fn(); strategy._passReqToCallback.should.eql(true); - strategy._authnRequestBinding.should.eql('HTTP-POST'); done(); } catch (err2) { done(err2); @@ -218,12 +214,10 @@ describe('strategy#generateServiceProviderMetadata', function() { it('passes options on to saml strategy', function(done) { var passportOptions = { passReqToCallback: true, - authnRequestBinding: 'HTTP-POST', getSamlOptions: function (req, fn) { try { fn(); strategy._passReqToCallback.should.eql(true); - strategy._authnRequestBinding.should.eql('HTTP-POST'); done(); } catch (err2) { done(err2); From 9f28d9c491e4408c7b0e620580c67a007c507da1 Mon Sep 17 00:00:00 2001 From: Mike Hesler Date: Mon, 1 Feb 2021 14:02:10 -0500 Subject: [PATCH 2/6] update tests --- test/multiSamlStrategy.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/multiSamlStrategy.js b/test/multiSamlStrategy.js index 5a7b23a0..43f70133 100644 --- a/test/multiSamlStrategy.js +++ b/test/multiSamlStrategy.js @@ -69,6 +69,7 @@ describe('strategy#authenticate', function() { var superAuthenticateStub = this.superAuthenticateStub; var samlOptions = { issuer: 'http://foo.issuer', + authnRequestBinding: 'HTTP-POST', callbackUrl: 'http://foo.callback', cert: 'deadbeef', host: 'lvh', @@ -150,6 +151,7 @@ describe('strategy#logout', function() { var superLogoutMock = this.superLogoutMock; var samlOptions = { issuer: 'http://foo.issuer', + authnRequestBinding: 'HTTP-POST', callbackUrl: 'http://foo.callback', cert: 'deadbeef', host: 'lvh', From c81575bc71a360b034ebe762cb9960b6f1e5a217 Mon Sep 17 00:00:00 2001 From: Mike Hesler Date: Wed, 3 Feb 2021 13:50:19 -0500 Subject: [PATCH 3/6] fix quote styling - incorrectly merged --- src/passport-saml/strategy.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/passport-saml/strategy.ts b/src/passport-saml/strategy.ts index 976db0c4..8e42287b 100644 --- a/src/passport-saml/strategy.ts +++ b/src/passport-saml/strategy.ts @@ -98,7 +98,7 @@ class Strategy extends PassportStrategy { this._saml.validatePostRequest(req.body, validateCallback); } else { const requestHandler = { - 'login-request': () => { + "login-request": () => { if (this._saml.options.authnRequestBinding === "HTTP-POST") { this._saml.getAuthorizeForm(req, (err: Error | null, data?: any) => { if (err) { From 9d359d3edd91ae317cc2fc9767e1d43f730d9554 Mon Sep 17 00:00:00 2001 From: Mike Hesler Date: Wed, 3 Feb 2021 16:19:17 -0500 Subject: [PATCH 4/6] wip --- test/multiSamlStrategy.js | 29 +++++++++++++++++++--- test/strategy.js | 52 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 78 insertions(+), 3 deletions(-) create mode 100644 test/strategy.js diff --git a/test/multiSamlStrategy.js b/test/multiSamlStrategy.js index e715f20e..d308fe94 100644 --- a/test/multiSamlStrategy.js +++ b/test/multiSamlStrategy.js @@ -2,12 +2,13 @@ var sinon = require("sinon"); var should = require("should"); +var saml = require("../lib/passport-saml/saml.js"); var SamlStrategy = require("../lib/passport-saml/index.js").Strategy; -var MultiSamlStrategy = require("../multiSamlStrategy"); +var MultiSamlStrategy = require("../lib/passport-saml/multiSamlStrategy.js"); function verify() {} -describe("Strategy()", function () { +describe("MultiSamlStrategy()", function () { it("extends passport Strategy", function () { function getSamlOptions() { return {}; @@ -24,13 +25,17 @@ describe("Strategy()", function () { }); }); -describe("strategy#authenticate", function () { +describe.only("strategy#authenticate", function () { beforeEach(function () { this.superAuthenticateStub = sinon.stub(SamlStrategy.prototype, "authenticate"); + this.getAuthorizeFormStub = sinon.stub(saml.SAML.prototype, "getAuthorizeForm"); + this.getAuthorizeUrlStub = sinon.stub(saml.SAML.prototype, "getAuthorizeUrl"); }); afterEach(function () { this.superAuthenticateStub.restore(); + this.getAuthorizeFormStub.restore(); + this.getAuthorizeUrlStub.restore(); }); it("calls super with request and auth options", function (done) { @@ -106,6 +111,24 @@ describe("strategy#authenticate", function () { ); strategy.authenticate(); }); + + it("calls getAuthorizeForm when authnRequestBinding is HTTP-POST", function () { + function getSamlOptions(req, fn) { + fn(null, { authnRequestBinding: "HTTP-POST" }); + } + var strategy = new MultiSamlStrategy({ getSamlOptions }, verify); + strategy.authenticate({}, {}); + sinon.assert.calledOnce(this.getAuthorizeFormStub); + }) + + it("calls getAuthorizeUrl when authnRequestBinding is not HTTP-POST", function () { + function getSamlOptions(req, fn) { + fn(null, {}); + } + var strategy = new MultiSamlStrategy({ getSamlOptions }, verify); + strategy.authenticate({}, {}); + sinon.assert.calledOnce(this.getAuthorizeUrlStub); + }) }); describe("strategy#logout", function () { diff --git a/test/strategy.js b/test/strategy.js new file mode 100644 index 00000000..c7a0a001 --- /dev/null +++ b/test/strategy.js @@ -0,0 +1,52 @@ +"use strict"; + +var sinon = require("sinon"); +var saml = require("../lib/passport-saml/saml.js"); +var SamlStrategy = require("../lib/passport-saml/index.js").Strategy; +var MultiSamlStrategy = require("../lib/passport-saml/multiSamlStrategy.js"); + +function verify() {} + +describe.only("strategy#authenticate", function () { + beforeEach(function () { + this.getAuthorizeFormStub = sinon.stub(saml.SAML.prototype, "getAuthorizeForm"); + this.getAuthorizeUrlStub = sinon.stub(saml.SAML.prototype, "getAuthorizeUrl"); + }); + + afterEach(function () { + this.getAuthorizeFormStub.restore(); + this.getAuthorizeUrlStub.restore(); + }); + + it("calls getAuthorizeForm when authnRequestBinding is HTTP-POST", function () { + var strategy = new SamlStrategy({ + authnRequestBinding: "HTTP-POST" + }, verify); + strategy.authenticate({}, {}); + sinon.assert.calledOnce(this.getAuthorizeFormStub); + }) + + it("calls getAuthorizeUrl when authnRequestBinding is not HTTP-POST", function () { + var strategy = new SamlStrategy({}, verify); + strategy.authenticate({}, {}); + sinon.assert.calledOnce(this.getAuthorizeUrlStub); + }) + + it("calls getAuthorizeForm when authnRequestBinding is HTTP-POST", function () { + function getSamlOptions(req, fn) { + fn(null, { authnRequestBinding: "HTTP-POST" }); + } + var strategy = new MultiSamlStrategy({ getSamlOptions }, verify); + strategy.authenticate({}, {}); + sinon.assert.calledOnce(this.getAuthorizeFormStub); + }) + + it("calls getAuthorizeUrl when authnRequestBinding is not HTTP-POST", function () { + function getSamlOptions(req, fn) { + fn(null, {}); + } + var strategy = new MultiSamlStrategy({ getSamlOptions }, verify); + strategy.authenticate({}, {}); + sinon.assert.calledOnce(this.getAuthorizeUrlStub); + }) +}) \ No newline at end of file From e42a001eec5e5f4ec7b455d2f843485e1e433bcf Mon Sep 17 00:00:00 2001 From: Chris Barth Date: Mon, 15 Feb 2021 16:54:46 -0500 Subject: [PATCH 5/6] Fix tests --- src/passport-saml/saml.ts | 2 +- test/multiSamlStrategy.js | 22 ++++++++---- test/strategy.js | 72 +++++++++++++++------------------------ 3 files changed, 44 insertions(+), 52 deletions(-) diff --git a/src/passport-saml/saml.ts b/src/passport-saml/saml.ts index 50d547a9..f06c48be 100644 --- a/src/passport-saml/saml.ts +++ b/src/passport-saml/saml.ts @@ -198,7 +198,7 @@ class SAML { options.RACComparison = "exact"; } - options.authnRequestBinding = options.authnRequestBinding || 'HTTP-Redirect'; + options.authnRequestBinding = options.authnRequestBinding || "HTTP-Redirect"; return options as SAMLOptions; } diff --git a/test/multiSamlStrategy.js b/test/multiSamlStrategy.js index d308fe94..6bd9cba4 100644 --- a/test/multiSamlStrategy.js +++ b/test/multiSamlStrategy.js @@ -25,17 +25,13 @@ describe("MultiSamlStrategy()", function () { }); }); -describe.only("strategy#authenticate", function () { +describe("MultiSamlStrategy#authenticate", function () { beforeEach(function () { this.superAuthenticateStub = sinon.stub(SamlStrategy.prototype, "authenticate"); - this.getAuthorizeFormStub = sinon.stub(saml.SAML.prototype, "getAuthorizeForm"); - this.getAuthorizeUrlStub = sinon.stub(saml.SAML.prototype, "getAuthorizeUrl"); }); afterEach(function () { this.superAuthenticateStub.restore(); - this.getAuthorizeFormStub.restore(); - this.getAuthorizeUrlStub.restore(); }); it("calls super with request and auth options", function (done) { @@ -111,6 +107,18 @@ describe.only("strategy#authenticate", function () { ); strategy.authenticate(); }); +}); + +describe("MultiSamlStrategy#authorize", function () { + beforeEach(function () { + this.getAuthorizeFormStub = sinon.stub(saml.SAML.prototype, "getAuthorizeForm"); + this.getAuthorizeUrlStub = sinon.stub(saml.SAML.prototype, "getAuthorizeUrl"); + }); + + afterEach(function () { + this.getAuthorizeFormStub.restore(); + this.getAuthorizeUrlStub.restore(); + }); it("calls getAuthorizeForm when authnRequestBinding is HTTP-POST", function () { function getSamlOptions(req, fn) { @@ -119,7 +127,7 @@ describe.only("strategy#authenticate", function () { var strategy = new MultiSamlStrategy({ getSamlOptions }, verify); strategy.authenticate({}, {}); sinon.assert.calledOnce(this.getAuthorizeFormStub); - }) + }); it("calls getAuthorizeUrl when authnRequestBinding is not HTTP-POST", function () { function getSamlOptions(req, fn) { @@ -128,7 +136,7 @@ describe.only("strategy#authenticate", function () { var strategy = new MultiSamlStrategy({ getSamlOptions }, verify); strategy.authenticate({}, {}); sinon.assert.calledOnce(this.getAuthorizeUrlStub); - }) + }); }); describe("strategy#logout", function () { diff --git a/test/strategy.js b/test/strategy.js index c7a0a001..06f9c4eb 100644 --- a/test/strategy.js +++ b/test/strategy.js @@ -3,50 +3,34 @@ var sinon = require("sinon"); var saml = require("../lib/passport-saml/saml.js"); var SamlStrategy = require("../lib/passport-saml/index.js").Strategy; -var MultiSamlStrategy = require("../lib/passport-saml/multiSamlStrategy.js"); function verify() {} -describe.only("strategy#authenticate", function () { - beforeEach(function () { - this.getAuthorizeFormStub = sinon.stub(saml.SAML.prototype, "getAuthorizeForm"); - this.getAuthorizeUrlStub = sinon.stub(saml.SAML.prototype, "getAuthorizeUrl"); - }); - - afterEach(function () { - this.getAuthorizeFormStub.restore(); - this.getAuthorizeUrlStub.restore(); - }); - - it("calls getAuthorizeForm when authnRequestBinding is HTTP-POST", function () { - var strategy = new SamlStrategy({ - authnRequestBinding: "HTTP-POST" - }, verify); - strategy.authenticate({}, {}); - sinon.assert.calledOnce(this.getAuthorizeFormStub); - }) - - it("calls getAuthorizeUrl when authnRequestBinding is not HTTP-POST", function () { - var strategy = new SamlStrategy({}, verify); - strategy.authenticate({}, {}); - sinon.assert.calledOnce(this.getAuthorizeUrlStub); - }) - - it("calls getAuthorizeForm when authnRequestBinding is HTTP-POST", function () { - function getSamlOptions(req, fn) { - fn(null, { authnRequestBinding: "HTTP-POST" }); - } - var strategy = new MultiSamlStrategy({ getSamlOptions }, verify); - strategy.authenticate({}, {}); - sinon.assert.calledOnce(this.getAuthorizeFormStub); - }) - - it("calls getAuthorizeUrl when authnRequestBinding is not HTTP-POST", function () { - function getSamlOptions(req, fn) { - fn(null, {}); - } - var strategy = new MultiSamlStrategy({ getSamlOptions }, verify); - strategy.authenticate({}, {}); - sinon.assert.calledOnce(this.getAuthorizeUrlStub); - }) -}) \ No newline at end of file +describe("strategy#authorize", function () { + beforeEach(function () { + this.getAuthorizeFormStub = sinon.stub(saml.SAML.prototype, "getAuthorizeForm"); + this.getAuthorizeUrlStub = sinon.stub(saml.SAML.prototype, "getAuthorizeUrl"); + }); + + afterEach(function () { + this.getAuthorizeFormStub.restore(); + this.getAuthorizeUrlStub.restore(); + }); + + it("calls getAuthorizeForm when authnRequestBinding is HTTP-POST", function () { + var strategy = new SamlStrategy( + { + authnRequestBinding: "HTTP-POST", + }, + verify + ); + strategy.authenticate({}, {}); + sinon.assert.calledOnce(this.getAuthorizeFormStub); + }); + + it("calls getAuthorizeUrl when authnRequestBinding is not HTTP-POST", function () { + var strategy = new SamlStrategy({}, verify); + strategy.authenticate({}, {}); + sinon.assert.calledOnce(this.getAuthorizeUrlStub); + }); +}); From 9b977d9b3ed309fc5cd8a2f37e71f1ce7646ad22 Mon Sep 17 00:00:00 2001 From: Chris Barth Date: Mon, 15 Feb 2021 17:00:33 -0500 Subject: [PATCH 6/6] Clear up test names for consistency --- test/multiSamlStrategy.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/multiSamlStrategy.js b/test/multiSamlStrategy.js index 6bd9cba4..b1e7a235 100644 --- a/test/multiSamlStrategy.js +++ b/test/multiSamlStrategy.js @@ -139,7 +139,7 @@ describe("MultiSamlStrategy#authorize", function () { }); }); -describe("strategy#logout", function () { +describe("MultiSamlStrategy#logout", function () { beforeEach(function () { this.superLogoutMock = sinon.stub(SamlStrategy.prototype, "logout"); }); @@ -213,7 +213,7 @@ describe("strategy#logout", function () { }); }); -describe("strategy#generateServiceProviderMetadata", function () { +describe("MultiSamlStrategy#generateServiceProviderMetadata", function () { beforeEach(function () { this.superGenerateServiceProviderMetadata = sinon .stub(SamlStrategy.prototype, "generateServiceProviderMetadata")