Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

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

Merged
merged 1 commit into from
Jul 11, 2020

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

# for free to join this conversation on GitHub. Already have an account? # 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