HOTFIX: AWS S3 resource returned unsigned if 'public' in bucket name… (…

…#85)

* HOTFIX: AWS S3 resource returned unsigned if 'public' in bucket name and no credentials on Alyx

CHANGELOG.md

@@ -1,5 +1,11 @@
 # Changelog
-## [Latest](https://github.com/int-brain-lab/ONE/commits/main) [1.21.2]
+## [Latest](https://github.com/int-brain-lab/ONE/commits/main) [1.21.3]
+
+### Modified
+
+- HOTFIX: AWS S3 resource returned unsigned if 'public' in bucket name and no credentials on Alyx 
+
+## [1.21.2]
 
 ### Modified
 

one/__init__.py

@@ -1,2 +1,2 @@
 """The Open Neurophysiology Environment (ONE) API"""
-__version__ = '1.21.2'
+__version__ = '1.21.3'

one/remote/aws.py

@@ -160,10 +160,22 @@ def get_s3_from_alyx(alyx, repo_name=REPO_DEFAULT):
         An S3 ServiceResource instance with the provided.
     str
         The name of the S3 bucket.
+
+    Notes
+    -----
+    - If no credentials are present in the database, boto3 will use environment config or default
+     AWS profile settings instead.
+    - If there are no credentials for the bucket and the bucket has 'public' in the name, the
+     returned resource will use an unsigned signature.
     """
     session_keys, bucket_name = get_aws_access_keys(alyx, repo_name)
+    no_creds = not any(filter(None, (v for k, v in session_keys.items() if 'key' in k.lower())))
     session = boto3.Session(**session_keys)
-    s3 = session.resource('s3')
+    if no_creds and 'public' in bucket_name.lower():
+        config = Config(signature_version=UNSIGNED)
+    else:
+        config = None
+    s3 = session.resource('s3', config=config)
     return s3, bucket_name
 
 

one/tests/remote/test_remote_aws.py

@@ -11,6 +11,7 @@
 from one.webclient import AlyxClient
 try:
     import one.remote.aws as aws
+    from botocore import UNSIGNED
 except ModuleNotFoundError:
     raise unittest.SkipTest('boto3 module not installed')
 
@@ -68,11 +69,14 @@ class TestAWS(unittest.TestCase):
     repo = None
 
     tempdir = None
-    one = None
 
     @classmethod
     def setUpClass(cls) -> None:
         cls.tempdir = util.set_up_env()
+        cls.expected_credentials = {
+            'aws_access_key_id': 'ABCDEF',
+            'aws_secret_access_key': 'shhh',
+            'region_name': None}
         with mock.patch('one.params.iopar.getfile', new=partial(util.get_file, cls.tempdir.name)):
             # util.setup_test_params(token=True)
             cls.alyx = AlyxClient(
@@ -83,13 +87,33 @@ def setUpClass(cls) -> None:
     def test_credentials(self):
         """Test for one.remote.aws.get_aws_access_keys function."""
         cred, bucket_name = aws.get_aws_access_keys(self.alyx, 'aws_cortexlab')
-        expected = {
-            'aws_access_key_id': 'ABCDEF',
-            'aws_secret_access_key': 'shhh',
-            'region_name': None
-        }
-        self.assertDictEqual(cred, expected)
+        self.assertDictEqual(cred, self.expected_credentials)
+        self.assertEqual(bucket_name, 's3_bucket')
+
+    @mock.patch('boto3.Session')
+    def test_get_s3_from_alyx(self, session_mock):
+        """Tests for one.remote.aws.get_s3_from_alyx function"""
+        s3, bucket_name = aws.get_s3_from_alyx(self.alyx, 'aws_cortexlab')
         self.assertEqual(bucket_name, 's3_bucket')
+        session_mock.assert_called_once_with(**self.expected_credentials)
+        resource = session_mock().resource
+        resource.assert_called_once_with('s3', config=None)
+        self.assertIs(s3, resource())
+
+        # Assert that resource is unsigned when no credentials are returned
+        session_mock.reset_mock()
+        repo_json = {'json': {'bucket_name': 'public_foo'}}
+        with mock.patch.object(self.alyx, 'rest', return_value=repo_json):
+            s3, _ = aws.get_s3_from_alyx(self.alyx)
+        _, kwargs = resource.call_args
+        self.assertIs(kwargs['config'].signature_version, UNSIGNED)
+        # If the bucket does not have 'public' in the name, no assumptions should be made about
+        # the credentials
+        session_mock.reset_mock()
+        repo_json['json']['bucket_name'] = 'private_foo'
+        with mock.patch.object(self.alyx, 'rest', return_value=repo_json):
+            s3, _ = aws.get_s3_from_alyx(self.alyx)
+        resource.assert_called_once_with('s3', config=None)
 
 
 class TestUtils(unittest.TestCase):
@@ -112,5 +136,5 @@ def test_get_s3_virtual_host(self):
             aws.get_s3_virtual_host('s3://my-s3-bucket/path/to/file', 'wrong-foo-4')
 
 
-if __name__ == "__main__":
+if __name__ == '__main__':
     unittest.main(exit=False)