Fix multi saml strategy race conditions

node-saml#425
stavros-wb · Feb 26, 2020 · 6d0437d · 6d0437d
1 parent 63ba4dd
commit 6d0437d
Show file tree

Hide file tree

Showing 2 changed files with 62 additions and 26 deletions.
diff --git a/multiSamlStrategy.js b/multiSamlStrategy.js
@@ -1,20 +1,22 @@
 var util = require('util');
 var saml = require('./lib/passport-saml/saml');
-var InMemoryCacheProvider = require('./lib/passport-saml/inmemory-cache-provider').CacheProvider;
+var InMemoryCacheProvider = require('./lib/passport-saml/inmemory-cache-provider')
+  .CacheProvider;
 var SamlStrategy = require('./lib/passport-saml/strategy');
 
-function MultiSamlStrategy (options, verify) {
+function MultiSamlStrategy(options, verify) {
   if (!options || typeof options.getSamlOptions != 'function') {
     throw new Error('Please provide a getSamlOptions function');
   }
 
-  if(!options.requestIdExpirationPeriodMs){
-    options.requestIdExpirationPeriodMs = 28800000;  // 8 hours
+  if (!options.requestIdExpirationPeriodMs) {
+    options.requestIdExpirationPeriodMs = 28800000; // 8 hours
   }
 
-  if(!options.cacheProvider){
-      options.cacheProvider = new InMemoryCacheProvider(
-          {keyExpirationPeriodMs: options.requestIdExpirationPeriodMs });
+  if (!options.cacheProvider) {
+    options.cacheProvider = new InMemoryCacheProvider({
+      keyExpirationPeriodMs: options.requestIdExpirationPeriodMs,
+    });
   }
 
   SamlStrategy.call(this, options, verify);
@@ -23,46 +25,72 @@ function MultiSamlStrategy (options, verify) {
 
 util.inherits(MultiSamlStrategy, SamlStrategy);
 
-MultiSamlStrategy.prototype.authenticate = function (req, options) {
+MultiSamlStrategy.prototype.authenticate = function(req, options) {
   var self = this;
 
-  this._options.getSamlOptions(req, function (err, samlOptions) {
+  this._options.getSamlOptions(req, function(err, samlOptions) {
     if (err) {
       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);
   });
 };
 
-MultiSamlStrategy.prototype.logout = function (req, callback) {
+MultiSamlStrategy.prototype.logout = function(req, callback) {
   var self = this;
 
-  this._options.getSamlOptions(req, function (err, samlOptions) {
+  this._options.getSamlOptions(req, function(err, samlOptions) {
     if (err) {
       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);
   });
 };
 
-MultiSamlStrategy.prototype.generateServiceProviderMetadata = function( req, decryptionCert, signingCert, callback ) {
+MultiSamlStrategy.prototype.generateServiceProviderMetadata = function(
+  req,
+  decryptionCert,
+  signingCert,
+  callback,
+) {
   if (typeof callback !== 'function') {
-    throw new Error("Metadata can't be provided synchronously for MultiSamlStrategy.");
+    throw new Error(
+      "Metadata can't be provided synchronously for MultiSamlStrategy.",
+    );
   }
 
   var self = this;
 
-  return this._options.getSamlOptions(req, function (err, samlOptions) {
+  return this._options.getSamlOptions(req, function(err, samlOptions) {
     if (err) {
       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,
+      ),
+    );
   });
 };
 

diff --git a/test/multiSamlStrategy.js b/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);