launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #06517
[Merge] lp:~wgrant/launchpad/archive-view into lp:launchpad
William Grant has proposed merging lp:~wgrant/launchpad/archive-view into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~wgrant/launchpad/archive-view/+merge/94941
This branch moves most stuff from IArchivePublic to IArchiveView, hopefully preventing some data leakage from private archives. A few tests needed adjusting to cope (they tried to poke internal methods that now require launchpad.View), and I removed test_packages_link_unauthorized entirely (it was asserting that a user without launchpad.View couldn't see a menu item on a page that requires launchpad.View).
--
https://code.launchpad.net/~wgrant/launchpad/archive-view/+merge/94941
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/archive-view into lp:launchpad.
=== modified file 'lib/lp/code/model/tests/test_sourcepackagerecipe.py'
--- lib/lp/code/model/tests/test_sourcepackagerecipe.py 2012-02-11 02:28:33 +0000
+++ lib/lp/code/model/tests/test_sourcepackagerecipe.py 2012-02-28 11:20:25 +0000
@@ -372,8 +372,9 @@
ppa = self.factory.makeArchive()
removeSecurityProxy(ppa).disable()
(distroseries,) = list(recipe.distroseries)
- self.assertRaises(ArchiveDisabled, recipe.requestBuild, ppa,
- ppa.owner, distroseries, PackagePublishingPocket.RELEASE)
+ with person_logged_in(ppa.owner):
+ self.assertRaises(ArchiveDisabled, recipe.requestBuild, ppa,
+ ppa.owner, distroseries, PackagePublishingPocket.RELEASE)
def test_requestBuildScore(self):
"""Normal build requests have a relatively low queue score (2405)."""
=== modified file 'lib/lp/soyuz/browser/tests/test_archive_packages.py'
--- lib/lp/soyuz/browser/tests/test_archive_packages.py 2012-01-15 13:32:27 +0000
+++ lib/lp/soyuz/browser/tests/test_archive_packages.py 2012-02-28 11:20:25 +0000
@@ -108,12 +108,6 @@
menu = ArchiveNavigationMenu(view)
self.assertTrue(menu.packages().enabled)
- def test_packages_link_unauthorized(self):
- login_person(self.fred)
- view = create_initialized_view(self.private_ppa, "+index")
- menu = ArchiveNavigationMenu(view)
- self.assertFalse(menu.packages().enabled)
-
def test_packages_link_subscriber(self):
login_person(self.joe)
view = create_initialized_view(self.private_ppa, "+index")
=== modified file 'lib/lp/soyuz/doc/archive.txt'
--- lib/lp/soyuz/doc/archive.txt 2012-01-09 14:51:17 +0000
+++ lib/lp/soyuz/doc/archive.txt 2012-02-28 11:20:25 +0000
@@ -262,8 +262,6 @@
>>> print rebuild_archive.name
there-we-go
- >>> login(ANONYMOUS)
-
Please note that copy archive displayname doesn't follow the name change.
>>> rebuild_archive.displayname
@@ -1348,7 +1346,10 @@
Iteration over the own utility is performed against all archives,
including PPA, DEBUG, PRIMARY, PARTNER and COPY:
- >>> archive_purposes = [archive.purpose.name for archive in archive_set]
+ >>> from lp.testing import celebrity_logged_in
+ >>> with celebrity_logged_in('admin'):
+ ... archive_purposes = [
+ ... archive.purpose.name for archive in archive_set]
>>> len(archive_purposes)
18
@@ -1631,9 +1632,11 @@
PPA the list is not updated. The same happens for uploaded sources, since
they are essentially another source publication in this context.
- >>> copy = cprov_cdrkit.copyTo(
- ... ubuntu['hoary'], PackagePublishingPocket.RELEASE,
- ... cprov_private_ppa)
+ >>> from lp.testing import person_logged_in
+ >>> with person_logged_in(cprov):
+ ... copy = cprov_cdrkit.copyTo(
+ ... ubuntu['hoary'], PackagePublishingPocket.RELEASE,
+ ... cprov_private_ppa)
>>> print_latest_uploads()
cdrkit 1.0 in hoary PENDING mark
=== modified file 'lib/lp/soyuz/interfaces/archive.py'
--- lib/lp/soyuz/interfaces/archive.py 2012-02-18 00:45:05 +0000
+++ lib/lp/soyuz/interfaces/archive.py 2012-02-28 11:20:25 +0000
@@ -264,8 +264,14 @@
self.errors = errors
-class IArchivePublic(IHasOwner, IPrivacy):
+class IArchivePublic(IPrivacy, IHasOwner):
"""An Archive interface for publicly available operations."""
+ # Most of this stuff should really be on View, but it's needed for
+ # security checks and URL generation and things like that.
+ # Others are presently needed because invisible (private or disabled)
+ # archives can show up in copy histories and archive dependency
+ # lists.
+
id = Attribute("The archive ID.")
owner = exported(
@@ -287,19 +293,17 @@
title=_("Display name"), required=True,
description=_("A short title for the archive.")))
- title = TextLine(title=_("Name"), required=False, readonly=True)
+ distribution = exported(
+ Reference(
+ Interface, # Redefined to IDistribution later.
+ title=_("The distribution that uses or is used by this "
+ "archive.")))
enabled = Bool(
title=_("Enabled"), required=False,
description=_(
"Accept and build packages uploaded to the archive."))
- publish = Bool(
- title=_("Publish"), required=False,
- description=_("Whether or not to update the APT repository. If "
- "disabled, nothing will be published. If the archive is "
- "private then additionally no builds will be dispatched."))
-
# This is redefined from IPrivacy.private because the attribute is
# read-only. The value is guarded by a validator.
private = exported(
@@ -310,6 +314,46 @@
"subscribers. This can only be changed if the archive has "
"never had any sources published.")))
+ is_ppa = Attribute("True if this archive is a PPA.")
+
+ is_main = Bool(
+ title=_("True if archive is a main archive type"), required=False)
+
+ commercial = exported(
+ Bool(
+ title=_("Commercial"),
+ required=True,
+ description=_(
+ "Display the archive in Software Center's commercial "
+ "listings. Only private archives can be commercial.")))
+
+ def checkArchivePermission(person, component_or_package=None):
+ """Check to see if person is allowed to upload to component.
+
+ :param person: An `IPerson` whom should be checked for authentication.
+ :param component_or_package: The context `IComponent` or an
+ `ISourcePackageName` for the check. This parameter is
+ not required if the archive is a PPA.
+
+ :return: True if 'person' is allowed to upload to the specified
+ component or package name.
+ :raise TypeError: If component_or_package is not one of
+ `IComponent` or `ISourcePackageName`.
+
+ """
+
+
+class IArchiveView(IHasBuildRecords):
+ """Archive interface for operations restricted by view privilege."""
+
+ title = TextLine(title=_("Name"), required=False, readonly=True)
+
+ publish = Bool(
+ title=_("Publish"), required=False,
+ description=_("Whether or not to update the APT repository. If "
+ "disabled, nothing will be published. If the archive is "
+ "private then additionally no builds will be dispatched."))
+
require_virtualized = exported(
Bool(
title=_("Require virtualized builders"), required=False,
@@ -347,12 +391,6 @@
"Concatenation of the source and binary packages published in this "
"archive. Its content is used for indexed searches across archives.")
- distribution = exported(
- Reference(
- Interface, # Redefined to IDistribution later.
- title=_("The distribution that uses or is used by this "
- "archive.")))
-
signing_key = Object(
title=_('Repository sigining key.'), required=False, schema=IGPGKey)
@@ -367,15 +405,10 @@
archive_url = Attribute("External archive URL.")
- is_ppa = Attribute("True if this archive is a PPA.")
-
is_partner = Attribute("True if this archive is a partner archive.")
is_copy = Attribute("True if this archive is a copy archive.")
- is_main = Bool(
- title=_("True if archive is a main archive type"), required=False)
-
is_active = Bool(
title=_("True if the archive is in the active state"),
required=False, readonly=True)
@@ -452,14 +485,6 @@
readonly=True),
as_of='devel')
- commercial = exported(
- Bool(
- title=_("Commercial"),
- required=True,
- description=_(
- "Display the archive in Software Center's commercial "
- "listings. Only private archives can be commercial.")))
-
def getSourcesForDeletion(name=None, status=None, distroseries=None):
"""All `ISourcePackagePublishingHistory` available for deletion.
@@ -549,21 +574,6 @@
:return: A list of `IArchivePermission` records.
"""
- def checkArchivePermission(person, component_or_package=None):
- """Check to see if person is allowed to upload to component.
-
- :param person: An `IPerson` whom should be checked for authentication.
- :param component_or_package: The context `IComponent` or an
- `ISourcePackageName` for the check. This parameter is
- not required if the archive is a PPA.
-
- :return: True if 'person' is allowed to upload to the specified
- component or package name.
- :raise TypeError: If component_or_package is not one of
- `IComponent` or `ISourcePackageName`.
-
- """
-
def canUploadSuiteSourcePackage(person, suitesourcepackage):
"""Check if 'person' upload 'suitesourcepackage' to 'archive'.
@@ -892,9 +902,6 @@
"""Returns an instantiated `IOverridePolicy` for the archive."""
-class IArchiveView(IHasBuildRecords):
- """Archive interface for operations restricted by view privilege."""
-
buildd_secret = TextLine(
title=_("Build farm secret"), required=False,
description=_(
=== modified file 'lib/lp/soyuz/stories/webservice/xx-archive.txt'
--- lib/lp/soyuz/stories/webservice/xx-archive.txt 2012-02-18 00:45:05 +0000
+++ lib/lp/soyuz/stories/webservice/xx-archive.txt 2012-02-28 11:20:25 +0000
@@ -976,18 +976,18 @@
the IArchive context, in this case only Celso has it.
>>> pprint_entry(user_webservice.get("/~cprov/+archive/p3a").jsonBody())
- authorized_size: 2048
+ authorized_size: u'tag:launchpad.net:2008:redacted'
commercial: False
dependencies_collection_link:
u'http://.../~cprov/+archive/p3a/dependencies'
description: u'tag:launchpad.net:2008:redacted'
displayname: u'PPA named p3a for Celso Providelo'
distribution_link: u'http://.../ubuntu'
- external_dependencies: None
+ external_dependencies: u'tag:launchpad.net:2008:redacted'
name: u'p3a'
owner_link: u'http://.../~cprov'
private: True
- require_virtualized: True
+ require_virtualized: u'tag:launchpad.net:2008:redacted'
resource_type_link: u'http://.../#archive'
self_link: u'http://.../~cprov/+archive/p3a'
signing_key_fingerprint: u'tag:launchpad.net:2008:redacted'
=== modified file 'lib/lp/soyuz/tests/test_archive.py'
--- lib/lp/soyuz/tests/test_archive.py 2012-01-18 14:30:52 +0000
+++ lib/lp/soyuz/tests/test_archive.py 2012-02-28 11:20:25 +0000
@@ -616,7 +616,8 @@
def test_checkUpload_disabled_archive(self):
archive, distroseries = self.makeArchiveAndActiveDistroSeries(
purpose=ArchivePurpose.PRIMARY)
- removeSecurityProxy(archive).disable()
+ archive = removeSecurityProxy(archive)
+ archive.disable()
self.assertCannotUpload(
ArchiveDisabled, archive, self.factory.makePerson(),
self.factory.makeSourcePackageName(),
=== modified file 'lib/lp/soyuz/tests/test_archive_subscriptions.py'
--- lib/lp/soyuz/tests/test_archive_subscriptions.py 2012-01-01 02:58:52 +0000
+++ lib/lp/soyuz/tests/test_archive_subscriptions.py 2012-02-28 11:20:25 +0000
@@ -72,16 +72,18 @@
# Before a subscription, accessing the view name will raise.
login_person(self.subscriber)
+ self.assertRaises(
+ Unauthorized, create_initialized_view,
+ self.archive, '+index', principal=self.subscriber)
+
+ login_person(self.owner)
+ self.archive.newSubscription(
+ self.subscriber, registrant=self.archive.owner)
+
+ # When a subscription exists, it's fine.
+ login_person(self.subscriber)
view = create_initialized_view(
self.archive, '+index', principal=self.subscriber)
- self.assertRaises(Unauthorized, view.render)
-
- login_person(self.owner)
- self.archive.newSubscription(
- self.subscriber, registrant=self.archive.owner)
-
- # When a subscription exists, it's fine.
- login_person(self.subscriber)
self.assertIn(self.archive.displayname, view.render())
# Just to double check, by default, the subscriber still can't see the
=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py 2012-02-27 06:14:39 +0000
+++ lib/lp/testing/factory.py 2012-02-28 11:20:25 +0000
@@ -2690,11 +2690,12 @@
if purpose == ArchivePurpose.PRIMARY:
return distribution.main_archive
- archive = getUtility(IArchiveSet).new(
- owner=owner, purpose=purpose,
- distribution=distribution, name=name, displayname=displayname,
- enabled=enabled, require_virtualized=virtualized,
- description=description)
+ with person_logged_in(owner):
+ archive = getUtility(IArchiveSet).new(
+ owner=owner, purpose=purpose,
+ distribution=distribution, name=name, displayname=displayname,
+ enabled=enabled, require_virtualized=virtualized,
+ description=description)
if private:
naked_archive = removeSecurityProxy(archive)
@@ -3749,14 +3750,15 @@
status = BuildStatus.NEEDSBUILD
if date_created is None:
date_created = self.getUniqueDate()
- binary_package_build = getUtility(IBinaryPackageBuildSet).new(
- source_package_release=source_package_release,
- processor=processor,
- distro_arch_series=distroarchseries,
- status=status,
- archive=archive,
- pocket=pocket,
- date_created=date_created)
+ with person_logged_in(archive.owner):
+ binary_package_build = getUtility(IBinaryPackageBuildSet).new(
+ source_package_release=source_package_release,
+ processor=processor,
+ distro_arch_series=distroarchseries,
+ status=status,
+ archive=archive,
+ pocket=pocket,
+ date_created=date_created)
naked_build = removeSecurityProxy(binary_package_build)
naked_build.builder = builder
IStore(binary_package_build).flush()
@@ -3823,10 +3825,11 @@
archive=archive, distroseries=distroseries,
date_uploaded=date_uploaded, **kwargs)
- spph = getUtility(IPublishingSet).newSourcePublication(
- archive, sourcepackagerelease, distroseries,
- sourcepackagerelease.component, sourcepackagerelease.section,
- pocket, ancestor)
+ with person_logged_in(archive.owner):
+ spph = getUtility(IPublishingSet).newSourcePublication(
+ archive, sourcepackagerelease, distroseries,
+ sourcepackagerelease.component, sourcepackagerelease.section,
+ pocket, ancestor)
naked_spph = removeSecurityProxy(spph)
naked_spph.status = status
