launchpad-reviewers team mailing list archive

[Diogo Matsubara <matsubara@xxxxxxxxxxxxx>]

Diogo Matsubara has proposed merging lp:~matsubara/launchpad/bug-628510-oops-permission into lp:launchpad/devel.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #628510 OSError at /oops.py/  when using lp-oops
  https://bugs.launchpad.net/bugs/628510


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.
-- 
https://code.launchpad.net/~matsubara/launchpad/bug-628510-oops-permission/+merge/37991
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~matsubara/launchpad/bug-628510-oops-permission into lp:launchpad/devel.

=== 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-08 16:43:43 +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-08 16:43:43 +0000
@@ -10,6 +10,7 @@
 import logging
 import os
 import shutil
+import stat
 import StringIO
 import sys
 import tempfile
@@ -290,6 +291,15 @@
 
         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)
+        # Remove the file bits since we know this is a file.
+        file_permission = st.st_mode - stat.S_IFREG
+        self.assertEqual(file_permission, wanted_permission)
+
         lines = open(errorfile, 'r').readlines()
 
         # the header

=== 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-08 16:43:43 +0000
@@ -9,6 +9,7 @@
 import datetime
 import os
 import shutil
+import stat
 import tempfile
 import unittest
 
@@ -95,7 +96,7 @@
         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.
@@ -134,6 +135,17 @@
         # The namer should figure out the right highest serial.
         self.assertEqual(namer._findHighestSerial(output_dir), 10)
 
+    def test_output_dir_permission(self):
+        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)
+        # Remove the directory bits since we know it's a directory
+        dir_permission = st.st_mode - stat.S_IFDIR
+        self.assertEqual(dir_permission, wanted_permission)
+
 
 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-08 16:43:43 +0000
@@ -12,6 +12,7 @@
 import datetime
 import errno
 import os.path
+import stat
 import threading
 import time
 
@@ -159,7 +160,10 @@
                 self._last_output_dir = result
                 # make sure the directory exists
                 try:
-                    os.makedirs(result)
+                    # 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.makedirs(result, permission)
                 except OSError, e:
                     if e.errno != errno.EEXIST:
                         raise