panva / openid-client

OAuth 2 / OpenID Connect Client API for JavaScript Runtimes
MIT License
1.83k stars 392 forks source link

error while trying to connect several times #281

Closed JennyMet closed 4 years ago

JennyMet commented 4 years ago

HI,

I use the following code which works, However after few success calls (3-10), I sometimes get internal server error with the following error,

I use the 3.15.9 Any idea what could be wrong?

Error: did not find expected authorization request details in session, req.session["oidc:accounts.rvm.com"] is undefined
at /opt/node_app/app/node_modules/openid-client/lib/passport_strategy.js:125:13
at OpenIDConnectStrategy.authenticate (/opt/node_app/app/node_modules/openid-client/lib/passport_strategy.js:173:5)
at attempt (/opt/node_app/app/node_modules/passport/lib/middleware/authenticate.js:366:16)
at authenticate (/opt/node_app/app/node_modules/passport/lib/middleware/authenticate.js:367:7)
at /opt/node_app/app/src/logon.js:92:7
at Layer.handle [as handle_request] (/opt/node_app/app/node_modules/express/lib/router/layer.js:95:5)
at next (/opt/node_app/app/node_modules/express/lib/router/route.js:137:13)
at Route.dispatch (/opt/node_app/app/node_modules/express/lib/router/route.js:112:3)
at Layer.handle [as handle_request] (/opt/node_app/app/node_modules/express/lib/router/layer.js:95:5)
at /opt/node_app/app/node_modules/express/lib/router/index.js:281:22

My code from the stack is

at /opt/node_app/app/src/logon.js:92:7

which is the end of the code

    })(req, res, next);   //here is the error

This is the full code (I pass the app which is simply express server) , am I doing something wrong?

This is the index.js

const express = require('express');
const logon = require('./logon');

const app = express();
const port = process.env.PORT || 4000;

logon(app)
  .then(() => {
    console.log('process started');
  });
app.use(express.json());

app.listen(port,
  () => console.log(`listening on port: ${port}`));

This is the logon.js

const { Issuer, Strategy } = require('openid-client');
const cookieParser = require('cookie-parser');
const cookieSession = require('cookie-session');
const azpi = require('./azpi');
const bodyParser = require('body-parser');
const passport = require('passport');

module.exports = async (app) => {

  let oSrv;
  const durl = `${process.env.srvurl}/.well-known/openid-configuration`;
  try {
    oSrv = await Issuer.discover(durl);
  } catch (err) {
    console.log('error occured', err);
    return;
  }

  app.get('/', prs(), passport.authenticate('oidc'));

  const oSrvCli = new oSrv.Client({
    client_id: process.env.ci,
    client_secret: process.env.cs,
    token_endpoint_auth_method: 'client_secret_basic',
  });

  passport.serializeUser((user, done) => {
    done(null, user);
  });
  passport.deserializeUser((obj, done) => {
    done(null, obj);
  });

  const cfg = {
    scope: 'openid',
    redirect_uri: process.env.ruri,
    response_type: 'code',
    response_mode: 'form_post',
  };

  const prs = () => (req, res, next) => {
    passport.use(
      'oidc',
      new Strategy({ oSrvCli , cfg }, (tokenset, done) => {
        const claims = tokenset.claims();
        const user = {
          name: claims.name,
          id: claims.sub,
          id_token: tokenset.id_token,
        };
        return done(null, user);
      }),
    );
    next();
  };
  app.use(
    bodyParser.urlencoded({
      extended: false,
    }),
  );
  app.use(cookieParser('csec'));
  app.use(
    cookieSession({
      name: 'zta-auth',
      secret: 'csect',
    }),
  );

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

  app.get('/redirect', async (req, res, next) => {
    passport.authenticate('oidc', async (err, user) => {
      if (err) {
        console.log(`Authentication failed: ${err}`);
        return next(err);
      }
      if (!user) {
        return res.send('no identity');
      }

      req.login(user, async (e) => {
        if (e) {
          console.log('not able to login', e);
          return next(e);
        }
        try {
          const url = await azpi.GetUsers(user.id_token);
          return res.redirect(url);
        } catch (er) {
          res.send(er.message);
        }
      });
    })(req, res, next);   //here is the error
  });
};

if something is missing please let me know. we want to use this code in prod instead of java previous implementation ...

panva commented 4 years ago

I believe the error says it all, on the initial request that results in redirecting to the issuer's authorization_endpoint the strategy sets an object to the req.session's oidc:accounts.rvm.com property.

Said object is not there when getting the callback. You should investigate why that's the case. It’s not a problem with the strategy but rather, the underpinings used such as passport, cookie/express sessions etc.

JennyMet commented 4 years ago

Thanks for the quick answer, Im a bit lost with all the middleware stuff, it took some time to make it work with the right oreder of the middleware and it works but not stable. I use the latest version of all the components, could you please give me hint or something, where do you think the issue could be? as it works, but sometimes after 5-10 request I got this error.

panva commented 4 years ago

Debugging passport and cookie/express-session isn’t really in my capacity nor in my scope. This strategy conforms to the API passport requires and follows the implementation of other strategies only in a generic OIDC compliant way.

I can see one issue in your code, the way you’re passing “cfg” to the strategy isn’t right. Please refer to the documentation, there’s no recognized or used cfg property there.

JennyMet commented 4 years ago

Thanks you very much for the tip,

Do you mean like this? it use different lib, if not could you please provide a reference?

http://www.passportjs.org/docs/openid/

var passport = require('passport')
  , OpenIDStrategy = require('passport-openid').Strategy;

passport.use(new OpenIDStrategy({
    returnURL: 'http://www.example.com/auth/openid/return',
    realm: 'http://www.example.com/'
  },
  function(identifier, done) {
    User.findOrCreate({ openId: identifier }, function(err, user) {
      done(err, user);
    });
  }
));
panva commented 4 years ago

No. I obviously meant the documentation for THIS module.

JennyMet commented 4 years ago

Thanks a lot!!! Is there a way that I remove the passport usage ? as I use it to the serialize user session claims etc. Im not happy with it and I think that I should remove it, looking at my code is should be simple or I should refactor all ?

JennyMet commented 4 years ago

@panva looking at the following statement "Generic OpenID Connect Passport authentication middleware strategy." says that I need to use passport, is there example that can I use cleaner OIDC implementation without passport at all ?

panva commented 4 years ago

@panva looking at the following statement "Generic OpenID Connect Passport authentication middleware strategy." says that I need to use passport, is there example that can I use cleaner OIDC implementation without passport at all ?

https://github.com/panva/node-openid-client/issues/282#issuecomment-669004901

I really don't follow your line of reasoning. This is a generic client first (which is evident from just glancing over the readme or API documentation), exposing a generic OIDC passport strategy second. It is framework agnostic. The strategy is in place just because people kept asking for one. The quick starts in the project's readme explain the steps you ought to take for the very basic flows and you are responsible for integrating with the framework of your choice.

JennyMet commented 4 years ago

282 (comment) , this is someone else question :) , I'll check it out...