mirror of
https://github.com/vitorpamplona/amethyst.git
synced 2026-08-09 08:04:45 +00:00
Vitor PamplonaandClaude Opus 4.8
b0668baeec
Three defects, each of which made revocation look like it worked. **`revokeSessionGrants` had no callers.** It was added with the KDoc "so revoking an app takes effect immediately instead of lingering until this broker instance dies" and then never wired, so revoking an app in Connected Apps left its in-memory session grants active. The user revokes; the app keeps signing. **And it was broken as written.** `sessionAllows` keys are the account-namespaced `napplet:<signer>:<coordinate>|<op>`, but every revoke call site holds the BARE coordinate, so the prefix match found nothing. Wiring it naively would have looked correct and silently done nothing. It now namespaces before matching, and also clears the post-Cancel re-prompt cooldown so a revoked app prompts on next use instead of being quietly dropped. **Worse: there were three ledgers.** `NappletBrokerService`, `ConnectedAppsScreen` and `ConnectedAppDetailScreen` each constructed their own `NappletPermissionLedger`, while ALLOW_SESSION grants are per-instance in-memory state. So "Forget" cleared the screen's own always-empty session map while the grants the broker actually consults lived on. The KDoc described a process-wide singleton; it wasn't one. Promoted to a real singleton in AppModules alongside the existing permission store, and shared by all three. The screens are plain composables with no binder to the broker service, so rather than invent an IPC path the cached broker moved to the service's companion under a lock — matching the sibling main-process registries in that package. Both revoke paths call it: the Forget button and the per-op revoke. Also gives `NappletPermissionLedger.endSession()` its first caller, which promoting the ledger made necessary: it used to die with the service, so session grants had a natural bound. Now that it outlives the service, `onDestroy` restores exactly the lifetime ALLOW_SESSION already implied. The boundary is safe — the service is bind-only and is destroyed only once every applet and browser surface has unbound, so switching between two open applets never drops grants mid-use. Deliberately NOT wired to account switch (already handled by account-keying) or to backgrounding (would re-prompt mid-use). Test notes, kept honest: the revoke test was verified to fail before the namespacing fix. The `endSession` test PASSES without the change — `endSession` itself was always correct, the bug was that nobody called it — so it is characterization for the new lifetime contract, not a regression test. The `onDestroy` wiring and the composable click handlers have no automated coverage; `amethyst` has no Robolectric and no harness was invented for them. Known gap, left alone deliberately: changing an app's trust level to PARANOID does not drop its live session grants, because `sessionAllows` is consulted before the signer ledger. That is a revoke-shaped action and belongs in the same fix, but it is a behaviour change and was out of scope tonight. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>