launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #02263
lp:~adeuring/launchpad/webservice-access-to-private-bug-attamchments into lp:launchpad
Abel Deuring has proposed merging lp:~adeuring/launchpad/webservice-access-to-private-bug-attamchments into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
#620458 cannot access attachments of private bugs any more
https://bugs.launchpad.net/bugs/620458
#629804 remove workaround for private Librarian files for launchpadlib clients
https://bugs.launchpad.net/bugs/629804
For more details, see:
https://code.launchpad.net/~adeuring/launchpad/webservice-access-to-private-bug-attamchments/+merge/45022
This branch gives webservice clients access to restricted Librarian
files, and it removes a workaround giving machines in the DC direct
access to Librarian files.
Basically, this branch reverts r11506 which introduced the class
RestrictedLibraryBackedByteStorage.
The URL returned by this class for private Librarian files contained
an "unofficial" domain name (lplibrarian.internal or somesuch), thus
preventing access for machines outside of the DC.
This hack was needed because
- we wanted to use the restricted Librarian for attachments
of private bugs
- we did not want to use the app servers as proxies to access
potentially huge attachments
- the new token based access control to Librarian files was not
yet ready
- The apport retracers needed these files.
Details of the change:
lib/canonical/launchpad/browser/librarian.py:
lint removal; some typos fixed.
lib/canonical/launchpad/database/librarian.py,
lib/canonical/launchpad/interfaces/librarian.py:
A new method createToken() for the class LibraryFileAliasWithParent.
This method generates an access token for an LFA. This method requires
the permission Launchpad.View, thus ensuring that only those persons
having the "right permission" can acces the file.
The other cahnges of the interface file are lint removal.
lib/canonical/launchpad/rest/bytestorage.py:
The now obsoete class RestrictedLibraryBackedByteStorage is now gone;
the property alias_url now returns a URL with an access token for
restricted Librarian files, by calling
LibraryFileAliasWithParent.createToken().
lib/canonical/launchpad/security.py:
A "security guard" for the view permission of LibraryFileAliasWithParent.
lib/canonical/launchpad/tests/test_libraryfilealias_with_parent.py:
A test for the view permission of LFAWithParent.
lib/canonical/launchpad/zcml/librarian.zcml:
The View permission declaration for LFAWithParent.
lib/canonical/launchpad/zcml/webservice.zcml:
Removed the declarations for the deleted class
RestrictedLibraryBackedByteStorage
lib/lp/bugs/browser/tests/test_bugattachment_file_access.py:
Tests for accessing Librarian files via the webservice. The test for
restricted files is a bit convoluted because our test environment
cannot resolve the host names (1234.restricted.localhost), so the
ensures that the URL has the correct hostname, that it contains
the "right" path and that the access token is provided.
lib/lp/bugs/interfaces/bugattachment.py:
The field IBugAttachment.data is now again oy type Bytes instead of
RestrictedBytes.
lib/lp/bugs/stories/webservice/xx-bug.txt:
Deleted the test of the download URL of private bug attachments.
lib/lp/services/fields/__init__.py:
Removed the obsolete class RestrictedBytes
test: ./bin/test -vvt test_bugattachment_file_access
no lint
--
https://code.launchpad.net/~adeuring/launchpad/webservice-access-to-private-bug-attamchments/+merge/45022
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~adeuring/launchpad/webservice-access-to-private-bug-attamchments into lp:launchpad.
=== modified file 'lib/canonical/launchpad/browser/librarian.py'
--- lib/canonical/launchpad/browser/librarian.py 2010-12-16 19:22:28 +0000
+++ lib/canonical/launchpad/browser/librarian.py 2011-01-03 11:16:57 +0000
@@ -85,11 +85,11 @@
If the file is public, it will redirect to the files http url.
- Otherwise if the feature flag publicrestrictedlibrarian is set to 'on' this
- will allocate a token and redirect to the aliases private url.
+ Otherwise if the feature flag publicrestrictedlibrarian is set to 'on'
+ this will allocate a token and redirect to the aliases private url.
Otherwise it will proxy the file in the appserver.
-
+
Once we no longer have any proxy code at all it should be possible to
consolidate this with LibraryFileAliasView.
@@ -98,7 +98,7 @@
we have to take special care about their origin.
SafeStreamOrRedirectLibraryFileAliasView is used when we do not trust the
content, otherwise StreamOrRedirectLibraryFileAliasView. We are working
- to remove both of these views entirely, but some transition will be
+ to remove both of these views entirely, but some transition will be
needed.
The context provides a file-like interface - it can be opened and closed
@@ -161,7 +161,7 @@
def browserDefault(self, request):
"""Decides how to deliver the file.
-
+
The options are:
- redirect to the contexts http url
- redirect to a time limited secure url
@@ -182,7 +182,7 @@
# Public file, just point the client at the right place.
return RedirectionView(self.context.http_url, self.request), ()
if getFeatureFlag(u'publicrestrictedlibrarian') != 'on':
- # Restricted file and we have not enabled the public
+ # Restricted file and we have not enabled the public
# restricted librarian yet :- deliver inline.
self._when_streaming()
return self, ()
@@ -203,12 +203,12 @@
"""Hook for SafeStreamOrRedirectLibraryFileAliasView."""
-
class SafeStreamOrRedirectLibraryFileAliasView(MixedFileAliasView):
"""A view for Librarian files that sets the content disposition header."""
def _when_streaming(self):
- super(SafeStreamOrRedirectLibraryFileAliasView, self)._when_streaming()
+ super(
+ SafeStreamOrRedirectLibraryFileAliasView, self)._when_streaming()
self.request.response.setHeader(
'Content-Disposition', 'attachment')
@@ -257,18 +257,17 @@
class ProxiedLibraryFileAlias:
"""A `LibraryFileAlias` decorator for use in URL generation.
- The URL's output by this decorator will always point at the webapp. This is
- useful when:
- - we are proxying files via the webapp (as we do at the moment)
- - when the webapp has to be contacted to get access to a file (the case
- for restricted files in the future)
+ The URL's output by this decorator will always point at the webapp. This
+ is useful when:
+ - the webapp has to be contacted to get access to a file (required for
+ restricted files).
- files might change from public to private and thus not work even if the
user has access to the once its private, unless they go via the webapp.
This should be used anywhere we are outputting URL's to LibraryFileAliases
other than directly in rendered pages. For rendered pages, using a
LibraryFileAlias directly is OK as at that point the status of the file
- is know.
+ is known.
Overrides `ILibraryFileAlias.http_url` to always point to the webapp URL,
even when called from the webservice domain.
=== modified file 'lib/canonical/launchpad/database/librarian.py'
--- lib/canonical/launchpad/database/librarian.py 2010-11-08 12:52:43 +0000
+++ lib/canonical/launchpad/database/librarian.py 2011-01-03 11:16:57 +0000
@@ -156,8 +156,7 @@
self._datafile = self.client.getFileByAlias(self.id, timeout)
if self._datafile is None:
raise DownloadFailed(
- "Unable to retrieve LibraryFileAlias %d" % self.id
- )
+ "Unable to retrieve LibraryFileAlias %d" % self.id)
def read(self, chunksize=None, timeout=LIBRARIAN_SERVER_DEFAULT_TIMEOUT):
"""See ILibraryFileAlias."""
@@ -255,6 +254,10 @@
self.context = libraryfile
self.__parent__ = parent
+ def createToken(self):
+ """See `ILibraryFileAliasWithParent`."""
+ return TimeLimitedToken.allocate(self.private_url)
+
class LibraryFileAliasSet(object):
"""Create and find LibraryFileAliases."""
=== modified file 'lib/canonical/launchpad/interfaces/librarian.py'
--- lib/canonical/launchpad/interfaces/librarian.py 2010-09-05 10:49:21 +0000
+++ lib/canonical/launchpad/interfaces/librarian.py 2011-01-03 11:16:57 +0000
@@ -40,6 +40,7 @@
# should never be removed from the Librarian.
NEVER_EXPIRES = datetime(2038, 1, 1, 0, 0, 0, tzinfo=utc)
+
class ILibraryFileAlias(Interface):
id = Int(
title=_('Library File Alias ID'), required=True, readonly=True,
@@ -133,6 +134,9 @@
class ILibraryFileAliasWithParent(ILibraryFileAlias):
"""A ILibraryFileAlias that knows about its parent."""
+ def createToken(self):
+ """Create a token allowing time-limited access to this file."""
+
class ILibraryFileContent(Interface):
"""Actual data in the Librarian.
@@ -143,20 +147,14 @@
title=_('Library File Content ID'), required=True, readonly=True,
)
datecreated = Datetime(
- title=_('Date created'), required=True, readonly=True
- )
- filesize = Int(
- title=_('File size'), required=True, readonly=True
- )
- sha1 = TextLine(
- title=_('SHA-1 hash'), required=True, readonly=True
- )
- md5 = TextLine(
- title=_('MD5 hash'), required=True, readonly=True
- )
+ title=_('Date created'), required=True, readonly=True)
+ filesize = Int(title=_('File size'), required=True, readonly=True)
+ sha1 = TextLine(title=_('SHA-1 hash'), required=True, readonly=True)
+ md5 = TextLine(title=_('MD5 hash'), required=True, readonly=True)
class ILibraryFileAliasSet(Interface):
+
def create(name, size, file, contentType, expires=None, debugID=None,
restricted=False):
"""Create a file in the Librarian, returning the new alias.
=== modified file 'lib/canonical/launchpad/rest/bytestorage.py'
--- lib/canonical/launchpad/rest/bytestorage.py 2010-12-16 19:22:28 +0000
+++ lib/canonical/launchpad/rest/bytestorage.py 2011-01-03 11:16:57 +0000
@@ -6,18 +6,23 @@
__metaclass__ = type
__all__ = [
'LibraryBackedByteStorage',
- 'RestrictedLibraryBackedByteStorage',
]
from cStringIO import StringIO
from lazr.restful.interfaces import IByteStorage
-from zope.component import getUtility
+from zope.component import (
+ getMultiAdapter,
+ getUtility,
+ )
+
from zope.interface import implements
-from canonical.launchpad.browser.librarian import ProxiedLibraryFileAlias
-from canonical.launchpad.interfaces.librarian import ILibraryFileAliasSet
+from canonical.launchpad.interfaces.librarian import (
+ ILibraryFileAliasSet,
+ ILibraryFileAliasWithParent,
+ )
from canonical.launchpad.webapp.interfaces import ICanonicalUrlData
@@ -49,6 +54,12 @@
@property
def alias_url(self):
"""See `IByteStorage`."""
+ if self.file_alias.restricted:
+ lfa_with_parent = getMultiAdapter(
+ (self.file_alias, self.entry.context),
+ ILibraryFileAliasWithParent)
+ token = lfa_with_parent.createToken()
+ return self.file_alias.private_url + '?token=%s' % token
return self.file_alias.getURL()
@property
@@ -75,29 +86,3 @@
def deleteStored(self):
"""See `IByteStorage`."""
setattr(self.entry, self.field.__name__, None)
-
-
-class RestrictedLibraryBackedByteStorage(LibraryBackedByteStorage):
- """See `IByteStorage`.
-
- This variant of LibraryBackedByteStorage provides an alias_url
- which points to a StreamOrRedirectLibraryFileAliasView for
- restricted Librarian files.
- """
-
- @property
- def alias_url(self):
- """See `IByteStorage`."""
- if self.file_alias.restricted:
- # XXX Abel Deuring 2010-09-03, bug=629804
- # This is a bad hack: We can't give ordinary users API access to
- # restricted files at present. But we can allow access to
- # some machines in the data center (and should do that).
- # We need here an HTTP URL, not an HTTPS URL, so we don't
- # use getUrl(). Since http_url uses not an official domain
- # name, all we do is expose an unaccessible HTTP URL also
- # users outside of the data center, instead of the HTTPS URL
- # returned by getURL().
- return self.file_alias.http_url
- else:
- return self.file_alias.getURL()
=== modified file 'lib/canonical/launchpad/security.py'
--- lib/canonical/launchpad/security.py 2010-12-22 12:05:12 +0000
+++ lib/canonical/launchpad/security.py 2011-01-03 11:16:57 +0000
@@ -2522,3 +2522,23 @@
if parent is None:
return False
return check_permission(self.permission, parent)
+
+
+class ViewLibraryFileAliasWithParent(AuthorizationBase):
+ permission = 'launchpad.View'
+ usedfor = ILibraryFileAliasWithParent
+
+ def checkAuthenticated(self, user):
+ """Only persons which can edit an LFA's parent can edit an LFA.
+
+ By default, a LibraryFileAlias does not know about its parent.
+ Such aliases are never editable. Use an adapter to provide a
+ parent object.
+
+ If a parent is known, users which can edit the parent can also
+ edit properties of the LibraryFileAlias.
+ """
+ parent = getattr(self.obj, '__parent__', None)
+ if parent is None:
+ return False
+ return check_permission(self.permission, parent)
=== modified file 'lib/canonical/launchpad/tests/test_libraryfilealias_with_parent.py'
--- lib/canonical/launchpad/tests/test_libraryfilealias_with_parent.py 2010-10-04 19:50:45 +0000
+++ lib/canonical/launchpad/tests/test_libraryfilealias_with_parent.py 2011-01-03 11:16:57 +0000
@@ -56,3 +56,17 @@
self.assertRaises(
Unauthorized, setattr, self.lfa_with_parent, 'restricted', True)
+ def test_createToken_authorized_user(self):
+ # Persons having access to a parent object of a restricted
+ # Librarian file can call the method
+ # ILibraryFileAliasWithParent.createToken()
+ login_person(self.bug_owner)
+ self.lfa_with_parent.createToken()
+
+ def test_createToken_unauthorized_user(self):
+ # Users without access to a parent object cannot call
+ # ILibraryFileAliasWithParent.createToken()
+ other_person = self.factory.makePerson()
+ login_person(other_person)
+ self.assertRaises(
+ Unauthorized, getattr, self.lfa_with_parent, 'createToken')
=== modified file 'lib/canonical/launchpad/zcml/librarian.zcml'
--- lib/canonical/launchpad/zcml/librarian.zcml 2010-10-06 18:53:53 +0000
+++ lib/canonical/launchpad/zcml/librarian.zcml 2011-01-03 11:16:57 +0000
@@ -13,7 +13,10 @@
</class>
<class class="canonical.launchpad.database.librarian.LibraryFileAliasWithParent">
- <allow interface="canonical.launchpad.interfaces.librarian.ILibraryFileAliasWithParent" />
+ <allow interface="canonical.launchpad.interfaces.librarian.ILibraryFileAlias" />
+ <require
+ permission="launchpad.View"
+ attributes="createToken" />
<require
permission="launchpad.Edit"
set_attributes="mimetype restricted" />
=== modified file 'lib/canonical/launchpad/zcml/webservice.zcml'
--- lib/canonical/launchpad/zcml/webservice.zcml 2010-12-16 19:22:28 +0000
+++ lib/canonical/launchpad/zcml/webservice.zcml 2011-01-03 11:16:57 +0000
@@ -43,19 +43,6 @@
<allow interface='lazr.restful.interfaces.IByteStorage' />
</class>
- <!-- Registration for the class that manages an entry's RestrictedBytes
- fields. -->
- <adapter
- for="lazr.restful.interfaces.IEntry
- lp.services.fields.IRestrictedBytes"
- provides="lazr.restful.interfaces.IByteStorage"
- factory="canonical.launchpad.rest.bytestorage.RestrictedLibraryBackedByteStorage"
- />
-
- <class class="canonical.launchpad.rest.bytestorage.RestrictedLibraryBackedByteStorage">
- <allow interface='lazr.restful.interfaces.IByteStorage' />
- </class>
-
<!-- WebService uses the default LaunchpadRootNavigation -->
<view
for="canonical.launchpad.interfaces.launchpad.IWebServiceApplication"
=== modified file 'lib/lp/bugs/browser/tests/test_bugattachment_file_access.py'
--- lib/lp/bugs/browser/tests/test_bugattachment_file_access.py 2010-10-20 20:51:26 +0000
+++ lib/lp/bugs/browser/tests/test_bugattachment_file_access.py 2011-01-03 11:16:57 +0000
@@ -4,6 +4,10 @@
__metaclass__ = type
import re
+from urlparse import (
+ parse_qs,
+ urlparse,
+ )
import transaction
from zope.component import (
@@ -12,6 +16,7 @@
)
from zope.publisher.interfaces import NotFound
from zope.security.interfaces import Unauthorized
+from zope.security.management import endInteraction
from canonical.launchpad.browser.librarian import (
SafeStreamOrRedirectLibraryFileAliasView,
@@ -24,12 +29,15 @@
from canonical.launchpad.webapp.publisher import RedirectionView
from canonical.launchpad.webapp.servers import LaunchpadTestRequest
from canonical.testing.layers import LaunchpadFunctionalLayer
+from lazr.restfulclient.errors import Unauthorized as RestfulUnauthorized
from lp.bugs.browser.bugattachment import BugAttachmentFileNavigation
import lp.services.features
from lp.services.features.flags import NullFeatureController
from lp.testing import (
+ launchpadlib_for,
login_person,
TestCaseWithFactory,
+ ws_object,
)
@@ -149,3 +157,69 @@
next_view()
self.assertEqual(
'attachment', request.response.getHeader('Content-Disposition'))
+
+
+class TestWebserviceAccessToBugAttachmentFiles(TestCaseWithFactory):
+ """Tests access to bug attachments via the webservice."""
+
+ layer = LaunchpadFunctionalLayer
+
+ def setUp(self):
+ super(TestWebserviceAccessToBugAttachmentFiles, self).setUp()
+ self.bug_owner = self.factory.makePerson()
+ getUtility(ILaunchBag).clear()
+ login_person(self.bug_owner)
+ self.bug = self.factory.makeBug(owner=self.bug_owner)
+ self.bugattachment = self.factory.makeBugAttachment(
+ bug=self.bug, filename='foo.txt', data='file content')
+
+ def test_anon_access_to_public_bug_attachment(self):
+ # Attachments of public bugs can be accessed by anonymous users.
+ #
+ # Need to endInteraction() because launchpadlib_for_anonymous() will
+ # setup a new one.
+ endInteraction()
+ launchpad = launchpadlib_for('test', None, version='devel')
+ ws_bug = ws_object(launchpad, self.bug)
+ ws_bugattachment = ws_bug.attachments[0]
+ self.assertEqual(
+ 'file content', ws_bugattachment.data.open().read())
+
+ def test_user_access_to_private_bug_attachment(self):
+ # Users having access to private bugs can also read attachments
+ # of these bugs.
+ self.bug.setPrivate(True, self.bug_owner)
+ other_user = self.factory.makePerson()
+ launchpad = launchpadlib_for('test', self.bug_owner, version='devel')
+ ws_bug = ws_object(launchpad, self.bug)
+ ws_bugattachment = ws_bug.attachments[0]
+
+ # The attachment contains a link to a HostedBytes resource;
+ # accessing this link results normally in a redirect to a
+ # Librarian URL. We cannot simply access these Librarian URLS
+ # for restricted Librarian files because the host name used in
+ # the URLs is different for each file, and our test envireonment
+ # does not support wildcard DNS. So let's disable the redirection
+ # mechanism in our client's HTTP connection and inspect the
+ # the Librarian URL.
+ launchpad._browser._connection.follow_redirects = False
+ response, content = launchpad._browser.get(
+ ws_bugattachment.data._wadl_resource._url, return_response=True)
+ self.assertEqual(303, response.status)
+ parsed_url = urlparse(response['location'])
+ self.assertEqual('https', parsed_url.scheme)
+ mo = re.search(
+ r'^i\d+\.restricted\.localhost:58000$', parsed_url.netloc)
+ self.assertIsNot(None, mo)
+ mo = re.search(r'^/\d+/foo\.txt$', parsed_url.path)
+ self.assertIsNot(None, mo)
+ params = parse_qs(parsed_url.query)
+ self.assertEqual(['token'], params.keys())
+
+ # If a user which cannot access the private bug itself tries to
+ # to access the attachemnt, an Unauthorized error is raised.
+ other_launchpad = launchpadlib_for(
+ 'test_unauthenticated', other_user, version='devel')
+ self.assertRaises(
+ RestfulUnauthorized, other_launchpad._browser.get,
+ ws_bugattachment.data._wadl_resource._url)
=== modified file 'lib/lp/bugs/interfaces/bugattachment.py'
--- lib/lp/bugs/interfaces/bugattachment.py 2010-12-16 19:22:28 +0000
+++ lib/lp/bugs/interfaces/bugattachment.py 2011-01-03 11:16:57 +0000
@@ -39,7 +39,7 @@
from canonical.launchpad import _
from canonical.launchpad.interfaces.launchpad import IHasBug
from canonical.launchpad.interfaces.message import IMessage
-from lp.services.fields import RestrictedBytes, Title
+from lp.services.fields import Title
class BugAttachmentType(DBEnumeratedType):
@@ -115,7 +115,7 @@
libraryfile = Bytes(title=_("The attachment content."),
required=True)
data = exported(
- RestrictedBytes(title=_("The attachment content."),
+ Bytes(title=_("The attachment content."),
required=True,
readonly=True))
message = exported(
=== modified file 'lib/lp/bugs/stories/webservice/xx-bug.txt'
--- lib/lp/bugs/stories/webservice/xx-bug.txt 2010-12-16 19:22:28 +0000
+++ lib/lp/bugs/stories/webservice/xx-bug.txt 2011-01-03 11:16:57 +0000
@@ -1360,46 +1360,6 @@
---
-Attachments of private bugs
-~~~~~~~~~~~~~~~~~~~~~~~~~~~
-
-Attachments of private bugs are served by the app server itself.
-
- >>> print webservice.patch(
- ... bug_one['self_link'], 'application/json', dumps(dict(private=True)))
- HTTP/1.1 209 Content Returned...
-
- >>> response = webservice.named_post(
- ... bug_one['self_link'], 'addAttachment',
- ... data="67890", filename="more-numbers.txt", content_type='foo/bar',
- ... comment="The numbers you asked for.")
- >>> attachments = webservice.get(
- ... bug_one['attachments_collection_link']).jsonBody()
- >>> attachment = attachments['entries'][0]
- >>> pprint_entry(attachment)
- bug_link:...
- data_link: u'http://api.launchpad.dev/beta/bugs/1/+attachment/.../data'
- ...
-
-XXX Abel Deuring 2010-09-03, bug=629804: Please note that the http URLs in this
-example are part of a temporary hack. Please see the comment in
-canonical.launchpad.rest.bytestorage.RestrictedLibraryBackedByteStorage.
-alias_url for details.
-
- >>> response = webservice.get(attachment['data_link'])
- >>> print response
- HTTP/1.1 303 See Other
- Status: 303 See Other
- ...
- Location: http://localhost:58005/94/more-numbers.txt
- ...
-
- >>> # Let's make the bug public again for later tests.
- >>> print webservice.patch(
- ... bug_one['self_link'], 'application/json', dumps(dict(private=False)))
- HTTP/1.1 209 Content Returned...
-
-
Searching for bugs
------------------
@@ -2133,7 +2093,7 @@
next_collection_link: u'http://.../bugs/1/activity?ws.start=5&ws.size=5'
resource_type_link: u'http://.../#bug_activity-page-resource'
start: 0
- total_size: 26
+ total_size: 23
...
>>> bug_nine_activity = webservice.get(
=== modified file 'lib/lp/services/fields/__init__.py'
--- lib/lp/services/fields/__init__.py 2010-12-16 19:22:28 +0000
+++ lib/lp/services/fields/__init__.py 2011-01-03 11:16:57 +0000
@@ -22,7 +22,6 @@
'ILocationField',
'INoneableTextLine',
'IPasswordField',
- 'IRestrictedBytes',
'IStrippedTextLine',
'ISummary',
'ITag',
@@ -46,7 +45,6 @@
'ProductBugTracker',
'ProductNameField',
'PublicPersonChoice',
- 'RestrictedBytes',
'SearchTag',
'StrippedTextLine',
'Summary',
@@ -229,10 +227,6 @@
"""
-class IRestrictedBytes(IBytes):
- """A marker interface used for restricted LibraryFileAlias fields."""
-
-
class StrippedTextLine(TextLine):
implements(IStrippedTextLine)
@@ -842,8 +836,3 @@
else:
# The vocabulary prevents the revealing of private team names.
raise PrivateTeamNotAllowed(value)
-
-
-class RestrictedBytes(Bytes):
- """A field for restricted LibraryFileAlias records."""
- implements(IRestrictedBytes)
