From 5bff6b6578e2ae7b6ab93c43d9a0d19f4bce2230 Mon Sep 17 00:00:00 2001 From: Callum Stott Date: Tue, 3 Dec 2024 11:41:43 +0000 Subject: [PATCH 1/3] Merge pull request #6538 from grzesiek2010/COLLECT-6537 Fixed the filtering of duplicate columns --- .../database/entities/DatabaseEntitiesRepository.kt | 2 +- .../android/entities/EntitiesRepositoryTest.kt | 11 ++++++++++- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/collect_app/src/main/java/org/odk/collect/android/database/entities/DatabaseEntitiesRepository.kt b/collect_app/src/main/java/org/odk/collect/android/database/entities/DatabaseEntitiesRepository.kt index 4d89d544110..33f8b08fe4d 100644 --- a/collect_app/src/main/java/org/odk/collect/android/database/entities/DatabaseEntitiesRepository.kt +++ b/collect_app/src/main/java/org/odk/collect/android/database/entities/DatabaseEntitiesRepository.kt @@ -382,8 +382,8 @@ class DatabaseEntitiesRepository(context: Context, dbPath: String) : EntitiesRep val missingColumns = entity.properties .map { EntitiesTable.getPropertyColumn(it.first) } - .filterNot { columnNames.contains(it) } .distinctBy { it.lowercase() } + .filterNot { columnNames.contains(it) } if (missingColumns.isNotEmpty()) { databaseConnection.resetTransaction { diff --git a/collect_app/src/test/java/org/odk/collect/android/entities/EntitiesRepositoryTest.kt b/collect_app/src/test/java/org/odk/collect/android/entities/EntitiesRepositoryTest.kt index 759bbc87fa0..001049a6489 100644 --- a/collect_app/src/test/java/org/odk/collect/android/entities/EntitiesRepositoryTest.kt +++ b/collect_app/src/test/java/org/odk/collect/android/entities/EntitiesRepositoryTest.kt @@ -745,7 +745,16 @@ abstract class EntitiesRepositoryTest { ) repository.save("things", entity) - val savedEntities = repository.getEntities("things") + var savedEntities = repository.getEntities("things") + assertThat(savedEntities[0].properties.size, equalTo(1)) + assertThat(savedEntities[0].properties[0].first, equalTo("prop")) + + /** + * Attempt to save again to ensure that duplicate properties are correctly compared, not only + * within the current entity list, but also against properties already saved in the database. + */ + repository.save("things", entity) + savedEntities = repository.getEntities("things") assertThat(savedEntities[0].properties.size, equalTo(1)) assertThat(savedEntities[0].properties[0].first, equalTo("prop")) } From 264f3f179cd3802f2e72c39d871ee6ae2153301a Mon Sep 17 00:00:00 2001 From: Callum Stott Date: Fri, 6 Dec 2024 08:42:47 +0000 Subject: [PATCH 2/3] Merge pull request #6540 from grzesiek2010/COLLECT-6364_final Throw an exception when there is an attempt to access file outside of Collect directory --- .../collect/androidshared/utils/PathUtils.kt | 27 ------------------- .../androidshared/utils/PathUtilsTest.kt | 1 + .../android/database/DatabaseObjectMapper.kt | 2 +- .../DatabaseSavepointsRepository.kt | 2 +- .../fastexternalitemset/ItemsetDbAdapter.java | 2 +- .../download/ServerFormDownloaderTest.java | 2 +- .../java/org/odk/collect/shared/PathUtils.kt | 15 +++++++++++ 7 files changed, 20 insertions(+), 31 deletions(-) delete mode 100644 androidshared/src/main/java/org/odk/collect/androidshared/utils/PathUtils.kt 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 deleted file mode 100644 index 96975b681e3..00000000000 --- a/androidshared/src/main/java/org/odk/collect/androidshared/utils/PathUtils.kt +++ /dev/null @@ -1,27 +0,0 @@ -package org.odk.collect.androidshared.utils - -import org.odk.collect.shared.files.FileExt.sanitizedCanonicalPath -import timber.log.Timber -import java.io.File - -object PathUtils { - @JvmStatic - fun getAbsoluteFilePath(dirPath: String, filePath: String): String { - val absoluteFilePath = - if (filePath.startsWith(dirPath)) filePath else dirPath + File.separator + filePath - - 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" + - "dirPath: $dirPath\n" + - "filePath: $filePath\n" + - "absoluteFilePath: $absoluteFilePath\n" + - "canonicalAbsoluteFilePath: $canonicalAbsoluteFilePath\n" + - "canonicalDirPath: $canonicalDirPath" - ) - } - return absoluteFilePath - } -} diff --git a/androidshared/src/test/java/org/odk/collect/androidshared/utils/PathUtilsTest.kt b/androidshared/src/test/java/org/odk/collect/androidshared/utils/PathUtilsTest.kt index 472b285027d..27bad2a3d8a 100644 --- a/androidshared/src/test/java/org/odk/collect/androidshared/utils/PathUtilsTest.kt +++ b/androidshared/src/test/java/org/odk/collect/androidshared/utils/PathUtilsTest.kt @@ -3,6 +3,7 @@ package org.odk.collect.androidshared.utils import org.hamcrest.MatcherAssert.assertThat import org.hamcrest.Matchers.equalTo import org.junit.Test +import org.odk.collect.shared.PathUtils import org.odk.collect.shared.TempFiles import java.io.File diff --git a/collect_app/src/main/java/org/odk/collect/android/database/DatabaseObjectMapper.kt b/collect_app/src/main/java/org/odk/collect/android/database/DatabaseObjectMapper.kt index f24e3e632a4..53cc4442eb4 100644 --- a/collect_app/src/main/java/org/odk/collect/android/database/DatabaseObjectMapper.kt +++ b/collect_app/src/main/java/org/odk/collect/android/database/DatabaseObjectMapper.kt @@ -5,9 +5,9 @@ import android.database.Cursor import android.provider.BaseColumns import org.odk.collect.android.database.forms.DatabaseFormColumns import org.odk.collect.android.database.instances.DatabaseInstanceColumns -import org.odk.collect.androidshared.utils.PathUtils.getAbsoluteFilePath import org.odk.collect.forms.Form import org.odk.collect.forms.instances.Instance +import org.odk.collect.shared.PathUtils.getAbsoluteFilePath import org.odk.collect.shared.PathUtils.getRelativeFilePath import java.lang.Boolean diff --git a/collect_app/src/main/java/org/odk/collect/android/database/savepoints/DatabaseSavepointsRepository.kt b/collect_app/src/main/java/org/odk/collect/android/database/savepoints/DatabaseSavepointsRepository.kt index 6b126b04acd..363f2c0f779 100644 --- a/collect_app/src/main/java/org/odk/collect/android/database/savepoints/DatabaseSavepointsRepository.kt +++ b/collect_app/src/main/java/org/odk/collect/android/database/savepoints/DatabaseSavepointsRepository.kt @@ -8,7 +8,6 @@ import org.odk.collect.android.database.DatabaseConstants.SAVEPOINTS_DATABASE_VE import org.odk.collect.android.database.DatabaseConstants.SAVEPOINTS_TABLE_NAME import org.odk.collect.android.database.savepoints.DatabaseSavepointsColumns.FORM_DB_ID import org.odk.collect.android.database.savepoints.DatabaseSavepointsColumns.INSTANCE_DB_ID -import org.odk.collect.androidshared.utils.PathUtils.getAbsoluteFilePath import org.odk.collect.db.sqlite.CursorExt.foldAndClose import org.odk.collect.db.sqlite.DatabaseConnection import org.odk.collect.db.sqlite.SQLiteDatabaseExt.delete @@ -16,6 +15,7 @@ import org.odk.collect.db.sqlite.SQLiteDatabaseExt.query import org.odk.collect.forms.savepoints.Savepoint import org.odk.collect.forms.savepoints.SavepointsRepository import org.odk.collect.shared.PathUtils +import org.odk.collect.shared.PathUtils.getAbsoluteFilePath import java.io.File class DatabaseSavepointsRepository( diff --git a/collect_app/src/main/java/org/odk/collect/android/fastexternalitemset/ItemsetDbAdapter.java b/collect_app/src/main/java/org/odk/collect/android/fastexternalitemset/ItemsetDbAdapter.java index e19bfa8f1e1..c540c146362 100644 --- a/collect_app/src/main/java/org/odk/collect/android/fastexternalitemset/ItemsetDbAdapter.java +++ b/collect_app/src/main/java/org/odk/collect/android/fastexternalitemset/ItemsetDbAdapter.java @@ -1,6 +1,6 @@ package org.odk.collect.android.fastexternalitemset; -import static org.odk.collect.androidshared.utils.PathUtils.getAbsoluteFilePath; +import static org.odk.collect.shared.PathUtils.getAbsoluteFilePath; import android.content.ContentValues; import android.database.Cursor; diff --git a/collect_app/src/test/java/org/odk/collect/android/formmanagement/download/ServerFormDownloaderTest.java b/collect_app/src/test/java/org/odk/collect/android/formmanagement/download/ServerFormDownloaderTest.java index d83730bcc24..8edd9d61180 100644 --- a/collect_app/src/test/java/org/odk/collect/android/formmanagement/download/ServerFormDownloaderTest.java +++ b/collect_app/src/test/java/org/odk/collect/android/formmanagement/download/ServerFormDownloaderTest.java @@ -11,9 +11,9 @@ import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; import static org.odk.collect.android.utilities.FileUtils.read; -import static org.odk.collect.androidshared.utils.PathUtils.getAbsoluteFilePath; import static org.odk.collect.formstest.FormUtils.buildForm; import static org.odk.collect.formstest.FormUtils.createXFormBody; +import static org.odk.collect.shared.PathUtils.getAbsoluteFilePath; import static java.util.Arrays.asList; import com.google.common.io.Files; diff --git a/shared/src/main/java/org/odk/collect/shared/PathUtils.kt b/shared/src/main/java/org/odk/collect/shared/PathUtils.kt index 1f4ba227cc8..102c33e97b6 100644 --- a/shared/src/main/java/org/odk/collect/shared/PathUtils.kt +++ b/shared/src/main/java/org/odk/collect/shared/PathUtils.kt @@ -1,5 +1,8 @@ package org.odk.collect.shared +import org.odk.collect.shared.files.FileExt.sanitizedCanonicalPath +import java.io.File + object PathUtils { @JvmStatic @@ -10,4 +13,16 @@ object PathUtils { // https://stackoverflow.com/questions/2679699/what-characters-allowed-in-file-names-on-android @JvmStatic fun getPathSafeFileName(fileName: String) = fileName.replace("[\"*/:<>?\\\\|]".toRegex(), "_") + + @JvmStatic + fun getAbsoluteFilePath(dirPath: String, filePath: String): String { + val absoluteFilePath = + if (filePath.startsWith(dirPath)) filePath else dirPath + File.separator + filePath + + if (File(absoluteFilePath).sanitizedCanonicalPath().startsWith(File(dirPath).sanitizedCanonicalPath())) { + return absoluteFilePath + } else { + throw SecurityException("Contact support@getodk.org. Attempt to access file outside of Collect directory: $absoluteFilePath") + } + } } From 2ccb27f72d283fd58566169476ef108c1a02db0c Mon Sep 17 00:00:00 2001 From: Callum Stott Date: Mon, 9 Dec 2024 12:55:03 +0000 Subject: [PATCH 3/3] Merge pull request #6543 from grzesiek2010/COLLECT-6537_2 Fixed the filtering of duplicate columns --- .../entities/DatabaseEntitiesRepository.kt | 2 +- .../entities/EntitiesRepositoryTest.kt | 23 ++++++++++++++----- .../storage/InMemEntitiesRepository.kt | 8 ++++--- 3 files changed, 23 insertions(+), 10 deletions(-) diff --git a/collect_app/src/main/java/org/odk/collect/android/database/entities/DatabaseEntitiesRepository.kt b/collect_app/src/main/java/org/odk/collect/android/database/entities/DatabaseEntitiesRepository.kt index 33f8b08fe4d..87d8b39ece6 100644 --- a/collect_app/src/main/java/org/odk/collect/android/database/entities/DatabaseEntitiesRepository.kt +++ b/collect_app/src/main/java/org/odk/collect/android/database/entities/DatabaseEntitiesRepository.kt @@ -383,7 +383,7 @@ class DatabaseEntitiesRepository(context: Context, dbPath: String) : EntitiesRep val missingColumns = entity.properties .map { EntitiesTable.getPropertyColumn(it.first) } .distinctBy { it.lowercase() } - .filterNot { columnNames.contains(it) } + .filterNot { columnName -> columnNames.any { it.equals(columnName, ignoreCase = true) } } if (missingColumns.isNotEmpty()) { databaseConnection.resetTransaction { diff --git a/collect_app/src/test/java/org/odk/collect/android/entities/EntitiesRepositoryTest.kt b/collect_app/src/test/java/org/odk/collect/android/entities/EntitiesRepositoryTest.kt index 001049a6489..6a09b148bc9 100644 --- a/collect_app/src/test/java/org/odk/collect/android/entities/EntitiesRepositoryTest.kt +++ b/collect_app/src/test/java/org/odk/collect/android/entities/EntitiesRepositoryTest.kt @@ -736,7 +736,7 @@ abstract class EntitiesRepositoryTest { } @Test - fun `#save ignores case-insensitive duplicate properties`() { + fun `#save ignores case-insensitive duplicate new properties`() { val repository = buildSubject() val entity = Entity.New( "1", @@ -745,15 +745,26 @@ abstract class EntitiesRepositoryTest { ) repository.save("things", entity) - var savedEntities = repository.getEntities("things") + val savedEntities = repository.getEntities("things") assertThat(savedEntities[0].properties.size, equalTo(1)) assertThat(savedEntities[0].properties[0].first, equalTo("prop")) + } + + @Test + fun `#save ignores case-insensitive duplicate properties if one of them has already been saved`() { + val repository = buildSubject() + val entity = Entity.New( + "1", + "One", + properties = listOf(Pair("prop", "value")) + ) - /** - * Attempt to save again to ensure that duplicate properties are correctly compared, not only - * within the current entity list, but also against properties already saved in the database. - */ repository.save("things", entity) + var savedEntities = repository.getEntities("things") + assertThat(savedEntities[0].properties.size, equalTo(1)) + assertThat(savedEntities[0].properties[0].first, equalTo("prop")) + + repository.save("things", entity.copy(properties = listOf(Pair("Prop", "value")))) savedEntities = repository.getEntities("things") assertThat(savedEntities[0].properties.size, equalTo(1)) assertThat(savedEntities[0].properties[0].first, equalTo("prop")) diff --git a/entities/src/main/java/org/odk/collect/entities/storage/InMemEntitiesRepository.kt b/entities/src/main/java/org/odk/collect/entities/storage/InMemEntitiesRepository.kt index e467df1a104..dc07a81d9b1 100644 --- a/entities/src/main/java/org/odk/collect/entities/storage/InMemEntitiesRepository.kt +++ b/entities/src/main/java/org/odk/collect/entities/storage/InMemEntitiesRepository.kt @@ -121,13 +121,15 @@ class InMemEntitiesRepository : EntitiesRepository { private fun updateLists(list: String, entity: Entity) { lists.add(list) - listProperties.getOrPut(list) { + val properties = listProperties.getOrPut(list) { mutableSetOf() - }.addAll( + } + properties.addAll( entity .properties - .distinctBy { it.first.lowercase() } .map { it.first } + .distinctBy { it.lowercase() } + .filterNot { properties.any { property -> property.equals(it, ignoreCase = true) } } ) }