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

Passport types do not allow a reset of user session when deserializeUser fails #776

Open
attwad opened this issue May 28, 2020 · 2 comments

Comments

@attwad
Copy link

attwad commented May 28, 2020

As per #6 (comment), one needs to pass false or null to the done callback when the user could not be deserialized in order for passport to invalidate the session.

The typescript types however provide this interface:

deserializeUser<TUser, TID>(fn: (id: TID, done: (err: any, user?: TUser) => void) => void): void;
deserializeUser<TUser, TID, TR extends IncomingMessage = express.Request>(fn: (req: TR, id: TID, done: (err: any, user?: TUser) => void) => void): void;

which makes it impossible to pass false or null, one must pass either a TUser or undefined.

Passing undefined doesn't trigger the session invalidation code in authenticator.js:340 because the checks are done for ===null or ===false:

// a valid user existed when establishing the session, but that user has
// since been removed
if (user === null || user === false) { return done(null, false); }

Expected behavior

I expect to be able to use strict typescript or pass undefined to the done() callback and passport should interpret this correctly.

Actual behavior

Typescript error is shown, passing undefined doesn't work either.

Steps to reproduce

Setup a typescript project with passport+mongoose, in deserializeUser do:

passport.deserializeUser<UserDocument, string>((id, done) => {
            // Find the user based on its id in the cookie.
            User.findById(id)
                .then((user) => {
                    done(null, user); <-- strict typescript error: "Argument of type 'UserDocument | null' is not assignable to parameter of type 'UserDocument | undefined'.
  Type 'null' is not assignable to type 'UserDocument | undefined'.ts(2345)"
                    return;
                })
                .catch((e) => {
                    console.error('Failed to get user:', e);
                    done(e, undefined);
                });
        });

Sad workaround

Disabling strict typescript with "strict": false in tsconfig.json makes the code above works as expected.

Potential fix

Have authenticator.js check for user === undefined as well or change typescript types to accept a boolean or null as types for TUser params. I am not sure which one you'd prefer, I am not being super familiar with the whole typescript ecosystem so I'd gladly rely on people with more experience to make a decision.

Environment

  • Operating System: OS X Catalina 10.15.4
  • Node version: v12.16.2
  • passport version: passport@0.4.1
@smichel17
Copy link

This is not a bug in passport, which is javascript-only and does not not include Typescript definitions. @types/passport is a third party package to provide type definitions — which are currently wrong: as you say, passport actually accepts null or false, not undefined. It is part of DefinitelyTyped, so that repo is the right place to report issues. In this particular case, there is already an open PR to fix the types.

@jacekkopecky
Copy link

fyi, that PR has since been merged.

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

No branches or pull requests

3 participants