From 374705feffc77752221c77d38dcc97a2e8d672eb Mon Sep 17 00:00:00 2001
From: Dan Scales <dan.scales@gmail.com>
Date: Wed, 27 Mar 2024 12:49:44 -0700
Subject: [PATCH] GTC-2449 Catch access denied error in get_aws_files()

Currently, we are only catching NoSuchBucket errors, but there can also
be AccessDenied errors. If an 'access denied' happens, we get an
uncaught exception, which gives an internal server error. Strangely,
s3_client.exceptions doesn't have AccessDenied, so we just catch all
botocore exceptions (which is probably what we should do anyway).

I couldn't create any new test to emulate access denied, so will just
test it out in staging.
---
 app/routes/datasets/versions.py        | 2 +-
 app/utils/aws.py                       | 8 +++++++-
 tests/routes/datasets/test_versions.py | 4 ++--
 3 files changed, 10 insertions(+), 4 deletions(-)
 mode change 100644 => 100755 tests/routes/datasets/test_versions.py

diff --git a/app/routes/datasets/versions.py b/app/routes/datasets/versions.py
index ae4696138..50cdf0356 100644
--- a/app/routes/datasets/versions.py
+++ b/app/routes/datasets/versions.py
@@ -560,7 +560,7 @@ def _verify_source_file_access(sources: List[str]) -> None:
         raise HTTPException(
             status_code=400,
             detail=(
-                "Cannot access all of the source files. "
+                "Cannot access all of the source files (non-existent or access denied). "
                 f"Invalid sources: {invalid_sources}"
             ),
         )
diff --git a/app/utils/aws.py b/app/utils/aws.py
index fe981be42..2f9e2da66 100644
--- a/app/utils/aws.py
+++ b/app/utils/aws.py
@@ -1,8 +1,10 @@
 from typing import Any, Dict, List, Optional, Sequence
 
 import boto3
+import botocore
 import httpx
 from httpx_auth import AWS4Auth
+from fastapi.logger import logger
 
 from ..settings.globals import (
     AWS_REGION,
@@ -108,7 +110,11 @@ def get_aws_files(
             if exit_after_max and num_matches >= exit_after_max:
                 break
 
-    except s3_client.exceptions.NoSuchBucket:
+    # Strangely, s3_client.exceptions has NoSuchBucket, but doesn't have
+    # AccessDenied, even though you can get that error, so we just catch all botocore
+    # exceptions.
+    except botocore.exceptions.ClientError as error:
+        logger.warning(f"get_aws_file: {error}")
         matches = list()
 
     return matches
diff --git a/tests/routes/datasets/test_versions.py b/tests/routes/datasets/test_versions.py
old mode 100644
new mode 100755
index 492903d75..e1cc6a1aa
--- a/tests/routes/datasets/test_versions.py
+++ b/tests/routes/datasets/test_versions.py
@@ -310,7 +310,7 @@ async def test_invalid_source_uri(async_client: AsyncClient):
     assert response.json()["status"] == "failed"
     assert (
         response.json()["message"]
-        == f"Cannot access all of the source files. Invalid sources: ['{bad_uri}']"
+        == f"Cannot access all of the source files (non-existent or access denied). Invalid sources: ['{bad_uri}']"
     )
 
     # Create a version with a valid source_uri so we have something to append to
@@ -332,7 +332,7 @@ async def test_invalid_source_uri(async_client: AsyncClient):
     assert response.json()["status"] == "failed"
     assert (
         response.json()["message"]
-        == f"Cannot access all of the source files. Invalid sources: ['{bad_uri}']"
+        == f"Cannot access all of the source files (non-existent or access denied). Invalid sources: ['{bad_uri}']"
     )
 
     # Test appending to a version that DOESN'T exist