[MERGE][bug #230223] Make both http implementations raise appropriate exceptions on 403 Forbidden
andrew at canonical.com
Mon May 19 00:58:50 BST 2008
Vincent Ladeuil wrote:
> This bug was a due to a slight difference in the way both http
> implementations handled a 403 error code.
> I'm not totally happy with the fix for reasons mentioned below
> but I think the fix should be merged asap nevertheless.
> 1) There is no easy way to write a test for this bug since our
> http servers lacks the ability to handle POST requests, I filed
> bug #231649 about it.
That's not hard to fix, surely? Isn't it just this:
self.send_error(403, "Permission Denied")
http_server = http_server.HttpServer(Post403RequestHandler)
See also bzrlib.tests.http_utils.HTTPServerWithSmarts.
> 2) The fix itself, is a bit unclear in that it makes urllib
> accept the 403 and relies on http.send_http_smart_request to
> turn it into a SmartProtocolError like it does for pycurl.
> Historically, 403 special casing was introduced to fix
> But for the smart server that fix is kind of wrong since only 200
> is an acceptable answer for send_http_smart_request.
That explanation makes sense to me.
> I'm still lacking sufficient knowledge of the smart server to
> design a better fix.
> Andrew, I've put you in cc as I think you have to be the
> reviewer. The alternative is to finish the fix yourself ;-P
This fix is ok by me, so long as it is tested. An alternative might be to
change http_error_default to raise InvalidHttpResponse for 403 (but maybe that
would break the fix for 57644?), or to change send_http_smart_request to catch
that TransportError as well. But this fix is fine, I think.
Because it needs tests (one each for urllib and pycurl). Other comments inline:
> === modified file 'NEWS'
> --- NEWS 2008-05-17 00:41:33 +0000
> +++ NEWS 2008-05-18 14:16:54 +0000
> @@ -31,6 +31,9 @@
> used for an ssh scheme (sftp or bzr+ssh).
> (Vincent Ladeuil, #203186)
> + * Make both http implementations raise appropriate exceptions on 403
> + Forbidden.
> + (Vincent Ladeuil, #230223)
Maybe mention that this is just for POST requests, here?
> === modified file 'bzrlib/transport/http/_pycurl.py'
> --- bzrlib/transport/http/_pycurl.py 2007-12-20 16:36:44 +0000
> +++ bzrlib/transport/http/_pycurl.py 2008-05-18 13:59:54 +0000
> @@ -278,7 +278,8 @@
> # requests
> if code == 403:
> raise errors.TransportError(
> - 'Server refuses to fullfil the request for: %s' % url)
> + 'Server refuses to fulfill the request (403 Forbidden)'
> + ' for %s' % url)
> === modified file 'bzrlib/transport/http/_urllib.py'
> --- bzrlib/transport/http/_urllib.py 2008-03-05 15:48:27 +0000
> +++ bzrlib/transport/http/_urllib.py 2008-05-18 14:50:23 +0000
> @@ -133,7 +133,8 @@
> def _post(self, body_bytes):
> abspath = self._remote_path('.bzr/smart')
> - response = self._perform(Request('POST', abspath, body_bytes))
> + response = self._perform(Request('POST', abspath, body_bytes,
> + accepted_errors=[200, 403]))
This deserves a comment in the code, because this could be confusing. Something
# We include 403 in accepted_errors so that send_http_smart_request
# can handle a 403. Otherwise a 403 causes an unhandled TransportError.
More information about the bazaar