Merge lp:~therp-nl/openupgrade-server/7.0_lp1265463_use_password into lp:openupgrade-server

[Ronald Portier (Therp)]

Migrate script will use all database connection info found in the config file that is already a required parameter.

[Holger Brunn (Therp) (hbrunn)]

10ff: You need to check if this option exists (has_option) before accessing it, as also SafeConfigParser raises ConfigParser.NoOptionError on nonexisting options. Then you can remove 12f

Could you explain why you do the extra checks and unset PGPASSWORD? The environment you change will die with the script process, and the only other processes who see it are the openupgrade servers it starts, or am I mistaken?

review: Needs Fixing (code review)

[lp:~therp-nl/openupgrade-server/7.0_lp1265463_use_password]

4642. By Ronald Portier (Therp) on 2014-01-20

[FIX] Properly check for existance of option before use.

[Ronald Portier (Therp) (rportier1962)]

@Holger

I changed the code to properly test for the existance of a configuration key before use.

I took over the unsetting of PGPASSWORD from the database back-up code of standard OpenERP. I think it is not wrong to clear a plaintext password from memory as soon as possible. Although ofcourse there already is a certain security risk in having the password in a plain text config file in the first place.

review: Needs Resubmitting

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

By the looks of it, the whole environment manipulation is only done to copy a database. Would it be an idea to simply copy the database using the cursor, using

    CREATE DATABASE newdb WITH TEMPLATE originaldb OWNER dbuser;

?

review: Needs Information

[Holger Brunn (Therp) (hbrunn)]

That would be a splendid idea, if not this choked if originaldb is in use, whereas pg_dump can work on locked databases. Some people might be brave or careless enough to run the script on their production database...

Thanks for the other changes!

review: Approve (code review)

=== modified file 'scripts/migrate.py'
--- scripts/migrate.py	2013-05-09 13:49:52 +0000
+++ scripts/migrate.py	2014-01-20 21:57:21 +0000
@@ -94,13 +94,25 @@
 
 config.read(options.config)
 
-db_user=config.get('options', 'db_user')
+conn_parms = {}
+for parm in ('host', 'port', 'user', 'password'):                             
+    db_parm = 'db_' + parm
+    if config.has_option('options', db_parm):
+        conn_parms[parm] = config.get('options', db_parm)
+
+if not 'user' in conn_parms:
+    print 'No user found in configuration'
+    sys.exit()
+db_user = conn_parms['user']
+
 db_name=options.database or config.get('options', 'db_name')
 
 if not db_name or db_name=='' or db_name.isspace() or db_name.lower()=='false':
   parser.print_help()
   sys.exit()
 
+conn_parms['database'] = db_name
+
 if options.inplace:
   db=db_name
 else:
@@ -185,18 +197,36 @@
 if not options.inplace:
     print('copying database %(db_name)s to %(db)s...' % {'db_name': db_name, 
                                                          'db': db})
-    conn=psycopg2.connect(database=db_name, user=db_user)
+    conn = psycopg2.connect(**conn_parms)
     conn.set_isolation_level(psycopg2.extensions.ISOLATION_LEVEL_AUTOCOMMIT)
     cur=conn.cursor()
     cur.execute('drop database if exists "%(db)s"' % {'db': db})
     cur.execute('create database "%(db)s"' % {'db': db})
     cur.close()
-    os.system(('pg_dump --format=custom --user=%(user)s %(db_name)s | pg_restore '+
-    '--dbname=%(db)s') % {
-        'user': db_user, 
-        'db_name': db_name, 
-        'db': db,
-        })
+
+    os.environ['PGUSER'] = db_user             
+    if ('host' in conn_parms and conn_parms['host']
+    and not os.environ.get('PGHOST')):
+        os.environ['PGHOST'] = conn_parms['host']             
+
+    if ('port' in conn_parms and conn_parms['port']
+    and not os.environ.get('PGPORT')):
+        os.environ['PGPORT'] = conn_parms['port']             
+
+    password_set = False
+    if ('password' in conn_parms and conn_parms['password']
+    and not os.environ.get('PGPASSWORD')):
+        os.environ['PGPASSWORD'] = conn_parms['password']             
+        password_set = True
+
+    os.system(
+        ('pg_dump --format=custom --no-password %(db_name)s ' +
+         '| pg_restore --no-password --dbname=%(db)s') %
+        {'db_name': db_name, 'db': db}
+    )
+
+    if password_set:
+        del os.environ['PGPASSWORD'] 
 
 for version in options.migrations.split(','):
   print 'running migration for '+version