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..d7cec6d4ec1 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,86 @@ 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 start the Play read without waiting for the Stripe read', async () => { + let stripeSettled = false; + let stripeSettledWhenPlayStarted: boolean | undefined; + mockStripeHelper.fetchCustomer = jest.fn(async () => { + await new Promise((resolve) => setImmediate(resolve)); + stripeSettled = true; + return mockCustomer; + }); + mockPlaySubscriptions.getSubscriptions = jest.fn(async () => { + stripeSettledWhenPlayStarted = stripeSettled; + return []; + }); + + await runTest( + buildRoute(subscriptionsEnabled, playSubscriptionsEnabled), + request, + () => {} + ); + expect(stripeSettledWhenPlayStarted).toBe(false); + }); + + 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 +4495,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..51bf4388b25 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,38 +2233,66 @@ 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 { - const customer = await this.stripeHelper.fetchCustomer(uid as string, [ - 'subscriptions', - ]); - if (customer && customer.subscriptions) { - webSubscriptions = await this.stripeHelper.subscriptionsToResponse( - customer.subscriptions - ); - } - - if (this.config.subscriptions?.playApiServiceAccount?.enabled) { - const playSubscriptions = Container.get(PlaySubscriptions); - iapGooglePlaySubscriptions = ( - await playSubscriptions.getSubscriptions(uid as string) - ).map(playStoreSubscriptionPurchaseToPlayStoreSubscriptionDTO); - } - - if (this.config.subscriptions?.appStore?.enabled) { - 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; - } - } + const stripeHelper = this.stripeHelper; + await Promise.all([ + (async () => { + try { + const customer = await stripeHelper.fetchCustomer(uid as string, [ + 'subscriptions', + ]); + if (customer && customer.subscriptions) { + webSubscriptions = await stripeHelper.subscriptionsToResponse( + customer.subscriptions + ); + } + } catch (err) { + onSubscriptionsError(err); + } + })(), + (async () => { + if (!this.config.subscriptions?.playApiServiceAccount?.enabled) { + return; + } + try { + const playSubscriptions = Container.get(PlaySubscriptions); + iapGooglePlaySubscriptions = ( + await playSubscriptions.getSubscriptions(uid as string) + ).map(playStoreSubscriptionPurchaseToPlayStoreSubscriptionDTO); + } catch (err) { + onSubscriptionsError(err); + } + })(), + (async () => { + if (!this.config.subscriptions?.appStore?.enabled) { + return; + } + try { + const appStoreSubscriptions = Container.get(AppStoreSubscriptions); + iapAppStoreSubscriptions = ( + await appStoreSubscriptions.getSubscriptions(uid as string) + ).map(appStoreSubscriptionPurchaseToAppStoreSubscriptionDTO); + } catch (err) { + onSubscriptionsError(err); + } + })(), + ]); } + const subscriptions = [ + ...iapGooglePlaySubscriptions, + ...iapAppStoreSubscriptions, + ...webSubscriptions, + ]; + return { createdAt: account.createdAt, passwordCreatedAt: account.verifierSetAt, @@ -2277,11 +2306,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( + +