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

restrictToOwner hook needs to throw an error and not scope the query #127

Closed
beeplin opened this Issue Mar 25, 2016 · 2 comments

Comments

Projects
None yet
2 participants
@beeplin
Copy link

beeplin commented Mar 25, 2016

when owner is set to true, the source code uses the following logic:

    if (options.owner && !authorized) {
      // NOTE (EK): This just scopes the query for the resource requested to the
      // current user, which will result in a 404 if they are not the owner.
      hook.params.query[options.ownerField] = id;
      authorized = true;

which is fine for find method, coz when finding, if the query.userId is wrong, feathers will return 404.
But at least in mongoose, update/patch/remove/get don't rely on query to execute, and the behavior is like this:
update: the modification is executed despite it's neither admin or owner
patch and remove: the modification is not executed, and no error thrown. the unchanged data is returned to client
get: the data is returned to client despite that it's neither admin or owner.

so I think the right way to do this is just like what EK wrote in TODO:

      // TODO (EK): Maybe look up the actual document in this hook and throw a Forbidden error
      // if (field && id && field.toString() !== id.toString()) {
      //   throw new errors.Forbidden('You do not have valid permissions to access this.');
      // }

and the code could simply be like this (untested):

this.get(hook.id).then(data => {
  if (data[options.ownerField].toString() !== id.toString()) {
     throw new errors.Forbidden('You do not have valid permissions to access this.');
  }
  return hook;
});

(restrictToOwner has the same issue.)

@ekryski

This comment has been minimized.

Copy link
Member

ekryski commented Mar 26, 2016

@beeplin you are absolutely correct. I had thought about this but I think was too tired to really grok my error. I'm going to be doing a patch tomorrow for this so that it will throw a legit Forbidden error if you are not authorized.

@ekryski ekryski changed the title [bug] the new hook restrictToRoles with owner:true not work correctly with update/patch/remove restrictToOwner hook needs to throw an error and not scope the query Mar 26, 2016

@ekryski ekryski added the Bug label Mar 26, 2016

@ekryski ekryski modified the milestones: 1.0, 0.7 Mar 26, 2016

@beeplin

This comment has been minimized.

Copy link
Author

beeplin commented Mar 26, 2016

looking forward to v0.7 :))

@ekryski ekryski referenced this issue Mar 30, 2016

Merged

0.7 Release #139

17 of 17 tasks complete

@ekryski ekryski closed this in c73fea5 Mar 30, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.