Merge lp:~wgrant/launchpad/branch-type-policy-model into lp:launchpad

[William Grant]

Model and basic admin-only UI for branch sharing policies.

This branch adds functional branch sharing policies, and basic admin-only UI to control them, using the column added in lp:~wgrant/launchpad/branch-type-policy-db.

Branch sharing policies replace BranchVisibilityPolicy. A project owner will be able to select one to determine the default and allowed information types for their branches, from the following options (names are terrible so feel free to rewrite them, but at least it works for now):

 ‣ Public: Branches are public unless they contain sensitive security information.
 ‣ Public, can be proprietary: New branches are public, but can be made proprietary later.
 ‣ Proprietary, can be public: New branches are proprietary, but can be made public later. Only people who can see the project's proprietary information can create new branches.
 ‣ Proprietary: Branches are always proprietary. Only people who can see the project's proprietary information can create new branches.

Note that while the text says Proprietary, the implementation still says User Data. This will be changed in a later branch once Proprietary is more widely supported. Branch privacy still falls back to BranchVisibilityPolicy unless Product.branch_sharing_policy is set to a non-null value, which can only currently be achieved by ~registry using Product:+admin, and only when the disclosure.branch_sharing_policy.show_to_admin feature flag is enabled. It'll later be exposed somewhere for project owners, probably on +sharing.

You can see this in action on launchpad.dev:

 - Log in as <email address hidden>.
 - Add ~name12 to ~registry at <https://launchpad.dev/~registry/+addmember>.
 - At <https://launchpad.dev/+feature-rules>, add 'disclosure.branch_sharing_policy.show_to_admin default 0 true'.
 - Log in as <email address hidden> and head over to <https://code.launchpad.dev/landscape>.
 - Set the development focus to '~name12/landscape/feature-x'.
 - Observe the privacy portlet stating default branch privacy.
 - Change the branch sharing policy at <https://launchpad.dev/landscape/+admin>, and the User Data permissions at <https://launchpad.dev/landscape/+sharing>, and observe the change in the project privacy portlet and the available information types on <https://code.launchpad.dev/~name12/landscape/feature-x/+edit>.

I also added an enum and model/interface definition for bug_sharing_policy, but they're as-yet unused.

[Brad Crittenden (bac)]

Hi William,

Thanks for the nice write up for this merge proposal.

I wanted to see your changes so I appreciate the detailed instructions. Unfortunately at the step "Change the branch sharing policy..." I did not see the sharing policy listed. I then logged in as an administrator, saw the extra admin fields, but again did not see the branch sharing parts.

Otherwise your changes look good.

I'm going to abstain until I can verify the minimal UI. If you see a problem with the instructions you gave, or can verify it works for you still, please let me know.

review: Abstain (code)

[Brad Crittenden (bac)]

William I found the problem in my environment -- your instructions were perfect.

The code looks good. Thanks.

review: Approve (code)

=== modified file 'lib/lp/code/model/branchnamespace.py'
--- lib/lp/code/model/branchnamespace.py	2012-07-12 04:36:41 +0000
+++ lib/lp/code/model/branchnamespace.py	2012-07-13 08:51:30 +0000
@@ -19,6 +19,7 @@
 from zope.interface import implements
 from zope.security.proxy import removeSecurityProxy
 
+from lp.app.interfaces.services import IService
 from lp.code.enums import (
     BranchLifecycleStatus,
     BranchSubscriptionDiffSize,
@@ -45,6 +46,7 @@
 from lp.code.interfaces.branchtarget import IBranchTarget
 from lp.code.model.branch import Branch
 from lp.registry.enums import (
+    BranchSharingPolicy,
     InformationType,
     PRIVATE_INFORMATION_TYPES,
     PUBLIC_INFORMATION_TYPES,
@@ -79,6 +81,29 @@
     MAIN_STORE,
     )
 
+POLICY_ALLOWED_TYPES = {
+    BranchSharingPolicy.PUBLIC: PUBLIC_INFORMATION_TYPES,
+    BranchSharingPolicy.PUBLIC_OR_PROPRIETARY:
+        PUBLIC_INFORMATION_TYPES + PRIVATE_INFORMATION_TYPES,
+    BranchSharingPolicy.PROPRIETARY_OR_PUBLIC:
+        PUBLIC_INFORMATION_TYPES + PRIVATE_INFORMATION_TYPES,
+    BranchSharingPolicy.PROPRIETARY: PRIVATE_INFORMATION_TYPES,
+    }
+
+POLICY_DEFAULT_TYPES = {
+    BranchSharingPolicy.PUBLIC: InformationType.PUBLIC,
+    BranchSharingPolicy.PUBLIC_OR_PROPRIETARY: InformationType.PUBLIC,
+    BranchSharingPolicy.PROPRIETARY_OR_PUBLIC: InformationType.USERDATA,
+    BranchSharingPolicy.PROPRIETARY: InformationType.USERDATA,
+    }
+
+POLICY_REQUIRED_GRANTS = {
+    BranchSharingPolicy.PUBLIC: None,
+    BranchSharingPolicy.PUBLIC_OR_PROPRIETARY: None,
+    BranchSharingPolicy.PROPRIETARY_OR_PUBLIC: InformationType.USERDATA,
+    BranchSharingPolicy.PROPRIETARY: InformationType.USERDATA,
+    }
+
 
 class _BaseNamespace:
     """Common code for branch namespaces."""
@@ -340,6 +365,10 @@
         """See `IBranchNamespace`."""
         return IBranchTarget(self.product)
 
+    @property
+    def _using_branchvisibilitypolicy(self):
+        return self.product.branch_sharing_policy is None
+
     def _getRelatedPolicies(self):
         """Return the privacy policies relating to the owner."""
         policies = self.product.getBranchVisibilityTeamPolicies()
@@ -355,6 +384,12 @@
 
     def getPrivacySubscriber(self):
         """See `IBranchNamespace`."""
+        # New branch_sharing_policy-based privacy doesn't
+        # require a privacy subscriber, as branches are shared through
+        # AccessPolicyGrants.
+        if not self._using_branchvisibilitypolicy:
+            return None
+
         # If there is a rule defined for the owner, then there is no privacy
         # subscriber.
         rule = self.product.getBranchVisibilityRuleForTeam(self.owner)
@@ -371,6 +406,24 @@
 
     def getAllowedInformationTypes(self):
         """See `IBranchNamespace`."""
+        if not self._using_branchvisibilitypolicy:
+            # The project uses the new simplified branch_sharing_policy
+            # rules, so check them.
+
+            # Some policies require that the owner have full access to
+            # an information type. If it's required and the owner
+            # doesn't hold it, no information types are legal.
+            required_grant = POLICY_REQUIRED_GRANTS[
+                self.product.branch_sharing_policy]
+            if (required_grant is not None
+                and not getUtility(IService, 'sharing').checkPillarAccess(
+                    self.product, required_grant, self.owner)):
+                return []
+
+            return POLICY_ALLOWED_TYPES[
+                self.product.branch_sharing_policy]
+
+        # The project still uses BranchVisibilityPolicy, so check that.
         private_rules = (
             BranchVisibilityRule.PRIVATE,
             BranchVisibilityRule.PRIVATE_ONLY)
@@ -403,6 +456,17 @@
             types.extend(PRIVATE_INFORMATION_TYPES)
         return types
 
+    def getDefaultInformationType(self):
+        """See `IBranchNamespace`."""
+        if not self._using_branchvisibilitypolicy:
+            default_type = POLICY_DEFAULT_TYPES[
+                self.product.branch_sharing_policy]
+            if default_type not in self.getAllowedInformationTypes():
+                return None
+            return default_type
+
+        return super(ProductNamespace, self).getDefaultInformationType()
+
 
 class PackageNamespace(_BaseNamespace):
     """A namespace for source package branches.

=== modified file 'lib/lp/code/model/tests/test_branchnamespace.py'
--- lib/lp/code/model/tests/test_branchnamespace.py	2012-07-11 22:16:47 +0000
+++ lib/lp/code/model/tests/test_branchnamespace.py	2012-07-13 08:51:30 +0000
@@ -8,6 +8,7 @@
 from zope.component import getUtility
 from zope.security.proxy import removeSecurityProxy
 
+from lp.app.interfaces.services import IService
 from lp.app.validators import LaunchpadValidationError
 from lp.code.enums import (
     BranchLifecycleStatus,
@@ -36,9 +37,11 @@
     ProductNamespace,
     )
 from lp.registry.enums import (
+    BranchSharingPolicy,
     InformationType,
     PRIVATE_INFORMATION_TYPES,
     PUBLIC_INFORMATION_TYPES,
+    SharingPermission,
     )
 from lp.registry.errors import (
     NoSuchDistroSeries,
@@ -52,7 +55,11 @@
     )
 from lp.registry.interfaces.product import NoSuchProduct
 from lp.registry.model.sourcepackage import SourcePackage
-from lp.testing import TestCaseWithFactory
+from lp.services.features.testing import FeatureFixture
+from lp.testing import (
+    admin_logged_in,
+    TestCaseWithFactory,
+    )
 from lp.testing.layers import DatabaseFunctionalLayer
 
 
@@ -356,8 +363,12 @@
         self.assertEqual(IBranchTarget(product), namespace.target)
 
 
-class TestProductNamespacePrivacy(TestCaseWithFactory):
-    """Tests for the privacy aspects of `ProductNamespace`."""
+class TestProductNamespacePrivacyWithBranchVisibility(TestCaseWithFactory):
+    """Tests for the privacy aspects of `ProductNamespace`.
+
+    This tests the behaviour for a product using the old
+    BranchVisibilityPolicy rules.
+    """
 
     layer = DatabaseFunctionalLayer
 
@@ -436,6 +447,84 @@
         self.assertEqual(team, namespace.getPrivacySubscriber())
 
 
+class TestProductNamespacePrivacyWithInformationType(TestCaseWithFactory):
+    """Tests for the privacy aspects of `ProductNamespace`.
+
+    This tests the behaviour for a product using the new
+    branch_sharing_policy rules.
+    """
+
+    layer = DatabaseFunctionalLayer
+
+    def setUp(self):
+        super(TestProductNamespacePrivacyWithInformationType, self).setUp()
+        self.useFixture(FeatureFixture(
+            {'disclosure.enhanced_sharing.writable': 'true'}))
+
+    def makeProductNamespace(self, sharing_policy, person=None):
+        if person is None:
+            person = self.factory.makePerson()
+        product = self.factory.makeProduct()
+        removeSecurityProxy(product).branch_sharing_policy = (
+            sharing_policy)
+        namespace = ProductNamespace(person, product)
+        return namespace
+
+    def test_public_anyone(self):
+        namespace = self.makeProductNamespace(
+            BranchSharingPolicy.PUBLIC)
+        self.assertContentEqual(
+            PUBLIC_INFORMATION_TYPES, namespace.getAllowedInformationTypes())
+        self.assertEqual(
+            InformationType.PUBLIC, namespace.getDefaultInformationType())
+
+    def test_public_or_proprietary_anyone(self):
+        namespace = self.makeProductNamespace(
+            BranchSharingPolicy.PUBLIC_OR_PROPRIETARY)
+        self.assertContentEqual(
+            PUBLIC_INFORMATION_TYPES + PRIVATE_INFORMATION_TYPES,
+            namespace.getAllowedInformationTypes())
+        self.assertEqual(
+            InformationType.PUBLIC, namespace.getDefaultInformationType())
+
+    def test_proprietary_or_public_anyone(self):
+        namespace = self.makeProductNamespace(
+            BranchSharingPolicy.PROPRIETARY_OR_PUBLIC)
+        self.assertContentEqual([], namespace.getAllowedInformationTypes())
+        self.assertIs(None, namespace.getDefaultInformationType())
+
+    def test_proprietary_or_public_grantor(self):
+        namespace = self.makeProductNamespace(
+            BranchSharingPolicy.PROPRIETARY_OR_PUBLIC)
+        with admin_logged_in():
+            getUtility(IService, 'sharing').sharePillarInformation(
+                namespace.product, namespace.owner, namespace.product.owner,
+                {InformationType.USERDATA: SharingPermission.ALL})
+        self.assertContentEqual(
+            PUBLIC_INFORMATION_TYPES + PRIVATE_INFORMATION_TYPES,
+            namespace.getAllowedInformationTypes())
+        self.assertEqual(
+            InformationType.USERDATA, namespace.getDefaultInformationType())
+
+    def test_proprietary_anyone(self):
+        namespace = self.makeProductNamespace(
+            BranchSharingPolicy.PROPRIETARY)
+        self.assertContentEqual([], namespace.getAllowedInformationTypes())
+        self.assertIs(None, namespace.getDefaultInformationType())
+
+    def test_proprietary_grantor(self):
+        namespace = self.makeProductNamespace(
+            BranchSharingPolicy.PROPRIETARY)
+        with admin_logged_in():
+            getUtility(IService, 'sharing').sharePillarInformation(
+                namespace.product, namespace.owner, namespace.product.owner,
+                {InformationType.USERDATA: SharingPermission.ALL})
+        self.assertContentEqual(
+            PRIVATE_INFORMATION_TYPES, namespace.getAllowedInformationTypes())
+        self.assertEqual(
+            InformationType.USERDATA, namespace.getDefaultInformationType())
+
+
 class TestPackageNamespace(TestCaseWithFactory, NamespaceMixin):
     """Tests for `PackageNamespace`."""
 

=== modified file 'lib/lp/registry/browser/product.py'
--- lib/lp/registry/browser/product.py	2012-07-06 19:00:25 +0000
+++ lib/lp/registry/browser/product.py	2012-07-13 08:51:30 +0000
@@ -114,6 +114,7 @@
 from lp.app.widgets.itemswidgets import (
     CheckBoxMatrixWidget,
     LaunchpadRadioWidget,
+    LaunchpadRadioWidgetWithDescription,
     )
 from lp.app.widgets.popup import PersonPickerWidget
 from lp.app.widgets.product import (
@@ -1573,6 +1574,9 @@
         "private_bugs",
         ]
 
+    custom_widget(
+        'branch_sharing_policy', LaunchpadRadioWidgetWithDescription)
+
     @property
     def page_title(self):
         """The HTML page title."""
@@ -1590,6 +1594,8 @@
         if not admin:
             self.field_names.remove('owner')
             self.field_names.remove('autoupdate')
+        if getFeatureFlag('disclosure.branch_sharing_policy.show_to_admin'):
+            self.field_names.append('branch_sharing_policy')
         super(ProductAdminView, self).setUpFields()
         self.form_fields = self._createAliasesField() + self.form_fields
         if admin:

=== modified file 'lib/lp/registry/configure.zcml'
--- lib/lp/registry/configure.zcml	2012-06-29 02:15:05 +0000
+++ lib/lp/registry/configure.zcml	2012-07-13 08:51:30 +0000
@@ -1324,7 +1324,7 @@
                 project_reviewed
                 license_approved"
             set_schema="lp.registry.interfaces.product.IProductModerateRestricted"
-            set_attributes="active"/>
+            set_attributes="active branch_sharing_policy"/>
 
         <!-- IHasAliases -->
 

=== modified file 'lib/lp/registry/enums.py'
--- lib/lp/registry/enums.py	2012-05-16 02:09:30 +0000
+++ lib/lp/registry/enums.py	2012-07-13 08:51:30 +0000
@@ -5,6 +5,8 @@
 
 __metaclass__ = type
 __all__ = [
+    'BranchSharingPolicy',
+    'BugSharingPolicy',
     'DistroSeriesDifferenceStatus',
     'DistroSeriesDifferenceType',
     'InformationType',
@@ -102,6 +104,65 @@
         """)
 
 
