Address review feedback for FileManager

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)
     }
 }

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)
     }
 

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>)

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

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
 }

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)
     }
 }

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())
     }
 }

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