Merge lp:~stevenk/launchpad/proper-error-on-bug-attachment-bad-filename into lp:launchpad

[Steve Kowalik]

If adding an LFA raises an IntegrityError, catch it and re-raise it so the API gives Bad Request rather than an OOPS.

If adding the LFA in IBug.addAttachment raises an IntegrityError, catch it and raise it as BadRequest.

[Ian Booth (wallyworld)]

review: Approve

=== modified file 'lib/lp/bugs/tests/test_bugs_webservice.py'
--- lib/lp/bugs/tests/test_bugs_webservice.py	2012-08-30 06:18:38 +0000
+++ lib/lp/bugs/tests/test_bugs_webservice.py	2012-09-04 04:32:21 +0000
@@ -19,9 +19,7 @@
     Equals,
     LessThan,
     )
-from zope.component import (
-    getMultiAdapter,
-    )
+from zope.component import getMultiAdapter
 
 from lp.bugs.browser.bugtask import get_comments_for_bugtask
 from lp.bugs.interfaces.bug import IBug
@@ -344,7 +342,7 @@
 
 class TestErrorHandling(TestCaseWithFactory):
 
-    layer = DatabaseFunctionalLayer
+    layer = LaunchpadFunctionalLayer
 
     def test_add_duplicate_bugtask_for_project_gives_bad_request(self):
         bug = self.factory.makeBug()
@@ -375,6 +373,18 @@
         self.assertRaises(
             BadRequest, lp_bug.addTask, target=api_url(product2))
 
+    def test_add_attachment_with_bad_filename_raises_exception(self):
+        # Test that addAttachment raises BadRequest when the filename given
+        # contains slashes.
+        owner = self.factory.makePerson()
+        bug = self.factory.makeBug(owner=owner)
+        login_person(owner)
+        launchpad = launchpadlib_for('test', owner)
+        lp_bug = launchpad.load(api_url(bug))
+        self.assertRaises(
+            BadRequest, lp_bug.addAttachment, comment='foo', data='foo',
+            filename='/home/foo/bar.txt')
+
 
 class BugSetTestCase(TestCaseWithFactory):
 

=== modified file 'lib/lp/registry/errors.py'
--- lib/lp/registry/errors.py	2012-08-28 00:07:47 +0000
+++ lib/lp/registry/errors.py	2012-09-04 04:32:21 +0000
@@ -11,6 +11,7 @@
     'CommercialSubscribersOnly',
     'CountryMirrorAlreadySet',
     'DeleteSubscriptionError',
+    'InvalidFilename',
     'InvalidName',
     'JoinNotAllowed',
     'MirrorNotOfficial',
@@ -56,6 +57,11 @@
     """The name given for a person is not valid."""
 
 
+@error_status(httplib.BAD_REQUEST)
+class InvalidFilename(Exception):
+    """An invalid filename was used as an attachment filename."""
+
+
 class NoSuchDistroSeries(NameLookupFailed):
     """Raised when we try to find a DistroSeries that doesn't exist."""
     _message_prefix = "No such distribution series"

=== modified file 'lib/lp/services/librarian/model.py'
--- lib/lp/services/librarian/model.py	2011-12-30 06:14:56 +0000
+++ lib/lp/services/librarian/model.py	2012-09-04 04:32:21 +0000
@@ -1,8 +1,6 @@
 # Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
-# pylint: disable-msg=E0611,W0212
-
 __metaclass__ = type
 __all__ = [
     'LibraryFileAlias',
@@ -30,6 +28,7 @@
     SQLRelatedJoin,
     StringCol,
     )
+from storm.exceptions import IntegrityError
 from storm.locals import (
     Date,
     Desc,
@@ -46,6 +45,7 @@
     Interface,
     )
 
+from lp.registry.errors import InvalidFilename
 from lp.services.config import config
 from lp.services.database.constants import (
     DEFAULT,
@@ -269,15 +269,18 @@
 
     implements(ILibraryFileAliasSet)
 
-    def create(
-        self, name, size, file, contentType, expires=None, debugID=None,
-        restricted=False):
+    def create(self, name, size, file, contentType, expires=None,
+               debugID=None, restricted=False):
         """See `ILibraryFileAliasSet`"""
         if restricted:
             client = getUtility(IRestrictedLibrarianClient)
         else:
             client = getUtility(ILibrarianClient)
-        fid = client.addFile(name, size, file, contentType, expires, debugID)
+        try:
+            fid = client.addFile(
+                name, size, file, contentType, expires, debugID)
+        except IntegrityError:
+            raise InvalidFilename("Filename cannot contain slashes.")
         lfa = IMasterStore(LibraryFileAlias).find(
             LibraryFileAlias, LibraryFileAlias.id == fid).one()
         assert lfa is not None, "client.addFile didn't!"