Merge ~pappacena/launchpad:ocirecipe-private-reconcile-pillar into launchpad:master

[Thiago F. Pappacena]

Running reconcile for OCI recipes when an OCI project changes pillar

When we change an OCI project's pillar, we should reconcile access artifacts for every OCI recipe associated with that.

This MP got kind of big mostly because of a refactoring on reconcile_access_for_artifact signature, to allow bulk operations.

[Colin Watson (cjwatson)]

review: Approve

diff --git a/lib/lp/blueprints/model/specification.py b/lib/lp/blueprints/model/specification.py
index 55ce9d5..9f3b00a 100644
--- a/lib/lp/blueprints/model/specification.py
+++ b/lib/lp/blueprints/model/specification.py
@@ -922,14 +922,14 @@ class Specification(SQLBase, BugLinkTargetMixin, InformationTypeMixin):
         """See ISpecification."""
         # avoid circular imports.
         from lp.registry.model.accesspolicy import (
-            reconcile_access_for_artifact,
+            reconcile_access_for_artifacts,
             )
         if self.information_type == information_type:
             return False
         if information_type not in self.getAllowedInformationTypes(who):
             raise CannotChangeInformationType("Forbidden by project policy.")
         self.information_type = information_type
-        reconcile_access_for_artifact(self, information_type, [self.target])
+        reconcile_access_for_artifacts([self], information_type, [self.target])
         if (information_type in PRIVATE_INFORMATION_TYPES and
                 not self.subscribers.is_empty()):
             # Grant the subscribers access if they do not have a
diff --git a/lib/lp/bugs/model/bug.py b/lib/lp/bugs/model/bug.py
index f7ebc86..f2148e0 100644
--- a/lib/lp/bugs/model/bug.py
+++ b/lib/lp/bugs/model/bug.py
@@ -189,7 +189,7 @@ from lp.registry.interfaces.sharingjob import (
     IRemoveArtifactSubscriptionsJobSource,
     )
 from lp.registry.interfaces.sourcepackage import ISourcePackage
-from lp.registry.model.accesspolicy import reconcile_access_for_artifact
+from lp.registry.model.accesspolicy import reconcile_access_for_artifacts
 from lp.registry.model.person import (
     Person,
     person_sort_key,
@@ -2122,7 +2122,7 @@ class Bug(SQLBase, InformationTypeMixin):
             BugSubscription.person == person).is_empty()
 
     def _reconcileAccess(self):
-        # reconcile_access_for_artifact will only use the pillar list if
+        # reconcile_access_for_artifacts will only use the pillar list if
         # the information type is private. But affected_pillars iterates
         # over the tasks immediately, which is needless expense for
         # public bugs.
@@ -2130,8 +2130,8 @@ class Bug(SQLBase, InformationTypeMixin):
             pillars = self.affected_pillars
         else:
             pillars = []
-        reconcile_access_for_artifact(
-            self, self.information_type, pillars)
+        reconcile_access_for_artifacts(
+            [self], self.information_type, pillars)
 
     def _attachments_query(self):
         """Helper for the attachments* properties."""
diff --git a/lib/lp/code/model/branch.py b/lib/lp/code/model/branch.py
index 278db40..27746c7 100644
--- a/lib/lp/code/model/branch.py
+++ b/lib/lp/code/model/branch.py
@@ -160,7 +160,7 @@ from lp.registry.interfaces.sharingjob import (
     )
 from lp.registry.model.accesspolicy import (
     AccessPolicyGrant,
-    reconcile_access_for_artifact,
+    reconcile_access_for_artifacts,
     )
 from lp.registry.model.teammembership import TeamParticipation
 from lp.services.config import config
@@ -259,8 +259,8 @@ class Branch(SQLBase, WebhookTargetMixin, BzrIdentityMixin):
             # works, so only work for products for now.
             if self.product is not None:
                 pillars = [self.product]
-        reconcile_access_for_artifact(
-            self, self.information_type, pillars, wanted_links)
+        reconcile_access_for_artifacts(
+            [self], self.information_type, pillars, wanted_links)
 
     def setPrivate(self, private, user):
         """See `IBranch`."""
diff --git a/lib/lp/code/model/gitrepository.py b/lib/lp/code/model/gitrepository.py
index 21a6f82..75c6905 100644
--- a/lib/lp/code/model/gitrepository.py
+++ b/lib/lp/code/model/gitrepository.py
@@ -173,7 +173,7 @@ from lp.registry.interfaces.sharingjob import (
     )
 from lp.registry.model.accesspolicy import (
     AccessPolicyGrant,
-    reconcile_access_for_artifact,
+    reconcile_access_for_artifacts,
     )
 from lp.registry.model.person import Person
 from lp.registry.model.teammembership import TeamParticipation
@@ -618,8 +618,8 @@ class GitRepository(StormBase, WebhookTargetMixin, GitIdentityMixin):
             # works, so only work for projects for now.
             if self.project is not None:
                 pillars = [self.project]
-        reconcile_access_for_artifact(
-            self, self.information_type, pillars, wanted_links)
+        reconcile_access_for_artifacts(
+            [self], self.information_type, pillars, wanted_links)
 
     @property
     def refs(self):
diff --git a/lib/lp/oci/model/ocirecipe.py b/lib/lp/oci/model/ocirecipe.py
index 1a7dda3..2aa988c 100644
--- a/lib/lp/oci/model/ocirecipe.py
+++ b/lib/lp/oci/model/ocirecipe.py
@@ -94,7 +94,7 @@ from lp.registry.interfaces.person import (
     validate_public_person,
     )
 from lp.registry.interfaces.role import IPersonRoles
-from lp.registry.model.accesspolicy import reconcile_access_for_artifact
+from lp.registry.model.accesspolicy import reconcile_access_for_artifacts
 from lp.registry.model.distribution import Distribution
 from lp.registry.model.distroseries import DistroSeries
 from lp.registry.model.person import Person
@@ -293,8 +293,8 @@ class OCIRecipe(Storm, WebhookTargetMixin):
         Takes the privacy and pillar and makes the related AccessArtifact
         and AccessPolicyArtifacts match.
         """
-        reconcile_access_for_artifact(self, self.information_type,
-                                      [self.pillar])
+        reconcile_access_for_artifacts([self], self.information_type,
+                                       [self.pillar])
 
     def destroySelf(self):
         """See `IOCIRecipe`."""
diff --git a/lib/lp/oci/tests/test_ocirecipe.py b/lib/lp/oci/tests/test_ocirecipe.py
index 0cd3d54..246b069 100644
--- a/lib/lp/oci/tests/test_ocirecipe.py
+++ b/lib/lp/oci/tests/test_ocirecipe.py
@@ -67,6 +67,11 @@ from lp.registry.enums import (
     PersonVisibility,
     TeamMembershipPolicy,
     )
+from lp.registry.interfaces.accesspolicy import (
+    IAccessArtifactSource,
+    IAccessPolicyArtifactSource,
+    IAccessPolicySource,
+    )
 from lp.registry.interfaces.series import SeriesStatus
 from lp.services.config import config
 from lp.services.database.constants import (
@@ -830,6 +835,56 @@ class TestOCIRecipe(OCIConfigHelperMixin, TestCaseWithFactory):
             public_recipe, 'owner', private_team)
 
 
+class TestOCIRecipeAccessControl(TestCaseWithFactory, OCIConfigHelperMixin):
+    layer = DatabaseFunctionalLayer
+
+    def setUp(self):
+        super(TestOCIRecipeAccessControl, self).setUp()
+        self.setConfig()
+
+    def test_change_oci_project_pillar_reconciles_access(self):
+        person = self.factory.makePerson()
+        initial_project = self.factory.makeProduct(
+            name='initial-project',
+            owner=person, registrant=person)
+        final_project = self.factory.makeProduct(
+            name='final-project',
+            owner=person, registrant=person)
+        oci_project = self.factory.makeOCIProject(
+            ociprojectname='the-oci-project', pillar=initial_project,
+            registrant=person)
+        recipes = []
+        for i in range(10):
+            recipes.append(self.factory.makeOCIRecipe(
+                registrant=person,
+                oci_project=oci_project,
+                information_type=InformationType.USERDATA))
+
+        access_artifacts = getUtility(IAccessArtifactSource).find(recipes)
+        initial_access_policy = getUtility(IAccessPolicySource).find(
+            [(initial_project, InformationType.USERDATA)]).one()
+        apasource = getUtility(IAccessPolicyArtifactSource)
+        policy_artifacts = apasource.find(
+            [(recipe_artifact, initial_access_policy)
+             for recipe_artifact in access_artifacts])
+        self.assertEqual(
+            {i.policy.pillar for i in policy_artifacts}, {initial_project})
+
+        # Changing OCI project's pillar should move the policy artifacts of
+        # all OCI recipes associated to the new pillar.
+        flush_database_caches()
+        with admin_logged_in():
+            oci_project.pillar = final_project
+
+        final_access_policy = getUtility(IAccessPolicySource).find(
+            [(final_project, InformationType.USERDATA)]).one()
+        policy_artifacts = apasource.find(
+            [(recipe_artifact, final_access_policy)
+             for recipe_artifact in access_artifacts])
+        self.assertEqual(
+            {i.policy.pillar for i in policy_artifacts}, {final_project})
+
+
 class TestOCIRecipeProcessors(TestCaseWithFactory):
 
     layer = DatabaseFunctionalLayer
diff --git a/lib/lp/registry/model/accesspolicy.py b/lib/lp/registry/model/accesspolicy.py
index 64b69c8..010e49e 100644
--- a/lib/lp/registry/model/accesspolicy.py
+++ b/lib/lp/registry/model/accesspolicy.py
@@ -11,10 +11,11 @@ __all__ = [
     'AccessPolicyArtifact',
     'AccessPolicyGrant',
     'AccessPolicyGrantFlat',
-    'reconcile_access_for_artifact',
+    'reconcile_access_for_artifacts',
     ]
 
 from collections import defaultdict
+from itertools import product
 
 import pytz
 from storm.expr import (
@@ -57,14 +58,14 @@ from lp.services.database.interfaces import IStore
 from lp.services.database.stormbase import StormBase
 
 
-def reconcile_access_for_artifact(artifact, information_type, pillars,
-                                  wanted_links=None):
+def reconcile_access_for_artifacts(artifacts, information_type, pillars,
+                                   wanted_links=None):
     if information_type in PUBLIC_INFORMATION_TYPES:
         # If it's public we can delete all the access information.
         # IAccessArtifactSource handles the cascade.
-        getUtility(IAccessArtifactSource).delete([artifact])
+        getUtility(IAccessArtifactSource).delete(artifacts)
         return
-    [abstract_artifact] = getUtility(IAccessArtifactSource).ensure([artifact])
+    abstract_artifacts = getUtility(IAccessArtifactSource).ensure(artifacts)
     aps = getUtility(IAccessPolicySource).find(
         (pillar, information_type) for pillar in pillars)
     missing_pillars = set(pillars) - set([ap.pillar for ap in aps])
@@ -77,11 +78,11 @@ def reconcile_access_for_artifact(artifact, information_type, pillars,
     # Now determine the existing and desired links, and make them
     # match. The caller may have provided the wanted_links.
     apasource = getUtility(IAccessPolicyArtifactSource)
-    wanted_links = (wanted_links
-                    or set((abstract_artifact, policy) for policy in aps))
+    wanted_links = (
+            wanted_links or set(product(abstract_artifacts, aps)))
     existing_links = set([
         (apa.abstract_artifact, apa.policy)
-        for apa in apasource.findByArtifact([abstract_artifact])])
+        for apa in apasource.findByArtifact(abstract_artifacts)])
     apasource.create(wanted_links - existing_links)
     apasource.delete(existing_links - wanted_links)
 
diff --git a/lib/lp/registry/model/ociproject.py b/lib/lp/registry/model/ociproject.py
index eabde9b..b034602 100644
--- a/lib/lp/registry/model/ociproject.py
+++ b/lib/lp/registry/model/ociproject.py
@@ -1,4 +1,4 @@
-# Copyright 2019-2020 Canonical Ltd.  This software is licensed under the
+# Copyright 2019-2021 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """OCI Project implementation."""
@@ -11,6 +11,8 @@ __all__ = [
     'OCIProjectSet',
     ]
 
+from collections import defaultdict
+
 import pytz
 import six
 from six import text_type
@@ -42,6 +44,7 @@ from lp.registry.interfaces.ociprojectname import IOCIProjectNameSet
 from lp.registry.interfaces.person import IPersonSet
 from lp.registry.interfaces.product import IProduct
 from lp.registry.interfaces.series import SeriesStatus
+from lp.registry.model.accesspolicy import reconcile_access_for_artifacts
 from lp.registry.model.ociprojectname import OCIProjectName
 from lp.registry.model.ociprojectseries import OCIProjectSeries
 from lp.registry.model.person import Person
@@ -115,6 +118,11 @@ class OCIProject(BugTargetBase, StormBase):
 
     @pillar.setter
     def pillar(self, pillar):
+        """See `IBugTarget`."""
+        # We need to reconcile access for all OCI recipes from this OCI
+        # project if we are moving from one pillar to another.
+        needs_reconcile_access = (
+                self.pillar is not None and self.pillar != pillar)
         if IDistribution.providedBy(pillar):
             self.distribution = pillar
             self.project = None
@@ -125,6 +133,8 @@ class OCIProject(BugTargetBase, StormBase):
             raise ValueError(
                 'The target of an OCIProject must be either an IDistribution '
                 'or IProduct instance.')
+        if needs_reconcile_access:
+            self._reconcileAccess()
 
     @property
     def display_name(self):
@@ -135,6 +145,19 @@ class OCIProject(BugTargetBase, StormBase):
     bugtargetname = display_name
     bugtargetdisplayname = display_name
 
+    def _reconcileAccess(self):
+        """Reconcile access for all OCI recipes of this project."""
+        from lp.oci.model.ocirecipe import OCIRecipe
+        rs = IStore(OCIRecipe).find(
+            OCIRecipe,
+            OCIRecipe.oci_project == self)
+        recipes_per_info_type = defaultdict(set)
+        for recipe in rs:
+            recipes_per_info_type[recipe.information_type].add(recipe)
+        for information_type, recipes in recipes_per_info_type.items():
+            reconcile_access_for_artifacts(
+                recipes, information_type, [self.pillar])
+
     def newRecipe(self, name, registrant, owner, git_ref,
                   build_file, description=None, build_daily=False,
                   require_virtualized=True, build_args=None):
diff --git a/lib/lp/registry/tests/test_accesspolicy.py b/lib/lp/registry/tests/test_accesspolicy.py
index 6ab5418..ce70d43 100644
--- a/lib/lp/registry/tests/test_accesspolicy.py
+++ b/lib/lp/registry/tests/test_accesspolicy.py
@@ -1,4 +1,4 @@
-# Copyright 2011-2015 Canonical Ltd.  This software is licensed under the
+# Copyright 2011-2021 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __metaclass__ = type
@@ -22,12 +22,18 @@ from lp.registry.interfaces.accesspolicy import (
     IAccessPolicyGrantSource,
     IAccessPolicySource,
     )
-from lp.registry.model.accesspolicy import reconcile_access_for_artifact
+from lp.registry.model.accesspolicy import reconcile_access_for_artifacts
 from lp.registry.model.person import Person
 from lp.services.database.interfaces import IStore
-from lp.testing import TestCaseWithFactory
+from lp.testing import (
+    record_two_runs,
+    TestCaseWithFactory,
+    )
 from lp.testing.layers import DatabaseFunctionalLayer
-from lp.testing.matchers import Provides
+from lp.testing.matchers import (
+    HasQueryCount,
+    Provides,
+    )
 
 
 def get_policies_for_artifact(concrete_artifact):
@@ -729,57 +735,79 @@ class TestReconcileAccessPolicyArtifacts(TestCaseWithFactory):
             get_policies_for_artifact(bug))
 
     def test_creates_missing_accessartifact(self):
-        # reconcile_access_for_artifact creates an AccessArtifact for a
+        # reconcile_access_for_artifacts creates an AccessArtifact for a
         # private artifact if there isn't one already.
         bug = self.factory.makeBug()
 
         self.assertTrue(
             getUtility(IAccessArtifactSource).find([bug]).is_empty())
-        reconcile_access_for_artifact(bug, InformationType.USERDATA, [])
+        reconcile_access_for_artifacts([bug], InformationType.USERDATA, [])
         self.assertFalse(
             getUtility(IAccessArtifactSource).find([bug]).is_empty())
 
+    def test_bulk_creates_missing_accessartifact_query_count(self):
+        # reconcile_access_for_artifacts creates one for each AccessArtifact
+        # private artifact if there isn't one already.
+        bugs = [self.factory.makeBug()]
+
+        def create_bugs():
+            while len(bugs):
+                bugs.pop()
+            for i in range(10):
+                bugs.append(self.factory.makeBug())
+
+        def reconcile():
+            reconcile_access_for_artifacts(bugs, InformationType.USERDATA, [])
+
+        # Runs with original `bugs` list with 1 item, then cleanup that list
+        # and create another set of new bugs.
+        recorder1, recorder2 = record_two_runs(reconcile, create_bugs, 0, 1)
+        self.assertThat(recorder2, HasQueryCount.byEquality(recorder1))
+
+        self.assertEqual(
+            10, getUtility(IAccessArtifactSource).find(bugs).count())
+
     def test_removes_extra_accessartifact(self):
-        # reconcile_access_for_artifact removes an AccessArtifact for a
+        # reconcile_access_for_artifacts removes an AccessArtifact for a
         # public artifact if there's one left over.
         bug = self.factory.makeBug()
-        reconcile_access_for_artifact(bug, InformationType.USERDATA, [])
+        reconcile_access_for_artifacts([bug], InformationType.USERDATA, [])
 
         self.assertFalse(
             getUtility(IAccessArtifactSource).find([bug]).is_empty())
-        reconcile_access_for_artifact(bug, InformationType.PUBLIC, [])
+        reconcile_access_for_artifacts([bug], InformationType.PUBLIC, [])
         self.assertTrue(
             getUtility(IAccessArtifactSource).find([bug]).is_empty())
 
     def test_adds_missing_accesspolicyartifacts(self):
-        # reconcile_access_for_artifact adds missing links.
+        # reconcile_access_for_artifacts adds missing links.
         product = self.factory.makeProduct()
         bug = self.factory.makeBug(target=product)
-        reconcile_access_for_artifact(bug, InformationType.USERDATA, [])
+        reconcile_access_for_artifacts([bug], InformationType.USERDATA, [])
 
         self.assertPoliciesForBug([], bug)
-        reconcile_access_for_artifact(
-            bug, InformationType.USERDATA, [product])
+        reconcile_access_for_artifacts(
+            [bug], InformationType.USERDATA, [product])
         self.assertPoliciesForBug([(product, InformationType.USERDATA)], bug)
 
     def test_removes_extra_accesspolicyartifacts(self):
-        # reconcile_access_for_artifact removes excess links.
+        # reconcile_access_for_artifacts removes excess links.
         bug = self.factory.makeBug()
         product = self.factory.makeProduct()
         other_product = self.factory.makeProduct()
-        reconcile_access_for_artifact(
-            bug, InformationType.USERDATA, [product, other_product])
+        reconcile_access_for_artifacts(
+            [bug], InformationType.USERDATA, [product, other_product])
 
         self.assertPoliciesForBug(
             [(product, InformationType.USERDATA),
              (other_product, InformationType.USERDATA)],
             bug)
-        reconcile_access_for_artifact(
-            bug, InformationType.USERDATA, [product])
+        reconcile_access_for_artifacts(
+            [bug], InformationType.USERDATA, [product])
         self.assertPoliciesForBug([(product, InformationType.USERDATA)], bug)
 
     def test_raises_exception_on_missing_policies(self):
-        # reconcile_access_for_artifact raises an exception if a pillar is
+        # reconcile_access_for_artifacts raises an exception if a pillar is
         # missing an AccessPolicy.
         product = self.factory.makeProduct()
         # Creating a product will have created two APs, delete them.
@@ -792,5 +820,5 @@ class TestReconcileAccessPolicyArtifacts(TestCaseWithFactory):
             "Pillar(s) %s require an access policy for information type "
             "Private.") % product.name
         self.assertRaisesWithContent(
-            AssertionError, expected, reconcile_access_for_artifact, bug,
+            AssertionError, expected, reconcile_access_for_artifacts, [bug],
             InformationType.USERDATA, [product])
diff --git a/lib/lp/registry/tests/test_sharingjob.py b/lib/lp/registry/tests/test_sharingjob.py
index 32aec0f..73ad71f 100644
--- a/lib/lp/registry/tests/test_sharingjob.py
+++ b/lib/lp/registry/tests/test_sharingjob.py
@@ -27,7 +27,7 @@ from lp.registry.interfaces.sharingjob import (
     ISharingJob,
     ISharingJobSource,
     )
-from lp.registry.model.accesspolicy import reconcile_access_for_artifact
+from lp.registry.model.accesspolicy import reconcile_access_for_artifacts
 from lp.registry.model.sharingjob import (
     RemoveArtifactSubscriptionsJob,
     SharingJob,
@@ -378,8 +378,8 @@ class RemoveArtifactSubscriptionsJobTestCase(TestCaseWithFactory):
         # Change artifact attributes so that it can become inaccessible for
         # some users.
         change_callback(concrete_artifact)
-        reconcile_access_for_artifact(
-            concrete_artifact, concrete_artifact.information_type,
+        reconcile_access_for_artifacts(
+            [concrete_artifact], concrete_artifact.information_type,
             get_pillars(concrete_artifact))
 
         getUtility(IRemoveArtifactSubscriptionsJobSource).create(
diff --git a/lib/lp/snappy/model/snap.py b/lib/lp/snappy/model/snap.py
index 5b1be23..afa7e7d 100644
--- a/lib/lp/snappy/model/snap.py
+++ b/lib/lp/snappy/model/snap.py
@@ -132,7 +132,7 @@ from lp.registry.interfaces.role import (
     )
 from lp.registry.model.accesspolicy import (
     AccessPolicyGrant,
-    reconcile_access_for_artifact,
+    reconcile_access_for_artifacts,
     )
 from lp.registry.model.distroseries import DistroSeries
 from lp.registry.model.person import Person
@@ -1238,7 +1238,7 @@ class Snap(Storm, WebhookTargetMixin):
         if self.project is None:
             return
         pillars = [self.project]
-        reconcile_access_for_artifact(self, self.information_type, pillars)
+        reconcile_access_for_artifacts([self], self.information_type, pillars)
 
     def setProject(self, project):
         self.project = project