launchpad-reviewers team mailing list archive

[Curtis Hovey <curtis.hovey@xxxxxxxxxxxxx>]

Curtis Hovey has proposed merging lp:~sinzui/launchpad/delete-team-4 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #687676 Registry Admin does not have permission to delete teams
  https://bugs.launchpad.net/bugs/687676


Allow registry admin to delete teams with super teams.

    Launchpad bug: https://bugs.launchpad.net/bugs/687676
    Pre-implementation: no one
    Test command: ./bin/test -vv -t TestAdminTeamMergeView

The recent change to remove super teams before starting a merge breaks team
deletions by the registry admins. The team does not have permission to call
retractTeamMembership in many cases. There are two issues underlying this
symptom.

The loop is iterating over all teams participated in, not the direct
membership. retractTeamMembership() is safe to call in this case when
the user has permission. Even when the user has permission to call the
method on the direct team, there is a failure on the indirect team.

As with view code that prepares an object for deletion or merge, the case
requires super privileges to complete the cleanup. Either Registry admins
should have launchpad.Edit on the team, or gets special privileges on the
method, or the view removed the security proxy to complete the task.

--------------------------------------------------------------------

RULES

    * Person does not provide a method to get direct super teams, but that
      can be derived in an extra db call to get the indirect teams.
    * Use removeSecurityProxy() to permit registry admins to complete
      the implicit tasks implied by the delete action. This is already
      done when working with the team email addresses. It can be done
      when calling retractTeamMembership()


QA

    * As a registry admin, delete these teams:
      https://launchpad.net/people/?name=delete&searchfor=teamsonly


LINT

    lib/lp/registry/browser/peoplemerge.py
    lib/lp/registry/browser/tests/test_peoplemerge.py


IMPLEMENTATION

The set solution to get the direct team membership is easy, but I think it
odd that my search of the code did not find other cases where these teams
are needed. I decided not to create a method on IPerson since there would
be only one call site. Added a test that duplicated what I experienced in
production, then shut my eye and imported removeSecurityProxy.
    lib/lp/registry/browser/peoplemerge.py
    lib/lp/registry/browser/tests/test_peoplemerge.py
-- 
https://code.launchpad.net/~sinzui/launchpad/delete-team-4/+merge/43403
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~sinzui/launchpad/delete-team-4 into lp:launchpad.

=== modified file 'lib/lp/registry/browser/peoplemerge.py'
--- lib/lp/registry/browser/peoplemerge.py	2010-12-03 16:33:03 +0000
+++ lib/lp/registry/browser/peoplemerge.py	2010-12-10 22:26:59 +0000
@@ -241,10 +241,19 @@
         # Team email addresses are not transferable.
         self.dupe_person.setContactAddress(None)
         # The registry experts does not want to acquire super teams from a
-        # merge.
+        # merge. This operation requires unrestricted access to ensure
+        # the user who has permission to delete a team can remove the
+        # team from other teams.
         if self.target_person == self.registry_experts:
-            for team in self.dupe_person.teams_participated_in:
-                self.dupe_person.retractTeamMembership(team, self.user)
+            all_super_teams = set(self.dupe_person.teams_participated_in)
+            indirect_super_teams = set(
+                self.dupe_person.teams_indirectly_participated_in)
+            super_teams = all_super_teams - indirect_super_teams
+            from zope.security.proxy import removeSecurityProxy
+            naked_dupe_person = removeSecurityProxy(self.dupe_person)
+            for team in super_teams:
+                naked_dupe_person.retractTeamMembership(team, self.user)
+            del naked_dupe_person
         # We have sent another series of calls to the db, potentially a long
         # sequence depending on the merge. We want everything synced up
         # before proceeding.

=== modified file 'lib/lp/registry/browser/tests/test_peoplemerge.py'
--- lib/lp/registry/browser/tests/test_peoplemerge.py	2010-12-03 16:33:03 +0000
+++ lib/lp/registry/browser/tests/test_peoplemerge.py	2010-12-10 22:26:59 +0000
@@ -15,6 +15,7 @@
 from lp.registry.interfaces.person import IPersonSet
 from lp.registry.interfaces.mailinglist import MailingListStatus
 from lp.testing import (
+    celebrity_logged_in,
     login_celebrity,
     login_person,
     person_logged_in,
@@ -141,8 +142,7 @@
         self.assertEqual([], view.errors)
         self.assertEqual(self.target_team, self.dupe_team.merged)
 
-    def test_merge_team_as_delete_with_super_teams_into_registry_experts(
-            self):
+    def test_owner_delete_team_with_super_teams(self):
         # Super team memberships are removed.
         self.target_team = getUtility(ILaunchpadCelebrities).registry_experts
         super_team = self.factory.makeTeam()
@@ -152,3 +152,13 @@
         view = self.getView()
         self.assertEqual([], view.errors)
         self.assertEqual(self.target_team, self.dupe_team.merged)
+
+    def test_registry_delete_team_with_super_teams(self):
+        # Registry admins can delete teams with super team memberships.
+        self.target_team = getUtility(ILaunchpadCelebrities).registry_experts
+        super_team = self.factory.makeTeam()
+        with celebrity_logged_in('admin'):
+            self.dupe_team.join(super_team, super_team.teamowner)
+        view = self.getView()
+        self.assertEqual([], view.errors)
+        self.assertEqual(self.target_team, self.dupe_team.merged)