Conversation
|
| } | ||
|
|
||
| private suspend fun discardRingSession() { | ||
| val revocationError = runSuspendCatching { pubkyService.signOut() }.exceptionOrNull() ?: return |
There was a problem hiding this comment.
If a pick saves a session but later fails, a successful signOut() returns here without deleting PAYKIT_SESSION. The service revokes the session but does not remove that saved value, while rollback deletes its Ring reference. The failed pick therefore leaves a saved session with no Ring source to provide its secret key.
There was a problem hiding this comment.
Not an issue: a successful signOut() does not leave PAYKIT_SESSION behind. In paykit-rs rc56, sign_out revokes the grant and then calls clear_local_identity_state(). That calls the provider's clearSessionAccess() in PaykitSdkService.kt, which deletes PAYKIT_SESSION, PUBKY_SECRET_KEY and SHARED_PUBKY_SOURCE. If that local clear fails, sign_out returns an error and the rollback falls through to forgetSessionAccess(), then to clearSessionAccess() plus an explicit PAYKIT_SESSION delete.
There was a problem hiding this comment.
You’re right. With paykit-rs rc56, a successful sign_out() calls clear_local_identity_state(), and PaykitSdkSessionProvider.clearSessionAccess() removes PAYKIT_SESSION along with the other local credentials and Ring reference. If that cleanup fails, sign_out() returns an error and the rollback proceeds through forgetSessionAccess() and the explicit local cleanup. The saved-session scenario from my comment is therefore covered; this finding is invalid.
Tip: You can customize Greptile's behavior for this repo with .greptile/rules.md and .greptile/config.json.
|
|
||
| private suspend fun hasSessionChangedSince(previousSession: Result<String?>): Boolean { | ||
| val currentSession = runSuspendCatching { keychain.loadString(Keychain.Key.PAYKIT_SESSION.name) } | ||
| if (previousSession.isFailure || currentSession.isFailure) return true |
There was a problem hiding this comment.
Unreadable session is discarded
If an existing session could not be read during initialization, a Ring pick can proceed with no public key in memory. If that pick then fails before saving a new session, this read failure is still treated as proof that the session changed. Rollback can revoke or forget the user's pre-existing session and delete its Ring reference.
There was a problem hiding this comment.
Fixed in 26cab51: the pick now reads the saved session and the Ring reference before it writes anything, and aborts if either read fails. So an unreadable session is kept, not discarded. adoptRingIdentity should abort the pick when the saved session cannot be read covers it.
| val adoptedPubky = reference.removePrefix(SharedPubkyContract.RING_SOURCE_PREFIX) | ||
| Logger.warn("Adopted ring identity '${redacted(adoptedPubky)}' is gone, clearing session", context = TAG) | ||
| runSuspendCatching { pubkyService.clearSessionAccess() } | ||
| .onFailure { Logger.warn("Failed to clear adopted session access", it, context = TAG) } | ||
| clearLocalState() |
There was a problem hiding this comment.
Stale check clears new identity
A foreground source check can read the old Ring reference, then wait for the Ring response while a new pick commits. If the response says the old pubky is gone, this code clears the session and identity without checking whether the reference changed. The newly adopted identity can be cleared by the earlier check.
…d-ring-pick # Conflicts: # app/src/main/java/to/bitkit/repositories/PubkyRepo.kt # app/src/main/java/to/bitkit/services/PubkyAuthHandlerRegistrar.kt # app/src/test/java/to/bitkit/repositories/PubkyRepoTest.kt # app/src/test/java/to/bitkit/services/PubkyAuthHandlerRegistrarTest.kt
Regtest APKDownload bitkit-dev-debug universal APK (expires in 30 days). |
jvsena42
left a comment
There was a problem hiding this comment.
One MEDIUM inline. Ring adoption is unreleased; Paykit is on by default on master.
#1386 and #1393 both fix #1363 with opposite designs: #1386 rolls a pick back on Back, #1393 finishes it. Only one should merge. They conflict in PubkyRepo.kt and PubkyRepoTest.kt, but PaykitSdkService.kt auto-merges, so resolving PubkyRepo toward #1393 still brings in #1386's activateOrClearLocked. In #1393's flow, that cleanup deletes PAYKIT_SESSION before signInRingIdentity checks hasSessionChangedSince. A first pick then falls through to the sign-up fallback #1393 removed, and with the Ring reference already deleted, persistSessionAccess saves the Ring secret as a local key (the mechanism in greptile r4139882837). Close whichever one is not chosen.
Checked and clean:
- The manifest change is a comment only, and
MainActivityPubkyAuthis unchanged. adoptRingIdentity's only caller is a user tap, and no relaunch or deep link completes a pick.- A cancelled lock wait decrements
waitingRingPicks. - Failed-pick rollback deletes only a reference it wrote, and skips the sign-up fallback once a session was saved.
activateBootstrapResultresetspubkyStore/SDK state on an owner change.- Shipped local identities are untouched.
| } | ||
| .onFailure { | ||
| Logger.error("Failed to adopt ring identity", it, context = TAG) | ||
| if (it is PubkyAlreadySignedInError) { |
There was a problem hiding this comment.
Picking a different pubky while an abandoned pick finishes signs in as the abandoned one.
After Back, pick A keeps running under NonCancellable, and _publicKey is published only at the end of commitRingIdentity (PubkyRepo.kt:441), so isAuthenticated stays false for the rest of the pick. During that window, Drawer → Profile still routes to Pubky Choice. The new view model has adoptingPubky == null, and the Ring list is tappable.
Tapping pubky B queues on withRingPickLock. When A commits, B throws PubkyAlreadySignedInError (PubkyRepo.kt:381), and this branch opens Profile without a toast. The user picked B and is signed in as A. On master the same sequence ends as B, because Back rolls A back before B takes the lock.
Fix: only a retry of the same pubky needs the silent path. Compare the requested pubky with pubkyRepo.publicKey.value (PubkyPublicKeyFormat.matches) and show the auth error when they differ, or throw PubkyAlreadySignedInError from adoptRingIdentity only when _publicKey matches pubky. Add a PubkyChoiceViewModelTest case with a different signed-in pubky.
|
Closing in favour of #1386, which fixes #1363 by rolling the pick back on Back. The parts of this PR that don't depend on that choice will come in a follow-up once #1386 lands: the |
Fixes #1363
Refs: #1329, synonymdev/bitkit-ios#774
This PR makes a Pubky Ring pick on Pubky Choice end in one consistent state when the user presses Back mid-pick or activation fails, and re-enables the pubkyauth handler once Pubky Ring is reachable again.
Sign-in runs on
ServiceQueue.CORE, whose ownSupervisorJobkeeps it running after the caller is cancelled. A Back press at any point during sign-in therefore saved and activated the session, and only then cancelled the pick, skipping the signed-in state, the pending profile setup flag and the failure cleanup.Adoption was left cancellable in #1329 (r4093783423) because an abandoned pick that failed later could delete the reference of a retried pick. Serializing picks removes that race, so a pick can now finish.
#1339 reworked adoption on master while this was open: Back during sign-in rolled the pick back, and the pending flag was still written after the profile and contact load. This PR replaces that with the finish-on-Back behaviour above, and keeps #1339's lock order, its per-identity cache reset, its re-checked source-gone clear and its rule that an automatic restore retry does not replace a pick (
paykit-clock-changes.md, Connection loss step 5).Description
NonCancellable, the identity is published last, and contacts still load, matching iOS where the pick is never cancelled.pubkyauth://links reach Bitkit again without a restart.Out of Scope
PubkyRepo: process death mid-pick;NonCancellablecovers cancellation only, the same as iOS. A durable pick-in-progress marker would close it.PubkyRepo: remote residue of a failed pick, such as the receiver marker, the identity republish and a grant whose revoke fails. On the emulator, revoking after an injectedhandle.initialize()failure returnedidentity_error("restore Pubky grant before revocation") and the rollback fell back to forgetting the session locally.ui/screens/profile: a pick that finishes after Back shows no spinner if Pubky Choice is reopened and offers no contact import; the header turns signed in a few seconds after Back.PubkyRepo:createIdentity, sign-out and the Ring source check are not serialized with a pick.discardAbandonedSession, in a separate PR.Design
N/A — no UI changes.
Preview
N/A — no UI changes. Driven on an API 36 emulator with Pubky Ring at 57ab5c5, before the review fixes and the merge with master: Back mid-pick saved the session 3.4 s later and finished with the identity signed in and no toast, a retried pick opened Profile, an injected activation failure left no session or reference, and the auth handler turned off and on again as Pubky Ring was disabled and re-enabled.
QA Notes
Journeys
N/A — not drivable; see Manual Tests.
Manual Tests
MainActivityPubkyAuthdisabled; enable Pubky Ring, reopen Bitkit → enabled again without a restart — Pubky Ring not in Capabilitiesregression:tap a Ring pubky and wait → Profile, Pay Contacts or Create Profile as before — Pubky Ring not in CapabilitiesAutomated Checks
PubkyRepoTest.kt— a pick cancelled during sign-in (through the realServiceQueue), the profile lookup or the contact load finishes with the identity, pending setup and contacts, and one cancelled before the reference write writes nothingPubkyRepoTest.kt— a failed pick discards the session it saved before the reference, falls back to forgetting and then clearing it, never signs up after a saved session, and keeps a session from before the pickPubkyRepoTest.kt— a retried pick waits for an abandoned one and gets already signed in, and the Ring reachability flag follows the source check and a finished pickPaykitSdkServiceTest.kt— session teardown deletes the credentials before the Ring reference and keeps the reference when that failsPubkyChoiceViewModelTest.kt— an already signed-in pick opens Profile without a toast, and a second tap during a pick is ignoredPubkyAuthHandlerRegistrarTest.kt— the auth handler follows Pubky Ring becoming reachable or unreachable for the same pubkyPubkyRepoTest.kt— a cold start publishes a restored identity over a queued pick, an automatic restore retry yields to a pick waiting for the lock, and an unreadable session or reference aborts the pick with nothing writtenPubkyRepoTest.kt— a mismatching credential no longer touches the Ring referencePubkyRepoTest.kt— fix: recover interrupted paykit sessions #1339's adoption tests now expect a cancelled profile lookup to finish the pick and a wipe to wait for it, and a pick over a signed-in identity to be refused