Merge lp:~julian-edwards/launchpad/queue-accept-with-private-bugs into lp:launchpad

[Julian Edwards]

Fix an OOPS when an archive admin uses the +queue page to accept uploads that close private bugs.

= 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.

[Steve Kowalik (stevenk)]

This change looks sane to me.

review: Approve (code*)

[Tim Penhey (thumper)]

If the work should be moved, how about you file a bug and add an XXX comment?

Also I think your test is a bit more convoluted than it needs to be.

Instead of:
  removeSecurityProxy(spr).changelog_entry = "blah"
you should tweak
  self.factory.makeSourcePackageRelease()
to accept a changelog_entry. This is simple, quick, and avoids the need
for a removeSecurityProxy.

We don't care who the bug reporter is, so you can get rid of that makePerson,
and remove the owner from the makeBug and makeBugTask calls.

The archive_admin person isn't really an archive admin are they? they are
just some random person. Should it be a particular user? Or just someone
who can't see the bug?

Also, we don't need to be the bug reporter to see the change, we could just
use an admin user as we know they can see everything.

[Julian Edwards (julian-edwards)]

Hello Tim, haven't had one of your reviews for ages :)

On Wednesday 15 September 2010 05:53:43 Tim Penhey wrote:
> If the work should be moved, how about you file a bug and add an XXX
> comment?

I was waiting for someone to say that! It's highly unlikely it will ever get
done if this actually works, my comment is referring to utopia. I think we
should only file bugs that we really intend to fix.

> Also I think your test is a bit more convoluted than it needs to be.
>
> Instead of:
> removeSecurityProxy(spr).changelog_entry = "blah"
> you should tweak
> self.factory.makeSourcePackageRelease()
> to accept a changelog_entry. This is simple, quick, and avoids the need
> for a removeSecurityProxy.

Fair point, I'll fix it.

> We don't care who the bug reporter is, so you can get rid of that
> makePerson, and remove the owner from the makeBug and makeBugTask calls.

We kinda do actually, it's critical to the test that it demonstrates the two
people are different so you can see that the bug is fixed. Anyway, I've got
rid of the extra makePerson and replaced it with an assertion to prove that
the archive_admin person can't see the bug. That removes the implicit
assumption that the two users have different permissions and makes it
explicit.

The only thing that's annoying is the import from lp.bugs :/

> The archive_admin person isn't really an archive admin are they? they are
> just some random person. Should it be a particular user? Or just someone
> who can't see the bug?

It's just a meaningful variable name to show that the person coming along to
work the +queue page is an archive admin - nobody else has that permission.
Admittedly I'm not driving the queue page here but the point still stands.

> Also, we don't need to be the bug reporter to see the change, we could just
> use an admin user as we know they can see everything.

I'm not sure it makes any practical difference, but I changed it anyway at the
cost of an extra import.

Cheers.

[Tim Penhey (thumper)]

On Wed, 15 Sep 2010 22:09:58 you wrote:
> Hello Tim, haven't had one of your reviews for ages :)
>
> On Wednesday 15 September 2010 05:53:43 Tim Penhey wrote:
> > If the work should be moved, how about you file a bug and add an XXX
> > comment?
>
> I was waiting for someone to say that! It's highly unlikely it will ever
> get done if this actually works, my comment is referring to utopia. I
> think we should only file bugs that we really intend to fix.

Yeah... ok.

> > We don't care who the bug reporter is, so you can get rid of that
> > makePerson, and remove the owner from the makeBug and makeBugTask calls.
>
> We kinda do actually, it's critical to the test that it demonstrates the
> two people are different so you can see that the bug is fixed. Anyway,
> I've got rid of the extra makePerson and replaced it with an assertion to
> prove that the archive_admin person can't see the bug. That removes the
> implicit assumption that the two users have different permissions and
> makes it explicit.
>
> The only thing that's annoying is the import from lp.bugs :/

