feat: Data transfer support - #825
Conversation
There was a problem hiding this comment.
Pull request overview
Adds account data-transfer recovery by detecting device changes and re-deriving restored authentication tokens.
Changes:
- Persists Android ID bindings for user tokens via Room.
- Adds restoration orchestration, request blocking, retry/give-up UI, and error handling.
- Moves token derivation into Auth and updates account APIs.
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
CrossAppLogin/README.md |
Updates account-removal guidance. |
CrossAppLogin/Back/.../SharedDeviceIdStorage.kt |
Reuses the shared Android ID helper. |
CrossAppLogin/Back/.../BaseCrossAppLoginViewModel.kt |
Uses Auth token derivation APIs. |
Common/.../AndroidId.kt |
Adds the shared Android ID helper. |
Auth/.../UserAccountUtils.kt |
Stores token-device bindings. |
Auth/.../TokenInterceptor.kt |
Waits for restoration handling. |
Auth/.../room/UserDatabase.kt |
Adds the binding entity and migration. |
Auth/.../room/UserDao.kt |
Adds binding queries and user-list APIs. |
Auth/.../PersistedCurrentUserAccountUtils.kt |
Accepts deferred cleanup providers. |
Auth/.../models/TokenDeviceBinding.kt |
Defines token-device persistence. |
Auth/.../DerivedTokenGeneratorImpl.kt |
Moves derivation implementation into Auth. |
Auth/.../DerivedTokenGenerator.kt |
Exposes the derivation contract. |
Auth/.../DerivedTokenGenerator.IssueExtensions.kt |
Centralizes issue policies. |
Auth/.../CredentialManager.kt |
Integrates modern account utilities. |
Auth/.../backup/RestoringFromBackupFailedScreen.kt |
Adds restoration failure UI. |
Auth/.../backup/RestoreFromBackupManagerImpl.kt |
Implements restoration handling. |
Auth/.../backup/RestoreFromBackupManager.kt |
Defines restoration state and API. |
Auth/.../AbstractCurrentUserAccountUtils.kt |
Accepts deferred cleanup providers. |
Auth/schemas/.../9.json |
Records Room schema version 9. |
Auth/build.gradle.kts |
Adds Compose UI dependencies. |
| is AppIntegrityIssue.RetryLater, is AppIntegrityIssue.Internal -> true | ||
| is AppIntegrityIssue.DeviceIssue, is AppIntegrityIssue.DevError, is AppIntegrityIssue.SuspiciousError -> false | ||
| } | ||
| is Issue.ErrorResponse -> true |
| override val state: SharedFlow<State> = flow { | ||
| performRestorationHandlingIfNeeded() | ||
| emit(State.Settled) | ||
| }.distinctUntilChanged().shareIn(coroutineScope, SharingStarted.Eagerly, replay = 1) |
There was a problem hiding this comment.
@copilot Can you make a PR targeting this branch that tests it? I'm curious to see the approach you will pick.
|
TODO: Ensure we can disable this for the Authenticator app. |
| private suspend fun FlowCollector<State>.performRestorationHandlingIfNeeded() { | ||
| val users = userDao.allUsers() | ||
| val currentAndroidId = getAndroidId() | ||
| restoreAccounts(currentAndroidId = currentAndroidId, allUsers = users) |
There was a problem hiding this comment.
I find it easier to reason about like this, but let's talk about it offline.
There was a problem hiding this comment.
rotateTokenForUsers, allusers is too contextual.
| currentAndroidId: String, | ||
| allUsers: List<User>, | ||
| ) { | ||
| val usersToDeriveTokensFor: List<User> = coroutineScope { |
There was a problem hiding this comment.
An early return if allUsers is empty ?
There was a problem hiding this comment.
It's not going to change much, but added it.
| if (result !is Xor.Second) return@also | ||
| val issue = result.value | ||
| val errorMessage = "Failed to derive token" | ||
| val sentryUser = io.sentry.protocol.User().also { it.id = user.id.toString() } |
There was a problem hiding this comment.
fully qualified import. Could Be an alias ?
There was a problem hiding this comment.
Could be, but for one usage, and for a such a short FQDN, I thought this was good enough.
| } | ||
| } | ||
|
|
||
| private const val TAG = "RestoreFromBackupManagerImpl" |
There was a problem hiding this comment.
Any reason to not put this in a companion object ?
There was a problem hiding this comment.
Well, no need for a companion object which would lightly bloat the binary.
| fun RestoringFromBackupFailedScreen( | ||
| state: RestoreFromBackupManager.State.RestoringFromBackupFailed, | ||
| modifier: Modifier = Modifier, | ||
| ) = AspectRatioFlow(modifier) { isLandscape -> |
There was a problem hiding this comment.
The term Flow here is weird.
There was a problem hiding this comment.
It's the same as in FlowRow and FlowColumn, only that it can be any of Column or Row, here.
No Composable functions return a Flow, so at least, we don't have any ambiguity here.
| state: RestoreFromBackupManager.State.RestoringFromBackupFailed, | ||
| modifier: Modifier = Modifier, | ||
| ) = AspectRatioFlow(modifier) { isLandscape -> | ||
| if (isLandscape) Row( |
There was a problem hiding this comment.
having no bracket here but having them in the else make this code hard to read.
| retry = {}, | ||
| giveUp = {} | ||
| )) | ||
| } |
There was a problem hiding this comment.
I'm not sure we want to have UI things in the Auth module.
There was a problem hiding this comment.
Let's discuss this offline.
| } | ||
|
|
||
| internal fun Issue.shouldRetryAutomatically(): Boolean = when (this) { | ||
| is Issue.AppIntegrityCheckFailed -> when (details.issue) { |
There was a problem hiding this comment.
Not a big fan of the when in a when.
| private val userDataCleanableList: List<AssociatedUserDataCleanable> = emptyList(), | ||
| private val userDataCleanableList: () -> List<AssociatedUserDataCleanable> = { emptyList() }, | ||
| override val userDatabase: UserDatabase = UserDatabase.instantiateDataBase(appContext), | ||
| restoreFromBackupManager: RestoreFromBackupManager = RestoreFromBackupManager.instance, |
There was a problem hiding this comment.
Is there a case where restoreFromBackupManager is not RestoreFromBackupManager.instance
87eb958 to
bee0f91
Compare
This commit also renames LiveData returning getAll() to allAsLiveData()
This could cause notification and device/app registration to not be done again after the user got logged out and re-logged in.
This avoids clashes with the splashscreen's logo
We are not going to allow reusing them. However, we will probably consider using the "standard" API (over the "classic" one), to allow generating many tokens for cheaper thanks to the shared costly operation.
7fa8718 to
66a859f
Compare
|
No description provided.