summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
Diffstat
-rw-r--r--internal/api/google_tasks_oauth.go27
-rw-r--r--internal/api/google_tasks_oauth_test.go190
2 files changed, 135 insertions, 82 deletions
diff --git a/internal/api/google_tasks_oauth.go b/internal/api/google_tasks_oauth.go
index 1f5f8c5..6edba21 100644
--- a/internal/api/google_tasks_oauth.go
+++ b/internal/api/google_tasks_oauth.go
@@ -3,6 +3,7 @@ package api
import (
"context"
"fmt"
+ "time"
"golang.org/x/oauth2"
"golang.org/x/oauth2/google"
@@ -45,12 +46,14 @@ func GoogleTasksOAuthConfig(clientID, clientSecret, redirectURL string) *oauth2.
// call -- cheap enough to just hit the DB each time rather than add a
// second layer of in-process caching.
type dbTokenSource struct {
- ctx context.Context
store TokenStore
source string
config *oauth2.Config
}
+// tokenRefreshTimeout bounds a single refresh round-trip to Google.
+const tokenRefreshTimeout = 30 * time.Second
+
func (d *dbTokenSource) Token() (*oauth2.Token, error) {
current, err := d.store.GetOAuthToken(d.source)
if err != nil {
@@ -60,7 +63,19 @@ func (d *dbTokenSource) Token() (*oauth2.Token, error) {
return nil, fmt.Errorf("google tasks not connected -- visit /settings and connect it")
}
- refreshed, err := d.config.TokenSource(d.ctx, current).Token()
+ // Deliberately NOT a context captured at construction time. main.go builds
+ // the client with a short-lived init context and cancels it the moment
+ // construction returns; reusing that here meant the first refresh after the
+ // startup access token aged out — roughly an hour in — failed with
+ // "context canceled", and so did every one after it. Google Tasks was
+ // broken from that point until a restart, which bought another hour.
+ //
+ // A token refresh is not logically part of whichever request happened to
+ // trigger it, so it gets its own background context with its own bound.
+ ctx, cancel := context.WithTimeout(context.Background(), tokenRefreshTimeout)
+ defer cancel()
+
+ refreshed, err := d.config.TokenSource(ctx, current).Token()
if err != nil {
return nil, fmt.Errorf("refreshing token: %w", err)
}
@@ -87,7 +102,11 @@ func (d *dbTokenSource) Token() (*oauth2.Token, error) {
// succeeds; API calls fail with the dbTokenSource error above until a
// token exists (see HandleGoogleTasksOAuthCallback).
func NewGoogleTasksOAuthClient(ctx context.Context, oauthConfig *oauth2.Config, tokenStore TokenStore, tasklistID, timezone string) (*GoogleTasksClient, error) {
- src := &dbTokenSource{ctx: ctx, store: tokenStore, source: GoogleTasksOAuthSource, config: oauthConfig}
- httpClient := oauth2.NewClient(ctx, src)
+ src := &dbTokenSource{store: tokenStore, source: GoogleTasksOAuthSource, config: oauthConfig}
+ // context.Background(), not ctx: oauth2.NewClient binds the context it is
+ // given to the returned client's transport for its whole lifetime, and ctx
+ // here is the caller's short-lived init context. The per-call context still
+ // applies — it comes from the tasks service methods themselves.
+ httpClient := oauth2.NewClient(context.Background(), src)
return NewGoogleTasksClient(ctx, option.WithHTTPClient(httpClient), tasklistID, timezone)
}
diff --git a/internal/api/google_tasks_oauth_test.go b/internal/api/google_tasks_oauth_test.go
index b4ea6f3..3503cbb 100644
--- a/internal/api/google_tasks_oauth_test.go
+++ b/internal/api/google_tasks_oauth_test.go
@@ -2,121 +2,155 @@ package api
import (
"context"
- "encoding/json"
+ "errors"
"net/http"
"net/http/httptest"
+ "strings"
+ "sync"
"testing"
"time"
"golang.org/x/oauth2"
)
-// fakeTokenStore is a minimal in-memory TokenStore for tests.
-type fakeTokenStore struct {
- tokens map[string]*oauth2.Token
- saved []*oauth2.Token // records every SaveOAuthToken call, in order
+// memTokenStore is an in-memory TokenStore.
+type memTokenStore struct {
+ mu sync.Mutex
+ token *oauth2.Token
+ saved int
}
-func newFakeTokenStore() *fakeTokenStore {
- return &fakeTokenStore{tokens: make(map[string]*oauth2.Token)}
-}
-
-func (f *fakeTokenStore) GetOAuthToken(source string) (*oauth2.Token, error) {
- return f.tokens[source], nil
+func (m *memTokenStore) GetOAuthToken(string) (*oauth2.Token, error) {
+ m.mu.Lock()
+ defer m.mu.Unlock()
+ if m.token == nil {
+ return nil, nil
+ }
+ cp := *m.token
+ return &cp, nil
}
-func (f *fakeTokenStore) SaveOAuthToken(source string, tok *oauth2.Token) error {
- f.tokens[source] = tok
- f.saved = append(f.saved, tok)
+func (m *memTokenStore) SaveOAuthToken(_ string, tok *oauth2.Token) error {
+ m.mu.Lock()
+ defer m.mu.Unlock()
+ cp := *tok
+ m.token = &cp
+ m.saved++
return nil
}
-func TestDbTokenSource_NotConnected_ReturnsError(t *testing.T) {
- store := newFakeTokenStore()
- src := &dbTokenSource{ctx: context.Background(), store: store, source: "google_tasks", config: &oauth2.Config{}}
+// tokenServer stands in for Google's token endpoint.
+func tokenServer(t *testing.T, hits *int) *httptest.Server {
+ t.Helper()
+ srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+ *hits++
+ w.Header().Set("Content-Type", "application/json")
+ // A refresh response that omits refresh_token, as Google's does.
+ _, _ = w.Write([]byte(`{"access_token":"new-access","token_type":"Bearer","expires_in":3600}`))
+ }))
+ t.Cleanup(srv.Close)
+ return srv
+}
- if _, err := src.Token(); err == nil {
- t.Error("expected an error when no token is stored, got nil")
+func expiredToken() *oauth2.Token {
+ return &oauth2.Token{
+ AccessToken: "old-access",
+ RefreshToken: "stored-refresh",
+ TokenType: "Bearer",
+ Expiry: time.Now().Add(-time.Hour),
}
}
-func TestDbTokenSource_NotExpired_ReturnsWithoutPersisting(t *testing.T) {
- store := newFakeTokenStore()
- future := time.Now().Add(time.Hour)
- store.tokens["google_tasks"] = &oauth2.Token{
- AccessToken: "still-valid", RefreshToken: "r1", TokenType: "Bearer", Expiry: future,
+// The context handed to the client constructor is short-lived: main.go builds
+// it with a timeout and cancels it as soon as construction returns. Capturing
+// it and reusing it for every later token refresh means the first refresh
+// after the initial access token expires — about an hour after startup — fails
+// with "context canceled", and every one after it, permanently.
+//
+// This is what actually broke Google Tasks in production: four task lists, the
+// same error on every fetch, from the moment the startup token aged out.
+func TestDBTokenSource_SurvivesACancelledConstructionContext(t *testing.T) {
+ hits := 0
+ srv := tokenServer(t, &hits)
+
+ // The construction context is cancelled immediately, exactly as main.go
+ // does. Nothing the token source does later may depend on it.
+ ctx, cancel := context.WithCancel(context.Background())
+ cancel()
+ if ctx.Err() == nil {
+ t.Fatal("test setup: the construction context should already be cancelled")
+ }
+
+ store := &memTokenStore{token: expiredToken()}
+ src := &dbTokenSource{
+ store: store,
+ source: GoogleTasksOAuthSource,
+ config: &oauth2.Config{
+ ClientID: "id",
+ ClientSecret: "secret",
+ Endpoint: oauth2.Endpoint{TokenURL: srv.URL},
+ },
}
- src := &dbTokenSource{ctx: context.Background(), store: store, source: "google_tasks", config: &oauth2.Config{}}
tok, err := src.Token()
if err != nil {
- t.Fatalf("Token: %v", err)
+ if errors.Is(err, context.Canceled) {
+ t.Fatalf("token refresh used the cancelled construction context: %v", err)
+ }
+ t.Fatalf("Token() failed: %v", err)
}
- if tok.AccessToken != "still-valid" {
- t.Errorf("AccessToken = %q, want unchanged %q", tok.AccessToken, "still-valid")
+ if tok.AccessToken != "new-access" {
+ t.Errorf("AccessToken = %q, want the refreshed one", tok.AccessToken)
}
- if len(store.saved) != 0 {
- t.Errorf("SaveOAuthToken called %d times, want 0 (nothing changed, no refresh needed)", len(store.saved))
+ if hits == 0 {
+ t.Error("expected the token endpoint to be called")
}
}
-func TestDbTokenSource_Refresh_PreservesOriginalRefreshTokenWhenOmitted(t *testing.T) {
- // Simulates Google's real behavior: a refresh response often omits
- // refresh_token entirely. Without the fix, that would overwrite the
- // stored refresh token with "" and permanently break future refreshes.
- server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
- w.Header().Set("Content-Type", "application/json")
- _ = json.NewEncoder(w).Encode(map[string]interface{}{
- "access_token": "refreshed-access-token",
- "token_type": "Bearer",
- "expires_in": 3600,
- // no refresh_token in the response, matching Google's real behavior
- })
- }))
- defer server.Close()
-
- store := newFakeTokenStore()
- expired := time.Now().Add(-time.Hour)
- store.tokens["google_tasks"] = &oauth2.Token{
- AccessToken: "stale-access-token", RefreshToken: "original-refresh-token", TokenType: "Bearer", Expiry: expired,
+// Google omits refresh_token on a refresh response. Overwriting the stored one
+// with an empty string would permanently break every later refresh.
+func TestDBTokenSource_KeepsRefreshTokenWhenResponseOmitsIt(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},
+ },
}
- cfg := &oauth2.Config{
- ClientID: "test-client",
- ClientSecret: "test-secret",
- Endpoint: oauth2.Endpoint{TokenURL: server.URL},
+ if _, err := src.Token(); err != nil {
+ t.Fatalf("Token(): %v", err)
}
- src := &dbTokenSource{ctx: context.Background(), store: store, source: "google_tasks", config: cfg}
- tok, err := src.Token()
- if err != nil {
- t.Fatalf("Token: %v", err)
+ saved, _ := store.GetOAuthToken(GoogleTasksOAuthSource)
+ if saved.RefreshToken != "stored-refresh" {
+ t.Errorf("RefreshToken = %q, want the stored one preserved", saved.RefreshToken)
}
- if tok.AccessToken != "refreshed-access-token" {
- t.Errorf("AccessToken = %q, want %q", tok.AccessToken, "refreshed-access-token")
- }
- if tok.RefreshToken != "original-refresh-token" {
- t.Errorf("RefreshToken = %q, want original preserved %q", tok.RefreshToken, "original-refresh-token")
- }
-
- if len(store.saved) != 1 {
- t.Fatalf("SaveOAuthToken called %d times, want 1", len(store.saved))
- }
- if store.saved[0].RefreshToken != "original-refresh-token" {
- t.Errorf("persisted RefreshToken = %q, want %q", store.saved[0].RefreshToken, "original-refresh-token")
+ if saved.AccessToken != "new-access" {
+ t.Errorf("AccessToken = %q, want it updated", saved.AccessToken)
}
}
-func TestGoogleTasksOAuthConfig_HasTasksScopeAndEndpoint(t *testing.T) {
- cfg := GoogleTasksOAuthConfig("id", "secret", "https://doot.terst.org/settings/google-tasks/callback")
-
- if cfg.ClientID != "id" || cfg.ClientSecret != "secret" {
- t.Errorf("ClientID/Secret = %q/%q, want id/secret", cfg.ClientID, cfg.ClientSecret)
+// Before the user has connected, the error must say so rather than surfacing
+// as an opaque transport failure.
+func TestDBTokenSource_NotConnected(t *testing.T) {
+ src := &dbTokenSource{
+ store: &memTokenStore{},
+ source: GoogleTasksOAuthSource,
+ config: &oauth2.Config{},
}
- if cfg.RedirectURL != "https://doot.terst.org/settings/google-tasks/callback" {
- t.Errorf("RedirectURL = %q", cfg.RedirectURL)
+ _, err := src.Token()
+ if err == nil {
+ t.Fatal("expected an error when no token is stored")
}
- if len(cfg.Scopes) != 1 || cfg.Scopes[0] != "https://www.googleapis.com/auth/tasks" {
- t.Errorf("Scopes = %v, want [tasks scope]", cfg.Scopes)
+ if !strings.Contains(err.Error(), "not connected") {
+ t.Errorf("error = %q, want it to mention not being connected", err)
}
}
+
generated by cgit v1.2.3 (git 2.39.1) at 2026-11-14 21:23:54 +0000