From 73d751b72a9b198de21cdacee666dd4d38b04c6f Mon Sep 17 00:00:00 2001 From: "fxa-agent[bot]" <334546218+fxa-agent[bot]@users.noreply.github.com> Date: Sun, 4 Oct 2026 16:05:32 +0000 Subject: [PATCH] fix(auth,settings): keep account data usable on subscription errors Because: * One failed Stripe, Play or App Store read made GET /v1/account return a 5xx. * Settings rendered defaults on a failed account fetch, which looked like lost account data. * The password row showed a 1969 "Created" date when `passwordCreated` was missing. This commit: * Catches each subscriptions source separately, logs and reports the error, and omits `subscriptions` when a read failed and none loaded. * Treats omitted subscriptions as unknown (`null`) in Settings; Nav keeps the subscriptions link. * Throws on a rejected account fetch so Settings shows its error dialog with the Sentry event ID. * Hides the password "Created" date when `passwordCreated` is 0 or missing. * Adds unit tests per source and case, a Security story, and a functional test for the fetch-error dialog. Closes #FXA-14647 --- .../functional-tests/pages/settings/index.ts | 4 + .../tests/settings/accountFetchError.spec.ts | 39 ++++++ .../lib/routes/account.spec.ts | 125 ++++++++++++++++-- .../fxa-auth-server/lib/routes/account.ts | 42 ++++-- .../components/Settings/Nav/index.test.tsx | 17 +++ .../src/components/Settings/Nav/index.tsx | 4 +- .../Settings/Security/index.stories.tsx | 12 ++ .../Settings/Security/index.test.tsx | 21 +++ .../components/Settings/Security/index.tsx | 2 +- .../src/components/Settings/index.tsx | 7 +- .../fxa-settings/src/lib/account-storage.ts | 4 +- .../lib/hooks/useAccountData/index.test.ts | 55 ++++++++ .../src/lib/hooks/useAccountData/index.ts | 28 ++-- packages/fxa-settings/src/models/Account.ts | 2 +- .../models/contexts/AccountStateContext.tsx | 2 +- 15 files changed, 320 insertions(+), 44 deletions(-) create mode 100644 packages/functional-tests/tests/settings/accountFetchError.spec.ts diff --git a/packages/functional-tests/pages/settings/index.ts b/packages/functional-tests/pages/settings/index.ts index a458609692e..78927d779c9 100644 --- a/packages/functional-tests/pages/settings/index.ts +++ b/packages/functional-tests/pages/settings/index.ts @@ -48,6 +48,10 @@ export class SettingsPage extends SettingsLayout { return this.lazyRow('primary-email', PrimaryEmailRow); } + get errorLoadingApp() { + return this.page.getByTestId('error-loading-app'); + } + get secondaryEmail() { return this.lazyRow('secondary-email', SecondaryEmailRow); } diff --git a/packages/functional-tests/tests/settings/accountFetchError.spec.ts b/packages/functional-tests/tests/settings/accountFetchError.spec.ts new file mode 100644 index 00000000000..1153c16ec0a --- /dev/null +++ b/packages/functional-tests/tests/settings/accountFetchError.spec.ts @@ -0,0 +1,39 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ + +import { expect, test } from '../../lib/fixtures/standard'; + +test.describe('severity-2', () => { + test.beforeEach( + async ({ + target, + page, + pages: { signin, settings }, + testAccountTracker, + }) => { + const credentials = await testAccountTracker.signUp(); + await page.goto(target.contentServerUrl); + await signin.fillOutEmailFirstForm(credentials.email); + await signin.fillOutPasswordForm(credentials.password); + await expect(settings.settingsHeading).toBeVisible(); + } + ); + + test('settings shows the error dialog when the account fetch fails', async ({ + page, + pages: { settings }, + }) => { + await page.route('**/v1/account', (route) => + route.fulfill({ + status: 500, + contentType: 'application/json', + body: JSON.stringify({ code: 500, errno: 999, error: 'Internal' }), + }) + ); + await settings.goto(); + + await expect(settings.errorLoadingApp).toBeVisible(); + await expect(settings.settingsHeading).toBeHidden(); + }); +}); diff --git a/packages/fxa-auth-server/lib/routes/account.spec.ts b/packages/fxa-auth-server/lib/routes/account.spec.ts index f415010911b..1a8a458de0c 100644 --- a/packages/fxa-auth-server/lib/routes/account.spec.ts +++ b/packages/fxa-auth-server/lib/routes/account.spec.ts @@ -4023,6 +4023,10 @@ describe('/account', () => { Container.set(CapabilityService, {}); }); + afterEach(() => { + jest.restoreAllMocks(); + }); + describe('web subscriptions', () => { beforeEach(() => { mockCustomer = { @@ -4076,20 +4080,36 @@ describe('/account', () => { }); }); - it('should propagate other errors from stripe.customer', async () => { + it('should omit subscriptions and report other errors from stripe.customer', async () => { + const sentryModule = require('../sentry'); + const reportSpy = jest + .spyOn(sentryModule, 'reportSentryError') + .mockReturnValue({}); + const stripeError = error.unexpectedError(); mockStripeHelper.fetchCustomer = jest.fn(() => { - throw error.unexpectedError(); + throw stripeError; }); - let failed = false; - try { - await runTest(buildRoute(), request, () => {}); - } catch (err: any) { - failed = true; - expect(err.errno).toBe(error.ERRNO.UNEXPECTED_ERROR); - } + await runTest(buildRoute(), request, (result: any) => { + expect(result).not.toHaveProperty('subscriptions'); + }); + expect(log.error).toHaveBeenCalledWith( + 'Account.get.subscriptions.error', + { err: stripeError } + ); + expect(reportSpy).toHaveBeenCalledWith(stripeError, request); + }); + + it('should not log or report unknownCustomer errors', async () => { + const sentryModule = require('../sentry'); + const reportSpy = jest.spyOn(sentryModule, 'reportSentryError'); + mockStripeHelper.fetchCustomer = jest.fn(() => { + throw error.unknownCustomer(); + }); - expect(failed).toBe(true); + await runTest(buildRoute(), request, () => {}); + expect(log.error).not.toHaveBeenCalled(); + expect(reportSpy).not.toHaveBeenCalled(); }); it('should not return stripe.customer result when subscriptions are disabled', () => { @@ -4239,6 +4259,65 @@ describe('/account', () => { ); }); + it('should return web subscriptions when the Play read fails', async () => { + const sentryModule = require('../sentry'); + const reportSpy = jest + .spyOn(sentryModule, 'reportSentryError') + .mockReturnValue({}); + mockCustomer = { id: 1234, subscriptions: ['fake'] }; + mockWebSubscriptionsResponse = [webSubscription]; + mockStripeHelper.fetchCustomer = jest.fn(async () => mockCustomer); + mockStripeHelper.subscriptionsToResponse = jest.fn( + async () => mockWebSubscriptionsResponse + ); + const playError = new Error('play unavailable'); + mockPlaySubscriptions.getSubscriptions = jest.fn(async () => { + throw playError; + }); + + await runTest( + buildRoute(subscriptionsEnabled, playSubscriptionsEnabled), + request, + (result: any) => { + expect(result.subscriptions).toEqual([webSubscription]); + } + ); + expect(reportSpy).toHaveBeenCalledWith(playError, request); + }); + + it('should return Play subscriptions when the Stripe read fails', async () => { + jest.spyOn(require('../sentry'), 'reportSentryError').mockReturnValue({}); + mockStripeHelper.fetchCustomer = jest.fn(async () => { + throw error.unexpectedError(); + }); + + await runTest( + buildRoute(subscriptionsEnabled, playSubscriptionsEnabled), + request, + (result: any) => { + expect(result.subscriptions).toEqual([ + mockFormattedPlayStoreSubscription, + ]); + } + ); + }); + + it('should omit subscriptions when the Stripe read fails and Play has none', async () => { + jest.spyOn(require('../sentry'), 'reportSentryError').mockReturnValue({}); + mockStripeHelper.fetchCustomer = jest.fn(async () => { + throw error.unexpectedError(); + }); + mockPlaySubscriptions.getSubscriptions = jest.fn(async () => []); + + await runTest( + buildRoute(subscriptionsEnabled, playSubscriptionsEnabled), + request, + (result: any) => { + expect(result).not.toHaveProperty('subscriptions'); + } + ); + }); + it('should not return Play subscriptions when Play subscriptions are disabled', () => { playSubscriptionsEnabled = false; mockCustomer = { @@ -4395,6 +4474,32 @@ describe('/account', () => { ); }); + it('should return web subscriptions when the App Store read fails', async () => { + const sentryModule = require('../sentry'); + const reportSpy = jest + .spyOn(sentryModule, 'reportSentryError') + .mockReturnValue({}); + mockCustomer = { id: 1234, subscriptions: ['fake'] }; + mockWebSubscriptionsResponse = [webSubscription]; + mockStripeHelper.fetchCustomer = jest.fn(async () => mockCustomer); + mockStripeHelper.subscriptionsToResponse = jest.fn( + async () => mockWebSubscriptionsResponse + ); + const appStoreError = new Error('app store unavailable'); + mockAppStoreSubscriptions.getSubscriptions = jest.fn(async () => { + throw appStoreError; + }); + + await runTest( + buildRoute(subscriptionsEnabled, false, appStoreSubscriptionsEnabled), + request, + (result: any) => { + expect(result.subscriptions).toEqual([webSubscription]); + } + ); + expect(reportSpy).toHaveBeenCalledWith(appStoreError, request); + }); + it('should not return App Store subscriptions when App Store subscriptions are disabled', () => { appStoreSubscriptionsEnabled = false; mockCustomer = { diff --git a/packages/fxa-auth-server/lib/routes/account.ts b/packages/fxa-auth-server/lib/routes/account.ts index 52dc26c472c..1516c0951bf 100644 --- a/packages/fxa-auth-server/lib/routes/account.ts +++ b/packages/fxa-auth-server/lib/routes/account.ts @@ -74,6 +74,7 @@ import { escapeLikePattern, } from '@fxa/accounts/email-sender'; import { getClientServiceTags } from '../metrics/client-tags'; +import { reportSentryError } from '../sentry'; const METRICS_CONTEXT_SCHEMA = require('../metrics/context').schema; @@ -2232,6 +2233,13 @@ export class AccountHandler { let webSubscriptions: Awaited = []; let iapGooglePlaySubscriptions: Awaited = []; let iapAppStoreSubscriptions: Awaited = []; + let subscriptionsReadFailed = false; + const onSubscriptionsError = (err: any) => { + if (err.errno === error.ERRNO.UNKNOWN_SUBSCRIPTION_CUSTOMER) return; + subscriptionsReadFailed = true; + this.log.error('Account.get.subscriptions.error', { err }); + reportSentryError(err, request); + }; if (this.config.subscriptions?.enabled && this.stripeHelper) { try { @@ -2243,27 +2251,39 @@ export class AccountHandler { customer.subscriptions ); } + } catch (err) { + onSubscriptionsError(err); + } - if (this.config.subscriptions?.playApiServiceAccount?.enabled) { + if (this.config.subscriptions?.playApiServiceAccount?.enabled) { + try { const playSubscriptions = Container.get(PlaySubscriptions); iapGooglePlaySubscriptions = ( await playSubscriptions.getSubscriptions(uid as string) ).map(playStoreSubscriptionPurchaseToPlayStoreSubscriptionDTO); + } catch (err) { + onSubscriptionsError(err); } + } - if (this.config.subscriptions?.appStore?.enabled) { + if (this.config.subscriptions?.appStore?.enabled) { + try { const appStoreSubscriptions = Container.get(AppStoreSubscriptions); iapAppStoreSubscriptions = ( await appStoreSubscriptions.getSubscriptions(uid as string) ).map(appStoreSubscriptionPurchaseToAppStoreSubscriptionDTO); - } - } catch (err) { - if (err.errno !== error.ERRNO.UNKNOWN_SUBSCRIPTION_CUSTOMER) { - throw err; + } catch (err) { + onSubscriptionsError(err); } } } + const subscriptions = [ + ...iapGooglePlaySubscriptions, + ...iapAppStoreSubscriptions, + ...webSubscriptions, + ]; + return { createdAt: account.createdAt, passwordCreatedAt: account.verifierSetAt, @@ -2277,11 +2297,11 @@ export class AccountHandler { recoveryPhone, securityEvents, passkeys, - subscriptions: [ - ...iapGooglePlaySubscriptions, - ...iapAppStoreSubscriptions, - ...webSubscriptions, - ], + // An empty list after a failed read would mean "none", so it's omitted + // and clients treat subscriptions as unknown. + ...((subscriptions.length > 0 || !subscriptionsReadFailed) && { + subscriptions, + }), }; } } diff --git a/packages/fxa-settings/src/components/Settings/Nav/index.test.tsx b/packages/fxa-settings/src/components/Settings/Nav/index.test.tsx index d25f54f6932..59112df4289 100644 --- a/packages/fxa-settings/src/components/Settings/Nav/index.test.tsx +++ b/packages/fxa-settings/src/components/Settings/Nav/index.test.tsx @@ -76,6 +76,23 @@ describe('Nav', () => { ); }); + it('shows the subscriptions link when subscriptions are unknown', () => { + const account = { + primaryEmail: { email: 'stomlinson@mozilla.com' }, + subscriptions: null, + linkedAccounts: [], + } as unknown as Account; + renderWithLocalizationProvider( + +