launchpad-reviewers team mailing list archive

[Graham Binns <graham@xxxxxxxxxxxxx>]

Graham Binns has proposed merging lp:~gmb/lpsetup/dont-blow-away-ssh-keys-bug-1018823 into lp:lpsetup.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1018823 in lpsetup: "lpsetup probably shouldn't overwrite your SSH keys with nonsense"
  https://bugs.launchpad.net/lpsetup/+bug/1018823

For more details, see:
https://code.launchpad.net/~gmb/lpsetup/dont-blow-away-ssh-keys-bug-1018823/+merge/113192

This branch fixes the initialize step in init_host so as to prevent the user's SSH keys being overwritten with garbage if they happen to not be paying attention to what they're doing.

I couldn't find any direct tests for initialize(), so I moved the functionality that overwrites the SSH keyfiles out into a utility function and added a doctest for it. The new functionality is to raise an error if the file already exists with different contents from those we're trying to write to it (so opening the file, reading it and then writing its contents back again - our existing behaviour - shouldn't cause problems). I've also updated python-shelltoolboxes generate_ssh_keys() function to ensure that it raises an error should you try to generate new SSH keys with the same name as existing ones.
-- 
https://code.launchpad.net/~gmb/lpsetup/dont-blow-away-ssh-keys-bug-1018823/+merge/113192
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~gmb/lpsetup/dont-blow-away-ssh-keys-bug-1018823 into lp:lpsetup.

=== modified file 'lpsetup/subcommands/inithost.py'
--- lpsetup/subcommands/inithost.py	2012-06-28 10:33:46 +0000
+++ lpsetup/subcommands/inithost.py	2012-07-03 10:31:25 +0000
@@ -40,6 +40,47 @@
 from lpsetup.utils import call
 
 
+def write_file_contents(filename, contents, mode):
+    """Write `contents` to filename and chmod it to 0644.
+
+        >>> write_file_contents('/tmp/foo', 'Hello, world', 'w')
+        >>> print open('/tmp/foo', 'r').read()
+        Hello, world
+        <BLANKLINE>
+        >>> os.remove('/tmp/foo')
+
+    If the file already exists, `mode` is set to 'w', and the file's
+    contents differ from `contents`, write_file_contents() raises an
+    Exception.
+
+        >>> write_file_contents('/tmp/foo', 'Hello, world', 'w')
+        >>> write_file_contents(
+        ...     '/tmp/foo', 'Hello again, world', 'w') # doctest: +ELLIPSIS
+        Traceback (most recent call last):
+        Exception: File /tmp/foo already exists with different contents...
+        >>> os.remove('/tmp/foo')
+
+    If the existing file's contents don't differ from `contents`,
+    write_file_contents() will simply return.
+
+        >>> write_file_contents('/tmp/foo', 'Hello, world', 'w')
+        >>> write_file_contents(
+        ...     '/tmp/foo', 'Hello, world', 'w') # doctest: +ELLIPSIS
+        >>> os.remove('/tmp/foo')
+    """
+    if mode == 'w' and os.path.exists(filename):
+        with open(filename, 'r') as existing_file:
+            file_contents = existing_file.read()
+        if file_contents.strip() != contents.strip():
+            raise Exception(
+                "File %s already exists with different contents." % filename)
+        else:
+            return
+    with open(filename, mode) as f:
+        f.write(contents + '\n')
+    os.chmod(filename, 0644)
+
+
 def initialize(
     user, full_name, email, lpuser, private_key, public_key, valid_ssh_keys,
     ssh_key_path, feed_random):
@@ -71,9 +112,7 @@
             (auth_file, public_key, 'a'),
             (known_hosts, known_host_content, 'a'),
             ]:
-            with open(filename, mode) as f:
-                f.write(contents + '\n')
-            os.chmod(filename, 0644)
+            write_file_contents(filename, contents, mode)
         os.chmod(ssh_key_path, 0600)
         # Set up bzr and Launchpad authentication.
         call('bzr', 'whoami', formataddr([full_name, email]))