View Review RequestAdd HTTP Authentication support and clean up the cookie handling code in post-review
Submitted
Updated 4 months, 4 weeks ago
| Marc Hedlund | Reviewers | ||
| reviewboard | |||
| None | Review Board SVN | ||
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.
Expand changes| /trunk/reviewboard/contrib/tools/post-review | ||||
|---|---|---|---|---|
| Revision 1254 | New Change | |||
| ... | 9 lines hidden [Expand] | |||
| 10 | import subprocess |
10 | import subprocess |
|
| 11 | import sys |
11 | import sys |
|
| 12 | import urllib2 |
12 | import urllib2 |
|
| 13 | from optparse import OptionParser |
13 | from optparse import OptionParser |
|
| 14 | from tempfile import mkstemp |
14 | from tempfile import mkstemp |
|
| 15 | from urlparse import urljoin |
15 | from urlparse import urljoin, urlparse |
|
| 16 | 16 | |||
| 17 | VERSION = "0.6" |
17 | VERSION = "0.6" |
|
| 18 | 18 | |||
| 19 | # Who stole the cookies from the cookie jar? |
|||
| 20 | # Was it you? |
|||
| 21 | # >:( |
|||
| 22 | if 'USERPROFILE' in os.environ: |
|||
| 23 | homepath = os.path.join(os.environ["USERPROFILE"], "Local Settings", |
|||
| 24 | "Application Data") |
|||
| 25 | else: |
|||
| 26 | homepath = os.environ["HOME"] |
|||
| 27 | ||||
| 28 | cj = cookielib.MozillaCookieJar() |
|||
| 29 | cookiefile = os.path.join(homepath, ".post-review-cookies.txt") |
|||
| 30 | ||||
| 31 | opener = urllib2.build_opener(urllib2.HTTPCookieProcessor(cj)) |
|||
| 32 | opener.addheaders = [('User-agent', 'post-review/' + VERSION)] |
|||
| 33 | urllib2.install_opener(opener) |
|||
| 34 | ||||
| 35 | user_config = None |
19 | user_config = None |
|
| 36 | tempfiles = [] |
20 | tempfiles = [] |
|
| 37 | options = None |
21 | options = None |
|
| 38 | 22 | |||
| 39 | 23 | |||
| ... | 13 lines hidden [Expand] | |||
| 53 | def __str__(self): |
37 | def __str__(self): |
|
| 54 | return "Path: %s, Base path: %s, Supports changesets: %s" % \ |
38 | return "Path: %s, Base path: %s, Supports changesets: %s" % \ |
|
| 55 | (self.path, self.base_path, self.supports_changesets) |
39 | (self.path, self.base_path, self.supports_changesets) |
|
| 56 | 40 | |||
| 57 | 41 | |||
| 42 | class ReviewBoardHTTPPasswordMgr(urllib2.HTTPPasswordMgr): |
|||
| 43 | """ |
|||
| 44 | Python 2.4's password manager has a bug in http authentication when the |
|||
| 45 | target server uses a non-standard port. Since ReviewBoard supports 2.4 |
|||
| 46 | (and the patch contributor's 2.4 server uses a non-standard port), this |
|||
| 47 | overrides the default urllib2.HTTPPasswordMgrWithDefaultRealm to provide |
|||
| 48 | HTTP Authentication support. (See http://bugs.python.org/issue974757) |
|||
| 49 | This also allows post-review to prompt for passwords in a consistent way. |
|||
| 50 | """ |
|||
| 51 | def __init__(self, reviewboard_url): |
|||
| 52 | self.passwd = {} |
|||
| 53 | self.rb_url = reviewboard_url |
|||
| 54 | self.rb_user = None |
|||
| 55 | self.rb_pass = None |
|||
| 56 | ||||
| 57 | def find_user_password(self, realm, uri): |
|||
| 58 | if uri.startswith(self.rb_url): |
|||
| 59 | if self.rb_user is None or self.rb_pass is None: |
|||
| 60 | print "==> HTTP Authentication Required" |
|||
| 61 | print 'Enter username and password for "%s" at %s' % \ |
|||
| 62 | (realm, urlparse(uri)[1]) |
|||
| 63 | self.rb_user = raw_input('Username: ') |
|||
| 64 | self.rb_pass = getpass.getpass('Password: ') |
|||
| 65 | ||||
| 66 | return self.rb_user, self.rb_pass |
|||
| 67 | else: |
|||
| 68 | ||||
| 69 | # If this is an auth request for some other domain (since HTTP |
|||
| 70 | # handlers are global), fall back to standard password management. |
|||
| 71 | return urllib2.HTTPPasswordMgr.find_user_password(self, realm, uri) |
|||
| 72 | ||||
| 73 | ||||
| 58 | class ReviewBoardServer: |
74 | class ReviewBoardServer: |
|
| 59 | """ |
75 | """ |
|
| 60 | An instance of a Review Board server. |
76 | An instance of a Review Board server. |
|
| 61 | """ |
77 | """ |
|
| 62 | def __init__(self, url, info): |
78 | def __init__(self, url, info, cookie_file): |
|
| 63 | self.url = url |
79 | self.url = url |
|
| 64 | self.info = info |
80 | self.info = info |
|
| 81 | self.cookie_file = cookie_file |
|||
| 82 | self.cookie_jar = cookielib.MozillaCookieJar(self.cookie_file) |
|||
| 83 | ||||
| 84 | # Set up the HTTP libraries to support all of the features we need. |
|||
| 85 | cookie_handler = urllib2.HTTPCookieProcessor(self.cookie_jar) |
|||
| 86 | password_mgr = ReviewBoardHTTPPasswordMgr(self.url) |
|||
| 87 | auth_handler = urllib2.HTTPBasicAuthHandler(password_mgr) |
|||
| 88 | ||||
| 89 | opener = urllib2.build_opener(cookie_handler, auth_handler) |
|||
| 90 | opener.addheaders = [('User-agent', 'post-review/' + VERSION)] |
|||
| 91 | urllib2.install_opener(opener) |
|||
| 65 | 92 | |||
| 66 | def login(self): |
93 | def login(self): |
|
| 67 | """ |
94 | """ |
|
| 68 | Logs in to a Review Board server, prompting the user for login |
95 | Logs in to a Review Board server, prompting the user for login |
|
| 69 | information. |
96 | information if needed. |
|
| 70 | """ |
97 | """ |
|
| 71 | print "You must log in the first time." |
98 | if self.has_valid_cookie(): |
|
| 99 | return |
|||
| 100 | print "==> Review Board Login Required" |
|||
| 101 | print "Enter username and password for Review Board at %s" % self.url |
|||
| 72 | username = raw_input('Username: ') |
102 | username = raw_input('Username: ') |
|
| 73 | password = getpass.getpass('Password: ') |
103 | password = getpass.getpass('Password: ') |
|
| 74 | 104 | |||
| 75 | debug('Logging in with username "%s"' % username) |
105 | debug('Logging in with username "%s"' % username) |
|
| 76 | try: |
106 | try: |
|
| ... | 7 lines hidden [Expand] | |||
| 84 | die("Unable to log in: %s (%s)" % (rsp["err"]["msg"], |
114 | die("Unable to log in: %s (%s)" % (rsp["err"]["msg"], |
|
| 85 | rsp["err"]["code"])) |
115 | rsp["err"]["code"])) |
|
| 86 | 116 | |||
| 87 | debug("Logged in.") |
117 | debug("Logged in.") |
|
| 88 | 118 | |||
| 119 | def has_valid_cookie(self): |
|||
| 120 | """ |
|||
| 121 | Load up the user's cookie file, and see if they have a valid |
|||
| 122 | 'sessionid' cookie for the current Review Board server. Returns |
|||
| 123 | true if so and false otherwise. |
|||
| 124 | """ |
|||
| 125 | try: |
|||
| 126 | parsed_url = urlparse(self.url) |
|||
| 127 | host = parsed_url[1] |
|||
| 128 | path = parsed_url[2] or '/' |
|||
| 129 | ||||
| 130 | # Cookie files don't store port numbers, unfortunately, so |
|||
| 131 | # get rid of the port number if it's present. |
|||
| 132 | host = host.split(":")[0] |
|||
| 133 | ||||
| 134 | debug("Looking for '%s %s' cookie in %s" % \ |
|||
| 135 | (host, path, self.cookie_file)) |
|||
| 136 | self.cookie_jar.load(self.cookie_file, ignore_expires=True) |
|||
| 137 | ||||
| 138 | try: |
|||
| 139 | cookie = self.cookie_jar._cookies[host][path]['sessionid'] |
|||
| 140 | ||||
| 141 | if not cookie.is_expired(): |
|||
| 142 | debug("Loaded valid cookie -- no login required") |
|||
| 143 | return True |
|||
| 144 | else: |
|||
| 145 | debug("Cookie file loaded, but cookie has expired") |
|||
| 146 | except KeyError: |
|||
| 147 | debug("Cookie file loaded, but no cookie for this server") |
|||
| 148 | except IOError, error: |
|||
| 149 | debug("Couldn't load cookie file: %s" % error) |
|||
| 150 | ||||
| 151 | return False |
|||
| 152 | ||||
| 89 | def new_review_request(self, changenum, submit_as=None): |
153 | def new_review_request(self, changenum, submit_as=None): |
|
| 90 | """ |
154 | """ |
|
| 91 | Creates a review request on a Review Board server, updating an |
155 | Creates a review request on a Review Board server, updating an |
|
| 92 | existing one if the changeset number already exists. |
156 | existing one if the changeset number already exists. |
|
| 93 | 157 | |||
| ... | 100 lines hidden [Expand] | |||
| 194 | 258 | |||
| 195 | url = self._make_url(path) |
259 | url = self._make_url(path) |
|
| 196 | 260 | |||
| 197 | try: |
261 | try: |
|
| 198 | rsp = urllib2.urlopen(url).read() |
262 | rsp = urllib2.urlopen(url).read() |
|
| 199 | cj.save(cookiefile) |
263 | self.cookie_jar.save(self.cookie_file) |
|
| 200 | return rsp |
264 | return rsp |
|
| 201 | except urllib2.HTTPError, e: |
265 | except urllib2.HTTPError, e: |
|
| 202 | print "Unable to access %s (%s). The host path may be invalid" % \ |
266 | print "Unable to access %s (%s). The host path may be invalid" % \ |
|
| 203 | (url, e.code) |
267 | (url, e.code) |
|
| 204 | try: |
268 | try: |
|
| ... | 37 lines hidden [Expand] | |||
| 242 | } |
306 | } |
|
| 243 | 307 | |||
| 244 | try: |
308 | try: |
|
| 245 | r = urllib2.Request(url, body, headers) |
309 | r = urllib2.Request(url, body, headers) |
|
| 246 | data = urllib2.urlopen(r).read() |
310 | data = urllib2.urlopen(r).read() |
|
| 247 | cj.save(cookiefile) |
311 | self.cookie_jar.save(self.cookie_file) |
|
| 248 | return data |
312 | return data |
|
| 249 | except urllib2.URLError, e: |
313 | except urllib2.URLError, e: |
|
| 250 | try: |
314 | try: |
|
| 251 | debug(e.read()) |
315 | debug(e.read()) |
|
| 252 | except AttributeError: |
316 | except AttributeError: |
|
| ... | 827 lines hidden [Expand] | |||
| 1080 | 1144 | |||
| 1081 | return args |
1145 | return args |
|
| 1082 | 1146 | |||
| 1083 | 1147 | |||
| 1084 | def main(args): |
1148 | def main(args): |
|
| 1085 | # Load the config file |
1149 | if 'USERPROFILE' in os.environ: |
|
| 1150 | homepath = os.path.join(os.environ["USERPROFILE"], "Local Settings", |
|||
| 1151 | "Application Data") |
|||
| 1152 | else: |
|||
| 1153 | homepath = os.environ["HOME"] |
|||
| 1154 | ||||
| 1155 | # Load the config and cookie files |
|||
| 1086 | globals()['user_config'] = \ |
1156 | globals()['user_config'] = \ |
|
| 1087 | load_config_file(os.path.join(homepath, ".reviewboardrc")) |
1157 | load_config_file(os.path.join(homepath, ".reviewboardrc")) |
|
| 1158 | cookie_file = os.path.join(homepath, ".post-review-cookies.txt") |
|||
| 1088 | 1159 | |||
| 1089 | # Try to find the SCM Client we're going to be working with |
1160 | # Try to find the SCM Client we're going to be working with |
|
| 1090 | repository_info = None |
1161 | repository_info = None |
|
| 1091 | tool = None |
1162 | tool = None |
|
| 1092 | 1163 | |||
| ... | 20 lines hidden [Expand] | |||
| 1113 | 1184 | |||
| 1114 | if not server_url: |
1185 | if not server_url: |
|
| 1115 | print "Unable to find a Review Board server for this source code tree." |
1186 | print "Unable to find a Review Board server for this source code tree." |
|
| 1116 | sys.exit(1) |
1187 | sys.exit(1) |
|
| 1117 | 1188 | |||
| 1118 | server = ReviewBoardServer(server_url, repository_info) |
1189 | server = ReviewBoardServer(server_url, repository_info, cookie_file) |
|
| 1119 | 1190 | |||
| 1120 | if repository_info.supports_changesets: |
1191 | if repository_info.supports_changesets: |
|
| 1121 | changenum = args[0] |
1192 | changenum = args[0] |
|
| 1122 | else: |
1193 | else: |
|
| 1123 | changenum = None |
1194 | changenum = None |
|
| ... | 6 lines hidden [Expand] | |||
| 1130 | if options.output_diff_only: |
1201 | if options.output_diff_only: |
|
| 1131 | print diff |
1202 | print diff |
|
| 1132 | sys.exit(0) |
1203 | sys.exit(0) |
|
| 1133 | 1204 | |||
| 1134 | # Let's begin. |
1205 | # Let's begin. |
|
| 1135 | try: |
|||
| 1136 | cj.load(cookiefile) |
|||
| 1137 | except IOError: |
|||
| 1138 | server.login() |
1206 | server.login() |
|
| 1139 | 1207 | |||
| 1140 | review_url = tempt_fate(server, tool, changenum, diff_content=diff, |
1208 | review_url = tempt_fate(server, tool, changenum, diff_content=diff, |
|
| 1141 | submit_as=options.submit_as) |
1209 | submit_as=options.submit_as) |
|
| 1142 | 1210 | |||
| ... | 17 lines hidden [Expand] | |||
Comments