Merge lp:~cjwatson/launchpad/git-issue-access-tokens into lp:launchpad

[Colin Watson]

Add a macaroon issuer for Git access tokens.

These aren't honoured by the XML-RPC API yet, and there's no way for users to actually request them. Those will come in subsequent branches.

The expiry/persistence policy will likely need to change in future, but the current expiry time is short enough that we should be able to evolve this without too much difficulty.

[William Grant (wgrant)]

review: Approve (code)

=== modified file 'lib/lp/code/configure.zcml'
--- lib/lp/code/configure.zcml	2018-10-16 15:52:00 +0000
+++ lib/lp/code/configure.zcml	2019-05-01 16:44:22 +0000
@@ -1,4 +1,4 @@
-<!-- Copyright 2009-2018 Canonical Ltd.  This software is licensed under the
+<!-- Copyright 2009-2019 Canonical Ltd.  This software is licensed under the
      GNU Affero General Public License version 3 (see the file LICENSE).
 -->
 
@@ -856,6 +856,15 @@
     <allow interface="lp.code.interfaces.gitrepository.IGitRepositorySet" />
   </securedutility>
 
+  <!-- GitRepositoryMacaroonIssuer -->
+
+  <securedutility
+      class="lp.code.model.gitrepository.GitRepositoryMacaroonIssuer"
+      provides="lp.services.macaroons.interfaces.IMacaroonIssuer"
+      name="git-repository">
+    <allow interface="lp.services.macaroons.interfaces.IMacaroonIssuerPublic" />
+  </securedutility>
+
   <!-- GitRepositoryDelta -->
 
   <class class="lp.code.adapters.gitrepository.GitRepositoryDelta">

=== modified file 'lib/lp/code/model/codeimportjob.py'
--- lib/lp/code/model/codeimportjob.py	2019-04-26 13:13:37 +0000
+++ lib/lp/code/model/codeimportjob.py	2019-05-01 16:44:22 +0000
@@ -428,14 +428,14 @@
                 "launchpad.internal_macaroon_secret_key not configured.")
         return secret
 
-    def checkIssuingContext(self, context):
+    def checkIssuingContext(self, context, **kwargs):
         """See `MacaroonIssuerBase`."""
         if context.code_import.git_repository is None:
             raise BadMacaroonContext(
                 context, "context.code_import.git_repository is None")
         return context.id
 
-    def checkVerificationContext(self, context):
+    def checkVerificationContext(self, context, **kwargs):
         """See `MacaroonIssuerBase`.
 
         For verification, the context may be an `ICodeImportJob`, in which
@@ -461,7 +461,7 @@
                 context, "%r is not in the RUNNING state." % context)
         return context
 
-    def verifyPrimaryCaveat(self, caveat_value, context):
+    def verifyPrimaryCaveat(self, caveat_value, context, **kwargs):
         """See `MacaroonIssuerBase`."""
         if context is None:
             # We're only verifying that the macaroon could be valid for some

=== modified file 'lib/lp/code/model/gitrepository.py'
--- lib/lp/code/model/gitrepository.py	2019-04-02 08:12:27 +0000
+++ lib/lp/code/model/gitrepository.py	2019-05-01 16:44:22 +0000
@@ -13,7 +13,10 @@
     defaultdict,
     OrderedDict,
     )
-from datetime import datetime
+from datetime import (
+    datetime,
+    timedelta,
+    )
 import email
 from fnmatch import fnmatch
 from functools import partial
@@ -174,6 +177,7 @@
 from lp.services.database.decoratedresultset import DecoratedResultSet
 from lp.services.database.enumcol import EnumCol
 from lp.services.database.interfaces import IStore
+from lp.services.database.sqlbase import get_transaction_timestamp
 from lp.services.database.stormbase import StormBase
 from lp.services.database.stormexpr import (
     Array,
@@ -182,8 +186,12 @@
     BulkUpdate,
     Values,
     )
+from lp.services.features import getFeatureFlag
+from lp.services.identity.interfaces.account import IAccountSet
 from lp.services.job.interfaces.job import JobStatus
 from lp.services.job.model.job import Job
+from lp.services.macaroons.interfaces import IMacaroonIssuer
+from lp.services.macaroons.model import MacaroonIssuerBase
 from lp.services.mail.notificationrecipientset import NotificationRecipientSet
 from lp.services.propertycache import (
     cachedproperty,
@@ -1764,6 +1772,98 @@
             repository.project_id: repository for repository in repositories}
 
 
+@implementer(IMacaroonIssuer)
+class GitRepositoryMacaroonIssuer(MacaroonIssuerBase):
+
+    identifier = "git-repository"
+    allow_multiple = {"lp.expires"}
+
+    _timestamp_format = "%Y-%m-%dT%H:%M:%S.%f"
+
+    def __init__(self):
+        super(GitRepositoryMacaroonIssuer, self).__init__()
+        self.checkers = {
+            "lp.principal.openid-identifier": self.verifyOpenIDIdentifier,
+            "lp.expires": self.verifyExpires,
+            }
+
+    def checkIssuingContext(self, context, user=None, **kwargs):
+        """See `MacaroonIssuerBase`.
+
+        For issuing, the context is an `IGitRepository`.
+        """
+        if user is None:
+            raise Unauthorized(
+                "git-repository macaroons may only be issued for a logged-in "
+                "user.")
+        if not IGitRepository.providedBy(context):
+            raise ValueError("Cannot handle context %r." % context)
+        return context.id
+
+    def issueMacaroon(self, context, user=None, **kwargs):
+        """See `IMacaroonIssuer`."""
+        macaroon = super(GitRepositoryMacaroonIssuer, self).issueMacaroon(
+            context, user=user, **kwargs)
+        naked_account = removeSecurityProxy(user).account
+        macaroon.add_first_party_caveat(
+            "lp.principal.openid-identifier " +
+            naked_account.openid_identifiers.any().identifier)
+        store = IStore(GitRepository)
+        # XXX cjwatson 2019-04-09: Expire macaroons after the number of
+        # seconds given in the code.git.access_token_expiry feature flag,
+        # defaulting to a week.  This isn't very flexible, but for now it
+        # saves on having to implement macaroon persistence in order that
+        # users can revoke them.
+        expiry_seconds_str = getFeatureFlag("code.git.access_token_expiry")
+        if expiry_seconds_str is None:
+            expiry_seconds = 60 * 60 * 24 * 7
+        else:
+            expiry_seconds = int(expiry_seconds_str)
+        expiry = (
+            get_transaction_timestamp(store) +
+            timedelta(seconds=expiry_seconds))
+        macaroon.add_first_party_caveat(
+            "lp.expires " + expiry.strftime(self._timestamp_format))
+        return macaroon
+
+    def checkVerificationContext(self, context, **kwargs):
+        """See `MacaroonIssuerBase`.
+
+        For verification, the context is an `IGitRepository`.
+        """
+        if not IGitRepository.providedBy(context):
+            raise ValueError("Cannot handle context %r." % context)
+        return context
+
+    def verifyPrimaryCaveat(self, caveat_value, context, **kwargs):
+        """See `MacaroonIssuerBase`."""
+        if context is None:
+            # We're only verifying that the macaroon could be valid for some
+            # context.
+            return True
+        return caveat_value == str(context.id)
+
+    def verifyOpenIDIdentifier(self, caveat_value, context, **kwargs):
+        """Verify an lp.principal.openid-identifier caveat."""
+        user = kwargs.get("user")
+        try:
+            account = getUtility(IAccountSet).getByOpenIDIdentifier(
+                caveat_value)
+        except LookupError:
+            return False
+        return IPerson.providedBy(user) and user.account == account
+
+    def verifyExpires(self, caveat_value, context, **kwargs):
+        """Verify an lp.expires caveat."""
+        try:
+            expires = datetime.strptime(
+                caveat_value, self._timestamp_format).replace(tzinfo=pytz.UTC)
+        except ValueError:
+            return False
+        store = IStore(GitRepository)
+        return get_transaction_timestamp(store) < expires
+
+
 def get_git_repository_privacy_filter(user, repository_class=GitRepository):
     public_filter = repository_class.information_type.is_in(
         PUBLIC_INFORMATION_TYPES)

=== 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-05-01 16:44:22 +0000
@@ -17,12 +17,15 @@
 import json
 
 from bzrlib import urlutils
+from fixtures import MockPatch
 from lazr.lifecycle.event import ObjectModifiedEvent
+from pymacaroons import Macaroon
 import pytz
 from sqlobject import SQLObjectNotFound
 from storm.exceptions import LostObjectError
 from storm.store import Store
 from testtools.matchers import (
+    AnyMatch,
     EndsWith,
     Equals,
     Is,
@@ -34,6 +37,7 @@
     )
 import transaction
 from zope.component import getUtility
+from zope.publisher.xmlrpc import TestRequest
 from zope.security.interfaces import Unauthorized
 from zope.security.proxy import removeSecurityProxy
 
@@ -127,14 +131,22 @@
     )
 from lp.registry.interfaces.personproduct import IPersonProductFactory
 from lp.registry.tests.test_accesspolicy import get_policies_for_artifact
+from lp.services.authserver.xmlrpc import AuthServerAPIView
 from lp.services.config import config
 from lp.services.database.constants import UTC_NOW
 from lp.services.database.interfaces import IStore
 from lp.services.database.sqlbase import get_transaction_timestamp
+from lp.services.features.testing import FeatureFixture
 from lp.services.job.interfaces.job import JobStatus
 from lp.services.job.model.job import Job
 from lp.services.job.runner import JobRunner
+from lp.services.macaroons.interfaces import IMacaroonIssuer
+from lp.services.macaroons.testing import (
+    find_caveats_by_name,
+    MacaroonTestMixin,
+    )
 from lp.services.mail import stub
+from lp.services.openid.model.openididentifier import OpenIdIdentifier
 from lp.services.propertycache import clear_property_cache
 from lp.services.utils import seconds_since_epoch
 from lp.services.webapp.authorization import check_permission
@@ -163,6 +175,8 @@
     HasQueryCount,
     )
 from lp.testing.pages import webservice_for_person
+from lp.xmlrpc import faults
+from lp.xmlrpc.interfaces import IPrivateApplication
 
 
 class TestGitRepository(TestCaseWithFactory):
@@ -3894,3 +3908,222 @@
             "refs/heads/next": Equals(["push", "force-push"]),
             "refs/other": Equals([]),
             }))
+
+
+class TestGitRepositoryMacaroonIssuer(MacaroonTestMixin, TestCaseWithFactory):
+    """Test GitRepository macaroon issuing and verification."""
+
+    layer = DatabaseFunctionalLayer
+
+    def setUp(self):
+        super(TestGitRepositoryMacaroonIssuer, self).setUp()
+        self.pushConfig(
+            "launchpad", internal_macaroon_secret_key="some-secret")
+
+    def test_issueMacaroon_refuses_branch(self):
+        branch = self.factory.makeAnyBranch()
+        issuer = getUtility(IMacaroonIssuer, "git-repository")
+        self.assertRaises(
+            ValueError, removeSecurityProxy(issuer).issueMacaroon,
+            branch, user=branch.owner)
+
+    def test_issueMacaroon_good(self):
+        repository = self.factory.makeGitRepository()
+        issuer = getUtility(IMacaroonIssuer, "git-repository")
+        naked_account = removeSecurityProxy(repository.owner).account
+        identifier = naked_account.openid_identifiers.any().identifier
+        macaroon = removeSecurityProxy(issuer).issueMacaroon(
+            repository, user=repository.owner)
+        now = get_transaction_timestamp(Store.of(repository))
+        expires = now + timedelta(days=7)
+        self.assertThat(macaroon, MatchesStructure(
+            location=Equals(config.vhost.mainsite.hostname),
+            identifier=Equals("git-repository"),
+            caveats=MatchesListwise([
+                MatchesStructure.byEquality(
+                    caveat_id="lp.git-repository %s" % repository.id),
+                MatchesStructure.byEquality(
+                    caveat_id=(
+                        "lp.principal.openid-identifier %s" % identifier)),
+                MatchesStructure.byEquality(
+                    caveat_id="lp.expires %s" % (
+                        expires.strftime("%Y-%m-%dT%H:%M:%S.%f"))),
+                ])))
+
+    def test_issueMacaroon_expiry_feature_flag(self):
+        self.useFixture(FeatureFixture(
+            {"code.git.access_token_expiry": "3600"}))
+        repository = self.factory.makeGitRepository()
+        issuer = getUtility(IMacaroonIssuer, "git-repository")
+        macaroon = removeSecurityProxy(issuer).issueMacaroon(
+            repository, user=repository.owner)
+        now = get_transaction_timestamp(Store.of(repository))
+        expires = now + timedelta(hours=1)
+        self.assertThat(macaroon, MatchesStructure(
+            caveats=AnyMatch(
+                MatchesStructure.byEquality(
+                    caveat_id="lp.expires %s" % (
+                        expires.strftime("%Y-%m-%dT%H:%M:%S.%f"))))))
+
+    def test_issueMacaroon_no_user(self):
+        repository = self.factory.makeGitRepository()
+        issuer = getUtility(IMacaroonIssuer, "git-repository")
+        self.assertRaises(
+            Unauthorized,
+            removeSecurityProxy(issuer).issueMacaroon, repository)
+
+    def test_issueMacaroon_not_via_authserver(self):
+        repository = self.factory.makeGitRepository()
+        private_root = getUtility(IPrivateApplication)
+        authserver = AuthServerAPIView(private_root.authserver, TestRequest())
+        self.assertEqual(
+            faults.PermissionDenied(),
+            authserver.issueMacaroon(
+                "git-repository", "GitRepository", repository))
+
+    def test_verifyMacaroon_good(self):
+        repository = self.factory.makeGitRepository()
+        issuer = getUtility(IMacaroonIssuer, "git-repository")
+        macaroon = removeSecurityProxy(issuer).issueMacaroon(
+            repository, user=repository.owner)
+        self.assertMacaroonVerifies(
+            issuer, macaroon, repository, user=repository.owner)
+
+    def test_verifyMacaroon_wrong_location(self):
+        repository = self.factory.makeGitRepository()
+        issuer = getUtility(IMacaroonIssuer, "git-repository")
+        macaroon = Macaroon(
+            location="another-location",
+            key=removeSecurityProxy(issuer)._root_secret)
+        self.assertMacaroonDoesNotVerify(
+            ["Macaroon has unknown location 'another-location'."],
+            issuer, macaroon, repository, user=repository.owner)
+
+    def test_verifyMacaroon_wrong_key(self):
+        repository = self.factory.makeGitRepository()
+        issuer = getUtility(IMacaroonIssuer, "git-repository")
+        macaroon = Macaroon(
+            location=config.vhost.mainsite.hostname, key="another-secret")
+        self.assertMacaroonDoesNotVerify(
+            ["Signatures do not match"],
+            issuer, macaroon, repository, user=repository.owner)
+
+    def test_verifyMacaroon_wrong_repository(self):
+        repository = self.factory.makeGitRepository()
+        issuer = getUtility(IMacaroonIssuer, "git-repository")
+        macaroon = removeSecurityProxy(issuer).issueMacaroon(
+            repository, user=repository.owner)
+        self.assertMacaroonDoesNotVerify(
+            ["Caveat check for 'lp.git-repository %s' failed." %
+             repository.id],
+            issuer, macaroon, self.factory.makeGitRepository(),
+            user=repository.owner)
+
+    def test_verifyMacaroon_multiple_repository_caveats(self):
+        repository = self.factory.makeGitRepository()
+        issuer = getUtility(IMacaroonIssuer, "git-repository")
+        macaroon = removeSecurityProxy(issuer).issueMacaroon(
+            repository, user=repository.owner)
+        macaroon.add_first_party_caveat("lp.git-repository another")
+        self.assertMacaroonDoesNotVerify(
+            ["Multiple 'lp.git-repository' caveats are not allowed."],
+            issuer, macaroon, repository, user=repository.owner)
+
+    def test_verifyMacaroon_wrong_user(self):
+        repository = self.factory.makeGitRepository()
+        issuer = getUtility(IMacaroonIssuer, "git-repository")
+        naked_account = removeSecurityProxy(repository.owner).account
+        identifier = naked_account.openid_identifiers.any().identifier
+        macaroon = removeSecurityProxy(issuer).issueMacaroon(
+            repository, user=repository.owner)
+        self.assertMacaroonDoesNotVerify(
+            ["Caveat check for 'lp.principal.openid-identifier %s' failed." %
+             identifier],
+            issuer, macaroon, repository, user=self.factory.makePerson())
+
+    def test_verifyMacaroon_closed_account(self):
+        # A closed account no longer has an OpenID identifier, so the
+        # corresponding caveat doesn't match.
+        repository = self.factory.makeGitRepository()
+        owner = repository.owner
+        issuer = getUtility(IMacaroonIssuer, "git-repository")
+        naked_account = removeSecurityProxy(owner).account
+        identifier = naked_account.openid_identifiers.any().identifier
+        macaroon = removeSecurityProxy(issuer).issueMacaroon(
+            repository, user=owner)
+        IStore(OpenIdIdentifier).find(
+            OpenIdIdentifier,
+            OpenIdIdentifier.account_id == owner.account.id).remove()
+        self.assertMacaroonDoesNotVerify(
+            ["Caveat check for 'lp.principal.openid-identifier %s' failed." %
+             identifier],
+            issuer, macaroon, repository, user=owner)
+
+    def test_verifyMacaroon_multiple_openid_identifier_caveats(self):
+        repository = self.factory.makeGitRepository()
+        issuer = getUtility(IMacaroonIssuer, "git-repository")
+        macaroon = removeSecurityProxy(issuer).issueMacaroon(
+            repository, user=repository.owner)
+        macaroon.add_first_party_caveat(
+            "lp.principal.openid-identifier another")
+        self.assertMacaroonDoesNotVerify(
+            ["Multiple 'lp.principal.openid-identifier' caveats are not "
+             "allowed."],
+            issuer, macaroon, repository, user=repository.owner)
+
+    def test_verifyMacaroon_expired(self):
+        repository = self.factory.makeGitRepository()
+        issuer = getUtility(IMacaroonIssuer, "git-repository")
+        macaroon = removeSecurityProxy(issuer).issueMacaroon(
+            repository, user=repository.owner)
+        now = get_transaction_timestamp(Store.of(repository))
+        self.useFixture(MockPatch(
+            "lp.code.model.gitrepository.get_transaction_timestamp",
+            lambda _: now + timedelta(days=8)))
+        self.assertMacaroonDoesNotVerify(
+            ["Caveat check for '%s' failed." %
+             find_caveats_by_name(macaroon, "lp.expires")[0].caveat_id],
+            issuer, macaroon, repository, user=repository.owner)
+
+    def test_verifyMacaroon_multiple_expires_caveats(self):
+        # If somebody attaches another expires caveat to the macaroon,
+        # that's OK; we just take the strictest.
+        repository = self.factory.makeGitRepository()
+        issuer = getUtility(IMacaroonIssuer, "git-repository")
+        macaroon1 = removeSecurityProxy(issuer).issueMacaroon(
+            repository, user=repository.owner)
+        macaroon2 = removeSecurityProxy(issuer).issueMacaroon(
+            repository, user=repository.owner)
+        now = get_transaction_timestamp(Store.of(repository))
+        expires1 = now + timedelta(days=1)
+        expires2 = now + timedelta(days=14)
+        macaroon1.add_first_party_caveat(
+            "lp.expires " + expires1.strftime("%Y-%m-%dT%H:%M:%S.%f"))
+        macaroon2.add_first_party_caveat(
+            "lp.expires " + expires2.strftime("%Y-%m-%dT%H:%M:%S.%f"))
+        self.assertMacaroonVerifies(
+            issuer, macaroon1, repository, user=repository.owner)
+        self.assertMacaroonVerifies(
+            issuer, macaroon2, repository, user=repository.owner)
+        with MockPatch(
+                "lp.code.model.gitrepository.get_transaction_timestamp",
+                lambda _: now + timedelta(days=4)):
+            self.assertMacaroonDoesNotVerify(
+                ["Caveat check for '%s' failed." %
+                 find_caveats_by_name(macaroon1, "lp.expires")[1].caveat_id],
+                issuer, macaroon1, repository, user=repository.owner)
+            self.assertMacaroonVerifies(
+                issuer, macaroon2, repository, user=repository.owner)
+        with MockPatch(
+                "lp.code.model.gitrepository.get_transaction_timestamp",
+                lambda _: now + timedelta(days=8)):
+            self.assertMacaroonDoesNotVerify(
+                ["Caveat check for '%s' failed." %
+                 find_caveats_by_name(macaroon1, "lp.expires")[0].caveat_id,
+                 "Caveat check for '%s' failed." %
+                 find_caveats_by_name(macaroon1, "lp.expires")[1].caveat_id],
+                issuer, macaroon1, repository, user=repository.owner)
+            self.assertMacaroonDoesNotVerify(
+                ["Caveat check for '%s' failed." %
+                 find_caveats_by_name(macaroon2, "lp.expires")[0].caveat_id],
+                issuer, macaroon2, repository, user=repository.owner)

=== modified file 'lib/lp/services/authserver/tests/test_authserver.py'
--- lib/lp/services/authserver/tests/test_authserver.py	2019-04-25 09:55:35 +0000
+++ lib/lp/services/authserver/tests/test_authserver.py	2019-05-01 16:44:22 +0000
@@ -89,19 +89,19 @@
     issuable_via_authserver = True
     _root_secret = 'test'
 
-    def checkIssuingContext(self, context):
+    def checkIssuingContext(self, context, **kwargs):
         """See `MacaroonIssuerBase`."""
         if not ILibraryFileAlias.providedBy(context):
             raise BadMacaroonContext(context)
         return context.id
 
-    def checkVerificationContext(self, context):
+    def checkVerificationContext(self, context, **kwargs):
         """See `IMacaroonIssuerBase`."""
         if not ILibraryFileAlias.providedBy(context):
             raise BadMacaroonContext(context)
         return context
 
-    def verifyPrimaryCaveat(self, caveat_value, context):
+    def verifyPrimaryCaveat(self, caveat_value, context, **kwargs):
         """See `MacaroonIssuerBase`."""
         return caveat_value == str(context.id)
 

=== modified file 'lib/lp/services/macaroons/interfaces.py'
--- lib/lp/services/macaroons/interfaces.py	2019-04-25 22:10:36 +0000
+++ lib/lp/services/macaroons/interfaces.py	2019-05-01 16:44:22 +0000
@@ -31,7 +31,8 @@
     issuable_via_authserver = Bool(
         "Does this issuer allow issuing macaroons via the authserver?")
 
-    def verifyMacaroon(macaroon, context, require_context=True, errors=None):
+    def verifyMacaroon(macaroon, context, require_context=True, errors=None,
+                       **kwargs):
         """Verify that `macaroon` is valid for `context`.
 
         :param macaroon: A `Macaroon`.
@@ -43,6 +44,8 @@
             authentication/authorisation API.
         :param errors: If non-None, any verification error messages will be
             appended to this list.
+        :param kwargs: Additional arguments that issuers may require to
+            verify a macaroon.
         :return: True if `macaroon` is valid for `context`, otherwise False.
         """
 
@@ -50,11 +53,13 @@
 class IMacaroonIssuer(IMacaroonIssuerPublic):
     """Interface to a policy for issuing and verifying macaroons."""
 
-    def issueMacaroon(context):
+    def issueMacaroon(context, **kwargs):
         """Issue a macaroon for `context`.
 
         :param context: The context that the returned macaroon should relate
             to.
+        :param kwargs: Additional arguments that issuers may require to
+            issue a macaroon.
         :raises BadMacaroonContext: if the context is unsuitable.
         :return: A macaroon.
         """

=== modified file 'lib/lp/services/macaroons/model.py'
--- lib/lp/services/macaroons/model.py	2019-04-25 22:10:36 +0000
+++ lib/lp/services/macaroons/model.py	2019-05-01 16:44:22 +0000
@@ -25,6 +25,31 @@
 
     issuable_via_authserver = False
 
+    # A mapping of caveat names to "checker" callables that verify the
+    # corresponding caveat text.  The signature of each checker is
+    # (caveat_value, context, **kwargs) -> bool, where caveat_value is the
+    # text of the caveat with the caveat name removed, context is the
+    # issuer-specific context to check, and kwargs is any other keyword
+    # arguments that were given to verifyMacaroon; it should return True if
+    # the caveat is allowed, otherwise False.
+    #
+    # The context passed in may be None, in which case the checker may
+    # choose to only verify that the caveat could be valid for some context,
+    # or may simply return False if this is unsupported.  This is useful for
+    # issuers that support APIs with separate authentication and
+    # authorisation phases.
+    #
+    # The "primary context caveat" added to all macaroons issued by this
+    # base class does not need to be listed here; it is handled by the
+    # verifyContextCaveat method.
+    checkers = {}
+
+    # Caveat names in this set may appear more than once (in which case they
+    # have the usual subtractive semantics, so the union of all the
+    # constraints they express applies).  Any other caveats may only appear
+    # once.
+    allow_multiple = set()
+
     @property
     def identifier(self):
         """An identifying name for this issuer."""
@@ -43,7 +68,7 @@
                 "launchpad.internal_macaroon_secret_key not configured.")
         return secret
 
-    def checkIssuingContext(self, context):
+    def checkIssuingContext(self, context, **kwargs):
         """Check that the issuing context is suitable.
 
         Concrete implementations may implement this method to check that the
@@ -52,18 +77,16 @@
         was passed in or an adapted one.
 
         :param context: The context to check.
+        :param kwargs: Additional arguments that issuers may require to
+            issue a macaroon.
         :raises BadMacaroonContext: if the context is unsuitable.
         :return: The context to use to create the primary caveat.
         """
         return context
 
-    def issueMacaroon(self, context):
-        """See `IMacaroonIssuer`.
-
-        Concrete implementations should normally wrap this with some
-        additional checks of and/or changes to the context.
-        """
-        context = self.checkIssuingContext(context)
+    def issueMacaroon(self, context, **kwargs):
+        """See `IMacaroonIssuer`."""
+        context = self.checkIssuingContext(context, **kwargs)
         macaroon = Macaroon(
             location=config.vhost.mainsite.hostname,
             identifier=self.identifier, key=self._root_secret)
@@ -71,7 +94,7 @@
             "%s %s" % (self._primary_caveat_name, context))
         return macaroon
 
-    def checkVerificationContext(self, context):
+    def checkVerificationContext(self, context, **kwargs):
         """Check that the verification context is suitable.
 
         Concrete implementations may implement this method to check that the
@@ -80,23 +103,27 @@
         context that was passed in or an adapted one.
 
         :param context: The context to check.
+        :param kwargs: Additional arguments that issuers may require to
+            verify a macaroon.
         :raises BadMacaroonContext: if the context is unsuitable.
         :return: The context to pass to individual caveat checkers.
         """
         return context
 
-    def verifyPrimaryCaveat(self, caveat_value, context):
+    def verifyPrimaryCaveat(self, caveat_value, context, **kwargs):
         """Verify the primary context caveat on one of this issuer's macaroons.
 
-        :param caveat_value: The text of the caveat, with this issuer's
-            prefix removed.
+        :param caveat_value: The text of the caveat with the caveat name
+            removed.
         :param context: The context to check.
+        :param kwargs: Additional arguments that issuers may require to
+            verify a macaroon.
         :return: True if this caveat is allowed, otherwise False.
         """
         raise NotImplementedError
 
     def verifyMacaroon(self, macaroon, context, require_context=True,
-                       errors=None):
+                       errors=None, **kwargs):
         """See `IMacaroonIssuer`."""
         if macaroon.location != config.vhost.mainsite.hostname:
             if errors is not None:
@@ -115,6 +142,7 @@
                 if errors is not None:
                     errors.append(str(e))
                 return False
+        seen = set()
 
         def verify(caveat):
             try:
@@ -123,16 +151,22 @@
                 if errors is not None:
                     errors.append("Cannot parse caveat '%s'." % caveat)
                 return False
+            if caveat_name not in self.allow_multiple and caveat_name in seen:
+                if errors is not None:
+                    errors.append(
+                        "Multiple '%s' caveats are not allowed." % caveat_name)
+                return False
+            seen.add(caveat_name)
             if caveat_name == self._primary_caveat_name:
                 checker = self.verifyPrimaryCaveat
             else:
-                # XXX cjwatson 2019-04-09: For now we just fail closed if
-                # there are any other caveats, which is good enough for
-                # internal use.
-                if errors is not None:
-                    errors.append("Unhandled caveat name '%s'." % caveat_name)
-                return False
-            if not checker(caveat_value, context):
+                checker = self.checkers.get(caveat_name)
+                if checker is None:
+                    if errors is not None:
+                        errors.append(
+                            "Unhandled caveat name '%s'." % caveat_name)
+                    return False
+            if not checker(caveat_value, context, **kwargs):
                 if errors is not None:
                     errors.append("Caveat check for '%s' failed." % caveat)
                 return False

=== modified file 'lib/lp/snappy/model/snapbuild.py'
--- lib/lp/snappy/model/snapbuild.py	2019-04-25 09:34:40 +0000
+++ lib/lp/snappy/model/snapbuild.py	2019-05-01 16:44:22 +0000
@@ -599,7 +599,7 @@
     identifier = "snap-build"
     issuable_via_authserver = True
 
-    def checkIssuingContext(self, context):
+    def checkIssuingContext(self, context, **kwargs):
         """See `MacaroonIssuerBase`.
 
         For issuing, the context is an `ISnapBuild`.
@@ -611,13 +611,13 @@
                 context, "Refusing to issue macaroon for public build.")
         return removeSecurityProxy(context).id
 
-    def checkVerificationContext(self, context):
+    def checkVerificationContext(self, context, **kwargs):
         """See `MacaroonIssuerBase`."""
         if not IGitRepository.providedBy(context):
             raise BadMacaroonContext(context)
         return context
 
-    def verifyPrimaryCaveat(self, caveat_value, context):
+    def verifyPrimaryCaveat(self, caveat_value, context, **kwargs):
         """See `MacaroonIssuerBase`.
 
         For verification, the context is an `IGitRepository`.  We check that

=== modified file 'lib/lp/soyuz/model/binarypackagebuild.py'
--- lib/lp/soyuz/model/binarypackagebuild.py	2019-04-24 12:50:20 +0000
+++ lib/lp/soyuz/model/binarypackagebuild.py	2019-05-01 16:44:22 +0000
@@ -1387,7 +1387,7 @@
         # the named build directly.
         return "lp.principal.binary-package-build"
 
-    def checkIssuingContext(self, context):
+    def checkIssuingContext(self, context, **kwargs):
         """See `MacaroonIssuerBase`.
 
         For issuing, the context is an `IBinaryPackageBuild`.
@@ -1397,13 +1397,13 @@
                 context, "Refusing to issue macaroon for public build.")
         return removeSecurityProxy(context).id
 
-    def checkVerificationContext(self, context):
+    def checkVerificationContext(self, context, **kwargs):
         """See `MacaroonIssuerBase`."""
         if not ILibraryFileAlias.providedBy(context):
             raise BadMacaroonContext(context)
         return context
 
-    def verifyPrimaryCaveat(self, caveat_value, context):
+    def verifyPrimaryCaveat(self, caveat_value, context, **kwargs):
         """See `MacaroonIssuerBase`.
 
         For verification, the context is an `ILibraryFileAlias`.  We check