diff --git a/androidshared/src/main/java/org/odk/collect/androidshared/utils/PathUtils.kt b/androidshared/src/main/java/org/odk/collect/androidshared/utils/PathUtils.kt index 2f2be8c598c..96975b681e3 100644 --- a/androidshared/src/main/java/org/odk/collect/androidshared/utils/PathUtils.kt +++ b/androidshared/src/main/java/org/odk/collect/androidshared/utils/PathUtils.kt @@ -1,5 +1,6 @@ package org.odk.collect.androidshared.utils +import org.odk.collect.shared.files.FileExt.sanitizedCanonicalPath import timber.log.Timber import java.io.File @@ -9,8 +10,8 @@ object PathUtils { val absoluteFilePath = if (filePath.startsWith(dirPath)) filePath else dirPath + File.separator + filePath - val canonicalAbsoluteFilePath = File(absoluteFilePath).canonicalPath - val canonicalDirPath = File(dirPath).canonicalPath + val canonicalAbsoluteFilePath = File(absoluteFilePath).sanitizedCanonicalPath() + val canonicalDirPath = File(dirPath).sanitizedCanonicalPath() if (!canonicalAbsoluteFilePath.startsWith(canonicalDirPath)) { Timber.e( "Attempt to access file outside of Collect directory:\n" + diff --git a/collect_app/src/androidTest/java/org/odk/collect/android/support/rules/ResetStateRule.kt b/collect_app/src/androidTest/java/org/odk/collect/android/support/rules/ResetStateRule.kt index b4134f0fa14..040e6ecfdc4 100644 --- a/collect_app/src/androidTest/java/org/odk/collect/android/support/rules/ResetStateRule.kt +++ b/collect_app/src/androidTest/java/org/odk/collect/android/support/rules/ResetStateRule.kt @@ -14,7 +14,6 @@ import org.odk.collect.androidshared.ui.ToastUtils import org.odk.collect.androidshared.ui.multiclicksafe.MultiClickGuard import org.odk.collect.db.sqlite.DatabaseConnection import org.odk.collect.material.BottomSheetBehavior -import org.odk.collect.shared.files.DirectoryUtils import java.io.IOException private class ResetStateStatement( @@ -43,11 +42,11 @@ private class ResetStateStatement( private fun clearDisk() { try { val internalFilesDir = ApplicationProvider.getApplicationContext().filesDir - DirectoryUtils.deleteDirectory(internalFilesDir) + internalFilesDir.deleteRecursively() val externalFilesDir = ApplicationProvider.getApplicationContext().getExternalFilesDir(null)!! - DirectoryUtils.deleteDirectory(externalFilesDir) + externalFilesDir.deleteRecursively() } catch (e: IOException) { throw RuntimeException(e) } diff --git a/collect_app/src/main/java/org/odk/collect/android/application/initialization/ExistingProjectMigrator.kt b/collect_app/src/main/java/org/odk/collect/android/application/initialization/ExistingProjectMigrator.kt index 40365203e64..04f526e95d3 100644 --- a/collect_app/src/main/java/org/odk/collect/android/application/initialization/ExistingProjectMigrator.kt +++ b/collect_app/src/main/java/org/odk/collect/android/application/initialization/ExistingProjectMigrator.kt @@ -10,7 +10,6 @@ import org.odk.collect.settings.SettingsProvider import org.odk.collect.settings.importing.ProjectDetailsCreator import org.odk.collect.settings.keys.MetaKeys import org.odk.collect.settings.keys.ProjectKeys -import org.odk.collect.shared.files.DirectoryUtils import org.odk.collect.upgrade.Upgrade import java.io.File import java.io.FileNotFoundException @@ -67,7 +66,7 @@ class ExistingProjectMigrator( } try { - DirectoryUtils.deleteDirectory(File(rootDir, ".cache")) + File(rootDir, ".cache").deleteRecursively() } catch (e: Exception) { // ignore } diff --git a/collect_app/src/main/java/org/odk/collect/android/database/forms/DatabaseFormsRepository.java b/collect_app/src/main/java/org/odk/collect/android/database/forms/DatabaseFormsRepository.java index db305178f92..b8088d2be75 100644 --- a/collect_app/src/main/java/org/odk/collect/android/database/forms/DatabaseFormsRepository.java +++ b/collect_app/src/main/java/org/odk/collect/android/database/forms/DatabaseFormsRepository.java @@ -30,7 +30,7 @@ import org.odk.collect.forms.Form; import org.odk.collect.forms.FormsRepository; import org.odk.collect.forms.savepoints.SavepointsRepository; -import org.odk.collect.shared.files.DirectoryUtils; +import org.odk.collect.shared.files.FileExt; import org.odk.collect.shared.strings.Md5; import java.io.File; @@ -302,7 +302,7 @@ private void deleteFilesForForm(Form form) { File mediaDir = new File(form.getFormMediaPath()); if (mediaDir.isDirectory()) { - DirectoryUtils.deleteDirectory(mediaDir); + FileExt.deleteDirectory(mediaDir); } else { mediaDir.delete(); } diff --git a/collect_app/src/main/java/org/odk/collect/android/database/instances/DatabaseInstancesRepository.java b/collect_app/src/main/java/org/odk/collect/android/database/instances/DatabaseInstancesRepository.java index 8c9d33c3a44..10c1e5286fa 100644 --- a/collect_app/src/main/java/org/odk/collect/android/database/instances/DatabaseInstancesRepository.java +++ b/collect_app/src/main/java/org/odk/collect/android/database/instances/DatabaseInstancesRepository.java @@ -11,7 +11,7 @@ import org.odk.collect.android.database.DatabaseConstants; import org.odk.collect.forms.instances.Instance; import org.odk.collect.forms.instances.InstancesRepository; -import org.odk.collect.shared.files.DirectoryUtils; +import org.odk.collect.shared.files.FileExt; import java.io.File; import java.util.ArrayList; @@ -270,7 +270,7 @@ private void update(Long instanceId, ContentValues values) { } private void deleteInstanceFiles(Instance instance) { - DirectoryUtils.deleteDirectory(new File(instance.getInstanceFilePath()).getParentFile()); + FileExt.deleteDirectory(new File(instance.getInstanceFilePath()).getParentFile()); } private static List getInstancesFromCursor(Cursor cursor, String instancesPath) { diff --git a/collect_app/src/main/java/org/odk/collect/android/formmanagement/download/ServerFormDownloader.java b/collect_app/src/main/java/org/odk/collect/android/formmanagement/download/ServerFormDownloader.java index 330dbd8ce7c..6c427f934cd 100644 --- a/collect_app/src/main/java/org/odk/collect/android/formmanagement/download/ServerFormDownloader.java +++ b/collect_app/src/main/java/org/odk/collect/android/formmanagement/download/ServerFormDownloader.java @@ -16,7 +16,7 @@ import org.odk.collect.forms.FormSource; import org.odk.collect.forms.FormSourceException; import org.odk.collect.forms.FormsRepository; -import org.odk.collect.shared.files.DirectoryUtils; +import org.odk.collect.shared.files.FileExt; import org.odk.collect.shared.strings.Md5; import java.io.File; @@ -78,7 +78,7 @@ public void downloadForm(ServerFormDetails form, @Nullable ProgressReporter prog } catch (FormSourceException e) { throw new FormDownloadException.FormSourceError(e); } finally { - DirectoryUtils.deleteDirectory(tempDir); + FileExt.deleteDirectory(tempDir); for (Form formToDelete : preExistingFormsWithSameIdAndVersion) { formsRepository.delete(formToDelete.getDbId()); } diff --git a/collect_app/src/main/java/org/odk/collect/android/savepoints/SavepointTask.kt b/collect_app/src/main/java/org/odk/collect/android/savepoints/SavepointTask.kt index 9eca0146b2f..fd053e0d9cc 100644 --- a/collect_app/src/main/java/org/odk/collect/android/savepoints/SavepointTask.kt +++ b/collect_app/src/main/java/org/odk/collect/android/savepoints/SavepointTask.kt @@ -5,7 +5,7 @@ import org.odk.collect.async.Scheduler import org.odk.collect.async.SchedulerAsyncTaskMimic import org.odk.collect.forms.savepoints.Savepoint import org.odk.collect.forms.savepoints.SavepointsRepository -import org.odk.collect.shared.files.FileUtils +import org.odk.collect.shared.files.FileExt.saveToFile import timber.log.Timber import java.io.File @@ -32,7 +32,7 @@ class SavepointTask( val savepoint = Savepoint(formDbId, instanceDbId, savepointFile.absolutePath, formController.getInstanceFile()!!.absolutePath) if (priority == lastPriorityUsed) { - FileUtils.saveToFile(formController.getFilledInFormXml().payloadStream, savepointFile.absolutePath) + savepointFile.saveToFile(formController.getFilledInFormXml().payloadStream) savepointsRepository.save(savepoint) } diff --git a/collect_app/src/main/java/org/odk/collect/android/tasks/SaveFormToDisk.java b/collect_app/src/main/java/org/odk/collect/android/tasks/SaveFormToDisk.java index a07febf6865..867a294a87a 100644 --- a/collect_app/src/main/java/org/odk/collect/android/tasks/SaveFormToDisk.java +++ b/collect_app/src/main/java/org/odk/collect/android/tasks/SaveFormToDisk.java @@ -60,6 +60,7 @@ import org.odk.collect.forms.Form; import org.odk.collect.forms.instances.Instance; import org.odk.collect.forms.instances.InstancesRepository; +import org.odk.collect.shared.files.FileExt; import java.io.File; import java.io.IOException; @@ -333,19 +334,17 @@ private Instance exportData(boolean markCompleted, FormSaver.ProgressListener pr ByteArrayPayload payload = formController.getFilledInFormXml(); // write out xml - String instancePath = formController.getInstanceFile().getAbsolutePath(); - for (String fileName : tempFiles) { mediaUtils.deleteMediaFile(fileName); } progressListener.onProgressUpdate(getLocalizedString(Collect.getInstance(), org.odk.collect.strings.R.string.survey_saving_saving_message)); - writeFile(payload, instancePath); + writeFile(payload, formController.getInstanceFile()); // Write last-saved instance String lastSavedPath = formController.getLastSavedPath(); - writeFile(payload, lastSavedPath); + writeFile(payload, new File(lastSavedPath)); // update the uri. We have exported the reloadable instance, so update status... // Since we saved a reloadable instance, it is flagged as re-openable so that if any error @@ -373,7 +372,7 @@ private Instance exportData(boolean markCompleted, FormSaver.ProgressListener pr progressListener.onProgressUpdate( getLocalizedString(Collect.getInstance(), org.odk.collect.strings.R.string.survey_saving_finalizing_message)); - writeFile(payload, submissionXml.getAbsolutePath()); + writeFile(payload, submissionXml); // see if the form is encrypted and we can encrypt it... EncryptedFormInformation formInfo = EncryptionUtils.getEncryptedFormInformation(uri, formController.getSubmissionMetadata()); @@ -486,7 +485,7 @@ public static void manageFilesAfterSavingEncryptedForm(File instanceXml, File su /** * Writes payload contents to the disk. */ - public static void writeFile(ByteArrayPayload payload, String path) throws IOException { - org.odk.collect.shared.files.FileUtils.saveToFile(payload.getPayloadStream(), path); + public static void writeFile(ByteArrayPayload payload, File file) throws IOException { + FileExt.saveToFile(file, payload.getPayloadStream()); } } diff --git a/forms-test/src/main/java/org/odk/collect/formstest/InMemFormsRepository.java b/forms-test/src/main/java/org/odk/collect/formstest/InMemFormsRepository.java index 05fc83b3766..9ed36152404 100644 --- a/forms-test/src/main/java/org/odk/collect/formstest/InMemFormsRepository.java +++ b/forms-test/src/main/java/org/odk/collect/formstest/InMemFormsRepository.java @@ -5,7 +5,7 @@ import org.odk.collect.forms.Form; import org.odk.collect.forms.FormsRepository; import org.odk.collect.forms.savepoints.SavepointsRepository; -import org.odk.collect.shared.files.DirectoryUtils; +import org.odk.collect.shared.files.FileExt; import org.odk.collect.shared.strings.Md5; import org.odk.collect.shared.TempFiles; @@ -216,7 +216,7 @@ private void deleteFilesForForm(Form form) { File mediaDir = new File(form.getFormMediaPath()); if (mediaDir.isDirectory()) { - DirectoryUtils.deleteDirectory(mediaDir); + FileExt.deleteDirectory(mediaDir); } else { mediaDir.delete(); } diff --git a/forms-test/src/main/java/org/odk/collect/formstest/InMemInstancesRepository.java b/forms-test/src/main/java/org/odk/collect/formstest/InMemInstancesRepository.java index ac71ec2e9ab..b732582cec2 100644 --- a/forms-test/src/main/java/org/odk/collect/formstest/InMemInstancesRepository.java +++ b/forms-test/src/main/java/org/odk/collect/formstest/InMemInstancesRepository.java @@ -2,7 +2,7 @@ import org.odk.collect.forms.instances.Instance; import org.odk.collect.forms.instances.InstancesRepository; -import org.odk.collect.shared.files.DirectoryUtils; +import org.odk.collect.shared.files.FileExt; import java.io.File; import java.util.ArrayList; @@ -192,7 +192,7 @@ public void removeInstanceById(Long databaseId) { private void deleteInstanceFiles(Instance instance) { if (instance.getInstanceFilePath() != null) { - DirectoryUtils.deleteDirectory(new File(instance.getInstanceFilePath()).getParentFile()); + FileExt.deleteDirectory(new File(instance.getInstanceFilePath()).getParentFile()); } } } diff --git a/maps/src/main/java/org/odk/collect/maps/layers/DirectoryReferenceLayerRepository.kt b/maps/src/main/java/org/odk/collect/maps/layers/DirectoryReferenceLayerRepository.kt index 0c848ed4210..fdc4cae9eb7 100644 --- a/maps/src/main/java/org/odk/collect/maps/layers/DirectoryReferenceLayerRepository.kt +++ b/maps/src/main/java/org/odk/collect/maps/layers/DirectoryReferenceLayerRepository.kt @@ -2,7 +2,7 @@ package org.odk.collect.maps.layers import org.odk.collect.maps.MapConfigurator import org.odk.collect.shared.PathUtils -import org.odk.collect.shared.files.DirectoryUtils.listFilesRecursively +import org.odk.collect.shared.files.FileExt.listFilesRecursively import java.io.File class DirectoryReferenceLayerRepository( @@ -41,7 +41,7 @@ class DirectoryReferenceLayerRepository( } private fun getAllFilesWithDirectory() = listOf(sharedLayersDirPath, projectLayersDirPath).flatMap { dir -> - listFilesRecursively(File(dir)).map { file -> + File(dir).listFilesRecursively().map { file -> Pair(file, dir) } } diff --git a/shared/build.gradle.kts b/shared/build.gradle.kts index acf5e8581ba..21d79403009 100644 --- a/shared/build.gradle.kts +++ b/shared/build.gradle.kts @@ -18,6 +18,7 @@ dependencies { testImplementation(Dependencies.junit) testImplementation(Dependencies.hamcrest) + testImplementation(Dependencies.mockito_kotlin) } tasks.register("testDebug") { diff --git a/shared/src/main/java/org/odk/collect/shared/files/DirectoryUtils.kt b/shared/src/main/java/org/odk/collect/shared/files/DirectoryUtils.kt deleted file mode 100644 index c1f7bbabb49..00000000000 --- a/shared/src/main/java/org/odk/collect/shared/files/DirectoryUtils.kt +++ /dev/null @@ -1,23 +0,0 @@ -package org.odk.collect.shared.files - -import java.io.File - -object DirectoryUtils { - - @JvmStatic - fun listFilesRecursively(directory: File): List { - val listFiles = directory.listFiles() ?: emptyArray() - return listFiles.flatMap { - if (it.isDirectory) { - listFilesRecursively(it) - } else { - listOf(it) - } - } - } - - @JvmStatic - fun deleteDirectory(directory: File) { - directory.deleteRecursively() - } -} diff --git a/shared/src/main/java/org/odk/collect/shared/files/FileExt.kt b/shared/src/main/java/org/odk/collect/shared/files/FileExt.kt new file mode 100644 index 00000000000..c13fc06f509 --- /dev/null +++ b/shared/src/main/java/org/odk/collect/shared/files/FileExt.kt @@ -0,0 +1,54 @@ +package org.odk.collect.shared.files + +import java.io.File +import java.io.IOException +import java.io.InputStream +import kotlin.jvm.Throws + +object FileExt { + /** + * Original File.getCanonicalPath() may return paths with inconsistent letter casing for the + * /Android/data/ part of the path, such as /storage/emulated/0/android/data/... or /storage/emulated/0/Android/Data/... + * instead of the expected /storage/emulated/0/Android/data/... + * Since the Android file system is case-sensitive, this behavior appears to be a bug. + * + * For more details, see the discussion on Stack Overflow: + * https://stackoverflow.com/questions/78965720/file-getcanonicalpath-returns-inconsistent-letter-casing-in-path + */ + fun File.sanitizedCanonicalPath(): String { + val androidDataSegment = "/Android/data/" + val regex = Regex(androidDataSegment, RegexOption.IGNORE_CASE) + + return canonicalPath.replace(regex, androidDataSegment) + } + + fun File.listFilesRecursively(): List { + val listFiles = listFiles() ?: emptyArray() + return listFiles.flatMap { + if (it.isDirectory) { + it.listFilesRecursively() + } else { + listOf(it) + } + } + } + + @JvmStatic + fun File.deleteDirectory() { + deleteRecursively() + } + + @Throws(IOException::class) + @JvmStatic + fun File.saveToFile(inputStream: InputStream) { + if (exists() && !delete()) { + throw IOException("Cannot overwrite $absolutePath. Perhaps the file is locked?") + } + + inputStream.use { input -> + outputStream().use { output -> + input.copyTo(output) + } + } + } +} diff --git a/shared/src/main/java/org/odk/collect/shared/files/FileUtils.kt b/shared/src/main/java/org/odk/collect/shared/files/FileUtils.kt deleted file mode 100644 index bf1dc990e19..00000000000 --- a/shared/src/main/java/org/odk/collect/shared/files/FileUtils.kt +++ /dev/null @@ -1,23 +0,0 @@ -package org.odk.collect.shared.files - -import java.io.File -import java.io.IOException -import java.io.InputStream -import kotlin.jvm.Throws - -object FileUtils { - @Throws(IOException::class) - @JvmStatic - fun saveToFile(inputStream: InputStream, filePath: String) { - val file = File(filePath) - if (file.exists() && !file.delete()) { - throw IOException("Cannot overwrite $filePath. Perhaps the file is locked?") - } - - inputStream.use { input -> - file.outputStream().use { output -> - input.copyTo(output) - } - } - } -} diff --git a/shared/src/test/java/org/odk/collect/shared/files/DirectoryUtilsTest.kt b/shared/src/test/java/org/odk/collect/shared/files/FileExtTest.kt similarity index 51% rename from shared/src/test/java/org/odk/collect/shared/files/DirectoryUtilsTest.kt rename to shared/src/test/java/org/odk/collect/shared/files/FileExtTest.kt index f16b0aa8250..680ddcf4d47 100644 --- a/shared/src/test/java/org/odk/collect/shared/files/DirectoryUtilsTest.kt +++ b/shared/src/test/java/org/odk/collect/shared/files/FileExtTest.kt @@ -4,14 +4,63 @@ import org.hamcrest.MatcherAssert.assertThat import org.hamcrest.Matchers.containsInAnyOrder import org.hamcrest.Matchers.equalTo import org.junit.Test +import org.mockito.Mockito.mock +import org.mockito.kotlin.whenever import org.odk.collect.shared.TempFiles +import org.odk.collect.shared.files.FileExt.listFilesRecursively +import org.odk.collect.shared.files.FileExt.sanitizedCanonicalPath +import org.odk.collect.shared.files.FileExt.saveToFile import java.io.File -class DirectoryUtilsTest { +class FileExtTest { + @Test + fun `sanitizedCanonicalPath returns sanitized canonicalPath if androidData path segment is incorrect`() { + val file = mock().apply { + whenever(canonicalPath).thenReturn("/storage/emulated/0/android/Data/org.odk.collect.android/files/projects/DEMO/blah") + } + + assertThat(file.sanitizedCanonicalPath(), equalTo("/storage/emulated/0/Android/data/org.odk.collect.android/files/projects/DEMO/blah")) + } + + @Test + fun `sanitizedCanonicalPath returns original canonicalPath if androidData path segment is correct`() { + val file = mock().apply { + whenever(canonicalPath).thenReturn("/storage/emulated/0/Android/data/org.odk.collect.android/files/projects/DEMO/blah") + } + + assertThat(file.sanitizedCanonicalPath(), equalTo(file.canonicalPath)) + } + + @Test + fun `sanitizedCanonicalPath returns original canonicalPath if other part of the path than the androidData is incorrect`() { + val file = mock().apply { + whenever(canonicalPath).thenReturn("/Storage/Emulated/0/Android/data/org.odk.collect.android/files/projects/DEMO/blah") + } + + assertThat(file.sanitizedCanonicalPath(), equalTo(file.canonicalPath)) + } + + @Test + fun `sanitizedCanonicalPath returns original canonicalPath if it does not contain the androidData part`() { + val file = mock().apply { + whenever(canonicalPath).thenReturn("/data/data/DEMO/files/blah") + } + + assertThat(file.sanitizedCanonicalPath(), equalTo(file.canonicalPath)) + } + + @Test + fun `saveToFile saves data to file`() { + val file = TempFiles.createTempFile().apply { + saveToFile("blah".byteInputStream()) + } + + assertThat(file.readText(), equalTo("blah")) + } @Test fun listFilesRecursively_withEmptyDirectory_returnsEmptyList() { - val files = DirectoryUtils.listFilesRecursively(TempFiles.createTempDir()) + val files = TempFiles.createTempDir().listFilesRecursively() assertThat(files.size, equalTo(0)) } @@ -21,7 +70,7 @@ class DirectoryUtilsTest { val file1 = TempFiles.createTempFile(directory, "blah1", ".txt") val file2 = TempFiles.createTempFile(directory, "blah2", ".txt") - val files = DirectoryUtils.listFilesRecursively(directory) + val files = directory.listFilesRecursively() assertThat(files, containsInAnyOrder(file1, file2)) } @@ -32,7 +81,7 @@ class DirectoryUtilsTest { val file2 = TempFiles.createTempFile(directory, "blah2", ".txt") File(directory, "blah").mkdir() - val files = DirectoryUtils.listFilesRecursively(directory) + val files = directory.listFilesRecursively() assertThat(files, containsInAnyOrder(file1, file2)) } @@ -45,7 +94,7 @@ class DirectoryUtilsTest { val directory2 = File(directory1, "blah").also { it.mkdir() } val file3 = TempFiles.createTempFile(directory2, "blah3", ".txt") - val files = DirectoryUtils.listFilesRecursively(directory1) + val files = directory1.listFilesRecursively() assertThat(files, containsInAnyOrder(file1, file2, file3)) } @@ -61,14 +110,14 @@ class DirectoryUtilsTest { val directory3 = File(directory2, "blah").also { it.mkdir() } val file4 = TempFiles.createTempFile(directory3, "blah4", ".txt") - val files = DirectoryUtils.listFilesRecursively(directory1) + val files = directory1.listFilesRecursively() assertThat(files, containsInAnyOrder(file1, file2, file3, file4)) } @Test fun listFilesRecursively_whenDirectoryDoesNotExist_returnsEmptyList() { val directory = File(TempFiles.getPathInTempDir()) - val files = DirectoryUtils.listFilesRecursively(directory) + val files = directory.listFilesRecursively() assertThat(files.size, equalTo(0)) } }