From 4df1b245e8209824f2c7e1b3541dfe8202d76aa7 Mon Sep 17 00:00:00 2001 From: David Perez Date: Thu, 3 Apr 2025 17:02:53 -0500 Subject: [PATCH] PM-19873: Migrate the screen capture feature from user-scoped to app-scoped (#4972) --- .../datasource/disk/SettingsDiskSource.kt | 25 ++++---- .../datasource/disk/SettingsDiskSourceImpl.kt | 60 ++++++++++--------- .../repository/SettingsRepositoryImpl.kt | 31 ++-------- .../datasource/disk/SettingsDiskSourceTest.kt | 44 ++++---------- .../disk/util/FakeSettingsDiskSource.kt | 38 ++++-------- .../repository/SettingsRepositoryTest.kt | 9 +-- 6 files changed, 75 insertions(+), 132 deletions(-) diff --git a/app/src/main/java/com/x8bit/bitwarden/data/platform/datasource/disk/SettingsDiskSource.kt b/app/src/main/java/com/x8bit/bitwarden/data/platform/datasource/disk/SettingsDiskSource.kt index 2fed12817d..84d6365e12 100644 --- a/app/src/main/java/com/x8bit/bitwarden/data/platform/datasource/disk/SettingsDiskSource.kt +++ b/app/src/main/java/com/x8bit/bitwarden/data/platform/datasource/disk/SettingsDiskSource.kt @@ -24,6 +24,16 @@ interface SettingsDiskSource { */ val appLanguageFlow: Flow + /** + * The currently persisted setting for whether screen capture is allowed. + */ + var screenCaptureAllowed: Boolean? + + /** + * Emits updates that track [screenCaptureAllowed]. + */ + val screenCaptureAllowedFlow: Flow + /** * Has the initial autofill dialog been shown to the user. */ @@ -237,21 +247,6 @@ interface SettingsDiskSource { blockedAutofillUris: List?, ) - /** - * Gets whether or not the given [userId] has enabled screen capture. - */ - fun getScreenCaptureAllowed(userId: String): Boolean? - - /** - * Emits updates that track [getScreenCaptureAllowed] for the given [userId]. - */ - fun getScreenCaptureAllowedFlow(userId: String): Flow - - /** - * Stores whether or not [isScreenCaptureAllowed] for the given [userId]. - */ - fun storeScreenCaptureAllowed(userId: String, isScreenCaptureAllowed: Boolean?) - /** * Records a user sign in for the given [userId]. This data is expected to remain on * disk until storage is cleared or the app is uninstalled. diff --git a/app/src/main/java/com/x8bit/bitwarden/data/platform/datasource/disk/SettingsDiskSourceImpl.kt b/app/src/main/java/com/x8bit/bitwarden/data/platform/datasource/disk/SettingsDiskSourceImpl.kt index f98cdf46ce..1fdbffee25 100644 --- a/app/src/main/java/com/x8bit/bitwarden/data/platform/datasource/disk/SettingsDiskSourceImpl.kt +++ b/app/src/main/java/com/x8bit/bitwarden/data/platform/datasource/disk/SettingsDiskSourceImpl.kt @@ -1,6 +1,7 @@ package com.x8bit.bitwarden.data.platform.datasource.disk import android.content.SharedPreferences +import androidx.core.content.edit import com.bitwarden.core.data.repository.util.bufferedMutableSharedFlow import com.bitwarden.core.data.util.decodeFromStringOrNull import com.bitwarden.data.datasource.disk.BaseDiskSource @@ -50,7 +51,7 @@ private const val RESUME_SCREEN = "resumeScreen" */ @Suppress("TooManyFunctions") class SettingsDiskSourceImpl( - val sharedPreferences: SharedPreferences, + private val sharedPreferences: SharedPreferences, private val json: Json, ) : BaseDiskSource(sharedPreferences = sharedPreferences), SettingsDiskSource { @@ -87,12 +88,15 @@ class SettingsDiskSourceImpl( private val mutableHasSeenGeneratorCoachMarkFlow = bufferedMutableSharedFlow() - private val mutableScreenCaptureAllowedFlowMap = - mutableMapOf>() + private val mutableScreenCaptureAllowedFlow = MutableSharedFlow() private val mutableVaultRegisteredForExportFlow = mutableMapOf>() + init { + migrateScreenCaptureSetting() + } + override var appLanguage: AppLanguage? get() = getString(key = APP_LANGUAGE_KEY) ?.let { storedValue -> @@ -109,6 +113,16 @@ class SettingsDiskSourceImpl( override val appLanguageFlow: Flow get() = mutableAppLanguageFlow.onSubscription { emit(appLanguage) } + override var screenCaptureAllowed: Boolean? + get() = getBoolean(key = SCREEN_CAPTURE_ALLOW_KEY) + set(value) { + putBoolean(key = SCREEN_CAPTURE_ALLOW_KEY, value = value) + mutableScreenCaptureAllowedFlow.tryEmit(value) + } + + override val screenCaptureAllowedFlow: Flow + get() = mutableScreenCaptureAllowedFlow.onSubscription { emit(screenCaptureAllowed) } + override var initialAutofillDialogShown: Boolean? get() = getBoolean(key = INITIAL_AUTOFILL_DIALOG_SHOWN) set(value) { @@ -370,25 +384,6 @@ class SettingsDiskSourceImpl( ) } - override fun getScreenCaptureAllowed(userId: String): Boolean? { - return getBoolean(key = SCREEN_CAPTURE_ALLOW_KEY.appendIdentifier(userId)) - } - - override fun getScreenCaptureAllowedFlow(userId: String): Flow = - getMutableScreenCaptureAllowedFlow(userId) - .onSubscription { emit(getScreenCaptureAllowed(userId)) } - - override fun storeScreenCaptureAllowed( - userId: String, - isScreenCaptureAllowed: Boolean?, - ) { - putBoolean( - key = SCREEN_CAPTURE_ALLOW_KEY.appendIdentifier(userId), - value = isScreenCaptureAllowed, - ) - getMutableScreenCaptureAllowedFlow(userId).tryEmit(isScreenCaptureAllowed) - } - override fun storeUseHasLoggedInPreviously(userId: String) { putBoolean( key = HAS_USER_LOGGED_IN_OR_CREATED_AN_ACCOUNT_KEY.appendIdentifier(userId), @@ -568,11 +563,6 @@ class SettingsDiskSourceImpl( bufferedMutableSharedFlow(replay = 1) } - private fun getMutableScreenCaptureAllowedFlow(userId: String): MutableSharedFlow = - mutableScreenCaptureAllowedFlowMap.getOrPut(userId) { - bufferedMutableSharedFlow(replay = 1) - } - private fun getMutableShowAutoFillSettingBadgeFlow( userId: String, ): MutableSharedFlow = mutableShowAutoFillSettingBadgeFlowMap.getOrPut(userId) { @@ -596,4 +586,20 @@ class SettingsDiskSourceImpl( ): MutableSharedFlow = mutableVaultRegisteredForExportFlow.getOrPut(userId) { bufferedMutableSharedFlow(replay = 1) } + + /** + * Migrates the user-scoped screen capture state to an app-wide state. + * + * In the case that multiple users have different values stored for this feature, we will + * default to _not_ allowing screen capture. + */ + private fun migrateScreenCaptureSetting() { + val screenCaptureSettings = sharedPreferences.all.filter { + // The underscore is important to not grab the new app-scoped key + it.key.contains(other = "${SCREEN_CAPTURE_ALLOW_KEY}_") + } + if (screenCaptureSettings.isEmpty()) return + screenCaptureAllowed = screenCaptureSettings.all { it.value == true } + screenCaptureSettings.forEach { sharedPreferences.edit(commit = true) { remove(it.key) } } + } } diff --git a/app/src/main/java/com/x8bit/bitwarden/data/platform/repository/SettingsRepositoryImpl.kt b/app/src/main/java/com/x8bit/bitwarden/data/platform/repository/SettingsRepositoryImpl.kt index 239d9edebd..c4ae0248f1 100644 --- a/app/src/main/java/com/x8bit/bitwarden/data/platform/repository/SettingsRepositoryImpl.kt +++ b/app/src/main/java/com/x8bit/bitwarden/data/platform/repository/SettingsRepositoryImpl.kt @@ -24,12 +24,10 @@ import com.x8bit.bitwarden.data.vault.datasource.sdk.VaultSdkSource import com.x8bit.bitwarden.ui.platform.feature.settings.appearance.model.AppLanguage import com.x8bit.bitwarden.ui.platform.feature.settings.appearance.model.AppTheme import kotlinx.coroutines.CoroutineScope -import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.SharingStarted import kotlinx.coroutines.flow.StateFlow -import kotlinx.coroutines.flow.flatMapLatest import kotlinx.coroutines.flow.flowOf import kotlinx.coroutines.flow.launchIn import kotlinx.coroutines.flow.map @@ -333,36 +331,19 @@ class SettingsRepositoryImpl( autofillEnabledManager.isAutofillEnabledStateFlow override var isScreenCaptureAllowed: Boolean - get() = activeUserId - ?.let { settingsDiskSource.getScreenCaptureAllowed(it) } - ?: DEFAULT_IS_SCREEN_CAPTURE_ALLOWED + get() = settingsDiskSource.screenCaptureAllowed ?: DEFAULT_IS_SCREEN_CAPTURE_ALLOWED set(value) { - val userId = activeUserId ?: return - settingsDiskSource.storeScreenCaptureAllowed( - userId = userId, - isScreenCaptureAllowed = value, - ) + settingsDiskSource.screenCaptureAllowed = value } - @OptIn(ExperimentalCoroutinesApi::class) override val isScreenCaptureAllowedStateFlow: StateFlow - get() = authDiskSource - .userStateFlow - .flatMapLatest { userState -> - userState - ?.activeUserId - ?.let { - settingsDiskSource.getScreenCaptureAllowedFlow(userId = it) - .map { isAllowed -> isAllowed ?: DEFAULT_IS_SCREEN_CAPTURE_ALLOWED } - } - ?: flowOf(DEFAULT_IS_SCREEN_CAPTURE_ALLOWED) - } + get() = settingsDiskSource + .screenCaptureAllowedFlow + .map { isAllowed -> isAllowed ?: DEFAULT_IS_SCREEN_CAPTURE_ALLOWED } .stateIn( scope = unconfinedScope, started = SharingStarted.Lazily, - initialValue = activeUserId - ?.let { settingsDiskSource.getScreenCaptureAllowed(userId = it) } - ?: DEFAULT_IS_SCREEN_CAPTURE_ALLOWED, + initialValue = isScreenCaptureAllowed, ) init { diff --git a/app/src/test/java/com/x8bit/bitwarden/data/platform/datasource/disk/SettingsDiskSourceTest.kt b/app/src/test/java/com/x8bit/bitwarden/data/platform/datasource/disk/SettingsDiskSourceTest.kt index 10f322b1c2..9278cfc4e8 100644 --- a/app/src/test/java/com/x8bit/bitwarden/data/platform/datasource/disk/SettingsDiskSourceTest.kt +++ b/app/src/test/java/com/x8bit/bitwarden/data/platform/datasource/disk/SettingsDiskSourceTest.kt @@ -148,10 +148,6 @@ class SettingsDiskSourceTest { userId = userId, lastSyncTime = Instant.parse("2023-10-27T12:00:00Z"), ) - settingsDiskSource.storeScreenCaptureAllowed( - userId = userId, - isScreenCaptureAllowed = true, - ) settingsDiskSource.storeClearClipboardFrequencySeconds(userId = userId, frequency = 5) val systemBioIntegrityState = "system_biometrics_integrity_state" settingsDiskSource.storeAccountBiometricIntegrityValidity( @@ -166,7 +162,6 @@ class SettingsDiskSourceTest { settingsDiskSource.clearData(userId = userId) // We do not clear these even when you call clear storage - assertEquals(true, settingsDiskSource.getScreenCaptureAllowed(userId = userId)) assertTrue(settingsDiskSource.getShowUnlockSettingBadge(userId = userId) ?: false) assertTrue(settingsDiskSource.getShowAutoFillSettingBadge(userId = userId) ?: false) @@ -867,50 +862,31 @@ class SettingsDiskSourceTest { @Test fun `getScreenCaptureAllowed should pull from SharedPreferences`() { - val screenCaptureAllowBaseKey = "bwPreferencesStorage:screenCaptureAllowed" - val mockUserId = "mockUserId" + val screenCaptureAllowKey = "bwPreferencesStorage:screenCaptureAllowed" val isScreenCaptureAllowed = true fakeSharedPreferences.edit { - putBoolean("${screenCaptureAllowBaseKey}_$mockUserId", isScreenCaptureAllowed) + putBoolean(screenCaptureAllowKey, isScreenCaptureAllowed) } - val actual = settingsDiskSource.getScreenCaptureAllowed(userId = mockUserId) - assertEquals( - isScreenCaptureAllowed, - actual, - ) + val actual = settingsDiskSource.screenCaptureAllowed + assertEquals(isScreenCaptureAllowed, actual) } @Test fun `storeScreenCaptureAllowed for non-null values should update SharedPreferences`() { - val screenCaptureAllowBaseKey = "bwPreferencesStorage:screenCaptureAllowed" - val mockUserId = "mockUserId" + val screenCaptureAllowKey = "bwPreferencesStorage:screenCaptureAllowed" val isScreenCaptureAllowed = true - settingsDiskSource.storeScreenCaptureAllowed( - userId = mockUserId, - isScreenCaptureAllowed = isScreenCaptureAllowed, - ) - val actual = fakeSharedPreferences.getBoolean( - "${screenCaptureAllowBaseKey}_$mockUserId", - false, - ) - assertEquals( - isScreenCaptureAllowed, - actual, - ) + settingsDiskSource.screenCaptureAllowed = isScreenCaptureAllowed + val actual = fakeSharedPreferences.getBoolean(screenCaptureAllowKey, false) + assertEquals(isScreenCaptureAllowed, actual) } @Test fun `storeScreenCaptureAllowed for null values should clear SharedPreferences`() { - val screenCaptureAllowBaseKey = "bwPreferencesStorage:screenCaptureAllowed" - val mockUserId = "mockUserId" - val screenCaptureAllowKey = "${screenCaptureAllowBaseKey}_$mockUserId" + val screenCaptureAllowKey = "bwPreferencesStorage:screenCaptureAllowed" fakeSharedPreferences.edit { putBoolean(screenCaptureAllowKey, true) } - settingsDiskSource.storeScreenCaptureAllowed( - userId = mockUserId, - isScreenCaptureAllowed = null, - ) + settingsDiskSource.screenCaptureAllowed = null assertFalse(fakeSharedPreferences.contains(screenCaptureAllowKey)) } diff --git a/app/src/test/java/com/x8bit/bitwarden/data/platform/datasource/disk/util/FakeSettingsDiskSource.kt b/app/src/test/java/com/x8bit/bitwarden/data/platform/datasource/disk/util/FakeSettingsDiskSource.kt index 35ee580764..114d43750b 100644 --- a/app/src/test/java/com/x8bit/bitwarden/data/platform/datasource/disk/util/FakeSettingsDiskSource.kt +++ b/app/src/test/java/com/x8bit/bitwarden/data/platform/datasource/disk/util/FakeSettingsDiskSource.kt @@ -23,6 +23,8 @@ class FakeSettingsDiskSource : SettingsDiskSource { private val mutableAppThemeFlow = bufferedMutableSharedFlow(replay = 1) + private val mutableScreenCaptureAllowedFlow = bufferedMutableSharedFlow(replay = 1) + private val mutableLastSyncCallFlowMap = mutableMapOf>() private val mutableVaultTimeoutActionsFlowMap = @@ -45,9 +47,6 @@ class FakeSettingsDiskSource : SettingsDiskSource { private val mutableShouldShowAddLoginCoachMarkFlow = bufferedMutableSharedFlow() - private val mutableScreenCaptureAllowedFlowMap = - mutableMapOf>() - private val mutableShouldShowGeneratorCoachMarkFlow = bufferedMutableSharedFlow() @@ -67,7 +66,7 @@ class FakeSettingsDiskSource : SettingsDiskSource { private var storedIsCrashLoggingEnabled: Boolean? = null private var storedHasUserLoggedInOrCreatedAccount: Boolean? = null private var storedInitialAutofillDialogShown: Boolean? = null - private val storedScreenCaptureAllowed = mutableMapOf() + private var storedScreenCaptureAllowed: Boolean? = null private var storedSystemBiometricIntegritySource: String? = null private val storedAccountBiometricIntegrityValidity = mutableMapOf() private val storedAppResumeScreenData = mutableMapOf() @@ -116,6 +115,16 @@ class FakeSettingsDiskSource : SettingsDiskSource { emit(appTheme) } + override var screenCaptureAllowed: Boolean? + get() = storedScreenCaptureAllowed + set(value) { + storedScreenCaptureAllowed = value + mutableScreenCaptureAllowedFlow.tryEmit(value) + } + + override val screenCaptureAllowedFlow: Flow + get() = mutableScreenCaptureAllowedFlow.onSubscription { emit(screenCaptureAllowed) } + override var systemBiometricIntegritySource: String? get() = storedSystemBiometricIntegritySource set(value) { @@ -304,21 +313,6 @@ class FakeSettingsDiskSource : SettingsDiskSource { storedBlockedAutofillUris[userId] = blockedAutofillUris } - override fun getScreenCaptureAllowed(userId: String): Boolean? = - storedScreenCaptureAllowed[userId] - - override fun getScreenCaptureAllowedFlow(userId: String): Flow { - return getMutableScreenCaptureAllowedFlow(userId) - } - - override fun storeScreenCaptureAllowed( - userId: String, - isScreenCaptureAllowed: Boolean?, - ) { - storedScreenCaptureAllowed[userId] = isScreenCaptureAllowed - getMutableScreenCaptureAllowedFlow(userId).tryEmit(isScreenCaptureAllowed) - } - override fun storeUseHasLoggedInPreviously(userId: String) { userSignIns[userId] = true } @@ -437,12 +431,6 @@ class FakeSettingsDiskSource : SettingsDiskSource { } //region Private helper functions - private fun getMutableScreenCaptureAllowedFlow(userId: String): MutableSharedFlow { - return mutableScreenCaptureAllowedFlowMap.getOrPut(userId) { - bufferedMutableSharedFlow(replay = 1) - } - } - private fun getMutableLastSyncTimeFlow( userId: String, ): MutableSharedFlow = diff --git a/app/src/test/java/com/x8bit/bitwarden/data/platform/repository/SettingsRepositoryTest.kt b/app/src/test/java/com/x8bit/bitwarden/data/platform/repository/SettingsRepositoryTest.kt index c71ffc903f..f1a4ec4eeb 100644 --- a/app/src/test/java/com/x8bit/bitwarden/data/platform/repository/SettingsRepositoryTest.kt +++ b/app/src/test/java/com/x8bit/bitwarden/data/platform/repository/SettingsRepositoryTest.kt @@ -1003,22 +1003,19 @@ class SettingsRepositoryTest { @Test fun `isScreenCaptureAllowed property should update SettingsDiskSource and emit changes`() = runTest { - fakeAuthDiskSource.userState = MOCK_USER_STATE - - fakeSettingsDiskSource.storeScreenCaptureAllowed(USER_ID, false) - + fakeSettingsDiskSource.screenCaptureAllowed = false settingsRepository.isScreenCaptureAllowedStateFlow.test { assertFalse(awaitItem()) settingsRepository.isScreenCaptureAllowed = true assertTrue(awaitItem()) - assertEquals(true, fakeSettingsDiskSource.getScreenCaptureAllowed(USER_ID)) + assertEquals(true, fakeSettingsDiskSource.screenCaptureAllowed) settingsRepository.isScreenCaptureAllowed = false assertFalse(awaitItem()) - assertEquals(false, fakeSettingsDiskSource.getScreenCaptureAllowed(USER_ID)) + assertEquals(false, fakeSettingsDiskSource.screenCaptureAllowed) } }