Merge lp:~stevenk/launchpad/link-branch-to-private-bug into lp:launchpad

[Steve Kowalik]

When a branch is linked to a private bug, the branch should be made private too.

Clean up a bunch of lint too.

[Robert Collins (lifeless)]

So, this is problematic because it will grant private branches (a commercial feature) to anything that has security bugs.

It also doesn't require a security policy, which I suspect will make other parts of LP fall over in a heap.

-> I think this needs more consideration, specifically around the cases for how these things should interact.

[Robert Collins (lifeless)]

Oh, and it will also violate an existing axiom:
 - public branches can never become private

And will be surprising for folk that have toggle a branch public explicitly to have it become private when *anyone* links it to a private bug.

I'm 'upgrading' my review to needs-fixing (and I think it needs design level fixing, not a small matter of code).

review: Needs Fixing

Unmerged revisions

13688. By Steve Kowalik on 2011-08-15

Make the branch private if the bug is when linking the two together, and lint.

=== modified file 'lib/lp/bugs/model/bug.py'
--- lib/lp/bugs/model/bug.py	2011-08-01 05:56:43 +0000
+++ lib/lp/bugs/model/bug.py	2011-08-15 06:09:25 +0000
@@ -1275,6 +1275,9 @@
             if bug_branch.branch == branch:
                 return bug_branch
 
+        # If the bug is private, we should make the branch private too.
+        if self.private:
+            removeSecurityProxy(branch).private = True
         bug_branch = BugBranch(
             branch=branch, bug=self, registrant=registrant)
         branch.date_last_modified = UTC_NOW

=== modified file 'lib/lp/bugs/model/bugbranch.py'
--- lib/lp/bugs/model/bugbranch.py	2011-05-27 21:12:25 +0000
+++ lib/lp/bugs/model/bugbranch.py	2011-08-15 06:09:25 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 # pylint: disable-msg=E0611,W0212
@@ -7,8 +7,10 @@
 
 __metaclass__ = type
 
-__all__ = ["BugBranch",
-           "BugBranchSet"]
+__all__ = [
+    "BugBranch",
+    "BugBranchSet"
+    ]
 
 from sqlobject import (
     ForeignKey,
@@ -24,6 +26,7 @@
     )
 from zope.component import getUtility
 from zope.interface import implements
+from zope.security.interfaces import Unauthorized
 
 from canonical.database.constants import UTC_NOW
 from canonical.database.datetimecol import UtcDateTimeCol
@@ -82,7 +85,12 @@
         from lp.bugs.model.bug import Bug
         from lp.bugs.model.bugsubscription import BugSubscription
 
-        branch_ids = [branch.id for branch in branches]
+        branch_ids = []
+        for branch in branches:
+            try:
+                branch_ids.append(branch.id)
+            except Unauthorized:
+                continue # User has no permission for this branch.
         if branch_ids == []:
             return []
 

=== modified file 'lib/lp/bugs/tests/test_bugbranch.py'
--- lib/lp/bugs/tests/test_bugbranch.py	2011-02-10 14:42:11 +0000
+++ lib/lp/bugs/tests/test_bugbranch.py	2011-08-15 06:09:25 +0000
@@ -20,6 +20,7 @@
 from lp.testing import (
     anonymous_logged_in,
     celebrity_logged_in,
+    person_logged_in,
     TestCaseWithFactory,
     )
 
@@ -46,7 +47,6 @@
         branch_1 = self.factory.makeBranch()
         branch_2 = self.factory.makeBranch()
         bug_a = self.factory.makeBug()
-        bug_b = self.factory.makeBug()
         self.factory.loginAsAnyone()
         bug_a.linkBranch(branch_1, self.factory.makePerson())
         bug_a.linkBranch(branch_2, self.factory.makePerson())
@@ -120,15 +120,17 @@
     def test_getBranchesWithVisibleBugs_shows_private_bugs_to_sub(self):
         # getBranchesWithVisibleBugs will show private bugs to their
         # subscribers.
-        branch = self.factory.makeBranch()
+        user = self.factory.makePerson()
+        branch = self.factory.makeBranch(owner=user)
         bug = self.factory.makeBug(private=True)
-        user = self.factory.makePerson()
         with celebrity_logged_in('admin'):
             bug.subscribe(user, self.factory.makePerson())
             bug.linkBranch(branch, self.factory.makePerson())
         utility = getUtility(IBugBranchSet)
-        self.assertContentEqual(
-            [branch.id], utility.getBranchesWithVisibleBugs([branch], user))
+        with person_logged_in(user):
+            self.assertContentEqual(
+                [branch.id],
+                utility.getBranchesWithVisibleBugs([branch], user))
 
     def test_getBugBranchesForBugTasks(self):
         # IBugBranchSet.getBugBranchesForBugTasks returns all of the BugBranch
@@ -264,3 +266,13 @@
         self.factory.loginAsAnyone()
         bug.linkBranch(branch, self.factory.makePerson())
         self.assertTrue(bug.date_last_updated > last_updated)
+
+    def test_linking_branch_to_private_bug_makes_branch_private(self):
+        # Linking a branch to a private bug forces the branch to be private.
+        reporter = self.factory.makePerson()
+        bug = self.factory.makeBug(private=True, owner=reporter)
+        product = self.factory.makeProduct()
+        branch = self.factory.makeBranch(owner=reporter, product=product)
+        with person_logged_in(reporter):
+            bug.linkBranch(branch, reporter)
+            self.assertTrue(branch.private)