From 6431cad59bda3da4bafb58fdf91f6a946e42af43 Mon Sep 17 00:00:00 2001 From: Peter Stone Date: Sun, 20 Sep 2026 17:44:02 +0000 Subject: Surface a dead OAuth grant instead of reporting it as connected MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit With the cancelled-context bug fixed, the refresh actually reaches Google and the real state of the Google Tasks grant becomes visible: it answers invalid_grant. The stored refresh token has been dead since roughly 2026-08-16. The earlier bug had been masking this — the refresh never got far enough to be rejected. Settings treated the presence of a token row as "Connected", so it reported Google Tasks as connected for a month while every fetch failed. A refresh failure is now persisted (migration 035 adds last_error/last_error_at to oauth_tokens) and cleared on the next success, and settings reports "connected" only when there is no recorded failure. invalid_grant is classified specially, because it is the one failure retrying cannot fix: the message tells the user to reconnect, and notes that a weekly recurrence means the OAuth client is still in Testing publishing status, where Google expires refresh tokens after seven days. Other errors — a network blip, a 5xx — are left as transient rather than pushing the user through consent needlessly. Reconnecting at /settings is still a manual step; nothing here can perform consent on the user's behalf. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_018AwnALZL6bD4VBtATDVv3B --- internal/api/google_tasks_oauth.go | 25 +++++++++ internal/api/google_tasks_oauth_test.go | 91 +++++++++++++++++++++++++++++++-- internal/handlers/settings.go | 21 +++++++- internal/store/oauth_tokens.go | 48 ++++++++++++++++- internal/store/oauth_tokens_test.go | 4 +- migrations/035_oauth_token_errors.sql | 14 +++++ migrations/README.md | 3 +- 7 files changed, 198 insertions(+), 8 deletions(-) create mode 100644 migrations/035_oauth_token_errors.sql diff --git a/internal/api/google_tasks_oauth.go b/internal/api/google_tasks_oauth.go index 6edba21..c6f816c 100644 --- a/internal/api/google_tasks_oauth.go +++ b/internal/api/google_tasks_oauth.go @@ -3,6 +3,7 @@ package api import ( "context" "fmt" + "strings" "time" "golang.org/x/oauth2" @@ -18,6 +19,21 @@ import ( type TokenStore interface { GetOAuthToken(source string) (*oauth2.Token, error) SaveOAuthToken(source string, tok *oauth2.Token) error + // RecordOAuthTokenError persists why a refresh failed, so the settings page + // can distinguish "connected" from "has a row but the grant is dead". + RecordOAuthTokenError(source string, message string) error +} + +// NeedsReconnect reports whether an error means the stored grant is finished +// and only a fresh consent will fix it. +// +// Google answers invalid_grant when a refresh token has been revoked, +// superseded, or has simply aged out — which for an OAuth client still in +// "Testing" publishing status happens after seven days. Retrying cannot help; +// the user has to reconnect. Any other error (a network blip, a 5xx) is worth +// retrying and must not push the user through consent needlessly. +func NeedsReconnect(err error) bool { + return err != nil && strings.Contains(err.Error(), "invalid_grant") } // GoogleTasksOAuthSource is the key GoogleTasksClient's token is stored @@ -77,6 +93,15 @@ func (d *dbTokenSource) Token() (*oauth2.Token, error) { refreshed, err := d.config.TokenSource(ctx, current).Token() if err != nil { + // Record it: a failed refresh is invisible otherwise, and an + // invalid_grant in particular means no amount of waiting will fix it. + msg := err.Error() + if NeedsReconnect(err) { + msg = "Google rejected the saved authorisation (invalid_grant) — reconnect to continue. " + + "If this recurs weekly, the OAuth client is still in Testing publishing status, " + + "which expires refresh tokens after 7 days." + } + _ = d.store.RecordOAuthTokenError(d.source, msg) return nil, fmt.Errorf("refreshing token: %w", err) } diff --git a/internal/api/google_tasks_oauth_test.go b/internal/api/google_tasks_oauth_test.go index 93acd76..222baab 100644 --- a/internal/api/google_tasks_oauth_test.go +++ b/internal/api/google_tasks_oauth_test.go @@ -15,9 +15,10 @@ import ( // memTokenStore is an in-memory TokenStore. type memTokenStore struct { - mu sync.Mutex - token *oauth2.Token - saved int + mu sync.Mutex + token *oauth2.Token + saved int + lastError string } func (m *memTokenStore) GetOAuthToken(string) (*oauth2.Token, error) { @@ -30,6 +31,13 @@ func (m *memTokenStore) GetOAuthToken(string) (*oauth2.Token, error) { return &cp, nil } +func (m *memTokenStore) RecordOAuthTokenError(_ string, message string) error { + m.mu.Lock() + defer m.mu.Unlock() + m.lastError = message + return nil +} + func (m *memTokenStore) SaveOAuthToken(_ string, tok *oauth2.Token) error { m.mu.Lock() defer m.mu.Unlock() @@ -153,3 +161,80 @@ func TestDBTokenSource_NotConnected(t *testing.T) { t.Errorf("error = %q, want it to mention not being connected", err) } } + +// invalid_grant means the saved authorisation is finished — retrying cannot +// help, and the user has to reconnect. Anything else is worth retrying, so +// misclassifying it would push the user through consent for a network blip. +func TestNeedsReconnect(t *testing.T) { + cases := []struct { + err error + want bool + }{ + {errors.New(`oauth2: "invalid_grant" "Bad Request"`), true}, + {errors.New("refreshing token: oauth2: \"invalid_grant\" \"Token has been expired or revoked.\""), true}, + {errors.New("Post \"https://oauth2.googleapis.com/token\": dial tcp: i/o timeout"), false}, + {errors.New("oauth2: cannot fetch token: 503 Service Unavailable"), false}, + {nil, false}, + } + for _, c := range cases { + if got := NeedsReconnect(c.err); got != c.want { + t.Errorf("NeedsReconnect(%v) = %v, want %v", c.err, got, c.want) + } + } +} + +// A failed refresh must be recorded, or settings keeps showing "connected" +// while every background fetch fails — which is how Google Tasks appeared +// healthy for a month while returning nothing. +func TestDBTokenSource_RecordsRefreshFailure(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusBadRequest) + _, _ = w.Write([]byte(`{"error":"invalid_grant","error_description":"Token has been expired or revoked."}`)) + })) + defer srv.Close() + + store := &memTokenStore{token: expiredToken()} + src := &dbTokenSource{ + store: store, + source: GoogleTasksOAuthSource, + config: &oauth2.Config{ + ClientID: "id", + ClientSecret: "secret", + Endpoint: oauth2.Endpoint{TokenURL: srv.URL}, + }, + } + + if _, err := src.Token(); err == nil { + t.Fatal("expected the refresh to fail") + } + if store.lastError == "" { + t.Fatal("the failure was not recorded") + } + if !strings.Contains(store.lastError, "reconnect") { + t.Errorf("recorded error should tell the user to reconnect, got %q", store.lastError) + } +} + +// A successful refresh must clear a previously recorded failure; the store's +// SaveOAuthToken does that, so this pins that saving is what happens on success. +func TestDBTokenSource_SuccessSavesToken(t *testing.T) { + hits := 0 + srv := tokenServer(t, &hits) + store := &memTokenStore{token: expiredToken()} + src := &dbTokenSource{ + store: store, + source: GoogleTasksOAuthSource, + config: &oauth2.Config{ + ClientID: "id", + ClientSecret: "secret", + Endpoint: oauth2.Endpoint{TokenURL: srv.URL}, + }, + } + if _, err := src.Token(); err != nil { + t.Fatalf("Token(): %v", err) + } + if store.saved == 0 { + t.Error("a refreshed token should be persisted") + } +} diff --git a/internal/handlers/settings.go b/internal/handlers/settings.go index d6d78c0..fd63518 100644 --- a/internal/handlers/settings.go +++ b/internal/handlers/settings.go @@ -23,6 +23,10 @@ func (h *Handler) HandleSettingsPage(w http.ResponseWriter, r *http.Request) { } googleTasksToken, _ := h.store.GetOAuthToken(api.GoogleTasksOAuthSource) + // A stored row is not the same as a working grant: an invalid_grant refresh + // failure leaves the row in place while every fetch fails. Treat a recorded + // error as "needs reconnecting" rather than showing a reassuring "Connected". + googleTasksStatus, _ := h.store.GetOAuthTokenStatus(api.GoogleTasksOAuthSource) icsFeeds, _ := h.store.GetICSFeeds() data := struct { @@ -44,9 +48,12 @@ func (h *Handler) HandleSettingsPage(w http.ResponseWriter, r *http.Request) { CSRFToken: auth.GetCSRFTokenFromContext(r.Context()), WebAuthnEnabled: h.WebAuthnEnabled, GoogleTasksOAuthReady: h.googleTasksOAuthConfig != nil, - GoogleTasksConnected: googleTasksToken != nil, + GoogleTasksConnected: googleTasksToken != nil && googleTasksStatus.LastError == "", Feeds: toICSFeedViews(icsFeeds), - GoogleTasksError: r.URL.Query().Get("google_tasks_error"), + GoogleTasksError: firstNonEmpty( + r.URL.Query().Get("google_tasks_error"), + googleTasksStatus.LastError, + ), } if err := h.renderer.Render(w, "settings.html", data); err != nil { @@ -160,3 +167,13 @@ func (h *Handler) HandleToggleSourceConfig(w http.ResponseWriter, r *http.Reques w.Header().Set("Content-Type", "application/json") json.NewEncoder(w).Encode(map[string]bool{"enabled": enabled}) } + +// firstNonEmpty returns the first non-empty string, or "". +func firstNonEmpty(values ...string) string { + for _, v := range values { + if v != "" { + return v + } + } + return "" +} diff --git a/internal/store/oauth_tokens.go b/internal/store/oauth_tokens.go index 12df515..d216269 100644 --- a/internal/store/oauth_tokens.go +++ b/internal/store/oauth_tokens.go @@ -50,11 +50,57 @@ func (s *Store) SaveOAuthToken(source string, tok *oauth2.Token) error { refresh_token = excluded.refresh_token, token_type = excluded.token_type, expiry = excluded.expiry, - updated_at = CURRENT_TIMESTAMP + updated_at = CURRENT_TIMESTAMP, + -- A successful save means the grant works again; drop any recorded + -- failure so settings stops telling the user to reconnect. + last_error = NULL, + last_error_at = NULL `, source, tok.AccessToken, tok.RefreshToken, tok.TokenType, expiry) return err } +// RecordOAuthTokenError stores why a token stopped working. +// +// A stored row used to mean "connected" on its own, so a dead refresh token +// showed as connected while every background fetch failed. Persisting the +// reason is what lets settings say "reconnect" instead. +func (s *Store) RecordOAuthTokenError(source string, message string) error { + _, err := s.db.Exec(` + UPDATE oauth_tokens + SET last_error = ?, last_error_at = CURRENT_TIMESTAMP + WHERE source = ? + `, message, source) + return err +} + +// OAuthTokenStatus is a stored grant plus whether it is currently working. +type OAuthTokenStatus struct { + Connected bool + LastError string + LastErrorAt *time.Time +} + +// GetOAuthTokenStatus reports whether a source is usable, not merely present. +func (s *Store) GetOAuthTokenStatus(source string) (OAuthTokenStatus, error) { + var lastError sql.NullString + var lastErrorAt sql.NullTime + err := s.db.QueryRow( + `SELECT COALESCE(last_error, ''), last_error_at FROM oauth_tokens WHERE source = ?`, + source).Scan(&lastError, &lastErrorAt) + if err == sql.ErrNoRows { + return OAuthTokenStatus{}, nil + } + if err != nil { + return OAuthTokenStatus{}, err + } + st := OAuthTokenStatus{Connected: true, LastError: lastError.String} + if lastErrorAt.Valid { + t := lastErrorAt.Time + st.LastErrorAt = &t + } + return st, nil +} + // DeleteOAuthToken removes a source's stored token (disconnect). func (s *Store) DeleteOAuthToken(source string) error { _, err := s.db.Exec(`DELETE FROM oauth_tokens WHERE source = ?`, source) diff --git a/internal/store/oauth_tokens_test.go b/internal/store/oauth_tokens_test.go index 20c4a79..435a757 100644 --- a/internal/store/oauth_tokens_test.go +++ b/internal/store/oauth_tokens_test.go @@ -25,7 +25,9 @@ func newOAuthTokensTestStore(t *testing.T) *Store { refresh_token TEXT NOT NULL, token_type TEXT NOT NULL DEFAULT 'Bearer', expiry DATETIME, - updated_at DATETIME DEFAULT CURRENT_TIMESTAMP + updated_at DATETIME DEFAULT CURRENT_TIMESTAMP, + last_error TEXT, + last_error_at DATETIME ) `); err != nil { t.Fatal(err) diff --git a/migrations/035_oauth_token_errors.sql b/migrations/035_oauth_token_errors.sql new file mode 100644 index 0000000..c0100c0 --- /dev/null +++ b/migrations/035_oauth_token_errors.sql @@ -0,0 +1,14 @@ +-- Record why an OAuth token stopped working. +-- +-- A stored token row was treated as "connected", full stop. When the refresh +-- token itself goes bad — Google answers invalid_grant, which happens when it +-- is revoked, superseded, or (for an OAuth client still in Testing publishing +-- status) simply older than seven days — settings still showed Google Tasks as +-- connected while every fetch failed in the background. +-- +-- Persisting the failure lets settings say "reconnect" instead of "connected", +-- which is the difference between a five-second fix and not knowing anything +-- is wrong. Cleared on the next successful refresh. + +ALTER TABLE oauth_tokens ADD COLUMN last_error TEXT; +ALTER TABLE oauth_tokens ADD COLUMN last_error_at DATETIME; diff --git a/migrations/README.md b/migrations/README.md index 07320af..0382065 100644 --- a/migrations/README.md +++ b/migrations/README.md @@ -30,10 +30,11 @@ Migrations are applied automatically on application startup via `store.RunMigrat | 032 | `032_import_trello_shopping_items.sql` | One-time import of Trello Shopping-board cards (Hardware Store + unlisted) into user items, ahead of removing Trello as a shopping source | | 033 | `033_ics_feeds.sql` | Subscribed iCalendar (.ics) feeds, merged into calendar_events | | 034 | `034_shopping_stores_and_sections.sql` | Store/section registry + `user_shopping_items.section` | +| 035 | `035_oauth_token_errors.sql` | Persist why an OAuth refresh failed, so settings can say "reconnect" | ## Creating New Migrations -1. Create file: `migrations/NNN_description.sql` (next number is 035) +1. Create file: `migrations/NNN_description.sql` (next number is 036) 2. Write idempotent SQL (use `IF NOT EXISTS` where possible) 3. Test locally: `go run cmd/dashboard/main.go` 4. Verify: Check `schema_migrations` table for new entry -- cgit v1.2.3