From 6233aa19c92fa62797bf47cf4cabb00bf3b9e1af Mon Sep 17 00:00:00 2001 From: Will Hunt Date: Fri, 17 Mar 2023 16:38:39 +0000 Subject: [PATCH] Refactor hookshot to include permission grant system (#655) * Add support for checking connection grants * Make this less noisy * Remove import * Trying to generic-ize the grant system * Implement for Figma * More grant reformatting * Add tests * changelog * Fix todo * Refactor grants * Add logging to grant checkers * Ensure we provide a sender --- changelog.d/655.feature | 1 + src/Bridge.ts | 8 +- src/Config/Config.ts | 16 +- src/Config/Defaults.ts | 8 +- src/ConnectionManager.ts | 22 ++- src/Connections/CommandConnection.ts | 4 +- src/Connections/FigmaFileConnection.ts | 23 ++- src/Connections/GithubDiscussion.ts | 35 ++++- src/Connections/GithubDiscussionSpace.ts | 24 ++- src/Connections/GithubProject.ts | 21 ++- src/Connections/GithubRepo.ts | 44 ++++-- src/Connections/GithubUserSpace.ts | 21 ++- src/Connections/GitlabIssue.ts | 30 +++- src/Connections/GitlabRepo.ts | 54 +++++-- src/Connections/IConnection.ts | 13 ++ src/Connections/JiraProject.ts | 68 +++++---- src/Github/GrantChecker.ts | 39 +++++ src/Gitlab/GrantChecker.ts | 40 +++++ src/IntentUtils.ts | 4 +- src/Jira/GrantChecker.ts | 33 +++++ src/Managers/BotUsersManager.ts | 2 +- src/grants/GrantCheck.ts | 114 +++++++++++++++ tests/connections/GitlabRepoTest.ts | 3 + tests/grants/GrantChecker.spec.ts | 177 +++++++++++++++++++++++ tests/utils/AppserviceMock.ts | 6 + tests/utils/IntentMock.ts | 17 +++ 26 files changed, 731 insertions(+), 96 deletions(-) create mode 100644 changelog.d/655.feature create mode 100644 src/Github/GrantChecker.ts create mode 100644 src/Gitlab/GrantChecker.ts create mode 100644 src/Jira/GrantChecker.ts create mode 100644 src/grants/GrantCheck.ts create mode 100644 tests/grants/GrantChecker.spec.ts diff --git a/changelog.d/655.feature b/changelog.d/655.feature new file mode 100644 index 00000000..c57f77f8 --- /dev/null +++ b/changelog.d/655.feature @@ -0,0 +1 @@ +Implement grant system to internally record all approved connections in hookshot. \ No newline at end of file diff --git a/src/Bridge.ts b/src/Bridge.ts index f2c0a1ca..7d544e56 100644 --- a/src/Bridge.ts +++ b/src/Bridge.ts @@ -1,6 +1,6 @@ import { AdminAccountData } from "./AdminRoomCommandHandler"; import { AdminRoom, BRIDGE_ROOM_TYPE, LEGACY_BRIDGE_ROOM_TYPE } from "./AdminRoom"; -import { Appservice, RichRepliesPreprocessor, IRichReplyMetadata, StateEvent, EventKind, PowerLevelsEvent, IAppserviceRegistration, Intent } from "matrix-bot-sdk"; +import { Appservice, RichRepliesPreprocessor, IRichReplyMetadata, StateEvent, EventKind, PowerLevelsEvent, Intent } from "matrix-bot-sdk"; import BotUsersManager from "./Managers/BotUsersManager"; import { BridgeConfig, BridgePermissionLevel, GitLabInstance } from "./Config/Config"; import { BridgeWidgetApi } from "./Widgets/BridgeWidgetApi"; @@ -494,7 +494,7 @@ export class Bridge { this.tokenStore, this.commentProcessor, this.messageClient, - this.config.github, + this.config, ); connManager.push(discussionConnection); } catch (ex) { @@ -1294,7 +1294,7 @@ export class Bridge { adminRoom.on("open.project", async (project: ProjectsGetResponseData) => { const [connection] = this.connectionManager?.getForGitHubProject(project.id) || []; if (!connection) { - const connection = await GitHubProjectConnection.onOpenProject(project, this.as, intent, adminRoom.userId); + const connection = await GitHubProjectConnection.onOpenProject(project, this.as, intent, this.config, adminRoom.userId); this.connectionManager?.push(connection); } else { await intent.underlyingClient.inviteUser(adminRoom.userId, connection.roomId); @@ -1318,7 +1318,7 @@ export class Bridge { this.tokenStore, this.commentProcessor, this.messageClient, - this.config.gitlab, + this.config, ); this.connectionManager?.push(newConnection); return intent.underlyingClient.inviteUser(adminRoom.userId, newConnection.roomId); diff --git a/src/Config/Config.ts b/src/Config/Config.ts index 95b58092..3d269b8f 100644 --- a/src/Config/Config.ts +++ b/src/Config/Config.ts @@ -515,15 +515,15 @@ export class BridgeConfig { @hideKey() private readonly bridgePermissions: BridgePermissions; - constructor(configData: BridgeConfigRoot, env: {[key: string]: string|undefined}) { + constructor(configData: BridgeConfigRoot, env?: {[key: string]: string|undefined}) { this.bridge = configData.bridge; assert.ok(this.bridge); this.github = configData.github && new BridgeConfigGitHub(configData.github); - if (this.github?.auth && env["GITHUB_PRIVATE_KEY_FILE"]) { - this.github.auth.privateKeyFile = env["GITHUB_PRIVATE_KEY_FILE"]; + if (this.github?.auth && env?.["GITHUB_PRIVATE_KEY_FILE"]) { + this.github.auth.privateKeyFile = env?.["GITHUB_PRIVATE_KEY_FILE"]; } - if (this.github?.oauth && env["GITHUB_OAUTH_REDIRECT_URI"]) { - this.github.oauth.redirect_uri = env["GITHUB_OAUTH_REDIRECT_URI"]; + if (this.github?.oauth && env?.["GITHUB_OAUTH_REDIRECT_URI"]) { + this.github.oauth.redirect_uri = env?.["GITHUB_OAUTH_REDIRECT_URI"]; } this.gitlab = configData.gitlab && new BridgeConfigGitLab(configData.gitlab); this.figma = configData.figma; @@ -577,10 +577,10 @@ For more details, see https://github.com/matrix-org/matrix-hookshot/issues/594. } // TODO: Formalize env support - if (env.CFG_QUEUE_MONOLITHIC && ["false", "off", "no"].includes(env.CFG_QUEUE_MONOLITHIC)) { + if (env?.CFG_QUEUE_MONOLITHIC && ["false", "off", "no"].includes(env.CFG_QUEUE_MONOLITHIC)) { this.queue.monolithic = false; - this.queue.host = env.CFG_QUEUE_HOST; - this.queue.port = env.CFG_QUEUE_POST ? parseInt(env.CFG_QUEUE_POST, 10) : undefined; + this.queue.host = env?.CFG_QUEUE_HOST; + this.queue.port = env?.CFG_QUEUE_POST ? parseInt(env?.CFG_QUEUE_POST, 10) : undefined; } this.goNebMigrator = configData.goNebMigrator; diff --git a/src/Config/Defaults.ts b/src/Config/Defaults.ts index db9959cc..5e166f31 100644 --- a/src/Config/Defaults.ts +++ b/src/Config/Defaults.ts @@ -1,4 +1,4 @@ -import { BridgeConfig } from "./Config"; +import { BridgeConfig, BridgeConfigRoot } from "./Config"; import YAML from "yaml"; import { getConfigKeyMetadata, keyIsHidden } from "./Decorators"; import { Node, YAMLSeq } from "yaml/types"; @@ -8,7 +8,7 @@ import { DefaultDisallowedIpRanges } from "matrix-appservice-bridge"; const serverName = "example.com"; const hookshotWebhooksUrl = "https://example.com"; -export const DefaultConfig = new BridgeConfig({ +export const DefaultConfigRoot: BridgeConfigRoot = { bridge: { domain: serverName, url: "http://localhost:8008", @@ -147,7 +147,9 @@ export const DefaultConfig = new BridgeConfig({ resources: ['widgets'], } ] -}, {}); +}; + +export const DefaultConfig = new BridgeConfig(DefaultConfigRoot); function renderSection(doc: YAML.Document, obj: Record, parentNode?: YAMLSeq) { const entries = Object.entries(obj); diff --git a/src/ConnectionManager.ts b/src/ConnectionManager.ts index 419b4900..b509c7e1 100644 --- a/src/ConnectionManager.ts +++ b/src/ConnectionManager.ts @@ -166,7 +166,14 @@ export class ConnectionManager extends EventEmitter { } } - public createConnectionForState(roomId: string, state: StateEvent, rollbackBadState: boolean) { + /** + * This is called ONLY when we spot new state in a room and want to create a connection for it. + * @param roomId + * @param state + * @param rollbackBadState + * @returns + */ + public async createConnectionForState(roomId: string, state: StateEvent, rollbackBadState: boolean) { // Empty object == redacted if (state.content.disabled === true || Object.keys(state.content).length === 0) { log.debug(`${roomId} has disabled state for ${state.type}`); @@ -188,7 +195,7 @@ export class ConnectionManager extends EventEmitter { return; } - return connectionType.createConnectionForState(roomId, state, { + const connection = await connectionType.createConnectionForState(roomId, state, { as: this.as, intent: botUser.intent, config: this.config, @@ -198,8 +205,19 @@ export class ConnectionManager extends EventEmitter { storage: this.storage, github: this.github, }); + + // Finally, ensure the connection is allowed by us. + await connection.ensureGrant?.(state.sender); + return connection; } + /** + * This is called when hookshot starts up, or a hookshot service bot has left + * and we need to recalculate the right bots for all the connections in a room. + * @param roomId + * @param rollbackBadState + * @returns + */ public async createConnectionsForRoomId(roomId: string, rollbackBadState: boolean) { const botUser = this.botUsersManager.getBotUserInRoom(roomId); if (!botUser) { diff --git a/src/Connections/CommandConnection.ts b/src/Connections/CommandConnection.ts index a98fdaac..4d31d046 100644 --- a/src/Connections/CommandConnection.ts +++ b/src/Connections/CommandConnection.ts @@ -36,10 +36,10 @@ export abstract class CommandConnection) { - this.state = this.validateConnectionState(stateEv.content); + this.state = await this.validateConnectionState(stateEv.content); } - protected abstract validateConnectionState(content: unknown): ValidatedStateType; + protected abstract validateConnectionState(content: unknown): Promise|ValidatedStateType; public async onMessageEvent(ev: MatrixEvent, checkPermission: PermissionCheckFn) { const commandResult = await handleCommand( diff --git a/src/Connections/FigmaFileConnection.ts b/src/Connections/FigmaFileConnection.ts index bae5f847..f1e1fc21 100644 --- a/src/Connections/FigmaFileConnection.ts +++ b/src/Connections/FigmaFileConnection.ts @@ -5,8 +5,9 @@ import { BaseConnection } from "./BaseConnection"; import { IConnection, IConnectionState } from "."; import { Logger } from "matrix-appservice-bridge"; import { IBridgeStorageProvider } from "../Stores/StorageProvider"; -import { BridgeConfigFigma } from "../Config/Config"; +import { BridgeConfig } from "../Config/Config"; import { Connection, InstantiateConnectionOpts, ProvisionConnectionOpts } from "./IConnection"; +import { ConfigGrantChecker, GrantChecker } from "../grants/GrantCheck"; const log = new Logger("FigmaFileConnection"); @@ -47,7 +48,7 @@ export class FigmaFileConnection extends BaseConnection implements IConnection { if (!config.figma) { throw Error('Figma is not configured'); } - return new FigmaFileConnection(roomId, event.stateKey, event.content, config.figma, as, intent, storage); + return new FigmaFileConnection(roomId, event.stateKey, event.content, config, as, intent, storage); } static async provisionConnection(roomId: string, userId: string, data: Record = {}, {as, intent, config, storage}: ProvisionConnectionOpts) { @@ -55,7 +56,8 @@ export class FigmaFileConnection extends BaseConnection implements IConnection { throw Error('Figma is not configured'); } const validState = this.validateState(data); - const connection = new FigmaFileConnection(roomId, validState.fileId, validState, config.figma, as, intent, storage); + const connection = new FigmaFileConnection(roomId, validState.fileId, validState, config, as, intent, storage); + await new GrantChecker(as.botIntent, "figma").grantConnection(roomId, { fileId: validState.fileId, instanceName: validState.instanceName || "none"}); await intent.underlyingClient.sendStateEvent(roomId, FigmaFileConnection.CanonicalEventType, validState.fileId, validState); return { connection, @@ -63,12 +65,13 @@ export class FigmaFileConnection extends BaseConnection implements IConnection { } } + private readonly grantChecker: GrantChecker<{fileId: string, instanceName: string}> = new ConfigGrantChecker("figma", this.as, this.config); constructor( roomId: string, stateKey: string, private state: FigmaFileConnectionState, - private readonly config: BridgeConfigFigma, + private readonly config: BridgeConfig, private readonly as: Appservice, private readonly intent: Intent, private readonly storage: IBridgeStorageProvider) { @@ -91,6 +94,14 @@ export class FigmaFileConnection extends BaseConnection implements IConnection { return this.state.priority || super.priority; } + public async ensureGrant(sender?: string) { + return this.grantChecker.assertConnectionGranted(this.roomId, { fileId: this.state.fileId, instanceName: this.state.instanceName || "none"}, sender); + } + + public async onRemove() { + return this.grantChecker.ungrantConnection(this.roomId, { fileId: this.state.fileId, instanceName: this.state.instanceName || "none"}); + } + public async handleNewComment(payload: FigmaPayload) { // We need to check if the comment was actually new. // There isn't a way to tell how the comment has changed, so for now check the timestamps @@ -102,8 +113,8 @@ export class FigmaFileConnection extends BaseConnection implements IConnection { } let intent; - if (this.config.overrideUserId) { - intent = this.as.getIntentForUserId(this.config.overrideUserId); + if (this.config.figma?.overrideUserId) { + intent = this.as.getIntentForUserId(this.config.figma.overrideUserId); } else { intent = this.intent; } diff --git a/src/Connections/GithubDiscussion.ts b/src/Connections/GithubDiscussion.ts index 630f70ed..44407245 100644 --- a/src/Connections/GithubDiscussion.ts +++ b/src/Connections/GithubDiscussion.ts @@ -12,7 +12,9 @@ import { DiscussionCommentCreatedEvent } from "@octokit/webhooks-types"; import { GithubGraphQLClient } from "../Github/GithubInstance"; import { Logger } from "matrix-appservice-bridge"; import { BaseConnection } from "./BaseConnection"; -import { BridgeConfigGitHub } from "../Config/Config"; +import { BridgeConfig, BridgeConfigGitHub } from "../Config/Config"; +import { ConfigGrantChecker, GrantChecker } from "../grants/GrantCheck"; +import QuickLRU from "@alloc/quick-lru"; export interface GitHubDiscussionConnectionState { owner: string; repo: string; @@ -48,21 +50,20 @@ export class GitHubDiscussionConnection extends BaseConnection implements IConne } return new GitHubDiscussionConnection( roomId, as, intent, event.content, event.stateKey, tokenStore, commentProcessor, - messageClient, config.github, + messageClient, config, ); } - readonly sentEvents = new Set(); //TODO: Set some reasonable limits public static async createDiscussionRoom( as: Appservice, intent: Intent, userId: string|null, owner: string, repo: string, discussion: Discussion, tokenStore: UserTokenStore, commentProcessor: CommentProcessor, messageClient: MessageSenderClient, - config: BridgeConfigGitHub, + config: BridgeConfig, ) { const commentIntent = await getIntentForUser({ login: discussion.user.login, avatarUrl: discussion.user.avatar_url, - }, as, config.userIdPrefix); + }, as, config.github?.userIdPrefix); const state: GitHubDiscussionConnectionState = { owner, repo, @@ -94,9 +95,19 @@ export class GitHubDiscussionConnection extends BaseConnection implements IConne format: 'org.matrix.custom.html', }); await intent.ensureJoined(roomId); + return new GitHubDiscussionConnection(roomId, as, intent, state, '', tokenStore, commentProcessor, messageClient, config); } + private static grantKey(state: GitHubDiscussionConnectionState) { + return `${this.CanonicalEventType}/${state.owner}/${state.repo}`; + } + + private readonly sentEvents = new QuickLRU({ maxSize: 128 }); + private readonly grantChecker: GrantChecker; + + private readonly config: BridgeConfigGitHub; + constructor( roomId: string, private readonly as: Appservice, @@ -106,9 +117,14 @@ export class GitHubDiscussionConnection extends BaseConnection implements IConne private readonly tokenStore: UserTokenStore, private readonly commentProcessor: CommentProcessor, private readonly messageClient: MessageSenderClient, - private readonly config: BridgeConfigGitHub, + bridgeConfig: BridgeConfig, ) { super(roomId, stateKey, GitHubDiscussionConnection.CanonicalEventType); + if (!bridgeConfig.github) { + throw Error('Expected github to be enabled in config'); + } + this.config = bridgeConfig.github; + this.grantChecker = new ConfigGrantChecker("github", this.as, bridgeConfig); } public isInterestedInStateEvent(eventType: string, stateKey: string) { @@ -125,7 +141,7 @@ export class GitHubDiscussionConnection extends BaseConnection implements IConne const qlClient = new GithubGraphQLClient(octokit); const commentId = await qlClient.addDiscussionComment(this.state.internalId, ev.content.body); log.info(`Sent ${commentId} for ${ev.event_id} (${ev.sender})`); - this.sentEvents.add(commentId); + this.sentEvents.set(commentId, undefined); return true; } @@ -162,6 +178,7 @@ export class GitHubDiscussionConnection extends BaseConnection implements IConne public async onRemove() { log.info(`Removing ${this.toString()} for ${this.roomId}`); + await this.grantChecker.ungrantConnection(this.roomId, GitHubDiscussionConnection.grantKey(this.state)); // Do a sanity check that the event exists. try { await this.intent.underlyingClient.getRoomStateEvent(this.roomId, GitHubDiscussionConnection.CanonicalEventType, this.stateKey); @@ -171,4 +188,8 @@ export class GitHubDiscussionConnection extends BaseConnection implements IConne await this.intent.underlyingClient.sendStateEvent(this.roomId, GitHubDiscussionConnection.LegacyCanonicalEventType, this.stateKey, { disabled: true }); } } + + public async ensureGrant(sender?: string) { + await this.grantChecker.assertConnectionGranted(this.roomId, GitHubDiscussionConnection.grantKey(this.state), sender); + } } diff --git a/src/Connections/GithubDiscussionSpace.ts b/src/Connections/GithubDiscussionSpace.ts index fa158afc..4cc432d1 100644 --- a/src/Connections/GithubDiscussionSpace.ts +++ b/src/Connections/GithubDiscussionSpace.ts @@ -6,6 +6,8 @@ import axios from "axios"; import { GitHubDiscussionConnection } from "./GithubDiscussion"; import { GithubInstance } from "../Github/GithubInstance"; import { BaseConnection } from "./BaseConnection"; +import { ConfigGrantChecker, GrantChecker } from "../grants/GrantCheck"; +import { BridgeConfig } from "../Config/Config"; const log = new Logger("GitHubDiscussionSpace"); @@ -35,12 +37,13 @@ export class GitHubDiscussionSpace extends BaseConnection implements IConnection if (!github || !config.github) { throw Error('GitHub is not configured'); } + await new GrantChecker(as.botIntent, 'github').grantConnection(roomId, this.grantKey(event.content)); return new GitHubDiscussionSpace( - await intent.underlyingClient.getSpace(roomId), event.content, event.stateKey + as, config, await intent.underlyingClient.getSpace(roomId), event.content, event.stateKey ); } - static async onQueryRoom(result: RegExpExecArray, opts: {githubInstance: GithubInstance, as: Appservice}): Promise> { + public static async onQueryRoom(result: RegExpExecArray, opts: {githubInstance: GithubInstance, as: Appservice}): Promise> { if (!result || result.length < 2) { log.error(`Invalid alias pattern '${result}'`); throw Error("Could not find issue"); @@ -141,10 +144,19 @@ export class GitHubDiscussionSpace extends BaseConnection implements IConnection }; } - constructor(public readonly space: Space, + private static grantKey(state: GitHubDiscussionSpaceConnectionState) { + return `${this.CanonicalEventType}/${state.owner}/${state.repo}`; + } + + private readonly grantChecker: GrantChecker; + + constructor(as: Appservice, + config: BridgeConfig, + public readonly space: Space, private state: GitHubDiscussionSpaceConnectionState, stateKey: string) { super(space.roomId, stateKey, GitHubDiscussionSpace.CanonicalEventType) + this.grantChecker = new ConfigGrantChecker("github", as, config); } public isInterestedInStateEvent(eventType: string, stateKey: string) { @@ -167,9 +179,15 @@ export class GitHubDiscussionSpace extends BaseConnection implements IConnection log.info(`Adding connection to ${this.toString()}`); await this.space.addChildRoom(discussion.roomId); } + + + public async ensureGrant(sender?: string) { + await this.grantChecker.assertConnectionGranted(this.roomId, GitHubDiscussionSpace.grantKey(this.state), sender); + } public async onRemove() { log.info(`Removing ${this.toString()} for ${this.roomId}`); + this.grantChecker.ungrantConnection(this.roomId, GitHubDiscussionSpace.grantKey(this.state)); // Do a sanity check that the event exists. try { diff --git a/src/Connections/GithubProject.ts b/src/Connections/GithubProject.ts index 4b4c21d5..690cc70f 100644 --- a/src/Connections/GithubProject.ts +++ b/src/Connections/GithubProject.ts @@ -3,6 +3,8 @@ import { Appservice, Intent, StateEvent } from "matrix-bot-sdk"; import { Logger } from "matrix-appservice-bridge"; import { ProjectsGetResponseData } from "../Github/Types"; import { BaseConnection } from "./BaseConnection"; +import { ConfigGrantChecker, GrantChecker } from "../grants/GrantCheck"; +import { BridgeConfig } from "../Config/Config"; export interface GitHubProjectConnectionState { // eslint-disable-next-line camelcase @@ -28,10 +30,14 @@ export class GitHubProjectConnection extends BaseConnection implements IConnecti if (!config.github) { throw Error('GitHub is not configured'); } - return new GitHubProjectConnection(roomId, as, intent, event.content, event.stateKey); + return new GitHubProjectConnection(roomId, as, intent, config, event.content, event.stateKey); } - static async onOpenProject(project: ProjectsGetResponseData, as: Appservice, intent: Intent, inviteUser: string): Promise { + public static getGrantKey(projectId: number) { + return `${this.CanonicalEventType}/${projectId}`; + } + + static async onOpenProject(project: ProjectsGetResponseData, as: Appservice, intent: Intent, config: BridgeConfig, inviteUser: string): Promise { log.info(`Fetching ${project.name} ${project.id}`); // URL hack so we don't need to fetch the repo itself. @@ -55,22 +61,31 @@ export class GitHubProjectConnection extends BaseConnection implements IConnecti }, ], }); + await new GrantChecker(as.botIntent, 'github').grantConnection(roomId, this.getGrantKey(project.id)); - return new GitHubProjectConnection(roomId, as, intent, state, project.url) + return new GitHubProjectConnection(roomId, as, intent, config, state, project.url) } get projectId() { return this.state.project_id; } + private readonly grantChecker: GrantChecker; + constructor( public readonly roomId: string, as: Appservice, intent: Intent, + config: BridgeConfig, private state: GitHubProjectConnectionState, stateKey: string, ) { super(roomId, stateKey, GitHubProjectConnection.CanonicalEventType); + this.grantChecker = new ConfigGrantChecker("github", as, config); + } + + public ensureGrant(sender?: string) { + return this.grantChecker.assertConnectionGranted(this.roomId, GitHubProjectConnection.getGrantKey(this.state.project_id), sender); } public isInterestedInStateEvent(eventType: string, stateKey: string) { diff --git a/src/Connections/GithubRepo.ts b/src/Connections/GithubRepo.ts index b5a70887..f36707e4 100644 --- a/src/Connections/GithubRepo.ts +++ b/src/Connections/GithubRepo.ts @@ -28,6 +28,8 @@ import { PermissionCheckFn } from "."; import { MinimalGitHubIssue, MinimalGitHubRepo } from "../libRs"; import Ajv, { JSONSchemaType } from "ajv"; import { HookFilter } from "../HookFilter"; +import { GrantChecker } from "../grants/GrantCheck"; +import { GitHubGrantChecker } from "../Github/GrantChecker"; const log = new Logger("GitHubRepoConnection"); const md = new markdown(); @@ -359,11 +361,7 @@ export class GitHubRepoConnection extends CommandConnection, {as, intent, tokenStore, github, config}: ProvisionConnectionOpts) { - if (!github || !config.github) { - throw Error('GitHub is not configured'); - } - const validData = this.validateState(data); + static async assertUserHasAccessToRepo(userId: string, org: string, repo: string, github: GithubInstance, tokenStore: UserTokenStore) { const octokit = await tokenStore.getOctokitForUser(userId); if (!octokit) { throw new ApiError("User is not authenticated with GitHub", ErrCode.ForbiddenUser); @@ -371,8 +369,8 @@ export class GitHubRepoConnection extends CommandConnection, {as, intent, tokenStore, github, config}: ProvisionConnectionOpts) { + if (!github || !config.github) { + throw Error('GitHub is not configured'); + } + const validData = this.validateState(data); + await this.assertUserHasAccessToRepo(userId, validData.org, validData.repo, github, tokenStore); const appOctokit = await github.getSafeOctokitForRepo(validData.org, validData.repo); if (!appOctokit) { throw new ApiError( @@ -393,6 +399,7 @@ export class GitHubRepoConnection extends CommandConnection { @@ -501,6 +511,8 @@ export class GitHubRepoConnection extends CommandConnection, timeout: NodeJS.Timeout}>(); + private readonly grantChecker = new GitHubGrantChecker(this.as, this.githubInstance, this.tokenStore); + constructor( roomId: string, private readonly as: Appservice, @@ -557,8 +569,15 @@ export class GitHubRepoConnection extends CommandConnection) { @@ -1373,6 +1392,7 @@ export class GitHubRepoConnection extends CommandConnection, { - github, config, intent}: InstantiateConnectionOpts) { + github, config, intent, as}: InstantiateConnectionOpts) { if (!github || !config.github) { throw Error('GitHub is not configured'); } return new GitHubUserSpace( - await intent.underlyingClient.getSpace(roomId), event.content, event.stateKey + as, config, await intent.underlyingClient.getSpace(roomId), event.content, event.stateKey ); } @@ -137,12 +143,21 @@ export class GitHubUserSpace extends BaseConnection implements IConnection { }; } - constructor(public readonly space: Space, + private readonly grantChecker: GrantChecker; + + constructor(as: Appservice, + config: BridgeConfig, + public readonly space: Space, private state: GitHubUserSpaceConnectionState, stateKey: string) { super(space.roomId, stateKey, GitHubUserSpace.CanonicalEventType); + this.grantChecker = new ConfigGrantChecker("github", as, config); } + public ensureGrant(sender?: string) { + return this.grantChecker.assertConnectionGranted(this.roomId, GitHubUserSpace.grantKey(this.state), sender); + } + public isInterestedInStateEvent(eventType: string, stateKey: string) { return GitHubUserSpace.EventTypes.includes(eventType) && this.stateKey === stateKey; } diff --git a/src/Connections/GitlabIssue.ts b/src/Connections/GitlabIssue.ts index dc77be5b..742a6df8 100644 --- a/src/Connections/GitlabIssue.ts +++ b/src/Connections/GitlabIssue.ts @@ -5,11 +5,12 @@ import { UserTokenStore } from "../UserTokenStore"; import { Logger } from "matrix-appservice-bridge"; import { CommentProcessor } from "../CommentProcessor"; import { MessageSenderClient } from "../MatrixSender"; -import { BridgeConfigGitLab, GitLabInstance } from "../Config/Config"; +import { BridgeConfig, BridgeConfigGitLab, GitLabInstance } from "../Config/Config"; import { GetIssueResponse } from "../Gitlab/Types"; import { IGitLabWebhookNoteEvent } from "../Gitlab/WebhookTypes"; import { ensureUserIsInRoom, getIntentForUser } from "../IntentUtils"; import { BaseConnection } from "./BaseConnection"; +import { ConfigGrantChecker, GrantChecker } from "../grants/GrantCheck"; export interface GitLabIssueConnectionState { instance: string; @@ -70,7 +71,7 @@ export class GitLabIssueConnection extends BaseConnection implements IConnection commentProcessor, messageClient, instance, - config.gitlab, + config, ); } @@ -84,7 +85,7 @@ export class GitLabIssueConnection extends BaseConnection implements IConnection tokenStore: UserTokenStore, commentProcessor: CommentProcessor, messageSender: MessageSenderClient, - config: BridgeConfigGitLab, + config: BridgeConfig, ) { const state: GitLabIssueConnectionState = { projects, @@ -109,6 +110,11 @@ export class GitLabIssueConnection extends BaseConnection implements IConnection }, ], }); + await new GrantChecker(as.botIntent, "gitlab").grantConnection(roomId, { + instance: state.instance, + project: state.projects[0].toString(), + issue: state.iid.toString(), + }); return new GitLabIssueConnection(roomId, as, intent, state, issue.web_url, tokenStore, commentProcessor, messageSender, instance, config); } @@ -121,6 +127,9 @@ export class GitLabIssueConnection extends BaseConnection implements IConnection return this.instance.url; } + private readonly grantChecker: GrantChecker<{instance: string, project: string, issue: string}>; + private readonly config: BridgeConfigGitLab; + constructor( roomId: string, private readonly as: Appservice, @@ -131,9 +140,22 @@ export class GitLabIssueConnection extends BaseConnection implements IConnection private commentProcessor: CommentProcessor, private messageClient: MessageSenderClient, private instance: GitLabInstance, - private config: BridgeConfigGitLab, + config: BridgeConfig, ) { super(roomId, stateKey, GitLabIssueConnection.CanonicalEventType); + this.grantChecker = new ConfigGrantChecker("gitlab", as, config); + if (!config.gitlab) { + throw Error('No gitlab config!'); + } + this.config = config.gitlab; + } + + public ensureGrant(sender?: string) { + return this.grantChecker.assertConnectionGranted(this.roomId, { + instance: this.state.instance, + project: this.state.projects[0], + issue: this.state.iid.toString(), + }, sender); } public isInterestedInStateEvent(eventType: string, stateKey: string) { diff --git a/src/Connections/GitlabRepo.ts b/src/Connections/GitlabRepo.ts index 9b521688..9f5c0078 100644 --- a/src/Connections/GitlabRepo.ts +++ b/src/Connections/GitlabRepo.ts @@ -20,6 +20,7 @@ import { HookFilter } from "../HookFilter"; import { GitLabClient } from "../Gitlab/Client"; import { IBridgeStorageProvider } from "../Stores/StorageProvider"; import axios from "axios"; +import { GitLabGrantChecker } from "../Gitlab/GrantChecker"; export interface GitLabRepoConnectionState extends IConnectionState { instance: string; @@ -202,7 +203,7 @@ export class GitLabRepoConnection extends CommandConnection>, {intent, tokenStore, config}: InstantiateConnectionOpts) { + static async createConnectionForState(roomId: string, event: StateEvent>, {as, intent, tokenStore, config}: InstantiateConnectionOpts) { if (!config.gitlab) { throw Error('GitLab is not configured'); } @@ -211,18 +212,16 @@ export class GitLabRepoConnection extends CommandConnection, { config, intent, tokenStore, getAllConnectionsOfType }: ProvisionConnectionOpts) { - if (!config.gitlab) { - throw Error('GitLab is not configured'); - } - const gitlabConfig = config.gitlab; - const validData = this.validateState(data); - const instance = gitlabConfig.instances[validData.instance]; + public static async assertUserHasAccessToProject( + instanceName: string, path: string, requester: string, + tokenStore: UserTokenStore, config: BridgeConfigGitLab + ) { + const instance = config.instances[instanceName]; if (!instance) { - throw Error(`provisionConnection provided an instanceName of ${validData.instance} but the instance does not exist`); + throw Error(`provisionConnection provided an instanceName of ${instanceName} but the instance does not exist`); } const client = await tokenStore.getGitLabForUser(requester, instance.url); if (!client) { @@ -230,7 +229,7 @@ export class GitLabRepoConnection extends CommandConnection, { as, config, intent, tokenStore, getAllConnectionsOfType }: ProvisionConnectionOpts) { + if (!config.gitlab) { + throw Error('GitLab is not configured'); + } + const validData = this.validateState(data); + const gitlabConfig = config.gitlab; + const instance = gitlabConfig.instances[validData.instance]; + if (!instance) { + throw Error(`provisionConnection provided an instanceName of ${validData.instance} but the instance does not exist`); + } + const permissionLevel = await this.assertUserHasAccessToProject(validData.instance, validData.path, requester, tokenStore, gitlabConfig); + const client = await tokenStore.getGitLabForUser(requester, instance.url); + if (!client) { + throw new ApiError("User is not authenticated with GitLab", ErrCode.ForbiddenUser); + } const project = await client.projects.get(validData.path); - const stateEventKey = `${validData.instance}/${validData.path}`; - const connection = new GitLabRepoConnection(roomId, stateEventKey, intent, validData, tokenStore, instance); + const connection = new GitLabRepoConnection(roomId, stateEventKey, as, gitlabConfig, intent, validData, tokenStore, instance); const existingConnections = getAllConnectionsOfType(GitLabRepoConnection); const existing = existingConnections.find(c => c.roomId === roomId && c.instance.url === connection.instance.url && c.path === connection.path); @@ -282,6 +298,7 @@ export class GitLabRepoConnection extends CommandConnection({ maxSize: 100 }); private readonly hookFilter: HookFilter; + private readonly grantChecker; + constructor( roomId: string, stateKey: string, + as: Appservice, + config: BridgeConfigGitLab, private readonly intent: Intent, state: ConnectionStateValidated, private readonly tokenStore: UserTokenStore, @@ -397,6 +418,7 @@ export class GitLabRepoConnection extends CommandConnection) { + const validatedState = GitLabRepoConnection.validateState(stateEv.content); + await this.grantChecker.assertConnectionGranted(this.roomId, { + instance: validatedState.instance, + path: validatedState.path, + } , stateEv.sender); await super.onStateUpdate(stateEv); this.hookFilter.enabledHooks = this.state.enableHooks; } @@ -856,6 +883,7 @@ ${data.description}`; public async onRemove() { log.info(`Removing ${this.toString()} for ${this.roomId}`); + await this.grantChecker.ungrantConnection(this.roomId, { instance: this.state.instance, path: this.path }); // Do a sanity check that the event exists. try { await this.intent.underlyingClient.getRoomStateEvent(this.roomId, GitLabRepoConnection.CanonicalEventType, this.stateKey); diff --git a/src/Connections/IConnection.ts b/src/Connections/IConnection.ts index cc19aa07..b1f81157 100644 --- a/src/Connections/IConnection.ts +++ b/src/Connections/IConnection.ts @@ -25,6 +25,19 @@ export interface IConnection { priority: number; + /** + * Ensures that the current state loaded into the connection has been granted by + * the remote service. I.e. If the room is bridged into a GitHub repository, + * check that the *sender* has permission to bridge it. + * + * If a grant cannot be found, it may be determined by doing an API lookup against + * the remote service. + * + * @param sender The matrix ID of the sender of the event. + * @throws If the grant cannot be found, and cannot be detetermined, this will throw. + */ + ensureGrant?: (sender?: string) => void; + /** * The unique connection ID. This is a opaque hash of the roomId, connection type and state key. */ diff --git a/src/Connections/JiraProject.ts b/src/Connections/JiraProject.ts index 1df660d2..9d9a68cf 100644 --- a/src/Connections/JiraProject.ts +++ b/src/Connections/JiraProject.ts @@ -16,6 +16,8 @@ import JiraApi from "jira-client"; import { GetConnectionsResponseItem } from "../provisioning/api"; import { BridgeConfigJira } from "../Config/Config"; import { HookshotJiraApi } from "../Jira/Client"; +import { GrantChecker } from "../grants/GrantCheck"; +import { JiraGrantChecker } from "../Jira/GrantChecker"; type JiraAllowedEventsNames = "issue_created" | @@ -94,8 +96,6 @@ const md = new markdownit(); */ @Connection export class JiraProjectConnection extends CommandConnection implements IConnection { - - static readonly CanonicalEventType = "uk.half-shot.matrix-hookshot.jira.project"; static readonly LegacyCanonicalEventType = "uk.half-shot.matrix-github.jira.project"; @@ -107,35 +107,41 @@ export class JiraProjectConnection extends CommandConnection MatrixMessageContent; + static async assertUserHasAccessToProject(tokenStore: UserTokenStore, userId: string, urlStr: string) { + const url = new URL(urlStr); + const jiraClient = await tokenStore.getJiraForUser(userId, url.toString()); + if (!jiraClient) { + throw new ApiError("User is not authenticated with JIRA", ErrCode.ForbiddenUser); + } + const jiraResourceClient = await jiraClient.getClientForUrl(url); + if (!jiraResourceClient) { + throw new ApiError("User is not authenticated with this JIRA instance", ErrCode.ForbiddenUser); + } + const projectKey = JiraProjectConnection.getProjectKeyForUrl(url); + if (!projectKey) { + throw new ApiError("URL did not contain a valid project key", ErrCode.BadValue); + } + try { + // Need to check that the user can access this. + const project = await jiraResourceClient.getProject(projectKey); + return project; + } catch (ex) { + throw new ApiError("Requested project was not found", ErrCode.ForbiddenUser); + } + } + static async provisionConnection(roomId: string, userId: string, data: Record, {as, intent, tokenStore, config}: ProvisionConnectionOpts) { if (!config.jira) { throw new ApiError('JIRA integration is not configured', ErrCode.DisabledFeature); } const validData = validateJiraConnectionState(data); log.info(`Attempting to provisionConnection for ${roomId} ${validData.url} on behalf of ${userId}`); - const jiraClient = await tokenStore.getJiraForUser(userId, validData.url); - if (!jiraClient) { - throw new ApiError("User is not authenticated with JIRA", ErrCode.ForbiddenUser); - } - const jiraResourceClient = await jiraClient.getClientForUrl(new URL(validData.url)); - if (!jiraResourceClient) { - throw new ApiError("User is not authenticated with this JIRA instance", ErrCode.ForbiddenUser); - } + const project = await this.assertUserHasAccessToProject(tokenStore, userId, validData.url); const connection = new JiraProjectConnection(roomId, as, intent, validData, validData.url, tokenStore); - log.debug(`projectKey for ${validData.url} is ${connection.projectKey}`); - if (!connection.projectKey) { - throw Error('Expected projectKey to be defined'); - } - try { - // Need to check that the user can access this. - const project = await jiraResourceClient.getProject(connection.projectKey); - // Fetch the project's id now, to support events that identify projects by id instead of url - if (connection.state.id !== undefined && connection.state.id !== project.id) { - log.warn(`Updating ID of project ${connection.projectKey} from ${connection.state.id} to ${project.id}`); - connection.state.id = project.id; - } - } catch (ex) { - throw new ApiError("Requested project was not found", ErrCode.ForbiddenUser); + // Fetch the project's id now, to support events that identify projects by id instead of url + if (connection.state.id !== undefined && connection.state.id !== project.id) { + log.warn(`Updating ID of project ${connection.projectKey} from ${connection.state.id} to ${project.id}`); + connection.state.id = project.id; } await intent.underlyingClient.sendStateEvent(roomId, JiraProjectConnection.CanonicalEventType, connection.stateKey, validData); log.info(`Created connection via provisionConnection ${connection.toString()}`); @@ -200,13 +206,15 @@ export class JiraProjectConnection extends CommandConnection; + constructor( roomId: string, private readonly as: Appservice, private readonly intent: Intent, state: JiraProjectConnectionState, stateKey: string, - private readonly tokenStore: UserTokenStore, + private readonly tokenStore: UserTokenStore ) { super( roomId, @@ -227,6 +235,7 @@ export class JiraProjectConnection extends CommandConnection { + constructor(private readonly as: Appservice, private readonly github: GithubInstance, private readonly tokenStore: UserTokenStore) { + super(as.botIntent, "github") + } + + protected async checkFallback(roomId: string, connectionId: GitHubGrantConnectionId, sender?: string) { + if (!sender) { + log.debug(`Tried to check fallback for ${roomId} with a missing sender`); + // Cannot validate without a sender. + return false; + } + if (this.as.isNamespacedUser(sender)) { + // Bridge is always valid. + return true; + } + try { + await GitHubRepoConnection.assertUserHasAccessToRepo(sender, connectionId.org, connectionId.repo, this.github, this.tokenStore); + return true; + } catch (ex) { + log.info(`Tried to check fallback for ${roomId}: ${sender} does not have access to ${connectionId.org}/${connectionId.repo}`, ex); + return false; + } + } +} \ No newline at end of file diff --git a/src/Gitlab/GrantChecker.ts b/src/Gitlab/GrantChecker.ts new file mode 100644 index 00000000..1ce94731 --- /dev/null +++ b/src/Gitlab/GrantChecker.ts @@ -0,0 +1,40 @@ +import { Logger } from "matrix-appservice-bridge"; +import { Appservice } from "matrix-bot-sdk"; +import { BridgeConfigGitLab } from "../Config/Config"; +import { GitLabRepoConnection } from "../Connections"; +import { GrantChecker } from "../grants/GrantCheck"; +import { UserTokenStore } from "../UserTokenStore"; + +const log = new Logger('GitLabGrantChecker'); + +interface GitLabGrantConnectionId{ + instance: string; + path: string; +} + + + +export class GitLabGrantChecker extends GrantChecker { + constructor(private readonly as: Appservice, private readonly config: BridgeConfigGitLab, private readonly tokenStore: UserTokenStore) { + super(as.botIntent, "gitlab") + } + + protected async checkFallback(roomId: string, connectionId: GitLabGrantConnectionId, sender?: string) { + if (!sender) { + log.debug(`Tried to check fallback for ${roomId} with a missing sender`); + // Cannot validate without a sender. + return false; + } + if (this.as.isNamespacedUser(sender)) { + // Bridge is always valid. + return true; + } + try { + await GitLabRepoConnection.assertUserHasAccessToProject(connectionId.instance, connectionId.path, sender, this.tokenStore, this.config); + return true; + } catch (ex) { + log.info(`${sender} does not have access to ${connectionId.instance}/${connectionId.path}`, ex); + return false; + } + } +} \ No newline at end of file diff --git a/src/IntentUtils.ts b/src/IntentUtils.ts index e2c16172..31d2b5ba 100644 --- a/src/IntentUtils.ts +++ b/src/IntentUtils.ts @@ -35,9 +35,9 @@ export async function ensureUserIsInRoom(targetIntent: Intent, botClient: Matrix } } -export async function getIntentForUser(user: {avatarUrl?: string, login: string}, as: Appservice, prefix: string) { +export async function getIntentForUser(user: {avatarUrl?: string, login: string}, as: Appservice, prefix?: string) { const domain = as.botUserId.split(":")[1]; - const intent = as.getIntentForUserId(`@${prefix}${user.login}:${domain}`); + const intent = as.getIntentForUserId(`@${prefix ?? ''}${user.login}:${domain}`); const displayName = `${user.login}`; // Verify up-to-date profile let profile; diff --git a/src/Jira/GrantChecker.ts b/src/Jira/GrantChecker.ts new file mode 100644 index 00000000..211fc821 --- /dev/null +++ b/src/Jira/GrantChecker.ts @@ -0,0 +1,33 @@ +import { Appservice } from "matrix-bot-sdk"; +import { JiraProjectConnection } from "../Connections"; +import { GrantChecker } from "../grants/GrantCheck"; +import { UserTokenStore } from "../UserTokenStore"; + +interface JiraGrantConnectionId{ + url: string; +} + + + +export class JiraGrantChecker extends GrantChecker { + constructor(private readonly as: Appservice, private readonly tokenStore: UserTokenStore) { + super(as.botIntent, "jira") + } + + protected async checkFallback(roomId: string, connectionId: JiraGrantConnectionId, sender?: string) { + if (!sender) { + // Cannot validate without a sender. + return false; + } + if (this.as.isNamespacedUser(sender)) { + // Bridge is always valid. + return true; + } + try { + await JiraProjectConnection.assertUserHasAccessToProject(this.tokenStore, sender, connectionId.url); + return true; + } catch (ex) { + return false; + } + } +} \ No newline at end of file diff --git a/src/Managers/BotUsersManager.ts b/src/Managers/BotUsersManager.ts index a14d5217..46b26256 100644 --- a/src/Managers/BotUsersManager.ts +++ b/src/Managers/BotUsersManager.ts @@ -119,7 +119,7 @@ export default class BotUsersManager { * @param roomId */ onRoomJoin(botUser: BotUser, roomId: string): void { - log.info(`Bot user ${botUser.userId} joined room ${roomId}`); + log.debug(`Bot user ${botUser.userId} joined room ${roomId}`); const botUsers = this._botsInRooms.get(roomId) ?? new Set(); botUsers.add(botUser); this._botsInRooms.set(roomId, botUsers); diff --git a/src/grants/GrantCheck.ts b/src/grants/GrantCheck.ts new file mode 100644 index 00000000..b3eaa49f --- /dev/null +++ b/src/grants/GrantCheck.ts @@ -0,0 +1,114 @@ +import { Logger } from "matrix-appservice-bridge"; +import { Appservice, Intent, MatrixError } from "matrix-bot-sdk"; +import { BridgeConfig, BridgePermissionLevel } from "../Config/Config"; +import { FormatUtil } from "../FormatUtil"; + +const GRANT_ACCOUNT_DATA_KEY = "uk.half-shot.matrix-hookshot.grant"; + +interface GrantContent { + granted: boolean; +} + +const log = new Logger("GrantChecker"); + +export class GrantRejectedError extends Error { + constructor(public readonly roomId: string, public readonly connectionId: string) { + super(`No grant exists for ${roomId}/${connectionId}. Rejecting`); + } +} + + +type ConnectionId = string|object; + +export class GrantChecker { + private static stringifyConnectionId(connId: cId) { + if (typeof connId === "string") { + return FormatUtil.hashId(connId.toString()); + } + return FormatUtil.hashId(Object.entries(connId as Record).map((data) => `${data[0]}:${data[1]}`).join('')); + } + + constructor(private readonly intent: Intent, protected readonly grantType: string) { } + + /** + * If the connection hasn't been previously granted, we can use this function to check + * their permissions in the moment. + * + * By default, this always returns false. + */ + // eslint-disable-next-line @typescript-eslint/no-unused-vars + protected checkFallback(_roomId: string, _connectionId: cId, _sender?: string): Promise|boolean { + return false; + } + + private getKey(connectionIdStr: string): string { + return `${GRANT_ACCOUNT_DATA_KEY}/${this.grantType}/${connectionIdStr}`.toLowerCase(); + } + + public async assertConnectionGranted(roomId: string, connectionId: cId, sender?: string) { + const connId = GrantChecker.stringifyConnectionId(connectionId); + try { + const content = await this.intent.underlyingClient.getRoomAccountData(this.getKey(connId), roomId); + if (!content.granted) { + // Previously granted but now stale. + throw new GrantRejectedError(roomId, connId); + } + } catch (ex) { + if (ex instanceof MatrixError && ex.errcode === "M_NOT_FOUND") { + if (!await this.checkFallback?.(roomId, connectionId, sender)) { + throw new GrantRejectedError(roomId, connId); + } else { + log.info(`Grant fallback succeeded for ${roomId}/${connectionId}`); + await this.grantConnection(roomId, connectionId); + } + } else { + log.warn(`Failed to check grant in ${roomId}/${connectionId}`, ex); + throw new GrantRejectedError(roomId, connId); + } + } + } + + public async grantConnection(roomId: string, connectionId: cId) { + const cidStr = GrantChecker.stringifyConnectionId(connectionId); + log.info(`Granting ${roomId}/${cidStr}`); + await this.intent.underlyingClient.setRoomAccountData( + this.getKey(cidStr), + roomId, + { granted: true } as GrantContent + ); + } + + public async ungrantConnection(roomId: string, connectionId: cId) { + const cidStr = GrantChecker.stringifyConnectionId(connectionId); + log.info(`Ungranting ${roomId}/${cidStr}`); + await this.intent.underlyingClient.setRoomAccountData( + this.getKey(cidStr), + roomId, + { granted: false } as GrantContent + ); + } +} + +/** + * Check the grant of a given connection, falling back to checking the permissions of the user + * across the bridge. + */ +export class ConfigGrantChecker extends GrantChecker { + static ConfigMinAccessLevel = BridgePermissionLevel.admin; + + constructor(grantType: string, private readonly as: Appservice, private readonly config: BridgeConfig) { + super(as.botIntent, grantType) + } + + protected checkFallback(_roomId: string, _connectionId: cId, sender?: string) { + if (!sender) { + // Cannot validate without a sender. + return false; + } + if (this.as.isNamespacedUser(sender)) { + // Bridge is always valid. + return true; + } + return this.config.checkPermission(sender, this.grantType, ConfigGrantChecker.ConfigMinAccessLevel); + } +} \ No newline at end of file diff --git a/tests/connections/GitlabRepoTest.ts b/tests/connections/GitlabRepoTest.ts index eed83042..f310a884 100644 --- a/tests/connections/GitlabRepoTest.ts +++ b/tests/connections/GitlabRepoTest.ts @@ -4,6 +4,7 @@ import { AppserviceMock } from "../utils/AppserviceMock"; import { ApiError, ErrCode, ValidatorApiError } from "../../src/api"; import { GitLabRepoConnection, GitLabRepoConnectionState } from "../../src/Connections"; import { expect } from "chai"; +import { BridgeConfigGitLab } from "../../src/Config/Config"; const ROOM_ID = "!foo:bar"; @@ -42,6 +43,8 @@ function createConnection(state: Record = {}, isExistingState=f const connection = new GitLabRepoConnection( ROOM_ID, "state_key", + as, + {} as BridgeConfigGitLab, intent, GitLabRepoConnection.validateState({ instance: "bar", diff --git a/tests/grants/GrantChecker.spec.ts b/tests/grants/GrantChecker.spec.ts new file mode 100644 index 00000000..94c96f53 --- /dev/null +++ b/tests/grants/GrantChecker.spec.ts @@ -0,0 +1,177 @@ +import { expect } from "chai"; +import { BridgeConfig } from "../../src/Config/Config"; +import { DefaultConfigRoot } from "../../src/Config/Defaults"; +import { FormatUtil } from "../../src/FormatUtil"; +import { ConfigGrantChecker, GrantChecker, GrantRejectedError } from '../../src/grants/GrantCheck'; +import { AppserviceMock } from "../utils/AppserviceMock"; +import { IntentMock } from "../utils/IntentMock"; + +const ROOM_ID = '!a-room:bar'; +const CONNECTION_ID = '!a-room:bar'; +const ALWAYS_GRANT_USER = '@grant_me:bar'; +const GRANT_SERVICE_USER = '@grant_service_user:bar'; +const GRANT_SERVCE_LOW_PERMS = '@grant_service_user_without_perms:bar'; +const GRANT_WRONG_SERVCE_USER = '@grant_wrong_service_user:bar'; +const ALICE_USERID = '@alice:bar'; +const GRANT_SERVICE = 'example-grant'; + +async function doesAssert(checker: GrantChecker, roomId: string, connectionId: string, sender?: string) { + try { + await checker.assertConnectionGranted(roomId, connectionId, sender); + throw Error(`Expected ${roomId}/${connectionId} to have thrown an error`) + } catch (ex) { + expect(ex).instanceOf(GrantRejectedError, 'Error thrown, but was not a grant rejected error'); + expect(ex.roomId).to.equal(roomId, "Grant rejected, but roomId didn't match"); + // connectionIds are always hashed + expect(ex.connectionId).to.equal(FormatUtil.hashId(connectionId), "Grant rejected, but connectionId didn't match"); + return true; + } +} + +class TestGrantChecker extends GrantChecker { + protected checkFallback(roomId: string, connectionId: string | object, sender?: string | undefined) { + return sender === ALWAYS_GRANT_USER; + } +} + +describe("GrantChecker", () => { + describe('base grant system', () => { + let check: GrantChecker; + // eslint-disable-next-line @typescript-eslint/no-explicit-any + let intent: any; + beforeEach(() => { + intent = IntentMock.create('@foo:bar'); + check = new TestGrantChecker(intent, GRANT_SERVICE); + }); + + it('will grant a connection', async () => { + await check.grantConnection(ROOM_ID, CONNECTION_ID); + // And then to check that the grant has now been allowed. + await check.assertConnectionGranted(ROOM_ID, CONNECTION_ID); + }); + + it('will assert on a missing grant', async () => { + await doesAssert( + check, + ROOM_ID, + CONNECTION_ID + ); + }); + + it('will allow a missing grant if sender matches', async () => { + // Use the special user to grant the connection + await check.assertConnectionGranted(ROOM_ID, CONNECTION_ID, ALWAYS_GRANT_USER); + + // And then to check that the grant has now been allowed. + await check.assertConnectionGranted(ROOM_ID, CONNECTION_ID); + }); + + it('will not conflict with another connection id', async () => { + await check.grantConnection(ROOM_ID, CONNECTION_ID); + await doesAssert( + check, + ROOM_ID, + CONNECTION_ID + "2", + ); + }); + + it('will not conflict with another room', async () => { + await check.grantConnection(ROOM_ID, CONNECTION_ID); + await doesAssert( + check, + ROOM_ID + "2", + CONNECTION_ID + ); + }); + + it('will not conflict with another grant service', async () => { + const anotherchecker = new TestGrantChecker(intent, GRANT_SERVICE + "-two"); + await check.grantConnection(ROOM_ID, CONNECTION_ID); + + await doesAssert( + anotherchecker, + ROOM_ID, + CONNECTION_ID + ); + }); + }); + describe('config fallback', () => { + let check: GrantChecker; + let as: AppserviceMock; + beforeEach(() => { + const mockAs = AppserviceMock.create(); + as = mockAs; + const config = new BridgeConfig( + { + ...DefaultConfigRoot, + permissions: [{ + actor: ALWAYS_GRANT_USER, + services: [{ + service: '*', + level: "admin", + }], + }, + { + actor: GRANT_SERVICE_USER, + services: [{ + service: GRANT_SERVICE, + level: "admin", + }] + }, + { + actor: GRANT_SERVCE_LOW_PERMS, + services: [{ + service: GRANT_SERVICE, + level: 'notifications', + }] + }, + { + actor: GRANT_WRONG_SERVCE_USER, + services: [{ + service: 'another-service', + level: "admin", + }] + }], + } + ); + check = new ConfigGrantChecker(GRANT_SERVICE, mockAs, config); + }); + + it('will deny a missing grant if the sender is not provided', async () => { + await doesAssert( + check, + ROOM_ID, + CONNECTION_ID + ); + }); + + it('will deny a missing grant if the sender is not in the appservice whitelist', async () => { + await doesAssert( + check, + ROOM_ID, + CONNECTION_ID, + ALICE_USERID, + ); + }); + + it('will grant if the user is part of the appservice', async () => { + await check.assertConnectionGranted(ROOM_ID, CONNECTION_ID, as.namespace + "bot"); + }); + + it('will grant if the user has access to all services', async () => { + await check.assertConnectionGranted(ROOM_ID, CONNECTION_ID, ALWAYS_GRANT_USER); + }); + + it('will grant if the user has access to this service', async () => { + await check.assertConnectionGranted(ROOM_ID, CONNECTION_ID, GRANT_SERVICE_USER); + }); + + it('will not grant if the user has low access to this service', async () => { + await doesAssert(check, ROOM_ID, CONNECTION_ID, GRANT_SERVCE_LOW_PERMS); + }); + + it('will not grant if the user has access to a different service', async () => { + await doesAssert(check, ROOM_ID, CONNECTION_ID, GRANT_WRONG_SERVCE_USER); + }); + }); +}); diff --git a/tests/utils/AppserviceMock.ts b/tests/utils/AppserviceMock.ts index dccf3a78..c32dc363 100644 --- a/tests/utils/AppserviceMock.ts +++ b/tests/utils/AppserviceMock.ts @@ -4,6 +4,8 @@ export class AppserviceMock { // eslint-disable-next-line @typescript-eslint/no-explicit-any public readonly intentMap = new Map(); public readonly botIntent = IntentMock.create(`@bot:example.com`); + public namespace = "@hookshot_"; + static create(){ // eslint-disable-next-line @typescript-eslint/no-explicit-any return new this() as any; @@ -26,4 +28,8 @@ export class AppserviceMock { this.intentMap.set(userId, intent); return intent; } + + public isNamespacedUser(userId: string) { + return userId.startsWith(this.namespace); + } } diff --git a/tests/utils/IntentMock.ts b/tests/utils/IntentMock.ts index 17ee39e4..2a976060 100644 --- a/tests/utils/IntentMock.ts +++ b/tests/utils/IntentMock.ts @@ -1,5 +1,6 @@ import { expect } from "chai"; +import { MatrixError } from "matrix-bot-sdk"; export class MatrixClientMock { static create(){ @@ -9,6 +10,7 @@ export class MatrixClientMock { // map room Id → user Ids private joinedMembers: Map = new Map(); + public readonly roomAccountData: Map = new Map(); async setDisplayName() { return; @@ -28,6 +30,21 @@ export class MatrixClientMock { roomMembers.push(userId); this.joinedMembers.set(roomId, roomMembers); } + + async getRoomAccountData(key: string, roomId: string): Promise { + const data = this.roomAccountData.get(roomId+key); + if (data) { + return data; + } + throw new MatrixError({ + errcode: 'M_NOT_FOUND', + error: 'Test error: No account data', + }, 404); + } + + async setRoomAccountData(key: string, roomId: string, value: string): Promise { + this.roomAccountData.set(roomId+key, value); + } } export class IntentMock {