launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #26610
[Merge] ~pappacena/launchpad:ocirecipe-private-reconcile-pillar into launchpad:master
Thiago F. Pappacena has proposed merging ~pappacena/launchpad:ocirecipe-private-reconcile-pillar into launchpad:master with ~pappacena/launchpad:ocirecipe-private-accesspolicy as a prerequisite.
Commit message:
Running reconcile for OCI recipes when an OCI project changes pillar
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~pappacena/launchpad/+git/launchpad/+merge/399469
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.
--
Your team Launchpad code reviewers is requested to review the proposed merge of ~pappacena/launchpad:ocirecipe-private-reconcile-pillar into launchpad:master.
diff --git a/lib/lp/blueprints/model/specification.py b/lib/lp/blueprints/model/specification.py
index e0d4001..abd2786 100644
--- a/lib/lp/blueprints/model/specification.py
+++ b/lib/lp/blueprints/model/specification.py
@@ -1,4 +1,4 @@
-# Copyright 2009-2020 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2021 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
__metaclass__ = type
@@ -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 a9b177c..7c9326c 100644
--- a/lib/lp/bugs/model/bug.py
+++ b/lib/lp/bugs/model/bug.py
@@ -1,4 +1,4 @@
-# Copyright 2009-2020 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2021 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Launchpad bug-related database table classes."""
@@ -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 947aaf1..291045d 100644
--- a/lib/lp/code/model/branch.py
+++ b/lib/lp/code/model/branch.py
@@ -1,4 +1,4 @@
-# Copyright 2009-2020 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2021 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
__metaclass__ = type
@@ -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 74d94ec..60294f3 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
@@ -615,8 +615,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 64f4e9c..cabce57 100644
--- a/lib/lp/oci/model/ocirecipe.py
+++ b/lib/lp/oci/model/ocirecipe.py
@@ -86,7 +86,7 @@ from lp.oci.model.ocirecipejob import OCIRecipeJob
from lp.registry.interfaces.distribution import IDistributionSet
from lp.registry.interfaces.person import IPersonSet
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
@@ -251,8 +251,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 2879b37..6c08bb1 100644
--- a/lib/lp/oci/tests/test_ocirecipe.py
+++ b/lib/lp/oci/tests/test_ocirecipe.py
@@ -31,6 +31,7 @@ from zope.security.interfaces import (
)
from zope.security.proxy import removeSecurityProxy
+from lp.app.enums import InformationType
from lp.buildmaster.enums import BuildStatus
from lp.buildmaster.interfaces.processor import IProcessorSet
from lp.oci.interfaces.ocipushrule import (
@@ -60,6 +61,11 @@ from lp.oci.tests.helpers import (
MatchesOCIRegistryCredentials,
OCIConfigHelperMixin,
)
+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 (
@@ -787,6 +793,56 @@ class TestOCIRecipe(OCIConfigHelperMixin, TestCaseWithFactory):
self.assertEqual(recipe.name, recipe.image_name)
+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 58fa5c1..7ff2b5f 100644
--- a/lib/lp/registry/tests/test_sharingjob.py
+++ b/lib/lp/registry/tests/test_sharingjob.py
@@ -1,4 +1,4 @@
-# Copyright 2012-2015 Canonical Ltd. This software is licensed under the
+# Copyright 2012-2021 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Tests for SharingJobs."""
@@ -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,
@@ -360,8 +360,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 b0d9f92..76c2e5b 100644
--- a/lib/lp/snappy/model/snap.py
+++ b/lib/lp/snappy/model/snap.py
@@ -122,7 +122,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.series import ACTIVE_STATUSES
@@ -1159,7 +1159,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
Follow ups