launchpad-reviewers team mailing list archive

[Richard Harding <rick.harding@xxxxxxxxxxxxx>]

Richard Harding has proposed merging lp:~rharding/launchpad/filter_more_products into lp:launchpad.

Commit message:
Update product filters to prevent leaking non-public products in the translationgroup code.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~rharding/launchpad/filter_more_products/+merge/135164

= Summary =

A search for products brought up several places to look to make sure we filter
products so that non-public ones are not leaked.

== Pre Implementation ==

None, straight forward case of add filters.


== Implementation Notes ==

ITranslationGroup.products

The query needed to be adjusted to allow for use of the
getproductPrivacyFilter. Added tests for it.


ITranslationGroup.projects

This is actually querying for a list of ProjectGroups which do not have an
information type.

     ProjectGroup.selectBy(translationgroup=self.id, active=True)

For any returned project group you'd then have to query for the products
within it and those are all filtered appropriately.

ITranslationGroup.fetchProjectsForDisplay()

This needed to be updated to add the extra filter. Tests added to verify it
filters correctly.

ITranslationGroup.fetchProjectGroupsForDisplay()

This again is querying for ProjectGroups which fall under the .projects case
above.

ITranslationGroup.top_projects

This uses self.products, self.projects to get data. Since they've been checked
and updated it's safe to use.


== Tests ==

lib/lp/translations/tests/test_translationgroup.py

-- 
https://code.launchpad.net/~rharding/launchpad/filter_more_products/+merge/135164
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~rharding/launchpad/filter_more_products into lp:launchpad.

=== modified file 'lib/lp/registry/model/product.py'
--- lib/lp/registry/model/product.py	2012-11-19 06:26:32 +0000
+++ lib/lp/registry/model/product.py	2012-11-20 15:29:21 +0000
@@ -300,7 +300,7 @@
     def composeLicensesColumn(cls, for_class=None):
         """Compose a Storm column specification for licences.
 
-        Use this to render a list of `Product` linkes without querying
+        Use this to render a list of `Product` links without querying
         licences for each one individually.
 
         It lets you prefetch the licensing information in the same

=== modified file 'lib/lp/translations/model/translationgroup.py'
--- lib/lp/translations/model/translationgroup.py	2011-12-30 06:14:56 +0000
+++ lib/lp/translations/model/translationgroup.py	2012-11-20 15:29:21 +0000
@@ -24,6 +24,7 @@
     )
 from storm.store import Store
 from zope.interface import implements
+from zope.component import getUtility
 
 from lp.app.errors import NotFoundError
 from lp.registry.interfaces.person import validate_public_person
@@ -41,6 +42,7 @@
     LibraryFileAlias,
     LibraryFileContent,
     )
+from lp.services.webapp.interfaces import ILaunchBag
 from lp.services.worlddata.model.language import Language
 from lp.translations.interfaces.translationgroup import (
     ITranslationGroup,
@@ -109,9 +111,17 @@
     def products(self):
         """See `ITranslationGroup`."""
         # Avoid circular imports.
-        from lp.registry.model.product import Product
-
-        return Product.selectBy(translationgroup=self.id, active=True)
+        from lp.registry.model.product import (
+            Product,
+            ProductSet,
+        )
+        user = getUtility(ILaunchBag).user
+        results = Store.of(self).find(
+            Product,
+            Product.active==True,
+            Product.translationgroup==self.id,
+            ProductSet.getProductPrivacyFilter(user))
+        return results
 
     @property
     def projects(self):
@@ -187,12 +197,16 @@
         # Avoid circular imports.
         from lp.registry.model.product import (
             Product,
+            ProductSet,
             ProductWithLicenses,
             )
 
+        user = getUtility(ILaunchBag).user
         using = [
             Product,
-            LeftJoin(LibraryFileAlias, LibraryFileAlias.id == Product.iconID),
+            LeftJoin(
+                LibraryFileAlias,
+                LibraryFileAlias.id == Product.iconID),
             LeftJoin(
                 LibraryFileContent,
                 LibraryFileContent.id == LibraryFileAlias.contentID),
@@ -205,7 +219,9 @@
             )
         product_data = ISlaveStore(Product).using(*using).find(
             columns,
-            Product.translationgroupID == self.id, Product.active == True)
+            Product.translationgroup == self.id,
+            Product.active == True,
+            ProductSet.getProductPrivacyFilter(user))
         product_data = product_data.order_by(Product.displayname)
 
         return [

=== modified file 'lib/lp/translations/tests/test_translationgroup.py'
--- lib/lp/translations/tests/test_translationgroup.py	2012-01-01 02:58:52 +0000
+++ lib/lp/translations/tests/test_translationgroup.py	2012-11-20 15:29:21 +0000
@@ -9,11 +9,13 @@
 import transaction
 from zope.component import getUtility
 
+from lp.app.enums import InformationType
 from lp.registry.interfaces.teammembership import (
     ITeamMembershipSet,
     TeamMembershipStatus,
     )
 from lp.testing import (
+    person_logged_in,
     TestCaseWithFactory,
     WebServiceTestCase,
     )
@@ -21,6 +23,65 @@
 from lp.translations.interfaces.translationgroup import ITranslationGroupSet
 
 
+class TestTranslationGroup(TestCaseWithFactory):
+
+    layer = ZopelessDatabaseLayer
+
+    def _setup_products(self):
+        """Helper to setup one public one non-public product."""
+        user = self.factory.makePerson()
+        private_owner = self.factory.makePerson()
+        group = self.factory.makeTranslationGroup()
+
+        with person_logged_in(user):
+            public_product = self.factory.makeProduct()
+            public_product.translationgroup = group
+
+        with person_logged_in(private_owner):
+            private_product = self.factory.makeProduct(
+                information_type=InformationType.PROPRIETARY,
+                owner=private_owner)
+            private_product.translationgroup = group
+
+        return group, user, private_owner
+
+    def test_non_public_products_hidden(self):
+        """Non Public products are not returned via products attribute."""
+        group, public_user, private_user = self._setup_products()
+
+        with person_logged_in(public_user):
+            self.assertEqual(
+                1,
+                group.products.count(),
+                'There is only one public product for this user')
+
+        with person_logged_in(private_user):
+            self.assertEqual(
+                2,
+                group.products.count(),
+                'There are two for the private user.')
+
+    def test_non_public_products_hidden_for_display(self):
+        """Non Public products are not returned via fetchProjectsForDisplay."""
+        group, public_user, private_user = self._setup_products()
+
+        # Magical transaction so our data shows up via ISlaveStore
+        import transaction
+        transaction.commit()
+
+        with person_logged_in(public_user):
+            self.assertEqual(
+                1,
+                len(group.fetchProjectsForDisplay()),
+                'There is only one public product for the public user')
+
+        with person_logged_in(private_user):
+            self.assertEqual(
+                2,
+                len(group.fetchProjectsForDisplay()),
+                'Both products show for the private user.')
+
+
 class TestTranslationGroupSet(TestCaseWithFactory):
     layer = ZopelessDatabaseLayer