diff --git a/bugsnag-android-core/detekt-baseline.xml b/bugsnag-android-core/detekt-baseline.xml index 6dc0d187eb..9225a2d3ed 100644 --- a/bugsnag-android-core/detekt-baseline.xml +++ b/bugsnag-android-core/detekt-baseline.xml @@ -31,10 +31,10 @@ TooGenericExceptionCaught:CallbackState.kt$CallbackState$ex: Throwable TooGenericExceptionCaught:DefaultDelivery.kt$DefaultDelivery$exception: Exception TooGenericExceptionCaught:DeviceDataCollector.kt$DeviceDataCollector$exception: Exception + TooGenericExceptionCaught:DeviceIdStore.kt$DeviceIdStore$exc: Throwable TooGenericExceptionCaught:ManifestConfigLoader.kt$ManifestConfigLoader$exc: Exception TooGenericExceptionCaught:PluginClient.kt$PluginClient$exc: Throwable TooGenericExceptionCaught:Stacktrace.kt$Stacktrace$lineEx: Exception - TooGenericExceptionCaught:SynchronizedStreamableStore.kt$SynchronizedStreamableStore$exc: Throwable TooGenericExceptionThrown:BreadcrumbStateTest.kt$BreadcrumbStateTest$throw Exception("Oh no") TooManyFunctions:ConfigInternal.kt$ConfigInternal : CallbackAwareMetadataAwareUserAware TooManyFunctions:DeviceDataCollector.kt$DeviceDataCollector diff --git a/bugsnag-android-core/src/androidTest/java/com/bugsnag/android/ClientTest.java b/bugsnag-android-core/src/androidTest/java/com/bugsnag/android/ClientTest.java index c4b4737929..3e19254bd2 100644 --- a/bugsnag-android-core/src/androidTest/java/com/bugsnag/android/ClientTest.java +++ b/bugsnag-android-core/src/androidTest/java/com/bugsnag/android/ClientTest.java @@ -148,7 +148,6 @@ public void testStoreUserInPrefsDisabled() { // Check that the user was not stored in prefs SharedPreferences sharedPref = getSharedPrefs(context); - assertNotNull(sharedPref.getString("install.iud", null)); assertFalse(sharedPref.contains("user.id")); assertFalse(sharedPref.contains("user.email")); assertFalse(sharedPref.contains("user.name")); diff --git a/bugsnag-android-core/src/androidTest/java/com/bugsnag/android/DeviceIdStoreTest.kt b/bugsnag-android-core/src/androidTest/java/com/bugsnag/android/DeviceIdStoreTest.kt new file mode 100644 index 0000000000..1d2f8c3c36 --- /dev/null +++ b/bugsnag-android-core/src/androidTest/java/com/bugsnag/android/DeviceIdStoreTest.kt @@ -0,0 +1,167 @@ +package com.bugsnag.android + +import android.content.Context +import androidx.test.core.app.ApplicationProvider +import junit.framework.TestCase.assertNull +import org.junit.Assert.assertEquals +import org.junit.Before +import org.junit.Test +import java.io.File +import java.util.UUID +import java.util.concurrent.CountDownLatch +import java.util.concurrent.Executors +import java.util.concurrent.atomic.AtomicReference + +internal class DeviceIdStoreTest { + + lateinit var file: File + lateinit var storageDir: File + + private val uuidProvider = { + UUID.fromString("ab0c1482-2ffe-11eb-adc1-0242ac120002") + } + private val diffUuidProvider = { + UUID.fromString("d9901bff-2ffe-11eb-adc1-0242ac120002") + } + + @Before + fun setUp() { + val ctx = ApplicationProvider.getApplicationContext() + storageDir = ctx.cacheDir + file = File(storageDir, "device.json") + file.delete() + } + + /** + * A file should be created if it does not already exist + */ + @Test + fun nonExistentFile() { + val nonExistentFile = File(storageDir, "foo") + nonExistentFile.delete() + val store = DeviceIdStore(nonExistentFile, NoopLogger) + val deviceId = store.loadDeviceId( + NoopLogger, + uuidProvider + ) + requireNotNull(deviceId) + assertEquals("ab0c1482-2ffe-11eb-adc1-0242ac120002", deviceId) + } + + /** + * An empty file should be overwritten with a new device ID + */ + @Test + fun emptyFile() { + val store = DeviceIdStore(file, NoopLogger) + val deviceId = store.loadDeviceId(NoopLogger, uuidProvider) + requireNotNull(deviceId) + assertEquals("ab0c1482-2ffe-11eb-adc1-0242ac120002", deviceId) + } + + /** + * A file of the incorrect length should be overwritten with a new device ID + */ + @Test + fun incorrectFileLength() { + val store = DeviceIdStore(file, NoopLogger) + val deviceId = store.loadDeviceId(NoopLogger, uuidProvider) + requireNotNull(deviceId) + assertEquals("ab0c1482-2ffe-11eb-adc1-0242ac120002", deviceId) + } + + /** + * A file of the correct length with invalid contents should be overwritten with a new device ID + */ + @Test + fun invalidFileContents() { + file.writeText("{\"hamster\": 2}") + val store = DeviceIdStore(file, NoopLogger) + val deviceId = store.loadDeviceId(NoopLogger, uuidProvider) + requireNotNull(deviceId) + assertEquals("ab0c1482-2ffe-11eb-adc1-0242ac120002", deviceId) + } + + /** + * A valid file should not be overwritten with a new device ID + */ + @Test + fun validFileContents() { + file.writeText("{\"id\": \"24c51482-2ffe-11eb-adc1-0242ac120002\"}") + val store = DeviceIdStore(file, NoopLogger) + + // device ID is loaded without writing file + assertEquals( + "24c51482-2ffe-11eb-adc1-0242ac120002", + store.loadDeviceId(NoopLogger, uuidProvider) + ) + // same device ID is retrieved as before + assertEquals( + "24c51482-2ffe-11eb-adc1-0242ac120002", + store.loadDeviceId(NoopLogger, diffUuidProvider) + ) + } + + /** + * A non-writable file does not crash the app + */ + @Test + fun nonWritableFile() { + val nonReadableFile = File(storageDir, "foo").apply { + delete() + createNewFile() + setWritable(false) + } + val store = DeviceIdStore(nonReadableFile, NoopLogger) + val deviceId = store.loadDeviceId(NoopLogger, uuidProvider) + assertNull(deviceId) + } + + /** + * The device ID store should take out a file lock to prevent concurrent writes to the file. + */ + @Test(timeout = 2000) + fun fileLockUsed() { + val store = DeviceIdStore(file, NoopLogger) + val latch = CountDownLatch(1) + + // the main thread & executor thread race to persist a device ID first. + // this asserts that the first value written to disk is used + // and thus indirectly verifies that the file is appropriately synchronized. + val firstPersistedValue = AtomicReference() + var bgThreadValue: String? = null + + // persist a new device ID on a background thread + Executors.newSingleThreadExecutor().submit { + bgThreadValue = store.loadDeviceId(NoopLogger, uuidProvider) + firstPersistedValue.compareAndSet(null, bgThreadValue) + latch.countDown() + } + + // persist a new device ID on the main thread + val deviceId = store.loadDeviceId(NoopLogger, diffUuidProvider) + firstPersistedValue.compareAndSet(null, deviceId) + latch.await() + + // validate that the device ID is consistent for each. + // the device ID of whichever thread won the race first should be used. + val expected = firstPersistedValue.get() + assertEquals(expected, deviceId) + assertEquals(expected, bgThreadValue) + } + + @Test + fun sharedPrefMigration() { + val store = DeviceIdStore(file, NoopLogger) + val context = ApplicationProvider.getApplicationContext() + + val prefs = + context.getSharedPreferences("com.bugsnag.android", Context.MODE_PRIVATE) + prefs.edit().putString("install.iud", "55670bff-9024-fc0a-b392-0242ac88ccd9").commit() + + val prefDeviceId = SharedPrefMigrator(context).loadDeviceId() + val storeDeviceId = store.loadDeviceId(context) + assertEquals("55670bff-9024-fc0a-b392-0242ac88ccd9", storeDeviceId) + assertEquals(prefDeviceId, storeDeviceId) + } +} diff --git a/bugsnag-android-core/src/main/java/com/bugsnag/android/Client.java b/bugsnag-android-core/src/main/java/com/bugsnag/android/Client.java index bf609a74e3..240caf98af 100644 --- a/bugsnag-android-core/src/main/java/com/bugsnag/android/Client.java +++ b/bugsnag-android-core/src/main/java/com/bugsnag/android/Client.java @@ -18,8 +18,10 @@ import androidx.annotation.VisibleForTesting; import kotlin.Unit; +import kotlin.jvm.functions.Function0; import kotlin.jvm.functions.Function2; +import java.io.File; import java.util.ArrayList; import java.util.Collections; import java.util.Date; @@ -28,6 +30,7 @@ import java.util.Map; import java.util.Observer; import java.util.Set; +import java.util.UUID; import java.util.concurrent.RejectedExecutionException; /** @@ -152,29 +155,24 @@ public Unit invoke(Boolean hasConnection, String networkState) { sessionStore, logger); metadataState = copyMetadataState(configuration); - // Set up and collect constant app and device diagnostics - sharedPrefs = appContext.getSharedPreferences(SHARED_PREF_KEY, Context.MODE_PRIVATE); - ActivityManager am = (ActivityManager) appContext.getSystemService(Context.ACTIVITY_SERVICE); appDataCollector = new AppDataCollector(appContext, appContext.getPackageManager(), immutableConfig, sessionTracker, am, logger); - UserRepository userRepository = new UserRepository(sharedPrefs, - immutableConfig.getPersistUser()); - userState = new UserState(userRepository); - User user = configuration.getUser(); + sharedPrefs = appContext.getSharedPreferences(SHARED_PREF_KEY, Context.MODE_PRIVATE); - if (user.getId() != null || user.getEmail() != null || user.getName() != null) { - userState.setUser(user.getId(), user.getEmail(), user.getName()); - } + // load the device + user information + File deviceIdFile = new File(appContext.getFilesDir(), "device-id"); + DeviceIdStore deviceIdStore = new DeviceIdStore(deviceIdFile, logger); + String deviceId = deviceIdStore.loadDeviceId(appContext); + userState = loadUserState(configuration, deviceId); DeviceBuildInfo info = DeviceBuildInfo.Companion.defaultInfo(); Resources resources = appContext.getResources(); - String id = userRepository.getDeviceId(); - deviceDataCollector = new DeviceDataCollector(connectivity, appContext, resources, id, info, - Environment.getDataDirectory(), logger); + deviceDataCollector = new DeviceDataCollector(connectivity, appContext, + resources, deviceId, info, Environment.getDataDirectory(), logger); if (appContext instanceof Application) { @@ -253,6 +251,18 @@ public void run() { leaveAutoBreadcrumb("Bugsnag loaded", BreadcrumbType.STATE, data); } + private UserState loadUserState(@NonNull Configuration configuration, String deviceId) { + boolean persistUser = immutableConfig.getPersistUser(); + UserRepository userRepository = new UserRepository(sharedPrefs, persistUser, deviceId); + UserState state = new UserState(userRepository); + User user = configuration.getUser(); + + if (user.getId() != null || user.getEmail() != null || user.getName() != null) { + state.setUser(user.getId(), user.getEmail(), user.getName()); + } + return state; + } + @VisibleForTesting Client( ImmutableConfig immutableConfig, diff --git a/bugsnag-android-core/src/main/java/com/bugsnag/android/DeviceIdStore.kt b/bugsnag-android-core/src/main/java/com/bugsnag/android/DeviceIdStore.kt new file mode 100644 index 0000000000..83974c7aff --- /dev/null +++ b/bugsnag-android-core/src/main/java/com/bugsnag/android/DeviceIdStore.kt @@ -0,0 +1,165 @@ +package com.bugsnag.android + +import android.content.Context +import android.util.JsonReader +import java.io.File +import java.io.IOException +import java.lang.Thread +import java.nio.channels.FileChannel +import java.nio.channels.FileLock +import java.nio.channels.OverlappingFileLockException +import java.util.UUID + +internal class DeviceIdStore(val file: File, val logger: Logger) { + + private val synchronizedStreamableStore: SynchronizedStreamableStore + + init { + try { + if (!file.exists()) { + file.createNewFile() + } + } catch (exc: IOException) { + logger.w("Failed to created device ID file", exc) + } + this.synchronizedStreamableStore = SynchronizedStreamableStore(file) + } + + fun loadDeviceId(context: Context): String? { + val sharedPrefMigrator = SharedPrefMigrator(context) + val legacyDeviceId = sharedPrefMigrator.loadDeviceId() + + return loadDeviceId(logger) { + when (legacyDeviceId) { + null -> UUID.randomUUID() + else -> UUID.fromString(legacyDeviceId) + } + } + } + + internal fun loadDeviceId(logger: Logger, uuidProvider: () -> UUID): String? { + return try { + // optimistically read device ID without a lock - the majority of the time + // the device ID will already be present so no synchronization is required. + val deviceId = loadDeviceIdInternal(logger) + + if (deviceId?.id != null) { + deviceId.id + } else { + return persistNewDeviceUuid(logger, uuidProvider) + } + } catch (exc: Throwable) { + logger.w("Failed to load device ID", exc) + null + } + } + + /** + * Loads the device ID from the file. + * + * If the file has zero length it can't contain device ID, so reading will be skipped. + */ + private fun loadDeviceIdInternal(logger: Logger): DeviceId? { + if (file.length() > 0) { + try { + return synchronizedStreamableStore.load(DeviceId.Companion::fromReader) + } catch (exc: IOException) { + logger.w("Failed to load device ID", exc) + } + } + return null + } + + /** + * Write a new Device ID to the file. + */ + private fun persistNewDeviceUuid( + logger: Logger, + uuidProvider: () -> UUID + ): String? { + return try { + // acquire a FileLock to prevent Clients in different processes writing + // to the same file concurrently + file.outputStream().channel.use { channel -> + persistNewDeviceIdWithLock(channel, logger, uuidProvider) + } + } catch (exc: Throwable) { + logger.w("Failed to persist device ID", exc) + null + } + } + + private fun persistNewDeviceIdWithLock( + channel: FileChannel, + logger: Logger, + uuidProvider: () -> UUID + ): String? { + val lock = waitForFileLock(channel) ?: return null + + return try { + // read the device ID again as it could have changed + // between the last read and when the lock was acquired + val deviceId = loadDeviceIdInternal(logger) + + if (deviceId?.id != null) { + // the device ID changed between the last read + // and acquiring the lock, so return the generated value + deviceId.id + } else { + // generate a new device ID and persist it + val newId = DeviceId(uuidProvider().toString()) + synchronizedStreamableStore.persist(newId) + newId.id + } + } finally { + lock.release() + } + } + + /** + * Attempt to acquire a file lock. If [OverlappingFileLockException] is thrown + * then the method will wait for 50ms then try again, for a maximum of 10 attempts. + */ + private fun waitForFileLock(channel: FileChannel): FileLock? { + repeat(MAX_FILE_LOCK_ATTEMPTS) { + try { + return channel.tryLock() + } catch (exc: OverlappingFileLockException) { + Thread.sleep(FILE_LOCK_WAIT_MS) + } + } + return null + } + + companion object { + private const val MAX_FILE_LOCK_ATTEMPTS = 20 + private const val FILE_LOCK_WAIT_MS = 25L + } +} + +private class DeviceId(val id: String?) : JsonStream.Streamable { + + override fun toStream(stream: JsonStream) { + with(stream) { + beginObject() + name(KEY_ID) + value(id) + endObject() + } + } + + companion object : JsonReadable { + private const val KEY_ID = "id" + + override fun fromReader(reader: JsonReader): DeviceId { + var id: String? = null + with(reader) { + beginObject() + if (hasNext() && KEY_ID == nextName()) { + id = nextString() + } + } + return DeviceId(id) + } + } +} diff --git a/bugsnag-android-core/src/main/java/com/bugsnag/android/SharedPrefMigrator.kt b/bugsnag-android-core/src/main/java/com/bugsnag/android/SharedPrefMigrator.kt new file mode 100644 index 0000000000..6c417e57fb --- /dev/null +++ b/bugsnag-android-core/src/main/java/com/bugsnag/android/SharedPrefMigrator.kt @@ -0,0 +1,18 @@ +package com.bugsnag.android + +import android.content.Context + +/** + * Reads legacy information left in SharedPreferences and migrates it to the new location. + */ +internal class SharedPrefMigrator(context: Context) { + + private val prefs = context + .getSharedPreferences("com.bugsnag.android", Context.MODE_PRIVATE) + + fun loadDeviceId() = prefs.getString(INSTALL_ID_KEY, null) + + companion object { + private const val INSTALL_ID_KEY = "install.iud" + } +} diff --git a/bugsnag-android-core/src/main/java/com/bugsnag/android/SynchronizedStreamableStore.kt b/bugsnag-android-core/src/main/java/com/bugsnag/android/SynchronizedStreamableStore.kt index a2e8eb72b0..bac7c6786a 100644 --- a/bugsnag-android-core/src/main/java/com/bugsnag/android/SynchronizedStreamableStore.kt +++ b/bugsnag-android-core/src/main/java/com/bugsnag/android/SynchronizedStreamableStore.kt @@ -15,7 +15,7 @@ internal class SynchronizedStreamableStore( @Throws(IOException::class) fun persist(streamable: T) { lock.writeLock().withLock { - file.writer().use { + file.writer().buffered().use { streamable.toStream(JsonStream(it)) true } @@ -25,7 +25,7 @@ internal class SynchronizedStreamableStore( @Throws(IOException::class) fun load(loadCallback: (JsonReader) -> T): T { lock.readLock().withLock { - return file.reader().use { + return file.reader().buffered().use { loadCallback(JsonReader(it)) } } diff --git a/bugsnag-android-core/src/main/java/com/bugsnag/android/UserRepository.kt b/bugsnag-android-core/src/main/java/com/bugsnag/android/UserRepository.kt index 422e64422a..feaf9238aa 100644 --- a/bugsnag-android-core/src/main/java/com/bugsnag/android/UserRepository.kt +++ b/bugsnag-android-core/src/main/java/com/bugsnag/android/UserRepository.kt @@ -3,25 +3,27 @@ package com.bugsnag.android import android.content.SharedPreferences import java.util.UUID -internal class UserRepository(private val prefs: SharedPreferences, private val persist: Boolean) { +internal class UserRepository( + private val prefs: SharedPreferences, + private val persist: Boolean, + private val deviceId: String? +) { companion object { - private const val INSTALL_ID_KEY = "install.iud" private const val USER_ID_KEY = "user.id" private const val USER_NAME_KEY = "user.name" private const val USER_EMAIL_KEY = "user.email" } fun load(): User { - val installId = getDeviceId() return when { persist -> // Check to see if a user was stored in the SharedPreferences User( - prefs.getString(USER_ID_KEY, installId), + prefs.getString(USER_ID_KEY, deviceId), prefs.getString(USER_EMAIL_KEY, null), prefs.getString(USER_NAME_KEY, null) ) - else -> User(installId, null, null) + else -> User(deviceId, null, null) } } @@ -40,17 +42,4 @@ internal class UserRepository(private val prefs: SharedPreferences, private val } editor.apply() } - - /** - * Retrieves the device ID. This is a UUID which is generated per installation - * and persisted in SharedPreferences. - */ - fun getDeviceId(): String { - var installId = prefs.getString(INSTALL_ID_KEY, null) - if (installId == null) { - installId = UUID.randomUUID().toString() - prefs.edit().putString(INSTALL_ID_KEY, installId).apply() - } - return installId - } } diff --git a/bugsnag-android-core/src/test/java/com/bugsnag/android/SharedPrefMigratorTest.kt b/bugsnag-android-core/src/test/java/com/bugsnag/android/SharedPrefMigratorTest.kt new file mode 100644 index 0000000000..f65ebdc1ae --- /dev/null +++ b/bugsnag-android-core/src/test/java/com/bugsnag/android/SharedPrefMigratorTest.kt @@ -0,0 +1,43 @@ +package com.bugsnag.android + +import android.content.Context +import android.content.SharedPreferences +import org.junit.Assert.assertEquals +import org.junit.Assert.assertNull +import org.junit.Before +import org.junit.Test +import org.junit.runner.RunWith +import org.mockito.ArgumentMatchers.eq +import org.mockito.Mock +import org.mockito.Mockito.`when` +import org.mockito.junit.MockitoJUnitRunner + +@RunWith(MockitoJUnitRunner::class) +internal class SharedPrefMigratorTest { + + @Mock + lateinit var context: Context + + @Mock + lateinit var prefs: SharedPreferences + + lateinit var prefMigrator: SharedPrefMigrator + + @Before + fun setUp() { + `when`(context.getSharedPreferences(eq("com.bugsnag.android"), eq(0))).thenReturn(prefs) + prefMigrator = SharedPrefMigrator(context) + } + + @Test + fun nullDeviceId() { + `when`(prefs.getString("install.iud", null)).thenReturn(null) + assertNull(prefMigrator.loadDeviceId()) + } + + @Test + fun validDeviceId() { + `when`(prefs.getString("install.iud", null)).thenReturn("f09asdfb") + assertEquals("f09asdfb", prefMigrator.loadDeviceId()) + } +} diff --git a/bugsnag-android-core/src/test/java/com/bugsnag/android/UserRepositoryTest.kt b/bugsnag-android-core/src/test/java/com/bugsnag/android/UserRepositoryTest.kt index 02952b6c39..b15e215eea 100644 --- a/bugsnag-android-core/src/test/java/com/bugsnag/android/UserRepositoryTest.kt +++ b/bugsnag-android-core/src/test/java/com/bugsnag/android/UserRepositoryTest.kt @@ -2,7 +2,6 @@ package com.bugsnag.android import android.content.SharedPreferences import org.junit.Assert.assertEquals -import org.junit.Assert.assertNotNull import org.junit.Assert.assertNull import org.junit.Before import org.junit.Test @@ -34,7 +33,7 @@ class UserRepositoryTest { `when`(prefs.getString(contains("user.name"), any())).thenReturn("Jane Fonda") `when`(prefs.getString(contains("user.email"), any())).thenReturn("test@example.com") - val repository = UserRepository(prefs, true) + val repository = UserRepository(prefs, true, "0asdf") val user = repository.load() assertEquals("jf123", user.id) @@ -44,8 +43,7 @@ class UserRepositoryTest { @Test fun loadNoPersist() { - `when`(prefs.getString(contains("install.iud"), any())).thenReturn("device-id-123") - val repository = UserRepository(prefs, false) + val repository = UserRepository(prefs, false, "device-id-123") val user = repository.load() assertEquals("device-id-123", user.id) assertNull(user.email) @@ -54,7 +52,7 @@ class UserRepositoryTest { @Test fun saveWithPersist() { - val repository = UserRepository(prefs, true) + val repository = UserRepository(prefs, true, "") repository.save(User("123", "joe@yahoo.com", "Joe Bloggs")) verify(editor, times(1)).putString("user.id", "123") verify(editor, times(1)).putString("user.email", "joe@yahoo.com") @@ -64,28 +62,11 @@ class UserRepositoryTest { @Test fun saveNoPersist() { - val repository = UserRepository(prefs, false) + val repository = UserRepository(prefs, false, "") repository.save(User("123", "joe@yahoo.com", "Joe Bloggs")) verify(editor, times(1)).remove("user.id") verify(editor, times(1)).remove("user.email") verify(editor, times(1)).remove("user.name") verify(editor, times(1)).apply() } - - @Test - fun getDeviceIdFirstTime() { - `when`(prefs.getString(contains("install.iud"), any())).thenReturn(null) - val repository = UserRepository(prefs, false) - val deviceId = repository.getDeviceId() - assertNotNull(deviceId) - verify(editor, times(1)).putString(matches("install.iud"), any()) - } - - @Test - fun getDeviceIdExistingValue() { - `when`(prefs.getString(contains("install.iud"), any())).thenReturn("4a09cbe2") - val repository = UserRepository(prefs, false) - val deviceId = repository.getDeviceId() - assertEquals("4a09cbe2", deviceId) - } }