From a12fe3e810ab5d4041599dd4af2f70cec077b4ac Mon Sep 17 00:00:00 2001 From: Christophe Vila Date: Sun, 26 Jul 2026 21:23:24 +0200 Subject: [PATCH] fix(api): stop promoting the ephemeral Garmin client after setup completes handleSetupComplete reused the onboarding client object as-is in the permanent per-user cache, but its garmin.Config.TokenStorePath was fixed at construction to the ephemeral setup/{hash} directory and never corrected after that directory was renamed to the permanent {userID} path. The next respawn of that same client (e.g. any Profile-page save, which unconditionally calls UpdateCredentials) wrote a fresh token file back under setup/{hash}, forcing a real re-login/MFA on the next Garmin connect even though a valid session already existed under {userID}. Close the ephemeral client instead and let the next garminFor call build a fresh one against the correct, already-renamed directory. --- backend/internal/api/server.go | 19 +++++++++------ backend/internal/api/setup.go | 26 ++++++++++++++++---- backend/internal/api/setup_test.go | 39 +++++++++++++++++++++++------- 3 files changed, 63 insertions(+), 21 deletions(-) diff --git a/backend/internal/api/server.go b/backend/internal/api/server.go index cd87c26..24b73cd 100644 --- a/backend/internal/api/server.go +++ b/backend/internal/api/server.go @@ -74,10 +74,15 @@ func NewServer(db *store.DB, garminFactory func(garmin.Config) garmin.Client, ga // 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. +// rather than a user id. Closed (never promoted into Server.userGarmin) once +// /api/setup/complete actually creates the account -- its Client's +// garmin.Config.TokenStorePath is permanently pinned to the ephemeral +// setup/{hash} directory, so reusing the object after that directory is +// renamed to the permanent {userID} one would respawn against a stale path +// the next time anything closes and restarts its subprocess; a later +// garminFor(ctx, userID) call builds a fresh client with the correct path +// instead. Otherwise 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 @@ -162,9 +167,9 @@ func (s *Server) recordSetupAuthResult(sub string, res garmin.AuthResult) { // removeSetupSession closes and drops sub's ephemeral Garmin session, if // any, and best-effort removes its token-store directory. Used when -// abandoning onboarding (logout) -- NOT used during promotion in -// handleSetupComplete, which transfers ownership of the client (and -// renames the directory) instead of discarding them. +// abandoning onboarding (logout). handleSetupComplete closes the client the +// same way but keeps (renames) the directory instead of removing it, since +// that's the real, now-permanent session. func (s *Server) removeSetupSession(sub string) { s.mu.Lock() sess, ok := s.setupGarmin[sub] diff --git a/backend/internal/api/setup.go b/backend/internal/api/setup.go index cee4d67..b8d9e35 100644 --- a/backend/internal/api/setup.go +++ b/backend/internal/api/setup.go @@ -88,10 +88,13 @@ func (s *Server) handleSetupGarminMFA(w http.ResponseWriter, r *http.Request) { // handleSetupComplete is the single atomic commit point: only reachable // once the ephemeral session for this subject last reported // garmin.AuthSuccess. Provisions the account, persists the Garmin -// credentials, marks it connected, and promotes the already-authenticated -// ephemeral client into the permanent per-user cache instead of discarding -// it (no redundant re-authentication, no repeat MFA prompt, right after -// signup). +// credentials, marks it connected, closes the ephemeral client, and +// renames its token-store directory into the permanent per-user path -- +// the next real garminFor(ctx, userID) builds a fresh client from scratch +// against that now-permanent directory, whose subprocess's lazy +// startup-login resumes the just-renamed, still-valid session without +// needing to re-authenticate (a cheap local token-store resume, not a +// fresh Garmin login). func (s *Server) handleSetupComplete(w http.ResponseWriter, r *http.Request) { claims, ok := auth.ClaimsFromContext(r.Context()) if !ok { @@ -142,9 +145,22 @@ func (s *Server) handleSetupComplete(w http.ResponseWriter, r *http.Request) { return } + // The ephemeral client's own garmin.Config.TokenStorePath was set once, + // at construction time in replaceSetupSession, to the ephemeral + // setup/{hash} directory being renamed below -- there's no setter to + // correct it in place, so promoting this object into s.userGarmin would + // leave a client whose subprocess respawns (e.g. on the very next + // UpdateCredentials call from a Profile save) using that now-stale + // path, recreating a setup/{hash} directory next to the real one. + // Closing it here and leaving s.userGarmin empty for this user makes + // the next garminFor(ctx, userID) call build a fresh client against the + // correct, just-renamed {userID} directory instead -- its subprocess's + // lazy startup-login resumes that session without a real Garmin + // re-authentication. + sess.Client.Close() + s.mu.Lock() delete(s.setupGarmin, claims.Sub) - s.userGarmin[userID] = sess.Client s.userAuthStatus[userID] = sess.Status s.userAuthMessage[userID] = sess.Message s.mu.Unlock() diff --git a/backend/internal/api/setup_test.go b/backend/internal/api/setup_test.go index 5c22105..a358988 100644 --- a/backend/internal/api/setup_test.go +++ b/backend/internal/api/setup_test.go @@ -92,8 +92,20 @@ func TestSetupGarminMFA_RejectsWithoutPriorLoginAttempt(t *testing.T) { } } -func TestSetupComplete_CreatesAccountWithGarminCredentialsAndPromotesClient(t *testing.T) { - s, db, m := newUnprovisionedServer(t) +func TestSetupComplete_CreatesAccountWithGarminCredentialsAndClosesEphemeralClient(t *testing.T) { + db, err := store.Open(filepath.Join(t.TempDir(), "geniusrun_test.db")) + if err != nil { + t.Fatalf("store.Open: %v", err) + } + t.Cleanup(func() { db.Close() }) + m := &mock.Client{} + tokenStoreRoot := t.TempDir() + var factoryConfigs []garmin.Config + garminFactory := func(cfg garmin.Config) garmin.Client { + factoryConfigs = append(factoryConfigs, cfg) + return m + } + s := NewServer(db, garminFactory, garmin.Config{TokenStorePath: tokenStoreRoot}, appsync.Config{}, &authmock.Verifier{}, testSessionConfig) router := s.Router() rec := doJSON(t, router, http.MethodPost, "/api/setup/garmin/login", map[string]any{ @@ -124,18 +136,27 @@ func TestSetupComplete_CreatesAccountWithGarminCredentialsAndPromotesClient(t *t } if m.AuthenticateCalls != 1 { - t.Errorf("AuthenticateCalls = %d, want 1 (no redundant re-authentication after promotion)", m.AuthenticateCalls) + t.Errorf("AuthenticateCalls = %d, want 1 (only the original login, no redundant re-authentication)", m.AuthenticateCalls) } - if m.ClosedCalled { - t.Error("expected the promoted client to survive (not be Close()d)") + if !m.ClosedCalled { + t.Error("expected the ephemeral client to be Close()d at setup completion, not promoted as-is -- its cfg.TokenStorePath still points at the ephemeral setup/{hash} dir, which would go stale the moment anything (e.g. a Profile save) later respawns it") } - client, err := s.garminFor(newCtx(), u.ID) - if err != nil { + // A later real use must build a genuinely fresh client, configured + // against the permanent {userID} token store directory -- never the + // stale ephemeral setup/{hash} one the closed client was carrying. + if _, err := s.garminFor(newCtx(), u.ID); err != nil { t.Fatalf("garminFor: %v", err) } - if client != m { - t.Error("expected garminFor to return the promoted (already-authenticated) client") + wantTokenStorePath := filepath.Join(tokenStoreRoot, itoa(u.ID)) + var gotTokenStorePath string + for _, cfg := range factoryConfigs { + if cfg.GarminEmail == "runner@example.com" { + gotTokenStorePath = cfg.TokenStorePath + } + } + if gotTokenStorePath != wantTokenStorePath { + t.Errorf("garminFor built client with TokenStorePath = %q, want %q", gotTokenStorePath, wantTokenStorePath) } }