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

User (Entity) needs to be updated on the socket after authentication #293

Closed
sunabozu opened this Issue Sep 17, 2016 · 10 comments

Comments

Projects
None yet
5 participants
@sunabozu
Copy link

sunabozu commented Sep 17, 2016

So here is the problem.

  1. I authenticate
  2. I have a required role to create a particular resource, protected by the hook, and I create it successfully
  3. I edit my database (both directly and through the client), removing the required role from the user document
  4. I try to create the same resource and I succeed (<== problem)
  5. I logout, authenticate again and now the hook is working, I can't create the resource

So it seems like it caches the user object from the database or (which is much worse) just checks the object sent by the user himself. In the later case it's a security flaw, since any user can assign himself any role on the client side.

@marshallswain

This comment has been minimized.

Copy link
Member

marshallswain commented Sep 17, 2016

@Sunabozo, are you modifying the token payload at all? As in, are you putting storing permissions in the token? Where is the auth getting its stale data?

@sunabozu

This comment has been minimized.

Copy link
Author

sunabozu commented Sep 17, 2016

@marshallswain I don't touch the token. I use restrictToRoles in the list of the before hooks for my model. Particularly for the create method:

exports.before = {
  all: [],
  find: [],
  get: [],
  create: [
    authHooks.restrictToRoles({
      roles: ['admin']
    })
  ]
};

The user model has a roles field, where the roles are stored. So I modify that field for the current user and nothing else.
I hope I understood your questions correctly.

@marshallswain

This comment has been minimized.

Copy link
Member

marshallswain commented Sep 17, 2016

Yeah, looks like you understood what I was trying to say. What hooks are you running before the restrictToRoles hook?

@marshallswain

This comment has been minimized.

Copy link
Member

marshallswain commented Sep 17, 2016

I recommend using Visual Studio Code's debugger and setting a breakpoint here: https://github.com/feathersjs/feathers-authentication/blob/master/src/hooks/restrict-to-roles.js#L32 To see if you're still getting stale data, somehow. Using the populateUser hook just before this one should query the database for fresh data every time, unless you have some kind of cacheing mechanism in place. Not sure what your db setup is.

@marshallswain

This comment has been minimized.

Copy link
Member

marshallswain commented Sep 17, 2016

Setting up VSCode is a one-line change with a generated app. Just press the green play button to start the debugger for the first time and it will prompt you for a configuration type. Choose node, then change the highlighted line to look like this:

screen shot 2016-09-17 at 1 00 09 pm

Then you can just open that file in your node_modules and set a break point on that line. Run the query and it will stop at that point so you can inspect just like with Chrome debug tools.

@sunabozu

This comment has been minimized.

Copy link
Author

sunabozu commented Sep 17, 2016

it's the only hook I use for that model. Sorry, I can't use the VS Code at the moment, but I checked the sources and it seems like this hook checks the roles on the object sent by the browser, not the actual resource in the database.

So the solution here is simple - I just need to use these hooks first:

verifyToken
populateUser
restrictToAuthenticated

Reading the documentation I didn't realise that I must use restrictToRoles in conjunction with other hooks. I think it should be mentioned explicitly here. Sorry if I misunderstood the whole concept, I'm new to Feathers.

@marshallswain

This comment has been minimized.

Copy link
Member

marshallswain commented Sep 17, 2016

I completely agree. If you don't mind creating a PR that would be very much
appreciated. If that's not an option for you, then maybe just open an issue
in the Feathers-docs repository. Glad you got it working.
On Sat, Sep 17, 2016 at 1:12 PM troorl notifications@github.com wrote:

it's the only hook I use for that model. Sorry, I can't use the VS Code at
the moment, but I checked the sources and it seems like this hook checks
the roles on the object sent by the browser
https://github.com/feathersjs/feathers-authentication/blob/master/src/hooks/restrict-to-roles.js#L35,
not the actual resource in the database.

So the solution here is simple - I just need to use these hooks first:

verifyToken
populateUser
restrictToAuthenticated

Reading the documentation I didn't realise that I must use restrictToRoles
in conjunction with other hooks. I think it should be mentioned explicitly
here https://docs.feathersjs.com/authorization/bundled-hooks.html.
Sorry if I misunderstood the whole concept, I'm new to Feathers.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#293 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAH3WfdTDE8JwEtSRMWz-_v_whgLokmyks5qrDuogaJpZM4J_qwA
.

@ekryski

This comment has been minimized.

Copy link
Member

ekryski commented Nov 21, 2016

This may or may not be an issue with the new permissions. Will need to audit to see if we can reproduce with the new feathers-permissions module. I think this would only be happening on sockets. Over HTTP it's a new request and the user gets populated each time whenever the request is authenticated.

@daffl is working on having the authenticated socket listen for entity updates (ie. user updates) so that they are always up to date.

@ekryski ekryski changed the title restrictToRoles hook may not work if user's roles are changed during the session Permissions checks may not work if user's roles are changed during socket connection Nov 21, 2016

@ekryski ekryski added the Bug label Nov 21, 2016

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

@ekryski

This comment has been minimized.

Copy link
Member

ekryski commented Dec 30, 2016

💯 this is an issue. We're working on a fix for this and this is the last major thing required for Auth 1.0 besides docs IMHO.

@ekryski ekryski changed the title Permissions checks may not work if user's roles are changed during socket connection User (Entity) needs to be updated on the socket after authentication Apr 13, 2017

@jerfowler

This comment has been minimized.

Copy link
Contributor

jerfowler commented May 26, 2017

What is the best workaround for this? I've just been using app.service('users').get(id).then()... in all my hooks just to make sure I have the latest user info.

Is there a way to force a silent reauthentication with the socket.io connection in the background after I update the user in a hook?

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.