Merge pull request #2603 from kobotoolbox/2280-s3-private-storage

Fix S3 / private storage issues
kobotoolbox · Mar 24, 2020 · f94f7c0 · f94f7c0
2 parents f8afe37 + a691946
commit f94f7c0
Show file tree

Hide file tree

Showing 3 changed files with 9 additions and 35 deletions.
diff --git a/kobo/settings/testing.py b/kobo/settings/testing.py
@@ -10,8 +10,5 @@
 
 TESTING = True
 
-if 'KPI_AWS_STORAGE_BUCKET_NAME' in os.environ:
-    PRIVATE_STORAGE_S3_REVERSE_PROXY = False
-
 # Decrease prod value to speed-up tests
 SUBMISSION_LIST_LIMIT = 100
diff --git a/kpi/tests/api/v1/test_api_assets.py b/kpi/tests/api/v1/test_api_assets.py
@@ -257,15 +257,6 @@ def setUp(self):
         self.asset.deployment.mock_submissions([submission])
         settings.CELERY_TASK_ALWAYS_EAGER = True
 
-    @staticmethod
-    def result_stored_locally(detail_response):
-        """
-        Return `True` if the result is stored locally, or `False` if it's
-        housed externally (e.g. on Amazon S3)
-        """
-        export_task = ExportTask.objects.get(uid=detail_response.data['uid'])
-        return isinstance(export_task.result.storage, PrivateFileSystemStorage)
-
     def test_owner_can_create_export(self):
         post_url = reverse('exporttask-list')
         asset_url = reverse('asset-detail', args=[self.asset.uid])
@@ -282,13 +273,8 @@ def test_owner_can_create_export(self):
         self.assertEqual(detail_response.data['status'], 'complete')
         self.assertEqual(detail_response.data['messages'], {})
         # Get the result file
-        if self.result_stored_locally(detail_response):
-            result_response = self.client.get(detail_response.data['result'])
-            result_content = result_response.getvalue().decode('utf-8')
-        else:
-            result_response = requests.get(detail_response.data['result'])
-            result_response.encoding = 'utf-8'
-            result_content = result_response.text
+        result_response = self.client.get(detail_response.data['result'])
+        result_content = result_response.getvalue().decode('utf-8')
         self.assertEqual(result_response.status_code, status.HTTP_200_OK)
         expected_content = ''.join([
             '"q1";"_id";"_uuid";"_submission_time";"_validation_status";"_index"\r\n',
@@ -304,22 +290,16 @@ def test_other_user_cannot_access_export(self):
         self.client.login(username='otheruser', password='otheruser')
         response = self.client.get(detail_response.data['url'])
         self.assertEqual(response.status_code, status.HTTP_404_NOT_FOUND)
-        if self.result_stored_locally(detail_response):
-            # This check only makes sense for locally-stored results, since S3
-            # uses query parameters in the URL for access control
-            response = self.client.get(detail_response.data['result'])
-            self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN)
+        response = self.client.get(detail_response.data['result'])
+        self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN)
 
     def test_anon_cannot_access_export(self):
         detail_response = self.test_owner_can_create_export()
         self.client.logout()
         response = self.client.get(detail_response.data['url'])
         self.assertEqual(response.status_code, status.HTTP_404_NOT_FOUND)
-        if self.result_stored_locally(detail_response):
-            # This check only makes sense for locally-stored results, since S3
-            # uses query parameters in the URL for access control
-            response = self.client.get(detail_response.data['result'])
-            self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN)
+        response = self.client.get(detail_response.data['result'])
+        self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN)
 
     def test_owner_with_token_auth_can_access_export(self):
         detail_response = self.test_owner_can_create_export()
@@ -332,10 +312,7 @@ def test_owner_with_token_auth_can_access_export(self):
         response = self.client.get(detail_response.data['url'])
         self.assertEqual(response.status_code, status.HTTP_200_OK)
         # Get the result file
-        if self.result_stored_locally(detail_response):
-            result_response = self.client.get(detail_response.data['result'])
-        else:
-            result_response = requests.get(detail_response.data['result'])
+        result_response = self.client.get(detail_response.data['result'])
         self.assertEqual(result_response.status_code, status.HTTP_200_OK)
 
 

diff --git a/kpi/utils/private_storage.py b/kpi/utils/private_storage.py
@@ -21,7 +21,7 @@ def superuser_or_username_matches_prefix(private_file):
 
     user = private_file.request.user
 
-    if not user.is_authenticated():
+    if not user.is_authenticated:
         # Try all the DRF authentication methods before giving up
         request = DRFRequest(
             private_file.request,
@@ -30,7 +30,7 @@ def superuser_or_username_matches_prefix(private_file):
             ]
         )
         user = request.user
-        if not user.is_authenticated():
+        if not user.is_authenticated:
             return False
 
     if user.is_superuser: