Skip to content
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

Ask user to swith account when reading code while signed in #5271

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 55 additions & 0 deletions sync/sync-impl/lint-baseline.xml
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,17 @@
message="Defined here, included via layout/activity_sync.xml => layout/view_sync_disabled.xml defines @+id/syncSetupOtherPlatforms"/>
</issue>

<issue
id="DenyListedApi"
message="If you find yourself using this API in production, you&apos;re doing something wrong!!"
errorLine1=" this.seamlessAccountSwitching().setRawStoredState(State(true))"
errorLine2=" ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~">
<location
file="src/test/java/com/duckduckgo/sync/impl/AppSyncAccountRepositoryTest.kt"
line="100"
column="9"/>
</issue>

<issue
id="DenyListedApi"
message="No one remembers what these constants map to. Use the API level integer value directly since it&apos;s self-defining."
Expand Down Expand Up @@ -100,6 +111,28 @@
column="38"/>
</issue>

<issue
id="DenyListedApi"
message="If you find yourself using this API in production, you&apos;re doing something wrong!!"
errorLine1=" this.seamlessAccountSwitching().setRawStoredState(State(true))"
errorLine2=" ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~">
<location
file="src/test/java/com/duckduckgo/sync/impl/ui/EnterCodeViewModelTest.kt"
line="63"
column="9"/>
</issue>

<issue
id="DenyListedApi"
message="If you find yourself using this API in production, you&apos;re doing something wrong!!"
errorLine1=" syncFeature.seamlessAccountSwitching().setRawStoredState(State(false))"
errorLine2=" ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~">
<location
file="src/test/java/com/duckduckgo/sync/impl/ui/EnterCodeViewModelTest.kt"
line="137"
column="9"/>
</issue>

<issue
id="DenyListedApi"
message="No one remembers what these constants map to. Use the API level integer value directly since it&apos;s self-defining."
Expand Down Expand Up @@ -375,6 +408,28 @@
column="16"/>
</issue>

<issue
id="DenyListedApi"
message="If you find yourself using this API in production, you&apos;re doing something wrong!!"
errorLine1=" this.seamlessAccountSwitching().setRawStoredState(State(true))"
errorLine2=" ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~">
<location
file="src/test/java/com/duckduckgo/sync/impl/ui/SyncWithAnotherDeviceViewModelTest.kt"
line="62"
column="9"/>
</issue>

<issue
id="DenyListedApi"
message="If you find yourself using this API in production, you&apos;re doing something wrong!!"
errorLine1=" syncFeature.seamlessAccountSwitching().setRawStoredState(State(false))"
errorLine2=" ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~">
<location
file="src/test/java/com/duckduckgo/sync/impl/ui/SyncWithAnotherDeviceViewModelTest.kt"
line="135"
column="9"/>
</issue>

<issue
id="VectorRaster"
message="Limit vector icons sizes to 200×200 to keep icon drawing fast; see https://developer.android.com/studio/write/vector-asset-studio#when for more"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ interface SyncAccountRepository {
fun getConnectQR(): Result<String>
fun pollConnectionKeys(): Result<Boolean>
fun renameDevice(device: ConnectedDevice): Result<Boolean>
fun logoutAndJoinNewAccount(stringCode: String): Result<Boolean>
}

@ContributesBinding(AppScope::class)
Expand All @@ -73,8 +74,11 @@ class AppSyncAccountRepository @Inject constructor(
private val syncPixels: SyncPixels,
@AppCoroutineScope private val appCoroutineScope: CoroutineScope,
private val dispatcherProvider: DispatcherProvider,
private val syncFeature: SyncFeature,
) : SyncAccountRepository {

private val connectedDevicesCached: MutableList<ConnectedDevice> = mutableListOf()

override fun isSyncSupported(): Boolean {
return syncStore.isEncryptionSupported()
}
Expand Down Expand Up @@ -107,9 +111,20 @@ class AppSyncAccountRepository @Inject constructor(
}

private fun login(recoveryCode: RecoveryCode): Result<Boolean> {
var wasUserLogout = false
if (isSignedIn()) {
return Error(code = ALREADY_SIGNED_IN.code, reason = "Already signed in")
.alsoFireAlreadySignedInErrorPixel()
val allowSwitchAccount = syncFeature.seamlessAccountSwitching().isEnabled()
val error = Error(code = ALREADY_SIGNED_IN.code, reason = "Already signed in").alsoFireAlreadySignedInErrorPixel()
if (allowSwitchAccount && connectedDevicesCached.size == 1) {
val thisDeviceId = syncStore.deviceId.orEmpty()
val result = logout(thisDeviceId)
if (result is Error) {
return result
}
wasUserLogout = true
} else {
return error
}
}

val primaryKey = recoveryCode.primaryKey
Expand All @@ -119,6 +134,9 @@ class AppSyncAccountRepository @Inject constructor(

return performLogin(userId, deviceId, deviceName, primaryKey).onFailure {
it.alsoFireLoginErrorPixel()
if (wasUserLogout) {
syncPixels.fireUserSwitchedLoginError()
}
return it.copy(code = LOGIN_FAILED.code)
}
}
Expand Down Expand Up @@ -287,6 +305,7 @@ class AppSyncAccountRepository @Inject constructor(

return when (val result = syncApi.getDevices(token)) {
is Error -> {
connectedDevicesCached.clear()
result.alsoFireAccountErrorPixel().copy(code = GENERIC_ERROR.code)
}

Expand Down Expand Up @@ -314,6 +333,11 @@ class AppSyncAccountRepository @Inject constructor(
}
}.sortedWith { a, b ->
if (a.thisDevice) -1 else 1
}.also {
connectedDevicesCached.apply {
clear()
addAll(it)
}
},
)
}
Expand All @@ -322,6 +346,23 @@ class AppSyncAccountRepository @Inject constructor(

override fun isSignedIn() = syncStore.isSignedIn()

override fun logoutAndJoinNewAccount(stringCode: String): Result<Boolean> {
val thisDeviceId = syncStore.deviceId.orEmpty()
return when (val result = logout(thisDeviceId)) {
is Error -> {
syncPixels.fireUserSwitchedLogoutError()
result
}
is Result.Success -> {
val loginResult = processCode(stringCode)
if (loginResult is Error) {
syncPixels.fireUserSwitchedLoginError()
}
loginResult
}
}
}

private fun performCreateAccount(): Result<Boolean> {
val userId = syncDeviceIds.userId()
val account: AccountKeys = kotlin.runCatching {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ package com.duckduckgo.sync.impl
import com.duckduckgo.anvil.annotations.ContributesRemoteFeature
import com.duckduckgo.di.scopes.AppScope
import com.duckduckgo.feature.toggles.api.Toggle
import com.duckduckgo.feature.toggles.api.Toggle.InternalAlwaysEnabled

@ContributesRemoteFeature(
scope = AppScope::class,
Expand All @@ -43,4 +44,7 @@ interface SyncFeature {

@Toggle.DefaultValue(true)
fun gzipPatchRequests(): Toggle

@Toggle.DefaultValue(true)
fun seamlessAccountSwitching(): Toggle
}
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,13 @@ class SyncPixelParamRemovalPlugin @Inject constructor() : PixelParamRemovalPlugi
SyncPixelName.SYNC_GET_OTHER_DEVICES_SCREEN_SHOWN.pixelName to PixelParameter.removeAtb(),
SyncPixelName.SYNC_GET_OTHER_DEVICES_LINK_COPIED.pixelName to PixelParameter.removeAtb(),
SyncPixelName.SYNC_GET_OTHER_DEVICES_LINK_SHARED.pixelName to PixelParameter.removeAtb(),

SyncPixelName.SYNC_ASK_USER_TO_SWITCH_ACCOUNT.pixelName to PixelParameter.removeAtb(),
SyncPixelName.SYNC_USER_ACCEPTED_SWITCHING_ACCOUNT.pixelName to PixelParameter.removeAtb(),
SyncPixelName.SYNC_USER_CANCELLED_SWITCHING_ACCOUNT.pixelName to PixelParameter.removeAtb(),
SyncPixelName.SYNC_USER_SWITCHED_ACCOUNT.pixelName to PixelParameter.removeAtb(),
SyncPixelName.SYNC_USER_SWITCHED_LOGOUT_ERROR.pixelName to PixelParameter.removeAtb(),
SyncPixelName.SYNC_USER_SWITCHED_LOGIN_ERROR.pixelName to PixelParameter.removeAtb(),
)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,13 @@ interface SyncPixels {
feature: SyncableType,
apiError: Error,
)

fun fireAskUserToSwitchAccount()
fun fireUserAcceptedSwitchingAccount()
fun fireUserCancelledSwitchingAccount()
fun fireUserSwitchedAccount()
fun fireUserSwitchedLogoutError()
fun fireUserSwitchedLoginError()
}

@ContributesBinding(AppScope::class)
Expand Down Expand Up @@ -255,6 +262,30 @@ class RealSyncPixels @Inject constructor(
}
}

override fun fireUserSwitchedAccount() {
pixel.fire(SyncPixelName.SYNC_USER_SWITCHED_ACCOUNT)
}

override fun fireAskUserToSwitchAccount() {
pixel.fire(SyncPixelName.SYNC_ASK_USER_TO_SWITCH_ACCOUNT)
}

override fun fireUserAcceptedSwitchingAccount() {
pixel.fire(SyncPixelName.SYNC_USER_ACCEPTED_SWITCHING_ACCOUNT)
}

override fun fireUserCancelledSwitchingAccount() {
pixel.fire(SyncPixelName.SYNC_USER_CANCELLED_SWITCHING_ACCOUNT)
}

override fun fireUserSwitchedLoginError() {
pixel.fire(SyncPixelName.SYNC_USER_SWITCHED_LOGIN_ERROR)
}

override fun fireUserSwitchedLogoutError() {
pixel.fire(SyncPixelName.SYNC_USER_SWITCHED_LOGOUT_ERROR)
}

companion object {
private const val SYNC_PIXELS_PREF_FILE = "com.duckduckgo.sync.pixels.v1"
}
Expand Down Expand Up @@ -302,6 +333,12 @@ enum class SyncPixelName(override val pixelName: String) : Pixel.PixelName {
SYNC_GET_OTHER_DEVICES_SCREEN_SHOWN("sync_get_other_devices"),
SYNC_GET_OTHER_DEVICES_LINK_COPIED("sync_get_other_devices_copy"),
SYNC_GET_OTHER_DEVICES_LINK_SHARED("sync_get_other_devices_share"),
SYNC_ASK_USER_TO_SWITCH_ACCOUNT("sync_ask_user_to_switch_account"),
SYNC_USER_ACCEPTED_SWITCHING_ACCOUNT("sync_user_accepted_switching_account"),
SYNC_USER_CANCELLED_SWITCHING_ACCOUNT("sync_user_cancelled_switching_account"),
SYNC_USER_SWITCHED_ACCOUNT("sync_user_switched_account"),
SYNC_USER_SWITCHED_LOGOUT_ERROR("sync_user_switched_logout_error"),
SYNC_USER_SWITCHED_LOGIN_ERROR("sync_user_switched_login_error"),
}

object SyncPixelParameters {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,10 @@ import com.duckduckgo.sync.impl.ui.EnterCodeViewModel.AuthState
import com.duckduckgo.sync.impl.ui.EnterCodeViewModel.AuthState.Idle
import com.duckduckgo.sync.impl.ui.EnterCodeViewModel.AuthState.Loading
import com.duckduckgo.sync.impl.ui.EnterCodeViewModel.Command
import com.duckduckgo.sync.impl.ui.EnterCodeViewModel.Command.LoginSucess
import com.duckduckgo.sync.impl.ui.EnterCodeViewModel.Command.AskToSwitchAccount
import com.duckduckgo.sync.impl.ui.EnterCodeViewModel.Command.LoginSuccess
import com.duckduckgo.sync.impl.ui.EnterCodeViewModel.Command.ShowError
import com.duckduckgo.sync.impl.ui.EnterCodeViewModel.Command.SwitchAccountSuccess
import com.duckduckgo.sync.impl.ui.EnterCodeViewModel.ViewState
import kotlinx.coroutines.flow.launchIn
import kotlinx.coroutines.flow.onEach
Expand Down Expand Up @@ -96,14 +98,22 @@ class EnterCodeActivity : DuckDuckGoActivity() {

private fun processCommand(command: Command) {
when (command) {
LoginSucess -> {
LoginSuccess -> {
setResult(RESULT_OK)
finish()
}

is ShowError -> {
showError(command)
}

is AskToSwitchAccount -> askUserToSwitchAccount(command)
SwitchAccountSuccess -> {
val resultIntent = Intent()
resultIntent.putExtra(EXTRA_USER_SWITCHED_ACCOUNT, true)
setResult(RESULT_OK, resultIntent)
finish()
}
}
}

Expand All @@ -120,6 +130,26 @@ class EnterCodeActivity : DuckDuckGoActivity() {
).show()
}

private fun askUserToSwitchAccount(it: AskToSwitchAccount) {
viewModel.onUserAskedToSwitchAccount()
TextAlertDialogBuilder(this)
.setTitle(R.string.sync_dialog_switch_account_header)
.setMessage(R.string.sync_dialog_switch_account_description)
.setPositiveButton(R.string.sync_dialog_switch_account_primary_button)
.setNegativeButton(R.string.sync_dialog_switch_account_secondary_button)
.addEventListener(
object : TextAlertDialogBuilder.EventListener() {
override fun onPositiveButtonClicked() {
viewModel.onUserAcceptedJoiningNewAccount(it.encodedStringCode)
}

override fun onNegativeButtonClicked() {
viewModel.onUserCancelledJoiningNewAccount()
}
},
).show()
}

companion object {
enum class Code {
RECOVERY_CODE,
Expand All @@ -128,6 +158,8 @@ class EnterCodeActivity : DuckDuckGoActivity() {

private const val EXTRA_CODE_TYPE = "codeType"

const val EXTRA_USER_SWITCHED_ACCOUNT = "userSwitchedAccount"

internal fun intent(context: Context, codeType: Code): Intent {
return Intent(context, EnterCodeActivity::class.java).apply {
putExtra(EXTRA_CODE_TYPE, codeType)
Expand Down
Loading
Loading