feat(passkey): add delete passkey feature - #73
Merged
Conversation
this also moves the timing of the passkey creation after the item creation/selection
OffRange
commented
Aug 16, 2026
OffRange
left a comment
OffRange
left a comment
Owner
Author
There was a problem hiding this comment.
Review of the delete-passkey feature. The one I would treat as a blocker is CreatePasskeyActivity rendering caller-controlled rp.id as HTML; the rest are correctness and flow issues around the registration call moving after item selection.
One note with no line in the diff to hang on: rust/rust-code/lib/src/passkey/registration.rs:63 sets let user_name = creation_options.rp.name.clone() two lines above let user_display_name = creation_options.user.display_name.clone(), so the PasskeyUser this PR now writes carries the site's name rather than the account name. With rp.name = "Example Inc." and user.name = "alice@example.com" every passkey for that site is labelled with the site, which is exactly the case the PasskeyDao comment below makes destructive. Looks like it should read creation_options.user.name.
The relying party id in the passkey dialogs comes straight from the calling app's requestJson and was substituted into an HTML string resource that AnnotatedString.fromHtml then parsed, so a caller could put markup, including a tappable link, inside KeyGo's own credential dialogs. Add htmlStringResource, which HTML-escapes every format argument before substitution so only the resource itself can contribute markup, and route the "passkey already exists" and "delete passkey" dialogs through it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Registration moved out of runOperation into associatePasskeyAndFinish, which left nothing to stop it from running twice. confirmationEvent is only nulled inside the same onClick, so a double tap on Yes in the link-passkey dialog can land before recomposition. Two runs mint two key pairs and credential ids, store both, and send Finish with two different responses. The relying party only learns about whichever wins the race, so the other private key stays in the vault as a credential the site has never heard of and later reports the item as excluded on a credential id the relying party does not know. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Registration had moved to after the item was created and committed, so a failure left an orphan behind. In the create-new-item path a pending passkey keeps CreateNewOrUpdateLoginUseCase.isEmpty from firing even though buildCreate sets passkeyRPs to the empty set, so a login with nothing but a name gets written. A failing registerWithResult then aborts the activity and the user is left with a login that has no password, no TOTP, no passkey, and no hint where it came from. Register right after unlock again, so a failure aborts while there is still nothing to leave behind. The steps are strictly sequential, so express them that way: one coroutine running exclusion check, unlock, registration, item choice, store and finish, in that order. The two points where it waits on the user become suspension points on a CompletableDeferred each. Completing one is idempotent, so the guard against a repeat associatePasskeyAndFinish carries over from the previous commit without needing its own flag, and the registration response stays a local instead of becoming a field. The response also carries the rp id the authenticator resolved, which is populated even when the relying party omits rp.id from its request, so the confirmation dialog and the pending chip now read that instead of the request's own possibly empty value. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The null check missed the empty case. rp.id is optional per WebAuthn, and the
Rust side reads it as creation_options.rp.id.unwrap_or_default(), so a request
that omits it arrives as "". That passed the null check and added
LoginPasskeyInfo("", pending = true): a blank disabled chip under Passkey
Information, hasAnyContent flipped true so a name-only login looked saveable,
and a confirmation dialog asking whether to link the passkey for nothing.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Deleting by relying party is lossy the moment one login holds two passkeys for the same site, for example two accounts on example.com. LoginProjection.toDomain collapsed both rows into a single rp string, so the editor showed one chip for two credentials and the user could not see there were two. Deleting that chip put example.com into the removal set and deleteRPsNotIn wiped both rows, while the dialog spoke about "the passkey" in the singular. Those rows carry key material and cannot be reconstructed. Carry the credential id alongside the relying party: - PasskeyRef pairs a credential id with its rp, and Login.passkeyRPs becomes Login.passkeys, one entry per credential. - PasskeyDao.deleteRPsNotIn becomes deleteCredentialsNotIn, keyed on credential id. - UpsertLogin.removedPasskeyRPs becomes removedPasskeys and travels as refs, so the use case subtracts exactly the credentials the user dropped. - The editor renders one chip per credential and the delete dialog carries the ref it is about. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
#72 moved LegacyItemConverter.kt to legacy-migration and its copy of the Login(...) call had no passkeys argument, so whichever of the two branches merged second would fail to compile now that the default is gone from Login. #72 landed first, so the argument is carried over onto its relocated file here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #69