Merge lp:~cjwatson/launchpad/git-issue-access-tokens into lp:launchpad
- git-issue-access-tokens
- Merge into devel
Proposed by
Colin Watson
Status: | Merged |
---|---|
Merged at revision: | 18954 |
Proposed branch: | lp:~cjwatson/launchpad/git-issue-access-tokens |
Merge into: | lp:launchpad |
Prerequisite: | lp:~cjwatson/launchpad/refactor-code-import-macaroons |
Diff against target: |
745 lines (+417/-36) 9 files modified
lib/lp/code/configure.zcml (+10/-1) lib/lp/code/model/codeimportjob.py (+3/-3) lib/lp/code/model/gitrepository.py (+101/-1) lib/lp/code/model/tests/test_gitrepository.py (+233/-0) lib/lp/services/authserver/tests/test_authserver.py (+3/-3) lib/lp/services/macaroons/interfaces.py (+7/-2) lib/lp/services/macaroons/model.py (+54/-20) lib/lp/snappy/model/snapbuild.py (+3/-3) lib/lp/soyuz/model/binarypackagebuild.py (+3/-3) |
To merge this branch: | bzr merge lp:~cjwatson/launchpad/git-issue-access-tokens |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
William Grant | code | Approve | |
Review via email: mp+366052@code.launchpad.net |
Commit message
Add a macaroon issuer for Git access tokens.
Description of the change
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.
To post a comment you must log in.
Revision history for this message
William Grant (wgrant) : | # |
review:
Approve
(code)
Revision history for this message
Colin Watson (cjwatson) : | # |
Revision history for this message
Colin Watson (cjwatson) : | # |
Revision history for this message
William Grant (wgrant) : | # |
Preview Diff
[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1 | === modified file 'lib/lp/code/configure.zcml' | |||
2 | --- lib/lp/code/configure.zcml 2018-10-16 15:52:00 +0000 | |||
3 | +++ lib/lp/code/configure.zcml 2019-05-01 16:44:22 +0000 | |||
4 | @@ -1,4 +1,4 @@ | |||
6 | 1 | <!-- Copyright 2009-2018 Canonical Ltd. This software is licensed under the | 1 | <!-- Copyright 2009-2019 Canonical Ltd. This software is licensed under the |
7 | 2 | GNU Affero General Public License version 3 (see the file LICENSE). | 2 | GNU Affero General Public License version 3 (see the file LICENSE). |
8 | 3 | --> | 3 | --> |
9 | 4 | 4 | ||
10 | @@ -856,6 +856,15 @@ | |||
11 | 856 | <allow interface="lp.code.interfaces.gitrepository.IGitRepositorySet" /> | 856 | <allow interface="lp.code.interfaces.gitrepository.IGitRepositorySet" /> |
12 | 857 | </securedutility> | 857 | </securedutility> |
13 | 858 | 858 | ||
14 | 859 | <!-- GitRepositoryMacaroonIssuer --> | ||
15 | 860 | |||
16 | 861 | <securedutility | ||
17 | 862 | class="lp.code.model.gitrepository.GitRepositoryMacaroonIssuer" | ||
18 | 863 | provides="lp.services.macaroons.interfaces.IMacaroonIssuer" | ||
19 | 864 | name="git-repository"> | ||
20 | 865 | <allow interface="lp.services.macaroons.interfaces.IMacaroonIssuerPublic" /> | ||
21 | 866 | </securedutility> | ||
22 | 867 | |||
23 | 859 | <!-- GitRepositoryDelta --> | 868 | <!-- GitRepositoryDelta --> |
24 | 860 | 869 | ||
25 | 861 | <class class="lp.code.adapters.gitrepository.GitRepositoryDelta"> | 870 | <class class="lp.code.adapters.gitrepository.GitRepositoryDelta"> |
26 | 862 | 871 | ||
27 | === modified file 'lib/lp/code/model/codeimportjob.py' | |||
28 | --- lib/lp/code/model/codeimportjob.py 2019-04-26 13:13:37 +0000 | |||
29 | +++ lib/lp/code/model/codeimportjob.py 2019-05-01 16:44:22 +0000 | |||
30 | @@ -428,14 +428,14 @@ | |||
31 | 428 | "launchpad.internal_macaroon_secret_key not configured.") | 428 | "launchpad.internal_macaroon_secret_key not configured.") |
32 | 429 | return secret | 429 | return secret |
33 | 430 | 430 | ||
35 | 431 | def checkIssuingContext(self, context): | 431 | def checkIssuingContext(self, context, **kwargs): |
36 | 432 | """See `MacaroonIssuerBase`.""" | 432 | """See `MacaroonIssuerBase`.""" |
37 | 433 | if context.code_import.git_repository is None: | 433 | if context.code_import.git_repository is None: |
38 | 434 | raise BadMacaroonContext( | 434 | raise BadMacaroonContext( |
39 | 435 | context, "context.code_import.git_repository is None") | 435 | context, "context.code_import.git_repository is None") |
40 | 436 | return context.id | 436 | return context.id |
41 | 437 | 437 | ||
43 | 438 | def checkVerificationContext(self, context): | 438 | def checkVerificationContext(self, context, **kwargs): |
44 | 439 | """See `MacaroonIssuerBase`. | 439 | """See `MacaroonIssuerBase`. |
45 | 440 | 440 | ||
46 | 441 | For verification, the context may be an `ICodeImportJob`, in which | 441 | For verification, the context may be an `ICodeImportJob`, in which |
47 | @@ -461,7 +461,7 @@ | |||
48 | 461 | context, "%r is not in the RUNNING state." % context) | 461 | context, "%r is not in the RUNNING state." % context) |
49 | 462 | return context | 462 | return context |
50 | 463 | 463 | ||
52 | 464 | def verifyPrimaryCaveat(self, caveat_value, context): | 464 | def verifyPrimaryCaveat(self, caveat_value, context, **kwargs): |
53 | 465 | """See `MacaroonIssuerBase`.""" | 465 | """See `MacaroonIssuerBase`.""" |
54 | 466 | if context is None: | 466 | if context is None: |
55 | 467 | # We're only verifying that the macaroon could be valid for some | 467 | # We're only verifying that the macaroon could be valid for some |
56 | 468 | 468 | ||
57 | === modified file 'lib/lp/code/model/gitrepository.py' | |||
58 | --- lib/lp/code/model/gitrepository.py 2019-04-02 08:12:27 +0000 | |||
59 | +++ lib/lp/code/model/gitrepository.py 2019-05-01 16:44:22 +0000 | |||
60 | @@ -13,7 +13,10 @@ | |||
61 | 13 | defaultdict, | 13 | defaultdict, |
62 | 14 | OrderedDict, | 14 | OrderedDict, |
63 | 15 | ) | 15 | ) |
65 | 16 | from datetime import datetime | 16 | from datetime import ( |
66 | 17 | datetime, | ||
67 | 18 | timedelta, | ||
68 | 19 | ) | ||
69 | 17 | import email | 20 | import email |
70 | 18 | from fnmatch import fnmatch | 21 | from fnmatch import fnmatch |
71 | 19 | from functools import partial | 22 | from functools import partial |
72 | @@ -174,6 +177,7 @@ | |||
73 | 174 | from lp.services.database.decoratedresultset import DecoratedResultSet | 177 | from lp.services.database.decoratedresultset import DecoratedResultSet |
74 | 175 | from lp.services.database.enumcol import EnumCol | 178 | from lp.services.database.enumcol import EnumCol |
75 | 176 | from lp.services.database.interfaces import IStore | 179 | from lp.services.database.interfaces import IStore |
76 | 180 | from lp.services.database.sqlbase import get_transaction_timestamp | ||
77 | 177 | from lp.services.database.stormbase import StormBase | 181 | from lp.services.database.stormbase import StormBase |
78 | 178 | from lp.services.database.stormexpr import ( | 182 | from lp.services.database.stormexpr import ( |
79 | 179 | Array, | 183 | Array, |
80 | @@ -182,8 +186,12 @@ | |||
81 | 182 | BulkUpdate, | 186 | BulkUpdate, |
82 | 183 | Values, | 187 | Values, |
83 | 184 | ) | 188 | ) |
84 | 189 | from lp.services.features import getFeatureFlag | ||
85 | 190 | from lp.services.identity.interfaces.account import IAccountSet | ||
86 | 185 | from lp.services.job.interfaces.job import JobStatus | 191 | from lp.services.job.interfaces.job import JobStatus |
87 | 186 | from lp.services.job.model.job import Job | 192 | from lp.services.job.model.job import Job |
88 | 193 | from lp.services.macaroons.interfaces import IMacaroonIssuer | ||
89 | 194 | from lp.services.macaroons.model import MacaroonIssuerBase | ||
90 | 187 | from lp.services.mail.notificationrecipientset import NotificationRecipientSet | 195 | from lp.services.mail.notificationrecipientset import NotificationRecipientSet |
91 | 188 | from lp.services.propertycache import ( | 196 | from lp.services.propertycache import ( |
92 | 189 | cachedproperty, | 197 | cachedproperty, |
93 | @@ -1764,6 +1772,98 @@ | |||
94 | 1764 | repository.project_id: repository for repository in repositories} | 1772 | repository.project_id: repository for repository in repositories} |
95 | 1765 | 1773 | ||
96 | 1766 | 1774 | ||
97 | 1775 | @implementer(IMacaroonIssuer) | ||
98 | 1776 | class GitRepositoryMacaroonIssuer(MacaroonIssuerBase): | ||
99 | 1777 | |||
100 | 1778 | identifier = "git-repository" | ||
101 | 1779 | allow_multiple = {"lp.expires"} | ||
102 | 1780 | |||
103 | 1781 | _timestamp_format = "%Y-%m-%dT%H:%M:%S.%f" | ||
104 | 1782 | |||
105 | 1783 | def __init__(self): | ||
106 | 1784 | super(GitRepositoryMacaroonIssuer, self).__init__() | ||
107 | 1785 | self.checkers = { | ||
108 | 1786 | "lp.principal.openid-identifier": self.verifyOpenIDIdentifier, | ||
109 | 1787 | "lp.expires": self.verifyExpires, | ||
110 | 1788 | } | ||
111 | 1789 | |||
112 | 1790 | def checkIssuingContext(self, context, user=None, **kwargs): | ||
113 | 1791 | """See `MacaroonIssuerBase`. | ||
114 | 1792 | |||
115 | 1793 | For issuing, the context is an `IGitRepository`. | ||
116 | 1794 | """ | ||
117 | 1795 | if user is None: | ||
118 | 1796 | raise Unauthorized( | ||
119 | 1797 | "git-repository macaroons may only be issued for a logged-in " | ||
120 | 1798 | "user.") | ||
121 | 1799 | if not IGitRepository.providedBy(context): | ||
122 | 1800 | raise ValueError("Cannot handle context %r." % context) | ||
123 | 1801 | return context.id | ||
124 | 1802 | |||
125 | 1803 | def issueMacaroon(self, context, user=None, **kwargs): | ||
126 | 1804 | """See `IMacaroonIssuer`.""" | ||
127 | 1805 | macaroon = super(GitRepositoryMacaroonIssuer, self).issueMacaroon( | ||
128 | 1806 | context, user=user, **kwargs) | ||
129 | 1807 | naked_account = removeSecurityProxy(user).account | ||
130 | 1808 | macaroon.add_first_party_caveat( | ||
131 | 1809 | "lp.principal.openid-identifier " + | ||
132 | 1810 | naked_account.openid_identifiers.any().identifier) | ||
133 | 1811 | store = IStore(GitRepository) | ||
134 | 1812 | # XXX cjwatson 2019-04-09: Expire macaroons after the number of | ||
135 | 1813 | # seconds given in the code.git.access_token_expiry feature flag, | ||
136 | 1814 | # defaulting to a week. This isn't very flexible, but for now it | ||
137 | 1815 | # saves on having to implement macaroon persistence in order that | ||
138 | 1816 | # users can revoke them. | ||
139 | 1817 | expiry_seconds_str = getFeatureFlag("code.git.access_token_expiry") | ||
140 | 1818 | if expiry_seconds_str is None: | ||
141 | 1819 | expiry_seconds = 60 * 60 * 24 * 7 | ||
142 | 1820 | else: | ||
143 | 1821 | expiry_seconds = int(expiry_seconds_str) | ||
144 | 1822 | expiry = ( | ||
145 | 1823 | get_transaction_timestamp(store) + | ||
146 | 1824 | timedelta(seconds=expiry_seconds)) | ||
147 | 1825 | macaroon.add_first_party_caveat( | ||
148 | 1826 | "lp.expires " + expiry.strftime(self._timestamp_format)) | ||
149 | 1827 | return macaroon | ||
150 | 1828 | |||
151 | 1829 | def checkVerificationContext(self, context, **kwargs): | ||
152 | 1830 | """See `MacaroonIssuerBase`. | ||
153 | 1831 | |||
154 | 1832 | For verification, the context is an `IGitRepository`. | ||
155 | 1833 | """ | ||
156 | 1834 | if not IGitRepository.providedBy(context): | ||
157 | 1835 | raise ValueError("Cannot handle context %r." % context) | ||
158 | 1836 | return context | ||
159 | 1837 | |||
160 | 1838 | def verifyPrimaryCaveat(self, caveat_value, context, **kwargs): | ||
161 | 1839 | """See `MacaroonIssuerBase`.""" | ||
162 | 1840 | if context is None: | ||
163 | 1841 | # We're only verifying that the macaroon could be valid for some | ||
164 | 1842 | # context. | ||
165 | 1843 | return True | ||
166 | 1844 | return caveat_value == str(context.id) | ||
167 | 1845 | |||
168 | 1846 | def verifyOpenIDIdentifier(self, caveat_value, context, **kwargs): | ||
169 | 1847 | """Verify an lp.principal.openid-identifier caveat.""" | ||
170 | 1848 | user = kwargs.get("user") | ||
171 | 1849 | try: | ||
172 | 1850 | account = getUtility(IAccountSet).getByOpenIDIdentifier( | ||
173 | 1851 | caveat_value) | ||
174 | 1852 | except LookupError: | ||
175 | 1853 | return False | ||
176 | 1854 | return IPerson.providedBy(user) and user.account == account | ||
177 | 1855 | |||
178 | 1856 | def verifyExpires(self, caveat_value, context, **kwargs): | ||
179 | 1857 | """Verify an lp.expires caveat.""" | ||
180 | 1858 | try: | ||
181 | 1859 | expires = datetime.strptime( | ||
182 | 1860 | caveat_value, self._timestamp_format).replace(tzinfo=pytz.UTC) | ||
183 | 1861 | except ValueError: | ||
184 | 1862 | return False | ||
185 | 1863 | store = IStore(GitRepository) | ||
186 | 1864 | return get_transaction_timestamp(store) < expires | ||
187 | 1865 | |||
188 | 1866 | |||
189 | 1767 | def get_git_repository_privacy_filter(user, repository_class=GitRepository): | 1867 | def get_git_repository_privacy_filter(user, repository_class=GitRepository): |
190 | 1768 | public_filter = repository_class.information_type.is_in( | 1868 | public_filter = repository_class.information_type.is_in( |
191 | 1769 | PUBLIC_INFORMATION_TYPES) | 1869 | PUBLIC_INFORMATION_TYPES) |
192 | 1770 | 1870 | ||
193 | === modified file 'lib/lp/code/model/tests/test_gitrepository.py' | |||
194 | --- lib/lp/code/model/tests/test_gitrepository.py 2019-02-11 12:31:06 +0000 | |||
195 | +++ lib/lp/code/model/tests/test_gitrepository.py 2019-05-01 16:44:22 +0000 | |||
196 | @@ -17,12 +17,15 @@ | |||
197 | 17 | import json | 17 | import json |
198 | 18 | 18 | ||
199 | 19 | from bzrlib import urlutils | 19 | from bzrlib import urlutils |
200 | 20 | from fixtures import MockPatch | ||
201 | 20 | from lazr.lifecycle.event import ObjectModifiedEvent | 21 | from lazr.lifecycle.event import ObjectModifiedEvent |
202 | 22 | from pymacaroons import Macaroon | ||
203 | 21 | import pytz | 23 | import pytz |
204 | 22 | from sqlobject import SQLObjectNotFound | 24 | from sqlobject import SQLObjectNotFound |
205 | 23 | from storm.exceptions import LostObjectError | 25 | from storm.exceptions import LostObjectError |
206 | 24 | from storm.store import Store | 26 | from storm.store import Store |
207 | 25 | from testtools.matchers import ( | 27 | from testtools.matchers import ( |
208 | 28 | AnyMatch, | ||
209 | 26 | EndsWith, | 29 | EndsWith, |
210 | 27 | Equals, | 30 | Equals, |
211 | 28 | Is, | 31 | Is, |
212 | @@ -34,6 +37,7 @@ | |||
213 | 34 | ) | 37 | ) |
214 | 35 | import transaction | 38 | import transaction |
215 | 36 | from zope.component import getUtility | 39 | from zope.component import getUtility |
216 | 40 | from zope.publisher.xmlrpc import TestRequest | ||
217 | 37 | from zope.security.interfaces import Unauthorized | 41 | from zope.security.interfaces import Unauthorized |
218 | 38 | from zope.security.proxy import removeSecurityProxy | 42 | from zope.security.proxy import removeSecurityProxy |
219 | 39 | 43 | ||
220 | @@ -127,14 +131,22 @@ | |||
221 | 127 | ) | 131 | ) |
222 | 128 | from lp.registry.interfaces.personproduct import IPersonProductFactory | 132 | from lp.registry.interfaces.personproduct import IPersonProductFactory |
223 | 129 | from lp.registry.tests.test_accesspolicy import get_policies_for_artifact | 133 | from lp.registry.tests.test_accesspolicy import get_policies_for_artifact |
224 | 134 | from lp.services.authserver.xmlrpc import AuthServerAPIView | ||
225 | 130 | from lp.services.config import config | 135 | from lp.services.config import config |
226 | 131 | from lp.services.database.constants import UTC_NOW | 136 | from lp.services.database.constants import UTC_NOW |
227 | 132 | from lp.services.database.interfaces import IStore | 137 | from lp.services.database.interfaces import IStore |
228 | 133 | from lp.services.database.sqlbase import get_transaction_timestamp | 138 | from lp.services.database.sqlbase import get_transaction_timestamp |
229 | 139 | from lp.services.features.testing import FeatureFixture | ||
230 | 134 | from lp.services.job.interfaces.job import JobStatus | 140 | from lp.services.job.interfaces.job import JobStatus |
231 | 135 | from lp.services.job.model.job import Job | 141 | from lp.services.job.model.job import Job |
232 | 136 | from lp.services.job.runner import JobRunner | 142 | from lp.services.job.runner import JobRunner |
233 | 143 | from lp.services.macaroons.interfaces import IMacaroonIssuer | ||
234 | 144 | from lp.services.macaroons.testing import ( | ||
235 | 145 | find_caveats_by_name, | ||
236 | 146 | MacaroonTestMixin, | ||
237 | 147 | ) | ||
238 | 137 | from lp.services.mail import stub | 148 | from lp.services.mail import stub |
239 | 149 | from lp.services.openid.model.openididentifier import OpenIdIdentifier | ||
240 | 138 | from lp.services.propertycache import clear_property_cache | 150 | from lp.services.propertycache import clear_property_cache |
241 | 139 | from lp.services.utils import seconds_since_epoch | 151 | from lp.services.utils import seconds_since_epoch |
242 | 140 | from lp.services.webapp.authorization import check_permission | 152 | from lp.services.webapp.authorization import check_permission |
243 | @@ -163,6 +175,8 @@ | |||
244 | 163 | HasQueryCount, | 175 | HasQueryCount, |
245 | 164 | ) | 176 | ) |
246 | 165 | from lp.testing.pages import webservice_for_person | 177 | from lp.testing.pages import webservice_for_person |
247 | 178 | from lp.xmlrpc import faults | ||
248 | 179 | from lp.xmlrpc.interfaces import IPrivateApplication | ||
249 | 166 | 180 | ||
250 | 167 | 181 | ||
251 | 168 | class TestGitRepository(TestCaseWithFactory): | 182 | class TestGitRepository(TestCaseWithFactory): |
252 | @@ -3894,3 +3908,222 @@ | |||
253 | 3894 | "refs/heads/next": Equals(["push", "force-push"]), | 3908 | "refs/heads/next": Equals(["push", "force-push"]), |
254 | 3895 | "refs/other": Equals([]), | 3909 | "refs/other": Equals([]), |
255 | 3896 | })) | 3910 | })) |
256 | 3911 | |||
257 | 3912 | |||
258 | 3913 | class TestGitRepositoryMacaroonIssuer(MacaroonTestMixin, TestCaseWithFactory): | ||
259 | 3914 | """Test GitRepository macaroon issuing and verification.""" | ||
260 | 3915 | |||
261 | 3916 | layer = DatabaseFunctionalLayer | ||
262 | 3917 | |||
263 | 3918 | def setUp(self): | ||
264 | 3919 | super(TestGitRepositoryMacaroonIssuer, self).setUp() | ||
265 | 3920 | self.pushConfig( | ||
266 | 3921 | "launchpad", internal_macaroon_secret_key="some-secret") | ||
267 | 3922 | |||
268 | 3923 | def test_issueMacaroon_refuses_branch(self): | ||
269 | 3924 | branch = self.factory.makeAnyBranch() | ||
270 | 3925 | issuer = getUtility(IMacaroonIssuer, "git-repository") | ||
271 | 3926 | self.assertRaises( | ||
272 | 3927 | ValueError, removeSecurityProxy(issuer).issueMacaroon, | ||
273 | 3928 | branch, user=branch.owner) | ||
274 | 3929 | |||
275 | 3930 | def test_issueMacaroon_good(self): | ||
276 | 3931 | repository = self.factory.makeGitRepository() | ||
277 | 3932 | issuer = getUtility(IMacaroonIssuer, "git-repository") | ||
278 | 3933 | naked_account = removeSecurityProxy(repository.owner).account | ||
279 | 3934 | identifier = naked_account.openid_identifiers.any().identifier | ||
280 | 3935 | macaroon = removeSecurityProxy(issuer).issueMacaroon( | ||
281 | 3936 | repository, user=repository.owner) | ||
282 | 3937 | now = get_transaction_timestamp(Store.of(repository)) | ||
283 | 3938 | expires = now + timedelta(days=7) | ||
284 | 3939 | self.assertThat(macaroon, MatchesStructure( | ||
285 | 3940 | location=Equals(config.vhost.mainsite.hostname), | ||
286 | 3941 | identifier=Equals("git-repository"), | ||
287 | 3942 | caveats=MatchesListwise([ | ||
288 | 3943 | MatchesStructure.byEquality( | ||
289 | 3944 | caveat_id="lp.git-repository %s" % repository.id), | ||
290 | 3945 | MatchesStructure.byEquality( | ||
291 | 3946 | caveat_id=( | ||
292 | 3947 | "lp.principal.openid-identifier %s" % identifier)), | ||
293 | 3948 | MatchesStructure.byEquality( | ||
294 | 3949 | caveat_id="lp.expires %s" % ( | ||
295 | 3950 | expires.strftime("%Y-%m-%dT%H:%M:%S.%f"))), | ||
296 | 3951 | ]))) | ||
297 | 3952 | |||
298 | 3953 | def test_issueMacaroon_expiry_feature_flag(self): | ||
299 | 3954 | self.useFixture(FeatureFixture( | ||
300 | 3955 | {"code.git.access_token_expiry": "3600"})) | ||
301 | 3956 | repository = self.factory.makeGitRepository() | ||
302 | 3957 | issuer = getUtility(IMacaroonIssuer, "git-repository") | ||
303 | 3958 | macaroon = removeSecurityProxy(issuer).issueMacaroon( | ||
304 | 3959 | repository, user=repository.owner) | ||
305 | 3960 | now = get_transaction_timestamp(Store.of(repository)) | ||
306 | 3961 | expires = now + timedelta(hours=1) | ||
307 | 3962 | self.assertThat(macaroon, MatchesStructure( | ||
308 | 3963 | caveats=AnyMatch( | ||
309 | 3964 | MatchesStructure.byEquality( | ||
310 | 3965 | caveat_id="lp.expires %s" % ( | ||
311 | 3966 | expires.strftime("%Y-%m-%dT%H:%M:%S.%f")))))) | ||
312 | 3967 | |||
313 | 3968 | def test_issueMacaroon_no_user(self): | ||
314 | 3969 | repository = self.factory.makeGitRepository() | ||
315 | 3970 | issuer = getUtility(IMacaroonIssuer, "git-repository") | ||
316 | 3971 | self.assertRaises( | ||
317 | 3972 | Unauthorized, | ||
318 | 3973 | removeSecurityProxy(issuer).issueMacaroon, repository) | ||
319 | 3974 | |||
320 | 3975 | def test_issueMacaroon_not_via_authserver(self): | ||
321 | 3976 | repository = self.factory.makeGitRepository() | ||
322 | 3977 | private_root = getUtility(IPrivateApplication) | ||
323 | 3978 | authserver = AuthServerAPIView(private_root.authserver, TestRequest()) | ||
324 | 3979 | self.assertEqual( | ||
325 | 3980 | faults.PermissionDenied(), | ||
326 | 3981 | authserver.issueMacaroon( | ||
327 | 3982 | "git-repository", "GitRepository", repository)) | ||
328 | 3983 | |||
329 | 3984 | def test_verifyMacaroon_good(self): | ||
330 | 3985 | repository = self.factory.makeGitRepository() | ||
331 | 3986 | issuer = getUtility(IMacaroonIssuer, "git-repository") | ||
332 | 3987 | macaroon = removeSecurityProxy(issuer).issueMacaroon( | ||
333 | 3988 | repository, user=repository.owner) | ||
334 | 3989 | self.assertMacaroonVerifies( | ||
335 | 3990 | issuer, macaroon, repository, user=repository.owner) | ||
336 | 3991 | |||
337 | 3992 | def test_verifyMacaroon_wrong_location(self): | ||
338 | 3993 | repository = self.factory.makeGitRepository() | ||
339 | 3994 | issuer = getUtility(IMacaroonIssuer, "git-repository") | ||
340 | 3995 | macaroon = Macaroon( | ||
341 | 3996 | location="another-location", | ||
342 | 3997 | key=removeSecurityProxy(issuer)._root_secret) | ||
343 | 3998 | self.assertMacaroonDoesNotVerify( | ||
344 | 3999 | ["Macaroon has unknown location 'another-location'."], | ||
345 | 4000 | issuer, macaroon, repository, user=repository.owner) | ||
346 | 4001 | |||
347 | 4002 | def test_verifyMacaroon_wrong_key(self): | ||
348 | 4003 | repository = self.factory.makeGitRepository() | ||
349 | 4004 | issuer = getUtility(IMacaroonIssuer, "git-repository") | ||
350 | 4005 | macaroon = Macaroon( | ||
351 | 4006 | location=config.vhost.mainsite.hostname, key="another-secret") | ||
352 | 4007 | self.assertMacaroonDoesNotVerify( | ||
353 | 4008 | ["Signatures do not match"], | ||
354 | 4009 | issuer, macaroon, repository, user=repository.owner) | ||
355 | 4010 | |||
356 | 4011 | def test_verifyMacaroon_wrong_repository(self): | ||
357 | 4012 | repository = self.factory.makeGitRepository() | ||
358 | 4013 | issuer = getUtility(IMacaroonIssuer, "git-repository") | ||
359 | 4014 | macaroon = removeSecurityProxy(issuer).issueMacaroon( | ||
360 | 4015 | repository, user=repository.owner) | ||
361 | 4016 | self.assertMacaroonDoesNotVerify( | ||
362 | 4017 | ["Caveat check for 'lp.git-repository %s' failed." % | ||
363 | 4018 | repository.id], | ||
364 | 4019 | issuer, macaroon, self.factory.makeGitRepository(), | ||
365 | 4020 | user=repository.owner) | ||
366 | 4021 | |||
367 | 4022 | def test_verifyMacaroon_multiple_repository_caveats(self): | ||
368 | 4023 | repository = self.factory.makeGitRepository() | ||
369 | 4024 | issuer = getUtility(IMacaroonIssuer, "git-repository") | ||
370 | 4025 | macaroon = removeSecurityProxy(issuer).issueMacaroon( | ||
371 | 4026 | repository, user=repository.owner) | ||
372 | 4027 | macaroon.add_first_party_caveat("lp.git-repository another") | ||
373 | 4028 | self.assertMacaroonDoesNotVerify( | ||
374 | 4029 | ["Multiple 'lp.git-repository' caveats are not allowed."], | ||
375 | 4030 | issuer, macaroon, repository, user=repository.owner) | ||
376 | 4031 | |||
377 | 4032 | def test_verifyMacaroon_wrong_user(self): | ||
378 | 4033 | repository = self.factory.makeGitRepository() | ||
379 | 4034 | issuer = getUtility(IMacaroonIssuer, "git-repository") | ||
380 | 4035 | naked_account = removeSecurityProxy(repository.owner).account | ||
381 | 4036 | identifier = naked_account.openid_identifiers.any().identifier | ||
382 | 4037 | macaroon = removeSecurityProxy(issuer).issueMacaroon( | ||
383 | 4038 | repository, user=repository.owner) | ||
384 | 4039 | self.assertMacaroonDoesNotVerify( | ||
385 | 4040 | ["Caveat check for 'lp.principal.openid-identifier %s' failed." % | ||
386 | 4041 | identifier], | ||
387 | 4042 | issuer, macaroon, repository, user=self.factory.makePerson()) | ||
388 | 4043 | |||
389 | 4044 | def test_verifyMacaroon_closed_account(self): | ||
390 | 4045 | # A closed account no longer has an OpenID identifier, so the | ||
391 | 4046 | # corresponding caveat doesn't match. | ||
392 | 4047 | repository = self.factory.makeGitRepository() | ||
393 | 4048 | owner = repository.owner | ||
394 | 4049 | issuer = getUtility(IMacaroonIssuer, "git-repository") | ||
395 | 4050 | naked_account = removeSecurityProxy(owner).account | ||
396 | 4051 | identifier = naked_account.openid_identifiers.any().identifier | ||
397 | 4052 | macaroon = removeSecurityProxy(issuer).issueMacaroon( | ||
398 | 4053 | repository, user=owner) | ||
399 | 4054 | IStore(OpenIdIdentifier).find( | ||
400 | 4055 | OpenIdIdentifier, | ||
401 | 4056 | OpenIdIdentifier.account_id == owner.account.id).remove() | ||
402 | 4057 | self.assertMacaroonDoesNotVerify( | ||
403 | 4058 | ["Caveat check for 'lp.principal.openid-identifier %s' failed." % | ||
404 | 4059 | identifier], | ||
405 | 4060 | issuer, macaroon, repository, user=owner) | ||
406 | 4061 | |||
407 | 4062 | def test_verifyMacaroon_multiple_openid_identifier_caveats(self): | ||
408 | 4063 | repository = self.factory.makeGitRepository() | ||
409 | 4064 | issuer = getUtility(IMacaroonIssuer, "git-repository") | ||
410 | 4065 | macaroon = removeSecurityProxy(issuer).issueMacaroon( | ||
411 | 4066 | repository, user=repository.owner) | ||
412 | 4067 | macaroon.add_first_party_caveat( | ||
413 | 4068 | "lp.principal.openid-identifier another") | ||
414 | 4069 | self.assertMacaroonDoesNotVerify( | ||
415 | 4070 | ["Multiple 'lp.principal.openid-identifier' caveats are not " | ||
416 | 4071 | "allowed."], | ||
417 | 4072 | issuer, macaroon, repository, user=repository.owner) | ||
418 | 4073 | |||
419 | 4074 | def test_verifyMacaroon_expired(self): | ||
420 | 4075 | repository = self.factory.makeGitRepository() | ||
421 | 4076 | issuer = getUtility(IMacaroonIssuer, "git-repository") | ||
422 | 4077 | macaroon = removeSecurityProxy(issuer).issueMacaroon( | ||
423 | 4078 | repository, user=repository.owner) | ||
424 | 4079 | now = get_transaction_timestamp(Store.of(repository)) | ||
425 | 4080 | self.useFixture(MockPatch( | ||
426 | 4081 | "lp.code.model.gitrepository.get_transaction_timestamp", | ||
427 | 4082 | lambda _: now + timedelta(days=8))) | ||
428 | 4083 | self.assertMacaroonDoesNotVerify( | ||
429 | 4084 | ["Caveat check for '%s' failed." % | ||
430 | 4085 | find_caveats_by_name(macaroon, "lp.expires")[0].caveat_id], | ||
431 | 4086 | issuer, macaroon, repository, user=repository.owner) | ||
432 | 4087 | |||
433 | 4088 | def test_verifyMacaroon_multiple_expires_caveats(self): | ||
434 | 4089 | # If somebody attaches another expires caveat to the macaroon, | ||
435 | 4090 | # that's OK; we just take the strictest. | ||
436 | 4091 | repository = self.factory.makeGitRepository() | ||
437 | 4092 | issuer = getUtility(IMacaroonIssuer, "git-repository") | ||
438 | 4093 | macaroon1 = removeSecurityProxy(issuer).issueMacaroon( | ||
439 | 4094 | repository, user=repository.owner) | ||
440 | 4095 | macaroon2 = removeSecurityProxy(issuer).issueMacaroon( | ||
441 | 4096 | repository, user=repository.owner) | ||
442 | 4097 | now = get_transaction_timestamp(Store.of(repository)) | ||
443 | 4098 | expires1 = now + timedelta(days=1) | ||
444 | 4099 | expires2 = now + timedelta(days=14) | ||
445 | 4100 | macaroon1.add_first_party_caveat( | ||
446 | 4101 | "lp.expires " + expires1.strftime("%Y-%m-%dT%H:%M:%S.%f")) | ||
447 | 4102 | macaroon2.add_first_party_caveat( | ||
448 | 4103 | "lp.expires " + expires2.strftime("%Y-%m-%dT%H:%M:%S.%f")) | ||
449 | 4104 | self.assertMacaroonVerifies( | ||
450 | 4105 | issuer, macaroon1, repository, user=repository.owner) | ||
451 | 4106 | self.assertMacaroonVerifies( | ||
452 | 4107 | issuer, macaroon2, repository, user=repository.owner) | ||
453 | 4108 | with MockPatch( | ||
454 | 4109 | "lp.code.model.gitrepository.get_transaction_timestamp", | ||
455 | 4110 | lambda _: now + timedelta(days=4)): | ||
456 | 4111 | self.assertMacaroonDoesNotVerify( | ||
457 | 4112 | ["Caveat check for '%s' failed." % | ||
458 | 4113 | find_caveats_by_name(macaroon1, "lp.expires")[1].caveat_id], | ||
459 | 4114 | issuer, macaroon1, repository, user=repository.owner) | ||
460 | 4115 | self.assertMacaroonVerifies( | ||
461 | 4116 | issuer, macaroon2, repository, user=repository.owner) | ||
462 | 4117 | with MockPatch( | ||
463 | 4118 | "lp.code.model.gitrepository.get_transaction_timestamp", | ||
464 | 4119 | lambda _: now + timedelta(days=8)): | ||
465 | 4120 | self.assertMacaroonDoesNotVerify( | ||
466 | 4121 | ["Caveat check for '%s' failed." % | ||
467 | 4122 | find_caveats_by_name(macaroon1, "lp.expires")[0].caveat_id, | ||
468 | 4123 | "Caveat check for '%s' failed." % | ||
469 | 4124 | find_caveats_by_name(macaroon1, "lp.expires")[1].caveat_id], | ||
470 | 4125 | issuer, macaroon1, repository, user=repository.owner) | ||
471 | 4126 | self.assertMacaroonDoesNotVerify( | ||
472 | 4127 | ["Caveat check for '%s' failed." % | ||
473 | 4128 | find_caveats_by_name(macaroon2, "lp.expires")[0].caveat_id], | ||
474 | 4129 | issuer, macaroon2, repository, user=repository.owner) | ||
475 | 3897 | 4130 | ||
476 | === modified file 'lib/lp/services/authserver/tests/test_authserver.py' | |||
477 | --- lib/lp/services/authserver/tests/test_authserver.py 2019-04-25 09:55:35 +0000 | |||
478 | +++ lib/lp/services/authserver/tests/test_authserver.py 2019-05-01 16:44:22 +0000 | |||
479 | @@ -89,19 +89,19 @@ | |||
480 | 89 | issuable_via_authserver = True | 89 | issuable_via_authserver = True |
481 | 90 | _root_secret = 'test' | 90 | _root_secret = 'test' |
482 | 91 | 91 | ||
484 | 92 | def checkIssuingContext(self, context): | 92 | def checkIssuingContext(self, context, **kwargs): |
485 | 93 | """See `MacaroonIssuerBase`.""" | 93 | """See `MacaroonIssuerBase`.""" |
486 | 94 | if not ILibraryFileAlias.providedBy(context): | 94 | if not ILibraryFileAlias.providedBy(context): |
487 | 95 | raise BadMacaroonContext(context) | 95 | raise BadMacaroonContext(context) |
488 | 96 | return context.id | 96 | return context.id |
489 | 97 | 97 | ||
491 | 98 | def checkVerificationContext(self, context): | 98 | def checkVerificationContext(self, context, **kwargs): |
492 | 99 | """See `IMacaroonIssuerBase`.""" | 99 | """See `IMacaroonIssuerBase`.""" |
493 | 100 | if not ILibraryFileAlias.providedBy(context): | 100 | if not ILibraryFileAlias.providedBy(context): |
494 | 101 | raise BadMacaroonContext(context) | 101 | raise BadMacaroonContext(context) |
495 | 102 | return context | 102 | return context |
496 | 103 | 103 | ||
498 | 104 | def verifyPrimaryCaveat(self, caveat_value, context): | 104 | def verifyPrimaryCaveat(self, caveat_value, context, **kwargs): |
499 | 105 | """See `MacaroonIssuerBase`.""" | 105 | """See `MacaroonIssuerBase`.""" |
500 | 106 | return caveat_value == str(context.id) | 106 | return caveat_value == str(context.id) |
501 | 107 | 107 | ||
502 | 108 | 108 | ||
503 | === modified file 'lib/lp/services/macaroons/interfaces.py' | |||
504 | --- lib/lp/services/macaroons/interfaces.py 2019-04-25 22:10:36 +0000 | |||
505 | +++ lib/lp/services/macaroons/interfaces.py 2019-05-01 16:44:22 +0000 | |||
506 | @@ -31,7 +31,8 @@ | |||
507 | 31 | issuable_via_authserver = Bool( | 31 | issuable_via_authserver = Bool( |
508 | 32 | "Does this issuer allow issuing macaroons via the authserver?") | 32 | "Does this issuer allow issuing macaroons via the authserver?") |
509 | 33 | 33 | ||
511 | 34 | def verifyMacaroon(macaroon, context, require_context=True, errors=None): | 34 | def verifyMacaroon(macaroon, context, require_context=True, errors=None, |
512 | 35 | **kwargs): | ||
513 | 35 | """Verify that `macaroon` is valid for `context`. | 36 | """Verify that `macaroon` is valid for `context`. |
514 | 36 | 37 | ||
515 | 37 | :param macaroon: A `Macaroon`. | 38 | :param macaroon: A `Macaroon`. |
516 | @@ -43,6 +44,8 @@ | |||
517 | 43 | authentication/authorisation API. | 44 | authentication/authorisation API. |
518 | 44 | :param errors: If non-None, any verification error messages will be | 45 | :param errors: If non-None, any verification error messages will be |
519 | 45 | appended to this list. | 46 | appended to this list. |
520 | 47 | :param kwargs: Additional arguments that issuers may require to | ||
521 | 48 | verify a macaroon. | ||
522 | 46 | :return: True if `macaroon` is valid for `context`, otherwise False. | 49 | :return: True if `macaroon` is valid for `context`, otherwise False. |
523 | 47 | """ | 50 | """ |
524 | 48 | 51 | ||
525 | @@ -50,11 +53,13 @@ | |||
526 | 50 | class IMacaroonIssuer(IMacaroonIssuerPublic): | 53 | class IMacaroonIssuer(IMacaroonIssuerPublic): |
527 | 51 | """Interface to a policy for issuing and verifying macaroons.""" | 54 | """Interface to a policy for issuing and verifying macaroons.""" |
528 | 52 | 55 | ||
530 | 53 | def issueMacaroon(context): | 56 | def issueMacaroon(context, **kwargs): |
531 | 54 | """Issue a macaroon for `context`. | 57 | """Issue a macaroon for `context`. |
532 | 55 | 58 | ||
533 | 56 | :param context: The context that the returned macaroon should relate | 59 | :param context: The context that the returned macaroon should relate |
534 | 57 | to. | 60 | to. |
535 | 61 | :param kwargs: Additional arguments that issuers may require to | ||
536 | 62 | issue a macaroon. | ||
537 | 58 | :raises BadMacaroonContext: if the context is unsuitable. | 63 | :raises BadMacaroonContext: if the context is unsuitable. |
538 | 59 | :return: A macaroon. | 64 | :return: A macaroon. |
539 | 60 | """ | 65 | """ |
540 | 61 | 66 | ||
541 | === modified file 'lib/lp/services/macaroons/model.py' | |||
542 | --- lib/lp/services/macaroons/model.py 2019-04-25 22:10:36 +0000 | |||
543 | +++ lib/lp/services/macaroons/model.py 2019-05-01 16:44:22 +0000 | |||
544 | @@ -25,6 +25,31 @@ | |||
545 | 25 | 25 | ||
546 | 26 | issuable_via_authserver = False | 26 | issuable_via_authserver = False |
547 | 27 | 27 | ||
548 | 28 | # A mapping of caveat names to "checker" callables that verify the | ||
549 | 29 | # corresponding caveat text. The signature of each checker is | ||
550 | 30 | # (caveat_value, context, **kwargs) -> bool, where caveat_value is the | ||
551 | 31 | # text of the caveat with the caveat name removed, context is the | ||
552 | 32 | # issuer-specific context to check, and kwargs is any other keyword | ||
553 | 33 | # arguments that were given to verifyMacaroon; it should return True if | ||
554 | 34 | # the caveat is allowed, otherwise False. | ||
555 | 35 | # | ||
556 | 36 | # The context passed in may be None, in which case the checker may | ||
557 | 37 | # choose to only verify that the caveat could be valid for some context, | ||
558 | 38 | # or may simply return False if this is unsupported. This is useful for | ||
559 | 39 | # issuers that support APIs with separate authentication and | ||
560 | 40 | # authorisation phases. | ||
561 | 41 | # | ||
562 | 42 | # The "primary context caveat" added to all macaroons issued by this | ||
563 | 43 | # base class does not need to be listed here; it is handled by the | ||
564 | 44 | # verifyContextCaveat method. | ||
565 | 45 | checkers = {} | ||
566 | 46 | |||
567 | 47 | # Caveat names in this set may appear more than once (in which case they | ||
568 | 48 | # have the usual subtractive semantics, so the union of all the | ||
569 | 49 | # constraints they express applies). Any other caveats may only appear | ||
570 | 50 | # once. | ||
571 | 51 | allow_multiple = set() | ||
572 | 52 | |||
573 | 28 | @property | 53 | @property |
574 | 29 | def identifier(self): | 54 | def identifier(self): |
575 | 30 | """An identifying name for this issuer.""" | 55 | """An identifying name for this issuer.""" |
576 | @@ -43,7 +68,7 @@ | |||
577 | 43 | "launchpad.internal_macaroon_secret_key not configured.") | 68 | "launchpad.internal_macaroon_secret_key not configured.") |
578 | 44 | return secret | 69 | return secret |
579 | 45 | 70 | ||
581 | 46 | def checkIssuingContext(self, context): | 71 | def checkIssuingContext(self, context, **kwargs): |
582 | 47 | """Check that the issuing context is suitable. | 72 | """Check that the issuing context is suitable. |
583 | 48 | 73 | ||
584 | 49 | Concrete implementations may implement this method to check that the | 74 | Concrete implementations may implement this method to check that the |
585 | @@ -52,18 +77,16 @@ | |||
586 | 52 | was passed in or an adapted one. | 77 | was passed in or an adapted one. |
587 | 53 | 78 | ||
588 | 54 | :param context: The context to check. | 79 | :param context: The context to check. |
589 | 80 | :param kwargs: Additional arguments that issuers may require to | ||
590 | 81 | issue a macaroon. | ||
591 | 55 | :raises BadMacaroonContext: if the context is unsuitable. | 82 | :raises BadMacaroonContext: if the context is unsuitable. |
592 | 56 | :return: The context to use to create the primary caveat. | 83 | :return: The context to use to create the primary caveat. |
593 | 57 | """ | 84 | """ |
594 | 58 | return context | 85 | return context |
595 | 59 | 86 | ||
603 | 60 | def issueMacaroon(self, context): | 87 | def issueMacaroon(self, context, **kwargs): |
604 | 61 | """See `IMacaroonIssuer`. | 88 | """See `IMacaroonIssuer`.""" |
605 | 62 | 89 | context = self.checkIssuingContext(context, **kwargs) | |
599 | 63 | Concrete implementations should normally wrap this with some | ||
600 | 64 | additional checks of and/or changes to the context. | ||
601 | 65 | """ | ||
602 | 66 | context = self.checkIssuingContext(context) | ||
606 | 67 | macaroon = Macaroon( | 90 | macaroon = Macaroon( |
607 | 68 | location=config.vhost.mainsite.hostname, | 91 | location=config.vhost.mainsite.hostname, |
608 | 69 | identifier=self.identifier, key=self._root_secret) | 92 | identifier=self.identifier, key=self._root_secret) |
609 | @@ -71,7 +94,7 @@ | |||
610 | 71 | "%s %s" % (self._primary_caveat_name, context)) | 94 | "%s %s" % (self._primary_caveat_name, context)) |
611 | 72 | return macaroon | 95 | return macaroon |
612 | 73 | 96 | ||
614 | 74 | def checkVerificationContext(self, context): | 97 | def checkVerificationContext(self, context, **kwargs): |
615 | 75 | """Check that the verification context is suitable. | 98 | """Check that the verification context is suitable. |
616 | 76 | 99 | ||
617 | 77 | Concrete implementations may implement this method to check that the | 100 | Concrete implementations may implement this method to check that the |
618 | @@ -80,23 +103,27 @@ | |||
619 | 80 | context that was passed in or an adapted one. | 103 | context that was passed in or an adapted one. |
620 | 81 | 104 | ||
621 | 82 | :param context: The context to check. | 105 | :param context: The context to check. |
622 | 106 | :param kwargs: Additional arguments that issuers may require to | ||
623 | 107 | verify a macaroon. | ||
624 | 83 | :raises BadMacaroonContext: if the context is unsuitable. | 108 | :raises BadMacaroonContext: if the context is unsuitable. |
625 | 84 | :return: The context to pass to individual caveat checkers. | 109 | :return: The context to pass to individual caveat checkers. |
626 | 85 | """ | 110 | """ |
627 | 86 | return context | 111 | return context |
628 | 87 | 112 | ||
630 | 88 | def verifyPrimaryCaveat(self, caveat_value, context): | 113 | def verifyPrimaryCaveat(self, caveat_value, context, **kwargs): |
631 | 89 | """Verify the primary context caveat on one of this issuer's macaroons. | 114 | """Verify the primary context caveat on one of this issuer's macaroons. |
632 | 90 | 115 | ||
635 | 91 | :param caveat_value: The text of the caveat, with this issuer's | 116 | :param caveat_value: The text of the caveat with the caveat name |
636 | 92 | prefix removed. | 117 | removed. |
637 | 93 | :param context: The context to check. | 118 | :param context: The context to check. |
638 | 119 | :param kwargs: Additional arguments that issuers may require to | ||
639 | 120 | verify a macaroon. | ||
640 | 94 | :return: True if this caveat is allowed, otherwise False. | 121 | :return: True if this caveat is allowed, otherwise False. |
641 | 95 | """ | 122 | """ |
642 | 96 | raise NotImplementedError | 123 | raise NotImplementedError |
643 | 97 | 124 | ||
644 | 98 | def verifyMacaroon(self, macaroon, context, require_context=True, | 125 | def verifyMacaroon(self, macaroon, context, require_context=True, |
646 | 99 | errors=None): | 126 | errors=None, **kwargs): |
647 | 100 | """See `IMacaroonIssuer`.""" | 127 | """See `IMacaroonIssuer`.""" |
648 | 101 | if macaroon.location != config.vhost.mainsite.hostname: | 128 | if macaroon.location != config.vhost.mainsite.hostname: |
649 | 102 | if errors is not None: | 129 | if errors is not None: |
650 | @@ -115,6 +142,7 @@ | |||
651 | 115 | if errors is not None: | 142 | if errors is not None: |
652 | 116 | errors.append(str(e)) | 143 | errors.append(str(e)) |
653 | 117 | return False | 144 | return False |
654 | 145 | seen = set() | ||
655 | 118 | 146 | ||
656 | 119 | def verify(caveat): | 147 | def verify(caveat): |
657 | 120 | try: | 148 | try: |
658 | @@ -123,16 +151,22 @@ | |||
659 | 123 | if errors is not None: | 151 | if errors is not None: |
660 | 124 | errors.append("Cannot parse caveat '%s'." % caveat) | 152 | errors.append("Cannot parse caveat '%s'." % caveat) |
661 | 125 | return False | 153 | return False |
662 | 154 | if caveat_name not in self.allow_multiple and caveat_name in seen: | ||
663 | 155 | if errors is not None: | ||
664 | 156 | errors.append( | ||
665 | 157 | "Multiple '%s' caveats are not allowed." % caveat_name) | ||
666 | 158 | return False | ||
667 | 159 | seen.add(caveat_name) | ||
668 | 126 | if caveat_name == self._primary_caveat_name: | 160 | if caveat_name == self._primary_caveat_name: |
669 | 127 | checker = self.verifyPrimaryCaveat | 161 | checker = self.verifyPrimaryCaveat |
670 | 128 | else: | 162 | else: |
678 | 129 | # XXX cjwatson 2019-04-09: For now we just fail closed if | 163 | checker = self.checkers.get(caveat_name) |
679 | 130 | # there are any other caveats, which is good enough for | 164 | if checker is None: |
680 | 131 | # internal use. | 165 | if errors is not None: |
681 | 132 | if errors is not None: | 166 | errors.append( |
682 | 133 | errors.append("Unhandled caveat name '%s'." % caveat_name) | 167 | "Unhandled caveat name '%s'." % caveat_name) |
683 | 134 | return False | 168 | return False |
684 | 135 | if not checker(caveat_value, context): | 169 | if not checker(caveat_value, context, **kwargs): |
685 | 136 | if errors is not None: | 170 | if errors is not None: |
686 | 137 | errors.append("Caveat check for '%s' failed." % caveat) | 171 | errors.append("Caveat check for '%s' failed." % caveat) |
687 | 138 | return False | 172 | return False |
688 | 139 | 173 | ||
689 | === modified file 'lib/lp/snappy/model/snapbuild.py' | |||
690 | --- lib/lp/snappy/model/snapbuild.py 2019-04-25 09:34:40 +0000 | |||
691 | +++ lib/lp/snappy/model/snapbuild.py 2019-05-01 16:44:22 +0000 | |||
692 | @@ -599,7 +599,7 @@ | |||
693 | 599 | identifier = "snap-build" | 599 | identifier = "snap-build" |
694 | 600 | issuable_via_authserver = True | 600 | issuable_via_authserver = True |
695 | 601 | 601 | ||
697 | 602 | def checkIssuingContext(self, context): | 602 | def checkIssuingContext(self, context, **kwargs): |
698 | 603 | """See `MacaroonIssuerBase`. | 603 | """See `MacaroonIssuerBase`. |
699 | 604 | 604 | ||
700 | 605 | For issuing, the context is an `ISnapBuild`. | 605 | For issuing, the context is an `ISnapBuild`. |
701 | @@ -611,13 +611,13 @@ | |||
702 | 611 | context, "Refusing to issue macaroon for public build.") | 611 | context, "Refusing to issue macaroon for public build.") |
703 | 612 | return removeSecurityProxy(context).id | 612 | return removeSecurityProxy(context).id |
704 | 613 | 613 | ||
706 | 614 | def checkVerificationContext(self, context): | 614 | def checkVerificationContext(self, context, **kwargs): |
707 | 615 | """See `MacaroonIssuerBase`.""" | 615 | """See `MacaroonIssuerBase`.""" |
708 | 616 | if not IGitRepository.providedBy(context): | 616 | if not IGitRepository.providedBy(context): |
709 | 617 | raise BadMacaroonContext(context) | 617 | raise BadMacaroonContext(context) |
710 | 618 | return context | 618 | return context |
711 | 619 | 619 | ||
713 | 620 | def verifyPrimaryCaveat(self, caveat_value, context): | 620 | def verifyPrimaryCaveat(self, caveat_value, context, **kwargs): |
714 | 621 | """See `MacaroonIssuerBase`. | 621 | """See `MacaroonIssuerBase`. |
715 | 622 | 622 | ||
716 | 623 | For verification, the context is an `IGitRepository`. We check that | 623 | For verification, the context is an `IGitRepository`. We check that |
717 | 624 | 624 | ||
718 | === modified file 'lib/lp/soyuz/model/binarypackagebuild.py' | |||
719 | --- lib/lp/soyuz/model/binarypackagebuild.py 2019-04-24 12:50:20 +0000 | |||
720 | +++ lib/lp/soyuz/model/binarypackagebuild.py 2019-05-01 16:44:22 +0000 | |||
721 | @@ -1387,7 +1387,7 @@ | |||
722 | 1387 | # the named build directly. | 1387 | # the named build directly. |
723 | 1388 | return "lp.principal.binary-package-build" | 1388 | return "lp.principal.binary-package-build" |
724 | 1389 | 1389 | ||
726 | 1390 | def checkIssuingContext(self, context): | 1390 | def checkIssuingContext(self, context, **kwargs): |
727 | 1391 | """See `MacaroonIssuerBase`. | 1391 | """See `MacaroonIssuerBase`. |
728 | 1392 | 1392 | ||
729 | 1393 | For issuing, the context is an `IBinaryPackageBuild`. | 1393 | For issuing, the context is an `IBinaryPackageBuild`. |
730 | @@ -1397,13 +1397,13 @@ | |||
731 | 1397 | context, "Refusing to issue macaroon for public build.") | 1397 | context, "Refusing to issue macaroon for public build.") |
732 | 1398 | return removeSecurityProxy(context).id | 1398 | return removeSecurityProxy(context).id |
733 | 1399 | 1399 | ||
735 | 1400 | def checkVerificationContext(self, context): | 1400 | def checkVerificationContext(self, context, **kwargs): |
736 | 1401 | """See `MacaroonIssuerBase`.""" | 1401 | """See `MacaroonIssuerBase`.""" |
737 | 1402 | if not ILibraryFileAlias.providedBy(context): | 1402 | if not ILibraryFileAlias.providedBy(context): |
738 | 1403 | raise BadMacaroonContext(context) | 1403 | raise BadMacaroonContext(context) |
739 | 1404 | return context | 1404 | return context |
740 | 1405 | 1405 | ||
742 | 1406 | def verifyPrimaryCaveat(self, caveat_value, context): | 1406 | def verifyPrimaryCaveat(self, caveat_value, context, **kwargs): |
743 | 1407 | """See `MacaroonIssuerBase`. | 1407 | """See `MacaroonIssuerBase`. |
744 | 1408 | 1408 | ||
745 | 1409 | For verification, the context is an `ILibraryFileAlias`. We check | 1409 | For verification, the context is an `ILibraryFileAlias`. We check |