Merge lp:~cjwatson/launchpad/refactor-macaroon-testing into lp:launchpad

[Colin Watson]

Add logging to macaroon verification, and refactor tests to check it.

It was all too easy for macaroon verification to fail for some reason other than the one that a test expected, and tests had no way to tell; indeed, I ran into and fixed exactly such a case while working on this branch. We now log details of verification failures, and I added a couple of test helpers to make it easy for tests to check these consistently.

[William Grant (wgrant)]

I wonder if it's better to refactor this to return a list of violations rather than directly logging them and returning False. Some callsites can then log without context if we really want, but the tests can do it less evilly.

review: Approve (code)

[Colin Watson (cjwatson)]

I thought actually returning a list of violations was a bit too confusing (because it would mean inverting the sense of boolean tests of verifyMacaroon), but passing a list as an optional argument which verifyMacaroon appends to seems not too horrible and works fine, so I've done that.

=== modified file 'lib/lp/code/model/codeimportjob.py'
--- lib/lp/code/model/codeimportjob.py	2019-04-24 12:50:20 +0000
+++ lib/lp/code/model/codeimportjob.py	2019-04-25 22:19:14 +0000
@@ -434,9 +434,11 @@
 
     def checkVerificationContext(self, context):
         """See `MacaroonIssuerBase`."""
-        if (not ICodeImportJob.providedBy(context) or
-                context.state != CodeImportJobState.RUNNING):
+        if not ICodeImportJob.providedBy(context):
             raise BadMacaroonContext(context)
+        if context.state != CodeImportJobState.RUNNING:
+            raise BadMacaroonContext(
+                context, "%r is not in the RUNNING state." % context)
         return context
 
     def verifyPrimaryCaveat(self, caveat_value, context):

=== modified file 'lib/lp/code/model/tests/test_codeimportjob.py'
--- lib/lp/code/model/tests/test_codeimportjob.py	2019-04-25 09:34:40 +0000
+++ lib/lp/code/model/tests/test_codeimportjob.py	2019-04-25 22:19:14 +0000
@@ -65,6 +65,7 @@
     BadMacaroonContext,
     IMacaroonIssuer,
     )
+from lp.services.macaroons.testing import MacaroonTestMixin
 from lp.services.webapp import canonical_url
 from lp.testing import (
     ANONYMOUS,
@@ -1271,7 +1272,7 @@
             get_feedback_messages(user_browser.contents))
 
 
-class TestCodeImportJobMacaroonIssuer(TestCaseWithFactory):
+class TestCodeImportJobMacaroonIssuer(MacaroonTestMixin, TestCaseWithFactory):
     """Test CodeImportJob macaroon issuing and verification."""
 
     layer = DatabaseFunctionalLayer
@@ -1324,7 +1325,7 @@
         issuer = getUtility(IMacaroonIssuer, "code-import-job")
         getUtility(ICodeImportJobWorkflow).startJob(job, machine)
         macaroon = removeSecurityProxy(issuer).issueMacaroon(job)
-        self.assertTrue(issuer.verifyMacaroon(macaroon, job))
+        self.assertMacaroonVerifies(issuer, macaroon, job)
 
     def test_verifyMacaroon_good_no_context(self):
         machine = self.factory.makeCodeImportMachine(set_online=True)
@@ -1332,8 +1333,8 @@
         issuer = getUtility(IMacaroonIssuer, "code-import-job")
         getUtility(ICodeImportJobWorkflow).startJob(job, machine)
         macaroon = removeSecurityProxy(issuer).issueMacaroon(job)
-        self.assertTrue(
-            issuer.verifyMacaroon(macaroon, None, require_context=False))
+        self.assertMacaroonVerifies(
+            issuer, macaroon, job, require_context=False)
 
     def test_verifyMacaroon_no_context_but_require_context(self):
         machine = self.factory.makeCodeImportMachine(set_online=True)
@@ -1341,7 +1342,9 @@
         issuer = getUtility(IMacaroonIssuer, "code-import-job")
         getUtility(ICodeImportJobWorkflow).startJob(job, machine)
         macaroon = removeSecurityProxy(issuer).issueMacaroon(job)
-        self.assertFalse(issuer.verifyMacaroon(macaroon, None))
+        self.assertMacaroonDoesNotVerify(
+            ["Expected macaroon verification context but got None."],
+            issuer, macaroon, None)
 
     def test_verifyMacaroon_wrong_location(self):
         machine = self.factory.makeCodeImportMachine(set_online=True)
@@ -1351,9 +1354,12 @@
         macaroon = Macaroon(
             location="another-location",
             key=removeSecurityProxy(issuer)._root_secret)
-        self.assertFalse(issuer.verifyMacaroon(macaroon, job))
-        self.assertFalse(
-            issuer.verifyMacaroon(macaroon, None, require_context=False))
+        self.assertMacaroonDoesNotVerify(
+            ["Macaroon has unknown location 'another-location'."],
+            issuer, macaroon, job)
+        self.assertMacaroonDoesNotVerify(
+            ["Macaroon has unknown location 'another-location'."],
+            issuer, macaroon, job, require_context=False)
 
     def test_verifyMacaroon_wrong_key(self):
         machine = self.factory.makeCodeImportMachine(set_online=True)
@@ -1362,19 +1368,28 @@
         getUtility(ICodeImportJobWorkflow).startJob(job, machine)
         macaroon = Macaroon(
             location=config.vhost.mainsite.hostname, key="another-secret")
-        self.assertFalse(issuer.verifyMacaroon(macaroon, job))
-        self.assertFalse(
-            issuer.verifyMacaroon(macaroon, None, require_context=False))
+        self.assertMacaroonDoesNotVerify(
+            ["Signatures do not match"], issuer, macaroon, job)
+        self.assertMacaroonDoesNotVerify(
+            ["Signatures do not match"],
+            issuer, macaroon, job, require_context=False)
 
     def test_verifyMacaroon_not_running(self):
         job = self.makeJob()
         issuer = getUtility(IMacaroonIssuer, "code-import-job")
         macaroon = removeSecurityProxy(issuer).issueMacaroon(job)
-        self.assertFalse(issuer.verifyMacaroon(macaroon, job))
+        self.assertMacaroonDoesNotVerify(
+            ["%r is not in the RUNNING state." % job],
+            issuer, macaroon, job)
 
     def test_verifyMacaroon_wrong_job(self):
+        machine = self.factory.makeCodeImportMachine(set_online=True)
         job = self.makeJob()
-        other_job = self.factory.makeCodeImportJob(code_import=job.code_import)
+        other_job = self.makeJob()
         issuer = getUtility(IMacaroonIssuer, "code-import-job")
+        getUtility(ICodeImportJobWorkflow).startJob(job, machine)
         macaroon = removeSecurityProxy(issuer).issueMacaroon(other_job)
-        self.assertFalse(issuer.verifyMacaroon(macaroon, job))
+        self.assertMacaroonDoesNotVerify(
+            ["Caveat check for 'lp.code-import-job %s' failed." %
+             other_job.id],
+            issuer, macaroon, job)

=== modified file 'lib/lp/services/macaroons/interfaces.py'
--- lib/lp/services/macaroons/interfaces.py	2019-04-25 09:55:35 +0000
+++ lib/lp/services/macaroons/interfaces.py	2019-04-25 22:19:14 +0000
@@ -31,7 +31,7 @@
     issuable_via_authserver = Bool(
         "Does this issuer allow issuing macaroons via the authserver?")
 
-    def verifyMacaroon(macaroon, context, require_context=True):
+    def verifyMacaroon(macaroon, context, require_context=True, errors=None):
         """Verify that `macaroon` is valid for `context`.
 
         :param macaroon: A `Macaroon`.
@@ -41,6 +41,8 @@
             verify that the macaroon could be valid for some context.  Use
             this in the authentication part of an
             authentication/authorisation API.
+        :param errors: If non-None, any verification error messages will be
+            appended to this list.
         :return: True if `macaroon` is valid for `context`, otherwise False.
         """
 

=== modified file 'lib/lp/services/macaroons/model.py'
--- lib/lp/services/macaroons/model.py	2019-04-24 16:46:31 +0000
+++ lib/lp/services/macaroons/model.py	2019-04-25 22:19:14 +0000
@@ -95,22 +95,33 @@
         """
         raise NotImplementedError
 
-    def verifyMacaroon(self, macaroon, context, require_context=True):
+    def verifyMacaroon(self, macaroon, context, require_context=True,
+                       errors=None):
         """See `IMacaroonIssuer`."""
         if macaroon.location != config.vhost.mainsite.hostname:
+            if errors is not None:
+                errors.append(
+                    "Macaroon has unknown location '%s'." % macaroon.location)
             return False
         if require_context and context is None:
+            if errors is not None:
+                errors.append(
+                    "Expected macaroon verification context but got None.")
             return False
         if context is not None:
             try:
                 context = self.checkVerificationContext(context)
-            except BadMacaroonContext:
+            except BadMacaroonContext as e:
+                if errors is not None:
+                    errors.append(str(e))
                 return False
 
         def verify(caveat):
             try:
                 caveat_name, caveat_value = caveat.split(" ", 1)
             except ValueError:
+                if errors is not None:
+                    errors.append("Cannot parse caveat '%s'." % caveat)
                 return False
             if caveat_name == self._primary_caveat_name:
                 checker = self.verifyPrimaryCaveat
@@ -118,8 +129,14 @@
                 # XXX cjwatson 2019-04-09: For now we just fail closed if
                 # there are any other caveats, which is good enough for
                 # internal use.
-                return False
-            return checker(caveat_value, context)
+                if errors is not None:
+                    errors.append("Unhandled caveat name '%s'." % caveat_name)
+                return False
+            if not checker(caveat_value, context):
+                if errors is not None:
+                    errors.append("Caveat check for '%s' failed." % caveat)
+                return False
+            return True
 
         try:
             verifier = Verifier()
@@ -130,5 +147,7 @@
         # but most of them are too broad to reasonably catch so we let them
         # turn into OOPSes for now.  Revisit this once
         # https://github.com/ecordell/pymacaroons/issues/51 is fixed.
-        except MacaroonVerificationFailedException:
+        except MacaroonVerificationFailedException as e:
+            if errors is not None and not errors:
+                errors.append(str(e))
             return False

=== added file 'lib/lp/services/macaroons/testing.py'
--- lib/lp/services/macaroons/testing.py	1970-01-01 00:00:00 +0000
+++ lib/lp/services/macaroons/testing.py	2019-04-25 22:19:14 +0000
@@ -0,0 +1,40 @@
+# Copyright 2019 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Macaroon testing helpers."""
+
+from __future__ import absolute_import, print_function, unicode_literals
+
+__metaclass__ = type
+__all__ = [
+    'find_caveats_by_name',
+    'MacaroonTestMixin',
+    ]
+
+from testtools.content import text_content
+
+
+def find_caveats_by_name(macaroon, caveat_name):
+    return [
+        caveat for caveat in macaroon.caveats
+        if caveat.caveat_id.startswith(caveat_name + " ")]
+
+
+class MacaroonTestMixin:
+
+    def assertMacaroonVerifies(self, issuer, macaroon, context, **kwargs):
+        errors = []
+        try:
+            self.assertTrue(issuer.verifyMacaroon(
+                macaroon, context, errors=errors, **kwargs))
+        except Exception:
+            if errors:
+                self.addDetail("errors", text_content("\n".join(errors)))
+            raise
+
+    def assertMacaroonDoesNotVerify(self, expected_errors, issuer, macaroon,
+                                    context, **kwargs):
+        errors = []
+        self.assertFalse(issuer.verifyMacaroon(
+            macaroon, context, errors=errors, **kwargs))
+        self.assertEqual(expected_errors, errors)

=== modified file 'lib/lp/snappy/tests/test_snapbuild.py'
--- lib/lp/snappy/tests/test_snapbuild.py	2019-04-25 09:34:40 +0000
+++ lib/lp/snappy/tests/test_snapbuild.py	2019-04-25 22:19:14 +0000
@@ -46,6 +46,7 @@
     BadMacaroonContext,
     IMacaroonIssuer,
     )
+from lp.services.macaroons.testing import MacaroonTestMixin
 from lp.services.propertycache import clear_property_cache
 from lp.services.webapp.interfaces import OAuthPermission
 from lp.services.webapp.publisher import canonical_url
@@ -785,7 +786,7 @@
             self.assertCanOpenRedirectedUrl(browser, file_url)
 
 
-class TestSnapBuildMacaroonIssuer(TestCaseWithFactory):
+class TestSnapBuildMacaroonIssuer(MacaroonTestMixin, TestCaseWithFactory):
     """Test SnapBuild macaroon issuing and verification."""
 
     layer = LaunchpadZopelessLayer
@@ -840,7 +841,7 @@
         issuer = removeSecurityProxy(
             getUtility(IMacaroonIssuer, "snap-build"))
         macaroon = issuer.issueMacaroon(build)
-        self.assertTrue(issuer.verifyMacaroon(macaroon, ref.repository))
+        self.assertMacaroonVerifies(issuer, macaroon, ref.repository)
 
     def test_verifyMacaroon_wrong_location(self):
         [ref] = self.factory.makeGitRefs(
@@ -852,7 +853,9 @@
             getUtility(IMacaroonIssuer, "snap-build"))
         macaroon = Macaroon(
             location="another-location", key=issuer._root_secret)
-        self.assertFalse(issuer.verifyMacaroon(macaroon, ref.repository))
+        self.assertMacaroonDoesNotVerify(
+            ["Macaroon has unknown location 'another-location'."],
+            issuer, macaroon, ref.repository)
 
     def test_verifyMacaroon_wrong_key(self):
         [ref] = self.factory.makeGitRefs(
@@ -864,7 +867,8 @@
             getUtility(IMacaroonIssuer, "snap-build"))
         macaroon = Macaroon(
             location=config.vhost.mainsite.hostname, key="another-secret")
-        self.assertFalse(issuer.verifyMacaroon(macaroon, ref.repository))
+        self.assertMacaroonDoesNotVerify(
+            ["Signatures do not match"], issuer, macaroon, ref.repository)
 
     def test_verifyMacaroon_refuses_branch(self):
         branch = self.factory.makeAnyBranch(
@@ -875,7 +879,8 @@
         issuer = removeSecurityProxy(
             getUtility(IMacaroonIssuer, "snap-build"))
         macaroon = issuer.issueMacaroon(build)
-        self.assertFalse(issuer.verifyMacaroon(macaroon, branch))
+        self.assertMacaroonDoesNotVerify(
+            ["Cannot handle context %r." % branch], issuer, macaroon, branch)
 
     def test_verifyMacaroon_not_building(self):
         [ref] = self.factory.makeGitRefs(
@@ -885,7 +890,9 @@
         issuer = removeSecurityProxy(
             getUtility(IMacaroonIssuer, "snap-build"))
         macaroon = issuer.issueMacaroon(build)
-        self.assertFalse(issuer.verifyMacaroon(macaroon, ref.repository))
+        self.assertMacaroonDoesNotVerify(
+            ["Caveat check for 'lp.snap-build %s' failed." % build.id],
+            issuer, macaroon, ref.repository)
 
     def test_verifyMacaroon_wrong_build(self):
         [ref] = self.factory.makeGitRefs(
@@ -899,7 +906,9 @@
         issuer = removeSecurityProxy(
             getUtility(IMacaroonIssuer, "snap-build"))
         macaroon = issuer.issueMacaroon(other_build)
-        self.assertFalse(issuer.verifyMacaroon(macaroon, ref.repository))
+        self.assertMacaroonDoesNotVerify(
+            ["Caveat check for 'lp.snap-build %s' failed." % other_build.id],
+            issuer, macaroon, ref.repository)
 
     def test_verifyMacaroon_wrong_repository(self):
         [ref] = self.factory.makeGitRefs(
@@ -911,4 +920,6 @@
         issuer = removeSecurityProxy(
             getUtility(IMacaroonIssuer, "snap-build"))
         macaroon = issuer.issueMacaroon(build)
-        self.assertFalse(issuer.verifyMacaroon(macaroon, other_repository))
+        self.assertMacaroonDoesNotVerify(
+            ["Caveat check for 'lp.snap-build %s' failed." % build.id],
+            issuer, macaroon, other_repository)

=== modified file 'lib/lp/soyuz/tests/test_binarypackagebuild.py'
--- lib/lp/soyuz/tests/test_binarypackagebuild.py	2019-04-25 09:34:40 +0000
+++ lib/lp/soyuz/tests/test_binarypackagebuild.py	2019-04-25 22:19:14 +0000
@@ -35,6 +35,7 @@
     BadMacaroonContext,
     IMacaroonIssuer,
     )
+from lp.services.macaroons.testing import MacaroonTestMixin
 from lp.services.webapp.interaction import ANONYMOUS
 from lp.services.webapp.interfaces import OAuthPermission
 from lp.soyuz.enums import (
@@ -913,7 +914,8 @@
                 archive, getUtility(ILaunchpadCelebrities).ppa_admin)
 
 
-class TestBinaryPackageBuildMacaroonIssuer(TestCaseWithFactory):
+class TestBinaryPackageBuildMacaroonIssuer(
+        MacaroonTestMixin, TestCaseWithFactory):
     """Test BinaryPackageBuild macaroon issuing and verification."""
 
     layer = LaunchpadZopelessLayer
@@ -961,7 +963,7 @@
         issuer = removeSecurityProxy(
             getUtility(IMacaroonIssuer, "binary-package-build"))
         macaroon = issuer.issueMacaroon(build)
-        self.assertTrue(issuer.verifyMacaroon(macaroon, sprf.libraryfile))
+        self.assertMacaroonVerifies(issuer, macaroon, sprf.libraryfile)
 
     def test_verifyMacaroon_wrong_location(self):
         build = self.factory.makeBinaryPackageBuild(
@@ -973,7 +975,9 @@
             getUtility(IMacaroonIssuer, "binary-package-build"))
         macaroon = Macaroon(
             location="another-location", key=issuer._root_secret)
-        self.assertFalse(issuer.verifyMacaroon(macaroon, sprf.libraryfile))
+        self.assertMacaroonDoesNotVerify(
+            ["Macaroon has unknown location 'another-location'."],
+            issuer, macaroon, sprf.libraryfile)
 
     def test_verifyMacaroon_wrong_key(self):
         build = self.factory.makeBinaryPackageBuild(
@@ -985,7 +989,8 @@
             getUtility(IMacaroonIssuer, "binary-package-build"))
         macaroon = Macaroon(
             location=config.vhost.mainsite.hostname, key="another-secret")
-        self.assertFalse(issuer.verifyMacaroon(macaroon, sprf.libraryfile))
+        self.assertMacaroonDoesNotVerify(
+            ["Signatures do not match"], issuer, macaroon, sprf.libraryfile)
 
     def test_verifyMacaroon_not_building(self):
         build = self.factory.makeBinaryPackageBuild(
@@ -995,7 +1000,10 @@
         issuer = removeSecurityProxy(
             getUtility(IMacaroonIssuer, "binary-package-build"))
         macaroon = issuer.issueMacaroon(build)
-        self.assertFalse(issuer.verifyMacaroon(macaroon, sprf.libraryfile))
+        self.assertMacaroonDoesNotVerify(
+            ["Caveat check for 'lp.principal.binary-package-build %s' "
+             "failed." % build.id],
+            issuer, macaroon, sprf.libraryfile)
 
     def test_verifyMacaroon_wrong_build(self):
         build = self.factory.makeBinaryPackageBuild(
@@ -1009,7 +1017,10 @@
         issuer = removeSecurityProxy(
             getUtility(IMacaroonIssuer, "binary-package-build"))
         macaroon = issuer.issueMacaroon(other_build)
-        self.assertFalse(issuer.verifyMacaroon(macaroon, sprf.libraryfile))
+        self.assertMacaroonDoesNotVerify(
+            ["Caveat check for 'lp.principal.binary-package-build %s' "
+             "failed." % other_build.id],
+            issuer, macaroon, sprf.libraryfile)
 
     def test_verifyMacaroon_wrong_file(self):
         build = self.factory.makeBinaryPackageBuild(
@@ -1021,4 +1032,7 @@
         issuer = removeSecurityProxy(
             getUtility(IMacaroonIssuer, "binary-package-build"))
         macaroon = issuer.issueMacaroon(build)
-        self.assertFalse(issuer.verifyMacaroon(macaroon, lfa))
+        self.assertMacaroonDoesNotVerify(
+            ["Caveat check for 'lp.principal.binary-package-build %s' "
+             "failed." % build.id],
+            issuer, macaroon, lfa)