Why the security handler function does not have (rq, res, next) signature

[ClementVidal]

Hello,

I'm trying to use security middleware in conjunction with passport, and i'm stuck :

The signature of the callback function called by the security middleware is:

• (req, authOrSecDef, scopesOrApiKey, callback)

Within this callback i do not have access to the res and next argument used by passport and by standard connect middleware

Why the signature of this callback is not:

• (req, res, next, authOrSecDef, scopesOrApiKey, callback) instead ?

Is there any reason to that ?
Do you know how can i overcome this situation ?
Or maybe i'm not using security middleware the right way...

And also:
How can i specify and HTTP response code in case the security callback fail to authenticate the request ?

Thanks !!

[whitlockjc]

I'm going to cc @theganyo to get his take since he wrote this middleware.

[rguikers]

Same question was posted by me. Sorry for the duplicate. A small solution is this :

oauth: function(req, def, scopes, callback) {
  passport.authenticate('bearer', function(err, user, info) {
    if(err){
      callback(new Error('Error in passport authenticate'));
    }
    if(!user){
      callback(new Error('Failed to authenticate oAuth token'));
    }
    req.user = user;
    return callback();
  })(req, null, callback);

make a custom handler for passport, and use null for response object (not needed in custom implementation when handling your own error's)

Still, would be much easier to have the response object available. Especially when combining with swagger router... Router is not able to work with passport as it does work with standard express router..

Hope this helps..

[whitlockjc]

I agree that this needs to be done. One thing I neglected to remember when discussing this in #214 is that you need to call passport.authenticate programmatically from within the swagger-security function which explains why the usage you have and the usage in the Passport documentation is different. Of course, manually invoking a middleware does indeed require the res object.

This has a direct impact on some other tooling using this feature so we'll need to do a patch release when this does come out. I need to talk with @theganyo about this a little more before I jump into this.

[chrisfelix82]

Really would love to see this fixed So I can return 401 or 403 from my custom middleware Thanks!

[deefactorial]

+1 for this having an easy way to integrate passport middleware is essential to a secure API

[seansean11]

+1 this feature will determine whether or not I use swagger on my project. I'll try to hack something together following the code above.

[whitlockjc]

Don't let swagger-tools keep you from using Swagger. The only reason this feature isn't fixed already is because fixing it means rolling a major release to fix it which has a lot of other issues associated with it. Besides, you can just omit using the swagger-security middleware until it's fixed while still using the other completely stable and working pieces. I'll get to it ASAP.

[seansean11]

Thanks @whitlockjc! I admittedly don't have a ton of experience with Node/API dev, so I'm struggling to find out how to implement PassportJS with Swagger as it stands. Should I just write the PassportJS login/registration routes like I normally would, outside of Swagger for now?

[whitlockjc]

Swagger does not have a Passport integration or vice versa. You could just as easy throw PassportJS in front of your API without it being Swagger aware. The idea behind swagger-security is that you can register security handlers that are named the same as those in your Swagger document and then swagger-security would do the authentication/authorization for you. I agree we should get this done but in all honesty, you could live without swagger-security right now and then port to it when it's fixed. I'd love to help when we get to that point.

[deefactorial]

if you would like to see an example of how I integrated passport into my project you can see it here:

openmoney-api

I have been able in integrate the security handler the issue I'm having is with the response validation. The response validator expects a parsed json output and passport outputs stringifyied json. So I have to put the oauth2orize token end point before it validates the req/res .

[whitlockjc]

Response validation works based on your response schema definitions so it uses what you tell it to.

[whitlockjc]

I think I'm going to take a stab at getting 1.0.0 out and this would be included in the work.

[deefactorial]

I've opened an issue on OpenAPI spec about the response validation issue I'm having.
OAI/OpenAPI-Specification#552

Thanks @whitlockjc for your input about the schema definitions, your right that it can be validated as a type string but I was looking for something more.

[whitlockjc]

swagger-tools does response validation so I'm not sure what problem you're running into.

[deefactorial]

I'm moving the issue to here:
#333

sorry for the noise on this issue.

[djabraham]

Forgive me if this is already there, but I don't see any option to return anything other than an error.

I would like the ability to set failed login attempts as warnings, rather than errors. I think errors should be reserved for application faults of some kind, that are deserving of special attention.

[AilisObrian]

+1 and ping! I want 401 not 403.

[djabraham]

I think I eventually found a reference to req in the res and just did something like this..

app.use(middleware.swaggerSecurity({
  bearer: function (req, def, scopes, callback) {
    if (req.isAuthenticated()) {
      return callback();
    }
    req.res.status(401).json({
      message: "Please Authenticate"
    });
    req.res.end();
  }
}));

EDIT: I should add that this is a quick fix and I did not yet check for memory leaks, etc.. I am not rolling to production anytime soon.

[AilisObrian]

In my case,

function jwtVerifyPromise(token, verifyKey) {
  return new Promise((resolve, reject) => {
    const tokens = token.split(' ');
    if (tokens[0] === 'Bearer') {
      jwt.verify(tokens[1], verifyKey, (err, verified) => {
        if (err) {
          reject(err);
        } else {
          resolve(verified);
        }
      });
    } else {
      reject(Error('Not JsonWebToken'));
    }
  });
}

function _isUserToken(verifyKey) {
  return (req, authOrSecDef, scopesOrApiKey, cb) => {
    jwtVerifyPromise(scopesOrApiKey, verifyKey)
        .then(verified => {
          req.security = {
            authOrSecDef: authOrSecDef,
            scopesOrApiKey: scopesOrApiKey,
            verified: verified,
          };
          cb();
        })
        .catch(err => {
          err.statusCode = 401;
          // XXX: Raise 401, not 403!
          //      https://github.com/apigee-127/swagger-tools/issues/203

          cb(err);
        });
  };
}

[rmharrison]

I'm not sure if this is correct, but per @djabraham, I'm returning res.res withing calling the callback.
Using what I think is the swagger-express-mw error response (for consistency with validators):

  var config = {
    appRoot: __dirname, // required config
    swaggerSecurityHandlers: {
      JWT: function (req, authOrSecDef, scopesOrApiKey, cb) {
        return req.res.status(401).json({
            error: {
              message: "swaggerSecurityHandlers errors",
              statusCode: 401,
              code: 'client_error',
              errors: [{
                code: "Access denied!",
                name: "AuthenticationError",
                message: "No Authorization token in header, please include: `Authorization: Bearer <jwt_access_token>`",
                authorization_header: scopesOrApiKey
              }]
            }
          });
      }
    }
  };

I get very ugly error reporting if I use cb({...error packet...}).

Is it bad practice to not use the cb(err)?

[djabraham]

You can return an error object, which loggers format differently. They probably check it using instanceof Error.

var err = new Error('Failed to authenticate using bearer token');
err['statusCode'] = 403; // custom error code
callback(err);

UPDATE:
For example: https://github.com/nomiddlename/log4js-node/blob/master/lib/layouts.js#L31

[riazXrazor]

tnx @djabraham i managed to fix it this way

function (req, authOrSecDef, scopesOrApiKey, cb) {

        // check header
        var token = req.headers['authorization'];

        // decode token
        if (token) {

            // verifies secret and checks exp
            jwt.verify(token, app.get('superSecret'), function(err, decoded) {
                if (err) {
                    req.res.status(403).json({
                        "status": {
                            "statusCode": 50002,
                            "isSuccess" : false,
                            "message"   : 'Failed to authenticate token.'
                        }
                    });
                    req.res.end();
                    return;
                } else {
                    // if everything is good, save to request for use in other routes
                    if(!decoded)
                    {
                      
                        req.res.status(403).json({
                            "status": {
                                "statusCode": 50002,
                                "isSuccess" : false,
                                "message"   : 'Invalid token'
                            }
                        });
                        req.res.end();
                        return;

                    }
                    else {
                        req.user = decoded;
                        req.User = function () {
                            return user.findOne({
                                where : req.user.id
                            })
                        }
                        cb();
                        return;
                    }
                }
            });

        } else {

            // if there is no token
            // return an error
            req.res.status(403).json({
                "status": {
                    "statusCode": 50002,
                    "isSuccess" : false,
                    "message"   : 'access denied!'
                }
            });
            req.res.end();
            return;
        }

    }

[jonaslagoni]

Can anybody confirm that the response object is no longer accessible in the request object? Which makes the above solutions not working as of 2019 😞

Update
As a quick fix I just parsed the response object to the handler

return handler(req, secDef, getScopeOrAPIKey(req, secDef, name, secReq), cb, res);

Forked repository: https://github.com/jonaslagoni/swagger-tools/tree/parsing_responseobject_to_securityhandlers

[YuJianghao]

You can return an error object, which loggers format differently. They probably check it using instanceof Error.

var err = new Error('Failed to authenticate using bearer token');
err['statusCode'] = 403; // custom error code
callback(err);

UPDATE:
For example: https://github.com/nomiddlename/log4js-node/blob/master/lib/layouts.js#L31

I fix this by these steps ...

First.

Feed an error to callback function as @djabraham said. Then the err object have err.statusCode and err.message

Second

INSIDE initializeMiddleware

require('swagger-tools').initializeMiddleware(swaggerDoc, (middleware) => {
    app.use(middleware.swaggerMetadata())
    app.use(middleware.swaggerSecurity({
        basic_auth: basic_auth
    }));
    app.use(middleware.swaggerValidator());
    app.use(middleware.swaggerRouter(options));
    app.use(middleware.swaggerUi())

    // Error handlers
    app.use((err, req, res, next) => {
        if (err.statusCode === 401) {
            // do something you like
            return res.status(401).send({
                code: 401,
                message: err.message
            })
        }
        res.status(500).send(err)
    })
    app.listen(3000, () => console.log(app.locals.title, 'listening on port 3000!'))
})

Third

This is what I've got..

{
    "code": 401,
    "message": "Failed to authenticate using bearer token"
}

I think it's a right way to fix this.

THANKS to @djabraham again!

[maximgoncharenko]

Guys, what about bumping major version and make these changes after more than 5 years?