Merge lp:~wgrant/launchpad/remove-extra-privacy into lp:launchpad

[William Grant]

[r=stevenk][no-qa] Clean up obsolete bug visibility feature flags and code.

This branch removes two bug visibility feature flags.

disclosure.private_bug_visibility_cte.enabled turned on some performance-sensitive query changes. It's been enabled on production for some months now without a problem, so the old non-CTE code can go away.

disclosure.private_bug_visibility_rules.enabled was never enabled beyond ~launchpad, has security issues around multi-tenancy, is non-performant, and will soon be replaced by modern disclosure. I've removed the code enabled by this flag, reverting ~launchpad to the old behaviour.

This is preparation for the refactoring of bug searches to use a flattened fact table.

[Steve Kowalik (stevenk)]

This looks like excellent work, thanks!

review: Approve (code)

=== modified file 'lib/lp/bugs/browser/tests/test_bugtask.py'
--- lib/lp/bugs/browser/tests/test_bugtask.py	2012-02-08 10:43:29 +0000
+++ lib/lp/bugs/browser/tests/test_bugtask.py	2012-02-15 03:24:20 +0000
@@ -1403,41 +1403,6 @@
         notifications = view.request.response.notifications
         self.assertEqual(0, len(notifications))
 
-    def test_retarget_private_bug(self):
-        # If a private bug is re-targetted such that the bug is no longer
-        # visible to the user, they are redirected to the pillar's bug index
-        # page with a suitable message. This corner case can occur when the
-        # disclosure.private_bug_visibility_rules.enabled feature flag is on
-        # and a bugtask is re-targetted to a pillar for which the user is not
-        # authorised to see any private bugs.
-        first_product = self.factory.makeProduct(name='bunny')
-        with person_logged_in(first_product.owner):
-            bug = self.factory.makeBug(product=first_product, private=True)
-            bug_task = bug.bugtasks[0]
-        second_product = self.factory.makeProduct(name='duck')
-
-        # 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.
-        with person_logged_in(first_product.owner):
-            form = {
-                'bunny.target': 'product',
-                'bunny.target.product': 'duck',
-                'bunny.actions.save': 'Save Changes',
-                }
-            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)
-        self.assertEqual([], view.errors)
-        self.assertEqual(second_product, bug_task.target)
-        notifications = view.request.response.notifications
-        self.assertEqual(1, len(notifications))
-        expected = ('The bug you have just updated is now a private bug for')
-        self.assertTrue(notifications.pop().message.startswith(expected))
-
 
 class TestPersonBugs(TestCaseWithFactory):
     """Test the bugs overview page for distributions."""

=== modified file 'lib/lp/bugs/model/bugtasksearch.py'
--- lib/lp/bugs/model/bugtasksearch.py	2012-02-09 01:42:08 +0000
+++ lib/lp/bugs/model/bugtasksearch.py	2012-02-15 03:24:20 +0000
@@ -1464,118 +1464,23 @@
     # 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.
