ForbesLindesay / connect-roles

Provides dynamic roles based authorisation for node.js connect and express servers.
749 stars 62 forks source link

Add 'next' parameter to failureHandler #29

Open yamsellem opened 10 years ago

yamsellem commented 10 years ago

The actual failureHandler signature is req, res, act so the response has to be handled here. If we want to handle it elsewhere, there is only one option: throw an error in failureHandler like:

failureHandler: function (req, res, act) {
  throw new Error();
});

It would have been easier to change the method signature to req, res, next, act. So the above code could be:

failureHandler: function (req, res, next, act) {
  next(new Error());
});
iliocatallo commented 9 years ago

+1!

ForbesLindesay commented 9 years ago

What's the advantage next(new Error()); over throw new Error(); It seems preferable to me to explicitly throw? Is it that you want to do asynchronous operations in the failureHandler that could result in an error?

iliocatallo commented 9 years ago

Hi,

If a user requires a resource (e.g., /user/12) she cannot access, I would like to show the 404 page (similarly to what github does for private repositories). One way to do so would be to setup a generic error handler:

app.use(mError.errorPage);

whose responsability is to inform the user about the error; and let the failureHandler function create and augment the Error object:

var err = new Error("The Web Server could not find the requested resource");
err.http_code = 404;
err.http_error_message = "Page not Found";

At that point, moving to the error handler would be as simple as next(err).

Thanks!

ForbesLindesay commented 9 years ago

The 404 case is an interesting one. I have to admit that I hadn't considered that. I think it would be preferable to throw that error, rather than passing it to next though. The advantage of forcing people to throw is that it prevents you from accidentally calling next() or next(undefined) which would undo the whole process of denying access.