Merge lp:~matsubara/launchpad/bug-628510-oops-permission into lp:launchpad

[Diogo Matsubara]

Fixes bug 628510 by overriding the default permission value for the oops file and oops dir when those are created.

This branch fixes bug 628510 by overriding the default permission value for the oops file and oops dir when those are created.

To test:

bin/test -t test_uniquefileallocator
bin/test -t test_errorlog

Caveats:

- Documentation says to use bit OR (|) operator. I got the same result by summing the values for the stat.* constants. Same thing when I subtracted the file and dir constant (stat.S_IFREG, stat.S_IFDIR respectively) from the returned st_mode.
- I couldn't reproduce the problem described in the bug report locally, so when I wrote the test, it still passed because the default permission is exactly the same as the one I'm overriding in the fix. One thing I did to check if the overriding code worked was to make it set a different permission than the default and running the test. Tests broke because the overriding code did set the permission to a different value than the test was expecting.

[Gary Poster (gary)]

Thank you, Diogo.

- The bit OR and AND operators are the proper ways of doing this, rather than addition and subtraction. Since the values you are manipulating are single bits, sure, addition and subtraction would work, but why not do things properly? Practically, I don't know if using addition and subtraction would ever actually bite you--that is, if these values are ever anything other than single bits--but following the docs seems like an obvious win.

- You should write a test that fails initially. Use os.umask as test set up to make the permissions different. Reinstate the umask in the test's tear down.

[Diogo Matsubara (matsubara)]

Hi Gary,

thanks for the review. I addressed your review comments. As we spoke on IRC, the os.umask was spot on, since it actually caught the fact that the os.makedirs() mode settings was being ignored. I added those and changed the permission setting to use bit-wise operators rather than addition and subtraction.

Let me know if this is ready to land. I'll send to ec2 meanwhile.

[Gary Poster (gary)]

merge-conditional

Great, thank you Diogo.

1) Please run make lint and make sure that you respond to its concerns (though note that it has one error I'm aware of: tuples with a single item such as (1,) should be rendered without a trailing space after the comma, so "(1,)" is preferred over "(1, )").

2) You have indentation that's a bit non-standard in uniquefileallocator.py and test_uniquefileallocator.py. Here's an example.

        wanted_permission = (stat.S_IRWXU | stat.S_IRGRP | stat.S_IXGRP |
            stat.S_IROTH | stat.S_IXOTH)

I'd prefer to see that as follows:

        wanted_permission = (stat.S_IRWXU | stat.S_IRGRP | stat.S_IXGRP |
                             stat.S_IROTH | stat.S_IXOTH)

This is another acceptable alternate:

        wanted_permission = (
            stat.S_IRWXU | stat.S_IRGRP | stat.S_IXGRP | stat.S_IROTH |
            stat.S_IXOTH)

That's it. Thank you!

Gary

review: Approve

[Robert Collins (lifeless)]

A small note - cleanUp is better than tearDown.

[Diogo Matsubara (matsubara)]

On Fri, Oct 8, 2010 at 7:07 PM, Gary Poster <email address hidden> wrote:
> Review: Approve
> merge-conditional
>
> Great, thank you Diogo.
>
> 1) Please run make lint and make sure that you respond to its concerns (though note that it has one error I'm aware of: tuples with a single item such as (1,) should be rendered without a trailing space after the comma, so "(1,)" is preferred over "(1, )").

Fixed make lint warnings.

>
> 2) You have indentation that's a bit non-standard in uniquefileallocator.py and test_uniquefileallocator.py.  Here's an example.
>
>        wanted_permission = (stat.S_IRWXU | stat.S_IRGRP | stat.S_IXGRP |
>            stat.S_IROTH | stat.S_IXOTH)
>
> I'd prefer to see that as follows:
>
>        wanted_permission = (stat.S_IRWXU | stat.S_IRGRP | stat.S_IXGRP |
>                             stat.S_IROTH | stat.S_IXOTH)
>
> This is another acceptable alternate:
>
>        wanted_permission = (
>            stat.S_IRWXU | stat.S_IRGRP | stat.S_IXGRP | stat.S_IROTH |
>            stat.S_IXOTH)

Fixed the identation.

>
> That's it.  Thank you!
>

Thanks!
--
Diogo M. Matsubara

=== modified file 'lib/canonical/launchpad/webapp/errorlog.py'
--- lib/canonical/launchpad/webapp/errorlog.py	2010-09-22 09:57:56 +0000
+++ lib/canonical/launchpad/webapp/errorlog.py	2010-10-13 18:46:21 +0000
@@ -11,6 +11,8 @@
 import datetime
 from itertools import repeat
 import logging
+import os
+import stat
 import re
 import rfc822
 import types
@@ -352,6 +354,10 @@
             return
         filename = entry._filename
         entry.write(open(filename, 'wb'))
+        # Set file permission to: rw-r--r--
+        wanted_permission = (
+            stat.S_IRUSR | stat.S_IWUSR | stat.S_IRGRP | stat.S_IROTH)
+        os.chmod(filename, wanted_permission)
         if request:
             request.oopsid = entry.id
             request.oops = entry

=== modified file 'lib/canonical/launchpad/webapp/tests/test_errorlog.py'
--- lib/canonical/launchpad/webapp/tests/test_errorlog.py	2010-08-20 20:31:18 +0000
+++ lib/canonical/launchpad/webapp/tests/test_errorlog.py	2010-10-13 18:46:21 +0000
@@ -10,6 +10,7 @@
 import logging
 import os
 import shutil
+import stat
 import StringIO
 import sys
 import tempfile
@@ -104,7 +105,7 @@
                              ('HTTP_REFERER', 'http://localhost:9000/'),
                              ('name=foo', 'hello\nworld')],
                             [(1, 5, 'store_a', 'SELECT 1'),
-                             (5, 10,'store_b', 'SELECT\n2')], False)
+                             (5, 10, 'store_b', 'SELECT\n2')], False)
         fp = StringIO.StringIO()
         entry.write(fp)
         self.assertEqual(fp.getvalue(), dedent("""\
@@ -173,7 +174,6 @@
             entry.db_statements[1],
             (5, 10, 'store_b', 'SELECT 2'))
 
-
     def test_read_no_store_id(self):
         """Test ErrorReport.read() for old logs with no store_id."""
         fp = StringIO.StringIO(dedent("""\
@@ -216,6 +216,7 @@
 
 
 class TestErrorReportingUtility(testtools.TestCase):
+
     def setUp(self):
         super(TestErrorReportingUtility, self).setUp()
         # ErrorReportingUtility reads the global config to get the
@@ -283,13 +284,29 @@
         utility = ErrorReportingUtility()
         now = datetime.datetime(2006, 04, 01, 00, 30, 00, tzinfo=UTC)
 
+        # Set up default file creation mode to rwx------.
+        umask_permission = stat.S_IRWXG | stat.S_IRWXO
+        old_umask = os.umask(umask_permission)
+
         try:
             raise ArbitraryException('xyz')
         except ArbitraryException:
             utility.raising(sys.exc_info(), now=now)
 
-        errorfile = os.path.join(utility.log_namer.output_dir(now), '01800.T1')
+        errorfile = os.path.join(
+            utility.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)
+        # Restore the umask to the original value.
+        ignored = os.umask(old_umask)
+
         lines = open(errorfile, 'r').readlines()
 
         # the header
@@ -333,8 +350,7 @@
                     'name1': 'value3 \xa7',
                     'name2': 'value2',
                     u'\N{BLACK SQUARE}': u'value4',
-                    }
-                )
+                    })
         request.setInWSGIEnvironment('launchpad.pageid', 'IFoo:+foo-template')
 
         try:
@@ -342,7 +358,8 @@
         except ArbitraryException:
             utility.raising(sys.exc_info(), request, now=now)
 
-        errorfile = os.path.join(utility.log_namer.output_dir(now), '01800.T1')
+        errorfile = os.path.join(
+            utility.log_namer.output_dir(now), '01800.T1')
         self.assertTrue(os.path.exists(errorfile))
         lines = open(errorfile, 'r').readlines()
 
@@ -404,7 +421,8 @@
             raise ArbitraryException('xyz\nabc')
         except ArbitraryException:
             utility.raising(sys.exc_info(), request, now=now)
-        errorfile = os.path.join(utility.log_namer.output_dir(now), '01800.T1')
+        errorfile = os.path.join(
+            utility.log_namer.output_dir(now), '01800.T1')
         self.assertTrue(os.path.exists(errorfile))
         lines = open(errorfile, 'r').readlines()
         self.assertEqual(lines[16], 'xmlrpc args=(1, 2)\n')
@@ -459,7 +477,8 @@
                 ('name1', 'value3')], URL='https://launchpad.net/example')
             utility.raising(sys.exc_info(), request, now=now)
 
-        errorfile = os.path.join(utility.log_namer.output_dir(now), '01800.T1')
+        errorfile = os.path.join(
+            utility.log_namer.output_dir(now), '01800.T1')
         self.assertTrue(os.path.exists(errorfile))
         lines = open(errorfile, 'r').readlines()
 
@@ -501,6 +520,7 @@
         now = datetime.datetime(2006, 01, 01, 00, 30, 00, tzinfo=UTC)
 
         class UnprintableException(Exception):
+
             def __str__(self):
                 raise RuntimeError('arrgh')
             __repr__ = __str__
@@ -512,7 +532,8 @@
             utility.raising(sys.exc_info(), now=now)
         log.uninstall()
 
-        errorfile = os.path.join(utility.log_namer.output_dir(now), '01800.T1')
+        errorfile = os.path.join(
+            utility.log_namer.output_dir(now), '01800.T1')
         self.assertTrue(os.path.exists(errorfile))
         lines = open(errorfile, 'r').readlines()
 
@@ -555,7 +576,8 @@
             raise Unauthorized('xyz')
         except Unauthorized:
             utility.raising(sys.exc_info(), now=now)
-        errorfile = os.path.join(utility.log_namer.output_dir(now), '01800.T1')
+        errorfile = os.path.join(
+            utility.log_namer.output_dir(now), '01800.T1')
         self.failUnless(os.path.exists(errorfile))
 
     def test_raising_unauthorized_without_principal(self):
@@ -568,7 +590,8 @@
             raise Unauthorized('xyz')
         except Unauthorized:
             utility.raising(sys.exc_info(), request, now=now)
-        errorfile = os.path.join(utility.log_namer.output_dir(now), '01800.T1')
+        errorfile = os.path.join(
+            utility.log_namer.output_dir(now), '01800.T1')
         self.failUnless(os.path.exists(errorfile))
 
     def test_raising_unauthorized_with_unauthenticated_principal(self):
@@ -581,7 +604,8 @@
             raise Unauthorized('xyz')
         except Unauthorized:
             utility.raising(sys.exc_info(), request, now=now)
-        errorfile = os.path.join(utility.log_namer.output_dir(now), '01800.T1')
+        errorfile = os.path.join(
+            utility.log_namer.output_dir(now), '01800.T1')
         self.failIf(os.path.exists(errorfile))
 
     def test_raising_unauthorized_with_authenticated_principal(self):
@@ -594,7 +618,8 @@
             raise Unauthorized('xyz')
         except Unauthorized:
             utility.raising(sys.exc_info(), request, now=now)
-        errorfile = os.path.join(utility.log_namer.output_dir(now), '01800.T1')
+        errorfile = os.path.join(
+            utility.log_namer.output_dir(now), '01800.T1')
         self.failUnless(os.path.exists(errorfile))
 
     def test_raising_translation_unavailable(self):
@@ -612,7 +637,8 @@
         except TranslationUnavailable:
             utility.raising(sys.exc_info(), now=now)
 
-        errorfile = os.path.join(utility.log_namer.output_dir(now), '01800.T1')
+        errorfile = os.path.join(
+            utility.log_namer.output_dir(now), '01800.T1')
         self.assertFalse(os.path.exists(errorfile))
 
     def test_raising_no_referrer_error(self):
@@ -630,7 +656,8 @@
         except NoReferrerError:
             utility.raising(sys.exc_info(), now=now)
 
-        errorfile = os.path.join(utility.log_namer.output_dir(now), '01800.T1')
+        errorfile = os.path.join(
+            utility.log_namer.output_dir(now), '01800.T1')
         self.assertFalse(os.path.exists(errorfile))
 
     def test_raising_with_string_as_traceback(self):
@@ -650,7 +677,8 @@
             exc_tb = traceback.format_exc()
 
         utility.raising((exc_type, exc_value, exc_tb), now=now)
-        errorfile = os.path.join(utility.log_namer.output_dir(now), '01800.T1')
+        errorfile = os.path.join(
+            utility.log_namer.output_dir(now), '01800.T1')
 
         self.assertTrue(os.path.exists(errorfile))
         lines = open(errorfile, 'r').readlines()
@@ -688,7 +716,8 @@
         except ArbitraryException:
             utility.handling(sys.exc_info(), now=now)
 
-        errorfile = os.path.join(utility.log_namer.output_dir(now), '01800.T1')
+        errorfile = os.path.join(
+            utility.log_namer.output_dir(now), '01800.T1')
         self.assertTrue(os.path.exists(errorfile))
         lines = open(errorfile, 'r').readlines()
 
@@ -721,18 +750,18 @@
     def test_oopsMessage(self):
         """oopsMessage pushes and pops the messages."""
         utility = ErrorReportingUtility()
-        with utility.oopsMessage({'a':'b', 'c':'d'}):
+        with utility.oopsMessage({'a': 'b', 'c': 'd'}):
             self.assertEqual(
-                {0: {'a':'b', 'c':'d'}}, utility._oops_messages)
+                {0: {'a': 'b', 'c': 'd'}}, utility._oops_messages)
             # An additional message doesn't supplant the original message.
             with utility.oopsMessage(dict(e='f', a='z', c='d')):
                 self.assertEqual({
-                    0: {'a':'b', 'c':'d'},
+                    0: {'a': 'b', 'c': 'd'},
                     1: {'a': 'z', 'e': 'f', 'c': 'd'},
                     }, utility._oops_messages)
             # Messages are removed when out of context.
             self.assertEqual(
-                {0: {'a':'b', 'c':'d'}},
+                {0: {'a': 'b', 'c': 'd'}},
                 utility._oops_messages)
 
     def test__makeErrorReport_includes_oops_messages(self):
@@ -786,6 +815,7 @@
 
 
 class TestRequestWithPrincipal(TestRequest):
+
     def setInWSGIEnvironment(self, key, value):
         self._orig_env[key] = value
 
@@ -828,7 +858,8 @@
         self.error_utility.log_namer._output_root = tempfile.mkdtemp()
         self.logger.addHandler(
             OopsLoggingHandler(error_utility=self.error_utility))
-        self.addCleanup(remove_tree, self.error_utility.log_namer._output_root)
+        self.addCleanup(
+            remove_tree, self.error_utility.log_namer._output_root)
 
     def test_exception_records_oops(self):
         # When OopsLoggingHandler is a handler for a logger, any exceptions

=== modified file 'lib/lp/services/log/tests/test_uniquefileallocator.py'
--- lib/lp/services/log/tests/test_uniquefileallocator.py	2010-07-05 19:25:14 +0000
+++ lib/lp/services/log/tests/test_uniquefileallocator.py	2010-10-13 18:46:21 +0000
@@ -9,6 +9,7 @@
 import datetime
 import os
 import shutil
+import stat
 import tempfile
 import unittest
 
@@ -58,12 +59,12 @@
         # when combined with configuration changes causing disk scans. That
         # would also permit using a completely stubbed out file system,
         # reducing IO in tests that use UniqueFileAllocator (such as all the
-        # pagetests in Launchpad. At that point an interface to obtain a 
+        # pagetests in Launchpad. At that point an interface to obtain a
         # factory of UniqueFileAllocator's would be useful to parameterise the
         # entire test suite.
         namer = UniqueFileAllocator(self._tempdir, 'OOPS', 'T')
         # first name of the day
