From e4ed3bd8aff2b3b39e89cf39762f2a3b4bb0b479 Mon Sep 17 00:00:00 2001 From: Andrey Sobolev Date: Sun, 11 Aug 2024 18:09:36 +0700 Subject: [PATCH] UBERF-7836: Fix github integeration (#6313) Signed-off-by: Andrey Sobolev --- .vscode/launch.json | 1 + server/account/src/operations.ts | 54 +++++++++---- server/token/src/token.ts | 4 +- server/ws/src/server.ts | 4 +- services/github/pod-github/Dockerfile | 3 +- services/github/pod-github/src/platform.ts | 92 ++++++++++++++++++++-- services/github/pod-github/src/server.ts | 34 ++++++-- services/github/pod-github/src/worker.ts | 8 +- 8 files changed, 160 insertions(+), 40 deletions(-) diff --git a/.vscode/launch.json b/.vscode/launch.json index 8c868d56e2..1d7fade17a 100644 --- a/.vscode/launch.json +++ b/.vscode/launch.json @@ -244,6 +244,7 @@ "MINIO_ENDPOINT": "localhost", "MINIO_ACCESS_KEY": "minioadmin", "MINIO_SECRET_KEY": "minioadmin", + "PLATFORM_OPERATION_LOGGING": "true", "FRONT_URL": "http://localhost:8080", "PORT": "3500" }, diff --git a/server/account/src/operations.ts b/server/account/src/operations.ts index 2adea7cb3a..8c45f4a8f3 100644 --- a/server/account/src/operations.ts +++ b/server/account/src/operations.ts @@ -65,7 +65,7 @@ import { registerStringLoaders } from '@hcengineering/server-pipeline' import { buildStorageFromConfig, storageConfigFromEnv } from '@hcengineering/server-storage' -import { decodeToken, generateToken } from '@hcengineering/server-token' +import { decodeToken as decodeTokenRaw, generateToken, type Token } from '@hcengineering/server-token' import toolPlugin, { connect, initializeWorkspace, @@ -398,7 +398,7 @@ export async function getAccountInfoByToken ( let email: string = '' let workspace: WorkspaceId try { - ;({ email, workspace } = decodeToken(token)) + ;({ email, workspace } = decodeToken(ctx, token)) } catch (err: any) { Analytics.handleError(err) ctx.error('Invalid token', { token }) @@ -579,6 +579,21 @@ function getExtra (info: Account | AccountInfo | null, rec?: Record export const guestAccountEmail = '#guest@hc.engineering' +function decodeToken (ctx: MeasureContext, token: string): Token { + // eslint-disable-next-line no-useless-catch + try { + return decodeTokenRaw(token) + } catch (err: any) { + try { + // Ok we have error, but we need to log a proper message + ctx.warn('failed to verify token', { ...decodeTokenRaw(token, false) }) + } catch (err2: any) { + // Ignore + } + throw err + } +} + /** * @public */ @@ -592,7 +607,7 @@ export async function selectWorkspace ( kind: 'external' | 'internal', allowAdmin: boolean = true ): Promise { - const decodedToken = decodeToken(token) + const decodedToken = decodeToken(ctx, token) const email = cleanEmail(decodedToken.email) const endpointKind = kind === 'external' ? EndpointKind.External : EndpointKind.Internal @@ -776,7 +791,7 @@ export async function confirm ( branding: Branding | null, token: string ): Promise { - const decode = decodeToken(token) + const decode = decodeToken(ctx, token) const _email = decode.extra?.confirm if (_email === undefined) { ctx.error('confirm email invalid', { token: decode }) @@ -1010,7 +1025,7 @@ export async function listWorkspaces ( branding: Branding | null, token: string ): Promise { - decodeToken(token) // Just verify token is valid + decodeToken(ctx, token) // Just verify token is valid return (await db.collection(WORKSPACE_COLLECTION).find(withProductId(productId, {})).toArray()) .map((it) => ({ ...it, productId })) .filter((it) => it.disabled !== true) @@ -1447,7 +1462,7 @@ export const createUserWorkspace = token: string, workspaceName: string ): Promise => { - const { email } = decodeToken(token) + const { email } = decodeToken(ctx, token) ctx.info('Creating workspace', { workspaceName, email }) @@ -1567,7 +1582,7 @@ export async function getInviteLink ( role?: AccountRole, personId?: Ref ): Promise { - const { workspace, email } = decodeToken(token) + const { workspace, email } = decodeToken(ctx, token) const wsPromise = await getWorkspaceById(db, productId, workspace.name) if (wsPromise === null) { ctx.error('workspace not found', { workspace, email }) @@ -1620,7 +1635,7 @@ export async function getUserWorkspaces ( branding: Branding | null, token: string ): Promise { - const { email } = decodeToken(token) + const { email } = decodeToken(ctx, token) const account = await getAccount(db, email) if (account === null) { ctx.error('account not found', { email }) @@ -1648,7 +1663,7 @@ export async function getWorkspaceInfo ( token: string, _updateLastVisit: boolean = false ): Promise { - const { email, workspace, extra } = decodeToken(token) + const { email, workspace, extra } = decodeToken(ctx, token) const guest = extra?.guest === 'true' let account: Pick | Account | null = null const query: Filter = { @@ -1772,7 +1787,7 @@ export async function createMissingEmployee ( branding: Branding | null, token: string ): Promise { - const { email } = decodeToken(token) + const { email } = decodeToken(ctx, token) const wsInfo = await getWorkspaceInfo(ctx, db, productId, branding, token) const account = await getAccount(db, email) @@ -2020,7 +2035,7 @@ export async function changePassword ( oldPassword: string, password: string ): Promise { - const { email } = decodeToken(token) + const { email } = decodeToken(ctx, token) const account = await getAccountInfo(ctx, db, productId, branding, email, oldPassword) const salt = randomBytes(32) @@ -2121,7 +2136,7 @@ export async function restorePassword ( token: string, password: string ): Promise { - const decode = decodeToken(token) + const decode = decodeToken(ctx, token) const email = decode.extra?.restore if (email === undefined) { throw new PlatformError(new Status(Severity.ERROR, platform.status.AccountNotFound, { account: email })) @@ -2180,7 +2195,7 @@ export async function checkJoin ( token: string, inviteId: ObjectId ): Promise { - const { email } = decodeToken(token) + const { email } = decodeToken(ctx, token) const invite = await getInvite(db, inviteId) const workspace = await checkInvite(ctx, invite, email) const ws = await getWorkspaceById(db, productId, workspace.name) @@ -2283,7 +2298,7 @@ export async function leaveWorkspace ( token: string, email: string ): Promise { - const tokenData = decodeToken(token) + const tokenData = decodeToken(ctx, token) const currentAccount = await getAccount(db, tokenData.email) if (currentAccount === null) { @@ -2324,7 +2339,7 @@ export async function sendInvite ( personId?: Ref, role?: AccountRole ): Promise { - const tokenData = decodeToken(token) + const tokenData = decodeToken(ctx, token) const currentAccount = await getAccount(db, tokenData.email) if (currentAccount === null) { throw new PlatformError(new Status(Severity.ERROR, platform.status.AccountNotFound, { account: tokenData.email })) @@ -2449,6 +2464,13 @@ function wrap ( err instanceof PlatformError ? err.status : new Status(Severity.ERROR, platform.status.InternalServerError, {}) + + if (((err.message as string) ?? '') === 'Signature verification failed') { + // Let's send un authorized + return { + error: new Status(Severity.ERROR, platform.status.Unauthorized, {}) + } + } if (status.code === platform.status.InternalServerError) { Analytics.handleError(err) ctx.error('error', { status, err }) @@ -2617,7 +2639,7 @@ export async function changeUsername ( first: string, last: string ): Promise { - const { email } = decodeToken(token) + const { email } = decodeToken(ctx, token) const account = await getAccount(db, email) if (account == null) { diff --git a/server/token/src/token.ts b/server/token/src/token.ts index fa7186b88f..b64c677a4c 100644 --- a/server/token/src/token.ts +++ b/server/token/src/token.ts @@ -26,8 +26,8 @@ export function generateToken (email: string, workspace: WorkspaceId, extra?: Re /** * @public */ -export function decodeToken (token: string): Token { - const value = decode(token, getSecret(), false) +export function decodeToken (token: string, verify: boolean = true): Token { + const value = decode(token, getSecret(), !verify) const { email, workspace, productId, ...extra } = value return { email, workspace: getWorkspaceId(workspace, productId), extra } } diff --git a/server/ws/src/server.ts b/server/ws/src/server.ts index 9c167bafaa..beeae39ec5 100644 --- a/server/ws/src/server.ts +++ b/server/ws/src/server.ts @@ -316,7 +316,9 @@ class TSessionManager implements SessionManager { ) { ctx.warn('model version mismatch', { version: this.modelVersion, - workspaceVersion: versionToString(workspaceInfo.version) + workspaceVersion: versionToString(workspaceInfo.version), + workspace: workspaceInfo.workspaceId, + workspaceUrl: workspaceInfo.workspaceUrl }) // Version mismatch, return upgrading. return { upgrade: true, upgradeInfo: workspaceInfo.upgrade } diff --git a/services/github/pod-github/Dockerfile b/services/github/pod-github/Dockerfile index 88893a347c..8b585ad151 100644 --- a/services/github/pod-github/Dockerfile +++ b/services/github/pod-github/Dockerfile @@ -5,6 +5,7 @@ WORKDIR /usr/src/app RUN npm install --ignore-scripts=false --verbose bufferutil utf-8-validate @mongodb-js/zstd snappy --unsafe-perm COPY bundle/bundle.js ./ +COPY bundle/bundle.js.map ./ RUN apt-get update RUN apt-get install libjemalloc2 @@ -13,4 +14,4 @@ ENV LD_PRELOAD=libjemalloc.so.2 ENV MALLOC_CONF=dirty_decay_ms:1000,narenas:2,background_thread:true EXPOSE 3078 -CMD [ "node", "--inspect", "--async-stack-traces", "bundle.js" ] +CMD [ "node", "--inspect", "--async-stack-traces", "--enable-source-maps", "bundle.js" ] diff --git a/services/github/pod-github/src/platform.ts b/services/github/pod-github/src/platform.ts index 3875c61a79..046db1e59c 100644 --- a/services/github/pod-github/src/platform.ts +++ b/services/github/pod-github/src/platform.ts @@ -15,8 +15,8 @@ import core, { Ref, TxOperations } from '@hcengineering/core' -import github, { GithubAuthentication, makeQuery } from '@hcengineering/github' -import { MongoClientReference, getMongoClient } from '@hcengineering/mongo' +import github, { GithubAuthentication, makeQuery, type GithubIntegration } from '@hcengineering/github' +import { getMongoClient, MongoClientReference } from '@hcengineering/mongo' import { setMetadata } from '@hcengineering/platform' import { buildStorageFromConfig, storageConfigFromEnv } from '@hcengineering/server-storage' import serverToken, { generateToken } from '@hcengineering/server-token' @@ -199,9 +199,39 @@ export class PlatformWorker { installationId: number, accountId: Ref ): Promise { - if (this.integrations.find((it) => it.installationId === installationId) != null) { + const oldInstallation = this.integrations.find((it) => it.installationId === installationId) + if (oldInstallation != null) { + ctx.info('update integration', { workspace, installationId, accountId }) // What to do with installation in different workspace? // Let's remove it and sync to new one. + if (oldInstallation.workspace !== workspace) { + // + const oldWorkspace = oldInstallation.workspace + + await this.integrationCollection.updateOne( + { installationId: oldInstallation.installationId }, + { $set: { workspace } } + ) + oldInstallation.workspace = workspace + + const oldWorker = this.clients.get(oldWorkspace) as GithubWorker + if (oldWorker !== undefined) { + await this.removeInstallationFromWorkspace(oldWorker.client, installationId) + await oldWorker.reloadRepositories(installationId) + } else { + let client: Client | undefined + try { + client = await createPlatformClient(oldWorkspace, config.ProductID, 30000) + await this.removeInstallationFromWorkspace(oldWorker, installationId) + await client.close() + } catch (err: any) { + ctx.error('failed to remove old installation from workspace', { workspace: oldWorkspace, installationId }) + } + } + } + + await this.updateInstallation(installationId) + const worker = this.clients.get(workspace) as GithubWorker await worker?.reloadRepositories(installationId) worker?.triggerUpdate() @@ -214,7 +244,9 @@ export class PlatformWorker { installationId, accountId } - await ctx.withLog('add integration', { workspace, installationId, accountId }, async (ctx) => { + ctx.info('add integration', { workspace, installationId, accountId }) + + await ctx.with('add integration', { workspace, installationId, accountId }, async (ctx) => { await this.integrationCollection.insertOne(record) this.integrations.push(record) }) @@ -228,12 +260,43 @@ export class PlatformWorker { this.triggerCheckWorkspaces() } + private async removeInstallationFromWorkspace (client: Client, installationId: number): Promise { + const wsIntegerations = await client.findAll(github.class.GithubIntegration, { installationId }) + + for (const intValue of wsIntegerations) { + const ops = new TxOperations(client, core.account.System) + await ops.remove(intValue) + } + } + async removeInstallation (ctx: MeasureContext, workspace: string, installationId: number): Promise { const installation = this.installations.get(installationId) if (installation !== undefined) { - await installation.octokit.rest.apps.deleteInstallation({ - installation_id: installationId - }) + try { + await installation.octokit.rest.apps.deleteInstallation({ + installation_id: installationId + }) + } catch (err: any) { + if (err.status !== 404) { + // Already deleted. + ctx.error('error from github api', { error: err }) + } + await this.handleInstallationEventDelete(installationId) + } + // Let's check if workspace somehow still have installation and remove it + const worker = this.clients.get(workspace) as GithubWorker + if (worker !== undefined) { + await GithubWorker.checkIntegrations(worker.client, this.installations) + } else { + let client: Client | undefined + try { + client = await createPlatformClient(workspace, config.ProductID, 30000) + await GithubWorker.checkIntegrations(client, this.installations) + await client.close() + } catch (err: any) { + ctx.error('failed to clean installation from workspace', { workspace, installationId }) + } + } } this.triggerCheckWorkspaces() } @@ -578,6 +641,12 @@ export class PlatformWorker { const rateLimiter = new RateLimiter(5) let errors = 0 let idx = 0 + const connecting = new Map() + const connectingInfo = setInterval(() => { + for (const [c, d] of connecting.entries()) { + this.ctx.info('connecting to workspace', { workspace: c, time: Date.now() - d }) + } + }, 5000) for (const workspace of workspaces) { const widx = ++idx if (this.clients.has(workspace)) { @@ -607,8 +676,14 @@ export class PlatformWorker { errors++ return } + if (workspaceInfo?.disabled === true) { + this.ctx.error('Workspace is disabled workspaceId', { workspace }) + return + } const branding = Object.values(this.brandingMap).find((b) => b.key === workspaceInfo?.branding) ?? null const workerCtx = this.ctx.newChild('worker', { workspace: workspaceInfo.workspace }, {}) + + connecting.set(workspaceInfo.workspace, Date.now()) workerCtx.info('************************* Register worker ************************* ', { workspaceId: workspaceInfo.workspaceId, workspace: workspaceInfo.workspace, @@ -658,6 +733,8 @@ export class PlatformWorker { this.ctx.info("Couldn't create WS worker", { workspace, error: e }) console.error(e) errors++ + } finally { + connecting.delete(workspaceInfo.workspace) } }) } @@ -667,6 +744,7 @@ export class PlatformWorker { Analytics.handleError(e) errors++ } + clearInterval(connectingInfo) // Close deleted workspaces for (const deleted of Array.from(toDelete.keys())) { const ws = this.clients.get(deleted) diff --git a/services/github/pod-github/src/server.ts b/services/github/pod-github/src/server.ts index 2064f49c5f..b0d8d0d9ae 100644 --- a/services/github/pod-github/src/server.ts +++ b/services/github/pod-github/src/server.ts @@ -73,13 +73,12 @@ export async function start (ctx: MeasureContext, brandingMap: BrandingMap): Pro // eslint-disable-next-line @typescript-eslint/no-misused-promises app.post('/api/v1/installation', async (req, res) => { + const payloadData: { + installationId: number + accountId: Ref + token: string + } = req.body try { - const payloadData: { - installationId: number - accountId: Ref - token: string - } = req.body - const decodedToken = decodeToken(payloadData.token) ctx.info('/api/v1/installation', { email: decodedToken.email, @@ -87,6 +86,10 @@ export async function start (ctx: MeasureContext, brandingMap: BrandingMap): Pro body: req.body }) + ctx.info('map-installation', { + workspace: decodedToken.workspace.name, + installationid: payloadData.installationId + }) await ctx.withLog('map-installation', {}, async (ctx) => { await worker.mapInstallation( ctx, @@ -99,6 +102,12 @@ export async function start (ctx: MeasureContext, brandingMap: BrandingMap): Pro res.json({}) } catch (err: any) { Analytics.handleError(err) + const tok = decodeToken(payloadData.token, false) + ctx.error('failed to map-installation', { + workspace: tok.workspace?.name, + installationid: payloadData.installationId, + email: tok?.email + }) res.status(401) res.json({ error: err.message }) } @@ -121,7 +130,12 @@ export async function start (ctx: MeasureContext, brandingMap: BrandingMap): Pro } = JSON.parse(atob(payloadData.state)) const decodedToken = decodeToken(decodedData.token) - + ctx.info('request github access-token', { + workspace: decodedToken.workspace.name, + accountId: payloadData.accountId, + code: payloadData.code, + state: payloadData.state + }) await ctx.withLog('request-github-access-token', {}, async (ctx) => { await worker.requestGithubAccessToken({ workspace: decodedToken.workspace.name, @@ -154,7 +168,11 @@ export async function start (ctx: MeasureContext, brandingMap: BrandingMap): Pro body: req.body }) - await ctx.withLog('map-installation', {}, async (ctx) => { + ctx.info('remove-installation', { + workspace: decodedToken.workspace.name, + installationId: payloadData.installationId + }) + await ctx.withLog('remove-installation', {}, async (ctx) => { await worker.removeInstallation(ctx, decodedToken.workspace.name, payloadData.installationId) }) res.status(200) diff --git a/services/github/pod-github/src/worker.ts b/services/github/pod-github/src/worker.ts index 44f15e84a6..c3883e4193 100644 --- a/services/github/pod-github/src/worker.ts +++ b/services/github/pod-github/src/worker.ts @@ -624,7 +624,7 @@ export class GithubWorker implements IntegrationManager { return statuses.filter((it) => allowedTypes.has(it._id)) } - async init (): Promise { + async init (): Promise { this.registerNotifyHandler() await this.queryAccounts() @@ -692,7 +692,6 @@ export class GithubWorker implements IntegrationManager { this.triggerRequests = 1 this.updateRequests = 1 this.syncPromise = this.syncAndWait() - return true } projects: GithubProject[] = [] @@ -1504,9 +1503,8 @@ export class GithubWorker implements IntegrationManager { branding ) ctx.info('Init worker', { workspace: workspace.workspaceUrl, workspaceId: workspace.workspaceName }) - if (await worker.init()) { - return worker - } + void worker.init() + return worker } catch (err: any) { ctx.error('timeout during to connect', { workspace, error: err }) await client?.close()