Commit 77ddff29 authored by dbates@webkit.org's avatar dbates@webkit.org
Browse files

2010-09-27 Daniel Bates <dbates@rim.com>

        Reviewed by Adam Barth.

        sheriffbot can't roll out security patches
        https://bugs.webkit.org/show_bug.cgi?id=39136

        Make SheriffBot determine if it's authorized to view a bug
        whose change it wants to rollout before it tries to rollout
        the change.

        Moreover, make both webkit-patch and Sheriffbot provide human-
        readable error messages when they are not authorized to view
        a bug and when a bug number is invalid.

        Currently, Sheriffbot does not parse Bugzilla bugs for
        <bug error="...">, which indicates an error when retrieving
        a bug. In particular, error="NotPermitted" if a person (or bot)
        is not authorized to view a bug. For such error="NotPermitted" bugs,
        Sheriffbot raises an exception when parsing the bug report and
        this exception does not explicitly indicate Sheriffbot's lack
        of authorization. Instead, Sheriffbot should explicitly check
        for the presence <bug error="..."> before operating on a bug
        and error with a human-readable message if it's not permitted
        to view it.

        * Scripts/webkitpy/common/net/bugzilla.py: Added BugzillaError class.
        * Scripts/webkitpy/common/net/bugzilla_unittest.py:
          - Added unit test test_bug_parsing_for_bugzilla_not_permitted_error().
          - Added unit test test_bug_parsing_for_bugzilla_not_found_error().
          - Added unit test test_bug_parsing_for_bugzilla_invalid_bug_id_error().
        * Scripts/webkitpy/tool/bot/sheriff.py: Modified to catch BugzillaError.
        * Scripts/webkitpy/tool/commands/download.py: Ditto.
        * Scripts/webkitpy/tool/commands/queues.py: Ditto.
        * Scripts/webkitpy/tool/commands/upload.py: Ditto.
        * Scripts/webkitpy/tool/steps/closebug.py: Ditto.
        * Scripts/webkitpy/tool/steps/obsoletepatches.py: Ditto.
        * Scripts/webkitpy/tool/steps/preparechangelog.py: Ditto.
        * Scripts/webkitpy/tool/steps/updatechangelogswithreviewer.py: Ditto.


git-svn-id: http://svn.webkit.org/repository/webkit/trunk@68493 268f45cc-cd09-0410-ab3c-d52691b4dbfc
parent c34ac996
2010-09-27 Daniel Bates <dbates@rim.com>
Reviewed by Adam Barth.
sheriffbot can't roll out security patches
https://bugs.webkit.org/show_bug.cgi?id=39136
Make SheriffBot determine if it's authorized to view a bug
whose change it wants to rollout before it tries to rollout
the change.
Moreover, make both webkit-patch and Sheriffbot provide human-
readable error messages when they are not authorized to view
a bug and when a bug number is invalid.
Currently, Sheriffbot does not parse Bugzilla bugs for
<bug error="...">, which indicates an error when retrieving
a bug. In particular, error="NotPermitted" if a person (or bot)
is not authorized to view a bug. For such error="NotPermitted" bugs,
Sheriffbot raises an exception when parsing the bug report and
this exception does not explicitly indicate Sheriffbot's lack
of authorization. Instead, Sheriffbot should explicitly check
for the presence <bug error="..."> before operating on a bug
and error with a human-readable message if it's not permitted
to view it.
* Scripts/webkitpy/common/net/bugzilla.py: Added BugzillaError class.
* Scripts/webkitpy/common/net/bugzilla_unittest.py:
- Added unit test test_bug_parsing_for_bugzilla_not_permitted_error().
- Added unit test test_bug_parsing_for_bugzilla_not_found_error().
- Added unit test test_bug_parsing_for_bugzilla_invalid_bug_id_error().
* Scripts/webkitpy/tool/bot/sheriff.py: Modified to catch BugzillaError.
* Scripts/webkitpy/tool/commands/download.py: Ditto.
* Scripts/webkitpy/tool/commands/queues.py: Ditto.
* Scripts/webkitpy/tool/commands/upload.py: Ditto.
* Scripts/webkitpy/tool/steps/closebug.py: Ditto.
* Scripts/webkitpy/tool/steps/obsoletepatches.py: Ditto.
* Scripts/webkitpy/tool/steps/preparechangelog.py: Ditto.
* Scripts/webkitpy/tool/steps/updatechangelogswithreviewer.py: Ditto.
2010-09-27 Adam Barth <abarth@webkit.org>
 
