Open artificialhoney opened 5 years ago
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?
@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.
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).
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
)
@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
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',
})
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?
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.
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.
@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
@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>)
Odd... Yeah I might need a minimum repo to debug that one. I don't see why it'd affect sessions.
That will take some work. Let me see if I can figure out how to do it...
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.
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. 😄
Here ya go https://github.com/johnbiundo/nestpasspr94
@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)
@Sieabah I may not have a chance to try this again for a week or two. Currently on jury duty.
@Sieabah
OK, I had a chance to try this again. Still getting the same error. Here's what I do:
npm install
, npm run build
.npm install ../passport
-- this installs your lib. I can see it in my project under node_modules/@nestjs/passport
with your codeI 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.
Alright I'll take another look this weekend.
thank you @johnbiundo you saved my day!
@ruslanguns Great! Glad it helped!
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
... 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.
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?
I could do it like that:
@Module({
imports: [
PassportModule.register({
accessType: 'offline',
prompt: 'consent',
}),
],
controllers: [...],
})
export class AuthModule {}
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?
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.
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"]
}));
}
https://github.com/nestjs/passport/blob/master/lib/auth.guard.ts#L82
We pass
AuthModuleOptions
to theauthenticate()
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.
I'm submitting a...
Current behavior
Extending
PassportStrategy
does not work as expected. In case of extendingPassportStrategy(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:
Environment