launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #23465
[Merge] lp:~cjwatson/launchpad/snap-change-code-check-privacy into lp:launchpad
Colin Watson has proposed merging lp:~cjwatson/launchpad/snap-change-code-check-privacy into lp:launchpad.
Commit message:
Apply privacy checks when changing various Snap attributes.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #1639975 in Launchpad itself: "support for building private snaps"
https://bugs.launchpad.net/launchpad/+bug/1639975
For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/snap-change-code-check-privacy/+merge/365294
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 accessors), but all except the checks in the accessors now serve the purpose of producing clearer error messages rather than being security-critical.
--
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/snap-change-code-check-privacy into lp:launchpad.
=== modified file 'lib/lp/code/model/branch.py'
--- lib/lp/code/model/branch.py 2019-02-01 14:08:34 +0000
+++ lib/lp/code/model/branch.py 2019-03-29 17:17:13 +0000
@@ -288,6 +288,8 @@
if (verify_policy
and information_type not in self.getAllowedInformationTypes(who)):
raise CannotChangeInformationType("Forbidden by project policy.")
+ # XXX cjwatson 2019-03-29: Check privacy rules on snaps that use
+ # this branch.
self.information_type = information_type
self._reconcileAccess()
if information_type in PRIVATE_INFORMATION_TYPES and self.subscribers:
=== modified file 'lib/lp/code/model/gitrepository.py'
--- lib/lp/code/model/gitrepository.py 2019-02-11 12:31:06 +0000
+++ lib/lp/code/model/gitrepository.py 2019-03-29 17:17:13 +0000
@@ -791,6 +791,8 @@
if (verify_policy and
information_type not in self.getAllowedInformationTypes(user)):
raise CannotChangeInformationType("Forbidden by project policy.")
+ # XXX cjwatson 2019-03-29: Check privacy rules on snaps that use
+ # this repository.
self.information_type = information_type
self._reconcileAccess()
if (information_type in PRIVATE_INFORMATION_TYPES and
=== modified file 'lib/lp/snappy/browser/snap.py'
--- lib/lp/snappy/browser/snap.py 2019-02-07 11:06:47 +0000
+++ lib/lp/snappy/browser/snap.py 2019-03-29 17:17:13 +0000
@@ -635,6 +635,28 @@
self.widgets['store_channels'].context.required = store_upload
super(BaseSnapEditView, self).validate_widgets(data, names=names)
+ def validate(self, data):
+ super(BaseSnapEditView, self).validate(data)
+ if data.get('private', self.context.private) is False:
+ if 'private' in data or 'owner' in data:
+ owner = data.get('owner', self.context.owner)
+ if owner is not None and owner.private:
+ self.setFieldError(
+ 'private' if 'private' in data else 'owner',
+ u'A public snap cannot have a private owner.')
+ if 'private' in data or 'branch' in data:
+ branch = data.get('branch', self.context.branch)
+ if branch is not None and branch.private:
+ self.setFieldError(
+ 'private' if 'private' in data else 'branch',
+ u'A public snap cannot have a private branch.')
+ if 'private' in data or 'git_ref' in data:
+ ref = data.get('git_ref', self.context.git_ref)
+ if ref is not None and ref.private:
+ self.setFieldError(
+ 'private' if 'private' in data else 'git_ref',
+ u'A public snap cannot have a private branch.')
+
def _needStoreReauth(self, data):
"""Does this change require reauthorizing to the store?"""
store_upload = data.get('store_upload', False)
@@ -703,16 +725,13 @@
def validate(self, data):
super(SnapAdminView, self).validate(data)
- private = data.get('private', None)
- if private is not None:
- if not getUtility(ISnapSet).isValidPrivacy(
- private, self.context.owner, self.context.branch,
- self.context.git_ref):
+ # BaseSnapEditView.validate checks the rules for 'private' in
+ # combination with other attributes.
+ if data.get('private', None) is True:
+ if not getFeatureFlag(SNAP_PRIVATE_FEATURE_FLAG):
self.setFieldError(
'private',
- u'This snap contains private information and cannot '
- u'be public.'
- )
+ u'You do not have permission to create private snaps.')
class SnapEditView(BaseSnapEditView, EnableProcessorsMixin):
=== modified file 'lib/lp/snappy/browser/tests/test_snap.py'
--- lib/lp/snappy/browser/tests/test_snap.py 2019-02-07 11:06:47 +0000
+++ lib/lp/snappy/browser/tests/test_snap.py 2019-03-29 17:17:13 +0000
@@ -677,7 +677,7 @@
browser.getControl("Private").selected = False
browser.getControl("Update snap package").click()
self.assertEqual(
- 'This snap contains private information and cannot be public.',
+ 'A public snap cannot have a private owner.',
extract_text(find_tags_by_class(browser.contents, "message")[1]))
def test_admin_snap_sets_date_last_modified(self):
@@ -803,6 +803,71 @@
"name.",
extract_text(find_tags_by_class(browser.contents, "message")[1]))
+ def test_edit_public_snap_private_owner(self):
+ series = self.factory.makeUbuntuDistroSeries()
+ with admin_logged_in():
+ snappy_series = self.factory.makeSnappySeries(
+ usable_distro_series=[series])
+ login_person(self.person)
+ snap = self.factory.makeSnap(
+ registrant=self.person, owner=self.person, distroseries=series,
+ store_series=snappy_series)
+ private_team = self.factory.makeTeam(
+ owner=self.person, visibility=PersonVisibility.PRIVATE)
+ private_team_name = private_team.name
+ browser = self.getViewBrowser(snap, user=self.person)
+ browser.getLink("Edit snap package").click()
+ browser.getControl("Owner").value = [private_team_name]
+ browser.getControl("Update snap package").click()
+ self.assertEqual(
+ "A public snap cannot have a private owner.",
+ extract_text(find_tags_by_class(browser.contents, "message")[1]))
+
+ def test_edit_public_snap_private_branch(self):
+ series = self.factory.makeUbuntuDistroSeries()
+ with admin_logged_in():
+ snappy_series = self.factory.makeSnappySeries(
+ usable_distro_series=[series])
+ login_person(self.person)
+ snap = self.factory.makeSnap(
+ registrant=self.person, owner=self.person, distroseries=series,
+ branch=self.factory.makeAnyBranch(), store_series=snappy_series)
+ private_branch = self.factory.makeAnyBranch(
+ owner=self.person,
+ information_type=InformationType.PRIVATESECURITY)
+ private_branch_name = private_branch.unique_name
+ browser = self.getViewBrowser(snap, user=self.person)
+ browser.getLink("Edit snap package").click()
+ browser.getControl("Bazaar branch").value = private_branch_name
+ browser.getControl("Update snap package").click()
+ self.assertEqual(
+ "A public snap cannot have a private branch.",
+ extract_text(find_tags_by_class(browser.contents, "message")[1]))
+
+ def test_edit_public_snap_private_git_ref(self):
+ series = self.factory.makeUbuntuDistroSeries()
+ with admin_logged_in():
+ snappy_series = self.factory.makeSnappySeries(
+ usable_distro_series=[series])
+ login_person(self.person)
+ snap = self.factory.makeSnap(
+ registrant=self.person, owner=self.person, distroseries=series,
+ git_ref=self.factory.makeGitRefs()[0], store_series=snappy_series)
+ login_person(self.person)
+ [private_ref] = self.factory.makeGitRefs(
+ owner=self.person,
+ information_type=InformationType.PRIVATESECURITY)
+ private_ref_identity = private_ref.repository.identity
+ private_ref_path = private_ref.path
+ browser = self.getViewBrowser(snap, user=self.person)
+ browser.getLink("Edit snap package").click()
+ browser.getControl("Git repository").value = private_ref_identity
+ browser.getControl("Git branch").value = private_ref_path
+ browser.getControl("Update snap package").click()
+ self.assertEqual(
+ "A public snap cannot have a private branch.",
+ extract_text(find_tags_by_class(browser.contents, "message")[1]))
+
def test_edit_snap_git_url(self):
series = self.factory.makeUbuntuDistroSeries()
with admin_logged_in():
=== modified file 'lib/lp/snappy/interfaces/snap.py'
--- lib/lp/snappy/interfaces/snap.py 2019-02-07 10:30:03 +0000
+++ lib/lp/snappy/interfaces/snap.py 2019-03-29 17:17:13 +0000
@@ -206,8 +206,9 @@
class SnapPrivacyMismatch(Exception):
"""Snap package privacy does not match its content."""
- def __init__(self):
+ def __init__(self, message=None):
super(SnapPrivacyMismatch, self).__init__(
+ message or
"Snap contains private information and cannot be public.")
@@ -690,11 +691,8 @@
"snapcraft.yaml, or .snapcraft.yaml recipe at the top level."))
_api_git_path = exported(
TextLine(
- title=_("Git branch path"), required=False, readonly=False,
- description=_(
- "The path of the Git branch containing a snap/snapcraft.yaml, "
- "snapcraft.yaml, or .snapcraft.yaml recipe at the top "
- "level.")),
+ title=git_path.title, required=False, readonly=False,
+ description=git_path.description),
exported_as="git_path")
git_ref = exported(Reference(
=== modified file 'lib/lp/snappy/model/snap.py'
--- lib/lp/snappy/model/snap.py 2019-02-26 06:47:45 +0000
+++ lib/lp/snappy/model/snap.py 2019-03-29 17:17:13 +0000
@@ -49,7 +49,6 @@
ArchiveFormatterAPI,
DateTimeFormatterAPI,
)
-from lp.app.enums import PRIVATE_INFORMATION_TYPES
from lp.app.errors import IncompatibleArguments
from lp.app.interfaces.security import IAuthorization
from lp.buildmaster.enums import BuildStatus
@@ -82,7 +81,6 @@
from lp.code.model.branchcollection import GenericBranchCollection
from lp.code.model.gitcollection import GenericGitCollection
from lp.code.model.gitrepository import GitRepository
-from lp.registry.enums import PersonVisibility
from lp.registry.interfaces.person import (
IPerson,
IPersonSet,
@@ -262,7 +260,7 @@
registrant = Reference(registrant_id, 'Person.id')
owner_id = Int(name='owner', allow_none=False)
- owner = Reference(owner_id, 'Person.id')
+ _owner = Reference(owner_id, 'Person.id')
distro_series_id = Int(name='distro_series', allow_none=True)
distro_series = Reference(distro_series_id, 'DistroSeries.id')
@@ -272,7 +270,7 @@
description = Unicode(name='description', allow_none=True)
branch_id = Int(name='branch', allow_none=True)
- branch = Reference(branch_id, 'Branch.id')
+ _branch = Reference(branch_id, 'Branch.id')
git_repository_id = Int(name='git_repository', allow_none=True)
git_repository = Reference(git_repository_id, 'GitRepository.id')
@@ -294,7 +292,7 @@
require_virtualized = Bool(name='require_virtualized')
- private = Bool(name='private')
+ _private = Bool(name='private')
allow_internet = Bool(name='allow_internet', allow_none=False)
@@ -321,6 +319,11 @@
store_channels=None):
"""Construct a `Snap`."""
super(Snap, self).__init__()
+
+ # Set the private flag first so that other accessors can perform
+ # suitable privacy checks.
+ self.private = private
+
self.registrant = registrant
self.owner = owner
self.distro_series = distro_series
@@ -335,7 +338,6 @@
self.require_virtualized = require_virtualized
self.date_created = date_created
self.date_last_modified = date_created
- self.private = private
self.allow_internet = allow_internet
self.build_source_tarball = build_source_tarball
self.store_upload = store_upload
@@ -352,6 +354,29 @@
return ["snap:build:0.1"]
@property
+ def owner(self):
+ return self._owner
+
+ @owner.setter
+ def owner(self, value):
+ # Public snaps may not be owned by private teams.
+ if not self.private and value is not None and value.private:
+ raise SnapPrivacyMismatch(
+ "A public snap cannot have a private owner.")
+ self._owner = value
+
+ @property
+ def branch(self):
+ return self._branch
+
+ @branch.setter
+ def branch(self, value):
+ if not self.private and value is not None and value.private:
+ raise SnapPrivacyMismatch(
+ "A public snap cannot have a private branch.")
+ self._branch = value
+
+ @property
def _api_git_path(self):
return self.git_path
@@ -379,6 +404,9 @@
def git_ref(self, value):
"""See `ISnap`."""
if value is not None:
+ if not self.private and value.private:
+ raise SnapPrivacyMismatch(
+ "A public snap cannot have a private branch.")
self.git_repository = value.repository
self.git_repository_url = value.repository_url
self.git_path = value.path
@@ -397,6 +425,17 @@
return None
@property
+ def private(self):
+ return self._private
+
+ @private.setter
+ def private(self, value):
+ if not getUtility(ISnapSet).isValidPrivacy(
+ value, self.owner, self.branch, self.git_ref):
+ raise SnapPrivacyMismatch
+ self._private = value
+
+ @property
def available_processors(self):
"""See `ISnap`."""
clauses = [Processor.id == DistroArchSeries.processor_id]
@@ -1022,6 +1061,9 @@
if self.exists(owner, name):
raise DuplicateSnapName
+ # The relevant accessors will do their own checks as well, but we do
+ # a single up-front check here in order to avoid an IntegrityError
+ # due to exceptions being raised during object creation.
if not self.isValidPrivacy(private, owner, branch, git_ref):
raise SnapPrivacyMismatch
@@ -1058,18 +1100,18 @@
# Public snaps with private sources are not allowed.
source = branch or git_ref
- if source.information_type in PRIVATE_INFORMATION_TYPES:
+ if source is not None and source.private:
return False
# Public snaps owned by private teams are not allowed.
- if owner.is_team and owner.visibility == PersonVisibility.PRIVATE:
+ if owner is not None and owner.private:
return False
return True
def _getByName(self, owner, name):
return IStore(Snap).find(
- Snap, Snap.owner == owner, Snap.name == name).one()
+ Snap, Snap._owner == owner, Snap.name == name).one()
def exists(self, owner, name):
"""See `ISnapSet`."""
@@ -1091,12 +1133,12 @@
ids = collection.getRepositoryIds()
expressions = [id_column.is_in(ids._get_select())]
if owner is not None:
- expressions.append(Snap.owner == owner)
+ expressions.append(Snap._owner == owner)
return IStore(Snap).find(Snap, *expressions)
def findByOwner(self, owner):
"""See `ISnapSet`."""
- return IStore(Snap).find(Snap, Snap.owner == owner)
+ return IStore(Snap).find(Snap, Snap._owner == owner)
def findByPerson(self, person, visible_by_user=None):
"""See `ISnapSet`."""
@@ -1111,7 +1153,7 @@
git_collection = removeSecurityProxy(getUtility(IAllGitRepositories))
git_snaps = _getSnaps(git_collection)
git_url_snaps = IStore(Snap).find(
- Snap, Snap.owner == person, Snap.git_repository_url != None)
+ Snap, Snap._owner == person, Snap.git_repository_url != None)
return bzr_snaps.union(git_snaps).union(git_url_snaps)
def findByProject(self, project, visible_by_user=None):
@@ -1126,7 +1168,7 @@
def findByBranch(self, branch):
"""See `ISnapSet`."""
- return IStore(Snap).find(Snap, Snap.branch == branch)
+ return IStore(Snap).find(Snap, Snap._branch == branch)
def findByGitRepository(self, repository, paths=None):
"""See `ISnapSet`."""
@@ -1168,14 +1210,14 @@
# don't yet have the access grant infrastructure to do better, and
# in any case the numbers involved should be very small.
if visible_by_user is None:
- return Snap.private == False
+ return Snap._private == False
else:
roles = IPersonRoles(visible_by_user)
if roles.in_admin or roles.in_commercial_admin:
return True
else:
return Or(
- Snap.private == False,
+ Snap._private == False,
Snap.owner_id.is_in(Select(
TeamParticipation.teamID,
TeamParticipation.person == visible_by_user)))
@@ -1184,7 +1226,7 @@
"""See `ISnapSet`."""
clauses = [Snap.git_repository_url == url]
if owner is not None:
- clauses.append(Snap.owner == owner)
+ clauses.append(Snap._owner == owner)
clauses.append(self._findByURLVisibilityClause(visible_by_user))
return IStore(Snap).find(Snap, *clauses)
@@ -1201,7 +1243,7 @@
for url_prefix in url_prefixes]
clauses = [Or(*prefix_clauses)]
if owner is not None:
- clauses.append(Snap.owner == owner)
+ clauses.append(Snap._owner == owner)
clauses.append(self._findByURLVisibilityClause(visible_by_user))
return IStore(Snap).find(Snap, *clauses)
=== modified file 'lib/lp/snappy/tests/test_snap.py'
--- lib/lp/snappy/tests/test_snap.py 2019-02-26 06:47:45 +0000
+++ lib/lp/snappy/tests/test_snap.py 2019-03-29 17:17:13 +0000
@@ -2376,6 +2376,81 @@
self.assertEqual(
"Test Person is not a member of Other Team.", response.body)
+ def test_cannot_make_snap_with_private_components_public(self):
+ # If a Snap has private components, then trying to make it public
+ # fails.
+ branch = self.factory.makeAnyBranch(
+ owner=self.person,
+ information_type=InformationType.PRIVATESECURITY)
+ snap = self.factory.makeSnap(
+ registrant=self.person, owner=self.person, branch=branch,
+ private=True)
+ admin = getUtility(ILaunchpadCelebrities).admin.teamowner
+ with person_logged_in(self.person):
+ snap_url = api_url(snap)
+ logout()
+ admin_webservice = webservice_for_person(
+ admin, permission=OAuthPermission.WRITE_PRIVATE)
+ admin_webservice.default_api_version = "devel"
+ response = admin_webservice.patch(
+ snap_url, "application/json", json.dumps({"private": False}))
+ self.assertEqual(400, response.status)
+ self.assertEqual(
+ "Snap contains private information and cannot be public.",
+ response.body)
+
+ def test_cannot_set_private_components_of_public_snap(self):
+ # If a Snap is public, then trying to change any of its owner,
+ # branch, or git_repository components to be private fails.
+ bzr_snap = self.factory.makeSnap(
+ registrant=self.person, owner=self.person,
+ branch=self.factory.makeAnyBranch())
+ git_snap = self.factory.makeSnap(
+ registrant=self.person, owner=self.person,
+ git_ref=self.factory.makeGitRefs()[0])
+ private_team = self.factory.makeTeam(
+ owner=self.person, visibility=PersonVisibility.PRIVATE)
+ private_branch = self.factory.makeAnyBranch(
+ owner=self.person,
+ information_type=InformationType.PRIVATESECURITY)
+ [private_ref] = self.factory.makeGitRefs(
+ owner=self.person,
+ information_type=InformationType.PRIVATESECURITY)
+ bzr_snap_url = api_url(bzr_snap)
+ git_snap_url = api_url(git_snap)
+ with person_logged_in(self.person):
+ private_team_url = api_url(private_team)
+ private_branch_url = api_url(private_branch)
+ private_ref_url = api_url(private_ref)
+ logout()
+ private_webservice = webservice_for_person(
+ self.person, permission=OAuthPermission.WRITE_PRIVATE)
+ private_webservice.default_api_version = "devel"
+ response = private_webservice.patch(
+ bzr_snap_url, "application/json",
+ json.dumps({"owner_link": private_team_url}))
+ self.assertEqual(400, response.status)
+ self.assertEqual(
+ "A public snap cannot have a private owner.", response.body)
+ response = private_webservice.patch(
+ bzr_snap_url, "application/json",
+ json.dumps({"branch_link": private_branch_url}))
+ self.assertEqual(400, response.status)
+ self.assertEqual(
+ "A public snap cannot have a private branch.", response.body)
+ response = private_webservice.patch(
+ git_snap_url, "application/json",
+ json.dumps({"owner_link": private_team_url}))
+ self.assertEqual(400, response.status)
+ self.assertEqual(
+ "A public snap cannot have a private owner.", response.body)
+ response = private_webservice.patch(
+ git_snap_url, "application/json",
+ json.dumps({"git_ref_link": private_ref_url}))
+ self.assertEqual(400, response.status)
+ self.assertEqual(
+ "A public snap cannot have a private branch.", response.body)
+
def test_cannot_set_git_path_for_bzr(self):
# Setting git_path on a Bazaar-based Snap fails.
snap = self.makeSnap(branch=self.factory.makeAnyBranch())
Follow ups
