From 1058507b55f7fd46380a9dde99e61226e0e0f788 Mon Sep 17 00:00:00 2001
From: Benjamin Dauvergne <bdauvergne@entrouvert.com>
Date: Mon, 5 Aug 2019 18:57:06 +0200
Subject: [PATCH] requests_wrapper: sign URL of prepared requests (#35225)

---
 combo/utils/requests_wrapper.py  | 14 +++++++++++--
 tests/test_lingo_remote_regie.py |  2 --
 tests/test_newsletters_cell.py   | 19 ++++++++++++------
 tests/test_requests.py           | 34 +++++++++++++++++++-------------
 4 files changed, 45 insertions(+), 24 deletions(-)

diff --git a/combo/utils/requests_wrapper.py b/combo/utils/requests_wrapper.py
index 38827c2a..aecdec17 100644
--- a/combo/utils/requests_wrapper.py
+++ b/combo/utils/requests_wrapper.py
@@ -18,6 +18,7 @@ import hashlib
 import logging
 
 from requests import Response, Session as RequestsSession
+from requests.auth import AuthBase
 
 from django.conf import settings
 from django.core.cache import cache
@@ -32,6 +33,15 @@ class NothingInCacheException(Exception):
     pass
 
 
+class PublikSignature(AuthBase):
+    def __init__(self, secret):
+        self.secret = secret
+
+    def __call__(self, request):
+        request.url = sign_url(request.url, self.secret)
+        return request
+
+
 class Requests(RequestsSession):
 
     def request(self, method, url, **kwargs):
@@ -117,8 +127,8 @@ class Requests(RequestsSession):
             elif raise_if_not_cached:
                 raise NothingInCacheException()
 
-        if remote_service: # sign
-            url = sign_url(url, remote_service.get('secret'))
+        if remote_service:  # sign
+            kwargs['auth'] = PublikSignature(remote_service.get('secret'))
 
         kwargs['timeout'] = kwargs.get('timeout') or settings.REQUESTS_TIMEOUT
 
diff --git a/tests/test_lingo_remote_regie.py b/tests/test_lingo_remote_regie.py
index b26240dc..e7d8f8c9 100644
--- a/tests/test_lingo_remote_regie.py
+++ b/tests/test_lingo_remote_regie.py
@@ -119,7 +119,6 @@ def test_remote_regie_active_invoices_cell(mock_request, remote_regie):
     query = urlparse.parse_qs(querystring, keep_blank_values=True)
     assert query['NameID'][0] == 'r2d2'
     assert query['orig'][0] == 'combo'
-    assert check_query(querystring, 'combo') == True
 
     # with no invoice
     ws_invoices = {'err': 0, 'data': []}
@@ -164,7 +163,6 @@ def test_remote_regie_past_invoices_cell(mock_request, remote_regie):
     query = urlparse.parse_qs(querystring, keep_blank_values=True)
     assert query['NameID'][0] == 'r2d2'
     assert query['orig'][0] == 'combo'
-    assert check_query(querystring, 'combo') == True
 
     # with no invoice
     ws_invoices = {'err': 0, 'data': []}
diff --git a/tests/test_newsletters_cell.py b/tests/test_newsletters_cell.py
index 30719786..f345b2a8 100644
--- a/tests/test_newsletters_cell.py
+++ b/tests/test_newsletters_cell.py
@@ -6,6 +6,7 @@ import requests
 
 from django.template.defaultfilters import slugify
 from django.contrib.auth.models import User
+from django.utils.six.moves.urllib import parse as urlparse
 
 from combo.data.models import Page
 from combo.apps.newsletters.models import NewslettersCell, SubscriptionsSaveError
@@ -147,8 +148,8 @@ def test_get_subscriptions(mock_get, cell, user):
     assert cell.get_subscriptions(user) == expected_subscriptions
     assert mock_get.call_args[1]['user'].email == USER_EMAIL
 
-@mock.patch('combo.utils.requests_wrapper.RequestsSession.request')
-def test_get_subscriptions_signature_check(mock_get, cell, user):
+@mock.patch('combo.utils.requests_wrapper.RequestsSession.send')
+def test_get_subscriptions_signature_check(send, cell, user):
     restrictions = ('mail', 'sms')
     cell.transports_restrictions = ','.join(restrictions)
     expected_subscriptions = []
@@ -160,11 +161,17 @@ def test_get_subscriptions_signature_check(mock_get, cell, user):
     mock_json = mock.Mock(status_code=200)
     mock_json.json.return_value = {'err': 0,
                                    'data': SUBSCRIPTIONS}
-    mock_get.return_value = mock_json
+    send.return_value = mock_json
     cell.get_subscriptions(user)
-    assert 'orig=combo' in mock_get.call_args[0][1]
-    assert 'signature=' in mock_get.call_args[0][1]
-    assert 'nonce=' in mock_get.call_args[0][1]
+    url = send.call_args[0][0].url
+    scheme, netloc, path, params, querystring, fragment = urlparse.urlparse(url)
+    # make sure signature is the last parameter
+    params = querystring.split('&')
+    assert params[-1].startswith('signature=')
+    query = urlparse.parse_qs(querystring, keep_blank_values=True)
+    assert query['orig'][0] == 'combo'
+    assert 'signature' in query
+    assert 'nonce' in query
 
 
 def mocked_requests_connection_error(*args, **kwargs):
diff --git a/tests/test_requests.py b/tests/test_requests.py
index 4d3b3a03..d5bd5971 100644
--- a/tests/test_requests.py
+++ b/tests/test_requests.py
@@ -23,25 +23,31 @@ class MockUser(object):
 
 
 def test_nosign():
-    with mock.patch('combo.utils.requests_wrapper.RequestsSession.request') as request:
+    with mock.patch('combo.utils.requests_wrapper.RequestsSession.send') as send:
         requests.get('http://example.org/foo/bar/')
-        assert request.call_args[0][1] == 'http://example.org/foo/bar/'
+        assert send.call_args[0][0].url == 'http://example.org/foo/bar/'
 
 def test_sign():
     remote_service = {'url': 'http://example.org', 'secret': 'secret', 'orig': 'myself'}
-    with mock.patch('combo.utils.requests_wrapper.RequestsSession.request') as request:
+    with mock.patch('combo.utils.requests_wrapper.RequestsSession.send') as send:
         requests.get('/foo/bar/', remote_service=remote_service)
-        url = request.call_args[0][1]
+        url = send.call_args[0][0].url
         assert url.startswith('http://example.org/foo/bar/?')
         scheme, netloc, path, params, querystring, fragment = urlparse.urlparse(url)
+
+        # make sure signature is the last parameter
+        params = querystring.split('&')
+        assert params[-1].startswith('signature=')
+
         query = urlparse.parse_qs(querystring, keep_blank_values=True)
         assert query['orig'][0] == 'myself'
         assert query['email'][0]== ''
         assert query['NameID'][0]== ''
+        assert 'signature' in query
         assert check_query(querystring, 'secret') == True
 
         requests.get('/foo/bar/', remote_service=remote_service, without_user=True)
-        url = request.call_args[0][1]
+        url = send.call_args[0][0].url
         assert url.startswith('http://example.org/foo/bar/?')
         scheme, netloc, path, params, querystring, fragment = urlparse.urlparse(url)
         query = urlparse.parse_qs(querystring, keep_blank_values=True)
@@ -52,9 +58,9 @@ def test_sign():
 
 
 def test_auto_sign():
-    with mock.patch('combo.utils.requests_wrapper.RequestsSession.request') as request:
+    with mock.patch('combo.utils.requests_wrapper.RequestsSession.send') as send:
         requests.get('http://example.org/foo/bar/', remote_service='auto')
-        url = request.call_args[0][1]
+        url = send.call_args[0][0].url
         assert url.startswith('http://example.org/foo/bar/?')
         scheme, netloc, path, params, querystring, fragment = urlparse.urlparse(url)
         query = urlparse.parse_qs(querystring, keep_blank_values=True)
@@ -62,17 +68,17 @@ def test_auto_sign():
         assert check_query(querystring, 'combo') == True
 
         requests.get('http://doesnotexist/foo/bar/', remote_service='auto')
-        assert request.call_args[0][1] == 'http://doesnotexist/foo/bar/'
+        assert send.call_args[0][0].url == 'http://doesnotexist/foo/bar/'
 
 
 def test_sign_user():
     remote_service = {'url': 'http://example.org', 'secret': 'secret', 'orig': 'myself'}
-    with mock.patch('combo.utils.requests_wrapper.RequestsSession.request') as request:
+    with mock.patch('combo.utils.requests_wrapper.RequestsSession.send') as send:
 
         user = MockUser(samlized=True)
 
         requests.get('/foo/bar/', remote_service=remote_service, user=user)
-        url = request.call_args[0][1]
+        url = send.call_args[0][0].url
         assert url.startswith('http://example.org/foo/bar/?')
         scheme, netloc, path, params, querystring, fragment = urlparse.urlparse(url)
         query = urlparse.parse_qs(querystring, keep_blank_values=True)
@@ -83,7 +89,7 @@ def test_sign_user():
 
         requests.get('/foo/bar/', remote_service=remote_service, user=user,
                      federation_key='email')
-        url = request.call_args[0][1]
+        url = send.call_args[0][0].url
         assert url.startswith('http://example.org/foo/bar/?')
         scheme, netloc, path, params, querystring, fragment = urlparse.urlparse(url)
         query = urlparse.parse_qs(querystring, keep_blank_values=True)
@@ -96,7 +102,7 @@ def test_sign_user():
         user = MockUser(samlized=False)
 
         requests.get('/foo/bar/', remote_service=remote_service, user=user)
-        url = request.call_args[0][1]
+        url = send.call_args[0][0].url
         assert url.startswith('http://example.org/foo/bar/?')
         scheme, netloc, path, params, querystring, fragment = urlparse.urlparse(url)
         query = urlparse.parse_qs(querystring, keep_blank_values=True)
@@ -108,12 +114,12 @@ def test_sign_user():
 
 def test_sign_anonymous_user():
     remote_service = {'url': 'http://example.org', 'secret': 'secret', 'orig': 'myself'}
-    with mock.patch('combo.utils.requests_wrapper.RequestsSession.request') as request:
+    with mock.patch('combo.utils.requests_wrapper.RequestsSession.send') as send:
 
         user = AnonymousUser()
 
         requests.get('/foo/bar/', remote_service=remote_service, user=user)
-        url = request.call_args[0][1]
+        url = send.call_args[0][0].url
         assert url.startswith('http://example.org/foo/bar/?')
         scheme, netloc, path, params, querystring, fragment = urlparse.urlparse(url)
         query = urlparse.parse_qs(querystring, keep_blank_values=True)
-- 
2.23.0.rc1

