Repository navigation
fix: open pubky sign-in links while the session restore is deferred - #1543
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
[Medium impact] The changes are centered on Pubky sign-in and authorization. The PR appears safe to merge, with a non-blocking gap in tests for the approval setup rule. Findings
SummaryOpens Pubky authorization from a stored local key while session restoration is deferred.
Acknowledged scope: Diagram%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Pubky sign-in link] --> B[Attempt session restore]
B --> C{Identity ready?}
C -->|Yes| F[Open Authorize sheet]
C -->|No| D{Stored local key without Ring source?}
D -->|Yes| F
D -->|No| E[Keep waiting for restore]
F --> G[User approves]
G --> H{Claim includes Paykit access?}
H -->|Yes| I[Wait for full runtime setup]
H -->|No| J[Ensure platform setup]
I --> K[Send signed approval]
J --> K
Reviews (1) · Last reviewed commit: "fix: open pubky sign-in links while the ..." · Reviewed by Greptile |
|
Pushed 51339d0:
|
|
Pushed 0f20f71: a |
talosmachina
left a comment
There was a problem hiding this comment.
No findings. Lets a cold-start sign-in link open the Authorize sheet with the stored key's identity while the session restore is deferred, and lets approval wait only for the platform setup unless the claim grants Paykit access. Reviewed 51339d0, full tier (auth and keys path). Reasoned from the code and CI; the branch was not built here.
What I checked, and 5 candidates I ruled out
Read in full: AppViewModel.kt (awaitPubkyAuthIdentity, handlePubkyAuth, showPubkyIdentityUnavailableToast), PubkyAuthApprovalViewModel.kt (load, authorize, approveSignInRequest), PubkyRepo.kt (localIdentityPublicKey, approveAuth, approveAuthWithCompanionClaim, activeSecretKeyHex, managedSecretKeyFor, initializeSession, restoreSessionBackupState), PaykitSdkService.kt (initialize, approveAuth, approveAuthWithCompanionClaim, ensurePlatformSetup, persistSessionAccess, activateBootstrapResult)
Call sites traced: localIdentityPublicKey (3), ensurePlatformSetup (2), every writer of PUBKY_SECRET_KEY and PAYKIT_SESSION
CI: lint, detekt, build-local green at 51339d0; the build job with the unit tests was still running when this was posted
Ruled out
- Sheet shows one identity, approval signs with another: with
_publicKeynull,activeSecretKeyHex()returns the samePUBKY_SECRET_KEYthatlocalIdentityPublicKey()derives from, and every writer (persistSessionAccess,restoreSessionBackupState,createLocalIdentitySession) writes or deletes it together with the session. Narrowest of the five. - Ring-sourced identity signs with a stale local key:
hasLocalSecretKey()excludesSHARED_PUBKY_SOURCE, andpersistSessionAccessdeletes the local key when an adopted pubky exists. - Grant signup re-creating an identity:
requiresIdentityCreation(identityPublicKey != null)now routes it toapproveAuth; before, it went toapproveSignupAuthand failed withPubkyAlreadySignedInError. ensurePlatformSetupracinginitialize()outsidesetupMutex:PaykitAndroid.initializeOrThrowalready runs outside that lock inlaunchIdentityRepublish(PaykitSdkService.kt:389), so a repeated call is an existing pattern.- Offline cold start skipping the wait:
51339d0gates the early return onConnectivityState.CONNECTED, and the newAppViewModelSendFlowTestcase covers it.
Merge confidence: 4/5, no findings, but the change is on the auth path and the unit-test job had not finished on this head.
Regtest APKDownload bitkit-dev-debug universal APK (expires in 30 days). |
jvsena42
left a comment
There was a problem hiding this comment.
Code scan at 0f20f71: no code findings. I reviewed it as an auth and consent change. It addresses #1541.
Checked and clean:
- Signer equals displayed identity. The early path requires
hasSavedSession() && hasLocalSecretKey()(PubkyRepo.kt:1539-1545) and shows the identity derived fromPUBKY_SECRET_KEY. With_publicKey == null, approval signs withactiveSecretKeyHex(), the same key (:2094).- Every writer keeps
PAYKIT_SESSIONandPUBKY_SECRET_KEYpaired:persistSessionAccess(PaykitSdkService.kt:1409-1417),restoreSessionBackupStateand every clear path. - If the restore lands between
load()and Authorize,managedSecretKeyFor(:2055-2081) returns only a key whose pubkey equals the restored identity, so it fails closed rather than signing as another identity.
- Every writer keeps
- Ring and stale keys.
hasLocalSecretKey()requires noSHARED_PUBKY_SOURCE, and Ring adoption writes the source first. A session with no key, or a key with no session, is excluded. - Signup.
requiresIdentityCreation(identityPublicKey != null)(VM:129) matcheshandlePubkyAuth. If the key vanishes,approveSignupAuthrechecks underinitializeMutex. - Lock screen. Unchanged:
launchScandefers while not authenticated, and the sheet's local-auth step is untouched. - Lifecycle. Background restore retries continue with
notifyFailure = false. The early open is gated onCONNECTED; offline is covered by a test. - SDK lock. Approval calls
ensurePlatformSetup()(:1560-1562) outsidesetupMutexand then waits FIFO onoperationLock, so there is no lock-order inversion or deadlock. Paykit-access claims still wait for full setup, as the body says. - Parity. iOS already opens the sheet from the keychain session and secret key (
AppViewModel.swift:1168-1215).
LOW, journeys: #### Journeys says N/A, not drivable. journeys/pubky-auth/signin-cold-start.xml drives exactly this route: force-stop, fresh signin_grant link, unlock, loading toast, Authorize, PubkyAuthOK, and the requester receives the grant. List it. Its <description> still says the app "waits visibly for the saved Pubky session", but the toast now ends after the first deferred attempt; update that sentence.
Performance-relevant (user-facing wait): the sign-in wait now ends after the first deferred restore attempt instead of up to 60 s. An Authorize tap during an in-flight restore attempt waits behind operationLock for up to one attempt (~20 s on the #1514 gate log).
Device gate (partial): 0f20f71 — not run yet. Pending: the #1541 repro (create a profile, then force-stop and open a fresh signin_grant link, ~8 times). On runs logging Deferred session restoration … shared_state_busy, expect the sheet within seconds, the sheet's pubkey equal to the later Restored paykit session key, Authorize → PubkyAuthOK, and the grant from that key. Also: the PIN-on variant, an offline 25 s cut, and the time from Authorize to OK during an in-flight attempt. The device result will follow.
@jvsena42 done in 0c623ec: the Journeys list names |
jvsena42
left a comment
There was a problem hiding this comment.
Approving at 0c623ec. My journeys LOW is addressed: signin-cold-start.xml is listed and its description now covers the deferred restore, a docs-only change on top of the gated 0f20f71.
Device gate: 0f20f71 (app code identical at 0c623ec) — journeys: signin-cold-start.xml 9/9 pass, with the restore deferred twice in that run. perf: am start → sheet 14.7–16 s without a deferral, ~22–26 s with one; Authorize → PubkyAuthOK 8–14 s, and 5.5 s when tapped with a deferral in flight. No crash or ANR. l10n: no UI text changed.
- #1541 repro: 9 force-stop +
am startruns on a fresh profile. Every run opened the Authorize sheet, and none showed "Couldn't Load Your Pubky Profile". The 3 runs that deferred (r5, r8, r9) continued with the stored key at ~22 s:18:35:36.939 WARN [PubkyRepo.kt:531] Deferred session restoration, keeping saved session [SharedStateBusy='code=shared_state_busy, …'] 18:35:50.585 WARN [PubkyRepo.kt:531] Deferred session restoration, keeping saved session [AppError='code=shared_state_busy, …'] 18:35:50.605 DEBUG [AppViewModel.kt:3153] Continuing sign-in link with the stored Pubky key 18:35:54.832 INFO [PubkyAuthApprovalViewModel.kt:251] Auth approved for 'gate.test' 18:36:44.167 INFO [PubkyRepo.kt:479] Restored paykit session for 'pubkygb…4a9xmto' - Identity: in all 10 runs, the sheet's key
GBD1…XMTOequals the later restoredpubkygb…4a9xmtoand the key the requester received. - PIN on: the PIN pad shows first, with no sheet behind it for 37 s. The sheet opens about 3 s after unlock, and Authorize asks for the PIN before
PubkyAuthOK. - Airplane cut (~27 s): the loading toast stays up offline with no failure toast, and the stored-key shortcut is not taken offline.
Retrying Pubky session restorefires onCONNECTED, and the sheet follows.
Seen but outside this PR: BackupRepo.kt:600 Backup failed for: 'WALLET' … shared_state_busy is logged at ERROR while the shared state is locked.
CI: 13 checks green; one was still pending when I posted.
ben-kaufman
left a comment
There was a problem hiding this comment.
Reviewed 0c623ec and the existing feedback. No blocking findings. The saved-session identity path and separation of platform authorization from Paykit setup are covered by 779 passing focused repository, service, authorization, and navigation tests. Current hosted checks are green. Device journeys were not rerun as part of this review.
Closes #1541
Refs:
This PR opens the Authorize sheet for a Pubky sign-in link that cold-starts Bitkit while the Pubky session restore is deferred. Today the link waits for the restore and fails after 60 s when the Paykit shared state stays locked (device logs).
Description
shared_state_busy. The session import itself takes no lock, but the Paykit runtime start that follows it (initializeand the noise key publication) takes the homeserver lock on the shared state. That lock has a 60 s lease, and a killed process leaves it held until the lease runs out (up to 5 minutes after an interrupted write). The restore is deferred for that whole time.persistSessionAccesswrites both, and deletes the key when the session has none). A Ring-sourced identity is still excluded (hasLocalSecretKey), so it keeps waiting for the restore as before. Approval still signs only with that key.Out of Scope
AppViewModel: a later link replacing a pending sign-in link, now tracked in bug: a link opened during a pending pubky sign-in drops the sign-in #1542.PubkyRepo: the restore retry schedule for the rest of the app (profile, contacts, payments) while the shared state stays locked.PaykitSdkService: sign-in links whose Bitkit claim grants Paykit access still wait for the full Paykit setup; the Pubky Shop sign-in carries no Bitkit claim.Design
N/A — no UI changes.
Preview
N/A — no new UI; the existing Authorize sheet opens sooner. The locked shared state cannot be produced on demand, so the change is covered by unit tests and the manual test below.
QA Notes
Journeys
signin-cold-start.xml— the sign-in link opens the Authorize sheet after the loading toast, also when the restore is deferred while onlineManual Tests
Deferred session restoration … shared_state_busy, and Authorize succeeds — a locked shared state is not in CapabilitiesAutomated Checks
AppViewModelSendFlowTest.kt— a cold sign-in link opens the sheet with the stored key after one deferred restore while online, and keeps waiting offlinePubkyAuthApprovalViewModelTest.kt— a grant signup with only a stored key shows that identity and authorizes without identity creationPaykitSdkServiceTest.kt— approval waits only for platform setup, unless its claim grants Paykit accessPubkyRepoTest.kt— the stored key's identity is read only with a saved session and without a Ring source