Patchwork [8,of,8] http: reuse authentication info after the first failed request (issue3567)

login
register
mail settings
Submitter Mads Kiilerich
Date Jan. 20, 2014, 12:32 a.m.
Message ID <1748dd9992a0f2eea93e.1390177952@localhost.localdomain>
Download mbox | patch
Permalink /patch/3394/
State Superseded
Commit a939eeb94833b73d65a92eac158475aeae8f3a57
Headers show

Comments

Mads Kiilerich - Jan. 20, 2014, 12:32 a.m.
# HG changeset patch
# User Stéphane Klein <contact@stephane-klein.info>
# Date 1387547765 -3600
#      Fri Dec 20 14:56:05 2013 +0100
# Node ID 1748dd9992a0f2eea93eb8afb1ed2804373dbf55
# Parent  5ec933e1841d7ea04fee58f00b0e9f9929da2ca2
http: reuse authentication info after the first failed request (issue3567)

Context : mercurial access to repository server with http access, and this
server is protected by basic auth.

Before patch :

* mercurial try an anonymous access to server, server return 401 response and
  mercurial resend request with login / password information

After patch :

* mercurial try an anonymous access to server, server return
  401 response. For all next requests, mercurial keep in memory this
  information (this server need basic auth information).

This patch reduce the number of http access against mercurial server.

Example, before patch :

10.10.168.170 - - [25/Oct/2013:15:44:51 +0200] "GET /hg/testagt?cmd=capabilities
HTTP/1.1" 401 260 "-" "mercurial/proto-1.0"
10.10.168.170 - - [25/Oct/2013:15:44:52 +0200] "GET /hg/testagt?cmd=capabilities
HTTP/1.1" 200 147 "-" "mercurial/proto-1.0"
10.10.168.170 - - [25/Oct/2013:15:45:00 +0200] "GET /hg/testagt?cmd=capabilities
HTTP/1.1" 401 260 "-" "mercurial/proto-1.0"
10.10.168.170 - - [25/Oct/2013:15:45:01 +0200] "GET /hg/testagt?cmd=capabilities
HTTP/1.1" 200 147 "-" "mercurial/proto-1.0"
10.10.168.170 - - [25/Oct/2013:15:45:03 +0200] "GET /hg/testagt?cmd=batch
HTTP/1.1" 401 260 "-" "mercurial/proto-1.0"
10.10.168.170 - - [25/Oct/2013:15:45:04 +0200] "GET /hg/testagt?cmd=batch
HTTP/1.1" 200 42 "-" "mercurial/proto-1.0"
10.10.168.170 - - [25/Oct/2013:15:45:06 +0200] "GET /hg/testagt?cmd=getbundle
HTTP/1.1" 401 260 "-" "mercurial/proto-1.0"
10.10.168.170 - - [25/Oct/2013:15:45:07 +0200] "GET /hg/testagt?cmd=getbundle
HTTP/1.1" 200 61184 "-" "mercurial/proto-1.0"
10.10.168.170 - - [25/Oct/2013:15:45:09 +0200] "GET /hg/testagt?cmd=listkeys
HTTP/1.1" 401 260 "-" "mercurial/proto-1.0"
10.10.168.170 - - [25/Oct/2013:15:45:10 +0200] "GET /hg/testagt?cmd=listkeys
HTTP/1.1" 200 15 "-" "mercurial/proto-1.0"
10.10.168.170 - - [25/Oct/2013:15:45:12 +0200] "GET /hg/testagt?cmd=listkeys
HTTP/1.1" 401 260 "-" "mercurial/proto-1.0"
10.10.168.170 - - [25/Oct/2013:15:45:12 +0200] "GET /hg/testagt?cmd=listkeys
HTTP/1.1" 200 - "-" "mercurial/proto-1.0"

Example after patch :

