nestjs/passport

passportModule options lost in custom AuthGuard

yd021976 opened this issue · 3 comments

Feature Request

Is your feature request related to a problem? Please describe.

When extending the AuthGuard class, if the extending class needs specific constructor, the PassportModule options are "lost".

Exemple :
We want passport to write "user" information in "authentication" property of request object instead of "user" property of request.
We import passportModule with register and set option "property" to "authentication"

Module({
  imports: [
    PassportModule.register({ property: "authentication" })
...

in our JWT AuthGuard if we do this :

@Injectable()
export class JwtAuthGuard extends AuthGuard('jwt') {
    constructor() {
        super()
    }
...

then when this guard is triggered by a request, the user info will be store in "user" property of request object instead of "authentication" configured previously with passportModule.

Expected result :
req.authentication = {...}

Current result :
req.user = {...}

Describe the solution you'd like

So to keep passportModule options, the JWT guard constructor MUST add dependency to AuthModuleOptions and call super() constructor with this dependency. This should look like

@Injectable()
export class JwtAuthGuard extends AuthGuard('jwt') {
    constructor(private options: AuthModuleOptions) {
        super(options)
    }
...

Teachability, Documentation, Adoption, Migration Strategy

The NestJs documentation should at least be updated in the code block at the end of section https://docs.nestjs.com/security/authentication#enable-authentication-globally
because the constructor described in the documentation will break passportModule options.

What is the motivation / use case for changing the behavior?

When a user want to change the request property to store user information and when he uses a guard that extends AuthGuard class with a custom constructor

@kamilmysliwiec would this be a good use case for property based injection in the AuthGuard? Is that doable inside of a mixin? This way we can still do the option injection and let devs override/extend the class without worrying about the options?

Sounds good. To preserve backward compatibility, we could make the constructor's argument optional and only override the property if ctor options !== undefined

Any update on this? Is there any other way I can take to be able to pass these options in?