launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #19956
[Merge] lp:~cprov/launchpad/snap-privacy-take2 into lp:launchpad
Celso Providelo has proposed merging lp:~cprov/launchpad/snap-privacy-take2 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~cprov/launchpad/snap-privacy-take2/+merge/285147
Preventing private snaps to be create via new feature-flag, so we can have more time to evaluate implications of underlying permission changes on branches.
Updating browser code to prevent +add-snap on private content according to the new feature-flag.
Also re-fixing ViewSnap to restrict access only to owners of the snap, not users who can view the owner (sounds trickier than it is, in fact).
--
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cprov/launchpad/snap-privacy-take2 into lp:launchpad.
=== modified file 'lib/lp/security.py'
--- lib/lp/security.py 2016-02-04 00:45:12 +0000
+++ lib/lp/security.py 2016-02-05 14:47:09 +0000
@@ -3121,9 +3121,13 @@
return not self.obj.private
def checkAuthenticated(self, user):
+ if not self.obj.private:
+ return True
+
return (
- not self.obj.private or
- self.forwardCheckAuthenticated(user, self.obj.owner))
+ user.isOwner(self.obj) or
+ user.in_commercial_admin or
+ user.in_admin)
class EditSnap(AuthorizationBase):
=== modified file 'lib/lp/snappy/browser/hassnaps.py'
--- lib/lp/snappy/browser/hassnaps.py 2015-10-05 13:36:06 +0000
+++ lib/lp/snappy/browser/hassnaps.py 2016-02-05 14:47:09 +0000
@@ -22,6 +22,7 @@
from lp.snappy.interfaces.snap import (
ISnapSet,
SNAP_FEATURE_FLAG,
+ SNAP_PRIVATE_FEATURE_FLAG,
)
@@ -38,10 +39,16 @@
return Link('+snaps', text, icon='info', enabled=enabled)
def create_snap(self):
- # You can't yet create a snap for a private branch.
- enabled = (
- bool(getFeatureFlag(SNAP_FEATURE_FLAG)) and
- not self.context.private)
+ # Only enabled if the general snap feature flag is enabled
+ # for public contexts and additionally if the snap_private
+ # flag is enabled for private contexts.
+ if not bool(getFeatureFlag(SNAP_FEATURE_FLAG)):
+ enabled = False
+ else:
+ enabled = (
+ not self.context.private or
+ bool(getFeatureFlag(SNAP_PRIVATE_FEATURE_FLAG)))
+
text = 'Create snap package'
return Link('+new-snap', text, enabled=enabled, icon='add')
=== modified file 'lib/lp/snappy/browser/snap.py'
--- lib/lp/snappy/browser/snap.py 2016-02-04 00:59:43 +0000
+++ lib/lp/snappy/browser/snap.py 2016-02-05 14:47:09 +0000
@@ -36,6 +36,8 @@
)
from lp.app.browser.lazrjs import InlinePersonEditPickerWidget
from lp.app.browser.tales import format_link
+from lp.app.enums import PRIVATE_INFORMATION_TYPES
+from lp.app.interfaces.informationtype import IInformationType
from lp.app.interfaces.launchpad import ILaunchpadCelebrities
from lp.app.widgets.itemswidgets import (
LabeledMultiCheckBoxWidget,
@@ -69,8 +71,10 @@
ISnapSet,
NoSuchSnap,
SNAP_FEATURE_FLAG,
+ SNAP_PRIVATE_FEATURE_FLAG,
SnapBuildAlreadyPending,
SnapFeatureDisabled,
+ SnapPrivateFeatureDisabled,
)
from lp.snappy.interfaces.snapbuild import ISnapBuildSet
from lp.soyuz.browser.archive import EnableProcessorsMixin
@@ -299,8 +303,16 @@
"""See `LaunchpadView`."""
if not getFeatureFlag(SNAP_FEATURE_FLAG):
raise SnapFeatureDisabled
+
super(SnapAddView, self).initialize()
+ # Once initialized, if the private_snap flag is disabled, it
+ # prevents snap creation for private contexts.
+ if not getFeatureFlag(SNAP_PRIVATE_FEATURE_FLAG):
+ if (IInformationType.providedBy(self.context) and
+ self.context.information_type in PRIVATE_INFORMATION_TYPES):
+ raise SnapPrivateFeatureDisabled
+
@property
def cancel_url(self):
return canonical_url(self.context)
=== modified file 'lib/lp/snappy/browser/tests/test_snap.py'
--- lib/lp/snappy/browser/tests/test_snap.py 2016-02-04 13:00:14 +0000
+++ lib/lp/snappy/browser/tests/test_snap.py 2016-02-05 14:47:09 +0000
@@ -24,6 +24,7 @@
from zope.publisher.interfaces import NotFound
from zope.security.interfaces import Unauthorized
+from lp.app.enums import InformationType
from lp.app.interfaces.launchpad import ILaunchpadCelebrities
from lp.buildmaster.enums import BuildStatus
from lp.buildmaster.interfaces.processor import IProcessorSet
@@ -42,13 +43,17 @@
from lp.snappy.interfaces.snap import (
CannotModifySnapProcessor,
SNAP_FEATURE_FLAG,
+ SNAP_TESTING_FLAGS,
SnapFeatureDisabled,
+ SnapPrivateFeatureDisabled,
)
from lp.testing import (
BrowserTestCase,
+ feature_flags,
login,
login_person,
person_logged_in,
+ set_feature_flag,
TestCaseWithFactory,
time_counter,
)
@@ -79,7 +84,7 @@
def setUp(self):
super(TestSnapNavigation, self).setUp()
- self.useFixture(FeatureFixture({SNAP_FEATURE_FLAG: u"on"}))
+ self.useFixture(FeatureFixture(SNAP_TESTING_FLAGS))
def test_canonical_url(self):
owner = self.factory.makePerson(name="person")
@@ -105,6 +110,19 @@
self.assertRaises(
SnapFeatureDisabled, create_initialized_view, branch, "+new-snap")
+ def test_private_feature_flag_disabled(self):
+ # Without a private_snap feature flag, we will not create Snaps for
+ # private contexts.
+ owner = self.factory.makePerson()
+ branch = self.factory.makeAnyBranch(
+ owner=owner, information_type=InformationType.USERDATA)
+ with feature_flags():
+ set_feature_flag(SNAP_FEATURE_FLAG, u'on')
+ with person_logged_in(owner):
+ self.assertRaises(
+ SnapPrivateFeatureDisabled, create_initialized_view,
+ branch, "+new-snap")
+
class TestSnapAddView(BrowserTestCase):
@@ -112,7 +130,7 @@
def setUp(self):
super(TestSnapAddView, self).setUp()
- self.useFixture(FeatureFixture({SNAP_FEATURE_FLAG: u"on"}))
+ self.useFixture(FeatureFixture(SNAP_TESTING_FLAGS))
self.useFixture(FakeLogger())
self.person = self.factory.makePerson(
name="test-person", displayname="Test Person")
@@ -215,6 +233,22 @@
extract_text(find_tag_by_id(browser.contents, "privacy"))
)
+ def test_create_new_snap_private_link(self):
+ # Link for create new snaps for private content is only displayed
+ # if the 'snap.allow_private' is enabled.
+ login_person(self.person)
+ branch = self.factory.makeAnyBranch(
+ owner=self.person,
+ information_type=InformationType.USERDATA)
+
+ browser = self.getViewBrowser(branch, user=self.person)
+ browser.getLink('Create snap package')
+
+ with FeatureFixture({SNAP_FEATURE_FLAG: u'on'}):
+ browser = self.getViewBrowser(branch, user=self.person)
+ self.assertRaises(
+ LinkNotFoundError, browser.getLink, "Create snap package")
+
def test_create_new_snap_private(self):
# Private teams will automatically create private snaps.
login_person(self.person)
@@ -243,7 +277,7 @@
def setUp(self):
super(TestSnapAdminView, self).setUp()
- self.useFixture(FeatureFixture({SNAP_FEATURE_FLAG: u"on"}))
+ self.useFixture(FeatureFixture(SNAP_TESTING_FLAGS))
self.useFixture(FakeLogger())
self.person = self.factory.makePerson(
name="test-person", displayname="Test Person")
@@ -263,14 +297,14 @@
def test_admin_snap(self):
# Admins can change require_virtualized and privacy.
login("admin@xxxxxxxxxxxxx")
- ppa_admin = self.factory.makePerson(
- member_of=[getUtility(ILaunchpadCelebrities).ppa_admin])
+ commercial_admin = self.factory.makePerson(
+ member_of=[getUtility(ILaunchpadCelebrities).commercial_admin])
login_person(self.person)
snap = self.factory.makeSnap(registrant=self.person)
self.assertTrue(snap.require_virtualized)
self.assertFalse(snap.private)
- browser = self.getViewBrowser(snap, user=ppa_admin)
+ browser = self.getViewBrowser(snap, user=commercial_admin)
browser.getLink("Administer snap package").click()
browser.getControl("Require virtualized builders").selected = False
browser.getControl("Private").selected = True
@@ -321,7 +355,7 @@
def setUp(self):
super(TestSnapEditView, self).setUp()
- self.useFixture(FeatureFixture({SNAP_FEATURE_FLAG: u"on"}))
+ self.useFixture(FeatureFixture(SNAP_TESTING_FLAGS))
self.useFixture(FakeLogger())
self.person = self.factory.makePerson(
name="test-person", displayname="Test Person")
@@ -531,7 +565,7 @@
def setUp(self):
super(TestSnapDeleteView, self).setUp()
- self.useFixture(FeatureFixture({SNAP_FEATURE_FLAG: u"on"}))
+ self.useFixture(FeatureFixture(SNAP_TESTING_FLAGS))
self.person = self.factory.makePerson(
name="test-person", displayname="Test Person")
@@ -577,7 +611,7 @@
def setUp(self):
super(TestSnapView, self).setUp()
- self.useFixture(FeatureFixture({SNAP_FEATURE_FLAG: u"on"}))
+ self.useFixture(FeatureFixture(SNAP_TESTING_FLAGS))
self.ubuntu = getUtility(ILaunchpadCelebrities).ubuntu
self.distroseries = self.factory.makeDistroSeries(
distribution=self.ubuntu, name="shiny", displayname="Shiny")
@@ -748,7 +782,7 @@
def setUp(self):
super(TestSnapRequestBuildsView, self).setUp()
- self.useFixture(FeatureFixture({SNAP_FEATURE_FLAG: u"on"}))
+ self.useFixture(FeatureFixture(SNAP_TESTING_FLAGS))
self.useFixture(FakeLogger())
self.ubuntu = getUtility(ILaunchpadCelebrities).ubuntu
self.distroseries = self.factory.makeDistroSeries(
=== modified file 'lib/lp/snappy/interfaces/snap.py'
--- lib/lp/snappy/interfaces/snap.py 2016-02-04 00:45:12 +0000
+++ lib/lp/snappy/interfaces/snap.py 2016-02-05 14:47:09 +0000
@@ -16,12 +16,15 @@
'NoSourceForSnap',
'NoSuchSnap',
'SNAP_FEATURE_FLAG',
+ 'SNAP_PRIVATE_FEATURE_FLAG',
+ 'SNAP_TESTING_FLAGS',
'SnapBuildAlreadyPending',
'SnapBuildArchiveOwnerMismatch',
'SnapBuildDisallowedArchitecture',
'SnapFeatureDisabled',
'SnapNotOwner',
'SnapPrivacyMismatch',
+ 'SnapPrivateFeatureDisabled',
]
import httplib
@@ -87,6 +90,13 @@
SNAP_FEATURE_FLAG = u"snap.allow_new"
+SNAP_PRIVATE_FEATURE_FLAG = u"snap.allow_private"
+
+
+SNAP_TESTING_FLAGS = {
+ SNAP_FEATURE_FLAG: u"on",
+ SNAP_PRIVATE_FEATURE_FLAG: u"on",
+}
@error_status(httplib.BAD_REQUEST)
@@ -136,6 +146,15 @@
"builds.")
+@error_status(httplib.UNAUTHORIZED)
+class SnapPrivateFeatureDisabled(Unauthorized):
+ """Only certain users can create private snap objects."""
+
+ def __init__(self):
+ super(SnapPrivateFeatureDisabled, self).__init__(
+ "You do not have permission to create private snaps")
+
+
@error_status(httplib.BAD_REQUEST)
class DuplicateSnapName(Exception):
"""Raised for snap packages with duplicate name/owner."""
=== modified file 'lib/lp/snappy/model/snap.py'
--- lib/lp/snappy/model/snap.py 2016-02-04 13:00:14 +0000
+++ lib/lp/snappy/model/snap.py 2016-02-05 14:47:09 +0000
@@ -81,12 +81,14 @@
NoSourceForSnap,
NoSuchSnap,
SNAP_FEATURE_FLAG,
+ SNAP_PRIVATE_FEATURE_FLAG,
SnapBuildAlreadyPending,
SnapBuildArchiveOwnerMismatch,
SnapBuildDisallowedArchitecture,
SnapFeatureDisabled,
SnapNotOwner,
SnapPrivacyMismatch,
+ SnapPrivateFeatureDisabled,
)
from lp.snappy.interfaces.snapbuild import ISnapBuildSet
from lp.snappy.model.snapbuild import SnapBuild
@@ -411,8 +413,11 @@
def isValidPrivacy(self, private, owner, branch=None, git_ref=None):
"""See `ISnapSet`."""
- # Private snaps may contain anything.
+ # Private snaps may contain anything ...
if private:
+ # If appropriately enabled via feature flag.
+ if not getFeatureFlag(SNAP_PRIVATE_FEATURE_FLAG):
+ raise SnapPrivateFeatureDisabled
return True
# Public snaps with private sources are not allowed.
=== modified file 'lib/lp/snappy/tests/test_snap.py'
--- lib/lp/snappy/tests/test_snap.py 2016-02-04 00:45:12 +0000
+++ lib/lp/snappy/tests/test_snap.py 2016-02-05 14:47:09 +0000
@@ -42,19 +42,23 @@
ISnapView,
NoSourceForSnap,
SNAP_FEATURE_FLAG,
+ SNAP_TESTING_FLAGS,
SnapBuildAlreadyPending,
SnapBuildDisallowedArchitecture,
SnapFeatureDisabled,
SnapPrivacyMismatch,
+ SnapPrivateFeatureDisabled,
)
from lp.snappy.interfaces.snapbuild import ISnapBuild
from lp.testing import (
admin_logged_in,
ANONYMOUS,
api_url,
+ feature_flags,
login,
logout,
person_logged_in,
+ set_feature_flag,
StormStatementRecorder,
TestCaseWithFactory,
)
@@ -81,6 +85,16 @@
SnapFeatureDisabled, getUtility(ISnapSet).new,
person, person, None, None, branch=self.factory.makeAnyBranch())
+ def test_private_feature_flag_disabled(self):
+ # Without a private feature flag, we will not create new private Snaps.
+ person = self.factory.makePerson()
+ with feature_flags():
+ set_feature_flag(SNAP_FEATURE_FLAG, u'on')
+ self.assertRaises(
+ SnapPrivateFeatureDisabled, getUtility(ISnapSet).new,
+ person, person, None, None,
+ branch=self.factory.makeAnyBranch(), private=True)
+
class TestSnap(TestCaseWithFactory):
@@ -88,7 +102,7 @@
def setUp(self):
super(TestSnap, self).setUp()
- self.useFixture(FeatureFixture({SNAP_FEATURE_FLAG: u"on"}))
+ self.useFixture(FeatureFixture(SNAP_TESTING_FLAGS))
def test_implements_interfaces(self):
# Snap implements ISnap.
@@ -364,7 +378,7 @@
def setUp(self):
super(TestSnapSet, self).setUp()
- self.useFixture(FeatureFixture({SNAP_FEATURE_FLAG: u"on"}))
+ self.useFixture(FeatureFixture(SNAP_TESTING_FLAGS))
def test_class_implements_interfaces(self):
# The SnapSet class implements ISnapSet.
@@ -668,7 +682,7 @@
def setUp(self):
super(TestSnapProcessors, self).setUp(user="foo.bar@xxxxxxxxxxxxx")
- self.useFixture(FeatureFixture({SNAP_FEATURE_FLAG: u"on"}))
+ self.useFixture(FeatureFixture(SNAP_TESTING_FLAGS))
self.default_procs = [
getUtility(IProcessorSet).getByName("386"),
getUtility(IProcessorSet).getByName("amd64")]
@@ -755,7 +769,7 @@
def setUp(self):
super(TestSnapWebservice, self).setUp()
- self.useFixture(FeatureFixture({SNAP_FEATURE_FLAG: u"on"}))
+ self.useFixture(FeatureFixture(SNAP_TESTING_FLAGS))
self.person = self.factory.makePerson(displayname="Test Person")
self.webservice = webservice_for_person(
self.person, permission=OAuthPermission.WRITE_PUBLIC)
=== modified file 'lib/lp/snappy/tests/test_snapbuild.py'
--- lib/lp/snappy/tests/test_snapbuild.py 2016-02-04 00:45:12 +0000
+++ lib/lp/snappy/tests/test_snapbuild.py 2016-02-05 14:47:09 +0000
@@ -30,7 +30,7 @@
from lp.services.librarian.browser import ProxiedLibraryFileAlias
from lp.services.webapp.interfaces import OAuthPermission
from lp.snappy.interfaces.snap import (
- SNAP_FEATURE_FLAG,
+ SNAP_TESTING_FLAGS,
SnapFeatureDisabled,
)
from lp.snappy.interfaces.snapbuild import (
@@ -89,7 +89,7 @@
def setUp(self):
super(TestSnapBuild, self).setUp()
- self.useFixture(FeatureFixture({SNAP_FEATURE_FLAG: u"on"}))
+ self.useFixture(FeatureFixture(SNAP_TESTING_FLAGS))
self.build = self.factory.makeSnapBuild()
def test_implements_interfaces(self):
@@ -127,7 +127,8 @@
visibility=PersonVisibility.PRIVATE)
with person_logged_in(private_team.teamowner):
build = self.factory.makeSnapBuild(
- requester=private_team.teamowner, owner=private_team, private=True)
+ requester=private_team.teamowner, owner=private_team,
+ private=True)
self.assertTrue(build.is_private)
private_archive = self.factory.makeArchive(private=True)
with person_logged_in(private_archive.owner):
@@ -287,7 +288,7 @@
def setUp(self):
super(TestSnapBuildSet, self).setUp()
- self.useFixture(FeatureFixture({SNAP_FEATURE_FLAG: u"on"}))
+ self.useFixture(FeatureFixture(SNAP_TESTING_FLAGS))
def test_getByBuildFarmJob_works(self):
build = self.factory.makeSnapBuild()
@@ -318,7 +319,7 @@
def setUp(self):
super(TestSnapBuildWebservice, self).setUp()
- self.useFixture(FeatureFixture({SNAP_FEATURE_FLAG: u"on"}))
+ self.useFixture(FeatureFixture(SNAP_TESTING_FLAGS))
self.person = self.factory.makePerson()
self.webservice = webservice_for_person(
self.person, permission=OAuthPermission.WRITE_PRIVATE)
Follow ups
