launchpad-reviewers team mailing list archive

[Julian Edwards <julian.edwards@xxxxxxxxxxxxx>]

Julian Edwards has proposed merging lp:~julian-edwards/launchpad/queue-accept-with-private-bugs into lp:launchpad/devel.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #564491 Cannot accept package which closes inaccessible bug
  https://bugs.launchpad.net/bugs/564491
  #566339 Cannot accept package which would notify private email addresses
  https://bugs.launchpad.net/bugs/566339


= Summary =
Fix private bug closing when accepting package uploads.

As described in the linked bugs, we have a problem when someone accepts a held 
upload if that upload is trying to access a bug that the user is not allowed 
to see.

== Proposed fix ==
Normally this is not a problem for auto-accepted uploads because they run in 
zopeless scripts, which don't care about the security.  For this reason, I 
feel it is ok to remove the security proxy in the code that is shared with the 
webapp request.

== Pre-implementation notes ==
See the bug(s).

== Implementation details ==
A simple removeSecurityProxy() is used to remove the wrapper from the bug 
before it changes its status to RELEASED and adds the janitor message.

I'm inviting Deryck and Robert to review this as well, to see if you think 
this approach is safe.

== Tests ==
bin/test -cvvt TestQueuePageClosingBugs

== Demo and Q/A ==
Set up a private bugtask on a source package using user "A".  Make an upload 
of that package to a frozen distroseries, with a Launchpad-bugs-fixed: header 
referring to it.  The package will be held in the unapproved queue, where user 
"B" can try to accept it.
-- 
https://code.launchpad.net/~julian-edwards/launchpad/queue-accept-with-private-bugs/+merge/35438
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~julian-edwards/launchpad/queue-accept-with-private-bugs into lp:launchpad/devel.

=== modified file 'lib/lp/soyuz/scripts/processaccepted.py'
--- lib/lp/soyuz/scripts/processaccepted.py	2010-09-10 12:56:30 +0000
+++ lib/lp/soyuz/scripts/processaccepted.py	2010-09-14 17:01:16 +0000
@@ -7,6 +7,7 @@
 __all__ = [
     'close_bugs',
     'close_bugs_for_queue_item',
+    'close_bugs_for_sourcepackagerelease',
     'close_bugs_for_sourcepublication',
     'get_bugs_from_changes_file',
     'ProcessAccepted',
@@ -16,6 +17,7 @@
 import sys
 
 from zope.component import getUtility
+from zope.security.proxy import removeSecurityProxy
 
 from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
 from canonical.launchpad.webapp.errorlog import (
@@ -165,6 +167,11 @@
 
     janitor = getUtility(ILaunchpadCelebrities).janitor
     for bug in bugs_to_close:
+        # We need to remove the security proxy here because the bug
+        # might be private and if this code is called via someone using
+        # the +queue page they will get an OOPS.  Ideally, we should
+        # migrate this code to the Job system though.
+        bug = removeSecurityProxy(bug)
         edited_task = bug.setStatus(
             target=source_release.sourcepackage,
             status=BugTaskStatus.FIXRELEASED,

=== modified file 'lib/lp/soyuz/scripts/tests/test_queue.py'
--- lib/lp/soyuz/scripts/tests/test_queue.py	2010-08-27 12:21:25 +0000
+++ lib/lp/soyuz/scripts/tests/test_queue.py	2010-09-14 17:01:16 +0000
@@ -3,6 +3,8 @@
 
 """queue tool base class tests."""
 
+from __future__ import with_statement
+
 __metaclass__ = type
 __all__ = [
     'upload_bar_source',
@@ -12,6 +14,7 @@
 import hashlib
 import os
 import shutil
+from StringIO import StringIO
 import tempfile
 from unittest import (
     TestCase,
@@ -33,7 +36,10 @@
     fillLibrarianFile,
     )
 from canonical.librarian.utils import filechunks
-from canonical.testing import LaunchpadZopelessLayer
+from canonical.testing import (
+    LaunchpadFunctionalLayer, 
+    LaunchpadZopelessLayer,
+    )
 from lp.archiveuploader.nascentupload import NascentUpload
 from lp.archiveuploader.tests import (
     datadir,
@@ -42,7 +48,10 @@
     mock_logger_quiet,
     )
 from lp.bugs.interfaces.bug import IBugSet
-from lp.bugs.interfaces.bugtask import IBugTaskSet
+from lp.bugs.interfaces.bugtask import (
+    BugTaskStatus,
+    IBugTaskSet,
+    )
 from lp.registry.interfaces.distribution import IDistributionSet
 from lp.registry.interfaces.person import IPersonSet
 from lp.registry.interfaces.pocket import PackagePublishingPocket
@@ -56,6 +65,9 @@
 from lp.soyuz.interfaces.archive import (
     IArchiveSet,
     )
+from lp.soyuz.scripts.processaccepted import (
+    close_bugs_for_sourcepackagerelease,
+    )
 from lp.soyuz.interfaces.queue import (
     IPackageUploadSet,
     )
@@ -64,6 +76,10 @@
     CommandRunnerError,
     name_queue_map,
     )
+from lp.testing import (
+    person_logged_in,
+    TestCaseWithFactory,
+    )
 
 
 class TestQueueBase(TestCase):
@@ -926,6 +942,32 @@
             'override binary pmount', component_name='partner')
 
 
+class TestQueuePageClosingBugs(TestCaseWithFactory):
+    # The distroseries +queue page can close bug when accepting
+    # packages.  Unit tests for that belong here.
+
+    layer = LaunchpadFunctionalLayer
+
+    def test_close_bugs_for_sourcepackagerelease_with_private_bug(self):
+        # lp.soyuz.scripts.processaccepted.close_bugs_for_sourcepackagerelease
+        # should work with private bugs where the person using the queue
+        # page doesn't have access to it.
+        changes_file_template = "Format: 1.7\nLaunchpad-bugs-fixed: %s\n"
+        spr = self.factory.makeSourcePackageRelease()
+        # Required for assertion inside the func. we're testing.
+        removeSecurityProxy(spr).changelog_entry = "blah"
+        bug_reporter = self.factory.makePerson()
+        archive_admin = self.factory.makePerson()
+        bug = self.factory.makeBug(owner=bug_reporter, private=True)
+        bug_task = self.factory.makeBugTask(
+            owner=bug_reporter, target=spr.sourcepackage, bug=bug)
+        changes = StringIO(changes_file_template % bug.id)
+        with person_logged_in(archive_admin):
+            close_bugs_for_sourcepackagerelease(spr, changes)
+        with person_logged_in(bug_reporter):
+            self.assertEqual(bug_task.status, BugTaskStatus.FIXRELEASED)
+
+
 class TestQueueToolInJail(TestQueueBase):
     layer = LaunchpadZopelessLayer
     dbuser = config.uploadqueue.dbuser