launchpad-reviewers team mailing list archive

[Curtis Hovey <curtis.hovey@xxxxxxxxxxxxx>]

Curtis Hovey has proposed merging lp:~sinzui/launchpad/project-branch-permissions into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1038156 in Launchpad itself: "Project maintainers cannot edit branch data to prevent disclosure"
  https://bugs.launchpad.net/launchpad/+bug/1038156

For more details, see:
https://code.launchpad.net/~sinzui/launchpad/project-branch-permissions/+merge/120219

Project maintainers cannot update branches owned by contributors to
secure the project.

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

RULES

    Pre-implementation: jcsackett
    * Several attributes on IBranchEditableAttributes and IBranchEdit
      can be moved to IBranchModerate.
    * Add a security checker for launchpad.Moderate on IBranchModerate
      that permits the anyone with launchpad.Edit + the product.owner
      and commercial admins permission to change the data.


QA

    * Visit https://code.qastaging.launchpad.net/lp-dev-utils
    * Verify the branches owned by former Canonical employees are Private
    * Run the test api script
    * Verify all the branches are Private branches are now Proprietary

    {{{
    import logging
    from launchpadlib.launchpad import Launchpad


    logging.basicConfig()
    log = logging.getLogger(SCRIPT_NAME)
    log.setLevel(logging.INFO)


    lp = Launchpad.login_with(
        'testing', service_root='https://api.qastaging.launchpad.net',
        version='devel')
    project = lp.projects['lp-dev-utils']
    statuses = [
        'Experimental', 'Development', 'Mature', 'Merged', 'Abandoned']
    for branch in project.getBranches(status=statuses):
        if branch.information_type == 'Private':
            try:
                log.info(
                    'Changing %s information type to Proprietary',
                    branch.unique_name)
                branch.transitionToInformationType(
                    information_type='Proprietary')
            except:
                log.info(
                    'Failed to change %s information type.',
                    branch.unique_name)
    }}


LINT

    lib/lp/security.py
    lib/lp/code/configure.zcml
    lib/lp/code/interfaces/branch.py
    lib/lp/code/model/tests/test_branch.py


TEST

    ./bin/test -vvc -t BranchModerateTestCase lp.code.model.tests.test_branch


IMPLEMENTATION

Added a security checker for launchpad.Moderate on branches. Anyone with
launchpad.Edit plus the product.owner and commercial admins have
permission.
    lib/lp/security.py

Created IBranchModerateAttributes and moved name, description, reviewer,
and lifecycle_status from IBranchEditableAttributes. Created
IBranchModerate and moved transitionToInformationType() from
IBranchEditable.
    lib/lp/code/configure.zcml
    lib/lp/code/interfaces/branch.py
    lib/lp/code/model/tests/test_branch.py

-- 
https://code.launchpad.net/~sinzui/launchpad/project-branch-permissions/+merge/120219
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~sinzui/launchpad/project-branch-permissions into lp:launchpad.

=== modified file 'lib/lp/code/configure.zcml'
--- lib/lp/code/configure.zcml	2012-04-27 16:22:05 +0000
+++ lib/lp/code/configure.zcml	2012-08-17 18:55:24 +0000
@@ -446,11 +446,16 @@
         permission="launchpad.View"
         interface="lp.app.interfaces.launchpad.IPrivacy
                    lp.code.interfaces.branch.IBranchAnyone
+                   lp.code.interfaces.branch.IBranchModerateAttributes
                    lp.code.interfaces.branch.IBranchEditableAttributes
                    lp.code.interfaces.branch.IBranchPublic
                    lp.code.interfaces.branch.IBranchView"
         attributes="merge_queue merge_queue_config"/>
     <require
+        permission="launchpad.Moderate"
+        interface="lp.code.interfaces.branch.IBranchModerate"
+        set_schema="lp.code.interfaces.branch.IBranchModerateAttributes" />
+    <require
         permission="launchpad.Edit"
         interface="lp.code.interfaces.branch.IBranchEdit"
         set_schema="lp.code.interfaces.branch.IBranchEditableAttributes"

=== modified file 'lib/lp/code/interfaces/branch.py'
--- lib/lp/code/interfaces/branch.py	2012-08-16 20:13:14 +0000
+++ lib/lp/code/interfaces/branch.py	2012-08-17 18:55:24 +0000
@@ -981,11 +981,8 @@
         """
 
 
-class IBranchEditableAttributes(Interface):
-    """IBranch attributes that can be edited.
-
-    These attributes need launchpad.View to see, and launchpad.Edit to change.
-    """
+class IBranchModerateAttributes(Interface):
+    """IBranch attributes that can be edited by a more than one community."""
 
     name = exported(
         TextLine(
@@ -1004,6 +1001,44 @@
                           "that is responsible for reviewing proposals and "
                           "merging into this branch.")))
 
+    description = exported(
+        Text(
+            title=_('Description'), required=False,
+            description=_(
+                'A short description of the changes in this branch.')))
+
+    lifecycle_status = exported(
+        Choice(
+            title=_('Status'), vocabulary=BranchLifecycleStatus,
+            default=BranchLifecycleStatus.DEVELOPMENT))
+
+
+class IBranchModerate(Interface):
+    """IBranch methods that can be edited by a more than one community."""
+
+    @operation_parameters(
+        information_type=copy_field(IBranchPublic['information_type']),
+        )
+    @call_with(who=REQUEST_USER, verify_policy=True)
+    @export_write_operation()
+    @operation_for_version("devel")
+    def transitionToInformationType(information_type, who,
+                                    verify_policy=True):
+        """Set the information type for this branch.
+
+        :param information_type: The `InformationType` to transition to.
+        :param who: The `IPerson` who is making the change.
+        :param verify_policy: Check if the new information type complies
+            with the `IBranchNamespacePolicy`.
+        """
+
+
+class IBranchEditableAttributes(Interface):
+    """IBranch attributes that can be edited.
+
+    These attributes need launchpad.View to see, and launchpad.Edit to change.
+    """
+
     url = exported(
         BranchURIField(
             title=_('Branch URL'), required=False,
@@ -1027,17 +1062,6 @@
             title=_("Branch Type"), required=True, readonly=True,
             vocabulary=BranchType))
 
-    description = exported(
-        Text(
-            title=_('Description'), required=False,
-            description=_(
-                'A short description of the changes in this branch.')))
-
-    lifecycle_status = exported(
-        Choice(
-            title=_('Status'), vocabulary=BranchLifecycleStatus,
-            default=BranchLifecycleStatus.DEVELOPMENT))
-
     branch_format = exported(
         Choice(
             title=_("Branch Format"),
@@ -1132,22 +1156,6 @@
         :raise: CannotDeleteBranch if the branch cannot be deleted.
         """
 
-    @operation_parameters(
-        information_type=copy_field(IBranchPublic['information_type']),
-        )
-    @call_with(who=REQUEST_USER, verify_policy=True)
-    @export_write_operation()
-    @operation_for_version("devel")
-    def transitionToInformationType(information_type, who,
-                                    verify_policy=True):
-        """Set the information type for this branch.
-
-        :param information_type: The `InformationType` to transition to.
-        :param who: The `IPerson` who is making the change.
-        :param verify_policy: Check if the new information type complies
-            with the `IBranchNamespacePolicy`.
-        """
-
 
 class IMergeQueueable(Interface):
     """An interface for branches that can be queued."""
@@ -1197,7 +1205,8 @@
 
 
 class IBranch(IBranchPublic, IBranchView, IBranchEdit,
-              IBranchEditableAttributes, IBranchAnyone, IMergeQueueable):
+              IBranchEditableAttributes, IBranchModerate,
+              IBranchModerateAttributes, IBranchAnyone, IMergeQueueable):
     """A Bazaar branch."""
 
     # Mark branches as exported entries for the Launchpad API.

=== modified file 'lib/lp/code/model/tests/test_branch.py'
--- lib/lp/code/model/tests/test_branch.py	2012-08-17 05:05:37 +0000
+++ lib/lp/code/model/tests/test_branch.py	2012-08-17 18:55:24 +0000
@@ -138,6 +138,7 @@
     )
 from lp.services.osutils import override_environ
 from lp.services.propertycache import clear_property_cache
+from lp.services.webapp.authorization import check_permission
 from lp.services.webapp.interfaces import IOpenLaunchBag
 from lp.testing import (
     admin_logged_in,
@@ -2582,8 +2583,50 @@
             InformationType.PRIVATESECURITY, branch.information_type)
 
 
+class BranchModerateTestCase(TestCaseWithFactory):
+    """Test that product owners and commercial admins can moderate branches."""
+
+    layer = DatabaseFunctionalLayer
+
+    def test_moderate_permission(self):
+        # Test the ModerateBranch security checker.
+        branch = self.factory.makeProductBranch()
+        with person_logged_in(branch.product.owner):
+            self.assertTrue(
+                check_permission('launchpad.Moderate', branch))
+        with celebrity_logged_in('commercial_admin'):
+            self.assertTrue(
+                check_permission('launchpad.Moderate', branch))
+
+    def test_methods_smoketest(self):
+        # Users with launchpad.Moderate can call transitionToInformationType.
+        branch = self.factory.makeProductBranch()
+        with celebrity_logged_in('commercial_admin') as admin:
+            branch.product.setBranchSharingPolicy(
+                BranchSharingPolicy.PUBLIC, admin)
+        with person_logged_in(branch.product.owner):
+            branch.transitionToInformationType(
+                InformationType.PRIVATESECURITY, branch.product.owner)
+        self.assertEqual(
+            InformationType.PRIVATESECURITY, branch.information_type)
+
+    def test_attribute_smoketest(self):
+        # Users with launchpad.Moderate can set attrs.
+        branch = self.factory.makeProductBranch()
+        with person_logged_in(branch.product.owner):
+            branch.name = 'not-secret'
+            branch.description = 'redacted'
+            branch.reviewer = branch.product.owner
+            branch.lifecycle_status = BranchLifecycleStatus.EXPERIMENTAL
+        self.assertEqual('not-secret', branch.name)
+        self.assertEqual('redacted', branch.description)
+        self.assertEqual(branch.product.owner, branch.reviewer)
+        self.assertEqual(
+            BranchLifecycleStatus.EXPERIMENTAL, branch.lifecycle_status)
+
+
 class TestBranchCommitsForDays(TestCaseWithFactory):
-    """Tests for `Branch.commitsForDays`."""
+    """Tests for `Branch.commitsFornDays`."""
 
     layer = DatabaseFunctionalLayer
 

=== modified file 'lib/lp/security.py'
--- lib/lp/security.py	2012-08-14 23:27:07 +0000
+++ lib/lp/security.py	2012-08-17 18:55:24 +0000
@@ -2095,6 +2095,19 @@
                 and user.inTeam(code_import.registrant)))
 
 
+class ModerateBranch(EditBranch):
+    """The owners, product owners, and admins can moderate branches."""
+    permission = 'launchpad.Moderate'
+
+    def checkAuthenticated(self, user):
+        if super(ModerateBranch, self).checkAuthenticated(user):
+            return True
+        branch = self.obj
+        if branch.product is not None and user.inTeam(branch.product.owner):
+            return True
+        return user.in_commercial_admin
+
+
 def can_upload_linked_package(person_role, branch):
     """True if person may upload the package linked to `branch`."""
     # No associated `ISuiteSourcePackage` data -> not an official branch.