launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Review: Approve



Diff comments:

> === modified file 'lib/lp/registry/model/product.py'
> --- lib/lp/registry/model/product.py	2015-06-24 21:14:20 +0000
> +++ lib/lp/registry/model/product.py	2015-06-26 06:42:59 +0000
> @@ -38,7 +38,9 @@
>  from storm.locals import (
>      And,
>      Desc,
> +    Int,
>      Join,
> +    List,
>      Not,
>      Or,
>      Select,
> @@ -439,6 +441,11 @@
>          name='remote_product', allow_none=True, default=None)
>      vcs = EnumCol(enum=VCSType, notNull=False)
>  
> +    # Cache of AccessPolicy.ids that convey launchpad.LimitedView.
> +    # Unlike artifacts' cached access_policies, an AccessArtifactGrant
> +    # to an artifact in the policy is sufficient for access.
> +    access_policies = List(type=Int())
> +
>      @property
>      def date_next_suggest_packaging(self):
>          """See `IProduct`
> @@ -582,6 +589,7 @@
>                  self.setSpecificationSharingPolicy(
>                      specification_policy_default[value])
>              self._ensurePolicies([value])
> +            self._cacheAccessPolicies()

In the creation case, you call _cacheAccessPolicies after calling _ensurePolicies.  In the revocation case, you call _cacheAccessPolicies from _pruneUnusedPolicies.  It would be more consistent and harder to get wrong in future if you moved this call as well as that in _prepare_to_set_sharing_policy to the end of _ensurePolicies.

>  
>      information_type = property(_get_information_type, _set_information_type)
>  
> @@ -753,6 +761,7 @@
>                  raise ProprietaryProduct(
>                      "The project is %s." % self.information_type.title)
>          self._ensurePolicies(allowed_types[var])
> +        self._cacheAccessPolicies()
>  
>      def setBranchSharingPolicy(self, branch_sharing_policy):
>          """See `IProductEditRestricted`."""
> @@ -815,6 +824,23 @@
>              grants.append((p, self.owner, self.owner))
>          getUtility(IAccessPolicyGrantSource).grant(grants)
>  
> +    def _cacheAccessPolicies(self):
> +        # Update the cache of AccessPolicy.ids for which an
> +        # AccessPolicyGrant or AccessArtifactGrant is sufficient to
> +        # convey launchpad.LimitedView on this Product.
> +        #
> +        # We only need a cache for proprietary types, and it only
> +        # includes proprietary policies in case a policy like Private
> +        # Security was somehow left around when a project was
> +        # transitioned to Proprietary.
> +        if self.information_type in PROPRIETARY_INFORMATION_TYPES:
> +            self.access_policies = [
> +                policy.id for policy in
> +                getUtility(IAccessPolicySource).findByPillar([self])
> +                if policy.type in PROPRIETARY_INFORMATION_TYPES]

Perhaps this could use AccessPolicySource.find instead of filtering by type afterwards.

> +        else:
> +            self.access_policies = None
> +
>      def _pruneUnusedPolicies(self):
>          allowed_bug_types = set(
>              BUG_POLICY_ALLOWED_TYPES.get(
> @@ -840,6 +866,7 @@
>              and apa_source.findByPolicy([ap]).is_empty()]
>          getUtility(IAccessPolicyGrantSource).revokeByPolicy(unused_aps)
>          ap_source.delete([(ap.pillar, ap.type) for ap in unused_aps])
> +        self._cacheAccessPolicies()
>  
>      @cachedproperty
>      def commercial_subscription(self):
> 
> === modified file 'lib/lp/registry/tests/test_product.py'
> --- lib/lp/registry/tests/test_product.py	2015-06-25 07:39:40 +0000
> +++ lib/lp/registry/tests/test_product.py	2015-06-26 06:42:59 +0000
> @@ -527,6 +527,40 @@
>              SpecificationSharingPolicy.PROPRIETARY,
>              product.specification_sharing_policy)
>  
> +    def test_cacheAccessPolicies(self):
> +        # Product.access_policies is a list caching AccessPolicy.ids for
> +        # which an AccessPolicyGrant or AccessArtifactGrant gives a
> +        # principal LimitedView on the Product.
> +        aps = getUtility(IAccessPolicySource)
> +
> +        # Public projects don't need a cache.
> +        product = self.factory.makeProduct()
> +        naked_product = removeSecurityProxy(product)
> +        self.assertContentEqual(
> +            [InformationType.USERDATA, InformationType.PRIVATESECURITY],
> +            [p.type for p in aps.findByPillar([product])])
> +        self.assertIs(None, naked_product.access_policies)
> +
> +        # A private project normally just allows the Proprietary policy,
> +        # even if there is still another policy like Private Security.
> +        naked_product.information_type = InformationType.PROPRIETARY
> +        [prop_policy] = aps.find([(product, InformationType.PROPRIETARY)])
> +        self.assertEqual([prop_policy.id], naked_product.access_policies)
> +
> +        # If we switch it back to public, the cache is no longer
> +        # required.
> +        naked_product.information_type = InformationType.PUBLIC
> +        self.assertIs(None, naked_product.access_policies)
> +
> +        # Projects can also be Embargoed because of reasons. Since they
> +        # can have both Proprietary and Embargoed artifacts, and someone
> +        # who can see either needs LimitedView on the pillar they're on,
> +        # both policies are permissible.
> +        naked_product.information_type = InformationType.EMBARGOED
> +        [emb_policy] = aps.find([(product, InformationType.EMBARGOED)])
> +        self.assertContentEqual(
> +            [prop_policy.id, emb_policy.id], naked_product.access_policies)
> +
>      def test_checkInformationType_bug_supervisor(self):
>          # Bug supervisors of proprietary products must not have inclusive
>          # membership policies.
> 


-- 
https://code.launchpad.net/~wgrant/launchpad/product-aps-set/+merge/263062
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.