Fix a login race condition

go.mod

@@ -3,16 +3,17 @@ module github.com/webshare-proxy/webshare-cli
 go 1.23.0
 
 require (
+	github.com/gofrs/flock v0.12.1
 	github.com/spf13/cobra v1.10.2
 	github.com/webshare-proxy/webshare-go v0.0.0-20260728085640-4f665a81a4e7
+	github.com/zalando/go-keyring v0.2.8
+	golang.org/x/oauth2 v0.30.0
 )
 
 require (
 	github.com/danieljoos/wincred v1.2.3 // indirect
 	github.com/godbus/dbus/v5 v5.2.2 // indirect
 	github.com/inconshreveable/mousetrap v1.1.0 // indirect
 	github.com/spf13/pflag v1.0.9 // indirect
-	github.com/zalando/go-keyring v0.2.8 // indirect
-	golang.org/x/oauth2 v0.30.0 // indirect
 	golang.org/x/sys v0.27.0 // indirect
 )

go.sum

@@ -1,15 +1,25 @@
 github.com/cpuguy83/go-md2man/v2 v2.0.6/go.mod h1:oOW0eioCTA6cOiMLiUPZOpcVxMig6NIQQ7OS05n1F4g=
 github.com/danieljoos/wincred v1.2.3 h1:v7dZC2x32Ut3nEfRH+vhoZGvN72+dQ/snVXo/vMFLdQ=
 github.com/danieljoos/wincred v1.2.3/go.mod h1:6qqX0WNrS4RzPZ1tnroDzq9kY3fu1KwE7MRLQK4X0bs=
+github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c=
+github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
 github.com/godbus/dbus/v5 v5.2.2 h1:TUR3TgtSVDmjiXOgAAyaZbYmIeP3DPkld3jgKGV8mXQ=
 github.com/godbus/dbus/v5 v5.2.2/go.mod h1:3AAv2+hPq5rdnr5txxxRwiGjPXamgoIHgz9FPBfOp3c=
+github.com/gofrs/flock v0.12.1 h1:MTLVXXHf8ekldpJk3AKicLij9MdwOWkZ+a/jHHZby9E=
+github.com/gofrs/flock v0.12.1/go.mod h1:9zxTsyu5xtJ9DK+1tFZyibEV7y3uwDxPPfbxeeHCoD0=
 github.com/inconshreveable/mousetrap v1.1.0 h1:wN+x4NVGpMsO7ErUn/mUI3vEoE6Jt13X2s0bqwp9tc8=
 github.com/inconshreveable/mousetrap v1.1.0/go.mod h1:vpF70FUmC8bwa3OWnCshd2FqLfsEA9PFc4w1p2J65bw=
+github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM=
+github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4=
 github.com/russross/blackfriday/v2 v2.1.0/go.mod h1:+Rmxgy9KzJVeS9/2gXHxylqXiyQDYRxCVz55jmeOWTM=
 github.com/spf13/cobra v1.10.2 h1:DMTTonx5m65Ic0GOoRY2c16WCbHxOOw6xxezuLaBpcU=
 github.com/spf13/cobra v1.10.2/go.mod h1:7C1pvHqHw5A4vrJfjNwvOdzYu0Gml16OCs2GRiTUUS4=
 github.com/spf13/pflag v1.0.9 h1:9exaQaMOCwffKiiiYk6/BndUBv+iRViNW+4lEMi0PvY=
 github.com/spf13/pflag v1.0.9/go.mod h1:McXfInJRrz4CZXVZOBLb0bTZqETkiAhM9Iw0y3An2Bg=
