launchpad-reviewers team mailing list archive

[Deryck Hodge <deryck.hodge@xxxxxxxxxxxxx>]

Deryck Hodge has proposed merging lp:~deryck/launchpad/top-level-bug-search-and-privacy-1069895 into lp:launchpad.

Commit message:
Ensure that bugtask search accounts for whether or not any related products are private, which will keep the top-level bugs homepage from erroring out if there are any public bugs against proprietary or embargoed projects.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1069895 in Launchpad itself: "Top-level bug searches fail when results contain public bug against a proprietary product"
  https://bugs.launchpad.net/launchpad/+bug/1069895

For more details, see:
https://code.launchpad.net/~deryck/launchpad/top-level-bug-search-and-privacy-1069895/+merge/134164

This branch fixes bug 1069895 by ensuring that any bugtask search results that is a mix of  public bugs with those that have private projects will exclude bugs with private projects.  This is kind of an odd situation to be in and we have other work in progress to prevent public bugs on proprietary projects but this fix will make sure we don't error (especially on the top-level bugs homepage) if we get into this kind of mixed data situation.

The fix is pretty straight forward. It amounts to updating the bugtask search query to account for private projects.  There is also a test to ensure the specific condition we want to avoid is safe.  Finally, a handful of bugtask search query count tests had to be updated. I'm honestly at a loss for how these counts could change, given that I'm adding a join to an existing query. I was worried that maybe I was returning more data than originally and some loop over the data was doing extra queries. But some of these tests here even assert that the expected number of bugtasks are returned, and that the size of the queries doesn't change as more data is added.

So I'm not sure what's up here, but it should be safe, given that it's only adding a query or two in each case.
-- 
https://code.launchpad.net/~deryck/launchpad/top-level-bug-search-and-privacy-1069895/+merge/134164
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~deryck/launchpad/top-level-bug-search-and-privacy-1069895 into lp:launchpad.

=== modified file 'lib/lp/bugs/browser/tests/test_bugs.py'
--- lib/lp/bugs/browser/tests/test_bugs.py	2012-10-08 06:13:17 +0000
+++ lib/lp/bugs/browser/tests/test_bugs.py	2012-11-13 18:06:26 +0000
@@ -6,7 +6,6 @@
 __metaclass__ = type
 
 from zope.component import getUtility
-from zope.security.proxy import removeSecurityProxy
 
 from lp.app.enums import InformationType
 from lp.bugs.interfaces.bugtask import BugTaskStatus
@@ -16,16 +15,19 @@
 from lp.registry.interfaces.product import License
 from lp.services.webapp.publisher import canonical_url
 from lp.testing import (
+    BrowserTestCase,
     celebrity_logged_in,
     person_logged_in,
-    TestCaseWithFactory,
     )
 from lp.testing.layers import DatabaseFunctionalLayer
-from lp.testing.pages import find_tag_by_id
+from lp.testing.pages import (
+    find_tag_by_id,
+    setupBrowserForUser,
+    )
 from lp.testing.views import create_initialized_view
 
 
-class TestMaloneView(TestCaseWithFactory):
+class TestMaloneView(BrowserTestCase):
     """Test the MaloneView for the Bugs application."""
     layer = DatabaseFunctionalLayer
 
@@ -104,6 +106,35 @@
         # we should get some valid content out of this
         self.assertIn('Search all bugs', content)
 
+    def test_search_bugs_with_proprietary_product(self):
+        owner = self.factory.makePerson()
+        anon_user = self.factory.makePerson()
+        information_type = InformationType.PROPRIETARY
+        product = self.factory.makeProduct(
+            owner=owner, information_type=information_type)
+        title = 'huhdwudwyhbdwhbdwhbwdwhb'
+        query_string = (
+                '?field.searchtext=%s&search=Search+Bug+Reports'
+                '&field.scope=all&field.scope.target=' % title)
+        base_url = canonical_url(
+            self.application, view_name='+bugs', rootsite='bugs')
+        url = '%s%s' % (base_url, query_string)
+        with person_logged_in(owner):
+            product.setBugSharingPolicy(
+                BugSharingPolicy.PROPRIETARY_OR_PUBLIC)
+            bug = self.factory.makeBug(
+                target=product, information_type=InformationType.PUBLIC,
+                title=title)
+            self.assertEqual(bug.information_type, InformationType.PUBLIC)
+            self.assertEqual(bug.title, title)
+        with person_logged_in(anon_user):
+            browser = setupBrowserForUser(user=anon_user)
+            browser.open(url)
+            # It doesn't matter what we assert here.  The test is really
+            # just to confirm the browser.open call above didn't raise
+            # an Unauthorized error.
+            self.assertIn('No results for search', browser.contents)
+
     def _assert_getBugData(self, related_bug=None):
         # The getBugData method works as expected.
         owner = self.factory.makePerson()

=== modified file 'lib/lp/bugs/browser/tests/test_bugtask.py'
--- lib/lp/bugs/browser/tests/test_bugtask.py	2012-10-15 09:15:29 +0000
+++ lib/lp/bugs/browser/tests/test_bugtask.py	2012-11-13 18:06:26 +0000
@@ -141,7 +141,7 @@
         # "SELECT id, product, project, distribution FROM PillarName ..."
         # query by previously browsing the task url, in which case the
         # query count is decreased by one.
-        self.assertThat(recorder, HasQueryCount(LessThan(82)))
+        self.assertThat(recorder, HasQueryCount(LessThan(85)))
         count_with_no_teams = recorder.count
         # count with many teams
         self.invalidate_caches(task)
@@ -157,7 +157,7 @@
     def test_rendered_query_counts_constant_with_attachments(self):
         with celebrity_logged_in('admin'):
             browses_under_limit = BrowsesWithQueryLimit(
-                85, self.factory.makePerson())
+                87, self.factory.makePerson())
 
             # First test with a single attachment.
             task = self.factory.makeBugTask()
@@ -251,7 +251,7 @@
         # Render the view with one activity.
         with celebrity_logged_in('admin'):
             browses_under_limit = BrowsesWithQueryLimit(
-                83, self.factory.makePerson())
+                85, self.factory.makePerson())
             person = self.factory.makePerson()
             add_activity("description", person)
 
@@ -2036,10 +2036,10 @@
         self.invalidate_caches(bug)
         # count with single task
         self.getUserBrowser(url)
-        self.assertThat(recorder, HasQueryCount(LessThan(35)))
+        self.assertThat(recorder, HasQueryCount(LessThan(37)))
         # count with many tasks
         self.getUserBrowser(buggy_url)
-        self.assertThat(recorder, HasQueryCount(LessThan(35)))
+        self.assertThat(recorder, HasQueryCount(LessThan(37)))
 
     def test_mustache_model_in_json(self):
         """The IJSONRequestCache should contain mustache_model.

=== modified file 'lib/lp/bugs/model/bugtasksearch.py'
--- lib/lp/bugs/model/bugtasksearch.py	2012-10-25 06:59:34 +0000
+++ lib/lp/bugs/model/bugtasksearch.py	2012-11-13 18:06:26 +0000
@@ -671,10 +671,26 @@
                 Milestone.dateexpected <= dateexpected_before)
 
     if not params.ignore_privacy:
-        clause, decorator = _get_bug_privacy_filter_with_decorator(params.user)
+        clause, decorator = _get_bug_privacy_filter_with_decorator(
+            params.user)
         if clause:
             extra_clauses.append(clause)
             decorators.append(decorator)
+        cross_context_search = (
+            params.product is None
+            and params.project is None
+            and params.distribution is None
+            and params.productseries is None
+            and params.distroseries is None)
+        if params.product is not None or cross_context_search:
+            if Product not in clauseTables:
+                clauseTables.append(Product)
+                extra_clauses.append(
+                    Or(
+                        BugTaskFlat.product_id == Product.id,
+                        BugTaskFlat.product_id == None))
+            extra_clauses.append(
+                ProductSet.getProductPrivacyFilter(params.user))
 
     hw_clause = _build_hardware_related_clause(params)
     if hw_clause is not None:

=== modified file 'lib/lp/registry/browser/tests/test_milestone.py'
--- lib/lp/registry/browser/tests/test_milestone.py	2012-10-26 09:01:27 +0000
+++ lib/lp/registry/browser/tests/test_milestone.py	2012-11-13 18:06:26 +0000
@@ -327,12 +327,12 @@
         #  10. All related people
         bugtask_count = 10
         self.assert_bugtasks_query_count(
-            self.milestone, bugtask_count, query_limit=11)
+            self.milestone, bugtask_count, query_limit=12)
 
     def test_milestone_eager_loading(self):
         # Verify that the number of queries does not increase with more
         # bugs with different assignees.
-        browses_under_limit = BrowsesWithQueryLimit(36, self.owner)
+        browses_under_limit = BrowsesWithQueryLimit(37, self.owner)
         self.add_bug(3)
         self.assertThat(self.milestone, browses_under_limit)
         self.add_bug(10)
@@ -450,7 +450,7 @@
         #  10. All related people.
         bugtask_count = 10
         self.assert_bugtasks_query_count(
-            self.milestone, bugtask_count, query_limit=11)
+            self.milestone, bugtask_count, query_limit=12)
 
     def test_milestone_eager_loading(self):
         # Verify that the number of queries does not increase with more
@@ -509,12 +509,12 @@
         #  9. Load links to milestones.
         bugtask_count = 10
         self.assert_bugtasks_query_count(
-            self.milestone, bugtask_count, query_limit=11)
+            self.milestone, bugtask_count, query_limit=12)
 
     def test_milestone_eager_loading(self):
         # Verify that the number of queries does not increase with more
         # bugs with different assignees.
-        browses_under_limit = BrowsesWithQueryLimit(35, self.owner)
+        browses_under_limit = BrowsesWithQueryLimit(36, self.owner)
         self.add_bug(4)
         self.assertThat(self.milestone, browses_under_limit)
         self.add_bug(10)
@@ -588,4 +588,4 @@
         # bugtasks retrieval.
         bugtask_count = 10
         self.assert_bugtasks_query_count(
-            self.milestonetag, bugtask_count, query_limit=11)
+            self.milestonetag, bugtask_count, query_limit=12)

=== modified file 'lib/lp/registry/browser/tests/test_pillar_sharing.py'
--- lib/lp/registry/browser/tests/test_pillar_sharing.py	2012-10-09 10:28:02 +0000
+++ lib/lp/registry/browser/tests/test_pillar_sharing.py	2012-11-13 18:06:26 +0000
@@ -215,7 +215,7 @@
         IStore(self.pillar).invalidate()
         with StormStatementRecorder() as recorder:
             create_initialized_view(pillarperson, '+index')
-        self.assertThat(recorder, HasQueryCount(LessThan(13)))
+        self.assertThat(recorder, HasQueryCount(LessThan(14)))
 
 
 class TestProductSharingDetailsView(

=== modified file 'lib/lp/registry/model/product.py'
--- lib/lp/registry/model/product.py	2012-11-12 11:21:14 +0000
+++ lib/lp/registry/model/product.py	2012-11-13 18:06:26 +0000
@@ -1715,7 +1715,13 @@
     @staticmethod
     def getProductPrivacyFilter(user):
         if user is not None:
-            roles = IPersonRoles(user)
+            # Generally our `user` here is a Person object,
+            # but sometimes it's a PersonRoles object.
+            if IPersonRoles.providedBy(user):
+                roles = user
+                user = user.person
+            else:
+                roles = IPersonRoles(user)
             if roles.in_admin or roles.in_commercial_admin:
                 return True
         granted_products = And(