Feathers params not available to user service hooks

[thomas-p-wilson]

Steps to reproduce

feathers()
    .configure(hooks())
    .configure(rest())
    .configure(socketio((io) => {
        io.use((socket, next) => {
            socket.feathers.data = 'Hello, world';
            next();
        });
    }))
    .use((req, res, next) => {
        req.feathers.data = 'Hello world';
        next();
    });

The above should give us feathers parameters for both RESTful and Socket requests, and indeed under any other circumstance it does!

The issue here is specifically with https://github.com/feathersjs/feathers-authentication-local/blob/master/src/verifier.js#L73, wherein we set the query, but do not pass in the feathers params.

Expected behavior

A find hook on the user service should be provided with any Feathers params specified in socket and rest middleware.

Actual behavior

The find hook on the user service does not receive any additional data other than the query constructed at https://github.com/feathersjs/feathers-authentication-local/blob/master/src/verifier.js#L67

System configuration

Module versions (especially the part that's not working):

• feathers-authentication-local - 0.3.4

I've fixed the issue locally, but without sufficient quality for a PR. Will clean up and PR shortly!

[ekryski]

@thomas-p-wilson I think this does make sense but what is your use case for needing the feathers params at that point? It is intended to be an internal call so the intention was that if you needed to pass additional params when looking up the service that you should just extend the verifier and do so explicitly.

[thomas-p-wilson]

That works, yes. But if I extend the verified and the verified changes upstream, then it's more work. The feathers hooks are designed to receive parameters as specified in the RESTful and Socket middleware, and it seems sensible for it to remain consistent across the board.

I use hooks for auditing and tenancy related tasks, and had the hardest time figuring out why only auto wouldn't work. Took me the better part of a day to figure out why params wouldn't pass through.

If you intend to keep it this way, then it would be good to document the fact that params don't work here like they do elsewhere.

On Apr 14, 2017 5:11 PM, "Eric Kryski" notifications@github.com wrote:

@thomas-p-wilson https://github.com/thomas-p-wilson I think this does make sense but what is your use case for needing the feathers params at that point? It is intended to be an internal call so the intention was that if you needed to pass additional params when looking up the service that you should just extend the verifier and do so explicitly.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/feathersjs/feathers-authentication-local/issues/14#issuecomment-294237514, or mute the thread https://github.com/notifications/unsubscribe-auth/AAkeuzt8swcBea3yjC6u8rSEMqeOKjmKks5rv-DqgaJpZM4M9xSR .

[ekryski]

@thomas-p-wilson for sure, I can empathize with the frustration. We may be able to support this in a secure way but can you explain your use case in a bit more detail?

What sort of data do you need to pass through at this point and why? Just trying to get a better understanding...

[thomas-p-wilson]

Sorry, was on my phone most of the day yesterday (happy easter right?).

I did a little experiment just to confirm my suspicions, and was able to verify that Feathers params are not the same as query params as it concerns them being passed into a service. Consider the following:

{
  query: {
    param1: 'hello'
  },
  provider: 'rest',
  data: 'Hello world'
}

You can see that the GET parameter gets passed in under the query object, while the req.feathers.data param gets passed in at the root level. It is not the query bit I'm interested in, rather those root-level Feathers params.

With regard to my PR #15, the solution to your concerns, if I'm not mistaken, would be to omit the query key entirely. Retaining the remainder of the object should pose no risk, as they do not provide the end-user with the opportunity to inject arbitrary content. Apologies for overlooking this injection possibility.

All that said, if you prefer the project remain unaltered, I would alternately propose a documentation update to make it clear that a custom verifier is the way to go. My reasoning was merely to keep things consistent with the rest of the feathers ecosystem, and to keep project maintainability simpler by avoiding a custom verifier. But it's also a completely valid design decision to go the other way. I leave it in your hands!

[thomas-p-wilson]

Addendum: just reviewed my PR and noticed that I don't keep the query parameters around at all, as I reassign that key with the query produced on https://github.com/feathersjs/feathers-authentication-local/blob/master/src/verifier.js#L68

[ekryski]

just reviewed my PR and noticed that I don't keep the query parameters around at all, as I reassign that key with the query produced on https://github.com/feathersjs/feathers-authentication-local/blob/master/src/verifier.js#L68

Yup you are correct. I missed that as well. I might just add a note in there so that in the future we don't accidentally merge params in and inadvertently introduce a security leak, but I think your PR is fine.

@thomas-p-wilson I'm fine merging #15 but you haven't explained your use case where you need to pass additional data down the pipe like that. 😉 I haven't run into that situation so I'm curious. If you could shed some light there that would be great and then I think we can get this merged and released!

We'll likely need to update the OAuth and JWT feathers auth plugins as well.

[thomas-p-wilson]

Yes, sorry. I'm on my phone presently. Will hop on and write out a case with example in the morning!

Also will take the opportunity to test the PR again and make sure I haven't missed anything. And will add the note as you suggest

On Apr 18, 2017 12:14 AM, "Eric Kryski" notifications@github.com wrote:

just reviewed my PR and noticed that I don't keep the query parameters around at all, as I reassign that key with the query produced on https://github.com/feathersjs/feathers-authentication-local/ blob/master/src/verifier.js#L68

Yup you are correct. I missed that as well. I might just add a note in there so that in the future we don't accidentally merge params in and inadvertently introduce a security leak, but I think your PR is fine.

@thomas-p-wilson https://github.com/thomas-p-wilson I'm fine merging #15 https://github.com/feathersjs/feathers-authentication-local/pull/15 but you haven't explained your use case where you need to pass additional data down the pipe like that. 😉 I haven't run into that situation so I'm curious. If you could shed some light there that would be great!

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/feathersjs/feathers-authentication-local/issues/14#issuecomment-294670980, or mute the thread https://github.com/notifications/unsubscribe-auth/AAkeuxm-OPeQZotltoRIIfpSQcCvSe0bks5rxDi9gaJpZM4M9xSR .

[ekryski]

@thomas-p-wilson ping! 😄

[thomas-p-wilson]

Sincerest apologies! I was temporarily pulled off project, and hadn't had the time to get back to this.

The simplest explanation for why I need to pass feathers params down to the hooks is that I am working on an application that supports multi-tenancy through the use of an organization id. In order to determine which user is logging in, I need to have that organization id available when I make a request to the user service.

So while a normal request might just look for a match for the username/email and password, my request must look for a match for the username/email, password, and organization id. So the params that get passed to the service end up looking like this:

{
  query: {
    email: 'john.smith@example.org',
    $limit: 1
  },
  organization_id: 'abc123'
}

All my services have hooks that deal with the tenancy stuff by receiving the organization_id as a feathers parameter, as illustrated above. It worked fine everywhere but login ;)

