From 6073abebed8843bb367033a31fef002614fb4ddc Mon Sep 17 00:00:00 2001 From: garmr-ulfr Date: Wed, 14 Oct 2026 12:54:48 -0700 Subject: [PATCH 1/8] fix: persisted identity and account correctness Settings: - Run the legacy settings migration once, gated by a .settings-migrated marker. Adopt only the winning candidate's identity and auth keys; a non-pro candidate wins only with a user_id; an unreadable candidate leaves the migration pending. Stop reading the Windows cross-dir settings.json. - Set and Patch take effect only after a successful save. Clear and the new Apply keep in-memory changes when the save fails. Skip writes the file already holds; changes before InitSettings stay in memory. - Diff uses reflect.DeepEqual; GetAllFor omits unset keys. - Startup settings are written once, through Apply, in common.Init and NewLocalBackend. - atomicfile no longer removes the target before renaming on Windows. Account: - Discard user-data and purchase responses requested before the account changed (ErrAccountChanged). - An account switch is one write that never exposes an empty identity. - Store the device list from a device-limited login. - Decode the referral v1 response as JSON. - One storeSalt path; recovery stores the new salt only for the current account; a purchaser switch clears the salt and OAuth flags. - Clear the OAuth flags on ClearUser, SignUp and Login. - Return copies of UserData rather than the stored value. - Check the pro-server error envelope once, in sendProRequest. - Switch to the purchasing account inside account on purchase/restore. --- account/client.go | 38 +- account/client_test.go | 10 + account/subscription.go | 56 +- account/subscription_test.go | 96 +++- account/user.go | 250 ++++----- account/user_refresh_test.go | 104 ++++ account/user_test.go | 141 +++++- backend/radiance.go | 4 +- common/atomicfile/atomic_file.go | 4 - common/init.go | 23 +- common/settings/legacy_crossdir.go | 45 +- common/settings/legacy_yaml.go | 11 +- common/settings/legacy_yaml_test.go | 16 +- common/settings/settings.go | 298 +++++++---- common/settings/settings_test.go | 757 +++++++++++++++++++--------- 15 files changed, 1304 insertions(+), 549 deletions(-) diff --git a/account/client.go b/account/client.go index c8aff73a..72a192d7 100644 --- a/account/client.go +++ b/account/client.go @@ -38,7 +38,9 @@ type Client struct { salt []byte saltPath string - mu sync.RWMutex + // generation advances whenever the active account may change; guarded by mu. + generation uint64 + mu sync.RWMutex } // NewClient returns a Client that caches its salt under dataDir. A nil logger @@ -72,6 +74,25 @@ func (a *Client) setSalt(salt []byte) { a.salt = salt } +func (a *Client) storeSalt(salt []byte) error { + a.mu.Lock() + defer a.mu.Unlock() + return a.storeSaltLocked(salt) +} + +// storeSaltLocked caches salt before persisting it, so a failed write never leaves a stale salt +// cached. It requires a.mu to be held. +func (a *Client) storeSaltLocked(salt []byte) error { + a.salt = salt + return writeSalt(salt, a.saltPath) +} + +func (a *Client) identityGeneration() uint64 { + a.mu.RLock() + defer a.mu.RUnlock() + return a.generation +} + func (a *Client) proBaseURL() string { if a.proURL != "" { return a.proURL @@ -177,7 +198,8 @@ func (a *Client) sendRequest( // sendProRequest sends a request to the Pro server, automatically adding the required headers, // including the device ID, user ID, and Pro token from settings, if available. If the URL is relative, -// the Pro server base URL will be prepended. +// the Pro server base URL will be prepended. A 2xx response carrying a non-empty error envelope +// is returned as an error. func (a *Client) sendProRequest( ctx context.Context, method, url string, @@ -197,7 +219,17 @@ func (a *Client) sendProRequest( headers[common.UserIDHeader] = uid } maps.Copy(headers, additionalheaders) - return a.sendRequest(ctx, method, url, queryParams, headers, body) + resp, err := a.sendRequest(ctx, method, url, queryParams, headers, body) + if err != nil { + return nil, err + } + var envelope struct { + Error string `json:"error"` + } + if json.Unmarshal(resp, &envelope) == nil && envelope.Error != "" { + return nil, fmt.Errorf("received bad response: %s", envelope.Error) + } + return resp, nil } // curlFromRequest renders req as a curl command, replacing req.Body with an diff --git a/account/client_test.go b/account/client_test.go index 2212daa6..a48d7827 100644 --- a/account/client_test.go +++ b/account/client_test.go @@ -45,3 +45,13 @@ func TestErrorResponseBodyOmittedFromLog(t *testing.T) { assert.Contains(t, buf.String(), "status=400", "error log dropped the status") assert.Contains(t, err.Error(), "invalid_code", "returned error dropped the server error code") } + +func TestProErrorEnvelope(t *testing.T) { + a, _ := newTestClient(t) + useProServer(t, a, serveJSON(map[string]string{"error": "device not found"})) + + _, err := a.RemoveDevice(context.Background(), "device") + assert.ErrorContains(t, err, "device not found") + _, err = a.ActivationCode(context.Background(), "me@example.com", "code") + assert.ErrorContains(t, err, "device not found") +} diff --git a/account/subscription.go b/account/subscription.go index 86e30625..f0ee5beb 100644 --- a/account/subscription.go +++ b/account/subscription.go @@ -74,11 +74,6 @@ func (a *Client) SubscriptionPlans(ctx context.Context, channel string) (string, if err := json.Unmarshal(resp, &plans); err != nil { return "", traces.RecordError(ctx, fmt.Errorf("unmarshaling plans response: %w", err)) } - if plans.BaseResponse != nil && plans.Error != "" { - err = fmt.Errorf("received bad response: %s", plans.Error) - a.logger.Error("retrieving plans", "error", err) - return "", traces.RecordError(ctx, err) - } return string(resp), nil } @@ -112,6 +107,9 @@ type VerifySubscriptionResponse struct { // should contain the information required by service to verify the subscription, such as the // purchase token for Google Play or the receipt for Apple. The status and subscription ID are returned // along with any error that occurred during the verification process. +// +// When the purchase belongs to a different account, the client switches to that account before +// returning, unless the active account may have changed while the request was in flight. func (a *Client) VerifySubscription(ctx context.Context, service SubscriptionService, data map[string]string) (string, error) { ctx, span := otel.Tracer(tracerName).Start(ctx, "verify_subscription") defer span.End() @@ -127,13 +125,52 @@ func (a *Client) VerifySubscription(ctx context.Context, service SubscriptionSer return "", traces.RecordError(ctx, fmt.Errorf("unsupported service: %s", service)) } + generation := a.identityGeneration() resp, err := a.sendProRequest(ctx, "POST", path, nil, nil, data) if err != nil { a.logger.Error("verifying subscription", "error", err) return "", traces.RecordError(ctx, fmt.Errorf("verifying subscription: %w", err)) } + var result VerifySubscriptionResponse + if err := json.Unmarshal(resp, &result); err != nil { + a.logger.Error("unmarshaling verify subscription response", "error", err) + } else { + a.switchToPurchaser(ctx, generation, result.ActualUserID, result.ActualUserToken) + } return string(resp), nil +} +// switchToPurchaser switches to the purchasing account unless the account may have changed since +// generation was captured. It logs rather than returns failures, because the purchase already succeeded. +func (a *Client) switchToPurchaser(ctx context.Context, generation uint64, id int64, token string) { + if id == 0 || token == "" { + return + } + a.mu.Lock() + defer a.mu.Unlock() + if a.generation != generation { + a.logger.Warn("not switching to the purchasing account", "error", traces.RecordError(ctx, ErrAccountChanged)) + return + } + switching := id != settings.GetInt64(settings.UserIDKey) + if !switching && token == settings.GetString(settings.TokenKey) { + return + } + a.generation++ + var extra settings.Settings + if switching { + // The previous account's OAuth sign-in doesn't carry over to the purchaser. + extra = settings.Settings{settings.OAuthLoginKey: false, settings.OAuthProviderKey: ""} + } + if err := a.storeIdentityLocked(id, token, extra); err != nil { + a.logger.Error("saving the purchasing account", "error", traces.RecordError(ctx, err)) + } + if switching { + // The salt cache isn't keyed by email, so the previous account's salt must go. + if err := a.storeSaltLocked(nil); err != nil { + a.logger.Error("clearing the previous account's salt", "error", traces.RecordError(ctx, err)) + } + } } type RestoreSubscriptionResponse struct { @@ -147,6 +184,9 @@ type RestoreSubscriptionResponse struct { // data should contain the fields required by the backend for the chosen service (e.g. // "purchaseToken" for Google, "receipt" for Apple); callers are responsible for populating // it. Services other than [GoogleService] and [AppleService] are rejected. +// +// When the subscription belongs to a different account, the client switches to that account +// before returning, unless the active account may have changed while the request was in flight. func (a *Client) RestoreSubscription(ctx context.Context, service SubscriptionService, data map[string]string) (*RestoreSubscriptionResponse, error) { ctx, span := otel.Tracer(tracerName).Start(ctx, "restore_subscription") defer span.End() @@ -161,6 +201,7 @@ func (a *Client) RestoreSubscription(ctx context.Context, service SubscriptionSe return nil, traces.RecordError(ctx, fmt.Errorf("unsupported service: %s", service)) } + generation := a.identityGeneration() resp, err := a.sendProRequest(ctx, "POST", path, nil, nil, data) if err != nil { a.logger.Error("restoring subscription", "error", err) @@ -170,6 +211,7 @@ func (a *Client) RestoreSubscription(ctx context.Context, service SubscriptionSe if err := json.Unmarshal(resp, &result); err != nil { return nil, traces.RecordError(ctx, fmt.Errorf("unmarshaling restore subscription response: %w", err)) } + a.switchToPurchaser(ctx, generation, result.ActualUserID, result.ActualUserToken) return &result, nil } @@ -281,9 +323,5 @@ func (a *Client) ActivationCode(ctx context.Context, email, resellerCode string) if err := json.Unmarshal(resp, &purchase); err != nil { return nil, traces.RecordError(ctx, fmt.Errorf("unmarshaling purchase response: %w", err)) } - if purchase.BaseResponse != nil && purchase.Error != "" { - a.logger.Error("retrieving subscription status", "error", purchase.Error) - return nil, traces.RecordError(ctx, fmt.Errorf("received bad response: %s", purchase.Error)) - } return &purchase, nil } diff --git a/account/subscription_test.go b/account/subscription_test.go index 5a4d4110..8d37f5a2 100644 --- a/account/subscription_test.go +++ b/account/subscription_test.go @@ -2,12 +2,17 @@ package account import ( "context" + "net/http" "testing" + "time" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/getlantern/radiance/account/protos" "github.com/getlantern/radiance/common" + "github.com/getlantern/radiance/common/settings" + "github.com/getlantern/radiance/events" ) func TestSubscriptionPaymentRedirect(t *testing.T) { @@ -142,12 +147,36 @@ func TestRestoreSubscription(t *testing.T) { assert.Equal(t, "token", resp.ActualUserToken) }) - t.Run("google", func(t *testing.T) { + t.Run("google switches to the purchasing account", func(t *testing.T) { ac, _ := newTestClient(t) + ac.setData(&UserData{ + LegacyID: 1, + LegacyToken: "old-token", + Token: "old-jwt", + LegacyUserData: &protos.LoginResponse_UserData{UserId: 1, Email: "old@example.com", UserLevel: "pro"}, + }) + setOAuthFlags(t) + changes := make(chan struct{}, 4) + sub := events.Subscribe(func(UserChangeEvent) { changes <- struct{}{} }) + defer sub.Unsubscribe() + resp, err := ac.RestoreSubscription(context.Background(), GoogleService, map[string]string{"purchaseToken": "t"}) require.NoError(t, err) require.NotNil(t, resp) assert.Equal(t, "active", resp.Status) + + assert.Equal(t, int64(1234), settings.GetInt64(settings.UserIDKey)) + assert.Equal(t, "token", settings.GetString(settings.TokenKey)) + assert.Empty(t, settings.GetString(settings.JwtTokenKey)) + assert.Empty(t, settings.GetString(settings.EmailKey)) + assert.False(t, settings.IsPro()) + assert.False(t, settings.GetBool(settings.OAuthLoginKey)) + assert.Empty(t, settings.GetString(settings.OAuthProviderKey)) + select { + case <-changes: + case <-time.After(3 * time.Second): + t.Fatal("account switch not reported") + } }) t.Run("unsupported", func(t *testing.T) { @@ -181,3 +210,68 @@ func TestNewStripeSubscriptionOmitsEmptyCouponCode(t *testing.T) { assert.NotEmpty(t, resp) assert.False(t, ts.stripeSubscriptionHasCouponCode) } + +func TestVerifySwitchesToPurchaser(t *testing.T) { + a, _ := newTestClient(t) + a.setData(&UserData{LegacyID: 1, LegacyToken: "old-token"}) + require.NoError(t, a.storeSalt([]byte("previous-salt"))) + useProServer(t, a, serveJSON(VerifySubscriptionResponse{Status: "active", ActualUserID: 99, ActualUserToken: "purchaser"})) + + _, err := a.VerifySubscription(context.Background(), AppleService, map[string]string{"receipt": "r"}) + require.NoError(t, err) + + assert.Equal(t, int64(99), settings.GetInt64(settings.UserIDKey)) + assert.Equal(t, "purchaser", settings.GetString(settings.TokenKey)) + assert.Nil(t, a.getSaltCached(), "the previous account's salt must not carry over") + saved, err := readSalt(a.saltPath) + require.NoError(t, err) + assert.Nil(t, saved) +} + +func TestLatePurchaseResponseDoesNotSwitch(t *testing.T) { + a, _ := newTestClient(t) + a.setData(&UserData{LegacyID: 1, LegacyToken: "a"}) + handler, requested, release := heldJSON(VerifySubscriptionResponse{Status: "active", ActualUserID: 99, ActualUserToken: "purchaser"}) + useProServer(t, a, handler) + + errc := make(chan error, 1) + go func() { + _, err := a.VerifySubscription(context.Background(), AppleService, map[string]string{"receipt": "r"}) + errc <- err + }() + <-requested + a.setData(&UserData{LegacyID: 2, LegacyToken: "b"}) + close(release) + + assert.NoError(t, <-errc, "the purchase itself succeeded") + assert.Equal(t, int64(2), settings.GetInt64(settings.UserIDKey)) + assert.Equal(t, "b", settings.GetString(settings.TokenKey)) +} + +func TestSameAccountPurchaseKeepsSaltAndOAuth(t *testing.T) { + a, _ := newTestClient(t) + a.setData(&UserData{LegacyID: 99, LegacyToken: "old-token"}) + require.NoError(t, a.storeSalt([]byte("my-salt"))) + setOAuthFlags(t) + useProServer(t, a, serveJSON(VerifySubscriptionResponse{Status: "active", ActualUserID: 99, ActualUserToken: "new-token"})) + + _, err := a.VerifySubscription(context.Background(), AppleService, map[string]string{"receipt": "r"}) + require.NoError(t, err) + + assert.Equal(t, "new-token", settings.GetString(settings.TokenKey)) + assert.Equal(t, []byte("my-salt"), a.getSaltCached()) + saved, err := readSalt(a.saltPath) + require.NoError(t, err) + assert.Equal(t, []byte("my-salt"), saved) + assert.True(t, settings.GetBool(settings.OAuthLoginKey)) + assert.Equal(t, "google", settings.GetString(settings.OAuthProviderKey)) +} + +func TestVerifySucceedsWithAnUnexpectedBody(t *testing.T) { + a, _ := newTestClient(t) + useProServer(t, a, http.HandlerFunc(func(http.ResponseWriter, *http.Request) {})) + + _, err := a.VerifySubscription(context.Background(), AppleService, map[string]string{"receipt": "r"}) + + assert.NoError(t, err) +} diff --git a/account/user.go b/account/user.go index 573be2a9..8ac6e2a8 100644 --- a/account/user.go +++ b/account/user.go @@ -6,6 +6,7 @@ import ( "errors" "fmt" "io/fs" + "maps" "net/url" "strings" @@ -36,6 +37,7 @@ func (a *Client) NewUser(ctx context.Context) (*UserData, error) { ctx, span := otel.Tracer(tracerName).Start(ctx, "new_user") defer span.End() + generation := a.identityGeneration() resp, err := a.sendProRequest(ctx, "POST", "/user-create", nil, nil, nil) if err != nil { a.logger.Error("creating new user", "error", err) @@ -45,7 +47,7 @@ func (a *Client) NewUser(ctx context.Context) (*UserData, error) { if err := json.Unmarshal(resp, &userResp); err != nil { return nil, traces.RecordError(ctx, fmt.Errorf("error unmarshalling new user response: %w", err)) } - userData, err := a.storeData(ctx, userResp) + userData, err := a.storeData(ctx, userResp, generation) if err != nil { return nil, err } @@ -61,6 +63,7 @@ func (a *Client) FetchUserData(ctx context.Context) (*UserData, error) { // fetchUserData fetches the user's account data and stores it. func (a *Client) fetchUserData(ctx context.Context) (*UserData, error) { + generation := a.identityGeneration() resp, err := a.sendProRequest(ctx, "GET", "/user-data", nil, nil, nil) if err != nil { a.logger.Error("user data", "error", err) @@ -70,15 +73,16 @@ func (a *Client) fetchUserData(ctx context.Context) (*UserData, error) { if err := json.Unmarshal(resp, &userResp); err != nil { return nil, traces.RecordError(ctx, fmt.Errorf("error unmarshalling new user response: %w", err)) } - return a.storeData(ctx, userResp) + return a.storeData(ctx, userResp, generation) } -func (a *Client) storeData(ctx context.Context, resp UserDataResponse) (*UserData, error) { - if resp.BaseResponse != nil && resp.Error != "" { - err := fmt.Errorf("received bad response: %s", resp.Error) - a.logger.Error("user data", "error", err) - return nil, traces.RecordError(ctx, err) - } +// ErrAccountChanged is returned when the account may have changed while the request was in flight; +// the response was discarded, and a retry acts on the current account. +var ErrAccountChanged = errors.New("account changed during request") + +// storeData stores resp unless the account changed after generation was captured, in which case it +// returns [ErrAccountChanged]. +func (a *Client) storeData(ctx context.Context, resp UserDataResponse, generation uint64) (*UserData, error) { if resp.LoginResponse_UserData == nil { a.logger.Error("user data", "error", "no user data in response") return nil, traces.RecordError(ctx, fmt.Errorf("no user data in response")) @@ -86,8 +90,14 @@ func (a *Client) storeData(ctx context.Context, resp UserDataResponse) (*UserDat resp.DeviceID = settings.GetString(settings.DeviceIDKey) a.mu.Lock() defer a.mu.Unlock() - - if resp.Token == "" && resp.UserId != 0 && resp.UserId == settings.GetInt64(settings.UserIDKey) { + if a.generation != generation { + return nil, traces.RecordError(ctx, ErrAccountChanged) + } + sameUser := resp.UserId != 0 && resp.UserId == settings.GetInt64(settings.UserIDKey) + if resp.UserId != 0 && !sameUser { + a.generation++ + } + if resp.Token == "" && sameUser { resp.Token = settings.GetString(settings.TokenKey) } login := &UserData{ @@ -182,13 +192,13 @@ func (a *Client) SignUp(ctx context.Context, email, password string) ([]byte, *p if err := proto.Unmarshal(resp, &signupData); err != nil { return nil, nil, traces.RecordError(ctx, fmt.Errorf("error unmarshalling sign up response: %w", err)) } - if err := a.storeIdentity(signupData.LegacyID, signupData.ProToken); err != nil { + if err := a.storeIdentity(signupData.LegacyID, signupData.ProToken, settings.Settings{ + settings.JwtTokenKey: signupData.Token, + settings.OAuthLoginKey: false, + settings.OAuthProviderKey: "", + }); err != nil { return nil, nil, traces.RecordError(ctx, fmt.Errorf("saving signup identity: %w", err)) } - jwtTokenErr := settings.Set(settings.JwtTokenKey, signupData.Token) - if jwtTokenErr != nil { - return nil, nil, traces.RecordError(ctx, fmt.Errorf("could not save JWT token: %w", jwtTokenErr)) - } return salt, &signupData, nil } @@ -285,12 +295,12 @@ func (a *Client) Login(ctx context.Context, email, password string) (*UserData, // regardless of state we need to save login information // We have device flow limit on login a.setData(&loginResp) - a.setSalt(salt) - if saltErr := writeSalt(salt, a.saltPath); saltErr != nil { + if saltErr := a.storeSalt(salt); saltErr != nil { return nil, traces.RecordError(ctx, saltErr) } - settings.Set(settings.OAuthLoginKey, false) - settings.Set(settings.OAuthProviderKey, "") + if err := settings.Clear(settings.OAuthLoginKey, settings.OAuthProviderKey); err != nil { + a.logger.Warn("failed to clear OAuth login", "error", err) + } return &loginResp, nil } @@ -315,10 +325,7 @@ func (a *Client) Logout(ctx context.Context, email string) (*UserData, error) { return nil, traces.RecordError(ctx, fmt.Errorf("logging out: %w", err)) } a.ClearUser() - a.setSalt(nil) - settings.Set(settings.OAuthLoginKey, false) - settings.Set(settings.OAuthProviderKey, "") - if err := writeSalt(nil, a.saltPath); err != nil { + if err := a.storeSalt(nil); err != nil { return nil, traces.RecordError(ctx, fmt.Errorf("writing salt after logout: %w", err)) } return a.NewUser(ctx) @@ -362,8 +369,11 @@ func (a *Client) CompleteRecoveryByEmail(ctx context.Context, email, newPassword if err != nil { return traces.RecordError(ctx, fmt.Errorf("failed to complete recovery by email: %w", err)) } - if err = writeSalt(newSalt, a.saltPath); err != nil { - return traces.RecordError(ctx, fmt.Errorf("failed to write new salt: %w", err)) + // The salt cache isn't keyed by email, so only the current account's new salt belongs in it. + if strings.EqualFold(settings.GetString(settings.EmailKey), lowerCaseEmail) { + if err = a.storeSalt(newSalt); err != nil { + return traces.RecordError(ctx, fmt.Errorf("failed to write new salt: %w", err)) + } } return nil } @@ -461,14 +471,12 @@ func (a *Client) CompleteChangeEmail(ctx context.Context, newEmail, password, co if err != nil { return traces.RecordError(ctx, err) } - if err := writeSalt(newSalt, a.saltPath); err != nil { + if err := a.storeSalt(newSalt); err != nil { return traces.RecordError(ctx, err) } if err := settings.Set(settings.EmailKey, newEmail); err != nil { return traces.RecordError(ctx, err) } - - a.setSalt(newSalt) return nil } @@ -506,8 +514,7 @@ func (a *Client) DeleteAccount(ctx context.Context, email, password string) (*Us } a.ClearUser() - a.setSalt(nil) - if err := writeSalt(nil, a.saltPath); err != nil { + if err := a.storeSalt(nil); err != nil { return nil, traces.RecordError(ctx, fmt.Errorf("failed to write salt during account deletion cleanup: %w", err)) } @@ -561,11 +568,13 @@ func (a *Client) OAuthLoginCallback(ctx context.Context, oAuthToken string) (*Us return nil, fmt.Errorf("error getting user data: %w", err) } - if err := settings.Set(settings.JwtTokenKey, oAuthToken); err != nil { + if err := settings.Patch(settings.Settings{ + settings.JwtTokenKey: oAuthToken, + settings.OAuthLoginKey: true, + }); err != nil { a.logger.Error("Failed to persist JWT token", "error", err) return nil, fmt.Errorf("failed to persist JWT token: %w", err) } - settings.Set(settings.OAuthLoginKey, true) user.Id = jwtUserInfo.Email user.EmailConfirmed = true a.setData(user) @@ -583,7 +592,7 @@ func (a *Client) OAuthDeviceLimitCallback(ctx context.Context, oAuthToken string if jwtUserInfo.LegacyUserID == 0 || jwtUserInfo.LegacyToken == "" { return fmt.Errorf("%w: device-limit token is missing the account identity", ErrInvalidToken) } - return a.storeIdentity(jwtUserInfo.LegacyUserID, jwtUserInfo.LegacyToken) + return a.storeIdentity(jwtUserInfo.LegacyUserID, jwtUserInfo.LegacyToken, nil) } // ErrInvalidToken distinguishes an unusable OAuth callback token from a @@ -611,9 +620,6 @@ func (a *Client) RemoveDevice(ctx context.Context, deviceID string) (*LinkRespon if err := json.Unmarshal(resp, &link); err != nil { return nil, traces.RecordError(ctx, fmt.Errorf("error unmarshalling remove device response: %w", err)) } - if link.BaseResponse != nil && link.BaseResponse.Error != "" { - return nil, traces.RecordError(ctx, fmt.Errorf("failed to remove device: %s", link.BaseResponse.Error)) - } return &link, nil } @@ -648,12 +654,9 @@ func (a *Client) referralAttachV1(ctx context.Context, code string) (*ReferralAt return nil, traces.RecordError(ctx, err) } var baseResp protos.BaseResponse - if err := proto.Unmarshal(resp, &baseResp); err != nil { + if err := json.Unmarshal(resp, &baseResp); err != nil { return nil, traces.RecordError(ctx, fmt.Errorf("error unmarshalling referral attach response: %w", err)) } - if baseResp.Error != "" { - return nil, traces.RecordError(ctx, errors.New(baseResp.Error)) - } return &ReferralAttachResponse{ BaseResponse: &baseResp, }, nil @@ -677,9 +680,6 @@ func (a *Client) referralAttachV2(ctx context.Context, code, channel string) (*R if err := json.Unmarshal(resp, &referral); err != nil { return nil, traces.RecordError(ctx, fmt.Errorf("error unmarshalling referral attach v2 response: %w", err)) } - if referral.BaseResponse != nil && referral.Error != "" { - return nil, traces.RecordError(ctx, errors.New(referral.Error)) - } return &referral, nil } @@ -688,23 +688,31 @@ type UserChangeEvent struct { events.Event } -func (a *Client) storeIdentity(id int64, token string) error { +func (a *Client) storeIdentity(id int64, token string, extra settings.Settings) error { a.mu.Lock() defer a.mu.Unlock() - changed, err := a.storeIdentityLocked(id, token) - if changed { - events.Emit(UserChangeEvent{}) - } - return err -} - -// storeIdentityLocked requires a.mu to be held. -func (a *Client) storeIdentityLocked(id int64, token string) (bool, error) { - previousID := settings.GetInt64(settings.UserIDKey) - previousToken := settings.GetString(settings.TokenKey) - updates := settings.Settings{} + a.generation++ + return a.storeIdentityLocked(id, token, extra) +} + +// storeIdentityLocked saves id and token, each only if set, together with extra in one write, and +// emits a [UserChangeEvent] if the [accountState] changed. A different nonzero id clears the +// previous account's credentials and cached data, and the whole write then applies in memory even +// if the save fails, so the previous credentials never outlive the switch; otherwise it applies +// only once saved. The OAuth flags are kept, because an OAuth sign-in records its provider before +// switching. It requires a.mu to be held. +func (a *Client) storeIdentityLocked(id int64, token string, extra settings.Settings) error { + before := currentAccount() + defer func() { + if currentAccount() != before { + events.Emit(UserChangeEvent{}) + } + }() // The cached response can lag behind signup or OAuth, so compare the active identity. - if id != 0 && id != previousID { + switching := id != 0 && id != settings.GetInt64(settings.UserIDKey) + updates := settings.Settings{} + if switching { + // Overwrite rather than delete, so unlocked readers never see the account without an id. updates[settings.TokenKey] = "" updates[settings.JwtTokenKey] = "" updates[settings.EmailKey] = "" @@ -712,18 +720,37 @@ func (a *Client) storeIdentityLocked(id int64, token string) (bool, error) { updates[settings.DevicesKey] = []settings.Device{} updates[settings.UserDataKey] = nil } + maps.Copy(updates, extra) if id != 0 { updates[settings.UserIDKey] = id } if token != "" { updates[settings.TokenKey] = token } - if len(updates) == 0 { - return false, nil + switch { + case len(updates) == 0: + return nil + case switching: + return settings.Apply(updates) + default: + return settings.Patch(updates) + } +} + +// accountState holds the settings whose change is reported as a [UserChangeEvent]. +type accountState struct { + id int64 + token, level, email, jwt string +} + +func currentAccount() accountState { + return accountState{ + id: settings.GetInt64(settings.UserIDKey), + token: settings.GetString(settings.TokenKey), + level: settings.GetString(settings.UserLevelKey), + email: settings.GetString(settings.EmailKey), + jwt: settings.GetString(settings.JwtTokenKey), } - err := settings.Patch(updates) - changed := settings.GetInt64(settings.UserIDKey) != previousID || settings.GetString(settings.TokenKey) != previousToken - return changed, err } func (a *Client) setData(data *UserData) { @@ -733,47 +760,18 @@ func (a *Client) setData(data *UserData) { } a.mu.Lock() defer a.mu.Unlock() + a.generation++ a.setDataLocked(data, false) } -// setDataLocked requires a.mu to be held and data to be non-nil. +// setDataLocked stores a copy of data, merged with the cached response of the same account when +// preserveLoginFields is set. Data without user data stores only its id, token and devices. It +// logs rather than returns save failures, and returns data, possibly merged, which the caller may +// modify without affecting settings. It requires a.mu to be held and data to be non-nil. func (a *Client) setDataLocked(data *UserData, preserveLoginFields bool) *UserData { - changed, err := a.storeIdentityLocked(data.LegacyID, data.LegacyToken) - if err != nil { - a.logger.Error("failed to store account identity", "error", err) - } - defer func() { - if changed { - events.Emit(UserChangeEvent{}) - } - }() - if data.LegacyUserData == nil { - a.logger.Info("no user data to set, storing id and token only") - return data - } - - if data.LegacyUserData.UserLevel != "" { - oldUserLevel := settings.GetString(settings.UserLevelKey) - changed = changed || oldUserLevel != data.LegacyUserData.UserLevel - if err := settings.Set(settings.UserLevelKey, data.LegacyUserData.UserLevel); err != nil { - a.logger.Error("failed to set user level in settings", "error", err) - } - } - if data.LegacyUserData.Email != "" { - oldEmail := settings.GetString(settings.EmailKey) - changed = changed || oldEmail != data.LegacyUserData.Email - if err := settings.Set(settings.EmailKey, data.LegacyUserData.Email); err != nil { - a.logger.Error("failed to set email in settings", "error", err) - } - } - if data.Token != "" { - oldJwtToken := settings.GetString(settings.JwtTokenKey) - changed = changed || oldJwtToken != data.Token - if err := settings.Set(settings.JwtTokenKey, data.Token); err != nil { - a.logger.Error("failed to set JWT token in settings", "error", err) - } - } - + updates := settings.Settings{} + // A device-limited login carries no user data, but its device list is what the user needs to + // pick a device to remove. if len(data.Devices) > 0 { devices := []settings.Device{} for _, d := range data.Devices { @@ -782,26 +780,36 @@ func (a *Client) setDataLocked(data *UserData, preserveLoginFields bool) *UserDa ID: d.Id, }) } - if err := settings.Set(settings.DevicesKey, devices); err != nil { - a.logger.Error("failed to set devices in settings", "error", err) - } + updates[settings.DevicesKey] = devices } - - if preserveLoginFields { - // Merge only the cached response so old login fields cannot overwrite newer settings. - cached := new(UserData) - if err := settings.GetStruct(settings.UserDataKey, cached); err != nil { - a.logger.Warn("ignoring invalid cached user data", "error", err) - } else if data.LegacyID != 0 && cached.LegacyID == data.LegacyID { - cached.LegacyToken = data.LegacyToken - cached.LegacyUserData = data.LegacyUserData - data = cached + if data.LegacyUserData == nil { + a.logger.Info("no user data to set, storing id, token and devices only") + } else { + if data.LegacyUserData.UserLevel != "" { + updates[settings.UserLevelKey] = data.LegacyUserData.UserLevel + } + if data.LegacyUserData.Email != "" { + updates[settings.EmailKey] = data.LegacyUserData.Email + } + if data.Token != "" { + updates[settings.JwtTokenKey] = data.Token } + if preserveLoginFields { + // Merge only the cached response so old login fields cannot overwrite newer settings. + cached := new(UserData) + if err := settings.GetStruct(settings.UserDataKey, cached); err != nil { + a.logger.Warn("ignoring invalid cached user data", "error", err) + } else if data.LegacyID != 0 && cached.LegacyID == data.LegacyID { + cached.LegacyToken = data.LegacyToken + cached.LegacyUserData = data.LegacyUserData + data = cached + } + } + updates[settings.UserDataKey] = proto.Clone(data) } - if err := settings.Set(settings.UserDataKey, data); err != nil { - a.logger.Error("failed to set login response in settings", "error", err) + if err := a.storeIdentityLocked(data.LegacyID, data.LegacyToken, updates); err != nil { + a.logger.Error("failed to store account data", "error", err) } - return data } @@ -809,17 +817,9 @@ func (a *Client) setDataLocked(data *UserData, preserveLoginFields bool) *UserDa func (a *Client) ClearUser() { a.mu.Lock() defer a.mu.Unlock() + a.generation++ hadIdentity := settings.GetInt64(settings.UserIDKey) != 0 || settings.GetString(settings.TokenKey) != "" - err := settings.Clear( - settings.UserIDKey, - settings.TokenKey, - settings.UserLevelKey, - settings.EmailKey, - settings.DevicesKey, - settings.JwtTokenKey, - settings.UserDataKey, - ) - if err != nil { + if err := settings.Clear(settings.UserKeys...); err != nil { a.logger.Warn("failed to clear user info", "error", err) } if hadIdentity { diff --git a/account/user_refresh_test.go b/account/user_refresh_test.go index 69705f0e..602e082f 100644 --- a/account/user_refresh_test.go +++ b/account/user_refresh_test.go @@ -269,3 +269,107 @@ func TestFetchUserDataDoesNotReplayLoginSettings(t *testing.T) { require.NoError(t, err) assert.Equal(t, devices, storedDevices) } + +func TestStaleUserDataIsDiscarded(t *testing.T) { + for _, tt := range []struct { + name string + change func(a *Client) + }{ + {"A to B", func(a *Client) { + a.setData(&UserData{LegacyID: 2, LegacyToken: "b"}) + }}, + {"A to B to A", func(a *Client) { + a.setData(&UserData{LegacyID: 2, LegacyToken: "b"}) + a.setData(&UserData{LegacyID: 1, LegacyToken: "a"}) + }}, + {"logout", func(a *Client) { a.ClearUser() }}, + } { + t.Run(tt.name, func(t *testing.T) { + a, _ := newTestClient(t) + a.setData(&UserData{LegacyID: 1, LegacyToken: "a"}) + handler, requested, release := heldJSON(UserDataResponse{ + LoginResponse_UserData: &protos.LoginResponse_UserData{UserId: 1, Token: "a", UserLevel: "pro"}, + }) + useProServer(t, a, handler) + + errc := make(chan error, 1) + go func() { + _, err := a.FetchUserData(context.Background()) + errc <- err + }() + <-requested + tt.change(a) + after := currentAccount() + close(release) + + assert.ErrorIs(t, <-errc, ErrAccountChanged) + assert.Equal(t, after, currentAccount(), "a stale response must not change the account") + assert.False(t, settings.IsPro()) + }) + } +} + +func TestNewUserDiscardsInFlightUserData(t *testing.T) { + a, _ := newTestClient(t) + a.setData(&UserData{LegacyID: 1, LegacyToken: "a"}) + userData, requested, release := heldJSON(UserDataResponse{ + LoginResponse_UserData: &protos.LoginResponse_UserData{UserId: 1, Token: "a"}, + }) + mux := http.NewServeMux() + mux.Handle("/user-data", userData) + mux.Handle("/user-create", serveJSON(UserDataResponse{ + LoginResponse_UserData: &protos.LoginResponse_UserData{UserId: 2, Token: "b"}, + })) + useProServer(t, a, mux) + + errc := make(chan error, 1) + go func() { + _, err := a.FetchUserData(context.Background()) + errc <- err + }() + <-requested + _, err := a.NewUser(context.Background()) + require.NoError(t, err) + close(release) + + assert.ErrorIs(t, <-errc, ErrAccountChanged) + assert.Equal(t, int64(2), settings.GetInt64(settings.UserIDKey)) +} + +func TestReturnedUserDataIsACopy(t *testing.T) { + a, _ := newTestClient(t) + got, err := a.FetchUserData(context.Background()) + require.NoError(t, err) + + got.LegacyUserData.UserLevel = "pro" + got.Id = "mutated" + + var stored UserData + require.NoError(t, settings.GetStruct(settings.UserDataKey, &stored)) + assert.Empty(t, stored.Id) + assert.Empty(t, stored.LegacyUserData.UserLevel) +} + +func TestAccountSwitchNeverExposesAnEmptyIdentity(t *testing.T) { + a, _ := newTestClient(t) + a.setData(&UserData{LegacyID: 1, LegacyToken: "a"}) + stop := make(chan struct{}) + sawEmpty := make(chan bool, 1) + go func() { + empty := false + for { + select { + case <-stop: + sawEmpty <- empty + return + default: + empty = empty || settings.GetInt64(settings.UserIDKey) == 0 || settings.GetString(settings.TokenKey) == "" + } + } + }() + for i := range 50 { + a.setData(&UserData{LegacyID: int64(2 + i%2), LegacyToken: "t"}) + } + close(stop) + assert.False(t, <-sawEmpty, "readers must never see an account without an id or token") +} diff --git a/account/user_test.go b/account/user_test.go index eb0a3afc..691b273b 100644 --- a/account/user_test.go +++ b/account/user_test.go @@ -11,6 +11,7 @@ import ( "math/big" "net/http" "net/http/httptest" + "os" "path/filepath" "testing" @@ -195,7 +196,7 @@ func newTestServer(t *testing.T) (*httptest.Server, *testServer) { } json.NewDecoder(r.Body).Decode(&body) state.referralAttachV1Code = body.Code - writeProtoResponse(w, &protos.BaseResponse{Error: state.referralAttachV1Error}) + writeJSONResponse(w, &protos.BaseResponse{Error: state.referralAttachV1Error}) }) mux.HandleFunc("/referral-attach-v2", func(w http.ResponseWriter, r *http.Request) { @@ -288,7 +289,12 @@ func newTestServer(t *testing.T) (*httptest.Server, *testServer) { func newTestClient(t *testing.T) (*Client, *testServer) { ts, state := newTestServer(t) - settings.InitSettings(t.TempDir()) + // Legacy settings discovery reads these roots; empty ones keep the host's own files out. + for _, v := range []string{"HOME", "XDG_CONFIG_HOME", "APPDATA", "PUBLIC"} { + t.Setenv(v, t.TempDir()) + } + settings.Reset() + require.NoError(t, settings.InitSettings(t.TempDir())) t.Cleanup(settings.Reset) return &Client{ httpClient: ts.Client(), @@ -320,6 +326,36 @@ func newTestClientWithSRP(t *testing.T, email, password string) (*Client, *testS return ac, state } +// useProServer points a's pro-server requests at handler for the rest of the test. +func useProServer(t *testing.T, a *Client, handler http.Handler) { + t.Helper() + server := httptest.NewServer(handler) + t.Cleanup(server.Close) + a.proURL = server.URL +} + +func serveJSON(v any) http.HandlerFunc { + return func(w http.ResponseWriter, r *http.Request) { writeJSONResponse(w, v) } +} + +// heldJSON writes v only once release is closed, closing requested when the request arrives. +func heldJSON(v any) (handler http.HandlerFunc, requested, release chan struct{}) { + requested, release = make(chan struct{}), make(chan struct{}) + return func(w http.ResponseWriter, r *http.Request) { + close(requested) + <-release + writeJSONResponse(w, v) + }, requested, release +} + +func setOAuthFlags(t *testing.T) { + t.Helper() + require.NoError(t, settings.Patch(settings.Settings{ + settings.OAuthLoginKey: true, + settings.OAuthProviderKey: "google", + })) +} + func TestSignUp(t *testing.T) { ac, _ := newTestClient(t) settings.Set(settings.TokenKey, "test-token") @@ -352,6 +388,43 @@ func TestLogin(t *testing.T) { assert.NoError(t, err) } +func TestDeviceLimitedLoginStoresDevices(t *testing.T) { + for _, tt := range []struct { + name string + existing *UserData + }{ + {name: "fresh identity"}, + {name: "existing identity", existing: &UserData{ + LegacyID: 7, + LegacyToken: "tok", + Devices: []*protos.LoginResponse_Device{{Id: "stale", Name: "Old"}}, + LegacyUserData: &protos.LoginResponse_UserData{UserId: 7, UserLevel: "free"}, + }}, + } { + t.Run(tt.name, func(t *testing.T) { + a, server := newTestClientWithSRP(t, "test@example.com", "password") + if tt.existing != nil { + a.setData(tt.existing) + } + userDataBefore := settings.Exists(settings.UserDataKey) + server.loginResponse = &UserData{ + Devices: []*protos.LoginResponse_Device{ + {Id: "phone", Name: "Phone"}, + {Id: "laptop", Name: "Laptop"}, + }, + } + + _, err := a.Login(context.Background(), "test@example.com", "password") + require.NoError(t, err) + + devices, err := settings.Devices() + require.NoError(t, err) + assert.Equal(t, []settings.Device{{ID: "phone", Name: "Phone"}, {ID: "laptop", Name: "Laptop"}}, devices) + assert.Equal(t, userDataBefore, settings.Exists(settings.UserDataKey), + "a device-limited login is not a successful login") + }) + } +} func TestLogout(t *testing.T) { ac, _ := newTestClient(t) settings.Set(settings.DeviceIDKey, "deviceId") @@ -392,6 +465,50 @@ func TestCompleteRecoveryByEmail(t *testing.T) { assert.NoError(t, err) } +func TestRecoverySalt(t *testing.T) { + completeRecovery := func(t *testing.T, a *Client, email string) error { + t.Helper() + return a.CompleteRecoveryByEmail(context.Background(), email, "new-password", "code") + } + + t.Run("current account caches and saves the new salt", func(t *testing.T) { + a, _ := newTestClient(t) + require.NoError(t, settings.Set(settings.EmailKey, "me@example.com")) + a.salt = []byte("old-salt") + + require.NoError(t, completeRecovery(t, a, "Me@example.com")) + + saved, err := readSalt(a.saltPath) + require.NoError(t, err) + assert.NotEqual(t, []byte("old-salt"), a.getSaltCached()) + assert.Equal(t, a.getSaltCached(), saved) + }) + + t.Run("a failed save still replaces the cached salt", func(t *testing.T) { + a, _ := newTestClient(t) + require.NoError(t, settings.Set(settings.EmailKey, "me@example.com")) + a.salt = []byte("old-salt") + a.saltPath = filepath.Join(t.TempDir(), "missing", saltFileName) + + assert.Error(t, completeRecovery(t, a, "me@example.com")) + assert.NotEqual(t, []byte("old-salt"), a.getSaltCached()) + }) + + t.Run("another account's salt is not stored", func(t *testing.T) { + a, _ := newTestClient(t) + require.NoError(t, settings.Set(settings.EmailKey, "me@example.com")) + a.salt = []byte("my-salt") + require.NoError(t, writeSalt([]byte("my-salt"), a.saltPath)) + + require.NoError(t, completeRecovery(t, a, "other@example.com")) + + assert.Equal(t, []byte("my-salt"), a.getSaltCached()) + saved, err := os.ReadFile(a.saltPath) + require.NoError(t, err) + assert.Equal(t, []byte("my-salt"), saved) + }) +} + func TestValidateEmailRecoveryCode(t *testing.T) { ac, _ := newTestClient(t) err := ac.ValidateEmailRecoveryCode(context.Background(), "test@example.com", "code") @@ -553,3 +670,23 @@ func TestReferralAttach_V2Error(t *testing.T) { assert.Nil(t, resp) assert.Contains(t, err.Error(), "invalid code") } + +func TestOAuthFlagsCleared(t *testing.T) { + t.Run("ClearUser", func(t *testing.T) { + a, _ := newTestClient(t) + setOAuthFlags(t) + a.ClearUser() + assert.False(t, settings.GetBool(settings.OAuthLoginKey)) + assert.Empty(t, settings.GetString(settings.OAuthProviderKey)) + }) + + t.Run("SignUp", func(t *testing.T) { + a, _ := newTestClient(t) + require.NoError(t, settings.Patch(settings.Settings{settings.TokenKey: "t", settings.UserIDKey: "123"})) + setOAuthFlags(t) + _, _, err := a.SignUp(context.Background(), "new@example.com", "password") + require.NoError(t, err) + assert.False(t, settings.GetBool(settings.OAuthLoginKey)) + assert.Empty(t, settings.GetString(settings.OAuthProviderKey)) + }) +} diff --git a/backend/radiance.go b/backend/radiance.go index d19788fc..935a9704 100644 --- a/backend/radiance.go +++ b/backend/radiance.go @@ -183,7 +183,9 @@ func NewLocalBackend(ctx context.Context, opts Options) (*LocalBackend, error) { if opts.TimeZone != "" { startup[settings.TimeZoneKey] = opts.TimeZone } - settings.Patch(startup) + if err := settings.Apply(startup); err != nil { + slog.Warn("Failed to save startup settings", "error", err) + } accountClient := account.NewClient(kindling.HTTPClient(), dataDir, slog.Default().With("service", "account")) diff --git a/common/atomicfile/atomic_file.go b/common/atomicfile/atomic_file.go index 5aa23eae..ce4e68cc 100644 --- a/common/atomicfile/atomic_file.go +++ b/common/atomicfile/atomic_file.go @@ -36,10 +36,6 @@ func WriteFile(filename string, data []byte, perm os.FileMode) (err error) { if err = f.Close(); err != nil { return err } - // os.Rename will fail on Windows if the target file already exists so we remove it first. - if runtime.GOOS == "windows" { - _ = os.Remove(filename) - } return os.Rename(f.Name(), filename) } diff --git a/common/init.go b/common/init.go index e79fdb9b..29fc9508 100644 --- a/common/init.go +++ b/common/init.go @@ -80,13 +80,18 @@ func Init(dataDir, logDir, logLevel string) (err error) { return fmt.Errorf("failed to initialize settings: %w", err) } - settings.Set(settings.DataPathKey, data) - settings.Set(settings.LogPathKey, logs) + startup := settings.Settings{ + settings.DataPathKey: data, + settings.LogPathKey: logs, + } // env override wins; otherwise preserve any persisted value; otherwise seed from the arg. if v := env.GetString(env.LogLevel); v != "" { - settings.Set(settings.LogLevelKey, v) + startup[settings.LogLevelKey] = v } else if !settings.Exists(settings.LogLevelKey) { - settings.Set(settings.LogLevelKey, logLevel) + startup[settings.LogLevelKey] = logLevel + } + if err := settings.Apply(startup); err != nil { + slog.Warn("Failed to save startup settings", "error", err) } logger := log.NewLogger(log.Config{ @@ -191,14 +196,8 @@ func setupDirectories(data, logs string) (dataDir, logDir string, err error) { } else if logs == "" { logs = internal.DefaultLogPath() } - // Honor the caller's path as-is. A previous version of this function - // unconditionally appended /data and /logs suffixes here even when the - // caller passed a fully-resolved path (e.g. Android passes - // /.lantern). That broke upgrade continuity: v9.0.x had - // written settings.json under /, while v9.1.x reads from - // /data/, so every existing install lost its persisted - // user_id, device_id, jwt token, and user_level on upgrade — surfacing - // as "Pro is suddenly expired after the update." + // Use the caller's path as-is; earlier versions' settings live directly under it, so a + // suffix hides them from migration. data, _ = filepath.Abs(data) logs, _ = filepath.Abs(logs) for _, path := range []string{data, logs} { diff --git a/common/settings/legacy_crossdir.go b/common/settings/legacy_crossdir.go index ed2e7392..080603d9 100644 --- a/common/settings/legacy_crossdir.go +++ b/common/settings/legacy_crossdir.go @@ -2,9 +2,7 @@ package settings import ( "errors" - "fmt" "io/fs" - "log/slog" "os" "path/filepath" "runtime" @@ -57,34 +55,21 @@ func samePathWindows(a, b string) bool { // readWindowsCrossDirCandidates is split out so tests can drive the path // resolution directly without needing to spoof %PUBLIC% / GOOS. func readWindowsCrossDirCandidates(v90xDir string) []candidateSource { - // legacySettingsFileName is tried first because it's the actual - // v9.0.x name; settingsFileName is included as a defensive - // fallback for users whose v9.0.x file got renamed by a partial / - // failed earlier upgrade attempt. - specs := []struct { - name, label string - }{ - {legacySettingsFileName, fmt.Sprintf("v9.0.x Windows %s", filepath.Join(v90xDir, legacySettingsFileName))}, - {settingsFileName, fmt.Sprintf("v9.0.x Windows %s", filepath.Join(v90xDir, settingsFileName))}, + // settings.json is deliberately not read: in this directory it is the live UI's settings, not a + // v9.0.x artifact, so adopting it would import whatever the UI last wrote. + full := filepath.Join(v90xDir, legacySettingsFileName) + label := "v9.0.x Windows " + full + b, err := os.ReadFile(full) + if errors.Is(err, fs.ErrNotExist) { + return nil } - var out []candidateSource - for _, s := range specs { - full := filepath.Join(v90xDir, s.name) - b, err := os.ReadFile(full) - switch { - case err == nil: - out = append(out, candidateSource{ - path: full, - contents: b, - exists: true, - label: s.label, - }) - case errors.Is(err, fs.ErrNotExist): - // Expected — fresh install or that filename never existed for this user. - default: - slog.Warn("v9.0.x Windows cross-dir read failed", - "path", full, "error", err) - } + if err != nil { + return []candidateSource{{path: full, label: label, err: err}} } - return out + return []candidateSource{{ + path: full, + contents: b, + exists: true, + label: label, + }} } diff --git a/common/settings/legacy_yaml.go b/common/settings/legacy_yaml.go index 9e38b4a2..4a219e8f 100644 --- a/common/settings/legacy_yaml.go +++ b/common/settings/legacy_yaml.go @@ -26,13 +26,14 @@ func legacyYAMLCandidate(fileDir string) candidateSource { if path == "" { return candidateSource{} } + label := fmt.Sprintf("pre-9.x %s yaml", layout) raw, err := os.ReadFile(path) - if err != nil { - if !errors.Is(err, fs.ErrNotExist) { - slog.Warn("pre-9.x yaml read failed", "path", path, "error", err) - } + if errors.Is(err, fs.ErrNotExist) { return candidateSource{} } + if err != nil { + return candidateSource{path: path, label: label, err: err} + } translated, err := translateLegacyYAML(raw, layout) if err != nil { slog.Warn("pre-9.x yaml translate failed", "path", path, "error", err) @@ -42,7 +43,7 @@ func legacyYAMLCandidate(fileDir string) candidateSource { path: path, contents: translated, exists: true, - label: fmt.Sprintf("pre-9.x %s yaml", layout), + label: label, } } diff --git a/common/settings/legacy_yaml_test.go b/common/settings/legacy_yaml_test.go index 03a591a2..6430416a 100644 --- a/common/settings/legacy_yaml_test.go +++ b/common/settings/legacy_yaml_test.go @@ -11,8 +11,7 @@ import ( func TestTranslateLegacyYAML_Desktop(t *testing.T) { t.Run("pro user with all fields", func(t *testing.T) { // Shape mirrors what the pre-9.x desktop client (flashlight + - // lantern-client) wrote into ~/.lantern/settings.yaml / - // %APPDATA%\Lantern\settings.yaml / ~/.config/lantern/settings.yaml. + // lantern-client) wrote to the paths legacyYAMLPath resolves. yaml := []byte(`userID: 3580849 deviceID: 84e9c7b2-2a54-44f3-9ec6-276086017e49 userPro: true @@ -42,7 +41,7 @@ userToken: tok `) out, err := translateLegacyYAML(yaml, "desktop") require.NoError(t, err) - assert.Equal(t, "free", userLevelInJSON(out), + assert.Equal(t, "free", userLevelIn(t, out), "a known but non-pro user should translate to user_level=free") }) @@ -52,7 +51,7 @@ userToken: "" `) out, err := translateLegacyYAML(yaml, "desktop") require.NoError(t, err) - assert.Equal(t, "", userLevelInJSON(out), + assert.Equal(t, "", userLevelIn(t, out), "no user_id means we shouldn't claim 'free'; let next login decide") }) @@ -62,6 +61,15 @@ userToken: "" }) } +func userLevelIn(t *testing.T, contents []byte) string { + t.Helper() + var got struct { + UserLevel string `json:"user_level"` + } + require.NoError(t, json.Unmarshal(contents, &got)) + return got.UserLevel +} + func TestTranslateLegacyYAML_iOS(t *testing.T) { yaml := []byte(`AppName: lantern DeviceID: ios-device-9000 diff --git a/common/settings/settings.go b/common/settings/settings.go index e848ac7a..74999aa8 100644 --- a/common/settings/settings.go +++ b/common/settings/settings.go @@ -2,6 +2,7 @@ package settings import ( + "bytes" jsonpkg "encoding/json" "errors" "fmt" @@ -10,6 +11,7 @@ import ( "maps" "os" "path/filepath" + "reflect" "strconv" "strings" "sync" @@ -71,15 +73,15 @@ const ( PreferredLocationKey _key = "preferred_location" // [common.PreferredLocation] settingsFileName = "settings.json" - // legacySettingsFileName is what v9.0.x called the same file (it was - // renamed). On upgrade from v9.0.x, the user's - // persisted user_id / token / user_level live at /local.json; - // migrateLegacySettingsIfNeeded reads it from there so Pro state - // survives the rename. + // legacySettingsFileName is v9.0.x's name for the settings file. legacySettingsFileName = "local.json" settingsInvalidFileName = "settings.invalid.json" + // migrationMarkerFileName lives beside settings.json rather than inside it, so losing the + // settings file doesn't re-run a migration that already adopted an identity. + migrationMarkerFileName = ".settings-migrated" + // Development only keys ConfigFetchDisabledKey _key = "config_fetch_disabled" // bool FeatureOverridesKey _key = "feature_overrides" // string @@ -103,16 +105,18 @@ type settings struct { } var k = &settings{ - k: koanf.New("."), + k: newDefaults(), } -func init() { - // set default values. - k.k.Set(LocaleKey.String(), "fa-IR") - k.k.Set(UserLevelKey.String(), "free") +func newDefaults() *koanf.Koanf { + d := koanf.New(".") + d.Set(LocaleKey.String(), "fa-IR") + d.Set(UserLevelKey.String(), "free") + return d } -// InitSettings initializes the config for user settings. +// InitSettings initializes the config for user settings. Until it succeeds, changes are kept in +// memory only. func InitSettings(fileDir string) error { k.mu.Lock() defer k.mu.Unlock() @@ -123,28 +127,64 @@ func InitSettings(fileDir string) error { return fmt.Errorf("failed to create data directory: %v", err) } k.filePath = filepath.Join(fileDir, settingsFileName) - migrateLegacySettingsIfNeeded(fileDir, k.filePath) + defer func() { + if !k.initialized { + k.filePath = "" + } + }() + markerPath := filepath.Join(fileDir, migrationMarkerFileName) + migrate := migrationPending(markerPath) + var identity map[string]any + if migrate { + identity, migrate = legacyIdentity(fileDir, k.filePath) + } + fresh := false switch err := loadSettings(k.filePath); { case errors.Is(err, fs.ErrNotExist): slog.Warn("settings file not found", "path", k.filePath) // file may not have been created yet - if err := save(); err != nil { - return err - } + fresh = true case errors.Is(err, errParseSettings): - // An invalid settings file must not be fatal: quarantine it, then start - // from in-memory defaults and overwrite the bad file with them. + // An invalid settings file must not be fatal; quarantine it before it's overwritten. slog.Error("Settings file is invalid; starting from defaults", "path", k.filePath, "error", err) quarantineInvalidSettings(k.filePath) - if err := save(); err != nil { - return err - } + fresh = true case err != nil: return fmt.Errorf("loading settings: %w", err) } + switch { + case identity != nil: + err := updateLocked(func(kk *koanf.Koanf) error { return adoptIdentity(kk, identity) }) + if err != nil && fresh { + return err + } + if err != nil { + slog.Warn("legacy settings migration: saving adopted identity failed; will retry", "error", err) + migrate = false + } + case fresh: + if err := saveLocked(k.k); err != nil { + return err + } + } + if migrate { + if err := atomicfile.WriteFile(markerPath, nil, fileperm.File); err != nil { + slog.Warn("legacy settings migration: writing marker failed; will retry", "path", markerPath, "error", err) + } + } k.initialized = true return nil } +// migrationPending reports whether the legacy migration has yet to complete. An unreadable marker +// counts as complete, because re-adopting a legacy identity can undo a logout. +func migrationPending(markerPath string) bool { + _, err := os.Stat(markerPath) + if err != nil && !errors.Is(err, fs.ErrNotExist) { + slog.Warn("legacy settings migration: marker unreadable; skipping", "path", markerPath, "error", err) + } + return errors.Is(err, fs.ErrNotExist) +} + // candidateSource is one possible location of persisted user state. // contents is always canonical JSON — direct for v9.x, translated for // pre-9.x YAML. @@ -153,25 +193,28 @@ type candidateSource struct { contents []byte exists bool label string + // err is the read error, other than fs.ErrNotExist; exists is false when it is set. + err error } -// migrateLegacySettingsIfNeeded recovers persisted user state written -// by older client versions. Candidates in priority order: +// legacyIdentity returns the identity keys to adopt from settings written by older client +// versions, or nil when no legacy candidate outranks the canonical file. ok is false when any +// candidate can't be read, so the migration can't be judged yet. +// +// Candidates in priority order: // // 1. /settings.json — canonical // 2. /local.json — v9.0.x (renamed) -// 3. Windows ${PUBLIC}\Lantern\data\* — v9.0.x cross-dir; -// spliced in below, Windows only -// 4. pre-9.x platform-specific YAML; spliced in below +// 3. ${PUBLIC}\Lantern\data\local.json — v9.0.x cross-dir, Windows only +// 4. pre-9.x platform-specific YAML // 5. /data/settings.json — v9.1.x (bugged: // setupDirectories appended an // unconditional "/data" suffix) // -// Pick the highest-priority candidate with user_level=="pro"; if none -// is pro, the highest-priority candidate that exists. Losing Pro is -// recoverable; losing the device registration creates server-side -// orphans, so identifier continuity wins ties. -func migrateLegacySettingsIfNeeded(fileDir, canonicalPath string) { +// A candidate counts only if it holds a JSON object. Pick the highest-priority candidate with +// user_level=="pro"; if none is pro, the highest-priority candidate with a user_id, so the device +// registration is kept and a canonical file holding only defaults never wins. +func legacyIdentity(fileDir, canonicalPath string) (identity map[string]any, ok bool) { candidates := []candidateSource{ {path: canonicalPath, label: "canonical settings.json"}, {path: filepath.Join(fileDir, legacySettingsFileName), label: "v9.0.x local.json"}, @@ -184,23 +227,13 @@ func migrateLegacySettingsIfNeeded(fileDir, canonicalPath string) { candidates[i].contents = b candidates[i].exists = true case errors.Is(err, fs.ErrNotExist): - // Expected — file just isn't there. Treat as not-present. default: - // Permission / I/O error — log it but don't bail outright. If - // it's the canonical path that's unreadable for non-ENOENT - // reasons, skip migration entirely so we don't try to write - // over a file the OS is telling us we can't see; for legacy - // or nested paths, treat the same as not-present. - slog.Warn("legacy settings migration: read failed", - "path", candidates[i].path, "error", err) - if candidates[i].path == canonicalPath { - return - } + candidates[i].err = err } } // Optional candidates splice in at index 2, inserted oldest-first so // newer generations end up higher priority. - if yc := legacyYAMLCandidate(fileDir); yc.exists { + if yc := legacyYAMLCandidate(fileDir); yc.exists || yc.err != nil { candidates = append(candidates[:2], append([]candidateSource{yc}, candidates[2:]...)...) } // Windows v9.0.x cross-dir candidates (${PUBLIC}\Lantern\data) are the @@ -212,63 +245,74 @@ func migrateLegacySettingsIfNeeded(fileDir, canonicalPath string) { if winExtras := windowsCrossDirCandidatesFn(fileDir); len(winExtras) > 0 { candidates = append(candidates[:2], append(winExtras, candidates[2:]...)...) } + for _, c := range candidates { + if c.err != nil { + slog.Warn("legacy settings migration: read failed; will retry", "path", c.path, "error", c.err) + return nil, false + } + } - // Pick: highest-priority file with user_level=="pro"; if none has pro, - // highest-priority file that exists at all (with non-empty contents). - pickIdx := -1 + parsed := make([]map[string]any, len(candidates)) for i, c := range candidates { - if c.exists && userLevelInJSON(c.contents) == "pro" { + if !c.exists { + continue + } + if err := jsonpkg.Unmarshal(c.contents, &parsed[i]); err != nil || parsed[i] == nil { + slog.Warn("legacy settings migration: ignoring invalid candidate", "path", c.path, "error", err) + parsed[i] = nil + } + } + pickIdx := -1 + for i, p := range parsed { + if p != nil && p[UserLevelKey.String()] == "pro" { pickIdx = i break } } if pickIdx == -1 { - for i, c := range candidates { - if c.exists { + for i, p := range parsed { + if id := toString(p[UserIDKey.String()]); id != "" && id != "0" { pickIdx = i break } } } - if pickIdx == -1 { - // Nothing on disk yet — fresh install, normal path. No-op. - return + if pickIdx == -1 || candidates[pickIdx].path == canonicalPath { + return nil, true } - if candidates[pickIdx].path == canonicalPath { - // Canonical already wins — no migration needed. - return + identity = make(map[string]any) + for _, key := range identityKeys { + if v, ok := parsed[pickIdx][key.String()]; ok { + identity[key.String()] = v + } } - writeMigrated(canonicalPath, candidates[pickIdx].contents, candidates[pickIdx].label) + slog.Info("legacy settings migration: recovering persisted identity", "source", candidates[pickIdx].label) + return identity, true } -// writeMigrated overwrites the canonical settings file with the recovered -// contents and logs the outcome. Uses atomicfile.WriteFile (the same -// mechanism the normal save path uses) so a crash mid-write can't leave -// a half-written settings.json on disk. Errors are logged-and-swallowed: -// if the write fails the caller falls through to the fresh-install path, -// which is a worse UX but not a corruption risk. -func writeMigrated(canonicalPath string, contents []byte, source string) { - if err := atomicfile.WriteFile(canonicalPath, contents, fileperm.File); err != nil { - slog.Warn("legacy settings migration: write failed", - "dst", canonicalPath, "source", source, "error", err) - return - } - slog.Info("legacy settings migration: recovered persisted state", - "dst", canonicalPath, "source", source, "bytes", len(contents)) +var identityKeys = []_key{ + UserIDKey, TokenKey, UserLevelKey, EmailKey, DeviceIDKey, + JwtTokenKey, OAuthLoginKey, OAuthProviderKey, +} + +// UserKeys are the settings that belong to the active user rather than the device. +var UserKeys = []_key{ + UserIDKey, TokenKey, UserLevelKey, EmailKey, + JwtTokenKey, DevicesKey, UserDataKey, OAuthLoginKey, OAuthProviderKey, } -// userLevelInJSON returns the value of the "user_level" key from a JSON -// settings blob, or "" if the key is missing / the blob is malformed. -// Lightweight extraction so the migration doesn't need to load the full -// koanf state machine before we've decided which file to read. -func userLevelInJSON(contents []byte) string { - var s struct { - UserLevel string `json:"user_level"` +func adoptIdentity(kk *koanf.Koanf, identity map[string]any) error { + if toString(kk.Get(UserIDKey.String())) != toString(identity[UserIDKey.String()]) { + for _, key := range UserKeys { + kk.Delete(key.String()) + } } - if err := jsonpkg.Unmarshal(contents, &s); err != nil { - return "" + for key, v := range identity { + if err := kk.Set(key, v); err != nil { + return fmt.Errorf("could not set key %s: %w", key, err) + } } - return s.UserLevel + return nil } // loadSettings applies the file to the in-memory settings. @@ -319,10 +363,13 @@ func Get(key _key) any { } func GetString(key _key) string { + return toString(Get(key)) +} + +func toString(value any) string { // JSON round-trip turns all numbers into float64 and since koanf uses Sprintf("%v") for string // conversion, large integers (i.e. userID) get converted to scientific notation (e.g. 3.87286618e+08) // so we handle float64 separately - value := Get(key) if value == nil { return "" } @@ -368,34 +415,29 @@ func Exists(key _key) bool { return k.k.Exists(key.String()) } +// Set stores value under key. On error the previous value remains. func Set(key _key, value any) error { - // take lock for the entire duration of the Set + save sequence to prevent multiple Set - // calls from interleaving and leaving the file in an inconsistent state until the next write. - k.mu.Lock() - defer k.mu.Unlock() - err := k.k.Set(key.String(), value) - if err != nil { - return fmt.Errorf("could not set key %s: %w", key, err) - } - return save() + return Patch(Settings{key: value}) } +// Clear removes keys and saves the settings. Unlike [Set] and [Patch], the keys stay removed in +// memory even when the save fails; the save error is still returned. func Clear(keys ..._key) error { - // take lock for the entire duration. See [Set] for explanation. k.mu.Lock() defer k.mu.Unlock() for _, key := range keys { k.k.Delete(key.String()) } - return save() + return saveLocked(k.k) } type Settings map[_key]any +// Diff returns the entries of s that are absent from s2 or hold a different value there. func (s Settings) Diff(s2 Settings) Settings { diff := make(Settings) for k, v1 := range s { - if v2, ok := s2[k]; !ok || v1 != v2 { + if v2, ok := s2[k]; !ok || !reflect.DeepEqual(v1, v2) { diff[k] = v1 } } @@ -410,48 +452,96 @@ func GetAll() Settings { return s } +// GetAllFor returns the values of keys that are set, omitting unset keys; with no keys it returns +// all settings. func GetAllFor(keys ..._key) Settings { if len(keys) == 0 { return GetAll() } s := make(Settings) for _, key := range keys { - s[key] = k.k.Get(key.String()) + if k.k.Exists(key.String()) { + s[key] = k.k.Get(key.String()) + } } return s } -// Patch takes a map of settings to update and applies them all at once. +// Patch applies updates all at once. On error the previous values remain. func Patch(updates Settings) error { - // take lock for the entire duration. See [Set] for explanation. k.mu.Lock() defer k.mu.Unlock() + return updateLocked(func(kk *koanf.Koanf) error { return setAll(kk, updates) }) +} + +// Apply is like [Patch], except the updates take effect in memory even when the save fails; the +// save error is still returned. +func Apply(updates Settings) error { + k.mu.Lock() + defer k.mu.Unlock() + if err := setAll(k.k, updates); err != nil { + return err + } + return saveLocked(k.k) +} + +func setAll(kk *koanf.Koanf, updates Settings) error { for key, value := range updates { - if err := k.k.Set(_key(key).String(), value); err != nil { + if err := kk.Set(key.String(), value); err != nil { return fmt.Errorf("could not set key %s: %w", key, err) } } - return save() + return nil } -func save() error { - out, err := k.k.Marshal(json.Parser()) - if err != nil { - return fmt.Errorf("could not marshal koanf file: %w", err) +// updateLocked runs mutate on a copy of the settings, saves the copy, and only then makes it the +// live settings, so a failed save leaves them unchanged. It requires k.mu to be held. +func updateLocked(mutate func(*koanf.Koanf) error) error { + staged := k.k.Copy() + if err := mutate(staged); err != nil { + return err } + if err := saveLocked(staged); err != nil { + return err + } + // Getters read k.k without k.mu, so replace its contents in place rather than the pointer. + return k.k.Load(mapProvider(staged.Raw()), nil, koanf.WithMergeFunc(replaceAll)) +} + +// mapProvider is a koanf provider for an already-parsed settings map. +type mapProvider map[string]any + +func (p mapProvider) Read() (map[string]any, error) { return p, nil } - err = atomicfile.WriteFile(k.filePath, out, fileperm.File) +func (mapProvider) ReadBytes() ([]byte, error) { + return nil, errors.New("mapProvider has no raw bytes") +} + +// saveLocked writes src to the settings file unless the file already holds it. It requires k.mu +// to be held. +func saveLocked(src *koanf.Koanf) error { + if k.filePath == "" { + return nil + } + out, err := src.Marshal(json.Parser()) if err != nil { + return fmt.Errorf("could not marshal koanf file: %w", err) + } + if current, err := atomicfile.ReadFile(k.filePath); err == nil && bytes.Equal(out, current) { + return nil + } + if err := atomicfile.WriteFile(k.filePath, out, fileperm.File); err != nil { return fmt.Errorf("could not write koanf file: %w", err) } return nil } -// Reset clears the current settings in memory primarily for testing purposes. +// Reset restores the default, uninitialized settings, primarily for testing purposes. func Reset() { k.mu.Lock() defer k.mu.Unlock() - k.k = koanf.New(".") + k.k = newDefaults() + k.filePath = "" k.initialized = false } diff --git a/common/settings/settings_test.go b/common/settings/settings_test.go index 8c755dbe..3850d27c 100644 --- a/common/settings/settings_test.go +++ b/common/settings/settings_test.go @@ -1,6 +1,7 @@ package settings import ( + "encoding/json" "os" "path/filepath" "runtime" @@ -42,20 +43,15 @@ func TestWindowsCrossDirCandidates(t *testing.T) { assert.Contains(t, cs[0].label, "v9.0.x Windows", "label must identify the cross-dir source") }) - t.Run("readWindowsCrossDirCandidates picks up both local.json and settings.json", func(t *testing.T) { + t.Run("readWindowsCrossDirCandidates ignores settings.json", func(t *testing.T) { dir := t.TempDir() local := []byte(`{"user_level":"pro","user_id":1}`) - canon := []byte(`{"user_level":"expired","user_id":2}`) require.NoError(t, os.WriteFile(filepath.Join(dir, legacySettingsFileName), local, 0o644)) - require.NoError(t, os.WriteFile(filepath.Join(dir, settingsFileName), canon, 0o644)) + require.NoError(t, os.WriteFile(filepath.Join(dir, settingsFileName), []byte(`{"user_id":2}`), 0o644)) cs := readWindowsCrossDirCandidates(dir) - require.Len(t, cs, 2, "both filenames should be returned when both exist") - // local.json must come first — it's the actual v9.0.x name; the - // settings.json fallback is for users who got renamed by a - // partial earlier upgrade. - assert.Equal(t, local, cs[0].contents, "local.json must be first") - assert.Equal(t, canon, cs[1].contents, "settings.json fallback must be second") + require.Len(t, cs, 1) + assert.Equal(t, local, cs[0].contents) }) t.Run("readWindowsCrossDirCandidates returns empty when dir is empty", func(t *testing.T) { @@ -88,14 +84,57 @@ func TestWindowsCrossDirCandidates(t *testing.T) { }) } +// isolateDiscovery stops InitSettings from reading the host's own legacy settings files. +func isolateDiscovery(t *testing.T) { + t.Helper() + prevYAML, prevWin := legacyYAMLPathFn, windowsCrossDirCandidatesFn + legacyYAMLPathFn = func(string) (string, string) { return "", "" } + windowsCrossDirCandidatesFn = func(string) []candidateSource { return nil } + t.Cleanup(func() { + legacyYAMLPathFn, windowsCrossDirCandidatesFn = prevYAML, prevWin + }) +} + +func initAt(t *testing.T, dir string) { + t.Helper() + Reset() + t.Cleanup(Reset) + require.NoError(t, InitSettings(dir)) +} + +// readOnlyDir makes dir readable but not writable until the returned func restores it, skipping the +// test where that cannot be enforced. +func readOnlyDir(t *testing.T, dir string) (restore func()) { + t.Helper() + if runtime.GOOS == "windows" { + t.Skip("directory permissions differ on windows") + } + if os.Geteuid() == 0 { + t.Skip("root writes regardless of mode") + } + require.NoError(t, os.Chmod(dir, 0o555)) + t.Cleanup(func() { _ = os.Chmod(dir, 0o755) }) + return func() { require.NoError(t, os.Chmod(dir, 0o755)) } +} + +// moveAside makes dir unavailable until the returned func moves it back. +func moveAside(t *testing.T, dir string) (restore func()) { + t.Helper() + aside := dir + ".aside" + require.NoError(t, os.Rename(dir, aside)) + return func() { require.NoError(t, os.Rename(aside, dir)) } +} + func TestInitSettings(t *testing.T) { + isolateDiscovery(t) + t.Run("existing valid config file", func(t *testing.T) { tempDir := t.TempDir() path := filepath.Join(tempDir, settingsFileName) content := []byte(`{"locale": "en-US", "country_code": "US"}`) require.NoError(t, os.WriteFile(path, content, 0644), "failed to create test config file") - require.NoError(t, InitSettings(tempDir), "failed to initialize settings") + initAt(t, tempDir) assert.Equal(t, "en-US", Get(LocaleKey)) assert.Equal(t, "US", Get(CountryCodeKey)) }) @@ -108,403 +147,622 @@ func TestInitSettings(t *testing.T) { }) t.Run("invalid config file recovers", func(t *testing.T) { - Reset() - t.Cleanup(Reset) tempDir := t.TempDir() path := filepath.Join(tempDir, settingsFileName) require.NoError(t, os.WriteFile(path, []byte(`{invalid json}`), 0644)) - require.NoError(t, InitSettings(tempDir), "an invalid settings file must not block initialization") + initAt(t, tempDir) invalidPath := filepath.Join(tempDir, settingsInvalidFileName) assert.FileExists(t, invalidPath, "the invalid file must be preserved for diagnostics") assert.NoError(t, loadSettings(path), "settings.json must be rewritten with a parseable default") }) -} -func TestMigrateLegacySettingsIfNeeded(t *testing.T) { - // Redirect the OS-specific pre-9.x YAML lookup to nowhere by - // default so individual tests don't pick up the host machine's - // actual ~/Library/Application Support/Lantern/settings.yaml or - // equivalent. Sub-tests that exercise the YAML path opt in by - // pointing the function at their tempDir. - prevYAMLPath := legacyYAMLPathFn - legacyYAMLPathFn = func(string) (string, string) { return "", "" } - t.Cleanup(func() { legacyYAMLPathFn = prevYAMLPath }) + t.Run("changes before InitSettings stay in memory", func(t *testing.T) { + Reset() + t.Cleanup(Reset) - // Same treatment for the Windows v9.0.x cross-dir lookup: default - // to nothing so tests don't accidentally pick up a real - // %PUBLIC%\Lantern\data on a developer's Windows machine. Tests that - // exercise this path opt in. - prevWinCrossDir := windowsCrossDirCandidatesFn - windowsCrossDirCandidatesFn = func(string) []candidateSource { return nil } - t.Cleanup(func() { windowsCrossDirCandidatesFn = prevWinCrossDir }) + require.NoError(t, Set(LocaleKey, "en-US")) + + assert.Equal(t, "en-US", GetString(LocaleKey)) + }) + + t.Run("Reset restores defaults", func(t *testing.T) { + initAt(t, t.TempDir()) + require.NoError(t, Set(UserLevelKey, "pro")) + Reset() + assert.Equal(t, "free", GetString(UserLevelKey)) + assert.Equal(t, "fa-IR", GetString(LocaleKey)) + }) +} + +func TestLegacyMigration(t *testing.T) { + isolateDiscovery(t) - writeNested := func(t *testing.T, dir string, contents []byte) { + writeNested := func(t *testing.T, dir string, contents string) { t.Helper() nd := filepath.Join(dir, "data") require.NoError(t, os.MkdirAll(nd, 0o755)) - require.NoError(t, os.WriteFile(filepath.Join(nd, settingsFileName), contents, 0o644)) + require.NoError(t, os.WriteFile(filepath.Join(nd, settingsFileName), []byte(contents), 0o644)) } - writeLegacy := func(t *testing.T, dir string, contents []byte) { + writeLegacy := func(t *testing.T, dir string, contents string) { t.Helper() - require.NoError(t, os.WriteFile(filepath.Join(dir, legacySettingsFileName), contents, 0o644)) + require.NoError(t, os.WriteFile(filepath.Join(dir, legacySettingsFileName), []byte(contents), 0o644)) + } + writeCanonical := func(t *testing.T, dir string, contents string) { + t.Helper() + require.NoError(t, os.WriteFile(filepath.Join(dir, settingsFileName), []byte(contents), 0o644)) + } + useCrossDir := func(t *testing.T, v90xDir string) { + t.Helper() + prev := windowsCrossDirCandidatesFn + windowsCrossDirCandidatesFn = func(string) []candidateSource { + return readWindowsCrossDirCandidates(v90xDir) + } + t.Cleanup(func() { windowsCrossDirCandidatesFn = prev }) + } + useYAML := func(t *testing.T, dir, contents string) { + t.Helper() + yamlPath := filepath.Join(dir, "fake-pre9x-settings.yaml") + require.NoError(t, os.WriteFile(yamlPath, []byte(contents), 0o644)) + prev := legacyYAMLPathFn + legacyYAMLPathFn = func(string) (string, string) { return yamlPath, "desktop" } + t.Cleanup(func() { legacyYAMLPathFn = prev }) + } + // onDisk reads back the saved settings, so assertions cover what the next start sees. + onDisk := func(t *testing.T, dir string) map[string]any { + t.Helper() + b, err := os.ReadFile(filepath.Join(dir, settingsFileName)) + require.NoError(t, err) + var got map[string]any + require.NoError(t, json.Unmarshal(b, &got)) + return got } - t.Run("v9.0.x local.json recovered when canonical is missing (Derek's failing case)", func(t *testing.T) { - // User upgraded from v9.0.x straight to the fixed build. v9.0.x wrote - // to /local.json; canonical settings.json doesn't exist; - // no v9.1.x nested file. The fix must read local.json so Pro survives. + t.Run("v9.0.x local.json recovered when canonical is missing", func(t *testing.T) { tempDir := t.TempDir() - want := []byte(`{"user_id": 3580849, "user_level": "pro", "token": "abc"}`) - writeLegacy(t, tempDir, want) + writeLegacy(t, tempDir, `{"user_id": 3580849, "user_level": "pro", "token": "abc"}`) - canonical := filepath.Join(tempDir, settingsFileName) - migrateLegacySettingsIfNeeded(tempDir, canonical) + initAt(t, tempDir) - got, err := os.ReadFile(canonical) - require.NoError(t, err) - assert.Equal(t, want, got, "v9.0.x local.json should be migrated to canonical") + assert.Equal(t, "3580849", GetString(UserIDKey)) + assert.Equal(t, "pro", GetString(UserLevelKey)) + assert.Equal(t, "abc", GetString(TokenKey)) + assert.Equal(t, "abc", onDisk(t, tempDir)["token"], "adopted identity must be saved") + assert.FileExists(t, filepath.Join(tempDir, migrationMarkerFileName)) }) t.Run("v9.1.x nested file recovered when canonical is missing", func(t *testing.T) { tempDir := t.TempDir() - want := []byte(`{"user_id": 135809562, "user_level": "pro", "device_id": "abc"}`) - writeNested(t, tempDir, want) + writeNested(t, tempDir, `{"user_id": 135809562, "user_level": "pro", "device_id": "abc"}`) - canonical := filepath.Join(tempDir, settingsFileName) - migrateLegacySettingsIfNeeded(tempDir, canonical) + initAt(t, tempDir) - got, err := os.ReadFile(canonical) - require.NoError(t, err) - assert.Equal(t, want, got, "v9.1.x nested file should be migrated to canonical") + assert.Equal(t, "135809562", GetString(UserIDKey)) + assert.Equal(t, "abc", GetString(DeviceIDKey)) }) t.Run("v9.0.x local.json wins over v9.1.x expired nested", func(t *testing.T) { - // Upgrade chain v9.0.x → v9.1.x → fix: legacy has pro, nested has - // expired (because v9.1.x lost the user_id). Migration must pick - // legacy so Pro survives. tempDir := t.TempDir() - canonical := filepath.Join(tempDir, settingsFileName) - legacyPro := []byte(`{"user_id": 1, "user_level": "pro"}`) - writeLegacy(t, tempDir, legacyPro) - writeNested(t, tempDir, []byte(`{"user_id": 999, "user_level": "expired"}`)) + writeLegacy(t, tempDir, `{"user_id": 1, "user_level": "pro"}`) + writeNested(t, tempDir, `{"user_id": 999, "user_level": "expired"}`) - migrateLegacySettingsIfNeeded(tempDir, canonical) + initAt(t, tempDir) - got, err := os.ReadFile(canonical) - require.NoError(t, err) - assert.Equal(t, legacyPro, got, "legacy local.json with pro should beat nested expired") + assert.Equal(t, "1", GetString(UserIDKey)) + assert.True(t, IsPro()) }) t.Run("canonical-pro wins over nested-expired", func(t *testing.T) { tempDir := t.TempDir() - canonical := filepath.Join(tempDir, settingsFileName) - canonicalPro := []byte(`{"user_id": 1, "user_level": "pro"}`) - require.NoError(t, os.WriteFile(canonical, canonicalPro, 0o644)) - writeNested(t, tempDir, []byte(`{"user_id": 999, "user_level": "expired"}`)) + writeCanonical(t, tempDir, `{"user_id": 1, "user_level": "pro"}`) + writeNested(t, tempDir, `{"user_id": 999, "user_level": "expired"}`) - migrateLegacySettingsIfNeeded(tempDir, canonical) + initAt(t, tempDir) - got, err := os.ReadFile(canonical) - require.NoError(t, err) - assert.Equal(t, canonicalPro, got, "canonical-pro should survive") + assert.Equal(t, "1", GetString(UserIDKey)) + assert.True(t, IsPro()) }) t.Run("nested-pro wins over canonical-expired and legacy-expired", func(t *testing.T) { // e.g., user paid via Shepherd while on v9.1.x, so the nested file // legitimately holds pro state. tempDir := t.TempDir() - canonical := filepath.Join(tempDir, settingsFileName) - require.NoError(t, os.WriteFile(canonical, []byte(`{"user_id": 1, "user_level": "expired"}`), 0o644)) - writeLegacy(t, tempDir, []byte(`{"user_id": 1, "user_level": "expired"}`)) - nestedPro := []byte(`{"user_id": 2, "user_level": "pro"}`) - writeNested(t, tempDir, nestedPro) + writeCanonical(t, tempDir, `{"user_id": 1, "user_level": "expired", "token": "t1", "jwt_token": "j1", "locale": "en-US"}`) + writeLegacy(t, tempDir, `{"user_id": 1, "user_level": "expired"}`) + writeNested(t, tempDir, `{"user_id": 2, "user_level": "pro"}`) - migrateLegacySettingsIfNeeded(tempDir, canonical) + initAt(t, tempDir) - got, err := os.ReadFile(canonical) - require.NoError(t, err) - assert.Equal(t, nestedPro, got, "nested-pro should beat both canonical and legacy when only it has pro") + assert.Equal(t, "2", GetString(UserIDKey)) + assert.True(t, IsPro()) + assert.False(t, Exists(TokenKey), "the replaced user's token must not carry over") + assert.False(t, Exists(JwtTokenKey), "the replaced user's JWT must not carry over") + assert.Equal(t, "en-US", GetString(LocaleKey), "non-identity settings are kept") + }) + + t.Run("same user keeps its credentials when a legacy file restores pro", func(t *testing.T) { + tempDir := t.TempDir() + writeCanonical(t, tempDir, `{"user_id": 1, "user_level": "expired", "jwt_token": "j1"}`) + writeLegacy(t, tempDir, `{"user_id": 1, "user_level": "pro"}`) + + initAt(t, tempDir) + + assert.True(t, IsPro()) + assert.Equal(t, "j1", GetString(JwtTokenKey)) }) t.Run("all-pro: canonical wins (most recent deliberate state)", func(t *testing.T) { tempDir := t.TempDir() - canonical := filepath.Join(tempDir, settingsFileName) - canonicalContents := []byte(`{"user_id": 1, "user_level": "pro"}`) - require.NoError(t, os.WriteFile(canonical, canonicalContents, 0o644)) - writeLegacy(t, tempDir, []byte(`{"user_id": 2, "user_level": "pro"}`)) - writeNested(t, tempDir, []byte(`{"user_id": 3, "user_level": "pro"}`)) + writeCanonical(t, tempDir, `{"user_id": 1, "user_level": "pro"}`) + writeLegacy(t, tempDir, `{"user_id": 2, "user_level": "pro"}`) + writeNested(t, tempDir, `{"user_id": 3, "user_level": "pro"}`) - migrateLegacySettingsIfNeeded(tempDir, canonical) + initAt(t, tempDir) - got, err := os.ReadFile(canonical) - require.NoError(t, err) - assert.Equal(t, canonicalContents, got, "canonical preferred when all have pro") + assert.Equal(t, "1", GetString(UserIDKey)) }) t.Run("none have pro: legacy wins over nested when canonical missing", func(t *testing.T) { // User identifiers must survive even when Pro state is non-pro, // to keep the device registration intact server-side. tempDir := t.TempDir() - canonical := filepath.Join(tempDir, settingsFileName) - legacyContents := []byte(`{"user_id": 1, "user_level": "free", "token": "abc"}`) - writeLegacy(t, tempDir, legacyContents) - writeNested(t, tempDir, []byte(`{"user_id": 2, "user_level": "free", "token": "xyz"}`)) + writeLegacy(t, tempDir, `{"user_id": 1, "user_level": "free", "token": "abc"}`) + writeNested(t, tempDir, `{"user_id": 2, "user_level": "free", "token": "xyz"}`) - migrateLegacySettingsIfNeeded(tempDir, canonical) + initAt(t, tempDir) - got, err := os.ReadFile(canonical) - require.NoError(t, err) - assert.Equal(t, legacyContents, got, "legacy preferred over nested when canonical missing and neither has pro") + assert.Equal(t, "1", GetString(UserIDKey)) + assert.Equal(t, "abc", GetString(TokenKey)) }) - t.Run("nothing on disk is a no-op", func(t *testing.T) { + t.Run("adopts the recovered account's OAuth state", func(t *testing.T) { tempDir := t.TempDir() - canonical := filepath.Join(tempDir, settingsFileName) + writeNested(t, tempDir, `{"user_id": 3, "user_level": "pro", "jwt_token": "jwt", "oauth_login": true, "oauth_provider": "apple"}`) + + initAt(t, tempDir) + + assert.Equal(t, "3", GetString(UserIDKey)) + assert.Equal(t, "jwt", GetString(JwtTokenKey)) + assert.True(t, GetBool(OAuthLoginKey)) + assert.Equal(t, "apple", GetString(OAuthProviderKey)) + }) + + t.Run("a canonical file without a user id doesn't outrank a legacy identity", func(t *testing.T) { + tempDir := t.TempDir() + writeCanonical(t, tempDir, `{"locale": "en-US", "device_id": "dev"}`) + writeLegacy(t, tempDir, `{"user_id": 7, "user_level": "free", "token": "tok"}`) + + initAt(t, tempDir) + + assert.Equal(t, "7", GetString(UserIDKey)) + assert.Equal(t, "en-US", GetString(LocaleKey)) + assert.Equal(t, "dev", GetString(DeviceIDKey)) + }) + + t.Run("an unreadable legacy file leaves the migration pending", func(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("permission semantics differ on windows") + } + if os.Geteuid() == 0 { + t.Skip("root reads files regardless of mode") + } + tempDir := t.TempDir() + writeLegacy(t, tempDir, `{"user_id": 7, "user_level": "free", "token": "tok"}`) + legacy := filepath.Join(tempDir, legacySettingsFileName) + require.NoError(t, os.Chmod(legacy, 0o000)) + t.Cleanup(func() { _ = os.Chmod(legacy, 0o644) }) + + initAt(t, tempDir) + assert.False(t, Exists(UserIDKey)) + assert.NoFileExists(t, filepath.Join(tempDir, migrationMarkerFileName)) + require.NoError(t, Set(DeviceIDKey, "dev")) + + require.NoError(t, os.Chmod(legacy, 0o644)) + initAt(t, tempDir) + assert.Equal(t, "7", GetString(UserIDKey)) + assert.FileExists(t, filepath.Join(tempDir, migrationMarkerFileName)) + }) + + t.Run("adopts identity keys only", func(t *testing.T) { + tempDir := t.TempDir() + writeLegacy(t, tempDir, `{"user_id": 1, "user_level": "pro", "peer_share_enabled": true, "locale": "ru-RU"}`) - migrateLegacySettingsIfNeeded(tempDir, canonical) + initAt(t, tempDir) - _, err := os.Stat(canonical) - assert.True(t, os.IsNotExist(err), "no migration when no source files exist") + assert.Equal(t, "1", GetString(UserIDKey)) + assert.False(t, Exists(PeerShareEnabledKey)) + assert.Equal(t, "fa-IR", GetString(LocaleKey)) + }) + + t.Run("fresh install writes defaults and the marker", func(t *testing.T) { + tempDir := t.TempDir() + + initAt(t, tempDir) + + assert.Equal(t, "free", GetString(UserLevelKey)) + assert.FileExists(t, filepath.Join(tempDir, settingsFileName)) + assert.FileExists(t, filepath.Join(tempDir, migrationMarkerFileName)) + }) + + t.Run("a completed migration never resurrects a cleared account", func(t *testing.T) { + tempDir := t.TempDir() + writeLegacy(t, tempDir, `{"user_id": 111, "user_level": "pro", "token": "OLD-PRO-TOKEN", "email": "old@example.com"}`) + initAt(t, tempDir) + require.Equal(t, "111", GetString(UserIDKey)) + + require.NoError(t, Clear(UserIDKey, TokenKey, UserLevelKey, EmailKey)) + require.NoError(t, Patch(Settings{UserIDKey: 222, UserLevelKey: "free"})) + initAt(t, tempDir) + + assert.Equal(t, "222", GetString(UserIDKey)) + assert.Equal(t, "free", GetString(UserLevelKey)) + assert.False(t, Exists(TokenKey)) + + require.NoError(t, Set(UserLevelKey, "expired")) + initAt(t, tempDir) + assert.Equal(t, "expired", GetString(UserLevelKey)) + }) + + t.Run("a missing canonical file after migration starts fresh", func(t *testing.T) { + tempDir := t.TempDir() + writeLegacy(t, tempDir, `{"user_id": 111, "user_level": "pro"}`) + initAt(t, tempDir) + require.NoError(t, os.Remove(filepath.Join(tempDir, settingsFileName))) + + initAt(t, tempDir) + + assert.False(t, Exists(UserIDKey)) + }) + + t.Run("malformed canonical loses to a valid legacy file", func(t *testing.T) { + tempDir := t.TempDir() + writeCanonical(t, tempDir, `{invalid json}`) + writeLegacy(t, tempDir, `{"user_id": 7, "user_level": "free", "token": "tok"}`) + + initAt(t, tempDir) + + assert.Equal(t, "7", GetString(UserIDKey)) + assert.Equal(t, "tok", GetString(TokenKey)) + assert.FileExists(t, filepath.Join(tempDir, settingsInvalidFileName)) + }) + + t.Run("malformed legacy candidate is ignored", func(t *testing.T) { + tempDir := t.TempDir() + writeLegacy(t, tempDir, `{"user_level": "pro"`) + writeNested(t, tempDir, `{"user_id": 5, "user_level": "free"}`) + + initAt(t, tempDir) + + assert.Equal(t, "5", GetString(UserIDKey)) + }) + + t.Run("a failed save is retried on the next start", func(t *testing.T) { + tempDir := t.TempDir() + writeCanonical(t, tempDir, `{"user_id": 1, "user_level": "expired"}`) + writeLegacy(t, tempDir, `{"user_id": 2, "user_level": "pro"}`) + restore := readOnlyDir(t, tempDir) + + initAt(t, tempDir) + assert.Equal(t, "1", GetString(UserIDKey), "an unsaved adoption must not take effect") + assert.NoFileExists(t, filepath.Join(tempDir, migrationMarkerFileName)) + + restore() + initAt(t, tempDir) + assert.Equal(t, "2", GetString(UserIDKey)) + assert.FileExists(t, filepath.Join(tempDir, migrationMarkerFileName)) + }) + + t.Run("a new canonical file carries the adopted identity", func(t *testing.T) { + tempDir := t.TempDir() + writeLegacy(t, tempDir, `{"user_id": 7, "user_level": "free", "token": "tok"}`) + + initAt(t, tempDir) + + assert.Equal(t, float64(7), onDisk(t, tempDir)["user_id"]) + }) + + t.Run("a failed first write leaves a non-pro legacy identity recoverable", func(t *testing.T) { + tempDir := t.TempDir() + writeLegacy(t, tempDir, `{"user_id": 7, "user_level": "free", "token": "tok"}`) + restore := readOnlyDir(t, tempDir) + + Reset() + t.Cleanup(Reset) + require.Error(t, InitSettings(tempDir)) + assert.NoFileExists(t, filepath.Join(tempDir, settingsFileName)) + + restore() + initAt(t, tempDir) + assert.Equal(t, "7", GetString(UserIDKey)) }) t.Run("pre-9.x desktop YAML recovered when no JSON candidates exist", func(t *testing.T) { - // Redirect the YAML lookup at a tempDir-local file so the test - // is portable across OSes. tempDir := t.TempDir() - yamlPath := filepath.Join(tempDir, "fake-pre9x-settings.yaml") - require.NoError(t, os.WriteFile(yamlPath, []byte(`userID: 3580849 + useYAML(t, tempDir, `userID: 3580849 deviceID: legacy-device-id userPro: true userToken: legacy-token emailAddress: derek@example.com -`), 0o644)) - legacyYAMLPathFn = func(string) (string, string) { return yamlPath, "desktop" } - t.Cleanup(func() { legacyYAMLPathFn = func(string) (string, string) { return "", "" } }) +`) - canonical := filepath.Join(tempDir, settingsFileName) - migrateLegacySettingsIfNeeded(tempDir, canonical) + initAt(t, tempDir) - got, err := os.ReadFile(canonical) - require.NoError(t, err) - gotStr := string(got) - assert.Contains(t, gotStr, `"user_id":3580849`) - assert.Contains(t, gotStr, `"device_id":"legacy-device-id"`) - assert.Contains(t, gotStr, `"user_level":"pro"`) - assert.Contains(t, gotStr, `"token":"legacy-token"`) - assert.Contains(t, gotStr, `"email":"derek@example.com"`) + assert.Equal(t, "3580849", GetString(UserIDKey)) + assert.Equal(t, "legacy-device-id", GetString(DeviceIDKey)) + assert.Equal(t, "pro", GetString(UserLevelKey)) + assert.Equal(t, "legacy-token", GetString(TokenKey)) + assert.Equal(t, "derek@example.com", GetString(EmailKey)) }) t.Run("v9.0.x local.json beats pre-9.x YAML", func(t *testing.T) { - // Both exist with pro state. local.json is the higher-priority - // (more recent) source, so it should win. tempDir := t.TempDir() - yamlPath := filepath.Join(tempDir, "fake-pre9x-settings.yaml") - require.NoError(t, os.WriteFile(yamlPath, []byte(`userID: 1 + useYAML(t, tempDir, `userID: 1 userPro: true userToken: legacy-token -`), 0o644)) - legacyYAMLPathFn = func(string) (string, string) { return yamlPath, "desktop" } - t.Cleanup(func() { legacyYAMLPathFn = func(string) (string, string) { return "", "" } }) +`) + writeLegacy(t, tempDir, `{"user_id": 2, "user_level": "pro", "token": "v9.0-token"}`) - writeLegacy(t, tempDir, []byte(`{"user_id": 2, "user_level": "pro", "token": "v9.0-token"}`)) - - canonical := filepath.Join(tempDir, settingsFileName) - migrateLegacySettingsIfNeeded(tempDir, canonical) + initAt(t, tempDir) - got, err := os.ReadFile(canonical) - require.NoError(t, err) - assert.Contains(t, string(got), `"user_id": 2`, - "v9.0.x local.json should win over pre-9.x YAML when both have pro") - assert.Contains(t, string(got), `"v9.0-token"`) + assert.Equal(t, "2", GetString(UserIDKey)) + assert.Equal(t, "v9.0-token", GetString(TokenKey)) }) t.Run("pre-9.x YAML beats v9.1.x bugged nested file", func(t *testing.T) { - // Pre-9.x has pro; v9.1.x nested has expired (the bugged case). - // Pre-9.x must win. tempDir := t.TempDir() - yamlPath := filepath.Join(tempDir, "fake-pre9x-settings.yaml") - require.NoError(t, os.WriteFile(yamlPath, []byte(`userID: 1 + useYAML(t, tempDir, `userID: 1 userPro: true userToken: legacy-token -`), 0o644)) - legacyYAMLPathFn = func(string) (string, string) { return yamlPath, "desktop" } - t.Cleanup(func() { legacyYAMLPathFn = func(string) (string, string) { return "", "" } }) +`) + writeNested(t, tempDir, `{"user_id": 999, "user_level": "expired"}`) - writeNested(t, tempDir, []byte(`{"user_id": 999, "user_level": "expired"}`)) + initAt(t, tempDir) - canonical := filepath.Join(tempDir, settingsFileName) - migrateLegacySettingsIfNeeded(tempDir, canonical) - - got, err := os.ReadFile(canonical) - require.NoError(t, err) - assert.Contains(t, string(got), `"user_level":"pro"`, - "pre-9.x YAML with pro should win over v9.1.x nested expired") - assert.Contains(t, string(got), `"legacy-token"`) + assert.Equal(t, "pro", GetString(UserLevelKey)) + assert.Equal(t, "legacy-token", GetString(TokenKey)) }) t.Run("iOS userconfig.yaml recovered when canonical is missing", func(t *testing.T) { - // On iOS the legacy YAML is sandbox-relative — it lives next to - // where settings.json now lives, so legacyYAMLCandidate reads - // from fileDir directly and we can exercise it from a test - // without monkeypatching $HOME or $APPDATA. (Desktop legacy - // paths are covered via translateLegacyYAML's unit tests, which - // don't depend on the OS-specific path resolution.) if runtime.GOOS != "ios" { t.Skip("iOS-only path: legacy YAML elsewhere is OS-specific") } + prev := legacyYAMLPathFn + legacyYAMLPathFn = legacyYAMLPath + t.Cleanup(func() { legacyYAMLPathFn = prev }) tempDir := t.TempDir() - yamlPath := filepath.Join(tempDir, "userconfig.yaml") - require.NoError(t, os.WriteFile(yamlPath, []byte(`UserID: 7777 + require.NoError(t, os.WriteFile(filepath.Join(tempDir, "userconfig.yaml"), []byte(`UserID: 7777 DeviceID: ios-device Token: tok `), 0o644)) - canonical := filepath.Join(tempDir, settingsFileName) - migrateLegacySettingsIfNeeded(tempDir, canonical) - got, err := os.ReadFile(canonical) - require.NoError(t, err) - assert.Contains(t, string(got), `"user_id":7777`) - assert.Contains(t, string(got), `"device_id":"ios-device"`) + initAt(t, tempDir) + + assert.Equal(t, "7777", GetString(UserIDKey)) + assert.Equal(t, "ios-device", GetString(DeviceIDKey)) }) t.Run("Windows v9.0.x cross-dir local.json recovered when canonical/same-dir/nested all missing", func(t *testing.T) { - // FD #174606 / engineering#3460: PR #370 moved the Windows daemon - // from \Lantern\data (where v9.0.x Flutter+FFI wrote - // local.json) to \Lantern. The same-dir scan can't - // see across that filesystem boundary, so cross-dir candidates - // are spliced in by windowsCrossDirCandidatesFn. Verify recovery. tempDir := t.TempDir() v90xDir := filepath.Join(tempDir, "windows-v90x") require.NoError(t, os.MkdirAll(v90xDir, 0o755)) - want := []byte(`{"user_id": 195646669, "user_level": "pro", "token": "preserved"}`) - require.NoError(t, os.WriteFile(filepath.Join(v90xDir, legacySettingsFileName), want, 0o644)) - - windowsCrossDirCandidatesFn = func(string) []candidateSource { - return readWindowsCrossDirCandidates(v90xDir) - } - t.Cleanup(func() { - windowsCrossDirCandidatesFn = func(string) []candidateSource { return nil } - }) + require.NoError(t, os.WriteFile(filepath.Join(v90xDir, legacySettingsFileName), + []byte(`{"user_id": 195646669, "user_level": "pro", "token": "preserved"}`), 0o644)) + useCrossDir(t, v90xDir) - canonical := filepath.Join(tempDir, settingsFileName) - migrateLegacySettingsIfNeeded(tempDir, canonical) + initAt(t, tempDir) - got, err := os.ReadFile(canonical) - require.NoError(t, err) - assert.Equal(t, want, got, "v9.0.x cross-dir local.json must be migrated to canonical") + assert.Equal(t, "195646669", GetString(UserIDKey)) + assert.Equal(t, "preserved", GetString(TokenKey)) }) - t.Run("Windows v9.0.x cross-dir falls back to settings.json when local.json missing", func(t *testing.T) { - // Defensive case: a user whose v9.0.x file got renamed by an - // earlier partial upgrade attempt. The cross-dir scan should - // still pick up settings.json under ${PUBLIC}\Lantern\data. + t.Run("Windows cross-dir settings.json is never adopted", func(t *testing.T) { tempDir := t.TempDir() v90xDir := filepath.Join(tempDir, "windows-v90x") require.NoError(t, os.MkdirAll(v90xDir, 0o755)) - want := []byte(`{"user_id": 42, "user_level": "pro"}`) - require.NoError(t, os.WriteFile(filepath.Join(v90xDir, settingsFileName), want, 0o644)) - - windowsCrossDirCandidatesFn = func(string) []candidateSource { - return readWindowsCrossDirCandidates(v90xDir) - } - t.Cleanup(func() { - windowsCrossDirCandidatesFn = func(string) []candidateSource { return nil } - }) + require.NoError(t, os.WriteFile(filepath.Join(v90xDir, settingsFileName), + []byte(`{"user_id": 42, "user_level": "pro", "peer_share_enabled": true}`), 0o644)) + useCrossDir(t, v90xDir) - canonical := filepath.Join(tempDir, settingsFileName) - migrateLegacySettingsIfNeeded(tempDir, canonical) + initAt(t, tempDir) - got, err := os.ReadFile(canonical) - require.NoError(t, err) - assert.Equal(t, want, got, "v9.0.x cross-dir settings.json must be migrated when local.json absent") + assert.False(t, Exists(UserIDKey)) + assert.False(t, Exists(PeerShareEnabledKey)) }) t.Run("Windows v9.0.x cross-dir loses to same-dir v9.0.x local.json", func(t *testing.T) { - // If both exist, same-dir wins — it's strictly more recent state. - // (Cross-dir is only a thing because Windows users skipped through - // the v9.0.x → v9.1.x transition; same-dir local.json means the - // daemon already migrated once and the cross-dir copy is stale.) tempDir := t.TempDir() v90xDir := filepath.Join(tempDir, "windows-v90x") require.NoError(t, os.MkdirAll(v90xDir, 0o755)) require.NoError(t, os.WriteFile(filepath.Join(v90xDir, legacySettingsFileName), []byte(`{"user_id": 999, "user_level": "pro"}`), 0o644)) - sameDirPro := []byte(`{"user_id": 1, "user_level": "pro", "token": "same-dir"}`) - require.NoError(t, os.WriteFile(filepath.Join(tempDir, legacySettingsFileName), sameDirPro, 0o644)) + writeLegacy(t, tempDir, `{"user_id": 1, "user_level": "pro", "token": "same-dir"}`) + useCrossDir(t, v90xDir) - windowsCrossDirCandidatesFn = func(string) []candidateSource { - return readWindowsCrossDirCandidates(v90xDir) - } - t.Cleanup(func() { - windowsCrossDirCandidatesFn = func(string) []candidateSource { return nil } - }) - - canonical := filepath.Join(tempDir, settingsFileName) - migrateLegacySettingsIfNeeded(tempDir, canonical) + initAt(t, tempDir) - got, err := os.ReadFile(canonical) - require.NoError(t, err) - assert.Equal(t, sameDirPro, got, "same-dir v9.0.x must outrank cross-dir v9.0.x") + assert.Equal(t, "1", GetString(UserIDKey)) + assert.Equal(t, "same-dir", GetString(TokenKey)) }) t.Run("Windows v9.0.x cross-dir-pro beats v9.1.x nested-expired", func(t *testing.T) { - // The headline case: user upgrades from v9.0.x Windows (Pro state - // at ${PUBLIC}\Lantern\data\local.json) through a buggy v9.1.x - // build (which wrote an expired record at \Lantern\data\settings.json) - // to the fix. The cross-dir Pro must win over the nested expired. tempDir := t.TempDir() v90xDir := filepath.Join(tempDir, "windows-v90x") require.NoError(t, os.MkdirAll(v90xDir, 0o755)) - crossDirPro := []byte(`{"user_id": 195646669, "user_level": "pro"}`) - require.NoError(t, os.WriteFile(filepath.Join(v90xDir, legacySettingsFileName), crossDirPro, 0o644)) - writeNested(t, tempDir, []byte(`{"user_id": 99, "user_level": "expired"}`)) - - windowsCrossDirCandidatesFn = func(string) []candidateSource { - return readWindowsCrossDirCandidates(v90xDir) - } - t.Cleanup(func() { - windowsCrossDirCandidatesFn = func(string) []candidateSource { return nil } - }) + require.NoError(t, os.WriteFile(filepath.Join(v90xDir, legacySettingsFileName), + []byte(`{"user_id": 195646669, "user_level": "pro"}`), 0o644)) + writeNested(t, tempDir, `{"user_id": 99, "user_level": "expired"}`) + useCrossDir(t, v90xDir) - canonical := filepath.Join(tempDir, settingsFileName) - migrateLegacySettingsIfNeeded(tempDir, canonical) + initAt(t, tempDir) - got, err := os.ReadFile(canonical) - require.NoError(t, err) - assert.Equal(t, crossDirPro, got, "cross-dir pro must beat nested expired (engineering#3460)") + assert.Equal(t, "195646669", GetString(UserIDKey)) + assert.True(t, IsPro()) }) t.Run("unreadable canonical (non-ENOENT) skips migration", func(t *testing.T) { - // Permission error on the canonical path: don't fall through and - // overwrite a file we couldn't read. unix only — windows handles - // permissions differently and chmod wouldn't reproduce this. if runtime.GOOS == "windows" { t.Skip("permission semantics differ on windows") } + if os.Geteuid() == 0 { + t.Skip("root reads files regardless of mode") + } tempDir := t.TempDir() canonical := filepath.Join(tempDir, settingsFileName) - require.NoError(t, os.WriteFile(canonical, []byte(`{"user_level": "expired"}`), 0o644)) - // Make the file unreadable. + writeCanonical(t, tempDir, `{"user_level": "expired"}`) require.NoError(t, os.Chmod(canonical, 0o000)) t.Cleanup(func() { _ = os.Chmod(canonical, 0o644) }) - // Stage a legacy-pro candidate that would otherwise win. - writeLegacy(t, tempDir, []byte(`{"user_id": 1, "user_level": "pro"}`)) + writeLegacy(t, tempDir, `{"user_id": 1, "user_level": "pro"}`) - migrateLegacySettingsIfNeeded(tempDir, canonical) + Reset() + t.Cleanup(Reset) + assert.Error(t, InitSettings(tempDir)) - // Restore readability and confirm the canonical contents are - // unchanged (still the expired body, not the legacy-pro body). require.NoError(t, os.Chmod(canonical, 0o644)) got, err := os.ReadFile(canonical) require.NoError(t, err) assert.Equal(t, `{"user_level": "expired"}`, string(got), "canonical with non-ENOENT read error should be left alone") + assert.NoFileExists(t, filepath.Join(tempDir, migrationMarkerFileName)) + }) +} + +func TestDiff(t *testing.T) { + curr := Settings{ + DevicesKey: []any{map[string]any{"ID": "a"}}, + UserDataKey: nil, + LocaleKey: "en-US", + FeatureOverridesKey: map[string]any{"x": true}, + } + updates := Settings{ + DevicesKey: []any{map[string]any{"ID": "a"}}, + UserDataKey: nil, + LocaleKey: "fa-IR", + FeatureOverridesKey: map[string]any{"x": false}, + EmailKey: nil, + } + + diff := updates.Diff(curr) + + assert.Equal(t, Settings{ + LocaleKey: "fa-IR", + FeatureOverridesKey: map[string]any{"x": false}, + EmailKey: nil, + }, diff, "equal slices and nulls drop out; a null for an absent key stays") +} + +func TestGetAllForOmitsAbsentKeys(t *testing.T) { + isolateDiscovery(t) + initAt(t, t.TempDir()) + require.NoError(t, Set(UserDataKey, nil)) + + got := GetAllFor(UserDataKey, EmailKey) + + assert.Equal(t, Settings{UserDataKey: nil}, got) +} + +func TestSaveFailure(t *testing.T) { + isolateDiscovery(t) + + t.Run("Set and Patch take effect only once saved", func(t *testing.T) { + dir := t.TempDir() + initAt(t, dir) + require.NoError(t, Set(LocaleKey, "en-US")) + before, err := os.ReadFile(filepath.Join(dir, settingsFileName)) + require.NoError(t, err) + restore := moveAside(t, dir) + + assert.Error(t, Set(LocaleKey, "ru-RU")) + assert.Error(t, Patch(Settings{LocaleKey: "ru-RU", CountryCodeKey: "RU"})) + assert.Equal(t, "en-US", GetString(LocaleKey)) + assert.False(t, Exists(CountryCodeKey)) + restore() + after, err := os.ReadFile(filepath.Join(dir, settingsFileName)) + require.NoError(t, err) + assert.Equal(t, before, after, "the file keeps its previous contents") + assert.Equal(t, Settings{LocaleKey: "ru-RU"}, Settings{LocaleKey: "ru-RU"}.Diff(GetAllFor(LocaleKey)), + "a retry still sees the change") + + require.NoError(t, Patch(Settings{LocaleKey: "ru-RU", CountryCodeKey: "RU"})) + assert.Equal(t, "ru-RU", GetString(LocaleKey)) + assert.Equal(t, "RU", GetString(CountryCodeKey)) + }) + + t.Run("Apply takes effect even when the save fails", func(t *testing.T) { + dir := t.TempDir() + initAt(t, dir) + defer moveAside(t, dir)() + + assert.Error(t, Apply(Settings{DataPathKey: "/data"})) + + assert.Equal(t, "/data", GetString(DataPathKey)) + }) + + t.Run("Clear removes keys even when the save fails", func(t *testing.T) { + dir := t.TempDir() + initAt(t, dir) + require.NoError(t, Patch(Settings{TokenKey: "secret", UserIDKey: 1})) + defer moveAside(t, dir)() + + assert.Error(t, Clear(TokenKey, UserIDKey)) + + assert.False(t, Exists(TokenKey)) + assert.False(t, Exists(UserIDKey)) }) } +func TestPatchSkipsUnchangedWrites(t *testing.T) { + isolateDiscovery(t) + dir := t.TempDir() + initAt(t, dir) + path := filepath.Join(dir, settingsFileName) + stat := func() os.FileInfo { + t.Helper() + info, err := os.Stat(path) + require.NoError(t, err) + return info + } + initial := stat() + + require.NoError(t, Patch(Settings{LocaleKey: "en-US", CountryCodeKey: "US"})) + changed := stat() + require.NoError(t, Patch(Settings{LocaleKey: "en-US"})) + require.NoError(t, Set(CountryCodeKey, "US")) + + // atomicfile renames a new file into place, so a file with the same identity was never rewritten. + assert.False(t, os.SameFile(initial, changed), "a real change is written") + assert.True(t, os.SameFile(changed, stat()), "unchanged values are not rewritten") +} + +func TestPatchRewritesAFileChangedElsewhere(t *testing.T) { + isolateDiscovery(t) + dir := t.TempDir() + initAt(t, dir) + require.NoError(t, Set(LocaleKey, "en-US")) + path := filepath.Join(dir, settingsFileName) + require.NoError(t, os.WriteFile(path, []byte(`{"locale": "ru-RU"}`), 0o644)) + + require.NoError(t, Set(LocaleKey, "en-US")) + + require.NoError(t, Reload()) + assert.Equal(t, "en-US", GetString(LocaleKey)) +} + +func TestFailedInitKeepsChangesInMemory(t *testing.T) { + isolateDiscovery(t) + Reset() + t.Cleanup(Reset) + dir := t.TempDir() + readOnlyDir(t, dir) + require.Error(t, InitSettings(dir)) + + require.NoError(t, Set(LocaleKey, "en-US")) + + assert.Equal(t, "en-US", GetString(LocaleKey)) +} + func TestReload(t *testing.T) { + isolateDiscovery(t) initSettingsAt := func(t *testing.T, contents string) string { t.Helper() Reset() @@ -572,6 +830,7 @@ func TestReload(t *testing.T) { } func TestReloadRacesNothing(t *testing.T) { + isolateDiscovery(t) Reset() t.Cleanup(Reset) dir := t.TempDir() From ceb9c3a116ef7bdf331908c0178145c4cc0c216e Mon Sep 17 00:00:00 2001 From: garmr-ulfr Date: Wed, 14 Oct 2026 14:59:13 -0700 Subject: [PATCH 2/8] fix: apply settings changes only after they are saved Every radiance file lives in one data directory, so a failed save almost always means every save fails, and a restart would restore the old values anyway. Use one policy: a settings change takes effect only once it is saved. - settings.Clear stages its removal like Patch; on error the previous values remain. - settings.Apply, which keeps changes in memory when the save fails, is used only for startup inputs recomputed every run (paths, device ID, locale, time zone, telemetry consent). - Account operations return save errors instead of logging them: ClearUser now returns an error, and Login, Logout, DeleteAccount, OAuthLoginCallback, NewUser and FetchUserData pass it on. Logout and DeleteAccount say when the server side already succeeded. - An account switch is one Patch; Login folds its OAuth-flag reset into the login save. - A purchaser switch that fails to save keeps the old account and salt. - SignUp caches the new salt only after its identity is saved. --- account/subscription.go | 1 + account/subscription_test.go | 24 ++++++++-- account/user.go | 82 ++++++++++++++++++-------------- account/user_change_test.go | 8 ++-- account/user_refresh_test.go | 26 +++++----- account/user_test.go | 71 +++++++++++++++++++++++++-- common/settings/settings.go | 36 +++++++------- common/settings/settings_test.go | 14 ++++-- 8 files changed, 180 insertions(+), 82 deletions(-) diff --git a/account/subscription.go b/account/subscription.go index f0ee5beb..073723ca 100644 --- a/account/subscription.go +++ b/account/subscription.go @@ -164,6 +164,7 @@ func (a *Client) switchToPurchaser(ctx context.Context, generation uint64, id in } if err := a.storeIdentityLocked(id, token, extra); err != nil { a.logger.Error("saving the purchasing account", "error", traces.RecordError(ctx, err)) + return } if switching { // The salt cache isn't keyed by email, so the previous account's salt must go. diff --git a/account/subscription_test.go b/account/subscription_test.go index 8d37f5a2..240a9d7c 100644 --- a/account/subscription_test.go +++ b/account/subscription_test.go @@ -154,7 +154,7 @@ func TestRestoreSubscription(t *testing.T) { LegacyToken: "old-token", Token: "old-jwt", LegacyUserData: &protos.LoginResponse_UserData{UserId: 1, Email: "old@example.com", UserLevel: "pro"}, - }) + }, nil) setOAuthFlags(t) changes := make(chan struct{}, 4) sub := events.Subscribe(func(UserChangeEvent) { changes <- struct{}{} }) @@ -213,7 +213,7 @@ func TestNewStripeSubscriptionOmitsEmptyCouponCode(t *testing.T) { func TestVerifySwitchesToPurchaser(t *testing.T) { a, _ := newTestClient(t) - a.setData(&UserData{LegacyID: 1, LegacyToken: "old-token"}) + a.setData(&UserData{LegacyID: 1, LegacyToken: "old-token"}, nil) require.NoError(t, a.storeSalt([]byte("previous-salt"))) useProServer(t, a, serveJSON(VerifySubscriptionResponse{Status: "active", ActualUserID: 99, ActualUserToken: "purchaser"})) @@ -228,9 +228,23 @@ func TestVerifySwitchesToPurchaser(t *testing.T) { assert.Nil(t, saved) } +func TestPurchaseSwitchNotSavedKeepsAccount(t *testing.T) { + a, _ := newTestClient(t) + require.NoError(t, a.setData(&UserData{LegacyID: 1, LegacyToken: "old-token"}, nil)) + require.NoError(t, a.storeSalt([]byte("previous-salt"))) + useProServer(t, a, serveJSON(VerifySubscriptionResponse{Status: "active", ActualUserID: 99, ActualUserToken: "purchaser"})) + defer failSettingsSaves(t)() + + _, err := a.VerifySubscription(context.Background(), AppleService, map[string]string{"receipt": "r"}) + + assert.NoError(t, err, "the purchase itself succeeded") + assert.Equal(t, int64(1), settings.GetInt64(settings.UserIDKey)) + assert.Equal(t, []byte("previous-salt"), a.getSaltCached()) +} + func TestLatePurchaseResponseDoesNotSwitch(t *testing.T) { a, _ := newTestClient(t) - a.setData(&UserData{LegacyID: 1, LegacyToken: "a"}) + a.setData(&UserData{LegacyID: 1, LegacyToken: "a"}, nil) handler, requested, release := heldJSON(VerifySubscriptionResponse{Status: "active", ActualUserID: 99, ActualUserToken: "purchaser"}) useProServer(t, a, handler) @@ -240,7 +254,7 @@ func TestLatePurchaseResponseDoesNotSwitch(t *testing.T) { errc <- err }() <-requested - a.setData(&UserData{LegacyID: 2, LegacyToken: "b"}) + a.setData(&UserData{LegacyID: 2, LegacyToken: "b"}, nil) close(release) assert.NoError(t, <-errc, "the purchase itself succeeded") @@ -250,7 +264,7 @@ func TestLatePurchaseResponseDoesNotSwitch(t *testing.T) { func TestSameAccountPurchaseKeepsSaltAndOAuth(t *testing.T) { a, _ := newTestClient(t) - a.setData(&UserData{LegacyID: 99, LegacyToken: "old-token"}) + a.setData(&UserData{LegacyID: 99, LegacyToken: "old-token"}, nil) require.NoError(t, a.storeSalt([]byte("my-salt"))) setOAuthFlags(t) useProServer(t, a, serveJSON(VerifySubscriptionResponse{Status: "active", ActualUserID: 99, ActualUserToken: "new-token"})) diff --git a/account/user.go b/account/user.go index 8ac6e2a8..2cecda38 100644 --- a/account/user.go +++ b/account/user.go @@ -105,7 +105,11 @@ func (a *Client) storeData(ctx context.Context, resp UserDataResponse, generatio LegacyToken: resp.Token, LegacyUserData: resp.LoginResponse_UserData, } - return a.setDataLocked(login, true), nil + data, err := a.setDataLocked(login, true, nil) + if err != nil { + return nil, traces.RecordError(ctx, fmt.Errorf("saving user data: %w", err)) + } + return data, nil } type DataCapInfo struct { @@ -186,7 +190,6 @@ func (a *Client) SignUp(ctx context.Context, email, password string) ([]byte, *p if err != nil { return nil, nil, traces.RecordError(ctx, err) } - a.setSalt(salt) var signupData protos.SignupResponse if err := proto.Unmarshal(resp, &signupData); err != nil { @@ -199,6 +202,7 @@ func (a *Client) SignUp(ctx context.Context, email, password string) ([]byte, *p }); err != nil { return nil, nil, traces.RecordError(ctx, fmt.Errorf("saving signup identity: %w", err)) } + a.setSalt(salt) return salt, &signupData, nil } @@ -294,13 +298,13 @@ func (a *Client) Login(ctx context.Context, email, password string) (*UserData, // regardless of state we need to save login information // We have device flow limit on login - a.setData(&loginResp) + oauth := settings.Settings{settings.OAuthLoginKey: false, settings.OAuthProviderKey: ""} + if err := a.setData(&loginResp, oauth); err != nil { + return nil, traces.RecordError(ctx, fmt.Errorf("saving login: %w", err)) + } if saltErr := a.storeSalt(salt); saltErr != nil { return nil, traces.RecordError(ctx, saltErr) } - if err := settings.Clear(settings.OAuthLoginKey, settings.OAuthProviderKey); err != nil { - a.logger.Warn("failed to clear OAuth login", "error", err) - } return &loginResp, nil } @@ -324,7 +328,9 @@ func (a *Client) Logout(ctx context.Context, email string) (*UserData, error) { if err != nil { return nil, traces.RecordError(ctx, fmt.Errorf("logging out: %w", err)) } - a.ClearUser() + if err := a.ClearUser(); err != nil { + return nil, traces.RecordError(ctx, fmt.Errorf("logged out on the server, but clearing the local account failed: %w", err)) + } if err := a.storeSalt(nil); err != nil { return nil, traces.RecordError(ctx, fmt.Errorf("writing salt after logout: %w", err)) } @@ -513,7 +519,9 @@ func (a *Client) DeleteAccount(ctx context.Context, email, password string) (*Us return nil, traces.RecordError(ctx, err) } - a.ClearUser() + if err := a.ClearUser(); err != nil { + return nil, traces.RecordError(ctx, fmt.Errorf("account deleted on the server, but clearing the local account failed: %w", err)) + } if err := a.storeSalt(nil); err != nil { return nil, traces.RecordError(ctx, fmt.Errorf("failed to write salt during account deletion cleanup: %w", err)) } @@ -562,7 +570,9 @@ func (a *Client) OAuthLoginCallback(ctx context.Context, oAuthToken string) (*Us Email: jwtUserInfo.Email, }, } - a.setData(login) + if err := a.setData(login, nil); err != nil { + return nil, fmt.Errorf("saving OAuth identity: %w", err) + } user, err := a.fetchUserData(ctx) if err != nil { return nil, fmt.Errorf("error getting user data: %w", err) @@ -577,7 +587,9 @@ func (a *Client) OAuthLoginCallback(ctx context.Context, oAuthToken string) (*Us } user.Id = jwtUserInfo.Email user.EmailConfirmed = true - a.setData(user) + if err := a.setData(user, nil); err != nil { + return nil, fmt.Errorf("saving OAuth user data: %w", err) + } return user, nil } @@ -695,12 +707,11 @@ func (a *Client) storeIdentity(id int64, token string, extra settings.Settings) return a.storeIdentityLocked(id, token, extra) } -// storeIdentityLocked saves id and token, each only if set, together with extra in one write, and -// emits a [UserChangeEvent] if the [accountState] changed. A different nonzero id clears the -// previous account's credentials and cached data, and the whole write then applies in memory even -// if the save fails, so the previous credentials never outlive the switch; otherwise it applies -// only once saved. The OAuth flags are kept, because an OAuth sign-in records its provider before -// switching. It requires a.mu to be held. +// storeIdentityLocked saves id and token, each only if set, together with extra in one write that +// takes effect only once saved, and emits a [UserChangeEvent] if the [accountState] changed. A +// different nonzero id also clears the previous account's credentials and cached data. The OAuth +// flags are kept, because an OAuth sign-in records its provider before switching. It requires a.mu +// to be held. func (a *Client) storeIdentityLocked(id int64, token string, extra settings.Settings) error { before := currentAccount() defer func() { @@ -727,14 +738,10 @@ func (a *Client) storeIdentityLocked(id int64, token string, extra settings.Sett if token != "" { updates[settings.TokenKey] = token } - switch { - case len(updates) == 0: + if len(updates) == 0 { return nil - case switching: - return settings.Apply(updates) - default: - return settings.Patch(updates) } + return settings.Patch(updates) } // accountState holds the settings whose change is reported as a [UserChangeEvent]. @@ -753,22 +760,23 @@ func currentAccount() accountState { } } -func (a *Client) setData(data *UserData) { +// setData stores data together with extra, or clears the account when data is nil. +func (a *Client) setData(data *UserData, extra settings.Settings) error { if data == nil { - a.ClearUser() - return + return a.ClearUser() } a.mu.Lock() defer a.mu.Unlock() a.generation++ - a.setDataLocked(data, false) + _, err := a.setDataLocked(data, false, extra) + return err } -// setDataLocked stores a copy of data, merged with the cached response of the same account when -// preserveLoginFields is set. Data without user data stores only its id, token and devices. It -// logs rather than returns save failures, and returns data, possibly merged, which the caller may -// modify without affecting settings. It requires a.mu to be held and data to be non-nil. -func (a *Client) setDataLocked(data *UserData, preserveLoginFields bool) *UserData { +// setDataLocked stores a copy of data together with extra, first merging data with the cached +// response of the same account when preserveLoginFields is set. Data without user data stores only its id, token +// and devices. It returns data, possibly merged, which the caller may modify without affecting +// settings. It requires a.mu to be held and data to be non-nil. +func (a *Client) setDataLocked(data *UserData, preserveLoginFields bool, extra settings.Settings) (*UserData, error) { updates := settings.Settings{} // A device-limited login carries no user data, but its device list is what the user needs to // pick a device to remove. @@ -807,22 +815,24 @@ func (a *Client) setDataLocked(data *UserData, preserveLoginFields bool) *UserDa } updates[settings.UserDataKey] = proto.Clone(data) } + maps.Copy(updates, extra) if err := a.storeIdentityLocked(data.LegacyID, data.LegacyToken, updates); err != nil { - a.logger.Error("failed to store account data", "error", err) + return nil, err } - return data + return data, nil } -// ClearUser removes the cached account and its credentials. -func (a *Client) ClearUser() { +// ClearUser removes the cached account and its credentials. On error the account remains. +func (a *Client) ClearUser() error { a.mu.Lock() defer a.mu.Unlock() a.generation++ hadIdentity := settings.GetInt64(settings.UserIDKey) != 0 || settings.GetString(settings.TokenKey) != "" if err := settings.Clear(settings.UserKeys...); err != nil { - a.logger.Warn("failed to clear user info", "error", err) + return fmt.Errorf("clearing user info: %w", err) } if hadIdentity { events.Emit(UserChangeEvent{}) } + return nil } diff --git a/account/user_change_test.go b/account/user_change_test.go index 714f6c53..0e6bd7f7 100644 --- a/account/user_change_test.go +++ b/account/user_change_test.go @@ -40,13 +40,13 @@ func TestAccountChangesNotifyWithoutBackendWrappers(t *testing.T) { _, err = a.FetchUserData(ctx) require.NoError(t, err) expect(0) - a.setData(&UserData{LegacyUserData: &protos.LoginResponse_UserData{UserLevel: "pro"}}) + a.setData(&UserData{LegacyUserData: &protos.LoginResponse_UserData{UserLevel: "pro"}}, nil) expect(1) - a.setData(&UserData{LegacyUserData: &protos.LoginResponse_UserData{UserLevel: "pro"}}) + a.setData(&UserData{LegacyUserData: &protos.LoginResponse_UserData{UserLevel: "pro"}}, nil) expect(0) - a.setData(&UserData{LegacyID: 456, LegacyToken: "new-token"}) + a.setData(&UserData{LegacyID: 456, LegacyToken: "new-token"}, nil) expect(1) - a.setData(&UserData{LegacyID: 456, LegacyToken: "new-token"}) + a.setData(&UserData{LegacyID: 456, LegacyToken: "new-token"}, nil) expect(0) _, err = a.Logout(ctx, "test@example.com") require.NoError(t, err) diff --git a/account/user_refresh_test.go b/account/user_refresh_test.go index 602e082f..1637beec 100644 --- a/account/user_refresh_test.go +++ b/account/user_refresh_test.go @@ -92,7 +92,7 @@ func TestFetchUserDataWithoutMatchingLogin(t *testing.T) { LegacyUserData: &protos.LoginResponse_UserData{ UserId: tt.cachedID, Email: "old@example.com", UserLevel: "pro", }, - }) + }, nil) } got, err := ac.FetchUserData(context.Background()) require.NoError(t, err) @@ -122,7 +122,7 @@ func TestFetchUserDataWithoutToken(t *testing.T) { ac.setData(&UserData{ LegacyID: 123, LegacyToken: "cached-token", Id: "account-id", LegacyUserData: &protos.LoginResponse_UserData{UserId: 123, Token: "cached-token"}, - }) + }, nil) require.NoError(t, settings.Set(settings.TokenKey, "current-token")) server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { writeJSONResponse(w, UserDataResponse{ @@ -158,7 +158,7 @@ func TestSignupSwitchesAccountBeforeRefresh(t *testing.T) { LegacyID: 456, LegacyToken: "old-token", Token: "old-jwt", Devices: []*protos.LoginResponse_Device{{Id: "old-device"}}, LegacyUserData: &protos.LoginResponse_UserData{UserId: 456}, - }) + }, nil) server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { writeProtoResponse(w, &protos.SignupResponse{LegacyID: 123, ProToken: "signup-token", Token: "signup-jwt"}) })) @@ -184,7 +184,7 @@ func TestLoginReplacesCachedFields(t *testing.T) { Token: "previous-jwt", Devices: []*protos.LoginResponse_Device{{Id: "old-device"}}, LegacyUserData: &protos.LoginResponse_UserData{UserId: 123}, - }) + }, nil) server.loginResponse = &UserData{ LegacyID: 123, LegacyUserData: &protos.LoginResponse_UserData{UserId: 123}, @@ -235,7 +235,7 @@ func TestSetDataNilClearsCachedUser(t *testing.T) { ac, _ := newTestClient(t) _, err := ac.NewUser(context.Background()) require.NoError(t, err) - ac.setData(nil) + ac.setData(nil, nil) assert.False(t, settings.Exists(settings.UserDataKey)) assert.Empty(t, settings.GetString(settings.TokenKey)) } @@ -247,7 +247,7 @@ func TestFetchUserDataDoesNotReplayLoginSettings(t *testing.T) { Token: "old-jwt", Devices: []*protos.LoginResponse_Device{{Id: "old-device"}}, LegacyUserData: &protos.LoginResponse_UserData{UserId: 123}, - }) + }, nil) devices := []settings.Device{{ID: "new-device", Name: "Laptop"}} require.NoError(t, settings.Patch(settings.Settings{ settings.JwtTokenKey: "new-jwt", @@ -276,17 +276,17 @@ func TestStaleUserDataIsDiscarded(t *testing.T) { change func(a *Client) }{ {"A to B", func(a *Client) { - a.setData(&UserData{LegacyID: 2, LegacyToken: "b"}) + a.setData(&UserData{LegacyID: 2, LegacyToken: "b"}, nil) }}, {"A to B to A", func(a *Client) { - a.setData(&UserData{LegacyID: 2, LegacyToken: "b"}) - a.setData(&UserData{LegacyID: 1, LegacyToken: "a"}) + a.setData(&UserData{LegacyID: 2, LegacyToken: "b"}, nil) + a.setData(&UserData{LegacyID: 1, LegacyToken: "a"}, nil) }}, {"logout", func(a *Client) { a.ClearUser() }}, } { t.Run(tt.name, func(t *testing.T) { a, _ := newTestClient(t) - a.setData(&UserData{LegacyID: 1, LegacyToken: "a"}) + a.setData(&UserData{LegacyID: 1, LegacyToken: "a"}, nil) handler, requested, release := heldJSON(UserDataResponse{ LoginResponse_UserData: &protos.LoginResponse_UserData{UserId: 1, Token: "a", UserLevel: "pro"}, }) @@ -311,7 +311,7 @@ func TestStaleUserDataIsDiscarded(t *testing.T) { func TestNewUserDiscardsInFlightUserData(t *testing.T) { a, _ := newTestClient(t) - a.setData(&UserData{LegacyID: 1, LegacyToken: "a"}) + a.setData(&UserData{LegacyID: 1, LegacyToken: "a"}, nil) userData, requested, release := heldJSON(UserDataResponse{ LoginResponse_UserData: &protos.LoginResponse_UserData{UserId: 1, Token: "a"}, }) @@ -352,7 +352,7 @@ func TestReturnedUserDataIsACopy(t *testing.T) { func TestAccountSwitchNeverExposesAnEmptyIdentity(t *testing.T) { a, _ := newTestClient(t) - a.setData(&UserData{LegacyID: 1, LegacyToken: "a"}) + a.setData(&UserData{LegacyID: 1, LegacyToken: "a"}, nil) stop := make(chan struct{}) sawEmpty := make(chan bool, 1) go func() { @@ -368,7 +368,7 @@ func TestAccountSwitchNeverExposesAnEmptyIdentity(t *testing.T) { } }() for i := range 50 { - a.setData(&UserData{LegacyID: int64(2 + i%2), LegacyToken: "t"}) + a.setData(&UserData{LegacyID: int64(2 + i%2), LegacyToken: "t"}, nil) } close(stop) assert.False(t, <-sawEmpty, "readers must never see an account without an id or token") diff --git a/account/user_test.go b/account/user_test.go index 691b273b..721e45ed 100644 --- a/account/user_test.go +++ b/account/user_test.go @@ -294,7 +294,9 @@ func newTestClient(t *testing.T) (*Client, *testServer) { t.Setenv(v, t.TempDir()) } settings.Reset() - require.NoError(t, settings.InitSettings(t.TempDir())) + dataDir := t.TempDir() + require.NoError(t, settings.InitSettings(dataDir)) + require.NoError(t, settings.Set(settings.DataPathKey, dataDir)) t.Cleanup(settings.Reset) return &Client{ httpClient: ts.Client(), @@ -305,6 +307,15 @@ func newTestClient(t *testing.T) (*Client, *testServer) { }, state } +// failSettingsSaves makes settings saves fail until the returned func restores the data directory. +func failSettingsSaves(t *testing.T) (restore func()) { + t.Helper() + dir := settings.GetString(settings.DataPathKey) + aside := dir + ".aside" + require.NoError(t, os.Rename(dir, aside)) + return func() { require.NoError(t, os.Rename(aside, dir)) } +} + // newTestClientWithSRP creates a test client and pre-registers an email/password on the mock server. func newTestClientWithSRP(t *testing.T, email, password string) (*Client, *testServer) { ac, state := newTestClient(t) @@ -366,6 +377,18 @@ func TestSignUp(t *testing.T) { assert.NotNil(t, signupResponse) } +func TestSignUpKeepsSaltWhenSaveFails(t *testing.T) { + ac, _ := newTestClient(t) + require.NoError(t, settings.Patch(settings.Settings{settings.TokenKey: "t", settings.UserIDKey: "123"})) + ac.salt = []byte("account-salt") + defer failSettingsSaves(t)() + + _, _, err := ac.SignUp(context.Background(), "new@example.com", "password") + + assert.ErrorContains(t, err, "saving signup identity") + assert.Equal(t, []byte("account-salt"), ac.getSaltCached()) +} + func TestSignupEmailResendCode(t *testing.T) { ac, _ := newTestClient(t) ac.salt = []byte("salt") @@ -404,7 +427,7 @@ func TestDeviceLimitedLoginStoresDevices(t *testing.T) { t.Run(tt.name, func(t *testing.T) { a, server := newTestClientWithSRP(t, "test@example.com", "password") if tt.existing != nil { - a.setData(tt.existing) + a.setData(tt.existing, nil) } userDataBefore := settings.Exists(settings.UserDataKey) server.loginResponse = &UserData{ @@ -432,6 +455,36 @@ func TestLogout(t *testing.T) { assert.NoError(t, err) } +func TestLogoutKeepsAccountWhenSaveFails(t *testing.T) { + ac, _ := newTestClient(t) + require.NoError(t, ac.setData(&UserData{LegacyID: 7, LegacyToken: "tok"}, nil)) + defer failSettingsSaves(t)() + + _, err := ac.Logout(context.Background(), "test@example.com") + + assert.ErrorContains(t, err, "logged out on the server") + assert.Equal(t, int64(7), settings.GetInt64(settings.UserIDKey)) + assert.Equal(t, "tok", settings.GetString(settings.TokenKey)) +} + +func TestLoginKeepsAccountWhenSaveFails(t *testing.T) { + ac, server := newTestClientWithSRP(t, "test@example.com", "password") + require.NoError(t, ac.setData(&UserData{LegacyID: 7, LegacyToken: "tok"}, nil)) + server.loginResponse = &UserData{ + LegacyID: 8, + LegacyToken: "new", + LegacyUserData: &protos.LoginResponse_UserData{UserId: 8, UserLevel: "pro"}, + } + defer failSettingsSaves(t)() + + _, err := ac.Login(context.Background(), "test@example.com", "password") + + assert.ErrorContains(t, err, "saving login") + assert.Equal(t, int64(7), settings.GetInt64(settings.UserIDKey)) + assert.Equal(t, "tok", settings.GetString(settings.TokenKey)) + assert.False(t, settings.IsPro()) +} + func TestLogoutOmitsCredentials(t *testing.T) { const ( legacyToken = "legacy-token-value" @@ -554,6 +607,18 @@ func TestDeleteAccount(t *testing.T) { assert.NoError(t, err) } +func TestDeleteAccountKeepsAccountWhenSaveFails(t *testing.T) { + email := "test@example.com" + ac, _ := newTestClientWithSRP(t, email, "password") + require.NoError(t, ac.setData(&UserData{LegacyID: 7, LegacyToken: "tok"}, nil)) + defer failSettingsSaves(t)() + + _, err := ac.DeleteAccount(context.Background(), email, "password") + + assert.ErrorContains(t, err, "account deleted on the server") + assert.Equal(t, int64(7), settings.GetInt64(settings.UserIDKey)) +} + func TestOAuthLoginUrl(t *testing.T) { ac, _ := newTestClient(t) url, err := ac.OAuthLoginURL(context.Background(), "google") @@ -568,7 +633,7 @@ func TestOAuthLoginCallback(t *testing.T) { LegacyID: 456, Token: "old-jwt", Devices: []*protos.LoginResponse_Device{{Id: "old-device"}}, LegacyUserData: &protos.LoginResponse_UserData{UserId: 456}, - }) + }, nil) // Mock JWT with unverified signature — decodeJWT uses ParseUnverified so this succeeds. mockToken := "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJlbWFpbCI6InRlc3RAZXhhbXBsZS5jb20iLCJsZWdhY3lfdXNlcl9pZCI6MTIzNDUsImxlZ2FjeV90b2tlbiI6InRlc3QtdG9rZW4ifQ.test" diff --git a/common/settings/settings.go b/common/settings/settings.go index 74999aa8..c3c823b1 100644 --- a/common/settings/settings.go +++ b/common/settings/settings.go @@ -420,15 +420,16 @@ func Set(key _key, value any) error { return Patch(Settings{key: value}) } -// Clear removes keys and saves the settings. Unlike [Set] and [Patch], the keys stay removed in -// memory even when the save fails; the save error is still returned. +// Clear removes keys. On error the previous values remain. func Clear(keys ..._key) error { k.mu.Lock() defer k.mu.Unlock() - for _, key := range keys { - k.k.Delete(key.String()) - } - return saveLocked(k.k) + return updateLocked(func(kk *koanf.Koanf) error { + for _, key := range keys { + kk.Delete(key.String()) + } + return nil + }) } type Settings map[_key]any @@ -471,27 +472,28 @@ func GetAllFor(keys ..._key) Settings { func Patch(updates Settings) error { k.mu.Lock() defer k.mu.Unlock() - return updateLocked(func(kk *koanf.Koanf) error { return setAll(kk, updates) }) + return updateLocked(func(kk *koanf.Koanf) error { + for key, value := range updates { + if err := kk.Set(key.String(), value); err != nil { + return fmt.Errorf("could not set key %s: %w", key, err) + } + } + return nil + }) } // Apply is like [Patch], except the updates take effect in memory even when the save fails; the -// save error is still returned. +// save error is still returned. It is for inputs the process recomputes on every start, such as +// paths and the host's telemetry consent, which must hold for the current run regardless. func Apply(updates Settings) error { k.mu.Lock() defer k.mu.Unlock() - if err := setAll(k.k, updates); err != nil { - return err - } - return saveLocked(k.k) -} - -func setAll(kk *koanf.Koanf, updates Settings) error { for key, value := range updates { - if err := kk.Set(key.String(), value); err != nil { + if err := k.k.Set(key.String(), value); err != nil { return fmt.Errorf("could not set key %s: %w", key, err) } } - return nil + return saveLocked(k.k) } // updateLocked runs mutate on a copy of the settings, saves the copy, and only then makes it the diff --git a/common/settings/settings_test.go b/common/settings/settings_test.go index 3850d27c..3f183d6d 100644 --- a/common/settings/settings_test.go +++ b/common/settings/settings_test.go @@ -691,21 +691,27 @@ func TestSaveFailure(t *testing.T) { t.Run("Apply takes effect even when the save fails", func(t *testing.T) { dir := t.TempDir() initAt(t, dir) + require.NoError(t, Patch(Settings{TelemetryKey: true})) defer moveAside(t, dir)() - assert.Error(t, Apply(Settings{DataPathKey: "/data"})) + assert.Error(t, Apply(Settings{TelemetryKey: false, LocaleKey: "en-US"})) - assert.Equal(t, "/data", GetString(DataPathKey)) + assert.False(t, GetBool(TelemetryKey)) + assert.Equal(t, "en-US", GetString(LocaleKey)) }) - t.Run("Clear removes keys even when the save fails", func(t *testing.T) { + t.Run("Clear takes effect only once saved", func(t *testing.T) { dir := t.TempDir() initAt(t, dir) require.NoError(t, Patch(Settings{TokenKey: "secret", UserIDKey: 1})) - defer moveAside(t, dir)() + restore := moveAside(t, dir) assert.Error(t, Clear(TokenKey, UserIDKey)) + assert.Equal(t, "secret", GetString(TokenKey)) + assert.Equal(t, "1", GetString(UserIDKey)) + restore() + require.NoError(t, Clear(TokenKey, UserIDKey)) assert.False(t, Exists(TokenKey)) assert.False(t, Exists(UserIDKey)) }) From 8e458f7b20a698f1765ed3943fa563e8ee953f82 Mon Sep 17 00:00:00 2001 From: garmr-ulfr Date: Wed, 14 Oct 2026 15:39:53 -0700 Subject: [PATCH 3/8] fix: close account races around identity and salt writes - Route PatchSettings patches that touch user keys through the new account.Client.PatchAccountSettings, which saves them as an account change so pending user-data and purchase responses for the previous account are discarded. - SignUp saves its identity and caches its salt in one locked section, so a concurrent account switch can't receive the signup salt. - Password recovery holds the account lock across the current-email check and the salt write. --- account/user.go | 23 +++++++++++++++++++---- account/user_refresh_test.go | 3 +++ backend/radiance.go | 14 +++++++++++--- backend/user_messages_test.go | 32 ++++++++++++++++++++++++++++++++ 4 files changed, 65 insertions(+), 7 deletions(-) diff --git a/account/user.go b/account/user.go index 2cecda38..7a1fc85c 100644 --- a/account/user.go +++ b/account/user.go @@ -195,14 +195,20 @@ func (a *Client) SignUp(ctx context.Context, email, password string) ([]byte, *p if err := proto.Unmarshal(resp, &signupData); err != nil { return nil, nil, traces.RecordError(ctx, fmt.Errorf("error unmarshalling sign up response: %w", err)) } - if err := a.storeIdentity(signupData.LegacyID, signupData.ProToken, settings.Settings{ + a.mu.Lock() + a.generation++ + err = a.storeIdentityLocked(signupData.LegacyID, signupData.ProToken, settings.Settings{ settings.JwtTokenKey: signupData.Token, settings.OAuthLoginKey: false, settings.OAuthProviderKey: "", - }); err != nil { + }) + if err == nil { + a.salt = salt + } + a.mu.Unlock() + if err != nil { return nil, nil, traces.RecordError(ctx, fmt.Errorf("saving signup identity: %w", err)) } - a.setSalt(salt) return salt, &signupData, nil } @@ -375,9 +381,11 @@ func (a *Client) CompleteRecoveryByEmail(ctx context.Context, email, newPassword if err != nil { return traces.RecordError(ctx, fmt.Errorf("failed to complete recovery by email: %w", err)) } + a.mu.Lock() + defer a.mu.Unlock() // The salt cache isn't keyed by email, so only the current account's new salt belongs in it. if strings.EqualFold(settings.GetString(settings.EmailKey), lowerCaseEmail) { - if err = a.storeSalt(newSalt); err != nil { + if err = a.storeSaltLocked(newSalt); err != nil { return traces.RecordError(ctx, fmt.Errorf("failed to write new salt: %w", err)) } } @@ -707,6 +715,13 @@ func (a *Client) storeIdentity(id int64, token string, extra settings.Settings) return a.storeIdentityLocked(id, token, extra) } +// PatchAccountSettings saves updates as a change of the active account. Pending user-data and +// purchase responses requested before the call are discarded, and a [UserChangeEvent] is emitted +// if the account state changed. +func (a *Client) PatchAccountSettings(updates settings.Settings) error { + return a.storeIdentity(0, "", updates) +} + // storeIdentityLocked saves id and token, each only if set, together with extra in one write that // takes effect only once saved, and emits a [UserChangeEvent] if the [accountState] changed. A // different nonzero id also clears the previous account's credentials and cached data. The OAuth diff --git a/account/user_refresh_test.go b/account/user_refresh_test.go index 1637beec..d2be92eb 100644 --- a/account/user_refresh_test.go +++ b/account/user_refresh_test.go @@ -283,6 +283,9 @@ func TestStaleUserDataIsDiscarded(t *testing.T) { a.setData(&UserData{LegacyID: 1, LegacyToken: "a"}, nil) }}, {"logout", func(a *Client) { a.ClearUser() }}, + {"settings patch", func(a *Client) { + a.PatchAccountSettings(settings.Settings{settings.UserIDKey: int64(2), settings.TokenKey: "b"}) + }}, } { t.Run(tt.name, func(t *testing.T) { a, _ := newTestClient(t) diff --git a/backend/radiance.go b/backend/radiance.go index 935a9704..8f24fbac 100644 --- a/backend/radiance.go +++ b/backend/radiance.go @@ -720,7 +720,15 @@ func (r *LocalBackend) PatchSettings(updates settings.Settings) error { if len(diff) == 0 { return errs } - if err := settings.Patch(diff); err != nil { + patch := settings.Patch + for _, key := range settings.UserKeys { + if _, ok := diff[key]; ok { + // settings.Patch would let a stale user-data or purchase response overwrite this change. + patch = r.accountClient.PatchAccountSettings + break + } + } + if err := patch(diff); err != nil { return errors.Join(errs, fmt.Errorf("failed to update settings: %w", err)) } if _, ok := diff[settings.LocaleKey]; ok { @@ -739,7 +747,7 @@ func (r *LocalBackend) PatchSettings(updates settings.Settings) error { // vpn settings // - // settings.Patch above already persisted the whole diff, so an early return + // The patch above already persisted the whole diff, so an early return // on a handler error would leave a persisted key that no runtime state // matches — the divergence applyPeerShare's rollback exists to prevent. Run // every handler and join their errors so the caller sees write failures. @@ -758,7 +766,7 @@ func (r *LocalBackend) PatchSettings(updates settings.Settings) error { } // Drive the Unbounded widget proxy off the toggle change immediately // rather than waiting for the next NewConfigEvent to re-evaluate. - // settings.Patch above has already persisted the new value, so go + // The patch above has already persisted the new value, so go // straight to Apply() — SetEnabled would short-circuit on the // already-matching persisted value and never re-evaluate the // manager. Apply re-checks the three-condition predicate against diff --git a/backend/user_messages_test.go b/backend/user_messages_test.go index 0b44f24a..3a1edcd0 100644 --- a/backend/user_messages_test.go +++ b/backend/user_messages_test.go @@ -30,6 +30,38 @@ type accountTransportFunc func(*http.Request) (*http.Response, error) func (f accountTransportFunc) RoundTrip(req *http.Request) (*http.Response, error) { return f(req) } +func TestPatchSettingsAccountKeysDiscardStaleUserData(t *testing.T) { + settings.Reset() + t.Cleanup(settings.Reset) + require.NoError(t, settings.InitSettings(t.TempDir())) + require.NoError(t, settings.Patch(settings.Settings{settings.UserIDKey: int64(1), settings.TokenKey: "a"})) + requested, release := make(chan struct{}), make(chan struct{}) + a := account.NewClient(&http.Client{Transport: accountTransportFunc(func(req *http.Request) (*http.Response, error) { + close(requested) + <-release + return &http.Response{ + StatusCode: http.StatusOK, + Header: make(http.Header), + Body: io.NopCloser(strings.NewReader(`{"userId":1,"token":"a","userLevel":"pro"}`)), + Request: req, + }, nil + })}, t.TempDir(), log.NoOpLogger()) + r := &LocalBackend{accountClient: a} + errc := make(chan error, 1) + go func() { + _, err := a.FetchUserData(context.Background()) + errc <- err + }() + <-requested + + require.NoError(t, r.PatchSettings(settings.Settings{settings.UserIDKey: int64(2), settings.TokenKey: "b"})) + close(release) + + require.ErrorIs(t, <-errc, account.ErrAccountChanged) + require.Equal(t, int64(2), settings.GetInt64(settings.UserIDKey)) + require.False(t, settings.IsPro()) +} + func TestUserMessagesFollowAccountEvents(t *testing.T) { settings.Reset() t.Cleanup(settings.Reset) From ec155680323ba8210a49d68f403a1664f24628dc Mon Sep 17 00:00:00 2001 From: garmr-ulfr Date: Thu, 15 Oct 2026 10:05:20 -0700 Subject: [PATCH 4/8] fix: commit OAuth sign-in and account patches atomically OAuthLoginCallback fetches user data with the token's credentials and saves the account in one write, discarding it if another account change landed meanwhile. PatchAccountSettings detects a switch, clears the previous account, skips unchanged values, and rejects invalid user IDs. CompleteChangeEmail saves only to the account that requested it. --- account/subscription.go | 7 -- account/subscription_test.go | 3 + account/user.go | 189 +++++++++++++++++++++++---------- account/user_change_test.go | 7 ++ account/user_refresh_test.go | 147 ++++++++++++++++++++++++++ account/user_test.go | 193 +++++++++++++++++++++++++++++++++- backend/radiance.go | 12 +-- backend/user_messages_test.go | 61 +++++++++++ ipc/server.go | 6 +- 9 files changed, 555 insertions(+), 70 deletions(-) diff --git a/account/subscription.go b/account/subscription.go index 073723ca..90055dc4 100644 --- a/account/subscription.go +++ b/account/subscription.go @@ -164,13 +164,6 @@ func (a *Client) switchToPurchaser(ctx context.Context, generation uint64, id in } if err := a.storeIdentityLocked(id, token, extra); err != nil { a.logger.Error("saving the purchasing account", "error", traces.RecordError(ctx, err)) - return - } - if switching { - // The salt cache isn't keyed by email, so the previous account's salt must go. - if err := a.storeSaltLocked(nil); err != nil { - a.logger.Error("clearing the previous account's salt", "error", traces.RecordError(ctx, err)) - } } } diff --git a/account/subscription_test.go b/account/subscription_test.go index 240a9d7c..f7a09c77 100644 --- a/account/subscription_test.go +++ b/account/subscription_test.go @@ -240,6 +240,9 @@ func TestPurchaseSwitchNotSavedKeepsAccount(t *testing.T) { assert.NoError(t, err, "the purchase itself succeeded") assert.Equal(t, int64(1), settings.GetInt64(settings.UserIDKey)) assert.Equal(t, []byte("previous-salt"), a.getSaltCached()) + salt, err := readSalt(a.saltPath) + require.NoError(t, err) + assert.Nil(t, salt) } func TestLatePurchaseResponseDoesNotSwitch(t *testing.T) { diff --git a/account/user.go b/account/user.go index 7a1fc85c..0cf06d46 100644 --- a/account/user.go +++ b/account/user.go @@ -7,7 +7,10 @@ import ( "fmt" "io/fs" "maps" + "math" "net/url" + "slices" + "strconv" "strings" "go.opentelemetry.io/otel" @@ -47,33 +50,35 @@ func (a *Client) NewUser(ctx context.Context) (*UserData, error) { if err := json.Unmarshal(resp, &userResp); err != nil { return nil, traces.RecordError(ctx, fmt.Errorf("error unmarshalling new user response: %w", err)) } - userData, err := a.storeData(ctx, userResp, generation) - if err != nil { - return nil, err - } - return userData, nil + return a.storeData(ctx, userResp, generation) } // FetchUserData fetches user data from the server. func (a *Client) FetchUserData(ctx context.Context) (*UserData, error) { ctx, span := otel.Tracer(tracerName).Start(ctx, "fetch_user_data") defer span.End() - return a.fetchUserData(ctx) + generation := a.identityGeneration() + resp, err := a.requestUserData(ctx, nil) + if err != nil { + return nil, err + } + return a.storeData(ctx, resp, generation) } -// fetchUserData fetches the user's account data and stores it. -func (a *Client) fetchUserData(ctx context.Context) (*UserData, error) { - generation := a.identityGeneration() - resp, err := a.sendProRequest(ctx, "GET", "/user-data", nil, nil, nil) +func (a *Client) requestUserData(ctx context.Context, headers map[string]string) (UserDataResponse, error) { + resp, err := a.sendProRequest(ctx, "GET", "/user-data", nil, headers, nil) if err != nil { a.logger.Error("user data", "error", err) - return nil, traces.RecordError(ctx, fmt.Errorf("getting user data: %w", err)) + return UserDataResponse{}, traces.RecordError(ctx, fmt.Errorf("getting user data: %w", err)) } var userResp UserDataResponse if err := json.Unmarshal(resp, &userResp); err != nil { - return nil, traces.RecordError(ctx, fmt.Errorf("error unmarshalling new user response: %w", err)) + return UserDataResponse{}, traces.RecordError(ctx, fmt.Errorf("unmarshalling user data response: %w", err)) } - return a.storeData(ctx, userResp, generation) + if userResp.LoginResponse_UserData == nil { + return UserDataResponse{}, traces.RecordError(ctx, errors.New("no user data in response")) + } + return userResp, nil } // ErrAccountChanged is returned when the account may have changed while the request was in flight; @@ -453,8 +458,9 @@ func (a *Client) StartChangeEmail(ctx context.Context, newEmail, password string return traces.RecordError(ctx, err) } -// CompleteChangeEmail completes a change of the email address associated with this user account, -// using the code received via email. +// CompleteChangeEmail changes the account's email using the code received via email. +// If another account is active when the response arrives, only the server is updated and an error +// is returned. func (a *Client) CompleteChangeEmail(ctx context.Context, newEmail, password, code string) error { ctx, span := otel.Tracer(tracerName).Start(ctx, "complete_change_email") defer span.End() @@ -474,6 +480,7 @@ func (a *Client) CompleteChangeEmail(ctx context.Context, newEmail, password, co return traces.RecordError(ctx, err) } + userID := settings.GetInt64(settings.UserIDKey) data := &protos.CompleteChangeEmailRequest{ OldEmail: settings.GetString(settings.EmailKey), NewEmail: newEmail, @@ -485,10 +492,16 @@ func (a *Client) CompleteChangeEmail(ctx context.Context, newEmail, password, co if err != nil { return traces.RecordError(ctx, err) } - if err := a.storeSalt(newSalt); err != nil { + a.mu.Lock() + defer a.mu.Unlock() + if settings.GetInt64(settings.UserIDKey) != userID { + return traces.RecordError(ctx, errors.New("email changed on the server, but another account is now active locally")) + } + a.generation++ + if err := a.storeSaltLocked(newSalt); err != nil { return traces.RecordError(ctx, err) } - if err := settings.Set(settings.EmailKey, newEmail); err != nil { + if err := a.storeIdentityLocked(0, "", settings.Settings{settings.EmailKey: newEmail}); err != nil { return traces.RecordError(ctx, err) } return nil @@ -560,42 +573,55 @@ func (a *Client) OAuthLoginURL(ctx context.Context, provider string) (string, er return loginURL.String(), nil } +// OAuthLoginCallback signs in with the token from an OAuth callback. It returns [ErrAccountChanged] +// if the account changes before the sign-in is saved. func (a *Client) OAuthLoginCallback(ctx context.Context, oAuthToken string) (*UserData, error) { a.logger.Debug("Getting OAuth login callback") jwtUserInfo, err := decodeJWT(oAuthToken) if err != nil { - return nil, fmt.Errorf("error decoding JWT: %w", err) + return nil, fmt.Errorf("%w: error decoding JWT: %w", ErrInvalidToken, err) + } + if jwtUserInfo.LegacyUserID == 0 || jwtUserInfo.LegacyToken == "" { + return nil, fmt.Errorf("%w: login token is missing the account identity", ErrInvalidToken) } - // Seed the identity first so the fetchUserData call below can authenticate. - login := &UserData{ - LegacyID: jwtUserInfo.LegacyUserID, - LegacyToken: jwtUserInfo.LegacyToken, - LegacyUserData: &protos.LoginResponse_UserData{ - UserId: jwtUserInfo.LegacyUserID, - Token: jwtUserInfo.LegacyToken, - DeviceID: jwtUserInfo.DeviceID, - Email: jwtUserInfo.Email, - }, + generation := a.identityGeneration() + resp, err := a.requestUserData(ctx, map[string]string{ + common.UserIDHeader: strconv.FormatInt(jwtUserInfo.LegacyUserID, 10), + common.ProTokenHeader: jwtUserInfo.LegacyToken, + }) + if err != nil { + return nil, fmt.Errorf("getting OAuth user data: %w", err) } - if err := a.setData(login, nil); err != nil { - return nil, fmt.Errorf("saving OAuth identity: %w", err) + if resp.UserId == 0 { + return nil, errors.New("OAuth user data is missing the account identity") } - user, err := a.fetchUserData(ctx) - if err != nil { - return nil, fmt.Errorf("error getting user data: %w", err) + if resp.UserId == jwtUserInfo.LegacyUserID { + if resp.Token == "" { + resp.Token = jwtUserInfo.LegacyToken + } + if resp.Email == "" { + resp.Email = jwtUserInfo.Email + } } - - if err := settings.Patch(settings.Settings{ - settings.JwtTokenKey: oAuthToken, + resp.DeviceID = settings.GetString(settings.DeviceIDKey) + user := &UserData{ + LegacyID: resp.UserId, + LegacyToken: resp.Token, + LegacyUserData: resp.LoginResponse_UserData, + Id: jwtUserInfo.Email, + EmailConfirmed: true, + Token: oAuthToken, + } + a.mu.Lock() + defer a.mu.Unlock() + if a.generation != generation { + return nil, ErrAccountChanged + } + a.generation++ + if _, err := a.setDataLocked(user, false, settings.Settings{ settings.OAuthLoginKey: true, }); err != nil { - a.logger.Error("Failed to persist JWT token", "error", err) - return nil, fmt.Errorf("failed to persist JWT token: %w", err) - } - user.Id = jwtUserInfo.Email - user.EmailConfirmed = true - if err := a.setData(user, nil); err != nil { return nil, fmt.Errorf("saving OAuth user data: %w", err) } return user, nil @@ -715,18 +741,59 @@ func (a *Client) storeIdentity(id int64, token string, extra settings.Settings) return a.storeIdentityLocked(id, token, extra) } -// PatchAccountSettings saves updates as a change of the active account. Pending user-data and -// purchase responses requested before the call are discarded, and a [UserChangeEvent] is emitted -// if the account state changed. +// PatchAccountSettings saves updates as a change to the active account. If any value differs from +// its stored setting, user-data and purchase responses requested before the call are discarded; +// otherwise the call does nothing. +// A different user ID clears the previous account's credentials, cached data, salt and OAuth flags. +// A supplied user ID must be a positive int64-compatible number or base-10 string. +// A changed account emits a [UserChangeEvent]; on error the active identity remains unchanged. func (a *Client) PatchAccountSettings(updates settings.Settings) error { - return a.storeIdentity(0, "", updates) + a.mu.Lock() + defer a.mu.Unlock() + var id int64 + if value, ok := updates[settings.UserIDKey]; ok { + var err error + id, err = parseUserID(value) + if err != nil { + return err + } + } + var extra settings.Settings + if id != 0 && id != settings.GetInt64(settings.UserIDKey) { + extra = settings.Settings{settings.OAuthLoginKey: false, settings.OAuthProviderKey: ""} + maps.Copy(extra, updates) + } else { + // Diff also compares types, so the user ID, already matched numerically, is left out. + extra = updates.Diff(settings.GetAllFor(slices.Collect(maps.Keys(updates))...)) + delete(extra, settings.UserIDKey) + if len(extra) == 0 { + return nil + } + } + a.generation++ + return a.storeIdentityLocked(id, "", extra) +} + +func parseUserID(v any) (int64, error) { + value := fmt.Sprint(v) + if f, ok := v.(float64); ok { + if math.Trunc(f) != f { + return 0, fmt.Errorf("invalid user ID %v: expected an integer", f) + } + value = strconv.FormatFloat(f, 'f', 0, 64) + } + id, err := strconv.ParseInt(value, 10, 64) + if err != nil || id <= 0 { + return 0, fmt.Errorf("invalid user ID %q: expected a positive int64", value) + } + return id, nil } -// storeIdentityLocked saves id and token, each only if set, together with extra in one write that -// takes effect only once saved, and emits a [UserChangeEvent] if the [accountState] changed. A -// different nonzero id also clears the previous account's credentials and cached data. The OAuth -// flags are kept, because an OAuth sign-in records its provider before switching. It requires a.mu -// to be held. +// storeIdentityLocked saves nonzero id and nonempty token together with extra, emitting a +// [UserChangeEvent] if the account changed. Switching accounts clears credentials, cached data +// and salt, but keeps OAuth flags because sign-in records its provider before switching. +// A failed settings write leaves the identity unchanged but may discard the persisted salt. +// It requires a.mu to be held. func (a *Client) storeIdentityLocked(id int64, token string, extra settings.Settings) error { before := currentAccount() defer func() { @@ -756,7 +823,19 @@ func (a *Client) storeIdentityLocked(id int64, token string, extra settings.Sett if len(updates) == 0 { return nil } - return settings.Patch(updates) + if switching { + // Invalidate the unkeyed salt on disk before publishing another identity. + if err := writeSalt(nil, a.saltPath); err != nil { + return fmt.Errorf("clearing the previous account's salt: %w", err) + } + } + if err := settings.Patch(updates); err != nil { + return err + } + if switching { + a.salt = nil + } + return nil } // accountState holds the settings whose change is reported as a [UserChangeEvent]. @@ -788,9 +867,9 @@ func (a *Client) setData(data *UserData, extra settings.Settings) error { } // setDataLocked stores a copy of data together with extra, first merging data with the cached -// response of the same account when preserveLoginFields is set. Data without user data stores only its id, token -// and devices. It returns data, possibly merged, which the caller may modify without affecting -// settings. It requires a.mu to be held and data to be non-nil. +// response of the same account when preserveLoginFields is set. Data without user data stores only +// its id, token and devices. It returns data, possibly merged, which the caller may modify without +// affecting settings. It requires a.mu to be held and data to be non-nil. func (a *Client) setDataLocked(data *UserData, preserveLoginFields bool, extra settings.Settings) (*UserData, error) { updates := settings.Settings{} // A device-limited login carries no user data, but its device list is what the user needs to diff --git a/account/user_change_test.go b/account/user_change_test.go index 0e6bd7f7..b9055b83 100644 --- a/account/user_change_test.go +++ b/account/user_change_test.go @@ -56,6 +56,13 @@ func TestAccountChangesNotifyWithoutBackendWrappers(t *testing.T) { a.ClearUser() expect(0) + oauthToken := mockJWT(t, map[string]any{ + "legacy_user_id": 123, "legacy_token": "oauth-token", "email": "test@example.com", + }) + _, err = a.OAuthLoginCallback(ctx, oauthToken) + require.NoError(t, err) + expect(1) + token := mockJWT(t, map[string]any{"legacy_user_id": 789, "legacy_token": "oauth-token"}) require.NoError(t, a.OAuthDeviceLimitCallback(ctx, token)) expect(1) diff --git a/account/user_refresh_test.go b/account/user_refresh_test.go index d2be92eb..265a8dbe 100644 --- a/account/user_refresh_test.go +++ b/account/user_refresh_test.go @@ -3,8 +3,11 @@ package account import ( "context" "fmt" + "math" "net/http" "net/http/httptest" + "os" + "path/filepath" "testing" "github.com/stretchr/testify/assert" @@ -376,3 +379,147 @@ func TestAccountSwitchNeverExposesAnEmptyIdentity(t *testing.T) { close(stop) assert.False(t, <-sawEmpty, "readers must never see an account without an id or token") } + +func TestPatchAccountSwitchClearsOld(t *testing.T) { + for _, id := range []any{ + int(2), int8(2), int16(2), int32(2), int64(2), + uint(2), uint8(2), uint16(2), uint32(2), uint64(2), + float64(2), "2", + } { + t.Run(fmt.Sprintf("%T", id), func(t *testing.T) { + a, _ := newTestClient(t) + require.NoError(t, a.setData(&UserData{ + LegacyID: 1, + LegacyToken: "a", + Token: "a-jwt", + LegacyUserData: &protos.LoginResponse_UserData{UserId: 1, Email: "a@example.com", UserLevel: "pro"}, + }, nil)) + require.NoError(t, a.storeSalt([]byte("a-salt"))) + setOAuthFlags(t) + updates := settings.Settings{settings.UserIDKey: id, settings.TokenKey: "b"} + + require.NoError(t, a.PatchAccountSettings(updates)) + + assert.Equal(t, id, updates[settings.UserIDKey]) + assert.Equal(t, int64(2), settings.Get(settings.UserIDKey)) + require.NoError(t, settings.Reload()) + assert.Equal(t, int64(2), settings.GetInt64(settings.UserIDKey)) + assert.Equal(t, "b", settings.GetString(settings.TokenKey)) + assert.Empty(t, settings.GetString(settings.JwtTokenKey)) + assert.Empty(t, settings.GetString(settings.EmailKey)) + assert.Empty(t, settings.Get(settings.UserDataKey)) + assert.False(t, settings.IsPro()) + assert.False(t, settings.GetBool(settings.OAuthLoginKey)) + assert.Empty(t, settings.GetString(settings.OAuthProviderKey)) + assert.Nil(t, a.getSaltCached()) + salt, err := readSalt(a.saltPath) + require.NoError(t, err) + assert.Nil(t, salt) + }) + } +} + +func TestPatchAccountRejectsBadID(t *testing.T) { + for _, id := range []any{ + nil, true, "", "invalid", "2.5", "9223372036854775808", + 0, -1, uint64(math.MaxInt64) + 1, 2.5, float32(2.5), + float64(math.MaxInt64), math.Inf(1), math.NaN(), + } { + t.Run(fmt.Sprintf("%T/%v", id, id), func(t *testing.T) { + a, _ := newTestClient(t) + require.NoError(t, a.PatchAccountSettings(settings.Settings{ + settings.UserIDKey: int64(1), settings.TokenKey: "a", + })) + before := settings.GetAll() + generation := a.identityGeneration() + + err := a.PatchAccountSettings(settings.Settings{settings.UserIDKey: id, settings.TokenKey: "b"}) + + require.ErrorContains(t, err, "invalid user ID") + assert.Equal(t, before, settings.GetAll()) + assert.Equal(t, generation, a.identityGeneration()) + }) + } +} + +func TestParseUserIDLarge(t *testing.T) { + for _, tt := range []struct { + value any + want int64 + }{ + {float64(1e9), 1e9}, + {float64(1<<60) + 256, 1152921504606847232}, + {float64(math.MaxInt64) - 1024, 9223372036854774784}, + {int64(math.MaxInt64), math.MaxInt64}, + {uint64(math.MaxInt64), math.MaxInt64}, + {"9223372036854775807", math.MaxInt64}, + } { + got, err := parseUserID(tt.value) + require.NoError(t, err) + assert.Equal(t, tt.want, got) + } +} + +func TestSwitchNeedsSaltCleared(t *testing.T) { + a, _ := newTestClient(t) + require.NoError(t, a.PatchAccountSettings(settings.Settings{ + settings.UserIDKey: int64(1), settings.TokenKey: "a", + })) + require.NoError(t, a.storeSalt([]byte("a-salt"))) + before := settings.GetAll() + dir := filepath.Dir(a.saltPath) + require.NoError(t, os.Rename(dir, dir+".aside")) + t.Cleanup(func() { require.NoError(t, os.Rename(dir+".aside", dir)) }) + + err := a.PatchAccountSettings(settings.Settings{settings.UserIDKey: int64(2), settings.TokenKey: "b"}) + + require.ErrorContains(t, err, "clearing the previous account's salt") + assert.Equal(t, before, settings.GetAll()) + assert.Equal(t, []byte("a-salt"), a.getSaltCached()) + salt, err := readSalt(filepath.Join(dir+".aside", saltFileName)) + require.NoError(t, err) + assert.Equal(t, []byte("a-salt"), salt) +} + +func TestPatchAccountNoopKeepsPending(t *testing.T) { + a, _ := newTestClient(t) + require.NoError(t, a.PatchAccountSettings(settings.Settings{ + settings.UserIDKey: int64(1), settings.TokenKey: "a", settings.OAuthLoginKey: true, + })) + require.NoError(t, settings.Reload()) + handler, requested, release := heldJSON(UserDataResponse{ + LoginResponse_UserData: &protos.LoginResponse_UserData{UserId: 1, Token: "a", UserLevel: "pro"}, + }) + useProServer(t, a, handler) + errc := make(chan error, 1) + go func() { + _, err := a.FetchUserData(context.Background()) + errc <- err + }() + <-requested + + require.NoError(t, a.PatchAccountSettings(settings.Settings{ + settings.UserIDKey: "1", settings.TokenKey: "a", settings.OAuthLoginKey: true, + })) + close(release) + + require.NoError(t, <-errc) + assert.True(t, settings.IsPro()) +} + +func TestPatchAccountSameKeepsFields(t *testing.T) { + a, _ := newTestClient(t) + require.NoError(t, a.setData(&UserData{ + LegacyID: 1, + LegacyToken: "a", + Token: "a-jwt", + LegacyUserData: &protos.LoginResponse_UserData{UserId: 1, Email: "a@example.com"}, + }, nil)) + require.NoError(t, a.storeSalt([]byte("a-salt"))) + + require.NoError(t, a.PatchAccountSettings(settings.Settings{settings.EmailKey: "new@example.com"})) + + assert.Equal(t, "new@example.com", settings.GetString(settings.EmailKey)) + assert.Equal(t, "a-jwt", settings.GetString(settings.JwtTokenKey)) + assert.Equal(t, []byte("a-salt"), a.getSaltCached()) +} diff --git a/account/user_test.go b/account/user_test.go index 721e45ed..b18233d0 100644 --- a/account/user_test.go +++ b/account/user_test.go @@ -599,6 +599,72 @@ func TestCompleteChangeEmail(t *testing.T) { assert.NoError(t, err) } +func TestChangeEmailDiscardsStale(t *testing.T) { + ac, _ := newTestClient(t) + require.NoError(t, ac.setData(&UserData{ + LegacyID: 1, + LegacyToken: "a", + LegacyUserData: &protos.LoginResponse_UserData{UserId: 1, Email: "old@example.com"}, + }, nil)) + handler, requested, release := heldJSON(UserDataResponse{ + LoginResponse_UserData: &protos.LoginResponse_UserData{UserId: 1, Token: "a", Email: "old@example.com"}, + }) + useProServer(t, ac, handler) + errc := make(chan error, 1) + go func() { + _, err := ac.FetchUserData(context.Background()) + errc <- err + }() + <-requested + + require.NoError(t, ac.CompleteChangeEmail(context.Background(), "new@example.com", "password", "code")) + close(release) + + assert.ErrorIs(t, <-errc, ErrAccountChanged) + assert.Equal(t, "new@example.com", settings.GetString(settings.EmailKey)) +} + +func TestChangeEmailSurvivesUpdate(t *testing.T) { + ac, _ := newTestClient(t) + require.NoError(t, ac.setData(&UserData{ + LegacyID: 1, + LegacyToken: "a", + LegacyUserData: &protos.LoginResponse_UserData{UserId: 1, Email: "old@example.com"}, + }, nil)) + ac.salt = []byte("old-salt") + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + assert.NoError(t, ac.PatchAccountSettings(settings.Settings{settings.UserLevelKey: "pro"})) + writeProtoResponse(w, &protos.EmptyResponse{}) + })) + defer server.Close() + ac.authURL = server.URL + + require.NoError(t, ac.CompleteChangeEmail(context.Background(), "new@example.com", "password", "code")) + + assert.Equal(t, "new@example.com", settings.GetString(settings.EmailKey)) + assert.NotEqual(t, []byte("old-salt"), ac.getSaltCached()) +} + +func TestChangeEmailSkipsOtherAccount(t *testing.T) { + ac, _ := newTestClient(t) + require.NoError(t, ac.setData(&UserData{ + LegacyID: 1, + LegacyToken: "a", + LegacyUserData: &protos.LoginResponse_UserData{UserId: 1, Email: "old@example.com"}, + }, nil)) + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + assert.NoError(t, ac.PatchAccountSettings(settings.Settings{settings.UserIDKey: int64(2), settings.TokenKey: "b"})) + writeProtoResponse(w, &protos.EmptyResponse{}) + })) + defer server.Close() + ac.authURL = server.URL + + assert.Error(t, ac.CompleteChangeEmail(context.Background(), "new@example.com", "password", "code")) + + assert.Equal(t, int64(2), settings.GetInt64(settings.UserIDKey)) + assert.Empty(t, settings.GetString(settings.EmailKey)) +} + func TestDeleteAccount(t *testing.T) { email := "test@example.com" ac, _ := newTestClientWithSRP(t, email, "password") @@ -647,14 +713,139 @@ func TestOAuthLoginCallback(t *testing.T) { assert.Empty(t, devices) } +func TestOAuthLoginDiscardedOnSwitch(t *testing.T) { + ac, _ := newTestClient(t) + useProServer(t, ac, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + assert.NoError(t, ac.PatchAccountSettings(settings.Settings{settings.UserIDKey: int64(99), settings.TokenKey: "other"})) + writeJSONResponse(w, UserDataResponse{ + LoginResponse_UserData: &protos.LoginResponse_UserData{UserId: 12345, Token: "test-token"}, + }) + })) + mockToken := "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJlbWFpbCI6InRlc3RAZXhhbXBsZS5jb20iLCJsZWdhY3lfdXNlcl9pZCI6MTIzNDUsImxlZ2FjeV90b2tlbiI6InRlc3QtdG9rZW4ifQ.test" + + _, err := ac.OAuthLoginCallback(context.Background(), mockToken) + + assert.ErrorIs(t, err, ErrAccountChanged) + assert.Equal(t, int64(99), settings.GetInt64(settings.UserIDKey)) + assert.Empty(t, settings.GetString(settings.JwtTokenKey)) + assert.False(t, settings.GetBool(settings.OAuthLoginKey)) +} + +func TestOAuthLoginSavesAccount(t *testing.T) { + ac, _ := newTestClient(t) + require.NoError(t, ac.setData(&UserData{ + LegacyID: 1, LegacyToken: "old-token", Token: "old-jwt", + LegacyUserData: &protos.LoginResponse_UserData{UserId: 1, Email: "old@example.com"}, + }, nil)) + require.NoError(t, ac.storeSalt([]byte("old-salt"))) + require.NoError(t, settings.Set(settings.DeviceIDKey, "device")) + require.NoError(t, settings.Set(settings.OAuthProviderKey, "google")) + before := currentAccount() + generation := ac.identityGeneration() + useProServer(t, ac, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + assert.Equal(t, "2", r.Header.Get(common.UserIDHeader)) + assert.Equal(t, "oauth-token", r.Header.Get(common.ProTokenHeader)) + assert.Equal(t, "device", r.Header.Get(common.DeviceIDHeader)) + assert.Equal(t, before, currentAccount()) + assert.Equal(t, []byte("old-salt"), ac.getSaltCached()) + writeJSONResponse(w, UserDataResponse{ + LoginResponse_UserData: &protos.LoginResponse_UserData{UserId: 2, UserLevel: "pro"}, + }) + })) + token := mockJWT(t, map[string]any{ + "legacy_user_id": 2, "legacy_token": "oauth-token", "email": "new@example.com", + }) + + user, err := ac.OAuthLoginCallback(context.Background(), token) + + require.NoError(t, err) + require.NotNil(t, user) + assert.Equal(t, generation+1, ac.identityGeneration()) + assert.Equal(t, "new@example.com", user.Id) + assert.True(t, user.EmailConfirmed) + assert.Equal(t, token, user.Token) + assert.Equal(t, "oauth-token", user.LegacyToken) + assert.Equal(t, "new@example.com", user.LegacyUserData.Email) + assert.Equal(t, "device", user.LegacyUserData.DeviceID) + require.NoError(t, settings.Reload()) + assert.Equal(t, int64(2), settings.GetInt64(settings.UserIDKey)) + assert.Equal(t, "oauth-token", settings.GetString(settings.TokenKey)) + assert.Equal(t, token, settings.GetString(settings.JwtTokenKey)) + assert.Equal(t, "new@example.com", settings.GetString(settings.EmailKey)) + assert.True(t, settings.GetBool(settings.OAuthLoginKey)) + assert.Equal(t, "google", settings.GetString(settings.OAuthProviderKey)) + assert.True(t, settings.IsPro()) + var cached UserData + require.NoError(t, settings.GetStruct(settings.UserDataKey, &cached)) + assert.True(t, proto.Equal(user, &cached)) + assert.Nil(t, ac.getSaltCached()) +} + +func TestOAuthLoginFailureKeepsAccount(t *testing.T) { + for _, tt := range []struct { + name string + status int + body string + failSave bool + }{ + {name: "request", status: http.StatusServiceUnavailable, body: "unavailable"}, + {name: "json", status: http.StatusOK, body: "invalid"}, + {name: "missing user data", status: http.StatusOK, body: "{}"}, + {name: "missing identity", status: http.StatusOK, body: `{"email":"new@example.com"}`}, + {name: "save", status: http.StatusOK, body: `{"userId":2,"token":"oauth-token"}`, failSave: true}, + } { + t.Run(tt.name, func(t *testing.T) { + ac, _ := newTestClient(t) + require.NoError(t, ac.setData(&UserData{ + LegacyID: 1, LegacyToken: "old-token", Token: "old-jwt", + LegacyUserData: &protos.LoginResponse_UserData{UserId: 1, Email: "old@example.com"}, + }, nil)) + require.NoError(t, ac.storeSalt([]byte("old-salt"))) + before := settings.GetAll() + useProServer(t, ac, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(tt.status) + _, _ = io.WriteString(w, tt.body) + })) + if tt.failSave { + defer failSettingsSaves(t)() + } + token := mockJWT(t, map[string]any{"legacy_user_id": 2, "legacy_token": "oauth-token"}) + + user, err := ac.OAuthLoginCallback(context.Background(), token) + + require.Error(t, err) + assert.Nil(t, user) + assert.Equal(t, before, settings.GetAll()) + assert.Equal(t, []byte("old-salt"), ac.getSaltCached()) + }) + } +} + func TestOAuthLoginCallback_InvalidToken(t *testing.T) { ac, _ := newTestClient(t) _, err := ac.OAuthLoginCallback(context.Background(), "invalid-token") - assert.Error(t, err) + assert.ErrorIs(t, err, ErrInvalidToken) assert.Contains(t, err.Error(), "error decoding JWT") } +func TestOAuthLoginRejectsNoIdentity(t *testing.T) { + for _, claims := range []map[string]any{ + {"legacy_token": "oauth-token"}, + {"legacy_user_id": 2}, + } { + ac, _ := newTestClient(t) + useProServer(t, ac, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + t.Error("no request should be sent for a token without an account identity") + })) + + _, err := ac.OAuthLoginCallback(context.Background(), mockJWT(t, claims)) + + assert.ErrorIs(t, err, ErrInvalidToken) + } +} + // mockJWT builds an unsigned JWT from the given claims; decodeJWT uses // ParseUnverified, so the fake signature is never checked. func mockJWT(t *testing.T, claims map[string]any) string { diff --git a/backend/radiance.go b/backend/radiance.go index 8f24fbac..9c54a843 100644 --- a/backend/radiance.go +++ b/backend/radiance.go @@ -717,17 +717,17 @@ func (r *LocalBackend) PatchSettings(updates settings.Settings) error { diff = maps.Clone(diff) delete(diff, settings.PeerShareEnabledKey) } - if len(diff) == 0 { - return errs - } patch := settings.Patch for _, key := range settings.UserKeys { - if _, ok := diff[key]; ok { - // settings.Patch would let a stale user-data or purchase response overwrite this change. + if value, ok := updates[key]; ok { + // Account fields must be applied together against the identity active at commit time. + diff[key] = value patch = r.accountClient.PatchAccountSettings - break } } + if len(diff) == 0 { + return errs + } if err := patch(diff); err != nil { return errors.Join(errs, fmt.Errorf("failed to update settings: %w", err)) } diff --git a/backend/user_messages_test.go b/backend/user_messages_test.go index 3a1edcd0..948722e9 100644 --- a/backend/user_messages_test.go +++ b/backend/user_messages_test.go @@ -62,6 +62,30 @@ func TestPatchSettingsAccountKeysDiscardStaleUserData(t *testing.T) { require.False(t, settings.IsPro()) } +func TestPatchSettingsSwitchKeepsMatches(t *testing.T) { + settings.Reset() + t.Cleanup(settings.Reset) + require.NoError(t, settings.InitSettings(t.TempDir())) + require.NoError(t, settings.Patch(settings.Settings{ + settings.UserIDKey: int64(1), + settings.TokenKey: "a", + settings.UserLevelKey: "pro", + settings.OAuthLoginKey: true, + })) + r := &LocalBackend{accountClient: account.NewClient(http.DefaultClient, t.TempDir(), log.NoOpLogger())} + + require.NoError(t, r.PatchSettings(settings.Settings{ + settings.UserIDKey: int64(2), + settings.TokenKey: "b", + settings.UserLevelKey: "pro", + settings.OAuthLoginKey: true, + })) + + require.Equal(t, int64(2), settings.GetInt64(settings.UserIDKey)) + require.True(t, settings.IsPro()) + require.True(t, settings.GetBool(settings.OAuthLoginKey)) +} + func TestUserMessagesFollowAccountEvents(t *testing.T) { settings.Reset() t.Cleanup(settings.Reset) @@ -155,3 +179,40 @@ func TestUserMessagesFollowAccountEvents(t *testing.T) { require.NoError(t, err) require.Nil(t, message) } + +type switchingPeer struct { + *fakePeerController + onStart func() +} + +func (p *switchingPeer) Start(ctx context.Context) error { + p.onStart() + return p.fakePeerController.Start(ctx) +} + +func TestPatchSettingsAccountKeysWin(t *testing.T) { + for _, token := range []string{"a", "new-a"} { + t.Run(token, func(t *testing.T) { + settings.Reset() + t.Cleanup(settings.Reset) + require.NoError(t, settings.InitSettings(t.TempDir())) + require.NoError(t, settings.Patch(settings.Settings{settings.UserIDKey: int64(1), settings.TokenKey: "a"})) + a := account.NewClient(http.DefaultClient, t.TempDir(), log.NoOpLogger()) + peer := &switchingPeer{fakePeerController: &fakePeerController{}, onStart: func() { + require.NoError(t, a.PatchAccountSettings(settings.Settings{settings.UserIDKey: int64(2), settings.TokenKey: "b"})) + }} + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + r := &LocalBackend{ctx: ctx, peerClient: peer, accountClient: a} + + require.NoError(t, r.PatchSettings(settings.Settings{ + settings.PeerShareEnabledKey: true, + settings.UserIDKey: int64(1), + settings.TokenKey: token, + })) + + require.Equal(t, int64(1), settings.GetInt64(settings.UserIDKey)) + require.Equal(t, token, settings.GetString(settings.TokenKey)) + }) + } +} diff --git a/ipc/server.go b/ipc/server.go index 84bb2bc2..08ebd196 100644 --- a/ipc/server.go +++ b/ipc/server.go @@ -1272,7 +1272,11 @@ func (s *localapi) accountOAuthHandler(w http.ResponseWriter, r *http.Request) { } userData, err := s.backend(r.Context()).OAuthLoginCallback(r.Context(), req.OAuthToken) if err != nil { - http.Error(w, err.Error(), http.StatusInternalServerError) + status := http.StatusInternalServerError + if errors.Is(err, account.ErrInvalidToken) { + status = http.StatusBadRequest + } + http.Error(w, err.Error(), status) return } writeJSON(w, http.StatusOK, userData) From e91962ebe252572d4e8fb3bb43099831243cd5bd Mon Sep 17 00:00:00 2001 From: garmr-ulfr Date: Thu, 15 Oct 2026 10:26:15 -0700 Subject: [PATCH 5/8] fix: save login identity and salt under one lock --- account/user.go | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/account/user.go b/account/user.go index 0cf06d46..453e43d6 100644 --- a/account/user.go +++ b/account/user.go @@ -310,11 +310,14 @@ func (a *Client) Login(ctx context.Context, email, password string) (*UserData, // regardless of state we need to save login information // We have device flow limit on login oauth := settings.Settings{settings.OAuthLoginKey: false, settings.OAuthProviderKey: ""} - if err := a.setData(&loginResp, oauth); err != nil { + a.mu.Lock() + defer a.mu.Unlock() + a.generation++ + if _, err := a.setDataLocked(&loginResp, false, oauth); err != nil { return nil, traces.RecordError(ctx, fmt.Errorf("saving login: %w", err)) } - if saltErr := a.storeSalt(salt); saltErr != nil { - return nil, traces.RecordError(ctx, saltErr) + if err := a.storeSaltLocked(salt); err != nil { + return nil, traces.RecordError(ctx, err) } return &loginResp, nil } From e091d08543188ba56491778ee981450915de0a93 Mon Sep 17 00:00:00 2001 From: garmr-ulfr Date: Fri, 16 Oct 2026 11:03:11 -0700 Subject: [PATCH 6/8] fix test --- common/settings/settings_test.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/common/settings/settings_test.go b/common/settings/settings_test.go index ea6ae197..f8fe94df 100644 --- a/common/settings/settings_test.go +++ b/common/settings/settings_test.go @@ -748,9 +748,13 @@ func TestPatchSkipsUnchangedWrites(t *testing.T) { dir := t.TempDir() initAt(t, dir) path := filepath.Join(dir, settingsFileName) + // On Windows, os.Stat defers the file ID lookup to os.SameFile; a handle's Stat records it now. stat := func() os.FileInfo { t.Helper() - info, err := os.Stat(path) + f, err := os.Open(path) + require.NoError(t, err) + defer f.Close() + info, err := f.Stat() require.NoError(t, err) return info } From a5d4fbe7fa58d1eff73668d0564cdd959068f575 Mon Sep 17 00:00:00 2001 From: garmr-ulfr Date: Fri, 16 Oct 2026 15:58:58 -0700 Subject: [PATCH 7/8] fix: treat unreadable legacy settings as missing An unreadable legacy file now counts as missing instead of leaving the migration pending, and any settings.json holding a JSON object wins, matching main's rule. --- common/settings/legacy_crossdir.go | 13 +++---- common/settings/legacy_yaml.go | 3 +- common/settings/settings.go | 60 +++++++++++------------------- common/settings/settings_test.go | 22 +++++------ 4 files changed, 38 insertions(+), 60 deletions(-) diff --git a/common/settings/legacy_crossdir.go b/common/settings/legacy_crossdir.go index 080603d9..9105989e 100644 --- a/common/settings/legacy_crossdir.go +++ b/common/settings/legacy_crossdir.go @@ -3,6 +3,7 @@ package settings import ( "errors" "io/fs" + "log/slog" "os" "path/filepath" "runtime" @@ -13,13 +14,8 @@ import ( // lookup to a temp dir without touching the host's %PUBLIC%. var windowsCrossDirCandidatesFn = windowsCrossDirCandidates -// windowsCrossDirCandidates returns settings candidates from the v9.0.x -// Windows data directory (${PUBLIC}\Lantern\data), which the same-dir -// migration doesn't cover: radiance's data dir moved to -// ${ProgramData}\Lantern and the two directories don't share a parent, so -// the same-dir candidates never see the v9.0.x file. -// -// Returns nil on non-Windows hosts or when %PUBLIC% is unset. +// windowsCrossDirCandidates returns the v9.0.x local.json from ${PUBLIC}\Lantern\data, which the +// same-dir candidates miss because radiance's data dir moved to ${ProgramData}\Lantern. func windowsCrossDirCandidates(fileDir string) []candidateSource { if runtime.GOOS != "windows" { return nil @@ -64,7 +60,8 @@ func readWindowsCrossDirCandidates(v90xDir string) []candidateSource { return nil } if err != nil { - return []candidateSource{{path: full, label: label, err: err}} + slog.Warn("legacy settings migration: read failed", "path", full, "error", err) + return nil } return []candidateSource{{ path: full, diff --git a/common/settings/legacy_yaml.go b/common/settings/legacy_yaml.go index 4a219e8f..54d2fd67 100644 --- a/common/settings/legacy_yaml.go +++ b/common/settings/legacy_yaml.go @@ -32,7 +32,8 @@ func legacyYAMLCandidate(fileDir string) candidateSource { return candidateSource{} } if err != nil { - return candidateSource{path: path, label: label, err: err} + slog.Warn("legacy settings migration: read failed", "path", path, "error", err) + return candidateSource{} } translated, err := translateLegacyYAML(raw, layout) if err != nil { diff --git a/common/settings/settings.go b/common/settings/settings.go index f59e221a..a9817df8 100644 --- a/common/settings/settings.go +++ b/common/settings/settings.go @@ -134,7 +134,7 @@ func InitSettings(fileDir string) error { migrate := migrationPending(markerPath) var identity map[string]any if migrate { - identity, migrate = legacyIdentity(fileDir, k.filePath) + identity = legacyIdentity(fileDir, k.filePath) } fresh := false switch err := loadSettings(k.filePath); { @@ -183,45 +183,39 @@ func migrationPending(markerPath string) bool { return errors.Is(err, fs.ErrNotExist) } -// candidateSource is one possible location of persisted user state. -// contents is always canonical JSON — direct for v9.x, translated for -// pre-9.x YAML. +// candidateSource is one possible location of persisted user state. contents is v9.x JSON, or +// pre-9.x YAML translated to JSON. type candidateSource struct { path string contents []byte exists bool label string - // err is the read error, other than fs.ErrNotExist; exists is false when it is set. - err error } // legacyIdentity returns the identity keys to adopt from settings written by older client -// versions, or nil when nothing should be adopted. ok is false when any file can't be read, so the -// migration can't be judged yet. +// versions, or nil when nothing should be adopted or canonicalPath can't be read. A legacy candidate +// that can't be read counts as missing. // // Legacy candidates in priority order: // // 1. /local.json — v9.0.x (renamed) // 2. ${PUBLIC}\Lantern\data\local.json — v9.0.x cross-dir, Windows only // 3. pre-9.x platform-specific YAML -// 4. /data/settings.json — v9.1.x (bugged: -// setupDirectories appended an -// unconditional "/data" suffix) +// 4. /data/settings.json — v9.1.x (misplaced under data/) // -// A canonical file with a non-zero user_id always wins, because it is account state this client -// already accepted. One without may be the defaults written while an unreadable candidate kept the -// migration pending, so it never wins. Otherwise a candidate counts only if it holds a JSON object: -// pick the highest-priority one with user_level=="pro", or if none is pro, the highest-priority one -// with a non-zero user_id, so the device registration is kept. -func legacyIdentity(fileDir, canonicalPath string) (identity map[string]any, ok bool) { +// A canonical file holding a JSON object always wins, even without an account, because it is +// state this client already accepted. Otherwise a legacy candidate counts only if it holds a JSON +// object: pick the highest-priority one with user_level=="pro", or if none is pro, the +// highest-priority one with a non-zero user_id, so the device registration is kept. +func legacyIdentity(fileDir, canonicalPath string) map[string]any { switch raw, err := os.ReadFile(canonicalPath); { case err == nil: - if p, err := (settingsJSON{}).Unmarshal(raw); err == nil && hasAccount(p) { - return nil, true + if p, err := (settingsJSON{}).Unmarshal(raw); err == nil && p != nil { + return nil } case !errors.Is(err, fs.ErrNotExist): - slog.Warn("legacy settings migration: read failed; will retry", "path", canonicalPath, "error", err) - return nil, false + // InitSettings fails on the same read before marking the migration complete. + return nil } candidates := []candidateSource{ {path: filepath.Join(fileDir, legacySettingsFileName), label: "v9.0.x local.json"}, @@ -235,29 +229,17 @@ func legacyIdentity(fileDir, canonicalPath string) (identity map[string]any, ok candidates[i].exists = true case errors.Is(err, fs.ErrNotExist): default: - candidates[i].err = err + slog.Warn("legacy settings migration: read failed", "path", candidates[i].path, "error", err) } } // Optional candidates are inserted oldest-first, so newer generations rank higher. - if yc := legacyYAMLCandidate(fileDir); yc.exists || yc.err != nil { + if yc := legacyYAMLCandidate(fileDir); yc.exists { candidates = append(candidates[:1], append([]candidateSource{yc}, candidates[1:]...)...) } - // Windows v9.0.x cross-dir candidates (${PUBLIC}\Lantern\data) are the - // same generation of state as the same-dir local.json, just stored - // under a different filesystem root because lanternd's data dir moved - // to ${ProgramData}\Lantern. On every other GOOS / when the - // env is unset windowsCrossDirCandidatesFn returns nil and this is a - // no-op. + // Cross-dir candidates are the same generation as local.json. if winExtras := windowsCrossDirCandidatesFn(fileDir); len(winExtras) > 0 { candidates = append(candidates[:1], append(winExtras, candidates[1:]...)...) } - for _, c := range candidates { - if c.err != nil { - slog.Warn("legacy settings migration: read failed; will retry", "path", c.path, "error", c.err) - return nil, false - } - } - parsed := make([]map[string]any, len(candidates)) for i, c := range candidates { if !c.exists { @@ -286,16 +268,16 @@ func legacyIdentity(fileDir, canonicalPath string) (identity map[string]any, ok } } if pickIdx == -1 { - return nil, true + return nil } - identity = make(map[string]any) + identity := make(map[string]any) for _, key := range identityKeys { if v, ok := parsed[pickIdx][key.String()]; ok { identity[key.String()] = v } } slog.Info("legacy settings migration: recovering persisted identity", "source", candidates[pickIdx].label) - return identity, true + return identity } func hasAccount(settings map[string]any) bool { diff --git a/common/settings/settings_test.go b/common/settings/settings_test.go index f8fe94df..aa72d5f7 100644 --- a/common/settings/settings_test.go +++ b/common/settings/settings_test.go @@ -354,19 +354,20 @@ func TestLegacyMigration(t *testing.T) { assert.Equal(t, "apple", GetString(OAuthProviderKey)) }) - t.Run("a canonical file without a user id doesn't outrank a legacy identity", func(t *testing.T) { + t.Run("logged out canonical settings survive stale Pro recovery files", func(t *testing.T) { tempDir := t.TempDir() writeCanonical(t, tempDir, `{"locale": "en-US", "device_id": "dev"}`) - writeLegacy(t, tempDir, `{"user_id": 7, "user_level": "free", "token": "tok"}`) + writeLegacy(t, tempDir, `{"user_id": 7, "user_level": "pro", "token": "stale-token"}`) initAt(t, tempDir) - assert.Equal(t, "7", GetString(UserIDKey)) + assert.False(t, Exists(UserIDKey)) + assert.False(t, Exists(TokenKey)) assert.Equal(t, "en-US", GetString(LocaleKey)) assert.Equal(t, "dev", GetString(DeviceIDKey)) }) - t.Run("an unreadable legacy file leaves the migration pending", func(t *testing.T) { + t.Run("an unreadable legacy file counts as missing", func(t *testing.T) { if runtime.GOOS == "windows" { t.Skip("permission semantics differ on windows") } @@ -374,19 +375,15 @@ func TestLegacyMigration(t *testing.T) { t.Skip("root reads files regardless of mode") } tempDir := t.TempDir() - writeLegacy(t, tempDir, `{"user_id": 7, "user_level": "free", "token": "tok"}`) + writeLegacy(t, tempDir, `{"user_id": 7, "user_level": "pro", "token": "tok"}`) legacy := filepath.Join(tempDir, legacySettingsFileName) require.NoError(t, os.Chmod(legacy, 0o000)) t.Cleanup(func() { _ = os.Chmod(legacy, 0o644) }) + writeNested(t, tempDir, `{"user_id": 5, "user_level": "free"}`) initAt(t, tempDir) - assert.False(t, Exists(UserIDKey)) - assert.NoFileExists(t, filepath.Join(tempDir, migrationMarkerFileName)) - require.NoError(t, Set(DeviceIDKey, "dev")) - require.NoError(t, os.Chmod(legacy, 0o644)) - initAt(t, tempDir) - assert.Equal(t, "7", GetString(UserIDKey)) + assert.Equal(t, "5", GetString(UserIDKey)) assert.FileExists(t, filepath.Join(tempDir, migrationMarkerFileName)) }) @@ -465,7 +462,8 @@ func TestLegacyMigration(t *testing.T) { t.Run("a failed save is retried on the next start", func(t *testing.T) { tempDir := t.TempDir() - writeCanonical(t, tempDir, `{"locale": "en-US"}`) + // null loads as empty settings but isn't a JSON object, so the legacy identity is adopted. + writeCanonical(t, tempDir, `null`) writeLegacy(t, tempDir, `{"user_id": 2, "user_level": "pro"}`) restore := readOnlyDir(t, tempDir) From a7d899b2a5f550c9995dec0848f05860c86af26d Mon Sep 17 00:00:00 2001 From: garmr-ulfr Date: Mon, 26 Oct 2026 14:40:52 -0700 Subject: [PATCH 8/8] fix: persist the signup salt and skip permission tests as root --- account/user.go | 8 +++++--- account/user_test.go | 3 +++ common/settings/settings_test.go | 11 +++++------ 3 files changed, 13 insertions(+), 9 deletions(-) diff --git a/account/user.go b/account/user.go index a9e9b0cc..d64736dc 100644 --- a/account/user.go +++ b/account/user.go @@ -217,12 +217,14 @@ func (a *Client) SignUp(ctx context.Context, email, password string) ([]byte, *p settings.OAuthLoginKey: false, settings.OAuthProviderKey: "", }) - if err == nil { - a.salt = salt + if err != nil { + a.mu.Unlock() + return nil, nil, traces.RecordError(ctx, fmt.Errorf("saving signup identity: %w", err)) } + err = a.storeSaltLocked(salt) a.mu.Unlock() if err != nil { - return nil, nil, traces.RecordError(ctx, fmt.Errorf("saving signup identity: %w", err)) + return nil, nil, traces.RecordError(ctx, fmt.Errorf("saving signup salt: %w", err)) } return salt, &signupData, nil diff --git a/account/user_test.go b/account/user_test.go index ffe4a4a0..77100d93 100644 --- a/account/user_test.go +++ b/account/user_test.go @@ -382,6 +382,9 @@ func TestSignUp(t *testing.T) { assert.NoError(t, err) assert.NotNil(t, salt) assert.NotNil(t, signupResponse) + saved, err := readSalt(ac.saltPath) + require.NoError(t, err) + assert.Equal(t, salt, saved, "the signup salt must survive a restart") } func TestSignUpKeepsSaltWhenSaveFails(t *testing.T) { diff --git a/common/settings/settings_test.go b/common/settings/settings_test.go index 2c9fd196..66975eca 100644 --- a/common/settings/settings_test.go +++ b/common/settings/settings_test.go @@ -971,23 +971,19 @@ func TestWriteAdoptsOtherProcessState(t *testing.T) { } func TestFailedApplyIsNotRevertedByLaterWrites(t *testing.T) { - if runtime.GOOS == "windows" { - t.Skip("uses a read-only directory to force save failures") - } Reset() t.Cleanup(Reset) dir := t.TempDir() require.NoError(t, InitSettings(dir)) require.NoError(t, Set(DataPathKey, "old")) - require.NoError(t, os.Chmod(dir, 0o500)) - t.Cleanup(func() { os.Chmod(dir, 0o755) }) + restore := readOnlyDir(t, dir) require.Error(t, Apply(Settings{DataPathKey: "new"}), "save must fail in a read-only directory") assert.Equal(t, "new", GetString(DataPathKey), "Apply takes effect despite the failed save") require.Error(t, Set(LocaleKey, "en-US")) assert.Equal(t, "new", GetString(DataPathKey), "a failed later write must not revert it") - require.NoError(t, os.Chmod(dir, 0o755)) + restore() require.NoError(t, Set(LocaleKey, "fa-IR")) assert.Equal(t, "new", GetString(DataPathKey), "a later write must not revert the applied value") @@ -1074,6 +1070,9 @@ func TestUnreadableFileLeavesSettings(t *testing.T) { if runtime.GOOS == "windows" { t.Skip("uses file permissions to force read failures") } + if os.Geteuid() == 0 { + t.Skip("root reads files regardless of mode") + } Reset() t.Cleanup(Reset) dir := t.TempDir()