Merge lp:~sinzui/launchpad/delete-team-4 into lp:launchpad

[Curtis Hovey]

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

[Robert Collins (lifeless)]

I think adding a method would be better for several reasons:
 - it would allow using APIs to manage teams (e.g. for
sysadmins/registry experts)
 - the retractTeamMembership calls should be in model code anyway.

[Curtis Hovey (sinzui)]

Previous conversations concluded registry admins do not have permission to manage membership.

[Robert Collins (lifeless)]

On Mon, Dec 13, 2010 at 8:09 AM, Curtis Hovey
<email address hidden> wrote:
> Previous conversations concluded registry admins do not have permission to manage membership.

Well, sysadmins certainly do, and scripts can be useful. It will also
make for better layers and more efficient tests if all the domain code
is in model/.

-Rob

[Curtis Hovey (sinzui)]

Admins and team owner can use a script to remove super team now. The issues is that admins takes days to do this. Helping users manage their data is not the admin'a primary responsibility. Owners often need assistance, or in cases where teams are reorganising themselves, there is not user as an owner who can do this.

This really is the domain of registry admins, but Francis has stated he does not want this power to be in the hands of many people, even if they are trusted. Launchpad engineers are too many to be trusted. We instead use the UI to help registry admins do they job. This is such as case. deletion is an invasive action that will always involve access to other object that the user does not have access too. The UI recognises that the user's right to delete a team, release, milestone, or series it supersedes the right of the owner of the linked object to have such as link.

This issue is a regression. I had power to help users, now I do not. We have conflicts of interest in the concepts of admin, moderate, and edit. I am a moderator, the team owner is an editor. and we do not have a permission that permits us to state that both roles can make the change. And to repeat my earlier remark, the conclusion is that I should not have the power retract memberships, only the power the delete the team.

[Robert Collins (lifeless)]

On Mon, Dec 13, 2010 at 9:17 AM, Curtis Hovey
<email address hidden> wrote:
> This issue is a regression. I had power to help users, now I do not. We have conflicts of interest in the concepts of admin, moderate, and edit. I am a moderator, the team owner is an editor. and we do not have a permission that permits us to state that both roles can make the change. And to repeat my earlier remark, the conclusion is that I should not have the power retract memberships, only the power the delete the team.

Sure, nothing you've said is in conflict with what I'm proposing, is it?

-Rob

[Curtis Hovey (sinzui)]

Your proposal does conflict. I cannot have access to retractTeamMembership. Admins are not asking for script. In fact, they are asking why they are assigned tasks to involving user data--Lp should be run by users and expert teams

[Robert Collins (lifeless)]

On Mon, Dec 13, 2010 at 9:36 AM, Curtis Hovey
<email address hidden> wrote:
> Your proposal does conflict. I cannot have access to retractTeamMembership. Admins are not asking for script. In fact, they are asking why they are assigned tasks to involving user data--Lp should be run by users and expert teams

I haven't proposed that you have access to retractTeamMembership.

-Rob

[Curtis Hovey (sinzui)]

What are you proposing then?

[Robert Collins (lifeless)]

On Mon, Dec 13, 2010 at 10:07 AM, Curtis Hovey
<email address hidden> wrote:
> What are you proposing then?

That the request to delete the team be forwarded to model code, and
there the model code can call retractTeam in confidence, without
needing to remove the security proxy.

E.g. PersonOrTeam.deleteSelf()

There is existing code doing this, spelt slightly differently (it
might be nice to use the same method name, I just don't recall it
offhand).

-Rob

[Curtis Hovey (sinzui)]

Sorry for being obtuse. I now understand your suggestion. Yes we should move these rules into the model. Edwin is landing a more complicated rule at the moment. I will refactor my branch once his changes are in trunk.

The method will behave like other dangerous delete methods--raise an error if the team has mailing-lists or PPAs that the user needs to review and disable separately. The membership removals and auto purging of lists and emails will be managed by the method.

[Henning Eggers (henninge)]

Looks fine to me, except I don't see a reason why removeSecurityProxy should not be imported at the top of the file. Please see if you can move that.
Also, you mentioned on IRC that you wanted to add a comment to the test.

Thanks for the fix! ;)

review: Approve (code)

=== 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-13 16:32:06 +0000
@@ -16,6 +16,7 @@
 
 
 from zope.component import getUtility
+from zope.security.proxy import removeSecurityProxy
 
 from canonical.database.sqlbase import flush_database_updates
 from canonical.launchpad import _
@@ -89,7 +90,6 @@
         logintokenset = getUtility(ILoginTokenSet)
         # Need to remove the security proxy because the dupe account may have
         # hidden email addresses.
-        from zope.security.proxy import removeSecurityProxy
         token = logintokenset.new(
             self.user, login, removeSecurityProxy(email).email,
             LoginTokenType.ACCOUNTMERGE)
@@ -153,7 +153,6 @@
 
     def doMerge(self, data):
         """Merge the two person/team entries specified in the form."""
-        from zope.security.proxy import removeSecurityProxy
         for email in self.dupe_person_emails:
             email = IMasterObject(email)
             # EmailAddress.person and EmailAddress.account are readonly
@@ -241,10 +240,18 @@
         # 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
+            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.
@@ -398,7 +405,6 @@
         assert result_count == 1
         # Need to remove the security proxy because the dupe account may have
         # hidden email addresses.
-        from zope.security.proxy import removeSecurityProxy
         self.dupe_email = removeSecurityProxy(results[0]).email
 
     def render(self):
@@ -446,7 +452,6 @@
             # If the email addresses are hidden we must send a merge request
             # to each of them.  But first we've got to remove the security
             # proxy so we can get to them.
-            from zope.security.proxy import removeSecurityProxy
             email_addresses = [removeSecurityProxy(email).email
                                for email in self.dupeemails]
         else:

=== 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-13 16:32:06 +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,15 @@
         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()
+        # Use admin to avoid the team invitation dance. The Registry admin
+        # is logged back in.
+        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)