skonves / express-http-context

Get and set request-scoped context anywhere
MIT License
302 stars 27 forks source link

Context doesn't work with passport #13

Open khmelevskii opened 6 years ago

khmelevskii commented 6 years ago

after

    .use(passport.session())

context is not works

skonves commented 6 years ago

Thanks for the feedback 😄

What version of passport and what version of node are you using?

khmelevskii commented 6 years ago

node - v8.11.3 passport - 0.4.0

thank you for quick feedback

skonves commented 6 years ago

I forked passport's Express 4 example app and added express-http-context in an effort to reproduce the issue. You can see what I did at https://github.com/skonves/express-4.x-local-example.

My steps to test:

  1. Set node version: nvm use 8.11.3
  2. Start the server: npm start
  3. In my browser, access http://localhost:3000

I observed the following console output which indicates that I did not do enough to reproduce your error:

param1 (fat arrow) value1
param2 (fat arrow) value2
param1 (function) value1
param2 (function) value2
param1 (/) value1
param2 (/) value2
::1 - - [06/Jul/2018:03:18:59 +0000] "GET / HTTP/1.1" 304 - "-" "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_13_3) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/67.0.3396.99 Safari/537.36"

Things that will help me at this point:

  1. Post your code that is not working (unless it's proprietary)
  2. Let me know what you are doing different from the repo I linked to
  3. Submit a PR to the repo I posted that reproduces the error

(The last option would be the most helpful, but I also understand that it will be the most effort for you)

thkorte commented 5 years ago

Hi there,

I also have an issue using passport (0.4.0, passport-local 1.0.0) with express-http-context and just tested your example with different versions of nodejs (10.15.0, 10.15.3, 8.11.3). I figured out that all tested version makes some trouble:

param1 (fat arrow) value1  
param2 (fat arrow) value2  
param1 (function) value1
param2 (function) value2 
::1 - - [14/Mar/2019:11:29:22 +0000] "GET /login HTTP/1.1" 200 266 "http://localhost:4000/" "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_14_3) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/72.0.3626.121 Safari/537.36"
param1 (fat arrow) undefined
param2 (fat arrow) undefined
param1 (function) undefined
param2 (function) undefined
::1 - - [14/Mar/2019:11:29:30 +0000] "POST /login HTTP/1.1" 302 46 "http://localhost:4000/login" "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_14_3) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/72.0.3626.121 Safari/537.36"

As you can see, the context get's lost within the POST /login route. If I use GET for the /login route, everything is fine. Btw, a POST route without password.authenticate causes the same problem.

Any clue what's going here?

Cheers Thomas

thkorte commented 5 years ago

Got it working by adding bindEmitter calls right before the first context.set:

app.use((req, res, next) => {
   context.ns.bindEmitter( req );
   context.ns.bindEmitter( res );
   context.set('param1', 'value1');
   next();
});
vKongv commented 4 years ago

Having the same issue with passport after the authentication middleware being applied. bindEmitter does not work for me.

Using: express-http-context: 1.2.3 "passport": "^0.4.0", "passport-http": "^0.3.0", "passport-jwt": "^4.0.0", "passport-local": "^1.0.0",

alxyuu commented 3 years ago

for anyone else having trouble: using passport.authenticate directly causes some issues with async hooks getting confused.

while the behavior I observed was not that context was not available, I did notice several requests sharing context when they should be independent under concurrent requests.

my test setup used a cluster of 50 workers hitting an authorized endpoint sequentially 5 times each. in my testing, usually between 200 and 220 unique contexts were created (tested by storing a uuid on the context) when there should have been 250.

the way I resolved this was by promisifying passport.authenticate.

rather than doing

app.use(
  passport.authenticate(...),
  (req, res, next) => {
    // authenticated handler
  }
);

I did something like this:

const authenticateMiddleware = (strategy) => {
  return async (req, res, next) => {
    const authenticatePromise = new Promise((resolve, reject) => {
      passport.authenticate(strategy, (err, user, info) => {
        // you may have some custom handling, this is just an example
        // the promise just needs to reject or resolve under the correct conditions
        if (err || info) {
          reject(err || info);
          return;
        }
        resolve(user);
      })(req, res);
    });

    try {
      const user = await authenticatePromise;
      context.set('user', user);
      next();
    } catch (e) {
      next(e);
    }
};

app.use(
  authenticateMiddleware(...),
  (req, res, next) => {
    // authenticated handler
  }
);

Since express-http-context and cls-hooked us async_hooks under the hood, the tip to promisify callback style functions helped a lot: https://nodejs.org/docs/latest-v12.x/api/async_hooks.html#async_hooks_troubleshooting