launchpad-reviewers team mailing list archive

[Curtis Hovey <curtis.hovey@xxxxxxxxxxxxx>]

Curtis Hovey has proposed merging lp:~sinzui/launchpad/commercial-admin-sharing into lp:launchpad with lp:~sinzui/launchpad/entitle-branch-sharing as a prerequisite.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1039054 in Launchpad itself: "Commercial admins cannot help configure sharing"
  https://bugs.launchpad.net/launchpad/+bug/1039054

For more details, see:
https://code.launchpad.net/~sinzui/launchpad/commercial-admin-sharing/+merge/120464

Commercial admins are responsible for helping commercial project setup,
but they do not have permission to access +sharing. There is some irony
here because bug 750871 indicates that they are the only group that can
configure branch sharing policies :(

We want to grant launchpad.view on +sharing to commercial admins. This
will not give them permission to see the private bugs and branches. It
will allow them to see and set the sharing policies. They may need to
share/unshare with teams, but that should not be undertaken if the
changes are complex.

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

RULES

    Pre-implementation: jcsackett
    * Change the permission on +sharing to allow commercial admins.
    * Ensure commercial admins can set the bug and branch sharing
      policies
    * The permissions to access and change are launchpad.Driver and
      launchpad.Edit, and giving the commercial admins unqualified access
      also gives privileges to edit a project and nominate for series.
    * Editing a project is not bad in the case of a commercial project.
      Commercial Admins only need this extra permission for commercial
      projects.


QA

    As a commercial admin
    * Visit http://qastaging.launchpad.net/fusionapp
    * Verify you can see the link to Sharing
    * Follow the link.
    * Verify you can see the Sharing page.
    * Set the bug sharing policy to proprietary, can be public.
    * Set the branch sharing policy to proprietary, only proprietary.

    * Visit https://qastaging.launchpad.net/zeitgeist-datasources
    * Verify you do not see the Sharing link
    * Verify you cannot access
      https://qastaging.launchpad.net/zeitgeist-datasources/+sharing


LINT

    lib/lp/security.py
    lib/lp/registry/tests/test_product.py


TEST

    ./bin/test -vvc -t ProductPermissionTestCase lp.registry.tests.test_product


IMPLEMENTATION

Extended the existing permission checkers to permit the user to edit or
drive a product if the user is a commercial admin and the product has a
commercial subscription. Though the driver role has permission to edit
bugs and blueprints, it is not conveyed to the commercial admin because
the IProduct driver and edit permissions are very specific to creating
a series and configuring the product apps.
    lib/lp/security.py
    lib/lp/registry/tests/test_product.py
-- 
https://code.launchpad.net/~sinzui/launchpad/commercial-admin-sharing/+merge/120464
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~sinzui/launchpad/commercial-admin-sharing into lp:launchpad.

=== 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-20 20:24:33 +0000
@@ -57,6 +57,7 @@
     UnDeactivateable,
     )
 from lp.registry.model.productlicense import ProductLicense
+from lp.services.webapp.authorization import check_permission
 from lp.testing import (
     admin_logged_in,
     celebrity_logged_in,
@@ -452,6 +453,51 @@
             product.getDefaultBugInformationType())
 
 
+class ProductPermissionTestCase(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    def test_owner_can_edit(self):
+        product = self.factory.makeProduct()
+        with person_logged_in(product.owner):
+            self.assertTrue(check_permission('launchpad.Edit', product))
+
+    def test_commercial_admin_cannot_edit_non_commercial(self):
+        product = self.factory.makeProduct()
+        with celebrity_logged_in('commercial_admin'):
+            self.assertFalse(check_permission('launchpad.Edit', product))
+
+    def test_commercial_admin_can_edit_commercial(self):
+        product = self.factory.makeProduct()
+        self.factory.makeCommercialSubscription(product)
+        with celebrity_logged_in('commercial_admin'):
+            self.assertTrue(check_permission('launchpad.Edit', product))
+
+    def test_owner_can_driver(self):
+        product = self.factory.makeProduct()
+        with person_logged_in(product.owner):
+            self.assertTrue(check_permission('launchpad.Driver', product))
+
+    def test_driver_can_driver(self):
+        product = self.factory.makeProduct()
+        driver = self.factory.makePerson()
+        with person_logged_in(product.owner):
+            product.driver = driver
+        with person_logged_in(driver):
+            self.assertTrue(check_permission('launchpad.Driver', product))
+
+    def test_commercial_admin_cannot_drive_non_commercial(self):
+        product = self.factory.makeProduct()
+        with celebrity_logged_in('commercial_admin'):
+            self.assertFalse(check_permission('launchpad.Driver', product))
+
+    def test_commercial_admin_can_drive_commercial(self):
+        product = self.factory.makeProduct()
+        self.factory.makeCommercialSubscription(product)
+        with celebrity_logged_in('commercial_admin'):
+            self.assertTrue(check_permission('launchpad.Driver', product))
+
+
 class TestProductFiles(TestCase):
     """Tests for downloadable product files."""
 

=== modified file 'lib/lp/security.py'
--- lib/lp/security.py	2012-08-20 20:24:33 +0000
+++ lib/lp/security.py	2012-08-20 20:24:33 +0000
@@ -242,6 +242,11 @@
     )
 
 
+def is_commercial_case(obj, user):
+    """Is this a commercial project and the user is a commercial admin?"""
+    return obj.has_current_commercial_subscription and user.in_commercial_admin
+
+
 class ViewByLoggedInUser(AuthorizationBase):
     """The default ruleset for the launchpad.View permission.
 
@@ -359,7 +364,7 @@
     permission = 'launchpad.Driver'
 
     def checkAuthenticated(self, user):
-        """The Admins & Commercial Admins can see inactive pillars."""
+        """Maintainers, drivers, and admins can drive projects."""
         return (user.in_admin or
                 user.isOwner(self.obj.pillar) or
                 user.isOneOfDrivers(self.obj.pillar))
@@ -427,6 +432,13 @@
 class EditProduct(EditByOwnersOrAdmins):
     usedfor = IProduct
 
+    def checkAuthenticated(self, user):
+        # Commercial admins may help setup commercial projects.
+        return (
+            super(EditProduct, self).checkAuthenticated(user)
+            or is_commercial_case(self.obj, user)
+            or False)
+
 
 class EditPackaging(EditByOwnersOrAdmins):
     usedfor = IPackaging
@@ -1222,6 +1234,19 @@
         return self.obj.personHasDriverRights(user)
 
 
+class DriveProduct(SeriesDrivers):
+
+    permission = 'launchpad.Driver'
+    usedfor = IProduct
+
+    def checkAuthenticated(self, user):
+        # Commercial admins may help setup commercial projects.
+        return (
+            super(DriveProduct, self).checkAuthenticated(user)
+            or is_commercial_case(self.obj, user)
+            or False)
+
+
 class ViewProductSeries(AnonymousAuthorization):
 
     usedfor = IProductSeries