launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

William Grant has proposed merging lp:~wgrant/launchpad/bug-882902 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #882902 in Launchpad itself: "Bug.userCanView and get_bug_privacy_filter_with_decorator duplicate bug privacy logic"
  https://bugs.launchpad.net/launchpad/+bug/882902

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/bug-882902/+merge/80656

As part of the disclosure work we're making large/complete changes to the bug visibility logic. Our efforts are hampered by the duplication of this logic: once in procedural form in Bug.userCanView, and once as SQL in get_bug_privacy_filter. This branch replaces most of Bug.userCanView with a query based on get_bug_privacy_filter. A few uncontroversial short-circuits (handling public bugs, anonymous users, admins and existing cached viewers) remain implemented in Python, but everything else has a single SQL implementation.

There were some slight differences between the implementations. Most notably, userCanView has for some time allowed pillar owners to see bugs in their pillars, while get_bug_privacy_filter only recently gained such a policy, and then only under a feature flag which is presently disabled. Bug #702429 describes the situation a little. Bugs caught in this inconsistency are unsearchable, and render without any tasks. This branch will instead make them inaccessible until we turn the flag on -- after we are sure that projects are secure.

Several newish tests needed fixing to cope with owners no longer having access without the flag.
-- 
https://code.launchpad.net/~wgrant/launchpad/bug-882902/+merge/80656
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/bug-882902 into lp:launchpad.

=== modified file 'lib/lp/bugs/browser/tests/test_bugtask.py'
--- lib/lp/bugs/browser/tests/test_bugtask.py	2011-10-27 13:03:04 +0000
+++ lib/lp/bugs/browser/tests/test_bugtask.py	2011-10-28 09:39:38 +0000
@@ -56,10 +56,6 @@
     IBugTask,
     IBugTaskSet,
     )
-from lp.services.features.model import (
-    FeatureFlag,
-    getFeatureStore,
-    )
 from lp.services.features.testing import FeatureFixture
 from lp.services.propertycache import get_property_cache
 from lp.soyuz.interfaces.component import IComponentSet
@@ -997,9 +993,6 @@
             bug = self.factory.makeBug(product=first_product, private=True)
             bug_task = bug.bugtasks[0]
         second_product = self.factory.makeProduct(name='duck')
-        getFeatureStore().add(FeatureFlag(
-            scope=u'default', value=u'on', priority=1,
-            flag=u'disclosure.private_bug_visibility_rules.enabled'))
 
         # The first product owner can see the private bug. We will re-target
         # it to second_product where it will not be visible to that user.
@@ -1009,8 +1002,10 @@
                 'bunny.target.product': 'duck',
                 'bunny.actions.save': 'Save Changes',
                 }
-            view = create_initialized_view(
-                bug_task, name='+editstatus', form=form)
+            with FeatureFixture({
+                'disclosure.private_bug_visibility_rules.enabled': 'on'}):
+                view = create_initialized_view(
+                    bug_task, name='+editstatus', form=form)
             self.assertEqual(
                 canonical_url(bug_task.pillar, rootsite='bugs'),
                 view.next_url)

=== modified file 'lib/lp/bugs/mail/tests/test_commands.py'
--- lib/lp/bugs/mail/tests/test_commands.py	2011-10-25 02:12:44 +0000
+++ lib/lp/bugs/mail/tests/test_commands.py	2011-10-28 09:39:38 +0000
@@ -5,7 +5,6 @@
     IObjectCreatedEvent,
     IObjectModifiedEvent,
     )
-
 from canonical.testing.layers import (
     DatabaseFunctionalLayer,
     LaunchpadFunctionalLayer,
@@ -287,7 +286,7 @@
         product = self.factory.makeProduct()
         bug = self.factory.makeBug(private=True, product=product)
         self.factory.makeProduct(name='fnord')
-        login_person(bug.bugtasks[0].target.owner)
+        login_celebrity('admin')
         command = AffectsEmailCommand('affects', ['fnord'])
         error = self.assertRaises(
             EmailProcessingError, command.execute, bug, None)
@@ -348,7 +347,7 @@
 
     def test_execute_bug(self):
         bug = self.factory.makeBug()
-        login_person(bug.bugtasks[0].target.owner)
+        login_person(bug.owner)
         command = PrivateEmailCommand('private', ['yes'])
         exec_bug, event = command.execute(bug, None)
         self.assertEqual(bug, exec_bug)
@@ -386,7 +385,7 @@
 
     def test_execute_bug(self):
         bug = self.factory.makeBug()
-        login_person(bug.bugtasks[0].target.owner)
+        login_person(bug.owner)
         command = SecurityEmailCommand('security', ['yes'])
         exec_bug, event = command.execute(bug, None)
         self.assertEqual(bug, exec_bug)

=== modified file 'lib/lp/bugs/model/bug.py'
--- lib/lp/bugs/model/bug.py	2011-10-25 02:12:44 +0000
+++ lib/lp/bugs/model/bug.py	2011-10-28 09:39:38 +0000
@@ -190,7 +190,6 @@
     )
 from lp.registry.interfaces.product import IProduct
 from lp.registry.interfaces.productseries import IProductSeries
-from lp.registry.interfaces.role import IPersonRoles
 from lp.registry.interfaces.series import SeriesStatus
 from lp.registry.interfaces.sourcepackage import ISourcePackage
 from lp.registry.model.person import (
@@ -2090,6 +2089,10 @@
         authenticated users.  It is also called in other contexts where the
         user may be anonymous.
 
+        Most logic is delegated to the query provided by
+        get_bug_privacy_filter, but some short-circuits and caching are
+        reimplemented here.
+
         If bug privacy rights are changed here, corresponding changes need
         to be made to the queries which screen for privacy.  See
         Bug.searchAsUser and BugTask.get_bug_privacy_filter_with_decorator.
@@ -2104,29 +2107,13 @@
         if user.id in self._known_viewers:
             return True
 
-        elif IPersonRoles(user).in_admin:
-            # Admins can view all bugs.
+        filter = get_bug_privacy_filter(user)
+        store = Store.of(self)
+        store.flush()
+        if (not filter or
+            not store.find(Bug, Bug.id == self.id, filter).is_empty()):
+            self._known_viewers.add(user.id)
             return True
-        else:
-            # At this point we know the bug is private and the user is
-            # unprivileged.
-
-            # Assignees to bugtasks can see the private bug.
-            for bugtask in self.bugtasks:
-                if user.inTeam(bugtask.assignee):
-                    self._known_viewers.add(user.id)
-                    return True
-            # Explicit subscribers may also view it.
-            for subscription in self.subscriptions:
-                if user.inTeam(subscription.person):
-                    self._known_viewers.add(user.id)
-                    return True
-            # Pillar owners can view it. Note that this is contentious and
-            # possibly incorrect: see bug 702429.
-            for pillar_owner in [bt.pillar.owner for bt in self.bugtasks]:
-                if user.inTeam(pillar_owner):
-                    self._known_viewers.add(user.id)
-                    return True
         return False
 
     def linkHWSubmission(self, submission):

=== modified file 'lib/lp/bugs/stories/bug-also-affects/xx-bug-also-affects.txt'
--- lib/lp/bugs/stories/bug-also-affects/xx-bug-also-affects.txt	2011-10-25 02:12:44 +0000
+++ lib/lp/bugs/stories/bug-also-affects/xx-bug-also-affects.txt	2011-10-28 09:39:38 +0000
@@ -205,6 +205,8 @@
     >>> private_bug = firefox.createBug(params)
     >>> private_bug.setPrivate(True, current_user())
     True
+    >>> private_bug.subscribe(firefox.owner, firefox.owner)
+    <lp.bugs.model.bugsubscription.BugSubscription object at ...>
     >>> logout()
 
     >>> browser.open(canonical_url(private_bug, rootsite='bugs'))

=== modified file 'lib/lp/bugs/tests/test_bugchanges.py'
--- lib/lp/bugs/tests/test_bugchanges.py	2011-10-19 23:21:09 +0000
+++ lib/lp/bugs/tests/test_bugchanges.py	2011-10-28 09:39:38 +0000
@@ -27,6 +27,7 @@
 from lp.bugs.scripts.bugnotification import construct_email_notifications
 from lp.services.features.testing import FeatureFixture
 from lp.testing import (
+    celebrity_logged_in,
     login_person,
     person_logged_in,
     TestCaseWithFactory,
@@ -1086,7 +1087,7 @@
             bug, maintainer, bug_supervisor)
 
         # Now make the bug visible to the bug supervisor and re-test.
-        with person_logged_in(bug.default_bugtask.pillar.owner):
+        with celebrity_logged_in('admin'):
             bug.default_bugtask.transitionToAssignee(bug_supervisor)
 
         # Test with bug supervisor = maintainer.

=== modified file 'lib/lp/bugs/tests/test_bugvisibility.py'
--- lib/lp/bugs/tests/test_bugvisibility.py	2011-06-06 20:42:44 +0000
+++ lib/lp/bugs/tests/test_bugvisibility.py	2011-10-28 09:39:38 +0000
@@ -5,7 +5,7 @@
 
 from canonical.testing.layers import LaunchpadFunctionalLayer
 from lp.testing import (
-    person_logged_in,
+    celebrity_logged_in,
     TestCaseWithFactory,
     )
 
@@ -44,7 +44,7 @@
         self.bug_team = self.factory.makeTeam(
             name="bugteam", owner=self.product.owner)
         self.bug_team_member = self.factory.makePerson(name="bugteammember")
-        with person_logged_in(self.product.owner):
+        with celebrity_logged_in('admin'):
             self.bug_team.addMember(self.bug_team_member, self.product.owner)
             self.product.setBugSupervisor(
                 bug_supervisor=self.bug_team,
@@ -68,14 +68,14 @@
     def test_privateBugSubscriber(self):
         # A person subscribed to a private bug can see it.
         user = self.factory.makePerson()
-        with person_logged_in(self.owner):
+        with celebrity_logged_in('admin'):
             self.bug.subscribe(user, self.owner)
         self.assertTrue(self.bug.userCanView(user))
 
     def test_privateBugAssignee(self):
         # The bug assignee can see the private bug.
         bug_assignee = self.factory.makePerson(name="bugassignee")
-        with person_logged_in(self.product.owner):
+        with celebrity_logged_in('admin'):
             self.bug.default_bugtask.transitionToAssignee(bug_assignee)
         self.assertTrue(self.bug.userCanView(bug_assignee))
 

=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py	2011-10-27 13:03:04 +0000
+++ lib/lp/testing/factory.py	2011-10-28 09:39:38 +0000
@@ -1698,10 +1698,13 @@
                 getUtility(IBugWatchSet).fromText(bug_watch_url, bug, owner)
         bugtask = bug.default_bugtask
         if date_closed is not None:
-            bugtask.transitionToStatus(
-                BugTaskStatus.FIXRELEASED, owner, when=date_closed)
+            with person_logged_in(owner):
+                bugtask.transitionToStatus(
+                    BugTaskStatus.FIXRELEASED, owner, when=date_closed)
         if milestone is not None:
-            bugtask.transitionToMilestone(milestone, milestone.target.owner)
+            with person_logged_in(owner):
+                bugtask.transitionToMilestone(
+                    milestone, milestone.target.owner)
         if series is not None:
             with person_logged_in(owner):
                 task = bug.addTask(owner, series)