jaredhanson / passport-oauth2

OAuth 2.0 authentication strategy for Passport and Node.js.
https://www.passportjs.org/packages/passport-oauth2/?utm_source=github&utm_medium=referral&utm_campaign=passport-oauth2&utm_content=about
MIT License
607 stars 343 forks source link

Arity check does not work. Therefore Id token will never work. #168

Open arturohernandez10 opened 2 years ago

arturohernandez10 commented 2 years ago

The google client code, returns an accessToken, refreshToken, and the result itself. Which now returns the idToken, or at least in my case. The way to get that back id to read the "params".

For that this library attempts to check the arity but because of the many layers of passport it proves to be a bad interface choice. Passport can apply a mixin, in which case the callback has zero parameters during runtime. Effectively breaking this code:

Mixin code

var arity = self._verify.length;
if (arity == 5) {
  self._verify(accessToken, refreshToken, params, profile, verified);
} else { // arity == 4
  self._verify(accessToken, refreshToken, profile, verified);
}

the solution really is to do away with the arity and leave the optional parameters to the end. To avoid breaking changes you could do something like this:

var arity = self._verify.length;
if (legacyCheck && arity == 5) {
  self._verify(accessToken, refreshToken, params, profile, verified);
} else (legacyCheck) { // arity == 4
  self._verify(accessToken, refreshToken, profile, verified);
} else { 
  self._verify(accessToken, refreshToken, profile, verified, params);
}

Ideally legacyCheck is a configuration variable so that users can keep the old behavior. The biggest issue is the typescript types.

jaredhanson commented 2 years ago

This seems to be related to how Nest is wrapping Passport, correct? If so, I think the issue would be best addressed in the wrapper rather than here. Passport is expecting a function as an argument, in which case the arity checking works as intended. It's unclear to me how that is impacted by mixins (which seem to be a Nest construct?).

xenia-lang commented 1 year ago

In order to work with as many passport strategies as possible, the callback the Nest wrapper uses, is a variadic function. The arguments passed to the callback are forwarded to the user implementation and the wrapper takes care of calling the callback it was passed with the result or the thrown exception.

The problem arises due to the fact that the rest parameter does not contribute to the arity of a function. This means that the callback used by Nest has an arity of 0. Therefore, as a Nest user, it is impossible to aquire the params argument the oauth strategy passes to _verify without redoing the work the Nest people already did.

xenia-lang commented 1 year ago

Unfortunately, the solution suggested by @arturohernandez10 does not work, since nest expects the callback it is passed (verified in the oauth2 strategy code) to be the last argument.

Can you (or another Developer) elaborate on why the arity check was implemented? Is it for legacy reasons? If so, what where the reasons to not choose a feature flag?

perf2711 commented 1 year ago

A quick fix for that is to override the function Nest provides:

// Typescript
const verify: (...args: any[]) => any = (this as any)._verify;
(this as any)._verify = (arg1: any, arg2: any, arg3: any, arg4: any, arg5: any) =>
    verify(arg1, arg2, arg3, arg4, arg5);
// Javascript
const verify = this._verify;
this._verify = (arg1, arg2, arg3, arg4, arg5) => verify(arg1, arg2, arg3, arg4, arg5);

However, would be nice to see a fix for this issue. Maybe one idea is to keep the arity check as it is, but add another parameter to options like passParamsToVerify?