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

Proposed by Diogo Matsubara on 2010-10-08
Status: Merged
Approved by: Gary Poster on 2010-10-08
Approved revision: no longer in the source branch.
Merged at revision: 11700
Proposed branch: lp:~matsubara/launchpad/bug-628510-oops-permission
Merge into: lp:launchpad
Diff against target: 450 lines (+106/-45)
4 files modified
lib/canonical/launchpad/webapp/errorlog.py (+6/-0)
lib/canonical/launchpad/webapp/tests/test_errorlog.py (+53/-22)
lib/lp/services/log/tests/test_uniquefileallocator.py (+27/-9)
lib/lp/services/log/uniquefileallocator.py (+20/-14)
To merge this branch: bzr merge lp:~matsubara/launchpad/bug-628510-oops-permission
Reviewer Review Type Date Requested Status
Gary Poster (community) 2010-10-08 Approve on 2010-10-08
Review via email: mp+37991@code.launchpad.net

Commit message

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

Description of the change

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.

To post a comment you must log in.
Gary Poster (gary) wrote :

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) wrote :

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) wrote :

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) wrote :

A small note - cleanUp is better than tearDown.

Diogo Matsubara (matsubara) wrote :

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/canonical/launchpad/webapp/errorlog.py'
2--- lib/canonical/launchpad/webapp/errorlog.py 2010-09-22 09:57:56 +0000
3+++ lib/canonical/launchpad/webapp/errorlog.py 2010-10-13 18:46:21 +0000
4@@ -11,6 +11,8 @@
5 import datetime
6 from itertools import repeat
7 import logging
8+import os
9+import stat
10 import re
11 import rfc822
12 import types
13@@ -352,6 +354,10 @@
14 return
15 filename = entry._filename
16 entry.write(open(filename, 'wb'))
17+ # Set file permission to: rw-r--r--
18+ wanted_permission = (
19+ stat.S_IRUSR | stat.S_IWUSR | stat.S_IRGRP | stat.S_IROTH)
20+ os.chmod(filename, wanted_permission)
21 if request:
22 request.oopsid = entry.id
23 request.oops = entry
24
25=== modified file 'lib/canonical/launchpad/webapp/tests/test_errorlog.py'
26--- lib/canonical/launchpad/webapp/tests/test_errorlog.py 2010-08-20 20:31:18 +0000
27+++ lib/canonical/launchpad/webapp/tests/test_errorlog.py 2010-10-13 18:46:21 +0000
28@@ -10,6 +10,7 @@
29 import logging
30 import os
31 import shutil
32+import stat
33 import StringIO
34 import sys
35 import tempfile
36@@ -104,7 +105,7 @@
37 ('HTTP_REFERER', 'http://localhost:9000/'),
38 ('name=foo', 'hello\nworld')],
39 [(1, 5, 'store_a', 'SELECT 1'),
40- (5, 10,'store_b', 'SELECT\n2')], False)
41+ (5, 10, 'store_b', 'SELECT\n2')], False)
42 fp = StringIO.StringIO()
43 entry.write(fp)
44 self.assertEqual(fp.getvalue(), dedent("""\
45@@ -173,7 +174,6 @@
46 entry.db_statements[1],
47 (5, 10, 'store_b', 'SELECT 2'))
48
49-
50 def test_read_no_store_id(self):
51 """Test ErrorReport.read() for old logs with no store_id."""
52 fp = StringIO.StringIO(dedent("""\
53@@ -216,6 +216,7 @@
54
55
56 class TestErrorReportingUtility(testtools.TestCase):
57+
58 def setUp(self):
59 super(TestErrorReportingUtility, self).setUp()
60 # ErrorReportingUtility reads the global config to get the
61@@ -283,13 +284,29 @@
62 utility = ErrorReportingUtility()
63 now = datetime.datetime(2006, 04, 01, 00, 30, 00, tzinfo=UTC)
64
65+ # Set up default file creation mode to rwx------.
66+ umask_permission = stat.S_IRWXG | stat.S_IRWXO
67+ old_umask = os.umask(umask_permission)
68+
69 try:
70 raise ArbitraryException('xyz')
71 except ArbitraryException:
72 utility.raising(sys.exc_info(), now=now)
73
74- errorfile = os.path.join(utility.log_namer.output_dir(now), '01800.T1')
75+ errorfile = os.path.join(
76+ utility.log_namer.output_dir(now), '01800.T1')
77 self.assertTrue(os.path.exists(errorfile))
78+
79+ # Check errorfile is set with the correct permission: rw-r--r--
80+ st = os.stat(errorfile)
81+ wanted_permission = (
82+ stat.S_IRUSR | stat.S_IWUSR | stat.S_IRGRP | stat.S_IROTH)
83+ # Get only the permission bits for this file.
84+ file_permission = stat.S_IMODE(st.st_mode)
85+ self.assertEqual(file_permission, wanted_permission)
86+ # Restore the umask to the original value.
87+ ignored = os.umask(old_umask)
88+
89 lines = open(errorfile, 'r').readlines()
90
91 # the header
92@@ -333,8 +350,7 @@
93 'name1': 'value3 \xa7',
94 'name2': 'value2',
95 u'\N{BLACK SQUARE}': u'value4',
96- }
97- )
98+ })
99 request.setInWSGIEnvironment('launchpad.pageid', 'IFoo:+foo-template')
100
101 try:
102@@ -342,7 +358,8 @@
103 except ArbitraryException:
104 utility.raising(sys.exc_info(), request, now=now)
105
106- errorfile = os.path.join(utility.log_namer.output_dir(now), '01800.T1')
107+ errorfile = os.path.join(
108+ utility.log_namer.output_dir(now), '01800.T1')
109 self.assertTrue(os.path.exists(errorfile))
110 lines = open(errorfile, 'r').readlines()
111
112@@ -404,7 +421,8 @@
113 raise ArbitraryException('xyz\nabc')
114 except ArbitraryException:
115 utility.raising(sys.exc_info(), request, now=now)
116- errorfile = os.path.join(utility.log_namer.output_dir(now), '01800.T1')
117+ errorfile = os.path.join(
118+ utility.log_namer.output_dir(now), '01800.T1')
119 self.assertTrue(os.path.exists(errorfile))
120 lines = open(errorfile, 'r').readlines()
121 self.assertEqual(lines[16], 'xmlrpc args=(1, 2)\n')
122@@ -459,7 +477,8 @@
123 ('name1', 'value3')], URL='https://launchpad.net/example')
124 utility.raising(sys.exc_info(), request, now=now)
125
126- errorfile = os.path.join(utility.log_namer.output_dir(now), '01800.T1')
127+ errorfile = os.path.join(
128+ utility.log_namer.output_dir(now), '01800.T1')
129 self.assertTrue(os.path.exists(errorfile))
130 lines = open(errorfile, 'r').readlines()
131
132@@ -501,6 +520,7 @@
133 now = datetime.datetime(2006, 01, 01, 00, 30, 00, tzinfo=UTC)
134
135 class UnprintableException(Exception):
136+
137 def __str__(self):
138 raise RuntimeError('arrgh')
139 __repr__ = __str__
140@@ -512,7 +532,8 @@
141 utility.raising(sys.exc_info(), now=now)
142 log.uninstall()
143
144- errorfile = os.path.join(utility.log_namer.output_dir(now), '01800.T1')
145+ errorfile = os.path.join(
146+ utility.log_namer.output_dir(now), '01800.T1')
147 self.assertTrue(os.path.exists(errorfile))
148 lines = open(errorfile, 'r').readlines()
149
150@@ -555,7 +576,8 @@
151 raise Unauthorized('xyz')
152 except Unauthorized:
153 utility.raising(sys.exc_info(), now=now)
154- errorfile = os.path.join(utility.log_namer.output_dir(now), '01800.T1')
155+ errorfile = os.path.join(
156+ utility.log_namer.output_dir(now), '01800.T1')
157 self.failUnless(os.path.exists(errorfile))
158
159 def test_raising_unauthorized_without_principal(self):
160@@ -568,7 +590,8 @@
161 raise Unauthorized('xyz')
162 except Unauthorized:
163 utility.raising(sys.exc_info(), request, now=now)
164- errorfile = os.path.join(utility.log_namer.output_dir(now), '01800.T1')
165+ errorfile = os.path.join(
166+ utility.log_namer.output_dir(now), '01800.T1')
167 self.failUnless(os.path.exists(errorfile))
168
169 def test_raising_unauthorized_with_unauthenticated_principal(self):
170@@ -581,7 +604,8 @@
171 raise Unauthorized('xyz')
172 except Unauthorized:
173 utility.raising(sys.exc_info(), request, now=now)
174- errorfile = os.path.join(utility.log_namer.output_dir(now), '01800.T1')
175+ errorfile = os.path.join(
176+ utility.log_namer.output_dir(now), '01800.T1')
177 self.failIf(os.path.exists(errorfile))
178
179 def test_raising_unauthorized_with_authenticated_principal(self):
180@@ -594,7 +618,8 @@
181 raise Unauthorized('xyz')
182 except Unauthorized:
183 utility.raising(sys.exc_info(), request, now=now)
184- errorfile = os.path.join(utility.log_namer.output_dir(now), '01800.T1')
185+ errorfile = os.path.join(
186+ utility.log_namer.output_dir(now), '01800.T1')
187 self.failUnless(os.path.exists(errorfile))
188
189 def test_raising_translation_unavailable(self):
190@@ -612,7 +637,8 @@
191 except TranslationUnavailable:
192 utility.raising(sys.exc_info(), now=now)
193
194- errorfile = os.path.join(utility.log_namer.output_dir(now), '01800.T1')
195+ errorfile = os.path.join(
196+ utility.log_namer.output_dir(now), '01800.T1')
197 self.assertFalse(os.path.exists(errorfile))
198
199 def test_raising_no_referrer_error(self):
200@@ -630,7 +656,8 @@
201 except NoReferrerError:
202 utility.raising(sys.exc_info(), now=now)
203
204- errorfile = os.path.join(utility.log_namer.output_dir(now), '01800.T1')
205+ errorfile = os.path.join(
206+ utility.log_namer.output_dir(now), '01800.T1')
207 self.assertFalse(os.path.exists(errorfile))
208
209 def test_raising_with_string_as_traceback(self):
210@@ -650,7 +677,8 @@
211 exc_tb = traceback.format_exc()
212
213 utility.raising((exc_type, exc_value, exc_tb), now=now)
214- errorfile = os.path.join(utility.log_namer.output_dir(now), '01800.T1')
215+ errorfile = os.path.join(
216+ utility.log_namer.output_dir(now), '01800.T1')
217
218 self.assertTrue(os.path.exists(errorfile))
219 lines = open(errorfile, 'r').readlines()
220@@ -688,7 +716,8 @@
221 except ArbitraryException:
222 utility.handling(sys.exc_info(), now=now)
223
224- errorfile = os.path.join(utility.log_namer.output_dir(now), '01800.T1')
225+ errorfile = os.path.join(
226+ utility.log_namer.output_dir(now), '01800.T1')
227 self.assertTrue(os.path.exists(errorfile))
228 lines = open(errorfile, 'r').readlines()
229
230@@ -721,18 +750,18 @@
231 def test_oopsMessage(self):
232 """oopsMessage pushes and pops the messages."""
233 utility = ErrorReportingUtility()
234- with utility.oopsMessage({'a':'b', 'c':'d'}):
235+ with utility.oopsMessage({'a': 'b', 'c': 'd'}):
236 self.assertEqual(
237- {0: {'a':'b', 'c':'d'}}, utility._oops_messages)
238+ {0: {'a': 'b', 'c': 'd'}}, utility._oops_messages)
239 # An additional message doesn't supplant the original message.
240 with utility.oopsMessage(dict(e='f', a='z', c='d')):
241 self.assertEqual({
242- 0: {'a':'b', 'c':'d'},
243+ 0: {'a': 'b', 'c': 'd'},
244 1: {'a': 'z', 'e': 'f', 'c': 'd'},
245 }, utility._oops_messages)
246 # Messages are removed when out of context.
247 self.assertEqual(
248- {0: {'a':'b', 'c':'d'}},
249+ {0: {'a': 'b', 'c': 'd'}},
250 utility._oops_messages)
251
252 def test__makeErrorReport_includes_oops_messages(self):
253@@ -786,6 +815,7 @@
254
255
256 class TestRequestWithPrincipal(TestRequest):
257+
258 def setInWSGIEnvironment(self, key, value):
259 self._orig_env[key] = value
260
261@@ -828,7 +858,8 @@
262 self.error_utility.log_namer._output_root = tempfile.mkdtemp()
263 self.logger.addHandler(
264 OopsLoggingHandler(error_utility=self.error_utility))
265- self.addCleanup(remove_tree, self.error_utility.log_namer._output_root)
266+ self.addCleanup(
267+ remove_tree, self.error_utility.log_namer._output_root)
268
269 def test_exception_records_oops(self):
270 # When OopsLoggingHandler is a handler for a logger, any exceptions
271
272=== modified file 'lib/lp/services/log/tests/test_uniquefileallocator.py'
273--- lib/lp/services/log/tests/test_uniquefileallocator.py 2010-07-05 19:25:14 +0000
274+++ lib/lp/services/log/tests/test_uniquefileallocator.py 2010-10-13 18:46:21 +0000
275@@ -9,6 +9,7 @@
276 import datetime
277 import os
278 import shutil
279+import stat
280 import tempfile
281 import unittest
282
283@@ -58,12 +59,12 @@
284 # when combined with configuration changes causing disk scans. That
285 # would also permit using a completely stubbed out file system,
286 # reducing IO in tests that use UniqueFileAllocator (such as all the
287- # pagetests in Launchpad. At that point an interface to obtain a
288+ # pagetests in Launchpad. At that point an interface to obtain a
289 # factory of UniqueFileAllocator's would be useful to parameterise the
290 # entire test suite.
291 namer = UniqueFileAllocator(self._tempdir, 'OOPS', 'T')
292 # first name of the day
293- self.assertUniqueFileAllocator(namer,
294+ self.assertUniqueFileAllocator(namer,
295 datetime.datetime(2006, 04, 01, 00, 30, 00, tzinfo=UTC),
296 'OOPS-91T1', 1, '2006-04-01/01800.T1', '2006-04-01')
297 # second name of the day
298@@ -73,7 +74,7 @@
299
300 # first name of the following day sets a new dir and the id starts
301 # over.
302- self.assertUniqueFileAllocator(namer,
303+ self.assertUniqueFileAllocator(namer,
304 datetime.datetime(2006, 04, 02, 00, 30, 00, tzinfo=UTC),
305 'OOPS-92T1', 1, '2006-04-02/01800.T1', '2006-04-02')
306
307@@ -95,16 +96,16 @@
308 self.assertRaises(ValueError, namer.newId, now)
309
310 def test_changeErrorDir(self):
311- """Test changing the log output dur."""
312+ """Test changing the log output dir."""
313 namer = UniqueFileAllocator(self._tempdir, 'OOPS', 'T')
314
315 # First an id in the original error directory.
316- self.assertUniqueFileAllocator(namer,
317+ self.assertUniqueFileAllocator(namer,
318 datetime.datetime(2006, 04, 01, 00, 30, 00, tzinfo=UTC),
319 'OOPS-91T1', 1, '2006-04-01/01800.T1', '2006-04-01')
320
321- # UniqueFileAllocator uses the _output_root attribute to get the current output
322- # directory.
323+ # UniqueFileAllocator uses the _output_root attribute to get the
324+ # current output directory.
325 new_output_dir = tempfile.mkdtemp()
326 self.addCleanup(shutil.rmtree, new_output_dir, ignore_errors=True)
327 namer._output_root = new_output_dir
328@@ -113,8 +114,8 @@
329 now = datetime.datetime(2006, 04, 01, 12, 00, 00, tzinfo=UTC)
330 log_id, filename = namer.newId(now)
331
332- # Since it's a new directory, with no previous logs, the id is 1 again,
333- # rather than 2.
334+ # Since it's a new directory, with no previous logs, the id is 1
335+ # again, rather than 2.
336 self.assertEqual(log_id, 'OOPS-91T1')
337 self.assertEqual(namer._last_serial, 1)
338 self.assertEqual(namer._last_output_dir,
339@@ -134,6 +135,23 @@
340 # The namer should figure out the right highest serial.
341 self.assertEqual(namer._findHighestSerial(output_dir), 10)
342
343+ def test_output_dir_permission(self):
344+ # Set up default dir creation mode to rwx------.
345+ umask_permission = stat.S_IRWXG | stat.S_IRWXO
346+ old_umask = os.umask(umask_permission)
347+ namer = UniqueFileAllocator(self._tempdir, "OOPS", "T")
348+ output_dir = namer.output_dir()
349+ st = os.stat(output_dir)
350+ # Permission we want here is: rwxr-xr-x
351+ wanted_permission = (
352+ stat.S_IRWXU | stat.S_IRGRP | stat.S_IXGRP | stat.S_IROTH |
353+ stat.S_IXOTH)
354+ # Get only the permission bits for this directory.
355+ dir_permission = stat.S_IMODE(st.st_mode)
356+ self.assertEqual(dir_permission, wanted_permission)
357+ # Restore the umask to the original value.
358+ ignored = os.umask(old_umask)
359+
360
361 def test_suite():
362 return unittest.TestLoader().loadTestsFromName(__name__)
363
364=== modified file 'lib/lp/services/log/uniquefileallocator.py'
365--- lib/lp/services/log/uniquefileallocator.py 2010-08-20 20:31:18 +0000
366+++ lib/lp/services/log/uniquefileallocator.py 2010-10-13 18:46:21 +0000
367@@ -12,8 +12,8 @@
368 import datetime
369 import errno
370 import os.path
371+import stat
372 import threading
373-import time
374
375 import pytz
376
377@@ -27,7 +27,7 @@
378
379 class UniqueFileAllocator:
380 """Assign unique file names to logs being written from an app/script.
381-
382+
383 UniqueFileAllocator causes logs written from one process to be uniquely
384 named. It is not safe for use in multiple processes with the same output
385 root - each process must have a unique output root.
386@@ -90,8 +90,8 @@
387 if the logging application is restarted.
388
389 This method is not thread safe, and only intended to be called
390- from the constructor (but it is called from other places in integration
391- tests).
392+ from the constructor (but it is called from other places in
393+ integration tests).
394 """
395 return self._findHighestSerialFilename(directory)[0]
396
397@@ -105,7 +105,8 @@
398 output_dir = self.output_dir(time)
399 second_in_day = time.hour * 3600 + time.minute * 60 + time.second
400 return os.path.join(
401- output_dir, '%05d.%s%s' % (second_in_day, log_subtype, log_serial))
402+ output_dir, '%05d.%s%s' % (
403+ second_in_day, log_subtype, log_serial))
404
405 def get_log_infix(self):
406 """Return the current log infix to use in ids and file names."""
407@@ -114,8 +115,8 @@
408 def newId(self, now=None):
409 """Returns an (id, filename) pair for use by the caller.
410
411- The ID is composed of a short string to identify the Launchpad instance
412- followed by an ID that is unique for the day.
413+ The ID is composed of a short string to identify the Launchpad
414+ instance followed by an ID that is unique for the day.
415
416 The filename is composed of the zero padded second in the day
417 followed by the ID. This ensures that reports are in date order when
418@@ -163,12 +164,17 @@
419 except OSError, e:
420 if e.errno != errno.EEXIST:
421 raise
422+ # Make sure the directory permission is set to: rwxr-xr-x
423+ permission = (
424+ stat.S_IRWXU | stat.S_IRGRP | stat.S_IXGRP |
425+ stat.S_IROTH | stat.S_IXOTH)
426+ os.chmod(result, permission)
427 # TODO: Note that only one process can do this safely: its not
428- # cross-process safe, and also not entirely threadsafe: another
429- # thread that has a new log and hasn't written it could then
430- # use that serial number. We should either make it really safe,
431- # or remove the contention entirely and log uniquely per thread
432- # of execution.
433+ # cross-process safe, and also not entirely threadsafe:
434+ # another # thread that has a new log and hasn't written it
435+ # could then use that serial number. We should either make it
436+ # really safe, or remove the contention entirely and log
437+ # uniquely per thread of execution.
438 self._last_serial = self._findHighestSerial(result)
439 finally:
440 self._lock.release()
441@@ -178,7 +184,7 @@
442 """Append a string to the log subtype in filenames and log ids.
443
444 :param token: a string to append..
445- Scripts that run multiple processes can use this to create a unique
446- identifier for each process.
447+ Scripts that run multiple processes can use this to create a
448+ unique identifier for each process.
449 """
450 self._log_token = token