launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:snap-build-macaroon-snap-base into launchpad:master with ~cjwatson/launchpad:macaroon-verifies-matcher as a prerequisite.

Commit message:
Allow snap-build macaroons to authorize SnapBase archive dependencies

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/401316

Technically adding SnapBase archive dependencies on private archives won't quite work yet because nothing issues the relevant macaroons yet, but allowing it at this stage makes it much easier to write tests, and we're almost there anyway.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:snap-build-macaroon-snap-base into launchpad:master.

diff --git a/lib/lp/snappy/model/snapbase.py b/lib/lp/snappy/model/snapbase.py
index 615c783..8916c61 100644
--- a/lib/lp/snappy/model/snapbase.py
+++ b/lib/lp/snappy/model/snapbase.py
@@ -105,11 +105,6 @@ class SnapBase(Storm):
         if archive_dependency is not None:
             raise ArchiveDependencyError(
                 "This dependency is already registered.")
-        # XXX cjwatson 2021-03-19: Relax this once we have a way to dispatch
-        # appropriate tokens for snap builds whose base has dependencies on
-        # private archives.
-        if dependency.private:
-            raise ArchiveDependencyError("This dependency is private.")
         if not dependency.enabled:
             raise ArchiveDependencyError("Dependencies must not be disabled.")
 
diff --git a/lib/lp/snappy/model/snapbuild.py b/lib/lp/snappy/model/snapbuild.py
index b742fae..76ccc0e 100644
--- a/lib/lp/snappy/model/snapbuild.py
+++ b/lib/lp/snappy/model/snapbuild.py
@@ -93,6 +93,7 @@ from lp.snappy.interfaces.snapbuild import (
     )
 from lp.snappy.interfaces.snapbuildjob import ISnapStoreUploadJobSource
 from lp.snappy.mail.snapbuild import SnapBuildMailer
+from lp.snappy.model.snapbase import SnapBase
 from lp.snappy.model.snapbuildjob import (
     SnapBuildJob,
     SnapBuildJobType,
@@ -656,9 +657,9 @@ class SnapBuildMacaroonIssuer(MacaroonIssuerBase):
         """
         if not ISnapBuild.providedBy(context):
             raise BadMacaroonContext(context)
-        if not removeSecurityProxy(context).is_private:
-            raise BadMacaroonContext(
-                context, "Refusing to issue macaroon for public build.")
+        # We allow issuing macaroons for public builds.  It's harmless, and
+        # it allows using SnapBases that have archive dependencies on
+        # private PPAs.
         return removeSecurityProxy(context).id
 
     def checkVerificationContext(self, context, **kwargs):
@@ -712,6 +713,11 @@ class SnapBuildMacaroonIssuer(MacaroonIssuerBase):
                         Archive.id,
                         where=And(
                             ArchiveDependency.archive == Archive.id,
+                            ArchiveDependency.dependency == context))),
+                    SnapBuild.snap_base_id.is_in(Select(
+                        SnapBase.id,
+                        where=And(
+                            ArchiveDependency.snap_base == SnapBase.id,
                             ArchiveDependency.dependency == context)))))
         else:
             return False
diff --git a/lib/lp/snappy/tests/test_snapbuild.py b/lib/lp/snappy/tests/test_snapbuild.py
index 8887249..521cfca 100644
--- a/lib/lp/snappy/tests/test_snapbuild.py
+++ b/lib/lp/snappy/tests/test_snapbuild.py
@@ -902,13 +902,6 @@ class TestSnapBuildMacaroonIssuer(MacaroonTestMixin, TestCaseWithFactory):
         self.pushConfig(
             "launchpad", internal_macaroon_secret_key="some-secret")
 
-    def test_issueMacaroon_refuses_public_snap(self):
-        build = self.factory.makeSnapBuild()
-        issuer = getUtility(IMacaroonIssuer, "snap-build")
-        self.assertRaises(
-            BadMacaroonContext, removeSecurityProxy(issuer).issueMacaroon,
-            build)
-
     def test_issueMacaroon_good(self):
         build = self.factory.makeSnapBuild(
             snap=self.factory.makeSnap(private=True))
@@ -972,6 +965,18 @@ class TestSnapBuildMacaroonIssuer(MacaroonTestMixin, TestCaseWithFactory):
         macaroon = issuer.issueMacaroon(build)
         self.assertMacaroonVerifies(issuer, macaroon, dependency)
 
+    def test_verifyMacaroon_good_snap_base_archive(self):
+        snap_base = self.factory.makeSnapBase()
+        dependency = self.factory.makeArchive(private=True)
+        snap_base.addArchiveDependency(
+            dependency, PackagePublishingPocket.RELEASE)
+        build = self.factory.makeSnapBuild(snap_base=snap_base)
+        build.updateStatus(BuildStatus.BUILDING)
+        issuer = removeSecurityProxy(
+            getUtility(IMacaroonIssuer, "snap-build"))
+        macaroon = issuer.issueMacaroon(build)
+        self.assertMacaroonVerifies(issuer, macaroon, dependency)
+
     def test_verifyMacaroon_good_no_context(self):
         [ref] = self.factory.makeGitRefs(
             information_type=InformationType.USERDATA)