View Review RequestAdd HTTP Authentication support and clean up the cookie handling code in post-review
Submitted
Updated 7 months, 3 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 | ||||
|---|---|---|---|---|
| Diff Revision 1 | Diff Revision 2 | |||
| ... | 50 lines hidden [Expand] | |||
| 51 | def __init__(self, reviewboard_url): |
51 | def __init__(self, reviewboard_url): |
|
| 52 | self.passwd = {} |
52 | self.passwd = {} |
|
| 53 | self.rb_url = reviewboard_url |
53 | self.rb_url = reviewboard_url |
|
| 54 | self.rb_user = None |
54 | self.rb_user = None |
|
| 55 | self.rb_pass = None |
55 | self.rb_pass = None |
|
| 56 | |
56 | ||
| 57 | def find_user_password(self, realm, uri): |
57 | def find_user_password(self, realm, uri): |
|
| 58 | if uri.startswith(self.rb_url): |
58 | if uri.startswith(self.rb_url): |
|
| 59 | if self.rb_user is None or self.rb_pass is None: |
59 | if self.rb_user is None or self.rb_pass is None: |
|
| 60 | print "==> HTTP Authentication Required" |
60 | print "==> HTTP Authentication Required" |
|
| 61 | print "Enter username and password for \"%s\" at %s" % \ |
61 | print 'Enter username and password for "%s" at %s' % \ |
|
| 62 | (realm, urlparse(uri)[1]) |
62 | (realm, urlparse(uri)[1]) |
|
| 63 | self.rb_user = raw_input('Username: ') |
63 | self.rb_user = raw_input('Username: ') |
|
| 64 | self.rb_pass = getpass.getpass('Password: ') |
64 | self.rb_pass = getpass.getpass('Password: ') |
|
| 65 | |
65 | ||
| 66 | return self.rb_user, self.rb_pass |
66 | return self.rb_user, self.rb_pass |
|
| 67 | else: |
67 | else: |
|
| 68 | ||||
| 68 | # If this is an auth request for some other domain (since HTTP |
69 | # If this is an auth request for some other domain (since HTTP |
|
| 69 | # handlers are global), fall back to standard password management. |
70 | # handlers are global), fall back to standard password management. |
|
| 70 | return urllib2.HTTPPasswordMgr.find_user_password(self, realm, uri) |
71 | return urllib2.HTTPPasswordMgr.find_user_password(self, realm, uri) |
|
| 71 | |
|||
| 72 | 72 | |||
| 73 | ||||
| 73 | class ReviewBoardServer: |
74 | class ReviewBoardServer: |
|
| 74 | """ |
75 | """ |
|
| 75 | An instance of a Review Board server. |
76 | An instance of a Review Board server. |
|
| 76 | """ |
77 | """ |
|
| 77 | def __init__(self, url, info, cookie_file): |
78 | def __init__(self, url, info, cookie_file): |
|
| 78 | self.url = url |
79 | self.url = url |
|
| 79 | self.info = info |
80 | self.info = info |
|
| 80 | self.cookie_file = cookie_file |
81 | self.cookie_file = cookie_file |
|
| 81 | self.cookie_jar = cookielib.MozillaCookieJar(self.cookie_file) |
82 | self.cookie_jar = cookielib.MozillaCookieJar(self.cookie_file) |
|
| 82 | |
83 | ||
| 83 | # Set up the HTTP libraries to support all of the features we need. |
84 | # Set up the HTTP libraries to support all of the features we need. |
|
| 84 | cookie_handler = urllib2.HTTPCookieProcessor(self.cookie_jar) |
85 | cookie_handler = urllib2.HTTPCookieProcessor(self.cookie_jar) |
|
| 85 | password_mgr = ReviewBoardHTTPPasswordMgr(self.url) |
86 | password_mgr = ReviewBoardHTTPPasswordMgr(self.url) |
|
| 86 | auth_handler = urllib2.HTTPBasicAuthHandler(password_mgr) |
87 | auth_handler = urllib2.HTTPBasicAuthHandler(password_mgr) |
|
| 87 | |
88 | ||
| 88 | opener = urllib2.build_opener(cookie_handler, auth_handler) |
89 | opener = urllib2.build_opener(cookie_handler, auth_handler) |
|
| 89 | opener.addheaders = [('User-agent', 'post-review/' + VERSION)] |
90 | opener.addheaders = [('User-agent', 'post-review/' + VERSION)] |
|
| 90 | urllib2.install_opener(opener) |
91 | urllib2.install_opener(opener) |
|
| 91 | |
92 | ||
| 92 | def login(self): |
93 | def login(self): |
|
| 93 | """ |
94 | """ |
|
| 94 | 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 |
|
| 95 | information if needed. |
96 | information if needed. |
|
| 96 | """ |
97 | """ |
|
| ... | 15 lines hidden [Expand] | |||
| 112 | 113 | |||
| 113 | die("Unable to log in: %s (%s)" % (rsp["err"]["msg"], |
114 | die("Unable to log in: %s (%s)" % (rsp["err"]["msg"], |
|
| 114 | rsp["err"]["code"])) |
115 | rsp["err"]["code"])) |
|
| 115 | 116 | |||
| 116 | debug("Logged in.") |
117 | debug("Logged in.") |
|
| 117 | |
118 | ||
| 118 | def has_valid_cookie(self): |
119 | def has_valid_cookie(self): |
|
| 119 | """ |
120 | """ |
|
| 120 | Load up the user's cookie file, and see if they have a valid |
121 | Load up the user's cookie file, and see if they have a valid |
|
| 121 | 'sessionid' cookie for the current Review Board server. Returns |
122 | 'sessionid' cookie for the current Review Board server. Returns |
|
| 122 | true if so and false otherwise. |
123 | true if so and false otherwise. |
|
| 123 | """ |
124 | """ |
|
| 124 | try: |
125 | try: |
|
| 125 | parsed_url = urlparse(self.url) |
126 | parsed_url = urlparse(self.url) |
|
| 126 | |
|||
| 127 | host = parsed_url[1] |
127 | host = parsed_url[1] |
|
| 128 | # Cookie files don't store port numbers, unfortunately... |
|||
| 129 | port_idx = host.find(':') |
|||
| 130 | if port_idx != -1: |
|||
| 131 | host = host[0:port_idx] |
|||
| 132 | |
|||
| 133 | path = parsed_url[2] or '/' |
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" % \ |
134 | debug("Looking for '%s %s' cookie in %s" % \ |
|
| 135 | (host, path, self.cookie_file)) |
135 | (host, path, self.cookie_file)) |
|
| 136 | self.cookie_jar.load(self.cookie_file, ignore_expires=True) |
136 | self.cookie_jar.load(self.cookie_file, ignore_expires=True) |
|
| 137 | ||||
| 137 | try: |
138 | try: |
|
| 138 | cookie = self.cookie_jar._cookies[host][path]['sessionid'] |
139 | cookie = self.cookie_jar._cookies[host][path]['sessionid'] |
|
| 140 | ||||
| 139 | if not cookie.is_expired(): |
141 | if not cookie.is_expired(): |
|
| 140 | debug("Loaded valid cookie -- no login required") |
142 | debug("Loaded valid cookie -- no login required") |
|
| 141 | return True |
143 | return True |
|
| 142 | else: |
144 | else: |
|
| 143 | debug("Cookie file loaded, but cookie has expired") |
145 | debug("Cookie file loaded, but cookie has expired") |
|
| 144 | except KeyError: |
146 | except KeyError: |
|
| 145 | debug("Cookie file loaded, but no cookie for this server") |
147 | debug("Cookie file loaded, but no cookie for this server") |
|
| 146 | except IOError, error: |
148 | except IOError, error: |
|
| 147 | debug("Couldn't load cookie file: %s" % error) |
149 | debug("Couldn't load cookie file: %s" % error) |
|
| 150 | ||||
| 148 | return False |
151 | return False |
|
| 149 | |
152 | ||
| 150 | def new_review_request(self, changenum, submit_as=None): |
153 | def new_review_request(self, changenum, submit_as=None): |
|
| 151 | """ |
154 | """ |
|
| 152 | Creates a review request on a Review Board server, updating an |
155 | Creates a review request on a Review Board server, updating an |
|
| 153 | existing one if the changeset number already exists. |
156 | existing one if the changeset number already exists. |
|
| 154 | 157 | |||
| ... | 991 lines hidden [Expand] | |||
| 1146 | if 'USERPROFILE' in os.environ: |
1149 | if 'USERPROFILE' in os.environ: |
|
| 1147 | homepath = os.path.join(os.environ["USERPROFILE"], "Local Settings", |
1150 | homepath = os.path.join(os.environ["USERPROFILE"], "Local Settings", |
|
| 1148 | "Application Data") |
1151 | "Application Data") |
|
| 1149 | else: |
1152 | else: |
|
| 1150 | homepath = os.environ["HOME"] |
1153 | homepath = os.environ["HOME"] |
|
| 1151 | |
1154 | ||
| 1152 | # Load the config and cookie files |
1155 | # Load the config and cookie files |
|
| 1153 | globals()['user_config'] = \ |
1156 | globals()['user_config'] = \ |
|
| 1154 | load_config_file(os.path.join(homepath, ".reviewboardrc")) |
1157 | load_config_file(os.path.join(homepath, ".reviewboardrc")) |
|
| 1155 | cookie_file = os.path.join(homepath, ".post-review-cookies.txt") |
1158 | cookie_file = os.path.join(homepath, ".post-review-cookies.txt") |
|
| 1156 | 1159 | |||
| ... | 68 lines hidden [Expand] | |||
Comments