Add support promises flow

[alexpts]

Return promise from all api method for work with passport via await/yield/then/(without callback) flow.

It will be nice! All new modules uses async/await now via babel or nodejs 7. It really more simple and clean.

[nhooey]

Since #225 and #243 have been created, it seems that a lot of people have been using promises and async/await for simpler control flow.

It's possible to promisify req.logIn in at least an Express app, but not obvious how to promisify passport.authenticate. It would be very helpful if promise support was added.

[nhooey]

@jaredhanson What are your thoughts on introducing Promise support now that using Promises and async/await is more common?

[mazing]

👍

[nhooey]

@jaredhanson If I could reply to your sentiment about promises mentioned in your Stack Overflow answer from 2012:

Idiomatic Node.js

everyauth makes extensive use of promises, instead of Node's approach of using callbacks and closures. Promises are an alternative approach to async programming. While useful in some high-level situations, I wasn't comfortable with an authentication library forcing this choice upon my application.

Furthermore, I find that proper use of callbacks and closures yields concise, well architected (almost functional style) code. Much of the power of Node itself comes from this fact, and Passport follows suit.

I'd say that given the new async and await features of JavaScript, mentioned in the EMCAScript 2017 Draft (ECMA-262), and supported in stable Node.js releases with the --harmony-async-await command-line parameter, there is no reason to believe that callbacks yield "concise, well architected code" in relative terms. I believe the danger of creating "callback hell", where it's almost impossible to follow the control flow of the code, is the primary concern when writing well-architected code. Also, using Promises with async/await also doesn't prevent one from writing functional code.

Any decent, modern programming language with parallelism built in, and even old ones with parallelism tacked on, use Promises or Futures. Wikipedia has a list of languages that implement Promises, which includes the following languages with native Promise support: C++, Dart, Java, JavaScript, C#, Python, R, Scala, Swift, and Visual Basic, and more.

If you don't want to force Promises on your users, it would make sense to make a new major version of the Passport API while still fixing bugs in the old version.

Using a real concurrency construct like Promises should be the way forward for any Node.js library.

[mxstbr]

Just to note, Promises and callbacks aren't necessarily exclusive, you can support both in one function:

const asyncFunc = (callback) => {
  const doAsyncThing = (cb) => {/* Do something asynchronous here, then call cb() */};

  if (!callback) {
    return new Promise(resolve => doAsyncThing(resolve));
  } else {
    doAsyncThing(callback);
  }
}

Now both of these work, which means this would only be a feature release rather than a major release since it doesn't break anybody!

asyncFunc(() => { /* Callback when finished */ });

asyncFunc()
  .then(() => { /* Resolve promise when finished */ });

[nhooey]

@jaredhanson Do you have any thoughts on adding promise support to Passport?

[alexandernanberg]

Any update on this? It would make everything so much cleaner!

[walleXD]

Has any one tried to use the new promisify function builtin into the util module from node v 8.0 and up?

[nhooey]

I tried using the promisify function to promisify things in Passport a few months ago. Although I can't remember much about what I did, I remember stopping at some point because I would have to change a lot of code within Passport and libraries it depended on. The library needs an overhaul, but @jaredhanson hasn't replied to a single message about this for almost 7 months now.

Someone should just fork the project and support promises.

[jaredhanson]

What benefits would promises offer an application built on Express, using the middleware pattern?

For example, passport is used via middleware like:

app.post('/login', passport.authenticate('local'));

Why does that need to return a promise?

[walleXD]

I am using express with graphql. So, I am using passport to secure my graphql endpoint but the issue is that graphql only works with promises and so it becomes necessary to wrap functions like passport.authenticate with another function which returns a promise

Also, most of the JS community (browser & node) is definitely moving fast towards adoption of async/await to handle async actions with promises over callback. So, adding support for promises would make passport easier to use with newer libraries which are coming out now and also in the future as they tend to mostly use promises (graphql being an example from the last few years)

[mikemaccana]

@jaredhanson Also, re: middleware, Express 5 is going to have explicit promises support.

[zenflow]

Someone needs to PR with an alternate interface

[zenflow]

@mikemaccana Where did you read that?
https://expressjs.com/en/guide/migrating-5.html
no mention of promises... ?

[michaelwalloschke]

