Merge lp:~cjwatson/launchpad/snap-change-code-check-privacy into lp:launchpad
- snap-change-code-check-privacy
- Merge into devel
Status: | Merged | ||||
---|---|---|---|---|---|
Merged at revision: | 18917 | ||||
Proposed branch: | lp:~cjwatson/launchpad/snap-change-code-check-privacy | ||||
Merge into: | lp:launchpad | ||||
Diff against target: |
413 lines (+225/-24) 7 files modified
lib/lp/code/model/branch.py (+2/-0) lib/lp/code/model/gitrepository.py (+2/-0) lib/lp/snappy/browser/snap.py (+27/-8) lib/lp/snappy/browser/tests/test_snap.py (+66/-1) lib/lp/snappy/interfaces/snap.py (+4/-6) lib/lp/snappy/model/snap.py (+49/-9) lib/lp/snappy/tests/test_snap.py (+75/-0) |
||||
To merge this branch: | bzr merge lp:~cjwatson/launchpad/snap-change-code-check-privacy | ||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
William Grant | code | Approve | |
Review via email: mp+365294@code.launchpad.net |
Commit message
Apply privacy checks when changing various Snap attributes.
Description of the change
I think it was an oversight in the earlier rather basic privacy work for snaps that we didn't do this. There's some repetition here (for instance, creating a snap checks privacy rules twice, once in SnapSet.new and once in the various validators), but all except the checks in the validators now serve the purpose of producing clearer error messages rather than being security-critical.
Colin Watson (cjwatson) wrote : | # |
I tried Storm validators first, but the problem there was that they interfered with object creation because they required a new SQL statement in the middle of constructing the row. That said, my first attempt also involved deleting the up-front isValidPrivacy check, so I'll see if I can make it work while leaving that check in place, relying on the Storm cache.
Colin Watson (cjwatson) wrote : | # |
OK, I managed to get that to work in the end. Thanks for the prod.
Preview Diff
1 | === modified file 'lib/lp/code/model/branch.py' |
2 | --- lib/lp/code/model/branch.py 2019-02-01 14:08:34 +0000 |
3 | +++ lib/lp/code/model/branch.py 2019-04-01 09:07:26 +0000 |
4 | @@ -288,6 +288,8 @@ |
5 | if (verify_policy |
6 | and information_type not in self.getAllowedInformationTypes(who)): |
7 | raise CannotChangeInformationType("Forbidden by project policy.") |
8 | + # XXX cjwatson 2019-03-29: Check privacy rules on snaps that use |
9 | + # this branch. |
10 | self.information_type = information_type |
11 | self._reconcileAccess() |
12 | if information_type in PRIVATE_INFORMATION_TYPES and self.subscribers: |
13 | |
14 | === modified file 'lib/lp/code/model/gitrepository.py' |
15 | --- lib/lp/code/model/gitrepository.py 2019-02-11 12:31:06 +0000 |
16 | +++ lib/lp/code/model/gitrepository.py 2019-04-01 09:07:26 +0000 |
17 | @@ -791,6 +791,8 @@ |
18 | if (verify_policy and |
19 | information_type not in self.getAllowedInformationTypes(user)): |
20 | raise CannotChangeInformationType("Forbidden by project policy.") |
21 | + # XXX cjwatson 2019-03-29: Check privacy rules on snaps that use |
22 | + # this repository. |
23 | self.information_type = information_type |
24 | self._reconcileAccess() |
25 | if (information_type in PRIVATE_INFORMATION_TYPES and |
26 | |
27 | === modified file 'lib/lp/snappy/browser/snap.py' |
28 | --- lib/lp/snappy/browser/snap.py 2019-02-07 11:06:47 +0000 |
29 | +++ lib/lp/snappy/browser/snap.py 2019-04-01 09:07:26 +0000 |
30 | @@ -635,6 +635,28 @@ |
31 | self.widgets['store_channels'].context.required = store_upload |
32 | super(BaseSnapEditView, self).validate_widgets(data, names=names) |
33 | |
34 | + def validate(self, data): |
35 | + super(BaseSnapEditView, self).validate(data) |
36 | + if data.get('private', self.context.private) is False: |
37 | + if 'private' in data or 'owner' in data: |
38 | + owner = data.get('owner', self.context.owner) |
39 | + if owner is not None and owner.private: |
40 | + self.setFieldError( |
41 | + 'private' if 'private' in data else 'owner', |
42 | + u'A public snap cannot have a private owner.') |
43 | + if 'private' in data or 'branch' in data: |
44 | + branch = data.get('branch', self.context.branch) |
45 | + if branch is not None and branch.private: |
46 | + self.setFieldError( |
47 | + 'private' if 'private' in data else 'branch', |
48 | + u'A public snap cannot have a private branch.') |
49 | + if 'private' in data or 'git_ref' in data: |
50 | + ref = data.get('git_ref', self.context.git_ref) |
51 | + if ref is not None and ref.private: |
52 | + self.setFieldError( |
53 | + 'private' if 'private' in data else 'git_ref', |
54 | + u'A public snap cannot have a private repository.') |
55 | + |
56 | def _needStoreReauth(self, data): |
57 | """Does this change require reauthorizing to the store?""" |
58 | store_upload = data.get('store_upload', False) |
59 | @@ -703,16 +725,13 @@ |
60 | |
61 | def validate(self, data): |
62 | super(SnapAdminView, self).validate(data) |
63 | - private = data.get('private', None) |
64 | - if private is not None: |
65 | - if not getUtility(ISnapSet).isValidPrivacy( |
66 | - private, self.context.owner, self.context.branch, |
67 | - self.context.git_ref): |
68 | + # BaseSnapEditView.validate checks the rules for 'private' in |
69 | + # combination with other attributes. |
70 | + if data.get('private', None) is True: |
71 | + if not getFeatureFlag(SNAP_PRIVATE_FEATURE_FLAG): |
72 | self.setFieldError( |
73 | 'private', |
74 | - u'This snap contains private information and cannot ' |
75 | - u'be public.' |
76 | - ) |
77 | + u'You do not have permission to create private snaps.') |
78 | |
79 | |
80 | class SnapEditView(BaseSnapEditView, EnableProcessorsMixin): |
81 | |
82 | === modified file 'lib/lp/snappy/browser/tests/test_snap.py' |
83 | --- lib/lp/snappy/browser/tests/test_snap.py 2019-02-07 11:06:47 +0000 |
84 | +++ lib/lp/snappy/browser/tests/test_snap.py 2019-04-01 09:07:26 +0000 |
85 | @@ -677,7 +677,7 @@ |
86 | browser.getControl("Private").selected = False |
87 | browser.getControl("Update snap package").click() |
88 | self.assertEqual( |
89 | - 'This snap contains private information and cannot be public.', |
90 | + 'A public snap cannot have a private owner.', |
91 | extract_text(find_tags_by_class(browser.contents, "message")[1])) |
92 | |
93 | def test_admin_snap_sets_date_last_modified(self): |
94 | @@ -803,6 +803,71 @@ |
95 | "name.", |
96 | extract_text(find_tags_by_class(browser.contents, "message")[1])) |
97 | |
98 | + def test_edit_public_snap_private_owner(self): |
99 | + series = self.factory.makeUbuntuDistroSeries() |
100 | + with admin_logged_in(): |
101 | + snappy_series = self.factory.makeSnappySeries( |
102 | + usable_distro_series=[series]) |
103 | + login_person(self.person) |
104 | + snap = self.factory.makeSnap( |
105 | + registrant=self.person, owner=self.person, distroseries=series, |
106 | + store_series=snappy_series) |
107 | + private_team = self.factory.makeTeam( |
108 | + owner=self.person, visibility=PersonVisibility.PRIVATE) |
109 | + private_team_name = private_team.name |
110 | + browser = self.getViewBrowser(snap, user=self.person) |
111 | + browser.getLink("Edit snap package").click() |
112 | + browser.getControl("Owner").value = [private_team_name] |
113 | + browser.getControl("Update snap package").click() |
114 | + self.assertEqual( |
115 | + "A public snap cannot have a private owner.", |
116 | + extract_text(find_tags_by_class(browser.contents, "message")[1])) |
117 | + |
118 | + def test_edit_public_snap_private_branch(self): |
119 | + series = self.factory.makeUbuntuDistroSeries() |
120 | + with admin_logged_in(): |
121 | + snappy_series = self.factory.makeSnappySeries( |
122 | + usable_distro_series=[series]) |
123 | + login_person(self.person) |
124 | + snap = self.factory.makeSnap( |
125 | + registrant=self.person, owner=self.person, distroseries=series, |
126 | + branch=self.factory.makeAnyBranch(), store_series=snappy_series) |
127 | + private_branch = self.factory.makeAnyBranch( |
128 | + owner=self.person, |
129 | + information_type=InformationType.PRIVATESECURITY) |
130 | + private_branch_name = private_branch.unique_name |
131 | + browser = self.getViewBrowser(snap, user=self.person) |
132 | + browser.getLink("Edit snap package").click() |
133 | + browser.getControl("Bazaar branch").value = private_branch_name |
134 | + browser.getControl("Update snap package").click() |
135 | + self.assertEqual( |
136 | + "A public snap cannot have a private branch.", |
137 | + extract_text(find_tags_by_class(browser.contents, "message")[1])) |
138 | + |
139 | + def test_edit_public_snap_private_git_ref(self): |
140 | + series = self.factory.makeUbuntuDistroSeries() |
141 | + with admin_logged_in(): |
142 | + snappy_series = self.factory.makeSnappySeries( |
143 | + usable_distro_series=[series]) |
144 | + login_person(self.person) |
145 | + snap = self.factory.makeSnap( |
146 | + registrant=self.person, owner=self.person, distroseries=series, |
147 | + git_ref=self.factory.makeGitRefs()[0], store_series=snappy_series) |
148 | + login_person(self.person) |
149 | + [private_ref] = self.factory.makeGitRefs( |
150 | + owner=self.person, |
151 | + information_type=InformationType.PRIVATESECURITY) |
152 | + private_ref_identity = private_ref.repository.identity |
153 | + private_ref_path = private_ref.path |
154 | + browser = self.getViewBrowser(snap, user=self.person) |
155 | + browser.getLink("Edit snap package").click() |
156 | + browser.getControl("Git repository").value = private_ref_identity |
157 | + browser.getControl("Git branch").value = private_ref_path |
158 | + browser.getControl("Update snap package").click() |
159 | + self.assertEqual( |
160 | + "A public snap cannot have a private repository.", |
161 | + extract_text(find_tags_by_class(browser.contents, "message")[1])) |
162 | + |
163 | def test_edit_snap_git_url(self): |
164 | series = self.factory.makeUbuntuDistroSeries() |
165 | with admin_logged_in(): |
166 | |
167 | === modified file 'lib/lp/snappy/interfaces/snap.py' |
168 | --- lib/lp/snappy/interfaces/snap.py 2019-02-07 10:30:03 +0000 |
169 | +++ lib/lp/snappy/interfaces/snap.py 2019-04-01 09:07:26 +0000 |
170 | @@ -206,8 +206,9 @@ |
171 | class SnapPrivacyMismatch(Exception): |
172 | """Snap package privacy does not match its content.""" |
173 | |
174 | - def __init__(self): |
175 | + def __init__(self, message=None): |
176 | super(SnapPrivacyMismatch, self).__init__( |
177 | + message or |
178 | "Snap contains private information and cannot be public.") |
179 | |
180 | |
181 | @@ -690,11 +691,8 @@ |
182 | "snapcraft.yaml, or .snapcraft.yaml recipe at the top level.")) |
183 | _api_git_path = exported( |
184 | TextLine( |
185 | - title=_("Git branch path"), required=False, readonly=False, |
186 | - description=_( |
187 | - "The path of the Git branch containing a snap/snapcraft.yaml, " |
188 | - "snapcraft.yaml, or .snapcraft.yaml recipe at the top " |
189 | - "level.")), |
190 | + title=git_path.title, required=False, readonly=False, |
191 | + description=git_path.description), |
192 | exported_as="git_path") |
193 | |
194 | git_ref = exported(Reference( |
195 | |
196 | === modified file 'lib/lp/snappy/model/snap.py' |
197 | --- lib/lp/snappy/model/snap.py 2019-02-26 06:47:45 +0000 |
198 | +++ lib/lp/snappy/model/snap.py 2019-04-01 09:07:26 +0000 |
199 | @@ -49,7 +49,6 @@ |
200 | ArchiveFormatterAPI, |
201 | DateTimeFormatterAPI, |
202 | ) |
203 | -from lp.app.enums import PRIVATE_INFORMATION_TYPES |
204 | from lp.app.errors import IncompatibleArguments |
205 | from lp.app.interfaces.security import IAuthorization |
206 | from lp.buildmaster.enums import BuildStatus |
207 | @@ -82,10 +81,11 @@ |
208 | from lp.code.model.branchcollection import GenericBranchCollection |
209 | from lp.code.model.gitcollection import GenericGitCollection |
210 | from lp.code.model.gitrepository import GitRepository |
211 | -from lp.registry.enums import PersonVisibility |
212 | +from lp.registry.errors import PrivatePersonLinkageError |
213 | from lp.registry.interfaces.person import ( |
214 | IPerson, |
215 | IPersonSet, |
216 | + validate_public_person, |
217 | ) |
218 | from lp.registry.interfaces.pocket import PackagePublishingPocket |
219 | from lp.registry.interfaces.product import IProduct |
220 | @@ -261,7 +261,16 @@ |
221 | registrant_id = Int(name='registrant', allow_none=False) |
222 | registrant = Reference(registrant_id, 'Person.id') |
223 | |
224 | - owner_id = Int(name='owner', allow_none=False) |
225 | + def _validate_owner(self, attr, value): |
226 | + if not self.private: |
227 | + try: |
228 | + validate_public_person(self, attr, value) |
229 | + except PrivatePersonLinkageError: |
230 | + raise SnapPrivacyMismatch( |
231 | + "A public snap cannot have a private owner.") |
232 | + return value |
233 | + |
234 | + owner_id = Int(name='owner', allow_none=False, validator=_validate_owner) |
235 | owner = Reference(owner_id, 'Person.id') |
236 | |
237 | distro_series_id = Int(name='distro_series', allow_none=True) |
238 | @@ -271,10 +280,26 @@ |
239 | |
240 | description = Unicode(name='description', allow_none=True) |
241 | |
242 | - branch_id = Int(name='branch', allow_none=True) |
243 | + def _validate_branch(self, attr, value): |
244 | + if not self.private and value is not None: |
245 | + if IStore(Branch).get(Branch, value).private: |
246 | + raise SnapPrivacyMismatch( |
247 | + "A public snap cannot have a private branch.") |
248 | + return value |
249 | + |
250 | + branch_id = Int(name='branch', allow_none=True, validator=_validate_branch) |
251 | branch = Reference(branch_id, 'Branch.id') |
252 | |
253 | - git_repository_id = Int(name='git_repository', allow_none=True) |
254 | + def _validate_git_repository(self, attr, value): |
255 | + if not self.private and value is not None: |
256 | + if IStore(GitRepository).get(GitRepository, value).private: |
257 | + raise SnapPrivacyMismatch( |
258 | + "A public snap cannot have a private repository.") |
259 | + return value |
260 | + |
261 | + git_repository_id = Int( |
262 | + name='git_repository', allow_none=True, |
263 | + validator=_validate_git_repository) |
264 | git_repository = Reference(git_repository_id, 'GitRepository.id') |
265 | |
266 | git_repository_url = Unicode(name='git_repository_url', allow_none=True) |
267 | @@ -294,7 +319,13 @@ |
268 | |
269 | require_virtualized = Bool(name='require_virtualized') |
270 | |
271 | - private = Bool(name='private') |
272 | + def _validate_private(self, attr, value): |
273 | + if not getUtility(ISnapSet).isValidPrivacy( |
274 | + value, self.owner, self.branch, self.git_ref): |
275 | + raise SnapPrivacyMismatch |
276 | + return value |
277 | + |
278 | + private = Bool(name='private', validator=_validate_private) |
279 | |
280 | allow_internet = Bool(name='allow_internet', allow_none=False) |
281 | |
282 | @@ -321,6 +352,11 @@ |
283 | store_channels=None): |
284 | """Construct a `Snap`.""" |
285 | super(Snap, self).__init__() |
286 | + |
287 | + # Set the private flag first so that other validators can perform |
288 | + # suitable privacy checks. |
289 | + self.private = private |
290 | + |
291 | self.registrant = registrant |
292 | self.owner = owner |
293 | self.distro_series = distro_series |
294 | @@ -335,7 +371,6 @@ |
295 | self.require_virtualized = require_virtualized |
296 | self.date_created = date_created |
297 | self.date_last_modified = date_created |
298 | - self.private = private |
299 | self.allow_internet = allow_internet |
300 | self.build_source_tarball = build_source_tarball |
301 | self.store_upload = store_upload |
302 | @@ -1022,6 +1057,11 @@ |
303 | if self.exists(owner, name): |
304 | raise DuplicateSnapName |
305 | |
306 | + # The relevant validators will do their own checks as well, but we |
307 | + # do a single up-front check here in order to avoid an |
308 | + # IntegrityError due to exceptions being raised during object |
309 | + # creation and to ensure that everything relevant is in the Storm |
310 | + # cache. |
311 | if not self.isValidPrivacy(private, owner, branch, git_ref): |
312 | raise SnapPrivacyMismatch |
313 | |
314 | @@ -1058,11 +1098,11 @@ |
315 | |
316 | # Public snaps with private sources are not allowed. |
317 | source = branch or git_ref |
318 | - if source.information_type in PRIVATE_INFORMATION_TYPES: |
319 | + if source is not None and source.private: |
320 | return False |
321 | |
322 | # Public snaps owned by private teams are not allowed. |
323 | - if owner.is_team and owner.visibility == PersonVisibility.PRIVATE: |
324 | + if owner is not None and owner.private: |
325 | return False |
326 | |
327 | return True |
328 | |
329 | === modified file 'lib/lp/snappy/tests/test_snap.py' |
330 | --- lib/lp/snappy/tests/test_snap.py 2019-02-26 06:47:45 +0000 |
331 | +++ lib/lp/snappy/tests/test_snap.py 2019-04-01 09:07:26 +0000 |
332 | @@ -2376,6 +2376,81 @@ |
333 | self.assertEqual( |
334 | "Test Person is not a member of Other Team.", response.body) |
335 | |
336 | + def test_cannot_make_snap_with_private_components_public(self): |
337 | + # If a Snap has private components, then trying to make it public |
338 | + # fails. |
339 | + branch = self.factory.makeAnyBranch( |
340 | + owner=self.person, |
341 | + information_type=InformationType.PRIVATESECURITY) |
342 | + snap = self.factory.makeSnap( |
343 | + registrant=self.person, owner=self.person, branch=branch, |
344 | + private=True) |
345 | + admin = getUtility(ILaunchpadCelebrities).admin.teamowner |
346 | + with person_logged_in(self.person): |
347 | + snap_url = api_url(snap) |
348 | + logout() |
349 | + admin_webservice = webservice_for_person( |
350 | + admin, permission=OAuthPermission.WRITE_PRIVATE) |
351 | + admin_webservice.default_api_version = "devel" |
352 | + response = admin_webservice.patch( |
353 | + snap_url, "application/json", json.dumps({"private": False})) |
354 | + self.assertEqual(400, response.status) |
355 | + self.assertEqual( |
356 | + "Snap contains private information and cannot be public.", |
357 | + response.body) |
358 | + |
359 | + def test_cannot_set_private_components_of_public_snap(self): |
360 | + # If a Snap is public, then trying to change any of its owner, |
361 | + # branch, or git_repository components to be private fails. |
362 | + bzr_snap = self.factory.makeSnap( |
363 | + registrant=self.person, owner=self.person, |
364 | + branch=self.factory.makeAnyBranch()) |
365 | + git_snap = self.factory.makeSnap( |
366 | + registrant=self.person, owner=self.person, |
367 | + git_ref=self.factory.makeGitRefs()[0]) |
368 | + private_team = self.factory.makeTeam( |
369 | + owner=self.person, visibility=PersonVisibility.PRIVATE) |
370 | + private_branch = self.factory.makeAnyBranch( |
371 | + owner=self.person, |
372 | + information_type=InformationType.PRIVATESECURITY) |
373 | + [private_ref] = self.factory.makeGitRefs( |
374 | + owner=self.person, |
375 | + information_type=InformationType.PRIVATESECURITY) |
376 | + bzr_snap_url = api_url(bzr_snap) |
377 | + git_snap_url = api_url(git_snap) |
378 | + with person_logged_in(self.person): |
379 | + private_team_url = api_url(private_team) |
380 | + private_branch_url = api_url(private_branch) |
381 | + private_ref_url = api_url(private_ref) |
382 | + logout() |
383 | + private_webservice = webservice_for_person( |
384 | + self.person, permission=OAuthPermission.WRITE_PRIVATE) |
385 | + private_webservice.default_api_version = "devel" |
386 | + response = private_webservice.patch( |
387 | + bzr_snap_url, "application/json", |
388 | + json.dumps({"owner_link": private_team_url})) |
389 | + self.assertEqual(400, response.status) |
390 | + self.assertEqual( |
391 | + "A public snap cannot have a private owner.", response.body) |
392 | + response = private_webservice.patch( |
393 | + bzr_snap_url, "application/json", |
394 | + json.dumps({"branch_link": private_branch_url})) |
395 | + self.assertEqual(400, response.status) |
396 | + self.assertEqual( |
397 | + "A public snap cannot have a private branch.", response.body) |
398 | + response = private_webservice.patch( |
399 | + git_snap_url, "application/json", |
400 | + json.dumps({"owner_link": private_team_url})) |
401 | + self.assertEqual(400, response.status) |
402 | + self.assertEqual( |
403 | + "A public snap cannot have a private owner.", response.body) |
404 | + response = private_webservice.patch( |
405 | + git_snap_url, "application/json", |
406 | + json.dumps({"git_ref_link": private_ref_url})) |
407 | + self.assertEqual(400, response.status) |
408 | + self.assertEqual( |
409 | + "A public snap cannot have a private repository.", response.body) |
410 | + |
411 | def test_cannot_set_git_path_for_bzr(self): |
412 | # Setting git_path on a Bazaar-based Snap fails. |
413 | snap = self.makeSnap(branch=self.factory.makeAnyBranch()) |
I'd prefer seeing these all as Storm column validators rather than ugly prefixed properties.