launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

William Grant has proposed merging lp:~wgrant/launchpad/product-aps-use into lp:launchpad with lp:~wgrant/launchpad/product-aps-db-2 as a prerequisite.

Commit message:
Reimplement getProductPrivacyFilter using Product.access_policies, and port LimitedView and getAffiliatedPillars checks to it.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1452949 in Launchpad itself: "Timeout error while trying to access team site"
  https://bugs.launchpad.net/launchpad/+bug/1452949

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/product-aps-use/+merge/263064

Reimplement getProductPrivacyFilter using Product.access_policies, and port LimitedView and getAffiliatedPillars checks to it.

There are two slight behaviour changes here:

 - The IProduct launchpad.LimitedView adapter previously allowed anyone
   with access to an artifact or policy of any information type, but it
   now requires that to be a proprietary type. That shouldn't actually
   occur in practice, as a proprietary project cannot have artifacts
   or policies of a non-proprietary type.

 - ProductSet.getProductPrivacyFilter previously required an artifact
   or policy grant on the policy matching the project's information
   type. That's now relaxed to matching any proprietary information
   type, just like LimitedView.

This gets getAffiliatedPillars for a user like me down from >500ms to ~10ms.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/product-aps-use into lp:launchpad.

=== modified file 'lib/lp/registry/interfaces/product.py'
--- lib/lp/registry/interfaces/product.py	2015-06-24 21:14:20 +0000
+++ lib/lp/registry/interfaces/product.py	2015-06-26 06:40:05 +0000
@@ -393,6 +393,9 @@
     def userCanView(user):
         """True if the given user has access to this product."""
 
+    def userCanLimitedView(user):
+        """True if the given user has limited access to this product."""
+
     private = exported(
         Bool(
             title=_("Product is confidential"), required=False,

=== modified file 'lib/lp/registry/interfaces/sharingservice.py'
--- lib/lp/registry/interfaces/sharingservice.py	2015-03-05 16:23:26 +0000
+++ lib/lp/registry/interfaces/sharingservice.py	2015-06-26 06:40:05 +0000
@@ -113,9 +113,6 @@
         :return: a (bugtasks, branches, gitrepositories, specifications) tuple
         """
 
-    def checkPillarArtifactAccess(pillar, user):
-        """Return True if user has any grants on pillar else return False."""
-
     @export_read_operation()
     @call_with(user=REQUEST_USER)
     @operation_parameters(

=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py	2015-04-19 12:56:32 +0000
+++ lib/lp/registry/model/person.py	2015-06-26 06:40:05 +0000
@@ -249,6 +249,7 @@
 from lp.services.database.interfaces import IStore
 from lp.services.database.policy import MasterDatabasePolicy
 from lp.services.database.sqlbase import (
+    convert_storm_clause_to_string,
     cursor,
     quote,
     SQLBase,
@@ -1013,52 +1014,23 @@
 
     def _genAffiliatedProductSql(self, user=None):
         """Helper to generate the product sql for getAffiliatePillars"""
+        from lp.registry.model.product import ProductSet
         base_query = """
             SELECT name, 3 as kind, displayname
-            FROM product p
+            FROM product
             WHERE
-                p.active = True
+                product.active = True
                 AND (
-                    p.driver = %(person)s
-                    OR p.owner = %(person)s
-                    OR p.bug_supervisor = %(person)s
+                    product.driver = %(person)s
+                    OR product.owner = %(person)s
+                    OR product.bug_supervisor = %(person)s
                 )
         """ % sqlvalues(person=self)
 
-        if user is not None:
-            roles = IPersonRoles(user)
-            if roles.in_admin or roles.in_commercial_admin:
-                return base_query
-
-        # This is the raw sql version of model/product getProductPrivacyFilter
-        granted_products = """
-            SELECT p.id
-            FROM product p,
-                 accesspolicygrantflat apflat,
-                 teamparticipation part,
-                 accesspolicy ap
-             WHERE
-                apflat.grantee = part.team
-                AND part.person = %(user)s
-                AND apflat.policy = ap.id
-                AND ap.product = p.id
-                AND ap.type = p.information_type
-        """ % sqlvalues(user=user)
-
-        # We have to generate the sqlvalues first so that they're properly
-        # setup and escaped. Then we combine the above query which is already
-        # processed.
-        query_values = sqlvalues(information_type=InformationType.PUBLIC)
-        query_values.update(granted_sql=granted_products)
-
-        query = base_query + """
-                AND (
-                    p.information_type = %(information_type)s
-                    OR p.information_type is NULL
-                    OR p.id IN (%(granted_sql)s)
-                )
-        """ % query_values
-        return query
+        return (
+            base_query + " AND "
+            + convert_storm_clause_to_string(
+                ProductSet.getProductPrivacyFilter(user)))
 
     def getAffiliatedPillars(self, user):
         """See `IPerson`."""

=== modified file 'lib/lp/registry/model/product.py'
--- lib/lp/registry/model/product.py	2015-06-26 06:40:04 +0000
+++ lib/lp/registry/model/product.py	2015-06-26 06:40:05 +0000
@@ -32,19 +32,20 @@
     StringCol,
     )
 from storm.expr import (
-    LeftJoin,
-    NamedFunc,
-    )
-from storm.locals import (
     And,
+    Coalesce,
     Desc,
-    Int,
     Join,
-    List,
+    LeftJoin,
+    NamedFunc,
     Not,
     Or,
     Select,
     SQL,
+    )
+from storm.locals import (
+    Int,
+    List,
     Store,
     Unicode,
     )
@@ -165,10 +166,7 @@
     )
 from lp.registry.interfaces.productrelease import IProductReleaseSet
 from lp.registry.interfaces.role import IPersonRoles
-from lp.registry.model.accesspolicy import (
-    AccessPolicy,
-    AccessPolicyGrantFlat,
-    )
+from lp.registry.model.accesspolicy import AccessPolicyGrantFlat
 from lp.registry.model.announcement import MakesAnnouncements
 from lp.registry.model.commercialsubscription import CommercialSubscription
 from lp.registry.model.distribution import Distribution
@@ -199,7 +197,11 @@
     SQLBase,
     sqlvalues,
     )
-from lp.services.database.stormexpr import fti_search
+from lp.services.database.stormexpr import (
+    ArrayAgg,
+    ArrayIntersects,
+    fti_search,
+    )
 from lp.services.propertycache import (
     cachedproperty,
     get_property_cache,
@@ -1626,6 +1628,17 @@
             return True
         return False
 
+    def userCanLimitedView(self, user):
+        """See `IProductPublic`."""
+        if self.userCanView(user):
+            return True
+        if user is None:
+            return False
+        return not Store.of(self).find(
+            Product,
+            Product.id == self.id,
+            ProductSet.getProductPrivacyFilter(user.person)).is_empty()
+
 
 def get_precached_products(products, need_licences=False,
                            need_projectgroups=False, need_series=False,
@@ -1816,19 +1829,35 @@
 
     @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)))
+        # Anonymous users can only see public projects.
+        public_filter = Product._information_type == InformationType.PUBLIC
+        if user is None:
+            return public_filter
+
+        # (Commercial) admins can see any project.
+        roles = IPersonRoles(user)
+        if roles.in_admin or roles.in_commercial_admin:
+            return True
+
+        # Normal users can see any project for which they can see either
+        # an entire policy or an artifact.
+        # XXX wgrant 2015-06-26: This is slower than ideal for people in
+        # teams with lots of artifact grants, as there can be tens of
+        # thousands of APGF rows for a single policy. But it's tens of
+        # milliseconds at most.
+        grant_filter = Coalesce(
+            ArrayIntersects(
+                Product.access_policies,
+                Select(
+                    ArrayAgg(AccessPolicyGrantFlat.policy_id),
+                    tables=(AccessPolicyGrantFlat,
+                            Join(TeamParticipation,
+                                TeamParticipation.teamID ==
+                                AccessPolicyGrantFlat.grantee_id)),
+                    where=(TeamParticipation.person == user)
+                )),
+            False)
+        return Or(public_filter, grant_filter)
 
     @classmethod
     def get_users_private_products(cls, user):

=== modified file 'lib/lp/registry/services/sharingservice.py'
--- lib/lp/registry/services/sharingservice.py	2015-02-27 11:27:04 +0000
+++ lib/lp/registry/services/sharingservice.py	2015-06-26 06:40:05 +0000
@@ -240,21 +240,6 @@
 
         return bugtasks, branches, gitrepositories, specifications
 
-    def checkPillarArtifactAccess(self, pillar, user):
-        """See `ISharingService`."""
-        tables = [
-            AccessPolicyGrantFlat,
-            Join(
-                TeamParticipation,
-                TeamParticipation.teamID == AccessPolicyGrantFlat.grantee_id),
-            Join(
-                AccessPolicy,
-                AccessPolicy.id == AccessPolicyGrantFlat.policy_id)]
-        return not IStore(AccessPolicyGrantFlat).using(*tables).find(
-            AccessPolicyGrantFlat,
-            AccessPolicy.product_id == pillar.id,
-            TeamParticipation.personID == user.id).is_empty()
-
     @available_with_permission('launchpad.Driver', 'pillar')
     def getSharedBugs(self, pillar, person, user):
         """See `ISharingService`."""

=== modified file 'lib/lp/registry/services/tests/test_sharingservice.py'
--- lib/lp/registry/services/tests/test_sharingservice.py	2015-05-14 13:57:51 +0000
+++ lib/lp/registry/services/tests/test_sharingservice.py	2015-06-26 06:40:05 +0000
@@ -1890,18 +1890,6 @@
             self.service.checkPillarAccess(
                 [product], InformationType.USERDATA, right_person))
 
-    def test_checkPillarArtifactAccess_respects_teams(self):
-        owner = self.factory.makePerson()
-        product = self.factory.makeProduct(
-            information_type=InformationType.PROPRIETARY, owner=owner)
-        user = self.factory.makePerson()
-        team = self.factory.makeTeam(
-            membership_policy=TeamMembershipPolicy.MODERATED, members=[user])
-        with person_logged_in(owner):
-            bug = self.factory.makeBug(target=product)
-            bug.subscribe(team, owner)
-        self.assertTrue(self.service.checkPillarArtifactAccess(product, user))
-
     def test_checkPillarAccess_no_policy(self):
         # checkPillarAccess returns False if there's no policy.
         self.assertFalse(

=== modified file 'lib/lp/registry/tests/test_product.py'
--- lib/lp/registry/tests/test_product.py	2015-06-26 06:40:04 +0000
+++ lib/lp/registry/tests/test_product.py	2015-06-26 06:40:05 +0000
@@ -2346,9 +2346,9 @@
         # 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)
+        self.factory.makeAccessPolicy(
+            proprietary, InformationType.PRIVATESECURITY)
+        self.grant(proprietary, InformationType.PRIVATESECURITY, grantee)
         result = self.filterFind(grantee)
         self.assertIn(public, result)
         self.assertNotIn(embargoed, result)

=== modified file 'lib/lp/security.py'
--- lib/lp/security.py	2015-04-22 16:11:40 +0000
+++ lib/lp/security.py	2015-06-26 06:40:05 +0000
@@ -462,8 +462,7 @@
     def checkAuthenticated(self, user):
         return (
             super(LimitedViewProduct, self).checkAuthenticated(user) or
-            getUtility(IService, 'sharing').checkPillarArtifactAccess(
-                self.obj, user))
+            self.obj.userCanLimitedView(user))
 
 
 class EditProduct(EditByOwnersOrAdmins):