From a91ce9b76051ebd9dd955f788b56bcb923a00168 Mon Sep 17 00:00:00 2001 From: Kerry Archibald Date: Tue, 6 Dec 2022 22:40:34 +1300 Subject: [PATCH 1/5] add handling for unverifiable sessions --- .../settings/devices/DeviceSecurityCard.tsx | 1 + .../devices/DeviceSecurityLearnMore.tsx | 20 ++++++++ .../devices/DeviceVerificationStatusCard.tsx | 50 ++++++++++++++----- .../settings/devices/FilteredDeviceList.tsx | 6 +++ .../views/settings/devices/filter.ts | 6 ++- .../views/settings/devices/types.ts | 3 ++ src/i18n/strings/en_EN.json | 6 ++- 7 files changed, 77 insertions(+), 15 deletions(-) diff --git a/src/components/views/settings/devices/DeviceSecurityCard.tsx b/src/components/views/settings/devices/DeviceSecurityCard.tsx index 3e4eadb3229..aaf0a5362fe 100644 --- a/src/components/views/settings/devices/DeviceSecurityCard.tsx +++ b/src/components/views/settings/devices/DeviceSecurityCard.tsx @@ -32,6 +32,7 @@ const VariationIcon: Record = ({ variation }) => { diff --git a/src/components/views/settings/devices/DeviceSecurityLearnMore.tsx b/src/components/views/settings/devices/DeviceSecurityLearnMore.tsx index 2c10b02eccf..57fe23b0863 100644 --- a/src/components/views/settings/devices/DeviceSecurityLearnMore.tsx +++ b/src/components/views/settings/devices/DeviceSecurityLearnMore.tsx @@ -56,6 +56,26 @@ const securityCardContent: Record , }, + // unverifiable uses single-session case + // because it is only ever displayed on a single session detail + [DeviceSecurityVariation.Unverifiable]: { + title: _t('Unverified session'), + description: <> +

{ _t(`This session doesn't support encryption, so it can't be verified.`) } +

+

+ { _t( + `You won't be able to participate in rooms where encryption is enabled when using this session.`, + ) + } +

+ { _t( + `For best security and privacy, it is recommended to use Matrix clients that support encryption.`, + ) + } +

