diff --git a/backend/internal/api/api_test.go b/backend/internal/api/api_test.go index 9edfb48..a6db7f3 100644 --- a/backend/internal/api/api_test.go +++ b/backend/internal/api/api_test.go @@ -765,6 +765,56 @@ func TestProfile_RejectsInvalidHRZones(t *testing.T) { } } +func TestSessionMe_ReportsGarminConnectedAfterSuccessfulAuth(t *testing.T) { + s, _, _ := newTestServer(t) + router := s.Router() + + rec := doJSON(t, router, http.MethodGet, "/api/session/me", nil) + var before sessionMeResponse + unmarshalBody(t, rec, &before) + if before.GarminConnected { + t.Fatal("expected a fresh account to report garmin_connected=false") + } + + rec = doJSON(t, router, http.MethodPost, "/api/auth/login", nil) // mock.Client defaults to AuthSuccess + if rec.Code != http.StatusOK { + t.Fatalf("auth/login status = %d, body = %s", rec.Code, rec.Body.String()) + } + + rec = doJSON(t, router, http.MethodGet, "/api/session/me", nil) + var after sessionMeResponse + unmarshalBody(t, rec, &after) + if !after.GarminConnected { + t.Fatal("expected garmin_connected=true after a successful auth") + } +} + +func TestSessionMe_GarminConnectedStaysFalseOnMFARequiredOrFailed(t *testing.T) { + s, _, userID := newTestServer(t) + client, err := s.garminFor(context.Background(), userID) + if err != nil { + t.Fatalf("garminFor: %v", err) + } + mockClient, ok := client.(*mock.Client) + if !ok { + t.Fatalf("expected *mock.Client, got %T", client) + } + mockClient.AuthResults = []garmin.AuthResult{{Status: garmin.AuthMFARequired, Message: "MFA required"}} + + router := s.Router() + rec := doJSON(t, router, http.MethodPost, "/api/auth/login", nil) + if rec.Code != http.StatusOK { + t.Fatalf("auth/login status = %d, body = %s", rec.Code, rec.Body.String()) + } + + rec = doJSON(t, router, http.MethodGet, "/api/session/me", nil) + var me sessionMeResponse + unmarshalBody(t, rec, &me) + if me.GarminConnected { + t.Fatal("expected garmin_connected=false after mfa_required") + } +} + func TestDeleteProfile_DeletesUserAndTerminatesGarminClient(t *testing.T) { s, db, userID := newTestServer(t) router := s.Router() diff --git a/backend/internal/api/auth.go b/backend/internal/api/auth.go index 295e2ff..dea11d7 100644 --- a/backend/internal/api/auth.go +++ b/backend/internal/api/auth.go @@ -1,7 +1,9 @@ package api import ( + "context" "encoding/json" + "log" "net/http" "geniusrun/backend/internal/garmin" @@ -25,11 +27,22 @@ func authStatusString(s garmin.AuthStatus) string { } } -func (s *Server) recordAuthResult(userID int64, res garmin.AuthResult) { +// recordAuthResult updates the in-memory auth status/message for userID, +// and -- on a successful authentication -- persists that this account has +// connected to Garmin at least once (store.MarkGarminConnected), which is +// what the login gate actually checks (the in-memory auth status resets on +// every backend restart; this doesn't). +func (s *Server) recordAuthResult(ctx context.Context, userID int64, res garmin.AuthResult) { s.mu.Lock() s.userAuthStatus[userID] = res.Status s.userAuthMessage[userID] = res.Message s.mu.Unlock() + + if res.Status == garmin.AuthSuccess { + if err := s.DB.MarkGarminConnected(ctx, userID); err != nil { + log.Printf("api: mark garmin connected for user %d: %v", userID, err) + } + } } func (s *Server) handleAuthLogin(w http.ResponseWriter, r *http.Request) { @@ -44,7 +57,7 @@ func (s *Server) handleAuthLogin(w http.ResponseWriter, r *http.Request) { writeError(w, http.StatusBadGateway, err.Error()) return } - s.recordAuthResult(userID, res) + s.recordAuthResult(r.Context(), userID, res) writeJSON(w, http.StatusOK, authResponse{Status: authStatusString(res.Status), Message: res.Message}) } @@ -72,7 +85,7 @@ func (s *Server) handleAuthMFA(w http.ResponseWriter, r *http.Request) { writeError(w, http.StatusBadGateway, err.Error()) return } - s.recordAuthResult(userID, res) + s.recordAuthResult(r.Context(), userID, res) writeJSON(w, http.StatusOK, authResponse{Status: authStatusString(res.Status), Message: res.Message}) } diff --git a/backend/internal/api/isolation_test.go b/backend/internal/api/isolation_test.go index f25e4e6..0572354 100644 --- a/backend/internal/api/isolation_test.go +++ b/backend/internal/api/isolation_test.go @@ -181,3 +181,25 @@ func TestIsolation_DeleteProfileOnlyDeletesOwnAccount(t *testing.T) { t.Fatalf("userA profile status after userB deleted themselves = %d, body = %s", rec.Code, rec.Body.String()) } } + +// TestIsolation_GarminConnectedNeverLeaksAcrossUsers confirms one user's +// successful Garmin auth never flips garmin_connected for another user. +func TestIsolation_GarminConnectedNeverLeaksAcrossUsers(t *testing.T) { + s, db, _ := newTestServer(t) // provisions "test-user" (userA) + if _, err := db.ProvisionUser(newCtx(), "user-b", "B"); err != nil { + t.Fatalf("ProvisionUser(b): %v", err) + } + + router := s.Router() + rec := doJSON(t, router, http.MethodPost, "/api/auth/login", nil) // as userA + if rec.Code != http.StatusOK { + t.Fatalf("auth/login status = %d, body = %s", rec.Code, rec.Body.String()) + } + + rec = doJSONAs(t, router, "user-b", http.MethodGet, "/api/session/me", nil) + var meB sessionMeResponse + unmarshalBody(t, rec, &meB) + if meB.GarminConnected { + t.Fatal("userA's successful Garmin auth leaked into userB's garmin_connected flag") + } +} diff --git a/backend/internal/api/session.go b/backend/internal/api/session.go index 3376e36..d35c666 100644 --- a/backend/internal/api/session.go +++ b/backend/internal/api/session.go @@ -32,10 +32,11 @@ type SessionConfig struct { } type sessionMeResponse struct { - Name string `json:"name"` - Email string `json:"email"` - HasProfile bool `json:"has_profile"` - DisplayName string `json:"display_name,omitempty"` + Name string `json:"name"` + Email string `json:"email"` + HasProfile bool `json:"has_profile"` + DisplayName string `json:"display_name,omitempty"` + GarminConnected bool `json:"garmin_connected"` } func (s *Server) handleSessionLogin(w http.ResponseWriter, r *http.Request) { @@ -113,6 +114,12 @@ func (s *Server) handleSessionMe(w http.ResponseWriter, r *http.Request) { if u, found := userFromContext(r.Context()); found { resp.HasProfile = true resp.DisplayName = u.DisplayName + profile, err := s.DB.GetProfile(r.Context(), u.ID) + if err != nil { + writeError(w, http.StatusInternalServerError, err.Error()) + return + } + resp.GarminConnected = profile.GarminConnectedAt != nil } writeJSON(w, http.StatusOK, resp) } diff --git a/backend/internal/store/isolation_test.go b/backend/internal/store/isolation_test.go index 2a32b39..cf6ac74 100644 --- a/backend/internal/store/isolation_test.go +++ b/backend/internal/store/isolation_test.go @@ -204,3 +204,31 @@ func TestIsolation_DeleteUserLeavesOtherUsersDataIntact(t *testing.T) { t.Fatalf("userB's activities affected by deleting userA: len=%d err=%v", len(activitiesB), err) } } + +// TestIsolation_MarkGarminConnectedNeverLeaksAcrossUsers confirms marking +// one user's Garmin connection never sets another user's flag. +func TestIsolation_MarkGarminConnectedNeverLeaksAcrossUsers(t *testing.T) { + db := openTestDB(t) + ctx := context.Background() + + userA, err := db.ProvisionUser(ctx, "sub-a", "A") + if err != nil { + t.Fatalf("ProvisionUser(a): %v", err) + } + userB, err := db.ProvisionUser(ctx, "sub-b", "B") + if err != nil { + t.Fatalf("ProvisionUser(b): %v", err) + } + + if err := db.MarkGarminConnected(ctx, userA); err != nil { + t.Fatalf("MarkGarminConnected(a): %v", err) + } + + profileB, err := db.GetProfile(ctx, userB) + if err != nil { + t.Fatalf("GetProfile(b): %v", err) + } + if profileB.GarminConnectedAt != nil { + t.Fatal("userA's MarkGarminConnected call leaked into userB's profile") + } +} diff --git a/backend/internal/store/profile.go b/backend/internal/store/profile.go index b7e8bcb..a4de766 100644 --- a/backend/internal/store/profile.go +++ b/backend/internal/store/profile.go @@ -10,9 +10,15 @@ import ( type Profile struct { // Name labels this profile so a future multi-profile setup can show // which one is active. Only one profile row exists today (id=1). - Name string - GarminEmail string - GarminPassword string + Name string + GarminEmail string + GarminPassword string + // GarminConnectedAt is nil until this user's first successful Garmin + // authentication (see store.MarkGarminConnected) -- the login gate uses + // it, not UpdateProfile, so it's deliberately excluded from + // UpdateProfile's SET clause below and can only ever move from nil to + // set, never reset by a normal profile save. + GarminConnectedAt *string RollingWindowDays int // BackfillHorizonDays bounds how far back "Sync now" reaches when // walking backward from today; it's read fresh on every sync (not fixed @@ -70,7 +76,7 @@ type Profile struct { } const profileColumns = ` - name, garmin_email, garmin_password, rolling_window_days, backfill_horizon_days, max_heart_rate, resting_heart_rate, + name, garmin_email, garmin_password, garmin_connected_at, rolling_window_days, backfill_horizon_days, max_heart_rate, resting_heart_rate, hr_zone1_min_pct, hr_zone1_max_pct, hr_zone2_min_pct, hr_zone2_max_pct, hr_zone3_min_pct, hr_zone3_max_pct, hr_zone4_min_pct, hr_zone4_max_pct, hr_zone5_min_pct, hr_zone5_max_pct, @@ -85,7 +91,7 @@ const profileColumns = ` func (db *DB) GetProfile(ctx context.Context, userID int64) (Profile, error) { var p Profile err := db.QueryRowContext(ctx, `SELECT `+profileColumns+` FROM profile WHERE user_id = ?`, userID).Scan( - &p.Name, &p.GarminEmail, &p.GarminPassword, &p.RollingWindowDays, &p.BackfillHorizonDays, &p.MaxHeartRate, &p.RestingHeartRate, + &p.Name, &p.GarminEmail, &p.GarminPassword, &p.GarminConnectedAt, &p.RollingWindowDays, &p.BackfillHorizonDays, &p.MaxHeartRate, &p.RestingHeartRate, &p.HRZone1MinPct, &p.HRZone1MaxPct, &p.HRZone2MinPct, &p.HRZone2MaxPct, &p.HRZone3MinPct, &p.HRZone3MaxPct, &p.HRZone4MinPct, &p.HRZone4MaxPct, &p.HRZone5MinPct, &p.HRZone5MaxPct, @@ -132,3 +138,14 @@ func (db *DB) UpdateProfile(ctx context.Context, userID int64, p Profile) error } return nil } + +// MarkGarminConnected records the first time userID successfully +// authenticates with Garmin. A no-op if already set, so it always reflects +// the first connection, not the most recent one. +func (db *DB) MarkGarminConnected(ctx context.Context, userID int64) error { + _, err := db.ExecContext(ctx, `UPDATE profile SET garmin_connected_at = datetime('now') WHERE user_id = ? AND garmin_connected_at IS NULL`, userID) + if err != nil { + return fmt.Errorf("mark garmin connected for user %d: %w", userID, err) + } + return nil +} diff --git a/backend/internal/store/profile_test.go b/backend/internal/store/profile_test.go index 34eb22e..67eb157 100644 --- a/backend/internal/store/profile_test.go +++ b/backend/internal/store/profile_test.go @@ -107,3 +107,44 @@ func TestProfile_DefaultsThenUpdate(t *testing.T) { t.Errorf("TargetBrightenPct after update = %v, want 50", got.TargetBrightenPct) } } + +func TestMarkGarminConnected_SetsTimestampOnceOnFirstCall(t *testing.T) { + db := openTestDB(t) + ctx := context.Background() + userID, err := db.ProvisionUser(ctx, "test-sub", "Test") + if err != nil { + t.Fatalf("ProvisionUser: %v", err) + } + + before, err := db.GetProfile(ctx, userID) + if err != nil { + t.Fatalf("GetProfile: %v", err) + } + if before.GarminConnectedAt != nil { + t.Fatalf("expected a fresh profile to have nil GarminConnectedAt, got %v", *before.GarminConnectedAt) + } + + if err := db.MarkGarminConnected(ctx, userID); err != nil { + t.Fatalf("MarkGarminConnected: %v", err) + } + afterFirst, err := db.GetProfile(ctx, userID) + if err != nil { + t.Fatalf("GetProfile after first mark: %v", err) + } + if afterFirst.GarminConnectedAt == nil { + t.Fatal("expected GarminConnectedAt to be set after MarkGarminConnected") + } + firstValue := *afterFirst.GarminConnectedAt + + // A second call must not change the recorded first-connection time. + if err := db.MarkGarminConnected(ctx, userID); err != nil { + t.Fatalf("MarkGarminConnected (second call): %v", err) + } + afterSecond, err := db.GetProfile(ctx, userID) + if err != nil { + t.Fatalf("GetProfile after second mark: %v", err) + } + if afterSecond.GarminConnectedAt == nil || *afterSecond.GarminConnectedAt != firstValue { + t.Fatalf("GarminConnectedAt changed on second call: first=%q second=%v", firstValue, afterSecond.GarminConnectedAt) + } +} diff --git a/backend/internal/store/schema.sql b/backend/internal/store/schema.sql index db2ea9f..a5d7442 100644 --- a/backend/internal/store/schema.sql +++ b/backend/internal/store/schema.sql @@ -30,6 +30,12 @@ CREATE TABLE profile ( name TEXT NOT NULL DEFAULT 'Default', garmin_email TEXT NOT NULL DEFAULT '', garmin_password TEXT NOT NULL DEFAULT '', + -- Set once, the first time this user successfully authenticates with + -- Garmin (handleAuthLogin/handleAuthMFA). Null means never connected -- + -- the login gate uses this (not the in-memory auth status, which + -- resets on restart) to decide whether a returning user must + -- reconnect before entering the app. + garmin_connected_at TEXT, rolling_window_days INTEGER NOT NULL DEFAULT 90, -- BackfillHorizonDays bounds how far back "Sync now" reaches when -- walking backward from today; read fresh on every sync (not cached at diff --git a/docs/DATABASE.md b/docs/DATABASE.md index b22a462..1afc19f 100644 --- a/docs/DATABASE.md +++ b/docs/DATABASE.md @@ -35,6 +35,12 @@ CREATE TABLE profile ( name TEXT NOT NULL DEFAULT 'Default', garmin_email TEXT NOT NULL DEFAULT '', garmin_password TEXT NOT NULL DEFAULT '', + -- Set once, the first time this user successfully authenticates with + -- Garmin (handleAuthLogin/handleAuthMFA). Null means never connected -- + -- the login gate uses this (not the in-memory auth status, which + -- resets on restart) to decide whether a returning user must + -- reconnect before entering the app. + garmin_connected_at TEXT, rolling_window_days INTEGER NOT NULL DEFAULT 90, -- BackfillHorizonDays bounds how far back "Sync now" reaches when -- walking backward from today; read fresh on every sync (not cached at diff --git a/docs/IDEAS.md b/docs/IDEAS.md index 69a4fed..5bd6ce6 100644 --- a/docs/IDEAS.md +++ b/docs/IDEAS.md @@ -8,7 +8,6 @@ for that history) and remove it from here once a spec exists. ## Backlog -- improve first connection page: in CreateProfile.tsx, in addition to the display name, we should directly ask garmin login/password and manage MFA from there, so the synchronization experience in profile page will be easier later on - dedicated workouts section: add workouts table in database and create a dedicated page in UI (use an "objective" icon) - improve activities/workouts download: make it modal, add progress bar, error management - better UX when backend is not available (instead of "TypeError failed to fetch") diff --git a/frontend/src/ConnectGarmin.tsx b/frontend/src/ConnectGarmin.tsx new file mode 100644 index 0000000..555ca10 --- /dev/null +++ b/frontend/src/ConnectGarmin.tsx @@ -0,0 +1,117 @@ +import { useState } from "react"; +import { api, BASE_URL } from "./api/client"; +import "./CreateProfile.css"; +import type { AuthResponse } from "./types/api"; + +// Shown after CreateProfile (or on any later login, until it succeeds -- +// see LoginGate) since connecting Garmin is mandatory: the account exists +// but this is the last gate before the main app. Deliberately not a reuse +// of GarminConnection.tsx (the Profile page's version), which also renders +// Sync now/Disconnect/Reset all -- none of which make sense here. +export function ConnectGarmin({ onConnected }: { onConnected: () => void }) { + const [email, setEmail] = useState(""); + const [password, setPassword] = useState(""); + const [code, setCode] = useState(""); + const [auth, setAuth] = useState(null); + const [busy, setBusy] = useState(false); + const [error, setError] = useState(null); + + async function connect(e: React.FormEvent) { + e.preventDefault(); + if (!email.trim() || !password) { + setError("Please enter your Garmin email and password."); + return; + } + setBusy(true); + setError(null); + try { + const profile = await api.getProfile(); + await api.updateProfile({ ...profile, GarminEmail: email.trim(), GarminPassword: password }); + setAuth(await api.login()); + } catch (err) { + setError(err instanceof Error ? err.message : "Something went wrong, please try again."); + } finally { + setBusy(false); + } + } + + async function submitMFA() { + if (!code.trim()) return; + setBusy(true); + setError(null); + try { + setAuth(await api.submitMFA(code.trim())); + setCode(""); + } catch (err) { + setError(err instanceof Error ? err.message : "Something went wrong, please try again."); + } finally { + setBusy(false); + } + } + + const status = auth?.status; + + return ( +
+

🧞‍♀️ Connect your Garmin account

+

geniusrun needs your Garmin credentials to sync your activities.

+ + {status !== "authenticated" && status !== "mfa_required" && ( +
+ setEmail(e.target.value)} + disabled={busy} + autoFocus + /> + setPassword(e.target.value)} + disabled={busy} + /> + +
+ )} + + {status === "mfa_required" && ( +
+ setCode(e.target.value)} + onKeyDown={(e) => e.key === "Enter" && submitMFA()} + disabled={busy} + autoFocus + /> + +
+ )} + + {status === "authenticated" && ( + <> +

Connected to Garmin.

+ + + )} + + {auth?.message && status !== "authenticated" &&

{auth.message}

} + {error &&

{error}

} + +
+ +
+
+ ); +} diff --git a/frontend/src/CreateProfile.css b/frontend/src/CreateProfile.css index 01bda25..3b3698b 100644 --- a/frontend/src/CreateProfile.css +++ b/frontend/src/CreateProfile.css @@ -41,3 +41,16 @@ .create-profile-error { color: #ef4444; } + +.create-profile-message { + margin: 0; + color: #9ca3af; +} + +.create-profile-mfa { + display: flex; + flex-direction: column; + gap: 0.75rem; + width: 100%; + max-width: 320px; +} diff --git a/frontend/src/CreateProfile.tsx b/frontend/src/CreateProfile.tsx index ae621b3..4618f69 100644 --- a/frontend/src/CreateProfile.tsx +++ b/frontend/src/CreateProfile.tsx @@ -1,11 +1,12 @@ import { useState } from "react"; -import { api } from "./api/client"; +import { api, BASE_URL } from "./api/client"; import "./CreateProfile.css"; // Shown once, right after a brand-new OIDC login, before the account has a -// geniusrun profile at all. The only thing asked for is a display name -- -// Garmin credentials and every other tunable are filled in afterward via -// the existing Profile screen, same as a fresh single-user install today. +// geniusrun profile at all. Only asks for a display name -- Garmin +// credentials are collected next, on the ConnectGarmin screen (see +// LoginGate), which every other tunable is still filled in afterward via +// the existing Profile screen. export function CreateProfile({ onCreated }: { onCreated: (displayName: string) => void }) { const [displayName, setDisplayName] = useState(""); const [submitting, setSubmitting] = useState(false); @@ -32,7 +33,7 @@ export function CreateProfile({ onCreated }: { onCreated: (displayName: string) return (

🧞‍♀️ Welcome to geniusrun

-

Let's set up your profile. You can add your Garmin account afterward.

+

Let's set up your profile. You'll connect your Garmin account next.

+
+ +
); } diff --git a/frontend/src/LoginGate.tsx b/frontend/src/LoginGate.tsx index 73e63f1..b533cd1 100644 --- a/frontend/src/LoginGate.tsx +++ b/frontend/src/LoginGate.tsx @@ -2,6 +2,7 @@ import { useEffect, useState } from "react"; import { api, BASE_URL } from "./api/client"; import "./LoginGate.css"; import App from "./App"; +import { ConnectGarmin } from "./ConnectGarmin"; import { CreateProfile } from "./CreateProfile"; import type { SessionInfo } from "./types/api"; @@ -17,7 +18,10 @@ const AUTH_ERROR_MESSAGES: Record = { // this is the first fork -- "show the login screen" vs "show the app." A // second fork, once authenticated, is whether the session's account has a // provisioned profile yet (session.has_profile) -- a brand-new OIDC login -// sees CreateProfile instead of App until it submits one. +// sees CreateProfile instead of App until it submits one. A third fork, +// once provisioned, is whether Garmin has ever been successfully connected +// (session.garmin_connected) -- mandatory, and re-checked on every login, +// not just immediately after signup. export function LoginGate() { const [status, setStatus] = useState("loading"); const [session, setSession] = useState(null); @@ -57,5 +61,9 @@ export function LoginGate() { ); } + if (!session!.garmin_connected) { + return setSession((s) => (s ? { ...s, garmin_connected: true } : s))} />; + } + return ; } diff --git a/frontend/src/types/api.ts b/frontend/src/types/api.ts index 0c2359d..fb7f7e2 100644 --- a/frontend/src/types/api.ts +++ b/frontend/src/types/api.ts @@ -191,6 +191,7 @@ export interface SessionInfo { email: string; has_profile: boolean; display_name?: string; + garmin_connected: boolean; } export interface DetailFillProgress {