From db8221b60a51ad7316a8e249ce279bfc16fa6251 Mon Sep 17 00:00:00 2001 From: Andrew Ferrazzutti Date: Tue, 8 Nov 2022 10:19:41 -0500 Subject: [PATCH] Allow GitLab connections without hook permissions (#567) * Allow GitLab connections without hook permissions Warn instead of fail when connecting a GitLab project that Hookshot cannot provision a webhook for. * Mention manual "Secret token" for GitLab webhooks * Refactor warning pane into a separate component * Recolour warning pane for better contrast --- changelog.d/567.feature | 1 + .../room_configuration/gitlab_project.md | 4 +-- src/ConnectionManager.ts | 8 ++--- src/Connections/GitlabRepo.ts | 16 +++++++--- src/Connections/IConnection.ts | 4 +-- src/Connections/SetupConnection.ts | 4 +-- src/Widgets/BridgeWidgetApi.ts | 9 ++++-- src/provisioning/api.ts | 6 ++++ src/provisioning/provisioner.ts | 15 ++++++---- web/BridgeAPI.ts | 2 +- web/components/elements/ErrorPane.tsx | 2 +- .../elements/WarningPane.module.scss | 4 +++ web/components/elements/WarningPane.tsx | 9 ++++++ web/components/elements/index.ts | 3 +- web/components/roomConfig/RoomConfig.tsx | 29 +++++++++++++------ web/icons/error-badge.svg | 5 ++++ web/icons/warning-badge.svg | 2 +- web/typings/images.d.ts | 4 +++ 18 files changed, 91 insertions(+), 36 deletions(-) create mode 100644 changelog.d/567.feature create mode 100644 web/components/elements/WarningPane.module.scss create mode 100644 web/components/elements/WarningPane.tsx create mode 100644 web/icons/error-badge.svg diff --git a/changelog.d/567.feature b/changelog.d/567.feature new file mode 100644 index 00000000..33461aac --- /dev/null +++ b/changelog.d/567.feature @@ -0,0 +1 @@ +Allow adding connections to GitLab projects even when Hookshot doesn't have permissions to automatically provision a webhook for it. When that occurs, tell the user to ask a project admin to add the webhook. diff --git a/docs/usage/room_configuration/gitlab_project.md b/docs/usage/room_configuration/gitlab_project.md index 9a31bc9f..80819070 100644 --- a/docs/usage/room_configuration/gitlab_project.md +++ b/docs/usage/room_configuration/gitlab_project.md @@ -14,8 +14,8 @@ To set up a connection to a GitLab project in a new room: 3. Give the bridge bot moderator permissions or higher (power level 50) (or otherwise configure the room so the bot can edit room state). 4. Send the command `!hookshot gitlab project https://mydomain/my/project`. 5. If you have permission to bridge this repo, the bridge will respond with a confirmation message. (Users with `Developer` permissions or greater can bridge projects.) - 6. If you have configured the bridge with a `publicUrl` inside `gitlab.webhook`, it will automatically provision the webhook for you. - 7. Otherwise, you'll need to manually configure the webhook to point to your public address for the webhooks listener. +6. If you have configured the bridge with a `publicUrl` inside `gitlab.webhook`, and you have `Maintainer` permissions or greater on the project, the bot will automatically provision the webhook for you. +7. Otherwise, you'll need to manually configure the project with a webhook that points to your public address for the webhooks listener, sets the "Secret token" to the one you put in your Hookshot configuration (`gitlab.webhook.secret`), and enables all Triggers that need to be bridged (as Hookshot can only bridge events for enabled Triggers). This can be configured on the GitLab webpage for the project under Settings > Webhook Settings. If you do not have access to this page, you must ask someone who does (i.e. someone with at least `Maintainer` permissions on the project) to add the webhook for you. ## Configuration diff --git a/src/ConnectionManager.ts b/src/ConnectionManager.ts index 6badf88f..0b3af130 100644 --- a/src/ConnectionManager.ts +++ b/src/ConnectionManager.ts @@ -68,14 +68,14 @@ export class ConnectionManager extends EventEmitter { * @param data The data corresponding to the connection state. This will be validated. * @returns The resulting connection. */ - public async provisionConnection(roomId: string, userId: string, type: string, data: Record): Promise { + public async provisionConnection(roomId: string, userId: string, type: string, data: Record) { log.info(`Looking to provision connection for ${roomId} ${type} for ${userId} with data ${JSON.stringify(data)}`); const connectionType = ConnectionDeclarations.find(c => c.EventTypes.includes(type)); if (connectionType?.provisionConnection) { if (!this.config.checkPermission(userId, connectionType.ServiceCategory, BridgePermissionLevel.manageConnections)) { throw new ApiError(`User is not permitted to provision connections for this type of service.`, ErrCode.ForbiddenUser); } - const { connection } = await connectionType.provisionConnection(roomId, userId, data, { + const result = await connectionType.provisionConnection(roomId, userId, data, { as: this.as, config: this.config, tokenStore: this.tokenStore, @@ -85,8 +85,8 @@ export class ConnectionManager extends EventEmitter { github: this.github, getAllConnectionsOfType: this.getAllConnectionsOfType.bind(this), }); - this.push(connection); - return connection; + this.push(result.connection); + return result; } throw new ApiError(`Connection type not known`); } diff --git a/src/Connections/GitlabRepo.ts b/src/Connections/GitlabRepo.ts index c8c1131c..741fe2e7 100644 --- a/src/Connections/GitlabRepo.ts +++ b/src/Connections/GitlabRepo.ts @@ -10,7 +10,7 @@ import { BridgeConfigGitLab, GitLabInstance } from "../Config/Config"; import { IGitlabMergeRequest, IGitlabProject, IGitlabUser, IGitLabWebhookMREvent, IGitLabWebhookNoteEvent, IGitLabWebhookPushEvent, IGitLabWebhookReleaseEvent, IGitLabWebhookTagPushEvent, IGitLabWebhookWikiPageEvent } from "../Gitlab/WebhookTypes"; import { CommandConnection } from "./CommandConnection"; import { Connection, IConnection, IConnectionState, InstantiateConnectionOpts, ProvisionConnectionOpts } from "./IConnection"; -import { GetConnectionsResponseItem } from "../provisioning/api"; +import { ConnectionWarning, GetConnectionsResponseItem } from "../provisioning/api"; import { ErrCode, ApiError, ValidatorApiError } from "../api" import { AccessLevel } from "../Gitlab/Types"; import Ajv, { JSONSchemaType } from "ajv"; @@ -217,7 +217,9 @@ export class GitLabRepoConnection extends CommandConnection= AccessLevel.Maintainer) { const hooks = await client.projects.hooks.list(project.id); const hasHook = hooks.find(h => h.url === gitlabConfig.webhook.publicUrl); if (!hasHook) { @@ -235,11 +237,17 @@ export class GitLabRepoConnection extends CommandConnection { EventTypes: string[]; ServiceCategory: string; - provisionConnection?: (roomId: string, userId: string, data: Record, opts: ProvisionConnectionOpts) => Promise<{connection: C}>; + provisionConnection?: (roomId: string, userId: string, data: Record, opts: ProvisionConnectionOpts) => Promise<{connection: C, warning?: ConnectionWarning}>; createConnectionForState: (roomId: string, state: StateEvent>, opts: InstantiateConnectionOpts) => C|Promise } diff --git a/src/Connections/SetupConnection.ts b/src/Connections/SetupConnection.ts index 17fe1fe1..6ef889c7 100644 --- a/src/Connections/SetupConnection.ts +++ b/src/Connections/SetupConnection.ts @@ -108,9 +108,9 @@ export class SetupConnection extends CommandConnection { if (!path) { throw new CommandError("Invalid GitLab url", "The GitLab project url you entered was not valid."); } - const {connection} = await GitLabRepoConnection.provisionConnection(this.roomId, userId, {path, instance: name}, this.provisionOpts); + const {connection, warning} = await GitLabRepoConnection.provisionConnection(this.roomId, userId, {path, instance: name}, this.provisionOpts); this.pushConnections(connection); - await this.as.botClient.sendNotice(this.roomId, `Room configured to bridge ${connection.prettyPath}`); + await this.as.botClient.sendNotice(this.roomId, `Room configured to bridge ${connection.prettyPath}` + (warning ? `\n${warning.header}: ${warning.message}` : "")); } private async checkJiraLogin(userId: string, urlStr: string) { diff --git a/src/Widgets/BridgeWidgetApi.ts b/src/Widgets/BridgeWidgetApi.ts index 7b57057d..795cfdd2 100644 --- a/src/Widgets/BridgeWidgetApi.ts +++ b/src/Widgets/BridgeWidgetApi.ts @@ -134,11 +134,14 @@ export class BridgeWidgetApi { throw new ApiError("A JSON body must be provided", ErrCode.BadValue); } this.connMan.validateCommandPrefix(req.params.roomId, req.body); - const connection = await this.connMan.provisionConnection(req.params.roomId as string, req.userId, req.params.type as string, req.body as Record); - if (!connection.getProvisionerDetails) { + const result = await this.connMan.provisionConnection(req.params.roomId, req.userId, req.params.type, req.body); + if (!result.connection.getProvisionerDetails) { throw new Error('Connection supported provisioning but not getProvisionerDetails'); } - res.send(connection.getProvisionerDetails(true)); + res.send({ + ...result.connection.getProvisionerDetails(true), + warning: result.warning, + }); } catch (ex) { log.error(`Failed to create connection for ${req.params.roomId}`, ex); throw ex; diff --git a/src/provisioning/api.ts b/src/provisioning/api.ts index 0cb44869..207600ab 100644 --- a/src/provisioning/api.ts +++ b/src/provisioning/api.ts @@ -9,11 +9,17 @@ export interface GetConnectionTypeResponseItem { botUserId: string; } +export interface ConnectionWarning { + header: string, + message: string, +} + export interface GetConnectionsResponseItem extends GetConnectionTypeResponseItem { id: string; config: Config; secrets?: Secrets; canEdit?: boolean; + warning?: ConnectionWarning; } const log = new Logger("Provisioner.api"); diff --git a/src/provisioning/provisioner.ts b/src/provisioning/provisioner.ts index 1f448065..005ac944 100644 --- a/src/provisioning/provisioner.ts +++ b/src/provisioning/provisioner.ts @@ -129,18 +129,21 @@ export class Provisioner { return res.send(connection.getProvisionerDetails()); } - private async putConnection(req: Request<{roomId: string, type: string}, unknown, Record, {userId: string}>, res: Response, next: NextFunction) { + private async putConnection(req: Request<{roomId: string, type: string}, unknown, Record, {userId: string}>, res: Response, next: NextFunction) { // Need to figure out which connections are available try { if (!req.body || typeof req.body !== "object") { - throw new ApiError("A JSON body must be provided.", ErrCode.BadValue); + throw new ApiError("A JSON body must be provided", ErrCode.BadValue); } this.connMan.validateCommandPrefix(req.params.roomId, req.body); - const connection = await this.connMan.provisionConnection(req.params.roomId, req.query.userId, req.params.type, req.body); - if (!connection.getProvisionerDetails) { - throw new Error('Connection supported provisioning but not getProvisionerDetails.'); + const result = await this.connMan.provisionConnection(req.params.roomId, req.query.userId, req.params.type, req.body); + if (!result.connection.getProvisionerDetails) { + throw new Error('Connection supported provisioning but not getProvisionerDetails'); } - res.send(connection.getProvisionerDetails(true)); + res.send({ + ...result.connection.getProvisionerDetails(true), + warning: result.warning, + }); } catch (ex) { log.error(`Failed to create connection for ${req.params.roomId}`, ex); return next(ex); diff --git a/web/BridgeAPI.ts b/web/BridgeAPI.ts index 22b4defa..79a25339 100644 --- a/web/BridgeAPI.ts +++ b/web/BridgeAPI.ts @@ -117,7 +117,7 @@ export class BridgeAPI { return this.request('GET', `/widgetapi/v1/${encodeURIComponent(roomId)}/connections/${encodeURIComponent(service)}`); } - async createConnection(roomId: string, type: string, config: IConnectionState) { + async createConnection(roomId: string, type: string, config: IConnectionState): Promise { return this.request('POST', `/widgetapi/v1/${encodeURIComponent(roomId)}/connections/${encodeURIComponent(type)}`, config); } diff --git a/web/components/elements/ErrorPane.tsx b/web/components/elements/ErrorPane.tsx index c481a603..54628acc 100644 --- a/web/components/elements/ErrorPane.tsx +++ b/web/components/elements/ErrorPane.tsx @@ -1,5 +1,5 @@ import { h, FunctionComponent } from "preact"; -import ErrorBadge from "../../icons/warning-badge.svg"; +import ErrorBadge from "../../icons/error-badge.svg"; import style from "./ErrorPane.module.scss"; export const ErrorPane: FunctionComponent<{header?: string}> = ({ children, header }) => { diff --git a/web/components/elements/WarningPane.module.scss b/web/components/elements/WarningPane.module.scss new file mode 100644 index 00000000..1797885e --- /dev/null +++ b/web/components/elements/WarningPane.module.scss @@ -0,0 +1,4 @@ +.warningPane { + max-width: 480px; + color: #FF812D; +} \ No newline at end of file diff --git a/web/components/elements/WarningPane.tsx b/web/components/elements/WarningPane.tsx new file mode 100644 index 00000000..021edd51 --- /dev/null +++ b/web/components/elements/WarningPane.tsx @@ -0,0 +1,9 @@ +import { h, FunctionComponent } from "preact"; +import WarningBadge from "../../icons/warning-badge.svg"; +import style from "./WarningPane.module.scss"; + +export const WarningPane: FunctionComponent<{header?: string}> = ({ children, header }) => { + return
+

{ header || "Problem occured during widget load" }: {children}

+
; +}; \ No newline at end of file diff --git a/web/components/elements/index.ts b/web/components/elements/index.ts index db676c55..11f01d71 100644 --- a/web/components/elements/index.ts +++ b/web/components/elements/index.ts @@ -2,4 +2,5 @@ export * from "./Button"; export * from "./ButtonSet"; export * from "./ErrorPane"; export * from "./InputField"; -export * from "./ListItem"; \ No newline at end of file +export * from "./ListItem"; +export * from "./WarningPane"; \ No newline at end of file diff --git a/web/components/roomConfig/RoomConfig.tsx b/web/components/roomConfig/RoomConfig.tsx index bbe98dbd..3fbea7b9 100644 --- a/web/components/roomConfig/RoomConfig.tsx +++ b/web/components/roomConfig/RoomConfig.tsx @@ -1,7 +1,7 @@ import { h, FunctionComponent } from "preact"; import { useCallback, useEffect, useReducer, useState } from "preact/hooks" import { BridgeAPI, BridgeAPIError } from "../../BridgeAPI"; -import { ErrorPane, ListItem } from "../elements"; +import { ErrorPane, ListItem, WarningPane } from "../elements"; import style from "./RoomConfig.module.scss"; import { GetConnectionsResponseItem } from "../../../src/provisioning/api"; import { IConnectionState } from "../../../src/Connections"; @@ -34,18 +34,22 @@ interface IRoomConfigProps(props: IRoomConfigProps) { const { api, roomId, type, headerImg, text, listItemName, connectionEventType } = props; const ConnectionConfigComponent = props.connectionConfigComponent; - const [ error, setError ] = useState(null); + const [ error, setError ] = useState(null); const [ connections, setConnections ] = useState(null); const [ serviceConfig, setServiceConfig ] = useState(null); const [ canEditRoom, setCanEditRoom ] = useState(false); // We need to increment this every time we create a connection in order to properly reset the state. const [ newConnectionKey, incrementConnectionKey ] = useReducer(n => n+1, 0); + const clearCurrentError = () => { + setError(error => error?.forPrevious ? error : null); + } + useEffect(() => { api.getConnectionsForService(roomId, type).then(res => { setCanEditRoom(res.canEdit); setConnections(res.connections); - setError(null); + clearCurrentError(); }).catch(ex => { console.warn("Failed to fetch existing connections", ex); setError({ @@ -58,9 +62,7 @@ export const RoomConfig = function { api.getServiceConfig(type) .then(setServiceConfig) - .then(() => { - setError(null); - }) + .then(clearCurrentError) .catch(ex => { console.warn("Failed to fetch service config", ex); setError({ @@ -71,10 +73,15 @@ export const RoomConfig = function { - api.createConnection(roomId, connectionEventType, config).then(() => { + api.createConnection(roomId, connectionEventType, config).then(result => { // Force reload incrementConnectionKey(undefined); - setError(null); + setError(!result.warning ? null : { + header: result.warning.header, + message: result.warning.message, + isWarning: true, + forPrevious: true, + }); }).catch(ex => { console.warn("Failed to create connection", ex); setError({ @@ -86,7 +93,11 @@ export const RoomConfig = function { - error && {error.message} + error && + (!error.isWarning + ? {error.message} + : {error.message} + ) }
diff --git a/web/icons/error-badge.svg b/web/icons/error-badge.svg new file mode 100644 index 00000000..b35dabb7 --- /dev/null +++ b/web/icons/error-badge.svg @@ -0,0 +1,5 @@ + + + + + diff --git a/web/icons/warning-badge.svg b/web/icons/warning-badge.svg index b35dabb7..95e99e47 100644 --- a/web/icons/warning-badge.svg +++ b/web/icons/warning-badge.svg @@ -1,5 +1,5 @@ - + diff --git a/web/typings/images.d.ts b/web/typings/images.d.ts index 476fa54d..68826e08 100644 --- a/web/typings/images.d.ts +++ b/web/typings/images.d.ts @@ -1,4 +1,8 @@ declare module "*.png" { const content: string export = content +} +declare module "*.svg" { + const content: string + export = content } \ No newline at end of file