Merge lp:~camptocamp/ocb-addons/improve_auth_crypt-nbi into lp:ocb-addons

[Nicolas Bessi - Camptocamp]

(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.

[Alexandre Fayolle - camptocamp (alexandre-fayolle-c2c)]

code LGTM. Not tested.

review: Approve (code review, no test)

[Stefan Rijnhart (Opener) (stefan-opener)]

This works, and I appreciate the contribution. However, if I understand correctly this breaks compatibility with standard OpenERP as well as things like OpenUpgrade so I am not in favour of merging this until it is actually merged in trunk (and there is no proposal yet on trunk).

review: Disapprove

[Nicolas Bessi - Camptocamp (nbessi-c2c-deactivatedaccount)]

Hello,

This will not breaks compatibility with OpenERP standards as the magic method key is encoded in plain at the beginning of the password. OpenERP will choose the correct way to decrypt password md5 or sha256 as said in comment.

Also I do not see in how it will break OpenUpgrade without further explaination I can only ask you to reconcider your review.

Regards

Nicolas

review: Approve

[Stefan Rijnhart (Opener) (stefan-opener)]

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.

[Nicolas Bessi - Camptocamp (nbessi-c2c-deactivatedaccount)]

Hello,

Ok, I get it know :).

I will propose the MP on official branch and see if tey accept it.

If they don't I will do a MP that will only fix the fact that deactivated user are not encrypted.
And that will encrypt in all clear password in md5.

Do you agree with that?

Regards

Nicolas.

[Stefan Rijnhart (Opener) (stefan-opener)]

Excellent, thanks!

[Nicolas Bessi - Camptocamp (nbessi-c2c-deactivatedaccount)]

Hello,

MP is modified as disscussed with Olivier only init function will be accepted.
The same patch is proposed on official addons

[Stefan Rijnhart (Opener) (stefan-opener)]

Hi Nicolas, did you not want to take Olivier's suggestion of refactoring out the code that performs the encryption?

review: Needs Information

[Nicolas Bessi - Camptocamp (nbessi-c2c-deactivatedaccount)]

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

[Stefan Rijnhart (Opener) (stefan-opener)]

OK, thanks for the info!

review: Approve

[Nicolas Bessi - Camptocamp (nbessi-c2c-deactivatedaccount)]

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

[Nicolas Bessi - Camptocamp (nbessi-c2c-deactivatedaccount)]

I have done the small refactor proposed by Olivier (not the one with passlib).

Please redo a quick review before merging.

Regards

Nicolas

[Pedro Manuel Baeza (pedro.baeza)]

LGTM!

Better to have only one method that makes the encryption.

Thanks.

review: Approve (code review)

[Stefan Rijnhart (Opener) (stefan-opener)]

LGTM, thanks!

review: Approve

[Holger Brunn (Therp) (hbrunn)]

review: Approve (code review)

[Stefan Rijnhart (Opener) (stefan-opener)]

Setting to approved, as I count three approvals after the last change, even if that was in the superseded MP.

[Laurent Mignon (Acsone) (lmi)]

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...

review: Disapprove

[Holger Brunn (Therp) (hbrunn)]

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.

[Holger Brunn (Therp) (hbrunn)]

...and unfortunately, migration scripts are only called for updating a module, not when you install them, no matter what version you install

[Laurent Mignon (Acsone) (lmi)]

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

review: Approve

[Holger Brunn (Therp) (hbrunn)]

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

[Stefan Rijnhart (Opener) (stefan-opener)]

FYI: I'm attempting to make a migration script run upon the installation of another module in 7.0, and it does not seem to work. It must have worked in earlier versions of OpenERP, because we did take special precautions early on in OpenUpgrade to prevent migration scripts to run upon module installation (with version=None indeed).

[Laurent Mignon (Acsone) (lmi)]

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.

=== modified file 'auth_crypt/auth_crypt.py'
--- auth_crypt/auth_crypt.py	2013-08-12 10:29:50 +0000
+++ auth_crypt/auth_crypt.py	2014-03-19 14:54:21 +0000
@@ -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: