Skip to content
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

In v2 of the SDK, the auto_refresh parameter of HTTPClient is not respected. #151

Closed
mishas opened this issue Apr 8, 2020 · 6 comments
Closed
Labels
bug Something isn't working

Comments

@mishas
Copy link
Contributor

mishas commented Apr 8, 2020

The documentation for the auto_refresh parameter of HTTPClient says:

auto_refresh (bool): Perform token refresh following HTTP 401 response from server. Defaults to True.

But there seems not to be any code to catch 401 and refresh the token.

Furthermore, in the request method, on line 223, there's credentials = kwargs.pop("credentials", None), where it should probably be credentials = kwargs.pop("credentials", self._credentials).

Expected behavior

When there's a 401, a new token should be generated, and a retry should happen.

Current behavior

I'm starting to get requests.exceptions.HTTPError: 401 Client Error: Unauthorized for url: https://api.us.cdl.paloaltonetworks.com:443/query/v2/jobs after ~1 hour of use.

Your Environment

  • Version used: 2.0.0a9
  • Environment name and version (e.g. Chrome 59, node.js 5.4, python 3.7.3): python3.7
  • Operating System and version (desktop or mobile): Debian 10
@mishas mishas added the bug Something isn't working label Apr 8, 2020
@sserrata
Copy link
Member

sserrata commented Apr 8, 2020

Concerning the HTTPClient auto_refresh attribute/kwarg, the docstring will be updated/corrected to reflect the true behavior:

"Perform token refresh prior to request if access_token is None or expired. Defaults to True"

@sserrata
Copy link
Member

sserrata commented Apr 8, 2020

Furthermore, in the request method, on line 223, there's credentials = kwargs.pop("credentials", None), where it should probably be credentials = kwargs.pop("credentials", self._credentials).

With respect to kwargs.pop("credentials", None), this was changed in order to avoid inadvertently applying credentials twice. In controlled tests, I've observed auto_refresh behaving as expected. Please see the test steps below:

  1. Generate credentials profile with client_id, client_secret and refresh_token
  2. Attempt an API request and observe initial auto_refresh behavior due to None condition (see below)

Note: an extra INFO log was added to log the headers value in the request method

(env) DFWMAC1P6LVDN:examples sserrata$ ./test.py 
DEBUG:pan_cortex_data_lake.httpclient:Default headers applied: {'User-Agent': 'cortex-data-lake-python/2.0.0-a9', 'Accept-Encoding': 'gzip, deflate', 'Accept': 'application/json', 'Connection': 'keep-alive'}
DEBUG:pan_cortex_data_lake.httpclient:Default headers applied: {'User-Agent': 'cortex-data-lake-python/2.0.0-a9', 'Accept-Encoding': 'gzip, deflate', 'Accept': 'application/json', 'Connection': 'keep-alive'}
DEBUG:pan_cortex_data_lake.httpclient:Applying session-level credentials
INFO:pan_cortex_data_lake.httpclient:{'User-Agent': 'cortex-data-lake-python/2.0.0-a9', 'Accept-Encoding': 'gzip, deflate', 'Accept': 'application/json', 'Connection': 'keep-alive'}
DEBUG:urllib3.connectionpool:Starting new HTTPS connection (1): api.paloaltonetworks.com:443
DEBUG:urllib3.connectionpool:https://api.paloaltonetworks.com:443 "POST /api/oauth2/RequestToken HTTP/1.1" 200 630
DEBUG:pan_cortex_data_lake.httpclient:Token refreshed due to 'None' condition
DEBUG:pan_cortex_data_lake.httpclient:Credentials applied to authorization header
INFO:pan_cortex_data_lake.httpclient:{'User-Agent': 'cortex-data-lake-python/2.0.0-a9', 'Accept-Encoding': 'gzip, deflate', 'Accept': 'application/json', 'Connection': 'keep-alive', 'Authorization': 'Bearer eyJ...'}
DEBUG:urllib3.connectionpool:Starting new HTTPS connection (1): api.us.cdl.paloaltonetworks.com:443
DEBUG:urllib3.connectionpool:https://api.us.cdl.paloaltonetworks.com:443 "POST /query/v2/jobs HTTP/1.1" 201 108
  1. Manually edit the credentials.json profile to replace existing access_token with an expired token.
  2. Attempt API request again and observe auto_refresh behavior, this time refreshing due to "Expired" condition.
