kaliop-uk / ezmigrationbundle

This bundle makes it easy to handle eZPlatform / eZPublish5 content upgrades/migrations
GNU General Public License v2.0
53 stars 81 forks source link

Added note for hashing passwords in kaliop migrations #214

Closed Rubinum closed 5 years ago

Rubinum commented 5 years ago

Hey,

I stumbled over the fact, that the given password string in a kaliop migration will be automatically hashed. I think this should be mentioned in the docs so here is the change.

codecov-io commented 5 years ago

Codecov Report

Merging #214 into master will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #214   +/-   ##
=========================================
  Coverage     67.81%   67.81%           
  Complexity     2623     2623           
=========================================
  Files           124      124           
  Lines          6833     6833           
=========================================
  Hits           4634     4634           
  Misses         2199     2199

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 9858b42...237dc5c. Read the comment docs.

gggeek commented 5 years ago

Hello. The password stored in the db will be hashed, and that is a basic (good) security practice. This is transparent to the migration bundle, as it is a native feature of the cms. I think that, if we want to add this information to the docs, we should explain a bit more - as it stands, one might get the impression that after creating a user account, the hash of the password will be needed for eg. logging in...

crevillo commented 5 years ago

Maybe a bit offtopic, but if you write plain password here and you commit migration to yr repo, in the repo you will have the unhshed password, right? We tend to void this type of migration because of this.

Rubinum commented 5 years ago

@crevillo Maybe we should declare a little warning for clear passwords, because of the fact you mentioned? I could add that in this PR too, if you want.

@gggeek I created this PR, because for me it was not clear if I should provide the fully hashed password for the password value or a plain password. Yes, hashing passwords is a importent thing and a standard functionality of the framework, but as I wrote my first kaliop migration for a user, I guessed, that the password value would be a hashed string and not a plain password because of security reasons. So it's not a "hey its obvious that ez stores its passwords not in clear text and this bundle will also not store passwords in plain text, we should add a hint for that" criticism, it's more like a "hey its not clear if user should provide a hashed password or a plain text password for that value". More like a comfort note for future devs.

gggeek commented 5 years ago

Seems that we're reaching a consensus... I'll merge and then reword a bit