10.10.168.170 - - [28/Oct/2013:11:49:14 +0100] "GET /hg/testagt?cmd=capabilities
HTTP/1.1" 401 260 "-" "mercurial/proto-1.0"
10.10.168.170 - - [28/Oct/2013:11:49:15 +0100] "GET /hg/testagt?cmd=capabilities
HTTP/1.1" 200 147 "-" "mercurial/proto-1.0"
10.10.168.170 - - [28/Oct/2013:11:49:17 +0100] "GET /hg/testagt?cmd=batch
HTTP/1.1" 200 42 "-" "mercurial/proto-1.0"
10.10.168.170 - - [28/Oct/2013:11:49:19 +0100] "GET /hg/testagt?cmd=getbundle
HTTP/1.1" 200 61184 "-" "mercurial/proto-1.0"
10.10.168.170 - - [28/Oct/2013:11:49:22 +0100] "GET /hg/testagt?cmd=listkeys
HTTP/1.1" 200 15 "-" "mercurial/proto-1.0"
10.10.168.170 - - [28/Oct/2013:11:49:24 +0100] "GET /hg/testagt?cmd=listkeys
HTTP/1.1" 200 - "-" "mercurial/proto-1.0"

In this last example, you can see only one 401 response.
Matt Mackall - Jan. 20, 2014, 11:19 p.m.
On Mon, 2014-01-20 at 01:32 +0100, Mads Kiilerich wrote:
> # HG changeset patch
> # User Stéphane Klein <contact@stephane-klein.info>
> # Date 1387547765 -3600
> #      Fri Dec 20 14:56:05 2013 +0100
> # Node ID 1748dd9992a0f2eea93eb8afb1ed2804373dbf55
> # Parent  5ec933e1841d7ea04fee58f00b0e9f9929da2ca2
> http: reuse authentication info after the first failed request (issue3567)

This is tricky. This is clearly a bugfix, but there's some question
about whether it's the right fix, it's at the end of a series of
miscellaneous changes that aren't all stable-appropriate, and arrived
after the nominal freeze date. It looks like I can't take it by itself
as its test will break.

So, if you're confident this is the way to go, please pull just the fix
portion of this out and submit for stable, and we'll revisit the other
bits for post-2.9.

I've queued patches 3 and 5, which looked sufficiently bug-fixy and
orthogonal to go in on their own, thanks.
Mads Kiilerich - Jan. 21, 2014, 12:16 a.m.
On 01/21/2014 12:19 AM, Matt Mackall wrote:
> On Mon, 2014-01-20 at 01:32 +0100, Mads Kiilerich wrote:
>> # HG changeset patch
>> # User Stéphane Klein <contact@stephane-klein.info>
>> # Date 1387547765 -3600
>> #      Fri Dec 20 14:56:05 2013 +0100
>> # Node ID 1748dd9992a0f2eea93eb8afb1ed2804373dbf55
>> # Parent  5ec933e1841d7ea04fee58f00b0e9f9929da2ca2
>> http: reuse authentication info after the first failed request (issue3567)
> This is tricky. This is clearly a bugfix, but there's some question
> about whether it's the right fix, it's at the end of a series of
> miscellaneous changes that aren't all stable-appropriate, and arrived
> after the nominal freeze date. It looks like I can't take it by itself
> as its test will break.
>
> So, if you're confident this is the way to go, please pull just the fix
> portion of this out and submit for stable, and we'll revisit the other
> bits for post-2.9.
>
> I've queued patches 3 and 5, which looked sufficiently bug-fixy and
> orthogonal to go in on their own, thanks.

2 is necessary to let the tests pass on Fedora. It can be taken without 
the 1 that made it possible to find the problem.

6 fixes existing test failure with Python 2.4, 2.4.1 and 2.4.2. The 
2.4.2 on the build farm must be a patched version.

8 is an augmented version of Stéphane's original patch, augmented with 
some test coverage. If you want it without 7 then just ignore the test 
changes ... or take Stéphane's version.

/Mads
Matt Mackall - Jan. 30, 2014, 8:52 p.m.
On Tue, 2014-01-21 at 01:16 +0100, Mads Kiilerich wrote:
> On 01/21/2014 12:19 AM, Matt Mackall wrote:
> > On Mon, 2014-01-20 at 01:32 +0100, Mads Kiilerich wrote:
> >> # HG changeset patch
> >> # User Stéphane Klein <contact@stephane-klein.info>
> >> # Date 1387547765 -3600
> >> #      Fri Dec 20 14:56:05 2013 +0100
> >> # Node ID 1748dd9992a0f2eea93eb8afb1ed2804373dbf55
> >> # Parent  5ec933e1841d7ea04fee58f00b0e9f9929da2ca2
> >> http: reuse authentication info after the first failed request (issue3567)
> > This is tricky. This is clearly a bugfix, but there's some question
> > about whether it's the right fix, it's at the end of a series of
> > miscellaneous changes that aren't all stable-appropriate, and arrived
> > after the nominal freeze date. It looks like I can't take it by itself
> > as its test will break.
> >
> > So, if you're confident this is the way to go, please pull just the fix
> > portion of this out and submit for stable, and we'll revisit the other
> > bits for post-2.9.
> >
> > I've queued patches 3 and 5, which looked sufficiently bug-fixy and
> > orthogonal to go in on their own, thanks.
> 
> 2 is necessary to let the tests pass on Fedora. It can be taken without 
> the 1 that made it possible to find the problem.
> 
> 6 fixes existing test failure with Python 2.4, 2.4.1 and 2.4.2. The 
> 2.4.2 on the build farm must be a patched version.

