Merge lp:~sinzui/launchpad/closed-teams-1 into lp:launchpad

[Curtis Hovey]

Do not permit teams to be compromised by changes to the subscription policy.

    Launchpad bug:
        https://bugs.launchpad.net/bugs/662844
        https://bugs.launchpad.net/bugs/654476
    Pre-implementation: jcsacket, gary, benji, flacoste, bac, bigjools
    Test command: ./bin/test -vv -t TestTeamSubscriptionPolicy

A restricted or moderated team is effectively an open team if it has an open
team as a member.

The branch that updated the picker fixed the first issue in this bug -- closed
teams cannot add an open team as a member or make an open team team the owner.
The remaining issue is to guard changes to the subscription policy:

  * A closed team cannot become open if it is a member of another closed team
  * An open team cannot become closed if it has an open team as a member

The guard only needs to check the immediate super teams or sub teams, since
the deeper teams in the hierarchy must also conform to these rules.

ADDENDUM

Open teams cannot have PPAs, only vetted members may upload to the archive.

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

RULES

    * Add TeamSubscriptionPolicyError when a subscription policy change is
      not permitted
    * Add a validator to check that changes to a team's subscription policies
      are enforced.
      1. Closed teams cannot become open if they are members of closed team.
         Rule 1 means a closed team cannot become open if it is a member of
         any team
      2. Open teams cannot become closed if it has any direct or indirect
         open teams as members.
    * The validator can raise TeamSubscriptionPolicyError when the state is
      invalid. it is also: webservice_error(httplib.FORBIDDEN)
    * Use the validator when subscriptionpolicy is changed.
      * A field validator is needed to ensure api changes are sane
    * Add a guard to prevent closed teams with PPAs from becoming open.

QA

    * Try to change ~launchpad to open (rule 1)
    * Try to change an open team with open teams to closed (rule 2)
    * Try to change a team with a PPA to an open team.

LINT

    lib/lp/registry/errors.py
    lib/lp/registry/interfaces/person.py
    lib/lp/registry/tests/test_team.py

IMPLEMENTATION

Added TeamSubscriptionPolicyError that will show the specific error message
in the UI. Created TeamSubsciptionPolicyChoice that enforces both the
vocabulary and the constraints imposed by team memberships and ppas. The
contraint work is actually done by a helper function called
team_subscription_policy_can_transition.
    lib/lp/registry/errors.py
    lib/lp/registry/interfaces/person.py
    lib/lp/registry/tests/test_team.py

[Brad Crittenden (bac)]

Hi Curtis,

27 + The error can be raised because a super team or member team prevents
28 + the this team from setting a specific policy. The error can also be
29 + raised if the team has an active PPA.

s/the this/this/

There is a logic problem we discussed on IRC
elif policy != TeamSubscriptionPolicy.OPEN:
should be
elif team.subscriptionpolicy != TeamSubscriptionPolicy.OPEN:

In your tests, I think it would be clearer if your two teams were parent_team and child_team. I find other_team switching roles based on the test to be confusing.

Otherwise it looks good. I'll mark it approved since we discussed the changes you'll make.

Thanks for the fix and killing two bugs at once.

review: Approve

=== modified file 'lib/lp/registry/errors.py'
--- lib/lp/registry/errors.py	2010-11-11 11:55:53 +0000
+++ lib/lp/registry/errors.py	2010-12-10 20:58:39 +0000
@@ -18,6 +18,7 @@
     'PPACreationError',
     'PrivatePersonLinkageError',
     'TeamMembershipTransitionError',
+    'TeamSubscriptionPolicyError',
     'UserCannotChangeMembershipSilently',
     'UserCannotSubscribePerson',
     ]
@@ -25,6 +26,7 @@
 import httplib
 
 from lazr.restful.declarations import webservice_error
+from zope.schema.interfaces import ConstraintNotSatisfied
 from zope.security.interfaces import Unauthorized
 
 from lp.app.errors import NameLookupFailed
@@ -132,6 +134,30 @@
     webservice_error(httplib.BAD_REQUEST)
 
 
+class TeamSubscriptionPolicyError(ConstraintNotSatisfied):
+    """The team cannot have the specified TeamSubscriptionPolicy.
+
+    The error can be raised because a super team or member team prevents
+    this team from setting a specific policy. The error can also be
+    raised if the team has an active PPA.
+    """
+    webservice_error(httplib.BAD_REQUEST)
+
+    _default_message = "Team Subscription Policy Error"
+
+    def __init__(self, message=None):
+        if message is None:
+            message = self._default_message
+        self.message = message
+
+    def doc(self):
+        """See `Invalid`."""
+        return self.message
+
+    def __str__(self):
+        return self.message
+
+
 class JoinNotAllowed(Exception):
     """User is not allowed to join a given team."""
     webservice_error(httplib.BAD_REQUEST)

=== modified file 'lib/lp/registry/interfaces/person.py'
--- lib/lp/registry/interfaces/person.py	2010-12-10 07:52:21 +0000
+++ lib/lp/registry/interfaces/person.py	2010-12-10 20:58:39 +0000
@@ -114,7 +114,10 @@
     IHasRequestedReviews,
     )
 from lp.code.interfaces.hasrecipes import IHasRecipes
-from lp.registry.errors import PrivatePersonLinkageError
+from lp.registry.errors import (
+    PrivatePersonLinkageError,
+    TeamSubscriptionPolicyError,
+    )
 from lp.registry.interfaces.gpg import IGPGKey
 from lp.registry.interfaces.irc import IIrcID
 from lp.registry.interfaces.jabber import IJabberID
@@ -146,6 +149,7 @@
     StrippedTextLine,
     )
 from lp.services.worlddata.interfaces.language import ILanguage
+from lp.soyuz.enums import ArchiveStatus
 from lp.translations.interfaces.hastranslationimports import (
     IHasTranslationImports,
     )
@@ -483,6 +487,80 @@
         super(PersonNameField, self)._validate(input)
 
 
+def team_subscription_policy_can_transition(team, policy):
+    """Can the team can change its subscription policy
+
+    Returns True when the policy can change. or raises an error. OPEN teams
+    cannot be members of MODERATED or RESTRICTED teams. OPEN teams
+    cannot have PPAs. Changes from between OPEN and the two closed states
+    can be blocked by team membership and team artifacts.
+
+    :param team: The team to change.
+    :param policy: The TeamSubsciptionPolicy to change to.
+    :raises TeamSubsciptionPolicyError: Raised when a membership constrain
+        or a team artifact prevents the policy from being set.
+    """
+    if team is None or policy == team.subscriptionpolicy:
+        # The team is being initialized or the policy is not changing.
+        return True
+    elif policy == TeamSubscriptionPolicy.OPEN:
+        # The team can be open if its super teams are open.
+        for team in team.super_teams:
+            if team.subscriptionpolicy != TeamSubscriptionPolicy.OPEN:
+                raise TeamSubscriptionPolicyError(
+                    "The team subscription policy cannot be %s because one "
+                    "or more if its super teams are not open." % policy)
+        # The team can be open if it has PPAs.
+        for ppa in team.ppas:
+            if ppa.status != ArchiveStatus.DELETED:
+                raise TeamSubscriptionPolicyError(
+                    "The team subscription policy cannot be %s because it "
+                    "has one or more active PPAs." % policy)
+    elif team.subscriptionpolicy == TeamSubscriptionPolicy.OPEN:
+        # The team can become MODERATED or RESTRICTED if its member teams
+        # are not OPEN.
+        for member in team.activemembers:
+            if member.subscriptionpolicy == TeamSubscriptionPolicy.OPEN:
+                raise TeamSubscriptionPolicyError(
+                    "The team subscription policy cannot be %s because one "
+                    "or more if its member teams are Open." % policy)
+    else:
+        # The policy change is between MODERATED and RESTRICTED.
+        pass
+    return True
+
+
+class TeamSubsciptionPolicyChoice(Choice):
+    """A valid team subscription policy."""
+
+    def _getTeam(self):
+        """Return the context if it is a team or None."""
+        if IPerson.providedBy(self.context):
+            return self.context
+        else:
+            return None
+
+    def constraint(self, value):
+        """See `IField`."""
+        team = self._getTeam()
+        policy = value
+        try:
+            return team_subscription_policy_can_transition(team, policy)
+        except TeamSubscriptionPolicyError:
+            return False
+
+    def _validate(self, value):
+        """Ensure the TeamSubsciptionPolicy is valid for state of the team.
+
+        Returns True if the team can change its subscription policy to the
+        `TeamSubscriptionPolicy`, otherwise raise TeamSubscriptionPolicyError.
+        """
+        team = self._getTeam()
+        policy = value
+        team_subscription_policy_can_transition(team, policy)
+        super(TeamSubsciptionPolicyChoice, self)._validate(value)
+
+
 class IPersonClaim(Interface):
     """The schema used by IPerson's +claim form."""
 
@@ -1688,7 +1766,7 @@
         exported_as='team_description')
 
     subscriptionpolicy = exported(
-        Choice(title=_('Subscription policy'),
+        TeamSubsciptionPolicyChoice(title=_('Subscription policy'),
                vocabulary=TeamSubscriptionPolicy,
                default=TeamSubscriptionPolicy.MODERATED, required=True,
                description=_(

=== modified file 'lib/lp/registry/tests/test_team.py'
--- lib/lp/registry/tests/test_team.py	2010-12-01 17:07:49 +0000
+++ lib/lp/registry/tests/test_team.py	2010-12-10 20:58:39 +0000
@@ -8,19 +8,27 @@
 import transaction
 from zope.component import getUtility
 from zope.interface.exceptions import Invalid
+from zope.security.proxy import removeSecurityProxy
 
 from canonical.launchpad.database.emailaddress import EmailAddress
 from canonical.launchpad.interfaces.emailaddress import IEmailAddressSet
 from canonical.launchpad.interfaces.lpstorm import IMasterStore
-from canonical.testing.layers import DatabaseFunctionalLayer
+from canonical.testing.layers import (
+    DatabaseFunctionalLayer,
+    FunctionalLayer,
+    )
 from lp.registry.enum import PersonTransferJobType
+from lp.registry.errors import TeamSubscriptionPolicyError
 from lp.registry.interfaces.mailinglist import MailingListStatus
 from lp.registry.interfaces.person import (
+    IPersonSet,
     ITeamPublic,
     PersonVisibility,
     TeamMembershipRenewalPolicy,
+    TeamSubscriptionPolicy,
     )
 from lp.registry.model.persontransferjob import PersonTransferJob
+from lp.soyuz.enums import ArchiveStatus
 from lp.testing import (
     login_celebrity,
     login_person,
@@ -191,6 +199,132 @@
         ITeamPublic['defaultmembershipperiod'].validate(3650)
 
 
+class TestTeamSubscriptionPolicyError(TestCaseWithFactory):
+    """Test `TeamSubscriptionPolicyError` messages."""
+
+    layer = FunctionalLayer
+
+    def test_default_message(self):
+        error = TeamSubscriptionPolicyError()
+        self.assertEqual('Team Subscription Policy Error', error.message)
+
+    def test_str(self):
+        # The string is the error message.
+        error = TeamSubscriptionPolicyError('a message')
+        self.assertEqual('a message', str(error))
+
+    def test_doc(self):
+        # The doc() method returns the message.  It is called when rendering
+        # an error in the UI. eg structure error.
+        error = TeamSubscriptionPolicyError('a message')
+        self.assertEqual('a message', error.doc())
+
+
+class TestTeamSubscriptionPolicyChoice(TestCaseWithFactory):
+    """Test `TeamSubsciptionPolicyChoice` constraints."""
+
+    layer = DatabaseFunctionalLayer
+
+    def setUpTeams(self, policy, other_policy=None):
+        if other_policy is None:
+            other_policy = policy
+        self.team = self.factory.makeTeam(subscription_policy=policy)
+        self.other_team = self.factory.makeTeam(
+            subscription_policy=other_policy, owner=self.team.teamowner)
+        self.field = ITeamPublic['subscriptionpolicy'].bind(self.team)
+        login_person(self.team.teamowner)
+
+    def test___getTeam_with_team(self):
+        # _getTeam returns the context team for team updates.
+        self.setUpTeams(TeamSubscriptionPolicy.MODERATED)
+        self.assertEqual(self.team, self.field._getTeam())
+
+    def test___getTeam_with_person_set(self):
+        # _getTeam returns the context person set for team creation.
+        person_set = getUtility(IPersonSet)
+        field = ITeamPublic['subscriptionpolicy'].bind(person_set)
+        self.assertEqual(None, field._getTeam())
+
+    def test_closed_team_with_closed_super_team_cannot_become_open(self):
+        # The team cannot compromise the membership of the super team
+        # by becoming open. The user must remove his team from the super team
+        # first.
+        self.setUpTeams(TeamSubscriptionPolicy.MODERATED)
+        self.other_team.addMember(self.team, self.team.teamowner)
+        self.assertFalse(
+            self.field.constraint(TeamSubscriptionPolicy.OPEN))
+        self.assertRaises(
+            TeamSubscriptionPolicyError, self.field.validate,
+            TeamSubscriptionPolicy.OPEN)
+
+    def test_closed_team_with_open_super_team_can_become_open(self):
+        # The team can become open if its super teams are open.
+        self.setUpTeams(
+            TeamSubscriptionPolicy.MODERATED, TeamSubscriptionPolicy.OPEN)
+        self.other_team.addMember(self.team, self.team.teamowner)
+        self.assertTrue(
+            self.field.constraint(TeamSubscriptionPolicy.OPEN))
+        self.assertEqual(
+            None, self.field.validate(TeamSubscriptionPolicy.OPEN))
+
+    def test_open_team_with_open_sub_team_cannot_become_closed(self):
+        # The team cannot become closed if its membership will be
+        # compromised by an open subteam. The user must remove the subteam
+        # first
+        self.setUpTeams(TeamSubscriptionPolicy.OPEN)
+        self.team.addMember(self.other_team, self.team.teamowner)
+        self.assertFalse(
+            self.field.constraint(TeamSubscriptionPolicy.MODERATED))
+        self.assertRaises(
+            TeamSubscriptionPolicyError, self.field.validate,
+            TeamSubscriptionPolicy.MODERATED)
+
+    def test_closed_team_can_change_to_another_closed_policy(self):
+        # A closed team can change between the two closed polcies.
+        self.setUpTeams(TeamSubscriptionPolicy.MODERATED)
+        self.team.addMember(self.other_team, self.team.teamowner)
+        super_team = self.factory.makeTeam(
+            subscription_policy=TeamSubscriptionPolicy.MODERATED,
+            owner=self.team.teamowner)
+        super_team.addMember(self.team, self.team.teamowner)
+        self.assertTrue(
+            self.field.constraint(TeamSubscriptionPolicy.RESTRICTED))
+        self.assertEqual(
+            None, self.field.validate(TeamSubscriptionPolicy.RESTRICTED))
+
+    def test_open_team_with_closed_sub_team_can_become_closed(self):
+        # The team can become closed.
+        self.setUpTeams(
+            TeamSubscriptionPolicy.OPEN, TeamSubscriptionPolicy.MODERATED)
+        self.team.addMember(self.other_team, self.team.teamowner)
+        self.assertTrue(
+            self.field.constraint(TeamSubscriptionPolicy.MODERATED))
+        self.assertEqual(
+            None, self.field.validate(TeamSubscriptionPolicy.MODERATED))
+
+    def test_closed_team_with_active_ppas_cannot_become_open(self):
+        # The team cannot become open if it has PPA because it compromises the
+        # the control of who can upload.
+        self.setUpTeams(TeamSubscriptionPolicy.MODERATED)
+        self.team.createPPA()
+        self.assertFalse(
+            self.field.constraint(TeamSubscriptionPolicy.OPEN))
+        self.assertRaises(
+            TeamSubscriptionPolicyError, self.field.validate,
+            TeamSubscriptionPolicy.OPEN)
+
+    def test_closed_team_without_active_ppas_can_become_open(self):
+        # The team can become if it has deleted PPAs.
+        self.setUpTeams(TeamSubscriptionPolicy.MODERATED)
+        ppa = self.team.createPPA()
+        ppa.delete(self.team.teamowner)
+        removeSecurityProxy(ppa).status = ArchiveStatus.DELETED
+        self.assertTrue(
+            self.field.constraint(TeamSubscriptionPolicy.OPEN))
+        self.assertEqual(
+            None, self.field.validate(TeamSubscriptionPolicy.OPEN))
+
+
 class TestVisibilityConsistencyWarning(TestCaseWithFactory):
 
     layer = DatabaseFunctionalLayer