upload: Strip leading slash from deleted S3 export paths.

Previously, S3UploadBackend.delete_export_tarball failed to strip the
leading ‘/’ from the export path.  This mistake is now caught by Moto
1.3.15.  I expect it caused deletion failures in the real S3, although
I haven’t verified this.

We store export_path in the audit log with a leading ‘/’, but the
actual S3 keys do not have a leading ‘/’.  Changing either system
would require a migration.  So the new convention is that the
variables named ‘export_path’ have a leading ‘/’, while variables
named ‘path_id’ or ‘key’ do not.

Signed-off-by: Anders Kaseorg <anders@zulip.com>

zerver/lib/upload.py

@@ -253,7 +253,7 @@ class ZulipUploadBackend:
                               percent_callback: Optional[Callable[[Any], None]]=None) -> str:
         raise NotImplementedError()
 
-    def delete_export_tarball(self, path_id: str) -> Optional[str]:
+    def delete_export_tarball(self, export_path: str) -> Optional[str]:
         raise NotImplementedError()
 
     def get_export_tarball_url(self, realm: Realm, export_path: str) -> str:
@@ -606,9 +606,11 @@ class S3UploadBackend(ZulipUploadBackend):
         )
         return public_url
 
-    def delete_export_tarball(self, path_id: str) -> Optional[str]:
+    def delete_export_tarball(self, export_path: str) -> Optional[str]:
+        assert export_path.startswith("/")
+        path_id = export_path[1:]
         if self.delete_file_from_s3(path_id, self.avatar_bucket):
-            return path_id
+            return export_path
         return None
 
 ### Local
@@ -825,11 +827,12 @@ class LocalUploadBackend(ZulipUploadBackend):
         public_url = realm.uri + '/user_avatars/' + path
         return public_url
 
-    def delete_export_tarball(self, path_id: str) -> Optional[str]:
+    def delete_export_tarball(self, export_path: str) -> Optional[str]:
         # Get the last element of a list in the form ['user_avatars', '<file_path>']
-        file_path = path_id.strip('/').split('/', 1)[-1]
+        assert export_path.startswith("/")
+        file_path = export_path[1:].split('/', 1)[-1]
         if delete_local_file('avatars', file_path):
-            return path_id
+            return export_path
         return None
 
     def get_export_tarball_url(self, realm: Realm, export_path: str) -> str:
@@ -904,5 +907,5 @@ def upload_export_tarball(realm: Realm, tarball_path: str,
     return upload_backend.upload_export_tarball(realm, tarball_path,
                                                 percent_callback=percent_callback)
 
-def delete_export_tarball(path_id: str) -> Optional[str]:
-    return upload_backend.delete_export_tarball(path_id)
+def delete_export_tarball(export_path: str) -> Optional[str]:
+    return upload_backend.delete_export_tarball(export_path)

zerver/tests/test_realm_export.py

@@ -63,8 +63,9 @@ class RealmExportTest(ZulipTestCase):
         self.assertEqual(audit_log_entry.acting_user_id, admin.id)
 
         # Test that the file is hosted, and the contents are as expected.
-        path_id = orjson.loads(audit_log_entry.extra_data).get('export_path')
-        self.assertIsNotNone(path_id)
+        export_path = orjson.loads(audit_log_entry.extra_data)['export_path']
+        assert export_path.startswith('/')
+        path_id = export_path[1:]
         self.assertEqual(bucket.Object(path_id).get()['Body'].read(), b'zulip!')
 
         result = self.client_get('/json/export/realm')
@@ -74,7 +75,7 @@ class RealmExportTest(ZulipTestCase):
         export_dict = result.json()['exports']
         self.assertEqual(export_dict[0]['id'], audit_log_entry.id)
         self.assertEqual(export_dict[0]['export_url'],
-                         'https://test-avatar-bucket.s3.amazonaws.com' + path_id)
+                         'https://test-avatar-bucket.s3.amazonaws.com' + export_path)
         self.assertEqual(export_dict[0]['acting_user_id'], admin.id)
         self.assert_length(export_dict,
                            RealmAuditLog.objects.filter(
@@ -123,10 +124,10 @@ class RealmExportTest(ZulipTestCase):
         self.assertEqual(audit_log_entry.acting_user_id, admin.id)
 
         # Test that the file is hosted, and the contents are as expected.
-        path_id = orjson.loads(audit_log_entry.extra_data).get('export_path')
-        response = self.client_get(path_id)
+        export_path = orjson.loads(audit_log_entry.extra_data).get('export_path')
+        response = self.client_get(export_path)
         self.assertEqual(response.status_code, 200)
-        self.assert_url_serves_contents_of_file(path_id, b'zulip!')
+        self.assert_url_serves_contents_of_file(export_path, b'zulip!')
 
         result = self.client_get('/json/export/realm')
         self.assert_json_success(result)
@@ -134,7 +135,7 @@ class RealmExportTest(ZulipTestCase):
         # Test that the export we have is the export we created.
         export_dict = result.json()['exports']
         self.assertEqual(export_dict[0]['id'], audit_log_entry.id)
-        self.assertEqual(export_dict[0]['export_url'], admin.realm.uri + path_id)
+        self.assertEqual(export_dict[0]['export_url'], admin.realm.uri + export_path)
         self.assertEqual(export_dict[0]['acting_user_id'], admin.id)
         self.assert_length(export_dict,
                            RealmAuditLog.objects.filter(
@@ -144,7 +145,7 @@ class RealmExportTest(ZulipTestCase):
         # Finally, delete the file.
         result = self.client_delete(f'/json/export/realm/{audit_log_entry.id}')
         self.assert_json_success(result)
-        response = self.client_get(path_id)
+        response = self.client_get(export_path)
         self.assertEqual(response.status_code, 404)
 
         # Try to delete an export with a `deleted_timestamp` key.

zerver/tests/test_upload.py

@@ -1598,7 +1598,7 @@ class LocalStorageTest(UploadSerializeMixin, ZulipTestCase):
 
         # Delete the tarball.
         with self.assertLogs(level='WARNING') as warn_log:
-            self.assertIsNone(delete_export_tarball('not_a_file'))
+            self.assertIsNone(delete_export_tarball('/not_a_file'))
         self.assertEqual(warn_log.output, [
             'WARNING:root:not_a_file does not exist. Its entry in the database will be removed.'
         ])
@@ -1682,7 +1682,9 @@ class S3Test(ZulipTestCase):
 
         response = self.client_get(uri)
         redirect_url = response['Location']
-        key = urllib.parse.urlparse(redirect_url).path
+        path = urllib.parse.urlparse(redirect_url).path
+        assert path.startswith("/")
+        key = path[1:]
         self.assertEqual(b"zulip!", bucket.Object(key).get()['Body'].read())
 
         # Now try the endpoint that's supposed to return a temporary url for access
@@ -1691,7 +1693,9 @@ class S3Test(ZulipTestCase):
         self.assert_json_success(result)
         data = result.json()
         url_only_url = data['url']
-        key = urllib.parse.urlparse(url_only_url).path
+        path = urllib.parse.urlparse(url_only_url).path
+        assert path.startswith("/")
+        key = path[1:]
         self.assertEqual(b"zulip!", bucket.Object(key).get()['Body'].read())
 
         # Note: Depending on whether the calls happened in the same
@@ -1912,7 +1916,7 @@ class S3Test(ZulipTestCase):
 
         # Delete the tarball.
         with self.assertLogs(level='WARNING') as warn_log:
-            self.assertIsNone(delete_export_tarball('not_a_file'))
+            self.assertIsNone(delete_export_tarball('/not_a_file'))
         self.assertEqual(warn_log.output, [
             'WARNING:root:not_a_file does not exist. Its entry in the database will be removed.'
         ])