Launchpad itself

Merge lp:~adeuring/launchpad/bug-612779 into lp:launchpad/db-devel

  1. bug-612779
  2. Merge into db-devel
Proposed by Abel Deuring
Status: Merged
Approved by: Māris Fogels
Approved revision: no longer in the source branch.
Merged at revision: 9612
Proposed branch: lp:~adeuring/launchpad/bug-612779
Merge into: lp:launchpad/db-devel
Diff against target: 157 lines (+57/-9)
4 files modified
lib/canonical/launchpad/browser/librarian.py (+6/-5)
lib/lp/bugs/browser/bugattachment.py (+20/-1)
lib/lp/bugs/browser/configure.zcml (+5/-0)
lib/lp/bugs/browser/tests/test_bugattachment_file_access.py (+26/-3)
To merge this branch: bzr merge lp:~adeuring/launchpad/bug-612779
Reviewer Review Type Date Requested Status
Māris Fogels (community) code Approve
Review via email: mp+31637@code.launchpad.net

Description of the change

This branch fixes bug 612779:user controlled data will be exposed in the launchpad.net domains at the next release.

It ensures that HTTP responses for restircted LFA (ie.e, the content of bug attachments for private bugs) always have the HTTP header Content-Disposition: attachment.

The branch defines a new class SafeStreamOrRedirectLibraryFileAliasView which inherits from canonical.launchpad.browser.librarian.StreamOrRedirectLibraryFileAliasView. The latter class is by default used to serve the content of restricted Librarian files. The COntent-Disposition header is set in the overloaded method __call__().

Instances of StreamOrRedirectLibraryFileAliasView are created in canonical.launchpad.browser.librarian.FileNavigationMixin.traverse_files(). I changed this method so that the view class used in traverse_files() can now be easily changed in derived classes.

Finally, I set the view class used in BugAttachmentFileNavigation to SafeStreamOrRedirectLibraryFileAliasView.

test: ./bin/test -vvt test_bugattachment_file_access

no lint

To post a comment you must log in.
Revision history for this message
Māris Fogels (mars) wrote :

Hi Abel,

As noted on IRC, I think the class name StreamOrRedirectLibraryFileAliasView indicates a serious object design problem: it sounds like you should have three polymorphic classes instead (StreamAliasView, SafeStreamAliasView, and RedirectedAliasView). But also as noted on IRC, Robert is is going to overhaul the entire LibrarianFileAlias implementation.

The only other potential issue I see is in the test body: the test says it is "ensuring that restricted files ALWAYS have a disposition of 'attachement'", but this test only checks that the one file served has said disposition. Whether or not the incoming request had the disposition added by the code is a mystery. I would prefer a "before and after" comparison that shows the disposition being added.

With the above points considered, r=mars.

Maris

review: Approve (code)
Revision history for this message
Abel Deuring (adeuring) wrote :

Hi Māris,

Agreed, the word "always" is questionable in this context. But a comparison "before/after" is a test is a bit difficult, I think. OK, I could monkey patch the view_class attribute of BugAttachmentFileNavigation so that it is again StreamOrRedirectLibraryFileAliasView, but I think a better comment and an
assertIsInstance() is sufficient:

    def test_content_disposition_of_restricted_file(self):
        # The content of restricted Librarian files for bug attachments
        # is served by instances of SafeStreamOrRedirectLibraryFileAliasView
        # which set the content disposition header of the HTTP response for
        # to "attachment".
        lfa_with_parent = getMultiAdapter(
            (self.bugattachment.libraryfile, self.bugattachment),
            ILibraryFileAliasWithParent)
        lfa_with_parent.restricted = True
        self.bug.setPrivate(True, self.bug_owner)
        transaction.commit()
        request = LaunchpadTestRequest()
        request.setTraversalStack(['foo.txt'])
        navigation = BugAttachmentFileNavigation(self.bugattachment, request)
        view = navigation.publishTraverse(request, '+files')
        next_view, traversal_path = view.browserDefault(request)
        self.assertIsInstance(
            next_view, SafeStreamOrRedirectLibraryFileAliasView)
        next_view()
        self.assertEqual(
            'attachment', request.response.getHeader('Content-Disposition'))

Revision history for this message
Robert Collins (lifeless) wrote :

Abel, could you do a small follow on patch for me?

I'd like a little more prose/documentation in the docstrings for the
views so that developers can see when they should use one or the other
view class.

rs=lifeless for such a tweak.

Thanks,
Rob

Revision history for this message
Abel Deuring (adeuring) wrote :

On 04.08.2010 02:52, Robert Collins wrote:
> Abel, could you do a small follow on patch for me?
>
> I'd like a little more prose/documentation in the docstrings for the
> views so that developers can see when they should use one or the other
> view class.
>
> rs=lifeless for such a tweak.
>
> Thanks,
> Rob

Robert,

I'd like to postpone this to when PQM is closed ;) reasons:

- I need to land at least two more branches to fix the "private bug
  attachments" bug.
