-
Notifications
You must be signed in to change notification settings - Fork 10
Fix/eid wallet deeplink rendezvous #1141
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
Sahil2004
wants to merge
19
commits into
main
Choose a base branch
from
fix/eid-wallet-deeplink-rendezvous
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
19 commits
Select commit
Hold shift + click to select a range
932d291
fix(eid-wallet): show the consent screen when a deep link races cold-…
Sahil2004 9028542
refactor(eid-wallet): move deep-link storage into lib/stores
Sahil2004 0ad2a8c
refactor(eid-wallet): collapse the deep-link payload to one slot
Sahil2004 2342963
refactor(eid-wallet): drop the deep-link pass-through layer
Sahil2004 220cd97
docs(eid-wallet): move the deep-link architecture out of the source
Sahil2004 775f12c
refactor(eid-wallet): name the deep-link auth accessors for their con…
Sahil2004 c8197ee
refactor(eid-wallet): move session auth onto a SessionController
Sahil2004 721a1b8
chore(eid-wallet): drop a comment restating what logout already shows
Sahil2004 a64538a
chore(eid-wallet): drop the splash comment about not diverting to /login
Sahil2004 c5523cc
chore(eid-wallet): drop the pointer to where auth state lives
Sahil2004 452a277
fix(eid-wallet): prompt biometrics only on the splash
Sahil2004 da8cf88
test(eid-wallet): exercise the real routing code, not a copy of it
Sahil2004 7002676
docs(eid-wallet): correct four comments that outlived the code
Sahil2004 98ce3cc
fix(eid-wallet): stop the layout dispatching the event it listens for
Sahil2004 032bcea
fix(eid-wallet): treat finishing onboarding as being signed in
Sahil2004 7ea74e9
chore(eid-wallet): bump version to 1.1.1
Bekiboo 1360fdd
fix(eid-wallet): put node/pnpm on PATH for the Xcode build phase
Bekiboo 3dc6907
chore(eid-wallet): bump Android versionCode to 31
Bekiboo e165d35
chore(eid-wallet): set iOS build number to 1.1.1.1
Bekiboo File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
191 changes: 191 additions & 0 deletions
191
infrastructure/eid-wallet/docs/architecture/deepLink.md
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,191 @@ | ||
| # Deep-link login | ||
|
|
||
| How a `w3ds://` login request from a third-party site reaches the | ||
| Approve/Decline consent screen. | ||
|
|
||
| ## The problem | ||
|
|
||
| A platform (Pictique, Blabsy, ...) shows a login QR. The user scans it with the | ||
| system camera or taps it, and Android hands the wallet a URL: | ||
|
|
||
| ``` | ||
| w3ds://auth?session=<uuid>&platform=<name>&redirect=<origin> | ||
| ``` | ||
|
|
||
| Showing the consent screen for that URL requires two independent things to | ||
| finish, in an order nobody controls: | ||
|
|
||
| 1. **The URL arriving.** The root layout imports the deep-link plugin | ||
| asynchronously, then asks it for the launch URL. | ||
| 2. **The user authenticating.** On a cold start the splash prompts for | ||
| biometrics, which can succeed in ~200ms or take several seconds. | ||
|
|
||
| Neither reliably happens first. On a cold start with a fingerprint already on | ||
| the sensor, authentication wins. On a slower unlock, the URL wins. | ||
|
|
||
| ## The design: whoever finishes last routes | ||
|
|
||
| Both sides check the same two explicitly-recorded facts, so neither can act on | ||
| a half-finished picture: | ||
|
|
||
| - **The URL arrives while unauthenticated** — store it, route nothing. The | ||
| screen that completes authentication picks it up. | ||
| - **The URL arrives while already authenticated** — route to the consent screen | ||
| immediately. | ||
| - **Authentication completes** — if a payload is stored, go to `/scan-qr`; | ||
| otherwise `/main`. | ||
|
|
||
| ```mermaid | ||
| flowchart TD | ||
| A["Android intent w3ds://"] --> B["root +layout<br/>onOpenUrl / getCurrent"] | ||
| B --> C["parse payload"] | ||
| C --> D["storeDeepLink(payload)"] | ||
| D --> E{"globalState.sessionController.isAuthenticated?"} | ||
| E -- "no" --> F["route nothing:<br/>the auth path will collect it"] | ||
| E -- "yes" --> G["goto /scan-qr"] | ||
|
|
||
| S["splash +page"] --> S1["intro, poll globalState"] | ||
| S1 --> S2["authenticate() biometric"] | ||
| S2 -- "ok" --> P["continueAfterSuccessfulAuth"] | ||
| S2 -- "cancel / unavailable" --> L["/login PIN pad"] | ||
| L -- "pin ok" --> P | ||
|
|
||
| P --> P1["sessionController.markAuthenticated()<br/>BEFORE any await"] | ||
| P1 --> P2{"hasDeepLink()?"} | ||
| P2 -- "yes" --> G | ||
| P2 -- "no" --> M["goto /main"] | ||
|
|
||
| G --> R["scanLogic: peekDeepLink()<br/>open consent drawer"] | ||
| R --> R1["Approve -> POST + openUrl"] | ||
| R --> R2["Decline -> /main"] | ||
| ``` | ||
|
|
||
| ## One payload slot | ||
|
|
||
| Storing a deep link never implies permission to act on it. That is | ||
| `globalState.sessionController.isAuthenticated`, which every consumer checks anyway. | ||
|
|
||
| An earlier version had two slots, `pendingDeepLink` and `deepLinkData`, and | ||
| "promoted" between them once the user authenticated. The copy carried no | ||
| information: the payload was byte-identical on both sides, and the only | ||
| difference was a label meaning "actionable now". The duplication leaked | ||
| outward — `/scan-qr` read one key and fell back to the other, then had to clear | ||
| both, and any reader forgetting a key was a silent bug. | ||
|
|
||
| ## Why authentication is recorded explicitly | ||
|
|
||
| The original implementation asked | ||
| `isAuthenticatedRoute(window.location.pathname)` at the instant of delivery, as | ||
| a proxy for "has the user authenticated?". | ||
|
|
||
| That is unsound on a cold start. The path is `/` (the splash) regardless of how | ||
| the race went, so a user who had **already** authenticated was still classified | ||
| as logged out. The payload was stored for a screen that had finished running, | ||
| nothing collected it, and the user landed on `/main` with the consent screen | ||
| never shown. That was the bug this design replaces. | ||
|
|
||
| Authentication state is therefore written by the code that performs the | ||
| authentication, and never derived from the URL or the route. | ||
|
|
||
| `sessionController.markAuthenticated()` must be called **before any await** that precedes the | ||
| caller's navigation. A deep link delivered while post-login chores are in | ||
| flight has to see the user as authenticated, or it will store a payload nobody | ||
| is left to collect. | ||
|
|
||
| ## Storage choice | ||
|
|
||
| `sessionStorage`, deliberately — not a Svelte store, not `localStorage`, and | ||
| not an in-memory field on the controller. | ||
|
|
||
| - **A Svelte store is in-memory.** This state has to survive the full-page | ||
| navigations the wallet performs between the splash, `/login` and `/scan-qr`. | ||
| An in-memory store is empty on the other side. | ||
| - **`localStorage` would survive the app being killed**, which is exactly wrong | ||
| for the authenticated flag. A deep link arriving after a cold start must | ||
| trigger a real authentication rather than inheriting one from a previous run. | ||
| Being forgotten on relaunch is the property that makes it safe. | ||
| - **An in-memory field would not survive the WEBVIEW being rebuilt**, which is | ||
| a different event from the app being killed. Android may reload the webview | ||
| while the app is backgrounded by `openUrl`, and the approve path does a | ||
| document navigation to the platform's redirect. Neither is a new run of the | ||
| app, and the flow has no way to re-prompt mid-handoff, so the user would be | ||
| stranded. `SessionController` therefore takes no store and reads | ||
| sessionStorage directly. | ||
|
|
||
| Every accessor degrades to "nothing stored" when storage is unavailable | ||
| (private mode, storage disabled) rather than throwing, because these run inside | ||
| deep-link callbacks where a throw is invisible to the user and strands the flow. | ||
|
|
||
| ## Logout | ||
|
|
||
| `GlobalState.reset()` clears the sign-in flag, via `SessionController.clear()`; | ||
| `performLogout()` clears the pending payload alongside it. This is required, not | ||
| defensive: logout does `goto("/")`, an SPA navigation that leaves | ||
| `sessionStorage` intact. Without it the session would keep claiming the user is | ||
| authenticated, and the next deep link would route straight to the consent screen | ||
| on the strength of a login that had already ended. | ||
|
|
||
| ## The splash is the only biometric prompt | ||
|
|
||
| `/login` is the PIN fallback. The splash prompts for biometrics and routes | ||
| onward itself; it does not divert a deep-link launch to `/login`, because doing | ||
| so would downgrade a returning user to the PIN pad for the flow most likely to | ||
| be used in a hurry. | ||
|
|
||
| `/login` contains no call to `authenticate()` at all, and the splash is the only | ||
| file in the app that imports it from `@tauri-apps/plugin-biometric`. Everything | ||
| else imports `checkStatus` to read availability. The guarantee is therefore | ||
| structural rather than coordinated: there is no flag to get wrong. | ||
|
|
||
| An earlier design had both screens prompt, suppressed by a | ||
| `biometricAttemptedOnSplash` handshake. It was racy — the splash wrote the flag | ||
| only after two awaits resolved `biometricAvailable`, while `/login` read it | ||
| behind a globalState poll of up to five seconds, so anything routing to `/login` | ||
| inside that window got a second prompt. The flag is gone. | ||
|
|
||
| One consequence worth knowing: the splash's async `onMount` carries liveness | ||
| checks (`destroyed`). Unmounting a Svelte component does not cancel a | ||
| continuation parked on an `await`, so it could otherwise wake after the consent | ||
| drawer opened and navigate away from it. | ||
|
|
||
| The trade-off is deliberate: cancelling the biometric prompt leaves the user on | ||
| the PIN pad with no way to retry biometrics without relaunching. `/login` is | ||
| defined as the fallback, so that is the intended behaviour. | ||
|
|
||
| ## What authentication actually means here | ||
|
|
||
| Worth stating plainly, because the names are misleading. | ||
|
|
||
| | Fact | Question it answers | Lifetime | | ||
| |---|---|---| | ||
| | `vaultController.vault` | Is an identity **enrolled** on this device? | Disk, survives reboot | | ||
| | `securityController.pinHash` | Is a PIN **configured**? | Disk | | ||
| | `walletAuthenticated` | Has the user authenticated **this session**? | sessionStorage | | ||
|
|
||
| `walletAuthenticated` is owned by `GlobalState.sessionController`, alongside the | ||
| other controllers. It is the session's authentication state, not a deep-link | ||
| concept; the deep-link flow is simply its only reader today. `lib/stores/deepLink.ts` | ||
| owns only the pending payload. | ||
|
|
||
| The `(app)` route guard checks the **vault**, i.e. enrolment, not | ||
| authentication. It stops a never-onboarded or logged-out user; it does not stop | ||
| an unauthenticated one, since a cold-start user has a vault sitting on disk. | ||
|
|
||
| What actually forces authentication on a cold start is that the splash owns the | ||
| only normal path into `(app)`, plus the fact that `walletAuthenticated` dies | ||
| with the webview. A deep link is a **second door** into the app, which is why it | ||
| needs an explicit flag to consult rather than relying on that implicit | ||
| guarantee. | ||
|
|
||
| ## Known gaps | ||
|
|
||
| - **Not covered by tests:** vitest here is node-only and mounts no Svelte | ||
| components, so the specs pin this module's semantics, not the call sites in | ||
| `.svelte` files. Device testing via `pnpm build:apk` is the only real proof. | ||
| - **PIN change and passphrase rotation** have not been traced. If either ends a | ||
| session without going through `globalState.reset()`, the authenticated flag | ||
| would survive when it should not. | ||
| - **The Ok confirmation card is not shown after a deep-link login.** Approving | ||
| runs `goto("/main")` before `openUrl`, which unmounts the page that owns the | ||
| drawer, and the subsequent Activity restart wipes `sessionStorage` anyway. | ||
| This is pre-existing behaviour, unrelated to the race, and still open. |
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
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
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
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
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
72 changes: 72 additions & 0 deletions
72
infrastructure/eid-wallet/src/lib/global/controllers/session.ts
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,72 @@ | ||
| /** | ||
| * Runtime state for the current run of the app, as opposed to the persisted | ||
| * configuration the other controllers own. | ||
| * | ||
| * Every other controller wraps the Tauri store, so everything they hold | ||
| * survives the app being killed: is a PIN set, is an identity enrolled, are | ||
| * biometrics enabled. Those are configuration questions. | ||
| * | ||
| * "Has the user authenticated?" is not one of them. It must be forgotten when | ||
| * the app is killed, or a deep link arriving on a cold start would inherit a | ||
| * login from a previous run and skip the prompt entirely. | ||
| * | ||
| * It must equally SURVIVE the webview being rebuilt, which is not the same | ||
| * event. Approving a deep-link login hands off to the browser via openUrl, and | ||
| * Android is free to reload the backgrounded webview; the approve path also | ||
| * does a document navigation to the platform's redirect. Neither is a new run | ||
| * of the app, and the current flow has no way to re-prompt in the middle of | ||
| * one, so an in-memory field would strand the user. | ||
| * | ||
| * sessionStorage is exactly that lifetime: dies with the tab/app, survives a | ||
| * reload. Hence a controller that, unlike its siblings, takes no Store and | ||
| * reads sessionStorage directly. | ||
| * | ||
| * See docs/architecture/deepLink.md. | ||
| */ | ||
| export class SessionController { | ||
| static readonly #AUTHENTICATED_KEY = "walletAuthenticated"; | ||
|
|
||
| #storage(): Storage | null { | ||
| try { | ||
| return typeof sessionStorage === "undefined" | ||
| ? null | ||
| : sessionStorage; | ||
| } catch { | ||
| // Private mode / storage disabled. Degrade to "not authenticated" | ||
| // rather than throwing inside a deep-link callback, where a throw | ||
| // is invisible to the user and strands the flow. | ||
| return null; | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Record that the user is through the authentication gate. | ||
| * | ||
| * Callers must do this BEFORE any await that precedes their navigation, so | ||
| * a deep link delivered mid-flight sees the user as authenticated and | ||
| * routes itself rather than storing a payload nobody is left to collect. | ||
| */ | ||
| markAuthenticated(): void { | ||
| this.#storage()?.setItem(SessionController.#AUTHENTICATED_KEY, "true"); | ||
| } | ||
|
|
||
| get isAuthenticated(): boolean { | ||
| return ( | ||
| this.#storage()?.getItem(SessionController.#AUTHENTICATED_KEY) === | ||
| "true" | ||
| ); | ||
| } | ||
|
|
||
| /** | ||
| * Called by GlobalState.reset() on logout. | ||
| * | ||
| * Required, not defensive: logout does an SPA navigation to "/", which | ||
| * leaves sessionStorage intact. Without this the session would keep | ||
| * claiming the user is authenticated, and the next deep link would route | ||
| * itself straight to the consent screen on the strength of a login that | ||
| * has already ended. | ||
| */ | ||
| async clear(): Promise<void> { | ||
| this.#storage()?.removeItem(SessionController.#AUTHENTICATED_KEY); | ||
| } | ||
| } | ||
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Catch errors from each storage operation.
#storage()catches only access tosessionStorage.getItem,setItem, andremoveItemcan still throw when storage is disabled or full.A
setItemfailure interrupts authentication. AgetItemfailure interrupts deep-link routing. AremoveItemfailure rejectsclear()and stops the remaining cleanup inGlobalState.reset().Wrap each complete storage operation in
try/catchand preserve the documented fallback behavior.Also applies to: 55-56, 70-70
🤖 Prompt for AI Agents