+github.com/stretchr/objx v0.5.2 h1:xuMeJ0Sdp5ZMRXx/aWO6RZxdr3beISkG5/G/aIRr3pY=
+github.com/stretchr/objx v0.5.2/go.mod h1:FRsXN1f5AsAjCGJKqEizvkpNtU+EGNCLh3NxZ/8L+MA=
+github.com/stretchr/testify v1.11.1 h1:7s2iGBzp5EwR7/aIZr8ao5+dra3wiQyKjjFuvgVKu7U=
+github.com/stretchr/testify v1.11.1/go.mod h1:wZwfW3scLgRK+23gO65QZefKpKQRnfz6sD981Nm4B6U=
 github.com/webshare-proxy/webshare-go v0.0.0-20260728085640-4f665a81a4e7 h1:6mmoTlwFIOGvQXf3AQ7jXrcolz9QaeX6RJ/Ikv0e3vY=
 github.com/webshare-proxy/webshare-go v0.0.0-20260728085640-4f665a81a4e7/go.mod h1:WrMcbXqIzvTEWoacXEIpnALevumOg6rNaYuzp9tpdHA=
 github.com/zalando/go-keyring v0.2.8 h1:6sD/Ucpl7jNq10rM2pgqTs0sZ9V3qMrqfIIy5YPccHs=
@@ -20,3 +30,5 @@ golang.org/x/oauth2 v0.30.0/go.mod h1:B++QgG3ZKulg6sRPGD/mqlHQs5rB3Ml9erfeDY7xKl
 golang.org/x/sys v0.27.0 h1:wBqf8DvsY9Y/2P8gAfPDEYNuS30J4lPHJxXSb/nJZ+s=
 golang.org/x/sys v0.27.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA=
 gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0=
+gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA=
+gopkg.in/yaml.v3 v3.0.1/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM=

internal/auth/auth_test.go

