From dfded8d4d30a8b2fdaae22eed22687f8af8661c3 Mon Sep 17 00:00:00 2001 From: Kerry Date: Tue, 22 Aug 2023 14:25:34 +1200 Subject: [PATCH] OIDC: disable multi session signout for OIDC-aware servers in session manager (#11431) * util for account url * test cases * disable multi session selection on device list * remove sign out all from context menus when oidc-aware * comment * remove unused param * typo --- .../settings/devices/FilteredDeviceList.tsx | 74 ++++++--- .../devices/FilteredDeviceListHeader.tsx | 24 +-- .../devices/OtherSessionsSectionHeading.tsx | 36 ++-- .../tabs/user/GeneralUserSettingsTab.tsx | 6 +- .../settings/tabs/user/SessionManagerTab.tsx | 23 ++- src/utils/oidc/getDelegatedAuthAccountUrl.ts | 27 +++ .../tabs/user/SessionManagerTab-test.tsx | 157 +++++++++++++++++- .../SessionManagerTab-test.tsx.snap | 15 ++ .../oidc/getDelegatedAuthAccountUrl-test.ts | 61 +++++++ 9 files changed, 362 insertions(+), 61 deletions(-) create mode 100644 src/utils/oidc/getDelegatedAuthAccountUrl.ts create mode 100644 test/utils/oidc/getDelegatedAuthAccountUrl-test.ts diff --git a/src/components/views/settings/devices/FilteredDeviceList.tsx b/src/components/views/settings/devices/FilteredDeviceList.tsx index b6086979bc..76d157d233 100644 --- a/src/components/views/settings/devices/FilteredDeviceList.tsx +++ b/src/components/views/settings/devices/FilteredDeviceList.tsx @@ -31,6 +31,7 @@ import { DevicesState } from "./useOwnDevices"; import FilteredDeviceListHeader from "./FilteredDeviceListHeader"; import Spinner from "../../elements/Spinner"; import { DeviceSecurityLearnMore } from "./DeviceSecurityLearnMore"; +import DeviceTile from "./DeviceTile"; interface Props { devices: DevicesDictionary; @@ -48,6 +49,11 @@ interface Props { setPushNotifications: (deviceId: string, enabled: boolean) => Promise; setSelectedDeviceIds: (deviceIds: ExtendedDevice["device_id"][]) => void; supportsMSC3881?: boolean | undefined; + /** + * Only allow sessions to be signed out individually + * Removes checkboxes and multi selection header + */ + disableMultipleSignout?: boolean; } const isDeviceSelected = ( @@ -178,6 +184,7 @@ const DeviceListItem: React.FC<{ toggleSelected: () => void; setPushNotifications: (deviceId: string, enabled: boolean) => Promise; supportsMSC3881?: boolean | undefined; + isSelectDisabled?: boolean; }> = ({ device, pusher, @@ -192,33 +199,47 @@ const DeviceListItem: React.FC<{ setPushNotifications, toggleSelected, supportsMSC3881, -}) => ( -
  • - + isSelectDisabled, +}) => { + const tileContent = ( + <> {isSigningOut && } - - {isExpanded && ( - - )} -
    • -); + + ); + return ( +
  • + {isSelectDisabled ? ( + + {tileContent} + + ) : ( + + {tileContent} + + )} + {isExpanded && ( + + )} +
    • + ); +}; /** * Filtered list of devices @@ -242,6 +263,7 @@ export const FilteredDeviceList = forwardRef( setPushNotifications, setSelectedDeviceIds, supportsMSC3881, + disableMultipleSignout, }: Props, ref: ForwardedRef, ) => { @@ -302,6 +324,7 @@ export const FilteredDeviceList = forwardRef( selectedDeviceCount={selectedDeviceIds.length} isAllSelected={isAllSelected} toggleSelectAll={toggleSelectAll} + isSelectDisabled={disableMultipleSignout} > {selectedDeviceIds.length ? ( <> @@ -351,6 +374,7 @@ export const FilteredDeviceList = forwardRef( isExpanded={expandedDeviceIds.includes(device.device_id)} isSigningOut={signingOutDeviceIds.includes(device.device_id)} isSelected={isDeviceSelected(device.device_id, selectedDeviceIds)} + isSelectDisabled={disableMultipleSignout} onDeviceExpandToggle={() => onDeviceExpandToggle(device.device_id)} onSignOutDevice={() => onSignOutDevices([device.device_id])} saveDeviceName={(deviceName: string) => saveDeviceName(device.device_id, deviceName)} diff --git a/src/components/views/settings/devices/FilteredDeviceListHeader.tsx b/src/components/views/settings/devices/FilteredDeviceListHeader.tsx index ffbe8a2b6c..c3e917cdd1 100644 --- a/src/components/views/settings/devices/FilteredDeviceListHeader.tsx +++ b/src/components/views/settings/devices/FilteredDeviceListHeader.tsx @@ -24,6 +24,7 @@ import TooltipTarget from "../../elements/TooltipTarget"; interface Props extends Omit, "className"> { selectedDeviceCount: number; isAllSelected: boolean; + isSelectDisabled?: boolean; toggleSelectAll: () => void; children?: React.ReactNode; } @@ -31,6 +32,7 @@ interface Props extends Omit, "className"> { const FilteredDeviceListHeader: React.FC = ({ selectedDeviceCount, isAllSelected, + isSelectDisabled, toggleSelectAll, children, ...rest @@ -38,16 +40,18 @@ const FilteredDeviceListHeader: React.FC = ({ const checkboxLabel = isAllSelected ? _t("Deselect all") : _t("Select all"); return (
    - - - + {!isSelectDisabled && ( + + + + )} {selectedDeviceCount > 0 ? _t("%(count)s sessions selected", { count: selectedDeviceCount }) diff --git a/src/components/views/settings/devices/OtherSessionsSectionHeading.tsx b/src/components/views/settings/devices/OtherSessionsSectionHeading.tsx index f2ab2ac320..c8a066ce5a 100644 --- a/src/components/views/settings/devices/OtherSessionsSectionHeading.tsx +++ b/src/components/views/settings/devices/OtherSessionsSectionHeading.tsx @@ -20,6 +20,7 @@ import { _t } from "../../../../languageHandler"; import { KebabContextMenu } from "../../context_menus/KebabContextMenu"; import { SettingsSubsectionHeading } from "../shared/SettingsSubsectionHeading"; import { IconizedContextMenuOption } from "../../context_menus/IconizedContextMenu"; +import { filterBoolean } from "../../../../utils/arrays"; interface Props { // total count of other sessions @@ -27,7 +28,8 @@ interface Props { // not affected by filters otherSessionsCount: number; disabled?: boolean; - signOutAllOtherSessions: () => void; + // not provided when sign out all other sessions is not available + signOutAllOtherSessions?: () => void; } export const OtherSessionsSectionHeading: React.FC = ({ @@ -35,22 +37,26 @@ export const OtherSessionsSectionHeading: React.FC = ({ disabled, signOutAllOtherSessions, }) => { - const menuOptions = [ - , - ]; + const menuOptions = filterBoolean([ + signOutAllOtherSessions ? ( + + ) : null, + ]); return ( - + {!!menuOptions.length && ( + + )} ); }; diff --git a/src/components/views/settings/tabs/user/GeneralUserSettingsTab.tsx b/src/components/views/settings/tabs/user/GeneralUserSettingsTab.tsx index ff0e237dd3..678dd702c3 100644 --- a/src/components/views/settings/tabs/user/GeneralUserSettingsTab.tsx +++ b/src/components/views/settings/tabs/user/GeneralUserSettingsTab.tsx @@ -17,7 +17,7 @@ limitations under the License. */ import React, { ReactNode } from "react"; -import { SERVICE_TYPES, IDelegatedAuthConfig, M_AUTHENTICATION, HTTPError } from "matrix-js-sdk/src/matrix"; +import { SERVICE_TYPES, HTTPError } from "matrix-js-sdk/src/matrix"; import { IThreepid, ThreepidMedium } from "matrix-js-sdk/src/@types/threepids"; import { logger } from "matrix-js-sdk/src/logger"; @@ -59,6 +59,7 @@ import Heading from "../../../typography/Heading"; import InlineSpinner from "../../../elements/InlineSpinner"; import MatrixClientContext from "../../../../../contexts/MatrixClientContext"; import { ThirdPartyIdentifier } from "../../../../../AddThreepid"; +import { getDelegatedAuthAccountUrl } from "../../../../../utils/oidc/getDelegatedAuthAccountUrl"; interface IProps { closeSettingsFn: () => void; @@ -172,8 +173,7 @@ export default class GeneralUserSettingsTab extends React.Component(cli.getClientWellKnown()); - const externalAccountManagementUrl = delegatedAuthConfig?.account; + const externalAccountManagementUrl = getDelegatedAuthAccountUrl(cli.getClientWellKnown()); this.setState({ canChangePassword, externalAccountManagementUrl }); } diff --git a/src/components/views/settings/tabs/user/SessionManagerTab.tsx b/src/components/views/settings/tabs/user/SessionManagerTab.tsx index 9be48848cc..8041f6b8a5 100644 --- a/src/components/views/settings/tabs/user/SessionManagerTab.tsx +++ b/src/components/views/settings/tabs/user/SessionManagerTab.tsx @@ -39,6 +39,7 @@ import QuestionDialog from "../../../dialogs/QuestionDialog"; import { FilterVariation } from "../../devices/filter"; import { OtherSessionsSectionHeading } from "../../devices/OtherSessionsSectionHeading"; import { SettingsSection } from "../../shared/SettingsSection"; +import { getDelegatedAuthAccountUrl } from "../../../../../utils/oidc/getDelegatedAuthAccountUrl"; const confirmSignOut = async (sessionsToSignOutCount: number): Promise => { const { finished } = Modal.createDialog(QuestionDialog, { @@ -130,6 +131,14 @@ const SessionManagerTab: React.FC = () => { const scrollIntoViewTimeoutRef = useRef(); const matrixClient = useContext(MatrixClientContext); + /** + * If we have a delegated auth account management URL, all sessions but the current session need to be managed in the + * delegated auth provider. + * See https://github.com/matrix-org/matrix-spec-proposals/pull/3824 + */ + const delegatedAuthAccountUrl = getDelegatedAuthAccountUrl(matrixClient.getClientWellKnown()); + const disableMultipleSignout = !!delegatedAuthAccountUrl; + const userId = matrixClient?.getUserId(); const currentUserMember = (userId && matrixClient?.getUser(userId)) || undefined; const clientVersions = useAsyncMemo(() => matrixClient.getVersions(), [matrixClient]); @@ -205,11 +214,12 @@ const SessionManagerTab: React.FC = () => { setSelectedDeviceIds([]); }, [filter, setSelectedDeviceIds]); - const signOutAllOtherSessions = shouldShowOtherSessions - ? () => { - onSignOutOtherDevices(Object.keys(otherDevices)); - } - : undefined; + const signOutAllOtherSessions = + shouldShowOtherSessions && !disableMultipleSignout + ? () => { + onSignOutOtherDevices(Object.keys(otherDevices)); + } + : undefined; const [signInWithQrMode, setSignInWithQrMode] = useState(); @@ -250,7 +260,7 @@ const SessionManagerTab: React.FC = () => { heading={ } @@ -280,6 +290,7 @@ const SessionManagerTab: React.FC = () => { setPushNotifications={setPushNotifications} ref={filteredDeviceListRef} supportsMSC3881={supportsMSC3881} + disableMultipleSignout={disableMultipleSignout} /> )} diff --git a/src/utils/oidc/getDelegatedAuthAccountUrl.ts b/src/utils/oidc/getDelegatedAuthAccountUrl.ts new file mode 100644 index 0000000000..cfb61cb443 --- /dev/null +++ b/src/utils/oidc/getDelegatedAuthAccountUrl.ts @@ -0,0 +1,27 @@ +/* +Copyright 2023 The Matrix.org Foundation C.I.C. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +import { IClientWellKnown, IDelegatedAuthConfig, M_AUTHENTICATION } from "matrix-js-sdk/src/matrix"; + +/** + * Get the delegated auth account management url if configured + * @param clientWellKnown from MatrixClient.getClientWellKnown + * @returns the account management url, or undefined + */ +export const getDelegatedAuthAccountUrl = (clientWellKnown: IClientWellKnown | undefined): string | undefined => { + const delegatedAuthConfig = M_AUTHENTICATION.findIn(clientWellKnown); + return delegatedAuthConfig?.account; +}; diff --git a/test/components/views/settings/tabs/user/SessionManagerTab-test.tsx b/test/components/views/settings/tabs/user/SessionManagerTab-test.tsx index bb34acb9d5..45fc665c6b 100644 --- a/test/components/views/settings/tabs/user/SessionManagerTab-test.tsx +++ b/test/components/views/settings/tabs/user/SessionManagerTab-test.tsx @@ -15,7 +15,7 @@ limitations under the License. */ import React from "react"; -import { act, fireEvent, render, RenderResult } from "@testing-library/react"; +import { act, fireEvent, render, RenderResult, screen } from "@testing-library/react"; import { DeviceInfo } from "matrix-js-sdk/src/crypto/deviceinfo"; import { logger } from "matrix-js-sdk/src/logger"; import { VerificationRequest } from "matrix-js-sdk/src/crypto-api"; @@ -32,6 +32,7 @@ import { CryptoApi, DeviceVerificationStatus, MatrixError, + M_AUTHENTICATION, } from "matrix-js-sdk/src/matrix"; import { mocked } from "jest-mock"; @@ -87,7 +88,7 @@ describe("", () => { requestDeviceVerification: jest.fn().mockResolvedValue(mockVerificationRequest), } as unknown as CryptoApi); - const mockClient = getMockClientWithEventEmitter({ + let mockClient = getMockClientWithEventEmitter({ ...mockClientMethodsUser(aliceId), getCrypto: jest.fn().mockReturnValue(mockCrypto), getDevices: jest.fn(), @@ -104,6 +105,7 @@ describe("", () => { setLocalNotificationSettings: jest.fn(), getVersions: jest.fn().mockResolvedValue({}), getCapabilities: jest.fn().mockResolvedValue({}), + getClientWellKnown: jest.fn().mockReturnValue({}), }); const defaultProps = {}; @@ -173,6 +175,25 @@ describe("", () => { }; beforeEach(async () => { + mockClient = getMockClientWithEventEmitter({ + ...mockClientMethodsUser(aliceId), + getCrypto: jest.fn().mockReturnValue(mockCrypto), + getDevices: jest.fn(), + getStoredDevice: jest.fn(), + getDeviceId: jest.fn().mockReturnValue(deviceId), + deleteMultipleDevices: jest.fn(), + generateClientSecret: jest.fn(), + setDeviceDetails: jest.fn(), + getAccountData: jest.fn(), + deleteAccountData: jest.fn(), + doesServerSupportUnstableFeature: jest.fn().mockResolvedValue(true), + getPushers: jest.fn(), + setPusher: jest.fn(), + setLocalNotificationSettings: jest.fn(), + getVersions: jest.fn().mockResolvedValue({}), + getCapabilities: jest.fn().mockResolvedValue({}), + getClientWellKnown: jest.fn().mockReturnValue({}), + }); jest.clearAllMocks(); jest.spyOn(logger, "error").mockRestore(); mockClient.getStoredDevice.mockImplementation((_userId, id) => { @@ -1012,6 +1033,138 @@ describe("", () => { ); }); }); + + describe("for an OIDC-aware server", () => { + beforeEach(() => { + mockClient.getClientWellKnown.mockReturnValue({ + [M_AUTHENTICATION.name]: { + issuer: "https://issuer.org", + account: "https://issuer.org/account", + }, + }); + }); + + // signing out the current device works as usual + it("Signs out of current device", async () => { + const modalSpy = jest.spyOn(Modal, "createDialog"); + + mockClient.getDevices.mockResolvedValue({ + devices: [alicesDevice], + }); + const { getByTestId } = render(getComponent()); + + await act(async () => { + await flushPromises(); + }); + + toggleDeviceDetails(getByTestId, alicesDevice.device_id); + + const signOutButton = getByTestId("device-detail-sign-out-cta"); + expect(signOutButton).toMatchSnapshot(); + fireEvent.click(signOutButton); + + // logout dialog opened + expect(modalSpy).toHaveBeenCalledWith(LogoutDialog, {}, undefined, false, true); + }); + + it("does not allow signing out of all other devices from current session context menu", async () => { + mockClient.getDevices.mockResolvedValue({ + devices: [alicesDevice, alicesMobileDevice, alicesOlderMobileDevice], + }); + const { getByTestId } = render(getComponent()); + + await act(async () => { + await flushPromises(); + }); + + fireEvent.click(getByTestId("current-session-menu")); + expect(screen.queryByLabelText("Sign out of all other sessions (2)")).not.toBeInTheDocument(); + }); + + describe("other devices", () => { + // signing out a single device still works + // this test will be updated once redirect to MAS is added + // https://github.com/vector-im/element-web/issues/26000 + it("deletes a device when interactive auth is not required", async () => { + mockClient.deleteMultipleDevices.mockResolvedValue({}); + mockClient.getDevices + .mockResolvedValueOnce({ + devices: [alicesDevice, alicesMobileDevice, alicesOlderMobileDevice], + }) + // pretend it was really deleted on refresh + .mockResolvedValueOnce({ + devices: [alicesDevice, alicesOlderMobileDevice], + }); + + const { getByTestId } = render(getComponent()); + + await act(async () => { + await flushPromises(); + }); + + toggleDeviceDetails(getByTestId, alicesMobileDevice.device_id); + + const deviceDetails = getByTestId(`device-detail-${alicesMobileDevice.device_id}`); + const signOutButton = deviceDetails.querySelector( + '[data-testid="device-detail-sign-out-cta"]', + ) as Element; + fireEvent.click(signOutButton); + + await confirmSignout(getByTestId); + + // sign out button is disabled with spinner + expect( + ( + deviceDetails.querySelector('[data-testid="device-detail-sign-out-cta"]') as Element + ).getAttribute("aria-disabled"), + ).toEqual("true"); + // delete called + expect(mockClient.deleteMultipleDevices).toHaveBeenCalledWith( + [alicesMobileDevice.device_id], + undefined, + ); + + await flushPromises(); + + // devices refreshed + expect(mockClient.getDevices).toHaveBeenCalled(); + }); + + it("does not allow removing multiple devices at once", async () => { + mockClient.getDevices.mockResolvedValue({ + devices: [alicesDevice, alicesMobileDevice, alicesOlderMobileDevice, alicesInactiveDevice], + }); + + render(getComponent()); + + await act(async () => { + await flushPromises(); + }); + + // sessions don't have checkboxes + expect( + screen.queryByTestId(`device-tile-checkbox-${alicesMobileDevice.device_id}`), + ).not.toBeInTheDocument(); + // no select all + expect(screen.queryByLabelText("Select all")).not.toBeInTheDocument(); + }); + + it("does not allow signing out of all other devices from other sessions context menu", async () => { + mockClient.getDevices.mockResolvedValue({ + devices: [alicesDevice, alicesMobileDevice, alicesOlderMobileDevice], + }); + render(getComponent()); + + await act(async () => { + await flushPromises(); + }); + + // no context menu because 'sign out all' is the only option + // and it is not allowed when server is oidc-aware + expect(screen.queryByTestId("other-sessions-menu")).not.toBeInTheDocument(); + }); + }); + }); }); describe("Rename sessions", () => { 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 ae427da9ec..df9f7a4147 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 @@ -53,6 +53,21 @@ exports[` Sign out Signs out of current device 1`] = `
    `; +exports[` Sign out for an OIDC-aware server Signs out of current device 1`] = ` +
    + + Sign out of this session + +
    +`; + exports[` current session section renders current session section with a verified session 1`] = `
    { + it("should return undefined when wk is undefined", () => { + expect(getDelegatedAuthAccountUrl(undefined)).toBeUndefined(); + }); + + it("should return undefined when wk has no authentication config", () => { + expect(getDelegatedAuthAccountUrl({})).toBeUndefined(); + }); + + it("should return undefined when wk authentication config has no configured account url", () => { + expect( + getDelegatedAuthAccountUrl({ + [M_AUTHENTICATION.stable!]: { + issuer: "issuer.org", + }, + }), + ).toBeUndefined(); + }); + + it("should return the account url for authentication config using the unstable prefix", () => { + expect( + getDelegatedAuthAccountUrl({ + [M_AUTHENTICATION.unstable!]: { + issuer: "issuer.org", + account: "issuer.org/account", + }, + }), + ).toEqual("issuer.org/account"); + }); + + it("should return the account url for authentication config using the stable prefix", () => { + expect( + getDelegatedAuthAccountUrl({ + [M_AUTHENTICATION.stable!]: { + issuer: "issuer.org", + account: "issuer.org/account", + }, + }), + ).toEqual("issuer.org/account"); + }); +});