Resolves #14 - Passes Feathers params to service hooks #15
Conversation
|
Thank you for the pull request. Is this fix working for you? The problem is that user services should discard the password field when |
|
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" ***@***.***> 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
<#15 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAkeu9EUx_BikVf8emJPxRLqytaeikL3ks5rv6gAgaJpZM4M9zaW>
.
|
|
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 As per my comment #14 (comment), the lack of params passing was done intentionally so that one can't call If the use case seems legit, then we'll need to also make sure we omit |
thomas-p-wilson
force-pushed
the
fix/14-feathers-params-service-hooks
branch
2 times, most recently
from
302717f
to
79075da
Compare
|
@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. |
|
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" ***@***.***> 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
<#15 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAkeu3DGEcyzT6k0E5or6jNGiul3QXJ2ks5sGY1ugaJpZM4M9zaW>
.
|
|
@thomas-p-wilson could you resolve the small conflict with the latest master? |
|
Yep. No problem. Will do in the morning!
…On Jun 22, 2017 2:19 AM, "Marshall Thompson" ***@***.***> 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
<#15 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAkeu_2EIbxTmARYm-dY8mtf3XfhK1BAks5sGgd0gaJpZM4M9zaW>
.
|
|
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" ***@***.***>
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
> <#15 (comment)>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AAkeu_2EIbxTmARYm-dY8mtf3XfhK1BAks5sGgd0gaJpZM4M9zaW>
> .
>
|
thomas-p-wilson
force-pushed
the
fix/14-feathers-params-service-hooks
branch
from
79075da
to
cff3852
Compare
| .then(response => { | ||
| const results = response.data || response | ||
| if (!results.length) { | ||
| debug(`a record with ${this.options.usernameField} of '${username}' did not exist`); | ||
| debug(`a record with ${usernameField} of '${username}' did not exist`); |
There was a problem hiding this comment.
Note this additional change. Master did not use the apparently new username constant defined on line 72. Figured it was a quick fix :)
|
Rebased my PR. Tests passing. Should be good to go! |
|
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 Thanks. That was really fast! |
|
released as |
|
Cheers!
…On Thu, Jun 22, 2017 at 8:43 AM, Marshall Thompson ***@***.*** > wrote:
released as ***@***.*** 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
<#15 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAkeu4OltI0fU8wDtB8QX0T2xgV1VJQgks5sGmFjgaJpZM4M9zaW>
.
|
|
Thanks guys! Nice work! |
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.