8000 fix(authentication): Include query params when authenticating via authenticate hook by burn2delete · Pull Request #2009 · feathersjs/feathers · GitHub
[go: up one dir, main page]

Skip to content

fix(authentication): Include query params when authenticating via authenticate hook#2009

Merged
daffl merged 1 commit intofeathersjs:masterfrom < 8000 /span>
burn2delete:fix-authenticate-hook
Jul 11, 2020
Merged

fix(authentication): Include query params when authenticating via authenticate hook#2009
daffl merged 1 commit intofeathersjs:masterfrom
burn2delete:fix-authenticate-hook

Conversation

@burn2delete
Copy link
Contributor

Supersedes #2008 fixes #2007

@daffl daffl changed the title Include query params when authenticating via authenticate hook fix(authentication): Include query params when authenticating via authenticate hook Jul 11, 2020
@daffl
Copy link
Member
daffl commented Jul 11, 2020

Looks like nothing is breaking and I reviewed the dependent calls and there really appears no reason anymore for not including the query. Thanks for the PR!

@daffl daffl merged commit f4a9538 into feathersjs:master Jul 11, 2020
daffl pushed a commit that referenced this pull request Jul 11, 2020
@DevBayer
Copy link

It seems to me that something is failing in his logic, including the query parameter is breaking my code, I use the authenticate('jwt') hook as a header everywhere and is performing the query to the service by adding in the query a column that obviously does not exist in the users model because the main query is directed on another service that uses another object model.

@jchamb
Copy link
jchamb commented Jul 12, 2020

@daffl @DevBayer Same thing for me. letting the query param through basically broke every request in my app, as it was trying to attach extra fields into the user query. I downgraded back to 4.5.3 and everything went back to normal

@daffl
Copy link
Member
daffl commented Jul 12, 2020

Not sure how i missed that, that's exactly what I was worried about. Fixed in v4.5.6.

@burn2delete
Copy link
Contributor Author

@daffl Would you like to review #2008 as a possible alternative solution?

@jchamb
Copy link
jchamb commented Jul 13, 2020

Do most of the adapters support pulling the model attributes like sequelize? It would be pretty easy to apply a whitelist of allowed fields by just pulling the models attributes.

@burn2delete
Copy link
Contributor Author
burn2delete commented Jul 13, 2020

@jchamb that would assume the user service is backed by a database adapter, and not for example a proxy to another service, on another server.

@jchamb
Copy link
jchamb commented Jul 13, 2020

@flyboarder very true. Was just thinking as an easy default whitelist for what is prob the more common/standard installs. I would say that, that would also be an easy config, but if your going to have to config something might as well just make be explicit like your #2008

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Issue authenticating user via JWT when users service requires query params

4 participants

0