Merge ~cjwatson/launchpad:access-token-rename-context-to-target into launchpad:master
- Git
- lp:~cjwatson/launchpad
- access-token-rename-context-to-target
- Merge into master
Proposed by
Colin Watson
Status: | Merged |
---|---|
Approved by: | Colin Watson |
Approved revision: | 327282bbb1d3367bbe15b05eefe6d0675507b853 |
Merge reported by: | Otto Co-Pilot |
Merged at revision: | not available |
Proposed branch: | ~cjwatson/launchpad:access-token-rename-context-to-target |
Merge into: | launchpad:master |
Diff against target: |
262 lines (+46/-47) 7 files modified
lib/lp/security.py (+4/-4) lib/lp/services/auth/interfaces.py (+5/-5) lib/lp/services/auth/model.py (+11/-11) lib/lp/services/auth/tests/test_model.py (+17/-18) lib/lp/services/webapp/tests/test_servers.py (+4/-4) lib/lp/services/webservice/configuration.py (+1/-1) lib/lp/testing/factory.py (+4/-4) |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Cristian Gonzalez (community) | Approve | ||
Review via email: mp+410144@code.launchpad.net |
Commit message
Rename AccessToken.context to AccessToken.target
Description of the change
Exporting a "context" attribute over the webservice turns out to be awkward, because it clashes with internals of lazr.restful. Let's just rename this now while it isn't too inconvenient to do so.
To post a comment you must log in.
Preview Diff
[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1 | diff --git a/lib/lp/security.py b/lib/lp/security.py | |||
2 | index 02f2816..0c871fc 100644 | |||
3 | --- a/lib/lp/security.py | |||
4 | +++ b/lib/lp/security.py | |||
5 | @@ -505,11 +505,11 @@ class EditAccessToken(AuthorizationBase): | |||
6 | 505 | if user.inTeam(self.obj.owner): | 505 | if user.inTeam(self.obj.owner): |
7 | 506 | return True | 506 | return True |
8 | 507 | # Being able to edit the token doesn't allow extracting the secret, | 507 | # Being able to edit the token doesn't allow extracting the secret, |
12 | 508 | # so it's OK to allow the owner of the context to do so too. This | 508 | # so it's OK to allow the owner of the target to do so too. This |
13 | 509 | # allows context owners to exercise some control over access to | 509 | # allows target owners to exercise some control over access to their |
14 | 510 | # their object. | 510 | # object. |
15 | 511 | adapter = queryAdapter( | 511 | adapter = queryAdapter( |
17 | 512 | self.obj.context, IAuthorization, 'launchpad.Edit') | 512 | self.obj.target, IAuthorization, 'launchpad.Edit') |
18 | 513 | if adapter is not None and adapter.checkAuthenticated(user): | 513 | if adapter is not None and adapter.checkAuthenticated(user): |
19 | 514 | return True | 514 | return True |
20 | 515 | return False | 515 | return False |
21 | diff --git a/lib/lp/services/auth/interfaces.py b/lib/lp/services/auth/interfaces.py | |||
22 | index 9394cec..b8e3e7a 100644 | |||
23 | --- a/lib/lp/services/auth/interfaces.py | |||
24 | +++ b/lib/lp/services/auth/interfaces.py | |||
25 | @@ -77,13 +77,13 @@ class IAccessToken(Interface): | |||
26 | 77 | class IAccessTokenSet(Interface): | 77 | class IAccessTokenSet(Interface): |
27 | 78 | """The set of all personal access tokens.""" | 78 | """The set of all personal access tokens.""" |
28 | 79 | 79 | ||
30 | 80 | def new(secret, owner, description, context, scopes): | 80 | def new(secret, owner, description, target, scopes): |
31 | 81 | """Return a new access token with a given secret. | 81 | """Return a new access token with a given secret. |
32 | 82 | 82 | ||
33 | 83 | :param secret: A text string. | 83 | :param secret: A text string. |
34 | 84 | :param owner: An `IPerson` who is creating the token. | 84 | :param owner: An `IPerson` who is creating the token. |
35 | 85 | :param description: A short description of the token. | 85 | :param description: A short description of the token. |
37 | 86 | :param context: An `IGitRepository` for which the token is being | 86 | :param target: An `IAccessTokenTarget` for which the token is being |
38 | 87 | issued. | 87 | issued. |
39 | 88 | :param scopes: A list of `AccessTokenScope`s to be granted by the | 88 | :param scopes: A list of `AccessTokenScope`s to be granted by the |
40 | 89 | token. | 89 | token. |
41 | @@ -101,10 +101,10 @@ class IAccessTokenSet(Interface): | |||
42 | 101 | :param owner: An `IPerson`. | 101 | :param owner: An `IPerson`. |
43 | 102 | """ | 102 | """ |
44 | 103 | 103 | ||
47 | 104 | def findByContext(context): | 104 | def findByTarget(target): |
48 | 105 | """Return all access tokens for this context. | 105 | """Return all access tokens for this target. |
49 | 106 | 106 | ||
51 | 107 | :param context: An `IGitRepository`. | 107 | :param target: An `IGitRepository`. |
52 | 108 | """ | 108 | """ |
53 | 109 | 109 | ||
54 | 110 | 110 | ||
55 | diff --git a/lib/lp/services/auth/model.py b/lib/lp/services/auth/model.py | |||
56 | index c1228d1..437be7e 100644 | |||
57 | --- a/lib/lp/services/auth/model.py | |||
58 | +++ b/lib/lp/services/auth/model.py | |||
59 | @@ -78,20 +78,20 @@ class AccessToken(StormBase): | |||
60 | 78 | 78 | ||
61 | 79 | resolution = timedelta(minutes=10) | 79 | resolution = timedelta(minutes=10) |
62 | 80 | 80 | ||
64 | 81 | def __init__(self, secret, owner, description, context, scopes): | 81 | def __init__(self, secret, owner, description, target, scopes): |
65 | 82 | """Construct an `AccessToken`.""" | 82 | """Construct an `AccessToken`.""" |
66 | 83 | self._token_sha256 = hashlib.sha256(secret.encode()).hexdigest() | 83 | self._token_sha256 = hashlib.sha256(secret.encode()).hexdigest() |
67 | 84 | self.owner = owner | 84 | self.owner = owner |
68 | 85 | self.description = description | 85 | self.description = description |
71 | 86 | if IGitRepository.providedBy(context): | 86 | if IGitRepository.providedBy(target): |
72 | 87 | self.git_repository = context | 87 | self.git_repository = target |
73 | 88 | else: | 88 | else: |
75 | 89 | raise TypeError("Unsupported context: {!r}".format(context)) | 89 | raise TypeError("Unsupported target: {!r}".format(target)) |
76 | 90 | self.scopes = scopes | 90 | self.scopes = scopes |
77 | 91 | self.date_created = UTC_NOW | 91 | self.date_created = UTC_NOW |
78 | 92 | 92 | ||
79 | 93 | @property | 93 | @property |
81 | 94 | def context(self): | 94 | def target(self): |
82 | 95 | """See `IAccessToken`.""" | 95 | """See `IAccessToken`.""" |
83 | 96 | return self.git_repository | 96 | return self.git_repository |
84 | 97 | 97 | ||
85 | @@ -139,10 +139,10 @@ class AccessToken(StormBase): | |||
86 | 139 | @implementer(IAccessTokenSet) | 139 | @implementer(IAccessTokenSet) |
87 | 140 | class AccessTokenSet: | 140 | class AccessTokenSet: |
88 | 141 | 141 | ||
90 | 142 | def new(self, secret, owner, description, context, scopes): | 142 | def new(self, secret, owner, description, target, scopes): |
91 | 143 | """See `IAccessTokenSet`.""" | 143 | """See `IAccessTokenSet`.""" |
92 | 144 | store = IStore(AccessToken) | 144 | store = IStore(AccessToken) |
94 | 145 | token = AccessToken(secret, owner, description, context, scopes) | 145 | token = AccessToken(secret, owner, description, target, scopes) |
95 | 146 | store.add(token) | 146 | store.add(token) |
96 | 147 | return token | 147 | return token |
97 | 148 | 148 | ||
98 | @@ -156,11 +156,11 @@ class AccessTokenSet: | |||
99 | 156 | """See `IAccessTokenSet`.""" | 156 | """See `IAccessTokenSet`.""" |
100 | 157 | return IStore(AccessToken).find(AccessToken, owner=owner) | 157 | return IStore(AccessToken).find(AccessToken, owner=owner) |
101 | 158 | 158 | ||
103 | 159 | def findByContext(self, context): | 159 | def findByTarget(self, target): |
104 | 160 | """See `IAccessTokenSet`.""" | 160 | """See `IAccessTokenSet`.""" |
105 | 161 | kwargs = {} | 161 | kwargs = {} |
108 | 162 | if IGitRepository.providedBy(context): | 162 | if IGitRepository.providedBy(target): |
109 | 163 | kwargs["git_repository"] = context | 163 | kwargs["git_repository"] = target |
110 | 164 | else: | 164 | else: |
112 | 165 | raise TypeError("Unsupported context: {!r}".format(context)) | 165 | raise TypeError("Unsupported target: {!r}".format(target)) |
113 | 166 | return IStore(AccessToken).find(AccessToken, **kwargs) | 166 | return IStore(AccessToken).find(AccessToken, **kwargs) |
114 | diff --git a/lib/lp/services/auth/tests/test_model.py b/lib/lp/services/auth/tests/test_model.py | |||
115 | index 77ce073..f97d498 100644 | |||
116 | --- a/lib/lp/services/auth/tests/test_model.py | |||
117 | +++ b/lib/lp/services/auth/tests/test_model.py | |||
118 | @@ -49,11 +49,11 @@ class TestAccessToken(TestCaseWithFactory): | |||
119 | 49 | login_person(owner) | 49 | login_person(owner) |
120 | 50 | self.assertTrue(check_permission("launchpad.Edit", token)) | 50 | self.assertTrue(check_permission("launchpad.Edit", token)) |
121 | 51 | 51 | ||
127 | 52 | def test_context_owner_can_edit(self): | 52 | def test_target_owner_can_edit(self): |
128 | 53 | context_owner = self.factory.makePerson() | 53 | target_owner = self.factory.makePerson() |
129 | 54 | repository = self.factory.makeGitRepository(owner=context_owner) | 54 | repository = self.factory.makeGitRepository(owner=target_owner) |
130 | 55 | _, token = self.factory.makeAccessToken(context=repository) | 55 | _, token = self.factory.makeAccessToken(target=repository) |
131 | 56 | login_person(context_owner) | 56 | login_person(target_owner) |
132 | 57 | self.assertTrue(check_permission("launchpad.Edit", token)) | 57 | self.assertTrue(check_permission("launchpad.Edit", token)) |
133 | 58 | 58 | ||
134 | 59 | def test_other_user_cannot_edit(self): | 59 | def test_other_user_cannot_edit(self): |
135 | @@ -173,15 +173,15 @@ class TestAccessTokenSet(TestCaseWithFactory): | |||
136 | 173 | self.assertEqual(64, len(secret)) | 173 | self.assertEqual(64, len(secret)) |
137 | 174 | owner = self.factory.makePerson() | 174 | owner = self.factory.makePerson() |
138 | 175 | description = "Test token" | 175 | description = "Test token" |
140 | 176 | context = self.factory.makeGitRepository() | 176 | target = self.factory.makeGitRepository() |
141 | 177 | scopes = [AccessTokenScope.REPOSITORY_BUILD_STATUS] | 177 | scopes = [AccessTokenScope.REPOSITORY_BUILD_STATUS] |
142 | 178 | _, token = self.factory.makeAccessToken( | 178 | _, token = self.factory.makeAccessToken( |
145 | 179 | secret=secret, owner=owner, description=description, | 179 | secret=secret, owner=owner, description=description, target=target, |
146 | 180 | context=context, scopes=scopes) | 180 | scopes=scopes) |
147 | 181 | self.assertThat( | 181 | self.assertThat( |
148 | 182 | removeSecurityProxy(token), MatchesStructure.byEquality( | 182 | removeSecurityProxy(token), MatchesStructure.byEquality( |
149 | 183 | _token_sha256=hashlib.sha256(secret.encode()).hexdigest(), | 183 | _token_sha256=hashlib.sha256(secret.encode()).hexdigest(), |
151 | 184 | owner=owner, description=description, context=context, | 184 | owner=owner, description=description, target=target, |
152 | 185 | scopes=scopes)) | 185 | scopes=scopes)) |
153 | 186 | 186 | ||
154 | 187 | def test_getBySecret(self): | 187 | def test_getBySecret(self): |
155 | @@ -206,17 +206,16 @@ class TestAccessTokenSet(TestCaseWithFactory): | |||
156 | 206 | self.assertContentEqual( | 206 | self.assertContentEqual( |
157 | 207 | [], getUtility(IAccessTokenSet).findByOwner(owners[2])) | 207 | [], getUtility(IAccessTokenSet).findByOwner(owners[2])) |
158 | 208 | 208 | ||
161 | 209 | def test_findByContext(self): | 209 | def test_findByTarget(self): |
162 | 210 | contexts = [self.factory.makeGitRepository() for _ in range(3)] | 210 | targets = [self.factory.makeGitRepository() for _ in range(3)] |
163 | 211 | tokens = [ | 211 | tokens = [ |
167 | 212 | self.factory.makeAccessToken(context=contexts[0])[1], | 212 | self.factory.makeAccessToken(target=targets[0])[1], |
168 | 213 | self.factory.makeAccessToken(context=contexts[0])[1], | 213 | self.factory.makeAccessToken(target=targets[0])[1], |
169 | 214 | self.factory.makeAccessToken(context=contexts[1])[1], | 214 | self.factory.makeAccessToken(target=targets[1])[1], |
170 | 215 | ] | 215 | ] |
171 | 216 | self.assertContentEqual( | 216 | self.assertContentEqual( |
173 | 217 | tokens[:2], getUtility(IAccessTokenSet).findByContext(contexts[0])) | 217 | tokens[:2], getUtility(IAccessTokenSet).findByTarget(targets[0])) |
174 | 218 | self.assertContentEqual( | 218 | self.assertContentEqual( |
177 | 219 | [tokens[2]], | 219 | [tokens[2]], getUtility(IAccessTokenSet).findByTarget(targets[1])) |
176 | 220 | getUtility(IAccessTokenSet).findByContext(contexts[1])) | ||
178 | 221 | self.assertContentEqual( | 220 | self.assertContentEqual( |
180 | 222 | [], getUtility(IAccessTokenSet).findByContext(contexts[2])) | 221 | [], getUtility(IAccessTokenSet).findByTarget(targets[2])) |
181 | diff --git a/lib/lp/services/webapp/tests/test_servers.py b/lib/lp/services/webapp/tests/test_servers.py | |||
182 | index a551f78..aa5123f 100644 | |||
183 | --- a/lib/lp/services/webapp/tests/test_servers.py | |||
184 | +++ b/lib/lp/services/webapp/tests/test_servers.py | |||
185 | @@ -892,7 +892,7 @@ class TestWebServiceAccessTokens(TestCaseWithFactory): | |||
186 | 892 | def test_checkRequest_valid(self): | 892 | def test_checkRequest_valid(self): |
187 | 893 | repository = self.factory.makeGitRepository() | 893 | repository = self.factory.makeGitRepository() |
188 | 894 | self._makeAccessTokenVerifiedRequest( | 894 | self._makeAccessTokenVerifiedRequest( |
190 | 895 | context=repository, | 895 | target=repository, |
191 | 896 | scopes=[AccessTokenScope.REPOSITORY_BUILD_STATUS]) | 896 | scopes=[AccessTokenScope.REPOSITORY_BUILD_STATUS]) |
192 | 897 | getUtility(IWebServiceConfiguration).checkRequest( | 897 | getUtility(IWebServiceConfiguration).checkRequest( |
193 | 898 | repository, | 898 | repository, |
194 | @@ -901,7 +901,7 @@ class TestWebServiceAccessTokens(TestCaseWithFactory): | |||
195 | 901 | def test_checkRequest_bad_context(self): | 901 | def test_checkRequest_bad_context(self): |
196 | 902 | repository = self.factory.makeGitRepository() | 902 | repository = self.factory.makeGitRepository() |
197 | 903 | self._makeAccessTokenVerifiedRequest( | 903 | self._makeAccessTokenVerifiedRequest( |
199 | 904 | context=repository, | 904 | target=repository, |
200 | 905 | scopes=[AccessTokenScope.REPOSITORY_BUILD_STATUS]) | 905 | scopes=[AccessTokenScope.REPOSITORY_BUILD_STATUS]) |
201 | 906 | self.assertRaisesWithContent( | 906 | self.assertRaisesWithContent( |
202 | 907 | Unauthorized, | 907 | Unauthorized, |
203 | @@ -912,7 +912,7 @@ class TestWebServiceAccessTokens(TestCaseWithFactory): | |||
204 | 912 | def test_checkRequest_unscoped_method(self): | 912 | def test_checkRequest_unscoped_method(self): |
205 | 913 | repository = self.factory.makeGitRepository() | 913 | repository = self.factory.makeGitRepository() |
206 | 914 | self._makeAccessTokenVerifiedRequest( | 914 | self._makeAccessTokenVerifiedRequest( |
208 | 915 | context=repository, | 915 | target=repository, |
209 | 916 | scopes=[AccessTokenScope.REPOSITORY_BUILD_STATUS]) | 916 | scopes=[AccessTokenScope.REPOSITORY_BUILD_STATUS]) |
210 | 917 | self.assertRaisesWithContent( | 917 | self.assertRaisesWithContent( |
211 | 918 | Unauthorized, | 918 | Unauthorized, |
212 | @@ -923,7 +923,7 @@ class TestWebServiceAccessTokens(TestCaseWithFactory): | |||
213 | 923 | def test_checkRequest_wrong_scope(self): | 923 | def test_checkRequest_wrong_scope(self): |
214 | 924 | repository = self.factory.makeGitRepository() | 924 | repository = self.factory.makeGitRepository() |
215 | 925 | self._makeAccessTokenVerifiedRequest( | 925 | self._makeAccessTokenVerifiedRequest( |
217 | 926 | context=repository, | 926 | target=repository, |
218 | 927 | scopes=[ | 927 | scopes=[ |
219 | 928 | AccessTokenScope.REPOSITORY_BUILD_STATUS, | 928 | AccessTokenScope.REPOSITORY_BUILD_STATUS, |
220 | 929 | AccessTokenScope.REPOSITORY_PUSH, | 929 | AccessTokenScope.REPOSITORY_PUSH, |
221 | diff --git a/lib/lp/services/webservice/configuration.py b/lib/lp/services/webservice/configuration.py | |||
222 | index f52918a..8b56842 100644 | |||
223 | --- a/lib/lp/services/webservice/configuration.py | |||
224 | +++ b/lib/lp/services/webservice/configuration.py | |||
225 | @@ -102,7 +102,7 @@ class LaunchpadWebServiceConfiguration(BaseWebServiceConfiguration): | |||
226 | 102 | access_token = get_interaction_extras().access_token | 102 | access_token = get_interaction_extras().access_token |
227 | 103 | if access_token is None: | 103 | if access_token is None: |
228 | 104 | return | 104 | return |
230 | 105 | if access_token.context != context: | 105 | if access_token.target != context: |
231 | 106 | raise Unauthorized( | 106 | raise Unauthorized( |
232 | 107 | "Current authentication does not allow access to this object.") | 107 | "Current authentication does not allow access to this object.") |
233 | 108 | if not required_scopes: | 108 | if not required_scopes: |
234 | diff --git a/lib/lp/testing/factory.py b/lib/lp/testing/factory.py | |||
235 | index 3ba8333..4fccdec 100644 | |||
236 | --- a/lib/lp/testing/factory.py | |||
237 | +++ b/lib/lp/testing/factory.py | |||
238 | @@ -4519,7 +4519,7 @@ class BareLaunchpadObjectFactory(ObjectFactory): | |||
239 | 4519 | return request_token.createAccessToken() | 4519 | return request_token.createAccessToken() |
240 | 4520 | 4520 | ||
241 | 4521 | def makeAccessToken(self, secret=None, owner=None, description=None, | 4521 | def makeAccessToken(self, secret=None, owner=None, description=None, |
243 | 4522 | context=None, scopes=None): | 4522 | target=None, scopes=None): |
244 | 4523 | """Create a personal access token. | 4523 | """Create a personal access token. |
245 | 4524 | 4524 | ||
246 | 4525 | :return: A tuple of the secret for the new token and the token | 4525 | :return: A tuple of the secret for the new token and the token |
247 | @@ -4531,12 +4531,12 @@ class BareLaunchpadObjectFactory(ObjectFactory): | |||
248 | 4531 | owner = self.makePerson() | 4531 | owner = self.makePerson() |
249 | 4532 | if description is None: | 4532 | if description is None: |
250 | 4533 | description = self.getUniqueUnicode() | 4533 | description = self.getUniqueUnicode() |
253 | 4534 | if context is None: | 4534 | if target is None: |
254 | 4535 | context = self.makeGitRepository() | 4535 | target = self.makeGitRepository() |
255 | 4536 | if scopes is None: | 4536 | if scopes is None: |
256 | 4537 | scopes = [] | 4537 | scopes = [] |
257 | 4538 | token = getUtility(IAccessTokenSet).new( | 4538 | token = getUtility(IAccessTokenSet).new( |
259 | 4539 | secret, owner, description, context, scopes) | 4539 | secret, owner, description, target, scopes) |
260 | 4540 | return secret, token | 4540 | return secret, token |
261 | 4541 | 4541 | ||
262 | 4542 | def makeCVE(self, sequence, description=None, | 4542 | def makeCVE(self, sequence, description=None, |
Looks good!