Merge lp:~adeuring/launchpad/webservice-access-to-private-bug-attamchments into lp:launchpad

[Abel Deuring]

[r=gmb,leonardr][ui=none][bug=620458,629804] use LaunchpadWebServiceCaller to check the HTTP response in test_user_access_to_private_bug_attachment()

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

[Graham Binns (gmb)]

Hi Abel,

We discussed on IRC the refactoring that I mentioned below; I've also
got a couple of nitpicks. Otherwise this is r=me.

243 +class ViewLibraryFileAliasWithParent(AuthorizationBase):
244 + permission = 'launchpad.View'

This needs a docstring.

382 + def test_anon_access_to_public_bug_attachment(self):
383 + # Attachments of public bugs can be accessed by anonymous users.
384 + #
385 + # Need to endInteraction() because launchpadlib_for_anonymous() will

The empty comment line looks a little odd. I think you can remove the
'#' and just leave the blank line for separation in this case.

403 + # The attachment contains a link to a HostedBytes resource;
404 + # accessing this link results normally in a redirect to a
405 + # Librarian URL. We cannot simply access these Librarian URLS
406 + # for restricted Librarian files because the host name used in
407 + # the URLs is different for each file, and our test envireonment
408 + # does not support wildcard DNS. So let's disable the redirection
409 + # mechanism in our client's HTTP connection and inspect the
410 + # the Librarian URL.
411 + launchpad._browser._connection.follow_redirects = False
412 + response, content = launchpad._browser.get(
413 + ws_bugattachment.data._wadl_resource._url, return_response=True)
414 + self.assertEqual(303, response.status)
415 + parsed_url = urlparse(response['location'])
416 + self.assertEqual('https', parsed_url.scheme)
417 + mo = re.search(
418 + r'^i\d+\.restricted\.localhost:58000$', parsed_url.netloc)
419 + self.assertIsNot(None, mo)
420 + mo = re.search(r'^/\d+/foo\.txt$', parsed_url.path)
421 + self.assertIsNot(None, mo)
422 + params = parse_qs(parsed_url.query)
423 + self.assertEqual(['token'], params.keys())

I know we don't use this elsewhere at the moment, but it may be worth
worth refactoring it out into either a method on the TestCase or a
standalone function so that it can be reused later if necessary.

review: Approve (code)

[Leonard Richardson (leonardr)]

I'm a little concerned about the getRaw method you introduce in revision 12117. We already have a mechanism for doing HTTP-level testing of the web service: the LaunchpadWebServiceCaller. Can we add the ability to follow links to WebServiceCaller? I realize this would mean doing a lazr.restful branch and release. I don't like the idea of re-implementing the low-level interface as methods of the high-level interface.

[Abel Deuring (adeuring)]

I wasn't aware of LaunchpadWebServiceCaller, thanks for the heads-up! Actually, we _don't_ want to follow the redirect in test_user_access_to_private_bug_attachment, so the current behaviour of WebServiceCaller is just right :)

Leonard, could you have a look at the incremental diff below?

=== modified file 'lib/lp/bugs/browser/tests/test_bugattachment_file_access.py'
--- lib/lp/bugs/browser/tests/test_bugattachment_file_access.py 2011-01-05 09:39:00 +0000
+++ lib/lp/bugs/browser/tests/test_bugattachment_file_access.py 2011-01-06 11:40:15 +0000
@@ -25,6 +25,7 @@
 from canonical.launchpad.interfaces.librarian import (
     ILibraryFileAliasWithParent,
     )
+from canonical.launchpad.testing.pages import LaunchpadWebServiceCaller
 from canonical.launchpad.webapp.interfaces import ILaunchBag
 from canonical.launchpad.webapp.publisher import RedirectionView
 from canonical.launchpad.webapp.servers import LaunchpadTestRequest
@@ -202,16 +203,19 @@
         # 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.
+ # does not support wildcard DNS, and because the Launchpadlib
+ # browser automatically follows redirects.
+ # LaunchpadWebServiceCaller, on the other hand, gives us
+ # access to a raw HTTPResonse object.
+ webservice = LaunchpadWebServiceCaller(
+ 'launchpad-library', 'salgado-change-anything')
+ response = webservice.get(ws_bugattachment.data._wadl_resource._url)
+ self.assertEqual(303, response.status)
+
         # The Librarian URL has, for our test case, the form
         # "https://NNNN.restricted.localhost:58000/NNNN/foo.txt?token=..."
         # where NNNN is an integer.
- response, content = launchpad.rawGet(
- ws_bugattachment.data._wadl_resource._url, follow_redirects=False)
- self.assertEqual(303, response.status)
- parsed_url = urlparse(response['location'])
+ parsed_url = urlparse(response.getHeader('location'))
         self.assertEqual('https', parsed_url.scheme)
         mo = re.search(
             r'^i\d+\.restricted\.localhost:58000$', parsed_url.netloc)
@@ -222,7 +226,7 @@
         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.
+ # to access the attachment, an Unauthorized error is raised.
         other_launchpad = launchpadlib_for(
             'test_unauthenticated', other_user, version='devel')
         self.assertRaises(

=== modified file 'lib/lp/testing/_webservice.py'
--- lib/lp/testing/_webservice.py 2011-01-04 13:34:53 +0000
+++ lib/lp/testing/_webservice.py 2011-01-06 11:46:25 +0000
@@ -145,31 +145,7 @@
     version = version or Launchpad.DEFAULT_VERSION
     cache = tempfile.mkdtemp(prefix='launchpadlib-cache-')
     zope.testi...

[Leonard Richardson (leonardr)]

The change looks good.

The magic strings are also kept in launchpadlib. launchpadlib.testing.helpers.salgado_with_full_permissions.credentials is a Credentials object corresponding to the credentials you want. It's not optimized for your case, but maybe you could get it to work with a hack and we can fix it up later.

[Leonard Richardson (leonardr)]

review: Approve

=== modified file 'lib/lp/bugs/browser/tests/test_bugattachment_file_access.py'
--- lib/lp/bugs/browser/tests/test_bugattachment_file_access.py	2011-01-05 09:39:00 +0000
+++ lib/lp/bugs/browser/tests/test_bugattachment_file_access.py	2011-01-14 12:13:51 +0000
@@ -25,6 +25,7 @@
 from canonical.launchpad.interfaces.librarian import (
     ILibraryFileAliasWithParent,
     )
+from canonical.launchpad.testing.pages import LaunchpadWebServiceCaller
 from canonical.launchpad.webapp.interfaces import ILaunchBag
 from canonical.launchpad.webapp.publisher import RedirectionView
 from canonical.launchpad.webapp.servers import LaunchpadTestRequest
@@ -202,16 +203,19 @@
         # 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.
+        # does not support wildcard DNS, and because the Launchpadlib
+        # browser automatically follows redirects.
+        # LaunchpadWebServiceCaller, on the other hand, gives us
+        # access to a raw HTTPResonse object.
+        webservice = LaunchpadWebServiceCaller(
+            'launchpad-library', 'salgado-change-anything')
+        response = webservice.get(ws_bugattachment.data._wadl_resource._url)
+        self.assertEqual(303, response.status)
+
         # The Librarian URL has, for our test case, the form
         # "https://NNNN.restricted.localhost:58000/NNNN/foo.txt?token=..."
         # where NNNN is an integer.
-        response, content = launchpad.rawGet(
-            ws_bugattachment.data._wadl_resource._url, follow_redirects=False)
-        self.assertEqual(303, response.status)
-        parsed_url = urlparse(response['location'])
+        parsed_url = urlparse(response.getHeader('location'))
         self.assertEqual('https', parsed_url.scheme)
         mo = re.search(
             r'^i\d+\.restricted\.localhost:58000$', parsed_url.netloc)
@@ -222,7 +226,7 @@
         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.
+        # to access the attachment, an Unauthorized error is raised.
         other_launchpad = launchpadlib_for(
             'test_unauthenticated', other_user, version='devel')
         self.assertRaises(

=== modified file 'lib/lp/testing/_webservice.py'
--- lib/lp/testing/_webservice.py	2011-01-05 18:56:04 +0000
+++ lib/lp/testing/_webservice.py	2011-01-14 12:13:51 +0000
@@ -145,31 +145,8 @@
     version = version or Launchpad.DEFAULT_VERSION
     cache = tempfile.mkdtemp(prefix='launchpadlib-cache-')
     zope.testing.cleanup.addCleanUp(_clean_up_cache, (cache,))
-    return TestLaunchpad(credentials, None, None, service_root=service_root,
-                         version=version, cache=cache)
-
-
-class TestLaunchpad(Launchpad):
-    """A variant of the Launchpad service root class providing test helpers.
-    """
-
-    def rawGet(self, url, follow_redirects=None):
-        """Return a the result of a GET request for the given URL.
-
-        :param url: The URL of the request.
-        :param follow_redirects: If None, keep the default behaviour.
-            If True, follow redirect responses.
-            If False, do not follow a redirect response but return the
-            plain redirect.
-
-        :return: The tuple (response, content)
-        """
-        default_redirect = self._browser._connection.follow_redirects
-        if follow_redirects is not None:
-            self._browser._connection.follow_redirects = follow_redirects
-        response, content = self._browser.get(url, return_response=True)
-        self._browser._connection.follow_redirects = default_redirect
-        return response, content
+    return Launchpad(credentials, None, None, service_root=service_root,
+                     version=version, cache=cache)
 
 
 class QueryCollector: