From 88aa958d0f781d9d0f321843d6785149d5663272 Mon Sep 17 00:00:00 2001 From: Jason Bittel Date: Wed, 22 Feb 2017 15:15:57 -0800 Subject: [PATCH] Replace gevent with requests-futures to address #30 and #40 --- docs/settings.rst | 13 ++-------- mama_cas/compat.py | 6 ++--- mama_cas/models.py | 49 +++++++++-------------------------- mama_cas/tests/test_models.py | 24 ++++------------- mama_cas/tests/test_views.py | 2 +- 5 files changed, 23 insertions(+), 71 deletions(-) diff --git a/docs/settings.rst b/docs/settings.rst index 5184b8b..30c2418 100644 --- a/docs/settings.rst +++ b/docs/settings.rst @@ -16,15 +16,6 @@ customize behavior and improve security. process. When enabled, an additional checkbox will be displayed on the login form. -.. attribute:: MAMA_CAS_ASYNC_CONCURRENCY - - :default: ``2`` - - If single logout is enabled and `gevent`_ is installed, this setting - limits the concurrency of requests sent for a logout event. If the number - of requests reaches this limit, additional requests block until there is - room. Setting this value to zero disables this limiting. - .. attribute:: MAMA_CAS_ATTRIBUTE_CALLBACKS :default: ``()`` @@ -68,7 +59,7 @@ customize behavior and improve security. .. note:: By default, the single logout requests are sent synchronously. If - `gevent`_ is installed, they are sent asynchronously. + `requests-futures`_ is installed, they are sent asynchronously. .. warning:: @@ -201,4 +192,4 @@ customize behavior and improve security. A path to the warning template to use. Make sure Django can find this template using normal Django template discovery rules. -.. _gevent: http://www.gevent.org/ +.. _requests-futures: https://github.com/ross/requests-futures diff --git a/mama_cas/compat.py b/mama_cas/compat.py index f6324c0..10cc6d6 100644 --- a/mama_cas/compat.py +++ b/mama_cas/compat.py @@ -9,12 +9,12 @@ import xml.etree.ElementTree as etree -# gevent is optional, and allows for asynchronous single logout +# requests-futures is optional, and allows for asynchronous single logout # requests. If it is not present, synchronous requests will be sent. try: - import gevent + from requests_futures.sessions import FuturesSession as Session except ImportError: # pragma: no cover - gevent = None + from requests import Session # defusedxml is optional, and is used for the /samlValidate diff --git a/mama_cas/models.py b/mama_cas/models.py index fad4e9b..dbe16ac 100644 --- a/mama_cas/models.py +++ b/mama_cas/models.py @@ -16,7 +16,7 @@ import requests -from mama_cas.compat import gevent +from mama_cas.compat import Session from mama_cas.exceptions import InvalidProxyCallback from mama_cas.exceptions import InvalidRequest from mama_cas.exceptions import InvalidService @@ -34,11 +34,6 @@ from mama_cas.utils import is_scheme_https from mama_cas.utils import match_service -if gevent: - from gevent.pool import Pool - from gevent import monkey - monkey.patch_all(thread=False, select=False) - logger = logging.getLogger(__name__) @@ -214,26 +209,12 @@ def request_sign_out(self, user): specified user. This is called at logout when single logout is enabled. - If gevent is installed, asynchronous requests will be sent. - Otherwise, synchronous requests will be sent. Setting - ``MAMA_CAS_ASYNC_CONCURRENCY`` limits concurrent requests for - a logout event to the specified value. + If requests-futures is installed, asynchronous requests will + be sent. Otherwise, synchronous requests will be sent. """ - def spawn(ticket, pool=None): - if pool is not None: - return pool.spawn(ticket.request_sign_out) - return gevent.spawn(ticket.request_sign_out) - - tickets = list(self.filter(user=user, consumed__gte=user.last_login)) - - if gevent: - size = getattr(settings, 'MAMA_CAS_ASYNC_CONCURRENCY', 2) - pool = Pool(size) if size else None - sign_out_requests = [spawn(t, pool=pool) for t in tickets] - gevent.joinall(sign_out_requests) - else: - for ticket in tickets: - ticket.request_sign_out() + session = Session() + for ticket in self.filter(user=user, consumed__gte=user.last_login): + ticket.request_sign_out(session=session) class ServiceTicket(Ticket): @@ -263,22 +244,16 @@ def is_primary(self): return True return False - def request_sign_out(self): + def request_sign_out(self, session=requests): """ Send a POST request to the ``ServiceTicket``s logout URL to request sign-out. """ - if not logout_allowed(self.service): - return - request = SingleSignOutRequest(context={'ticket': self}) - url = get_logout_url(self.service) or self.service - try: - resp = requests.post(url, data={'logoutRequest': request.render_content()}) - resp.raise_for_status() - except requests.exceptions.RequestException as e: - logger.warning("Single sign-out request to %s returned %s" % (url, e)) - else: - logger.debug("Single sign-out request sent to %s" % url) + if logout_allowed(self.service): + request = SingleSignOutRequest(context={'ticket': self}) + url = get_logout_url(self.service) or self.service + session.post(url, data={'logoutRequest': request.render_content()}) + logger.info("Single sign-out request sent to %s" % url) class ProxyTicket(Ticket): diff --git a/mama_cas/tests/test_models.py b/mama_cas/tests/test_models.py index 0fd8e02..3d7cb6d 100644 --- a/mama_cas/tests/test_models.py +++ b/mama_cas/tests/test_models.py @@ -274,21 +274,7 @@ def test_request_sign_out(self): """ ServiceTicketFactory(consume=True) ServiceTicketFactory(consume=True) - with patch('requests.post') as mock: - mock.return_value.status_code = 200 - ServiceTicket.objects.request_sign_out(self.user) - self.assertEqual(mock.call_count, 2) - - @override_settings(MAMA_CAS_ASYNC_CONCURRENCY=0) - def test_request_sign_out_no_pool(self): - """ - Calling the ``request_sign_out()`` manager method with - concurrency disabled should issue a POST request for each - consumed ticket for the provided user. - """ - ServiceTicketFactory(consume=True) - ServiceTicketFactory(consume=True) - with patch('requests.post') as mock: + with patch('requests.Session.post') as mock: mock.return_value.status_code = 200 ServiceTicket.objects.request_sign_out(self.user) self.assertEqual(mock.call_count, 2) @@ -328,7 +314,7 @@ def test_request_sign_out(self): cause any side-effects. """ st = ServiceTicketFactory() - with patch('requests.post') as mock: + with patch('requests.Session.post') as mock: mock.return_value.status_code = 200 st.request_sign_out() @@ -338,7 +324,7 @@ def test_request_sign_out_exception(self): it should be handled. """ st = ServiceTicketFactory() - with patch('requests.post') as mock: + with patch('requests.Session.post') as mock: mock.side_effect = requests.exceptions.RequestException st.request_sign_out() @@ -348,7 +334,7 @@ def test_request_sign_out_invalid_status(self): status code, the resulting exception should be handled. """ st = ServiceTicketFactory() - with patch('requests.post') as mock: + with patch('requests.Session.post') as mock: mock.return_value.status_code = 500 st.request_sign_out() @@ -358,7 +344,7 @@ def test_request_sign_out_logout_allow_false(self): request should not be sent. """ st = ServiceTicketFactory(service='http://example.com') - with patch('requests.post') as mock: + with patch('requests.Session.post') as mock: mock.return_value.status_code = 500 st.request_sign_out() self.assertEqual(mock.call_count, 0) diff --git a/mama_cas/tests/test_views.py b/mama_cas/tests/test_views.py index ad732f0..4855f06 100644 --- a/mama_cas/tests/test_views.py +++ b/mama_cas/tests/test_views.py @@ -276,7 +276,7 @@ def test_logout_single_sign_out(self): ServiceTicketFactory() ServiceTicketFactory() self.client.login(**self.user_info) - with patch('requests.post') as mock: + with patch('requests.Session.post') as mock: self.client.get(reverse('cas_logout')) self.assertEqual(mock.call_count, 2)