[MERGE][bug #230223] Make both http implementations raise appropriate exceptions on 403 Forbidden

[Andrew Bennetts]

Vincent Ladeuil wrote:
> Hi,
> 
> 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:

    class Post403RequestHandler(http_server.TestingHTTPRequestHandler):
    
        def do_POST(self):
            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
> https://bugs.edge.launchpad.net/bzr/+bug/57644.
> 
> 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

Ok :) 

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.

So,

bb:tweak

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)

Good idea.

> === 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
like:

        # We include 403 in accepted_errors so that send_http_smart_request
	# can handle a 403.  Otherwise a 403 causes an unhandled TransportError.

-Andrew.