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

Context doesn't work with passport #13

Open
khmelevskii opened this issue Jul 5, 2018 · 7 comments
Open

Context doesn't work with passport #13

khmelevskii opened this issue Jul 5, 2018 · 7 comments

Comments

@khmelevskii
Copy link

khmelevskii commented Jul 5, 2018

after

    .use(passport.session())

context is not works

@khmelevskii khmelevskii changed the title Context doesnt Context doesn't work with passport Jul 5, 2018
@skonves
Copy link
Owner

skonves commented Jul 5, 2018

Thanks for the feedback 😄

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

@khmelevskii
Copy link
Author

node - v8.11.3
passport - 0.4.0

thank you for quick feedback

@skonves
Copy link
Owner

skonves commented Jul 6, 2018

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
Copy link

thkorte commented Mar 14, 2019

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
Copy link

thkorte commented Mar 14, 2019

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
Copy link

vKongv commented Feb 17, 2020

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
Copy link

alxyuu commented Aug 19, 2021

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

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

5 participants