Skip to content
This repository has been archived by the owner on Sep 29, 2023. It is now read-only.

Add verify and proxies to context #142

Merged
merged 3 commits into from
May 15, 2018
Merged

Add verify and proxies to context #142

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
35f9e00
Add verify and proxies to context
lmazuel May 7, 2018
56bbb8f
Fix verify logic
lmazuel May 8, 2018
87f029a
Review
lmazuel May 8, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions adal/authentication_context.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ class AuthenticationContext(object):

def __init__(
self, authority, validate_authority=None, cache=None,
api_version='1.0', timeout=None, enable_pii=False):
api_version='1.0', timeout=None, enable_pii=False, verify_ssl=None, proxies=None):
'''Creates a new AuthenticationContext object.

By default the authority will be checked against a list of known Azure
Expand Down Expand Up @@ -75,11 +75,17 @@ def __init__(
read timeout) <timeouts>` tuple.
:param enable_pii: (optional) Unless this is set to True,
there will be no Personally Identifiable Information (PII) written in log.
:param verify_ssl: (optional) requests verify. Either a boolean, in which case it
controls whether we verify the server's TLS certificate, or a string, in which
case it must be a path to a CA bundle to use. This value is ignored if env
variable ADAL_PYTHON_SSL_NO_VERIFY is used.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be more natural that we do the other way around, i.e. env var will be ignored if function argument is explicitly provided? I know there might not be an industry standard on this behavior but, it happens that the Python standard lib urllib uses the same approach as what I suggested here:

Alternatively, the optional proxies argument may be used to explicitly specify proxies. It must be a dictionary mapping scheme names to proxy URLs, where an empty dictionary causes no proxies to be used, and None (the default value) causes environmental proxy settings to be used as discussed above. For example: ...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @rayluo on this

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, in progress

:param proxies: (optional) requests proxies. Dictionary mapping protocol to the URL
of the proxy.
Copy link
Collaborator

@rayluo rayluo May 8, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is true that this is the same concise docstring used by requests library. But they also provide intensive documentation to provide the examples on what "dictionary mapping" actually looks like. Since we don't currently have those documentation effort, I would suggest we at least add one more sentence here. Either explicitly mentions an example:

Its content will look like proxies = { 'http': 'http://10.10.1.10:3128', 'https': 'http://10.10.1.10:1080', }

Or simply add a link to its official documentation in requests (since we are exposing this requests behavior anyway).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer external link, scale better in the long term IMHO :)

'''
self.authority = Authority(authority, validate_authority is None or validate_authority)
self._oauth2client = None
self.correlation_id = None
env_value = os.environ.get('ADAL_PYTHON_SSL_NO_VERIFY')
env_value = os.environ.get('ADAL_PYTHON_SSL_NO_VERIFY', verify_ssl)
if api_version is not None:
warnings.warn(
"""The default behavior of including api-version=1.0 on the wire
Expand All @@ -95,6 +101,7 @@ def __init__(
'options': GLOBAL_ADAL_OPTIONS,
'api_version': api_version,
'verify_ssl': None if env_value is None else not env_value, # mainly for tracing through proxy
Copy link
Contributor

@yugangw-msft yugangw-msft May 8, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, this line will need to be updated; otherwise the argument value will not be propagated particularly when it is a string to a bundle

Copy link
Contributor Author

@lmazuel lmazuel May 8, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, the argument value was passed as default parameter in the os.environ, so yes, it was propagated, but I will simplify this line

'proxies':proxies,
'timeout':timeout,
"enable_pii": enable_pii,
}
Expand Down
3 changes: 2 additions & 1 deletion adal/authority.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,8 @@ def _perform_dynamic_instance_discovery(self):

try:
resp = requests.get(discovery_endpoint.geturl(), headers=get_options['headers'],
verify=self._call_context.get('verify_ssl', None))
verify=self._call_context.get('verify_ssl', None),
proxies=self._call_context.get('proxies', None))
util.log_return_correlation_id(self._log, operation, resp)
except Exception:
self._log.exception("%(operation)s request failed",
Expand Down
3 changes: 2 additions & 1 deletion adal/mex.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,8 @@ def discover(self):
try:
operation = "Mex Get"
resp = requests.get(self._url, headers=options['headers'],
verify=self._call_context.get('verify_ssl', None))
verify=self._call_context.get('verify_ssl', None),
proxies=self._call_context.get('proxies', None))
util.log_return_correlation_id(self._log, operation, resp)
except Exception:
self._log.exception(
Expand Down
3 changes: 3 additions & 0 deletions adal/oauth2_client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -264,6 +264,7 @@ def get_token(self, oauth_parameters):
data=url_encoded_token_request,
headers=post_options['headers'],
verify=self._call_context.get('verify_ssl', None),
proxies=self._call_context.get('proxies', None),
timeout=self._call_context.get('timeout', None))

util.log_return_correlation_id(self._log, operation, resp)
Expand Down Expand Up @@ -298,6 +299,7 @@ def get_user_code_info(self, oauth_parameters):
data=url_encoded_code_request,
headers=post_options['headers'],
verify=self._call_context.get('verify_ssl', None),
proxies=self._call_context.get('proxies', None),
timeout=self._call_context.get('timeout', None))
util.log_return_correlation_id(self._log, operation, resp)
except Exception:
Expand Down Expand Up @@ -339,6 +341,7 @@ def get_token_with_polling(self, oauth_parameters, refresh_internal, expires_in)
resp = requests.post(
token_url.geturl(),
data=url_encoded_code_request, headers=post_options['headers'],
proxies=self._call_context.get('proxies', None),
verify=self._call_context.get('verify_ssl', None))
if resp.status_code == 429:
resp.raise_for_status() # Will raise requests.exceptions.HTTPError
Expand Down
1 change: 1 addition & 0 deletions adal/user_realm.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,7 @@ def discover(self):

operation = 'User Realm Discovery'
resp = requests.get(user_realm_url.geturl(), headers=options['headers'],
proxies=self._call_context.get('proxies', None),
verify=self._call_context.get('verify_ssl', None))
util.log_return_correlation_id(self._log, operation, resp)

Expand Down
1 change: 1 addition & 0 deletions adal/wstrust_request.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,7 @@ def acquire_token(self, username, password):
resp = requests.post(self._wstrust_endpoint_url, headers=options['headers'], data=rst,
allow_redirects=True,
verify=self._call_context.get('verify_ssl', None),
proxies=self._call_context.get('proxies', None),
timeout=self._call_context.get('timeout', None))

util.log_return_correlation_id(self._log, operation, resp)
Expand Down