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

[Vincent Ladeuil]

>>>>> "Andrew" == Andrew Bennetts <andrew at canonical.com> writes:

    Andrew> 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.

    Andrew> That's not hard to fix, surely?  Isn't it just this:

    Andrew>     class Post403RequestHandler(http_server.TestingHTTPRequestHandler):
    
    Andrew>         def do_POST(self):
    Andrew>             self.send_error(403, "Permission Denied")

It is ! Simpler even, see patch.        
    
    Andrew>     http_server = http_server.HttpServer(Post403RequestHandler)

    Andrew> See also bzrlib.tests.http_utils.HTTPServerWithSmarts.

Of course, I totally forgot that one.

    >> 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.

    Andrew> That explanation makes sense to me.

Good. I settled with a simple patch then, I mentioned some
refactoring on IRC, but it turned out it will have been renaming
_post into _send_smart_request and that seemed gratuitous at that
time.

    Andrew> This fix is ok by me, so long as it is tested.

That was my concern too, thanks for the hint.

    Andrew> An alternative might be to change http_error_default
    Andrew> to raise InvalidHttpResponse for 403 (but maybe that
    Andrew> would break the fix for 57644?),

Yes.

    Andrew> or to change send_http_smart_request to catch that
    Andrew> TransportError as well.

Which I found a bit too generic (that may not matter in that
context but I prefer to have a deeper understanding of hpss
before trying that).

<snip/>

    Andrew> Maybe mention that this is just for POST requests, here?

Done.

    >> === 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)

    Andrew> Good idea.

Not mine :) But there was a discussion about the spelling error
and the lack of 403 reference (either on IRC or here, I can't
remember), so that was the perfect time to take them into
account.

    Andrew> This deserves a comment in the code, because this
    Andrew> could be confusing.  Something like:

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

Done.

Thanks, the bug itself was caused by nearly the same lack of
comments, I think that one capture exactly what was missing.

I'll merge that.

        Vincent