Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes user authentication when registering via the module API #10257

Merged
merged 7 commits into from
Mar 7, 2023
Merged
Show file tree
Hide file tree
Changes from 3 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
18 changes: 14 additions & 4 deletions src/Lifecycle.ts
Original file line number Diff line number Diff line change
Expand Up @@ -73,8 +73,9 @@ dis.register((payload) => {
onLoggedOut();
} else if (payload.action === Action.OverwriteLogin) {
const typed = <OverwriteLoginPayload>payload;
// is invoked without dispatching of "on_logging_in"
// noinspection JSIgnoredPromiseFromCall - we don't care if it fails
doSetLoggedIn(typed.credentials, true);
doSetLoggedIn(typed.credentials, true, false);
}
});

Expand Down Expand Up @@ -573,10 +574,14 @@ export async function hydrateSession(credentials: IMatrixClientCreds): Promise<M
*
* @param {IMatrixClientCreds} credentials
* @param {Boolean} clearStorageEnabled
*
* @param {Boolean} dispatchOnLoggingIn if true then "on_logging_in" is dispatched
* @returns {Promise} promise which resolves to the new MatrixClient once it has been started
*/
async function doSetLoggedIn(credentials: IMatrixClientCreds, clearStorageEnabled: boolean): Promise<MatrixClient> {
async function doSetLoggedIn(
credentials: IMatrixClientCreds,
clearStorageEnabled: boolean,
dispatchOnLoggingIn = true,
): Promise<MatrixClient> {
credentials.guest = Boolean(credentials.guest);

const softLogout = isSoftLogout();
Expand All @@ -602,7 +607,12 @@ async function doSetLoggedIn(credentials: IMatrixClientCreds, clearStorageEnable
//
// we fire it *synchronously* to make sure it fires before on_logged_in.
// (dis.dispatch uses `window.setTimeout`, which does not guarantee ordering.)
dis.dispatch({ action: "on_logging_in" }, true);
//
// can be disabled to resolve "Cannot dispatch in the middle of a dispatch."
// error when it is invoked via another dispatch that is not yet finished.
if (dispatchOnLoggingIn) {
dis.dispatch({ action: "on_logging_in" }, true);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

"on_logging_in" doesn't seem to be used in the react-sdk or element-web code

Copy link
Contributor

Choose a reason for hiding this comment

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

When creating new actions, please use the Action enum.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have added "on_logging_in" to Action, but this action wasn't created in this PR. I just added an if condition around it to fix the dispatcher.

}

if (clearStorageEnabled) {
await clearStorage();
Expand Down
18 changes: 18 additions & 0 deletions src/modules/ProxiedModuleApi.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ import { MatrixClientPeg } from "../MatrixClientPeg";
import { getCachedRoomIDForAlias } from "../RoomAliasCache";
import { Action } from "../dispatcher/actions";
import { OverwriteLoginPayload } from "../dispatcher/payloads/OverwriteLoginPayload";
import { ActionPayload } from "../dispatcher/payloads";

/**
* Glue between the `ModuleApi` interface and the react-sdk. Anticipates one instance
Expand All @@ -44,6 +45,18 @@ import { OverwriteLoginPayload } from "../dispatcher/payloads/OverwriteLoginPayl
export class ProxiedModuleApi implements ModuleApi {
private cachedTranslations: Optional<TranslationStringsObject>;

private overrideLoginResolve?: () => void;

public constructor() {
dispatcher.register(this.onAction);
}

private onAction = (payload: ActionPayload): void => {
if (payload.action === Action.OnLoggedIn) {
this.overrideLoginResolve?.();
}
};

/**
* All custom translations used by the associated module.
*/
Expand Down Expand Up @@ -154,6 +167,11 @@ export class ProxiedModuleApi implements ModuleApi {
},
true,
); // require to be sync to match inherited interface behaviour

// wait for login to complete
await new Promise<void>((resolve) => {
this.overrideLoginResolve = resolve;
});
}

/**
Expand Down
86 changes: 86 additions & 0 deletions test/Lifecycle-test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
/*
Copyright 2023 Mikhail Aheichyk
Copyright 2023 Nordeck IT + Consulting GmbH.

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 { AccountAuthInfo } from "@matrix-org/react-sdk-module-api/lib/types/AccountAuthInfo";
import { logger } from "matrix-js-sdk/src/logger";

import defaultDispatcher from "../src/dispatcher/dispatcher";
import { OverwriteLoginPayload } from "../src/dispatcher/payloads/OverwriteLoginPayload";
import { Action } from "../src/dispatcher/actions";
import { setLoggedIn } from "../src/Lifecycle";
import { stubClient } from "./test-utils";
import { ActionPayload } from "../src/dispatcher/payloads";

jest.mock("../src/utils/createMatrixClient", () => ({
__esModule: true,
default: jest.fn().mockReturnValue({
clearStores: jest.fn(),
}),
}));

describe("Lifecycle", () => {
stubClient();

jest.spyOn(logger, "error").mockReturnValue(undefined);
jest.spyOn(logger, "warn").mockReturnValue(undefined);
jest.spyOn(logger, "log").mockImplementation(undefined);

it("should call 'overwrite_login' callback and receive 'on_logged_in'", async () => {
// promise to wait 'on_logged_in'
const loggedInPromise = new Promise((resolve, reject) => {
defaultDispatcher.register((payload: ActionPayload) => {
if (payload.action === Action.OnLoggedIn) {
resolve(undefined);
}
});
});

// dispatch 'overwrite_login'
const accountInfo = {} as unknown as AccountAuthInfo;
defaultDispatcher.dispatch<OverwriteLoginPayload>(
{
action: Action.OverwriteLogin,
credentials: {
...accountInfo,
guest: false,
},
},
true,
);

await expect(loggedInPromise).resolves.toBeUndefined();
});

it("should setLoggedIn", async () => {
// promise to wait 'on_logging_in'
const loggingInPromise = new Promise((resolve, reject) => {
defaultDispatcher.register((payload: ActionPayload) => {
if (payload.action === "on_logging_in") {
resolve(undefined);
}
});
});

await setLoggedIn({
accessToken: "some_token",
homeserverUrl: "https://example.com",
userId: "@some_user_id",
});

await expect(loggingInPromise).resolves.toBeUndefined();
});
});
26 changes: 26 additions & 0 deletions test/modules/ProxiedModuleApi-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,15 @@ limitations under the License.
*/

import { TranslationStringsObject } from "@matrix-org/react-sdk-module-api/lib/types/translations";
import { AccountAuthInfo } from "@matrix-org/react-sdk-module-api/lib/types/AccountAuthInfo";

import { ProxiedModuleApi } from "../../src/modules/ProxiedModuleApi";
import { stubClient } from "../test-utils";
import { setLanguage } from "../../src/languageHandler";
import { ModuleRunner } from "../../src/modules/ModuleRunner";
import { registerMockModule } from "./MockModule";
import defaultDispatcher from "../../src/dispatcher/dispatcher";
import { Action } from "../../src/dispatcher/actions";

describe("ProxiedApiModule", () => {
afterEach(() => {
Expand All @@ -44,6 +47,29 @@ describe("ProxiedApiModule", () => {
expect(api.translations).toBe(translations);
});

it("should overwriteAccountAuth", async () => {
const dispatchSpy = jest.spyOn(defaultDispatcher, "dispatch");

const api = new ProxiedModuleApi();
const accountInfo = {} as unknown as AccountAuthInfo;
const promise = api.overwriteAccountAuth(accountInfo);

expect(dispatchSpy).toHaveBeenCalledWith(
expect.objectContaining({
action: Action.OverwriteLogin,
credentials: {
...accountInfo,
guest: false,
},
}),
expect.anything(),
);

defaultDispatcher.fire(Action.OnLoggedIn);

await expect(promise).resolves.toBeUndefined();
});

describe("integration", () => {
it("should translate strings using translation system", async () => {
// Test setup
Expand Down
3 changes: 3 additions & 0 deletions test/test-utils/test-utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -231,6 +231,9 @@ export function createTestClient(): MatrixClient {
room_id: roomId,
});
}),
startClient: jest.fn().mockResolvedValue(undefined),
stopClient: jest.fn().mockReturnValue(undefined),
removeAllListeners: jest.fn().mockReturnValue(undefined),
} as unknown as MatrixClient;

client.reEmitter = new ReEmitter(client);
Expand Down