More descriptive error in createPassportContext

[TBG-FR]

PR Checklist

Please check if your PR fulfills the following requirements:

• [x] The commit message follows our guidelines: https://github.com/nestjs/nest/blob/master/CONTRIBUTING.md
• [ ] Tests for the changes have been added (for bug fixes / features)
• [ ] Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

• [x] Bugfix
• [ ] Feature
• [ ] Code style update (formatting, local variables)
• [ ] Refactoring (no functional changes, no api changes)
• [ ] Build related changes
• [ ] CI related changes
• [ ] Other... Please describe:

What is the current behavior?

Issue Number: N/A

When encountering an error from a Passport Strategy, we end up with a page with that message, without further information:

{
  "statusCode": 401,
  "message": "Unauthorized"
}

What is the new behavior?

This small change introduce more information in the error message (taking it from the Passport Strategy error), making it easier to identify the issue and fix it

{
  "statusCode": 401,
  "message": "Unauthorized (RPError: JWT not active yet, now 1646898893, nbf 1646898895)"
}

Does this PR introduce a breaking change?

• [ ] Yes
• [x] No

Other information

Don't hesitate if you see any improvement that could be made 😉

[jmcdo29]

According to #164 and #163 I don't this will be merged in. I'll leave it up to Kamil, but I'll add my two cents that we see a lot of issues on Discord about just getting back a 401 and not knowing why. Maybe we make our docs clearer that if there is a 401 with the AuthGuard() it probably means that passport initially failed to parse the request.

[kamilmysliwiec]

@jmcdo29 what about using logger.verbose (or "debug") to log the cause of this error down to the console?

[TBG-FR]

According to #164 and #163 I don't this will be merged in. I'll leave it up to Kamil, but I'll add my two cents that we see a lot of issues on Discord about just getting back a 401 and not knowing why. Maybe we make our docs clearer that if there is a 401 with the AuthGuard() it probably means that passport initially failed to parse the request.

I understand, but for now when you're developing and getting a 401 without further information, you need to search through the lib(s) files and add a log in the right place, which is not really convenient... Without breaking anything, that little change would at least display the real issue associated with the 401 😅

Maybe we should follow that advice then https://github.com/nestjs/passport/issues/163#issuecomment-570921251 but I feel this change would avoid the need of spending a lot of time to find a solution... (and I think even with my ExceptionFilter, I'd still get only the "UnauthorizedException" without further information)

[jmcdo29]

@jmcdo29 what about using logger.verbose (or "debug") to log the cause of this error down to the console?

I like the idea of printing this error in the logs but still throwing the minimal exception. Gets the devs the information necessary, without sending back too much information

[kamilmysliwiec]

Would you like to update this PR accordingly then? @TBG-FR

[TBG-FR]

Would you like to update this PR accordingly then? @TBG-FR

Done ! Thanks for the quick answers 😉

[TBG-FR]

@kamilmysliwiec I know I didn't update this since March but it's still "active", I just had tons of things to work on 😅

[TBG-FR]

@kamilmysliwiec Can you open this PR again (or tell me if something changed about it) so I finish it ?

[sezanzeb]

@kamilmysliwiec I'm interested in reading the discussion that lead to commit f86fe8297d99202c4ba58528620a32117c28b037, which reverts this change. Could you please link it here?

Edit: there it is: https://github.com/nestjs/passport/issues/1173