Review Board 2.0 RC2 (dev)


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

Review Request #334 - Created March 30, 2008 and submitted

Information
Marc Hedlund
Review Board SVN (deprecated)
Reviewers
reviewboard
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.)
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.
Posted (March 30, 2008, 6:28 p.m.)
This looks real good. Just a few minor syntax changes I'd like to see. (I swear I'll write a style guide someday.)
The whitespace here should be cleaned up. Same with other locations in the file.
  1. Hm, I think the whitespace style this implies breaks folding in TextMate (my text editor), but let me look into it.  I made the changes per your style -- if I'm going to bring up a whitespace complaint I'll do it offlist.  :)
Small cleanup, but you can do:

print 'Enter username and password for "%s" at %s'
  1. Done.
It's hard to tell from the diff, but make sure this wraps to 80 columns.
  1. 80 exactly.
Blank line before each comment.
  1. Done.
0 is implied. You can use host\[:port_idx\]

Might be slightly cleaner to use split() instead for this though. Not a big deal either way.
  1. You're right, split() is more concise.  Done.
Might be cleaner to just put this up by host's definition, and clump those with parsed_url's.
  1. Done.
Align this paren to the first parameter of debug().
  1. Done.
Blank line before this.
  1. Done.
Blank line before this.
  1. Done.
Blank line before this.
  1. Done.
Posted (March 30, 2008, 8:09 p.m.)



  
Do you want me to change the version?  If so, to what, 0.7 or 0.6.1?
Ship it!
Posted (March 31, 2008, 7:30 p.m.)
Committed in r1256. Thanks\!