Repository: twisted/treq

Issue:
add_auth() agent doesn't implement IAgent
The agent returned by `treq.auth.add_auth()` doesn't provide `twisted.web.iweb.IAgent`. The same goes for `treq.auth.add_basic_auth()`.

Discussion:

Patch:
diff --git a/changelog.d/268.feature.rst b/changelog.d/268.feature.rst
new file mode 100644
index 0000000..0d7ae85
--- /dev/null
+++ b/changelog.d/268.feature.rst
@@ -0,0 +1,1 @@
+The *auth* parameter now accepts arbitrary text and `bytes` for usernames and passwords. Text is encoded as UTF-8, per :rfc:`7617`. Previously only ASCII was allowed.
diff --git a/changelog.d/312.bugfix.rst b/changelog.d/312.bugfix.rst
new file mode 100644
index 0000000..59d8336
--- /dev/null
+++ b/changelog.d/312.bugfix.rst
@@ -0,0 +1,1 @@
+The agent returned by :func:`treq.auth.add_auth()` and :func:`treq.auth.add_basic_auth()` is now marked to provide :class:`twisted.web.iweb.IAgent`.
diff --git a/changelog.d/313.doc.rst b/changelog.d/313.doc.rst
new file mode 100644
index 0000000..cc6b0b8
--- /dev/null
+++ b/changelog.d/313.doc.rst
@@ -0,0 +1,1 @@
+The :mod:`treq.auth` module has been documented.
diff --git a/changelog.d/314.bugfix.rst b/changelog.d/314.bugfix.rst
new file mode 100644
index 0000000..c5e1f3b
--- /dev/null
+++ b/changelog.d/314.bugfix.rst
@@ -0,0 +1,1 @@
+treq request APIs no longer mutates a :class:`http_headers.Headers <twisted.web.http_headers.Headers>` passed as the *headers* parameter when the *auth* parameter is also passed.
diff --git a/docs/api.rst b/docs/api.rst
index af968ce..b01ee01 100644
--- a/docs/api.rst
+++ b/docs/api.rst
@@ -78,6 +78,16 @@ Augmented Response Objects
 
         See :meth:`IResponse.setPreviousResponse() <twisted.web.iweb.IResponse.setPreviousResponse>`
 
+Authentication
+--------------
+
+.. module:: treq.auth
+
+.. autofunction:: add_auth
+
+.. autofunction:: add_basic_auth
+
+.. autoexception:: UnknownAuthConfig
 
 Test Helpers
 ------------
diff --git a/src/treq/_agentspy.py b/src/treq/_agentspy.py
new file mode 100644
index 0000000..52fc3f5
--- /dev/null
+++ b/src/treq/_agentspy.py
@@ -0,0 +1,92 @@
+# Copyright (c) The treq Authors.
+# See LICENSE for details.
+from typing import Callable, List, Optional, Tuple
+
+import attr
+from twisted.internet.defer import Deferred
+from twisted.web.http_headers import Headers
+from twisted.web.iweb import IAgent, IBodyProducer, IResponse
+from zope.interface import implementer
+
+
+@attr.s(frozen=True, order=False, slots=True)
+class RequestRecord(object):
+    """
+    The details of a call to :meth:`_AgentSpy.request`
+
+    :ivar method: The *method* argument to :meth:`IAgent.request`
+    :ivar uri: The *uri* argument to :meth:`IAgent.request`
+    :ivar headers: The *headers* argument to :meth:`IAgent.request`
+    :ivar bodyProducer: The *bodyProducer* argument to :meth:`IAgent.request`
+    :ivar deferred: The :class:`Deferred` returned by :meth:`IAgent.request`
+    """
+
+    method = attr.ib()  # type: bytes
+    uri = attr.ib()  # type: bytes
+    headers = attr.ib()  # type: Optional[Headers]
+    bodyProducer = attr.ib()  # type: Optional[IBodyProducer]
+    deferred = attr.ib()  # type: Deferred
+
+
+@implementer(IAgent)
+@attr.s
+class _AgentSpy(object):
+    """
+    An agent that records HTTP requests
+
+    :ivar _callback:
+        A function called with each :class:`RequestRecord`
+    """
+
+    _callback = attr.ib()  # type: Callable[Tuple[RequestRecord], None]
+
+    def request(self, method, uri, headers=None, bodyProducer=None):
+        # type: (bytes, bytes, Optional[Headers], Optional[IBodyProducer]) -> Deferred[IResponse]  # noqa
+        if not isinstance(method, bytes):
+            raise TypeError(
+                "method must be bytes, not {!r} of type {}".format(method, type(method))
+            )
+        if not isinstance(uri, bytes):
+            raise TypeError(
+                "uri must be bytes, not {!r} of type {}".format(uri, type(uri))
+            )
+        if headers is not None and not isinstance(headers, Headers):
+            raise TypeError(
+                "headers must be {}, not {!r} of type {}".format(
+                    type(Headers), headers, type(headers)
+                )
+            )
+        if bodyProducer is not None and not IBodyProducer.providedBy(bodyProducer):
+            raise TypeError(
+                (
+                    "bodyProducer must implement IBodyProducer, but {!r} does not."
+                    " Is the implementation marked with @implementer(IBodyProducer)?"
+                ).format(bodyProducer)
+            )
+        d = Deferred()
+        record = RequestRecord(method, uri, headers, bodyProducer, d)
+        self._callback(record)
+        return d
+
+
+def agent_spy():
+    # type: () -> Tuple[IAgent, List[RequestRecord]]
+    """
+    Record HTTP requests made with an agent
+
+    This is suitable for low-level testing of wrapper agents. It validates
+    the parameters of each call to :meth:`IAgent.request` (synchronously
+    raising :exc:`TypeError`) and captures them as a :class:`RequestRecord`,
+    which can then be used to inspect the request or generate a response by
+    firing the :attr:`~RequestRecord.deferred`.
+
+    :returns:
+        A two-tuple of:
+
+         - An :class:`twisted.web.iweb.IAgent`
+         - A list of calls made to the agent's
+           :meth:`~twisted.web.iweb.IAgent.request()` method
+    """
+    records = []
+    agent = _AgentSpy(records.append)
+    return agent, records
diff --git a/src/treq/api.py b/src/treq/api.py
index 9439d41..f1eec5b 100644
--- a/src/treq/api.py
+++ b/src/treq/api.py
@@ -91,7 +91,8 @@ def request(method, url, **kwargs):
     :param bool persistent: Use persistent HTTP connections.  Default: ``True``
     :param bool allow_redirects: Follow HTTP redirects.  Default: ``True``
 
-    :param auth: HTTP Basic Authentication information.
+    :param auth: HTTP Basic Authentication information --- see
+        :func:`treq.auth.add_auth`.
     :type auth: tuple of ``('username', 'password')``.
 
     :param cookies: Cookies to send with this request.  The HTTP kind, not the
diff --git a/src/treq/auth.py b/src/treq/auth.py
index d693ff3..fad3df6 100644
--- a/src/treq/auth.py
+++ b/src/treq/auth.py
@@ -1,40 +1,97 @@
+# Copyright 2012-2020 The treq Authors.
+# See LICENSE for details.
 from __future__ import absolute_import, division, print_function
 
+import binascii
+from typing import Union
+
 from twisted.web.http_headers import Headers
-import base64
+from twisted.web.iweb import IAgent
+from zope.interface import implementer
 
 
 class UnknownAuthConfig(Exception):
+    """
+    The authentication config provided couldn't be interpreted.
+    """
     def __init__(self, config):
         super(Exception, self).__init__(
             '{0!r} not of a known type.'.format(config))
 
 
-class _RequestHeaderSettingAgent(object):
-    def __init__(self, agent, request_headers):
+@implementer(IAgent)
+class _RequestHeaderSetterAgent(object):
+    """
+    Wrap an agent to set request headers
+
+    :ivar _agent: The wrapped agent.
+
+    :ivar _request_headers:
+        Headers to set on each request before forwarding it to the wrapped
+        agent.
+    """
+    def __init__(self, agent, headers):
         self._agent = agent
-        self._request_headers = request_headers
+        self._headers = headers
 
     def request(self, method, uri, headers=None, bodyProducer=None):
         if headers is None:
-            headers = self._request_headers
+            requestHeaders = self._headers
         else:
