feat: Support backing up tokens with the Block Store - #857
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Asynchronous writes can falsely succeed, and missing Block Store data prevents the existing derivation fallback.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds Auth token backup and restoration through Google Block Store while excluding credentials from Android full backups.
Changes:
- Adds flavor-specific Block Store support.
- Redacts database tokens during full backup.
- Restores tokens before passkey derivation.
File summaries
| File | Description |
|---|---|
gradle/core.versions.toml |
Registers Block Store dependency. |
Auth/build.gradle.kts |
Adds standard-flavor dependencies. |
Auth/src/standard/.../BlockStoreBackup.kt |
Implements token storage and restoration. |
Auth/src/fdroid/.../BlockStoreBackup.kt |
Adds unsupported-flavor stub. |
Auth/src/main/.../BlockStoreTokensBackup.kt |
Redacts tokens during full backup. |
Auth/src/main/.../RestoreFromBackupManagerImpl.kt |
Integrates restoration into account recovery. |
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 8
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🟡 Changes recommended
The current implementation can lose refresh tokens, overwrite valid credentials, and leak token remnants through file-level database backup.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (4)
Previously missed (1) — in code that hasn't changed since the last review.
Auth/src/main/kotlin/com/infomaniak/core/auth/backup/BlockStoreTokensBackup.kt:28
- This public API exposes
FullBackupAgentin its signature, but Auth declares:Commonas animplementationdependency, so an Auth consumer does not receive that type on its compile classpath and cannot use this helper without separately adding Common. Either expose Common as anapidependency or redesign/internalize the helper so its public signature does not leak an implementation dependency.
This issue also appears in the following locations of the same file:
- line 40
- line 63
Auth/src/main/kotlin/com/infomaniak/core/auth/backup/BlockStoreTokensBackup.kt:42
- Temporarily blanking SQL columns does not guarantee that the original credentials are absent from the file copied by full backup: SQLite can retain the previous values in WAL/journal files or unused page bytes. Since
defaultBackupCalls()backs up the database at file level, access and refresh tokens may still enter the regular Android backup. Exclude the live database and back up a sanitized export/copy instead (theFullBackupAgentdocumentation describes this staging approach).
val tokens = db.getUsersAndRemoveTokens()
try {
defaultBackupCalls()
Auth/src/main/kotlin/com/infomaniak/core/auth/backup/BlockStoreTokensBackup.kt:65
- This restores only the access token, so every successful backup permanently clears any non-null refresh token from the live database. That breaks the default offline-token flow:
TokenAuthenticator.kt:51-58treats a null refresh token as infinite and cannot refresh it after a 401. Restore the complete token snapshot after the temporary redaction.
usersWithTokens.forEach { user ->
// We don't need the refreshToken even if it's there because we're using passkeys instead.
userDao().updateUserToken(user.id, user.apiToken.accessToken)
Auth/src/standard/kotlin/com/infomaniak/core/auth/backup/BlockStoreBackup.kt:108
restoreTokens(targetUsers)is entered because only some users have empty tokens, but this loop applies every stored entry and can overwrite another user's still-valid local token with an older Block Store value. Restrict updates to the empty-token target users (preferably atomically in the DAO) so partial recovery cannot roll back valid credentials.
backup.forEach { userId, accessToken ->
db.userDao().updateUserToken(userId = userId.toInt(), accessToken = accessToken)
}
- Files reviewed: 8/8 changed files
- Comments generated: 1
- Review effort level: Balanced
9e9c1ec to
ccf4dc9
Compare
There was a problem hiding this comment.
🟢 Approval recommended
The current changes address the previously identified correctness and dependency-attribution issues without leaving a verified blocker.
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 0 new
- Review effort level: Balanced
ccf4dc9 to
2c87bf3
Compare
1baa1fd to
d67e84f
Compare
This fixes worker scheduling loop.
This will ensure we lose track of the fact that a token is temporary token. It will also allow them to be backed-up, though it's very unlikely that a backup happens during this transition period.
480de64 to
8f25c3c
Compare
|
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The backup flow permanently loses refresh tokens and leaves temporary-user credentials in the Android backup.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 2
No description provided.