ModuleAPI: overwrite_login action was not stopping the existing client resulting in the action failing with rust-sdk (#12272)

* Fix overwrite login action not stopping client

* remove unneeded fixture for overwrite login test

* Fix playwrite bad import of app sources

* revert uneeded change on fore OnLoggedIn causing side effects

* Add unit test for overwrite login action

* remove un needed ts-ignore

playwright/e2e/login/overwrite_login.spec.ts

@@ -0,0 +1,53 @@
+/*
+Copyright 2024 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 { test, expect } from "../../element-web-test";
+import { logIntoElement } from "../crypto/utils";
+
+test.describe("Overwrite login action", () => {
+    test("Try replace existing login with new one", async ({ page, app, credentials, homeserver }) => {
+        await logIntoElement(page, homeserver, credentials);
+
+        const userMenu = await app.openUserMenu();
+        await expect(userMenu.getByText(credentials.userId)).toBeVisible();
+
+        const bobRegister = await homeserver.registerUser("BobOverwrite", "p@ssword1!", "BOB");
+
+        // just assert that it's a different user
+        expect(credentials.userId).not.toBe(bobRegister.userId);
+
+        const clientCredentials /* IMatrixClientCreds */ = {
+            homeserverUrl: homeserver.config.baseUrl,
+            ...bobRegister,
+        };
+
+        // Trigger the overwrite login action
+        await app.client.evaluate(async (cli, clientCredentials) => {
+            // @ts-ignore - raw access to the dispatcher to simulate the action
+            window.mxDispatcher.dispatch(
+                {
+                    action: "overwrite_login",
+                    credentials: clientCredentials,
+                },
+                true,
+            );
+        }, clientCredentials);
+
+        // It should be now another user!!
+        const newUserMenu = await app.openUserMenu();
+        await expect(newUserMenu.getByText(bobRegister.userId)).toBeVisible();
+    });
+});

src/Lifecycle.ts

@@ -97,8 +97,20 @@ dis.register((payload) => {
         onLoggedOut();
     } else if (payload.action === Action.OverwriteLogin) {
         const typed = <OverwriteLoginPayload>payload;
-        // noinspection JSIgnoredPromiseFromCall - we don't care if it fails
+        // Stop the current client before overwriting the login.
-        doSetLoggedIn(typed.credentials, true);
+        // If not done it might be impossible to clear the storage, as the
+        // rust crypto backend might be holding an open connection to the indexeddb store.
+        // We also use the `unsetClient` flag to false, because at this point we are
+        // already in the logged in flows of the `MatrixChat` component, and it will
+        // always expect to have a client (calls to `MatrixClientPeg.safeGet()`).
+        // If we unset the client and the component is updated,  the render will fail and unmount everything.
+        // (The module dialog closes and fires a `aria_unhide_main_app` that will trigger a re-render)
+        stopMatrixClient(false);
+        doSetLoggedIn(typed.credentials, true).catch((e) => {
+            // XXX we might want to fire a new event here to let the app know that the login failed ?
+            // The module api could use it to display a message to the user.
+            logger.warn("Failed to overwrite login", e);
+        });
     }
 });
 

src/modules/ProxiedModuleApi.ts

@@ -160,6 +160,12 @@ export class ProxiedModuleApi implements ModuleApi {
      * @override
      */
     public async overwriteAccountAuth(accountInfo: AccountAuthInfo): Promise<void> {
+        // We want to wait for the new login to complete before returning.
+        // See `Action.OnLoggedIn` in dispatcher.
+        const awaitNewLogin = new Promise<void>((resolve) => {
+            this.overrideLoginResolve = resolve;
+        });
+
         dispatcher.dispatch<OverwriteLoginPayload>(
             {
                 action: Action.OverwriteLogin,
@@ -172,9 +178,7 @@ export class ProxiedModuleApi implements ModuleApi {
         ); // require to be sync to match inherited interface behaviour
 
         // wait for login to complete
-        await new Promise<void>((resolve) => {
+        await awaitNewLogin;
-            this.overrideLoginResolve = resolve;
-        });
     }
 
     /**

test/Lifecycle-test.ts

@@ -32,6 +32,7 @@ import ToastStore from "../src/stores/ToastStore";
 import { OidcClientStore } from "../src/stores/oidc/OidcClientStore";
 import { makeDelegatedAuthConfig } from "./test-utils/oidc";
 import { persistOidcAuthenticatedSettings } from "../src/utils/oidc/persistOidcSettings";
+import { Action } from "../src/dispatcher/actions";
 
 const webCrypto = new Crypto();
 
@@ -823,4 +824,75 @@ describe("Lifecycle", () => {
             expect(oidcClientStore.revokeTokens).toHaveBeenCalledWith(accessToken, refreshToken);
         });
     });
+
+    describe("overwritelogin", () => {
+        beforeEach(async () => {
+            jest.spyOn(MatrixJs, "createClient").mockReturnValue(mockClient);
+        });
+
+        it("should replace the current login with a new one", async () => {
+            const stopSpy = jest.spyOn(mockClient, "stopClient").mockReturnValue(undefined);
+            const dis = window.mxDispatcher;
+
+            const firstLoginEvent: Promise<void> = new Promise((resolve) => {
+                dis.register(({ action }) => {
+                    if (action === Action.OnLoggedIn) {
+                        resolve();
+                    }
+                });
+            });
+            // set a logged in state
+            await setLoggedIn(credentials);
+
+            await firstLoginEvent;
+
+            expect(stopSpy).toHaveBeenCalledTimes(1);
+            // important the overwrite action should not call unset before replacing.
+            // So spy on it and make sure it's not called.
+            jest.spyOn(MatrixClientPeg, "unset").mockReturnValue(undefined);
+
+            expect(MatrixClientPeg.replaceUsingCreds).toHaveBeenCalledWith(
+                expect.objectContaining({
+                    userId,
+                }),
+                undefined,
+            );
+
+            const otherCredentials = {
+                ...credentials,
+                userId: "@bob:server.org",
+                deviceId: "def456",
+            };
+
+            const secondLoginEvent: Promise<void> = new Promise((resolve) => {
+                dis.register(({ action }) => {
+                    if (action === Action.OnLoggedIn) {
+                        resolve();
+                    }
+                });
+            });
+
+            // Trigger the overwrite login action
+            dis.dispatch(
+                {
+                    action: "overwrite_login",
+                    credentials: otherCredentials,
+                },
+                true,
+            );
+
+            await secondLoginEvent;
+            // the client should have been stopped
+            expect(stopSpy).toHaveBeenCalledTimes(2);
+
+            expect(MatrixClientPeg.replaceUsingCreds).toHaveBeenCalledWith(
+                expect.objectContaining({
+                    userId: otherCredentials.userId,
+                }),
+                undefined,
+            );
+
+            expect(MatrixClientPeg.unset).not.toHaveBeenCalled();
+        });
+    });
 });