This repository was archived by the owner on Mar 22, 2022. It is now read-only.
Repository navigation
This repository was archived by the owner on Mar 22, 2022. It is now read-only.
Find "query" is replaced by token #64
Description
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 👍
Activity
daffl commentedon 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 commentedon Feb 15, 2016
A way to fix it could be using the object.assign call
Lorenzo Bernardi
marshallswain commentedon Feb 15, 2016
I think this might be related to this issue: feathersjs/feathers#224
marshallswain commentedon Feb 15, 2016
Oh. it's on the query. nm.
fastlorenzo commentedon 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 commentedon Feb 15, 2016
Would you mind creating a PR? Thanks.
ekryski commentedon 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 commentedon Feb 15, 2016
@ekryski sure, no issue 👍
fastlorenzo commentedon 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 commentedon 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.queryandhooks.params.dataare 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 commentedon 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 commentedon 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 commentedon Feb 15, 2016
I added the proposed changes in my PR
daffl commentedon Feb 15, 2016
That probably means that we don't need
populateSocketParamsat all anymore right? Besides that, the PR looks great, thank you for your help!ekryski commentedon Feb 15, 2016
@fastlorenzo I'll take a look. Yes, currently
tokengets 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 commentedon Feb 15, 2016
I've issued a new PR (#67) that addresses all this and adds some more tests.
ekryski commentedon Feb 15, 2016
Close by #65 and #67.