Conversation
There was a problem hiding this comment.
Pull request overview
This PR makes backup restoration restartable by splitting restoration into resumable steps (tracked via a new account status) and by making passkey deletion idempotent, while also refactoring key management instantiation and adding key enumeration/sorting to support restoration recovery.
Changes:
- Introduce
AccountRestorerand a newAccountEntitystatus to allow restoration to resume after cancellation/restart. - Make passkey deletion tolerant of “already deleted” (404) responses (
deletePasskeyIfExists). - Refactor
KeyPairManagerconstruction to an expect/actual factory and addgetSortedKeyIds()for restoration logic.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| multiplatform-lib/src/commonMain/kotlin/internal/repositories/WebAuthnRepository.kt | Adds deletePasskeyIfExists to ignore 404 during passkey deletion. |
| multiplatform-lib/src/commonMain/kotlin/internal/managers/MigrationManager.kt | Delegates restoration to new AccountRestorer. |
| multiplatform-lib/src/commonMain/kotlin/internal/managers/AuthenticatorManager.kt | Switches to KeyPairManager() factory and uses deletePasskeyIfExists. |
| multiplatform-lib/src/commonMain/kotlin/internal/managers/AccountRestorer.kt | New restartable restoration workflow handling aborted runs. |
| multiplatform-lib/src/commonMain/kotlin/internal/db/AccountEntity.kt | Adds DeletingOldKeyAfterRestoration status for resumable restoration. |
| multiplatform-lib/src/commonMain/kotlin/internal/KeyPairManagerImpl.kt | Removes the previous expect KeyPairManagerImpl. |
| multiplatform-lib/src/commonMain/kotlin/internal/KeyManager.kt | Introduces expect fun KeyPairManager() factory and getSortedKeyIds(). |
| multiplatform-lib/src/commonMain/kotlin/internal/AuthenticatorFacadeImpl.kt | Treats the new restoration status as restorable; minor comment/whitespace tweaks. |
| multiplatform-lib/src/appleMain/kotlin/internal/extensions/TollFreeBridgedTypes.kt | Adds CFDateRef → NSDate bridging helper. |
| multiplatform-lib/src/appleMain/kotlin/internal/extensions/CFMutableDictionaryRef.kt | Adds dictionary get operator + imports to support attribute access. |
| multiplatform-lib/src/appleMain/kotlin/internal/extensions/CFArrayRef.kt | Adds CFArray helpers (get, size) used by keychain enumeration. |
| multiplatform-lib/src/appleMain/kotlin/internal/KeyPairManagerImpl.apple.kt | Implements KeyPairManager() factory, adds getSortedKeyIds(), refactors keychain enumeration. |
| multiplatform-lib/src/androidMain/kotlin/internal/KeyPairManagerImpl.android.kt | Implements KeyPairManager() factory and getSortedKeyIds() via filesystem enumeration. |
| multiplatform-lib/src/androidDeviceTest/kotlin/internal/KeyPairManagerTest.kt | Updates test package and switches to the new KeyPairManager() factory. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
2ac8e29 to
46e8ff3
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (1)
multiplatform-lib/src/appleMain/kotlin/internal/KeyPairManagerImpl.apple.kt:190
- deleteKeysMatching() iterates only private keys (query uses
kSecAttrKeyClassPrivate) and deletes by the private tag. Since key generation stores the public key under a different tag ("$tag.pub"), this leaves the public key orphaned in the Keychain. Consider also deleting the corresponding public key tag when deleting a private key match.
for (i in 0 until count) {
val tag = extractTagFromItem(resultsArray[i.toLong()]) ?: continue
if (predicate(tag)) {
deleteKeyByTag(tag)
hasDeletedAtLeastOneKey = true
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| internal fun CFDateRef.toNSDate(): NSDate = CFBridgingRelease(this) as NSDate | ||
|
|
There was a problem hiding this comment.
CFDateRef.toNSDate() uses CFBridgingRelease(this) which releases the CoreFoundation object. That is only safe if the CFDateRef is a +1 (owned) reference; for values fetched from a dictionary/attributes, it's typically unowned and releasing can lead to over-release and crashes. Consider either retaining before bridging release, or providing a conversion that doesn't release ownership when the reference isn't owned.
|



No description provided.