-        self.assertUniqueFileAllocator(namer, 
+        self.assertUniqueFileAllocator(namer,
             datetime.datetime(2006, 04, 01, 00, 30, 00, tzinfo=UTC),
             'OOPS-91T1', 1, '2006-04-01/01800.T1', '2006-04-01')
         # second name of the day
@@ -73,7 +74,7 @@
 
         # first name of the following day sets a new dir and the id starts
         # over.
-        self.assertUniqueFileAllocator(namer, 
+        self.assertUniqueFileAllocator(namer,
             datetime.datetime(2006, 04, 02, 00, 30, 00, tzinfo=UTC),
             'OOPS-92T1', 1, '2006-04-02/01800.T1', '2006-04-02')
 
@@ -95,16 +96,16 @@
         self.assertRaises(ValueError, namer.newId, now)
 
     def test_changeErrorDir(self):
-        """Test changing the log output dur."""
+        """Test changing the log output dir."""
         namer = UniqueFileAllocator(self._tempdir, 'OOPS', 'T')
 
         # First an id in the original error directory.
-        self.assertUniqueFileAllocator(namer, 
+        self.assertUniqueFileAllocator(namer,
             datetime.datetime(2006, 04, 01, 00, 30, 00, tzinfo=UTC),
             'OOPS-91T1', 1, '2006-04-01/01800.T1', '2006-04-01')
 
-        # UniqueFileAllocator uses the _output_root attribute to get the current output
-        # directory.
+        # UniqueFileAllocator uses the _output_root attribute to get the
+        # current output directory.
         new_output_dir = tempfile.mkdtemp()
         self.addCleanup(shutil.rmtree, new_output_dir, ignore_errors=True)
         namer._output_root = new_output_dir
@@ -113,8 +114,8 @@
         now = datetime.datetime(2006, 04, 01, 12, 00, 00, tzinfo=UTC)
         log_id, filename = namer.newId(now)
 
-        # Since it's a new directory, with no previous logs, the id is 1 again,
-        # rather than 2.
+        # Since it's a new directory, with no previous logs, the id is 1
+        # again, rather than 2.
         self.assertEqual(log_id, 'OOPS-91T1')
         self.assertEqual(namer._last_serial, 1)
         self.assertEqual(namer._last_output_dir,
@@ -134,6 +135,23 @@
         # The namer should figure out the right highest serial.
         self.assertEqual(namer._findHighestSerial(output_dir), 10)
 
+    def test_output_dir_permission(self):
+        # Set up default dir creation mode to rwx------.
+        umask_permission = stat.S_IRWXG | stat.S_IRWXO
+        old_umask = os.umask(umask_permission)
+        namer = UniqueFileAllocator(self._tempdir, "OOPS", "T")
+        output_dir = namer.output_dir()
+        st = os.stat(output_dir)
+        # Permission we want here is: rwxr-xr-x
+        wanted_permission = (
+            stat.S_IRWXU | stat.S_IRGRP | stat.S_IXGRP | stat.S_IROTH |
+            stat.S_IXOTH)
+        # Get only the permission bits for this directory.
+        dir_permission = stat.S_IMODE(st.st_mode)
+        self.assertEqual(dir_permission, wanted_permission)
+        # Restore the umask to the original value.
+        ignored = os.umask(old_umask)
+
 
 def test_suite():
     return unittest.TestLoader().loadTestsFromName(__name__)

=== modified file 'lib/lp/services/log/uniquefileallocator.py'
--- lib/lp/services/log/uniquefileallocator.py	2010-08-20 20:31:18 +0000
+++ lib/lp/services/log/uniquefileallocator.py	2010-10-13 18:46:21 +0000
@@ -12,8 +12,8 @@
 import datetime
 import errno
 import os.path
+import stat
 import threading
-import time
 
 import pytz
 
@@ -27,7 +27,7 @@
 
 class UniqueFileAllocator:
     """Assign unique file names to logs being written from an app/script.
-    
+
     UniqueFileAllocator causes logs written from one process to be uniquely
     named. It is not safe for use in multiple processes with the same output
     root - each process must have a unique output root.
@@ -90,8 +90,8 @@
         if the logging application is restarted.
 
         This method is not thread safe, and only intended to be called
-        from the constructor (but it is called from other places in integration
-        tests).
+        from the constructor (but it is called from other places in
+        integration tests).
         """
         return self._findHighestSerialFilename(directory)[0]
 
@@ -105,7 +105,8 @@
         output_dir = self.output_dir(time)
         second_in_day = time.hour * 3600 + time.minute * 60 + time.second
         return os.path.join(
-            output_dir, '%05d.%s%s' % (second_in_day, log_subtype, log_serial))
+            output_dir, '%05d.%s%s' % (
+            second_in_day, log_subtype, log_serial))
 
     def get_log_infix(self):
         """Return the current log infix to use in ids and file names."""
@@ -114,8 +115,8 @@
     def newId(self, now=None):
         """Returns an (id, filename) pair for use by the caller.
 
-        The ID is composed of a short string to identify the Launchpad instance
-        followed by an ID that is unique for the day.
+        The ID is composed of a short string to identify the Launchpad
+        instance followed by an ID that is unique for the day.
 
         The filename is composed of the zero padded second in the day
         followed by the ID.  This ensures that reports are in date order when
@@ -163,12 +164,17 @@
                 except OSError, e:
                     if e.errno != errno.EEXIST:
                         raise
+                # Make sure the directory permission is set to: rwxr-xr-x
+                permission = (
+                    stat.S_IRWXU | stat.S_IRGRP | stat.S_IXGRP |
+                    stat.S_IROTH | stat.S_IXOTH)
+                os.chmod(result, permission)
                 # TODO: Note that only one process can do this safely: its not
-                # cross-process safe, and also not entirely threadsafe: another
-                # thread that has a new log and hasn't written it could then
-                # use that serial number. We should either make it really safe,
-                # or remove the contention entirely and log uniquely per thread
-                # of execution.
+                # cross-process safe, and also not entirely threadsafe:
+                # another # thread that has a new log and hasn't written it
+                # could then use that serial number. We should either make it
+                # really safe, or remove the contention entirely and log
+                # uniquely per thread of execution.
                 self._last_serial = self._findHighestSerial(result)
             finally:
                 self._lock.release()
@@ -178,7 +184,7 @@
         """Append a string to the log subtype in filenames and log ids.
 
         :param token: a string to append..
-            Scripts that run multiple processes can use this to create a unique
-            identifier for each process.
+            Scripts that run multiple processes can use this to create a
+            unique identifier for each process.
         """
         self._log_token = token