-    pillar_privacy_filters = ''
-    if features.getFeatureFlag(
-        'disclosure.private_bug_visibility_cte.enabled'):
-        if features.getFeatureFlag(
-            'disclosure.private_bug_visibility_rules.enabled'):
-            pillar_privacy_filters = """
-                UNION ALL
-                SELECT BugTask.bug
-                FROM BugTask, Product
-                WHERE Product.owner IN (SELECT team FROM teams) AND
-                    BugTask.product = Product.id AND
-                    BugTask.bug = Bug.id AND
-                    Bug.security_related IS False
-                UNION ALL
-                SELECT BugTask.bug
-                FROM BugTask, ProductSeries
-                WHERE ProductSeries.owner IN (SELECT team FROM teams) AND
-                    BugTask.productseries = ProductSeries.id AND
-                    BugTask.bug = Bug.id AND
-                    Bug.security_related IS False
-                UNION ALL
-                SELECT BugTask.bug
-                FROM BugTask, Distribution
-                WHERE Distribution.owner IN (SELECT team FROM teams) AND
-                    BugTask.distribution = Distribution.id AND
-                    BugTask.bug = Bug.id AND
-                    Bug.security_related IS False
-                UNION ALL
-                SELECT BugTask.bug
-                FROM BugTask, DistroSeries, Distribution
-                WHERE Distribution.owner IN (SELECT team FROM teams) AND
-                    DistroSeries.distribution = Distribution.id AND
-                    BugTask.distroseries = DistroSeries.id AND
-                    BugTask.bug = Bug.id AND
-                    Bug.security_related IS False
-            """
-        query = """
-            (%(public_bug_filter)s EXISTS (
-                WITH teams AS (
-                    SELECT team from TeamParticipation
-                    WHERE person = %(personid)s
-                )
-                SELECT BugSubscription.bug
-                FROM BugSubscription
-                WHERE BugSubscription.person IN (SELECT team FROM teams) AND
-                    BugSubscription.bug = Bug.id
-                UNION ALL
-                SELECT BugTask.bug
-                FROM BugTask
-                WHERE BugTask.assignee IN (SELECT team FROM teams) AND
-                    BugTask.bug = Bug.id
-                %(extra_filters)s
-                    ))
-            """ % dict(
-                    personid=quote(user.id),
-                    public_bug_filter=public_bug_filter,
-                    extra_filters=pillar_privacy_filters)
-    else:
-        if features.getFeatureFlag(
-            'disclosure.private_bug_visibility_rules.enabled'):
-            pillar_privacy_filters = """
-                UNION ALL
-                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 ALL
-                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 ALL
-                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 ALL
-                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 = """
-            (%(public_bug_filter)s EXISTS (
-                SELECT BugSubscription.bug
-                FROM BugSubscription, TeamParticipation
-                WHERE TeamParticipation.person = %(personid)s AND
-                    TeamParticipation.team = BugSubscription.person AND
-                    BugSubscription.bug = Bug.id
-                UNION ALL
-                SELECT BugTask.bug
-                FROM BugTask, TeamParticipation
-                WHERE TeamParticipation.person = %(personid)s AND
-                    TeamParticipation.team = BugTask.assignee AND
-                    BugTask.bug = Bug.id
-                %(extra_filters)s
-                    ))
-            """ % dict(
-                    personid=quote(user.id),
-                    public_bug_filter=public_bug_filter,
-                    extra_filters=pillar_privacy_filters)
+    query = """
+        (%(public_bug_filter)s EXISTS (
+            WITH teams AS (
+                SELECT team from TeamParticipation
+                WHERE person = %(personid)s
+            )
+            SELECT BugSubscription.bug
+            FROM BugSubscription
+            WHERE BugSubscription.person IN (SELECT team FROM teams) AND
+                BugSubscription.bug = Bug.id
+            UNION ALL
+            SELECT BugTask.bug
+            FROM BugTask
+            WHERE BugTask.assignee IN (SELECT team FROM teams) AND
+                BugTask.bug = Bug.id
+                ))
+        """ % dict(
+                personid=quote(user.id),
+                public_bug_filter=public_bug_filter)
     return query, _make_cache_user_can_view_bug(user)

=== modified file 'lib/lp/bugs/tests/test_bugtask_search.py'
--- lib/lp/bugs/tests/test_bugtask_search.py	2012-02-02 21:52:43 +0000
+++ lib/lp/bugs/tests/test_bugtask_search.py	2012-02-15 03:24:20 +0000
@@ -53,12 +53,6 @@
 from lp.testing.matchers import HasQueryCount
 
 
-PRIVATE_BUG_VISIBILITY_FLAG = {
-    'disclosure.private_bug_visibility_rules.enabled': 'on'}
-PRIVATE_BUG_VISIBILITY_CTE_FLAG = {
-    'disclosure.private_bug_visibility_cte.enabled': 'on'}
-
-
 class SearchTestBase:
     """A mixin class with tests useful for all targets and search variants."""
 
@@ -151,55 +145,6 @@
         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_private_bug_in_search_result_pillar_owners_cte(self):
-        # Like test_private_bug_in_search_result_pillar_owners, but with
-        # the new CTE-based visibility query.
-        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.
-        flags = dict()
-        flags.update(PRIVATE_BUG_VISIBILITY_FLAG)
-        flags.update(PRIVATE_BUG_VISIBILITY_CTE_FLAG)
-        with FeatureFixture(flags):
-            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]

=== modified file 'lib/lp/bugs/tests/test_bugvisibility.py'
--- lib/lp/bugs/tests/test_bugvisibility.py	2012-01-01 02:58:52 +0000
+++ lib/lp/bugs/tests/test_bugvisibility.py	2012-02-15 03:24:20 +0000
@@ -3,7 +3,6 @@
 
 """Tests for visibility of a bug."""
 
-from lp.services.features.testing import FeatureFixture
 from lp.testing import (
     celebrity_logged_in,
     TestCaseWithFactory,
@@ -83,15 +82,3 @@
     def test_publicBugAnonUser(self):
         # Since the bug is private, the anonymous user cannot see it.
         self.assertFalse(self.bug.userCanView(None))
-
-
-class TestPrivateBugVisibilityWithCTE(TestPrivateBugVisibility):
-    """Test visibility for private bugs, without the TeamParticipation CTE.
-
-    The flag exists only as an emergency performance switch.
-    """
-
-    def setUp(self):
-        super(TestPrivateBugVisibilityWithCTE, self).setUp()
-        self.useFixture(FeatureFixture(
-            {'disclosure.private_bug_visibility_cte.enabled': 'on'}))

=== modified file 'lib/lp/services/features/flags.py'
--- lib/lp/services/features/flags.py	2012-02-03 04:48:28 +0000
+++ lib/lp/services/features/flags.py	2012-02-15 03:24:20 +0000
@@ -200,13 +200,6 @@
      '',
      '',
      ''),
-    ('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.'),
-     '',
-     '',
-     ''),
     ('disclosure.enhanced_private_bug_subscriptions.enabled',
      'boolean',
      ('Enables the auto subscribing and unsubscribing of users as a bug '