-
Notifications
You must be signed in to change notification settings - Fork 183
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add an SSLContextAdapter #231
base: master
Are you sure you want to change the base?
Changes from 2 commits
c946346
8e81ef4
32c4f93
538df54
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -50,3 +50,5 @@ Patches and Suggestions | |
|
||
- Achim Herwig <[email protected]> | ||
|
||
- Nikos Graser <[email protected]> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,53 @@ | ||
# -*- coding: utf-8 -*- | ||
"""This file contains an implementation of the SSLContextAdapter. | ||
|
||
It requires a version of requests >= 2.4.0. | ||
""" | ||
|
||
import requests | ||
from requests.adapters import HTTPAdapter | ||
|
||
|
||
class SSLContextAdapter(HTTPAdapter): | ||
"""An adapter that lets the user inject a custom SSL context for all | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please follow the style of other class and module docstrings in this library. |
||
requests made through it. | ||
|
||
The SSL context will simply be passed through to urllib3, which | ||
causes it to skip creation of its own context. | ||
|
||
Note that the SSLContext is not persisted when pickling - this is on | ||
purpose. | ||
So, after unpickling the SSLContextAdapter will behave like an | ||
HTTPAdapter until a new SSLContext is set. | ||
|
||
Example usage: | ||
|
||
.. code-block:: python | ||
|
||
import requests | ||
from ssl import create_default_context | ||
from requests import Session | ||
from requests_toolbelt.adapters.ssl_context import SSLContextAdapter | ||
|
||
s = Session() | ||
s.mount('https://', SSLContextAdapter(ssl_context=create_default_context())) | ||
""" | ||
|
||
def __init__(self, **kwargs): | ||
self.ssl_context = None | ||
if 'ssl_context' in kwargs: | ||
self.ssl_context = kwargs['ssl_context'] | ||
del kwargs['ssl_context'] | ||
|
||
super(SSLContextAdapter, self).__init__(**kwargs) | ||
|
||
def __setstate__(self, state): | ||
# SSLContext objects aren't picklable and shouldn't be persisted anyway | ||
self.ssl_context = None | ||
super(SSLContextAdapter, self).__setstate__(state) | ||
|
||
def init_poolmanager(self, *args, **kwargs): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please add docstrings to these methods. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi @sigmavirus24, I'm not sure what kind of information would be expected here in a docstring. The only useful information I could come up with would be something like "Calls superclass' init_poolmanager method with an extra arg on requests >= 2.4.0". This seems redundant, given that the methods themselves are really not complicated. Adding docstrings here would also be inconsistent with the implementations of the other Adapters in this module. If all methods are supposed to have some kind of docstring despite these arguments, I'll be happy to provide some - just trying to understand the reasoning here. Regards |
||
if requests.__build__ >= 0x020400: | ||
if 'ssl_context' not in kwargs: | ||
kwargs['ssl_context'] = self.ssl_context | ||
super(SSLContextAdapter, self).init_poolmanager(*args, **kwargs) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,60 @@ | ||
# -*- coding: utf-8 -*- | ||
"""Tests for the SSLContextAdapter.""" | ||
|
||
import pickle | ||
from ssl import SSLContext, PROTOCOL_TLSv1 | ||
|
||
import mock | ||
import pytest | ||
import requests | ||
from requests.adapters import HTTPAdapter | ||
from requests_toolbelt.adapters.ssl_context import SSLContextAdapter | ||
|
||
|
||
@pytest.mark.skipif(requests.__build__ < 0x020400, | ||
reason="Test case for newer requests versions.") | ||
@mock.patch.object(HTTPAdapter, 'init_poolmanager') | ||
def test_ssl_context_arg_is_passed_on_newer_requests(init_poolmanager): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please add docstrings to the test functions There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See also my comment on docstrings in the actual module - I've added slightly wordier descriptions as docstrings now. Is that what you expected @sigmavirus24? |
||
ssl_context = SSLContext(PROTOCOL_TLSv1) | ||
SSLContextAdapter( | ||
pool_connections=10, | ||
pool_maxsize=5, | ||
max_retries=0, | ||
pool_block=True, | ||
ssl_context=ssl_context | ||
) | ||
init_poolmanager.assert_called_once_with( | ||
10, 5, block=True, ssl_context=ssl_context | ||
) | ||
|
||
|
||
@pytest.mark.skipif(requests.__build__ >= 0x020400, | ||
reason="Test case for older requests versions.") | ||
@mock.patch.object(HTTPAdapter, 'init_poolmanager') | ||
def test_ssl_context_arg_is_not_passed_on_older_requests(init_poolmanager): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here too |
||
ssl_context = SSLContext(PROTOCOL_TLSv1) | ||
SSLContextAdapter( | ||
pool_connections=10, | ||
pool_maxsize=5, | ||
max_retries=0, | ||
pool_block=True, | ||
ssl_context=ssl_context | ||
) | ||
init_poolmanager.assert_called_once_with( | ||
10, 5, block=True | ||
) | ||
|
||
|
||
def test_adapter_has_ssl_context_attr(): | ||
ssl_context = SSLContext(PROTOCOL_TLSv1) | ||
adapter = SSLContextAdapter(ssl_context=ssl_context) | ||
|
||
assert adapter.ssl_context is ssl_context | ||
|
||
|
||
def test_adapter_loses_ssl_context_after_pickling(): | ||
ssl_context = SSLContext(PROTOCOL_TLSv1) | ||
adapter = SSLContextAdapter(ssl_context=ssl_context) | ||
adapter = pickle.loads(pickle.dumps(adapter)) | ||
|
||
assert adapter.ssl_context is None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please look at other submodules for the library's style and follow that