Conversation
|
There was a problem hiding this comment.
Pull request overview
This PR adjusts the backup-restore migration flow to avoid deleting newly-created passkey keys and to improve status signaling during restore.
Changes:
- Update backup restore to fetch a token with the old passkey, register a new passkey, then delete only the old passkey/key material.
- Introduce reusable
KeyPairManager.Filtershelpers for userId/passkeyId key selection. - Emit
AttemptingToConnectstatus during restore-from-backup attempts.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
multiplatform-lib/src/commonMain/kotlin/internal/managers/MigrationManager.kt |
Changes restore sequence and key deletion strategy during backup restoration. |
multiplatform-lib/src/commonMain/kotlin/internal/managers/AuthenticatorManager.kt |
Switches to shared filter helpers and exposes keyPairManager for external cleanup use. |
multiplatform-lib/src/commonMain/kotlin/internal/KeyManager.kt |
Adds KeyPairManager.Filters predicate helpers for locating/deleting keys. |
multiplatform-lib/src/commonMain/kotlin/internal/AuthenticatorFacadeImpl.kt |
Emits an “AttemptingToConnect” status during restore-from-backup retries. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| object Filters { | ||
| fun forUserId(userId: Long): (name: String) -> Boolean = { it.startsWith("$userId-") } | ||
| fun forPasskeyId(passkeyId: String): (name: String) -> Boolean = { "-$passkeyId-" in it } |
There was a problem hiding this comment.
Filters.forPasskeyId() currently matches names containing "-$passkeyId-". That works for Android file names like "$userId-$keyId-private.key", but it won’t match the Apple Keychain tag format used in KeyPairManagerImpl.apple.kt (tag is "$userId-$keyId"), so deleteKeysMatching(Filters.forPasskeyId(...)) will be a no-op on Apple and can leave stale keys that may be picked up later by findKeyIdFor(Filters.forUserId(...)). Consider making the predicate compatible with both naming schemes (e.g., match "-$passkeyId-" OR endsWith("-$passkeyId"), or parse the keyId out of the name/tag and compare exactly).
| fun forPasskeyId(passkeyId: String): (name: String) -> Boolean = { "-$passkeyId-" in it } | |
| fun forPasskeyId(passkeyId: String): (name: String) -> Boolean = { | |
| "-$passkeyId-" in it || it.endsWith("-$passkeyId") | |
| } |
| persistToken(account.id, tokenWithNewPassKey.accessToken) | ||
| dao.upsert(account.copy(status = AccountEntity.Status.LoggedIn)) | ||
| // We can safely delete the old passkey, as the new one is working and the old token won't be valid anymore | ||
| authenticatorManager.deleteKeysFor(account.id) | ||
| webAuthnRepository.deletePasskey(tokenWithNewPassKey.accessToken, keyId) | ||
| webAuthnRepository.deletePasskey(tokenWithNewPassKey.accessToken, oldKeyId) | ||
| val _ = authenticatorManager.keyPairManager.deleteKeysMatching(keyFilters.forPasskeyId(oldKeyId)) | ||
| dao.upsert(account.copy(status = AccountEntity.Status.LoggedIn)) |
There was a problem hiding this comment.
dao.upsert(...LoggedIn) is now executed after backend/local cleanup. Since this method runs under withRetries, any failure in deletePasskey(...) will trigger a retry, but the retry path will register yet another new passkey again—potentially creating multiple passkeys server-side. Consider marking the account LoggedIn (and persisting the token) as soon as the new passkey token is confirmed, and then perform old-passkey/key cleanup as best-effort (or at least ensure cleanup failures don’t cause repeated passkey registrations).
| private val cryptoObjectsBuilder by lazy { CryptoObjectsBuilder() } | ||
| private val keyPairManager by lazy { KeyPairManagerImpl() } | ||
| val keyPairManager: KeyPairManager by lazy { KeyPairManagerImpl() } | ||
|
|
There was a problem hiding this comment.
keyPairManager was changed from private to an exposed val so other classes can reach into the underlying key store. This increases coupling and makes it harder to change key management internals later. Consider keeping keyPairManager private and adding a narrow AuthenticatorManager API for the needed operation (e.g. delete keys for a specific passkeyId) so callers don’t need direct access to the manager implementation.
No description provided.