+class BranchSharingPolicy(DBEnumeratedType):
+
+    PUBLIC = DBItem(1, """
+        Public
+
+        Branches are public unless they contain sensitive security
+        information.
+        """)
+
+    PUBLIC_OR_PROPRIETARY = DBItem(2, """
+        Public, can be proprietary
+
+        New branches are public, but can be made proprietary later.
+        """)
+
+    PROPRIETARY_OR_PUBLIC = DBItem(3, """
+        Proprietary, can be public
+
+        New branches are proprietary, but can be made public later. Only
+        people who can see the project's proprietary information can create
+        new branches.
+        """)
+
+    PROPRIETARY = DBItem(4, """
+        Proprietary
+
+        Branches are always proprietary. Only people who can see the
+        project's proprietary information can create new branches.
+        """)
+
+
+class BugSharingPolicy(DBEnumeratedType):
+
+    PUBLIC = DBItem(1, """
+        Public
+
+        Bugs are public unless they contain sensitive security
+        information.
+        """)
+
+    PUBLIC_OR_PROPRIETARY = DBItem(2, """
+        Public, can be proprietary
+
+        New bugs are public, but can be made proprietary later.
+        """)
+
+    PROPRIETARY_OR_PUBLIC = DBItem(3, """
+        Proprietary, can be public
+
+        New bugs are proprietary, but can be made public later.
+        """)
+
+    PROPRIETARY = DBItem(4, """
+        Proprietary
+
+        Bugs are always proprietary.
+        """)
+
+
 class DistroSeriesDifferenceStatus(DBEnumeratedType):
     """Distribution series difference status.
 

=== modified file 'lib/lp/registry/interfaces/product.py'
--- lib/lp/registry/interfaces/product.py	2012-05-25 21:18:48 +0000
+++ lib/lp/registry/interfaces/product.py	2012-07-13 08:51:30 +0000
@@ -108,6 +108,10 @@
     IHasMergeProposals,
     )
 from lp.code.interfaces.hasrecipes import IHasRecipes
+from lp.registry.enums import (
+    BranchSharingPolicy,
+    BugSharingPolicy,
+    )
 from lp.registry.interfaces.announcement import IMakesAnnouncements
 from lp.registry.interfaces.commercialsubscription import (
     ICommercialSubscription,
@@ -634,6 +638,17 @@
                         description=_(
                             "Whether or not bugs reported into this project "
                             "are private by default.")))
+    branch_sharing_policy = exported(Choice(
+        title=_('Branch sharing policy'),
+        description=_("Sharing policy for this project's branches."),
+        required=False, readonly=False, vocabulary=BranchSharingPolicy),
+        as_of='devel')
+    bug_sharing_policy = exported(Choice(
+        title=_('Bug sharing policy'),
+        description=_("Sharing policy for this project's bugs."),
+        required=False, readonly=True, vocabulary=BugSharingPolicy),
+        as_of='devel')
+
     licenses = exported(
         Set(title=_('Licences'),
             value_type=Choice(vocabulary=License)))

=== modified file 'lib/lp/registry/interfaces/sharingservice.py'
--- lib/lp/registry/interfaces/sharingservice.py	2012-06-18 03:23:18 +0000
+++ lib/lp/registry/interfaces/sharingservice.py	2012-07-13 08:51:30 +0000
@@ -45,6 +45,13 @@
     # version 'devel'
     export_as_webservice_entry(publish_web_link=False, as_of='beta')
 
+    def checkPillarAccess(pillar, information_type, person):
+        """Check the person's access to the given pillar and information type.
+
+        :return: True if the user has access to all the pillar's information
+            of that type, False otherwise
+        """
+
     def getSharedArtifacts(pillar, person, user):
         """Return the artifacts shared between the pillar and person.
 

=== modified file 'lib/lp/registry/model/product.py'
--- lib/lp/registry/model/product.py	2012-07-09 04:14:09 +0000
+++ lib/lp/registry/model/product.py	2012-07-13 08:51:30 +0000
@@ -116,7 +116,11 @@
     )
 from lp.code.model.sourcepackagerecipe import SourcePackageRecipe
 from lp.code.model.sourcepackagerecipedata import SourcePackageRecipeData
-from lp.registry.enums import InformationType
+from lp.registry.enums import (
+    BranchSharingPolicy,
+    BugSharingPolicy,
+    InformationType,
+    )
 from lp.registry.errors import CommercialSubscribersOnly
 from lp.registry.interfaces.accesspolicy import (
     IAccessPolicySource,
@@ -469,6 +473,10 @@
     reviewer_whiteboard = StringCol(notNull=False, default=None)
     private_bugs = BoolCol(
         dbName='private_bugs', notNull=True, default=False)
+    bug_sharing_policy = EnumCol(
+        enum=BugSharingPolicy, notNull=False, default=None)
+    branch_sharing_policy = EnumCol(
+        enum=BranchSharingPolicy, notNull=False, default=None)
     autoupdate = BoolCol(dbName='autoupdate', notNull=True, default=False)
     freshmeatproject = StringCol(notNull=False, default=None)
     sourceforgeproject = StringCol(notNull=False, default=None)

=== modified file 'lib/lp/registry/services/sharingservice.py'
--- lib/lp/registry/services/sharingservice.py	2012-07-02 23:40:56 +0000
+++ lib/lp/registry/services/sharingservice.py	2012-07-13 08:51:30 +0000
@@ -84,6 +84,25 @@
             bool(getFeatureFlag(
             'disclosure.enhanced_sharing.writable')))
 
+    def checkPillarAccess(self, pillar, information_type, person):
+        """See `ISharingService`."""
+        policy = getUtility(IAccessPolicySource).find(
+            [(pillar, information_type)]).one()
+        if policy is None:
+            return False
+        store = IStore(AccessPolicyGrant)
+        tables = [
+            AccessPolicyGrant,
+            Join(
+                TeamParticipation,
+                TeamParticipation.teamID == AccessPolicyGrant.grantee_id),
+            ]
+        result = store.using(*tables).find(
+            AccessPolicyGrant,
+            AccessPolicyGrant.policy_id == policy.id,
+            TeamParticipation.personID == person.id)
+        return not result.is_empty()
+
     def getSharedArtifacts(self, pillar, person, user):
         """See `ISharingService`."""
         policies = getUtility(IAccessPolicySource).findByPillar([pillar])

=== modified file 'lib/lp/registry/services/tests/test_sharingservice.py'
--- lib/lp/registry/services/tests/test_sharingservice.py	2012-07-11 01:21:56 +0000
+++ lib/lp/registry/services/tests/test_sharingservice.py	2012-07-13 08:51:30 +0000
@@ -39,6 +39,7 @@
 from lp.services.webapp.interfaces import ILaunchpadRoot
 from lp.services.webapp.publisher import canonical_url
 from lp.testing import (
+    admin_logged_in,
     login,
     login_person,
     StormStatementRecorder,
@@ -493,7 +494,7 @@
                  [InformationType.USERDATA,
                   InformationType.EMBARGOEDSECURITY]),
                  ]
-        else: 
+        else:
             expected_sharee_grants = [
                 (sharee,
                  {es_policy: SharingPermission.ALL,
@@ -1253,6 +1254,38 @@
 
         self._assert_getVisibleArtifacts_bug_change(retarget_bugtask)
 
+    def test_checkPillarAccess(self):
+        # checkPillarAccess checks whether the user has full access to
+        # an information type.
+        product = self.factory.makeProduct()
+        right_person = self.factory.makePerson()
+        right_team = self.factory.makeTeam(members=[right_person])
+        wrong_person = self.factory.makePerson()
+        with FeatureFixture(WRITE_FLAG):
+            with admin_logged_in():
+                self.service.sharePillarInformation(
+                    product, right_team, product.owner,
+                    {InformationType.USERDATA: SharingPermission.ALL})
+                self.service.sharePillarInformation(
+                    product, wrong_person, product.owner,
+                    {InformationType.EMBARGOEDSECURITY: SharingPermission.ALL})
+        self.assertEqual(
+            False,
+            self.service.checkPillarAccess(
+                product, InformationType.USERDATA, wrong_person))
+        self.assertEqual(
+            True,
+            self.service.checkPillarAccess(
+                product, InformationType.USERDATA, right_person))
+
+    def test_checkPillarAccess_no_policy(self):
+        # checkPillarAccess returns False if there's no policy.
+        self.assertEqual(
+            False,
+            self.service.checkPillarAccess(
+                self.factory.makeProduct(), InformationType.PUBLIC,
+                self.factory.makePerson()))
+
 
 class ApiTestMixin:
     """Common tests for launchpadlib and webservice."""

=== modified file 'lib/lp/services/features/flags.py'
--- lib/lp/services/features/flags.py	2012-07-11 01:24:42 +0000
+++ lib/lp/services/features/flags.py	2012-07-13 08:51:30 +0000
@@ -318,6 +318,13 @@
      '',
      '',
      ''),
+    ('disclosure.branch_sharing_policy.show_to_admin',
+     'boolean',
+     ('If true, the branch sharing policy field is shown on '
+      'Product:+admin, letting BranchVisibilityPolicy be overridden.'),
+     '',
+     '',
+     ''),
     ])
 
 # The set of all flag names that are documented.