Resolves #14 - Passes Feathers params to service hooks

[thomas-p-wilson]

Summary

Resolves the issue set forth in #14, where Feathers params specified in REST and Socket middleware are not passed through to the user service during authentication.

[daffl]

Thank you for the pull request. Is this fix working for you? The problem is that user services should discard the password field when params.provider is set (see https://github.com/feathersjs/generator-feathers/blob/master/generators/service/templates/hooks-user.js#L29) so there is no way for local authentication to compare password when passing it through. We could make a copy of params and omit(params, 'provider')

[thomas-p-wilson]

Ahh yes, I understand. It does work for me, but that's an excellent point. I'm on the road at the moment, but once I'm settled I'll update my PR accordingly.

On Apr 14, 2017 1:08 PM, "David Luecke" notifications@github.com wrote:

Thank you for the pull request. Is this fix working for you? The problem is that user services should discard the password field when params.provider is set (see https://github.com/feathersjs/ generator-feathers/blob/master/generators/service/ templates/hooks-user.js#L29) so there is no way for local authentication to compare password when passing it through. We could make a copy or params and omit(params, 'provider')

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/feathersjs/feathers-authentication-local/pull/15#issuecomment-294192939, or mute the thread https://github.com/notifications/unsubscribe-auth/AAkeu9EUx_BikVf8emJPxRLqytaeikL3ks5rv6gAgaJpZM4M9zaW .

[ekryski]

I want to be very careful with this PR. There are potential side-effects (especially security ones) that could occur by passing feathers params to the users service during an authentication call.

As per my comment https://github.com/feathersjs/feathers-authentication-local/issues/14#issuecomment-294237514, the lack of params passing was done intentionally so that one can't call authenticate with query params and potentially get another user's id injected into their JWT access token.

If the use case seems legit, then we'll need to also make sure we omit params.query.

[ekryski]

@thomas-p-wilson thanks for fixing this up. I'm okay to merge this PR but I still also feel like this is precisely the reason we have custom verifiers, if you need to do something "custom" other than username and password.

Second opinions from others are welcome otherwise I'll just merge this in a bit.

[thomas-p-wilson]

If you'd really rather leave it as a custom verified I won't be offended :)

On Jun 21, 2017 5:38 PM, "Eric Kryski" notifications@github.com wrote:

@thomas-p-wilson https://github.com/thomas-p-wilson thanks for fixing this up. I'm okay to merge this PR but I still also feel like this is precisely the reason we have custom verifiers, if you need to do something "custom" other than username and password.

Second opinions from others are welcome otherwise I'll just merge this in a bit.

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

[marshallswain]

@thomas-p-wilson could you resolve the small conflict with the latest master?

[thomas-p-wilson]

Yep. No problem. Will do in the morning!

On Jun 22, 2017 2:19 AM, "Marshall Thompson" notifications@github.com wrote:

@thomas-p-wilson https://github.com/thomas-p-wilson could you resolve the small conflict with the latest master?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/feathersjs/feathers-authentication-local/pull/15#issuecomment-310286669, or mute the thread https://github.com/notifications/unsubscribe-auth/AAkeu_2EIbxTmARYm-dY8mtf3XfhK1BAks5sGgd0gaJpZM4M9zaW .

[thomas-p-wilson]

Pulled down, and merged in master, only to have tests fail. So I pulled down master on its own, and tests are failing in master.

edit: scratch that. updated deps it looks like

On Thu, Jun 22, 2017 at 4:19 AM, Thomas Wilson thomas.paul.w@gmail.com wrote:

Yep. No problem. Will do in the morning!

On Jun 22, 2017 2:19 AM, "Marshall Thompson" notifications@github.com wrote:

@thomas-p-wilson https://github.com/thomas-p-wilson could you resolve the small conflict with the latest master?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/feathersjs/feathers-authentication-local/pull/15#issuecomment-310286669, or mute the thread https://github.com/notifications/unsubscribe-auth/AAkeu_2EIbxTmARYm-dY8mtf3XfhK1BAks5sGgd0gaJpZM4M9zaW .

[thomas-p-wilson]

Rebased my PR. Tests passing. Should be good to go!

[marshallswain]

Ha! This happened to me last night. It was weird. The tests passed in Travis. I pulled them down and they all failed. Wiped out node_modules and reinstalled, but it still failed. When I wiped out the entire project folder and re-cloned it worked fine.

Thanks. That was really fast!

[marshallswain]

released as feathers-authentication-local@0.4.2. Thank you, @thomas-p-wilson!

[thomas-p-wilson]

Cheers!

On Thu, Jun 22, 2017 at 8:43 AM, Marshall Thompson <notifications@github.com

wrote:

released as feathers-authentication-local@0.4.2. Thank you, @thomas-p-wilson https://github.com/thomas-p-wilson!

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

[ekryski]

Thanks guys! Nice work!