-
-
Notifications
You must be signed in to change notification settings - Fork 29
Fix "Device" contacts vulnerable to deletion via unrelated account changes (#477) #495
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,6 +6,7 @@ import android.annotation.SuppressLint | |
| import android.content.ContentProviderOperation | ||
| import android.content.ContentResolver | ||
| import android.content.ContentUris | ||
| import android.content.ContentValues | ||
| import android.content.Context | ||
| import android.graphics.Bitmap | ||
| import android.graphics.BitmapFactory | ||
|
|
@@ -31,6 +32,7 @@ import com.bnyro.contacts.domain.model.ContactsGroup | |
| import com.bnyro.contacts.domain.model.ValueWithType | ||
| import com.bnyro.contacts.util.ContactsHelper | ||
| import com.bnyro.contacts.util.ImageHelper | ||
| import com.bnyro.contacts.util.PermissionHelper | ||
| import com.bnyro.contacts.util.Preferences | ||
| import com.bnyro.contacts.util.extension.boolValue | ||
| import com.bnyro.contacts.util.extension.intValue | ||
|
|
@@ -170,8 +172,10 @@ class DeviceContactsRepository(private val context: Context) : ContactsRepositor | |
| ContentProviderOperation.newInsert(ContactsContract.Groups.CONTENT_URI).apply { | ||
| withValue(ContactsContract.Groups.TITLE, groupName) | ||
| withValue(ContactsContract.Groups.GROUP_VISIBLE, 1) | ||
| withValue(ContactsContract.Groups.ACCOUNT_NAME, AccountType.androidDefault.name) | ||
| withValue(ContactsContract.Groups.ACCOUNT_TYPE, AccountType.androidDefault.type) | ||
| // see the comment in createContact() - a group needs a real null account too, | ||
| // not the "Device" UI sentinel, or it's vulnerable to the same cleanup | ||
| withValue(ContactsContract.Groups.ACCOUNT_NAME, null as String?) | ||
| withValue(ContactsContract.Groups.ACCOUNT_TYPE, null as String?) | ||
| operations.add(build()) | ||
| } | ||
|
|
||
|
|
@@ -309,10 +313,18 @@ class DeviceContactsRepository(private val context: Context) : ContactsRepositor | |
| override suspend fun createContact(contact: ContactData) { | ||
| withContext(Dispatchers.IO) { | ||
| val lastChosenAccount = Preferences.getLastChosenAccount() | ||
| val resolvedAccountType = contact.accountType ?: lastChosenAccount.type | ||
| val resolvedAccountName = contact.accountName ?: lastChosenAccount.name | ||
| // the local/"Device" account is a UI-only sentinel, not a real registered | ||
| // AccountManager account - writing it as a literal account_type/account_name | ||
| // makes ContactsProvider2 treat these raw contacts as belonging to an account | ||
| // that doesn't exist, so any unrelated account being added/removed anywhere on | ||
| // the device wipes them. null/null is Android's actual local-contact convention. | ||
| val isLocalAccount = resolvedAccountType == AccountType.androidDefault.type | ||
| val ops = listOfNotNull( | ||
| getCreateAction( | ||
| contact.accountType ?: lastChosenAccount.type, | ||
| contact.accountName ?: lastChosenAccount.name | ||
| if (isLocalAccount) null else resolvedAccountType, | ||
| if (isLocalAccount) null else resolvedAccountName | ||
| ), | ||
| getInsertAction( | ||
| StructuredName.CONTENT_ITEM_TYPE, | ||
|
|
@@ -440,9 +452,63 @@ class DeviceContactsRepository(private val context: Context) : ContactsRepositor | |
| return listOf(AccountType.androidDefault) + accounts.map { AccountType(it.name, it.type) } | ||
| } | ||
|
|
||
| /** | ||
| * Before the fix for #477, contacts/groups were created with AccountType.androidDefault | ||
| * (a UI-only "Device" sentinel) written as their literal account_type/account_name, as if | ||
| * it were a real account. ContactsProvider2 reconciles raw_contacts/data/groups against the | ||
| * real current account list on every account-list change anywhere on the device (any | ||
| * account, any type, add or remove) and deletes rows whose account no longer exists - since | ||
| * this sentinel never was a real account, every contact/group created this way was | ||
| * permanently exposed to that cleanup. This moves any such existing rows to the real | ||
| * local-contact convention (account_type=null, account_name=null) one time. | ||
| * | ||
| * Only runs if no real AccountManager account currently matches the sentinel's exact | ||
| * (type, name) - if one exists, this is a no-op, since there would be no way to tell which | ||
| * rows are ours vs. genuinely belonging to that real account. The flag is only set once a | ||
| * migration attempt actually completes (with permission and without a colliding real | ||
| * account), so an ambiguous or permission-denied run will naturally retry on a later launch. | ||
| */ | ||
| suspend fun migrateLegacyLocalAccountContacts() = withContext(Dispatchers.IO) { | ||
| if (Preferences.getBoolean(Preferences.legacyLocalAccountMigratedKey, false)) return@withContext | ||
| if (!PermissionHelper.hasPermission( | ||
| context, | ||
| Manifest.permission.READ_CONTACTS, | ||
| Manifest.permission.WRITE_CONTACTS | ||
| ) | ||
| ) { | ||
| return@withContext | ||
| } | ||
|
|
||
| val sentinelType = AccountType.androidDefault.type | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do you call this |
||
| val sentinelName = AccountType.androidDefault.name | ||
| val realAccountExists = AccountManager.get(context).accounts.any { | ||
| it.type == sentinelType && it.name == sentinelName | ||
| } | ||
| if (realAccountExists) return@withContext | ||
|
|
||
| runCatching { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why the |
||
| val selection = "${RawContacts.ACCOUNT_TYPE} = ? AND ${RawContacts.ACCOUNT_NAME} = ?" | ||
| val selectionArgs = arrayOf(sentinelType, sentinelName) | ||
| val nullAccountValues = ContentValues().apply { | ||
| putNull(RawContacts.ACCOUNT_TYPE) | ||
| putNull(RawContacts.ACCOUNT_NAME) | ||
| } | ||
|
|
||
| context.contentResolver.update(RawContacts.CONTENT_URI, nullAccountValues, selection, selectionArgs) | ||
| context.contentResolver.update( | ||
| ContactsContract.Groups.CONTENT_URI, | ||
| nullAccountValues, | ||
| selection, | ||
| selectionArgs | ||
| ) | ||
| } | ||
|
|
||
| Preferences.edit { putBoolean(Preferences.legacyLocalAccountMigratedKey, true) } | ||
| } | ||
|
|
||
| private fun getCreateAction( | ||
| accountType: String, | ||
| accountName: String | ||
| accountType: String?, | ||
| accountName: String? | ||
| ): ContentProviderOperation { | ||
| return ContentProviderOperation.newInsert(RawContacts.CONTENT_URI) | ||
| .withValue(RawContacts.ACCOUNT_TYPE, accountType) | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using
AccountType.androidDefaulthere is not really the cleanest solution, instead we should removeAccountType.androidDefaultand just always passnullaround as the account name and type. So the idea would be to modifysetLastChosenAccountandgetLastChosenAccountto also handlenullvalues. Then we don't have to do this case distinction here and can just rungetCreateAction(resolvedAccountType, resolvedAccountName).