diff --git a/zerver/lib/upload.py b/zerver/lib/upload.py index f1a36425b9..35492d569c 100644 --- a/zerver/lib/upload.py +++ b/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 = 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) diff --git a/zerver/tests/test_realm_export.py b/zerver/tests/test_realm_export.py index 6accd7bb63..84d3e918b6 100644 --- a/zerver/tests/test_realm_export.py +++ b/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. diff --git a/zerver/tests/test_upload.py b/zerver/tests/test_upload.py index c6006e9a02..ff8da24864 100644 --- a/zerver/tests/test_upload.py +++ b/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.' ])