From 74a577a97fbd67f667aa3f7382fea09003da03f0 Mon Sep 17 00:00:00 2001 From: Will Hunt Date: Mon, 27 Mar 2023 17:56:37 +0100 Subject: [PATCH] Ensure joined to room when checking widget API permissions. (#691) * Add an ensureJoined to assertUserPermissions * Ensure web component retries connection fetches * Ensure errors are caught and logged as JSON on the widget API * changelog * non-linear retry timer * lint * Use retry * Make getBotUserInRoom try to join the room * Make this fn safe to handle --- changelog.d/691.bugfix | 1 + src/Bridge.ts | 9 ++- src/PromiseUtil.ts | 16 +++-- src/Widgets/BridgeWidgetApi.ts | 75 ++++++++++++++---------- src/provisioning/api.ts | 7 ++- web/components/roomConfig/RoomConfig.tsx | 23 ++++++-- 6 files changed, 82 insertions(+), 49 deletions(-) create mode 100644 changelog.d/691.bugfix diff --git a/changelog.d/691.bugfix b/changelog.d/691.bugfix new file mode 100644 index 00000000..b5f29d09 --- /dev/null +++ b/changelog.d/691.bugfix @@ -0,0 +1 @@ +Improve resiliency to invite/join races when Hookshot is added by an integration manager. \ No newline at end of file diff --git a/src/Bridge.ts b/src/Bridge.ts index b3ef3f49..d26c0e0d 100644 --- a/src/Bridge.ts +++ b/src/Bridge.ts @@ -818,7 +818,14 @@ export class Bridge { } // Accept the invite - await retry(() => botUser.intent.joinRoom(roomId), 5); + await retry(async () => { + try { + await botUser.intent.joinRoom(roomId); + } catch (ex) { + log.warn(`Failed to join ${roomId}`, ex); + throw ex; + } + }, 5); if (event.content.is_direct) { await botUser.intent.underlyingClient.setRoomAccountData( BRIDGE_ROOM_TYPE, roomId, {admin_user: event.sender}, diff --git a/src/PromiseUtil.ts b/src/PromiseUtil.ts index 1bf31676..44094495 100644 --- a/src/PromiseUtil.ts +++ b/src/PromiseUtil.ts @@ -1,10 +1,8 @@ import { StatusCodes } from "http-status-codes"; -import { Logger } from "matrix-appservice-bridge"; -import { MatrixError } from "matrix-bot-sdk"; -const SLEEP_TIME_MS = 250; +const SLEEP_TIME_MS = 1000; +const EXPONENT_DIVISOR = 20; const DEFAULT_RETRY = () => true; -const log = new Logger("PromiseUtil"); type RetryFn = (error: Error) => boolean|number; @@ -16,8 +14,8 @@ type RetryFn = (error: Error) => boolean|number; * - A `number` if the action should be retried with a specific wait period. * - `false` if the action should not be retried.. */ -export function retryMatrixErrorFilter(err: Error) { - if (err instanceof MatrixError && err.statusCode >= 400 && err.statusCode <= 499) { +export function retryMatrixErrorFilter(err: Error|{statusCode: number, retryAfterMs?: number}) { + if ('statusCode' in err && err.statusCode >= 400 && err.statusCode <= 499) { if (err.statusCode === StatusCodes.TOO_MANY_REQUESTS) { return err.retryAfterMs ?? true; } @@ -35,7 +33,7 @@ export function retryMatrixErrorFilter(err: Error) { * @returns The result of actionFn * @throws If the `maxAttempts` limit is exceeded, or the `filterFn` returns false. */ -export async function retry(actionFn: () => T, +export async function retry(actionFn: () => PromiseLike, maxAttempts: number, waitFor: number = SLEEP_TIME_MS, filterFn: RetryFn = DEFAULT_RETRY): Promise { @@ -49,8 +47,8 @@ export async function retry(actionFn: () => T, if (shouldRetry) { // If the filter returns a retry ms, use that. const timeMs = typeof shouldRetry === "number" ? - shouldRetry : waitFor * Math.pow(2, attempts); - log.warn(`Action failed (${ex}), retrying in ${timeMs}ms`); + // + shouldRetry : Math.pow(waitFor, 1 + (attempts / EXPONENT_DIVISOR)); await new Promise((r) => setTimeout(r, timeMs)); } else { throw ex; diff --git a/src/Widgets/BridgeWidgetApi.ts b/src/Widgets/BridgeWidgetApi.ts index 1c2af384..27a21463 100644 --- a/src/Widgets/BridgeWidgetApi.ts +++ b/src/Widgets/BridgeWidgetApi.ts @@ -17,8 +17,7 @@ import { AllowedTokenTypes, TokenType, UserTokenStore } from '../UserTokenStore' const log = new Logger("BridgeWidgetApi"); -export class BridgeWidgetApi { - private readonly api: ProvisioningApi; +export class BridgeWidgetApi extends ProvisioningApi { private readonly goNebMigrator?: GoNebMigrator; constructor( @@ -32,7 +31,7 @@ export class BridgeWidgetApi { private readonly tokenStore: UserTokenStore, private readonly github?: GithubInstance, ) { - this.api = new ProvisioningApi( + super( storageProvider, { apiPrefix: "/widgetapi", @@ -42,29 +41,30 @@ export class BridgeWidgetApi { disallowedIpRanges: config.widgets?.disallowedIpRanges, openIdOverride: config.widgets?.openIdOverrides, }); + const wrapHandler = (handler: (req: ProvisioningRequest, res: Response, next?: NextFunction) => Promise) => { - return async (req: ProvisioningRequest, res: Response, next?: NextFunction) => { - try { - await handler.call(this, req, res); - } catch (ex) { - // Pass to error handler without the req - next?.(ex); - } + return async (req: ProvisioningRequest, res: Response) => { + await handler.call(this, req, res); } } - this.api.addRoute("get", "/v1/state", wrapHandler(this.getRoomState)); - this.api.addRoute("get", '/v1/config/sections', wrapHandler(this.getConfigSections)); - this.api.addRoute("get", '/v1/service/:service/config', wrapHandler(this.getServiceConfig)); - this.api.addRoute("get", '/v1/:roomId/connections', wrapHandler(this.getConnections)); - this.api.addRoute("get", '/v1/:roomId/connections/:service', wrapHandler(this.getConnectionsForService)); - this.api.addRoute("post", '/v1/:roomId/connections/:type', wrapHandler(this.createConnection)); - this.api.addRoute("put", '/v1/:roomId/connections/:connectionId', wrapHandler(this.updateConnection)); - this.api.addRoute("patch", '/v1/:roomId/connections/:connectionId', wrapHandler(this.updateConnection)); - this.api.addRoute("delete", '/v1/:roomId/connections/:connectionId', wrapHandler(this.deleteConnection)); - this.api.addRoute("get", '/v1/targets/:type', wrapHandler(this.getConnectionTargets)); - this.api.addRoute('get', '/v1/service/:service/auth', wrapHandler(this.getAuth)); - this.api.addRoute('get', '/v1/service/:service/auth/:state', wrapHandler(this.getAuthPoll)); - this.api.addRoute('post', '/v1/service/:service/auth/logout', wrapHandler(this.postAuthLogout)); + this.addRoute("get", "/v1/state", wrapHandler(this.getRoomState)); + this.addRoute("get", '/v1/config/sections', wrapHandler(this.getConfigSections)); + this.addRoute("get", '/v1/service/:service/config', wrapHandler(this.getServiceConfig)); + this.addRoute("get", '/v1/:roomId/connections', wrapHandler(this.getConnections)); + this.addRoute("get", '/v1/:roomId/connections/:service', wrapHandler(this.getConnectionsForService)); + this.addRoute("post", '/v1/:roomId/connections/:type', wrapHandler(this.createConnection)); + this.addRoute("put", '/v1/:roomId/connections/:connectionId', wrapHandler(this.updateConnection)); + this.addRoute("patch", '/v1/:roomId/connections/:connectionId', wrapHandler(this.updateConnection)); + this.addRoute("delete", '/v1/:roomId/connections/:connectionId', wrapHandler(this.deleteConnection)); + this.addRoute("get", '/v1/targets/:type', wrapHandler(this.getConnectionTargets)); + this.addRoute('get', '/v1/service/:service/auth', wrapHandler(this.getAuth)); + this.addRoute('get', '/v1/service/:service/auth/:state', wrapHandler(this.getAuthPoll)); + this.addRoute('post', '/v1/service/:service/auth/logout', wrapHandler(this.postAuthLogout)); + this.baseRoute.use((err: unknown, _req: Express.Request, _res: Express.Response, _next: NextFunction) => { + // Needed until https://github.com/matrix-org/matrix-appservice-bridge/pull/465 lands. + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (this as any).onError(err, _req, _res, _next); + }); if (this.config.goNebMigrator) { this.goNebMigrator = new GoNebMigrator( @@ -73,7 +73,7 @@ export class BridgeWidgetApi { ); } - this.api.addRoute("get", "/v1/:roomId/goNebConnections", wrapHandler(this.getGoNebConnections)); + this.addRoute("get", "/v1/:roomId/goNebConnections", wrapHandler(this.getGoNebConnections)); } private async getGoNebConnections(req: ProvisioningRequest, res: Response) { @@ -88,17 +88,28 @@ export class BridgeWidgetApi { throw Error('Cannot get connections without a valid userId'); } - const botUser = this.getBotUserInRoom(roomId); + const botUser = await this.getBotUserInRoom(roomId); await assertUserPermissionsInRoom(req.userId, roomId, "read", botUser.intent); const connections = await this.goNebMigrator.getConnectionsForRoom(roomId, req.userId); res.send(connections); } - private getBotUserInRoom(roomId: string, serviceType?: string): BotUser { - const botUser = this.botUsersManager.getBotUserInRoom(roomId, serviceType); + private async getBotUserInRoom(roomId: string, serviceType?: string): Promise { + let botUser = this.botUsersManager.getBotUserInRoom(roomId, serviceType); if (!botUser) { - throw new ApiError("Bot is not joined to the room.", ErrCode.NotInRoom); + // Not bot in the room...yet. Let's try an ensure join. + const intent = (serviceType && this.botUsersManager.getBotUserForService(serviceType)?.intent) || this.as.botIntent; + try { + await intent.ensureJoined(roomId); + } catch (ex) { + // Just fail with this, we couldn't join. + throw new ApiError("Bot was not invited to the room.", ErrCode.NotInRoom); + } + botUser = this.botUsersManager.getBotUserInRoom(roomId, serviceType); + if (!botUser) { + throw new ApiError("Bot is not joined to the room.", ErrCode.NotInRoom); + } } return botUser; } @@ -149,7 +160,7 @@ export class BridgeWidgetApi { const roomId = req.params.roomId; const serviceType = req.params.service; - const botUser = this.getBotUserInRoom(roomId, serviceType); + const botUser = await this.getBotUserInRoom(roomId, serviceType); await assertUserPermissionsInRoom(req.userId, roomId, "read", botUser.intent); const allConnections = this.connMan.getAllConnectionsForRoom(roomId); const powerlevel = new PowerLevelsEvent({content: await botUser.intent.underlyingClient.getRoomStateEvent(roomId, "m.room.power_levels", "")}); @@ -193,7 +204,7 @@ export class BridgeWidgetApi { } const serviceType = connectionType.ServiceCategory; - const botUser = this.getBotUserInRoom(roomId, serviceType); + const botUser = await this.getBotUserInRoom(roomId, serviceType); await assertUserPermissionsInRoom(req.userId, roomId, "write", botUser.intent); try { if (!req.body || typeof req.body !== "object") { @@ -222,7 +233,7 @@ export class BridgeWidgetApi { const serviceType = req.params.type; const connectionId = req.params.connectionId; - const botUser = this.getBotUserInRoom(roomId, serviceType); + const botUser = await this.getBotUserInRoom(roomId, serviceType); await assertUserPermissionsInRoom(req.userId, roomId, "write", botUser.intent); const connection = this.connMan.getConnectionById(roomId, connectionId); if (!connection) { @@ -244,7 +255,7 @@ export class BridgeWidgetApi { const serviceType = req.params.type; const connectionId = req.params.connectionId; - const botUser = this.getBotUserInRoom(roomId, serviceType); + const botUser = await this.getBotUserInRoom(roomId, serviceType); await assertUserPermissionsInRoom(req.userId, roomId, "write", botUser.intent); const connection = this.connMan.getConnectionById(roomId, connectionId); if (!connection) { diff --git a/src/provisioning/api.ts b/src/provisioning/api.ts index 207600ab..26be7511 100644 --- a/src/provisioning/api.ts +++ b/src/provisioning/api.ts @@ -1,4 +1,4 @@ -import { Intent, MembershipEventContent, PowerLevelsEventContent } from "matrix-bot-sdk"; +import { Intent, MatrixError, MembershipEventContent, PowerLevelsEventContent } from "matrix-bot-sdk"; import { ApiError, ErrCode } from "../api"; import { Logger } from "matrix-appservice-bridge"; @@ -25,6 +25,8 @@ export interface GetConnectionsResponseItem e const log = new Logger("Provisioner.api"); export async function assertUserPermissionsInRoom(userId: string, roomId: string, requiredPermission: "read"|"write", intent: Intent) { + // Always do an ensureJoined to clear any possible invites. + await intent.ensureJoined(roomId); try { const membership = await intent.underlyingClient.getRoomStateEvent(roomId, "m.room.member", intent.userId) as MembershipEventContent; if (membership.membership === "invite") { @@ -33,6 +35,9 @@ export async function assertUserPermissionsInRoom(userId: string, roomId: string throw new ApiError("Bot is not joined to the room.", ErrCode.NotInRoom); } } catch (ex) { + if (ex instanceof MatrixError && ex.errcode === "M_FORBIDDEN") { + throw new ApiError(`User ${intent.userId} is not invited to the room.`, ErrCode.NotInRoom); + } if (isNotFoundError(ex)) { throw new ApiError("User is not joined to the room.", ErrCode.NotInRoom); } diff --git a/web/components/roomConfig/RoomConfig.tsx b/web/components/roomConfig/RoomConfig.tsx index e1e337dc..03da44d2 100644 --- a/web/components/roomConfig/RoomConfig.tsx +++ b/web/components/roomConfig/RoomConfig.tsx @@ -6,8 +6,8 @@ import style from "./RoomConfig.module.scss"; import { GetConnectionsResponseItem } from "../../../src/provisioning/api"; import { IConnectionState } from "../../../src/Connections"; import { LoadingSpinner } from '../elements/LoadingSpinner'; - - +import { ErrCode } from "../../../src/api"; +import { retry } from "../../../src/PromiseUtil"; export interface ConnectionConfigurationProps { serviceConfig: SConfig; loginLabel?: string; @@ -40,6 +40,9 @@ interface IRoomConfigProps boolean; } + +const MAX_CONNECTION_FETCH_ATTEMPTS = 10; + export const RoomConfig = function(props: IRoomConfigProps) { const { api, @@ -67,17 +70,25 @@ export const RoomConfig = function { - api.getConnectionsForService(roomId, type).then(res => { + const fetchConnections = retry( + () => { + return api.getConnectionsForService(roomId, type); + }, + MAX_CONNECTION_FETCH_ATTEMPTS, + 1000, + (ex) => ex instanceof BridgeAPIError && ex.errcode === ErrCode.NotInRoom + ); + + fetchConnections.then((res) => { setCanEditRoom(res.canEdit); setConnections(res.connections); clearCurrentError(); }).catch(ex => { - console.warn("Failed to fetch existing connections", ex); setError({ header: "Failed to fetch existing connections", message: ex instanceof BridgeAPIError ? ex.message : "Unknown error" }); - }); + }) }, [api, roomId, type, newConnectionKey]); const [ toMigrate, setToMigrate ] = useState([]); @@ -158,7 +169,7 @@ export const RoomConfig = function} } - { connections === null && } + { !error && connections === null && } { !!connections?.length &&

{ canEditRoom ? text.listCanEdit : text.listCantEdit }

{ serviceConfig && connections?.map(c =>