Repository: scrapy/scrapy
Pull request #1637 (2015-12-11 04:08:13)

[MRG+1] py3 fix HttpProxy and Retry Middlewares

codecov-io: ## [Current coverage][1] is `82.93%`
> Merging **#1637** into **master** will not affect coverage as of [`68d2a28`][3]



[1]: https://codecov.io/github/scrapy/scrapy?ref=68d2a28498ff3785202daadb77b0846585023ddd
[2]: https://codecov.io/github/scrapy/scrapy/features/suggestions?ref=68d2a28498ff3785202daadb77b0846585023ddd
[3]: https://codecov.io/github/scrapy/scrapy/commit/68d2a28498ff3785202daadb77b0846585023ddd
[4]: https://codecov.io/github/scrapy/scrapy/compare/b5285c3f38b6bd36a205fada3f0909b5d7f8f1e0...68d2a28498ff3785202daadb77b0846585023ddd

> Powered by [Codecov](https://codecov.io). Updated on successful CI builds.
eLRuLL: now encoding could be updated from settings, something to take into account:

    > to_bytes('%s:%s' % (unquote(u'm\u00E1n'), unquote(u'pass')))
    > 'm\xc3\xa1n:pass' # mán:pass
   
    > to_bytes('%s:%s' % (unquote(u'm\u00E1n'), unquote(u'pass')), encoding='latin-1')
    > 'm\xe1n:pass' # m?n:pass
    
kmike: Hey @eLRuLL,

I think that's a good idea to make encoding configurable, but a global setting doesn't look right - what if there are different proxies (e.g. rotating) which use different encodings? 
kmike: What about allowing users to pass bytes as username and password? This way users will be able to use any encoding. Latin1 can be kept default for unicode input.
eLRuLL: >Hey @eLRuLL,
>
>I think that's a good idea to make encoding configurable, but a global setting doesn't look right - what if there are different proxies (e.g. rotating) which use different encodings?

The middleware isn't accepting that kind of behaviour I think, as it is not processing the request if `proxy` already on `meta`: https://github.com/scrapy/scrapy/blob/master/scrapy/downloadermiddlewares/httpproxy.py#L38

> What about allowing users to pass bytes as username and password? This way users will be able to use any encoding. Latin1 can be kept default for unicode input.

for this I think I should change the `_parse_proxy` method (because of the bytes patterns).
kmike: @eLRuLL you're right, sorry! The PR looks good; I only have a couple of minor comments.
kmike: @eLRuLL :+1: 
Could you please rebase on master?

Patch:
diff --git a/docs/topics/downloader-middleware.rst b/docs/topics/downloader-middleware.rst
index 3641da231..a97d5a696 100644
--- a/docs/topics/downloader-middleware.rst
+++ b/docs/topics/downloader-middleware.rst
@@ -951,6 +951,18 @@ Default: ``False``
 Whether the AjaxCrawlMiddleware will be enabled. You may want to
 enable it for :ref:`broad crawls <topics-broad-crawls>`.
 
+HttpProxyMiddleware settings
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+.. setting:: HTTPPROXY_AUTH_ENCODING
+
+HTTPPROXY_AUTH_ENCODING
+^^^^^^^^^^^^^^^^^^^^^^^
+
+Default: ``"latin-1"``
+
+The default encoding for proxy authentication on :class:`HttpProxyMiddleware`.
+
 
 .. _DBM: http://en.wikipedia.org/wiki/Dbm
 .. _anydbm: https://docs.python.org/2/library/anydbm.html
diff --git a/scrapy/downloadermiddlewares/httpproxy.py b/scrapy/downloadermiddlewares/httpproxy.py
index dda6a3d2a..b01bab76d 100644
--- a/scrapy/downloadermiddlewares/httpproxy.py
+++ b/scrapy/downloadermiddlewares/httpproxy.py
@@ -9,11 +9,13 @@ from six.moves.urllib.parse import urlunparse
 
 from scrapy.utils.httpobj import urlparse_cached
 from scrapy.exceptions import NotConfigured
+from scrapy.utils.python import to_bytes
 
 
 class HttpProxyMiddleware(object):
 
-    def __init__(self):
+    def __init__(self, auth_encoding='latin-1'):
+        self.auth_encoding = auth_encoding
         self.proxies = {}
         for type, url in getproxies().items():
             self.proxies[type] = self._get_proxy(url, type)
@@ -21,12 +23,19 @@ class HttpProxyMiddleware(object):
         if not self.proxies:
             raise NotConfigured
 
+    @classmethod
+    def from_crawler(cls, crawler):
+        auth_encoding = crawler.settings.get('HTTPPROXY_AUTH_ENCODING')
+        return cls(auth_encoding)
+
     def _get_proxy(self, url, orig_type):
         proxy_type, user, password, hostport = _parse_proxy(url)
         proxy_url = urlunparse((proxy_type or orig_type, hostport, '', '', '', ''))
 
         if user:
-            user_pass = '%s:%s' % (unquote(user), unquote(password))
+            user_pass = to_bytes(
+                '%s:%s' % (unquote(user), unquote(password)),
+                encoding=self.auth_encoding)
             creds = base64.b64encode(user_pass).strip()
         else:
             creds = None
@@ -52,4 +61,4 @@ class HttpProxyMiddleware(object):
         creds, proxy = self.proxies[scheme]
         request.meta['proxy'] = proxy
         if creds:
-            request.headers['Proxy-Authorization'] = 'Basic ' + creds
+            request.headers['Proxy-Authorization'] = b'Basic ' + creds
diff --git a/scrapy/settings/default_settings.py b/scrapy/settings/default_settings.py
index b151933b6..44e74dc61 100644
--- a/scrapy/settings/default_settings.py
+++ b/scrapy/settings/default_settings.py
@@ -169,6 +169,8 @@ HTTPCACHE_DBM_MODULE = 'anydbm' if six.PY2 else 'dbm'
 HTTPCACHE_POLICY = 'scrapy.extensions.httpcache.DummyPolicy'
 HTTPCACHE_GZIP = False
 
+HTTPPROXY_AUTH_ENCODING = 'latin-1'
+
 ITEM_PROCESSOR = 'scrapy.pipelines.ItemPipelineManager'
 
 ITEM_PIPELINES = {}


Test patch:
diff --git a/tests/py3-ignores.txt b/tests/py3-ignores.txt
index 8c883ee3c..185a278fb 100644
--- a/tests/py3-ignores.txt
+++ b/tests/py3-ignores.txt
@@ -2,8 +2,6 @@ tests/test_closespider.py
 tests/test_exporters.py
 tests/test_linkextractors_deprecated.py
 tests/test_crawl.py
-tests/test_downloadermiddleware_httpproxy.py
-tests/test_downloadermiddleware_retry.py
 tests/test_mail.py
 tests/test_pipeline_files.py
 tests/test_pipeline_images.py
@@ -23,8 +21,6 @@ scrapy/pipelines/files.py
 scrapy/linkextractors/sgml.py
 scrapy/linkextractors/regex.py
 scrapy/linkextractors/htmlparser.py
-scrapy/downloadermiddlewares/retry.py
-scrapy/downloadermiddlewares/httpproxy.py
 scrapy/downloadermiddlewares/cookies.py
 scrapy/extensions/statsmailer.py
 scrapy/extensions/memusage.py
diff --git a/tests/test_downloadermiddleware_httpproxy.py b/tests/test_downloadermiddleware_httpproxy.py
index 5b9717a89..2b26431a4 100644
--- a/tests/test_downloadermiddleware_httpproxy.py
+++ b/tests/test_downloadermiddleware_httpproxy.py
@@ -9,6 +9,7 @@ from scrapy.spiders import Spider
 
 spider = Spider('foo')
 
+
 class TestDefaultHeadersMiddleware(TestCase):
 
     failureException = AssertionError
@@ -52,7 +53,7 @@ class TestDefaultHeadersMiddleware(TestCase):
         req = Request('http://scrapytest.org')
         assert mw.process_request(req, spider) is None
         self.assertEquals(req.meta, {'proxy': 'https://proxy:3128'})
-        self.assertEquals(req.headers.get('Proxy-Authorization'), 'Basic dXNlcjpwYXNz')
+        self.assertEquals(req.headers.get('Proxy-Authorization'), b'Basic dXNlcjpwYXNz')
 
     def test_proxy_auth_empty_passwd(self):
         os.environ['http_proxy'] = 'https://user:@proxy:3128'
@@ -60,7 +61,23 @@ class TestDefaultHeadersMiddleware(TestCase):
         req = Request('http://scrapytest.org')
         assert mw.process_request(req, spider) is None
         self.assertEquals(req.meta, {'proxy': 'https://proxy:3128'})
-        self.assertEquals(req.headers.get('Proxy-Authorization'), 'Basic dXNlcjo=')
+        self.assertEquals(req.headers.get('Proxy-Authorization'), b'Basic dXNlcjo=')
+
+    def test_proxy_auth_encoding(self):
+        # utf-8 encoding
+        os.environ['http_proxy'] = u'https://m\u00E1n:pass@proxy:3128'
+        mw = HttpProxyMiddleware(auth_encoding='utf-8')
+        req = Request('http://scrapytest.org')
+        assert mw.process_request(req, spider) is None
+        self.assertEquals(req.meta, {'proxy': 'https://proxy:3128'})
+        self.assertEquals(req.headers.get('Proxy-Authorization'), b'Basic bcOhbjpwYXNz')
+
+        # default latin-1 encoding
+        mw = HttpProxyMiddleware(auth_encoding='latin-1')
+        req = Request('http://scrapytest.org')
+        assert mw.process_request(req, spider) is None
+        self.assertEquals(req.meta, {'proxy': 'https://proxy:3128'})
+        self.assertEquals(req.headers.get('Proxy-Authorization'), b'Basic beFuOnBhc3M=')
 
     def test_proxy_already_seted(self):
         os.environ['http_proxy'] = http_proxy = 'https://proxy.for.http:3128'
@@ -69,7 +86,6 @@ class TestDefaultHeadersMiddleware(TestCase):
         assert mw.process_request(req, spider) is None
         assert 'proxy' in req.meta and req.meta['proxy'] is None
 
-
     def test_no_proxy(self):
         os.environ['http_proxy'] = http_proxy = 'https://proxy.for.http:3128'
         mw = HttpProxyMiddleware()
@@ -88,4 +104,3 @@ class TestDefaultHeadersMiddleware(TestCase):
         req = Request('http://noproxy.com')
         assert mw.process_request(req, spider) is None
         assert 'proxy' not in req.meta
-
diff --git a/tests/test_downloadermiddleware_retry.py b/tests/test_downloadermiddleware_retry.py
index 20561e771..3de9399cf 100644
--- a/tests/test_downloadermiddleware_retry.py
+++ b/tests/test_downloadermiddleware_retry.py
@@ -21,20 +21,20 @@ class RetryTest(unittest.TestCase):
 
     def test_priority_adjust(self):
         req = Request('http://www.scrapytest.org/503')
-        rsp = Response('http://www.scrapytest.org/503', body='', status=503)
+        rsp = Response('http://www.scrapytest.org/503', body=b'', status=503)
         req2 = self.mw.process_response(req, rsp, self.spider)
         assert req2.priority < req.priority
 
     def test_404(self):
         req = Request('http://www.scrapytest.org/404')
-        rsp = Response('http://www.scrapytest.org/404', body='', status=404)
+        rsp = Response('http://www.scrapytest.org/404', body=b'', status=404)
 
         # dont retry 404s
         assert self.mw.process_response(req, rsp, self.spider) is rsp
 
     def test_dont_retry(self):
         req = Request('http://www.scrapytest.org/503', meta={'dont_retry': True})
-        rsp = Response('http://www.scrapytest.org/503', body='', status=503)
+        rsp = Response('http://www.scrapytest.org/503', body=b'', status=503)
 
         # first retry
         r = self.mw.process_response(req, rsp, self.spider)
@@ -56,7 +56,7 @@ class RetryTest(unittest.TestCase):
 
     def test_503(self):
         req = Request('http://www.scrapytest.org/503')
-        rsp = Response('http://www.scrapytest.org/503', body='', status=503)
+        rsp = Response('http://www.scrapytest.org/503', body=b'', status=503)
 
         # first retry
         req = self.mw.process_response(req, rsp, self.spider)