Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Device manager - prune client information events after remote sign out #9874

Merged
merged 7 commits into from
Jan 10, 2023
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 12 additions & 8 deletions src/components/views/settings/devices/useOwnDevices.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ import { CryptoEvent } from "matrix-js-sdk/src/crypto";

import MatrixClientContext from "../../../../contexts/MatrixClientContext";
import { _t } from "../../../../languageHandler";
import { getDeviceClientInformation } from "../../../../utils/device/clientInformation";
import { getDeviceClientInformation, pruneClientInformation } from "../../../../utils/device/clientInformation";
import { DevicesDictionary, ExtendedDevice, ExtendedDeviceAppInfo } from "./types";
import { useEventEmitter } from "../../../../hooks/useEventEmitter";
import { parseUserAgent } from "../../../../utils/device/parseUserAgent";
Expand Down Expand Up @@ -116,8 +116,8 @@ export type DevicesState = {
export const useOwnDevices = (): DevicesState => {
const matrixClient = useContext(MatrixClientContext);

const currentDeviceId = matrixClient.getDeviceId();
const userId = matrixClient.getUserId();
const currentDeviceId = matrixClient.getDeviceId()!;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We maybe need getSafeDeviceId() 🙃

const userId = matrixClient.getSafeUserId();

const [devices, setDevices] = useState<DevicesState["devices"]>({});
const [pushers, setPushers] = useState<DevicesState["pushers"]>([]);
Expand All @@ -138,11 +138,6 @@ export const useOwnDevices = (): DevicesState => {
const refreshDevices = useCallback(async () => {
setIsLoadingDeviceList(true);
try {
// realistically we should never hit this
// but it satisfies types
if (!userId) {
throw new Error("Cannot fetch devices without user id");
}
const devices = await fetchDevicesWithVerification(matrixClient, userId);
setDevices(devices);

Expand Down Expand Up @@ -176,6 +171,15 @@ export const useOwnDevices = (): DevicesState => {
refreshDevices();
}, [refreshDevices]);

useEffect(() => {
const deviceIds = Object.keys(devices);
// empty devices means devices have not been fetched yet
// as there is always at least the current device
if (deviceIds.length) {
pruneClientInformation(Object.keys(devices), matrixClient);
kerryarchibald marked this conversation as resolved.
Show resolved Hide resolved
}
}, [devices, matrixClient]);

useEventEmitter(matrixClient, CryptoEvent.DevicesUpdated, (users: string[]): void => {
if (users.includes(userId)) {
refreshDevices();
Expand Down
31 changes: 24 additions & 7 deletions src/utils/device/clientInformation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,8 @@ const formatUrl = (): string | undefined => {
].join("");
};

export const getClientInformationEventType = (deviceId: string): string =>
`io.element.matrix_client_information.${deviceId}`;
const clientInformationEventPrefix = "io.element.matrix_client_information.";
export const getClientInformationEventType = (deviceId: string): string => `${clientInformationEventPrefix}${deviceId}`;

/**
* Record extra client information for the current device
Expand All @@ -52,7 +52,7 @@ export const recordClientInformation = async (
sdkConfig: IConfigOptions,
platform: BasePlatform,
): Promise<void> => {
const deviceId = matrixClient.getDeviceId();
const deviceId = matrixClient.getDeviceId()!;
const { brand } = sdkConfig;
const version = await platform.getAppVersion();
const type = getClientInformationEventType(deviceId);
Expand All @@ -66,12 +66,29 @@ export const recordClientInformation = async (
};

/**
* Remove extra client information
* @todo(kerrya) revisit after MSC3391: account data deletion is done
* (PSBE-12)
* Remove client information events for devices that no longer exist
* @param validDeviceIds - ids of current devices,
* client information for devices NOT in this list will be removed
*/
export const pruneClientInformation = async (validDeviceIds: string[], matrixClient: MatrixClient): Promise<void> => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can find neither an await nor a Promise return here. Should it really be async?

const orphanClientInformationEvents = Object.values(matrixClient.store.accountData).filter((event) => {
if (event.getType().startsWith(clientInformationEventPrefix)) {
const [, deviceId] = event.getType().split(clientInformationEventPrefix);
return deviceId && !validDeviceIds.includes(deviceId);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about calling deleteAccountData here directly?

}
return false;
});

orphanClientInformationEvents.forEach((event) => {
matrixClient.deleteAccountData(event.getType());
});
};

/**
* Remove extra client information for current device
*/
export const removeClientInformation = async (matrixClient: MatrixClient): Promise<void> => {
const deviceId = matrixClient.getDeviceId();
const deviceId = matrixClient.getDeviceId()!;
const type = getClientInformationEventType(deviceId);
const clientInformation = getDeviceClientInformation(matrixClient, deviceId);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ import LogoutDialog from "../../../../../../src/components/views/dialogs/LogoutD
import { DeviceSecurityVariation, ExtendedDevice } from "../../../../../../src/components/views/settings/devices/types";
import { INACTIVE_DEVICE_AGE_MS } from "../../../../../../src/components/views/settings/devices/filter";
import SettingsStore from "../../../../../../src/settings/SettingsStore";
import { getClientInformationEventType } from "../../../../../../src/utils/device/clientInformation";

mockPlatformPeg();

Expand Down Expand Up @@ -87,6 +88,7 @@ describe("<SessionManagerTab />", () => {
generateClientSecret: jest.fn(),
setDeviceDetails: jest.fn(),
getAccountData: jest.fn(),
deleteAccountData: jest.fn(),
doesServerSupportUnstableFeature: jest.fn().mockResolvedValue(true),
getPushers: jest.fn(),
setPusher: jest.fn(),
Expand Down Expand Up @@ -182,6 +184,9 @@ describe("<SessionManagerTab />", () => {
],
});

// @ts-ignore mock
mockClient.store = { accountData: {} };

mockClient.getAccountData.mockReset().mockImplementation((eventType) => {
if (eventType.startsWith(LOCAL_NOTIFICATION_SETTINGS_PREFIX.name)) {
return new MatrixEvent({
Expand Down Expand Up @@ -667,6 +672,47 @@ describe("<SessionManagerTab />", () => {
);
});

it("removes account data events for devices after sign out", async () => {
const mobileDeviceClientInfo = new MatrixEvent({
type: getClientInformationEventType(alicesMobileDevice.device_id),
content: {
name: "test",
},
});
// @ts-ignore setup mock
mockClient.store = {
// @ts-ignore setup mock
accountData: {
[mobileDeviceClientInfo.getType()]: mobileDeviceClientInfo,
},
};

mockClient.getDevices
.mockResolvedValueOnce({
devices: [alicesDevice, alicesMobileDevice, alicesOlderMobileDevice],
})
.mockResolvedValueOnce({
// refreshed devices after sign out
devices: [alicesDevice],
});

const { getByTestId, getByLabelText } = render(getComponent());

await act(async () => {
await flushPromises();
});

expect(mockClient.deleteAccountData).not.toHaveBeenCalled();

fireEvent.click(getByTestId("current-session-menu"));
fireEvent.click(getByLabelText("Sign out of all other sessions (2)"));
await confirmSignout(getByTestId);

// only called once for signed out device with account data event
expect(mockClient.deleteAccountData).toHaveBeenCalledTimes(1);
expect(mockClient.deleteAccountData).toHaveBeenCalledWith(mobileDeviceClientInfo.getType());
});

describe("other devices", () => {
const interactiveAuthError = { httpStatus: 401, data: { flows: [{ stages: ["m.login.password"] }] } };

Expand Down