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

Find "query" is replaced by token #64

Closed
fastlorenzo opened this Issue Feb 15, 2016 · 17 comments

Comments

Projects
None yet
4 participants
@fastlorenzo
Copy link
Contributor

fastlorenzo commented Feb 15, 2016

Hello,
In : feathers-authentication/src/client/hooks.js
export let populateSocketParams = function() { return function(hook) { if (hook.params.token) { ** hook.params.query = { token: hook.params.token };** } }; };

You should create a new object and keep the existing params, otherwise every param query passed to the 'find' action is replaced by the token only.

Thanks 👍

@daffl

This comment has been minimized.

Copy link
Member

daffl commented Feb 15, 2016

@ekryski Is there a reason for adding the token to the query? Shouldn't it be already set on the socket?

@fastlorenzo

This comment has been minimized.

Copy link
Contributor Author

fastlorenzo commented Feb 15, 2016

A way to fix it could be using the object.assign call

Lorenzo Bernardi

On 15 Feb 2016, at 19:28, David Luecke notifications@github.com wrote:

@ekryski Is there a reason for adding the token to the query? Shouldn't it be already set on the socket?


Reply to this email directly or view it on GitHub.

@marshallswain

This comment has been minimized.

Copy link
Member

marshallswain commented Feb 15, 2016

I think this might be related to this issue: feathersjs/feathers#224

@marshallswain

This comment has been minimized.

Copy link
Member

marshallswain commented Feb 15, 2016

Oh. it's on the query. nm.

@fastlorenzo

This comment has been minimized.

Copy link
Contributor Author

fastlorenzo commented Feb 15, 2016

Changing
hook.params.query = { token: hook.params.token };
to
hook.params.query = Object.assign({}, hook.params.query, { token: hook.params.token });
fixes the issue.
Could you integrate the fix or should I create a pull request ?

@marshallswain

This comment has been minimized.

Copy link
Member

marshallswain commented Feb 15, 2016

Would you mind creating a PR? Thanks.

@ekryski

This comment has been minimized.

Copy link
Member

ekryski commented Feb 15, 2016

@fastlorenzo thanks for the PR. Let me review. I think it's likely that it's my bad but I want to make sure it doesn't break anything. I haven't had time to write all the tests that I've needed to.

@fastlorenzo

This comment has been minimized.

Copy link
Contributor Author

fastlorenzo commented Feb 15, 2016

@ekryski sure, no issue 👍

@fastlorenzo

This comment has been minimized.

Copy link
Contributor Author

fastlorenzo commented Feb 15, 2016

PS: maybe you should also use the Object.assing for the following line in the same file :
19: hook.params.headers = {

@ekryski

This comment has been minimized.

Copy link
Member

ekryski commented Feb 15, 2016

@fastlorenzo ya I think you are right on the headers part. I actually think @daffl might be right and that for sockets we don't need to set the token each time. hook.params.query and hooks.params.data are the only things sent to the server by feathers-client but in the case of sockets because it is a persistent connection we should already have the token on server side.

So your PR might be moot. I'm going to tinker with it quickly. We might not even need to have that code in there.

@fastlorenzo

This comment has been minimized.

Copy link
Contributor Author

fastlorenzo commented Feb 15, 2016

Indeed, and I think sending the token each time in the query can be an issue, for example if you want to filter the query using a parameter named token.

@fastlorenzo

This comment has been minimized.

Copy link
Contributor Author

fastlorenzo commented Feb 15, 2016

@ekryski I quickly checked by removing the token to be sent in each request and it look ks like it's working, as @daffl said.
I'll create another pull request for that so you can check it.

@fastlorenzo

This comment has been minimized.

Copy link
Contributor Author

fastlorenzo commented Feb 15, 2016

I added the proposed changes in my PR

@daffl

This comment has been minimized.

Copy link
Member

daffl commented Feb 15, 2016

That probably means that we don't need populateSocketParams at all anymore right? Besides that, the PR looks great, thank you for your help!

@ekryski

This comment has been minimized.

Copy link
Member

ekryski commented Feb 15, 2016

@fastlorenzo I'll take a look. Yes, currently token gets filtered out of the query on the server side so that will need to be addressed.

@daffl you are right that is no longer needed.

@ekryski

This comment has been minimized.

Copy link
Member

ekryski commented Feb 15, 2016

I've issued a new PR (#67) that addresses all this and adds some more tests.

@ekryski ekryski closed this Feb 15, 2016

@ekryski ekryski reopened this Feb 15, 2016

@ekryski

This comment has been minimized.

Copy link
Member

ekryski commented Feb 15, 2016

Close by #65 and #67.

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