Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Nest can't resolve dependencies of the JwtAuthenticationGuard after upgrade 8.1.0 to 8.1.1 #823

Closed
2 tasks done
rvieceli opened this issue Feb 14, 2022 · 9 comments · Fixed by #824
Closed
2 tasks done

Comments

@rvieceli
Copy link

Did you read the migration guide?

  • I have read the whole migration guide

Is there an existing issue that is already proposing this?

  • I have searched the existing issues

Potential Commit/PR that introduced the regression

No response

Versions

No response

Describe the regression

After upgrade nest, in special @nestjs/passport, it started to throw an exception. Going back to 8.1.0 it works again.

The error

[Nest] 63736  - 14/02/2022, 17:05:19   ERROR [ExceptionHandler] Nest can't resolve dependencies of the JwtAuthenticationGuard. Please make sure that the "options" property is available in the current context.

Potential solutions:
- If AuthModuleOptions is a provider, is it part of the current EmailScheduleModule?
- If AuthModuleOptions is exported from a separate @Module, is that module imported within EmailScheduleModule?
  @Module({
    imports: [ /* the Module containing AuthModuleOptions */ ]
  })

Error: Nest can't resolve dependencies of the JwtAuthenticationGuard. Please make sure that the "options" property is available in the current context.

Potential solutions:
- If AuthModuleOptions is a provider, is it part of the current EmailScheduleModule?
- If AuthModuleOptions is exported from a separate @Module, is that module imported within EmailScheduleModule?
  @Module({
    imports: [ /* the Module containing AuthModuleOptions */ ]
  })

    at Injector.lookupComponentInParentModules (/Users/rvieceli/Projects/study/nestjs/wanago.io-nestjs/node_modules/@nestjs/core/injector/injector.js:202:19)
    at Injector.resolveComponentInstance (/Users/rvieceli/Projects/study/nestjs/wanago.io-nestjs/node_modules/@nestjs/core/injector/injector.js:157:33)
    at /Users/rvieceli/Projects/study/nestjs/wanago.io-nestjs/node_modules/@nestjs/core/injector/injector.js:260:38
    at async Promise.all (index 0)
    at Injector.resolveProperties (/Users/rvieceli/Projects/study/nestjs/wanago.io-nestjs/node_modules/@nestjs/core/injector/injector.js:251:27)
    at callback (/Users/rvieceli/Projects/study/nestjs/wanago.io-nestjs/node_modules/@nestjs/core/injector/injector.js:47:32)
    at Injector.resolveConstructorParams (/Users/rvieceli/Projects/study/nestjs/wanago.io-nestjs/node_modules/@nestjs/core/injector/injector.js:124:24)
    at Injector.loadInstance (/Users/rvieceli/Projects/study/nestjs/wanago.io-nestjs/node_modules/@nestjs/core/injector/injector.js:52:9)
    at Injector.loadInjectable (/Users/rvieceli/Projects/study/nestjs/wanago.io-nestjs/node_modules/@nestjs/core/injector/injector.js:70:9)
    at async Promise.all (index 0)

image

Minimum reproduction code

Look at my project https://github.com/rvieceli/wanago.io-nestjs

Expected behavior

It doesn't depend on AuthModuleOptions

Other

[System Information]
OS Version     : macOS Big Sur
NodeJS Version : v16.14.0
NPM Version    : 8.3.1 

[Nest CLI]
Nest CLI Version : 8.2.0 

[Nest Platform Information]
platform-express version : 8.3.0
elasticsearch version    : 8.0.0
schematics version       : 8.0.5
passport version         : 8.1.1
schedule version         : 1.0.2
typeorm version          : 8.0.3
testing version          : 8.2.6
common version           : 8.3.0
config version           : 1.2.0
core version             : 8.3.0
cqrs version             : 8.0.2
jwt version              : 8.0.0
cli version              : 8.2.0
@jmcdo29
Copy link
Member

jmcdo29 commented Feb 14, 2022

For a quick workaround, use PassportModule.register({}) instead of just PassportModule. I'll have a fix up for this later.

Thanks for reporting it it. I'll make sure the test suite gets updated with a case to cover this

@rvieceli
Copy link
Author

@jmcdo29 I tried, but it still doesn't work. I went back to 8.1.0. I'm just learning. Thanks.

@rvieceli
Copy link
Author

@jmcdo29 I looked at lib/auth.guard and I saw this In order to use "defaultStrategy", please, ensure to import PassportModule in each place where AuthGuard() is being used. Otherwise, passport won't work correctly..

Then, I added PassportModule.register({}) (only PassportModule don't work) in all the modules that use @UseGuards(JwtAuthenticationGuard) and it works with 8.1.1

@jmcdo29
Copy link
Member

jmcdo29 commented Feb 14, 2022

Okay, yeah, that tracks. I was using a single module setup for testing this. You would need to use the PassportModule.register({}) and then have your AuthModule have PassportModule in the exports and ensure that AuthModule was imported wherever you use the AuthGuard().

Turns out, I needed to add @Optional() to the property injector as well, so the fix is easy, it's just making sure our test suite is updated now

@rvieceli
Copy link
Author

@jmcdo29 in a normal situation (without this issue), should I always import AuthModule in all modules I use UseGuards?

@jmcdo29
Copy link
Member

jmcdo29 commented Feb 14, 2022

Only if

  1. you have a PassportModule.register() with options you want to have used (like user property )
  2. You have a guard that needs to AuthService

Otherwise, it shouldn't really be necessary, but it is at the moment because of the change

@cleytoncoro
Copy link

Following nest docs (https://docs.nestjs.com/security/authentication) i found this error too after implementing local strategy (after "Implementing Passport local" topic and before "Built-in Passport Guards").

Downgrading to 8.1.0 all things works fine.

I'm start learning nestjs and I don't now how to colaborate sending PR. Maybe this repo needs an update, or maybe nestjs docs needs to be updated.

image

@jmcdo29
Copy link
Member

jmcdo29 commented Feb 15, 2022

@cleytoncoro if you'd read the above comments in the issue, and the linked PR, you'd realize this already has a fix, and there's solutions in the thread already. Kamil will most likely publish a patch or minor update for this once the PR is merged.

@kamilmysliwiec
Copy link
Member

Fixed

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

Successfully merging a pull request may close this issue.

4 participants