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

code LGTM. Not tested.

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

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

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.

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.

Excellent, thanks!

Hello,

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

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

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

OK, thanks for the info!

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

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

Please redo a quick review before merging.

Regards

Nicolas

LGTM!

Better to have only one method that makes the encryption.

Thanks.

LGTM, thanks!