Submitter:	Marc Hedlund	Reviewers
Branch:		Groups:	reviewboard
Bugs:		People:
Change Number:	None	Repository:	Review Board SVN

Description:

This is a more extensive change to post-review.

First, I moved some of the global definitions out of the top of the script file. I encapsulated all of the HTTP handling code in the ReviewBoardServer class, since that's the only class that needs HTTP support, and it was previously relying on globals from distant parts of the file to take care of cookie handling. I also moved the 'homepath' discovery code down to the main() method since only that method should need to know the user's home dir.

Second, I added a ReviewBoardHTTPPasswordMgr class to provide HTTP Authentication support. This was a little more complicated than I'd hoped since http auth is a little busted in Python 2.4. I think this addition is reasonably secure, and I've tested it extensively with several http-auth-protected servers, using Python 2.4, with good results.

Third, I cleaned up the cookie handling code quite a bit. It used to be the case (I think, at least) that if you had *any* cookies in your ~/.post-review-cookies.txt file, you would never be prompted to log in, effectively limiting you to one RB server at a time. Since I'm using two (my company's and the Review Board RB itself), that wasn't working for me. (I went all anal on this part of things and added a bunch of cookie checks and debug messages, so that exactly what was going on with the cookie search would be apparent.)

Finally, I changed the login prompts so that the Review Board login and any HTTP Auth logins would be similar but (hopefully) clearly distinguished. (Note: HTTP Auth credentials are not saved between sessions, and arguably should be. Nor are they settable from command-line options.)

I'm hoping that the result of this is that the script works in more circumstances and is a little cleaner -- less reliance on globals and a little better encapsulation.

(One other note: I didn't change the script version, but I think that would be worth doing before committing this, since the http aspects of it are fairly different.)

Testing Done:

I've tested this version of post-review against both http://reviews.review-board.org and my company's Review Board server.  The tests covered a non-http-authenticated server (this one), an https/http-auth-protected server, and an https/http-auth-protected server on a non-standard port (which triggered a bug in python 2.4, worked around -- see comments).  All of my tests were with Python 2.4.

Diff revision 1

This is not the most recent revision of the diff. The latest diff is revision 2. See what's changed.

Jump to revision:	1 2
Changes between r1 and:	1 2

Files Changed:

/trunk/reviewboard/contrib/tools/post-review: 18 changes [ 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 ]

Expand changes

 import subprocess
 import sys
 import urllib2
 from optparse import OptionParser
 from tempfile import mkstemp
-from urlparse import urljoin
+from urlparse import urljoin, urlparse
 VERSION = "0.6"
-# Who stole the cookies from the cookie jar?
-# Was it you?
-# >:(
-if 'USERPROFILE' in os.environ:
-    homepath = os.path.join(os.environ["USERPROFILE"], "Local Settings",
-                            "Application Data")
-else:
-    homepath = os.environ["HOME"]
-cj = cookielib.MozillaCookieJar()
-cookiefile = os.path.join(homepath, ".post-review-cookies.txt")
-opener = urllib2.build_opener(urllib2.HTTPCookieProcessor(cj))
-opener.addheaders = [('User-agent', 'post-review/' + VERSION)]
-urllib2.install_opener(opener)
 user_config = None
 tempfiles = []
 options = None
     def __str__(self):
         return "Path: %s, Base path: %s, Supports changesets: %s" % \
             (self.path, self.base_path, self.supports_changesets)
 class ReviewBoardHTTPPasswordMgr(urllib2.HTTPPasswordMgr):
     """
     Python 2.4's password manager has a bug in http authentication when the
     target server uses a non-standard port.  Since ReviewBoard supports 2.4
     (and the patch contributor's 2.4 server uses a non-standard port), this
     overrides the default urllib2.HTTPPasswordMgrWithDefaultRealm to provide
     HTTP Authentication support.  (See http://bugs.python.org/issue974757)
     This also allows post-review to prompt for passwords in a consistent way.
     """
     def __init__(self, reviewboard_url):
         self.passwd  = {}
         self.rb_url  = reviewboard_url
         self.rb_user = None
         self.rb_pass = None
     def find_user_password(self, realm, uri):
         if uri.startswith(self.rb_url):
             if self.rb_user is None or self.rb_pass is None:
                 print "==> HTTP Authentication Required"
                 print "Enter username and password for \"%s\" at %s" % \
                     (realm, urlparse(uri)[1])
                 self.rb_user = raw_input('Username: ')
                 self.rb_pass = getpass.getpass('Password: ')
             return self.rb_user, self.rb_pass
         else:
             # If this is an auth request for some other domain (since HTTP
             # handlers are global), fall back to standard password management.
             return urllib2.HTTPPasswordMgr.find_user_password(self, realm, uri)
 class ReviewBoardServer:
     """
     An instance of a Review Board server.
     """
-    def __init__(self, url, info):
+    def __init__(self, url, info, cookie_file):
         self.url = url
         self.info = info
         self.cookie_file = cookie_file
         self.cookie_jar  = cookielib.MozillaCookieJar(self.cookie_file)
         # Set up the HTTP libraries to support all of the features we need.
         cookie_handler = urllib2.HTTPCookieProcessor(self.cookie_jar)
         password_mgr   = ReviewBoardHTTPPasswordMgr(self.url)
         auth_handler   = urllib2.HTTPBasicAuthHandler(password_mgr)
         opener = urllib2.build_opener(cookie_handler, auth_handler)
         opener.addheaders = [('User-agent', 'post-review/' + VERSION)]
         urllib2.install_opener(opener)
     def login(self):
         """
         Logs in to a Review Board server, prompting the user for login
-        information.
+        information if needed.
         """
-        print "You must log in the first time."
+        if self.has_valid_cookie():
             return
         print "==> Review Board Login Required"
         print "Enter username and password for Review Board at %s" % self.url
         username = raw_input('Username: ')
         password = getpass.getpass('Password: ')
         debug('Logging in with username "%s"' % username)
         try:
             die("Unable to log in: %s (%s)" % (rsp["err"]["msg"],
                                                rsp["err"]["code"]))
         debug("Logged in.")
     def has_valid_cookie(self):
         """
         Load up the user's cookie file, and see if they have a valid
         'sessionid' cookie for the current Review Board server.  Returns
         true if so and false otherwise.
         """
         try:
             parsed_url = urlparse(self.url)
             host = parsed_url[1]
             # Cookie files don't store port numbers, unfortunately...
             port_idx = host.find(':')
             if port_idx != -1:
                 host = host[0:port_idx]
             path = parsed_url[2] or '/'
             debug("Looking for '%s %s' cookie in %s" % \
                 (host, path, self.cookie_file))
             self.cookie_jar.load(self.cookie_file, ignore_expires=True)
             try:
                 cookie = self.cookie_jar._cookies[host][path]['sessionid']
                 if not cookie.is_expired():
                     debug("Loaded valid cookie -- no login required")
                     return True
                 else:
                     debug("Cookie file loaded, but cookie has expired")
             except KeyError:
                 debug("Cookie file loaded, but no cookie for this server")
         except IOError, error:
             debug("Couldn't load cookie file: %s" % error)
         return False
     def new_review_request(self, changenum, submit_as=None):
         """
         Creates a review request on a Review Board server, updating an
         existing one if the changeset number already exists.
         url = self._make_url(path)
         try:
             rsp = urllib2.urlopen(url).read()
-            cj.save(cookiefile)
+            self.cookie_jar.save(self.cookie_file)
             return rsp
         except urllib2.HTTPError, e:
             print "Unable to access %s (%s). The host path may be invalid" % \
                 (url, e.code)
             try:
+        }
         try:
             r = urllib2.Request(url, body, headers)
             data = urllib2.urlopen(r).read()
-            cj.save(cookiefile)
+            self.cookie_jar.save(self.cookie_file)
             return data
         except urllib2.URLError, e:
             try:
                 debug(e.read())
             except AttributeError:
     return args
 def main(args):
-    # Load the config file
+    if 'USERPROFILE' in os.environ:
         homepath = os.path.join(os.environ["USERPROFILE"], "Local Settings",
                                 "Application Data")
     else:
         homepath = os.environ["HOME"]
     # Load the config and cookie files
     globals()['user_config'] = \
         load_config_file(os.path.join(homepath, ".reviewboardrc"))
     cookie_file = os.path.join(homepath, ".post-review-cookies.txt")
     # Try to find the SCM Client we're going to be working with
     repository_info = None
     tool = None
     if not server_url:
         print "Unable to find a Review Board server for this source code tree."
         sys.exit(1)
-    server = ReviewBoardServer(server_url, repository_info)
+    server = ReviewBoardServer(server_url, repository_info, cookie_file)
     if repository_info.supports_changesets:
         changenum = args[0]
     else:
         changenum = None
     if options.output_diff_only:
         print diff
         sys.exit(0)
     # Let's begin.
-    try:
-        cj.load(cookiefile)
-    except IOError:
-        server.login()
+    server.login()
     review_url = tempt_fate(server, tool, changenum, diff_content=diff,
                             submit_as=options.submit_as)

/trunk/reviewboard/contrib/tools/post-review: 18 changes [ 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 ]

Review Board

beta

This change has been marked as submitted.

Add HTTP Authentication support and clean up the cookie handling code in post-review

Diff revision 1

Other reviews

Your comment