Allow accessibleBy to throw ForbiddenError when the query denied

[nitzano]

Is your feature request related to a problem? Please describe.

First thanks for this amazing library, it's really helpful in many ways.

By using @casl/mongoose and for example ExpressJS I'm trying to find a way to distinguish between
"document not found" and "forbidden" errors.

For example here:

app.get("/:todoId", async (req, res, next) => {
  try {
    const todo = await ToDo.findById(req.params.todoId).accessibleBy(req.ability);
    // problem: if todo is null - was it not found or forbidden?
    return res.json(todo);
  } catch (err) {  
    return next(err);
  }
}); 

In both cases when either the document is not found or the user is not allowed to access it
null is returned in todo variable.

Describe the solution you'd like

By giving the option (maybe by adding an argument to .accessibleBy or even better: plugin configuration) to throw ForbiddenError automatically in case of authorization failure we can return 403 status to any caught ForbiddenError thrown by the endpoints.

For example:

function errorHandler (err, req, res, next) {
  ...
  if (err.name === 'ForbiddenError') {
     res.status(403)
  } 
  ...
  res.render('error', { error: err })
}

Describe alternatives you've considered (optional)

Using mongoose's .orFail() method, but instead integrating this ability in @casl/mongoose can really ease the usage in most of the cases instead of patching every endpoint.

[stalniy]

Hi,

Thanks for the issue. Frankly speaking, it’s requirement is a bit hard to implement. And this is why:

• there may not be records in the database that correspond to defined permissions. should it throw error?
• accessibleBy returns a mongoose query instance which may be evaluated later. So, I did monkey patching of few it’s methods to return mocked value without reaching database. However, to be 100% sure, I also added non-existing property, in case query will be sent to database, so we get an empty result set.

Thus exception throwing needs to be implemented on internals of mongoose query, which is a bit unsafe and probably won’t work in all cases.

I also need to say that I haven’t checked mongoose source code for about year so maybe something changed and there is a better way. If you know how to do this, please share :) also if you can implement it and send PR, it’d be awesome

[nitzano]

Thanks for the quick reply, I think another (maybe easier) solution can be maybe changing orFail() response from returning DocumentNotFoundError to ForbiddenError but yet not sure if and how it's possible.

I'll try to look into mongoose's source code as well and maybe come up with something.

[stalniy]

Currently the workaround is to check permissions on subject type before constructing query.

Just do

if (!ability.can(“read”, “Todo”)){
  // user has no access to do at all
}

// user has access let’s see whether there is something in the database for him
Todo.accessibleBy(...)

[stalniy]

I found private _pre method in mongoose.Query that allows to add a pre exec hook which I can use to implement this functionality

[nitzano]

@stalniy : Is it documented somewhere? I never heard of it or saw it in Mongoose's documentation.

[stalniy]

Undocumented private method. There will be additional logic that will prevent data exposure in case of insufficient permissions.

update:

I created an issue Automattic/mongoose#9784

[stalniy]

I think that it's possible just to monkey patch Query.exec, at least all tests pass

[stalniy]

will be released in the next version

[nitzano]

will be released in the next version

@stalniy Thank you so much!

[stalniy]

🎉 This issue has been resolved in version 4.0.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[anuraagy]

This is not working for me and I'm still getting a null. Anyone else having this issue? I'm using casl 5.4.3 and casl/mongoose 5.0.0

const item = await Item.findOne({ _id: ObjectId(id) }).accessibleBy(req.session.abilities, "read");

[stalniy]

In order, to reproduce your case I need to see rules. There is only one reason for this thing to throw, if there is no rules or it can understand that rules will evaluate to nothing.

If there are conditions which are checked by db, and there is nothing found due to restrictions of conditions, there is no way to know whether the item exist and forbidden or it doesn’t exist at all. Very likely it’s your case

[anuraagy]

That makes sense, your evaluation is correct! Thank you!