Merge lp:~jcsackett/launchpad/private-team-to-private-team-519306 into lp:launchpad

[j.c.sackett]

Uses expose() on an exception in team validator so that the exception is properly passed through the API, rather than OOPSing.

Summary
=======

Exposes an exception across the API when a bad attempt is made to link private teams. This prevents the OOPS from the related bug.

Proposed Fix
============

Wrap the exception normally returned by the validator for the website in the expose method and tie it to a 403 error.

Implementation details
======================

Largely as in proposed fix. Instead of 403, it returns a 400, as webservice_error reverts to that if the exception is called from deeper in the call stack than the API named method. As this occurs in the validator, it will always be a 400 so the code reflects that.

Tests
=====

bin/test -t test_team_webservice

Demo and Q/A
============

Use launchpadlib to connect launchpad.dev; try to add one private team as a member to another (as the owner of both teams). You will get an HTTP 400 response.

Lint
====
make lint output:

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/registry/tests/test_project.py
  lib/lp/registry/tests/test_team_webservice.py

[Brad Crittenden (bac)]

Looks good Jon with these comments:

1) use httplib.BAD_REQUEST instead of a raw 400 throughout your changes. Please leave the comment that shows the raw number.

2) run utilities/format-imports on your changed files. Easier than me pointing out the issues.

3) 'e' is a pretty recognized variable for an exception but we still try to avoid one letter variables, save for i,j,k loop variables.

review: Approve (code)

=== modified file 'lib/lp/registry/browser/tests/test_person_webservice.py'
--- lib/lp/registry/browser/tests/test_person_webservice.py	2010-08-20 20:31:18 +0000
+++ lib/lp/registry/browser/tests/test_person_webservice.py	2010-09-27 14:03:46 +0000
@@ -14,6 +14,7 @@
 
 
 class TestPersonRepresentation(TestCaseWithFactory):
+
     layer = DatabaseFunctionalLayer
 
     def setUp(self):

=== modified file 'lib/lp/registry/interfaces/person.py'
--- lib/lp/registry/interfaces/person.py	2010-09-23 02:33:17 +0000
+++ lib/lp/registry/interfaces/person.py	2010-09-27 14:03:46 +0000
@@ -41,6 +41,7 @@
     'validate_public_person',
     ]
 
+import httplib
 
 from lazr.enum import (
     DBEnumeratedType,
@@ -66,6 +67,7 @@
     REQUEST_USER,
     webservice_error,
     )
+from lazr.restful.error import expose
 from lazr.restful.fields import (
     CollectionField,
     Reference,
@@ -158,6 +160,10 @@
 
 class PrivatePersonLinkageError(ValueError):
     """An attempt was made to link a private person/team to something."""
+    # HTTP 400 -- BAD REQUEST
+    # HTTP 403 would be better, but as this excpetion is raised inside a
+    # validator, it will default to 400 anyway.
+    webservice_error(httplib.BAD_REQUEST)
 
 
 @block_implicit_flushes
@@ -168,15 +174,14 @@
     assert isinstance(value, (int, long)), (
         "Expected int for Person foreign key reference, got %r" % type(value))
 
-    # XXX sinzui 2009-04-03 bug=354881: We do not want to import from the
-    # DB. This needs cleaning up.
+    # Importing here to avoid a cyclic import.
     from lp.registry.model.person import Person
     person = Person.get(value)
     if not validate_func(person):
-        raise PrivatePersonLinkageError(
+        raise expose(PrivatePersonLinkageError(
             "Cannot link person (name=%s, visibility=%s) to %s (name=%s)"
             % (person.name, person.visibility.name,
-               obj, getattr(obj, 'name', None)))
+               obj, getattr(obj, 'name', None))))
     return value
 
 

=== modified file 'lib/lp/registry/tests/test_project.py'
--- lib/lp/registry/tests/test_project.py	2010-09-07 21:29:33 +0000
+++ lib/lp/registry/tests/test_project.py	2010-09-27 14:03:46 +0000
@@ -5,20 +5,17 @@
 
 import unittest
 
+from lazr.restfulclient.errors import ClientError
 from zope.component import getUtility
-from lazr.restfulclient.errors import ClientError
 
-from canonical.launchpad.testing.pages import LaunchpadWebServiceCaller
 from canonical.launchpad.ftests import login
 from canonical.launchpad.webapp.errorlog import globalErrorUtility
 from canonical.testing import (
+    DatabaseFunctionalLayer,
     LaunchpadFunctionalLayer,
-    DatabaseFunctionalLayer,
     )
 from lp.registry.interfaces.projectgroup import IProjectGroupSet
-from lp.soyuz.enums import ArchivePurpose
 from lp.testing import (
-    celebrity_logged_in,
     launchpadlib_for,
     TestCaseWithFactory,
     )
@@ -111,6 +108,7 @@
 
 
 class TestLaunchpadlibAPI(TestCaseWithFactory):
+
     layer = DatabaseFunctionalLayer
 
     def test_inappropriate_deactivation_does_not_cause_an_OOPS(self):

=== added file 'lib/lp/registry/tests/test_team_webservice.py'
--- lib/lp/registry/tests/test_team_webservice.py	1970-01-01 00:00:00 +0000
+++ lib/lp/registry/tests/test_team_webservice.py	2010-09-27 14:03:46 +0000
@@ -0,0 +1,51 @@
+# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+__metaclass__ = type
+
+import unittest
+
+from lazr.restfulclient.errors import HTTPError
+
+from canonical.testing import DatabaseFunctionalLayer
+from lp.registry.interfaces.person import PersonVisibility
+from lp.testing import (
+    launchpadlib_for,
+    TestCaseWithFactory,
+    )
+
+
+class TestTeamLinking(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    def setUp(self):
+        super(TestTeamLinking, self).setUp()
+        self.team_owner = self.factory.makePerson(name='team-owner')
+        self.private_team_one = self.factory.makeTeam(
+            owner=self.team_owner,
+            name='private-team',
+            displayname='Private Team',
+            visibility=PersonVisibility.PRIVATE)
+        self.private_team_two = self.factory.makeTeam(
+            owner=self.team_owner,
+            name='private-team-two',
+            displayname='Private Team Two',
+            visibility=PersonVisibility.PRIVATE)
+
+    def test_private_links(self):
+        # A private team cannot be linked to another team, private or
+        # or otherwise.
+        launchpad = launchpadlib_for("test", self.team_owner.name)
+        team_one = launchpad.people['private-team']
+        team_two = launchpad.people['private-team-two']
+        api_error = self.assertRaises(
+            HTTPError,
+            team_one.addMember,
+            person=team_two)
+        self.assertIn('Cannot link person', api_error.content)
+        self.assertEqual(400, api_error.response.status)
+
+
+def test_suite():
+    return unittest.TestLoader().loadTestsFromName(__name__)