Skip to content

Remove optional dependency metadata inherited from base class #2581

Open
@gipde

Description

@gipde

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

i extend AuthGuard with a custom Implementation that depends on a provider of an other module (e.g. config).
if i try to use that custom AuthGuard in an other module, i have to specify a dependency to the config module, although this is already done in the module of the custom guard.

After a long debug session i found out, that AuthGuard use an Optional Parameter. https://github.com/nestjs/passport/blob/8acdc04e6210aefab7a6e2ac4009e788bc8de5b8/lib/auth.guard.ts#L29
Which means the injector does nothing, because it can not resolve the dependency (config) and the parameter of the base-class (AuthGuard) has an Optional Parameter

return undefined;

Expected behavior

Print out a clear Error message

Minimal reproduction of the problem with instructions

import {
  ExecutionContext,
  Injectable,
  Logger,
  UnauthorizedException,
} from '@nestjs/common';
import { AuthGuard } from '@nestjs/passport';
import { Configuration } from '../configuration/configuration';

@Injectable()
export class JwtAuthGuard extends AuthGuard('jwt') {
  constructor(private readonly config: Configuration) {
    super();
  }

  canActivate(context: ExecutionContext) {
    // Add your custom authentication logic here
    // for example, call super.logIn(request) to establish a session.
    // return true;
    // super.logIn(context.switchToHttp().getRequest());

    return super.canActivate(context);
  }

  handleRequest(err, user, _info) {
    if (err || !user) {
      if (this.config.withoutLogin()) {
        Logger.log('WITHOUT_LOGIN ==> NO LOGIN REQUIRED !!!');
        user = 'autologin';
        return user;
      }

      throw err || new UnauthorizedException();
    }
    return user;
  }
}

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

In General i find that nestjs should be more verbose on handling dependencies. i spend a lot time debugging in the past

Environment


Nest version: 6.2.4

 
For Tooling issues:
- Node version: v11.10.0 
- Platform: Mac

Others:

Activity

changed the title [-]Extending AuthGuard[/-] [+]Extending AuthGuard leads to problems[/+] on Jun 17, 2019
adamhathcock

adamhathcock commented on Jul 10, 2019

@adamhathcock

Thanks for this issue, I just got bit by this myself.

changed the title [-]Extending AuthGuard leads to problems[/-] [+]Remove optional dependency metadata inherited from base class[/+] on Jul 14, 2019
Enkosz

Enkosz commented on Feb 4, 2022

@Enkosz

Is someone working on this bug? I would like to take a look

kamilmysliwiec

kamilmysliwiec commented on Feb 4, 2022

@kamilmysliwiec
Member

PRs are more than welcome!

eabasir

eabasir commented on Mar 22, 2022

@eabasir

Still no update on this?

micalevisk

micalevisk commented on Mar 22, 2022

@micalevisk
Member

@jmcdo29 wasn't this fixed by your PR nestjs/passport#824

jmcdo29

jmcdo29 commented on Mar 22, 2022

@jmcdo29
Member

Specifically for the AuthGuard yes, but if that was just an example and we wanted to take this to a higher level (all child classes have separate DI metadata) then this would still be an issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

      Development

      Participants

      @adamhathcock@gipde@micalevisk@eabasir@kamilmysliwiec

      Issue actions

        Remove optional dependency metadata inherited from base class · Issue #2581 · nestjs/nest