Skip to content

Commit

Permalink
fix(sync): Always send firefox.fxaLogout on sign out
Browse files Browse the repository at this point in the history
Because:
* If users are signed into the browser but they've accessed account settings in a non-Sync context, the browser holds onto the old session token, leading to a 'Session Expired' page when they try to access CAD or account settings from the browser

This commit:
* Removes the isSync() check around the firefox.fxaLogout call
  • Loading branch information
LZoog committed Nov 15, 2024
1 parent 6a04b30 commit 834e88c
Show file tree
Hide file tree
Showing 7 changed files with 23 additions and 65 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -7,22 +7,20 @@ import HeaderLockup from '../HeaderLockup';
import ContentSkip from '../ContentSkip';
import Footer from 'fxa-react/components/Footer';
import { AlertBar } from '../AlertBar';
import { SettingsIntegration } from '../interfaces';

type AppLayoutProps = {
children: React.ReactNode;
integration: SettingsIntegration;
};

export const AppLayout = ({ children, integration }: AppLayoutProps) => {
export const AppLayout = ({ children }: AppLayoutProps) => {
return (
<div
className="flex flex-col justify-between min-h-screen"
data-testid="app"
>
<ContentSkip />
<div id="body-top" className="hidden mobileLandscape:block" />
<HeaderLockup {...{ integration }} />
<HeaderLockup />
<div className="max-w-screen-desktopXl flex-1 w-full mx-auto tablet:px-20 desktop:px-12">
<main id="main" data-testid="main" className="w-full">
<AlertBar />
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ import { withLocalization } from 'fxa-react/lib/storybooks';
import DropDownAvatarMenu from '.';
import { Account, AppContext } from 'fxa-settings/src/models';
import { mockAppContext, MOCK_ACCOUNT } from 'fxa-settings/src/models/mocks';
import { createMockSettingsIntegration } from '../mocks';

export default {
title: 'Components/Settings/DropDownAvatarMenu',
Expand All @@ -35,14 +34,12 @@ const accountWithoutAvatar = {
},
} as unknown as Account;

const integration = createMockSettingsIntegration();

const storyWithContext = (account: Partial<Account>) => {
const context = { account: account as Account };

const story = () => (
<AppContext.Provider value={mockAppContext(context)}>
<DropDownAvatarMenu {...{ integration }} />
<DropDownAvatarMenu />
</AppContext.Provider>
);
return story;
Expand All @@ -51,6 +48,4 @@ const storyWithContext = (account: Partial<Account>) => {
export const DefaultNoAvatarOrDisplayName =
storyWithContext(accountWithoutAvatar);

export const WithAvatarAndDisplayName = () => (
<DropDownAvatarMenu {...{ integration }} />
);
export const WithAvatarAndDisplayName = () => <DropDownAvatarMenu />;
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ import DropDownAvatarMenu from '.';
import { logViewEvent, settingsViewName } from 'fxa-settings/src/lib/metrics';
import { Account, AppContext } from '../../../models';
import { SettingsContext } from '../../../models/contexts/SettingsContext';
import { createMockSettingsIntegration } from '../mocks';
import firefox from '../../../lib/channels/firefox';

jest.mock('../../../models/AlertBarInfo');
Expand Down Expand Up @@ -48,7 +47,7 @@ describe('DropDownAvatarMenu', () => {
} as unknown as Account;
renderWithLocalizationProvider(
<AppContext.Provider value={mockAppContext({ account })}>
<DropDownAvatarMenu integration={createMockSettingsIntegration()} />
<DropDownAvatarMenu />
</AppContext.Provider>
);

Expand All @@ -75,7 +74,7 @@ describe('DropDownAvatarMenu', () => {
it('renders as expected with avatar url and displayName set', () => {
renderWithLocalizationProvider(
<AppContext.Provider value={mockAppContext({ account })}>
<DropDownAvatarMenu integration={createMockSettingsIntegration()} />
<DropDownAvatarMenu />
</AppContext.Provider>
);
fireEvent.click(screen.getByTestId('drop-down-avatar-menu-toggle'));
Expand All @@ -87,7 +86,7 @@ describe('DropDownAvatarMenu', () => {
it('closes on esc keypress', () => {
renderWithLocalizationProvider(
<AppContext.Provider value={mockAppContext({ account })}>
<DropDownAvatarMenu integration={createMockSettingsIntegration()} />
<DropDownAvatarMenu />
</AppContext.Provider>
);

Expand All @@ -102,7 +101,7 @@ describe('DropDownAvatarMenu', () => {
<AppContext.Provider value={mockAppContext({ account })}>
<div className="w-full flex justify-end">
<div className="flex pr-10 pt-4">
<DropDownAvatarMenu integration={createMockSettingsIntegration()} />
<DropDownAvatarMenu />
</div>
</div>
</AppContext.Provider>
Expand All @@ -127,7 +126,7 @@ describe('DropDownAvatarMenu', () => {
<AppContext.Provider
value={mockAppContext({ account, session: mockSession() })}
>
<DropDownAvatarMenu integration={createMockSettingsIntegration()} />
<DropDownAvatarMenu />
</AppContext.Provider>
);

Expand All @@ -153,7 +152,7 @@ describe('DropDownAvatarMenu', () => {
renderWithLocalizationProvider(
<AppContext.Provider value={context}>
<SettingsContext.Provider value={settingsContext}>
<DropDownAvatarMenu integration={createMockSettingsIntegration()} />
<DropDownAvatarMenu />
</SettingsContext.Provider>
</AppContext.Provider>
);
Expand All @@ -174,14 +173,12 @@ describe('DropDownAvatarMenu', () => {
afterEach(() => {
jest.restoreAllMocks();
});
it('is called integration is sync', async () => {
it('is called', async () => {
renderWithLocalizationProvider(
<AppContext.Provider
value={mockAppContext({ account, session: mockSession() })}
>
<DropDownAvatarMenu
integration={createMockSettingsIntegration({ isSync: true })}
/>
<DropDownAvatarMenu />
</AppContext.Provider>
);
fireEvent.click(screen.getByTestId('drop-down-avatar-menu-toggle'));
Expand All @@ -190,20 +187,5 @@ describe('DropDownAvatarMenu', () => {
});
expect(fxaLogoutSpy).toBeCalledWith({ uid: account.uid });
});

it('is not called when integration is not sync', async () => {
renderWithLocalizationProvider(
<AppContext.Provider
value={mockAppContext({ account, session: mockSession() })}
>
<DropDownAvatarMenu integration={createMockSettingsIntegration()} />
</AppContext.Provider>
);
fireEvent.click(screen.getByTestId('drop-down-avatar-menu-toggle'));
await act(async () => {
fireEvent.click(screen.getByTestId('avatar-menu-sign-out'));
});
expect(fxaLogoutSpy).not.toBeCalled();
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -11,13 +11,8 @@ import { ReactComponent as SignOut } from './sign-out.svg';
import { logViewEvent, settingsViewName } from '../../../lib/metrics';
import { Localized, useLocalization } from '@fluent/react';
import firefox from '../../../lib/channels/firefox';
import { SettingsIntegration } from '../interfaces';

export const DropDownAvatarMenu = ({
integration,
}: {
integration: SettingsIntegration;
}) => {
export const DropDownAvatarMenu = () => {
const { displayName, primaryEmail, avatar, uid } = useAccount();
const session = useSession();
const [isRevealed, setRevealed] = useState(false);
Expand All @@ -39,9 +34,10 @@ export const DropDownAvatarMenu = ({
try {
await session.destroy();

if (integration.isSync()) {
firefox.fxaLogout({ uid });
}
// Send a logout event to Firefox even if the user is in a non-Sync flow.
// If the user is signed into the browser, they need to drop the now
// destroyed session token.
firefox.fxaLogout({ uid });

logViewEvent(settingsViewName, 'signout.success');
window.location.assign(window.location.origin);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ import { withLocalization } from 'fxa-react/lib/storybooks';
import { HeaderLockup } from '.';
import { Account, AppContext } from '../../../models';
import { mockAppContext, MOCK_ACCOUNT } from 'fxa-settings/src/models/mocks';
import { createMockSettingsIntegration } from '../mocks';

export default {
title: 'Components/Settings/HeaderLockup',
Expand All @@ -26,19 +25,17 @@ const accountWithoutAvatar = {
},
} as unknown as Account;

const integration = createMockSettingsIntegration();

const storyWithContext = (account: Partial<Account>) => {
const context = { account: account as Account };

const story = () => (
<AppContext.Provider value={mockAppContext(context)}>
<HeaderLockup {...{ integration }} />
<HeaderLockup />
</AppContext.Provider>
);
return story;
};

export const WithDefaultAvatar = storyWithContext(accountWithoutAvatar);

export const WithCustomAvatar = () => <HeaderLockup {...{ integration }} />;
export const WithCustomAvatar = () => <HeaderLockup />;
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import React from 'react';
import { screen } from '@testing-library/react';
import { renderWithLocalizationProvider } from 'fxa-react/lib/test-utils/localizationProvider';
import HeaderLockup from '.';
import { createMockSettingsIntegration } from '../mocks';
import { userEvent } from '@testing-library/user-event';
import GleanMetrics from '../../../lib/glean';

Expand All @@ -21,9 +20,7 @@ jest.mock('../../../lib/glean', () => ({

describe('HeaderLockup', () => {
it('renders as expected', () => {
renderWithLocalizationProvider(
<HeaderLockup integration={createMockSettingsIntegration()} />
);
renderWithLocalizationProvider(<HeaderLockup />);
const headerMenu = screen.getByTestId('header-menu');

expect(
Expand All @@ -45,9 +42,7 @@ describe('HeaderLockup', () => {
});

it('emits Glean event on help link click', async () => {
renderWithLocalizationProvider(
<HeaderLockup integration={createMockSettingsIntegration()} />
);
renderWithLocalizationProvider(<HeaderLockup />);
await userEvent.click(screen.getByRole('link', { name: 'Help' }));
expect(GleanMetrics.accountPref.help).toHaveBeenCalled();
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,15 +12,10 @@ import DropDownAvatarMenu from '../DropDownAvatarMenu';
import { ReactComponent as Help } from './help.svg';
import { ReactComponent as Menu } from './menu.svg';
import { ReactComponent as Close } from './close.svg';
import { SettingsIntegration } from '../interfaces';
import Sidebar from '../Sidebar';
import GleanMetrics from '../../../lib/glean';

export const HeaderLockup = ({
integration,
}: {
integration: SettingsIntegration;
}) => {
export const HeaderLockup = () => {
const [sidebarRevealedState, setNavState] = useState(false);
const { l10n } = useLocalization();
const localizedHelpText = l10n.getString('header-help', null, 'Help');
Expand Down Expand Up @@ -87,7 +82,7 @@ export const HeaderLockup = ({
/>
</LinkExternal>
<BentoMenu />
<DropDownAvatarMenu {...{ integration }} />
<DropDownAvatarMenu />
</>
);

Expand Down

0 comments on commit 834e88c

Please sign in to comment.