Extending PassportStrategy does not take provider specific OAuth2 options in account

[artificialhoney]

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

Extending PassportStrategy does not work as expected. In case of extending PassportStrategy(Strategy, 'google') additional OAuth2 options, respectively provider specific options like e.g. approval_prompt passed to Superconstructor are NOT applied. So it is not possible to obtain a REFRESH_TOKEN.

Expected behavior

Additional, provider specific OAuth2 options can be passed through the Superconstructor and become effective.

Minimal reproduction of the problem with instructions

My Google OAuth2 strategy implementation:

import { Injectable } from '@nestjs/common';
import { PassportStrategy } from '@nestjs/passport';
import { Strategy } from 'passport-google-oauth20';
import { AuthService, Provider } from './auth.service';
import { ConfigService } from '../config/config.service';
import { User } from '../api/user/user.interface';

@Injectable()
export class GoogleStrategy extends PassportStrategy(Strategy, 'google') {

  constructor(configService: ConfigService, private readonly authService: AuthService) {
    super({
      clientID: configService.get('OAUTH_CLIENT_ID'),
      clientSecret: configService.get('OAUTH_CLIENT_SECRET'),
      callbackURL: `${configService.baseUrl}/auth/google/callback`,
      passReqToCallback: true,
      scope: ['email', 'profile'],
      // NOT WORKING
      approval_prompt: 'force',
      access_type: 'offline',
    });
  }

  async validate(request: any, accessToken: string, refreshToken: string, profile: any, done: (err: any, result: any) => void) {
    // ...
  }

  // WORKAROUND: pass options to superclass auth call by overriding superclass method
  authorizationParams(options: any): any {
    return Object.assign(options, {
      approval_prompt: 'force',
      access_type: 'offline',
    });
  }
}

Environment

@nestjs/passport: 6.0.0

 
For Tooling issues:
- Node version: 10.11.0
- Platform:  Mac

[iveselin]

Having the same issue with hd param for google, can you show the usage of a workaround?

UPDATE:
According to this, you have to provide specific options to passport.authenticate(), any idea how one could do that with auth guard?

[artificialhoney]

@iveselin just try with:

@Injectable()
export class GoogleStrategy extends PassportStrategy(Strategy, 'google') {
  // ...
  // WILL be passed to authenticate()
  authorizationParams(options: any): any {
    return Object.assign(options, {
      hd: '<HD_PARAM>'
    });
  }
}

