launchpad-reviewers team mailing list archive

[Aaron Bentley <aaron@xxxxxxxxxxxxx>]

Aaron Bentley has proposed merging lp:~abentley/launchpad/productset-updates into lp:launchpad.

Commit message:
Fix ProductSet privacy handling.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1082094 in Launchpad itself: "Productset does not honour privacy"
  https://bugs.launchpad.net/launchpad/+bug/1082094

For more details, see:
https://code.launchpad.net/~abentley/launchpad/productset-updates/+merge/135737

= Summary =
Fix bug #1082094: Productset does not honour privacy

== Proposed fix ==
Update search and latest to filter by product, remove search_sqlobject

== Pre-implementation notes ==
None

== LOC Rationale ==
Part of Private Projects

== Implementation details ==
Update search and latest to use ProductSet.getProductPrivacyFilter.

Update web service export to call search with request user.
Unfortunately, search is also used for ProductSet's default content, and default_content does not honour call_with.  Added a wrapper, ProductSet._request_user_search to allow search to be used with default_content.

search_sqlobject exists to provide an SQLObject-friendly method for _loadProductsUsingBugTracker.  This was a problem because getProductPrivacyFilter is unlikely to ever support SQLObject.  I updated the caller to use Product.search instead, and removed search_sqlobject.

As a driveby, I deleted ProductSet.all_active, since it had only one call site left.

== Tests ==
bin/test -t product.txt -t test_search_respects_privacy -t test_latest_honours_privacy -t test_search_honours_privacy

== Demo and Q/A ==
None


= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/registry/vocabularies.py
  lib/lp/registry/doc/product.txt
  lib/lp/registry/configure.zcml
  lib/lp/registry/interfaces/product.py
  lib/lp/bugs/browser/bugalsoaffects.py
  lib/lp/registry/model/product.py
  lib/lp/registry/tests/test_product.py
-- 
https://code.launchpad.net/~abentley/launchpad/productset-updates/+merge/135737
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~abentley/launchpad/productset-updates into lp:launchpad.

=== modified file 'lib/lp/bugs/browser/bugalsoaffects.py'
--- lib/lp/bugs/browser/bugalsoaffects.py	2012-10-29 22:06:46 +0000
+++ lib/lp/bugs/browser/bugalsoaffects.py	2012-11-22 16:56:24 +0000
@@ -791,10 +791,11 @@
             # Use a local import as we don't want removeSecurityProxy used
             # anywhere else.
             from zope.security.proxy import removeSecurityProxy
+            from lp.registry.model.product import Product
             name_matches = removeSecurityProxy(
-                getUtility(IProductSet).search_sqlobject(
+                getUtility(IProductSet).search(self.user,
                 self.request.form.get('field.name')))
-            products = bugtracker.products.intersect(name_matches)
+            products = name_matches.find(Product.bugtracker == bugtracker.id)
             self.existing_products = list(
                 products[:self.MAX_PRODUCTS_TO_DISPLAY])
         else:

=== modified file 'lib/lp/registry/configure.zcml'
--- lib/lp/registry/configure.zcml	2012-11-12 23:03:24 +0000
+++ lib/lp/registry/configure.zcml	2012-11-22 16:56:24 +0000
@@ -1475,7 +1475,6 @@
             attributes="
                 title
                 people
-                all_active
                 get
                 get_all_active
                 getByName

=== modified file 'lib/lp/registry/doc/product.txt'
--- lib/lp/registry/doc/product.txt	2012-10-24 14:21:58 +0000
+++ lib/lp/registry/doc/product.txt	2012-11-22 16:56:24 +0000
@@ -154,7 +154,7 @@
 IProductSet can also retrieve the latest products registered.  By
 default the latest five are returned.
 
-    >>> latest = productset.latest()
+    >>> latest = productset.latest(None)
     >>> projects = [project.displayname for project in latest]
     >>> for project in sorted(projects):
     ...     print project
@@ -167,7 +167,7 @@
 The quantity can be specified and that many, if available, will be
 returned.
 
-    >>> latest = productset.latest(quantity=3)
+    >>> latest = productset.latest(None, quantity=3)
     >>> projects = [project.displayname for project in latest]
     >>> for project in sorted(projects):
     ...     print project

=== modified file 'lib/lp/registry/interfaces/product.py'
--- lib/lp/registry/interfaces/product.py	2012-11-21 19:38:47 +0000
+++ lib/lp/registry/interfaces/product.py	2012-11-22 16:56:24 +0000
@@ -945,9 +945,6 @@
         "The PersonSet, placed here so we can easily render "
         "the list of latest teams to register on the /projects/ page.")
 
-    all_active = Attribute(
-        "All the active products, sorted newest first.")
-
     def get_users_private_products(user):
         """Get users non-public products.
 
@@ -1055,10 +1052,14 @@
         """Return an iterator over products that need to be reviewed."""
 
     @collection_default_content()
+    def _request_user_search():
+        """Wrapper for search to use request user in default content."""
+
+    @call_with(user=REQUEST_USER)
     @operation_parameters(text=TextLine(title=_("Search text")))
     @operation_returns_collection_of(IProduct)
     @export_read_operation()
-    def search(text=None):
+    def search(user, text=None):
         """Search through the Registry database for products that match the
         query terms. text is a piece of text in the title / summary /
         description fields of product.
@@ -1067,18 +1068,14 @@
         needed for other callers.
         """
 
-    def search_sqlobject(text):
-        """A compatible sqlobject search for bugalsoaffects.py.
-
-        DO NOT ADD USES.
-        """
-
     @operation_returns_collection_of(IProduct)
-    @call_with(quantity=None)
+    @call_with(user=REQUEST_USER, quantity=None)
     @export_read_operation()
-    def latest(quantity=5):
+    def latest(user, quantity=5):
         """Return the latest projects registered in Launchpad.
 
+        The supplied user determines which objects are visible.
+
         If the quantity is not specified or is a value that is not 'None'
         then the set of projects returned is limited to that value (the
         default quantity is 5).  If quantity is 'None' then all projects are

=== modified file 'lib/lp/registry/model/product.py'
--- lib/lp/registry/model/product.py	2012-11-19 18:23:06 +0000
+++ lib/lp/registry/model/product.py	2012-11-22 16:56:24 +0000
@@ -1730,21 +1730,19 @@
 
     def __iter__(self):
         """See `IProductSet`."""
-        return iter(self.all_active)
+        return iter(self.get_all_active(None))
 
     @property
     def people(self):
         return getUtility(IPersonSet)
 
-    def latest(self, quantity=5):
-        if quantity is None:
-            return self.all_active
-        else:
-            return self.all_active[:quantity]
-
-    @property
-    def all_active(self):
-        return self.get_all_active(None)
+    @classmethod
+    def latest(cls, user, quantity=5):
+        """See `IProductSet`."""
+        result = cls.get_all_active(user)
+        if quantity is not None:
+            result = result[:quantity]
+        return result
 
     @staticmethod
     def getProductPrivacyFilter(user):
@@ -1996,9 +1994,15 @@
 
         return DecoratedResultSet(result, pre_iter_hook=eager_load)
 
-    def search(self, text=None):
+    @classmethod
+    def _request_user_search(cls):
+        return cls.search(getUtility(ILaunchBag).user)
+
+    @classmethod
+    def search(cls, user=None, text=None):
         """See lp.registry.interfaces.product.IProductSet."""
-        conditions = [Product.active]
+        conditions = [Product.active,
+                      cls.getProductPrivacyFilter(user)]
         if text:
             conditions.append(
                 SQL("Product.fti @@ ftq(%s) " % sqlvalues(text)))
@@ -2012,14 +2016,6 @@
 
         return DecoratedResultSet(result, pre_iter_hook=eager_load)
 
-    def search_sqlobject(self, text):
-        """See `IProductSet`"""
-        queries = ["Product.fti @@ ftq(%s) " % sqlvalues(text)]
-        queries.append('Product.active IS TRUE')
-        query = "Product.active IS TRUE AND Product.fti @@ ftq(%s)" \
-            % sqlvalues(text)
-        return Product.select(query)
-
     def getTranslatables(self):
         """See `IProductSet`"""
         user = getUtility(ILaunchBag).user

=== modified file 'lib/lp/registry/tests/test_product.py'
--- lib/lp/registry/tests/test_product.py	2012-11-19 06:26:32 +0000
+++ lib/lp/registry/tests/test_product.py	2012-11-22 16:56:24 +0000
@@ -2044,6 +2044,17 @@
         self.assertNotIn(proprietary, result)
         self.assertNotIn(embargoed, result)
 
+    def test_search_respects_privacy(self):
+        # Proprietary products are filtered from the results for people who
+        # cannot see them.
+        owner = self.factory.makePerson()
+        product = self.factory.makeProduct(owner=owner)
+        self.assertIn(product, ProductSet.search(None))
+        with person_logged_in(owner):
+            product.information_type = InformationType.PROPRIETARY
+        self.assertNotIn(product, ProductSet.search(None))
+        self.assertIn(product, ProductSet.search(owner))
+
     def test_getProductPrivacyFilterAnonymous(self):
         # Ignore proprietary products for anonymous users
         proprietary, embargoed, public = self.makeAllInformationTypes()
@@ -2154,3 +2165,32 @@
         with person_logged_in(user):
             translatables = getUtility(IProductSet).getTranslatables()
             self.assertIn(product, list(translatables))
+
+
+class TestProductSetWebService(WebServiceTestCase):
+
+    def test_latest_honours_privacy(self):
+        # Latest lists objects that the user can see, even if proprietary, and
+        # skips those the user can't see.
+        owner = self.factory.makePerson()
+        product = self.factory.makeProduct(
+            information_type=InformationType.PROPRIETARY, owner=owner)
+        with person_logged_in(owner):
+            name = product.name
+        productset = self.wsObject(ProductSet(), owner)
+        self.assertIn(name, [p.name for p in productset.latest()])
+        productset = self.wsObject(ProductSet(), self.factory.makePerson())
+        self.assertNotIn(name, [p.name for p in productset.latest()])
+
+    def test_search_honours_privacy(self):
+        # search lists objects that the user can see, even if proprietary, and
+        # skips those the user can't see.
+        owner = self.factory.makePerson()
+        product = self.factory.makeProduct(
+            information_type=InformationType.PROPRIETARY, owner=owner)
+        with person_logged_in(owner):
+            name = product.name
+        productset = self.wsObject(ProductSet(), owner)
+        self.assertIn(name, [p.name for p in productset.search()])
+        productset = self.wsObject(ProductSet(), self.factory.makePerson())
+        self.assertNotIn(name, [p.name for p in productset.search()])

=== modified file 'lib/lp/registry/vocabularies.py'
--- lib/lp/registry/vocabularies.py	2012-11-21 16:50:43 +0000
+++ lib/lp/registry/vocabularies.py	2012-11-22 16:56:24 +0000
@@ -1578,7 +1578,7 @@
         if user is None:
             return self.emptySelectResults()
         if self.is_commercial_admin:
-            projects = self.product_set.search(query)
+            projects = self.product_set.search(user, query)
         else:
             projects = user.getOwnedProjects(match_name=query)
         return projects