From 01a53cc3ee6815dba6fe8de0ac53895acbc01453 Mon Sep 17 00:00:00 2001 From: Sunik Kupfer Date: Wed, 6 Nov 2024 15:06:08 +0100 Subject: [PATCH 1/6] Always evaluate read only --- .../davdroid/resource/LocalAddressBook.kt | 45 +++++++++---------- 1 file changed, 22 insertions(+), 23 deletions(-) diff --git a/app/src/main/kotlin/at/bitfire/davdroid/resource/LocalAddressBook.kt b/app/src/main/kotlin/at/bitfire/davdroid/resource/LocalAddressBook.kt index b8579a16a..7a83498ca 100644 --- a/app/src/main/kotlin/at/bitfire/davdroid/resource/LocalAddressBook.kt +++ b/app/src/main/kotlin/at/bitfire/davdroid/resource/LocalAddressBook.kt @@ -175,31 +175,30 @@ open class LocalAddressBook @AssistedInject constructor( settings = contactsProviderSettings // Update force read only - if (forceReadOnly != null) { - val nowReadOnly = forceReadOnly || !info.privWriteContent || info.forceReadOnly - if (nowReadOnly != readOnly) { - logger.info("Address book now read-only = $nowReadOnly, updating contacts") - - // update address book itself - readOnly = nowReadOnly - - // update raw contacts - val rawContactValues = ContentValues(1) - rawContactValues.put(RawContacts.RAW_CONTACT_IS_READ_ONLY, if (nowReadOnly) 1 else 0) - provider!!.update(rawContactsSyncUri(), rawContactValues, null, null) - - // update data rows - val dataValues = ContentValues(1) - dataValues.put(ContactsContract.Data.IS_READ_ONLY, if (nowReadOnly) 1 else 0) - provider!!.update(syncAdapterURI(ContactsContract.Data.CONTENT_URI), dataValues, null, null) - - // update group rows - val groupValues = ContentValues(1) - groupValues.put(Groups.GROUP_IS_READ_ONLY, if (nowReadOnly) 1 else 0) - provider!!.update(groupsSyncUri(), groupValues, null, null) - } + val nowReadOnly = forceReadOnly == true || !info.privWriteContent || info.forceReadOnly + if (nowReadOnly != readOnly) { + logger.info("Address book now read-only = $nowReadOnly, updating contacts") + + // update address book itself + readOnly = nowReadOnly + + // update raw contacts + val rawContactValues = ContentValues(1) + rawContactValues.put(RawContacts.RAW_CONTACT_IS_READ_ONLY, if (nowReadOnly) 1 else 0) + provider!!.update(rawContactsSyncUri(), rawContactValues, null, null) + + // update data rows + val dataValues = ContentValues(1) + dataValues.put(ContactsContract.Data.IS_READ_ONLY, if (nowReadOnly) 1 else 0) + provider!!.update(syncAdapterURI(ContactsContract.Data.CONTENT_URI), dataValues, null, null) + + // update group rows + val groupValues = ContentValues(1) + groupValues.put(Groups.GROUP_IS_READ_ONLY, if (nowReadOnly) 1 else 0) + provider!!.update(groupsSyncUri(), groupValues, null, null) } + // make sure it will still be synchronized when contacts are updated updateSyncFrameworkSettings() } From 53211e83bf6a47e8182c68320814e98794b93396 Mon Sep 17 00:00:00 2001 From: Sunik Kupfer Date: Wed, 6 Nov 2024 15:12:37 +0100 Subject: [PATCH 2/6] Extract read only evaluation to companion method --- .../bitfire/davdroid/resource/LocalAddressBook.kt | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/app/src/main/kotlin/at/bitfire/davdroid/resource/LocalAddressBook.kt b/app/src/main/kotlin/at/bitfire/davdroid/resource/LocalAddressBook.kt index 7a83498ca..ee1c57c41 100644 --- a/app/src/main/kotlin/at/bitfire/davdroid/resource/LocalAddressBook.kt +++ b/app/src/main/kotlin/at/bitfire/davdroid/resource/LocalAddressBook.kt @@ -175,7 +175,7 @@ open class LocalAddressBook @AssistedInject constructor( settings = contactsProviderSettings // Update force read only - val nowReadOnly = forceReadOnly == true || !info.privWriteContent || info.forceReadOnly + val nowReadOnly = shouldBeReadOnly(info, forceReadOnly) if (nowReadOnly != readOnly) { logger.info("Address book now read-only = $nowReadOnly, updating contacts") @@ -431,11 +431,20 @@ open class LocalAddressBook @AssistedInject constructor( addressBook.updateSyncFrameworkSettings() addressBook.settings = contactsProviderSettings - addressBook.readOnly = forceReadOnly || !info.privWriteContent || info.forceReadOnly + addressBook.readOnly = shouldBeReadOnly(info, forceReadOnly) return addressBook } + /** + * Determines whether the address book should be set to read only. + * + * @param forceReadOnly Whether managed setting should overwrite read-only + * @param info Determine read-only flag from collection data + */ + private fun shouldBeReadOnly(info: Collection, forceReadOnly: Boolean? = null,): Boolean = + forceReadOnly == true || !info.privWriteContent || info.forceReadOnly + /** * Finds a [LocalAddressBook] based on its corresponding collection. * From e0aa24a1833fd6ee7b91e2d25304b4ba7e6cae66 Mon Sep 17 00:00:00 2001 From: Sunik Kupfer Date: Wed, 6 Nov 2024 15:24:47 +0100 Subject: [PATCH 3/6] Extract read only evaluation to companion method --- .../kotlin/at/bitfire/davdroid/resource/LocalAddressBook.kt | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/app/src/main/kotlin/at/bitfire/davdroid/resource/LocalAddressBook.kt b/app/src/main/kotlin/at/bitfire/davdroid/resource/LocalAddressBook.kt index ee1c57c41..25fa1f59c 100644 --- a/app/src/main/kotlin/at/bitfire/davdroid/resource/LocalAddressBook.kt +++ b/app/src/main/kotlin/at/bitfire/davdroid/resource/LocalAddressBook.kt @@ -442,8 +442,9 @@ open class LocalAddressBook @AssistedInject constructor( * @param forceReadOnly Whether managed setting should overwrite read-only * @param info Determine read-only flag from collection data */ - private fun shouldBeReadOnly(info: Collection, forceReadOnly: Boolean? = null,): Boolean = - forceReadOnly == true || !info.privWriteContent || info.forceReadOnly + @VisibleForTesting + internal fun shouldBeReadOnly(info: Collection, forceReadOnly: Boolean? = null,): Boolean = + forceReadOnly == true || info.readOnly() /** * Finds a [LocalAddressBook] based on its corresponding collection. From a24a36aa741e0ca00de294c006f7ab9de7c373c0 Mon Sep 17 00:00:00 2001 From: Sunik Kupfer Date: Wed, 6 Nov 2024 15:24:53 +0100 Subject: [PATCH 4/6] Add test --- .../davdroid/resource/LocalAddressBookTest.kt | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/app/src/androidTest/kotlin/at/bitfire/davdroid/resource/LocalAddressBookTest.kt b/app/src/androidTest/kotlin/at/bitfire/davdroid/resource/LocalAddressBookTest.kt index a6651b311..4be85ac12 100644 --- a/app/src/androidTest/kotlin/at/bitfire/davdroid/resource/LocalAddressBookTest.kt +++ b/app/src/androidTest/kotlin/at/bitfire/davdroid/resource/LocalAddressBookTest.kt @@ -12,6 +12,7 @@ import android.content.Context import android.provider.ContactsContract import androidx.test.platform.app.InstrumentationRegistry import androidx.test.rule.GrantPermissionRule +import at.bitfire.davdroid.db.Collection import at.bitfire.vcard4android.Contact import at.bitfire.vcard4android.GroupMethod import at.bitfire.vcard4android.LabeledProperty @@ -19,6 +20,8 @@ import dagger.hilt.android.qualifiers.ApplicationContext import dagger.hilt.android.testing.HiltAndroidRule import dagger.hilt.android.testing.HiltAndroidTest import ezvcard.property.Telephone +import io.mockk.every +import io.mockk.mockk import java.util.LinkedList import javax.inject.Inject import org.junit.After @@ -26,6 +29,7 @@ import org.junit.AfterClass import org.junit.Assert.assertEquals import org.junit.Assert.assertFalse import org.junit.Assert.assertNotNull +import org.junit.Assert.assertTrue import org.junit.Before import org.junit.BeforeClass import org.junit.ClassRule @@ -123,6 +127,21 @@ class LocalAddressBookTest { assertEquals("Test Group", group.displayName) } + /** + * Tests the calculation of read only state is correct + */ + @Test + fun test_shouldBeReadOnly() { + val collectionReadOnly = mockk { every { readOnly() } returns true } + assertTrue(LocalAddressBook.shouldBeReadOnly(collectionReadOnly, null)) + assertTrue(LocalAddressBook.shouldBeReadOnly(collectionReadOnly, false)) + assertTrue(LocalAddressBook.shouldBeReadOnly(collectionReadOnly, true)) + + val collectionNotReadOnly = mockk { every { readOnly() } returns false } + assertFalse(LocalAddressBook.shouldBeReadOnly(collectionNotReadOnly, null)) + assertFalse(LocalAddressBook.shouldBeReadOnly(collectionNotReadOnly, false)) + assertTrue(LocalAddressBook.shouldBeReadOnly(collectionNotReadOnly, true)) + } companion object { From c202d92379da733bb5ed4f2ed910ef77783dac55 Mon Sep 17 00:00:00 2001 From: Sunik Kupfer Date: Thu, 7 Nov 2024 10:40:47 +0100 Subject: [PATCH 5/6] Always pass forceReadOnly flag --- .../at/bitfire/davdroid/resource/LocalAddressBookTest.kt | 2 -- .../at/bitfire/davdroid/resource/LocalAddressBook.kt | 7 +++---- .../bitfire/davdroid/settings/AccountSettingsMigrations.kt | 2 +- 3 files changed, 4 insertions(+), 7 deletions(-) diff --git a/app/src/androidTest/kotlin/at/bitfire/davdroid/resource/LocalAddressBookTest.kt b/app/src/androidTest/kotlin/at/bitfire/davdroid/resource/LocalAddressBookTest.kt index 4be85ac12..e47cda020 100644 --- a/app/src/androidTest/kotlin/at/bitfire/davdroid/resource/LocalAddressBookTest.kt +++ b/app/src/androidTest/kotlin/at/bitfire/davdroid/resource/LocalAddressBookTest.kt @@ -133,12 +133,10 @@ class LocalAddressBookTest { @Test fun test_shouldBeReadOnly() { val collectionReadOnly = mockk { every { readOnly() } returns true } - assertTrue(LocalAddressBook.shouldBeReadOnly(collectionReadOnly, null)) assertTrue(LocalAddressBook.shouldBeReadOnly(collectionReadOnly, false)) assertTrue(LocalAddressBook.shouldBeReadOnly(collectionReadOnly, true)) val collectionNotReadOnly = mockk { every { readOnly() } returns false } - assertFalse(LocalAddressBook.shouldBeReadOnly(collectionNotReadOnly, null)) assertFalse(LocalAddressBook.shouldBeReadOnly(collectionNotReadOnly, false)) assertTrue(LocalAddressBook.shouldBeReadOnly(collectionNotReadOnly, true)) } diff --git a/app/src/main/kotlin/at/bitfire/davdroid/resource/LocalAddressBook.kt b/app/src/main/kotlin/at/bitfire/davdroid/resource/LocalAddressBook.kt index 25fa1f59c..cc20fbfb5 100644 --- a/app/src/main/kotlin/at/bitfire/davdroid/resource/LocalAddressBook.kt +++ b/app/src/main/kotlin/at/bitfire/davdroid/resource/LocalAddressBook.kt @@ -155,9 +155,8 @@ open class LocalAddressBook @AssistedInject constructor( * @param info collection where to take the settings from * @param forceReadOnly `true`: set the address book to "force read-only"; * `false`: determine read-only flag from [info]; - * `null`: don't change the existing value */ - fun update(info: Collection, forceReadOnly: Boolean? = null) { + fun update(info: Collection, forceReadOnly: Boolean) { logger.log(Level.INFO, "Updating local address book $addressBookAccount with collection $info") val accountManager = AccountManager.get(context) @@ -443,8 +442,8 @@ open class LocalAddressBook @AssistedInject constructor( * @param info Determine read-only flag from collection data */ @VisibleForTesting - internal fun shouldBeReadOnly(info: Collection, forceReadOnly: Boolean? = null,): Boolean = - forceReadOnly == true || info.readOnly() + internal fun shouldBeReadOnly(info: Collection, forceReadOnly: Boolean): Boolean = + info.readOnly() || forceReadOnly /** * Finds a [LocalAddressBook] based on its corresponding collection. diff --git a/app/src/main/kotlin/at/bitfire/davdroid/settings/AccountSettingsMigrations.kt b/app/src/main/kotlin/at/bitfire/davdroid/settings/AccountSettingsMigrations.kt index f2a7d46df..d21d00fa5 100644 --- a/app/src/main/kotlin/at/bitfire/davdroid/settings/AccountSettingsMigrations.kt +++ b/app/src/main/kotlin/at/bitfire/davdroid/settings/AccountSettingsMigrations.kt @@ -103,7 +103,7 @@ class AccountSettingsMigrations @AssistedInject constructor( collectionRepository.getByServiceAndUrl(service.id, url)?.let { collection -> // Set collection ID and rename the account val localAddressBook = localAddressBookFactory.create(oldAddressBookAccount, provider) - localAddressBook.update(collection) + localAddressBook.update(collection, false) } } } From 38a8d98549a2c72cfb42cf79c5ae723a2d294479 Mon Sep 17 00:00:00 2001 From: Ricki Hirner Date: Thu, 7 Nov 2024 11:39:04 +0100 Subject: [PATCH 6/6] Minor KDoc changes --- .../kotlin/at/bitfire/davdroid/resource/LocalAddressBook.kt | 6 +++--- .../bitfire/davdroid/settings/AccountSettingsMigrations.kt | 4 +--- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/app/src/main/kotlin/at/bitfire/davdroid/resource/LocalAddressBook.kt b/app/src/main/kotlin/at/bitfire/davdroid/resource/LocalAddressBook.kt index cc20fbfb5..e1ca08239 100644 --- a/app/src/main/kotlin/at/bitfire/davdroid/resource/LocalAddressBook.kt +++ b/app/src/main/kotlin/at/bitfire/davdroid/resource/LocalAddressBook.kt @@ -436,10 +436,10 @@ open class LocalAddressBook @AssistedInject constructor( } /** - * Determines whether the address book should be set to read only. + * Determines whether the address book should be set to read-only. * - * @param forceReadOnly Whether managed setting should overwrite read-only - * @param info Determine read-only flag from collection data + * @param forceReadOnly Whether (usually managed, app-wide) setting should overwrite local read-only information + * @param info Collection data to determine read-only status from (either user-set read-only flag or missing write privilege) */ @VisibleForTesting internal fun shouldBeReadOnly(info: Collection, forceReadOnly: Boolean): Boolean = diff --git a/app/src/main/kotlin/at/bitfire/davdroid/settings/AccountSettingsMigrations.kt b/app/src/main/kotlin/at/bitfire/davdroid/settings/AccountSettingsMigrations.kt index d21d00fa5..91558b94f 100644 --- a/app/src/main/kotlin/at/bitfire/davdroid/settings/AccountSettingsMigrations.kt +++ b/app/src/main/kotlin/at/bitfire/davdroid/settings/AccountSettingsMigrations.kt @@ -21,7 +21,6 @@ import androidx.work.WorkManager import at.bitfire.davdroid.R import at.bitfire.davdroid.db.AppDatabase import at.bitfire.davdroid.db.Service -import at.bitfire.davdroid.repository.AccountRepository import at.bitfire.davdroid.repository.DavCollectionRepository import at.bitfire.davdroid.repository.DavServiceRepository import at.bitfire.davdroid.resource.LocalAddressBook @@ -52,7 +51,6 @@ class AccountSettingsMigrations @AssistedInject constructor( @Assisted val account: Account, @Assisted val accountSettings: AccountSettings, @ApplicationContext val context: Context, - private val accountRepository: AccountRepository, private val collectionRepository: DavCollectionRepository, private val db: AppDatabase, private val localAddressBookFactory: LocalAddressBook.Factory, @@ -103,7 +101,7 @@ class AccountSettingsMigrations @AssistedInject constructor( collectionRepository.getByServiceAndUrl(service.id, url)?.let { collection -> // Set collection ID and rename the account val localAddressBook = localAddressBookFactory.create(oldAddressBookAccount, provider) - localAddressBook.update(collection, false) + localAddressBook.update(collection, /* read-only flag will be updated at next sync */ forceReadOnly = false) } } }