passReqToCallback: true is not compatible with @nestjs/passport

[insolite]

As mentioned in #6, the @nestjs/passport assumes that the last argument for verify is always a callback. And https://github.com/hydra-newmedia/passport-headerapikey/commit/70308832558ab22ec7c79fe5dfa9b5be59c2874b fixed the case for the passReqToCallback: false. But what about passReqToCallback: true? I guess it should call smth like

if (this.passReqToCallback) optionalCallbackParams.unshift(req);

instead of

if (this.passReqToCallback) optionalCallbackParams.push(req);

like passport-http-bearer do for example.

To me it looks weird that optional arg should be the leading arg, but that's how @nestjs/passport works, pehaps for a reason.

[guischdi]

Hi @insolite Pushing or unshifting to an empty array makes no difference. The array contains only this one element afterwards (if this.passReqToCallback is true).

---

I think, what you're trying to say is that we should change the order of the parameters when calling the verify function, eg. like this:

if (this.passReqToCallback) this.verify(req, apiKey, verified); // OR this.verify(apiKey, req, verified);
else this.verify(apiKey, verified);

This order is an implementation detail of the passport strategy. I can't find any specifications of which order is an de facto standard or which parameter should be last. If I change this order, this would be a breaking change to our API. My company has moved away from the express ecosystems for years, but the strategy should still be compatible, if one provides a correct verify function as per the docs. I don't know how NestJS handles passport strategies and where to provide the verify function, but this should be the way to solve your problem.

---

Let me know if you think, this breaking change should be necessary after all.

[insolite]

@guischdi Yes, you're right about the args order. I simply made a mistake in the suggestion, sorry. So the correct change might be like this:

this.verify(apiKey, verified, ...optionalCallbackParams);

=>

this.verify(...optionalCallbackParams, apiKey, verified);

So the actual monkey-patch that's currently working for me is:

diff --git a/node_modules/passport-headerapikey/lib/Strategy.js b/node_modules/passport-headerapikey/lib/Strategy.js
index 1edafa4..59976c0 100644
--- a/node_modules/passport-headerapikey/lib/Strategy.js
+++ b/node_modules/passport-headerapikey/lib/Strategy.js
@@ -65,7 +65,7 @@ var Strategy = /** @class */ (function (_super) {
         var optionalCallbackParams = [];
         if (this.passReqToCallback)
             optionalCallbackParams.push(req);
-        this.verify.apply(this, [apiKey, verified].concat(optionalCallbackParams));
+        this.verify.apply(this, optionalCallbackParams.concat([apiKey, verified]));
     };
     return Strategy;
 }(passport_strategy_1.Strategy));

Of course, that would be a breaking change. So I'd suggest releasing this by bumping the major version. But it's all up to you whether to accept it at all :slightly_smiling_face: I'm getting a bit lost in Passport docs too. So I'm not 100% sure if it's correct but it seems to be. Thanks for looking into this.

[guischdi]

Hi @insolite

as mentioned before, optional args at the beginning of a function signature are senseless in my opinion. Even more if the optional argument is not passed on default but only if a flag (passReqToCallback) is set. The implementation of the verify callback is not defined/standardized in the passport docs. So we can choose whichever count and order of arguments we want. So I will stick with our order and will not publish a new major version just for this edge case.

Explanation: I can't find any de facto standard for the argument order in the docs. The only similarity between the listed strategies seems to be that the done callback (named verified internally in our repo) is the last argument. But it seems to be a proper exception from this 'rule', that the req is only added when desired and therefor appended(!) to the parameters. I can't find any other standardization and if NestJS doesn't provide a possibility to pass a custom verify-callback (with your very own implementation of how you handle the arguments), it seems more like an issue with NestJS.