So based on your suggestion, I wrote a verifier that could pass in my org id like I needed it. And that's a fine solution. My only push for this is that doing so prevents folks from needing to create a verifier, and should the structure of the verifier ever change in the future, it's one less place for folks to refactor if they want to update :)

Also note that based on your earlier concern, I amended my pull request to omit query (overridden by auth-local anyways), provider, headers, session, and cookies, leaving only those parameters explicitly added by the developer as part of req.feathers.<param> or socket.feathers.<param>. I hope this is sufficient! Let me know if you have any other concerns, and again, apologies for the long wait :(

[ekryski]

Sorry I missed this comment @thomas-p-wilson! Thanks for updating your PR and that makes total sense. What I've done to support this use case is exactly what you did - create a custom verifier.

Thanks for the explanation. I think you are right that it is a little nicer for a developer so that they don't have to create a custom verifier just to support additional/non-query params although that really is the whole purpose behind a custom verifier, when you need to do something "custom".

[thomas-p-wilson]

Agree. I guess I just see it more like the custom verifier can be used to perform some "heavy lifting". You can do whatever you darn well please in a custom verifier. And I suppose my original thought was that I don't necessarily need to do any heavy lifting, since my services manage all that and they only need one additional param. After this discussion though, I think I see both points. And at the end of the day, it's really just a design decision. And that decision rests with you, a fact with which I am perfectly content.

On Wed, Jun 21, 2017 at 6:07 PM, Eric Kryski notifications@github.com wrote:

Sorry I missed this comment @thomas-p-wilson https://github.com/thomas-p-wilson! Thanks for updating your PR and that makes total sense. What I've done to support this use case is exactly what you did - create a custom verifier.

Thanks for the explanation. I think you are right that it is a little nicer for a developer so that they don't have to create a custom verifier just to support additional/non-query params although that really is the whole purpose behind a custom verifier, when you need to do something "custom".

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/feathersjs/feathers-authentication-local/issues/14#issuecomment-310218933, or mute the thread https://github.com/notifications/unsubscribe-auth/AAkeu-hbINRHphYEwK6dXbaz7D_Vwh3cks5sGZQLgaJpZM4M9xSR .