passport

passport copied to clipboard

Passport has an undocumented feature, where the request can be passed to the serializeUser/deserializeUser callback as the first argument, and whether it is or not is determined by the arguments in the function (if it accepts 3, the first is the request). This feature is used to get and pass the request to the callback.

The done callback is removed in favor of a promise being expected. A rejected promise (or an exception thrown within the handler) will be passed to passport as an error.

Also added types for everything as generics, defaulting to the type safe "unknown" instead of "any", allowing extenders to enforce type safety on this lower level, instead of just theirs.

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
• [N/A] Tests for the changes have been added (for bug fixes / features) - This package doesn't have any, and this PR doesn't add any
• [N/A] Docs have been added / updated (for bug fixes / features) - The serializer feature is not documented in docs.nestjs.com, and this PR doesn't add any.

PR Type

What kind of change does this PR introduce?

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

What is the current behavior?

There is no way to get the request into a serialize/deserialize handler, and the handler has to specify the types of the done callback and call it.

Issue Number: N/A

What is the new behavior?

The second argument is now the request, instead of the done callback. The handler is expected to return a promise, the value of which will be given to the done callback. If the promise is rejected, or an exception is thrown, that will be given as the error to the done callback.

Does this PR introduce a breaking change?

[x] Yes
[ ] No

Handlers will have to be changed into async functions that return the value instead of calling done with it. Any possible errors that they may have given to done must instead be thrown.

Serializers that don't need the request should be able to omit it from the implementation (since it's an optional second argument).

This change is trivial, but required non the less.

Other information

I've also made passport/www.passportjs.org#83 to document this feature.

Hi @boenrobot -- what about the response context? For example, if there is a session, handled by passport, and it needs to be destroyed, or there needs to be a response redirect ... without both req and res context, we may still be unable to handle all auth use cases.

All of those cases would require an extension of the AuthGuard, not the serialization/deserialization.

The serialization/deserialization should just fail if the session/jwt is invalid, but the actual error handling and recovery is in the guard.

@boenrobot what do you mean that it should be handled in the AuthGuard? Guards function below the express middleware. So, if done(err) is called, then it won't make it to a guard.

I can't seem to wrap my head around how to call req.session.destroy()... perhaps I should write custom middleware?