Merge lp:~sinzui/launchpad/entitle-branch-sharing into lp:launchpad

[Curtis Hovey]

Allow project maintainers to call setBranchSharingPolicy().

Launchpad must let users reconfigure their projects to use sharing
which enters beta next week.

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

RULES

    Pre-implementation: wgrant
    * Remove the guard in setBranchSharingPolicy() that enforces the
      deprecated rule that restricts branch policies to be set by a
      commercial admin.
    * Simplify tests that use a commmercial admin to setup the conditions.

QA

    As a non-commercial admin
    * Visit http://qastaging.launchpad.net/lp-dev-utils/+sharing
    * Verify you can change the branch sharing policy.

LINT

    lib/lp/code/browser/tests/test_branch.py
    lib/lp/code/model/tests/test_branch.py
    lib/lp/code/model/tests/test_branchnamespace.py
    lib/lp/registry/model/product.py

TEST

    ./bin/test -vvc -t transitionToInformationType -t methods_smoketest lp.code.model.tests.test_branch
    ./bin/test -vvc -t WithInformationType lp.code.model.tests.test_branchnamespace
    ./bin/test -vvc -t EditViewInformationTypes lp.code.browser.tests.test_branch

IMPLEMENTATION

Remove the commerncial admin guard from setBranchSharingPolicy().
    lib/lp/registry/model/product.py

Update tests to use the project owner instead of a commecial admin to
setup the test conditions.
    lib/lp/code/browser/tests/test_branch.py
    lib/lp/code/model/tests/test_branch.py
    lib/lp/code/model/tests/test_branchnamespace.py

[William Grant (wgrant)]

This can't land before <https://code.launchpad.net/~sinzui/launchpad/commercial-admin-sharing/+merge/120464>, as in this branch there's no access check at all on setBugSharingPolicy or setBranchSharingPolicy. If your new permission from the other branch is sufficient, perhaps remove the unused user argument from setFooSharingPolicy.

review: Approve (code)

[Curtis Hovey (sinzui)]

I agree it needs to land with the other branch. I noticed the user arg and pondered if we wanted to keep it to either log or queue a notification to the project maintainer to inform who made the change. Cody is very concerned that several oh his projects had policy changes he would not permit. A might have helped him close the potential hole before something went through it. What do you think?

[William Grant (wgrant)]

We should certainly hook it into an auditing mechanism at some point, but it's relatively easy to readd the argument later. I don't have a strong opinion either way.

=== modified file 'lib/lp/bugs/browser/tests/test_bugs.py'
--- lib/lp/bugs/browser/tests/test_bugs.py	2012-08-20 13:12:45 +0000
+++ lib/lp/bugs/browser/tests/test_bugs.py	2012-08-21 15:01:20 +0000
@@ -16,7 +16,6 @@
     BugSharingPolicy,
     InformationType,
     )
-from lp.registry.interfaces.person import IPersonSet
 from lp.registry.interfaces.product import License
 from lp.services.webapp.publisher import canonical_url
 from lp.testing import (
@@ -28,7 +27,6 @@
     )
 from lp.testing.layers import DatabaseFunctionalLayer
 from lp.testing.pages import find_tag_by_id
-from lp.testing.sampledata import COMMERCIAL_ADMIN_EMAIL
 from lp.testing.views import create_initialized_view
 
 
@@ -184,9 +182,8 @@
         # createBug() does not adapt the default kwargs when they are none.
         project = self.factory.makeProduct(
             licenses=[License.OTHER_PROPRIETARY])
-        comadmin = getUtility(IPersonSet).getByEmail(COMMERCIAL_ADMIN_EMAIL)
         project.setBugSharingPolicy(
-            BugSharingPolicy.PROPRIETARY_OR_PUBLIC, comadmin)
+            BugSharingPolicy.PROPRIETARY_OR_PUBLIC, project.owner)
         bug = self.application.createBug(
             project.owner, 'title', 'description', project)
         self.assertEqual(InformationType.PROPRIETARY, bug.information_type)
@@ -195,9 +192,8 @@
         # createBug() adapts a kwarg to InformationType if one is is not None.
         project = self.factory.makeProduct(
             licenses=[License.OTHER_PROPRIETARY])
-        comadmin = getUtility(IPersonSet).getByEmail(COMMERCIAL_ADMIN_EMAIL)
         project.setBugSharingPolicy(
-            BugSharingPolicy.PROPRIETARY_OR_PUBLIC, comadmin)
+            BugSharingPolicy.PROPRIETARY_OR_PUBLIC, project.owner)
         bug = self.application.createBug(
             project.owner, 'title', 'description', project, private=False)
         self.assertEqual(InformationType.PUBLIC, bug.information_type)
 
=== modified file 'lib/lp/bugs/browser/tests/test_bugtarget_filebug.py'
--- lib/lp/bugs/browser/tests/test_bugtarget_filebug.py	2012-08-20 13:26:21 +0000
+++ lib/lp/bugs/browser/tests/test_bugtarget_filebug.py	2012-08-21 15:01:20 +0000
@@ -29,7 +29,6 @@
     InformationType,
     PRIVATE_INFORMATION_TYPES,
     )
-from lp.registry.interfaces.person import IPersonSet
 from lp.registry.interfaces.projectgroup import IProjectGroup
 from lp.services.webapp.servers import LaunchpadTestRequest
 from lp.testing import (
@@ -43,7 +42,6 @@
     find_main_content,
     find_tag_by_id,
     )
-from lp.testing.sampledata import COMMERCIAL_ADMIN_EMAIL
 from lp.testing.views import (
     create_initialized_view,
     create_view,
@@ -369,9 +367,7 @@
             removeSecurityProxy(product).private_bugs = True
         if bug_sharing_policy:
             self.factory.makeCommercialSubscription(product=product)
-            comadmin = getUtility(IPersonSet).getByEmail(
-                COMMERCIAL_ADMIN_EMAIL)
-            product.setBugSharingPolicy(bug_sharing_policy, comadmin)
+            product.setBugSharingPolicy(bug_sharing_policy, product.owner)
         with person_logged_in(product.owner):
             view = create_view(
                 product, '+filebug', method='POST', form=form,
@@ -447,8 +443,8 @@
         # correctly for a project with a proprietary sharing policy.
         product = self.factory.makeProduct(official_malone=True)
         self.factory.makeCommercialSubscription(product=product)
-        comadmin = getUtility(IPersonSet).getByEmail(COMMERCIAL_ADMIN_EMAIL)
-        product.setBugSharingPolicy(BugSharingPolicy.PROPRIETARY, comadmin)
+        product.setBugSharingPolicy(
+            BugSharingPolicy.PROPRIETARY, product.owner)
         with person_logged_in(product.owner):
             view = create_initialized_view(
                 product, '+filebug', principal=product.owner)
@@ -499,9 +495,7 @@
             removeSecurityProxy(product).private_bugs = True
         if bug_sharing_policy:
             self.factory.makeCommercialSubscription(product=product)
-            comadmin = getUtility(IPersonSet).getByEmail(
-                COMMERCIAL_ADMIN_EMAIL)
-            product.setBugSharingPolicy(bug_sharing_policy, comadmin)
+            product.setBugSharingPolicy(bug_sharing_policy, product.owner)
         anyone = self.factory.makePerson()
         with person_logged_in(anyone):
             view = create_initialized_view(
 
=== modified file 'lib/lp/bugs/mail/tests/test_handler.py'
--- lib/lp/bugs/mail/tests/test_handler.py	2012-08-20 13:12:45 +0000
+++ lib/lp/bugs/mail/tests/test_handler.py	2012-08-21 15:01:20 +0000
@@ -32,7 +32,6 @@
     BugSharingPolicy,
     InformationType,
     )
-from lp.registry.interfaces.person import IPersonSet
 from lp.services.config import config
 from lp.services.identity.interfaces.emailaddress import EmailAddressStatus
 from lp.services.mail import stub
@@ -52,7 +51,6 @@
     LaunchpadZopelessLayer,
     )
 from lp.testing.mail_helpers import pop_notifications
-from lp.testing.sampledata import COMMERCIAL_ADMIN_EMAIL
 
 
 class TestMaloneHandler(TestCaseWithFactory):
@@ -261,8 +259,8 @@
     def test_new_bug_with_sharing_policy_proprietary(self):
         project = self.factory.makeProduct(name='fnord')
         self.factory.makeCommercialSubscription(product=project)
-        comadmin = getUtility(IPersonSet).getByEmail(COMMERCIAL_ADMIN_EMAIL)
-        project.setBugSharingPolicy(BugSharingPolicy.PROPRIETARY, comadmin)
+        project.setBugSharingPolicy(
+            BugSharingPolicy.PROPRIETARY, project.owner)
         transaction.commit()
         handler = MaloneHandler()
         with person_logged_in(project.owner):
 
=== modified file 'lib/lp/bugs/tests/test_bugs_webservice.py'
--- lib/lp/bugs/tests/test_bugs_webservice.py	2012-08-20 13:12:45 +0000
+++ lib/lp/bugs/tests/test_bugs_webservice.py	2012-08-21 15:01:20 +0000
@@ -21,7 +21,6 @@
     )
 from zope.component import (
     getMultiAdapter,
-    getUtility,
     )
 
 from lp.bugs.browser.bugtask import get_comments_for_bugtask
@@ -30,7 +29,6 @@
     BugSharingPolicy,
     InformationType,
     )
-from lp.registry.interfaces.person import IPersonSet
 from lp.registry.interfaces.product import License
 from lp.services.webapp import snapshot
 from lp.services.webapp.servers import LaunchpadTestRequest
@@ -52,7 +50,6 @@
 from lp.testing.pages import LaunchpadWebServiceCaller
 from lp.testing.sampledata import (
     ADMIN_EMAIL,
-    COMMERCIAL_ADMIN_EMAIL,
     USER_EMAIL,
     )
 
@@ -414,9 +411,8 @@
         # to the project's bug sharing policy.
         project = self.factory.makeProduct(
             licenses=[License.OTHER_PROPRIETARY])
-        comadmin = getUtility(IPersonSet).getByEmail(COMMERCIAL_ADMIN_EMAIL)
         project.setBugSharingPolicy(
-            BugSharingPolicy.PROPRIETARY_OR_PUBLIC, comadmin)
+            BugSharingPolicy.PROPRIETARY_OR_PUBLIC, project.owner)
         webservice = launchpadlib_for('test', 'salgado')
         bugs_collection = webservice.load('/bugs')
         bug = bugs_collection.createBug(
 
=== modified file 'lib/lp/code/browser/tests/test_branch.py'
--- lib/lp/code/browser/tests/test_branch.py	2012-08-16 06:24:32 +0000
+++ lib/lp/code/browser/tests/test_branch.py	2012-08-21 15:01:20 +0000
@@ -41,7 +41,6 @@
     )
 from lp.registry.interfaces.accesspolicy import IAccessPolicySource
 from lp.registry.interfaces.person import (
-    IPersonSet,
     PersonVisibility,
     )
 from lp.services.config import config
@@ -72,7 +71,6 @@
     setupBrowser,
     setupBrowserForUser,
     )
-from lp.testing.sampledata import COMMERCIAL_ADMIN_EMAIL
 from lp.testing.views import create_initialized_view
 
 
@@ -1086,10 +1084,9 @@
         owner = self.factory.makePerson()
         product = self.factory.makeProduct(owner=owner)
         self.factory.makeCommercialSubscription(product=product)
-        comadmin = getUtility(IPersonSet).getByEmail(COMMERCIAL_ADMIN_EMAIL)
-        product.setBranchSharingPolicy(
-            BranchSharingPolicy.EMBARGOED_OR_PROPRIETARY, comadmin)
         with person_logged_in(owner):
+            product.setBranchSharingPolicy(
+                BranchSharingPolicy.EMBARGOED_OR_PROPRIETARY, owner)
             branch = self.factory.makeBranch(
                 product=product, owner=owner,
                 information_type=InformationType.PROPRIETARY)
@@ -1102,10 +1099,9 @@
         owner = self.factory.makePerson()
         product = self.factory.makeProduct(owner=owner)
         self.factory.makeCommercialSubscription(product=product)
-        comadmin = getUtility(IPersonSet).getByEmail(COMMERCIAL_ADMIN_EMAIL)
-        product.setBranchSharingPolicy(
-            BranchSharingPolicy.PROPRIETARY, comadmin)
         with person_logged_in(owner):
+            product.setBranchSharingPolicy(
+                BranchSharingPolicy.PROPRIETARY, owner)
             branch = self.factory.makeBranch(
                 product=product, owner=owner,
                 information_type=InformationType.PROPRIETARY)
 
=== modified file 'lib/lp/code/browser/tests/test_branchlisting.py'
--- lib/lp/code/browser/tests/test_branchlisting.py	2012-08-14 23:27:07 +0000
+++ lib/lp/code/browser/tests/test_branchlisting.py	2012-08-21 15:01:20 +0000
@@ -46,6 +46,7 @@
 from lp.services.webapp.servers import LaunchpadTestRequest
 from lp.testing import (
     BrowserTestCase,
+    celebrity_logged_in,
     login_person,
     normalize_whitespace,
     person_logged_in,
@@ -66,7 +67,6 @@
     )
 from lp.testing.sampledata import (
     ADMIN_EMAIL,
-    COMMERCIAL_ADMIN_EMAIL,
     )
 from lp.testing.views import (
     create_initialized_view,
@@ -752,8 +752,8 @@
     def test_branch_visibility_link_commercial_admin(self):
         # A commercial admin will be displayed a link to define branch
         # visibility in the action portlet.
-        admin = getUtility(IPersonSet).getByEmail(COMMERCIAL_ADMIN_EMAIL)
-        self._testBranchVisibilityLink(admin)
+        with celebrity_logged_in('commercial_admin') as admin:
+            self._testBranchVisibilityLink(admin)
 
     def test_branch_visibility_link_non_admin(self):
         # A non-admin will not see the action portlet.
 
=== modified file 'lib/lp/code/model/tests/test_branch.py'
--- lib/lp/code/model/tests/test_branch.py	2012-08-20 14:03:01 +0000
+++ lib/lp/code/model/tests/test_branch.py	2012-08-21 15:01:20 +0000
@@ -2601,10 +2601,9 @@
     def test_methods_smoketest(self):
         # Users with launchpad.Moderate can call transitionToInformationType.
         branch = self.factory.makeProductBranch()
-        with celebrity_logged_in('commercial_admin') as admin:
+        with person_logged_in(branch.product.owner):
             branch.product.setBranchSharingPolicy(
-                BranchSharingPolicy.PUBLIC, admin)
-        with person_logged_in(branch.product.owner):
+                BranchSharingPolicy.PUBLIC, branch.product.owner)
             branch.transitionToInformationType(
                 InformationType.PRIVATESECURITY, branch.product.owner)
         self.assertEqual(
@@ -3283,12 +3282,9 @@
         """Test transitionToInformationType() API arguments."""
         product = self.factory.makeProduct()
         self.factory.makeCommercialSubscription(product)
-        with celebrity_logged_in('commercial_admin') as admin:
-            # XXX sinzui 2012-08-16: setBranchSharingPolicy() is guarded
-            # at this moment.
+        with person_logged_in(product.owner):
             product.setBranchSharingPolicy(
-                BranchSharingPolicy.PUBLIC_OR_PROPRIETARY, admin)
-        with person_logged_in(product.owner):
+                BranchSharingPolicy.PUBLIC_OR_PROPRIETARY, product.owner)
             db_branch = self.factory.makeBranch(product=product)
             launchpad = launchpadlib_for('test', db_branch.owner,
                 service_root=self.layer.appserver_root_url('api'))
 
=== modified file 'lib/lp/code/model/tests/test_branchnamespace.py'
--- lib/lp/code/model/tests/test_branchnamespace.py	2012-08-20 13:26:21 +0000
+++ lib/lp/code/model/tests/test_branchnamespace.py	2012-08-21 15:01:20 +0000
@@ -53,7 +53,6 @@
     )
 from lp.registry.interfaces.distribution import NoSuchDistribution
 from lp.registry.interfaces.person import (
-    IPersonSet,
     NoSuchPerson,
     )
 from lp.registry.interfaces.product import NoSuchProduct
@@ -64,7 +63,6 @@
     TestCaseWithFactory,
     )
 from lp.testing.layers import DatabaseFunctionalLayer
-from lp.testing.sampledata import COMMERCIAL_ADMIN_EMAIL
 
 
 class NamespaceMixin:
@@ -470,8 +468,7 @@
             person = self.factory.makePerson()
         product = self.factory.makeProduct()
         self.factory.makeCommercialSubscription(product=product)
-        comadmin = getUtility(IPersonSet).getByEmail(COMMERCIAL_ADMIN_EMAIL)
-        product.setBranchSharingPolicy(sharing_policy, comadmin)
+        product.setBranchSharingPolicy(sharing_policy, product.owner)
         namespace = ProductNamespace(person, product)
         return namespace
 
 
=== modified file 'lib/lp/registry/model/product.py'
--- lib/lp/registry/model/product.py	2012-08-20 13:26:21 +0000
+++ lib/lp/registry/model/product.py	2012-08-21 15:01:20 +0000
@@ -567,10 +567,6 @@
 
     def setBranchSharingPolicy(self, branch_sharing_policy, user):
         """See `IProductPublic`."""
-        if not user or not IPersonRoles(user).in_commercial_admin:
-            raise Unauthorized(
-                "Only commercial admins can configure sharing policies right "
-                "now.")
         if branch_sharing_policy != BranchSharingPolicy.PUBLIC:
             if not self.has_current_commercial_subscription:
                 raise CommercialSubscribersOnly(
@@ -585,10 +581,6 @@
 
     def setBugSharingPolicy(self, bug_sharing_policy, user):
         """See `IProductPublic`."""
-        if not user or not IPersonRoles(user).in_commercial_admin:
-            raise Unauthorized(
-                "Only commercial admins can configure sharing policies right "
-                "now.")
         if bug_sharing_policy != BugSharingPolicy.PUBLIC:
             if not self.has_current_commercial_subscription:
                 raise CommercialSubscribersOnly(
 
=== modified file 'lib/lp/registry/tests/test_product.py'
--- lib/lp/registry/tests/test_product.py	2012-08-20 13:26:21 +0000
+++ lib/lp/registry/tests/test_product.py	2012-08-21 15:01:20 +0000
@@ -45,7 +45,6 @@
     IAccessPolicySource,
     )
 from lp.registry.interfaces.oopsreferences import IHasOOPSReferences
-from lp.registry.interfaces.person import IPersonSet
 from lp.registry.interfaces.product import (
     IProduct,
     IProductSet,
@@ -81,7 +80,6 @@
     get_feedback_messages,
     setupBrowser,
     )
-from lp.testing.sampledata import COMMERCIAL_ADMIN_EMAIL
 from lp.translations.enums import TranslationPermission
 from lp.translations.interfaces.customlanguagecode import (
     IHasCustomLanguageCodes,
@@ -388,8 +386,7 @@
     def makeProductWithPolicy(self, bug_sharing_policy, private_bugs=False):
         product = self.factory.makeProduct(private_bugs=private_bugs)
         self.factory.makeCommercialSubscription(product=product)
-        comadmin = getUtility(IPersonSet).getByEmail(COMMERCIAL_ADMIN_EMAIL)
-        product.setBugSharingPolicy(bug_sharing_policy, comadmin)
+        product.setBugSharingPolicy(bug_sharing_policy, product.owner)
         return product
 
     def test_no_policy(self):
 
=== modified file 'lib/lp/registry/tests/test_product_webservice.py'
--- lib/lp/registry/tests/test_product_webservice.py	2012-08-10 06:40:08 +0000
+++ lib/lp/registry/tests/test_product_webservice.py	2012-08-21 15:01:20 +0000
@@ -57,8 +57,7 @@
         product = self.factory.makeProduct()
         self.factory.makeCommercialSubscription(product=product)
         webservice = webservice_for_person(
-            self.factory.makeCommercialAdmin(),
-            permission=OAuthPermission.WRITE_PRIVATE)
+            product.owner, permission=OAuthPermission.WRITE_PRIVATE)
         response = self.patch(
             webservice, product, branch_sharing_policy='Proprietary')
         self.assertEqual(209, response.status)
@@ -80,27 +79,12 @@
                       'proprietary branches.')))
         self.assertIs(None, product.branch_sharing_policy)
 
