Allow returnURL to be function

razor-x commented 5 years ago

I've verified this works and is backwards compatible, but the way _relyingParty is defined inside the constructor and then mocked in the tests, I don't actually see any simple way to update the tests to handle this simple (internal) change.

See related

adamhathcock commented 5 years ago

I appreciate updating the tests isn't simple but I can't accept this in a broken state.

razor-x commented 5 years ago

Right, I understand that 😄 . Looking for suggestions and ideas from anyone for a way that doesn't involve large refactoring of the internals.

adamhathcock commented 5 years ago

Passport isn't aging well as a codebase. A large refactoring might be due.

intellix commented 4 years ago

Since this won't get merged anytime soon, here's the patch-package (https://github.com/ds300/patch-package) with support for realm too:

patches/@passport-next+passport-openid+1.0.0.patch

diff --git a/node_modules/@passport-next/passport-openid/lib/passport-openid/strategy.js b/node_modules/@passport-next/passport-openid/lib/passport-openid/strategy.js
index 487ceba..5b049a7 100644
--- a/node_modules/@passport-next/passport-openid/lib/passport-openid/strategy.js
+++ b/node_modules/@passport-next/passport-openid/lib/passport-openid/strategy.js
@@ -137,9 +137,9 @@ function Strategy(options, verify) {
     extensions.push(oauth);
   }

-  this._relyingParty = new openid.RelyingParty(
-    options.returnURL,
-    options.realm,
+  this._relyingParty = req => new openid.RelyingParty(
+    options.returnURL instanceof Function ? options.returnURL(req) : options.returnURL,
+    options.realm instanceof Function ? options.realm(req) : options.realm,
     (options.stateless === undefined) ? false : options.stateless,
     (options.secure === undefined) ? true : options.secure,
     extensions);
@@ -180,7 +180,7 @@ Strategy.prototype.authenticate = function(req) {
     if (req.query['openid.mode'] === 'cancel') { return this.fail({ message: 'OpenID authentication canceled' }); }

     var self = this;
-    this._relyingParty.verifyAssertion(req.url, function(err, result) {
+    this._relyingParty(req).verifyAssertion(req.url, function(err, result) {
       if (err) { return self.error(new InternalOpenIDError('Failed to verify assertion', err)); }
       if (!result.authenticated) { return self.error(new Error('OpenID authentication failed')); }

@@ -245,7 +245,7 @@ Strategy.prototype.authenticate = function(req) {
     if (!identifier) { return this.fail(new BadRequestError('Missing OpenID identifier')); }

     var self = this;
-    this._relyingParty.authenticate(identifier, false, function(err, providerUrl) {
+    this._relyingParty(req).authenticate(identifier, false, function(err, providerUrl) {
       if (err || !providerUrl) { return self.error(new InternalOpenIDError('Failed to discover OP endpoint URL', err)); }
       self.redirect(providerUrl);
     });

passport-next / passport-openid

Allow returnURL to be function #3