launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/snap-build-record-code into lp:launchpad.

Commit message:
Record the code object from which a SnapBuild was created, for later use by security adapters.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1639975 in Launchpad itself: "support for building private snaps"
  https://bugs.launchpad.net/launchpad/+bug/1639975

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/snap-build-record-code/+merge/365356

This is annoying, but I don't see any other way to cope with privacy changes of code objects that have snaps attached to them without leaking information about old builds.

We shouldn't land this until https://code.launchpad.net/~cjwatson/launchpad/branch-delete-job/+merge/364907 and https://code.launchpad.net/~cjwatson/launchpad/git-repository-delete-job/+merge/364910 have landed, as it'll make deletions slower; and, assuming all of these are approved, we'll need to add SELECT and DELETE permissions for the snap-build-job user on public.snapbuild before landing this.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/snap-build-record-code into lp:launchpad.

=== modified file 'lib/lp/code/model/branch.py'
--- lib/lp/code/model/branch.py	2019-03-29 17:06:15 +0000
+++ lib/lp/code/model/branch.py	2019-04-01 15:03:40 +0000
@@ -196,6 +196,7 @@
 from lp.services.webhooks.interfaces import IWebhookSet
 from lp.services.webhooks.model import WebhookTargetMixin
 from lp.snappy.interfaces.snap import ISnapSet
+from lp.snappy.interfaces.snapbuild import ISnapBuildSet
 
 
 @implementer(IBranch, IPrivacy, IInformationType)
@@ -922,10 +923,9 @@
             DeletionCallable(
                 recipe, _('This recipe uses this branch.'), recipe.destroySelf)
             for recipe in self.recipes)
-        if not getUtility(ISnapSet).findByBranch(self).is_empty():
-            alteration_operations.append(DeletionCallable(
-                None, _('Some snap packages build from this branch.'),
-                getUtility(ISnapSet).detachFromBranch, self))
+        if (not getUtility(ISnapSet).findByBranch(self).is_empty() or
+                not getUtility(ISnapBuildSet).findByBranch(self).is_empty()):
+            alteration_operations.append(DetachSnaps(self))
         return (alteration_operations, deletion_operations)
 
     def deletionRequirements(self):
@@ -1639,6 +1639,19 @@
         getUtility(ICodeImportSet).delete(self.affected_object)
 
 
+class DetachSnaps(DeletionOperation):
+    """Deletion operation that detaches snaps from a branch."""
+
+    def __init__(self, branch):
+        super(DetachSnaps, self).__init__(
+            None, _('Some snap packages build from this branch.'))
+        self.branch = branch
+
+    def __call__(self):
+        getUtility(ISnapSet).detachFromBranch(self.branch)
+        getUtility(ISnapBuildSet).detachFromBranch(self.branch)
+
+
 @implementer(IBranchSet)
 class BranchSet:
     """The set of all branches."""

=== modified file 'lib/lp/code/model/gitrepository.py'
--- lib/lp/code/model/gitrepository.py	2019-03-29 17:06:15 +0000
+++ lib/lp/code/model/gitrepository.py	2019-04-01 15:03:40 +0000
@@ -194,6 +194,7 @@
 from lp.services.webhooks.interfaces import IWebhookSet
 from lp.services.webhooks.model import WebhookTargetMixin
 from lp.snappy.interfaces.snap import ISnapSet
+from lp.snappy.interfaces.snapbuild import ISnapBuildSet
 
 
 object_type_map = {
@@ -1467,9 +1468,7 @@
                 recipe.destroySelf)
             for recipe in self.recipes)
         if not getUtility(ISnapSet).findByGitRepository(self).is_empty():
-            alteration_operations.append(DeletionCallable(
-                None, msg("Some snap packages build from this repository."),
-                getUtility(ISnapSet).detachFromGitRepository, self))
+            alteration_operations.append(DetachSnaps(self))
 
         return (alteration_operations, deletion_operations)
 
@@ -1621,6 +1620,19 @@
         getUtility(ICodeImportSet).delete(self.affected_object)
 
 
+class DetachSnaps(DeletionOperation):
+    """Deletion operation that detaches snaps from a repository."""
+
+    def __init__(self, repository):
+        super(DetachSnaps, self).__init__(
+            None, msg("Some snap packages build from this repository."))
+        self.repository = repository
+
+    def __call__(self):
+        getUtility(ISnapSet).detachFromGitRepository(self.repository)
+        getUtility(ISnapBuildSet).detachFromGitRepository(self.repository)
+
+
 @implementer(IGitRepositorySet)
 class GitRepositorySet:
     """See `IGitRepositorySet`."""

=== modified file 'lib/lp/code/model/tests/test_branch.py'
--- lib/lp/code/model/tests/test_branch.py	2019-01-28 18:09:21 +0000
+++ lib/lp/code/model/tests/test_branch.py	2019-04-01 15:03:40 +0000
@@ -1697,7 +1697,7 @@
     def test_snap_requirements(self):
         # If a branch is used by a snap package, the deletion requirements
         # indicate this.
-        self.factory.makeSnap(branch=self.branch)
+        self.factory.makeSnapBuild(branch=self.branch)
         self.assertEqual(
             {None: ('alter', _('Some snap packages build from this branch.'))},
             self.branch.deletionRequirements())
@@ -1705,7 +1705,9 @@
     def test_snap_deletion(self):
         # break_references allows deleting a branch used by a snap package.
         snap1 = self.factory.makeSnap(branch=self.branch)
+        self.factory.makeSnapBuild(snap=snap1)
         snap2 = self.factory.makeSnap(branch=self.branch)
+        self.factory.makeSnapBuild(snap=snap2)
         self.branch.destroySelf(break_references=True)
         transaction.commit()
         self.assertIsNone(snap1.branch)

=== modified file 'lib/lp/code/model/tests/test_gitrepository.py'
--- lib/lp/code/model/tests/test_gitrepository.py	2019-02-11 12:31:06 +0000
+++ lib/lp/code/model/tests/test_gitrepository.py	2019-04-01 15:03:40 +0000
@@ -975,7 +975,7 @@
         # If a repository is used by a snap package, the deletion
         # requirements indicate this.
         [ref] = self.factory.makeGitRefs()
-        self.factory.makeSnap(git_ref=ref)
+        self.factory.makeSnapBuild(git_ref=ref)
         self.assertEqual(
             {None:
              ("alter", _("Some snap packages build from this repository."))},
@@ -987,7 +987,9 @@
         [ref1, ref2] = self.factory.makeGitRefs(
             repository=repository, paths=["refs/heads/1", "refs/heads/2"])
         snap1 = self.factory.makeSnap(git_ref=ref1)
+        self.factory.makeSnapBuild(snap=snap1)
         snap2 = self.factory.makeSnap(git_ref=ref2)
+        self.factory.makeSnapBuild(snap=snap2)
         repository.destroySelf(break_references=True)
         transaction.commit()
         self.assertIsNone(snap1.git_repository)

=== modified file 'lib/lp/snappy/interfaces/snapbuild.py'
--- lib/lp/snappy/interfaces/snapbuild.py	2018-12-12 10:31:40 +0000
+++ lib/lp/snappy/interfaces/snapbuild.py	2019-04-01 15:03:40 +0000
@@ -51,6 +51,8 @@
 from lp import _
 from lp.buildmaster.interfaces.buildfarmjob import ISpecificBuildFarmJobSource
 from lp.buildmaster.interfaces.packagebuild import IPackageBuild
+from lp.code.interfaces.branch import IBranch
+from lp.code.interfaces.gitrepository import IGitRepository
 from lp.registry.interfaces.person import IPerson
 from lp.registry.interfaces.pocket import PackagePublishingPocket
 from lp.services.database.constants import DEFAULT
@@ -248,6 +250,20 @@
     store_upload_metadata = Attribute(
         _("A dict of data about store upload progress."))
 
+    branch = Reference(
+        IBranch,
+        title=_("Bazaar branch"), required=False, readonly=True)
+
+    git_repository = Reference(
+        IGitRepository,
+        title=_("Git repository"), required=False, readonly=True)
+
+    git_repository_url = TextLine(
+        title=_("Git repository URL"), required=False, readonly=True)
+
+    git_path = TextLine(
+        title=_("Git branch path"), required=False, readonly=True)
+
     def getFiles():
         """Retrieve the build's `ISnapFile` records.
 
@@ -343,3 +359,15 @@
 
     def preloadBuildsData(builds):
         """Load the data related to a list of snap builds."""
+
+    def findByBranch(branch):
+        """Return all snap builds for the given Bazaar branch."""
+
+    def findByGitRepository(repository, paths=None):
+        """Return all snap builds for the given Git repository."""
+
+    def detachFromBranch(branch):
+        """Detach all snap builds from the given Bazaar branch."""
+
+    def detachFromGitRepository(repository):
+        """Detach all snap builds from the given Git repository."""

=== modified file 'lib/lp/snappy/model/snapbuild.py'
--- lib/lp/snappy/model/snapbuild.py	2018-12-18 18:14:37 +0000
+++ lib/lp/snappy/model/snapbuild.py	2019-04-01 15:03:40 +0000
@@ -177,6 +177,16 @@
 
     store_upload_metadata = JSON('store_upload_json_data', allow_none=True)
 
+    branch_id = Int(name='branch', allow_none=True)
+    branch = Reference(branch_id, 'Branch.id')
+
+    git_repository_id = Int(name='git_repository', allow_none=True)
+    git_repository = Reference(git_repository_id, 'GitRepository.id')
+
+    git_repository_url = Unicode(name='git_repository_url', allow_none=True)
+
+    git_path = Unicode(name='git_path', allow_none=True)
+
     def __init__(self, build_farm_job, requester, snap, archive,
                  distro_arch_series, pocket, channels, processor, virtualized,
                  date_created, store_upload_metadata=None, build_request=None):
@@ -196,6 +206,12 @@
         if build_request is not None:
             self.build_request_id = build_request.id
         self.status = BuildStatus.NEEDSBUILD
+        # Copy branch/repository information from the snap, for future
+        # privacy checks on this build.
+        self.branch = snap.branch
+        self.git_repository = snap.git_repository
+        self.git_repository_url = snap.git_repository_url
+        self.git_path = snap.git_path
 
     @property
     def build_request(self):
@@ -584,3 +600,21 @@
             SnapBuild, SnapBuild.build_farm_job_id.is_in(
                 bfj.id for bfj in build_farm_jobs))
         return DecoratedResultSet(rows, pre_iter_hook=self.preloadBuildsData)
+
+    def findByBranch(self, branch):
+        """See `ISnapBuildSet`."""
+        return IStore(SnapBuild).find(SnapBuild, SnapBuild.branch == branch)
+
+    def findByGitRepository(self, repository):
+        """See `ISnapBuildSet`."""
+        return IStore(SnapBuild).find(
+            SnapBuild, SnapBuild.git_repository == repository)
+
+    def detachFromBranch(self, branch):
+        """See `ISnapBuildSet`."""
+        self.findByBranch(branch).set(branch_id=None)
+
+    def detachFromGitRepository(self, repository):
+        """See `ISnapBuildSet`."""
+        self.findByGitRepository(repository).set(
+            git_repository_id=None, git_path=None)

=== modified file 'lib/lp/snappy/model/snapbuildbehaviour.py'
--- lib/lp/snappy/model/snapbuildbehaviour.py	2019-03-06 23:00:49 +0000
+++ lib/lp/snappy/model/snapbuildbehaviour.py	2019-04-01 15:03:40 +0000
@@ -123,6 +123,8 @@
             # dict, since otherwise we'll be unable to serialise it to
             # XML-RPC.
             args["channels"] = removeSecurityProxy(channels)
+        # XXX cjwatson 2019-04-01: Once new SnapBuilds are being created on
+        # production with code object columns, we should use them here.
         if build.snap.branch is not None:
             args["branch"] = build.snap.branch.bzr_identity
         elif build.snap.git_ref is not None:

=== modified file 'lib/lp/snappy/tests/test_snapbuild.py'
--- lib/lp/snappy/tests/test_snapbuild.py	2018-12-18 18:23:52 +0000
+++ lib/lp/snappy/tests/test_snapbuild.py	2019-04-01 15:03:40 +0000
@@ -163,6 +163,30 @@
             build = self.factory.makeSnapBuild(archive=private_archive)
             self.assertTrue(build.is_private)
 
+    def test_copies_code_information(self):
+        # Creating a SnapBuild copies code information from its parent Snap.
+        for args in (
+                {"branch": self.factory.makeAnyBranch()},
+                {"git_ref": self.factory.makeGitRefs()[0]},
+                {"git_ref": self.factory.makeGitRefRemote()},
+                ):
+            snap = self.factory.makeSnap(**args)
+            build = self.factory.makeSnapBuild(snap=snap)
+            snap.branch = self.factory.makeAnyBranch()
+            snap.git_ref = None
+            if "branch" in args:
+                self.assertThat(build, MatchesStructure(
+                    branch=Equals(args["branch"]),
+                    git_repository=Is(None),
+                    git_repository_url=Is(None),
+                    git_path=Is(None)))
+            else:
+                self.assertThat(build, MatchesStructure(
+                    branch=Is(None),
+                    git_repository=Equals(args["git_ref"].repository),
+                    git_repository_url=Equals(args["git_ref"].repository_url),
+                    git_path=Equals(args["git_ref"].path)))
+
     def test_can_be_cancelled(self):
         # For all states that can be cancelled, can_be_cancelled returns True.
         ok_cases = [