Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow returnURL to be function #3

Closed
wants to merge 1 commit into from

Conversation

razor-x
Copy link

@razor-x razor-x commented Aug 3, 2019

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
Copy link

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

@razor-x
Copy link
Author

razor-x commented Aug 5, 2019

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

@adamhathcock
Copy link

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

@intellix
Copy link

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);
     });

@razor-x razor-x closed this Aug 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants