Merge lp:~abentley/launchpad/private-product-listings into lp:launchpad

[Aaron Bentley]

Do not attempt to list Products for users who cannot view them.

= Summary =
Fix bug #1063264: Cannot view /projects when a private project is in the list

== Proposed fix ==
List only those products that the user can view.

== Pre-implementation notes ==
None

== LOC Rationale ==
Part of Private Projects

== Implementation details ==
Launchpad admins and commercial admins should be able to review all products. This is handled in getProductPrivacyFilter, but Product.userCanView did not implement this, so it was added.

ProductSetView.latest was implemented so that it could supply self.user to get_all_active.

== Tests ==
bin/test -t getProductPrivacyFilter -t test_get_all_active_omits_proprietary -t
test_admin_launchpad_View_proprietary_product -t test_review_include_proprietary_for_admin -t test_review_exclude_proprietary_for_expert -t test_proprietary_products_shown_to_owners_all -t test_proprietary_products_skipped_all -t test_proprietary_products_shown_to_owners -t test_proprietary_products_skipped

== Demo and Q/A ==
Create a proprietary product. It should be visible in /projects and projects/+all. Log in as a different user. It should not be visible. Share the product with another user. It should be visible to that user.

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/registry/doc/commercialsubscription.txt
  lib/lp/registry/browser/product.py
  lib/lp/registry/browser/tests/test_product.py
  lib/lp/registry/model/product.py
  lib/lp/registry/templates/products-index.pt
  lib/lp/registry/tests/test_product.py

[Richard Harding (rharding)]

review: Approve

=== modified file 'lib/lp/registry/browser/product.py'
--- lib/lp/registry/browser/product.py	2012-10-12 19:34:12 +0000
+++ lib/lp/registry/browser/product.py	2012-10-16 14:34:27 +0000
@@ -1700,7 +1700,8 @@
 
     @cachedproperty
     def all_batched(self):
-        return BatchNavigator(self.context.all_active, self.request)
+        return BatchNavigator(self.context.get_all_active(self.user),
+                              self.request)
 
     @cachedproperty
     def matches(self):
@@ -1718,6 +1719,9 @@
     def tooManyResultsFound(self):
         return self.matches > self.max_results_to_display
 
+    def latest(self):
+        return self.context.get_all_active(self.user)[:5]
+
 
 class ProductSetReviewLicensesView(LaunchpadFormView):
     """View for searching products to be reviewed."""
@@ -1793,8 +1797,8 @@
         search_params = self.initial_values
         # Override the defaults with the form values if available.
         search_params.update(data)
-        return BatchNavigator(self.context.forReview(**search_params),
-                              self.request, size=50)
+        result = self.context.forReview(self.user, **search_params)
+        return BatchNavigator(result, self.request, size=50)
 
 
 class ProductAddViewBase(ProductLicenseMixin, LaunchpadFormView):

=== modified file 'lib/lp/registry/browser/tests/test_product.py'
--- lib/lp/registry/browser/tests/test_product.py	2012-10-15 09:15:29 +0000
+++ lib/lp/registry/browser/tests/test_product.py	2012-10-16 14:34:27 +0000
@@ -637,3 +637,82 @@
             content_disposition, browser.headers['Content-disposition'])
         self.assertEqual(
             'application/rdf+xml', browser.headers['Content-type'])
+
+
+class TestProductSet(BrowserTestCase):
+
+    layer = DatabaseFunctionalLayer
+
+    def makeAllInformationTypes(self):
+        owner = self.factory.makePerson()
+        public = self.factory.makeProduct(
+            information_type=InformationType.PUBLIC, owner=owner)
+        proprietary = self.factory.makeProduct(
+            information_type=InformationType.PROPRIETARY, owner=owner)
+        embargoed = self.factory.makeProduct(
+            information_type=InformationType.EMBARGOED, owner=owner)
+        return owner, public, proprietary, embargoed
+
+    def test_proprietary_products_skipped(self):
+        # Ignore proprietary products for anonymous users
+        owner, public, proprietary, embargoed = self.makeAllInformationTypes()
+        browser = self.getViewBrowser(getUtility(IProductSet))
+        with person_logged_in(owner):
+            self.assertIn(public.name, browser.contents)
+            self.assertNotIn(proprietary.name, browser.contents)
+            self.assertNotIn(embargoed.name, browser.contents)
+
+    def test_proprietary_products_shown_to_owners(self):
+        # Owners will see their proprietary products listed
+        owner, public, proprietary, embargoed = self.makeAllInformationTypes()
+        transaction.commit()
+        browser = self.getViewBrowser(getUtility(IProductSet), user=owner)
+        with person_logged_in(owner):
+            self.assertIn(public.name, browser.contents)
+            self.assertIn(proprietary.name, browser.contents)
+            self.assertIn(embargoed.name, browser.contents)
+
+    def test_proprietary_products_skipped_all(self):
+        # Ignore proprietary products for anonymous users
+        owner, public, proprietary, embargoed = self.makeAllInformationTypes()
+        product_set = getUtility(IProductSet)
+        browser = self.getViewBrowser(product_set, view_name='+all')
+        with person_logged_in(owner):
+            self.assertIn(public.name, browser.contents)
+            self.assertNotIn(proprietary.name, browser.contents)
+            self.assertNotIn(embargoed.name, browser.contents)
+
+    def test_proprietary_products_shown_to_owners_all(self):
+        # Owners will see their proprietary products listed
+        owner, public, proprietary, embargoed = self.makeAllInformationTypes()
+        transaction.commit()
+        browser = self.getViewBrowser(getUtility(IProductSet), user=owner,
+                view_name='+all')
+        with person_logged_in(owner):
+            self.assertIn(public.name, browser.contents)
+            self.assertIn(proprietary.name, browser.contents)
+            self.assertIn(embargoed.name, browser.contents)
+
+    def test_review_exclude_proprietary_for_expert(self):
+        owner, public, proprietary, embargoed = self.makeAllInformationTypes()
+        transaction.commit()
+        expert = self.factory.makeRegistryExpert()
+        browser = self.getViewBrowser(getUtility(IProductSet),
+                                      view_name='+review-licenses',
+                                      user=expert)
+        with person_logged_in(owner):
+            self.assertIn(public.name, browser.contents)
+            self.assertNotIn(proprietary.name, browser.contents)
+            self.assertNotIn(embargoed.name, browser.contents)
+
+    def test_review_include_proprietary_for_admin(self):
+        owner, public, proprietary, embargoed = self.makeAllInformationTypes()
+        transaction.commit()
+        admin = self.factory.makeAdministrator()
+        browser = self.getViewBrowser(getUtility(IProductSet),
+                                      view_name='+review-licenses',
+                                      user=admin)
+        with person_logged_in(owner):
+            self.assertIn(public.name, browser.contents)
+            self.assertIn(proprietary.name, browser.contents)
+            self.assertIn(embargoed.name, browser.contents)

=== modified file 'lib/lp/registry/doc/commercialsubscription.txt'
--- lib/lp/registry/doc/commercialsubscription.txt	2012-07-07 13:01:46 +0000
+++ lib/lp/registry/doc/commercialsubscription.txt	2012-10-16 14:34:27 +0000
@@ -351,7 +351,8 @@
     >>> from datetime import timedelta
     >>> bzr.licenses = [License.GNU_GPL_V2, License.ECLIPSE]
     >>> flush_database_updates()
-    >>> for product in product_set.forReview(search_text='gnome'):
+    >>> for product in product_set.forReview(commercial_member,
+    ...                                      search_text='gnome'):
     ...     print product.displayname
     python gnome2 dev
     Evolution
@@ -362,7 +363,8 @@
 The license_info field is also searched for matching search_text:
 
     >>> bzr.license_info = 'Code in /contrib is under a mit-like licence.'
-    >>> for product in product_set.forReview(search_text='mit'):
+    >>> for product in product_set.forReview(commercial_member,
+    ...                                      search_text='mit'):
     ...     print product.name
     bzr
 
@@ -372,20 +374,22 @@
     >>> with celebrity_logged_in('registry_experts'):
     ...     bzr.reviewer_whiteboard = (
     ...         'cc-nc discriminates against commercial uses.')
-    >>> for product in product_set.forReview(search_text='cc-nc'):
+    >>> for product in product_set.forReview(commercial_member,
+    ...                                      search_text='cc-nc'):
     ...     print product.name
     bzr
 
 You can search for whether the product is active or not.
 
-    >>> for product in product_set.forReview(active=False):
+    >>> for product in product_set.forReview(commercial_member, active=False):
     ...     print product.name
     python-gnome2-dev
     unassigned
 
 You can search for whether the product is marked reviewed or not.
 
-    >>> for product in product_set.forReview(project_reviewed=True):
+    >>> for product in product_set.forReview(commercial_member,
+    ...                                      project_reviewed=True):
     ...     print product.name
     python-gnome2-dev
     unassigned
@@ -396,6 +400,7 @@
 any one of the licences listed.
 
     >>> for product in product_set.forReview(
+    ...     commercial_member,
     ...     licenses=[License.GNU_GPL_V2, License.BSD]):
     ...     print product.name
     bzr
@@ -404,6 +409,7 @@
 not approved
 
     >>> for product in product_set.forReview(
+    ...     commercial_member,
     ...     project_reviewed=True, license_approved=False):
     ...     print product.name
     python-gnome2-dev
@@ -414,6 +420,7 @@
 was created.
 
     >>> for product in product_set.forReview(
+    ...     commercial_member,
     ...     search_text='bzr',
     ...     created_after=bzr.datecreated,
     ...     created_before=bzr.datecreated):
@@ -425,6 +432,7 @@
 
     >>> date_expires = bzr.commercial_subscription.date_expires
     >>> for product in product_set.forReview(
+    ...     commercial_member,
     ...     search_text='bzr',
     ...     subscription_expires_after=date_expires,
     ...     subscription_expires_before=date_expires):
@@ -439,6 +447,7 @@
     >>> early_date = date(1980, 1, 1)
     >>> late_date = date_expires + timedelta(days=365 * 100)
     >>> for product in product_set.forReview(
+    ...     commercial_member,
     ...     search_text='bzr',
     ...     subscription_expires_after=date_expires,
     ...     subscription_expires_before=date_expires + one_day,
@@ -452,6 +461,7 @@
 A reviewer can search for projects without a commercial subscription.
 
     >>> for product in product_set.forReview(
+    ...     commercial_member,
     ...     has_subscription=False, licenses=[License.OTHER_PROPRIETARY]):
     ...     print product.name
     mega-money-maker
@@ -462,6 +472,7 @@
 
     >>> date_last_modified = bzr.commercial_subscription.date_last_modified
     >>> for product in product_set.forReview(
+    ...     commercial_member,
     ...     search_text='bzr',
     ...     subscription_modified_after=date_last_modified,
     ...     subscription_modified_before=date_last_modified):
@@ -471,14 +482,16 @@
 All the products are returned when no parameters are passed in.
 
     >>> from lp.registry.model.product import Product
-    >>> product_set.forReview().count() == Product.select().count()
+    >>> review_listing = product_set.forReview(commercial_member)
+    >>> review_listing.count() == Product.select().count()
     True
 
 The full text search will not match strings with dots in their name
 but a clause is included to search specifically for the name.
 
     >>> new_product = factory.makeProduct(name="abc.com")
-    >>> for product in product_set.forReview(search_text="abc.com"):
+    >>> for product in product_set.forReview(commercial_member,
+    ...                                      search_text="abc.com"):
     ...     print product.name
     abc.com
 
@@ -488,7 +501,7 @@
     >>> login('[e-mail]')
     >>> check_permission('launchpad.Moderate', product_set)
     False
-    >>> gnome =  product_set.forReview(search_text='gnome')
+    >>> gnome =  product_set.forReview(commercial_member, search_text='gnome')
     Traceback (most recent call last):
     ...
     Unauthorized:... 'forReview', 'launchpad.Moderate'...

=== modified file 'lib/lp/registry/interfaces/product.py'
--- lib/lp/registry/interfaces/product.py	2012-10-12 14:53:10 +0000
+++ lib/lp/registry/interfaces/product.py	2012-10-16 14:34:27 +0000
@@ -1026,7 +1026,9 @@
     @operation_returns_collection_of(IProduct)
     @export_read_operation()
     @export_operation_as('licensing_search')
-    def forReview(search_text=None,
+    @call_with(user=REQUEST_USER)
+    def forReview(user,
+                  search_text=None,
                   active=None,
                   project_reviewed=None,
                   licenses=None,

=== modified file 'lib/lp/registry/model/product.py'
--- lib/lp/registry/model/product.py	2012-10-15 09:15:29 +0000
+++ lib/lp/registry/model/product.py	2012-10-16 14:34:27 +0000
@@ -54,6 +54,10 @@
     )
 from zope.security.proxy import removeSecurityProxy
 
+from lp.registry.model.accesspolicy import (
+    AccessPolicy,
+    AccessPolicyGrantFlat,
+    )
 from lp.answers.enums import QUESTION_STATUS_DEFAULT_SEARCH
 from lp.answers.interfaces.faqtarget import IFAQTarget
 from lp.answers.model.faq import (
@@ -174,6 +178,7 @@
 from lp.registry.model.productlicense import ProductLicense
 from lp.registry.model.productrelease import ProductRelease
 from lp.registry.model.productseries import ProductSeries
+from lp.registry.model.teammembership import TeamParticipation
 from lp.registry.model.series import ACTIVE_STATUSES
 from lp.registry.model.sourcepackagename import SourcePackageName
 from lp.services.database import bulk
@@ -1729,11 +1734,29 @@
 
     @property
     def all_active(self):
-        return self.get_all_active()
-
-    def get_all_active(self, eager_load=True):
-        result = IStore(Product).find(Product, Product.active
-            ).order_by(Desc(Product.datecreated))
+        return self.get_all_active(None)
+
+    @staticmethod
+    def getProductPrivacyFilter(user):
+        if user is not None:
+            roles = IPersonRoles(user)
+            if roles.in_admin or roles.in_commercial_admin:
+                return True
+        granted_products = And(
+            AccessPolicyGrantFlat.grantee_id == TeamParticipation.teamID,
+            TeamParticipation.person == user,
+            AccessPolicyGrantFlat.policy == AccessPolicy.id,
+            AccessPolicy.product == Product.id,
+            AccessPolicy.type == Product._information_type)
+        return Or(Product._information_type == InformationType.PUBLIC,
+                  Product._information_type == None,
+                  Product.id.is_in(Select(Product.id, granted_products)))
+
+    @classmethod
+    def get_all_active(cls, user, eager_load=True):
+        clause = cls.getProductPrivacyFilter(user)
+        result = IStore(Product).find(Product, Product.active,
+                    clause).order_by(Desc(Product.datecreated))
         if not eager_load:
             return result
 
@@ -1843,7 +1866,7 @@
         product.development_focus = trunk
         return product
 
-    def forReview(self, search_text=None, active=None,
+    def forReview(self, user, search_text=None, active=None,
                   project_reviewed=None, license_approved=None, licenses=None,
                   created_after=None, created_before=None,
                   has_subscription=None,
@@ -1853,7 +1876,7 @@
                   subscription_modified_before=None):
         """See lp.registry.interfaces.product.IProductSet."""
 
-        conditions = []
+        conditions = [self.getProductPrivacyFilter(user)]
 
         if project_reviewed is not None:
             conditions.append(Product.project_reviewed == project_reviewed)

=== modified file 'lib/lp/registry/scripts/productreleasefinder/finder.py'
--- lib/lp/registry/scripts/productreleasefinder/finder.py	2012-09-04 20:50:35 +0000
+++ lib/lp/registry/scripts/productreleasefinder/finder.py	2012-10-16 14:34:27 +0000
@@ -103,7 +103,7 @@
 
         self.ztm.begin()
         products = getUtility(IProductSet)
-        for product in products.get_all_active(eager_load=False):
+        for product in products.get_all_active(None, eager_load=False):
             filters = []
 
             for series in product.series:

=== modified file 'lib/lp/registry/templates/products-index.pt'
--- lib/lp/registry/templates/products-index.pt	2012-06-02 12:25:39 +0000
+++ lib/lp/registry/templates/products-index.pt	2012-10-16 14:34:27 +0000
@@ -132,7 +132,7 @@
       <div class="portlet">
         <h2>Latest projects registered</h2>
         <table>
-          <tr tal:repeat="product context/latest">
+          <tr tal:repeat="product view/latest">
             <td>
               <tal:link replace="structure product/fmt:link" />
               registered

=== modified file 'lib/lp/registry/tests/test_product.py'
--- lib/lp/registry/tests/test_product.py	2012-10-15 09:15:29 +0000
+++ lib/lp/registry/tests/test_product.py	2012-10-16 14:34:27 +0000
@@ -71,9 +71,11 @@
 from lp.registry.interfaces.series import SeriesStatus
 from lp.registry.model.product import (
     Product,
+    ProductSet,
     UnDeactivateable,
     )
 from lp.registry.model.productlicense import ProductLicense
+from lp.services.database.lpstorm import IStore
 from lp.services.webapp.authorization import check_permission
 from lp.testing import (
     celebrity_logged_in,
@@ -757,6 +759,18 @@
             for attribute_name in names:
                 getattr(product, attribute_name)
 
+    def test_admin_launchpad_View_proprietary_product(self):
+        # Admins and commercial admins can view proprietary products.
+        product = self.factory.makeProduct(
+            information_type=InformationType.PROPRIETARY)
+        names = self.expected_get_permissions['launchpad.View']
+        with person_logged_in(self.factory.makeAdministrator()):
+            for attribute_name in names:
+                getattr(product, attribute_name)
+        with person_logged_in(self.factory.makeCommercialAdmin()):
+            for attribute_name in names:
+                getattr(product, attribute_name)
+
     def test_access_launchpad_AnyAllowedPerson_public_product(self):
         # Only logged in persons have access to properties of public products
         # that require the permission launchpad.AnyAllowedPerson.
@@ -1624,3 +1638,115 @@
         self.failUnlessEqual(
             [],
             ws_product.findReferencedOOPS(start_date=now - day, end_date=now))
+
+
+class TestProductSet(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    def makeAllInformationTypes(self):
+        proprietary = self.factory.makeProduct(
+            information_type=InformationType.PROPRIETARY)
+        embargoed = self.factory.makeProduct(
+            information_type=InformationType.EMBARGOED)
+        public = self.factory.makeProduct(
+            information_type=InformationType.PUBLIC)
+        return proprietary, embargoed, public
+
+    @staticmethod
+    def filterFind(user):
+        clause = ProductSet.getProductPrivacyFilter(user)
+        return IStore(Product).find(Product, clause)
+
+    def test_get_all_active_omits_proprietary(self):
+        # Ignore proprietary products for anonymous users
+        proprietary = self.factory.makeProduct(
+            information_type=InformationType.PROPRIETARY)
+        embargoed = self.factory.makeProduct(
+            information_type=InformationType.EMBARGOED)
+        result = ProductSet.get_all_active(None)
+        self.assertNotIn(proprietary, result)
+        self.assertNotIn(embargoed, result)
+
+    def test_getProductPrivacyFilterAnonymous(self):
+        # Ignore proprietary products for anonymous users
+        proprietary, embargoed, public = self.makeAllInformationTypes()
+        result = self.filterFind(None)
+        self.assertIn(public, result)
+        self.assertNotIn(embargoed, result)
+        self.assertNotIn(proprietary, result)
+
+    def test_getProductPrivacyFilter_excludes_random_users(self):
+        # Exclude proprietary products for anonymous users
+        random = self.factory.makePerson()
+        proprietary, embargoed, public = self.makeAllInformationTypes()
+        result = self.filterFind(random)
+        self.assertIn(public, result)
+        self.assertNotIn(embargoed, result)
+        self.assertNotIn(proprietary, result)
+
+    def grant(self, pillar, information_type, grantee):
+        policy_source = getUtility(IAccessPolicySource)
+        (policy,) = policy_source.find(
+            [(pillar, information_type)])
+        self.factory.makeAccessPolicyGrant(policy, grantee)
+
+    def test_getProductPrivacyFilter_respects_grants(self):
+        # Include proprietary products for users with right grants.
+        grantee = self.factory.makePerson()
+        proprietary, embargoed, public = self.makeAllInformationTypes()
+        self.grant(embargoed, InformationType.EMBARGOED, grantee)
+        self.grant(proprietary, InformationType.PROPRIETARY, grantee)
+        result = self.filterFind(grantee)
+        self.assertIn(public, result)
+        self.assertIn(embargoed, result)
+        self.assertIn(proprietary, result)
+
+    def test_getProductPrivacyFilter_ignores_wrong_product(self):
+        # Exclude proprietary products if grant is on wrong product.
+        grantee = self.factory.makePerson()
+        proprietary, embargoed, public = self.makeAllInformationTypes()
+        self.factory.makeAccessPolicyGrant(grantee=grantee)
+        result = self.filterFind(grantee)
+        self.assertIn(public, result)
+        self.assertNotIn(embargoed, result)
+        self.assertNotIn(proprietary, result)
+
+    def test_getProductPrivacyFilter_ignores_wrong_info_type(self):
+        # Exclude proprietary products if grant is on wrong information type.
+        grantee = self.factory.makePerson()
+        proprietary, embargoed, public = self.makeAllInformationTypes()
+        self.grant(embargoed, InformationType.PROPRIETARY, grantee)
+        self.factory.makeAccessPolicy(proprietary, InformationType.EMBARGOED)
+        self.grant(proprietary, InformationType.EMBARGOED, grantee)
+        result = self.filterFind(grantee)
+        self.assertIn(public, result)
+        self.assertNotIn(embargoed, result)
+        self.assertNotIn(proprietary, result)
+
+    def test_getProductPrivacyFilter_respects_team_grants(self):
+        # Include proprietary products for users in teams with right grants.
+        grantee = self.factory.makeTeam()
+        proprietary, embargoed, public = self.makeAllInformationTypes()
+        self.grant(embargoed, InformationType.EMBARGOED, grantee)
+        self.grant(proprietary, InformationType.PROPRIETARY, grantee)
+        result = self.filterFind(grantee.teamowner)
+        self.assertIn(public, result)
+        self.assertIn(embargoed, result)
+        self.assertIn(proprietary, result)
+
+    def test_getProductPrivacyFilter_includes_admins(self):
+        # Launchpad admins can see everything.
+        proprietary, embargoed, public = self.makeAllInformationTypes()
+        result = self.filterFind(self.factory.makeAdministrator())
+        self.assertIn(public, result)
+        self.assertIn(embargoed, result)
+        self.assertIn(proprietary, result)
+
+    def test_getProductPrivacyFilter_includes_commercial_admins(self):
+        # Commercial admins can see everything.
+        proprietary, embargoed, public = self.makeAllInformationTypes()
+        result = self.filterFind(self.factory.makeCommercialAdmin())
+        self.assertIn(public, result)
+        self.assertIn(embargoed, result)
+        self.assertIn(proprietary, result)