You could always move it into lp.bugs.enums :)
(which doesn't exist yet).

> > The archive_admin person isn't really an archive admin are they? they
> > are just some random person. Should it be a particular user? Or just
> > someone who can't see the bug?
>
> It's just a meaningful variable name to show that the person coming along
> to work the +queue page is an archive admin - nobody else has that
> permission. Admittedly I'm not driving the queue page here but the point
> still stands.

Well... no, it just confuses the issue. The person isn't in a special role,
but the name makes it appear that they are.

Also, does the test work in the DatabaseFunctionalLayer? That layer has less
dependencies.

[Tim Penhey (thumper)]

review: Approve (mentor)

[Julian Edwards (julian-edwards)]

On Thursday 16 September 2010 04:58:43 you wrote:
> Also, does the test work in the DatabaseFunctionalLayer? That layer has
> less dependencies.

Good call, I forgot about that one. It works fine.

Cheers for the review.

[Deryck Hodge (deryck)]

As mentioned on IRC, I would prefer a strongly worded comment to discourage doing anything else with the non-security-proxied object. This simple change to a bug task FixReleased status is no harm, but I don't want later developers coming along and working with non-proxied bugs without extreme caution about what they are doing.

Other than that, I recognize this is the easiest way to fix the OOPS currently and the change looks fine to me.

Cheers,
deryck

review: Approve (code)

=== 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-16 12:01:19 +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,14 @@
 
     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, but that's a lot
+        # of work.  If you don't do that and you're changing stuff in
+        # here, BE CAREFUL with the unproxied bug object and look at
+        # what you're doing with it that might violate security.
+        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-16 12:01:19 +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,
@@ -19,6 +22,7 @@
     )
 
 from zope.component import getUtility
+from zope.security.interfaces import ForbiddenAttribute
 from zope.security.proxy import removeSecurityProxy
 
 from canonical.config import config
@@ -33,7 +37,10 @@
     fillLibrarianFile,
     )
 from canonical.librarian.utils import filechunks
-from canonical.testing import LaunchpadZopelessLayer
+from canonical.testing import (
+    DatabaseFunctionalLayer, 
+    LaunchpadZopelessLayer,
+    )
 from lp.archiveuploader.nascentupload import NascentUpload
 from lp.archiveuploader.tests import (
     datadir,
@@ -42,7 +49,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 +66,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 +77,11 @@
     CommandRunnerError,
     name_queue_map,
     )
+from lp.testing import (
+    celebrity_logged_in,
+    person_logged_in,
+    TestCaseWithFactory,
+    )
 
 
 class TestQueueBase(TestCase):
@@ -926,6 +944,36 @@
             '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 = DatabaseFunctionalLayer
+
+    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"
+        # changelog_entry is required for an assertion inside the function
+        # we're testing.
+        spr = self.factory.makeSourcePackageRelease(changelog_entry="blah")
+        archive_admin = self.factory.makePerson()
+        bug = self.factory.makeBug(private=True)
+        bug_task = self.factory.makeBugTask(target=spr.sourcepackage, bug=bug)
+        changes = StringIO(changes_file_template % bug.id)
+
+        with person_logged_in(archive_admin):
+            # The archive admin user can't normally see this bug.
+            self.assertRaises(ForbiddenAttribute, bug, 'status')
+            # But the bug closure should work.
+            close_bugs_for_sourcepackagerelease(spr, changes)
+
+        # Verify it was closed.
+        with celebrity_logged_in("admin"):
+            self.assertEqual(bug_task.status, BugTaskStatus.FIXRELEASED)
+
+
 class TestQueueToolInJail(TestQueueBase):
     layer = LaunchpadZopelessLayer
     dbuser = config.uploadqueue.dbuser

=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py	2010-09-10 03:20:48 +0000
+++ lib/lp/testing/factory.py	2010-09-16 12:01:19 +0000
@@ -2537,6 +2537,7 @@
                                  source_package_recipe_build=None,
                                  dscsigningkey=None,
                                  user_defined_fields=None,
+                                 changelog_entry=None,
                                  homepage=None):
         """Make a `SourcePackageRelease`."""
         if distroseries is None:
@@ -2594,7 +2595,7 @@
             build_conflicts_indep=build_conflicts_indep,
             architecturehintlist=architecturehintlist,
             changelog=None,
-            changelog_entry=None,
+            changelog_entry=changelog_entry,
             dsc=None,
             copyright=self.getUniqueString(),
             dscsigningkey=dscsigningkey,