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

RequireAdmin on userService #36

Closed
mastertinner opened this Issue Feb 2, 2016 · 6 comments

Comments

Projects
None yet
3 participants
@mastertinner
Copy link
Contributor

mastertinner commented Feb 2, 2016

I followed the guides on http://docs.feathersjs.com/authorization/readme.html. Doing so, I created a userService which provides the /api/users endpoint. I'm trying to make it a secure endpoint using

userService.before({
  find: [auth.requireAdmin]
  remove: [auth.requireAdmin],
  create: [auth.requireAdmin, authHooks.hashPassword('password')]
});

where auth is an instance of the Auth class:

class Auth {

  requireAuth(hook) {
    if (!hook.params.user) {
      throw new Error('You must be logged in to do that.');
    }
  }

  requireAdmin(hook) {
    if (!hook.params.user) {
      throw new Error('You must be logged in to do that.');
    }
    if (!hook.params.user.admin) {
      throw new Error('Only admins can do that.');
    }
  }

}

When I'm trying to POST to the /api/login endpoint, the server runs into a 500 error and sends the following as a response to my request:

{
  "hook": {
    "params": {
      "internal": true,
      "query": {
        "username": "my-username"
      }
    },
    "method": "find",
    "type": "before"
  }
}

If I remove the find: [auth.requireAdmin] on userService, it works fine. Does that mean that I cannot put an ACL onto the GET /api/users endpoint? I think it's not a good idea to have this endpoint open because it would mean that anybody could look up my user list...

@mastertinner

This comment has been minimized.

Copy link
Contributor Author

mastertinner commented Feb 2, 2016

So I just learned about the restrictToSelf(idProp) method described here: http://docs.feathersjs.com/authorization/bundled-hooks.html#restricttoselfidprop. However, this doesn't seem to solve my problem... My hooks on the userService now look as follows:

userService.before({
  find: [authHooks.restrictToSelf()],
  get: [authHooks.restrictToSelf()],
  remove: [auth.requireAdmin],
  create: [
    auth.requireAdmin,
    authHooks.hashPassword(),
    authHooks.toLowerCase('username')
  ],
  update: [
    auth.requireAdmin,
    authHooks.hashPassword(),
    authHooks.toLowerCase('username')
  ],
  patch: [
    auth.requireAdmin,
    authHooks.hashPassword(),
    authHooks.toLowerCase('username')
  ]
});

but I'm still having the issue described above...

@marshallswain

This comment has been minimized.

Copy link
Member

marshallswain commented Feb 3, 2016

The solution to this is probably to update the hooks with a check for a hook.params.provider. If there's no provider then it's an internal call. I won't be able to get to this for a while. @mastertinner If you want to give it a shot, I'd really appreciate any feedback you can provide on how it works.

marshallswain added a commit that referenced this issue Feb 4, 2016

Check for params.provider in requireAuth hook
If there's no provider, it was an internal call, so let it through.  Fixes #36.
@marshallswain

This comment has been minimized.

Copy link
Member

marshallswain commented Feb 4, 2016

I got some time to look at this tonight and implemented a fix. Please try it out. I'm trying not to change this much since there's a big rewrite underway, but it's not really usable if you can't lock down your user service. :)

@marshallswain marshallswain reopened this Feb 4, 2016

@marshallswain

This comment has been minimized.

Copy link
Member

marshallswain commented Feb 4, 2016

Reopening for docs and tests.

@mastertinner

This comment has been minimized.

Copy link
Contributor Author

mastertinner commented Feb 5, 2016

Thanks a lot for the quick reply and fix, @marshallswain! I saw that you only fixed the requireAuth hook so i took the liberty of applying the same fix for queryWithUserId, setUserId, requireAdminToSetAdmin and restrictToSelf in #39.

@ekryski ekryski modified the milestone: 1.0 release Feb 8, 2016

@ekryski

This comment has been minimized.

Copy link
Member

ekryski commented Feb 28, 2016

This is now documented at http://docs.feathersjs.com/authorization/bundled-hooks.html. I'm going to close as we have another issue as a reminder to add more tests for things like this.

@ekryski ekryski closed this Feb 28, 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.