Merge lp:~edwin-grubbs/launchpad/bug-506965-merge-team-oops into lp:launchpad

[Edwin Grubbs]

[Edwin Grubbs (edwin-grubbs)]

Summary
-------

Fixed oops caused when a registry expert (non-admin) merges a team with an email address into another team.

Implementation details
----------------------

Set email status after removing security adapter to give the registry expert access in this special case.

Tests
-----

./bin/test -vv -t xx-adminteammerge.txt

Demo and Q/A
------------

* Add ~no-priv to the ~registry team.
* Login as ~no-priv.
* Open http://launchpad.dev/people/+newteam
  * Add a team with a contact email address.
  * You will need to verify the email address with the token it emails you. (On launchpad.dev, you can just look in the logintoken table and paste the token into https://launchpad.dev/token/$token)
* Open http://launchpad.dev/people/
  * Click on "Merge teams"
  * Enter you new team for the first team.
  * Enter "ubuntu-team" for the second team.
  * Click "Merge"
  * When you get a warning message, click "Deactivate members and merge".

[Curtis Hovey (sinzui)]

Hi Edwin.

There are some dangerous changes in the code and the test does is not what is happening in production.

We need to revert the permissions on the view. It must be admin because we do not want every team owner/admin permission to merge their team with *any* other team. Delete restricts the operation to the context team and ~registry.

> Demo and Q/A
> ------------
>
> * Add ~no-priv to the ~registry team.
> * Login as ~no-priv.
> * Open http://launchpad.dev/people/+newteam
> * Add a team with a contact email address.
> * You will need to verify the email address with the token it emails you.
> (On launchpad.dev, you can just look in the logintoken table and paste the
> token into https://launchpad.dev/token/$token)
> * Open http://launchpad.dev/people/
> * Click on "Merge teams"
> * Enter you new team for the first team.
> * Enter "ubuntu-team" for the second team.
> * Click "Merge"
> * When you get a warning message, click "Deactivate members and merge".

This story is wrong. The delete team scenario is a very restricted merge operation that registry administators may perform. Admins can do unrestricted merges.
    * As registry expert.
    * Visit https://edge.launchpad.net/~consulvision
      * This team has an email address in NEW status
      * team email addresses are not shown in the UI
      * only the team owner can access the address.
    * Choose Delete team.
    * Verify the team is gone.

The code change is the the view, so we want a test that verifies the view handles the real conditions. The test should be in lp/registry/tests/peoplemerge-views.txt. You can update the the Delete team section to cover this story
    * Create a team with an email address.
    * deletable_.setContactAddress(None)
    * login_person(registry_experts.teamowner)
    * form = {'field.actions.delete': 'Delete'}
    * view = create_initialized_view(deletable_team, '+delete', form=form)
    * view.errors
      []
    * for notification in view.request.response.notifications:
         print notification.message
      Team deleted.

review: Needs Fixing (code)

[Curtis Hovey (sinzui)]

> * Create a team with an email address.
> * deletable_.setContactAddress(None)

I think I meant deletable_team.

[Edwin Grubbs (edwin-grubbs)]

> Hi Edwin.
>
> There are some dangerous changes in the code and the test does is not what is
> happening in production.
>
> We need to revert the permissions on the view. It must be admin because we do
> not want every team owner/admin permission to merge their team with *any*
> other team. Delete restricts the operation to the context team and ~registry.

Hi Sinzui,

Thanks for the review. As discussed on IRC, the permissions for the view and for the +adminmergeteam link are fine since it uses launchpad.Moderate on IPersonSet, which does not include admins of any team.

> > Demo and Q/A
> > ------------
> >
> > * Add ~no-priv to the ~registry team.
> > * Login as ~no-priv.
> > * Open http://launchpad.dev/people/+newteam
> > * Add a team with a contact email address.
> > * You will need to verify the email address with the token it emails you.
> > (On launchpad.dev, you can just look in the logintoken table and paste the
> > token into https://launchpad.dev/token/$token)
> > * Open http://launchpad.dev/people/
> > * Click on "Merge teams"
> > * Enter you new team for the first team.
> > * Enter "ubuntu-team" for the second team.
> > * Click "Merge"
> > * When you get a warning message, click "Deactivate members and merge".
>
> This story is wrong. The delete team scenario is a very restricted merge
> operation that registry administators may perform. Admins can do unrestricted
> merges.
> * As registry expert.
> * Visit https://edge.launchpad.net/~consulvision
> * This team has an email address in NEW status
> * team email addresses are not shown in the UI
> * only the team owner can access the address.
> * Choose Delete team.
> * Verify the team is gone.
>
> The code change is the the view, so we want a test that verifies the view
> handles the real conditions. The test should be in lp/registry/tests
> /peoplemerge-views.txt. You can update the the Delete team section to cover
> this story
> * Create a team with an email address.
> * deletable_.setContactAddress(None)
> * login_person(registry_experts.teamowner)
> * form = {'field.actions.delete': 'Delete'}
> * view = create_initialized_view(deletable_team, '+delete', form=form)
> * view.errors
> []
> * for notification in view.request.response.notifications:
> print notification.message
> Team deleted.

I have added this test. I also changed other parts of the test that was using the owner of the registry team, which is mark. Since mark is also in the admin team, I don't think it is a very effective test of the registry experts abilities to use him.

-Edwin

Incremental diff:
{{{
=== modified file 'lib/lp/registry/browser/tests/peoplemerge-views.txt'
--- lib/lp/registry/browser/tests/peoplemerge-views.txt 2010-01-06 17:52:45 +0000
+++ lib/lp/registry/browser/tests/peoplemerge-views.txt 2010-01-19 23:53:42 +0000
@@ -6,9 +6,17 @@
 Team Merges
 -----------

+Create a member of the registry team that is not a member of the admins
+team.
+
     >>> from lp.registry.interfaces.person import IPersonSet
+ >>> from canonical.launchpad.interfaces import ILaunchpadCelebrities
+ >>> registry_e...

[Curtis Hovey (sinzui)]

Hi Edwin.

Thank you for correcting the registry-experts test. I am sure something
bad would have happened using a super-powerful moderator. This did
happen to me in a test for mirror admins, which user Mark owns.

I have one small suggestion for the test.

  review approve

On Wed, 2010-01-20 at 00:02 +0000, Edwin Grubbs wrote:
> Incremental diff:
> {{{
> === modified file 'lib/lp/registry/browser/tests/peoplemerge-views.txt'
> --- lib/lp/registry/browser/tests/peoplemerge-views.txt 2010-01-06 17:52:45 +0000
> +++ lib/lp/registry/browser/tests/peoplemerge-views.txt 2010-01-19 23:53:42 +0000
> @@ -6,9 +6,17 @@
> Team Merges
> -----------
...

> @@ -39,6 +47,7 @@
> >>> parent_team = factory.makeTeam()
> >>> child_team = factory.makeTeam(name='child-team')
> >>> random_team = factory.makeTeam()
> + >>> login('<email address hidden>')

Use login('<email address hidden>') so that the role is clear.

...

--
__Curtis C. Hovey_________
http://launchpad.net/

review: Approve

=== modified file 'lib/lp/registry/browser/menu.py'
--- lib/lp/registry/browser/menu.py	2009-08-21 20:04:25 +0000
+++ lib/lp/registry/browser/menu.py	2010-01-20 00:01:15 +0000
@@ -60,7 +60,7 @@
         text = 'Merge people'
         return Link('/people/+adminpeoplemerge', text, icon='edit')
 
-    @enabled_with_permission('launchpad.Admin')
+    @enabled_with_permission('launchpad.Moderate')
     def admin_merge_teams(self):
         text = 'Merge teams'
         return Link('/people/+adminteammerge', text, icon='edit')

=== modified file 'lib/lp/registry/browser/peoplemerge.py'
--- lib/lp/registry/browser/peoplemerge.py	2010-01-12 00:19:20 +0000
+++ lib/lp/registry/browser/peoplemerge.py	2010-01-20 00:01:15 +0000
@@ -140,15 +140,15 @@
         from zope.security.proxy import removeSecurityProxy
         for email in self.dupe_person_emails:
             email = IMasterObject(email)
-            # XXX: Guilherme Salgado 2007-10-15: Maybe this status change
-            # should be done only when merging people but not when merging
-            # teams.
-            email.status = EmailAddressStatus.NEW
             # EmailAddress.person and EmailAddress.account are readonly
             # fields, so we need to remove the security proxy here.
             naked_email = removeSecurityProxy(email)
             naked_email.personID = self.target_person.id
             naked_email.accountID = self.target_person.accountID
+            # XXX: Guilherme Salgado 2007-10-15: Maybe this status change
+            # should be done only when merging people but not when merging
+            # teams.
+            naked_email.status = EmailAddressStatus.NEW
         flush_database_updates()
         getUtility(IPersonSet).merge(self.dupe_person, self.target_person)
         self.request.response.addInfoNotification(self.merge_message)

=== modified file 'lib/lp/registry/browser/tests/peoplemerge-views.txt'
--- lib/lp/registry/browser/tests/peoplemerge-views.txt	2010-01-06 17:52:45 +0000
+++ lib/lp/registry/browser/tests/peoplemerge-views.txt	2010-01-20 00:01:15 +0000
@@ -6,9 +6,17 @@
 Team Merges
 -----------
 
+Create a member of the registry team that is not a member of the admins
+team.
+
     >>> from lp.registry.interfaces.person import IPersonSet
+    >>> from canonical.launchpad.interfaces import ILaunchpadCelebrities
+    >>> registry_experts = getUtility(ILaunchpadCelebrities).registry_experts
     >>> person_set = getUtility(IPersonSet)
-    >>> registry_expert = person_set.getByName('mark')
+    >>> registry_expert = factory.makePerson()
+    >>> login_person(registry_experts.teamowner)
+    >>> ignored = registry_experts.addMember(
+    ...     registry_expert, registry_experts.teamowner)
     >>> login_person(registry_expert)
 
 A team (name21) can be merged into another (ubuntu-team).
@@ -39,6 +47,7 @@
     >>> parent_team = factory.makeTeam()
     >>> child_team = factory.makeTeam(name='child-team')
     >>> random_team = factory.makeTeam()
+    >>> login('[e-mail]')
     >>> ignored = parent_team.addMember(
     ...     child_team, reviewer=parent_team.teamowner, force_team_add=True)
     >>> form = {'field.dupe_person': child_team.name,
@@ -112,10 +121,8 @@
 Users with launchpad.Moderate such as team admins and registry experts
 can delete teams.
 
-    >>> from canonical.launchpad.interfaces import ILaunchpadCelebrities
     >>> from canonical.launchpad.webapp.authorization import check_permission
 
-    >>> registry_experts = getUtility(ILaunchpadCelebrities).registry_experts
     >>> team_owner = factory.makePerson()
     >>> team_member = factory.makePerson()
     >>> deletable_team = factory.makeTeam(owner=team_owner, name='deletable')
@@ -127,7 +134,7 @@
     >>> check_permission('launchpad.Moderate', view)
     False
 
-    >>> login_person(registry_experts.teamowner)
+    >>> login_person(registry_expert)
     >>> check_permission('launchpad.Moderate', view)
     True
 
@@ -241,3 +248,26 @@
 
     >>> print find_tag_by_id(content, 'field.actions.delete')
     None
+
+The registry experts should be able to delete a team with an
+validated email address, which will be invisible, since only
+preferred email addresses are shown for teams.
+
+    >>> from canonical.launchpad.interfaces.emailaddress import (
+    ...     IEmailAddressSet)
+    >>> login_person(registry_expert)
+    >>> admin = getUtility(ILaunchpadCelebrities).admin
+    >>> registry_expert.inTeam(admin)
+    False
+    >>> deletable_team = factory.makeTeam(email="[e-mail]")
+    >>> deletable_team.setContactAddress(None)
+    >>> for email in getUtility(IEmailAddressSet).getByPerson(deletable_team):
+    ...     print email.email, email.status.title
+    [e-mail] Validated Email Address
+    >>> form = {'field.actions.delete': 'Delete'}
+    >>> view = create_initialized_view(deletable_team, '+delete', form=form)
+    >>> view.errors
+    []
+    >>> for notification in view.request.response.notifications:
+    ...     print notification.message
+    Team deleted.

=== modified file 'lib/lp/registry/stories/team/xx-adminteammerge.txt'
--- lib/lp/registry/stories/team/xx-adminteammerge.txt	2009-11-22 15:43:16 +0000
+++ lib/lp/registry/stories/team/xx-adminteammerge.txt	2010-01-20 00:01:15 +0000
@@ -4,30 +4,39 @@
 active members, so the user will first have to confirm that the
 members should be deactivated before the teams are merged.
 
-    >>> admin_browser.open('http://launchpad.dev/people/+adminteammerge')
-    >>> admin_browser.getControl('Duplicated Team').value = (
-    ...     'landscape-developers')
-    >>> admin_browser.getControl('Target Team').value = 'guadamen'
-    >>> admin_browser.getControl('Merge').click()
+    >>> from zope.component import getUtility
+    >>> from lp.registry.interfaces.person import IPersonSet
+    >>> login('[e-mail]')
+    >>> registry_expert = factory.makePerson(
+    ...     email='[e-mail]', password='test')
+    >>> new_team = factory.makeTeam(
+    ...     name="new-team", email="[e-mail]")
+    >>> registry_experts = getUtility(IPersonSet).getByName('registry')
+    >>> ignored = registry_experts.addMember(
+    ...     registry_expert, registry_experts.teamowner)
+    >>> logout()
+    >>> registry_browser = setupBrowser(auth='Basic [e-mail]:test')
+    >>> registry_browser.open('http://launchpad.dev/people/+adminteammerge')
+    >>> registry_browser.getControl('Duplicated Team').value = (
+    ...     'new-team')
+    >>> registry_browser.getControl('Target Team').value = 'guadamen'
+    >>> registry_browser.getControl('Merge').click()
 
-    >>> admin_browser.url
+    >>> registry_browser.url
     'http://launchpad.dev/people/+adminteammerge'
-    >>> print get_feedback_messages(admin_browser.contents)[0]
-    Landscape Developers has 2 active members which will have to be
-    deactivated before the teams can be merged.
+    >>> print get_feedback_messages(registry_browser.contents)[0]
+    New Team has 1 active members which will have to be deactivated
+    before the teams can be merged.
 
-    >>> admin_browser.getControl('Deactivate Members and Merge').click()
-    >>> admin_browser.url
+    >>> registry_browser.getControl('Deactivate Members and Merge').click()
+    >>> registry_browser.url
     'http://launchpad.dev/~guadamen'
 
     >>> from canonical.launchpad.ftests import ANONYMOUS, login, logout
-    >>> from canonical.launchpad.interfaces import IMailingListSet, IPersonSet
+    >>> from canonical.launchpad.interfaces import IMailingListSet
     >>> from zope.component import getUtility
     >>> login(ANONYMOUS)
-    >>> person_set = getUtility(IPersonSet)
-    >>> landscape = person_set.getByName(
-    ...     'landscape-developers-merged', ignore_merged=False)
-    >>> landscape.merged.name
+    >>> new_team.merged.name
     u'guadamen'
     >>> logout()
 
@@ -36,7 +45,7 @@
 
 Merged teams are not visible anymore.
 
-    >>> browser.open("http://launchpad.dev/~landscape-developers-merged")
+    >>> browser.open("http://launchpad.dev/~new-team-merged")
     Traceback (most recent call last):
     ...
     NotFound:...
@@ -48,45 +57,45 @@
 merged, though.
 
     >>> login(ANONYMOUS)
-    >>> guadamen = person_set.getByName('guadamen')
+    >>> guadamen = getUtility(IPersonSet).getByName('guadamen')
     >>> mailing_list = getUtility(IMailingListSet).new(guadamen)
     >>> logout()
 
-    >>> admin_browser.open('http://launchpad.dev/people/+adminteammerge')
-    >>> admin_browser.getControl('Duplicated Team').value = 'guadamen'
-    >>> admin_browser.getControl('Target Team').value = 'ubuntu-team'
-    >>> admin_browser.getControl('Merge').click()
+    >>> registry_browser.open('http://launchpad.dev/people/+adminteammerge')
+    >>> registry_browser.getControl('Duplicated Team').value = 'guadamen'
+    >>> registry_browser.getControl('Target Team').value = 'ubuntu-team'
+    >>> registry_browser.getControl('Merge').click()
 
-    >>> admin_browser.url
+    >>> registry_browser.url
     'http://launchpad.dev/people/+adminteammerge'
 
-    >>> print get_feedback_messages(admin_browser.contents)
+    >>> print get_feedback_messages(registry_browser.contents)
     [u'There is 1 error.',
      u"guadamen is associated with a Launchpad mailing list;
        we can't merge it."]
 
 We also can't (for obvious reasons) merge any person/team into itself.
 
-    >>> admin_browser.getControl('Duplicated Team').value = 'shipit-admins'
-    >>> admin_browser.getControl('Target Team').value = 'shipit-admins'
-    >>> admin_browser.getControl('Merge').click()
+    >>> registry_browser.getControl('Duplicated Team').value = 'shipit-admins'
+    >>> registry_browser.getControl('Target Team').value = 'shipit-admins'
+    >>> registry_browser.getControl('Merge').click()
 
-    >>> admin_browser.url
+    >>> registry_browser.url
     'http://launchpad.dev/people/+adminteammerge'
 
-    >>> print get_feedback_messages(admin_browser.contents)
+    >>> print get_feedback_messages(registry_browser.contents)
     [u'There is 1 error.', u"You can't merge shipit-admins into itself."]
 
 Nor can we merge a person into a team or a team into a person.
 
-    >>> admin_browser.getControl('Duplicated Team').value = 'ubuntu-team'
-    >>> admin_browser.getControl('Target Team').value = 'salgado'
-    >>> admin_browser.getControl('Merge').click()
+    >>> registry_browser.getControl('Duplicated Team').value = 'ubuntu-team'
+    >>> registry_browser.getControl('Target Team').value = 'salgado'
+    >>> registry_browser.getControl('Merge').click()
 
-    >>> admin_browser.url
+    >>> registry_browser.url
     'http://launchpad.dev/people/+adminteammerge'
 
     # Yes, the error message is not of much help, but this UI is only for
     # admins and they're supposed to know what they're doing.
-    >>> print get_feedback_messages(admin_browser.contents)
+    >>> print get_feedback_messages(registry_browser.contents)
     [u'There is 1 error.', u'Constraint not satisfied']