rkusa / koa-passport

Passport middleware for Koa
MIT License
774 stars 55 forks source link

async/await passport-jwt custom handler #63

Closed krutijan1 closed 8 years ago

krutijan1 commented 8 years ago

Hi, I am using koa-router and koa-passport with passport-jwt. The problem is that every error thrown inside passport custom callback is not catched by parent middleware. Am I missing something or is this intended behaviour? Can you please provide example how this should work with async/await and koa2?

Best regards

privateRouter.get('/test', isAuthenticated, users.test)

async function authenticate(ctx, next) {
  await passport.authenticate('jwt', { session: false }, async(user, userInfo) => {
       await next() // when next middleware throws error it won't be catched by parent middleware
       throw new Error() // this won't be catched by parent middleware
  })(ctx, next)
}
rkusa commented 8 years ago

Thanks for reporting, this is definitely not intended behavior. Unfortunately, I do not have a working async/await example myself. I have the plan to update koa-passport-example, but was waiting until async/wait lands in Node.js natively. Is it maybe possible that you extract a minimal working version of your app that shows this issue? Otherwise I'll try to create an async/await version of koa-passport-example earlier, but unfortunately cannot promise to get this done shortly 😕

krutijan1 commented 8 years ago

I understand, I have created minimal working example demonstrating issue here. Maybe later on I can provide pull request with suggested changes, when I find time to dig deeper. Meanwhile I am using this workaround if it can help anyone:

  async function authenticate(ctx, next) {
    await passport.authenticate('jwt', { session: false }, async(user, userInfo) => {
        try {
          await next() // when next middleware throws error it won't be catched by parent middleware
         } catch (e) {
            // set error response directly here to stop propagating upstream
            ctx.status = e.status
            cts.message = e.message
         }
    })(ctx, next)
  }
rkusa commented 8 years ago

Found the promise that was missing a catch 😖 Thanks a lot for providing the working example, that helped a lot! Published as 2.2.1

krutijan1 commented 8 years ago

I have incorporated your fix, works perfectly. Awesome job, thanks :simple_smile:

avaleriani commented 7 years ago

@krutijan1 you are a life saver thank you very much!