From 5c5bdd546d748a1e6ffdf8798ea2ae3ae74bb28c Mon Sep 17 00:00:00 2001 From: Will Hunt Date: Fri, 29 Dec 2023 16:11:08 +0000 Subject: [PATCH] Fix widgets not working when bound to the same listener as webhooks. (#872) * Bind listener in bridge. * Add a test to confirm behaviour * Fix test name * changelog --- changelog.d/872.bugfix | 1 + spec/util/e2e-test.ts | 12 ++++++++++- spec/widgets.spec.ts | 45 ++++++++++++++++++++++++++++++++++++++++++ src/App/BridgeApp.ts | 7 ------- src/Bridge.ts | 5 ++++- src/config/Config.ts | 3 +++ web/BridgeAPI.ts | 8 ++++---- 7 files changed, 68 insertions(+), 13 deletions(-) create mode 100644 changelog.d/872.bugfix create mode 100644 spec/widgets.spec.ts diff --git a/changelog.d/872.bugfix b/changelog.d/872.bugfix new file mode 100644 index 000000000..ac41d8a88 --- /dev/null +++ b/changelog.d/872.bugfix @@ -0,0 +1 @@ +Fix widgets not loading when bound to the same listener as "webhooks". diff --git a/spec/util/e2e-test.ts b/spec/util/e2e-test.ts index 2d079bc99..c66bfb548 100644 --- a/spec/util/e2e-test.ts +++ b/spec/util/e2e-test.ts @@ -151,8 +151,13 @@ export class E2ETestMatrixClient extends MatrixClient { } export class E2ETestEnv { + + static get workerId() { + return parseInt(process.env.JEST_WORKER_ID ?? '0'); + } + static async createTestEnv(opts: Opts): Promise { - const workerID = parseInt(process.env.JEST_WORKER_ID ?? '0'); + const workerID = this.workerId; const { matrixLocalparts, config: providedConfig } = opts; const keyPromise = new Promise((resolve, reject) => generateKeyPair("rsa", { modulusLength: 4096, @@ -178,6 +183,11 @@ export class E2ETestEnv { await writeFile(keyPath, privateKey, 'utf-8'); const webhooksPort = 9500 + workerID; + if (providedConfig?.widgets) { + providedConfig.widgets.openIdOverrides = { + 'hookshot': homeserver.url, + } + } const config = new BridgeConfig({ bridge: { domain: homeserver.domain, diff --git a/spec/widgets.spec.ts b/spec/widgets.spec.ts new file mode 100644 index 000000000..658710eaf --- /dev/null +++ b/spec/widgets.spec.ts @@ -0,0 +1,45 @@ +import { E2ESetupTestTimeout, E2ETestEnv } from "./util/e2e-test"; +import { describe, it, beforeEach, afterEach } from "@jest/globals"; +import { BridgeAPI } from "../web/BridgeAPI"; +import { WidgetApi } from "matrix-widget-api"; + +describe('Widgets', () => { + let testEnv: E2ETestEnv; + + beforeEach(async () => { + const webhooksPort = 9500 + E2ETestEnv.workerId; + testEnv = await E2ETestEnv.createTestEnv({matrixLocalparts: ['user'], config: { + widgets: { + publicUrl: `http://localhost:${webhooksPort}` + }, + listeners: [{ + port: webhooksPort, + bindAddress: '0.0.0.0', + // Bind to the SAME listener to ensure we don't have conflicts. + resources: ['webhooks', 'widgets'], + }], + + }}); + await testEnv.setUp(); + }, E2ESetupTestTimeout); + + afterEach(() => { + return testEnv?.tearDown(); + }); + + it('should be able to authenticate with the widget API', async () => { + const user = testEnv.getUser('user'); + const bridgeApi = await BridgeAPI.getBridgeAPI(testEnv.opts.config?.widgets?.publicUrl!, { + requestOpenIDConnectToken: () => { + return user.getOpenIDConnectToken() + }, + } as unknown as WidgetApi, { + getItem() { return null}, + setItem() { }, + } as unknown as Storage); + expect(await bridgeApi.verify()).toEqual({ + "type": "widget", + "userId": "@user:hookshot", + }); + }); +}); diff --git a/src/App/BridgeApp.ts b/src/App/BridgeApp.ts index c66112400..da63d4260 100644 --- a/src/App/BridgeApp.ts +++ b/src/App/BridgeApp.ts @@ -60,13 +60,6 @@ export async function start(config: BridgeConfig, registration: IAppserviceRegis // Don't care to await this, as the process is about to end storage.disconnect?.(); }); - - // XXX: Since the webhook listener listens on /, it must listen AFTER other resources - // have bound themselves. - if (config.queue.monolithic) { - const webhookHandler = new Webhooks(config); - listener.bindResource('webhooks', webhookHandler.expressRouter); - } return { bridgeApp, storage, diff --git a/src/Bridge.ts b/src/Bridge.ts index a4b93b9b5..c92517458 100644 --- a/src/Bridge.ts +++ b/src/Bridge.ts @@ -19,7 +19,7 @@ import { MessageQueue, MessageQueueMessageOut, createMessageQueue } from "./Mess import { MessageSenderClient } from "./MatrixSender"; import { NotifFilter, NotificationFilterStateContent } from "./NotificationFilters"; import { NotificationProcessor } from "./NotificationsProcessor"; -import { NotificationsEnableEvent, NotificationsDisableEvent } from "./Webhooks"; +import { NotificationsEnableEvent, NotificationsDisableEvent, Webhooks } from "./Webhooks"; import { GitHubOAuthToken, GitHubOAuthTokenResponse, ProjectsGetResponseData } from "./github/Types"; import { retry } from "./PromiseUtil"; import { UserNotificationsEvent } from "./Notifications/UserNotificationWatcher"; @@ -788,6 +788,9 @@ export class Bridge { ); } + const webhookHandler = new Webhooks(this.config); + this.listener.bindResource('webhooks', webhookHandler.expressRouter); + await this.as.begin(); log.info(`Bridge is now ready. Found ${this.connectionManager.size} connections`); this.ready = true; diff --git a/src/config/Config.ts b/src/config/Config.ts index 1b3ba734d..374c295ab 100644 --- a/src/config/Config.ts +++ b/src/config/Config.ts @@ -334,6 +334,9 @@ export class BridgeConfigGenericWebhooks { interface BridgeWidgetConfigYAML { publicUrl: string; + /** + * @deprecated Prefer using listener config. + */ port?: number; addToAdminRooms?: boolean; roomSetupWidget?: { diff --git a/web/BridgeAPI.ts b/web/BridgeAPI.ts index eed9f81f0..5e9d80ee8 100644 --- a/web/BridgeAPI.ts +++ b/web/BridgeAPI.ts @@ -24,9 +24,9 @@ interface RequestOpts { export class BridgeAPI { - static async getBridgeAPI(baseUrl: string, widgetApi: WidgetApi): Promise { + static async getBridgeAPI(baseUrl: string, widgetApi: WidgetApi, storage = localStorage): Promise { try { - const sessionToken = localStorage.getItem('hookshot-sessionToken'); + const sessionToken = storage.getItem('hookshot-sessionToken'); baseUrl = baseUrl.endsWith("/") ? baseUrl.slice(0, -1) : baseUrl; if (sessionToken) { const client = new BridgeAPI(baseUrl, sessionToken); @@ -36,7 +36,7 @@ export class BridgeAPI { } catch (ex) { // TODO: Check that the token is actually invalid, rather than just assuming we need to refresh. console.warn(`Failed to verify token, fetching new token`, ex); - localStorage.removeItem(sessionToken); + storage.removeItem(sessionToken); } } } catch (ex) { @@ -74,7 +74,7 @@ export class BridgeAPI { } const response = await res.json() as ExchangeOpenAPIResponseBody; try { - localStorage.setItem('hookshot-sessionToken', response.token); + storage.setItem('hookshot-sessionToken', response.token); } catch (ex) { // E.g. Browser prevents storage access. console.debug(`Failed to store session token, continuing`, ex);