From 16d2cccb73b121c650f7527a7afe6caeeb1829cd Mon Sep 17 00:00:00 2001 From: Michael Telatynski <7t3chguy@gmail.com> Date: Mon, 23 Dec 2024 22:35:43 +0000 Subject: [PATCH] OIDC settings tweaks (#28787) * Hide 3pid account settings if account is managed externally As they would be disabled and just confusing otherwise Signed-off-by: Michael Telatynski <7t3chguy@gmail.com> * Show manage device button instead of sign out button for other devices in OIDC mode Signed-off-by: Michael Telatynski <7t3chguy@gmail.com> * Tidy up Signed-off-by: Michael Telatynski <7t3chguy@gmail.com> * Fix tests Signed-off-by: Michael Telatynski <7t3chguy@gmail.com> --------- Signed-off-by: Michael Telatynski <7t3chguy@gmail.com> --- scripts/analyse_unused_exports.ts | 1 + .../views/dialogs/oidc/OidcLogoutDialog.tsx | 66 ---------- .../settings/devices/CurrentDeviceSection.tsx | 4 + .../views/settings/devices/DeviceDetails.tsx | 122 +++++++++++------- .../settings/devices/FilteredDeviceList.tsx | 16 ++- .../tabs/user/AccountUserSettingsTab.tsx | 4 +- .../settings/tabs/user/SessionManagerTab.tsx | 38 ++---- src/i18n/strings/en_EN.json | 2 +- src/utils/oidc/getOidcLogoutUrl.ts | 20 --- src/utils/oidc/urls.ts | 32 +++++ .../CurrentDeviceSection-test.tsx.snap | 2 +- .../tabs/user/AccountUserSettingsTab-test.tsx | 7 +- .../tabs/user/SessionManagerTab-test.tsx | 27 +--- .../AccountUserSettingsTab-test.tsx.snap | 2 +- 14 files changed, 143 insertions(+), 200 deletions(-) delete mode 100644 src/components/views/dialogs/oidc/OidcLogoutDialog.tsx delete mode 100644 src/utils/oidc/getOidcLogoutUrl.ts create mode 100644 src/utils/oidc/urls.ts diff --git a/scripts/analyse_unused_exports.ts b/scripts/analyse_unused_exports.ts index b20e6b285a..fd0e762d94 100755 --- a/scripts/analyse_unused_exports.ts +++ b/scripts/analyse_unused_exports.ts @@ -19,6 +19,7 @@ ignore.push("/OpenSpotlightPayload.ts"); ignore.push("/PinnedMessageBadge.tsx"); ignore.push("/editor/mock.ts"); ignore.push("DeviceIsolationModeController.ts"); +ignore.push("urls.ts"); ignore.push("/json.ts"); ignore.push("/ReleaseAnnouncementStore.ts"); ignore.push("/WidgetLayoutStore.ts"); diff --git a/src/components/views/dialogs/oidc/OidcLogoutDialog.tsx b/src/components/views/dialogs/oidc/OidcLogoutDialog.tsx deleted file mode 100644 index 83bcd8736f..0000000000 --- a/src/components/views/dialogs/oidc/OidcLogoutDialog.tsx +++ /dev/null @@ -1,66 +0,0 @@ -/* -Copyright 2024 New Vector Ltd. -Copyright 2023 The Matrix.org Foundation C.I.C. - -SPDX-License-Identifier: AGPL-3.0-only OR GPL-3.0-only -Please see LICENSE files in the repository root for full details. -*/ - -import React, { useState } from "react"; - -import { _t } from "../../../../languageHandler"; -import BaseDialog from "../BaseDialog"; -import { getOidcLogoutUrl } from "../../../../utils/oidc/getOidcLogoutUrl"; -import AccessibleButton from "../../elements/AccessibleButton"; - -export interface OidcLogoutDialogProps { - delegatedAuthAccountUrl: string; - deviceId: string; - onFinished(ok?: boolean): void; -} - -/** - * Handle logout of OIDC sessions other than the current session - * - ask for user confirmation to open the delegated auth provider - * - open the auth provider in a new tab - * - wait for the user to return and close the modal, we assume the user has completed sign out of the session in auth provider UI - * and trigger a refresh of the session list - */ -export const OidcLogoutDialog: React.FC = ({ - delegatedAuthAccountUrl, - deviceId, - onFinished, -}) => { - const [hasOpenedLogoutLink, setHasOpenedLogoutLink] = useState(false); - const logoutUrl = getOidcLogoutUrl(delegatedAuthAccountUrl, deviceId); - - return ( - -
- {_t("auth|oidc|logout_redirect_warning")} -
-
- {hasOpenedLogoutLink ? ( - onFinished(true)}> - {_t("action|close")} - - ) : ( - <> - onFinished(false)}> - {_t("action|cancel")} - - setHasOpenedLogoutLink(true)} - kind="primary" - href={logoutUrl} - target="_blank" - > - {_t("action|continue")} - - - )} -
- - ); -}; diff --git a/src/components/views/settings/devices/CurrentDeviceSection.tsx b/src/components/views/settings/devices/CurrentDeviceSection.tsx index 153a9c5d4b..9a3eb41015 100644 --- a/src/components/views/settings/devices/CurrentDeviceSection.tsx +++ b/src/components/views/settings/devices/CurrentDeviceSection.tsx @@ -34,6 +34,7 @@ interface Props { onSignOutCurrentDevice: () => void; signOutAllOtherSessions?: () => void; saveDeviceName: (deviceName: string) => Promise; + delegatedAuthAccountUrl?: string; } type CurrentDeviceSectionHeadingProps = Pick< @@ -90,6 +91,7 @@ const CurrentDeviceSection: React.FC = ({ onSignOutCurrentDevice, signOutAllOtherSessions, saveDeviceName, + delegatedAuthAccountUrl, }) => { const [isExpanded, setIsExpanded] = useState(false); @@ -126,6 +128,8 @@ const CurrentDeviceSection: React.FC = ({ onSignOutDevice={onSignOutCurrentDevice} saveDeviceName={saveDeviceName} className="mx_CurrentDeviceSection_deviceDetails" + delegatedAuthAccountUrl={delegatedAuthAccountUrl} + isCurrentDevice /> ) : ( <> diff --git a/src/components/views/settings/devices/DeviceDetails.tsx b/src/components/views/settings/devices/DeviceDetails.tsx index 1a418f5dd5..569a2a0199 100644 --- a/src/components/views/settings/devices/DeviceDetails.tsx +++ b/src/components/views/settings/devices/DeviceDetails.tsx @@ -18,6 +18,7 @@ import ToggleSwitch from "../../elements/ToggleSwitch"; import { DeviceDetailHeading } from "./DeviceDetailHeading"; import { DeviceVerificationStatusCard } from "./DeviceVerificationStatusCard"; import { ExtendedDevice } from "./types"; +import { getManageDeviceUrl } from "../../../../utils/oidc/urls.ts"; interface Props { device: ExtendedDevice; @@ -31,6 +32,7 @@ interface Props { supportsMSC3881?: boolean; className?: string; isCurrentDevice?: boolean; + delegatedAuthAccountUrl?: string; } interface MetadataTable { @@ -39,6 +41,22 @@ interface MetadataTable { values: { label: string; value?: string | React.ReactNode }[]; } +function isPushNotificationsEnabled(pusher?: IPusher, notificationSettings?: LocalNotificationSettings): boolean { + if (pusher) return !!pusher[PUSHER_ENABLED.name]; + if (notificationSettings) return !notificationSettings.is_silenced; + return true; +} + +function isCheckboxDisabled( + pusher?: IPusher, + notificationSettings?: LocalNotificationSettings, + supportsMSC3881?: boolean, +): boolean { + if (notificationSettings) return false; + if (pusher && !supportsMSC3881) return true; + return false; +} + const DeviceDetails: React.FC = ({ device, pusher, @@ -51,6 +69,7 @@ const DeviceDetails: React.FC = ({ supportsMSC3881, className, isCurrentDevice, + delegatedAuthAccountUrl, }) => { const metadata: MetadataTable[] = [ { @@ -95,18 +114,6 @@ const DeviceDetails: React.FC = ({ const showPushNotificationSection = !!pusher || !!localNotificationSettings; - function isPushNotificationsEnabled(pusher?: IPusher, notificationSettings?: LocalNotificationSettings): boolean { - if (pusher) return !!pusher[PUSHER_ENABLED.name]; - if (localNotificationSettings) return !localNotificationSettings.is_silenced; - return true; - } - - function isCheckboxDisabled(pusher?: IPusher, notificationSettings?: LocalNotificationSettings): boolean { - if (localNotificationSettings) return false; - if (pusher && !supportsMSC3881) return true; - return false; - } - return (
@@ -117,32 +124,34 @@ const DeviceDetails: React.FC = ({ isCurrentDevice={isCurrentDevice} />
-
-

{_t("settings|sessions|details_heading")}

- {metadata.map(({ heading, values, id }, index) => ( - - {heading && ( - - - - - - )} - - {values.map(({ label, value }) => ( - - - - - ))} - -
{heading}
{label}{value}
- ))} -
+ {!delegatedAuthAccountUrl && ( +
+

{_t("settings|sessions|details_heading")}

+ {metadata.map(({ heading, values, id }, index) => ( + + {heading && ( + + + + + + )} + + {values.map(({ label, value }) => ( + + + + + ))} + +
{heading}
{label}{value}
+ ))} +
+ )} {showPushNotificationSection && (
= ({ // For backwards compatibility, if `enabled` is missing // default to `true` checked={isPushNotificationsEnabled(pusher, localNotificationSettings)} - disabled={isCheckboxDisabled(pusher, localNotificationSettings)} + disabled={isCheckboxDisabled(pusher, localNotificationSettings, supportsMSC3881)} onChange={(checked) => setPushNotifications?.(device.device_id, checked)} title={_t("settings|sessions|push_toggle")} data-testid="device-detail-push-notification-checkbox" @@ -166,17 +175,30 @@ const DeviceDetails: React.FC = ({
)}
- - - {_t("settings|sessions|sign_out")} - {isSigningOut && } - - + {delegatedAuthAccountUrl && !isCurrentDevice ? ( + + {_t("settings|sessions|manage")} + + ) : ( + + + {_t("settings|sessions|sign_out")} + {isSigningOut && } + + + )}
); diff --git a/src/components/views/settings/devices/FilteredDeviceList.tsx b/src/components/views/settings/devices/FilteredDeviceList.tsx index e5f1a6a9a3..b1f2180a78 100644 --- a/src/components/views/settings/devices/FilteredDeviceList.tsx +++ b/src/components/views/settings/devices/FilteredDeviceList.tsx @@ -41,10 +41,12 @@ interface Props { setSelectedDeviceIds: (deviceIds: ExtendedDevice["device_id"][]) => void; supportsMSC3881?: boolean | undefined; /** - * Only allow sessions to be signed out individually + * If the user's account is managed externally then sessions must be signed out individually * Removes checkboxes and multi selection header + * Removes session info as that can be seen in the account management + * Changes sign out button to be a manage button */ - disableMultipleSignout?: boolean; + delegatedAuthAccountUrl?: string; } const isDeviceSelected = ( @@ -172,6 +174,7 @@ const DeviceListItem: React.FC<{ setPushNotifications: (deviceId: string, enabled: boolean) => Promise; supportsMSC3881?: boolean | undefined; isSelectDisabled?: boolean; + delegatedAuthAccountUrl?: string; }> = ({ device, pusher, @@ -187,6 +190,7 @@ const DeviceListItem: React.FC<{ toggleSelected, supportsMSC3881, isSelectDisabled, + delegatedAuthAccountUrl, }) => { const tileContent = ( <> @@ -222,6 +226,7 @@ const DeviceListItem: React.FC<{ setPushNotifications={setPushNotifications} supportsMSC3881={supportsMSC3881} className="mx_FilteredDeviceList_deviceDetails" + delegatedAuthAccountUrl={delegatedAuthAccountUrl} /> )} @@ -250,7 +255,7 @@ export const FilteredDeviceList = forwardRef( setPushNotifications, setSelectedDeviceIds, supportsMSC3881, - disableMultipleSignout, + delegatedAuthAccountUrl, }: Props, ref: ForwardedRef, ) => { @@ -311,7 +316,7 @@ export const FilteredDeviceList = forwardRef( selectedDeviceCount={selectedDeviceIds.length} isAllSelected={isAllSelected} toggleSelectAll={toggleSelectAll} - isSelectDisabled={disableMultipleSignout} + isSelectDisabled={!!delegatedAuthAccountUrl} > {selectedDeviceIds.length ? ( <> @@ -361,7 +366,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} + isSelectDisabled={!!delegatedAuthAccountUrl} onDeviceExpandToggle={() => onDeviceExpandToggle(device.device_id)} onSignOutDevice={() => onSignOutDevices([device.device_id])} saveDeviceName={(deviceName: string) => saveDeviceName(device.device_id, deviceName)} @@ -373,6 +378,7 @@ export const FilteredDeviceList = forwardRef( setPushNotifications={setPushNotifications} toggleSelected={() => toggleSelection(device.device_id)} supportsMSC3881={supportsMSC3881} + delegatedAuthAccountUrl={delegatedAuthAccountUrl} /> ))} diff --git a/src/components/views/settings/tabs/user/AccountUserSettingsTab.tsx b/src/components/views/settings/tabs/user/AccountUserSettingsTab.tsx index cd52b2a76b..cb1d2a3edd 100644 --- a/src/components/views/settings/tabs/user/AccountUserSettingsTab.tsx +++ b/src/components/views/settings/tabs/user/AccountUserSettingsTab.tsx @@ -186,7 +186,9 @@ const AccountUserSettingsTab: React.FC = ({ closeSettingsFn }) => { canSetDisplayName={canSetDisplayName} canSetAvatar={canSetAvatar} /> - + {(!isAccountManagedExternally || canMake3pidChanges) && ( + + )} return !!confirmed; }; -const confirmDelegatedAuthSignOut = async (delegatedAuthAccountUrl: string, deviceId: string): Promise => { - const { finished } = Modal.createDialog(OidcLogoutDialog, { - deviceId, - delegatedAuthAccountUrl, - }); - const [confirmed] = await finished; - - return !!confirmed; -}; - const useSignOut = ( matrixClient: MatrixClient, onSignoutResolvedCallback: () => Promise, @@ -93,20 +83,10 @@ const useSignOut = ( if (!deviceIds.length) { return; } - // we can only sign out exactly one OIDC-aware device at a time - // we should not encounter this - if (delegatedAuthAccountUrl && deviceIds.length !== 1) { - logger.warn("Unexpectedly tried to sign out multiple OIDC-aware devices."); - return; - } - // delegated auth logout flow confirms and signs out together - // so only confirm if we are NOT doing a delegated auth sign out - if (!delegatedAuthAccountUrl) { - const userConfirmedSignout = await confirmSignOut(deviceIds.length); - if (!userConfirmedSignout) { - return; - } + const userConfirmedSignout = await confirmSignOut(deviceIds.length); + if (!userConfirmedSignout) { + return; } let success = false; @@ -115,11 +95,8 @@ const useSignOut = ( if (delegatedAuthAccountUrl) { const [deviceId] = deviceIds; - try { - success = await confirmDelegatedAuthSignOut(delegatedAuthAccountUrl, deviceId); - } catch (error) { - logger.error("Error deleting OIDC-aware sessions", error); - } + const url = getManageDeviceUrl(delegatedAuthAccountUrl, deviceId); + window.open(url, "_blank"); } else { const deferredSuccess = defer(); await deleteDevicesWithInteractiveAuth(matrixClient, deviceIds, async (success) => { @@ -323,6 +300,7 @@ const SessionManagerTab: React.FC<{ onSignOutCurrentDevice={onSignOutCurrentDevice} signOutAllOtherSessions={signOutAllOtherSessions} otherSessionsCount={otherSessionsCount} + delegatedAuthAccountUrl={delegatedAuthAccountUrl} /> {shouldShowOtherSessions && ( )} diff --git a/src/i18n/strings/en_EN.json b/src/i18n/strings/en_EN.json index e0edd074c2..42cf08ae70 100644 --- a/src/i18n/strings/en_EN.json +++ b/src/i18n/strings/en_EN.json @@ -238,7 +238,6 @@ "oidc": { "error_title": "We couldn't log you in", "generic_auth_error": "Something went wrong during authentication. Go to the sign in page and try again.", - "logout_redirect_warning": "You will be redirected to your server's authentication provider to complete sign out.", "missing_or_invalid_stored_state": "We asked the browser to remember which homeserver you use to let you sign in, but unfortunately your browser has forgotten it. Go to the sign in page and try again." }, "password_field_keep_going_prompt": "Keep going…", @@ -2821,6 +2820,7 @@ "inactive_sessions_list_description": "Consider signing out from old sessions (%(inactiveAgeDays)s days or older) you don't use anymore.", "ip": "IP address", "last_activity": "Last activity", + "manage": "Manage this session", "mobile_session": "Mobile session", "n_sessions_selected": { "one": "%(count)s session selected", diff --git a/src/utils/oidc/getOidcLogoutUrl.ts b/src/utils/oidc/getOidcLogoutUrl.ts deleted file mode 100644 index be632b815a..0000000000 --- a/src/utils/oidc/getOidcLogoutUrl.ts +++ /dev/null @@ -1,20 +0,0 @@ -/* -Copyright 2024 New Vector Ltd. -Copyright 2023 The Matrix.org Foundation C.I.C. - -SPDX-License-Identifier: AGPL-3.0-only OR GPL-3.0-only -Please see LICENSE files in the repository root for full details. -*/ - -/** - * Create a delegated auth account management URL with logout params as per MSC3824 and MSC2965 - * https://github.com/matrix-org/matrix-spec-proposals/blob/hughns/sso-redirect-action/proposals/3824-oidc-aware-clients.md#definition-of-oidc-aware - * https://github.com/sandhose/matrix-doc/blob/msc/sandhose/oidc-discovery/proposals/2965-oidc-discovery.md#account-management-url-parameters - */ -export const getOidcLogoutUrl = (delegatedAuthAccountUrl: string, deviceId: string): string => { - const logoutUrl = new URL(delegatedAuthAccountUrl); - logoutUrl.searchParams.set("action", "session_end"); - logoutUrl.searchParams.set("device_id", deviceId); - - return logoutUrl.toString(); -}; diff --git a/src/utils/oidc/urls.ts b/src/utils/oidc/urls.ts new file mode 100644 index 0000000000..9e95e81c34 --- /dev/null +++ b/src/utils/oidc/urls.ts @@ -0,0 +1,32 @@ +/* +Copyright 2024 New Vector Ltd. +Copyright 2023 The Matrix.org Foundation C.I.C. + +SPDX-License-Identifier: AGPL-3.0-only OR GPL-3.0-only +Please see LICENSE files in the repository root for full details. +*/ + +enum Action { + Profile = "org.matrix.profile", + SessionsList = "org.matrix.sessions_list", + SessionView = "org.matrix.session_view", + SessionEnd = "org.matrix.session_end", + AccountDeactivate = "org.matrix.account_deactivate", + CrossSigningReset = "org.matrix.cross_signing_reset", +} + +const getUrl = (authUrl: string, action: Action): URL => { + const url = new URL(authUrl); + url.searchParams.set("action", action); + return url; +}; + +/** + * Create a delegated auth account management URL with logout params as per MSC4191 + * https://github.com/matrix-org/matrix-spec-proposals/blob/quenting/account-deeplink/proposals/4191-account-deeplink.md#possible-actions + */ +export const getManageDeviceUrl = (delegatedAuthAccountUrl: string, deviceId: string): string => { + const url = getUrl(delegatedAuthAccountUrl, Action.SessionView); + url.searchParams.set("device_id", deviceId); + return url.toString(); +}; diff --git a/test/unit-tests/components/views/settings/devices/__snapshots__/CurrentDeviceSection-test.tsx.snap b/test/unit-tests/components/views/settings/devices/__snapshots__/CurrentDeviceSection-test.tsx.snap index 74a0fb919b..a3465ab588 100644 --- a/test/unit-tests/components/views/settings/devices/__snapshots__/CurrentDeviceSection-test.tsx.snap +++ b/test/unit-tests/components/views/settings/devices/__snapshots__/CurrentDeviceSection-test.tsx.snap @@ -49,7 +49,7 @@ HTMLCollection [

- Verify or sign out from this session for best security and reliability. + Verify your current session for enhanced secure messaging.

", () => { expect(manageAccountLink.getAttribute("href")).toMatch(accountManagementLink); }); - describe("deactive account", () => { + describe("deactivate account", () => { it("should not render section when account deactivation feature is disabled", () => { jest.spyOn(SettingsStore, "getValue").mockImplementation( (settingName) => settingName !== UIFeature.Deactivate, @@ -198,6 +198,11 @@ describe("", () => { describe("3pids", () => { beforeEach(() => { + const mockOidcClientStore = { + accountManagementEndpoint: undefined, + } as unknown as OidcClientStore; + jest.spyOn(stores, "oidcClientStore", "get").mockReturnValue(mockOidcClientStore); + mockClient.getCapabilities.mockResolvedValue({ "m.3pid_changes": { enabled: true, diff --git a/test/unit-tests/components/views/settings/tabs/user/SessionManagerTab-test.tsx b/test/unit-tests/components/views/settings/tabs/user/SessionManagerTab-test.tsx index 87411e18a1..2f65776ef0 100644 --- a/test/unit-tests/components/views/settings/tabs/user/SessionManagerTab-test.tsx +++ b/test/unit-tests/components/views/settings/tabs/user/SessionManagerTab-test.tsx @@ -1234,34 +1234,13 @@ describe("", () => { toggleDeviceDetails(getByTestId, alicesMobileDevice.device_id); const deviceDetails = getByTestId(`device-detail-${alicesMobileDevice.device_id}`); - const signOutButton = deviceDetails.querySelector( + const manageDeviceButton = deviceDetails.querySelector( '[data-testid="device-detail-sign-out-cta"]', ) as Element; - fireEvent.click(signOutButton); - - await screen.findByRole("dialog"); - expect( - screen.getByText( - "You will be redirected to your server's authentication provider to complete sign out.", - ), - ).toBeInTheDocument(); - // correct link to auth provider - expect(screen.getByText("Continue")).toHaveAttribute( + expect(manageDeviceButton).toHaveAttribute( "href", - `https://issuer.org/account?action=session_end&device_id=${alicesMobileDevice.device_id}`, + `https://issuer.org/account?action=org.matrix.session_view&device_id=${alicesMobileDevice.device_id}`, ); - - // go to the link - fireEvent.click(screen.getByText("Continue")); - await flushPromises(); - - // come back from the link and close the modal - fireEvent.click(screen.getByText("Close")); - - await flushPromises(); - - // devices were refreshed - expect(mockClient.getDevices).toHaveBeenCalled(); }); it("does not allow removing multiple devices at once", async () => { diff --git a/test/unit-tests/components/views/settings/tabs/user/__snapshots__/AccountUserSettingsTab-test.tsx.snap b/test/unit-tests/components/views/settings/tabs/user/__snapshots__/AccountUserSettingsTab-test.tsx.snap index 5c6a8ac8ee..1387b984d8 100644 --- a/test/unit-tests/components/views/settings/tabs/user/__snapshots__/AccountUserSettingsTab-test.tsx.snap +++ b/test/unit-tests/components/views/settings/tabs/user/__snapshots__/AccountUserSettingsTab-test.tsx.snap @@ -169,7 +169,7 @@ exports[` 3pids should display 3pid email addresses an
`; -exports[` deactive account should render section when account deactivation feature is enabled 1`] = ` +exports[` deactivate account should render section when account deactivation feature is enabled 1`] = `