Timeouts on downloads do not raise TimeoutRequestError

[redshiftzero]

In #80 we added timeouts support into the SDK, such that in the client side, we could just handle the SDK's RequestTimeoutError which is what we are now doing: https://github.com/freedomofpress/securedrop-client/blob/master/securedrop_client/queue.py#L42.

However for reply and submission downloads RequestTimeoutErrors do not raise, due to this line: https://github.com/freedomofpress/securedrop-sdk/blob/master/sdclientapi/__init__.py#L607 which will cause the exception type raised to be BaseError.

This behavior was discovered by @creviera in freedomofpress/securedrop-client#421 (review)

Steps to reproduce

1. Apply the following diff in this repo to decrease the default timeouts for testing:

diff --git a/sdclientapi/__init__.py b/sdclientapi/__init__.py
index 9cf34f2..fb6a91e 100644
--- a/sdclientapi/__init__.py
+++ b/sdclientapi/__init__.py
@@ -19,8 +19,8 @@ from .sdlocalobjects import (
 )

 DEFAULT_PROXY_VM_NAME = "sd-proxy"
-DEFAULT_REQUEST_TIMEOUT = 20  # 20 seconds
-DEFAULT_DOWNLOAD_TIMEOUT = 60 * 60  # 60 minutes
+DEFAULT_REQUEST_TIMEOUT = 5  # 20 seconds
+DEFAULT_DOWNLOAD_TIMEOUT = 5  # 60 minutes

2. Add a time.sleep(10) to trigger timeouts in the API method for submission downloads: https://github.com/freedomofpress/securedrop/blob/develop/securedrop/journalist_app/api.py#L188

3. Start the SecureDrop server container

4. Now try to use the SDK to download files:

from sdclientapi import API, RequestTimeoutError

api = API(address='http://127.0.0.1:8081', username='journalist', passphrase='test', totp='123456')
api.authenticate()
submissions = api.get_all_submissions()
test_submission = api.get_submission_from_string(submissions[0].uuid, submissions[0].source_uuid)
try:
    api.download_submission(test_submission)
except RequestTimeoutError:
    print('We should get here!')

Expected Behavior

RequestTimeoutError is raised and then handled, and We should get here! is printed

Actual Behavior

The type of the exception that is ultimately raised is BaseError:

....
    raise ReadTimeoutError(self, url, "Read timed out. (read timeout=%s)" % timeout_value)
urllib3.exceptions.ReadTimeoutError: HTTPConnectionPool(host='127.0.0.1', port=8081): Read timed out. (read timeout=5)

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/Users/redshiftzero/Documents/Github/securedrop-sdk/sdclientapi/__init__.py", line 146, in _send_http_json_request
    result = requests.request(method, url, **kwargs)
  File "/Users/redshiftzero/.virtualenvs/securedrop-sdk-JZz8Wrdu/lib/python3.5/site-packages/requests/api.py", line 60, in request
    return session.request(method=method, url=url, **kwargs)
  File "/Users/redshiftzero/.virtualenvs/securedrop-sdk-JZz8Wrdu/lib/python3.5/site-packages/requests/sessions.py", line 524, in request
    resp = self.send(prep, **send_kwargs)
  File "/Users/redshiftzero/.virtualenvs/securedrop-sdk-JZz8Wrdu/lib/python3.5/site-packages/requests/sessions.py", line 637, in send
    r = adapter.send(request, **kwargs)
  File "/Users/redshiftzero/.virtualenvs/securedrop-sdk-JZz8Wrdu/lib/python3.5/site-packages/requests/adapters.py", line 529, in send
    raise ReadTimeout(e, request=request)
requests.exceptions.ReadTimeout: HTTPConnectionPool(host='127.0.0.1', port=8081): Read timed out. (read timeout=5)

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/Users/redshiftzero/Documents/Github/securedrop-sdk/sdclientapi/__init__.py", line 578, in download_submission
    timeout=timeout or self.default_download_timeout,
  File "/Users/redshiftzero/Documents/Github/securedrop-sdk/sdclientapi/__init__.py", line 126, in _send_json_request
    return func(method, path_query, body, headers, timeout)
  File "/Users/redshiftzero/Documents/Github/securedrop-sdk/sdclientapi/__init__.py", line 148, in _send_http_json_request
    raise RequestTimeoutError
sdclientapi.RequestTimeoutError: The request timed out.

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "test_sdk.py", line 8, in <module>
    api.download_submission(test_submission)
  File "/Users/redshiftzero/Documents/Github/securedrop-sdk/sdclientapi/__init__.py", line 607, in download_submission
    raise BaseError(err)
sdclientapi.sdlocalobjects.BaseError: The request timed out.

Solution

1. Assert the exceptions we think we are raising in the tests
2. Modify logic to allow RequestTimeoutError to raise for timeouts on download_submission and download_reply

[redshiftzero]

Removing this big try/except block in download_submission and download_reply seems like the obvious resolution here... but I might be missing something: do you remember @kushaldas why we added this logic? (looks like it was first added in 5f86412)

[kushaldas]

do you remember @kushaldas why we added this logic? (looks like it was first added in 5f86412)

IIRC I added it to make sure in any case we at least raise BaseError, but I think I should have done that only for the lines related to file saving etc.

[kushaldas]

After writing the tests, I suddenly realized that I am not sure if securedrop-proxy will need some modification or not.

With more coffee, I saw the right code in the file.