@zenflow Yeah, because they are already supported (somehow) in 4.x:
https://expressjs.com/en/advanced/best-practice-performance.html#use-promises

[mayeaux]

+1 for native promise support

[mattbrunetti]

@michaelwalloschke Hmm, using promises is definitely advocated, but it doesn't look like there's built-in support...

Note that the second example under your link uses a wrap function on the request handler:

app.get('/', wrap(async (req, res, next) => {
   ...

[mattbrunetti]

This looks like the wrap function used: express-async-wrap

[zhanwenchen]

@jaredhanson Mongoose and Sequelize prefer promises over callbacks now, so in a passport-local strategy, the common pattern will become

// strategy.js
...
User.findOne({ where: { email })
  .then((existingUser) => { throw new myUserExistsError(`user with email ${email} already exists`; })
  .then(() => User.create(userData))
  .then(newUser => done(null, newUser, { message: 'success' })
  .catch((error) => {
    if (error instanceof myUserExistsError || error instanceof Sequelize.UniqueConstraintError) {
      return done(error, false, { message: 'user already exists' } );
    }
    return done(error, false, { message: 'some other error' } );
  });
...

// handler.js
(req, res, next) => {
  return passport.authenticate('local-signup', (error, user, info) => {
    if (error) return res.status(400).json({ message: info });
    if (user) return res.status(200).json({ message: info });
    return res.status(401).json({ message: info });
  }
})(req, res, next);

where passport.authenticate can only accept a callback, making this a done hell when the strategy definition uses a few more thens and dones. Besides this forces many new to passport/promises into using an anti-pattern of mixing promises with callbacks, on top of ensuring done is called once and only once. Let's not assume people know how to convert between the two, especially people who follow the tutorials.

[mikemaccana]

lol I think done() is dead. Promises have gotten popular due to await needing them, not because we like endless method chaining!

[mikemaccana]

@zenflow See expressjs/express#2237, the Express 5 release tracking bug list. Top item:

Add support for Promises in all handlers #2259

if you follow it, you'll end up at Router 2.0 which is pillarjs/router#60

[mayeaux]

Await is way better than endless chaining and has been supported for a long time, it has my vote

[kerberjg]

Agreed. async/await has become the new de-facto standard, respected by many libraries such as Mongoose and Express.js

It would be only natural for Passport.js to support it as well, also given the growing demand

[jakubrpawlowski]

As of today passport's done() is the only oldschool error handling artifact in my app. It seems that everyone else moved to promises.
You asked about benefits @jaredhanson and I think one obvious benefit is productivity and that if you teach a junior like myself it's easier to teach one concept of error handling, not a few different ones. Also mixing callbacks and promises makes testing tricky. Like others mentioned here this doesn't have to be a breaking change because promises and callbacks don't have to be exclusive- check out bcryptjs and mongoose for example.
Thoughts?

[jaredhanson]

Are you referring to done as needed in verify callbacks?

[jakubrpawlowski]

I'm referring to LocalStrategy example where we pass errors to done(). In my app I have calls to database and async password hashing inside that callback which both return promises so if I only knew a way to make passport return a promise too I could get rid of done() and make it all look uniform. Does this make sense?

[jakubrpawlowski]

I thought about it all day- I've read entire documentation over and over again and it finally makes sense to me why you went with done() to cover all the possible outcomes as described in Verify Callback. As much as I wish there was a simpler way to do this I'm afraid that I can't come up with a more elegant solution that would work in Express 4.x. It looks like one may be possible in Express 5.x. Sorry for wasting your time!

[Misiur]

@jaredhanson is this issue on your radar?

[torenware]

What benefits would promises offer an application built on Express, using the middleware pattern?

I think one of the best arguments is how this can simply building unit tests. This article by Gustaf Holm shows how promises and async / await simply test code, and make it easier to write code that is easy to test.

[corysimmons]

Jared works at Auth0 now, and hasn't made any meaningful commits to this repo in forever. He does auth programming all day—for money. Why do you guys expect him to swoop in and save the day?

Passport doesn't seem like it's doing a ton of stuff. Maybe it's time for another hero to rise up and make some auth library from scratch?

Then again, it seems everyone could just use https://nodejs.org/api/util.html#util_util_promisify_original where needed.

[relativityboy]

@corysimmons - or someone(s) here could make more PRs on this lib. Do you think @jaredhanson would be able/willing to review PRs still?

If that's a no-go, what's better/more-active than passport in the js auth space?

[torenware]

This is an open source project. If @jaredhanson wanted to chime in here, he's had close to two years to do it. If there's a reasonable fix to this, someone should feel free to fork the project.

[torenware]

Project has an MIT license. There's no legal impediment to forking this. Moreover there are more than 20 pending PRs. Might make sense to review those PRs. If enough of us are interested, we can decide on where we want to do development, see if the PRs transfer where relevant, and restart development.

[relativityboy]

@torenware - there's some sense in what you say, but if he's going to let it go, given its popularity the responsible thing would be to turn it and the npm package over to a maintainer, or grant shared access at least. That's easier than ever to do now with github and npm having tools for that.

[rwky]

We've already forked it here https://github.com/passport-next/passport and we want to add promises, passport-next#7 if you want to contribute to the fork I'd be happy to look at any PRs.

[oehm-smith]

I have an issue related to async behaviour in passport - https://stackoverflow.com/questions/56389077/passport-azure-ad-seems-to-run-asynchronously.

When run as normal middleware:

class MyCtrl {
    @Get('/api/tasks')
   @UseBefore(passport.authenticate('oauth-bearer', { session: false }))
       get() {
 
   } 
}

Like this in the https://tsed.io framework I am using. That is equivalent to the proverbial:

server.get('/api/tasks', passport.authenticate('oauth-bearer', { session: false }));

Then it works fine. However you can also run it as a detached middleware. Something like:

@Post('/search')
@CustomAuth({role: 'admin', scope: ['busybody']})
async search(@Req() request: Req, @BodyParams() query: TranslationApiQuery): Promise<TranslationApiModel[]> {
     ...

 export function CustomAuth(options = {}): Function {
     return applyDecorators(
         UseAuth(CustomAuthMiddleware), 
         ...

 @Middleware()
 export class CustomAuthMiddleware implements IMiddleware {
     public use(
         @Req() request: Express.Request, @Res() response: Express.Response, @EndpointInfo() endpoint: EndpointInfo,
         @Next() next: Express.NextFunction
          ) {
              // retrieve options given to the @UseAuth decorator
              const options = endpoint.get(CustomAuthMiddleware) || {};

              Passport.authenticate('oauth-bearer', {session: false}})(request, response, next);
              ...

But this doesn't work. As discussed in the referenced Stackoverflow Question, the passport-azure-ad/lib/bearerstrategy.js uses async.waterfall() and from what I understand this has async behaviour.

I don't understand enough about Passport to know why it is only a problem when detached. Though if Passport returned a promise then the azure people could re-write this to wrap that async.waterfall in a promise

[thernstig]

The standard lib of Node.js is on a journey to create promise-based implementations of their APIs. Modules such as:
fs/promises
timers/promises
stream/promises
dns/promises
etc.

One can even compare prominent modules such as fs to see that they moved up and promoted the promise-based version in their documents, compare Node.js 12 to Node.js 14. More Node.js modules are moving in this direction.

In 2022 it seems async/await is the defacto future. With all respect towards this amazing library, I do hope it is time to consider implementing promise-based versions of all functions. It would be a great step forward to a cleaner code base for many users.

[tilleps]

I'm using this workaround to convert req.logIn and req.logOut into promises

import { promisify } from "node:util";
import passportHttpRequest from "passport/lib/http/request.js";

//  Workaround: Convert to promises before passport.initialize()
app.use(function (req, res, next) {
  req.logIn = promisify(passportHttpRequest.logIn);
  req.logOut = promisify(passportHttpRequest.logOut);
  next();
});

app.use(passport.initialize());
app.use(passport.session());

app.get("/login", async function (req, res, next) {
  try {
    const user = { id: "1234" };
    await req.login(user, { keepSessionInfo: true });

    res.send("LOGGED IN");
  } catch (err) {
    return next(err);
  }
});


app.get("/logout", async function (req, res, next) {
  try {
    await req.logout({ keepSessionInfo: true });

    res.send("LOGGED OUT");
  } catch (err) {
    return next(err);
  }
});