launchpad-reviewers team mailing list archive

["j.c.sackett" <jonathan.sackett@xxxxxxxxxxxxx>]

j.c.sackett has proposed merging lp:~jcsackett/launchpad/private-team-to-private-team-519306 into lp:launchpad/devel.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #519306 OOPS adding private team to another private team
  https://bugs.launchpad.net/bugs/519306


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
-- 
https://code.launchpad.net/~jcsackett/launchpad/private-team-to-private-team-519306/+merge/36589
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jcsackett/launchpad/private-team-to-private-team-519306 into lp:launchpad/devel.

=== 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-24 19:17:45 +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-24 19:17:45 +0000
@@ -66,6 +66,7 @@
     REQUEST_USER,
     webservice_error,
     )
+from lazr.restful.error import expose
 from lazr.restful.fields import (
     CollectionField,
     Reference,
@@ -158,6 +159,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(400)
 
 
 @block_implicit_flushes
@@ -168,15 +173,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-24 19:17:45 +0000
@@ -8,7 +8,6 @@
 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 (
@@ -16,9 +15,7 @@
     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-24 19:17:45 +0000
@@ -0,0 +1,50 @@
+# 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 canonical.testing import DatabaseFunctionalLayer
+from lazr.restfulclient.errors import HTTPError
+from lp.registry.interfaces.person import PersonVisibility
+from lp.testing import (
+    TestCaseWithFactory,
+    launchpadlib_for,
+    )
+
+
+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']
+        e = self.assertRaises(
+            HTTPError,
+            team_one.addMember,
+            person=team_two)
+        self.assertIn('Cannot link person', e.content)
+        self.assertEqual(400, e.response.status)
+
+
+def test_suite():
+    return unittest.TestLoader().loadTestsFromName(__name__)