-    def test_branch_sharing_policy_random_user(self):
-        # Arbitrary users can't set branch_sharing_policy.
-        product = self.factory.makeProduct()
-        webservice = webservice_for_person(
-            self.factory.makePerson(),
-            permission=OAuthPermission.WRITE_PRIVATE)
-        response = self.patch(
-            webservice, product, branch_sharing_policy='Proprietary')
-        self.assertThat(response, MatchesStructure.byEquality(
-                status=401,
-                body=('Only commercial admins can configure sharing policies '
-                      'right now.')))
-        self.assertIs(None, product.branch_sharing_policy)
-
     def test_bug_sharing_policy_can_be_set(self):
         # bug_sharing_policy can be set via the API.
         product = self.factory.makeProduct()
         self.factory.makeCommercialSubscription(product=product)
         webservice = webservice_for_person(
-            self.factory.makeCommercialAdmin(),
-            permission=OAuthPermission.WRITE_PRIVATE)
+            product.owner, permission=OAuthPermission.WRITE_PRIVATE)
         response = self.patch(
             webservice, product, bug_sharing_policy='Proprietary')
         self.assertEqual(209, response.status)
@@ -121,17 +105,3 @@
                 body=('A current commercial subscription is required to use '
                       'proprietary bugs.')))
         self.assertIs(None, product.bug_sharing_policy)
-
-    def test_bug_sharing_policy_random_user(self):
-        # Arbitrary users can't set bug_sharing_policy.
-        product = self.factory.makeProduct()
-        webservice = webservice_for_person(
-            self.factory.makePerson(),
-            permission=OAuthPermission.WRITE_PRIVATE)
-        response = self.patch(
-            webservice, product, bug_sharing_policy='Proprietary')
-        self.assertThat(response, MatchesStructure.byEquality(
-                status=401,
-                body=('Only commercial admins can configure sharing policies '
-                      'right now.')))
-        self.assertIs(None, product.bug_sharing_policy)
 
=== modified file 'lib/lp/soyuz/tests/test_archive.py'
--- lib/lp/soyuz/tests/test_archive.py	2012-08-20 14:31:11 +0000
+++ lib/lp/soyuz/tests/test_archive.py	2012-08-21 15:01:20 +0000
@@ -98,7 +98,6 @@
     LaunchpadFunctionalLayer,
     LaunchpadZopelessLayer,
     )
-from lp.testing.sampledata import COMMERCIAL_ADMIN_EMAIL
 
 
 class TestGetPublicationsInArchive(TestCaseWithFactory):
@@ -1405,9 +1404,9 @@
 
     def test_commercial_admin_can_set_build_debug_symbols(self):
         # A commercial admin can set it.
-        login(COMMERCIAL_ADMIN_EMAIL)
-        self.setBuildDebugSymbols(self.archive, True)
-        self.assertTrue(self.archive.build_debug_symbols)
+        with celebrity_logged_in('commercial_admin'):
+            self.setBuildDebugSymbols(self.archive, True)
+            self.assertTrue(self.archive.build_debug_symbols)
 
 
 class TestAddArchiveDependencies(TestCaseWithFactory):
 
=== modified file 'lib/lp/testing/sampledata.py'
--- lib/lp/testing/sampledata.py	2010-10-18 10:24:38 +0000
+++ lib/lp/testing/sampledata.py	2012-08-21 15:01:20 +0000
@@ -13,7 +13,6 @@
     'BOB_THE_BUILDER_NAME',
     'BUILDD_ADMIN_USERNAME',
     'CHROOT_LIBRARYFILEALIAS',
-    'COMMERCIAL_ADMIN_EMAIL',
     'FROG_THE_BUILDER_NAME',
     'HOARY_DISTROSERIES_NAME',
     'I386_ARCHITECTURE_NAME',
@@ -25,7 +24,6 @@
     'UBUNTU_DEVELOPER_ADMIN_NAME',
     'UBUNTU_DISTRIBUTION_NAME',
     'UBUNTU_UPLOAD_TEAM_NAME',
-    'UBUNTUTEST_DISTRIBUTION_NAME',
     'USER_EMAIL',
     'WARTY_DISTROSERIES_NAME',
     'WARTY_ONLY_SOURCEPACKAGENAME',
@@ -57,7 +55,6 @@
 NO_PRIVILEGE_EMAIL = '[e-mail]'
 USER_EMAIL = '[e-mail]'
 VCS_IMPORTS_MEMBER_EMAIL = '[e-mail]'
-COMMERCIAL_ADMIN_EMAIL = '[e-mail]'
 SAMPLE_PERSON_EMAIL = USER_EMAIL
 # A user that is an admin of ubuntu-team, which has upload rights
 # to Ubuntu.