Adding new fields PasswordField and IpAddressField #542
Conversation
|
Needs tests - but looks good. |
|
Thank you.Let's move forward. Adrian Costia |
|
Just to chime a couple of comments I have regarding the PasswordField. While I think having the implementation is nice, I would argue that it needs to be bit more flexible and not make implementation assumptions. The code here makes the assumption that the only use-case for password storage is via a one-way hash, which currently does not support hash salting at all. Also, there is some level of password validation, which I think, depending on how you look at it, may be entirely out of scope for managing a database field (i.e. what about circumstances of requiring having numbers, uppercase, etc.) and may be better left to, say, form validation. Finally, I think the flexibility should also allow for utilizing an encryption/decryption mechanism rather than a hash. I think many developers (including myself) are taking this approach now since hashing is susceptible for collision attacks. With all of this in mind, perhaps the better choice instead of a specific "PasswordField" is to have both a "HashField" and "EncryptedField". Just my two cents. |
|
updating PasswordField |
| def random_password(self,nchars=7): | ||
| chars = PasswordField.CHARS | ||
| hash = '' | ||
| for char in range(nchars): |
There was a problem hiding this comment.
This should use xrange for speed. If a lot of passwords are generated, it would save some time. Although, in Python 3.X it is a moot point.
|
A lot of this is really application specific and various frameworks provide hashing capabilities. For example django has some good algorithms for generating a hash: https://docs.djangoproject.com/en/dev/topics/auth/#increasing-the-work-factor If you use flask then you have werkzeug helpers - http://werkzeug.pocoo.org/docs/utils/#module-werkzeug.security The database doesn't do anything special with a password - its just a string and determining if its correct or not is application specific. As such I think I'd take the idea of PasswordField and add it to the relevant framework helper libraries eg: flask_mongoengine / django_mongoengine |
| return False | ||
| result = 0 | ||
| for x, y in zip(value1, value2): | ||
| print x,y |
|
Looks good - just did a mini code review. Also we should obey pep8 and have spaces between operators unless in method calls. |
|
just chasing this up :) |
|
thank you and I did some improvements :) |
No description provided.