launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #06372
[Merge] lp:~wgrant/launchpad/remove-extra-privacy into lp:launchpad
William Grant has proposed merging lp:~wgrant/launchpad/remove-extra-privacy into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~wgrant/launchpad/remove-extra-privacy/+merge/93122
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.
--
https://code.launchpad.net/~wgrant/launchpad/remove-extra-privacy/+merge/93122
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/remove-extra-privacy into lp:launchpad.
=== 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 '