- ProxiedLFA, FileNavigationMixin, RedirectOrStream.*View could need a
  bit more testing (I've added some unit tests for private attachments
  that are in fact more like tests of FileNavigationMixin and
  StreamOrRedirectLibraryFileAliasView)
- StreamOrRedirectLibraryFileAliasView currently lives in
  lib.lp.bugs.browser.bugattachment, but it should move to
  canonical.launchpad.browser.librarian

Abel

Revision history for this message
Robert Collins (lifeless) wrote :

Sure, thats fine, its just polish we should do.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'lib/canonical/launchpad/browser/librarian.py'
--- lib/canonical/launchpad/browser/librarian.py 2010-03-26 19:19:49 +0000
+++ lib/canonical/launchpad/browser/librarian.py 2010-08-03 15:23:48 +0000
@@ -42,8 +42,8 @@
42 """View to handle redirection for downloading files by URL.42 """View to handle redirection for downloading files by URL.
4343
44 Rather than reference downloadable files via the obscure Librarian44 Rather than reference downloadable files via the obscure Librarian
45 URL, downloadable files can be referenced via the Product Release URL, e.g.45 URL, downloadable files can be referenced via the Product Release URL,
46 http://launchpad.net/firefox/1.0./1.0.0/+download/firefox-1.0.0.tgz.46 e.g. http://launchpad.net/firefox/1.0./1.0.0/+download/firefox-1.0.0.tgz.
47 """47 """
4848
49 __used_for__ = ILibraryFileAlias49 __used_for__ = ILibraryFileAlias
@@ -173,19 +173,20 @@
173 extended in order to allow traversing to multiple files potentially173 extended in order to allow traversing to multiple files potentially
174 with the same filename (product files or bug attachments).174 with the same filename (product files or bug attachments).
175 """175 """
176 view_class = StreamOrRedirectLibraryFileAliasView
177
176 @stepthrough('+files')178 @stepthrough('+files')
177 def traverse_files(self, filename):179 def traverse_files(self, filename):
178 """Traverse on filename in the archive domain."""180 """Traverse on filename in the archive domain."""
179 if not check_permission('launchpad.View', self.context):181 if not check_permission('launchpad.View', self.context):
180 raise Unauthorized()182 raise Unauthorized()
181 library_file = self.context.getFileByName(filename)183 library_file = self.context.getFileByName(filename)
182184
183 # Deleted library files result in NotFound-like error.185 # Deleted library files result in NotFound-like error.
184 if library_file.deleted:186 if library_file.deleted:
185 raise DeletedProxiedLibraryFileAlias(filename, self.context)187 raise DeletedProxiedLibraryFileAlias(filename, self.context)
186188
187 return StreamOrRedirectLibraryFileAliasView(189 return self.view_class(library_file, self.request)
188 library_file, self.request)
189190
190191
191class ProxiedLibraryFileAlias:192class ProxiedLibraryFileAlias:
192193
=== modified file 'lib/lp/bugs/browser/bugattachment.py'
--- lib/lp/bugs/browser/bugattachment.py 2010-07-27 14:12:47 +0000
+++ lib/lp/bugs/browser/bugattachment.py 2010-08-03 15:23:48 +0000
@@ -10,6 +10,7 @@
10 'BugAttachmentSetNavigation',10 'BugAttachmentSetNavigation',
11 'BugAttachmentEditView',11 'BugAttachmentEditView',
12 'BugAttachmentURL',12 'BugAttachmentURL',
13 'SafeStreamOrRedirectLibraryFileAliasView',
13 ]14 ]
1415
15from cStringIO import StringIO16from cStringIO import StringIO
@@ -18,7 +19,8 @@
18from zope.component import getUtility19from zope.component import getUtility
19from zope.contenttype import guess_content_type20from zope.contenttype import guess_content_type
2021
21from canonical.launchpad.browser.librarian import FileNavigationMixin22from canonical.launchpad.browser.librarian import (
23 FileNavigationMixin, StreamOrRedirectLibraryFileAliasView)
22from canonical.launchpad.webapp import (24from canonical.launchpad.webapp import (
23 canonical_url, custom_widget, GetitemNavigation, Navigation)25 canonical_url, custom_widget, GetitemNavigation, Navigation)
24from canonical.launchpad.interfaces.librarian import ILibraryFileAliasSet26from canonical.launchpad.interfaces.librarian import ILibraryFileAliasSet
@@ -227,7 +229,24 @@
227 return self.context.type == BugAttachmentType.PATCH229 return self.context.type == BugAttachmentType.PATCH
228230
229231
232class SafeStreamOrRedirectLibraryFileAliasView(
233 StreamOrRedirectLibraryFileAliasView):
234 """A view for Librarian files that sets the content disposion header."""
235
236 def __call__(self):
237 """Stream the content of the context `ILibraryFileAlias`.
238
239 Set the content disposition header to the safe value "attachment".
240 """
241 self.request.response.setHeader(
242 'Content-Disposition', 'attachment')
243 return super(
244 SafeStreamOrRedirectLibraryFileAliasView, self).__call__()
245
246
230class BugAttachmentFileNavigation(Navigation, FileNavigationMixin):247class BugAttachmentFileNavigation(Navigation, FileNavigationMixin):
231 """Traversal to +files/${filename}."""248 """Traversal to +files/${filename}."""
232249
233 usedfor = IBugAttachment250 usedfor = IBugAttachment
251
252 view_class = SafeStreamOrRedirectLibraryFileAliasView
234253
=== modified file 'lib/lp/bugs/browser/configure.zcml'
--- lib/lp/bugs/browser/configure.zcml 2010-07-27 14:12:47 +0000
+++ lib/lp/bugs/browser/configure.zcml 2010-08-03 15:23:48 +0000
@@ -1159,4 +1159,9 @@
1159 classes="1159 classes="
1160 BugWatchSetNavigation"/>1160 BugWatchSetNavigation"/>
1161 </facet>1161 </facet>
1162 <class class="lp.bugs.browser.bugattachment.SafeStreamOrRedirectLibraryFileAliasView">
1163 <allow attributes="__call__" />
1164 <allow interface="zope.publisher.interfaces.browser.IBrowserPublisher" />
1165 </class>
1166
1162</configure>1167</configure>
11631168
=== modified file 'lib/lp/bugs/browser/tests/test_bugattachment_file_access.py'
--- lib/lp/bugs/browser/tests/test_bugattachment_file_access.py 2010-08-02 10:52:39 +0000
+++ lib/lp/bugs/browser/tests/test_bugattachment_file_access.py 2010-08-03 15:23:48 +0000
@@ -19,7 +19,8 @@
19from canonical.launchpad.webapp.publisher import RedirectionView19from canonical.launchpad.webapp.publisher import RedirectionView
20from canonical.launchpad.webapp.servers import LaunchpadTestRequest20from canonical.launchpad.webapp.servers import LaunchpadTestRequest
2121
22from lp.bugs.browser.bugattachment import BugAttachmentFileNavigation22from lp.bugs.browser.bugattachment import (
23 BugAttachmentFileNavigation, SafeStreamOrRedirectLibraryFileAliasView)
23from lp.testing import login_person, TestCaseWithFactory24from lp.testing import login_person, TestCaseWithFactory
2425
2526
@@ -69,7 +70,7 @@
69 '^http://localhost:58000/\d+/foo.txt$', next_view.target)70 '^http://localhost:58000/\d+/foo.txt$', next_view.target)
70 self.assertIsNot(None, mo)71 self.assertIsNot(None, mo)
7172
72 def test_access_to_restircted_file(self):73 def test_access_to_restricted_file(self):
73 # Requests of restricted files are handled by ProxiedLibraryFileAlias.74 # Requests of restricted files are handled by ProxiedLibraryFileAlias.
74 lfa_with_parent = getMultiAdapter(75 lfa_with_parent = getMultiAdapter(
75 (self.bugattachment.libraryfile, self.bugattachment),76 (self.bugattachment.libraryfile, self.bugattachment),
@@ -87,7 +88,7 @@
87 file_.seek(0)88 file_.seek(0)
88 self.assertEqual('file content', file_.read())89 self.assertEqual('file content', file_.read())
8990
90 def test_access_to_restircted_file_unauthorized(self):91 def test_access_to_restricted_file_unauthorized(self):
91 # If a user cannot access the bug attachment itself, he can neither92 # If a user cannot access the bug attachment itself, he can neither
92 # access the restricted Librarian file.93 # access the restricted Librarian file.
93 lfa_with_parent = getMultiAdapter(94 lfa_with_parent = getMultiAdapter(
@@ -104,3 +105,25 @@
104 navigation = BugAttachmentFileNavigation(self.bugattachment, request)105 navigation = BugAttachmentFileNavigation(self.bugattachment, request)
105 self.assertRaises(106 self.assertRaises(
106 Unauthorized, navigation.publishTraverse, request, '+files')107 Unauthorized, navigation.publishTraverse, request, '+files')
108
109 def test_content_disposition_of_restricted_file(self):
110 # The content of restricted Librarian files for bug attachments
111 # is served by instances of SafeStreamOrRedirectLibraryFileAliasView
112 # which set the content disposition header of the HTTP response for
113 # to "attachment".
114 lfa_with_parent = getMultiAdapter(
115 (self.bugattachment.libraryfile, self.bugattachment),
116 ILibraryFileAliasWithParent)
117 lfa_with_parent.restricted = True
118 self.bug.setPrivate(True, self.bug_owner)
119 transaction.commit()
120 request = LaunchpadTestRequest()
121 request.setTraversalStack(['foo.txt'])
122 navigation = BugAttachmentFileNavigation(self.bugattachment, request)
123 view = navigation.publishTraverse(request, '+files')
124 next_view, traversal_path = view.browserDefault(request)
125 self.assertIsInstance(
126 next_view, SafeStreamOrRedirectLibraryFileAliasView)
127 next_view()
128 self.assertEqual(
129 'attachment', request.response.getHeader('Content-Disposition'))

Subscribers

People subscribed via source and target branches

to status/vote changes: