Merge lp:~cjwatson/launchpad/snap-build-record-code into lp:launchpad

[Colin Watson]

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

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.

https://code.launchpad.net/~cjwatson/launchpad/db-snap-build-record-code/+merge/365355 is the corresponding database patch.

[Colin Watson (cjwatson)]

There are a couple of known problems with this, discussed on today's LP team call:

(1) When the snap build is detached, it will no longer have a private code artifact attached to it and thus may become public. Oops.
(2) It's not obvious that these are quite the semantics we want. Unlike source packages, Git repositories include history, but the history can be mutated (e.g. via git filter-branch), and the process of making a private repository public might well include redacting its history. If old snap builds automatically become public then that could be a problem.

We may need a private flag on the build, but it probably can't just be that because we need some way of knowing who can see it. Perhaps we could detach from public builds (thus keeping logs for old builds that are on public Ubuntu images, etc.) but delete private builds. Perhaps only the snap owner could see old detached private builds, or maybe even only admins. Or something else ...

[lp:~cjwatson/launchpad/snap-build-record-code]

18920. By Colin Watson on 2019-05-28

Add an explicitly_private flag to SnapBuild.

This lets us avoid accidental information leaks.

18921. By Colin Watson on 2019-05-28

Check privacy of code objects associated with snap builds.

18922. By Colin Watson on 2019-05-28

Merge devel.

18923. By Colin Watson on 2019-05-28

Add warning for future developers.

[Colin Watson (cjwatson)]

I've tightened up the privacy rules here as best I can. How's this?

[Colin Watson (cjwatson)]

Superseded by Thiago's work on project-based snap recipes with privacy controlled by the pillar.

Unmerged revisions

18923. By Colin Watson on 2019-05-28

Add warning for future developers.

18922. By Colin Watson on 2019-05-28

Merge devel.

18921. By Colin Watson on 2019-05-28

Check privacy of code objects associated with snap builds.

18920. By Colin Watson on 2019-05-28

Add an explicitly_private flag to SnapBuild.

This lets us avoid accidental information leaks.

18919. By Colin Watson on 2019-04-01

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

=== modified file 'lib/lp/code/model/branch.py'
--- lib/lp/code/model/branch.py	2019-04-17 10:34:13 +0000
+++ lib/lp/code/model/branch.py	2019-05-28 16:37:20 +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)
@@ -923,10 +924,9 @@
             DeletionCallable(
                 recipe, _('This recipe uses this branch.'), recipe.destroySelf)
             for recipe in 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, eager_load=False):
@@ -1644,6 +1644,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-05-03 13:18:52 +0000
+++ lib/lp/code/model/gitrepository.py	2019-05-28 16:37:20 +0000
@@ -202,6 +202,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 = {
@@ -1476,9 +1477,7 @@
                 recipe.destroySelf)
             for recipe in 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)
 
@@ -1630,6 +1629,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-05-28 16:37:20 +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-05-03 13:18:52 +0000
+++ lib/lp/code/model/tests/test_gitrepository.py	2019-05-28 16:37:20 +0000
@@ -989,7 +989,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."))},
@@ -1001,7 +1001,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/security.py'
--- lib/lp/security.py	2019-03-26 20:51:38 +0000
+++ lib/lp/security.py	2019-05-28 16:37:20 +0000
@@ -3363,13 +3363,34 @@
             obj, obj.snap, 'launchpad.View')
 
 
-class ViewSnapBuild(DelegatedAuthorization):
+class ViewSnapBuild(AuthorizationBase):
     permission = 'launchpad.View'
     usedfor = ISnapBuild
 
-    def iter_objects(self):
-        yield self.obj.snap
-        yield self.obj.archive
+    def checkUnauthenticated(self):
+        return not self.obj.is_private
+
+    def checkAuthenticated(self, user):
+        if not self.obj.is_private:
+            return True
+
+        for item in (
+                self.obj.snap, self.obj.archive,
+                self.obj.branch, self.obj.git_repository):
+            if (item is not None and
+                    not self.forwardCheckAuthenticated(user, item)):
+                return False
+
+        # Only the snap owner and admins can see explicitly-private builds,
+        # on top of any restrictions associated with the individual
+        # components above.  This may be overly-restrictive but is at least
+        # relatively safe; if need be we can investigate loosening this
+        # later, but be careful of the case where private code objects have
+        # been detached.
+        if self.obj.explicitly_private:
+            return EditSnap(self.obj.snap).checkAuthenticated(user)
+        else:
+            return True
 
 
 class EditSnapBuild(AdminByBuilddAdmin):
 
=== 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-05-28 16:37:20 +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
@@ -168,6 +170,14 @@
             "supported."),
         key_type=TextLine()))
 
+    explicitly_private = Bool(
+        title=_("Whether this build is explicitly private."),
+        description=_(
+            "An explicitly-private build remains private even if its snap and "
+            "other associated resources are public.  This allows snaps to "
+            "transition between private and public states without "
+            "inadvertently leaking old private information."))
+
     virtualized = Bool(
         title=_("If True, this build is virtualized."), readonly=True)
 
@@ -248,6 +258,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 +367,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/snap.py'
--- lib/lp/snappy/model/snap.py	2019-05-16 10:21:14 +0000
+++ lib/lp/snappy/model/snap.py	2019-05-28 16:37:20 +0000
@@ -650,7 +650,8 @@
 
         build = getUtility(ISnapBuildSet).new(
             requester, self, archive, distro_arch_series, pocket,
-            channels=channels, build_request=build_request)
+            channels=channels, build_request=build_request,
+            explicitly_private=self.private)
         build.queueBuild()
         notify(ObjectCreatedEvent(build, user=requester))
         return build
 
=== modified file 'lib/lp/snappy/model/snapbuild.py'
--- lib/lp/snappy/model/snapbuild.py	2019-05-09 15:44:59 +0000
+++ lib/lp/snappy/model/snapbuild.py	2019-05-28 16:37:20 +0000
@@ -154,6 +154,8 @@
 
     channels = JSON('channels', allow_none=True)
 
+    explicitly_private = Bool(name='explicitly_private')
+
     processor_id = Int(name='processor', allow_none=False)
     processor = Reference(processor_id, 'Processor.id')
     virtualized = Bool(name='virtualized')
@@ -184,9 +186,20 @@
 
     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):
+                 date_created, store_upload_metadata=None, build_request=None,
+                 explicitly_private=False):
         """Construct a `SnapBuild`."""
         super(SnapBuild, self).__init__()
         self.build_farm_job = build_farm_job
@@ -202,7 +215,14 @@
         self.store_upload_metadata = store_upload_metadata
         if build_request is not None:
             self.build_request_id = build_request.id
+        self.explicitly_private = explicitly_private
         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):
@@ -214,10 +234,15 @@
     def is_private(self):
         """See `IBuildFarmJob`."""
         return (
+            self.explicitly_private or
             self.snap.private or
             self.snap.owner.private or
-            self.archive.private
-        )
+            # We must check individual components here, since the Snap's
+            # configuration may have changed since this build was created.
+            self.archive.private or
+            (self.branch is not None and self.branch.private) or
+            (self.git_repository is not None and self.git_repository.private)
+            )
 
     @property
     def title(self):
@@ -518,7 +543,8 @@
 
     def new(self, requester, snap, archive, distro_arch_series, pocket,
             channels=None, date_created=DEFAULT,
-            store_upload_metadata=None, build_request=None):
+            store_upload_metadata=None, build_request=None,
+            explicitly_private=False):
         """See `ISnapBuildSet`."""
         store = IMasterStore(SnapBuild)
         build_farm_job = getUtility(IBuildFarmJobSource).new(
@@ -530,7 +556,7 @@
             not distro_arch_series.processor.supports_nonvirtualized
             or snap.require_virtualized or archive.require_virtualized,
             date_created, store_upload_metadata=store_upload_metadata,
-            build_request=build_request)
+            build_request=build_request, explicitly_private=explicitly_private)
         store.add(snapbuild)
         return snapbuild
 
@@ -592,6 +618,24 @@
                 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)
+
 
 @implementer(IMacaroonIssuer)
 class SnapBuildMacaroonIssuer(MacaroonIssuerBase):
 
=== modified file 'lib/lp/snappy/model/snapbuildbehaviour.py'
--- lib/lp/snappy/model/snapbuildbehaviour.py	2019-05-22 16:54:09 +0000
+++ lib/lp/snappy/model/snapbuildbehaviour.py	2019-05-28 16:37:20 +0000
@@ -139,6 +139,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	2019-05-09 15:44:59 +0000
+++ lib/lp/snappy/tests/test_snapbuild.py	2019-05-28 16:37:20 +0000
@@ -60,6 +60,7 @@
 from lp.snappy.interfaces.snapbuildjob import ISnapStoreUploadJobSource
 from lp.soyuz.enums import ArchivePurpose
 from lp.testing import (
+    admin_logged_in,
     ANONYMOUS,
     api_url,
     login,
@@ -158,20 +159,91 @@
         build = self.factory.makeSnapBuild(archive=self.factory.makeArchive())
         self.assertEqual("main", build.current_component.name)
 
-    def test_is_private(self):
-        # A SnapBuild is private iff its Snap and archive are.
+    def test_public(self):
+        # A SnapBuild is public if it has no associated private resources.
         self.assertFalse(self.build.is_private)
+
+    def test_explicitly_private(self):
+        # A SnapBuild is private if it is marked explicitly private due to
+        # its Snap being private.  It remains private even if the Snap is
+        # later made public, in order to avoid accidental information leaks.
+        with person_logged_in(self.factory.makePerson()) as owner:
+            build = self.factory.makeSnapBuild(
+                requester=owner, owner=owner, private=True)
+            self.assertTrue(build.explicitly_private)
+            self.assertTrue(build.is_private)
+        with admin_logged_in():
+            build.snap.private = False
+            self.assertTrue(build.is_private)
+
+    def test_private_if_owner_is_private(self):
+        # A SnapBuild is private if its owner is private.
         private_team = self.factory.makeTeam(
             visibility=PersonVisibility.PRIVATE)
         with person_logged_in(private_team.teamowner):
             build = self.factory.makeSnapBuild(
                 requester=private_team.teamowner, owner=private_team,
                 private=True)
+            self.assertTrue(build.explicitly_private)
             self.assertTrue(build.is_private)
+
+    def test_private_if_archive_is_private(self):
+        # A SnapBuild is private if its archive is private.
         private_archive = self.factory.makeArchive(private=True)
         with person_logged_in(private_archive.owner):
             build = self.factory.makeSnapBuild(archive=private_archive)
-            self.assertTrue(build.is_private)
+            self.assertFalse(build.explicitly_private)
+            self.assertTrue(build.is_private)
+
+    def test_private_if_branch_is_private(self):
+        # A SnapBuild is private if its Bazaar branch is private, even if
+        # the Snap is later reconfigured to use a public branch.
+        private_branch = self.factory.makeAnyBranch(
+            information_type=InformationType.USERDATA)
+        with person_logged_in(private_branch.owner):
+            build = self.factory.makeSnapBuild(
+                branch=private_branch, private=True)
+            self.assertTrue(build.explicitly_private)
+            self.assertTrue(build.is_private)
+            build.snap.branch = self.factory.makeAnyBranch()
+            self.assertTrue(build.is_private)
+
+    def test_private_if_git_repository_is_private(self):
+        # A SnapBuild is private if its Git repository is private, even if
+        # the Snap is later reconfigured to use a public repository.
+        [private_ref] = self.factory.makeGitRefs(
+            information_type=InformationType.USERDATA)
+        with person_logged_in(private_ref.owner):
+            build = self.factory.makeSnapBuild(
+                git_ref=private_ref, private=True)
+            self.assertTrue(build.explicitly_private)
+            self.assertTrue(build.is_private)
+            build.snap.git_ref = self.factory.makeGitRefs()[0]
+            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.
@@ -693,6 +765,22 @@
         # 404 since we aren't allowed to know that the private team exists.
         self.assertEqual(404, unpriv_webservice.get(build_url).status)
 
+    def test_private_snap_made_public(self):
+        # A SnapBuild with a Snap that was initially private but then made
+        # public is still private.
+        with person_logged_in(self.person):
+            db_build = self.factory.makeSnapBuild(
+                requester=self.person, owner=self.person, private=True)
+            build_url = api_url(db_build)
+        with admin_logged_in():
+            db_build.snap.private = False
+        unpriv_webservice = webservice_for_person(
+            self.factory.makePerson(), permission=OAuthPermission.WRITE_PUBLIC)
+        unpriv_webservice.default_api_version = "devel"
+        logout()
+        self.assertEqual(200, self.webservice.get(build_url).status)
+        self.assertEqual(401, unpriv_webservice.get(build_url).status)
+
     def test_private_archive(self):
         # A SnapBuild with a private archive is private.
         db_archive = self.factory.makeArchive(owner=self.person, private=True)
 
=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py	2019-05-06 14:22:34 +0000
+++ lib/lp/testing/factory.py	2019-05-28 16:37:20 +0000
@@ -4800,7 +4800,7 @@
         snapbuild = getUtility(ISnapBuildSet).new(
             requester, snap, archive, distroarchseries, pocket,
             channels=channels, date_created=date_created,
-            build_request=build_request)
+            build_request=build_request, explicitly_private=snap.private)
         if duration is not None:
             removeSecurityProxy(snapbuild).updateStatus(
                 BuildStatus.BUILDING, builder=builder,