Merge lp:~allenap/maas/maas-set-correct-file-permissions into lp:maas/trunk

[Gavin Panella]

Normalise permissions of all files and directories installed via install-pxe-image.

Previously this was not done, and restrictive permissions could cause failures later on the TFTP server.

This also reverts revision 947, an earlier attempt to fix the same bug.

This changes install_dir() to normalize

[Scott Moser (smoser)]

its curious that you're doing chmod after you've put the files in place, given the big in comments there about a race condition. (which I honestly do not think that is a large concern as this will be very small window and not a common operation).

But why wouldn't you fix permissions *before* you move it into place?

[lp:~allenap/maas/maas-set-correct-file-permissions]

955. By Gavin Panella on 2012-08-31

Normalise permissions before the move.

[Gavin Panella (allenap)]

> But why wouldn't you fix permissions *before* you move it into place?

Good point, I've changed it to before. Thanks for spotting that.

[Scott Moser (smoser)]

Can you please revert the changes to maas-import-ephemerals that were incorrectly made at
https://code.launchpad.net/~jtv/maas/bug-1042865/+merge/121990

review: Needs Fixing

[lp:~allenap/maas/maas-set-correct-file-permissions]

956. By Gavin Panella on 2012-08-31

Revert revision 947; it did not have the desired effect.

957. By Gavin Panella on 2012-08-31

Remove another previous attempt to make install_tftp_image() dtrt.

[Scott Moser (smoser)]

review: Approve

=== modified file 'scripts/maas-import-ephemerals'
--- scripts/maas-import-ephemerals	2012-08-30 05:57:05 +0000
+++ scripts/maas-import-ephemerals	2012-08-31 17:38:20 +0000
@@ -342,12 +342,6 @@
 EOF
 fi
 
-
-# All files we create here are public.  The TFTP user will need to be
-# able to read them.
-umask a+r
-
-
 updates=0
 for release in $RELEASES; do
     for arch in $ARCHES; do
@@ -374,7 +368,6 @@
                 "$r_serial" "$r_arch" "$r_url" "$r_name" ||
                 fail "failed to prepare image for $release/$arch"
 
-            chmod -R a+rX "$wd"
             install_tftp_image "$wd" "$r_arch" "generic" "$r_release"
 
             target_name="${TARGET_NAME_PREFIX}${r_name}"

=== modified file 'src/provisioningserver/pxe/install_image.py'
--- src/provisioningserver/pxe/install_image.py	2012-08-30 11:19:16 +0000
+++ src/provisioningserver/pxe/install_image.py	2012-08-31 17:38:20 +0000
@@ -27,6 +27,7 @@
     compose_image_path,
     locate_tftp_path,
     )
+from twisted.python.filepath import FilePath
 
 
 def make_destination(tftproot, arch, subarch, release, purpose):
@@ -104,6 +105,13 @@
     # much.
     copytree(new, '%s.new' % old)
 
+    # Normalise permissions.
+    for filepath in FilePath('%s.new' % old).walk():
+        if filepath.isdir():
+            filepath.chmod(0755)
+        else:
+            filepath.chmod(0644)
+
     # Start of critical window.
     if os.path.isdir(old):
         os.rename(old, '%s.old' % old)

=== modified file 'src/provisioningserver/pxe/tests/test_install_image.py'
--- src/provisioningserver/pxe/tests/test_install_image.py	2012-08-30 11:19:16 +0000
+++ src/provisioningserver/pxe/tests/test_install_image.py	2012-08-31 17:38:20 +0000
@@ -34,6 +34,7 @@
     FileExists,
     Not,
     )
+from twisted.python.filepath import FilePath
 
 
 def make_arch_subarch_release_purpose():
@@ -191,3 +192,20 @@
             FileContains(contents))
         self.assertThat('%s.old' % published_image, Not(DirExists()))
         self.assertThat('%s.new' % published_image, Not(DirExists()))
+
+    def test_install_dir_normalises_permissions(self):
+        # install_dir() normalises directory permissions to 0755 and file
+        # permissions to 0644.
+        target_dir = FilePath(self.make_dir())
+        new_dir = FilePath(self.make_dir())
+        new_dir.chmod(0700)
+        new_image = new_dir.child("image")
+        new_image.touch()
+        new_image.chmod(0600)
+        install_dir(new_dir.path, target_dir.path)
+        self.assertEqual(
+            "rwxr-xr-x",
+            target_dir.getPermissions().shorthand())
+        self.assertEqual(
+            "rw-r--r--",
+            target_dir.child("image").getPermissions().shorthand())