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

mongoose .orFail() does not work when permissions are implicitly denied #218

Closed
m-weeks opened this issue Aug 26, 2019 · 5 comments
Closed
Assignees

Comments

@m-weeks
Copy link
Contributor

m-weeks commented Aug 26, 2019

I ran into this issue accidentally today, but the way my application is set up, it expends models not found to throw an error.

ex:

    var user = await User.findById(req.params.id)
      .accessibleBy(req.ability)
      .orFail();

    res.json({ user });

When the permission to view users is set, it works fine and dandy. The conditions for this query were { _id: '5d600d0ea21964033b4fc1bb', '$and': [ {} ] }.

But if the permission is implicitly denied, the conditions are { _id: '5d600d0ea21964033b4fc1bb', __forbiddenByCasl__: 1 }. The problem is though that the user variable is returning null, and not throwing an exception.

@stalniy
Copy link
Owner

stalniy commented Aug 26, 2019

Could you please show the implementation of orFail function?

@m-weeks
Copy link
Contributor Author

m-weeks commented Aug 26, 2019

Query.prototype.orFail = function(err) {
  this.map(res => {
    switch (this.op) {
      case 'find':
        if (res.length === 0) {
          throw _orFailError(err, this);
        }
        break;
      case 'findOne':
        if (res == null) {
          throw _orFailError(err, this);
        }
        break;
      case 'update':
      case 'updateMany':
      case 'updateOne':
        if (get(res, 'result.nModified') === 0) {
          throw _orFailError(err, this);
        }
        break;
      case 'findOneAndDelete':
        if (get(res, 'lastErrorObject.n') === 0) {
          throw _orFailError(err, this);
        }
        break;
      case 'findOneAndUpdate':
      case 'findOneAndReplace':
        if (get(res, 'lastErrorObject.updatedExisting') === false) {
          throw _orFailError(err, this);
        }
        break;
      case 'deleteMany':
      case 'deleteOne':
      case 'remove':
        if (res.n === 0) {
          throw _orFailError(err, this);
        }
        break;
      default:
        break;
    }

    return res;
  });
  return this;
};

or you can view it at https://github.com/Automattic/mongoose/blob/b0fd1b0f9036505f1556155479d207c1fd3c4a8e/lib/query.js

@stalniy
Copy link
Owner

stalniy commented Aug 26, 2019

Thanks, I didn't know about orFail method of mongoose Query :)

I think this is a bug but I'll need some time to confirm this.

@stalniy stalniy added the bug label Sep 2, 2019
@stalniy
Copy link
Owner

stalniy commented Sep 2, 2019

Sorry, for the long response. So, yes it's a bug. @casl/mongoose patches mongoose.Query.prototype.exec object to prevent unnecessary request to database when it's clear that nothing will be returned.

Update: According to implementation details orFail adds additional transformation (callback function) which is applied after result from db is returned. This logic lives in exec method, that's why when user has no access the transformation doesn't work

@stalniy stalniy self-assigned this Sep 2, 2019
@stalniy
Copy link
Owner

stalniy commented Sep 14, 2019

fixed in @casl/[email protected].

Thanks for the issue!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants