launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #10916
[Merge] lp:~cjwatson/launchpad/pcj-auto-approve-pocket into lp:launchpad
Colin Watson has proposed merging lp:~cjwatson/launchpad/pcj-auto-approve-pocket into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #1036544 in Launchpad itself: "PackageCopyJob auto_approve doesn't handle pocket queue admin permissions"
https://bugs.launchpad.net/launchpad/+bug/1036544
For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/pcj-auto-approve-pocket/+merge/119497
== Summary ==
I missed a bit in my recent work on pocket queue admin permissions and the PCJ auto_approve flag, and auto_approve doesn't handle the case where the copier only has pocket queue admin permissions as opposed to component ones. This is obviously wrong, and it's the last thing that stops the Ubuntu security team converting fully to this.
== Proposed fix ==
Add pocket and distroseries arguments to the relevant Archive.canAdministerQueue calls.
== Implementation details ==
I refactored various PCJ tests to gain LoC.
== Tests ==
bin/test -vvc lp.soyuz.tests.test_packagecopyjob
== Demo and Q/A ==
Join ubuntu-security on dogfood (which has queue admin on the SECURITY pocket), arrange not to have any other queue admin permissions, and copy a package from a PPA into the SECURITY pocket with auto_approve=True (ideally using security-tools/unembargo from ubuntu-qa-tools, tweaked to talk to dogfood). The copy should go straight through without requiring approval.
--
https://code.launchpad.net/~cjwatson/launchpad/pcj-auto-approve-pocket/+merge/119497
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/pcj-auto-approve-pocket into lp:launchpad.
=== modified file 'lib/lp/soyuz/model/packagecopyjob.py'
--- lib/lp/soyuz/model/packagecopyjob.py 2012-08-08 13:53:44 +0000
+++ lib/lp/soyuz/model/packagecopyjob.py 2012-08-14 09:52:46 +0000
@@ -480,7 +480,8 @@
self.addSourceOverride(defaults[0])
if auto_approve:
auto_approve = self.target_archive.canAdministerQueue(
- self.requester, self.getSourceOverride().component)
+ self.requester, self.getSourceOverride().component,
+ self.target_pocket, self.target_distroseries)
approve_new = auto_approve or copy_policy.autoApproveNew(
self.target_archive, self.target_distroseries,
@@ -496,7 +497,8 @@
self.addSourceOverride(ancestry[0])
if auto_approve:
auto_approve = self.target_archive.canAdministerQueue(
- self.requester, self.getSourceOverride().component)
+ self.requester, self.getSourceOverride().component,
+ self.target_pocket, self.target_distroseries)
# The package is not new (it has ancestry) so check the copy
# policy for existing packages.
=== modified file 'lib/lp/soyuz/tests/test_packagecopyjob.py'
--- lib/lp/soyuz/tests/test_packagecopyjob.py 2012-08-08 13:53:44 +0000
+++ lib/lp/soyuz/tests/test_packagecopyjob.py 2012-08-14 09:52:46 +0000
@@ -56,9 +56,7 @@
from lp.soyuz.interfaces.sourcepackageformat import (
ISourcePackageFormatSelectionSet,
)
-from lp.soyuz.model.distroseriesdifferencejob import (
- FEATURE_FLAG_ENABLE_MODULE,
- )
+from lp.soyuz.model.distroseriesdifferencejob import FEATURE_FLAG_ENABLE_MODULE
from lp.soyuz.model.packagecopyjob import PackageCopyJob
from lp.soyuz.model.queue import PackageUpload
from lp.soyuz.tests.test_publishing import SoyuzTestPublisher
@@ -211,16 +209,16 @@
copy_policy=PackageCopyPolicy.MASS_SYNC,
requester=requester, sponsored=sponsored)
self.assertProvides(job, IPackageCopyJob)
- self.assertEquals(archive1.id, job.source_archive_id)
- self.assertEquals(archive1, job.source_archive)
- self.assertEquals(archive2.id, job.target_archive_id)
- self.assertEquals(archive2, job.target_archive)
- self.assertEquals(distroseries, job.target_distroseries)
- self.assertEquals(PackagePublishingPocket.RELEASE, job.target_pocket)
+ self.assertEqual(archive1.id, job.source_archive_id)
+ self.assertEqual(archive1, job.source_archive)
+ self.assertEqual(archive2.id, job.target_archive_id)
+ self.assertEqual(archive2, job.target_archive)
+ self.assertEqual(distroseries, job.target_distroseries)
+ self.assertEqual(PackagePublishingPocket.RELEASE, job.target_pocket)
self.assertEqual("foo", job.package_name)
self.assertEqual("1.0-1", job.package_version)
- self.assertEquals(False, job.include_binaries)
- self.assertEquals(PackageCopyPolicy.MASS_SYNC, job.copy_policy)
+ self.assertEqual(False, job.include_binaries)
+ self.assertEqual(PackageCopyPolicy.MASS_SYNC, job.copy_policy)
self.assertEqual(requester, job.requester)
self.assertEqual(sponsored, job.sponsored)
@@ -370,8 +368,7 @@
owner = pcj.target_archive.owner
switch_dbuser("launchpad_main")
with person_logged_in(owner):
- pcj.target_archive.newComponentUploader(
- pcj.requester, 'universe')
+ pcj.target_archive.newComponentUploader(pcj.requester, 'universe')
self.runJob(pcj)
new_publication = pcj.target_archive.getPublishedSources(
name=spn).one()
@@ -475,14 +472,10 @@
requester=requester)
oops_vars = job.getOopsVars()
naked_job = removeSecurityProxy(job)
- self.assertIn(
- ('source_archive_id', archive1.id), oops_vars)
- self.assertIn(
- ('target_archive_id', archive2.id), oops_vars)
- self.assertIn(
- ('target_distroseries_id', distroseries.id), oops_vars)
- self.assertIn(
- ('package_copy_job_id', naked_job.context.id), oops_vars)
+ self.assertIn(('source_archive_id', archive1.id), oops_vars)
+ self.assertIn(('target_archive_id', archive2.id), oops_vars)
+ self.assertIn(('target_distroseries_id', distroseries.id), oops_vars)
+ self.assertIn(('package_copy_job_id', naked_job.context.id), oops_vars)
self.assertIn(
('package_copy_job_type', naked_job.context.job_type.title),
oops_vars)
@@ -555,8 +548,7 @@
self.makeJob()
other_series = self.factory.makeDistroSeries()
job_source = getUtility(IPlainPackageCopyJobSource)
- self.assertEqual(
- {}, job_source.getPendingJobsPerPackage(other_series))
+ self.assertEqual({}, job_source.getPendingJobsPerPackage(other_series))
def test_getPendingJobsPerPackage_only_returns_pending_jobs(self):
# getPendingJobsPerPackage ignores jobs that have already been
@@ -670,14 +662,11 @@
with person_logged_in(target_archive.owner):
target_archive.newComponentUploader(requester, "restricted")
job = source.create(
- package_name="libc",
- package_version="2.8-1",
- source_archive=source_archive,
- target_archive=target_archive,
+ package_name="libc", package_version="2.8-1",
+ source_archive=source_archive, target_archive=target_archive,
target_distroseries=self.distroseries,
target_pocket=PackagePublishingPocket.RELEASE,
- include_binaries=False,
- requester=requester)
+ include_binaries=False, requester=requester)
self.runJob(job)
@@ -711,14 +700,11 @@
with person_logged_in(target_archive.owner):
target_archive.newComponentUploader(requester, "main")
job = source.create(
- package_name="libc",
- package_version="2.8-1",
- source_archive=source_archive,
- target_archive=target_archive,
+ package_name="libc", package_version="2.8-1",
+ source_archive=source_archive, target_archive=target_archive,
target_distroseries=self.distroseries,
target_pocket=PackagePublishingPocket.RELEASE,
- include_binaries=False,
- requester=requester)
+ include_binaries=False, requester=requester)
self.runJob(job)
self.assertEqual(JobStatus.COMPLETED, job.status)
@@ -749,14 +735,11 @@
with person_logged_in(target_archive.owner):
target_archive.newComponentUploader(requester, "main")
job = source.create(
- package_name="copyme",
- package_version="2.8-1",
- source_archive=source_archive,
- target_archive=target_archive,
+ package_name="copyme", package_version="2.8-1",
+ source_archive=source_archive, target_archive=target_archive,
target_distroseries=self.distroseries,
target_pocket=PackagePublishingPocket.RELEASE,
- include_binaries=False,
- requester=requester)
+ include_binaries=False, requester=requester)
self.runJob(job)
self.assertEqual(JobStatus.SUSPENDED, job.status)
@@ -806,14 +789,11 @@
source = getUtility(IPlainPackageCopyJobSource)
requester = self.factory.makePerson()
job = source.create(
- package_name="copyme",
- package_version="2.8-1",
- source_archive=source_archive,
- target_archive=target_archive,
+ package_name="copyme", package_version="2.8-1",
+ source_archive=source_archive, target_archive=target_archive,
target_distroseries=self.distroseries,
target_pocket=PackagePublishingPocket.RELEASE,
- include_binaries=False,
- requester=requester)
+ include_binaries=False, requester=requester)
# The job should be suspended and there's a PackageUpload with
# its package_copy_job set.
@@ -839,12 +819,9 @@
return source.create(
package_name=spph.sourcepackagerelease.name,
package_version=spph.sourcepackagerelease.version,
- source_archive=source_archive,
- target_archive=target_archive,
- target_distroseries=spph.distroseries,
- target_pocket=target_pocket,
- include_binaries=include_binaries,
- requester=requester,
+ source_archive=source_archive, target_archive=target_archive,
+ target_distroseries=spph.distroseries, target_pocket=target_pocket,
+ include_binaries=include_binaries, requester=requester,
**kwargs)
def createCopyJob(self, sourcename, component, section, version,
@@ -942,63 +919,78 @@
# the target archive.
self.assertEqual('main', pcj.metadata['component_override'])
- def createAutoApproveEnvironment(self, create_ancestry, component_names):
+ def createAutoApproveEnvironment(self, create_ancestry, component_names,
+ pocket_admin=False):
"""Create an environment for testing the auto_approve flag."""
if create_ancestry:
self.distroseries.status = SeriesStatus.FROZEN
- target_archive = self.factory.makeArchive(
+ target = self.factory.makeArchive(
self.distroseries.distribution, purpose=ArchivePurpose.PRIMARY)
- source_archive = self.factory.makeArchive()
+ source = self.factory.makeArchive()
requester = self.factory.makePerson()
- with person_logged_in(target_archive.owner):
+ with person_logged_in(target.owner):
for component_name in component_names:
- target_archive.newQueueAdmin(requester, component_name)
+ target.newQueueAdmin(requester, component_name)
+ if pocket_admin:
+ target.newPocketQueueAdmin(
+ requester, PackagePublishingPocket.RELEASE)
spph = self.publisher.getPubSource(
distroseries=self.distroseries,
- status=PackagePublishingStatus.PUBLISHED, archive=source_archive)
+ status=PackagePublishingStatus.PUBLISHED, archive=source)
spr = spph.sourcepackagerelease
if create_ancestry:
self.publisher.getPubSource(
distroseries=self.distroseries, sourcename=spr.name,
version="%s~" % spr.version,
- status=PackagePublishingStatus.PUBLISHED,
- archive=target_archive)
- return target_archive, source_archive, requester, spph, spr
+ status=PackagePublishingStatus.PUBLISHED, archive=target)
+ return spph, source, target, requester
+
+ def assertCanAutoApprove(self, create_ancestry, component_names,
+ pocket_admin=False):
+ spph, source, target, requester = self.createAutoApproveEnvironment(
+ create_ancestry, component_names, pocket_admin=pocket_admin)
+ job = self.createCopyJobForSPPH(
+ spph, source, target, requester=requester, auto_approve=True)
+ self.runJob(job)
+ self.assertEqual(JobStatus.COMPLETED, job.status)
+ spr = spph.sourcepackagerelease
+ new_publication = target.getPublishedSources(
+ name=spr.name, version=spr.version, exact_match=True).one()
+ self.assertEqual(target, new_publication.archive)
+
+ def assertCannotAutoApprove(self, create_ancestry, component_names,
+ pocket_admin=False):
+ spph, source, target, requester = self.createAutoApproveEnvironment(
+ create_ancestry, component_names, pocket_admin=pocket_admin)
+ job = self.createCopyJobForSPPH(
+ spph, source, target, requester=requester, auto_approve=True)
+ self.runJob(job)
+ self.assertEqual(JobStatus.SUSPENDED, job.status)
def test_auto_approve(self):
# The auto_approve flag causes the job to be processed immediately,
# even if it would normally have required manual approval.
- (target_archive, source_archive, requester,
- spph, spr) = self.createAutoApproveEnvironment(True, ["main"])
- job = self.createCopyJobForSPPH(
- spph, source_archive, target_archive, requester=requester,
- auto_approve=True)
- self.runJob(job)
- self.assertEqual(JobStatus.COMPLETED, job.status)
- new_publication = target_archive.getPublishedSources(
- name=spr.name, version=spr.version, exact_match=True).one()
- self.assertEqual(target_archive, new_publication.archive)
+ self.assertCanAutoApprove(True, ["main"])
def test_auto_approve_non_queue_admin(self):
# The auto_approve flag is ignored for people without queue admin
# permissions.
- (target_archive, source_archive, requester,
- spph, spr) = self.createAutoApproveEnvironment(True, [])
- job = self.createCopyJobForSPPH(
- spph, source_archive, target_archive, requester=requester,
- auto_approve=True)
- self.runJob(job)
- self.assertEqual(JobStatus.SUSPENDED, job.status)
+ self.assertCannotAutoApprove(True, [])
+
+ def test_auto_approve_pocket_queue_admin(self):
+ # The auto_approve flag is honoured for people with pocket queue
+ # admin permissions.
+ self.assertCanAutoApprove(True, [], pocket_admin=True)
def test_auto_approve_new(self):
# The auto_approve flag causes copies to bypass the NEW queue.
- (target_archive, source_archive, requester,
- spph, spr) = self.createAutoApproveEnvironment(False, ["universe"])
+ spph, source, target, requester = self.createAutoApproveEnvironment(
+ False, ["universe"])
# Without auto_approve, this job would be suspended and the upload
# moved to the NEW queue.
job = self.createCopyJobForSPPH(
- spph, source_archive, target_archive, requester=requester)
+ spph, source, target, requester=requester)
self.runJob(job)
self.assertEqual(JobStatus.SUSPENDED, job.status)
switch_dbuser("launchpad_main")
@@ -1008,24 +1000,23 @@
# With auto_approve, the job completes immediately.
job = self.createCopyJobForSPPH(
- spph, source_archive, target_archive, requester=requester,
- auto_approve=True)
+ spph, source, target, requester=requester, auto_approve=True)
self.runJob(job)
self.assertEqual(JobStatus.COMPLETED, job.status)
- new_publication = target_archive.getPublishedSources(
+ spr = spph.sourcepackagerelease
+ new_publication = target.getPublishedSources(
name=spr.name, version=spr.version, exact_match=True).one()
- self.assertEqual(target_archive, new_publication.archive)
+ self.assertEqual(target, new_publication.archive)
def test_auto_approve_new_non_queue_admin(self):
# For NEW packages, the auto_approve flag is ignored for people
# without queue admin permissions.
- (target_archive, source_archive, requester,
- spph, spr) = self.createAutoApproveEnvironment(False, [])
- job = self.createCopyJobForSPPH(
- spph, source_archive, target_archive, requester=requester,
- auto_approve=True)
- self.runJob(job)
- self.assertEqual(JobStatus.SUSPENDED, job.status)
+ self.assertCannotAutoApprove(False, [])
+
+ def test_auto_approve_new_pocket_queue_admin(self):
+ # For NEW packages, the auto_approve flag is honoured for people
+ # with pocket queue admin permissions.
+ self.assertCanAutoApprove(False, [], pocket_admin=True)
def test_copying_after_job_released(self):
# The first pass of the job may have created a PackageUpload and
@@ -1077,12 +1068,11 @@
emails = pop_notifications(sort_key=operator.itemgetter('To'))
# We expect an uploader email and an announcement to the changeslist.
- self.assertEquals(2, len(emails))
+ self.assertEqual(2, len(emails))
self.assertIn("requester@xxxxxxxxxxx", emails[0]['To'])
self.assertIn("changes@xxxxxxxxxxx", emails[1]['To'])
self.assertEqual(
- "Nancy Requester <requester@xxxxxxxxxxx>",
- emails[1]['From'])
+ "Nancy Requester <requester@xxxxxxxxxxx>", emails[1]['From'])
def test_copying_closes_bugs(self):
# Copying a package into a primary archive should close any bugs
@@ -1439,10 +1429,8 @@
# Patch the job's attemptCopy() method so it just raises an
# exception.
- def blow_up():
- raise Exception("I blew up")
naked_job = removeSecurityProxy(job)
- self.patch(naked_job, "attemptCopy", blow_up)
+ self.patch(naked_job, "attemptCopy", FakeMethod(failure=Exception()))
# Accept the upload to release the job then run it.
pu = getUtility(IPackageUploadSet).getByPackageCopyJobIDs(
@@ -1503,8 +1491,7 @@
def test_extendMetadata_edit_privilege_by_other(self):
# Random people cannot edit the copy job.
pcj = self.factory.makePlainPackageCopyJob()
- self.assertRaises(
- Unauthorized, getattr, pcj, "extendMetadata")
+ self.assertRaises(Unauthorized, getattr, pcj, "extendMetadata")
def test_PPA_edit_privilege_by_owner(self):
# A PCJ for a PPA allows the PPA owner to edit it.
@@ -1520,8 +1507,7 @@
pcj = self.factory.makePlainPackageCopyJob(target_archive=ppa)
person = self.factory.makePerson()
with person_logged_in(person):
- self.assertRaises(
- Unauthorized, getattr, pcj, "extendMetadata")
+ self.assertRaises(Unauthorized, getattr, pcj, "extendMetadata")
class TestPlainPackageCopyJobDbPrivileges(TestCaseWithFactory,
Follow ups
