From e7f4ebc00024fbde96fce3ca6fa71b1609d55b86 Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Mon, 16 Jul 2012 08:05:24 +0200 Subject: [PATCH 1/5] [IMP] move conversion of existing passwords to base_crypt's res.users.init bzr revid: xmo@openerp.com-20120716060524-bxxhu2hrowba2e0y --- addons/base_crypt/crypt.py | 38 +++++++++----------------------------- 1 file changed, 9 insertions(+), 29 deletions(-) diff --git a/addons/base_crypt/crypt.py b/addons/base_crypt/crypt.py index 9b91f7d3108..e08107c2045 100644 --- a/addons/base_crypt/crypt.py +++ b/addons/base_crypt/crypt.py @@ -135,6 +135,15 @@ class users(osv.osv): # agi - 022108 # Add handlers for 'input_pw' field. + def init(self, cr): + cr.execute("SELECT id, password FROM res_users " + "WHERE active=true AND password NOT LIKE '$%' " + "FOR UPDATE") + for id, password in cr.fetchall(): + cr.execute("UPDATE res_users SET password=%s WHERE id=%s", + (encrypt_md5(password, gen_salt()), id)) + + def set_pw(self, cr, uid, id, name, value, args, context): if not value: raise osv.except_osv(_('Error'), _("Please specify the password !")) @@ -196,12 +205,6 @@ class users(osv.osv): # Return early if no one has a login name like that. return False - stored_pw = self.maybe_encrypt(cr, stored_pw, id) - - if not stored_pw: - # means couldn't encrypt or user is not active! - return False - # Calculate an encrypted password from the user-provided # password. obj = pooler.get_pool(db).get('res.users') @@ -270,29 +273,6 @@ class users(osv.osv): else: self._uid_cache[db] = {uid: passwd} return bool(res) - - def maybe_encrypt(self, cr, pw, id): - """ Return the password 'pw', making sure it is encrypted. - - If the password 'pw' is not encrypted, then encrypt all active passwords - in the db. Returns the (possibly newly) encrypted password for 'id'. - """ - - if not pw.startswith(magic_md5): - cr.execute("SELECT id, password FROM res_users " \ - "WHERE active=true AND password NOT LIKE '$%'") - # Note that we skip all passwords like $.., in anticipation for - # more than md5 magic prefixes. - res = cr.fetchall() - for i, p in res: - encrypted = encrypt_md5(p, gen_salt()) - cr.execute('UPDATE res_users SET password=%s where id=%s', - (encrypted, i)) - if i == id: - encrypted_res = encrypted - cr.commit() - return encrypted_res - return pw users() # vim:expandtab:smartindent:tabstop=4:softtabstop=4:shiftwidth=4: From 835cb5bcfac1400f87c78a813cfa160cf927d942 Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Mon, 16 Jul 2012 08:49:03 +0200 Subject: [PATCH 2/5] [FIX] concurrency issue when installing base_crypt from web client Basically (and apparently), as base_crypt performs its conversion of passwords to encrypted passwords a new request arrives and tries to check its login, blowing up during the update. * For the conversion, lock the whole table before updating the passwords, as we need to touch every record anyway we can just put a big exclusive lock on res_users * Ideally we'd be able to just lock the relevant row of res_users in check/_login, but for some reason I don't quite get this does not work and still yields > TransactionRollbackError: could not serialize access due to concurrent update except on the SELECT FOR UPDATE query instead of the UPDATE one. The docs don't seem to help, and replacing the FOR UPDATE row-level lock by a table lock works even though they appear pretty much at the same point. Me no get. bzr revid: xmo@openerp.com-20120716064903-403pth90kupgfdv3 --- addons/base_crypt/crypt.py | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/addons/base_crypt/crypt.py b/addons/base_crypt/crypt.py index e08107c2045..3adba2864bc 100644 --- a/addons/base_crypt/crypt.py +++ b/addons/base_crypt/crypt.py @@ -35,9 +35,11 @@ # 59 Temple Place - Suite 330 # Boston, MA 02111-1307 # USA. - +from __future__ import with_statement +from contextlib import closing from random import seed, sample from string import ascii_letters, digits + from osv import fields,osv import pooler from tools.translate import _ @@ -136,13 +138,16 @@ class users(osv.osv): # Add handlers for 'input_pw' field. def init(self, cr): - cr.execute("SELECT id, password FROM res_users " - "WHERE active=true AND password NOT LIKE '$%' " - "FOR UPDATE") - for id, password in cr.fetchall(): - cr.execute("UPDATE res_users SET password=%s WHERE id=%s", - (encrypt_md5(password, gen_salt()), id)) + with closing(pooler.get_db(cr.dbname).cursor()) as cr: + cr.execute('LOCK res_users') + cr.execute("SELECT id, password FROM res_users " + "WHERE active=true AND password NOT LIKE '$%' ") + cr.executemany("UPDATE res_users SET password=%s WHERE id=%s", + ((encrypt_md5(password, gen_salt()), id) + for id, password in cr.fetchall())) + + cr.commit() def set_pw(self, cr, uid, id, name, value, args, context): if not value: @@ -244,6 +249,7 @@ class users(osv.osv): cr = pooler.get_db(db).cursor() try: + cr.execute('LOCK res_users') if uid not in self._salt_cache.get(db, {}): # If we don't have cache, we have to repeat the procedure # through the login function. From ef603ac33786bdd21ebbdb76c51562141e55c58e Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Mon, 16 Jul 2012 08:56:40 +0200 Subject: [PATCH 3/5] [IMP] replace manual try:finally:close patterns with 'with closing' ones bzr revid: xmo@openerp.com-20120716065640-qb69ojaljd5z6c39 --- addons/base_crypt/crypt.py | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/addons/base_crypt/crypt.py b/addons/base_crypt/crypt.py index 3adba2864bc..9029a4a5503 100644 --- a/addons/base_crypt/crypt.py +++ b/addons/base_crypt/crypt.py @@ -36,7 +36,9 @@ # Boston, MA 02111-1307 # USA. from __future__ import with_statement + from contextlib import closing +import logging from random import seed, sample from string import ascii_letters, digits @@ -188,17 +190,13 @@ class users(osv.osv): return False if db is False: raise RuntimeError("Cannot authenticate to False db!") - cr = None + try: - cr = pooler.get_db(db).cursor() - return self._login(cr, db, login, password) + with closing(pooler.get_db(db).cursor()) as cr: + return self._login(cr, db, login, password) except Exception: - import logging logging.getLogger('netsvc').exception('Could not authenticate') return Exception('Access Denied') - finally: - if cr is not None: - cr.close() def _login(self, cr, db, login, password): cr.execute( 'SELECT password, id FROM res_users WHERE login=%s AND active', @@ -247,8 +245,7 @@ class users(osv.osv): if (cached_pass is not None) and cached_pass == passwd: return True - cr = pooler.get_db(db).cursor() - try: + with closing(pooler.get_db(db).cursor()) as cr: cr.execute('LOCK res_users') if uid not in self._salt_cache.get(db, {}): # If we don't have cache, we have to repeat the procedure @@ -266,8 +263,6 @@ class users(osv.osv): cr.execute('SELECT COUNT(*) FROM res_users WHERE id=%s AND password=%s AND active', (int(uid), encrypt_md5(passwd, salt))) res = cr.fetchone()[0] - finally: - cr.close() if not bool(res): raise security.ExceptionNoTb('AccessDenied') From fb346a6de3254981c980b632006dd98a3b341be6 Mon Sep 17 00:00:00 2001 From: Olivier Dony Date: Mon, 13 Aug 2012 16:35:52 +0200 Subject: [PATCH 4/5] [FIX] base_crypt: avoid table lock on res_users during simple auth check Performing a full table lock is potentially very slow due to the number of tables holding foreign keys towards res_users, so we should avoid doing that for simple auth check() calls. The point was only to avoid failing the update of the last login timestamp, which we don't actually care much about. So we can replace that with a simple SELECT FOR UPDATE NOWAIT on that specific user row and and skip the update in case the LOCK was not granted bzr revid: odo@openerp.com-20120813143552-3g3z0q9n1iox8a0b --- addons/base_crypt/crypt.py | 32 ++++++++++++++++++++------------ 1 file changed, 20 insertions(+), 12 deletions(-) diff --git a/addons/base_crypt/crypt.py b/addons/base_crypt/crypt.py index 9029a4a5503..1abb7d99bae 100644 --- a/addons/base_crypt/crypt.py +++ b/addons/base_crypt/crypt.py @@ -217,18 +217,25 @@ class users(osv.osv): encrypted_pw = encrypt_md5(password, salt) # Check if the encrypted password matches against the one in the db. - cr.execute("""UPDATE res_users - SET date=now() AT TIME ZONE 'UTC' - WHERE id=%s AND password=%s AND active - RETURNING id""", - (int(id), encrypted_pw.encode('utf-8'))) - res = cr.fetchone() - cr.commit() + login_ok = encrypted_pw == stored_pw + + # Attempt to update last login time, but don't fail if the row is + # currently locked. + if login_ok: + try: + cr.execute("SELECT 1 FROM res_users WHERE id=%s FOR UPDATE NOWAIT", + params=(id,), log_exceptions=False) + cr.execute("""UPDATE res_users + SET date=now() AT TIME ZONE 'UTC' + WHERE id=%s""", (id,)) + cr.commit() + except Exception: + # Failing to acquire the lock on the res_users row probably means + # another request is holding it. No big deal, we don't want to + # prevent/delay login in that case just for updating login timestamp. + cr.rollback() - if res: - return res[0] - else: - return False + return login_ok and id def check(self, db, uid, passwd): if not passwd: @@ -245,8 +252,9 @@ class users(osv.osv): if (cached_pass is not None) and cached_pass == passwd: return True + # We should get here only if the registry was just reloaded + # (e.g. after server startup or module install/upgrade) with closing(pooler.get_db(db).cursor()) as cr: - cr.execute('LOCK res_users') if uid not in self._salt_cache.get(db, {}): # If we don't have cache, we have to repeat the procedure # through the login function. From 468ce1b1278a41c14d790f10dc69b77531722986 Mon Sep 17 00:00:00 2001 From: Olivier Dony Date: Mon, 13 Aug 2012 16:52:00 +0200 Subject: [PATCH 5/5] [FIX] base_crypt: safer initial crypting of user passwords Avoid locking the full users table unless there is really at least one user record to crypt. Avoid using a separate cursor that could create a deadlock or TransactionRollbackError in the main init() transaction. bzr revid: odo@openerp.com-20120813145200-cc0gfzz04aundxhw --- addons/base_crypt/crypt.py | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/addons/base_crypt/crypt.py b/addons/base_crypt/crypt.py index 1abb7d99bae..4126ea0eeb4 100644 --- a/addons/base_crypt/crypt.py +++ b/addons/base_crypt/crypt.py @@ -140,16 +140,18 @@ class users(osv.osv): # Add handlers for 'input_pw' field. def init(self, cr): - with closing(pooler.get_db(cr.dbname).cursor()) as cr: + cr.execute("SELECT id, password FROM res_users " + "WHERE active=true AND password NOT LIKE '$%' ") + to_crypt = cr.fetchall() + if to_crypt: + # Full table lock to guarantee we won't do a partial job or + # end up with a TransactionRollbackError if another transaction + # alters res_users in the mean time. The lock will be released + # by the commit() that follows the model initialization step cr.execute('LOCK res_users') - cr.execute("SELECT id, password FROM res_users " - "WHERE active=true AND password NOT LIKE '$%' ") - cr.executemany("UPDATE res_users SET password=%s WHERE id=%s", ((encrypt_md5(password, gen_salt()), id) - for id, password in cr.fetchall())) - - cr.commit() + for id, password in to_crypt)) def set_pw(self, cr, uid, id, name, value, args, context): if not value: