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.
This commit is contained in:
@@ -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
|
// setupSession is a temporary, not-yet-persisted Garmin authentication
|
||||||
// attempt made during onboarding, before any users/profile row exists --
|
// attempt made during onboarding, before any users/profile row exists --
|
||||||
// keyed by OIDC subject (the only stable identifier available pre-account)
|
// keyed by OIDC subject (the only stable identifier available pre-account)
|
||||||
// rather than a user id. Promoted into Server.userGarmin once
|
// rather than a user id. Closed (never promoted into Server.userGarmin) once
|
||||||
// /api/setup/complete actually creates the account; evicted lazily (the
|
// /api/setup/complete actually creates the account -- its Client's
|
||||||
// next setup-endpoint touch for that subject checks staleness first) once
|
// garmin.Config.TokenStorePath is permanently pinned to the ephemeral
|
||||||
// idle past setupSessionIdleTimeout.
|
// 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 {
|
type setupSession struct {
|
||||||
Client garmin.Client
|
Client garmin.Client
|
||||||
Email, Password string
|
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
|
// removeSetupSession closes and drops sub's ephemeral Garmin session, if
|
||||||
// any, and best-effort removes its token-store directory. Used when
|
// any, and best-effort removes its token-store directory. Used when
|
||||||
// abandoning onboarding (logout) -- NOT used during promotion in
|
// abandoning onboarding (logout). handleSetupComplete closes the client the
|
||||||
// handleSetupComplete, which transfers ownership of the client (and
|
// same way but keeps (renames) the directory instead of removing it, since
|
||||||
// renames the directory) instead of discarding them.
|
// that's the real, now-permanent session.
|
||||||
func (s *Server) removeSetupSession(sub string) {
|
func (s *Server) removeSetupSession(sub string) {
|
||||||
s.mu.Lock()
|
s.mu.Lock()
|
||||||
sess, ok := s.setupGarmin[sub]
|
sess, ok := s.setupGarmin[sub]
|
||||||
|
|||||||
@@ -88,10 +88,13 @@ func (s *Server) handleSetupGarminMFA(w http.ResponseWriter, r *http.Request) {
|
|||||||
// handleSetupComplete is the single atomic commit point: only reachable
|
// handleSetupComplete is the single atomic commit point: only reachable
|
||||||
// once the ephemeral session for this subject last reported
|
// once the ephemeral session for this subject last reported
|
||||||
// garmin.AuthSuccess. Provisions the account, persists the Garmin
|
// garmin.AuthSuccess. Provisions the account, persists the Garmin
|
||||||
// credentials, marks it connected, and promotes the already-authenticated
|
// credentials, marks it connected, closes the ephemeral client, and
|
||||||
// ephemeral client into the permanent per-user cache instead of discarding
|
// renames its token-store directory into the permanent per-user path --
|
||||||
// it (no redundant re-authentication, no repeat MFA prompt, right after
|
// the next real garminFor(ctx, userID) builds a fresh client from scratch
|
||||||
// signup).
|
// 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) {
|
func (s *Server) handleSetupComplete(w http.ResponseWriter, r *http.Request) {
|
||||||
claims, ok := auth.ClaimsFromContext(r.Context())
|
claims, ok := auth.ClaimsFromContext(r.Context())
|
||||||
if !ok {
|
if !ok {
|
||||||
@@ -142,9 +145,22 @@ func (s *Server) handleSetupComplete(w http.ResponseWriter, r *http.Request) {
|
|||||||
return
|
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()
|
s.mu.Lock()
|
||||||
delete(s.setupGarmin, claims.Sub)
|
delete(s.setupGarmin, claims.Sub)
|
||||||
s.userGarmin[userID] = sess.Client
|
|
||||||
s.userAuthStatus[userID] = sess.Status
|
s.userAuthStatus[userID] = sess.Status
|
||||||
s.userAuthMessage[userID] = sess.Message
|
s.userAuthMessage[userID] = sess.Message
|
||||||
s.mu.Unlock()
|
s.mu.Unlock()
|
||||||
|
|||||||
@@ -92,8 +92,20 @@ func TestSetupGarminMFA_RejectsWithoutPriorLoginAttempt(t *testing.T) {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
func TestSetupComplete_CreatesAccountWithGarminCredentialsAndPromotesClient(t *testing.T) {
|
func TestSetupComplete_CreatesAccountWithGarminCredentialsAndClosesEphemeralClient(t *testing.T) {
|
||||||
s, db, m := newUnprovisionedServer(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()
|
router := s.Router()
|
||||||
|
|
||||||
rec := doJSON(t, router, http.MethodPost, "/api/setup/garmin/login", map[string]any{
|
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 {
|
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 {
|
if !m.ClosedCalled {
|
||||||
t.Error("expected the promoted client to survive (not be Close()d)")
|
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)
|
// A later real use must build a genuinely fresh client, configured
|
||||||
if err != nil {
|
// 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)
|
t.Fatalf("garminFor: %v", err)
|
||||||
}
|
}
|
||||||
if client != m {
|
wantTokenStorePath := filepath.Join(tokenStoreRoot, itoa(u.ID))
|
||||||
t.Error("expected garminFor to return the promoted (already-authenticated) client")
|
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)
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user