DEBUG:pan_cortex_data_lake.httpclient:Default headers applied: {'User-Agent': 'cortex-data-lake-python/2.0.0-a9', 'Accept-Encoding': 'gzip, deflate', 'Accept': 'application/json', 'Connection': 'keep-alive'}
DEBUG:pan_cortex_data_lake.httpclient:Default headers applied: {'User-Agent': 'cortex-data-lake-python/2.0.0-a9', 'Accept-Encoding': 'gzip, deflate', 'Accept': 'application/json', 'Connection': 'keep-alive'}
DEBUG:pan_cortex_data_lake.httpclient:Applying session-level credentials
INFO:pan_cortex_data_lake.httpclient:{'User-Agent': 'cortex-data-lake-python/2.0.0-a9', 'Accept-Encoding': 'gzip, deflate', 'Accept': 'application/json', 'Connection': 'keep-alive'}
DEBUG:urllib3.connectionpool:Starting new HTTPS connection (1): api.paloaltonetworks.com:443
DEBUG:urllib3.connectionpool:https://api.paloaltonetworks.com:443 "POST /api/oauth2/RequestToken HTTP/1.1" 200 630
DEBUG:pan_cortex_data_lake.httpclient:Token refreshed due to 'expired' condition
DEBUG:pan_cortex_data_lake.httpclient:Credentials applied to authorization header
INFO:pan_cortex_data_lake.httpclient:{'User-Agent': 'cortex-data-lake-python/2.0.0-a9', 'Accept-Encoding': 'gzip, deflate', 'Accept': 'application/json', 'Connection': 'keep-alive', 'Authorization': 'Bearer eyJ...'}
DEBUG:urllib3.connectionpool:Starting new HTTPS connection (1): api.us.cdl.paloaltonetworks.com:443
DEBUG:urllib3.connectionpool:https://api.us.cdl.paloaltonetworks.com:443 "POST /query/v2/jobs HTTP/1.1" 201 108

As best as I can tell, everything appears to be working as expected. Please let me know if I'm missing something. Thanks!

@mishas
Copy link
Contributor Author

mishas commented Apr 8, 2020

Hello @sserrata,

In the test proposed above, you're recreating the object (and in that case, it indeed refreshes the token). But if you keep holding the object - it doesn't:

c = Credentials(refresh_token=MY_KEY, client_id=MY_CLIENT_ID, client_secret=MY_CLIENT_SECRET)
qs = QueryService(url="https://api.us.cdl.paloaltonetworks.com", credentials=c)
q = qs.create_query(query_params=query_params)  # This will work
time.sleep(60*60+ 5*60)  # Sleep for 1 hour and 5 minutes
q = qs.create_query(...)  # This will not work.

@sserrata
Copy link
Member

sserrata commented Apr 8, 2020

Hi @mishas - yes, I understand that after an hour the access_token would have expired. I'm attempting to simulate this by manually replacing the credentials.json access_token with an expired value. I will try to replicate using the same method you outlined above and will report back with the results (in ~1 hour).

@mishas
Copy link
Contributor Author

mishas commented Apr 8, 2020

You can simulate this by calling:
qs._httpclient.session.headers.update({"Authorization": "Bearer {}".format(old_token)})
instead of sleep.

(i'm not 100% sure about the above, didn't try it, but I think it should work)

sserrata added a commit to sserrata/pan-cortex-data-lake-python that referenced this issue Apr 9, 2020
@sserrata sserrata mentioned this issue Apr 9, 2020
4 tasks
@sserrata
Copy link
Member

sserrata commented Apr 9, 2020

Hi @mishas - please see PR #152 for a fix. I discovered a regression bug introduced in #140. In weighing different options, I realized that it made little sense to apply credentials at the session/HTTPClient layer, if they would ultimately get applied at the request layer. Thanks again for reporting this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants