nodejs/hookshot
nodejs/hookshot
1
0
Fork 0
mirror of https://github.com/matrix-org/matrix-hookshot.git synced 2025-03-10 13:17:08 +00:00
Code Issues Packages Projects Releases Wiki Activity

Ensure joined to room when checking widget API permissions. (#691)

Browse Source 
* 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
This commit is contained in:
Will Hunt 2023-03-27 17:56:37 +01:00 committed by GitHub
parent 037674115a
commit 74a577a97f
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
6 changed files with 82 additions and 49 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

1
changelog.d/691.bugfix Normal file
View File

@ -0,0 +1 @@
Improve resiliency to invite/join races when Hookshot is added by an integration manager.

9
src/Bridge.ts
View File

@ -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},

16
src/PromiseUtil.ts
View File

@ -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<T>(actionFn: () => T,
export async function retry<T>(actionFn: () => PromiseLike<T>,
maxAttempts: number,
waitFor: number = SLEEP_TIME_MS,
filterFn: RetryFn = DEFAULT_RETRY): Promise<T> {
@ -49,8 +47,8 @@ export async function retry<T>(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;

75
src/Widgets/BridgeWidgetApi.ts
View File

@ -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<unknown>) => {
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<BotUser> {
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) {

7
src/provisioning/api.ts
View File

@ -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<Config = object, Secrets = object> 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);
}

23
web/components/roomConfig/RoomConfig.tsx
View File

@ -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<SConfig, ConnectionType extends GetConnectionsResponseItem, ConnectionState extends IConnectionState> {
serviceConfig: SConfig;
loginLabel?: string;
@ -40,6 +40,9 @@ interface IRoomConfigProps<SConfig, ConnectionType extends GetConnectionsRespons
migrationCandidates?: ConnectionType[];
migrationComparator?: (migrated: ConnectionType, native: ConnectionType) => boolean;
}
const MAX_CONNECTION_FETCH_ATTEMPTS = 10;
export const RoomConfig = function<SConfig, ConnectionType extends GetConnectionsResponseItem, ConnectionState extends IConnectionState>(props: IRoomConfigProps<SConfig, ConnectionType, ConnectionState>) {
const {
api,
@ -67,17 +70,25 @@ export const RoomConfig = function<SConfig, ConnectionType extends GetConnection
}
useEffect(() => {
api.getConnectionsForService<ConnectionType>(roomId, type).then(res => {
const fetchConnections = retry(
() => {
return api.getConnectionsForService<ConnectionType>(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<ConnectionType[]>([]);
@ -158,7 +169,7 @@ export const RoomConfig = function<SConfig, ConnectionType extends GetConnection
showAuthPrompt={showAuthPrompt}
/>}
</section>}
{ connections === null && <LoadingSpinner /> }
{ !error && connections === null && <LoadingSpinner /> }
{ !!connections?.length && <section>
<h2>{ canEditRoom ? text.listCanEdit : text.listCantEdit }</h2>
{ serviceConfig && connections?.map(c => <ListItem key={c.id} text={listItemName(c)}>