launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:publisher-staged-file-modes into launchpad:master.

Commit message:
publish-ftpmaster: Ensure that staged files are world-readable

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1880960 in Launchpad itself: "Publisher publishing files without read permissions"
  https://bugs.launchpad.net/launchpad/+bug/1880960

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/398661

Very occasionally, files synced from other services (e.g. the command-not-found generator) via the staging area seem to turn up with non-world-readable permissions, perhaps due to bugs in those other services.  This breaks mirroring.  Ensure that everything published from the staging area has at least mode 0o444.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:publisher-staged-file-modes into launchpad:master.

diff --git a/lib/lp/archivepublisher/scripts/publish_ftpmaster.py b/lib/lp/archivepublisher/scripts/publish_ftpmaster.py
index 27427cf..e2d22a6 100644
--- a/lib/lp/archivepublisher/scripts/publish_ftpmaster.py
+++ b/lib/lp/archivepublisher/scripts/publish_ftpmaster.py
@@ -496,6 +496,12 @@ class PublishFTPMaster(LaunchpadCronScript):
                             current_path,
                             (math.ceil(st.st_atime), math.ceil(st.st_mtime)))
                         os.unlink(new_path)
+                    # Make sure that the file is world-readable, since
+                    # occasionally files synced from other services have
+                    # been known to end up mode 0o600 or similar and that
+                    # breaks mirroring.
+                    os.chmod(
+                        current_path, os.stat(current_path).st_mode | 0o444)
                     updated = True
         return updated
 
diff --git a/lib/lp/archivepublisher/tests/test_publish_ftpmaster.py b/lib/lp/archivepublisher/tests/test_publish_ftpmaster.py
index c8077b5..b9a82b7 100644
--- a/lib/lp/archivepublisher/tests/test_publish_ftpmaster.py
+++ b/lib/lp/archivepublisher/tests/test_publish_ftpmaster.py
@@ -1,4 +1,4 @@
-# Copyright 2011-2019 Canonical Ltd.  This software is licensed under the
+# Copyright 2011-2021 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Test publish-ftpmaster cron script."""
@@ -9,6 +9,7 @@ __metaclass__ = type
 
 import logging
 import os
+import stat
 from textwrap import dedent
 import time
 
@@ -94,15 +95,18 @@ def get_distscopy_root(pub_config):
     return get_archive_root(pub_config) + "-distscopy"
 
 
-def write_marker_file(path, contents):
+def write_marker_file(path, contents, mode=None):
     """Write a marker file for checking directory movements.
 
     :param path: A list of path components.
     :param contents: Text to write into the file.
+    :param mode: If given, explicitly set the file to this permission mode.
     """
     with open(os.path.join(*path), "w") as marker:
         marker.write(contents)
         marker.flush()
+        if mode is not None:
+            os.fchmod(marker.fileno(), mode)
 
 
 def read_marker_file(path):
@@ -794,6 +798,34 @@ class TestPublishFTPMasterScript(
         self.assertFalse(script.updateStagedFilesForSuite(
             archive_config, distroseries.name))
 
+    def test_updateStagedFilesForSuite_ensures_world_readable(self):
+        # updateStagedFilesForSuite ensures that files it stages have
+        # sufficient permissions not to break mirroring.
+        distro = self.makeDistroWithPublishDirectory()
+        distroseries = self.factory.makeDistroSeries(distribution=distro)
+        script = self.makeScript(distro)
+        script.setUp()
+        script.setUpDirs()
+        archive_config = getPubConfig(distro.main_archive)
+        backup_suite = os.path.join(
+            archive_config.archiveroot + "-distscopy", "dists",
+            distroseries.name)
+        os.makedirs(backup_suite)
+        staging_suite = os.path.join(
+            archive_config.stagingroot, distroseries.name)
+        os.makedirs(staging_suite)
+        for name, mode in (
+                ("Commands-amd64", 0o644), ("Commands-i386", 0o600)):
+            write_marker_file([staging_suite, name], name, mode=mode)
+        self.assertTrue(script.updateStagedFilesForSuite(
+            archive_config, distroseries.name))
+        for name in ("Commands-amd64", "Commands-i386"):
+            self.assertEqual(name, read_marker_file([backup_suite, name]))
+            self.assertEqual(
+                0o644,
+                stat.S_IMODE(
+                    os.stat(os.path.join(backup_suite, name)).st_mode))
+
     def test_updateStagedFiles_marks_suites_dirty(self):
         # updateStagedFiles marks the suites for which it updated staged
         # files as dirty.