launchpad-reviewers team mailing list archive

[Curtis Hovey <curtis.hovey@xxxxxxxxxxxxx>]

Curtis Hovey has proposed merging lp:~sinzui/launchpad/lookup-bug-redirect-0 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #257285 Url isn't correctly parsed causing oops
  https://bugs.launchpad.net/bugs/257285

For more details, see:
https://code.launchpad.net/~sinzui/launchpad/lookup-bug-redirect-0/+merge/51978

Ensure the bug_id is a string before looking it up.

    Launchpad bug: https://bugs.launchpad.net/bugs/257285
    Pre-implementation: no one
    Test command: ./bin/test -vv -t TestMaloneView

The MaloneView._redirectToBug assumes the bug_id is a string, but a hacked
or off site url might try to pass multiple ids:
    https://bugs.launchpad.net/?id=1234&id=5678

--------------------------------------------------------------------

RULES

    * Update MaloneView._redirectToBug to verify that bug_id is a
      basestring before calling a string operation. Let the list() instance
      fall through to the NotFound case.


QA

    * Visit https://bugs.qastaging.launchpad.net/?id=1234&id=5678
    * Verify you are shown a notification that says the bug is not
      registered.


LINT

    lib/lp/bugs/browser/bug.py
    lib/lp/bugs/browser/tests/test_bugs.py


IMPLEMENTATION

Added a test for the current behaviour. Added a guard to report an error
if the bug id is not a basestring.
    lib/lp/bugs/browser/bug.py
    lib/lp/bugs/browser/tests/test_bugs.py
-- 
https://code.launchpad.net/~sinzui/launchpad/lookup-bug-redirect-0/+merge/51978
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~sinzui/launchpad/lookup-bug-redirect-0 into lp:launchpad.

=== modified file 'lib/lp/bugs/browser/bug.py'
--- lib/lp/bugs/browser/bug.py	2011-02-03 11:17:19 +0000
+++ lib/lp/bugs/browser/bug.py	2011-03-02 23:04:11 +0000
@@ -371,6 +371,9 @@
 
     def _redirectToBug(self, bug_id):
         """Redirect to the specified bug id."""
+        if not isinstance(bug_id, basestring):
+            self.error_message = "Bug %r is not registered." % bug_id
+            return
         if bug_id.startswith("#"):
             # Be nice to users and chop off leading hashes
             bug_id = bug_id[1:]

=== added file 'lib/lp/bugs/browser/tests/test_bugs.py'
--- lib/lp/bugs/browser/tests/test_bugs.py	1970-01-01 00:00:00 +0000
+++ lib/lp/bugs/browser/tests/test_bugs.py	2011-03-02 23:04:11 +0000
@@ -0,0 +1,69 @@
+# Copyright 2011 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version (see the file LICENSE).
+
+"""Unit tests for bug set and bug application views."""
+
+__metaclass__ = type
+
+from zope.component import getUtility
+
+from canonical.launchpad.webapp.publisher import canonical_url
+from canonical.testing.layers import DatabaseFunctionalLayer
+from lp.bugs.interfaces.malone import IMaloneApplication
+from lp.bugs.publisher import BugsLayer
+from lp.testing import (
+    celebrity_logged_in,
+    TestCaseWithFactory,
+    )
+from lp.testing.views import create_initialized_view
+
+
+class TestMaloneView(TestCaseWithFactory):
+    """Test the MaloneView for the Bugs application."""
+    layer = DatabaseFunctionalLayer
+
+    def setUp(self):
+        super(TestMaloneView, self).setUp()
+        self.application = getUtility(IMaloneApplication)
+
+    def test_redirect_id_success(self):
+        # The view redirects to the bug when it is found.
+        bug = self.factory.makeBug()
+        form = dict(id=str(bug.id))
+        view = create_initialized_view(
+            self.application, name='+index', layer=BugsLayer, form=form)
+        self.assertEqual(None, view.error_message)
+        self.assertEqual(
+            canonical_url(bug), view.request.response.getHeader('Location'))
+
+    def test_redirect_name_success(self):
+        # The view redirects to the bug when it is found.
+        bug = self.factory.makeBug()
+        with celebrity_logged_in('admin'):
+            bug.name = 'bingo'
+        form = dict(id='bingo')
+        view = create_initialized_view(
+            self.application, name='+index', layer=BugsLayer, form=form)
+        self.assertEqual(None, view.error_message)
+        self.assertEqual(
+            canonical_url(bug), view.request.response.getHeader('Location'))
+
+    def test_redirect_unknown_bug_fail(self):
+        # The view reports an error and does not redirect if the bug is not
+        # found.
+        form = dict(id='fnord')
+        view = create_initialized_view(
+            self.application, name='+index', layer=BugsLayer, form=form)
+        self.assertEqual(
+            "Bug 'fnord' is not registered.", view.error_message)
+        self.assertEqual(None, view.request.response.getHeader('Location'))
+
+    def test_redirect_list_of_bug_fail(self):
+        # The view reports an error and does not redirect if list is provided
+        # instead of a string.
+        form = dict(id=['fnord', 'pting'])
+        view = create_initialized_view(
+            self.application, name='+index', layer=BugsLayer, form=form)
+        self.assertEqual(
+            "Bug ['fnord', 'pting'] is not registered.", view.error_message)
+        self.assertEqual(None, view.request.response.getHeader('Location'))