Merge lp:~wallyworld/launchpad/pillar-owners-private-bug-visibility-702429 into lp:launchpad

[Ian Booth]

[r=sinzui][bug=702429] Bugtask pillar owners can see private bugs.

Add extra search terms to get_bug_privacy_filter_with_decorator() so that pillar owners can see private bugs.

== Implementation ==

The bug report is a little out of date. BugTaskResultset doesn't exist any more and Bug.userCanView has already been updated to do the right thing. What remained was to update the construction of the filter terms used in bug queries.

BugTask.get_bug_privacy_filter_with_decorator() was updated. Then Bug.searchAsUser was inspected and instead of just copy and paste of the SQL, there was no reason I could see why I couldn't just call the existing BigTask.get_bug_privacy_filter() method. This allows the required SQL to be set up in only one place.

Also did some drive by lint fixes.

== Tests ==

Add a new test to test_bugtask_search:
  - test_private_bug_in_search_result_pillar_owners()

Also, I broke up the existing large test_private_bug_in_search_result test into a number of individual tests.

bin/test -vvct test_bugtask -t bug.txt

== Lint ==

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/bugs/model/bug.py
  lib/lp/bugs/model/bugtask.py
  lib/lp/bugs/tests/test_bugtask_search.py

[Robert Collins (lifeless)]

looks broadly fine but:
 - needs a feature flag to toggle on the new behaviour - we are likely to have perf issues and this lets us react quickly without panicking.
 - you need to update the security rules as well; bug *searches* inject visibility, but direct landing on such a bug won't, the user will still get a 404.

review: Needs Fixing

[Ian Booth (wallyworld)]

> looks broadly fine but:
> - needs a feature flag to toggle on the new behaviour - we are likely to have
> perf issues and this lets us react quickly without panicking.
> - you need to update the security rules as well; bug *searches* inject
> visibility, but direct landing on such a bug won't, the user will still get a
> 404.

I've introduced a new feature flag: disclosure.private_bug_visibility_rules.enabled
This will be used also for the work to allow defined project roles to see private bugs as per bug 696954.

The Bug.userCanView method had already been updated to include pillar owners so I believe direct landings already work as they should.

[Curtis Hovey (sinzui)]

I do not think the privacy access rules are correct. They do not take security_related into account per the PDF and the email I sent about giving maintainers access to private non-security items. I imagined that we would deal with the security contact role first to establish the exceptional behaviour of private and security_related that no other changes can break. I am fine with landing this first, but we must be certain that if the maintainer delegates security issues to another person, he does not have access to the private security_related bugs.

review: Needs Fixing (curtis)

[Ian Booth (wallyworld)]

I have updated the privacy filter to exclude security related bugs for maintainers.
The test_private_bug_in_search_result_pillar_owners test has also been updated.

[Curtis Hovey (sinzui)]

Thank you for making the revisions. I think the branch established some good rules to extend to other project roles in the future.

review: Approve (code)

=== modified file 'lib/lp/bugs/model/bug.py'
--- lib/lp/bugs/model/bug.py	2011-09-23 13:14:36 +0000
+++ lib/lp/bugs/model/bug.py	2011-09-26 02:10:30 +0000
@@ -170,6 +170,7 @@
 from lp.bugs.model.bugtask import (
     BugTask,
     bugtask_sort_key,
+    get_bug_privacy_filter
     )
 from lp.bugs.model.bugwatch import BugWatch
 from lp.bugs.model.structuralsubscription import (
@@ -2602,35 +2603,9 @@
         if duplicateof:
             where_clauses.append("Bug.duplicateof = %d" % duplicateof.id)
 
-        admins = getUtility(ILaunchpadCelebrities).admin
-        if user:
-            if not user.inTeam(admins):
-                # Enforce privacy-awareness for logged-in, non-admin users,
-                # so that they can only see the private bugs that they're
-                # allowed to see.
-                where_clauses.append("""
-                    (Bug.private = FALSE OR
-                      Bug.id in (
-                         -- Users who have a subscription to this bug.
-                         SELECT BugSubscription.bug
-                           FROM BugSubscription, TeamParticipation
-                           WHERE
-                             TeamParticipation.person = %(personid)s AND
-                             BugSubscription.person = TeamParticipation.team
-                         UNION
-                         -- Users who are the assignee for one of the bug's
-                         -- bugtasks.
-                         SELECT BugTask.bug
-                           FROM BugTask, TeamParticipation
-                           WHERE
-                             TeamParticipation.person = %(personid)s AND
-                             TeamParticipation.team = BugTask.assignee
-                      )
-                    )""" % sqlvalues(personid=user.id))
-        else:
-            # Anonymous user; filter to include only public bugs in
-            # the search results.
-            where_clauses.append("Bug.private = FALSE")
+        privacy_filter = get_bug_privacy_filter(user)
+        if privacy_filter:
+            where_clauses.append(privacy_filter)
 
         other_params = {}
         if orderBy:

=== modified file 'lib/lp/bugs/model/bugtask.py'
--- lib/lp/bugs/model/bugtask.py	2011-09-12 20:27:32 +0000
+++ lib/lp/bugs/model/bugtask.py	2011-09-26 02:10:30 +0000
@@ -1403,7 +1403,45 @@
     # part of the WHERE condition (i.e. the bit below.) The
     # other half of this condition (see code above) does not
     # use TeamParticipation at all.
-    return ("""
+    pillar_privacy_filters = ''
+    if features.getFeatureFlag(
+        'disclosure.private_bug_visibility_rules.enabled'):
+        pillar_privacy_filters = """
+             UNION
+             SELECT BugTask.bug
+             FROM BugTask, TeamParticipation, Product
+             WHERE TeamParticipation.person = %(personid)s AND
+                   TeamParticipation.team = Product.owner AND
+                   BugTask.product = Product.id AND
+                   BugTask.bug = Bug.id AND
+                   Bug.security_related IS False
+             UNION
+             SELECT BugTask.bug
+             FROM BugTask, TeamParticipation, ProductSeries
+             WHERE TeamParticipation.person = %(personid)s AND
+                   TeamParticipation.team = ProductSeries.owner AND
+                   BugTask.productseries = ProductSeries.id AND
+                   BugTask.bug = Bug.id AND
+                   Bug.security_related IS False
+             UNION
+             SELECT BugTask.bug
+             FROM BugTask, TeamParticipation, Distribution
+             WHERE TeamParticipation.person = %(personid)s AND
+                   TeamParticipation.team = Distribution.owner AND
+                   BugTask.distribution = Distribution.id AND
+                   BugTask.bug = Bug.id AND
+                   Bug.security_related IS False
+             UNION
+             SELECT BugTask.bug
+             FROM BugTask, TeamParticipation, DistroSeries, Distribution
+             WHERE TeamParticipation.person = %(personid)s AND
+                   TeamParticipation.team = Distribution.owner AND
+                   DistroSeries.distribution = Distribution.id AND
+                   BugTask.distroseries = DistroSeries.id AND
+                   BugTask.bug = Bug.id AND
+                   Bug.security_related IS False
+        """ % sqlvalues(personid=user.id)
+    query = """
         (Bug.private = FALSE OR EXISTS (
              SELECT BugSubscription.bug
              FROM BugSubscription, TeamParticipation
@@ -1416,9 +1454,12 @@
              WHERE TeamParticipation.person = %(personid)s AND
                    TeamParticipation.team = BugTask.assignee AND
                    BugTask.bug = Bug.id
+             %(extra_filters)s
                    ))
-                     """ % sqlvalues(personid=user.id),
-        _make_cache_user_can_view_bug(user))
+        """ % dict(
+                personid=quote(user.id),
+                extra_filters=pillar_privacy_filters)
+    return query, _make_cache_user_can_view_bug(user)
 
 
 def build_tag_set_query(joiner, tags):

=== modified file 'lib/lp/bugs/tests/test_bugtask_search.py'
--- lib/lp/bugs/tests/test_bugtask_search.py	2011-06-20 07:03:18 +0000
+++ lib/lp/bugs/tests/test_bugtask_search.py	2011-09-26 02:10:30 +0000
@@ -54,6 +54,9 @@
     )
 from lp.testing.matchers import HasQueryCount
 
+PRIVATE_BUG_VISIBILITY_FLAG = {
+    'disclosure.private_bug_visibility_rules.enabled': 'on'}
+
 
 class SearchTestBase:
     """A mixin class with tests useful for all targets and search variants."""
@@ -92,20 +95,26 @@
         params = self.getBugTaskSearchParams(user=None)
         self.assertSearchFinds(params, self.bugtasks)
 
-    def test_private_bug_in_search_result(self):
+    def test_private_bug_in_search_result_anonymous_users(self):
         # Private bugs are not included in search results for anonymous users.
         with person_logged_in(self.owner):
             self.bugtasks[-1].bug.setPrivate(True, self.owner)
         params = self.getBugTaskSearchParams(user=None)
         self.assertSearchFinds(params, self.bugtasks[:-1])
 
+    def test_private_bug_in_search_result_unauthorised_users(self):
         # Private bugs are not included in search results for ordinary users.
+        with person_logged_in(self.owner):
+            self.bugtasks[-1].bug.setPrivate(True, self.owner)
         user = self.factory.makePerson()
         params = self.getBugTaskSearchParams(user=user)
         self.assertSearchFinds(params, self.bugtasks[:-1])
 
+    def test_private_bug_in_search_result_subscribers(self):
         # If the user is subscribed to the bug, it is included in the
         # search result.
+        with person_logged_in(self.owner):
+            self.bugtasks[-1].bug.setPrivate(True, self.owner)
         user = self.factory.makePerson()
         admin = getUtility(IPersonSet).getByEmail('[e-mail]')
         with person_logged_in(admin):
@@ -114,18 +123,49 @@
         params = self.getBugTaskSearchParams(user=user)
         self.assertSearchFinds(params, self.bugtasks)
 
+    def test_private_bug_in_search_result_admins(self):
         # Private bugs are included in search results for admins.
+        with person_logged_in(self.owner):
+            self.bugtasks[-1].bug.setPrivate(True, self.owner)
+        admin = getUtility(IPersonSet).getByEmail('[e-mail]')
         params = self.getBugTaskSearchParams(user=admin)
         self.assertSearchFinds(params, self.bugtasks)
 
+    def test_private_bug_in_search_result_assignees(self):
         # Private bugs are included in search results for the assignee.
+        with person_logged_in(self.owner):
+            self.bugtasks[-1].bug.setPrivate(True, self.owner)
+        bugtask = self.bugtasks[-1]
         user = self.factory.makePerson()
-        bugtask = self.bugtasks[-1]
+        admin = getUtility(IPersonSet).getByEmail('[e-mail]')
         with person_logged_in(admin):
             bugtask.transitionToAssignee(user)
         params = self.getBugTaskSearchParams(user=user)
         self.assertSearchFinds(params, self.bugtasks)
 
+    def test_private_bug_in_search_result_pillar_owners(self):
+        # Private, non-security bugs are included in search results for the
+        # pillar owners if the correct feature flag is enabled.
+        bugtask = self.bugtasks[-1]
+        pillar_owner = bugtask.pillar.owner
+        with person_logged_in(self.owner):
+            bugtask.bug.setPrivate(True, self.owner)
+            bugtask.bug.unsubscribe(pillar_owner, self.owner)
+        params = self.getBugTaskSearchParams(user=pillar_owner)
+        # Check the results with the feature flag.
+        with FeatureFixture(PRIVATE_BUG_VISIBILITY_FLAG):
+            self.assertSearchFinds(params, self.bugtasks)
+        # Check the results without the feature flag.
+        self.assertSearchFinds(params, self.bugtasks[:-1])
+
+        # Make the bugtask security related.
+        with person_logged_in(self.owner):
+            bugtask.bug.setSecurityRelated(True, self.owner)
+            bugtask.bug.unsubscribe(pillar_owner, self.owner)
+        # It should now be excluded from the results.
+        with FeatureFixture(PRIVATE_BUG_VISIBILITY_FLAG):
+            self.assertSearchFinds(params, self.bugtasks[:-1])
+
     def test_search_by_bug_reporter(self):
         # Search results can be limited to bugs filed by a given person.
         bugtask = self.bugtasks[0]
@@ -415,10 +455,10 @@
         utc_now = datetime.now(pytz.timezone('UTC'))
         self.assertTrue(utc_now >= self.bugtasks[2].date_closed)
         params = self.getBugTaskSearchParams(
-            user=None, date_closed=greater_than(utc_now-timedelta(days=1)))
+            user=None, date_closed=greater_than(utc_now - timedelta(days=1)))
         self.assertSearchFinds(params, self.bugtasks[2:])
         params = self.getBugTaskSearchParams(
-            user=None, date_closed=greater_than(utc_now+timedelta(days=1)))
+            user=None, date_closed=greater_than(utc_now + timedelta(days=1)))
         self.assertSearchFinds(params, [])
 
     def test_created_since(self):
@@ -561,7 +601,6 @@
     def test_deactivated_listings_not_seen(self):
         # Someone without permission to see deactiveated projects does
         # not see bugtasks for deactivated projects.
-        nopriv = getUtility(IPersonSet).getByEmail('[e-mail]')
         bugtask_set = getUtility(IBugTaskSet)
         param = BugTaskSearchParams(user=None, fast_searchtext='Monkeys')
         results = bugtask_set.search(param, _noprejoins=True)
@@ -607,13 +646,13 @@
 
 class BugTargetTestBase:
     """A base class for the bug target mixin classes.
-    
+
     :ivar searchtarget: A bug context to search within.
     :ivar searchtarget2: A sibling bug context for testing cross-context
         searches. Created on demand when
         getBugTaskSearchParams(multitarget=True) is called.
-    :ivar bugtasks2: Bugtasks created for searchtarget2. Twice as many are made
-        as for searchtarget.
+    :ivar bugtasks2: Bugtasks created for searchtarget2. Twice as many are
+        made as for searchtarget.
     :ivar group_on: The columns to group on when calling countBugs. None
         if the target being testing is not sensible/implemented for counting
         bugs. For instance, grouping by project group may be interesting but
@@ -872,7 +911,7 @@
                 owner=self.owner, project=self.searchtarget)
             bug1 = self.factory.makeBug(product=product1)
             bug1.default_bugtask.updateTargetNameCache()
-            bug2 = self.factory.makeBug(product=product2)
+            self.factory.makeBug(product=product2)
         params = self.getBugTaskSearchParams(user=None, searchtext='uct-fo')
         # With no flag, we find the first bug.
         self.assertSearchFinds(params, [bug1.default_bugtask])

=== modified file 'lib/lp/services/features/flags.py'
--- lib/lp/services/features/flags.py	2011-09-16 16:19:22 +0000
+++ lib/lp/services/features/flags.py	2011-09-26 02:10:30 +0000
@@ -133,6 +133,11 @@
      'boolean',
      ('Enables the display and use of the enhanced target pickers.'),
      ''),
+    ('disclosure.private_bug_visibility_rules.enabled',
+     'boolean',
+     ('Enables the application of additional privacy filter terms in bug '
+      'queries to allow defined project roles to see private bugs.'),
+     ''),
     ('bugs.autoconfirm.enabled_distribution_names',
      'space delimited',
      ('Enables auto-confirming bugtasks for distributions (and their '