+ , + }, [DeviceSecurityVariation.Inactive]: { title: _t('Inactive sessions'), description: <> diff --git a/src/components/views/settings/devices/DeviceVerificationStatusCard.tsx b/src/components/views/settings/devices/DeviceVerificationStatusCard.tsx index 0ee37c9bc43..6740175ff6d 100644 --- a/src/components/views/settings/devices/DeviceVerificationStatusCard.tsx +++ b/src/components/views/settings/devices/DeviceVerificationStatusCard.tsx @@ -30,18 +30,33 @@ interface Props { onVerifyDevice?: () => void; } -export const DeviceVerificationStatusCard: React.FC = ({ - device, - onVerifyDevice, -}) => { - const securityCardProps = device.isVerified ? { - variation: DeviceSecurityVariation.Verified, - heading: _t('Verified session'), - description: <> - { _t('This session is ready for secure messaging.') } - - , - } : { +const getCardProps = (device: ExtendedDevice): { + variation: DeviceSecurityVariation; + heading: string; + description: React.ReactNode; +} => { + if (device.isVerified) { + return { + variation: DeviceSecurityVariation.Verified, + heading: _t('Verified session'), + description: <> + { _t('This session is ready for secure messaging.') } + + , + }; + } + if (device.isVerified === null) { + return { + variation: DeviceSecurityVariation.Unverified, + heading: _t('Unverified session'), + description: <> + { _t(`This session doesn't support encryption and thus can't be verified.`) } + + , + }; + } + + return { variation: DeviceSecurityVariation.Unverified, heading: _t('Unverified session'), description: <> @@ -49,10 +64,19 @@ export const DeviceVerificationStatusCard: React.FC = ({ , }; +}; + +export const DeviceVerificationStatusCard: React.FC = ({ + device, + onVerifyDevice, +}) => { + const securityCardProps = getCardProps(device); + return - { !device.isVerified && !!onVerifyDevice && + { /* check for explicit false to exclude unverifiable devices */ } + { device.isVerified === false && !!onVerifyDevice && !!device.last_seen_ts && device.last_seen_ts < Date.now() - INACTIVE_DEVICE_AGE_MS; -const filters: Record = { +const filters: Record = { [DeviceSecurityVariation.Verified]: device => !!device.isVerified, [DeviceSecurityVariation.Unverified]: device => !device.isVerified, [DeviceSecurityVariation.Inactive]: isDeviceInactive, diff --git a/src/components/views/settings/devices/types.ts b/src/components/views/settings/devices/types.ts index 3fa125a09f8..99afd5c759d 100644 --- a/src/components/views/settings/devices/types.ts +++ b/src/components/views/settings/devices/types.ts @@ -32,4 +32,7 @@ export enum DeviceSecurityVariation { Verified = 'Verified', Unverified = 'Unverified', Inactive = 'Inactive', + // sessions that do not support encryption + // eg a session that logged in via api to get an access token + Unverifiable = 'Unverifiable' } diff --git a/src/i18n/strings/en_EN.json b/src/i18n/strings/en_EN.json index 4c8e4d33c36..fc5a424fbc0 100644 --- a/src/i18n/strings/en_EN.json +++ b/src/i18n/strings/en_EN.json @@ -1795,6 +1795,10 @@ "Unverified sessions": "Unverified sessions", "Unverified sessions are sessions that have logged in with your credentials but have not been cross-verified.": "Unverified sessions are sessions that have logged in with your credentials but have not been cross-verified.", "You should make especially certain that you recognise these sessions as they could represent an unauthorised use of your account.": "You should make especially certain that you recognise these sessions as they could represent an unauthorised use of your account.", + "Unverified session": "Unverified session", + "This session doesn't support encryption, so it can't be verified.": "This session doesn't support encryption, so it can't be verified.", + "You won't be able to participate in rooms where encryption is enabled when using this session.": "You won't be able to participate in rooms where encryption is enabled when using this session.", + "For best security and privacy, it is recommended to use Matrix clients that support encryption.": "For best security and privacy, it is recommended to use Matrix clients that support encryption.", "Inactive sessions": "Inactive sessions", "Inactive sessions are sessions you have not used in some time, but they continue to receive encryption keys.": "Inactive sessions are sessions you have not used in some time, but they continue to receive encryption keys.", "Removing inactive sessions improves security and performance, and makes it easier for you to identify if a new session is suspicious.": "Removing inactive sessions improves security and performance, and makes it easier for you to identify if a new session is suspicious.", @@ -1807,7 +1811,7 @@ "Unknown session type": "Unknown session type", "Verified session": "Verified session", "This session is ready for secure messaging.": "This session is ready for secure messaging.", - "Unverified session": "Unverified session", + "This session doesn't support encryption and thus can't be verified.": "This session doesn't support encryption and thus can't be verified.", "Verify or sign out from this session for best security and reliability.": "Verify or sign out from this session for best security and reliability.", "Verify session": "Verify session", "For best security, sign out from any session that you don't recognize or use anymore.": "For best security, sign out from any session that you don't recognize or use anymore.", From 01e77a2cd21907ecaad1ffbd1358dd6fa0460c33 Mon Sep 17 00:00:00 2001 From: Kerry Archibald Date: Wed, 7 Dec 2022 21:37:23 +1300 Subject: [PATCH 2/5] test --- .../tabs/user/SessionManagerTab-test.tsx | 73 +++++++++++-- .../SessionManagerTab-test.tsx.snap | 103 +++++++----------- 2 files changed, 102 insertions(+), 74 deletions(-) diff --git a/test/components/views/settings/tabs/user/SessionManagerTab-test.tsx b/test/components/views/settings/tabs/user/SessionManagerTab-test.tsx index 3c4bfd470f6..58839d5e96c 100644 --- a/test/components/views/settings/tabs/user/SessionManagerTab-test.tsx +++ b/test/components/views/settings/tabs/user/SessionManagerTab-test.tsx @@ -255,12 +255,23 @@ describe('', () => { }); it('sets device verification status correctly', async () => { - mockClient.getDevices.mockResolvedValue({ devices: [alicesDevice, alicesMobileDevice] }); + mockClient.getDevices.mockResolvedValue({ devices: + [alicesDevice, alicesMobileDevice, alicesOlderMobileDevice], + }); + mockClient.getStoredDevice.mockImplementation((_userId, deviceId) => new DeviceInfo(deviceId)); mockCrossSigningInfo.checkDeviceTrust - // alices device is trusted - .mockReturnValueOnce(new DeviceTrustLevel(true, true, false, false)) - // alices mobile device is not - .mockReturnValueOnce(new DeviceTrustLevel(false, false, false, false)); + .mockImplementation((_userId, { deviceId }) => { + // alices device is trusted + if (deviceId === alicesDevice.device_id) { + return new DeviceTrustLevel(true, true, false, false); + } + // alices mobile device is not + if (deviceId === alicesMobileDevice.device_id) { + return new DeviceTrustLevel(false, false, false, false); + } + // alicesOlderMobileDevice does not support encryption + throw new Error('encryption not supported'); + }); const { getByTestId } = render(getComponent()); @@ -268,8 +279,20 @@ describe('', () => { await flushPromises(); }); - expect(mockCrossSigningInfo.checkDeviceTrust).toHaveBeenCalledTimes(2); - expect(getByTestId(`device-tile-${alicesDevice.device_id}`)).toMatchSnapshot(); + expect(mockCrossSigningInfo.checkDeviceTrust).toHaveBeenCalledTimes(3); + expect( + getByTestId(`device-tile-${alicesDevice.device_id}`) + .querySelector('[aria-label="Verified"]'), + ).toBeTruthy(); + expect( + getByTestId(`device-tile-${alicesMobileDevice.device_id}`) + .querySelector('[aria-label="Unverified"]'), + ).toBeTruthy(); + // sessions that dont support encryption use unverified badge + expect( + getByTestId(`device-tile-${alicesOlderMobileDevice.device_id}`) + .querySelector('[aria-label="Unverified"]'), + ).toBeTruthy(); }); it('extends device with client information when available', async () => { @@ -489,7 +512,7 @@ describe('', () => { if (deviceId === alicesDevice.device_id) { return new DeviceTrustLevel(true, true, false, false); } - throw new Error('everything else unverified'); + return new DeviceTrustLevel(false, false, false, false); }); const { getByTestId } = render(getComponent()); @@ -507,6 +530,38 @@ describe('', () => { expect(modalSpy).toHaveBeenCalled(); }); + it('does not allow device verification on session that do not support encryption', async () => { + mockClient.getDevices.mockResolvedValue({ devices: [alicesDevice, alicesMobileDevice] }); + mockClient.getStoredDevice.mockImplementation((_userId, deviceId) => new DeviceInfo(deviceId)); + mockCrossSigningInfo.checkDeviceTrust + .mockImplementation((_userId, { deviceId }) => { + // current session verified = able to verify other sessions + if (deviceId === alicesDevice.device_id) { + return new DeviceTrustLevel(true, true, false, false); + } + // but alicesMobileDevice doesn't support encryption + throw new Error('encryption not supported'); + }); + + const { + getByTestId, + queryByTestId, + } = render(getComponent()); + + await act(async () => { + await flushPromises(); + }); + + toggleDeviceDetails(getByTestId, alicesMobileDevice.device_id); + + // no verify button + expect(queryByTestId(`verification-status-button-${alicesMobileDevice.device_id}`)).toBeFalsy(); + expect( + getByTestId(`device-detail-${alicesMobileDevice.device_id}`) + .getElementsByClassName('mx_DeviceSecurityCard'), + ).toMatchSnapshot(); + }); + it('refreshes devices after verifying other device', async () => { const modalSpy = jest.spyOn(Modal, 'createDialog'); @@ -518,7 +573,7 @@ describe('', () => { if (deviceId === alicesDevice.device_id) { return new DeviceTrustLevel(true, true, false, false); } - throw new Error('everything else unverified'); + return new DeviceTrustLevel(false, false, false, false); }); const { getByTestId } = render(getComponent()); diff --git a/test/components/views/settings/tabs/user/__snapshots__/SessionManagerTab-test.tsx.snap b/test/components/views/settings/tabs/user/__snapshots__/SessionManagerTab-test.tsx.snap index a92f68c756f..166f54a32e3 100644 --- a/test/components/views/settings/tabs/user/__snapshots__/SessionManagerTab-test.tsx.snap +++ b/test/components/views/settings/tabs/user/__snapshots__/SessionManagerTab-test.tsx.snap @@ -1,5 +1,43 @@ // Jest Snapshot v1, https://goo.gl/fbAQLP +exports[` Device verification does not allow device verification on session that do not support encryption 1`] = ` +HTMLCollection [ +
+
+
+
+
+

+ Unverified session +

+

+ This session doesn't support encryption and thus can't be verified. +

+ Learn more +
+

+
+
, +] +`; + exports[` Sign out Signs out of current device 1`] = `
goes to filtered list from security recommendatio
`; - -exports[` sets device verification status correctly 1`] = ` -
-
-
- - -
-

- Alices device -

-
- - Verified - - ยท - - alices_device - -
-
-
-
-
-
-
-
-`; From 2e16e4c4394f85c5746cda3afa6d8a52e25453ed Mon Sep 17 00:00:00 2001 From: Kerry Archibald Date: Wed, 7 Dec 2022 22:03:17 +1300 Subject: [PATCH 3/5] update types for filtervariation --- .../settings/devices/FilteredDeviceList.tsx | 23 +++++++++++-------- .../views/settings/devices/filter.ts | 2 +- .../settings/tabs/user/SessionManagerTab.tsx | 7 +++--- 3 files changed, 19 insertions(+), 13 deletions(-) diff --git a/src/components/views/settings/devices/FilteredDeviceList.tsx b/src/components/views/settings/devices/FilteredDeviceList.tsx index 1f6f2bc6ecc..b702a456f2d 100644 --- a/src/components/views/settings/devices/FilteredDeviceList.tsx +++ b/src/components/views/settings/devices/FilteredDeviceList.tsx @@ -27,6 +27,7 @@ import { DeviceExpandDetailsButton } from './DeviceExpandDetailsButton'; import DeviceSecurityCard from './DeviceSecurityCard'; import { filterDevicesBySecurityRecommendation, + FilterVariation, INACTIVE_DEVICE_AGE_DAYS, } from './filter'; import SelectableDeviceTile from './SelectableDeviceTile'; @@ -47,8 +48,8 @@ interface Props { expandedDeviceIds: ExtendedDevice['device_id'][]; signingOutDeviceIds: ExtendedDevice['device_id'][]; selectedDeviceIds: ExtendedDevice['device_id'][]; - filter?: DeviceSecurityVariation; - onFilterChange: (filter: DeviceSecurityVariation | undefined) => void; + filter?: FilterVariation; + onFilterChange: (filter: FilterVariation | undefined) => void; onDeviceExpandToggle: (deviceId: ExtendedDevice['device_id']) => void; onSignOutDevices: (deviceIds: ExtendedDevice['device_id'][]) => void; saveDeviceName: DevicesState['saveDeviceName']; @@ -68,12 +69,12 @@ const sortDevicesByLatestActivityThenDisplayName = (left: ExtendedDevice, right: (right.last_seen_ts || 0) - (left.last_seen_ts || 0) || ((left.display_name || left.device_id).localeCompare(right.display_name || right.device_id)); -const getFilteredSortedDevices = (devices: DevicesDictionary, filter?: DeviceSecurityVariation) => +const getFilteredSortedDevices = (devices: DevicesDictionary, filter?: FilterVariation) => filterDevicesBySecurityRecommendation(Object.values(devices), filter ? [filter] : []) .sort(sortDevicesByLatestActivityThenDisplayName); const ALL_FILTER_ID = 'ALL'; -type DeviceFilterKey = DeviceSecurityVariation | typeof ALL_FILTER_ID; +type DeviceFilterKey = FilterVariation | typeof ALL_FILTER_ID; const securityCardContent: Record - Object.values(DeviceSecurityVariation).includes(filter); +const isSecurityVariation = (filter?: DeviceFilterKey): filter is FilterVariation => + ([ + DeviceSecurityVariation.Inactive, + DeviceSecurityVariation.Unverified, + DeviceSecurityVariation.Verified, + ] as string[]).includes(filter); const FilterSecurityCard: React.FC<{ filter?: DeviceFilterKey }> = ({ filter }) => { if (isSecurityVariation(filter)) { @@ -130,7 +135,7 @@ const FilterSecurityCard: React.FC<{ filter?: DeviceFilterKey }> = ({ filter }) return null; }; -const getNoResultsMessage = (filter?: DeviceSecurityVariation): string => { +const getNoResultsMessage = (filter?: FilterVariation): string => { switch (filter) { case DeviceSecurityVariation.Verified: return _t('No verified sessions found.'); @@ -142,7 +147,7 @@ const getNoResultsMessage = (filter?: DeviceSecurityVariation): string => { return _t('No sessions found.'); } }; -interface NoResultsProps { filter?: DeviceSecurityVariation, clearFilter: () => void} +interface NoResultsProps { filter?: FilterVariation, clearFilter: () => void} const NoResults: React.FC = ({ filter, clearFilter }) =>
{ getNoResultsMessage(filter) } @@ -279,7 +284,7 @@ export const FilteredDeviceList = ]; const onFilterOptionChange = (filterId: DeviceFilterKey) => { - onFilterChange(filterId === ALL_FILTER_ID ? undefined : filterId as DeviceSecurityVariation); + onFilterChange(filterId === ALL_FILTER_ID ? undefined : filterId as FilterVariation); }; const isAllSelected = selectedDeviceIds.length >= sortedDevices.length; diff --git a/src/components/views/settings/devices/filter.ts b/src/components/views/settings/devices/filter.ts index 968da4bdf44..542d079be07 100644 --- a/src/components/views/settings/devices/filter.ts +++ b/src/components/views/settings/devices/filter.ts @@ -37,7 +37,7 @@ const filters: Record = { export const filterDevicesBySecurityRecommendation = ( devices: ExtendedDevice[], - securityVariations: DeviceSecurityVariation[], + securityVariations: FilterVariation[], ) => { const activeFilters = securityVariations.map(variation => filters[variation]); if (!activeFilters.length) { diff --git a/src/components/views/settings/tabs/user/SessionManagerTab.tsx b/src/components/views/settings/tabs/user/SessionManagerTab.tsx index a2668201898..2d07d855a61 100644 --- a/src/components/views/settings/tabs/user/SessionManagerTab.tsx +++ b/src/components/views/settings/tabs/user/SessionManagerTab.tsx @@ -29,7 +29,7 @@ import { useOwnDevices } from '../../devices/useOwnDevices'; import { FilteredDeviceList } from '../../devices/FilteredDeviceList'; import CurrentDeviceSection from '../../devices/CurrentDeviceSection'; import SecurityRecommendations from '../../devices/SecurityRecommendations'; -import { DeviceSecurityVariation, ExtendedDevice } from '../../devices/types'; +import { ExtendedDevice } from '../../devices/types'; import { deleteDevicesWithInteractiveAuth } from '../../devices/deleteDevices'; import SettingsTab from '../SettingsTab'; import LoginWithQRSection from '../../devices/LoginWithQRSection'; @@ -37,6 +37,7 @@ import LoginWithQR, { Mode } from '../../../auth/LoginWithQR'; import SettingsStore from '../../../../../settings/SettingsStore'; import { useAsyncMemo } from '../../../../../hooks/useAsyncMemo'; import QuestionDialog from '../../../dialogs/QuestionDialog'; +import { FilterVariation } from '../../devices/filter'; const confirmSignOut = async (sessionsToSignOutCount: number): Promise => { const { finished } = Modal.createDialog(QuestionDialog, { @@ -123,7 +124,7 @@ const SessionManagerTab: React.FC = () => { setPushNotifications, supportsMSC3881, } = useOwnDevices(); - const [filter, setFilter] = useState(); + const [filter, setFilter] = useState(); const [expandedDeviceIds, setExpandedDeviceIds] = useState([]); const [selectedDeviceIds, setSelectedDeviceIds] = useState([]); const filteredDeviceListRef = useRef(null); @@ -142,7 +143,7 @@ const SessionManagerTab: React.FC = () => { } }; - const onGoToFilteredList = (filter: DeviceSecurityVariation) => { + const onGoToFilteredList = (filter: FilterVariation) => { setFilter(filter); clearTimeout(scrollIntoViewTimeoutRef.current); // wait a tick for the filtered section to rerender with different height From ba6d0a063899cd9313d82e27bf518368e3e9b53c Mon Sep 17 00:00:00 2001 From: Kerry Archibald Date: Thu, 8 Dec 2022 14:56:01 +1300 Subject: [PATCH 4/5] strict fixes --- src/components/views/settings/devices/FilteredDeviceList.tsx | 2 +- .../views/settings/devices/SecurityRecommendations.tsx | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/components/views/settings/devices/FilteredDeviceList.tsx b/src/components/views/settings/devices/FilteredDeviceList.tsx index b702a456f2d..880c023170d 100644 --- a/src/components/views/settings/devices/FilteredDeviceList.tsx +++ b/src/components/views/settings/devices/FilteredDeviceList.tsx @@ -108,7 +108,7 @@ const securityCardContent: Record - ([ + !!filter && ([ DeviceSecurityVariation.Inactive, DeviceSecurityVariation.Unverified, DeviceSecurityVariation.Verified, diff --git a/src/components/views/settings/devices/SecurityRecommendations.tsx b/src/components/views/settings/devices/SecurityRecommendations.tsx index 7b6381306b8..33cd566ee75 100644 --- a/src/components/views/settings/devices/SecurityRecommendations.tsx +++ b/src/components/views/settings/devices/SecurityRecommendations.tsx @@ -21,7 +21,7 @@ import AccessibleButton from '../../elements/AccessibleButton'; import SettingsSubsection from '../shared/SettingsSubsection'; import DeviceSecurityCard from './DeviceSecurityCard'; import { DeviceSecurityLearnMore } from './DeviceSecurityLearnMore'; -import { filterDevicesBySecurityRecommendation, INACTIVE_DEVICE_AGE_DAYS } from './filter'; +import { filterDevicesBySecurityRecommendation, FilterVariation, INACTIVE_DEVICE_AGE_DAYS } from './filter'; import { DeviceSecurityVariation, ExtendedDevice, @@ -31,7 +31,7 @@ import { interface Props { devices: DevicesDictionary; currentDeviceId: ExtendedDevice['device_id']; - goToFilteredList: (filter: DeviceSecurityVariation) => void; + goToFilteredList: (filter: FilterVariation) => void; } const SecurityRecommendations: React.FC = ({ From 10d273c0797851ca3c05c07e0332425d3699bd52 Mon Sep 17 00:00:00 2001 From: Kerry Archibald Date: Thu, 8 Dec 2022 16:17:26 +1300 Subject: [PATCH 5/5] avoid setting up cross signing in device man tests --- cypress/e2e/settings/device-management.spec.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/cypress/e2e/settings/device-management.spec.ts b/cypress/e2e/settings/device-management.spec.ts index ba88db48612..352bbb80bbd 100644 --- a/cypress/e2e/settings/device-management.spec.ts +++ b/cypress/e2e/settings/device-management.spec.ts @@ -49,7 +49,6 @@ describe("Device manager", () => { cy.get('[data-testid="current-session-section"]').within(() => { cy.contains('Unverified session').should('exist'); - cy.get('.mx_DeviceSecurityCard_actions [role="button"]').should('exist'); }); // current session details opened