diff --git a/packages/fxa-settings/src/components/Settings/ConnectedServices/index.test.tsx b/packages/fxa-settings/src/components/Settings/ConnectedServices/index.test.tsx index f98249832c8..e05e58aad83 100644 --- a/packages/fxa-settings/src/components/Settings/ConnectedServices/index.test.tsx +++ b/packages/fxa-settings/src/components/Settings/ConnectedServices/index.test.tsx @@ -5,12 +5,7 @@ import React from 'react'; import { act, fireEvent, screen } from '@testing-library/react'; import ConnectedServices, { sortAndFilterConnectedClients } from '.'; -import { - Account, - AlertBarInfo, - AppContext, - OAuthNativeClients, -} from '../../../models'; +import { Account, AlertBarInfo, AppContext } from '../../../models'; import { renderWithRouter, mockAppContext, @@ -158,9 +153,13 @@ describe('Connected Services', () => { expect(result[result.length - 1]).toHaveTextContent('6 months ago'); }); - const { sortedAndUniqueClients, groupedByName } = + const { sortedAndUniqueClients } = sortAndFilterConnectedClients(MOCK_SERVICES); + const monitorClients = MOCK_SERVICES.filter( + (item) => item.name === 'Mozilla Monitor' + ); + expect(sortedAndUniqueClients.length).toEqual(14); expect( @@ -172,7 +171,7 @@ describe('Connected Services', () => { (item) => item.name === 'Mozilla Monitor' )[0].lastAccessTime ).toEqual(1570736983000); - expect(groupedByName['Mozilla Monitor'].length).toEqual(2); + expect(monitorClients.length).toEqual(2); }); it('should show the monitor icon and link', async () => { @@ -608,85 +607,4 @@ describe('Connected Services', () => { expect(mockWindowAssign).toHaveBeenCalledWith('foo-bar/signin'); }); }); - - describe('scope-based sub row', () => { - const baseMockClient = { - deviceId: null, - sessionTokenId: null, - refreshTokenId: 'abc123', - isCurrentSession: false, - deviceType: null, - createdTime: 1571412069000, - lastAccessTime: 1571412069000, - location: { city: null, country: null, state: null, stateCode: null }, - userAgent: '', - os: null, - createdTimeFormatted: 'a month ago', - lastAccessTimeFormatted: 'a month ago', - approximateLastAccessTime: null, - approximateLastAccessTimeFormatted: null, - }; - - const renderWithClient = (client: Record) => { - const account = { - attachedClients: [client], - disconnectClient: jest.fn().mockResolvedValue(true), - } as unknown as Account; - renderWithRouter( - - - - ); - }; - - it('does not render scope sub-entry when scope is null and client ID is OAuthNative', async () => { - renderWithClient({ - ...baseMockClient, - clientId: OAuthNativeClients.FirefoxDesktop, - name: 'Firefox Desktop', - scope: null, - }); - await screen.findByTestId('settings-connected-service'); - expect(screen.queryByTestId('scope-service')).not.toBeInTheDocument(); - }); - - it('does not render scope sub-entry when scope has oldsync and profile but no relay', async () => { - renderWithClient({ - ...baseMockClient, - clientId: OAuthNativeClients.FirefoxDesktop, - name: 'Firefox Desktop', - scope: ['https://identity.mozilla.com/apps/oldsync', 'profile'], - }); - await screen.findByTestId('settings-connected-service'); - expect(screen.queryByTestId('scope-service')).not.toBeInTheDocument(); - }); - - it('renders Relay scope sub-entry when scope includes relay and client ID is OAuthNative', async () => { - renderWithClient({ - ...baseMockClient, - clientId: OAuthNativeClients.FirefoxIOS, - name: 'Firefox for iOS', - scope: [ - 'https://identity.mozilla.com/apps/oldsync', - 'profile', - 'https://identity.mozilla.com/apps/relay', - ], - }); - await screen.findByTestId('settings-connected-service'); - const scopeEntry = screen.getByTestId('scope-service'); - expect(scopeEntry).toBeInTheDocument(); - expect(scopeEntry).toHaveTextContent('Firefox Relay'); - }); - - it('does not render scope sub-entry when scope includes relay but client ID is not OAuthNative', async () => { - renderWithClient({ - ...baseMockClient, - clientId: '9ebfe2c2f9ea3c58', - name: 'Firefox Relay', - scope: ['profile', 'https://identity.mozilla.com/apps/relay'], - }); - await screen.findByTestId('settings-connected-service'); - expect(screen.queryByTestId('scope-service')).not.toBeInTheDocument(); - }); - }); }); diff --git a/packages/fxa-settings/src/components/Settings/ConnectedServices/index.tsx b/packages/fxa-settings/src/components/Settings/ConnectedServices/index.tsx index 0f90f618de2..4eb50236cb1 100644 --- a/packages/fxa-settings/src/components/Settings/ConnectedServices/index.tsx +++ b/packages/fxa-settings/src/components/Settings/ConnectedServices/index.tsx @@ -2,12 +2,14 @@ * 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 React from 'react'; +import { ApolloError } from '@apollo/client'; import { Localized, useLocalization } from '@fluent/react'; import { LinkExternal } from 'fxa-react/components/LinkExternal'; import { useBooleanState } from 'fxa-react/lib/hooks'; import groupBy from 'lodash.groupby'; import { forwardRef, useCallback, useState } from 'react'; -import { clearSignedInAccountUid, setSigningOut } from '../../../lib/cache'; +import { clearSignedInAccountUid } from '../../../lib/cache'; import { logViewEvent } from '../../../lib/metrics'; import { isMobileDevice } from '../../../lib/utilities'; import { AttachedClient, useAccount, useAlertBar } from '../../../models'; @@ -27,12 +29,14 @@ const DEVICES_SUPPORT_URL = export function sortAndFilterConnectedClients( attachedClients: Array ) { - const groupedByName = groupBy(attachedClients, 'name'); + // Group clients by deviceId (for sync devices) or name (for others). + // This avoids merging distinct devices that happen to share the same name. + const groupedByDevice = groupBy(attachedClients, (c) => c.deviceId || c.name); - // get a unique (by name) list and sort by time last accessed - const sortedAndUniqueClients = Object.keys(groupedByName) + // get a unique (by device or name) list and sort by time last accessed + const sortedAndUniqueClients = Object.keys(groupedByDevice) .map((key) => { - return groupedByName[key].sort( + return groupedByDevice[key].sort( (a: AttachedClient, b: AttachedClient) => b.lastAccessTime - a.lastAccessTime )[0]; @@ -47,14 +51,14 @@ export function sortAndFilterConnectedClients( } }); - return { groupedByName, sortedAndUniqueClients }; + return { groupedByDevice, sortedAndUniqueClients }; } export const ConnectedServices = forwardRef((_, ref) => { const alertBar = useAlertBar(); const account = useAccount(); const attachedClients = account.attachedClients; - const { groupedByName, sortedAndUniqueClients } = + const { groupedByDevice, sortedAndUniqueClients } = sortAndFilterConnectedClients([...attachedClients]); const showMobilePromo = !sortedAndUniqueClients.filter(isMobileDevice).length; @@ -82,7 +86,7 @@ export const ConnectedServices = forwardRef((_, ref) => { const [isRefreshingClients, setIsRefreshingClients] = useState(false); const clearDisconnectingState = useCallback( - (errorMessage?: string, error?: Error) => { + (errorMessage?: string, error?: ApolloError | Error) => { hideConfirmDisconnectModal(); setSelectedClient(null); setReason(''); @@ -102,17 +106,15 @@ export const ConnectedServices = forwardRef((_, ref) => { event: { reason: reasonValue }, }); - // disconnect all clients/sessions with this name since only unique names - // are displayed to the user. This is batched into one network request - // via BatchHttpLink - const groupByKey = client.name ?? 'undefined'; - const clientsWithMatchingName = groupedByName[groupByKey]; - const hasMultipleSessions = clientsWithMatchingName.length > 1; + // Disconnect all clients/sessions in the group (same deviceId or name). + // Since we only display one entry per group, signing out of that entry + // should revoke all associated sessions. This is batched via BatchHttpLink. + const groupByKey = (client.deviceId || client.name) ?? 'undefined'; + const sessionsInGroup = groupedByDevice[groupByKey]; + const hasMultipleSessions = sessionsInGroup.length > 1; if (hasMultipleSessions) { await Promise.all( - clientsWithMatchingName.map( - async (c) => await account.disconnectClient(c) - ) + sessionsInGroup.map(async (c) => await account.disconnectClient(c)) ); } else { await account.disconnectClient(client); @@ -123,9 +125,8 @@ export const ConnectedServices = forwardRef((_, ref) => { if ( client.isCurrentSession || (hasMultipleSessions && - clientsWithMatchingName.find((c) => c.isCurrentSession)) + sessionsInGroup.find((c) => c.isCurrentSession)) ) { - setSigningOut(true); clearSignedInAccountUid(); window.location.assign(`${window.location.origin}/signin`); } else if (reason === 'suspicious' || reason === 'lost') { @@ -150,7 +151,7 @@ export const ConnectedServices = forwardRef((_, ref) => { [ account, hideConfirmDisconnectModal, - groupedByName, + groupedByDevice, revealAdviceModal, alertBar, l10n, @@ -228,7 +229,7 @@ export const ConnectedServices = forwardRef((_, ref) => { {!!sortedAndUniqueClients.length && - sortedAndUniqueClients.map((client) => ( + sortedAndUniqueClients.map((client, i) => (