Merge lp:~cjwatson/launchpad/snap-change-code-check-privacy into lp:launchpad

Proposed by Colin Watson
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
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.

To post a comment you must log in.
Revision history for this message
William Grant (wgrant) wrote :

I'd prefer seeing these all as Storm column validators rather than ugly prefixed properties.

review: Approve (code)
Revision history for this message
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.

Revision history for this message
Colin Watson (cjwatson) wrote :

OK, I managed to get that to work in the end. Thanks for the prod.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
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())