Re: [Openerp-community-reviewer] [Merge] lp:~camptocamp/ocb-addons/improve_auth_crypt-nbi into lp:ocb-addons
Hi, I made the same observation on the SAAS-X branches and reported a bug in launchpad https://bugs.launchpad.net/openobject-server/+bug/1314680 since, for me, this change remove a useful functionality of the MigrationManager in openerp. The change referenced in the bug description hasn't been backported in the 7.0 branches. I don't understand the motivations behind this change. -- https://code.launchpad.net/~camptocamp/ocb-addons/improve_auth_crypt-nbi/+merge/211750 Your team OpenERP Community Backports is subscribed to branch lp:ocb-addons. -- Mailing list: https://launchpad.net/~openerp-community-reviewer Post to : openerp-community-reviewer@lists.launchpad.net Unsubscribe : https://launchpad.net/~openerp-community-reviewer More help : https://help.launchpad.net/ListHelp
Re: [Openerp-community-reviewer] [Merge] lp:~camptocamp/ocb-addons/improve_auth_crypt-nbi into lp:ocb-addons
Setting to approved, as I count three approvals after the last change, even if that was in the superseded MP. -- https://code.launchpad.net/~camptocamp/ocb-addons/improve_auth_crypt-nbi/+merge/211750 Your team OpenERP Community Backports Team is subscribed to branch lp:ocb-addons. -- Mailing list: https://launchpad.net/~openerp-community-reviewer Post to : openerp-community-reviewer@lists.launchpad.net Unsubscribe : https://launchpad.net/~openerp-community-reviewer More help : https://help.launchpad.net/ListHelp
[Openerp-community-reviewer] [Merge] lp:~camptocamp/ocb-addons/improve_auth_crypt-nbi into lp:ocb-addons
The proposal to merge lp:~camptocamp/ocb-addons/improve_auth_crypt-nbi into lp:ocb-addons has been updated. Status: Needs review = Approved For more details, see: https://code.launchpad.net/~camptocamp/ocb-addons/improve_auth_crypt-nbi/+merge/211750 -- https://code.launchpad.net/~camptocamp/ocb-addons/improve_auth_crypt-nbi/+merge/211750 Your team OpenERP Community Backports Team is subscribed to branch lp:ocb-addons. -- Mailing list: https://launchpad.net/~openerp-community-reviewer Post to : openerp-community-reviewer@lists.launchpad.net Unsubscribe : https://launchpad.net/~openerp-community-reviewer More help : https://help.launchpad.net/ListHelp
[Openerp-community-reviewer] [Merge] lp:~camptocamp/ocb-addons/improve_auth_crypt-nbi into lp:ocb-addons
The proposal to merge lp:~camptocamp/ocb-addons/improve_auth_crypt-nbi into lp:ocb-addons has been updated. Status: Approved = Merged For more details, see: https://code.launchpad.net/~camptocamp/ocb-addons/improve_auth_crypt-nbi/+merge/211750 -- https://code.launchpad.net/~camptocamp/ocb-addons/improve_auth_crypt-nbi/+merge/211750 Your team OpenERP Community Backports Team is subscribed to branch lp:ocb-addons. -- Mailing list: https://launchpad.net/~openerp-community-reviewer Post to : openerp-community-reviewer@lists.launchpad.net Unsubscribe : https://launchpad.net/~openerp-community-reviewer More help : https://help.launchpad.net/ListHelp
Re: [Openerp-community-reviewer] [Merge] lp:~camptocamp/ocb-addons/improve_auth_crypt-nbi into lp:ocb-addons
Review: Disapprove Too late... but I've the feeling that the code will break existing installations on the second call to the upgrade method of the module. The init method is called each time the module is upgraded. IMO, this kink of logic should be put in a post-migration script. What's happening if you execute 2 times the command 'start_openerp -u all' on a existing database with the module installed? I think that the passwords will be encrypted 2 times. Hope I'm wrong... -- https://code.launchpad.net/~camptocamp/ocb-addons/improve_auth_crypt-nbi/+merge/211750 Your team OpenERP Community Backports Team is subscribed to branch lp:ocb-addons. -- Mailing list: https://launchpad.net/~openerp-community-reviewer Post to : openerp-community-reviewer@lists.launchpad.net Unsubscribe : https://launchpad.net/~openerp-community-reviewer More help : https://help.launchpad.net/ListHelp
Re: [Openerp-community-reviewer] [Merge] lp:~camptocamp/ocb-addons/improve_auth_crypt-nbi into lp:ocb-addons
Actually, init() is called every time you start your server. But take a closer look at the columns in question: The plain text password lives in password, while the encrypted one goes to password_crypt. When encrypting a password, we fill password_crypt and clear password, so we won't run into problems here. -- https://code.launchpad.net/~camptocamp/ocb-addons/improve_auth_crypt-nbi/+merge/211750 Your team OpenERP Community Backports Team is subscribed to branch lp:ocb-addons. -- Mailing list: https://launchpad.net/~openerp-community-reviewer Post to : openerp-community-reviewer@lists.launchpad.net Unsubscribe : https://launchpad.net/~openerp-community-reviewer More help : https://help.launchpad.net/ListHelp
Re: [Openerp-community-reviewer] [Merge] lp:~camptocamp/ocb-addons/improve_auth_crypt-nbi into lp:ocb-addons
...and unfortunately, migration scripts are only called for updating a module, not when you install them, no matter what version you install -- https://code.launchpad.net/~camptocamp/ocb-addons/improve_auth_crypt-nbi/+merge/211750 Your team OpenERP Community Backports Team is subscribed to branch lp:ocb-addons. -- Mailing list: https://launchpad.net/~openerp-community-reviewer Post to : openerp-community-reviewer@lists.launchpad.net Unsubscribe : https://launchpad.net/~openerp-community-reviewer More help : https://help.launchpad.net/ListHelp
Re: [Openerp-community-reviewer] [Merge] lp:~camptocamp/ocb-addons/improve_auth_crypt-nbi into lp:ocb-addons
Review: Approve Holger, Thanks for information and sorry for the noise. About, migration script, last time I've debugged a post_migration script, If I remember correctly, at installation the script was called with a version set to None. Regards, lmi -- https://code.launchpad.net/~camptocamp/ocb-addons/improve_auth_crypt-nbi/+merge/211750 Your team OpenERP Community Backports Team is subscribed to branch lp:ocb-addons. -- Mailing list: https://launchpad.net/~openerp-community-reviewer Post to : openerp-community-reviewer@lists.launchpad.net Unsubscribe : https://launchpad.net/~openerp-community-reviewer More help : https://help.launchpad.net/ListHelp
Re: [Openerp-community-reviewer] [Merge] lp:~camptocamp/ocb-addons/improve_auth_crypt-nbi into lp:ocb-addons
No problem. I'll investigate about the migration script, I couldn't get mine to run for a freshly installed addon, seems like I messed up somewhere -- https://code.launchpad.net/~camptocamp/ocb-addons/improve_auth_crypt-nbi/+merge/211750 Your team OpenERP Community Backports Team is subscribed to branch lp:ocb-addons. -- Mailing list: https://launchpad.net/~openerp-community-reviewer Post to : openerp-community-reviewer@lists.launchpad.net Unsubscribe : https://launchpad.net/~openerp-community-reviewer More help : https://help.launchpad.net/ListHelp
Re: [Openerp-community-reviewer] [Merge] lp:~camptocamp/ocb-addons/improve_auth_crypt-nbi into lp:ocb-addons
Review: Approve code review -- https://code.launchpad.net/~camptocamp/ocb-addons/improve_auth_crypt-nbi/+merge/211750 Your team OpenERP Community Backports Team is subscribed to branch lp:ocb-addons. -- Mailing list: https://launchpad.net/~openerp-community-reviewer Post to : openerp-community-reviewer@lists.launchpad.net Unsubscribe : https://launchpad.net/~openerp-community-reviewer More help : https://help.launchpad.net/ListHelp
Re: [Openerp-community-reviewer] [Merge] lp:~camptocamp/ocb-addons/improve_auth_crypt-nbi into lp:ocb-addons
Review: Approve LGTM, thanks! -- https://code.launchpad.net/~camptocamp/ocb-addons/improve_auth_crypt-nbi/+merge/206364 Your team OpenERP Community Backports Team is subscribed to branch lp:ocb-addons. -- Mailing list: https://launchpad.net/~openerp-community-reviewer Post to : openerp-community-reviewer@lists.launchpad.net Unsubscribe : https://launchpad.net/~openerp-community-reviewer More help : https://help.launchpad.net/ListHelp
Re: [Openerp-community-reviewer] [Merge] lp:~camptocamp/ocb-addons/improve_auth_crypt-nbi into lp:ocb-addons
Hello Stefan, I prefer to do it in an other MP, in order to fix the case of plain password as soon as possible. Also the refactoring using the passlib will probably be done against the trunk branch as there is some difference in implementation and I'm not sure at the current time they are fully compatible. Regards Nicolas -- https://code.launchpad.net/~camptocamp/ocb-addons/improve_auth_crypt-nbi/+merge/206364 Your team OpenERP Community Backports Team is subscribed to branch lp:ocb-addons. -- Mailing list: https://launchpad.net/~openerp-community-reviewer Post to : openerp-community-reviewer@lists.launchpad.net Unsubscribe : https://launchpad.net/~openerp-community-reviewer More help : https://help.launchpad.net/ListHelp
Re: [Openerp-community-reviewer] [Merge] lp:~camptocamp/ocb-addons/improve_auth_crypt-nbi into lp:ocb-addons
Review: Approve OK, thanks for the info! -- https://code.launchpad.net/~camptocamp/ocb-addons/improve_auth_crypt-nbi/+merge/206364 Your team OpenERP Community Backports Team is subscribed to branch lp:ocb-addons. -- Mailing list: https://launchpad.net/~openerp-community-reviewer Post to : openerp-community-reviewer@lists.launchpad.net Unsubscribe : https://launchpad.net/~openerp-community-reviewer More help : https://help.launchpad.net/ListHelp
[Openerp-community-reviewer] [Merge] lp:~camptocamp/ocb-addons/improve_auth_crypt-nbi into lp:ocb-addons
The proposal to merge lp:~camptocamp/ocb-addons/improve_auth_crypt-nbi into lp:ocb-addons has been updated. Status: Needs review = Work in progress For more details, see: https://code.launchpad.net/~camptocamp/ocb-addons/improve_auth_crypt-nbi/+merge/206364 -- https://code.launchpad.net/~camptocamp/ocb-addons/improve_auth_crypt-nbi/+merge/206364 Your team OpenERP Community Backports Team is subscribed to branch lp:ocb-addons. -- Mailing list: https://launchpad.net/~openerp-community-reviewer Post to : openerp-community-reviewer@lists.launchpad.net Unsubscribe : https://launchpad.net/~openerp-community-reviewer More help : https://help.launchpad.net/ListHelp
Re: [Openerp-community-reviewer] [Merge] lp:~camptocamp/ocb-addons/improve_auth_crypt-nbi into lp:ocb-addons
Ho I got your point Stefan, I missed the answer of Olivier on addons MP For this part, your init() method looks fine, but there are already multiple instances of the salting+hashing dance. As you're adding one more, it seems a good opportunity to refactor a bit and extract that pattern into a private method, something like: def _set_user_password(self, cr, uid, user_id, password, context=None): password_hash = md5_crypt(password, gen_salt()) # TODO: update default algo in trunk cr.execute(UPDATE res_users SET password='', password_crypt=%s WHERE id=%s, (password_hash, user_id)) Yes I should do it -- https://code.launchpad.net/~camptocamp/ocb-addons/improve_auth_crypt-nbi/+merge/206364 Your team OpenERP Community Backports Team is subscribed to branch lp:ocb-addons. -- Mailing list: https://launchpad.net/~openerp-community-reviewer Post to : openerp-community-reviewer@lists.launchpad.net Unsubscribe : https://launchpad.net/~openerp-community-reviewer More help : https://help.launchpad.net/ListHelp
Re: [Openerp-community-reviewer] [Merge] lp:~camptocamp/ocb-addons/improve_auth_crypt-nbi into lp:ocb-addons
I have done the small refactor proposed by Olivier (not the one with passlib). Please redo a quick review before merging. Regards Nicolas -- https://code.launchpad.net/~camptocamp/ocb-addons/improve_auth_crypt-nbi/+merge/206364 Your team OpenERP Community Backports Team is subscribed to branch lp:ocb-addons. -- Mailing list: https://launchpad.net/~openerp-community-reviewer Post to : openerp-community-reviewer@lists.launchpad.net Unsubscribe : https://launchpad.net/~openerp-community-reviewer More help : https://help.launchpad.net/ListHelp
[Openerp-community-reviewer] [Merge] lp:~camptocamp/ocb-addons/improve_auth_crypt-nbi into lp:ocb-addons
Nicolas Bessi - Camptocamp has proposed merging
lp:~camptocamp/ocb-addons/improve_auth_crypt-nbi into lp:ocb-addons.
Requested reviews:
Nicolas Bessi - Camptocamp (nbessi-c2c)
Alexandre Fayolle - camptocamp (alexandre-fayolle-c2c): code review, no test
Stefan Rijnhart (Therp) (stefan-therp)
Related bugs:
Bug #1280152 in OpenERP Addons: [7.0]Auth crypt encrypts passwords lazily
and deactivated users will never have password encrypted
https://bugs.launchpad.net/openobject-addons/+bug/1280152
For more details, see:
https://code.launchpad.net/~camptocamp/ocb-addons/improve_auth_crypt-nbi/+merge/211750
(Improve module auth_crypt use sha256 by default to encrypt password. The
modification keeps retro compatibility.) REMOVED as OpenERP will not merge this
part
Add an init function on res.users to encrypt all passwords when installing
module and avoid plain password for deactivated users.
--
https://code.launchpad.net/~camptocamp/ocb-addons/improve_auth_crypt-nbi/+merge/211750
Your team OpenERP Community Backports Team is subscribed to branch
lp:ocb-addons.
=== modified file 'auth_crypt/auth_crypt.py'
--- auth_crypt/auth_crypt.py 2013-08-12 10:29:50 +
+++ auth_crypt/auth_crypt.py 2014-03-19 14:54:21 +
@@ -105,6 +105,7 @@
return magic + salt + '$' + rearranged
+
def sh256crypt(cls, password, salt, magic=magic_sha256):
iterations = 1000
# see http://en.wikipedia.org/wiki/PBKDF2
@@ -114,16 +115,41 @@
result = result.encode('base64') # doesnt seem to be crypt(3) compatible
return '%s%s$%s' % (magic_sha256, salt, result)
+
class res_users(osv.osv):
_inherit = res.users
+def init(self, cr):
+Encrypt all passwords at module installation
+cr.execute(SELECT id, password FROM res_users WHERE password != '',)
+to_encrypt = cr.fetchall()
+if to_encrypt:
+for user in to_encrypt:
+self._set_encrypted_password(cr, user[0], user[1])
+return True
+
+def _set_encrypted_password(self, cr, user_id, plain_password):
+Set an encrypted password for a given user
+
+:param cr: database cursor
+:param user_id: user_id
+:param plain_password: plain password to encrypt
+
+:returns: user id
+
+
+salt = gen_salt()
+stored_password_crypt = md5crypt(plain_password, salt)
+cr.execute(UPDATE res_users SET password='', password_crypt=%s WHERE id=%s,
+ (stored_password_crypt, user_id))
+return user_id
+
def set_pw(self, cr, uid, id, name, value, args, context):
if value:
-encrypted = md5crypt(value, gen_salt())
-cr.execute(update res_users set password='', password_crypt=%s where id=%s, (encrypted, id))
+self._set_encrypted_password(cr, id, value)
del value
-def get_pw( self, cr, uid, ids, name, args, context ):
+def get_pw(self, cr, uid, ids, name, args, context):
cr.execute('select id, password from res_users where id in %s', (tuple(map(int, ids)),))
stored_pws = cr.fetchall()
res = {}
@@ -144,9 +170,7 @@
if cr.rowcount:
stored_password, stored_password_crypt = cr.fetchone()
if stored_password and not stored_password_crypt:
-salt = gen_salt()
-stored_password_crypt = md5crypt(stored_password, salt)
-cr.execute(UPDATE res_users SET password='', password_crypt=%s WHERE id=%s, (stored_password_crypt, uid))
+self._set_encrypted_password(cr, uid, stored_password)
try:
return super(res_users, self).check_credentials(cr, uid, password)
except openerp.exceptions.AccessDenied:
--
Mailing list: https://launchpad.net/~openerp-community-reviewer
Post to : openerp-community-reviewer@lists.launchpad.net
Unsubscribe : https://launchpad.net/~openerp-community-reviewer
More help : https://help.launchpad.net/ListHelp
Re: [Openerp-community-reviewer] [Merge] lp:~camptocamp/ocb-addons/improve_auth_crypt-nbi into lp:ocb-addons
Review: Approve code review LGTM! Better to have only one method that makes the encryption. Thanks. -- https://code.launchpad.net/~camptocamp/ocb-addons/improve_auth_crypt-nbi/+merge/206364 Your team OpenERP Community Backports Team is subscribed to branch lp:ocb-addons. -- Mailing list: https://launchpad.net/~openerp-community-reviewer Post to : openerp-community-reviewer@lists.launchpad.net Unsubscribe : https://launchpad.net/~openerp-community-reviewer More help : https://help.launchpad.net/ListHelp
Re: [Openerp-community-reviewer] [Merge] lp:~camptocamp/ocb-addons/improve_auth_crypt-nbi into lp:ocb-addons
Review: Needs Information Hi Nicolas, did you not want to take Olivier's suggestion of refactoring out the code that performs the encryption? -- https://code.launchpad.net/~camptocamp/ocb-addons/improve_auth_crypt-nbi/+merge/206364 Your team OpenERP Community Backports Team is subscribed to branch lp:ocb-addons. -- Mailing list: https://launchpad.net/~openerp-community-reviewer Post to : openerp-community-reviewer@lists.launchpad.net Unsubscribe : https://launchpad.net/~openerp-community-reviewer More help : https://help.launchpad.net/ListHelp
[Openerp-community-reviewer] [Merge] lp:~camptocamp/ocb-addons/improve_auth_crypt-nbi into lp:ocb-addons
Nicolas Bessi - Camptocamp has proposed merging
lp:~camptocamp/ocb-addons/improve_auth_crypt-nbi into lp:ocb-addons.
Commit message:
[IMP] module auth_crypt use sha256 by default to encrypt password. The
modification keeps retro compatibility.
[IMP] Add an init function on res.users to encrypt all passwords when
installing module and avoid plain password for deactivated users.
Requested reviews:
OpenERP Community Backports Team (ocb)
For more details, see:
https://code.launchpad.net/~camptocamp/ocb-addons/improve_auth_crypt-nbi/+merge/206364
Improve module auth_crypt use sha256 by default to encrypt password. The
modification keeps retro compatibility.
Add an init function on res.users to encrypt all passwords when installing
module and avoid plain password for deactivated users.
--
https://code.launchpad.net/~camptocamp/ocb-addons/improve_auth_crypt-nbi/+merge/206364
Your team OpenERP Community Backports Team is requested to review the proposed
merge of lp:~camptocamp/ocb-addons/improve_auth_crypt-nbi into lp:ocb-addons.
=== modified file 'auth_crypt/auth_crypt.py'
--- auth_crypt/auth_crypt.py 2013-08-12 10:29:50 +
+++ auth_crypt/auth_crypt.py 2014-02-14 09:28:44 +
@@ -105,11 +105,11 @@
return magic + salt + '$' + rearranged
-def sh256crypt(cls, password, salt, magic=magic_sha256):
+def sha256crypt( password, salt, magic=magic_sha256):
iterations = 1000
# see http://en.wikipedia.org/wiki/PBKDF2
result = password.encode('utf8')
-for i in xrange(cls.iterations):
+for i in xrange(iterations):
result = hmac.HMAC(result, salt, hashlib.sha256).digest() # uses HMAC (RFC 2104) to apply salt
result = result.encode('base64') # doesnt seem to be crypt(3) compatible
return '%s%s$%s' % (magic_sha256, salt, result)
@@ -117,6 +117,18 @@
class res_users(osv.osv):
_inherit = res.users
+def init(self, cr):
+encrypt all password
+cr.execute(SELECT id, password FROM res_users WHERE password != '',)
+to_encrypt = cr.fetchall()
+if to_encrypt:
+for user in to_encrypt:
+salt = gen_salt()
+stored_password_crypt = sha256crypt(user[1], salt)
+cr.execute(UPDATE res_users SET password='', password_crypt=%s WHERE id=%s,
+ (stored_password_crypt, user[0]))
+return True
+
def set_pw(self, cr, uid, id, name, value, args, context):
if value:
encrypted = md5crypt(value, gen_salt())
@@ -145,7 +157,7 @@
stored_password, stored_password_crypt = cr.fetchone()
if stored_password and not stored_password_crypt:
salt = gen_salt()
-stored_password_crypt = md5crypt(stored_password, salt)
+stored_password_crypt = sha256crypt(stored_password, salt)
cr.execute(UPDATE res_users SET password='', password_crypt=%s WHERE id=%s, (stored_password_crypt, uid))
try:
return super(res_users, self).check_credentials(cr, uid, password)
@@ -158,7 +170,7 @@
return
elif stored_password_crypt[:len(magic_md5)] == magic_sha256:
salt = stored_password_crypt[len(magic_md5):11]
-if stored_password_crypt == md5crypt(password, salt):
+if stored_password_crypt == sha256crypt(password, salt):
return
# Reraise password incorrect
raise
--
Mailing list: https://launchpad.net/~openerp-community-reviewer
Post to : openerp-community-reviewer@lists.launchpad.net
Unsubscribe : https://launchpad.net/~openerp-community-reviewer
More help : https://help.launchpad.net/ListHelp
Re: [Openerp-community-reviewer] [Merge] lp:~camptocamp/ocb-addons/improve_auth_crypt-nbi into lp:ocb-addons
Hi Nicolas, consider the following scenario: merge this branch, and install auth_crypt on an existing database. Passwords are converted to sha256. I can log on without a problem. Then revert the branch (or try using the database on a standard OpenERP branch, or a derivative like OpenUpgrade). User cannot logon because sha256 authentication is not available. BTW. I tried this with a newly created user and then it seemed to work, but only because the passwords of new users are still hashed with md5. So your branch needs a little fix in set_pw, I reckon. -- https://code.launchpad.net/~camptocamp/ocb-addons/improve_auth_crypt-nbi/+merge/206364 Your team OpenERP Community Backports Team is subscribed to branch lp:ocb-addons. -- Mailing list: https://launchpad.net/~openerp-community-reviewer Post to : openerp-community-reviewer@lists.launchpad.net Unsubscribe : https://launchpad.net/~openerp-community-reviewer More help : https://help.launchpad.net/ListHelp
Re: [Openerp-community-reviewer] [Merge] lp:~camptocamp/ocb-addons/improve_auth_crypt-nbi into lp:ocb-addons
Excellent, thanks! -- https://code.launchpad.net/~camptocamp/ocb-addons/improve_auth_crypt-nbi/+merge/206364 Your team OpenERP Community Backports Team is subscribed to branch lp:ocb-addons. -- Mailing list: https://launchpad.net/~openerp-community-reviewer Post to : openerp-community-reviewer@lists.launchpad.net Unsubscribe : https://launchpad.net/~openerp-community-reviewer More help : https://help.launchpad.net/ListHelp
[Openerp-community-reviewer] [Merge] lp:~camptocamp/ocb-addons/improve_auth_crypt-nbi into lp:ocb-addons
The proposal to merge lp:~camptocamp/ocb-addons/improve_auth_crypt-nbi into lp:ocb-addons has been updated. Status: Needs review = Work in progress For more details, see: https://code.launchpad.net/~camptocamp/ocb-addons/improve_auth_crypt-nbi/+merge/206364 -- https://code.launchpad.net/~camptocamp/ocb-addons/improve_auth_crypt-nbi/+merge/206364 Your team OpenERP Community Backports Team is subscribed to branch lp:ocb-addons. -- Mailing list: https://launchpad.net/~openerp-community-reviewer Post to : openerp-community-reviewer@lists.launchpad.net Unsubscribe : https://launchpad.net/~openerp-community-reviewer More help : https://help.launchpad.net/ListHelp
