Fix multi saml strategy race conditions (#426)

* Fix multi saml strategy race conditions

#425

* Add warning for MultiSaml in Readme

README.md

@@ -79,6 +79,9 @@ You can override these defaults by passing a new value through the `getSamlOptio
 
 Using multiple providers supports `validateInResponseTo`, but all the `InResponse` values are stored on the same Cache. This means, if you're using the default `InMemoryCache`, that all providers have access to it and a provider might get its response validated against another's request. [Issue Report](!https://github.com/bergie/passport-saml/issues/334). To amend this you should provide a different cache provider per SAML provider, through the `getSamlOptions` function.
 
+> :warning: **There's a race condition [bug](https://github.com/bergie/passport-saml/issues/425) in versions < 1.3.3 which makes it vulnerable to DOS attacks**: Please use > 1.3.3 if you want to use this issue
+
+
 #### The profile object:
 
 The profile object referenced above contains the following:

multiSamlStrategy.js

@@ -31,8 +31,10 @@ MultiSamlStrategy.prototype.authenticate = function (req, options) {
       return self.error(err);
     }
 
-    self._saml = new saml.SAML(Object.assign({}, self._options, samlOptions));
-    self.constructor.super_.prototype.authenticate.call(self, req, options);
+    var samlService = new saml.SAML(Object.assign({}, self._options, samlOptions));
+    var strategy = Object.assign({}, self, {_saml: samlService});
+    Object.setPrototypeOf(strategy, self);
+    self.constructor.super_.prototype.authenticate.call(strategy, req, options);
   });
 };
 
@@ -44,8 +46,10 @@ MultiSamlStrategy.prototype.logout = function (req, callback) {
       return callback(err);
     }
 
-    self._saml = new saml.SAML(Object.assign({}, self._options, samlOptions));
-    self.constructor.super_.prototype.logout.call(self, req, callback);
+    var samlService = new saml.SAML(Object.assign({}, self._options, samlOptions));
+    var strategy = Object.assign({}, self, {_saml: samlService});
+    Object.setPrototypeOf(strategy, self);
+    self.constructor.super_.prototype.logout.call(strategy, req, callback);
   });
 };
 
@@ -61,8 +65,10 @@ MultiSamlStrategy.prototype.generateServiceProviderMetadata = function( req, dec
       return callback(err);
     }
 
-    self._saml = new saml.SAML(Object.assign({}, self._options, samlOptions));
-    return callback(null, self.constructor.super_.prototype.generateServiceProviderMetadata.call(self, decryptionCert, signingCert ));
+    var samlService = new saml.SAML(Object.assign({}, self._options, samlOptions));
+    var strategy = Object.assign({}, self, {_saml: samlService});
+    Object.setPrototypeOf(strategy, self);
+    return callback(null, self.constructor.super_.prototype.generateServiceProviderMetadata.call(strategy, decryptionCert, signingCert));
   });
 };
 

test/multiSamlStrategy.js

@@ -68,6 +68,7 @@ describe('strategy#authenticate', function() {
   });
 
   it('uses given options to setup internal saml provider', function(done) {
+    var superAuthenticateStub = this.superAuthenticateStub;
     var samlOptions = {
       issuer: 'http://foo.issuer',
       callbackUrl: 'http://foo.callback',
@@ -84,7 +85,9 @@ describe('strategy#authenticate', function() {
     function getSamlOptions (req, fn) {
       try {
         fn(null, samlOptions);
-        strategy._saml.options.should.containEql(Object.assign({},
+        sinon.assert.calledOnce(superAuthenticateStub)
+        superAuthenticateStub.calledWith(Object.assign(
+          {},
           { cacheProvider: 'mock cache provider' },
           samlOptions
         ));
@@ -104,19 +107,19 @@ describe('strategy#authenticate', function() {
 
 describe('strategy#logout', function() {
   beforeEach(function() {
-    this.superAuthenticateStub = sinon.stub(SamlStrategy.prototype, 'logout');
+    this.superLogoutMock = sinon.stub(SamlStrategy.prototype, 'logout');
   });
 
   afterEach(function() {
-    this.superAuthenticateStub.restore();
+    this.superLogoutMock.restore();
   });
 
   it('calls super with request and auth options', function(done) {
-    var superAuthenticateStub = this.superAuthenticateStub;
+    var superLogoutMock = this.superLogoutMock;
     function getSamlOptions (req, fn) {
       try {
         fn();
-        sinon.assert.calledOnce(superAuthenticateStub);
+        sinon.assert.calledOnce(superLogoutMock);
         done();
       } catch (err2) {
         done(err2);
@@ -148,6 +151,7 @@ describe('strategy#logout', function() {
   });
 
   it('uses given options to setup internal saml provider', function(done) {
+    var superLogoutMock = this.superLogoutMock;
     var samlOptions = {
       issuer: 'http://foo.issuer',
       callbackUrl: 'http://foo.callback',
@@ -164,7 +168,11 @@ describe('strategy#logout', function() {
     function getSamlOptions (req, fn) {
       try {
         fn(null, samlOptions);
-        strategy._saml.options.should.containEql(samlOptions);
+        sinon.assert.calledOnce(superLogoutMock)
+        superLogoutMock.calledWith(Object.assign(
+          {},
+          samlOptions
+        ));
         done();
       } catch (err2) {
         done(err2);