validate function is called with the wrong number of arguments

[andelz]

I'm submitting a...

[ ] Regression 
[x ] Bug report
[ ] Feature request
[ ] Documentation issue or request
[ ] Support request => Please do not submit support request here, instead post your question on Stack Overflow.

Current behavior

When running @nestjs/passport in version 7.1.x I encountered an issue when using any passport library that relies on passport-oauth2 (in my case passport-google-oauth20@2.0.0 and passport-github-2@0.1.12). It looks like the number of arguments added to my validate does not match the number of returned values.

async validate (accessToken: string, refreshToken: string, profile: any, done: VerifyCallback) {...} will get me the result that I would expect when writing async validate (accessToken: string, refreshToken: string, params: any, profile: any, done: VerifyCallback) {...}. So the done param actually holds the profile data. Because of that calling the done callback at the end will fail, obviously.

After some investigations it looks like that the length of the params passed to the passport library is wrong. Taking a look at https://github.com/nestjs/passport/blob/master/lib/passport/passport.strategy.ts I realized that in line 31 you increase the number of the callbacks arguments by one. So if I provided 4 params in my validate function the passport lib will than assume that the function has 5 arguments and act accordingly.

Using @nestjs/passport@7.0.0 fixes the problem.

Expected behavior

The number of arguments of my validate function should match the number of returned values.

Minimal reproduction of the problem with instructions

I basically followed this example from scratch: https://dev.to/imichaelowolabi/how-to-implement-login-with-google-in-nest-js-2aoa

Environment

Nest version: 7.5.1

 
For Tooling issues:
- Node version: 14.14.0
- Platform:  Windows

Others:

[kamilmysliwiec]

Fixed in v7.1.4

[egormkn]

@kamilmysliwiec I think that it would be better to revert that +1 and suggest @andelz to remove done argument, as this callback is executed internally by nestjs/passport with the value (user) returned from validate. Also this behaviour should be documented. In v7.1.4 it is required to add done to validate method arguments even if it is not used (as it normally should be, according to the docs (that done callback was not even mentioned there)). Actually, this issue is a result of wrong fix of the length bug, that was copied from some tutorials 😞

For example, with incremented length, instead of

async validate (accessToken: string, refreshToken: string, profile: any, done: VerifyCallback) {
  ...
  done(null, user);
  ...
}

it will be better to use

async validate (accessToken: string, refreshToken: string, profile: any) {
  ...
  return user;
  ...
}

[kamilmysliwiec]

@egormkn it seems that explicitly setting the length property to make sure we're compatible with strategies that check the number of arguments, introduced a breaking change leading to tons of unexpected regressions. I'll revert this change because no matter whether we stick to +1 solution (or the current one), developers will face random issues with their existing codebases.

[egormkn]

Perhaps it will be possible to include this change to the next major update of nestjs/passport, so existing codebases with passport ^7.x.x that execute done callback explicitly won't be broken by npm update?
The passport-vkontakte strategy that I used is broken now with 7.1.5, but it works with 7.1.3, so npm install --save-exact @nestjs/passport@7.1.3 can be used as a temporary workaround for similar issues.

[kamilmysliwiec]

Anyone who wants to use less popular strategies that are not compatible with @nestjs/passport should NOT extend the PassportStrategy() class. Instead, simply extend the strategy exposed by a library (e.g., passport-vkontakte) and inside the constructor, pass options to the parent class and register a strategy, as follows:

constructor(...options) {
   super(...options);
   passport.use(this);
}