Bazaar

Merge lp:~vila/bzr/395714-auth-redirect into lp:bzr

  1. 395714-auth-redirect
  2. Merge into bzr.dev
Proposed by Vincent Ladeuil
Status: Merged
Approved by: John A Meinel
Approved revision: not available
Merged at revision: not available
Proposed branch: lp:~vila/bzr/395714-auth-redirect
Merge into: lp:bzr
Diff against target: 417 lines (+141/-46)
5 files modified
NEWS (+9/-0)
bzrlib/tests/test_http.py (+115/-39)
bzrlib/transport/http/__init__.py (+1/-1)
bzrlib/transport/http/_urllib.py (+2/-2)
bzrlib/transport/http/_urllib2_wrappers.py (+14/-4)
To merge this branch: bzr merge lp:~vila/bzr/395714-auth-redirect
Reviewer Review Type Date Requested Status
John A Meinel Needs Fixing
Review via email: mp+15616@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Vincent Ladeuil (vila) wrote :

This patch fixes bug #395714 caused by bzr-svn using the old code path to handle the http redirections and that code path
wasn't handling the authentication.

The associated branch implements that.

For the record, bzr-svn used that code path because the bzr http clients don't
implement the http OPTIONS request.

I'll do that in a later submission.

Revision history for this message
John A Meinel (jameinel) wrote :

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Vincent Ladeuil wrote:
> Vincent Ladeuil has proposed merging lp:~vila/bzr/395714-auth-redirect into lp:bzr.
>
> Requested reviews:
> bzr-core (bzr-core)
>
>
> This patch fixes bug #395714 caused by bzr-svn using the old code path to handle the http redirections and that code path
> wasn't handling the authentication.
>
> The associated branch implements that.
>
>
> For the record, bzr-svn used that code path because the bzr http clients don't
> implement the http OPTIONS request.
>
> I'll do that in a later submission.
>

You have an odd mix
+ self.assertEqual('', ui.ui_factory.stdin.readline())
+ self.assertEquals('', stdout.getvalue())

I personally prefer the "assertEqual" form, but mostly I would just
recommend standardizing on one or the other.

Otherwise it seems fine to me. I assume the main change is:

auth['user'] => auth.get('user', None), etc.

And, this section:
+ # Put some common info in auth if the caller didn't
+ if auth.get('path', None) is None:
+ (protocol, _, _,
+ host, port, path) = urlutils.parse_url(request.get_full_url())
+ self.update_auth(auth, 'protocol', protocol)
+ self.update_auth(auth, 'host', host)
+ self.update_auth(auth, 'port', port)
+ self.update_auth(auth, 'path', path)

Which I don't fully understand. Is the assumption that when path isn't
set, it gets set here? Good enough, I guess.

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAksX920ACgkQJdeBCYSNAAOd9ACguNZT6P4r7xJprdpWer9lAf5e
2A4AoKVOpb96pbDPH6Pwo9KQZ7YiqUlx
=ftx7
-----END PGP SIGNATURE-----

Revision history for this message
John A Meinel (jameinel) wrote :

seems I forgot to vote.

review: Needs Fixing

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'NEWS'
2--- NEWS 2009-12-04 10:16:00 +0000
3+++ NEWS 2009-12-04 11:11:13 +0000
4@@ -202,6 +202,7 @@
5 * The fix for bug #186920 accidentally broke compatibility with python
6 2.4. (Vincent Ladeuil, #475585)
7
8+<<<<<<< TREE
9 * Using ``Repository.get_commit_builder().record_iter_changes()`` now
10 correctly sets ``self.inv_sha1`` to a sha1 string and
11 ``self.new_inventory`` to an Inventory instance after calling
12@@ -216,6 +217,14 @@
13
14 * Avoid unnecessarily flushing of trace file; it's now unbuffered at the
15 Python level. (Martin Pool)
16+=======
17+* Fixed bug with redirected URLs over authenticated HTTP.
18+ (Glen Mailer, Neil Martinsen-Burrell, Vincent Ladeuil, #395714)
19+
20+
21+Improvements
22+************
23+>>>>>>> MERGE-SOURCE
24
25 Documentation
26 *************
27
28=== modified file 'bzrlib/tests/test_http.py'
29--- bzrlib/tests/test_http.py 2009-11-18 16:02:53 +0000
30+++ bzrlib/tests/test_http.py 2009-12-04 11:11:13 +0000
31@@ -269,19 +269,19 @@
32
33 def test_empty_header(self):
34 scheme, remainder = self.parse_header('')
35- self.assertEquals('', scheme)
36+ self.assertEqual('', scheme)
37 self.assertIs(None, remainder)
38
39 def test_negotiate_header(self):
40 scheme, remainder = self.parse_header('Negotiate')
41- self.assertEquals('negotiate', scheme)
42+ self.assertEqual('negotiate', scheme)
43 self.assertIs(None, remainder)
44
45 def test_basic_header(self):
46 scheme, remainder = self.parse_header(
47 'Basic realm="Thou should not pass"')
48- self.assertEquals('basic', scheme)
49- self.assertEquals('realm="Thou should not pass"', remainder)
50+ self.assertEqual('basic', scheme)
51+ self.assertEqual('realm="Thou should not pass"', remainder)
52
53 def test_basic_extract_realm(self):
54 scheme, remainder = self.parse_header(
55@@ -289,13 +289,13 @@
56 _urllib2_wrappers.BasicAuthHandler)
57 match, realm = self.auth_handler.extract_realm(remainder)
58 self.assertTrue(match is not None)
59- self.assertEquals('Thou should not pass', realm)
60+ self.assertEqual('Thou should not pass', realm)
61
62 def test_digest_header(self):
63 scheme, remainder = self.parse_header(
64 'Digest realm="Thou should not pass"')
65- self.assertEquals('digest', scheme)
66- self.assertEquals('realm="Thou should not pass"', remainder)
67+ self.assertEqual('digest', scheme)
68+ self.assertEqual('realm="Thou should not pass"', remainder)
69
70
71 class TestHTTPServer(tests.TestCase):
72@@ -393,14 +393,14 @@
73 def test_url_parsing(self):
74 f = FakeManager()
75 url = http.extract_auth('http://example.com', f)
76- self.assertEquals('http://example.com', url)
77- self.assertEquals(0, len(f.credentials))
78+ self.assertEqual('http://example.com', url)
79+ self.assertEqual(0, len(f.credentials))
80 url = http.extract_auth(
81 'http://user:pass@www.bazaar-vcs.org/bzr/bzr.dev', f)
82- self.assertEquals('http://www.bazaar-vcs.org/bzr/bzr.dev', url)
83- self.assertEquals(1, len(f.credentials))
84- self.assertEquals([None, 'www.bazaar-vcs.org', 'user', 'pass'],
85- f.credentials[0])
86+ self.assertEqual('http://www.bazaar-vcs.org/bzr/bzr.dev', url)
87+ self.assertEqual(1, len(f.credentials))
88+ self.assertEqual([None, 'www.bazaar-vcs.org', 'user', 'pass'],
89+ f.credentials[0])
90
91
92 class TestHttpTransportUrls(tests.TestCase):
93@@ -1366,6 +1366,14 @@
94 self.follow_redirections = True
95
96
97+def install_redirected_request(test):
98+ test.original_class = _urllib2_wrappers.Request
99+ def restore():
100+ _urllib2_wrappers.Request = test.original_class
101+ _urllib2_wrappers.Request = RedirectedRequest
102+ test.addCleanup(restore)
103+
104+
105 class TestHTTPSilentRedirections(http_utils.TestCaseWithRedirectedWebserver):
106 """Test redirections.
107
108@@ -1385,8 +1393,7 @@
109 raise tests.TestNotApplicable(
110 "pycurl doesn't redirect silently annymore")
111 super(TestHTTPSilentRedirections, self).setUp()
112- self.setup_redirected_request()
113- self.addCleanup(self.cleanup_redirected_request)
114+ install_redirected_request(self)
115 self.build_tree_contents([('a','a'),
116 ('1/',),
117 ('1/a', 'redirected once'),
118@@ -1402,13 +1409,6 @@
119
120 self.old_transport = self._transport(self.old_server.get_url())
121
122- def setup_redirected_request(self):
123- self.original_class = _urllib2_wrappers.Request
124- _urllib2_wrappers.Request = RedirectedRequest
125-
126- def cleanup_redirected_request(self):
127- _urllib2_wrappers.Request = self.original_class
128-
129 def create_transport_secondary_server(self):
130 """Create the secondary server, redirections are defined in the tests"""
131 return http_utils.HTTPServerRedirecting(
132@@ -1418,18 +1418,16 @@
133 t = self.old_transport
134
135 req = RedirectedRequest('GET', t.abspath('a'))
136- req.follow_redirections = True
137 new_prefix = 'http://%s:%s' % (self.new_server.host,
138 self.new_server.port)
139 self.old_server.redirections = \
140 [('(.*)', r'%s/1\1' % (new_prefix), 301),]
141- self.assertEquals('redirected once',t._perform(req).read())
142+ self.assertEqual('redirected once',t._perform(req).read())
143
144 def test_five_redirections(self):
145 t = self.old_transport
146
147 req = RedirectedRequest('GET', t.abspath('a'))
148- req.follow_redirections = True
149 old_prefix = 'http://%s:%s' % (self.old_server.host,
150 self.old_server.port)
151 new_prefix = 'http://%s:%s' % (self.new_server.host,
152@@ -1441,7 +1439,7 @@
153 ('/4(.*)', r'%s/5\1' % (new_prefix), 301),
154 ('(/[^/]+)', r'%s/1\1' % (old_prefix), 301),
155 ]
156- self.assertEquals('redirected 5 times',t._perform(req).read())
157+ self.assertEqual('redirected 5 times',t._perform(req).read())
158
159
160 class TestDoCatchRedirections(http_utils.TestCaseWithRedirectedWebserver):
161@@ -1460,8 +1458,8 @@
162 t = self._transport(self.new_server.get_url())
163
164 # We use None for redirected so that we fail if redirected
165- self.assertEquals('0123456789',
166- transport.do_catching_redirections(
167+ self.assertEqual('0123456789',
168+ transport.do_catching_redirections(
169 self.get_a, t, None).read())
170
171 def test_one_redirection(self):
172@@ -1472,10 +1470,10 @@
173 dir, file = urlutils.split(exception.target)
174 return self._transport(dir)
175
176- self.assertEquals('0123456789',
177- transport.do_catching_redirections(
178+ self.assertEqual('0123456789',
179+ transport.do_catching_redirections(
180 self.get_a, self.old_transport, redirected).read())
181- self.assertEquals(1, self.redirections)
182+ self.assertEqual(1, self.redirections)
183
184 def test_redirection_loop(self):
185
186@@ -1580,8 +1578,8 @@
187 self.assertEqual('', ui.ui_factory.stdin.readline())
188 stderr.seek(0)
189 expected_prompt = self._expected_username_prompt(t._unqualified_scheme)
190- self.assertEquals(expected_prompt, stderr.read(len(expected_prompt)))
191- self.assertEquals('', stdout.getvalue())
192+ self.assertEqual(expected_prompt, stderr.read(len(expected_prompt)))
193+ self.assertEqual('', stdout.getvalue())
194 self._check_password_prompt(t._unqualified_scheme, 'joe',
195 stderr.readline())
196
197@@ -1602,7 +1600,7 @@
198 self.assertEqual('', ui.ui_factory.stdin.readline())
199 self._check_password_prompt(t._unqualified_scheme, 'joe',
200 stderr.getvalue())
201- self.assertEquals('', stdout.getvalue())
202+ self.assertEqual('', stdout.getvalue())
203 # And we shouldn't prompt again for a different request
204 # against the same transport.
205 self.assertEqual('contents of b\n',t.get('b').read())
206@@ -1618,7 +1616,7 @@
207 % (scheme.upper(),
208 user, self.server.host, self.server.port,
209 self.server.auth_realm)))
210- self.assertEquals(expected_prompt, actual_prompt)
211+ self.assertEqual(expected_prompt, actual_prompt)
212
213 def _expected_username_prompt(self, scheme):
214 return (self._username_prompt_prefix
215@@ -1638,7 +1636,7 @@
216 self.server.add_user(user, password)
217 t = self.get_user_transport(user, None)
218 ui.ui_factory = tests.TestUIFactory(stdin=stdin_content,
219- stdout=tests.StringIOWrapper())
220+ stderr=tests.StringIOWrapper())
221 # Create a minimal config file with the right password
222 conf = config.AuthenticationConfig()
223 conf._get_config().update(
224@@ -1856,7 +1854,7 @@
225 'http://www.example.com/foo/subdir')
226 self.assertIsInstance(r, type(t))
227 # Both transports share the some connection
228- self.assertEquals(t._get_connection(), r._get_connection())
229+ self.assertEqual(t._get_connection(), r._get_connection())
230
231 def test_redirected_to_self_with_slash(self):
232 t = self._transport('http://www.example.com/foo')
233@@ -1866,7 +1864,7 @@
234 # Both transports share the some connection (one can argue that we
235 # should return the exact same transport here, but that seems
236 # overkill).
237- self.assertEquals(t._get_connection(), r._get_connection())
238+ self.assertEqual(t._get_connection(), r._get_connection())
239
240 def test_redirected_to_host(self):
241 t = self._transport('http://www.example.com/foo')
242@@ -1891,7 +1889,7 @@
243 r = t._redirected_to('http://www.example.com/foo',
244 'https://foo.example.com/foo')
245 self.assertIsInstance(r, type(t))
246- self.assertEquals(t._user, r._user)
247+ self.assertEqual(t._user, r._user)
248
249
250 class PredefinedRequestHandler(http_server.TestingHTTPRequestHandler):
251@@ -2154,3 +2152,81 @@
252 def assertActivitiesMatch(self):
253 # Nothing to check here
254 pass
255+
256+
257+class TestAuthOnRedirected(http_utils.TestCaseWithRedirectedWebserver):
258+ """Test authentication on the redirected http server."""
259+
260+ _auth_header = 'Authorization'
261+ _password_prompt_prefix = ''
262+ _username_prompt_prefix = ''
263+ _auth_server = http_utils.HTTPBasicAuthServer
264+ _transport = _urllib.HttpTransport_urllib
265+
266+ def create_transport_readonly_server(self):
267+ return self._auth_server()
268+
269+ def create_transport_secondary_server(self):
270+ """Create the secondary server redirecting to the primary server"""
271+ new = self.get_readonly_server()
272+
273+ redirecting = http_utils.HTTPServerRedirecting()
274+ redirecting.redirect_to(new.host, new.port)
275+ return redirecting
276+
277+ def setUp(self):
278+ super(TestAuthOnRedirected, self).setUp()
279+ self.build_tree_contents([('a','a'),
280+ ('1/',),
281+ ('1/a', 'redirected once'),
282+ ],)
283+ new_prefix = 'http://%s:%s' % (self.new_server.host,
284+ self.new_server.port)
285+ self.old_server.redirections = [
286+ ('(.*)', r'%s/1\1' % (new_prefix), 301),]
287+ self.old_transport = self._transport(self.old_server.get_url())
288+ self.new_server.add_user('joe', 'foo')
289+
290+ def get_a(self, transport):
291+ return transport.get('a')
292+
293+ def test_auth_on_redirected_via_do_catching_redirections(self):
294+ self.redirections = 0
295+
296+ def redirected(transport, exception, redirection_notice):
297+ self.redirections += 1
298+ dir, file = urlutils.split(exception.target)
299+ return self._transport(dir)
300+
301+ stdout = tests.StringIOWrapper()
302+ stderr = tests.StringIOWrapper()
303+ ui.ui_factory = tests.TestUIFactory(stdin='joe\nfoo\n',
304+ stdout=stdout, stderr=stderr)
305+ self.assertEqual('redirected once',
306+ transport.do_catching_redirections(
307+ self.get_a, self.old_transport, redirected).read())
308+ self.assertEqual(1, self.redirections)
309+ # stdin should be empty
310+ self.assertEqual('', ui.ui_factory.stdin.readline())
311+ # stdout should be empty, stderr will contains the prompts
312+ self.assertEqual('', stdout.getvalue())
313+
314+ def test_auth_on_redirected_via_following_redirections(self):
315+ self.new_server.add_user('joe', 'foo')
316+ stdout = tests.StringIOWrapper()
317+ stderr = tests.StringIOWrapper()
318+ ui.ui_factory = tests.TestUIFactory(stdin='joe\nfoo\n',
319+ stdout=stdout, stderr=stderr)
320+ t = self.old_transport
321+ req = RedirectedRequest('GET', t.abspath('a'))
322+ new_prefix = 'http://%s:%s' % (self.new_server.host,
323+ self.new_server.port)
324+ self.old_server.redirections = [
325+ ('(.*)', r'%s/1\1' % (new_prefix), 301),]
326+ self.assertEqual('redirected once',t._perform(req).read())
327+ # stdin should be empty
328+ self.assertEqual('', ui.ui_factory.stdin.readline())
329+ # stdout should be empty, stderr will contains the prompts
330+ self.assertEqual('', stdout.getvalue())
331+
332+
333
334=== modified file 'bzrlib/transport/http/__init__.py'
335--- bzrlib/transport/http/__init__.py 2009-08-19 16:33:39 +0000
336+++ bzrlib/transport/http/__init__.py 2009-12-04 11:11:12 +0000
337@@ -154,7 +154,7 @@
338 None, None, self._host, self._port, path)
339
340 def _create_auth(self):
341- """Returns a dict returning the credentials provided at build time."""
342+ """Returns a dict containing the credentials provided at build time."""
343 auth = dict(host=self._host, port=self._port,
344 user=self._user, password=self._password,
345 protocol=self._unqualified_scheme,
346
347=== modified file 'bzrlib/transport/http/_urllib.py'
348--- bzrlib/transport/http/_urllib.py 2009-03-23 14:59:43 +0000
349+++ bzrlib/transport/http/_urllib.py 2009-12-04 11:11:13 +0000
350@@ -87,8 +87,8 @@
351 self._update_credentials((request.auth, request.proxy_auth))
352
353 code = response.code
354- if request.follow_redirections is False \
355- and code in (301, 302, 303, 307):
356+ if (request.follow_redirections is False
357+ and code in (301, 302, 303, 307)):
358 raise errors.RedirectRequested(request.get_full_url(),
359 request.redirected_to,
360 is_permanent=(code == 301))
361
362=== modified file 'bzrlib/transport/http/_urllib2_wrappers.py'
363--- bzrlib/transport/http/_urllib2_wrappers.py 2009-11-03 13:46:16 +0000
364+++ bzrlib/transport/http/_urllib2_wrappers.py 2009-12-04 11:11:13 +0000
365@@ -71,6 +71,7 @@
366 trace,
367 transport,
368 ui,
369+ urlutils,
370 )
371
372
373@@ -1066,6 +1067,14 @@
374
375 auth = self.get_auth(request)
376 auth['modified'] = False
377+ # Put some common info in auth if the caller didn't
378+ if auth.get('path', None) is None:
379+ (protocol, _, _,
380+ host, port, path) = urlutils.parse_url(request.get_full_url())
381+ self.update_auth(auth, 'protocol', protocol)
382+ self.update_auth(auth, 'host', host)
383+ self.update_auth(auth, 'port', port)
384+ self.update_auth(auth, 'path', path)
385 # FIXME: the auth handler should be selected at a single place instead
386 # of letting all handlers try to match all headers, but the current
387 # design doesn't allow a simple implementation.
388@@ -1167,8 +1176,8 @@
389 and then during dialog with the server).
390 """
391 auth_conf = config.AuthenticationConfig()
392- user = auth['user']
393- password = auth['password']
394+ user = auth.get('user', None)
395+ password = auth.get('password', None)
396 realm = auth['realm']
397
398 if user is None:
399@@ -1308,7 +1317,8 @@
400 # Put useful info into auth
401 self.update_auth(auth, 'scheme', scheme)
402 self.update_auth(auth, 'realm', realm)
403- if auth['user'] is None or auth['password'] is None:
404+ if (auth.get('user', None) is None
405+ or auth.get('password', None) is None):
406 user, password = self.get_user_password(auth)
407 self.update_auth(auth, 'user', user)
408 self.update_auth(auth, 'password', password)
409@@ -1373,7 +1383,7 @@
410 # Put useful info into auth
411 self.update_auth(auth, 'scheme', scheme)
412 self.update_auth(auth, 'realm', realm)
413- if auth['user'] is None or auth['password'] is None:
414+ if auth.get('user', None) is None or auth.get('password', None) is None:
415 user, password = self.get_user_password(auth)
416 self.update_auth(auth, 'user', user)
417 self.update_auth(auth, 'password', password)