random: fix password recipe when using storage-path and passwd

As discussed on bb841a7b (comment 219278)
when using storage-path and passwd option, the storage file could not
be updated to the new format because of AttributeError _needs_migration.

This changes to no longer try to detect if the storage needs migration,
but just compare the expected content of the storage file during install
and overwrite the file if it is different.

This new approach also fix a behavior that re-running buildout with
storage-path option and a different passwd option did not update the
storage file. Now it is also updated.

slapos/recipe/random.py

@@ -150,27 +150,26 @@ class Password(object):
     except KeyError:
       self.storage_path = options['storage-path'] = os.path.join(
         buildout['buildout']['parts-directory'], name)
-    passwd_dict = {
-      '': options.get('passwd')
-    }
-    if not passwd_dict['']:
-      if self.storage_path:
-        self._needs_migration = False
-        try:
-          with open(self.storage_path) as f:
-            content = f.read().strip('\n')
-            # new format: the file contains password and hashes in json format
-            try:
-              passwd_dict = json.loads(content)
-              if sys.version_info < (3, ):
-                passwd_dict = {k: v.encode() for k, v in passwd_dict.items()}
-            except ValueError:
-              # old format: the file only contains the password in plain text
-              passwd_dict[''] = content
-              self._needs_migration = True
-        except IOError as e:
-          if e.errno != errno.ENOENT:
-            raise
+
+    passwd_option = options.get('passwd')
+    passwd_dict = {'': passwd_option}
+    if self.storage_path:
+      try:
+        with open(self.storage_path) as f:
+          content = f.read().strip('\n')
+          # new format: the file contains password and hashes in json format
+          try:
+            passwd_dict = json.loads(content)
+            if sys.version_info < (3, ):
+              passwd_dict = {k: v.encode() for k, v in passwd_dict.items()}
+          except ValueError:
+            # old format: the file only contains the password in plain text
+            passwd_dict[''] = content
+      except IOError as e:
+        if e.errno != errno.ENOENT:
+          raise
+      if passwd_option and passwd_dict[''] != passwd_option:
+        passwd_dict[''] = passwd_option
 
       if not passwd_dict['']:
         passwd_dict[''] = self.generatePassword(int(options.get('bytes', '16')))
@@ -202,17 +201,17 @@ class Password(object):
 
   def install(self):
     if self.storage_path:
+      serialized = json.dumps(self.passwd_dict, sort_keys=True)
+      stored = None
       try:
-        # The following 2 lines are just an optimization to avoid recreating
-        # the file with the same content.
-        if self.create_once and os.stat(self.storage_path).st_size and not self._needs_migration:
-          return
-        os.unlink(self.storage_path)
-      except OSError as e:
+        with open(self.storage_path) as f:
+          stored = f.read()
+      except IOError as e:
         if e.errno != errno.ENOENT:
           raise
-      with open(self.storage_path, 'w') as f:
-        json.dump(self.passwd_dict, f)
+      if stored != serialized:
+        with open(self.storage_path, 'w') as f:
+          f.write(serialized)
       if not self.create_once:
         return self.storage_path
 

slapos/test/recipe/test_random.py

@@ -44,8 +44,23 @@ class TestPassword(unittest.TestCase):
     self._makeRecipe({'storage-path': tf.name}, "another").install()
     self.assertEqual(self.buildout["another"]["passwd"], passwd)
 
+  def test_storage_path_passwd_set_in_options(self):
+    tf = tempfile.NamedTemporaryFile(delete=False)
+    self.addCleanup(os.unlink, tf.name)
+    self._makeRecipe({'storage-path': tf.name, 'passwd': 'secret'}).install()
+    with open(tf.name) as f:
+      self.assertEqual(json.load(f), {'': 'secret'})
+
+    self._makeRecipe({'storage-path': tf.name}, "another").install()
+    self.assertEqual(self.buildout["another"]["passwd"], 'secret')
+
+    self._makeRecipe({'storage-path': tf.name, 'passwd': 'updated'}, "updated").install()
+    self.assertEqual(self.buildout["updated"]["passwd"], 'updated')
+    with open(tf.name) as f:
+      self.assertEqual(json.load(f), {'': 'updated'})
+
   def test_storage_path_legacy_format(self):
-    with tempfile.NamedTemporaryFile(delete=False) as tf:
+    with tempfile.NamedTemporaryFile() as tf:
       tf.write(b'secret\n')
       tf.flush()
 
@@ -56,8 +71,22 @@ class TestPassword(unittest.TestCase):
       with open(tf.name) as f:
         self.assertEqual(json.load(f), {'': 'secret'})
 
-    self._makeRecipe({'storage-path': tf.name}, "another").install()
-    self.assertEqual(self.buildout["another"]["passwd"], passwd)
+      self._makeRecipe({'storage-path': tf.name}, "another").install()
+      self.assertEqual(self.buildout["another"]["passwd"], passwd)
+
+  def test_storage_path_legacy_format_passwd_set_in_options(self):
+    with tempfile.NamedTemporaryFile() as tf:
+      tf.write(b'secret\n')
+      tf.flush()
+      self._makeRecipe({'storage-path': tf.name, 'passwd': 'secret'}).install()
+      passwd = self.buildout["random"]["passwd"]
+      self.assertEqual(passwd, 'secret')
+      tf.flush()
+      with open(tf.name) as f:
+        self.assertEqual(json.load(f), {'': 'secret'})
+
+      self._makeRecipe({'storage-path': tf.name}, "another").install()
+      self.assertEqual(self.buildout["another"]["passwd"], passwd)
 
   def test_bytes(self):
     self._makeRecipe({'bytes': '32'}).install()