@@ -13,6 +13,7 @@ import (
 	"os"
 	"path/filepath"
 	"strings"
+	"sync"
 	"testing"
 	"time"
 
@@ -88,6 +89,11 @@ func newFakeAS(t *testing.T) *fakeAS {
 			}
 			as.grantType = r.PostForm.Get("grant_type")
 			if as.grantType == "refresh_token" {
+				if r.PostForm.Get("refresh_token") != "refresh-1" || as.refreshed > 0 {
+					w.WriteHeader(http.StatusBadRequest)
+					writeJSON(w, map[string]any{"error": "invalid_grant"})
+					return
+				}
 				as.refreshed++
 				writeJSON(w, map[string]any{
 					"access_token": "access-2", "refresh_token": "refresh-2",
@@ -220,7 +226,7 @@ func TestExpiredAccessTokenIsRefreshedAndTheRotatedPairIsStored(t *testing.T) {
 		t.Fatalf("Save: %v", err)
 	}
 
-	source := NewTokenSource(context.Background(), as.config(t), expired)
+	source := NewTokenSource(as.config(t), expired)
 	token, err := source.Token(context.Background())
 	if err != nil {
 		t.Fatalf("Token: %v", err)
@@ -238,6 +244,48 @@ func TestExpiredAccessTokenIsRefreshedAndTheRotatedPairIsStored(t *testing.T) {
 	}
 }
 
+func TestCommandsRunningSideBySideRefreshOnlyOnce(t *testing.T) {
+	isolate(t)
+	// The file store, because it is what separate processes share; the mock
+	// keyring lives in this process's memory.
+	keyring.MockInitWithError(keyring.ErrUnsupportedPlatform)
+	as := newFakeAS(t)
+	expired := Credentials{
+		AccessToken: "access-1", RefreshToken: "refresh-1",
+		Expiry: time.Now().Add(-time.Hour), Issuer: as.issuer,
+		TokenEndpoint: as.server.URL + "/oauth/token/",
+	}
+	if err := Save(as.server.URL, &expired); err != nil {
+		t.Fatalf("Save: %v", err)
+	}
+
+	// Each source stands in for a separate command: its own copy of what was
+	// stored when it started, and nothing in memory shared with the other.
+	tokens := make([]string, 2)
+	failures := make([]error, 2)
+	var wg sync.WaitGroup
+	for i := range tokens {
+		credentials := expired
+		source := NewTokenSource(as.config(t), &credentials)
+		wg.Add(1)
+		go func() {
+			defer wg.Done()
+			token, err := source.Token(context.Background())
+			tokens[i], failures[i] = token.Value, err
+		}()
+	}
+	wg.Wait()
+
+	for i, err := range failures {
+		if err != nil {
+			t.Errorf("command %d: %v", i, err)
+		}
+	}
+	if as.refreshed != 1 || tokens[0] != "access-2" || tokens[1] != "access-2" {
+		t.Errorf("refreshed %d times, tokens = %v; want one refresh whose token both commands use", as.refreshed, tokens)
+	}
+}
+
 func TestLogoutRevokesBothTokensBeforeClearingThem(t *testing.T) {
 	isolate(t)
 	as := newFakeAS(t)

internal/auth/login.go

@@ -343,54 +343,73 @@ func revoke(ctx context.Context, cfg Config, endpoint, token string) error {
 }
 
 // NewTokenSource presents the stored access token to the API and refreshes it
-// when it expires. The context it is built with is the one a refresh uses,
-// because golang.org/x/oauth2 takes no context per call.
-func NewTokenSource(ctx context.Context, cfg Config, credentials *Credentials) webshare.TokenSource {
-	flow := &oauth2.Config{
-		ClientID: cfg.ClientID,
-		Endpoint: oauth2.Endpoint{TokenURL: credentials.TokenEndpoint, AuthStyle: oauth2.AuthStyleInParams},
-	}
+// when it expires.
+func NewTokenSource(cfg Config, credentials *Credentials) webshare.TokenSource {
 	return &persistingSource{
-		baseURL: cfg.BaseURL,
-		stored:  credentials,
-		source: flow.TokenSource(cfg.oauthContext(ctx), &oauth2.Token{
-			AccessToken:  credentials.AccessToken,
-			RefreshToken: credentials.RefreshToken,
-			Expiry:       credentials.Expiry,
-			TokenType:    "Bearer",
-		}),
+		cfg: cfg,
+		flow: &oauth2.Config{
+			ClientID: cfg.ClientID,
+			Endpoint: oauth2.Endpoint{TokenURL: credentials.TokenEndpoint, AuthStyle: oauth2.AuthStyleInParams},
+		},
+		stored: credentials,
 	}
 }
 
 // persistingSource writes the pair back whenever it rotates. The server
 // rotates both tokens on every refresh and refuses a refresh token twice, so
 // a rotation that is not stored leaves the next command with nothing usable.
 type persistingSource struct {
-	baseURL string
-	// mutex guards stored: oauth2's own source is safe for concurrent use and
-	// this wrapper has to be too, or two refreshes race and the loser spends a
-	// refresh token the server will never accept again.
+	cfg  Config
+	flow *oauth2.Config
+	// mutex guards stored: the client may ask for a token from several
+	// goroutines, and only one of them may refresh.
 	mutex  sync.Mutex
 	stored *Credentials
-	source oauth2.TokenSource
 }
 
-func (p *persistingSource) Token(context.Context) (webshare.Token, error) {
+func (p *persistingSource) Token(ctx context.Context) (webshare.Token, error) {
 	p.mutex.Lock()
 	defer p.mutex.Unlock()
-	token, err := p.source.Token()
+	if fresh(p.stored) {
+		return bearer(p.stored), nil
+	}
+	// Commands run side by side each start with the same refresh token, so
+	// they take turns: whoever gets the lock first refreshes, and the rest
+	// pick up the pair it stored instead of refreshing again.
+	unlock, err := lockRefresh(ctx)
+	if err != nil {
+		return webshare.Token{}, err
+	}
+	defer unlock()
+	latest, err := Load(p.cfg.BaseURL)
 	if err != nil {
 		return webshare.Token{}, fmt.Errorf("refreshing the stored login (run `webshare login` again): %w", err)
 	}
-	if token.AccessToken != p.stored.AccessToken {
-		p.stored.AccessToken = token.AccessToken
-		p.stored.RefreshToken = token.RefreshToken
-		p.stored.Expiry = token.Expiry
-		if err := Save(p.baseURL, p.stored); err != nil {
-			return webshare.Token{}, err
-		}
+	p.stored = latest
+	if fresh(latest) {
+		return bearer(latest), nil
 	}
-	return webshare.Token{Value: token.AccessToken, Scheme: "Bearer"}, nil
+	token, err := p.flow.TokenSource(p.cfg.oauthContext(ctx), &oauth2.Token{RefreshToken: latest.RefreshToken}).Token()
+	if err != nil {
+		return webshare.Token{}, fmt.Errorf("refreshing the stored login (run `webshare login` again): %w", err)
+	}
+	latest.AccessToken = token.AccessToken
+	latest.RefreshToken = token.RefreshToken
+	latest.Expiry = token.Expiry
+	if err := Save(p.cfg.BaseURL, latest); err != nil {
+		return webshare.Token{}, err
+	}
+	return bearer(latest), nil
+}
+
+// fresh reports whether the access token can still be sent, with the same
+// margin before expiry that golang.org/x/oauth2 allows.
+func fresh(credentials *Credentials) bool {
+	return (&oauth2.Token{AccessToken: credentials.AccessToken, Expiry: credentials.Expiry}).Valid()
+}
+
+func bearer(credentials *Credentials) webshare.Token {
+	return webshare.Token{Value: credentials.AccessToken, Scheme: "Bearer"}
 }
 
 func randomState() (string, error) {

internal/auth/store.go

@@ -2,6 +2,7 @@
 package auth
 
 import (
+	"context"
 	"encoding/json"
 	"errors"
 	"fmt"
@@ -10,6 +11,7 @@ import (
 	"path/filepath"
 	"time"
 
+	"github.com/gofrs/flock"
 	"github.com/zalando/go-keyring"
 )
 
@@ -41,15 +43,44 @@ func accountFor(baseURL string) string {
 	return parsed.Host
 }
 
-// fallbackPath is where the tokens go when the operating system has no
-// keyring to put them in. It never guesses: a relative path would drop a live
-// token into whatever directory the command was run from.
-func fallbackPath() (string, error) {
+// configDir is where the CLI keeps what it has to keep on disk. It never
+// guesses: a relative path would drop a live token into whatever directory the
+// command was run from.
+func configDir() (string, error) {
 	dir, err := os.UserConfigDir()
 	if err != nil {
 		return "", fmt.Errorf("finding somewhere to keep the login: %w", err)
 	}
-	return filepath.Join(dir, "webshare", "credentials.json"), nil
+	return filepath.Join(dir, "webshare"), nil
+}
+
+// fallbackPath is where the tokens go when the operating system has no
+// keyring to put them in.
+func fallbackPath() (string, error) {
+	dir, err := configDir()
+	if err != nil {
+		return "", err
+	}
+	return filepath.Join(dir, "credentials.json"), nil
+}
+
+// lockRefresh waits until no other webshare command is refreshing the login
+// and returns the function that lets the next one in. The operating system
+// releases the lock when its holder exits, so a command that crashes midway
+// leaves nobody waiting on it.
+func lockRefresh(ctx context.Context) (unlock func(), err error) {
+	dir, err := configDir()
+	if err != nil {
+		return nil, err
+	}
+	if err := os.MkdirAll(dir, 0o700); err != nil {
+		return nil, fmt.Errorf("creating %s: %w", dir, err)
+	}
+	lock := flock.New(filepath.Join(dir, "refresh.lock"))
+	if _, err := lock.TryLockContext(ctx, 50*time.Millisecond); err != nil {
+		return nil, fmt.Errorf("waiting for another webshare command to refresh the login: %w", err)
+	}
+	return func() { _ = lock.Unlock() }, nil
 }
 
 // Save stores the credentials in the OS keyring, falling back to a file only

internal/cmd/root.go

@@ -202,7 +202,7 @@ func newClient(ctx context.Context, flags *rootFlags) (*webshare.Client, error)
 			return nil, err
 		}
 		opts = append(opts, webshare.WithTokenSource(
-			auth.NewTokenSource(ctx, authConfig(flags, credentials.Scopes), credentials)))
+			auth.NewTokenSource(authConfig(flags, credentials.Scopes), credentials)))
 	}
 	return webshare.NewClient(opts...)
 }