Reviewed by Eric Seidel.
......
......@@ -45,6 +45,39 @@ from webkitpy.thirdparty.autoinstalled.mechanize import Browser
from webkitpy.thirdparty.BeautifulSoup import BeautifulSoup, SoupStrainer
class BugzillaError(Exception):
def __init__(self, bug_id, error_message=None):
Exception.__init__(self, error_message)
self.bug_id = bug_id
self.error_message = error_message
def bug_id(self):
return self.bug_id
def error_message(self):
return self.error_message
# The following error messages were extracted from <https://bugs.webkit.org/bugzilla.dtd> as of 09/12/2010.
def is_invalid_bug_id(self):
return self.error_message == "InvalidBugId"
def is_not_found(self):
return self.error_message == "NotFound"
def is_not_permitted_to_view_bug(self):
return self.error_message == "NotPermitted"
def __str__(self):
if self.is_invalid_bug_id():
return "Bug %s is invalid" % self.bug_id
if self.is_not_found():
return "Bug %s does not exist" % self.bug_id
if self.is_not_permitted_to_view_bug():
return "Not permitted to view bug %s" % self.bug_id
return "Bugzilla encountered an error when processing bug %s: '%s'" % (self.bug_id, self.error_message)
def parse_bug_id(message):
if not message:
return None
......@@ -510,8 +543,12 @@ class Bugzilla(object):
def _parse_bug_page(self, page):
soup = BeautifulSoup(page)
bug_element = soup.find("bug")
bug_id = soup.find("bug_id").string
if bug_element.has_key("error"):
raise BugzillaError(bug_id, bug_element["error"])
bug = {}
bug["id"] = int(soup.find("bug_id").string)
bug["id"] = int(bug_id)
bug["title"] = self._string_contents(soup.find("short_desc"))
bug["bug_status"] = self._string_contents(soup.find("bug_status"))
bug["reporter_email"] = self._string_contents(soup.find("reporter"))
......
......@@ -31,7 +31,7 @@ import unittest
import datetime
from webkitpy.common.config.committers import CommitterList, Reviewer, Committer
from webkitpy.common.net.bugzilla import Bugzilla, BugzillaQueries, parse_bug_id, CommitterValidator, Bug
from webkitpy.common.net.bugzilla import Bugzilla, BugzillaError, BugzillaQueries, parse_bug_id, CommitterValidator, Bug
from webkitpy.common.system.outputcapture import OutputCapture
from webkitpy.thirdparty.mock import Mock
from webkitpy.thirdparty.BeautifulSoup import BeautifulSoup
......@@ -72,6 +72,14 @@ class CommitterValidatorTest(unittest.TestCase):
self.assertEqual(validator._flag_permission_rejection_message("foo@foo.com", "review"), expected_messsage)
class BugzillaErrorTest(unittest.TestCase):
def test_str(self):
self.assertEqual(BugzillaError(bug_id=12345, error_message="InvalidBugId").__str__(), "Bug 12345 is invalid")
self.assertEqual(BugzillaError(bug_id=12345, error_message="NotFound").__str__(), "Bug 12345 does not exist")
self.assertEqual(BugzillaError(bug_id=12345, error_message="NotPermitted").__str__(), "Not permitted to view bug 12345")
self.assertEqual(BugzillaError(bug_id=12345, error_message="Thingamajig broke").__str__(), "Bugzilla encountered an error when processing bug 12345: 'Thingamajig broke'")
class BugzillaTest(unittest.TestCase):
_example_attachment = '''
<attachment
......@@ -141,6 +149,57 @@ class BugzillaTest(unittest.TestCase):
self.assertEquals(None, parse_bug_id("http://www.webkit.org/b/12345"))
self.assertEquals(None, parse_bug_id("http://bugs.webkit.org/show_bug.cgi?ctype=xml&id=12345"))
_example_unauthorized_bug = """
<?xml version="1.0" encoding="UTF-8" standalone="yes" ?>
<!DOCTYPE bugzilla SYSTEM "https://bugs.webkit.org/bugzilla.dtd">
<bugzilla version="3.2.3"
urlbase="https://bugs.webkit.org/"
maintainer="admin@webkit.org"
exporter="unauthorized_person@example.com"
>
<bug error="NotPermitted">
<bug_id>12345</bug_id>
</bug>
</bugzilla>
"""
_example_not_found_bug = """
<?xml version="1.0" encoding="UTF-8" standalone="yes" ?>
<!DOCTYPE bugzilla SYSTEM "https://bugs.webkit.org/bugzilla.dtd">
<bugzilla version="3.2.3"
urlbase="https://bugs.webkit.org/"
maintainer="admin@webkit.org"
exporter="unauthorized_person@example.com"
>
<bug error="NotFound">
<bug_id>12345</bug_id>
</bug>
</bugzilla>
"""
_example_invalid_bug_id_bug = """
<?xml version="1.0" encoding="UTF-8" standalone="yes" ?>
<!DOCTYPE bugzilla SYSTEM "https://bugs.webkit.org/bugzilla.dtd">
<bugzilla version="3.2.3"
urlbase="https://bugs.webkit.org/"
maintainer="admin@webkit.org"
exporter="unauthorized_person@example.com"
>
<bug error="InvalidBugId">
<bug_id>A</bug_id>
</bug>
</bugzilla>
"""
_example_bug = """
<?xml version="1.0" encoding="UTF-8" standalone="yes" ?>
<!DOCTYPE bugzilla SYSTEM "https://bugs.webkit.org/bugzilla.dtd">
......@@ -228,6 +287,33 @@ ZEZpbmlzaExvYWRXaXRoUmVhc29uOnJlYXNvbl07Cit9CisKIEBlbmQKIAogI2VuZGlmCg==
}],
}
def test_bug_parsing_for_bugzilla_not_permitted_error(self):
didPassTest = False
try:
Bugzilla()._parse_bug_page(self._example_unauthorized_bug)
except BugzillaError, e:
self.assertTrue(e.is_not_permitted_to_view_bug())
didPassTest = True
self.assertTrue(didPassTest, "Failed to raise exception.")
def test_bug_parsing_for_bugzilla_not_found_error(self):
didPassTest = False
try:
Bugzilla()._parse_bug_page(self._example_not_found_bug)
except BugzillaError, e:
self.assertTrue(e.is_not_found())
didPassTest = True
self.assertTrue(didPassTest, "Failed to raise exception.")
def test_bug_parsing_for_bugzilla_invalid_bug_id_error(self):
didPassTest = False
try:
Bugzilla()._parse_bug_page(self._example_invalid_bug_id_bug)
except BugzillaError, e:
self.assertTrue(e.is_invalid_bug_id())
didPassTest = True
self.assertTrue(didPassTest, "Failed to raise exception.")
# FIXME: This should move to a central location and be shared by more unit tests.
def _assert_dictionaries_equal(self, actual, expected):
# Make sure we aren't parsing more or less than we expect
......
......@@ -27,7 +27,7 @@
# OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
from webkitpy.common.checkout.changelog import view_source_url
from webkitpy.common.net.bugzilla import parse_bug_id
from webkitpy.common.net.bugzilla import Bugzilla, BugzillaError, parse_bug_id
from webkitpy.common.system.deprecated_logging import log
from webkitpy.common.system.executive import ScriptError
from webkitpy.tool.grammar import join_with_separators
......@@ -64,6 +64,13 @@ class Sheriff(object):
raise ScriptError(message="The rollout reason may not begin "
"with - (\"%s\")." % rollout_reason)
try:
self._tool.bugs.fetch_bug(self._tool.checkout().commit_info_for_revision(svn_revision))
except BugzillaError, e:
if (e.is_not_permitted_to_view_bug()):
raise ScriptError(message="SheriffBot is not authorized to view %s." % self._tool.bugs.bug_url_for_bug_id(e.bug_id()))
raise ScriptError(message="Could not determine the corresponding bug for r%s." % svn_revision)
output = self._sheriffbot.run_webkit_patch([
"create-rollout",
"--force-clean",
......
......@@ -34,6 +34,7 @@ from optparse import make_option
import webkitpy.tool.steps as steps
from webkitpy.common.checkout.changelog import ChangeLog, view_source_url
from webkitpy.common.net.bugzilla import BugzillaError
from webkitpy.common.system.executive import ScriptError
from webkitpy.tool.commands.abstractsequencedcommand import AbstractSequencedCommand
from webkitpy.tool.commands.stepsequence import StepSequence
......@@ -157,16 +158,31 @@ class AbstractPatchSequencingCommand(AbstractPatchProcessingCommand):
class ProcessAttachmentsMixin(object):
def _fetch_list_of_patches_to_process(self, options, args, tool):
return map(lambda patch_id: tool.bugs.fetch_attachment(patch_id), args)
all_patches = []
for patch_id in args:
try:
all_patches.append(tool.bugs.fetch_attachment(patch_id))
except BugzillaError, e:
if options.non_interactive:
raise
log("Failed to fetch attachment %s: '%s'; ignoring." % (patch_id, e))
continue
return all_patches
class ProcessBugsMixin(object):
def _fetch_list_of_patches_to_process(self, options, args, tool):
all_patches = []
for bug_id in args:
patches = tool.bugs.fetch_bug(bug_id).reviewed_patches()
log("%s found on bug %s." % (pluralize("reviewed patch", len(patches)), bug_id))
all_patches += patches
try:
patches = tool.bugs.fetch_bug(bug_id).reviewed_patches()
log("%s found on bug %s." % (pluralize("reviewed patch", len(patches)), bug_id))
all_patches += patches
except BugzillaError, e:
if options.non_interactive:
raise
log("'%s'; ignoring." % e)
continue
return all_patches
......
......@@ -35,7 +35,7 @@ from datetime import datetime
from optparse import make_option
from StringIO import StringIO
from webkitpy.common.net.bugzilla import CommitterValidator
from webkitpy.common.net.bugzilla import BugzillaError, CommitterValidator
from webkitpy.common.net.statusserver import StatusServer
from webkitpy.common.system.executive import ScriptError
from webkitpy.common.system.deprecated_logging import error, log
......@@ -353,7 +353,14 @@ class AbstractReviewQueue(AbstractPatchQueue, PersistentPatchCollectionDelegate,
def next_work_item(self):
patch_id = self._patches.next()
if patch_id:
return self._tool.bugs.fetch_attachment(patch_id)
try:
return self._tool.bugs.fetch_attachment(patch_id)
except BugzillaError, e:
# This may occur if the bug with attachment ID patch_id becomes a security
# bug between the time we fetched all attachment IDs from the review queue
# and now. So, we ignore this attachment.
log("'%s'; ignoring." % e)
pass
def should_proceed_with_work_item(self, patch):
raise NotImplementedError, "subclasses must implement"
......
......@@ -37,7 +37,7 @@ from optparse import make_option
import webkitpy.tool.steps as steps
from webkitpy.common.config.committers import CommitterList
from webkitpy.common.net.bugzilla import parse_bug_id
from webkitpy.common.net.bugzilla import BugzillaError, parse_bug_id
from webkitpy.common.system.user import User
from webkitpy.thirdparty.mock import Mock
from webkitpy.tool.commands.abstractsequencedcommand import AbstractSequencedCommand
......@@ -82,7 +82,14 @@ class CleanPendingCommit(AbstractDeclarativeCommand):
def execute(self, options, args, tool):
committers = CommitterList()
for bug_id in tool.bugs.queries.fetch_bug_ids_from_pending_commit_list():
bug = self._tool.bugs.fetch_bug(bug_id)
try:
bug = self._tool.bugs.fetch_bug(bug_id)
except BugzillaError, e:
# This may occur if the bug becomes a security bug between the time
# we fetched its ID from the pending commit list and now. We ignore
# this bug.
log(e)
continue
patches = bug.patches(include_obsolete=True)
for patch in patches:
flags_to_clear = self._flags_to_clear_on_patch(patch)
......@@ -104,7 +111,11 @@ class AssignToCommitter(AbstractDeclarativeCommand):
def _assign_bug_to_last_patch_attacher(self, bug_id):
committers = CommitterList()
bug = self._tool.bugs.fetch_bug(bug_id)
try:
bug = self._tool.bugs.fetch_bug(bug_id)
except BugzillaError, e:
log(e)
return
if not bug.is_unassigned():
assigned_to_email = bug.assigned_to_email()
log("Bug %s is already assigned to %s (%s)." % (bug_id, assigned_to_email, committers.committer_by_email(assigned_to_email)))
......
......@@ -28,6 +28,7 @@
from webkitpy.tool.steps.abstractstep import AbstractStep
from webkitpy.tool.steps.options import Options
from webkitpy.common.net.bugzilla import BugzillaError
from webkitpy.common.system.deprecated_logging import log
......@@ -43,7 +44,11 @@ class CloseBug(AbstractStep):
return
# Check to make sure there are no r? or r+ patches on the bug before closing.
# Assume that r- patches are just previous patches someone forgot to obsolete.
patches = self._tool.bugs.fetch_bug(state["patch"].bug_id()).patches()
try:
patches = self._tool.bugs.fetch_bug(state["patch"].bug_id()).patches()
except BugzillaError, e:
log(e)
return
for patch in patches:
if patch.review() == "?" or patch.review() == "+":
log("Not closing bug %s as attachment %s has review=%s. Assuming there are more patches to land from this bug." % (patch.bug_id(), patch.id(), patch.review()))
......
......@@ -29,6 +29,7 @@
from webkitpy.tool.grammar import pluralize
from webkitpy.tool.steps.abstractstep import AbstractStep
from webkitpy.tool.steps.options import Options
from webkitpy.common.net.bugzilla import BugzillaError
from webkitpy.common.system.deprecated_logging import log
......@@ -43,7 +44,11 @@ class ObsoletePatches(AbstractStep):
if not self._options.obsolete_patches:
return
bug_id = state["bug_id"]
patches = self._tool.bugs.fetch_bug(bug_id).patches()
try:
patches = self._tool.bugs.fetch_bug(bug_id).patches()
except BugzillaError, e:
log(e)
return
if not patches:
return
log("Obsoleting %s on bug %s" % (pluralize("old patch", len(patches)), bug_id))
......
......@@ -29,6 +29,7 @@
import os
from webkitpy.common.checkout.changelog import ChangeLog
from webkitpy.common.net.bugzilla import BugzillaError
from webkitpy.common.system.executive import ScriptError
from webkitpy.tool.steps.abstractstep import AbstractStep
from webkitpy.tool.steps.options import Options
......@@ -54,7 +55,7 @@ class PrepareChangeLog(AbstractStep):
changelog = ChangeLog(changelog_path)
if not changelog.latest_entry().bug_id():
changelog.set_short_description_and_bug_url(
self.cached_lookup(state, "bug_title"),
self.cached_lookup(state, "bug_title"), # Will raise an exception if not authorized to retrieve the bug title.
self._tool.bugs.bug_url_for_bug_id(bug_id))
def run(self, state):
......
......@@ -32,6 +32,7 @@ from webkitpy.common.checkout.changelog import ChangeLog
from webkitpy.tool.grammar import pluralize
from webkitpy.tool.steps.abstractstep import AbstractStep
from webkitpy.tool.steps.options import Options
from webkitpy.common.net.bugzilla import BugzillaError
from webkitpy.common.system.deprecated_logging import log, error
class UpdateChangeLogsWithReviewer(AbstractStep):
......@@ -43,7 +44,11 @@ class UpdateChangeLogsWithReviewer(AbstractStep):
]
def _guess_reviewer_from_bug(self, bug_id):
patches = self._tool.bugs.fetch_bug(bug_id).reviewed_patches()
try:
patches = self._tool.bugs.fetch_bug(bug_id).reviewed_patches()
except BugzillaError, e:
log(e)
return None
if len(patches) != 1:
log("%s on bug %s, cannot infer reviewer." % (pluralize("reviewed patch", len(patches)), bug_id))
return None
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment