OIDC: Redirect to delegated auth provider when signing out (#11432)

Kerry · hughns · andybalaam · web-flow · commit 23196d49e187 · 2023-08-22T11:15:35.000Z
* 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

* redirect to auth provider when signing out

* open auth provider in new tab, refresh sessions on return

* correct comment

* fix bad copy paste

* try to make sonar happy

* Update for latest revision of MSCs

* Update SessionManagerTab-test.tsx

* Make InteractiveAuthCallback async and await it

---------

Co-authored-by: Hugh Nimmo-Smith &lt;hughns@matrix.org&gt;
Co-authored-by: Hugh Nimmo-Smith &lt;hughns@users.noreply.github.com&gt;
Co-authored-by: Andy Balaam &lt;andy.balaam@matrix.org&gt;
diff --git a/src/components/structures/InteractiveAuth.tsx b/src/components/structures/InteractiveAuth.tsx
@@ -35,8 +35,8 @@ type InteractiveAuthCallbackSuccess<T> = (
     success: true,
     response: T,
     extra?: { emailSid?: string; clientSecret?: string },
-) => void;
-type InteractiveAuthCallbackFailure = (success: false, response: IAuthData | Error) => void;
+) => Promise<void>;
+type InteractiveAuthCallbackFailure = (success: false, response: IAuthData | Error) => Promise<void>;
 export type InteractiveAuthCallback<T> = InteractiveAuthCallbackSuccess<T> & InteractiveAuthCallbackFailure;
 
 export interface InteractiveAuthProps<T> {
@@ -141,15 +141,15 @@ export default class InteractiveAuthComponent<T> extends React.Component<Interac
     public componentDidMount(): void {
         this.authLogic
             .attemptAuth()
-            .then((result) => {
+            .then(async (result) => {
                 const extra = {
                     emailSid: this.authLogic.getEmailSid(),
                     clientSecret: this.authLogic.getClientSecret(),
                 };
-                this.props.onAuthFinished(true, result, extra);
+                await this.props.onAuthFinished(true, result, extra);
             })
-            .catch((error) => {
-                this.props.onAuthFinished(false, error);
+            .catch(async (error) => {
+                await this.props.onAuthFinished(false, error);
                 logger.error("Error during user-interactive auth:", error);
                 if (this.unmounted) {
                     return;
@@ -251,12 +251,12 @@ export default class InteractiveAuthComponent<T> extends React.Component<Interac
         this.props.onStagePhaseChange?.(this.state.authStage ?? null, newPhase || 0);
     };
 
-    private onStageCancel = (): void => {
-        this.props.onAuthFinished(false, ERROR_USER_CANCELLED);
+    private onStageCancel = async (): Promise<void> => {
+        await this.props.onAuthFinished(false, ERROR_USER_CANCELLED);
     };
 
-    private onAuthStageFailed = (e: Error): void => {
-        this.props.onAuthFinished(false, e);
+    private onAuthStageFailed = async (e: Error): Promise<void> => {
+        await this.props.onAuthFinished(false, e);
     };
 
     private setEmailSid = (sid: string): void => {
diff --git a/src/components/views/dialogs/DeactivateAccountDialog.tsx b/src/components/views/dialogs/DeactivateAccountDialog.tsx
@@ -110,7 +110,7 @@ export default class DeactivateAccountDialog extends React.Component<IProps, ISt
         this.setState({ bodyText, continueText, continueKind });
     };
 
-    private onUIAuthFinished: InteractiveAuthCallback<Awaited<ReturnType<MatrixClient["deactivateAccount"]>>> = (
+    private onUIAuthFinished: InteractiveAuthCallback<Awaited<ReturnType<MatrixClient["deactivateAccount"]>>> = async (
         success,
         result,
     ) => {
diff --git a/src/components/views/dialogs/InteractiveAuthDialog.tsx b/src/components/views/dialogs/InteractiveAuthDialog.tsx
@@ -116,7 +116,7 @@ export default class InteractiveAuthDialog<T> extends React.Component<Interactiv
         };
     }
 
-    private onAuthFinished: InteractiveAuthCallback<T> = (success, result): void => {
+    private onAuthFinished: InteractiveAuthCallback<T> = async (success, result): Promise<void> => {
         if (success) {
             this.props.onFinished(true, result);
         } else {
diff --git a/src/components/views/dialogs/oidc/OidcLogoutDialog.tsx b/src/components/views/dialogs/oidc/OidcLogoutDialog.tsx
@@ -0,0 +1,74 @@
+/*
+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 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<OidcLogoutDialogProps> = ({
+    delegatedAuthAccountUrl,
+    deviceId,
+    onFinished,
+}) => {
+    const [hasOpenedLogoutLink, setHasOpenedLogoutLink] = useState(false);
+    const logoutUrl = getOidcLogoutUrl(delegatedAuthAccountUrl, deviceId);
+
+    return (
+        <BaseDialog onFinished={onFinished} title={_t("Sign out")} contentId="mx_Dialog_content">
+            <div className="mx_Dialog_content" id="mx_Dialog_content">
+                {_t("You will be redirected to your server's authentication provider to complete sign out.")}
+            </div>
+            <div className="mx_Dialog_buttons">
+                {hasOpenedLogoutLink ? (
+                    <AccessibleButton kind="primary" onClick={() => onFinished(true)}>
+                        {_t("Close")}
+                    </AccessibleButton>
+                ) : (
+                    <>
+                        <AccessibleButton kind="secondary" onClick={() => onFinished(false)}>
+                            {_t("Cancel")}
+                        </AccessibleButton>
+                        <AccessibleButton
+                            element="a"
+                            onClick={() => setHasOpenedLogoutLink(true)}
+                            kind="primary"
+                            href={logoutUrl}
+                            target="_blank"
+                        >
+                            {_t("Continue")}
+                        </AccessibleButton>
+                    </>
+                )}
+            </div>
+        </BaseDialog>
+    );
+};
diff --git a/src/components/views/settings/devices/deleteDevices.tsx b/src/components/views/settings/devices/deleteDevices.tsx
@@ -40,7 +40,7 @@ export const deleteDevicesWithInteractiveAuth = async (
     try {
         await makeDeleteRequest(matrixClient, deviceIds)(null);
         // no interactive auth needed
-        onFinished(true, undefined);
+        await onFinished(true, undefined);
     } catch (error) {
         if (!(error instanceof MatrixError) || error.httpStatus !== 401 || !error.data?.flows) {
             // doesn't look like an interactive-auth failure
diff --git a/src/components/views/settings/tabs/user/SessionManagerTab.tsx b/src/components/views/settings/tabs/user/SessionManagerTab.tsx
@@ -40,6 +40,7 @@ import { FilterVariation } from "../../devices/filter";
 import { OtherSessionsSectionHeading } from "../../devices/OtherSessionsSectionHeading";
 import { SettingsSection } from "../../shared/SettingsSection";
 import { getDelegatedAuthAccountUrl } from "../../../../../utils/oidc/getDelegatedAuthAccountUrl";
+import { OidcLogoutDialog } from "../../../dialogs/oidc/OidcLogoutDialog";
 
 const confirmSignOut = async (sessionsToSignOutCount: number): Promise<boolean> => {
     const { finished } = Modal.createDialog(QuestionDialog, {
@@ -61,9 +62,20 @@ const confirmSignOut = async (sessionsToSignOutCount: number): Promise<boolean>
     return !!confirmed;
 };
 
+const confirmDelegatedAuthSignOut = async (delegatedAuthAccountUrl: string, deviceId: string): Promise<boolean> => {
+    const { finished } = Modal.createDialog(OidcLogoutDialog, {
+        deviceId,
+        delegatedAuthAccountUrl,
+    });
+    const [confirmed] = await finished;
+
+    return !!confirmed;
+};
+
 const useSignOut = (
     matrixClient: MatrixClient,
     onSignoutResolvedCallback: () => Promise<void>,
+    delegatedAuthAccountUrl?: string,
 ): {
     onSignOutCurrentDevice: () => void;
     onSignOutOtherDevices: (deviceIds: ExtendedDevice["device_id"][]) => Promise<void>;
@@ -85,19 +97,44 @@ const useSignOut = (
         if (!deviceIds.length) {
             return;
         }
-        const userConfirmedSignout = await confirmSignOut(deviceIds.length);
-        if (!userConfirmedSignout) {
+        // 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;
+            }
+        }
+
         try {
             setSigningOutDeviceIds([...signingOutDeviceIds, ...deviceIds]);
-            await deleteDevicesWithInteractiveAuth(matrixClient, deviceIds, async (success): Promise<void> => {
+
+            const onSignOutFinished = async (success: boolean): Promise<void> => {
                 if (success) {
                     await onSignoutResolvedCallback();
                 }
                 setSigningOutDeviceIds(signingOutDeviceIds.filter((deviceId) => !deviceIds.includes(deviceId)));
-            });
+            };
+
+            if (delegatedAuthAccountUrl) {
+                const [deviceId] = deviceIds;
+                try {
+                    setSigningOutDeviceIds([...signingOutDeviceIds, deviceId]);
+                    const success = await confirmDelegatedAuthSignOut(delegatedAuthAccountUrl, deviceId);
+                    await onSignOutFinished(success);
+                } catch (error) {
+                    logger.error("Error deleting OIDC-aware sessions", error);
+                }
+            } else {
+                await deleteDevicesWithInteractiveAuth(matrixClient, deviceIds, onSignOutFinished);
+            }
         } catch (error) {
             logger.error("Error deleting sessions", error);
             setSigningOutDeviceIds(signingOutDeviceIds.filter((deviceId) => !deviceIds.includes(deviceId)));
@@ -200,6 +237,7 @@ const SessionManagerTab: React.FC = () => {
     const { onSignOutCurrentDevice, onSignOutOtherDevices, signingOutDeviceIds } = useSignOut(
         matrixClient,
         onSignoutResolvedCallback,
+        delegatedAuthAccountUrl,
     );
 
     useEffect(
diff --git a/src/i18n/strings/en_EN.json b/src/i18n/strings/en_EN.json
@@ -3285,6 +3285,7 @@
     "Not a valid Security Key": "Not a valid Security Key",
     "Access your secure message history and set up secure messaging by entering your Security Key.": "Access your secure message history and set up secure messaging by entering your Security Key.",
     "If you've forgotten your Security Key you can <button>set up new recovery options</button>": "If you've forgotten your Security Key you can <button>set up new recovery options</button>",
+    "You will be redirected to your server's authentication provider to complete sign out.": "You will be redirected to your server's authentication provider to complete sign out.",
     "Send custom account data event": "Send custom account data event",
     "Send custom room account data event": "Send custom room account data event",
     "Event Type": "Event Type",
diff --git a/src/utils/oidc/getOidcLogoutUrl.ts b/src/utils/oidc/getOidcLogoutUrl.ts
@@ -0,0 +1,28 @@
+/*
+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.
+*/
+
+/**
+ * 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/test/components/views/settings/tabs/user/SessionManagerTab-test.tsx b/test/components/views/settings/tabs/user/SessionManagerTab-test.tsx
@@ -55,6 +55,9 @@ import { getClientInformationEventType } from "../../../../../../src/utils/devic
 
 mockPlatformPeg();
 
+// to restore later
+const realWindowLocation = window.location;
+
 describe("<SessionManagerTab />", () => {
     const aliceId = "@alice:server.org";
     const deviceId = "alices_device";
@@ -166,7 +169,7 @@ describe("<SessionManagerTab />", () => {
         confirm = true,
     ): Promise<void> => {
         // modal has sleeps in rendering process :(
-        await sleep(100);
+        await screen.findByRole("dialog");
         const buttonId = confirm ? "dialog-primary-button" : "dialog-cancel-button";
         fireEvent.click(getByTestId(buttonId));
 
@@ -227,11 +230,23 @@ describe("<SessionManagerTab />", () => {
             }
         });
 
+        // @ts-ignore allow delete of non-optional prop
+        delete window.location;
+        // @ts-ignore ugly mocking
+        window.location = {
+            href: "https://localhost/",
+            origin: "https://localhost/",
+        };
+
         // sometimes a verification modal is in modal state when these tests run
         // make sure the coast is clear
         await clearAllModals();
     });
 
+    afterAll(() => {
+        window.location = realWindowLocation;
+    });
+
     it("renders spinner while devices load", () => {
         const { container } = render(getComponent());
         expect(container.getElementsByClassName("mx_Spinner").length).toBeTruthy();
@@ -858,7 +873,7 @@ describe("<SessionManagerTab />", () => {
 
                 await flushPromises();
                 // modal rendering has some weird sleeps
-                await sleep(100);
+                await screen.findByRole("dialog");
 
                 expect(mockClient.deleteMultipleDevices).toHaveBeenCalledWith(
                     [alicesMobileDevice.device_id],
@@ -1082,26 +1097,20 @@ describe("<SessionManagerTab />", () => {
             });
 
             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],
-                        });
+                it("opens delegated auth provider to sign out a single device", async () => {
+                    mockClient.getDevices.mockResolvedValue({
+                        devices: [alicesDevice, alicesMobileDevice, alicesOlderMobileDevice],
+                    });
 
                     const { getByTestId } = render(getComponent());
 
                     await act(async () => {
                         await flushPromises();
                     });
 
+                    // reset call count
+                    mockClient.getDevices.mockClear();
+
                     toggleDeviceDetails(getByTestId, alicesMobileDevice.device_id);
 
                     const deviceDetails = getByTestId(`device-detail-${alicesMobileDevice.device_id}`);
@@ -1110,23 +1119,28 @@ describe("<SessionManagerTab />", () => {
                     ) as Element;
                     fireEvent.click(signOutButton);
 
-                    await confirmSignout(getByTestId);
-
-                    // sign out button is disabled with spinner
+                    await screen.findByRole("dialog");
                     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,
+                        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(
+                        "href",
+                        `https://issuer.org/account?action=session_end&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 refreshed
+                    // devices were refreshed
                     expect(mockClient.getDevices).toHaveBeenCalled();
                 });
 

Original file line number	Diff line number	Diff line change
`@@ -116,7 +116,7 @@ export default class InteractiveAuthDialog<T> extends React.Component<Interactiv`
`116`	`116`	`};`
`117`	`117`	`}`
`118`	`118`
`119`		`- private onAuthFinished: InteractiveAuthCallback<T> = (success, result): void => {`
	`119`	`+ private onAuthFinished: InteractiveAuthCallback<T> = async (success, result): Promise<void> => {`
`120`	`120`	`if (success) {`
`121`	`121`	`this.props.onFinished(true, result);`
`122`	`122`	`} else {`