launchpad-reviewers team mailing list archive

[Abel Deuring <abel.deuring@xxxxxxxxxxxxx>]

Abel Deuring has proposed merging lp:~adeuring/launchpad/correct-permission-check-for-iproduct into lp:launchpad with lp:~adeuring/launchpad/product-sharing-sec-adapter as a prerequisite.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~adeuring/launchpad/correct-permission-check-for-iproduct/+merge/127518

This is a follow-up branch for lp:~adeuring/launchpad/product-sharing-sec-adapter.

The previous branch changed the security adapters required to access attributes of IProduct but it did not include an important detail: If a user has a policy grant for the private product.

This branch fixes this problem.

tests:

./bin/test -vvt lp.registry.tests.test_product.TestProduct.test_userCanView_caches_known_users
./bin/test -vvt lp.registry.tests.test_product.TestProduct.test_access_launchpad_AnyAllowedPerson_proprietary_product
./bin/test -vvt lp.registry.tests.test_product.TestProduct.test_access_launchpad_View_.*_product

no lint
-- 
https://code.launchpad.net/~adeuring/launchpad/correct-permission-check-for-iproduct/+merge/127518
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~adeuring/launchpad/correct-permission-check-for-iproduct into lp:launchpad.

=== modified file 'lib/lp/registry/model/product.py'
--- lib/lp/registry/model/product.py	2012-10-02 15:55:49 +0000
+++ lib/lp/registry/model/product.py	2012-10-02 15:55:51 +0000
@@ -149,6 +149,10 @@
     LicenseStatus,
     )
 from lp.registry.interfaces.role import IPersonRoles
+from lp.registry.model.accesspolicy import(
+    AccessPolicy,
+    AccessPolicyGrant,
+    )
 from lp.registry.model.announcement import MakesAnnouncements
 from lp.registry.model.commercialsubscription import CommercialSubscription
 from lp.registry.model.distribution import Distribution
@@ -1521,11 +1525,34 @@
 
         return weight_function
 
+    @cachedproperty
+    def _known_viewers(self):
+        """A set of known persons able to view this product."""
+        return set()
+
     def userCanView(self, user):
         """See `IProductPublic`."""
         if self.information_type in PUBLIC_INFORMATION_TYPES:
             return True
-        return user.inTeam(self.owner)
+        if user is None:
+            return False
+        if user.id in self._known_viewers:
+            return True
+        store = Store.of(self)
+        grants_for_user = store.using(
+            AccessPolicy,
+            Join(
+                AccessPolicyGrant,
+                And(
+                    AccessPolicyGrant.policy_id == AccessPolicy.id,
+                    AccessPolicyGrant.grantee_id == user.id))).find(
+            AccessPolicyGrant,
+            AccessPolicy.product_id == self.id,
+            AccessPolicy.type == self.information_type)
+        if grants_for_user.is_empty():
+            return False
+        self._known_viewers.add(user.id)
+        return True
 
 
 class ProductSet:

=== modified file 'lib/lp/registry/tests/test_product.py'
--- lib/lp/registry/tests/test_product.py	2012-10-02 15:55:49 +0000
+++ lib/lp/registry/tests/test_product.py	2012-10-02 15:55:51 +0000
@@ -40,6 +40,12 @@
     BugSharingPolicy,
     EXCLUSIVE_TEAM_POLICY,
     INCLUSIVE_TEAM_POLICY,
+<<<<<<< TREE
+=======
+    BranchSharingPolicy,
+    BugSharingPolicy,
+    SharingPermission,
+>>>>>>> MERGE-SOURCE
     SpecificationSharingPolicy,
     )
 from lp.registry.errors import (
@@ -67,6 +73,7 @@
     celebrity_logged_in,
     login,
     person_logged_in,
+    StormStatementRecorder,
     TestCase,
     TestCaseWithFactory,
     WebServiceTestCase,
@@ -563,6 +570,7 @@
         # attributes protected by the permission launchpad.View.
         product = self.factory.makeProduct(
             information_type=InformationType.PROPRIETARY)
+        owner = removeSecurityProxy(product).owner
         names = self.expected_get_permissions['launchpad.View']
         with person_logged_in(None):
             for attribute_name in names:
@@ -573,7 +581,16 @@
             for attribute_name in names:
                 self.assertRaises(
                     Unauthorized, getattr, product, attribute_name)
-        with person_logged_in(removeSecurityProxy(product).owner):
+        with person_logged_in(owner):
+            for attribute_name in names:
+                getattr(product, attribute_name)
+        # A user with a policy grant for the product can access attributes
+        # of a private product.
+        with person_logged_in(owner):
+            getUtility(IService, 'sharing').sharePillarInformation(
+                product, ordinary_user, owner,
+                {InformationType.PROPRIETARY: SharingPermission.ALL})
+        with person_logged_in(ordinary_user):
             for attribute_name in names:
                 getattr(product, attribute_name)
 
@@ -599,6 +616,7 @@
         # attributes protected by the permission launchpad.AnyAllowedPerson.
         product = self.factory.makeProduct(
             information_type=InformationType.PROPRIETARY)
+        owner = removeSecurityProxy(product).owner
         names = self.expected_get_permissions['launchpad.AnyAllowedPerson']
         with person_logged_in(None):
             for attribute_name in names:
@@ -609,7 +627,16 @@
             for attribute_name in names:
                 self.assertRaises(
                     Unauthorized, getattr, product, attribute_name)
-        with person_logged_in(removeSecurityProxy(product).owner):
+        with person_logged_in(owner):
+            for attribute_name in names:
+                getattr(product, attribute_name)
+        # A user with a policy grant for the product can access attributes
+        # of a private product.
+        with person_logged_in(owner):
+            getUtility(IService, 'sharing').sharePillarInformation(
+                product, ordinary_user, owner,
+                {InformationType.PROPRIETARY: SharingPermission.ALL})
+        with person_logged_in(ordinary_user):
             for attribute_name in names:
                 getattr(product, attribute_name)
 
@@ -632,6 +659,7 @@
         # attributes protected by the permission launchpad.AnyAllowedPerson.
         product = self.factory.makeProduct(
             information_type=InformationType.PROPRIETARY)
+        owner = removeSecurityProxy(product).owner
         with person_logged_in(None):
             self.assertRaises(
                 Unauthorized, setattr, product, 'date_next_suggest_packaging',
@@ -641,8 +669,37 @@
             self.assertRaises(
                 Unauthorized, setattr, product, 'date_next_suggest_packaging',
                 'foo')
-        with person_logged_in(removeSecurityProxy(product).owner):
-            setattr(product, 'date_next_suggest_packaging', 'foo')
+        with person_logged_in(owner):
+            setattr(product, 'date_next_suggest_packaging', 'foo')
+        # A user with a policy grant for the product can access attributes
+        # of a private product.
+        with person_logged_in(owner):
+            getUtility(IService, 'sharing').sharePillarInformation(
+                product, ordinary_user, owner,
+                {InformationType.PROPRIETARY: SharingPermission.ALL})
+        with person_logged_in(ordinary_user):
+            setattr(product, 'date_next_suggest_packaging', 'foo')
+
+    def test_userCanView_caches_known_users(self):
+        # userCanView() maintains a cache of users known to have the
+        # permission to access a product.
+        product = self.factory.makeProduct(
+            information_type=InformationType.PROPRIETARY)
+        owner = removeSecurityProxy(product).owner
+        user = self.factory.makePerson()
+        with person_logged_in(owner):
+            getUtility(IService, 'sharing').sharePillarInformation(
+                product, user, owner,
+                {InformationType.PROPRIETARY: SharingPermission.ALL})
+        with person_logged_in(user):
+            with StormStatementRecorder() as recorder:
+                # The first access to a property of the product from
+                # a user requires a DB query.
+                product.homepageurl
+                self.assertEqual(1, len(recorder.queries))
+                # The second access does not require another query.
+                product.description
+                self.assertEqual(1, len(recorder.queries))
 
 
 class TestProductBugInformationTypes(TestCaseWithFactory):

=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py	2012-10-02 15:55:49 +0000
+++ lib/lp/testing/factory.py	2012-10-02 15:55:51 +0000
@@ -64,6 +64,7 @@
 
 from lp.app.enums import (
     InformationType,
+    PROPRIETARY_INFORMATION_TYPES,
     PUBLIC_INFORMATION_TYPES,
     ServiceUsage,
     )
@@ -1022,7 +1023,11 @@
             naked_product.setSpecificationSharingPolicy(
                 specification_sharing_policy)
         if information_type is not None:
+            owner = product.owner
             naked_product.information_type = information_type
+            if information_type in PROPRIETARY_INFORMATION_TYPES:
+                policy = self.makeAccessPolicy(product, information_type)
+                self.makeAccessPolicyGrant(policy, grantee=owner)
 
         return product