The problem is, that non-standard params passed through constructor super() call are ignored, which normally should work (https://github.com/jaredhanson/passport-google-oauth2/blob/master/lib/strategy.js#L159). Just use authorizationParams as in the example above.

[csidell-earny]

While that does work it skips the validation and sanitization the original function does. You're able to achieve the same by overriding the authenticate function and it retains all original behavior of the strategy.

@Injectable()
export class GoogleStrategy extends PassportStrategy(Strategy, 'google') {
    authenticate(req, options: any): any {
        super.authenticate(req, Object.assign(options, {
            hd: '<HD_PARAM>'
        });
    }
}

Edit: You can achieve similar results by calling super.authorizationParams, however if anyone stumbled upon custom_params or custom parameters this would be the way it'd work across providers (potentially).

[kamilmysliwiec]

The problem is, that non-standard params passed through constructor super() call are ignored, which normally should work

Can someone point me at the line of code that strips those options in @nestjs/passport? I looked at this PR https://github.com/nestjs/passport/pull/94/files and I actually don't see any reason why we should define an extra property (customParameters)

[Sieabah]

@kamilmysliwiec It's not that @nestjs/passport strips them it's that some providers don't take in those options on the constructor.

It may be specific to Google's passport but they never take the options that are passed into the constructor, so it forces us to override the authenticate function to be able to handle Google. Here is the offending line.
https://github.com/mstade/passport-google-oauth2/blob/master/lib/oauth2.js#L58

Edit:

Additionally the core OAuth2 strategy does not merge the options from the constructor https://github.com/jaredhanson/passport-oauth2/blob/master/lib/strategy.js#L130

[kamilmysliwiec]

https://github.com/nestjs/passport/blob/master/lib/auth.guard.ts#L82

We pass AuthModuleOptions to the authenticate() function, so you should be able to put them here:

PassportModule.register({
      approval_prompt: 'force',
      access_type: 'offline',
})

[csidell-earny]

Which works for a single auth provider but if you wanted to support Microsoft and Google and they have a collision in auth params you'd run into issues.

How would you support multiple passport providers? Assuming using auth0 isn't available.

Is the correct solution to create a passport module per auth provider? How would you correctly set up the AuthGuard in this instance?

[kamilmysliwiec]

Ahhh, got you now. That might be tricky. You could theoretically extend the guard and strip options properties depending on the provider in the constructor.

[csidell-earny]

Or provide them with a parameter in the strategy of the provider?

That's why I added the PR, so it can be done closest to where it is actually necessary.

[csidell-earny]

@kamilmysliwiec Should the strategy just override the authenticate function instead? I'm not sure how the auth guard would specifically solve this problem. An example with our projects would have multiple ways to authenticate with google/microsoft/facebook but sign our own jwt's with our own info. So we never use the google/microsoft/facebook auth guards as that's just for signup and login.

We have it working by just overriding the authenticate function but that requires specifically to be careful that @nestjs/passport doesn't change how authenticate is called (not that it would) whereas passing in a customParameters would allow this project to manage the plumbing and simplify the consumer strategies

[johnbiundo]

@csidell-earny I tried your PR with my app. Identical code with only swapping in/out your version of @nestjs/passport works with current version, fails as follows.

I can't make this repo public, but if it matters, I could see if I can strip it down. But it looks like it has to do with the fact that I'm also using Passport sessions, a la stuff like this in my main.ts:

app.use(session(...

app.use(passport.session())

Error: Failed to serialize user into session
[0]     at pass (/home/john/code/dwserver5/node_modules/passport/lib/authenticator.js:281:19)
[0]     at Authenticator.serializeUser (/home/john/code/dwserver5/node_modules/passport/lib/authenticator.js:299:5)
[0]     at SessionManager.logIn (/home/john/code/dwserver5/node_modules/passport/lib/sessionmanager.js:14:8)
[0]     at IncomingMessage.req.login.req.logIn (/home/john/code/passport-1/node_modules/passport/lib/http/request.js:50:33)
[0]     at Promise (/home/john/code/passport-1/dist/auth.guard.js:58:64)
[0]     at new Promise (<anonymous>)
[0]     at GoogleLoginGuard.<anonymous> (/home/john/code/passport-1/dist/auth.guard.js:58:23)
[0]     at Generator.next (<anonymous>)
[0]     at /home/john/code/passport-1/dist/auth.guard.js:19:71
[0]     at new Promise (<anonymous>)

[Sieabah]

Odd... Yeah I might need a minimum repo to debug that one. I don't see why it'd affect sessions.

[johnbiundo]

That will take some work. Let me see if I can figure out how to do it...

[Sieabah]

I'd imagine you could just create a skeleton project with the cli and just copy over some hardcoded auth stuff. Don't need to integrate with everything.

[johnbiundo]

Yeah. It's integrated with a client side Angular app. A few tricky things to unwind. The app actually manages the authentication UI (Google Auth Popup, etc.) from the client side instead of a regular redirect. Shouldn't be too bad. I hope. 😄

[johnbiundo]

Here ya go
https://github.com/johnbiundo/nestpasspr94

[Sieabah]

@johnbiundo Using your repo and swapping out for my library I can't get it to fail as you've explained.

As far as I can tell from your repo it works, I get the serialized user just fine.

I did have to remove a null reference to the usersModule in your AuthModule though. (Nothing else references it)

[johnbiundo]

@Sieabah I may not have a chance to try this again for a week or two. Currently on jury duty.

[johnbiundo]

@Sieabah

OK, I had a chance to try this again. Still getting the same error. Here's what I do:

1. Yeah, you're right about the extraneous UsersModule reference. Removed that.
2. Clone your repo, npm install, npm run build.
3. Clone my repo in a sibling folder.
4. npm install ../passport -- this installs your lib. I can see it in my project under node_modules/@nestjs/passport with your code
5. npm run start on my project, attempt Google login; after submitting the Google login page --> get the failure shown above.

[johnbiundo]

I updated my repo with those changes, and it also removed default install of @nestjs/passport, and includes the callback URL that should work for localhost.

[Sieabah]

Alright I'll take another look this weekend.

[ruslanguns]

thank you @johnbiundo you saved my day!

[johnbiundo]

@ruslanguns Great! Glad it helped!

[prateekkathal]

Ahhh, got you now. That might be tricky. You could theoretically extend the guard and strip options properties depending on the provider in the constructor.

I am not sure why we need a separate method just as @kamilmysliwiec said. The above suggestion makes perfect sense to me.

I have Google, Twitter & Facebook working altogether with separate Strategy & Guards. An authenticate method with customParameters seems un-necessary to me as well. All I am doing is passing different options to constructor of the new strategies.

Also, just to be clear, according to passport-google-oauth2's strategy.js,

approval_prompt: 'force',  // should be "approvalPrompt" - strategy.js#L183
access_type: 'offline', // should be "accessType" - strategy.js#L138

CC: @johnbiundo

[xyide]

... you have to provide specific options to passport.authenticate(), any idea how one could do that with auth guard?

I'm late to the party and not sure if this is something that has been supported with newer versions since you asked the question, but to provide specific options to the the passport authenticate function via an auth guard, you would extend the auth guard and specify the options in the constructor.

For example, I had to create an auth guard for Facebook that would make sure that auth_type=rerequest is always specified in the parameters when authenticating, so my guard looks like this:

export class FacebookAuthGuard extends AuthGuard('facebook') {
  constructor(private configService: ConfigService) {
    super({
      authType: 'rerequest',
    });
  }
}

With that, authTpe is always processed by passport's Strategy.prototype.authorizationParams prototype function and passed along to the authenticate function.

[sebastiangug]

Just want to say I'm at the end of 5 hours of debugging tracking this down. insane!

Can we at least document this behaviour for the future?

[Almaju]

I could do it like that:

@Module({
	imports: [
		PassportModule.register({
			accessType: 'offline',
			prompt: 'consent',
		}),
	],
	controllers: [...],
})
export class AuthModule {}

[knownasilya]

Same happens for me when using the local strategy:

import { Strategy } from 'passport-local';
import { PassportStrategy } from '@nestjs/passport';
import { Injectable, UnauthorizedException } from '@nestjs/common';
import { AuthService } from './auth.service';

@Injectable()
export class LocalStrategy extends PassportStrategy(Strategy) {
  constructor(private authService: AuthService) {
    super({ usernameField: 'email', passwordField: 'password' });
  }

  async validate(username: string, password: string): Promise<any> {
    let email = username;
    const user = await this.authService.validateUser(email, password);
    if (!user) {
      throw new UnauthorizedException();
    }
    return user;
  }
}

The passport strategy mixin class seems to get no arguments in the constructor

So the options never get set:

Maybe related to https://stackoverflow.com/a/36204599/483616?

[knownasilya]

Ok, for me it looks like some sort of build cache. Basically stopping and restarting the watch wasn't enough, I had to run yarn build and that nuked dist and it worked after. The code in dist was correct though, so not sure what was going on.

[TrejGun]

According to Facebook docs for api 2.4 (current is 3.2)

Fewer default fields for faster performance: To help improve performance on mobile network connections, we've reduced the number of fields that the API returns by default. You should now use the ?fields=field1,field2syntax to declare all the fields you want the API to return.

https://developers.facebook.com/blog/post/2015/07/08/graph-api-v2.4/

This means even if I use profileFields in Strategy constructor

      profileFields: ["id", "email", "name"],

I still have to pass additional params to authorize function like this

passport.use(new FacebookStrategy({
          clientID: "CLIENT_ID",
          clientSecret: "CLIENT_SECRET",
          callbackURL: "http://www.example.com/auth/facebook/callback"
          passReqToCallback : true,
          profileFields: ["id", "emails", "name"] 
    })
);

app.get("/connect/facebook", passport.authorize("facebook", { scope : ["email"] }));

Thanks to @csidell-earny his workaround works fine for me

  authenticate(req: Request, options?: Record<string, any>): void {
    super.authenticate(req, Object.assign(options, {
      scope: ["email"]
    }));
  }

[Romstar]

https://github.com/nestjs/passport/blob/master/lib/auth.guard.ts#L82

We pass AuthModuleOptions to the authenticate() function, so you should be able to put them here:

PassportModule.register({
      approval_prompt: 'force',
      access_type: 'offline',
})

thanks for this, I can't believe this issue still exists.