-            for header, values in self._request_headers.getAllRawHeaders():
-                headers.setRawHeaders(header, values)
+            requestHeaders = headers.copy()
+            for header, values in self._headers.getAllRawHeaders():
+                requestHeaders.setRawHeaders(header, values)
 
         return self._agent.request(
-            method, uri, headers=headers, bodyProducer=bodyProducer)
+            method, uri, headers=requestHeaders, bodyProducer=bodyProducer)
 
 
 def add_basic_auth(agent, username, password):
-    creds = base64.b64encode(
-        '{0}:{1}'.format(username, password).encode('ascii'))
-    return _RequestHeaderSettingAgent(
+    # type: (IAgent, Union[str, bytes], Union[str, bytes]) -> IAgent
+    """
+    Wrap an agent to add HTTP basic authentication
+
+    The returned agent sets the *Authorization* request header according to the
+    basic authentication scheme described in :rfc:`7617`. This header contains
+    the given *username* and *password* in plaintext, and thus should only be
+    used over an encrypted transport (HTTPS).
+
+    Note that the colon (``:``) is used as a delimiter between the *username*
+    and *password*, so if either parameter includes a colon the interpretation
+    of the *Authorization* header is server-defined.
+
+    :param agent: Agent to wrap.
+    :param username: The username.
+    :param password: The password.
+
+    :returns: :class:`~twisted.web.iweb.IAgent`
+    """
+    if not isinstance(username, bytes):
+        username = username.encode('utf-8')
+    if not isinstance(password, bytes):
+        password = password.encode('utf-8')
+
+    creds = binascii.b2a_base64(b'%s:%s' % (username, password)).rstrip(b'\n')
+    return _RequestHeaderSetterAgent(
         agent,
-        Headers({b'Authorization': [b'Basic ' + creds]}))
+        Headers({b'Authorization': [b'Basic ' + creds]}),
+    )
 
 
 def add_auth(agent, auth_config):
+    """
+    Wrap an agent to perform authentication
+
+    :param agent: Agent to wrap.
+
+    :param auth_config:
+        A ``('username', 'password')`` tuple --- see :func:`add_basic_auth`.
+
+    :returns: :class:`~twisted.web.iweb.IAgent`
+
+    :raises UnknownAuthConfig:
+        When the format *auth_config* isn't supported.
+    """
     if isinstance(auth_config, tuple):
         return add_basic_auth(agent, auth_config[0], auth_config[1])
 


Test patch:
diff --git a/src/treq/test/test_agentspy.py b/src/treq/test/test_agentspy.py
new file mode 100644
index 0000000..6103f6a
--- /dev/null
+++ b/src/treq/test/test_agentspy.py
@@ -0,0 +1,79 @@
+# Copyright (c) The treq Authors.
+# See LICENSE for details.
+from io import BytesIO
+
+from twisted.trial.unittest import SynchronousTestCase
+from twisted.web.client import FileBodyProducer
+from twisted.web.http_headers import Headers
+from twisted.web.iweb import IAgent
+
+from treq._agentspy import RequestRecord, agent_spy
+
+
+class APISpyTests(SynchronousTestCase):
+    """
+    The agent_spy API provides an agent that records each request made to it.
+    """
+
+    def test_provides_iagent(self):
+        """
+        The agent returned by agent_spy() provides the IAgent interface.
+        """
+        agent, _ = agent_spy()
+
+        self.assertTrue(IAgent.providedBy(agent))
+
+    def test_records(self):
+        """
+        Each request made with the agent is recorded.
+        """
+        agent, requests = agent_spy()
+
+        body = FileBodyProducer(BytesIO(b"..."))
+        d1 = agent.request(b"GET", b"https://foo")
+        d2 = agent.request(b"POST", b"http://bar", Headers({}))
+        d3 = agent.request(b"PUT", b"https://baz", None, bodyProducer=body)
+
+        self.assertEqual(
+            requests,
+            [
+                RequestRecord(b"GET", b"https://foo", None, None, d1),
+                RequestRecord(b"POST", b"http://bar", Headers({}), None, d2),
+                RequestRecord(b"PUT", b"https://baz", None, body, d3),
+            ],
+        )
+
+    def test_record_attributes(self):
+        """
+        Each parameter passed to `request` is available as an attribute of the
+        RequestRecord. Additionally, the deferred returned by the call is
+        available.
+        """
+        agent, requests = agent_spy()
+        headers = Headers()
+        body = FileBodyProducer(BytesIO(b"..."))
+
+        deferred = agent.request(b"method", b"uri", headers=headers, bodyProducer=body)
+
+        [rr] = requests
+        self.assertIs(rr.method, b"method")
+        self.assertIs(rr.uri, b"uri")
+        self.assertIs(rr.headers, headers)
+        self.assertIs(rr.bodyProducer, body)
+        self.assertIs(rr.deferred, deferred)
+
+    def test_type_validation(self):
+        """
+        The request method enforces correctness by raising TypeError when
+        passed parameters of the wrong type.
+        """
+        agent, _ = agent_spy()
+
+        self.assertRaises(TypeError, agent.request, u"method not bytes", b"uri")
+        self.assertRaises(TypeError, agent.request, b"method", u"uri not bytes")
+        self.assertRaises(
+            TypeError, agent.request, b"method", b"uri", {"not": "headers"}
+        )
+        self.assertRaises(
+            TypeError, agent.request, b"method", b"uri", None, b"not ibodyproducer"
+        )
diff --git a/src/treq/test/test_auth.py b/src/treq/test/test_auth.py
index 49afc15..4ba169d 100644
--- a/src/treq/test/test_auth.py
+++ b/src/treq/test/test_auth.py
@@ -1,75 +1,141 @@
-import mock
-
-from twisted.trial.unittest import TestCase
-from twisted.web.client import Agent
+# Copyright (c) The treq Authors.
+# See LICENSE for details.
+from twisted.trial.unittest import SynchronousTestCase
 from twisted.web.http_headers import Headers
+from twisted.web.iweb import IAgent
 
-from treq.auth import _RequestHeaderSettingAgent, add_auth, UnknownAuthConfig
+from treq._agentspy import agent_spy
+from treq.auth import _RequestHeaderSetterAgent, add_auth, UnknownAuthConfig
 
 
-class RequestHeaderSettingAgentTests(TestCase):
+class RequestHeaderSetterAgentTests(SynchronousTestCase):
     def setUp(self):
-        self.agent = mock.Mock(Agent)
+        self.agent, self.requests = agent_spy()
 
     def test_sets_headers(self):
-        agent = _RequestHeaderSettingAgent(
+        agent = _RequestHeaderSetterAgent(
             self.agent,
-            Headers({b'X-Test-Header': [b'Test-Header-Value']}))
+            Headers({b'X-Test-Header': [b'Test-Header-Value']}),
+        )
 
-        agent.request('method', 'uri')
+        agent.request(b'method', b'uri')
 
-        self.agent.request.assert_called_once_with(
-            'method', 'uri',
-            headers=Headers({b'X-Test-Header': [b'Test-Header-Value']}),
-            bodyProducer=None
+        self.assertEqual(
+            self.requests[0].headers,
+            Headers({b'X-Test-Header': [b'Test-Header-Value']}),
         )
 
     def test_overrides_per_request_headers(self):
-        agent = _RequestHeaderSettingAgent(
+        agent = _RequestHeaderSetterAgent(
             self.agent,
             Headers({b'X-Test-Header': [b'Test-Header-Value']})
         )
 
         agent.request(
-            'method', 'uri',
+            b'method', b'uri',
             Headers({b'X-Test-Header': [b'Unwanted-Value']})
         )
 
-        self.agent.request.assert_called_once_with(
-            'method', 'uri',
-            headers=Headers({b'X-Test-Header': [b'Test-Header-Value']}),
-            bodyProducer=None
+        self.assertEqual(
+            self.requests[0].headers,
+            Headers({b'X-Test-Header': [b'Test-Header-Value']}),
         )
 
+    def test_no_mutation(self):
+        """
+        The agent never mutates the headers passed to its request method.
 
-class AddAuthTests(TestCase):
-    def setUp(self):
-        self.rhsa_patcher = mock.patch('treq.auth._RequestHeaderSettingAgent')
-        self._RequestHeaderSettingAgent = self.rhsa_patcher.start()
-        self.addCleanup(self.rhsa_patcher.stop)
+        This reproduces https://github.com/twisted/treq/issues/314
+        """
+        requestHeaders = Headers({})
+        agent = _RequestHeaderSetterAgent(
+            self.agent,
+            Headers({b'Added': [b'1']}),
+        )
 
-    def test_add_basic_auth(self):
-        agent = mock.Mock()
+        agent.request(b'method', b'uri', headers=requestHeaders)
+
+        self.assertEqual(requestHeaders, Headers({}))
 
-        add_auth(agent, ('username', 'password'))
 
-        self._RequestHeaderSettingAgent.assert_called_once_with(
-            agent,
+class AddAuthTests(SynchronousTestCase):
+    def test_add_basic_auth(self):
+        """
+        add_auth() wraps the given agent with one that adds an ``Authorization:
+        Basic ...`` HTTP header that contains the given credentials.
+        """
+        agent, requests = agent_spy()
+        authAgent = add_auth(agent, ('username', 'password'))
+
+        authAgent.request(b'method', b'uri')
+
+        self.assertTrue(IAgent.providedBy(authAgent))
+        self.assertEqual(
+            requests[0].headers,
             Headers({b'authorization': [b'Basic dXNlcm5hbWU6cGFzc3dvcmQ=']})
         )
 
     def test_add_basic_auth_huge(self):
-        agent = mock.Mock()
+        """
+        The Authorization header doesn't include linebreaks, even if the
+        credentials are so long that Python's base64 implementation inserts
+        them.
+        """
+        agent, requests = agent_spy()
         pwd = ('verylongpasswordthatextendsbeyondthepointwheremultiplel'
                'inesaregenerated')
-        auth = (b'Basic dXNlcm5hbWU6dmVyeWxvbmdwYXNzd29yZHRoYXRleHRlbmRzY'
-                b'mV5b25kdGhlcG9pbnR3aGVyZW11bHRpcGxlbGluZXNhcmVnZW5lcmF0ZWQ=')
-        add_auth(agent, ('username', pwd))
+        expectedAuth = (
+            b'Basic dXNlcm5hbWU6dmVyeWxvbmdwYXNzd29yZHRoYXRleHRlbmRzY'
+            b'mV5b25kdGhlcG9pbnR3aGVyZW11bHRpcGxlbGluZXNhcmVnZW5lcmF0ZWQ='
+        )
+        authAgent = add_auth(agent, ('username', pwd))
+
+        authAgent.request(b'method', b'uri')
 
-        self._RequestHeaderSettingAgent.assert_called_once_with(
-            agent,
-            Headers({b'authorization': [auth]}))
+        self.assertEqual(
+            requests[0].headers,
+            Headers({b'authorization': [expectedAuth]}),
+        )
+
+    def test_add_basic_auth_utf8(self):
+        """
+        Basic auth username and passwords given as `str` are encoded as UTF-8.
+
+        https://developer.mozilla.org/en-US/docs/Web/HTTP/Authentication#Character_encoding_of_HTTP_authentication
+        """
+        agent, requests = agent_spy()
+        auth = (u'\u16d7', u'\u16b9')
+        authAgent = add_auth(agent, auth)
+
+        authAgent.request(b'method', b'uri')
+
+        self.assertEqual(
+            requests[0].headers,
+            Headers({b'Authorization': [b'Basic 4ZuXOuGauQ==']}),
+        )
+
+    def test_add_basic_auth_bytes(self):
+        """
+        Basic auth can be passed as `bytes`, allowing the user full control
+        over the encoding.
+        """
+        agent, requests = agent_spy()
+        auth = (b'\x01\x0f\xff', b'\xff\xf0\x01')
+        authAgent = add_auth(agent, auth)
+
+        authAgent.request(b'method', b'uri')
+
+        self.assertEqual(
+            requests[0].headers,
+            Headers({b'Authorization': [b'Basic AQ//Ov/wAQ==']}),
+        )
 
     def test_add_unknown_auth(self):
-        agent = mock.Mock()
-        self.assertRaises(UnknownAuthConfig, add_auth, agent, mock.Mock())
+        """
+        add_auth() raises UnknownAuthConfig when given anything other than
+        a tuple.
+        """
+        agent, _ = agent_spy()
+        invalidAuth = 1234
+
+        self.assertRaises(UnknownAuthConfig, add_auth, agent, invalidAuth)