2026-07-26 13:17:59 +02:00
|
|
|
# Onboarding wizard with deferred DB commit
|
|
|
|
|
|
|
|
|
|
Status: approved, not yet implemented.
|
|
|
|
|
|
|
|
|
|
## Problem
|
|
|
|
|
|
|
|
|
|
Feedback on the just-shipped mandatory Garmin-connect onboarding
|
|
|
|
|
(`docs/superpowers/specs/2026-07-26-improve-first-connection-design.md`):
|
|
|
|
|
|
|
|
|
|
1. UX: a wizard-style Previous/Next pair reads better than the current
|
|
|
|
|
Continue/Log out pairing across `CreateProfile` → `ConnectGarmin`.
|
|
|
|
|
2. If the user quits mid-flow without logging out, the account is left in a
|
|
|
|
|
provisioned-but-not-Garmin-connected state -- confirmed this should
|
|
|
|
|
instead mean **nothing is written to the database at all** until Garmin
|
|
|
|
|
authentication actually succeeds, at which point display name + Garmin
|
|
|
|
|
credentials + the connected flag are all committed together, atomically.
|
|
|
|
|
|
|
|
|
|
(A third piece of feedback, structured JSON logging, is unrelated and gets
|
|
|
|
|
its own design afterward.)
|
|
|
|
|
|
|
|
|
|
## Design
|
|
|
|
|
|
|
|
|
|
### Consequence: the three-way LoginGate fork collapses
|
|
|
|
|
|
|
|
|
|
Since account provisioning and Garmin-connection now happen together in one
|
|
|
|
|
atomic step, `has_profile=true && garmin_connected=false` becomes
|
|
|
|
|
structurally unreachable -- confirmed to collapse `LoginGate` back to a
|
|
|
|
|
two-way fork (`has_profile ? App : OnboardingWizard`). `profile.garmin_connected_at`
|
|
|
|
|
stays as an informational timestamp (still written by the new atomic
|
|
|
|
|
commit); it just no longer gates anything on its own.
|
|
|
|
|
|
|
|
|
|
### The core problem: Garmin auth needs an account today
|
|
|
|
|
|
2026-08-04 16:04:18 +02:00
|
|
|
`clientFor` builds a per-user `garmin.Client` keyed by a DB user id and
|
2026-07-26 13:17:59 +02:00
|
|
|
reads Garmin credentials from that user's `profile` row. To authenticate
|
|
|
|
|
with Garmin *before* an account exists, onboarding needs a **temporary,
|
|
|
|
|
not-yet-persisted** Garmin session -- keyed by the OIDC subject (already
|
|
|
|
|
known from the session cookie, before any `users` row exists) instead of a
|
|
|
|
|
user id. A long-held-open SQL transaction across the wizard's multiple
|
|
|
|
|
HTTP round trips is not an option: SQLite's single connection
|
|
|
|
|
(`SetMaxOpenConns(1)`, see `db.go`) means one held-open transaction would
|
|
|
|
|
block every other request in the app for as long as the user takes to
|
|
|
|
|
finish onboarding.
|
|
|
|
|
|
|
|
|
|
### Backend: ephemeral pre-account Garmin sessions
|
|
|
|
|
|
|
|
|
|
New type and `Server` field (`server.go`, alongside the existing
|
2026-08-04 16:04:18 +02:00
|
|
|
`userClient`/`userSync`/etc. per-user maps):
|
2026-07-26 13:17:59 +02:00
|
|
|
|
|
|
|
|
```go
|
|
|
|
|
// setupSession is a temporary, not-yet-persisted Garmin authentication
|
|
|
|
|
// attempt made during onboarding, before any users/profile row exists --
|
|
|
|
|
// keyed by OIDC subject (the only stable identifier available pre-account)
|
|
|
|
|
// rather than a user id. Promoted into Server.userGarmin once
|
|
|
|
|
// /api/setup/complete actually creates the account; evicted lazily (the
|
|
|
|
|
// next setup-endpoint touch for that subject checks staleness first) once
|
|
|
|
|
// idle past setupSessionIdleTimeout.
|
|
|
|
|
type setupSession struct {
|
|
|
|
|
Client garmin.Client
|
|
|
|
|
Email, Password string
|
|
|
|
|
Status garmin.AuthStatus
|
|
|
|
|
Message string
|
|
|
|
|
LastUsed time.Time
|
|
|
|
|
}
|
|
|
|
|
|
|
|
|
|
const setupSessionIdleTimeout = 15 * time.Minute
|
|
|
|
|
```
|
|
|
|
|
|
|
|
|
|
`Server` gains `setupGarmin map[string]*setupSession` (keyed by OIDC
|
|
|
|
|
subject string), initialized alongside the other maps in `NewServer`.
|
|
|
|
|
|
|
|
|
|
Helper methods on `Server` (`server.go`):
|
|
|
|
|
|
|
|
|
|
- `setupSessionFor(sub string) (*setupSession, bool)` -- returns the
|
|
|
|
|
session if present and not stale; a stale one is closed and evicted
|
|
|
|
|
first (lazy cleanup, no background sweep goroutine -- an abandoned
|
|
|
|
|
session otherwise leaks one idle subprocess until the backend restarts,
|
|
|
|
|
an accepted trade-off for a single-operator app).
|
|
|
|
|
- `replaceSetupSession(sub, email, password string) *setupSession` --
|
|
|
|
|
closes and replaces any existing session for `sub` (same "close old,
|
|
|
|
|
spawn new" semantics as `garmin.Client.UpdateCredentials`), builds a
|
|
|
|
|
fresh ephemeral client whose `TokenStorePath` is namespaced under
|
|
|
|
|
`{root}/setup/{sub}` (distinct from the permanent `{root}/{userID}`
|
2026-08-04 16:04:18 +02:00
|
|
|
namespacing `clientFor` uses, so two people onboarding concurrently never
|
2026-07-26 13:17:59 +02:00
|
|
|
collide, and so the ephemeral session's persisted Garmin tokens have
|
|
|
|
|
*some* home even though no user id exists yet).
|
|
|
|
|
- `recordSetupAuthResult(sub string, res garmin.AuthResult)` -- updates a
|
|
|
|
|
session's `Status`/`Message`/`LastUsed` after a login or MFA attempt.
|
|
|
|
|
- `removeSetupSession(sub string)` -- closes and drops `sub`'s session
|
|
|
|
|
(best-effort removes its `{root}/setup/{sub}` token-store directory too).
|
|
|
|
|
Called both when `/api/setup/complete` promotes a session into the
|
|
|
|
|
permanent cache, and from `handleSessionLogout` so abandoning mid-wizard
|
|
|
|
|
via logout doesn't linger until the idle timeout.
|
|
|
|
|
|
|
|
|
|
### Backend: three new endpoints replace `/api/setup`
|
|
|
|
|
|
|
|
|
|
Registered where `r.Post("/setup", s.handleSetup)` used to be (same
|
|
|
|
|
middleware group: `RequireSession` + `resolveUser`, but *not*
|
|
|
|
|
`requireProvisionedUser` -- these must work before a profile exists):
|
|
|
|
|
|
|
|
|
|
```go
|
|
|
|
|
r.Route("/setup", func(r chi.Router) {
|
|
|
|
|
r.Post("/complete", s.handleSetupComplete)
|
|
|
|
|
r.Route("/garmin", func(r chi.Router) {
|
|
|
|
|
r.Post("/login", s.handleSetupGarminLogin)
|
|
|
|
|
r.Post("/mfa", s.handleSetupGarminMFA)
|
|
|
|
|
})
|
|
|
|
|
})
|
|
|
|
|
```
|
|
|
|
|
|
|
|
|
|
- **`POST /api/setup/garmin/login`** `{garmin_email, garmin_password}`:
|
|
|
|
|
401 if unauthenticated; 409 if a profile already exists for this subject
|
|
|
|
|
(mirrors old `handleSetup`'s existing check); 400 on missing fields.
|
|
|
|
|
Otherwise `replaceSetupSession` + `Authenticate()` + `recordSetupAuthResult`,
|
|
|
|
|
responding with the same `{status, message}` shape `/api/auth/login`
|
|
|
|
|
already uses.
|
|
|
|
|
- **`POST /api/setup/garmin/mfa`** `{code}`: 409 if no session exists for
|
|
|
|
|
this subject (never started, or expired) -- frontend resets to the
|
|
|
|
|
Garmin step and asks the user to reconnect. Otherwise
|
|
|
|
|
`CompleteMFA()` + `recordSetupAuthResult` on the *same* session (never
|
|
|
|
|
replaces it -- MFA continues an in-progress attempt on the same
|
|
|
|
|
subprocess).
|
|
|
|
|
- **`POST /api/setup/complete`** `{display_name}`: 409 if already
|
|
|
|
|
provisioned; 409 if no session exists for this subject or its `Status !=
|
|
|
|
|
garmin.AuthSuccess` ("Garmin isn't connected yet"). Otherwise, in order:
|
|
|
|
|
1. `ProvisionUser(sub, displayName)` (unchanged).
|
|
|
|
|
2. `GetProfile` + set `GarminEmail`/`GarminPassword` from the session +
|
|
|
|
|
`UpdateProfile` (unchanged method, just called here instead of from
|
|
|
|
|
the Profile page).
|
|
|
|
|
3. `MarkGarminConnected(userID)`.
|
|
|
|
|
4. Promote: move the session's already-authenticated `garmin.Client`
|
|
|
|
|
into `s.userGarmin[userID]` and its recorded status into
|
|
|
|
|
`s.userAuthStatus[userID]`/`userAuthMessage[userID]` (avoids a
|
|
|
|
|
redundant re-authentication -- and possibly a repeat MFA prompt --
|
|
|
|
|
immediately after signup), then `delete(s.setupGarmin, sub)` without
|
|
|
|
|
closing the client (ownership transferred, not discarded).
|
|
|
|
|
5. Best-effort `os.Rename` the ephemeral token-store directory
|
|
|
|
|
(`{root}/setup/{sub}`) to the permanent one (`{root}/{userID}`), so a
|
|
|
|
|
later respawn of this user's subprocess (e.g. after a server restart)
|
|
|
|
|
resumes the already-established Garmin session instead of requiring a
|
|
|
|
|
fresh login. Logged, not fatal, on error.
|
|
|
|
|
|
2026-08-04 16:04:18 +02:00
|
|
|
`internal/api/auth.go`'s existing `handleGarminAuthLogin`/`handleGarminAuthMFA`/`handleGarminAuthStatus`
|
2026-07-26 13:17:59 +02:00
|
|
|
(used by the Profile page's `GarminConnection.tsx` for reconnecting an
|
|
|
|
|
already-provisioned user) are untouched.
|
|
|
|
|
|
|
|
|
|
### Frontend: one wizard component instead of two screens
|
|
|
|
|
|
|
|
|
|
`CreateProfile.tsx` and `ConnectGarmin.tsx` are deleted, replaced by
|
|
|
|
|
`frontend/src/OnboardingWizard.tsx` (and `OnboardingWizard.css`, replacing
|
|
|
|
|
`CreateProfile.css`). Two top-level steps (`"name" | "garmin"`); the
|
|
|
|
|
in-progress-MFA and already-authenticated states are just conditions on
|
|
|
|
|
`auth?.status` *within* the `"garmin"` step, not a third step value --
|
|
|
|
|
simpler than threading a separate `"mfa"` step through, and matches how
|
|
|
|
|
`GarminConnection.tsx` already branches on `status` today:
|
|
|
|
|
|
|
|
|
|
- **`name` step**: display name field, held in local state only -- no API
|
|
|
|
|
call. "Next" just validates non-empty and advances `step` to `"garmin"`.
|
|
|
|
|
- **`garmin` step**, branching on `auth?.status`:
|
|
|
|
|
- `undefined` / `"failed"`: email/password fields. "Previous" returns to
|
|
|
|
|
the `name` step (pure local state, nothing to undo server-side).
|
|
|
|
|
"Next" calls `api.setupGarminLogin(email, password)`.
|
|
|
|
|
- `"mfa_required"`: MFA code field. "Previous" clears `auth` back to
|
|
|
|
|
`null`, which re-shows the credentials form (submitting it again just
|
|
|
|
|
replaces the ephemeral session, same as the backend's "close old,
|
|
|
|
|
spawn new" semantics). "Submit code" calls `api.setupGarminMFA(code)`.
|
|
|
|
|
- `"authenticated"`: a confirmation message and "Continue to geniusrun",
|
|
|
|
|
which calls `api.setupComplete(displayName)` and then `onCreated(...)`.
|
|
|
|
|
- "Log out" (`<form method="post" action=".../session/logout">`) stays
|
|
|
|
|
visible throughout, the same escape hatch as before.
|
|
|
|
|
|
|
|
|
|
`frontend/src/api/client.ts`: remove `setup`; add `setupGarminLogin`,
|
|
|
|
|
`setupGarminMFA`, `setupComplete` (bodies/shapes as described above).
|
|
|
|
|
|
|
|
|
|
`frontend/src/LoginGate.tsx`: collapse back to
|
|
|
|
|
`!session.has_profile ? <OnboardingWizard onCreated={...}> : <App>` --
|
|
|
|
|
`garmin_connected` is no longer read here at all (still present on
|
|
|
|
|
`SessionInfo`/`session/me`, just unused by the gate).
|
|
|
|
|
|
|
|
|
|
### Error handling
|
|
|
|
|
|
|
|
|
|
- Garmin login/MFA failure: same as today -- message shown, fields stay
|
|
|
|
|
editable, retry always available, nothing persisted regardless of
|
|
|
|
|
outcome.
|
|
|
|
|
- `/api/setup/complete` with no session, or session not yet `authenticated`:
|
|
|
|
|
`409`, frontend falls back to the `garmin` step (credentials form) so the
|
|
|
|
|
user can (re)connect.
|
|
|
|
|
- `/api/setup/complete`/`/api/setup/garmin/login` when already provisioned
|
|
|
|
|
(double-submit, duplicate tab): `409` "profile already exists"; frontend
|
|
|
|
|
treats this as success and refreshes session info rather than showing an
|
|
|
|
|
error.
|
|
|
|
|
- Idle sessions (15 minutes unused) are evicted lazily on next touch --
|
|
|
|
|
`/api/setup/garmin/mfa` or `/api/setup/complete` against an expired
|
|
|
|
|
session behaves identically to "never connected" (409), prompting a
|
|
|
|
|
fresh `/api/setup/garmin/login`.
|
|
|
|
|
- Logging out mid-wizard closes and drops that subject's ephemeral session
|
|
|
|
|
and its token-store directory.
|
|
|
|
|
|
|
|
|
|
### Testing
|
|
|
|
|
|
|
|
|
|
- `internal/api`: `POST /api/setup/garmin/login` (mock `AuthSuccess`)
|
|
|
|
|
creates no DB user yet (`GetUserBySub` still not found); the
|
|
|
|
|
`mfa_required` path via `/api/setup/garmin/mfa` on the same session;
|
|
|
|
|
`/api/setup/complete` after a successful session creates the user with
|
|
|
|
|
`GarminEmail`/`GarminPassword`/`GarminConnectedAt` all set, and promotes
|
|
|
|
|
the *same* `*mock.Client` instance into `s.userGarmin[userID]` (asserted
|
2026-08-04 16:04:18 +02:00
|
|
|
via `clientFor` returning that exact pointer, and `ClosedCalled` still
|
2026-07-26 13:17:59 +02:00
|
|
|
false -- proving no redundant re-authentication happened);
|
|
|
|
|
`/api/setup/complete` with no prior login attempt → 409;
|
|
|
|
|
already-provisioned subject hitting either endpoint → 409; two different
|
|
|
|
|
OIDC subjects' ephemeral sessions never interfere (isolation test,
|
|
|
|
|
matching this repo's adversarial-isolation convention). A token-store
|
|
|
|
|
rename test mirroring the existing `TestDeleteProfile_RemovesTokenStoreDirectory`
|
|
|
|
|
pattern (set `GarminBase.TokenStorePath`, confirm the directory ends up
|
|
|
|
|
under the new user id's path after `/api/setup/complete`, not under
|
|
|
|
|
`setup/{sub}`).
|
|
|
|
|
- No frontend test suite -- manual smoke test: fresh account → name step →
|
|
|
|
|
Previous/Next between name and Garmin steps preserves typed input →
|
|
|
|
|
wrong password shows an error and stays editable → MFA prompt → Previous
|
|
|
|
|
from MFA returns to credentials → correct credentials (or MFA resolved)
|
|
|
|
|
→ "Continue to geniusrun" → lands in the app → confirm `GetUserBySub`
|
|
|
|
|
now exists with everything set. Separately: start the wizard, enter
|
|
|
|
|
Garmin credentials, then just close the tab without finishing --
|
|
|
|
|
confirm no `users`/`profile` row was created at all.
|
|
|
|
|
|
|
|
|
|
## Out of scope
|
|
|
|
|
|
|
|
|
|
- A background sweep goroutine for abandoned ephemeral sessions -- accepted
|
|
|
|
|
as a known, low-stakes trade-off (see Error handling).
|
|
|
|
|
- Structured JSON logging (the third piece of feedback) -- separate design.
|
|
|
|
|
- Any change to the Profile page's own reconnect flow
|
|
|
|
|
(`GarminConnection.tsx`, `/api/auth/*`) -- untouched.
|