Repository: Sage-Bionetworks/synapsePythonClient

Issue:
KeyError: 'X-Amz-Date' when trying to download HTAN data
Hi team,

I've been trying to download some HTAN data, but have been unable to due to a dict KeyError.

Thank you in advance for your help,
Nikolas

# Operating system

RHEL 8.6

# Client version

4.5.1, Python 3.12 (replicated error in Python 3.9)

## Description of the problem

Currently trying to download data from HTAN.

```
import synapseclient
syn = synapseclient.Synapse(debug=True)
syn.login(authToken=os.environ["SYNAPSE_TOKEN"])
syn.get('syn26017536')
```

### Expected behavior

The file being downloaded

### Actual behavior

[ERROR] Error occurred while running <coroutine object download_file_entity at 0x1555520efb50> in a sync context.
Traceback (most recent call last):
  File "/gpfs/home/nk4167/miniconda/lib/python3.12/site-packages/synapseclient/core/async_utils.py", line 93, in wrap_async_to_sync
    return asyncio.run(coroutine)
           ^^^^^^^^^^^^^^^^^^^^^^
  File "/gpfs/home/nk4167/miniconda/lib/python3.12/asyncio/runners.py", line 194, in run
    return runner.run(main)
           ^^^^^^^^^^^^^^^^
  File "/gpfs/home/nk4167/miniconda/lib/python3.12/asyncio/runners.py", line 118, in run
    return self._loop.run_until_complete(task)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/gpfs/home/nk4167/miniconda/lib/python3.12/asyncio/base_events.py", line 687, in run_until_complete
    return future.result()
           ^^^^^^^^^^^^^^^
  File "/gpfs/home/nk4167/miniconda/lib/python3.12/site-packages/synapseclient/core/download/download_functions.py", line 172, in download_file_entity
    download_path = await download_by_file_handle(
                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/gpfs/home/nk4167/miniconda/lib/python3.12/site-packages/synapseclient/core/download/download_functions.py", line 515, in download_by_file_handle
    downloaded_path = await loop.run_in_executor(
                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/gpfs/home/nk4167/miniconda/lib/python3.12/concurrent/futures/thread.py", line 58, in run
    result = self.fn(*self.args, **self.kwargs)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/gpfs/home/nk4167/miniconda/lib/python3.12/site-packages/synapseclient/core/download/download_functions.py", line 517, in <lambda>
    lambda: download_from_url(
            ^^^^^^^^^^^^^^^^^^
  File "/gpfs/home/nk4167/miniconda/lib/python3.12/site-packages/synapseclient/core/download/download_functions.py", line 751, in download_from_url
    ) + PresignedUrlProvider._TIME_BUFFER >= _pre_signed_url_expiration_time(
                                             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/gpfs/home/nk4167/miniconda/lib/python3.12/site-packages/synapseclient/core/download/download_async.py", line 225, in _pre_signed_url_expiration_time
    time_made = parsed_query["X-Amz-Date"][0]
                ~~~~~~~~~~~~^^^^^^^^^^^^^^
KeyError: 'X-Amz-Date'

KeyError: 'X-Amz-Date'

Also happens when downloading from command line.


Discussion:
pep8speaks: Hello @BryanFauble! Thanks for updating this PR. We checked the lines you've touched for [PEP 8](https://www.python.org/dev/peps/pep-0008) issues, and found:







* In the file [`tests/unit/synapseclient/core/unit_test_download.py`](https://github.com/Sage-Bionetworks/synapsePythonClient/blob/2dbb55dc88fcd75966709dacee95bb3c59d9c6f7/tests/unit/synapseclient/core/unit_test_download.py):

> [Line 837:89](https://github.com/Sage-Bionetworks/synapsePythonClient/blob/2dbb55dc88fcd75966709dacee95bb3c59d9c6f7/tests/unit/synapseclient/core/unit_test_download.py#L837): [E501](https://duckduckgo.com/?q=pep8%20E501) line too long (105 > 88 characters)
> [Line 838:89](https://github.com/Sage-Bionetworks/synapsePythonClient/blob/2dbb55dc88fcd75966709dacee95bb3c59d9c6f7/tests/unit/synapseclient/core/unit_test_download.py#L838): [E501](https://duckduckgo.com/?q=pep8%20E501) line too long (117 > 88 characters)



Patch:
diff --git a/synapseclient/core/download/download_async.py b/synapseclient/core/download/download_async.py
index 3c8ca557..748c309d 100644
--- a/synapseclient/core/download/download_async.py
+++ b/synapseclient/core/download/download_async.py
@@ -214,6 +214,7 @@ def _generate_chunk_ranges(
 def _pre_signed_url_expiration_time(url: str) -> datetime:
     """
     Returns time at which a presigned url will expire
+    for an AWS S3 pre-signed url
 
     Arguments:
         url: A pre-signed download url from AWS
diff --git a/synapseclient/core/download/download_functions.py b/synapseclient/core/download/download_functions.py
index 9ed1e6c0..7c79975a 100644
--- a/synapseclient/core/download/download_functions.py
+++ b/synapseclient/core/download/download_functions.py
@@ -743,7 +743,10 @@ def download_from_url(
             )
 
             try:
-                url_has_expiration = "Expires" in urllib_urlparse.urlparse(url).query
+                url_query = urllib_urlparse.urlparse(url).query
+                url_has_expiration = (
+                    "Expires" in url_query and "X-Amz-Expires" in url_query
+                )
                 url_is_expired = False
                 if url_has_expiration:
                     url_is_expired = datetime.datetime.now(
@@ -773,8 +776,9 @@ def download_from_url(
                 exceptions._raise_for_status(response, verbose=client.debug)
             except SynapseHTTPError as err:
                 if err.response.status_code == 403:
+                    url_query = urllib_urlparse.urlparse(url).query
                     url_has_expiration = (
-                        "Expires" in urllib_urlparse.urlparse(url).query
+                        "Expires" in url_query and "X-Amz-Expires" in url_query
                     )
                     url_is_expired = False
                     if url_has_expiration:


Test patch:
diff --git a/tests/unit/synapseclient/core/download/unit_test_download_async.py b/tests/unit/synapseclient/core/download/unit_test_download_async.py
index fd6c6943..4ee1fdc0 100644
--- a/tests/unit/synapseclient/core/download/unit_test_download_async.py
+++ b/tests/unit/synapseclient/core/download/unit_test_download_async.py
@@ -133,6 +133,7 @@ class TestPresignedUrlProvider:
             "&X-Amz-Expires=86400"
             "&X-Amz-SignedHeaders=host"
             "&X-Amz-Signature=signature-value"
+            "&Expires=1715000000"
         )
 
         expected = (
diff --git a/tests/unit/synapseclient/core/unit_test_download.py b/tests/unit/synapseclient/core/unit_test_download.py
index 07caad80..78d7de6d 100644
--- a/tests/unit/synapseclient/core/unit_test_download.py
+++ b/tests/unit/synapseclient/core/unit_test_download.py
@@ -834,8 +834,16 @@ class TestDownloadFromUrl:
             assert not mocked_remove.called
 
     async def test_download_expired_url(self, syn: Synapse) -> None:
-        url = "http://www.ayy.lmao/filerino.txt?Expires=0"
-        new_url = "http://www.ayy.lmao/new_url.txt?Expires=1715000000"
+        url = (
+            "http://www.ayy.lmao/filerino.txt?Expires=0"
+            "X-Amz-Date=20240509T180000Z"
+            "&X-Amz-Expires=1000"
+        )
+        new_url = (
+            "http://www.ayy.lmao/new_url.txt?Expires=1715000000"
+            "X-Amz-Date=20240509T180000Z"
+            "&X-Amz-Expires=1000"
+        )
         contents = "\n".join(str(i) for i in range(1000))
         temp_destination = os.path.normpath(
             os.path.expanduser("~/fake/path/filerino.txt.temp")
@@ -899,6 +907,70 @@ class TestDownloadFromUrl:
                 auth=None,
             )
 
+    async def test_download_no_aws_expiration(self, syn: Synapse) -> None:
+        url = "http://www.ayy.lmao/filerino.txt?Expires=0"
+        contents = "\n".join(str(i) for i in range(1000))
+        temp_destination = os.path.normpath(
+            os.path.expanduser("~/fake/path/filerino.txt.temp")
+        )
+        destination = os.path.normpath(os.path.expanduser("~/fake/path/filerino.txt"))
+
+        mock_requests_get = MockRequestGetFunction(
+            [
+                create_mock_response(
+                    url,
+                    "stream",
+                    contents=contents,
+                    buffer_size=1024,
+                    partial_end=len(contents),
+                    status_code=200,
+                ),
+            ]
+        )
+        with patch.object(
+            syn._requests_session, "get", side_effect=mock_requests_get
+        ) as mocked_get, patch(
+            "synapseclient.core.download.download_functions._pre_signed_url_expiration_time",
+            return_value=datetime.datetime(1900, 1, 1, tzinfo=datetime.timezone.utc),
+        ) as mocked_pre_signed_url_expiration_time, patch(
+            "synapseclient.core.download.download_functions.get_file_handle_for_download",
+        ) as mocked_get_file_handle_for_download, patch.object(
+            Synapse, "_generate_headers", side_effect=mock_generate_headers
+        ), patch.object(
+            utils, "temp_download_filename", return_value=temp_destination
+        ), patch(
+            "synapseclient.core.download.download_functions.open",
+            new_callable=mock_open(),
+            create=True,
+        ), patch.object(
+            hashlib, "new"
+        ) as mocked_hashlib_new, patch.object(
+            shutil, "move"
+        ), patch.object(
+            os, "remove"
+        ):
+            mocked_hashlib_new.return_value.hexdigest.return_value = "fake md5 is fake"
+            # WHEN I call download_from_url with an expired url
+            download_from_url(
+                url=url,
+                destination=destination,
+                entity_id=OBJECT_ID,
+                file_handle_associate_type=OBJECT_TYPE,
+                expected_md5="fake md5 is fake",
+            )
+            # I expect the expired url to be identified
+            mocked_pre_signed_url_expiration_time.assert_not_called()
+            # AND I expect the URL to be refreshed
+            mocked_get_file_handle_for_download.assert_not_called()
+            # AND I expect the download to be retried with the new URL
+            mocked_get.assert_called_with(
+                url=url,
+                headers=mock_generate_headers(self),
+                stream=True,
+                allow_redirects=False,
+                auth=None,
+            )
+
     async def test_download_url_no_expiration(self, syn: Synapse) -> None:
         url = "http://www.ayy.lmao/filerino.txt"
         contents = "\n".join(str(i) for i in range(1000))