Merge lp:~lifeless/python-oops-datedir-repo/bug-891251 into lp:python-oops-datedir-repo

[Robert Collins]

Allow txlongpoll oopses to be analysed and viewed.

[William Grant (wgrant)]

os.makedirs takes a mode argument, but apart from that this is good.

review: Approve (code)

=== modified file 'NEWS'
--- NEWS	2011-11-16 03:44:36 +0000
+++ NEWS	2011-11-17 00:02:23 +0000
@@ -6,6 +6,13 @@
 NEXT
 ----
 
+0.0.13
+------
+
+* The date directory created when writing an OOPS with hashed names has its
+  permissions chmodded in the same way that the OOPS report file itself does.
+  (Robert Collins, #891251)
+
 0.0.12
 ------
 

=== modified file 'oops_datedir_repo/__init__.py'
--- oops_datedir_repo/__init__.py	2011-11-16 03:44:36 +0000
+++ oops_datedir_repo/__init__.py	2011-11-17 00:02:23 +0000
@@ -25,7 +25,7 @@
 # established at this point, and setup.py will use a version of next-$(revno).
 # If the releaselevel is 'final', then the tarball will be major.minor.micro.
 # Otherwise it is major.minor.micro~$(revno).
-__version__ = (0, 0, 12, 'beta', 0)
+__version__ = (0, 0, 13, 'beta', 0)
 
 __all__ = [
     'DateDirRepo',

=== modified file 'oops_datedir_repo/repository.py'
--- oops_datedir_repo/repository.py	2011-11-14 03:20:31 +0000
+++ oops_datedir_repo/repository.py	2011-11-17 00:02:23 +0000
@@ -103,6 +103,11 @@
         :param now: The datetime to use as the current time.  Will be
             determined if not supplied.  Useful for testing.
         """
+        # We set file permission to: rw-r--r-- (so that reports from
+        # umask-restricted services can be gathered by a tool running as
+        # another user).
+        wanted_file_permission = (
+            stat.S_IRUSR | stat.S_IWUSR | stat.S_IRGRP | stat.S_IROTH)
         if now is not None:
             now = now.astimezone(utc)
         else:
@@ -118,6 +123,10 @@
             prefix = os.path.join(self.root, now.strftime('%Y-%m-%d'))
             if not os.path.isdir(prefix):
                 os.makedirs(prefix)
+                # For directories we need to set the x bits too.
+                os.chmod(
+                    prefix, wanted_file_permission | stat.S_IXUSR | stat.S_IXGRP |
+                    stat.S_IXOTH)
             filename = os.path.join(prefix, oopsid)
         if self.inherit_id:
             oopsid = report.get('id') or oopsid
@@ -126,12 +135,7 @@
         os.rename(filename + '.tmp', filename)
         if self.stash_path:
             original_report['datedir_repo_filepath'] = filename
-        # Set file permission to: rw-r--r-- (so that reports from
-        # umask-restricted services can be gathered by a tool running as
-        # another user).
-        wanted_permission = (
-            stat.S_IRUSR | stat.S_IWUSR | stat.S_IRGRP | stat.S_IROTH)
-        os.chmod(filename, wanted_permission)
+        os.chmod(filename, wanted_file_permission)
         return report['id']
 
     def republish(self, publisher):

=== modified file 'oops_datedir_repo/tests/test_repository.py'
--- oops_datedir_repo/tests/test_repository.py	2011-11-14 03:20:31 +0000
+++ oops_datedir_repo/tests/test_repository.py	2011-11-17 00:02:23 +0000
@@ -22,11 +22,17 @@
 import os.path
 import stat
 
-from fixtures import TempDir
+from fixtures import (
+    Fixture,
+    TempDir,
+    )
 import bson
 from pytz import utc
 import testtools
-from testtools.matchers import raises
+from testtools.matchers import (
+    Equals,
+    raises,
+    )
 
 from oops_datedir_repo import (
     DateDirRepo,
@@ -35,9 +41,39 @@
 from oops_datedir_repo.uniquefileallocator import UniqueFileAllocator
 
 
+class HasUnixPermissions:
+
+    def __init__(self, wanted_permission):
+        self.wanted_permission = wanted_permission
+
+    def match(self, path):
+        st = os.stat(path)
+        # Get only the permission bits from this mode.
+        file_permission = stat.S_IMODE(st.st_mode)
+        return Equals(self.wanted_permission).match(file_permission)
+
+    def __str__(self):
+        # TODO: might be nice to split out the bits in future, format nicely
+        # etc. Should also move this into testtools.
+        return "HasUnixPermissions(0%o)" % self.wanted_permission
+
+
+class UMaskFixture(Fixture):
+    """Set a umask temporarily."""
+
+    def __init__(self, mask):
+        super(UMaskFixture, self).__init__()
+        self.umask_permission = mask
+
+    def setUp(self):
+        super(UMaskFixture, self).setUp()
+        old_umask = os.umask(self.umask_permission)
+        self.addCleanup(os.umask, old_umask)
+
+
 class TestDateDirRepo(testtools.TestCase):
 
-    def test_publish_permissions(self):
+    def test_publish_permissions_lognamer(self):
         errordir = self.useFixture(TempDir()).path
         repo = DateDirRepo(errordir, 'T')
         report = {'id': 'OOPS-91T1'}
@@ -45,21 +81,36 @@
 
         # Set up default file creation mode to rwx------ as some restrictive
         # servers do.
-        umask_permission = stat.S_IRWXG | stat.S_IRWXO
-        old_umask = os.umask(umask_permission)
-        self.addCleanup(os.umask, old_umask)
+        self.useFixture(UMaskFixture(stat.S_IRWXG | stat.S_IRWXO))
         repo.publish(report, now)
 
         errorfile = os.path.join(repo.log_namer.output_dir(now), '01800.T1')
-        self.assertTrue(os.path.exists(errorfile))
-
-        # Check errorfile is set with the correct permission: rw-r--r--
-        st = os.stat(errorfile)
-        wanted_permission = (
-            stat.S_IRUSR | stat.S_IWUSR | stat.S_IRGRP | stat.S_IROTH)
-        # Get only the permission bits for this file.
-        file_permission = stat.S_IMODE(st.st_mode)
-        self.assertEqual(file_permission, wanted_permission)
+        # Check errorfile and directory are set with the correct permission:
+        # rw-r--r-- and rwxr-xr-x accordingly
+        file_perms = stat.S_IRUSR | stat.S_IWUSR | stat.S_IRGRP | stat.S_IROTH
+        has_file_permission = HasUnixPermissions(file_perms)
+        has_dir_permission = HasUnixPermissions(
+            file_perms | stat.S_IXUSR | stat.S_IXGRP | stat.S_IXOTH)
+        self.assertThat(errorfile, has_file_permission)
+        self.assertThat(repo.log_namer.output_dir(now), has_dir_permission)
+
+    def test_publish_permissions_hashnames(self):
+        repo = DateDirRepo(self.useFixture(TempDir()).path, stash_path=True)
+        report = {'id': 'OOPS-91T1'}
+        now = datetime.datetime(2006, 04, 01, 00, 30, 00, tzinfo=utc)
+
+        # Set up default file creation mode to rwx------ as some restrictive
+        # servers do.
+        self.useFixture(UMaskFixture(stat.S_IRWXG | stat.S_IRWXO))
+        repo.publish(report, now)
+        # Check errorfile and directory are set with the correct permission:
+        # rw-r--r--
+        file_perms = stat.S_IRUSR | stat.S_IWUSR | stat.S_IRGRP | stat.S_IROTH
+        has_file_permission = HasUnixPermissions(file_perms)
+        has_dir_permission = HasUnixPermissions(
+            file_perms | stat.S_IXUSR | stat.S_IXGRP | stat.S_IXOTH)
+        self.assertThat(report['datedir_repo_filepath'], has_file_permission)
+        self.assertThat(repo.root + '/2006-04-01', has_dir_permission)
 
     def test_sets_log_namer_to_a_UniqueFileAllocator(self):
         repo = DateDirRepo(self.useFixture(TempDir()).path, 'T')

=== modified file 'setup.py'
--- setup.py	2011-11-16 03:44:36 +0000
+++ setup.py	2011-11-17 00:02:23 +0000
@@ -22,7 +22,7 @@
 description = file(os.path.join(os.path.dirname(__file__), 'README'), 'rb').read()
 
 setup(name="oops_datedir_repo",
-      version="0.0.12",
+      version="0.0.13",
       description="OOPS disk serialisation and repository management.",
       long_description=description,
       maintainer="Launchpad Developers",