From 0d21126530443cf967caabacba1b557f8a96ab2b Mon Sep 17 00:00:00 2001
From: Torsten Grote <t@grobox.de>
Date: Wed, 20 Oct 2021 13:30:49 -0300
Subject: [PATCH] Address review feedback for FileManager
---
.../briarproject/mailbox/android/AppModule.kt | 4 +-
.../briarproject/mailbox/cli/JavaCliModule.kt | 4 +-
.../mailbox/core/files/FileManager.kt | 17 +++
.../mailbox/core/files/FileProvider.kt | 1 +
.../mailbox/core/TestComponent.kt | 2 +
.../briarproject/mailbox/core/TestModule.kt | 4 +-
.../core/files/FileManagerIntegrationTest.kt | 106 +++++++++---------
.../mailbox/core/files/FileManagerTest.kt | 5 +-
8 files changed, 81 insertions(+), 62 deletions(-)
diff --git a/mailbox-android/src/main/java/org/briarproject/mailbox/android/AppModule.kt b/mailbox-android/src/main/java/org/briarproject/mailbox/android/AppModule.kt
index d7cee665..1e1469a6 100644
--- a/mailbox-android/src/main/java/org/briarproject/mailbox/android/AppModule.kt
+++ b/mailbox-android/src/main/java/org/briarproject/mailbox/android/AppModule.kt
@@ -32,10 +32,10 @@ internal class AppModule {
@Provides
fun provideFileProvider(app: Application) = object : FileProvider {
private val tempFilesDir = File(app.applicationContext.cacheDir, "tmp").also { it.mkdirs() }
- private val filesDir = app.applicationContext.getDir("folders", MODE_PRIVATE)
+ override val folderRoot = app.applicationContext.getDir("folders", MODE_PRIVATE)
override fun getTemporaryFile(fileId: String) = File(tempFilesDir, fileId)
- override fun getFolder(folderId: String) = File(filesDir, folderId).also { it.mkdirs() }
+ override fun getFolder(folderId: String) = File(folderRoot, folderId).also { it.mkdirs() }
override fun getFile(folderId: String, fileId: String) = File(getFolder(folderId), fileId)
}
}
diff --git a/mailbox-cli/src/main/java/org/briarproject/mailbox/cli/JavaCliModule.kt b/mailbox-cli/src/main/java/org/briarproject/mailbox/cli/JavaCliModule.kt
index 18263ad2..e9d7cb74 100644
--- a/mailbox-cli/src/main/java/org/briarproject/mailbox/cli/JavaCliModule.kt
+++ b/mailbox-cli/src/main/java/org/briarproject/mailbox/cli/JavaCliModule.kt
@@ -86,10 +86,10 @@ internal class JavaCliModule {
@Provides
fun provideFileProvider() = object : FileProvider {
private val tempFilesDir = File(dataDir, "tmp").also { it.mkdirs() }
- private val filesDir = File(dataDir, "folders").also { it.mkdirs() }
+ override val folderRoot = File(dataDir, "folders").also { it.mkdirs() }
override fun getTemporaryFile(fileId: String) = File(tempFilesDir, fileId)
- override fun getFolder(folderId: String) = File(filesDir, folderId).also { it.mkdirs() }
+ override fun getFolder(folderId: String) = File(folderRoot, folderId).also { it.mkdirs() }
override fun getFile(folderId: String, fileId: String) = File(getFolder(folderId), fileId)
}
diff --git a/mailbox-core/src/main/java/org/briarproject/mailbox/core/files/FileManager.kt b/mailbox-core/src/main/java/org/briarproject/mailbox/core/files/FileManager.kt
index 6e7aac65..f38e3af8 100644
--- a/mailbox-core/src/main/java/org/briarproject/mailbox/core/files/FileManager.kt
+++ b/mailbox-core/src/main/java/org/briarproject/mailbox/core/files/FileManager.kt
@@ -14,8 +14,11 @@ import org.briarproject.mailbox.core.server.AuthManager
import org.briarproject.mailbox.core.server.MailboxPrincipal
import org.briarproject.mailbox.core.system.InvalidIdException
import org.briarproject.mailbox.core.system.RandomIdManager
+import org.slf4j.LoggerFactory.getLogger
import javax.inject.Inject
+private val LOG = getLogger(FileManager::class.java)
+
class FileManager @Inject constructor(
private val db: Database,
private val authManager: AuthManager,
@@ -134,6 +137,20 @@ class FileManager @Inject constructor(
}
call.respond(folderListResponse)
}
+
+ fun deleteAllFiles(): Boolean {
+ var allDeleted = true
+ fileProvider.folderRoot.listFiles()?.forEach { folder ->
+ if (!folder.deleteRecursively()) {
+ allDeleted = false
+ LOG.warn("Not everything in $folder could get deleted.")
+ }
+ } ?: run {
+ allDeleted = false
+ LOG.warn("Could not delete folders.")
+ }
+ return allDeleted
+ }
}
data class FileListResponse(val files: List<FileResponse>)
diff --git a/mailbox-core/src/main/java/org/briarproject/mailbox/core/files/FileProvider.kt b/mailbox-core/src/main/java/org/briarproject/mailbox/core/files/FileProvider.kt
index 7c01845b..58ff7c0d 100644
--- a/mailbox-core/src/main/java/org/briarproject/mailbox/core/files/FileProvider.kt
+++ b/mailbox-core/src/main/java/org/briarproject/mailbox/core/files/FileProvider.kt
@@ -3,6 +3,7 @@ package org.briarproject.mailbox.core.files
import java.io.File
interface FileProvider {
+ val folderRoot: File
fun getTemporaryFile(fileId: String): File
fun getFolder(folderId: String): File
fun getFile(folderId: String, fileId: String): File
diff --git a/mailbox-core/src/test/java/org/briarproject/mailbox/core/TestComponent.kt b/mailbox-core/src/test/java/org/briarproject/mailbox/core/TestComponent.kt
index a2127787..c374303c 100644
--- a/mailbox-core/src/test/java/org/briarproject/mailbox/core/TestComponent.kt
+++ b/mailbox-core/src/test/java/org/briarproject/mailbox/core/TestComponent.kt
@@ -2,6 +2,7 @@ package org.briarproject.mailbox.core
import dagger.Component
import org.briarproject.mailbox.core.db.Database
+import org.briarproject.mailbox.core.files.FileManager
import org.briarproject.mailbox.core.lifecycle.LifecycleManager
import org.briarproject.mailbox.core.settings.SettingsManager
import org.briarproject.mailbox.core.system.RandomIdManager
@@ -17,6 +18,7 @@ interface TestComponent {
fun injectCoreEagerSingletons(): CoreEagerSingletons
fun getLifecycleManager(): LifecycleManager
fun getSettingsManager(): SettingsManager
+ fun getFileManager(): FileManager
fun getDatabase(): Database
fun getRandomIdManager(): RandomIdManager
}
diff --git a/mailbox-core/src/test/java/org/briarproject/mailbox/core/TestModule.kt b/mailbox-core/src/test/java/org/briarproject/mailbox/core/TestModule.kt
index 571adf9d..9b270b45 100644
--- a/mailbox-core/src/test/java/org/briarproject/mailbox/core/TestModule.kt
+++ b/mailbox-core/src/test/java/org/briarproject/mailbox/core/TestModule.kt
@@ -41,10 +41,10 @@ internal class TestModule(private val tempDir: File) {
@Provides
fun provideFileProvider() = object : FileProvider {
private val tempFilesDir = File(tempDir, "tmp").also { it.mkdirs() }
- private val filesDir = File(tempDir, "folders")
+ override val folderRoot = File(tempDir, "folders")
override fun getTemporaryFile(fileId: String) = File(tempFilesDir, fileId)
- override fun getFolder(folderId: String) = File(filesDir, folderId).also { it.mkdirs() }
+ override fun getFolder(folderId: String) = File(folderRoot, folderId).also { it.mkdirs() }
override fun getFile(folderId: String, fileId: String) = File(getFolder(folderId), fileId)
}
}
diff --git a/mailbox-core/src/test/java/org/briarproject/mailbox/core/files/FileManagerIntegrationTest.kt b/mailbox-core/src/test/java/org/briarproject/mailbox/core/files/FileManagerIntegrationTest.kt
index ad2363bf..ef5cd616 100644
--- a/mailbox-core/src/test/java/org/briarproject/mailbox/core/files/FileManagerIntegrationTest.kt
+++ b/mailbox-core/src/test/java/org/briarproject/mailbox/core/files/FileManagerIntegrationTest.kt
@@ -11,14 +11,12 @@ import io.ktor.http.HttpStatusCode
import kotlinx.coroutines.runBlocking
import org.briarproject.mailbox.core.TestUtils.getNewRandomId
import org.briarproject.mailbox.core.server.IntegrationTest
+import org.junit.jupiter.api.AfterEach
import org.junit.jupiter.api.Assertions.assertArrayEquals
import org.junit.jupiter.api.Test
-import org.junit.jupiter.api.TestInstance
-import org.junit.jupiter.api.TestInstance.Lifecycle
import kotlin.random.Random
import kotlin.test.assertEquals
-@TestInstance(Lifecycle.PER_CLASS)
class FileManagerIntegrationTest : IntegrationTest() {
private val bytes = Random.nextBytes(2048)
@@ -29,13 +27,18 @@ class FileManagerIntegrationTest : IntegrationTest() {
addContact(contact2)
}
+ @AfterEach
+ fun cleanUp() {
+ testComponent.getFileManager().deleteAllFiles()
+ }
+
@Test
fun `post new file rejects wrong token`(): Unit = runBlocking {
val response: HttpResponse = httpClient.post("$baseUrl/files/${getNewRandomId()}") {
authenticateWithToken(token)
body = bytes
}
- assertEquals(HttpStatusCode.Unauthorized.value, response.status.value)
+ assertEquals(HttpStatusCode.Unauthorized, response.status)
}
@Test
@@ -44,7 +47,7 @@ class FileManagerIntegrationTest : IntegrationTest() {
authenticateWithToken(ownerToken)
body = bytes
}
- assertEquals(HttpStatusCode.Unauthorized.value, response.status.value)
+ assertEquals(HttpStatusCode.Unauthorized, response.status)
}
@Test
@@ -53,7 +56,7 @@ class FileManagerIntegrationTest : IntegrationTest() {
authenticateWithToken(ownerToken)
body = bytes
}
- assertEquals(HttpStatusCode.BadRequest.value, response.status.value)
+ assertEquals(HttpStatusCode.BadRequest, response.status)
assertEquals("Malformed ID: foo", response.readText())
}
@@ -64,13 +67,13 @@ class FileManagerIntegrationTest : IntegrationTest() {
authenticateWithToken(ownerToken)
body = bytes
}
- assertEquals(HttpStatusCode.OK.value, response.status.value)
+ assertEquals(HttpStatusCode.OK, response.status)
// contact can list the file
val listResponse: HttpResponse = httpClient.get("$baseUrl/files/${contact1.inboxId}") {
authenticateWithToken(contact1.token)
}
- assertEquals(HttpStatusCode.OK.value, listResponse.status.value)
+ assertEquals(HttpStatusCode.OK, listResponse.status)
val fileList: FileListResponse = listResponse.receive()
assertEquals(1, fileList.files.size)
@@ -80,7 +83,7 @@ class FileManagerIntegrationTest : IntegrationTest() {
httpClient.get("$baseUrl/files/${contact1.inboxId}/$fileId") {
authenticateWithToken(contact1.token)
}
- assertEquals(HttpStatusCode.OK.value, fileResponse.status.value)
+ assertEquals(HttpStatusCode.OK, fileResponse.status)
assertArrayEquals(bytes, fileResponse.readBytes())
// contact can delete the file
@@ -88,7 +91,7 @@ class FileManagerIntegrationTest : IntegrationTest() {
httpClient.delete("$baseUrl/files/${contact1.inboxId}/$fileId") {
authenticateWithToken(contact1.token)
}
- assertEquals(HttpStatusCode.OK.value, deleteResponse.status.value)
+ assertEquals(HttpStatusCode.OK, deleteResponse.status)
// now the list of files in that folder is empty again
val emptyFileListResponse: FileListResponse =
@@ -103,15 +106,33 @@ class FileManagerIntegrationTest : IntegrationTest() {
val response: HttpResponse = httpClient.get("$baseUrl/files/${getNewRandomId()}") {
authenticateWithToken(token)
}
- assertEquals(HttpStatusCode.Unauthorized.value, response.status.value)
+ assertEquals(HttpStatusCode.Unauthorized, response.status)
+
+ // upload a real file
+ val postResponse: HttpResponse = httpClient.post("$baseUrl/files/${contact1.inboxId}") {
+ authenticateWithToken(ownerToken)
+ body = bytes
+ }
+ assertEquals(HttpStatusCode.OK, postResponse.status)
+
+ // wrong token also gets rejected for real folder
+ val lastResponse: HttpResponse = httpClient.get("$baseUrl/files/${contact1.inboxId}") {
+ authenticateWithToken(token)
+ }
+ assertEquals(HttpStatusCode.Unauthorized, lastResponse.status)
}
@Test
fun `list files rejects unauthorized folder ID`(): Unit = runBlocking {
- val response: HttpResponse = httpClient.get("$baseUrl/files/${contact1.inboxId}") {
+ val response1: HttpResponse = httpClient.get("$baseUrl/files/${contact1.inboxId}") {
authenticateWithToken(ownerToken)
}
- assertEquals(HttpStatusCode.Unauthorized.value, response.status.value)
+ assertEquals(HttpStatusCode.Unauthorized, response1.status)
+
+ val response2: HttpResponse = httpClient.get("$baseUrl/files/${contact1.inboxId}") {
+ authenticateWithToken(contact2.token)
+ }
+ assertEquals(HttpStatusCode.Unauthorized, response2.status)
}
@Test
@@ -119,7 +140,7 @@ class FileManagerIntegrationTest : IntegrationTest() {
val response: HttpResponse = httpClient.get("$baseUrl/files/foo") {
authenticateWithToken(ownerToken)
}
- assertEquals(HttpStatusCode.BadRequest.value, response.status.value)
+ assertEquals(HttpStatusCode.BadRequest, response.status)
assertEquals("Malformed ID: foo", response.readText())
}
@@ -128,7 +149,7 @@ class FileManagerIntegrationTest : IntegrationTest() {
val response: HttpResponse = httpClient.get("$baseUrl/files/${contact1.outboxId}") {
authenticateWithToken(ownerToken)
}
- assertEquals(HttpStatusCode.OK.value, response.status.value)
+ assertEquals(HttpStatusCode.OK, response.status)
assertEquals("""{"files":[]}""", response.readText())
}
@@ -138,7 +159,7 @@ class FileManagerIntegrationTest : IntegrationTest() {
httpClient.get("$baseUrl/files/${getNewRandomId()}/${getNewRandomId()}") {
authenticateWithToken(token)
}
- assertEquals(HttpStatusCode.Unauthorized.value, response.status.value)
+ assertEquals(HttpStatusCode.Unauthorized, response.status)
}
@Test
@@ -147,7 +168,7 @@ class FileManagerIntegrationTest : IntegrationTest() {
httpClient.get("$baseUrl/files/${contact1.inboxId}/${getNewRandomId()}") {
authenticateWithToken(ownerToken)
}
- assertEquals(HttpStatusCode.Unauthorized.value, response.status.value)
+ assertEquals(HttpStatusCode.Unauthorized, response.status)
}
@Test
@@ -155,7 +176,7 @@ class FileManagerIntegrationTest : IntegrationTest() {
val response: HttpResponse = httpClient.get("$baseUrl/files/foo/${getNewRandomId()}") {
authenticateWithToken(ownerToken)
}
- assertEquals(HttpStatusCode.BadRequest.value, response.status.value)
+ assertEquals(HttpStatusCode.BadRequest, response.status)
assertEquals("Malformed ID: foo", response.readText())
}
@@ -164,7 +185,7 @@ class FileManagerIntegrationTest : IntegrationTest() {
val response: HttpResponse = httpClient.get("$baseUrl/files/${contact1.outboxId}/bar") {
authenticateWithToken(ownerToken)
}
- assertEquals(HttpStatusCode.BadRequest.value, response.status.value)
+ assertEquals(HttpStatusCode.BadRequest, response.status)
assertEquals("Malformed ID: bar", response.readText())
}
@@ -174,7 +195,7 @@ class FileManagerIntegrationTest : IntegrationTest() {
val response: HttpResponse = httpClient.get("$baseUrl/files/${contact1.outboxId}/$id") {
authenticateWithToken(ownerToken)
}
- assertEquals(HttpStatusCode.NotFound.value, response.status.value)
+ assertEquals(HttpStatusCode.NotFound, response.status)
}
@Test
@@ -183,7 +204,7 @@ class FileManagerIntegrationTest : IntegrationTest() {
httpClient.delete("$baseUrl/files/${getNewRandomId()}/${getNewRandomId()}") {
authenticateWithToken(token)
}
- assertEquals(HttpStatusCode.Unauthorized.value, response.status.value)
+ assertEquals(HttpStatusCode.Unauthorized, response.status)
}
@Test
@@ -192,7 +213,7 @@ class FileManagerIntegrationTest : IntegrationTest() {
httpClient.delete("$baseUrl/files/${contact1.inboxId}/${getNewRandomId()}") {
authenticateWithToken(ownerToken)
}
- assertEquals(HttpStatusCode.Unauthorized.value, response.status.value)
+ assertEquals(HttpStatusCode.Unauthorized, response.status)
}
@Test
@@ -200,7 +221,7 @@ class FileManagerIntegrationTest : IntegrationTest() {
val response: HttpResponse = httpClient.delete("$baseUrl/files/foo/${getNewRandomId()}") {
authenticateWithToken(ownerToken)
}
- assertEquals(HttpStatusCode.BadRequest.value, response.status.value)
+ assertEquals(HttpStatusCode.BadRequest, response.status)
assertEquals("Malformed ID: foo", response.readText())
}
@@ -209,7 +230,7 @@ class FileManagerIntegrationTest : IntegrationTest() {
val response: HttpResponse = httpClient.delete("$baseUrl/files/${contact1.outboxId}/bar") {
authenticateWithToken(ownerToken)
}
- assertEquals(HttpStatusCode.BadRequest.value, response.status.value)
+ assertEquals(HttpStatusCode.BadRequest, response.status)
assertEquals("Malformed ID: bar", response.readText())
}
@@ -219,7 +240,7 @@ class FileManagerIntegrationTest : IntegrationTest() {
val response: HttpResponse = httpClient.delete("$baseUrl/files/${contact1.outboxId}/$id") {
authenticateWithToken(ownerToken)
}
- assertEquals(HttpStatusCode.NotFound.value, response.status.value)
+ assertEquals(HttpStatusCode.NotFound, response.status)
}
@Test
@@ -227,7 +248,7 @@ class FileManagerIntegrationTest : IntegrationTest() {
val response: HttpResponse = httpClient.get("$baseUrl/folders") {
authenticateWithToken(contact1.token)
}
- assertEquals(HttpStatusCode.Unauthorized.value, response.status.value)
+ assertEquals(HttpStatusCode.Unauthorized, response.status)
}
@Test
@@ -235,7 +256,7 @@ class FileManagerIntegrationTest : IntegrationTest() {
val response: HttpResponse = httpClient.get("$baseUrl/folders") {
authenticateWithToken(ownerToken)
}
- assertEquals(HttpStatusCode.OK.value, response.status.value)
+ assertEquals(HttpStatusCode.OK, response.status)
assertEquals("""{"folders":[]}""", response.readText())
}
@@ -246,43 +267,20 @@ class FileManagerIntegrationTest : IntegrationTest() {
authenticateWithToken(contact1.token)
body = bytes
}
- assertEquals(HttpStatusCode.OK.value, response1.status.value)
+ assertEquals(HttpStatusCode.OK, response1.status)
// contact2 uploads a file
val response2: HttpResponse = httpClient.post("$baseUrl/files/${contact2.outboxId}") {
authenticateWithToken(contact2.token)
body = bytes
}
- assertEquals(HttpStatusCode.OK.value, response2.status.value)
+ assertEquals(HttpStatusCode.OK, response2.status)
// owner now sees both contacts' outboxes in folders list
val folderListResponse: FolderListResponse = httpClient.get("$baseUrl/folders") {
authenticateWithToken(ownerToken)
}
- val folderList =
- listOf(FolderResponse(contact1.outboxId), FolderResponse(contact2.outboxId))
- assertEquals(FolderListResponse(folderList), folderListResponse)
-
- // get the file IDs to be able to delete them to not interfere with other tests
- val fileListResponse1: FileListResponse =
- httpClient.get("$baseUrl/files/${contact1.outboxId}") {
- authenticateWithToken(ownerToken)
- }
- val id1 = fileListResponse1.files[0].name
- val deleteResponse1: HttpResponse =
- httpClient.delete("$baseUrl/files/${contact1.outboxId}/$id1") {
- authenticateWithToken(ownerToken)
- }
- assertEquals(HttpStatusCode.OK.value, deleteResponse1.status.value)
- val fileListResponse2: FileListResponse =
- httpClient.get("$baseUrl/files/${contact2.outboxId}") {
- authenticateWithToken(ownerToken)
- }
- val id2 = fileListResponse2.files[0].name
- val deleteResponse2: HttpResponse =
- httpClient.delete("$baseUrl/files/${contact2.outboxId}/$id2") {
- authenticateWithToken(ownerToken)
- }
- assertEquals(HttpStatusCode.OK.value, deleteResponse2.status.value)
+ val folderList = setOf(FolderResponse(contact1.outboxId), FolderResponse(contact2.outboxId))
+ assertEquals(folderList, folderListResponse.folders.toSet())
}
}
diff --git a/mailbox-core/src/test/java/org/briarproject/mailbox/core/files/FileManagerTest.kt b/mailbox-core/src/test/java/org/briarproject/mailbox/core/files/FileManagerTest.kt
index 638ed6af..49cba822 100644
--- a/mailbox-core/src/test/java/org/briarproject/mailbox/core/files/FileManagerTest.kt
+++ b/mailbox-core/src/test/java/org/briarproject/mailbox/core/files/FileManagerTest.kt
@@ -16,6 +16,7 @@ import org.briarproject.mailbox.core.TestUtils.getNewRandomId
import org.briarproject.mailbox.core.db.Database
import org.briarproject.mailbox.core.server.AuthManager
import org.briarproject.mailbox.core.server.MailboxPrincipal
+import org.briarproject.mailbox.core.server.MailboxPrincipal.OwnerPrincipal
import org.briarproject.mailbox.core.system.RandomIdManager
import org.junit.jupiter.api.Assertions.assertArrayEquals
import org.junit.jupiter.api.Test
@@ -50,8 +51,8 @@ class FileManagerTest {
val tmpFile = File(tempDir, "tmp")
val finalFile = File(tempDir, "final")
- every { call.principal<MailboxPrincipal>() } returns MailboxPrincipal.OwnerPrincipal
- every { authManager.assertCanPostToFolder(MailboxPrincipal.OwnerPrincipal, id) } just Runs
+ every { call.principal<MailboxPrincipal>() } returns OwnerPrincipal
+ every { authManager.assertCanPostToFolder(OwnerPrincipal, id) } just Runs
every { fileProvider.getTemporaryFile(any()) } returns tmpFile
coEvery { call.receiveStream() } returns ByteArrayInputStream(bytes)
every { fileProvider.getFile(id, any()) } returns finalFile
--
GitLab