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

added support for hashing passwords when hook.data is an array #225

Merged
merged 1 commit into from Jun 16, 2016

Conversation

Projects
None yet
4 participants
@eblin
Copy link
Contributor

eblin commented Jun 15, 2016

feathers-sequelize allows you to bulk create (maybe other adapters do too?), in my case I was bulk creating predefined set of users (like admins, etc) and I noticed the auth was failing and later discovered the passwords were not being hashed at all.

We could always loop thru the array and call .create() individually, but I think this should be supported by the hashPassword hook, I certainly thought it was!

added support for hashing passwords when hook.data is an array (like …
…feathers-sequelize does for bulk create)
@marshallswain

This comment has been minimized.

Copy link
Member

marshallswain commented Jun 15, 2016

This looks good. I absolutely agree that this should be possible. Other official db adapters support bulk actions, too.

Also FYI, it's likely that in an upcoming Feathers release that we'll be making bulk actions an opt-in feature per service.

@daffl

This comment has been minimized.

Copy link
Member

daffl commented Jun 15, 2016

Makes sense. We don't want to add the lib folder though. Everything in that folder is generated and should not be changed or checked in.

@eblin

This comment has been minimized.

Copy link
Contributor Author

eblin commented Jun 15, 2016

Ok thanks for the heads up,

Yes sorry about the lib folder commit, I thought those commits were not going to make it to the PR, anyways that was just needed for my build since npm does not run prepublish hooks when using your own forked modules (npm/npm#3055), so anyone forking the library and wants to use it would have to compile manually.

Sorry about that! Feel free to ignore/clean up the last 2 commits

@eblin eblin force-pushed the eblin:master branch from 9853803 to 7fbbc30 Jun 16, 2016

@eblin

This comment has been minimized.

Copy link
Contributor Author

eblin commented Jun 16, 2016

I went ahead and removed the last 2 commits and moved them to a branch to keep the PR clean.

I can still use my version in my build just fine thanks to npm letting you point to a branch or commit when using your own modules.

@daffl

This comment has been minimized.

Copy link
Member

daffl commented Jun 16, 2016

If we merge this PR and make a release you won't have to use your own branch anymore right?

@ekryski

This comment has been minimized.

Copy link
Member

ekryski commented Jun 16, 2016

👍 @eblin this is all good! Thanks for such a great PR!

@ekryski ekryski merged commit c13364e into feathersjs:master Jun 16, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@eblin

This comment has been minimized.

Copy link
Contributor Author

eblin commented Jun 17, 2016

@daffl that's correct.
@ekryski no problem, thank you guys for great framework!

@ekryski

This comment has been minimized.

Copy link
Member

ekryski commented Jun 20, 2016

Released v0.7.9 with this fix in! 🍻 @eblin!

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.