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 should not throw an error on mass deletions #175

Closed
thosakwe opened this Issue Apr 25, 2016 · 3 comments

Comments

Projects
None yet
2 participants
@thosakwe
Copy link

thosakwe commented Apr 25, 2016

Hi guys, really enjoy the work you guys have done. Just one problem.

See, I made this package, feathers-seeder. A key part of its functionality is being able to delete all records from a service if the configuration object dictates to do so.

However, this is prevented by this section of code in src/hooks/restrict-to-owner.js:
https://github.com/feathersjs/feathers-authentication/blob/master/src/hooks/restrict-to-owner.js#L15-17

if (!hook.id) {
      throw new errors.MethodNotAllowed(`The 'restrictToOwner' hook should only be used on the 'get', 'update', 'patch' and 'remove' service methods.`);
}

So if I call the following, the operation will fail:

service.remove(null, additionalParams);

As you can imagine, this renders feathers-seeder more or useless.

I am going to submit a PR that fixes this. Instead of checking if the hook has an id, the code should be changed to see if the method matches 'get', 'update', 'patch' or 'remove'.

thosakwe added a commit to thosakwe/feathers-authentication that referenced this issue Apr 25, 2016

@thosakwe

This comment has been minimized.

Copy link
Author

thosakwe commented Apr 25, 2016

Here is the PR (#176).

@ekryski

This comment has been minimized.

Copy link
Member

ekryski commented May 21, 2016

Providing the ability to conditionally run hooks (#210) should solve this problem.

@ekryski ekryski added this to the 1.0 milestone May 21, 2016

@ekryski

This comment has been minimized.

Copy link
Member

ekryski commented Jun 30, 2016

We actually came up with a better solution. You can simply wrap any existing hooks in your own, like so:

import { hooks } from 'feathers-authentication';

exports.hashPassword = function(options) {
  // Add any custom options

  function(hook) {
    return new Promise((resolve, reject) => {
      if (myCondition !== true) {
        return resolve(hook);
      }

      // call the original hook
      hooks.hashPassword(options)(hook)
        .then(hook => {
          // do something custom
          resolve(hook);
        })
        .catch(error => {
          // do any custom error handling
          error.message = 'my custom message';
          reject(error);
        });
    });
  });
}

This provides the ultimate flexibility and doesn't require a change to the core hooks.

@ekryski ekryski closed this Jun 30, 2016

@ekryski ekryski removed the Backlog label Jun 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.