It is.

> 8 is an augmented version of Stéphane's original patch, augmented with 
> some test coverage. If you want it without 7 then just ignore the test 
> changes ... or take Stéphane's version.

Standard operating procedure applies: nothing is happening here without
a resend, because everything else has long since been deleted from my
inbox.

Patch

diff --git a/mercurial/url.py b/mercurial/url.py
--- a/mercurial/url.py
+++ b/mercurial/url.py
@@ -8,6 +8,7 @@ 
 # GNU General Public License version 2 or any later version.
 
 import urllib, urllib2, httplib, os, socket, cStringIO
+import base64
 from i18n import _
 import keepalive, util, sslutil
 import httpconnection as httpconnectionmod
@@ -422,9 +423,22 @@  class httpdigestauthhandler(urllib2.HTTP
 
 class httpbasicauthhandler(urllib2.HTTPBasicAuthHandler):
     def __init__(self, *args, **kwargs):
+        self.auth = None
         urllib2.HTTPBasicAuthHandler.__init__(self, *args, **kwargs)
         self.retried_req = None
 
+    def http_request(self, request):
+        if self.auth:
+            request.add_unredirected_header(self.auth_header, self.auth)
+
+        return request
+
+    def https_request(self, request):
+        if self.auth:
+            request.add_unredirected_header(self.auth_header, self.auth)
+
+        return request
+
     def reset_retry_count(self):
         # Python 2.6.5 will call this on 401 or 407 errors and thus loop
         # forever. We disable reset_retry_count completely and reset in
@@ -439,6 +453,19 @@  class httpbasicauthhandler(urllib2.HTTPB
         return urllib2.HTTPBasicAuthHandler.http_error_auth_reqed(
                         self, auth_header, host, req, headers)
 
+    def retry_http_basic_auth(self, host, req, realm):
+        user, pw = self.passwd.find_user_password(realm, req.get_full_url())
+        if pw is not None:
+            raw = "%s:%s" % (user, pw)
+            auth = 'Basic %s' % base64.b64encode(raw).strip()
+            if req.headers.get(self.auth_header, None) == auth:
+                return None
+            self.auth = auth
+            req.add_unredirected_header(self.auth_header, auth)
+            return self.parent.open(req)
+        else:
+            return None
+
 handlerfuncs = []
 
 def opener(ui, authinfo=None):
diff --git a/tests/test-http.t b/tests/test-http.t
--- a/tests/test-http.t
+++ b/tests/test-http.t
@@ -226,37 +226,31 @@  test http authentication
   "GET /?cmd=lookup HTTP/1.1" 200 - x-hgarg-1:key=tip
   "GET /?cmd=listkeys HTTP/1.1" 401 - x-hgarg-1:namespace=namespaces
   "GET /?cmd=listkeys HTTP/1.1" 200 - x-hgarg-1:namespace=namespaces
-  "GET /?cmd=listkeys HTTP/1.1" 401 - x-hgarg-1:namespace=bookmarks
   "GET /?cmd=listkeys HTTP/1.1" 200 - x-hgarg-1:namespace=bookmarks
   "GET /?cmd=capabilities HTTP/1.1" 200 -
   "GET /?cmd=lookup HTTP/1.1" 200 - x-hgarg-1:key=tip
   "GET /?cmd=listkeys HTTP/1.1" 401 - x-hgarg-1:namespace=namespaces
   "GET /?cmd=listkeys HTTP/1.1" 200 - x-hgarg-1:namespace=namespaces
-  "GET /?cmd=listkeys HTTP/1.1" 401 - x-hgarg-1:namespace=bookmarks
   "GET /?cmd=listkeys HTTP/1.1" 200 - x-hgarg-1:namespace=bookmarks
   "GET /?cmd=capabilities HTTP/1.1" 200 -
   "GET /?cmd=lookup HTTP/1.1" 200 - x-hgarg-1:key=tip
   "GET /?cmd=listkeys HTTP/1.1" 401 - x-hgarg-1:namespace=namespaces
   "GET /?cmd=listkeys HTTP/1.1" 200 - x-hgarg-1:namespace=namespaces
-  "GET /?cmd=listkeys HTTP/1.1" 401 - x-hgarg-1:namespace=bookmarks
   "GET /?cmd=listkeys HTTP/1.1" 200 - x-hgarg-1:namespace=bookmarks
   "GET /?cmd=capabilities HTTP/1.1" 200 -
   "GET /?cmd=lookup HTTP/1.1" 200 - x-hgarg-1:key=tip
   "GET /?cmd=listkeys HTTP/1.1" 401 - x-hgarg-1:namespace=namespaces
   "GET /?cmd=listkeys HTTP/1.1" 200 - x-hgarg-1:namespace=namespaces
-  "GET /?cmd=listkeys HTTP/1.1" 401 - x-hgarg-1:namespace=bookmarks
   "GET /?cmd=listkeys HTTP/1.1" 200 - x-hgarg-1:namespace=bookmarks
   "GET /?cmd=capabilities HTTP/1.1" 200 -
   "GET /?cmd=lookup HTTP/1.1" 200 - x-hgarg-1:key=tip
   "GET /?cmd=listkeys HTTP/1.1" 401 - x-hgarg-1:namespace=namespaces
   "GET /?cmd=listkeys HTTP/1.1" 200 - x-hgarg-1:namespace=namespaces
-  "GET /?cmd=listkeys HTTP/1.1" 401 - x-hgarg-1:namespace=bookmarks
   "GET /?cmd=listkeys HTTP/1.1" 200 - x-hgarg-1:namespace=bookmarks
   "GET /?cmd=capabilities HTTP/1.1" 200 -
   "GET /?cmd=branchmap HTTP/1.1" 200 -
   "GET /?cmd=stream_out HTTP/1.1" 401 -
   "GET /?cmd=stream_out HTTP/1.1" 200 -
-  "GET /?cmd=listkeys HTTP/1.1" 401 - x-hgarg-1:namespace=bookmarks
   "GET /?cmd=listkeys HTTP/1.1" 200 - x-hgarg-1:namespace=bookmarks
   "GET /?cmd=capabilities HTTP/1.1" 200 -
   "GET /?cmd=lookup HTTP/1.1" 200 - x-hgarg-1:key=tip
@@ -271,13 +265,9 @@  test http authentication
   "GET /?cmd=branchmap HTTP/1.1" 200 -
   "GET /?cmd=listkeys HTTP/1.1" 401 - x-hgarg-1:namespace=bookmarks
   "GET /?cmd=listkeys HTTP/1.1" 200 - x-hgarg-1:namespace=bookmarks
-  "POST /?cmd=unbundle HTTP/1.1" 401 - x-hgarg-1:heads=686173686564+5eb5abfefeea63c80dd7553bcc3783f37e0c5524
   "POST /?cmd=unbundle HTTP/1.1" 200 - x-hgarg-1:heads=686173686564+5eb5abfefeea63c80dd7553bcc3783f37e0c5524
-  "GET /?cmd=listkeys HTTP/1.1" 401 - x-hgarg-1:namespace=phases
   "GET /?cmd=listkeys HTTP/1.1" 200 - x-hgarg-1:namespace=phases
-  "POST /?cmd=pushkey HTTP/1.1" 401 - x-hgarg-1:key=7f4e523d01f2cc3765ac8934da3d14db775ff872&namespace=phases&new=0&old=1
   "POST /?cmd=pushkey HTTP/1.1" 200 - x-hgarg-1:key=7f4e523d01f2cc3765ac8934da3d14db775ff872&namespace=phases&new=0&old=1
-  "GET /?cmd=listkeys HTTP/1.1" 401 - x-hgarg-1:namespace=bookmarks
   "GET /?cmd=listkeys HTTP/1.1" 200 - x-hgarg-1:namespace=bookmarks
 
 #endif